Re: [PATCH] x86/irq: do not insert IRQ_MSI_EMU in emuirq mappings

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 2 Nov 2023, Jan Beulich wrote:
> On 31.10.2023 17:33, Roger Pau Monné wrote:
> > On Tue, Oct 31, 2023 at 03:30:37PM +0200, Xenia Ragiadakou wrote:
> >> Do not use emuirq mappings for MSIs injected by emulated devices.
> >> This kind of pirq shares the same emuirq value and is not remapped.
> > 
> > AFAICT adding the extra emuirq mappings is harmless, and just adds
> > an extra layer of translation?
> > 
> > Or is this causing issues, but we haven't realized because we don't
> > provide emulated devices that use MSI(-X) by default?
> 
> Yeah, whether there's anything wrong with this or whether this change
> is merely trying to optimize things wants spelling out.
> 
> >> Fixes: 88fccdd11ca0 ('xen: event channel remapping for emulated MSIs')
> >> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@xxxxxxx>
> >> ---
> >>
> >> Question: is there any strong reason why Linux HVM guests still use pirqs?
> > 
> > Baggage I guess.  I've suggested in the past to switch PIRQs off by
> > default for HVM, but I had no figures to show how much of a
> > performance penalty that would be for passthrough devices.
> > 
> > My suggestion would be to introduce an xl.cfg option to select the
> > availability of PIRQs for HVM guests, and set it to off by default.
> > You would also need to make sure that migration (or save/restore)
> > works fine, and that incoming guests from previous Xen versions (that
> > won't have the option) will result in PIRQs still being enabled.
> > 
> > There's already a XEN_X86_EMU_USE_PIRQ flag in xen_arch_domainconfig,
> > so you just need to wire the tools side in order to allow selection by
> > users.
> > 
> >>
> >>  xen/arch/x86/irq.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> >> index f42ad539dc..cdc8dc5a55 100644
> >> --- a/xen/arch/x86/irq.c
> >> +++ b/xen/arch/x86/irq.c
> >> @@ -2684,7 +2684,7 @@ int map_domain_emuirq_pirq(struct domain *d, int 
> >> pirq, int emuirq)
> >>      }
> >>  
> >>      old_emuirq = domain_pirq_to_emuirq(d, pirq);
> >> -    if ( emuirq != IRQ_PT )
> >> +    if ( (emuirq != IRQ_PT) && (emuirq != IRQ_MSI_EMU) )
> >>          old_pirq = domain_emuirq_to_pirq(d, emuirq);
> > 
> > I think you can just use emuirq >= 0, as we then only need the emuirq
> > translation for passthrough interrupts, same for the rest of the
> > changed conditions.

I think this should work


> > Looking further, the function seems to be useless when called with
> > emuirq < 0, and hence it might be better to avoid such calls in the
> > first place?
> > 
> > I have to admit I've always been very confused with the PIRQ logic, so
> > it's possible I'm missing some relevant stuff here.
> 
> For any emuirq questions I'd like to suggest to Cc Stefano, who iirc was
> the one introducing this machinery. Like you I've never gained proper
> understanding of the concept and implementation, and hence I can always
> only refer back to Stefano.

At the time it was introduced as a minor performance improvement and
also because it helped us get the right hooks in place in Linux to
upstream Dom0 support (the MSI/MSI-X setup hooks). The feature came with
a high cost in complexity but it was worth it.

Now that many years have passed, Dom0 support has been in Linux for a
long time, the performance improvement alone is not worth keeping this
complexity in Xen. Especially considering direct interrupt injection is
a feature available in many modern interrupt controllers across arches
(x86, ARM, RISC-V).

I think it is time to get rid of XEN_X86_EMU_USE_PIRQ so that guests
stop using the feature. It is fragile. Removing XEN_X86_EMU_USE_PIRQ is
on my todo list but I welcome anyone doing it.