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

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

[Xenia Ragiadakou]

Do not use emuirq mappings for MSIs injected by emulated devices.
This kind of pirq shares the same emuirq value and is not remapped.

Fixes: 88fccdd11ca0 ('xen: event channel remapping for emulated MSIs')
Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
---

Question: is there any strong reason why Linux HVM guests still use pirqs?

 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);
 
     if ( (old_emuirq != IRQ_UNBOUND && (old_emuirq != emuirq) ) ||
@@ -2699,8 +2699,8 @@ int map_domain_emuirq_pirq(struct domain *d, int pirq, int emuirq)
     if ( !info )
         return -ENOMEM;
 
-    /* do not store emuirq mappings for pt devices */
-    if ( emuirq != IRQ_PT )
+    /* do not store emuirq mappings for pt devices and emulated MSIs */
+    if ( (emuirq != IRQ_PT) && (emuirq != IRQ_MSI_EMU) )
     {
         int err = radix_tree_insert(&d->arch.hvm.emuirq_pirq, emuirq,
                                     radix_tree_int_to_ptr(pirq));
@@ -2753,7 +2753,7 @@ int unmap_domain_pirq_emuirq(struct domain *d, int pirq)
         info->arch.hvm.emuirq = IRQ_UNBOUND;
         pirq_cleanup_check(info, d);
     }
-    if ( emuirq != IRQ_PT )
+    if ( (emuirq != IRQ_PT) && (emuirq != IRQ_MSI_EMU) )
         radix_tree_delete(&d->arch.hvm.emuirq_pirq, emuirq);
 
  done:
-- 
2.34.1

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

[Roger Pau Monné]

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?

> Fixes: 88fccdd11ca0 ('xen: event channel remapping for emulated MSIs')
> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> ---
> 
> 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.

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.

Thanks, Roger.

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

[Jan Beulich]

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@amd.com>
>> ---
>>
>> 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.
> 
> 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.

Jan

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3736 bytes --]

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@amd.com>
> >> ---
> >>
> >> 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.