Re: [PATCH] x86/hvm: don't expose XENFEAT_hvm_pirqs by default

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jan 11, 2024 at 05:47:30PM +0000, David Woodhouse wrote:
> On Wed, 2024-01-10 at 11:26 +0100, Jan Beulich wrote:
> > On 10.01.2024 10:53, Roger Pau Monne wrote:
> > > The HVM pirq feature allows routing interrupts from both physical and 
> > > emulated
> > > devices over event channels, this was done a performance improvement.  
> > > However
> > > its usage is fully undocumented, and the only reference implementation is 
> > > in
> > > Linux.  It defeats the purpose of local APIC hardware virtualization, 
> > > because
> > > when using it interrupts avoid the usage of the local APIC altogether.
> > 
> > So without sufficient APIC acceleration, isn't this arranging for degraded
> > performance then? IOW should the new default perhaps be dependent on the
> > degree of APIC acceleration?
> 
> In fact Linux already declines to use MSI → PIRQ routing if APIC
> acceleration is enabled.
> 
> static void __init xen_hvm_msi_init(void)
> {
>         if (!apic_is_disabled) {
>                 /*
>                  * If hardware supports (x2)APIC virtualization (as indicated
>                  * by hypervisor's leaf 4) then we don't need to use pirqs/
>                  * event channels for MSI handling and instead use regular
>                  * APIC processing
>                  */
>                 uint32_t eax = cpuid_eax(xen_cpuid_base() + 4);
> 
>                 if (((eax & XEN_HVM_CPUID_X2APIC_VIRT) && x2apic_mode) ||
>                     ((eax & XEN_HVM_CPUID_APIC_ACCESS_VIRT) && 
> boot_cpu_has(X86_FEATURE_APIC)))
>                         return;
>         }
>         xen_setup_pci_msi();
> }
> 
> > > It has also been reported to not work properly with certain devices, at 
> > > least
> > > when using some AMD GPUs Linux attempts to route interrupts over event
> > > channels, but Xen doesn't correctly detect such routing, which leads to 
> > > the
> > > hypervisor complaining with:
> > > 
> > > (XEN) d15v0: Unsupported MSI delivery mode 7 for Dom15
> > > 
> > > When MSIs are attempted to be routed over event channels the entry 
> > > delivery
> > > mode is set to ExtINT, but Xen doesn't detect such routing and attempts to
> > > inject the interrupt following the native MSI path, and the ExtINT 
> > > delivery
> > > mode is not supported.
> > 
> > Shouldn't this be properly addressed nevertheless? The way it's described
> > it sounds as if MSI wouldn't work at all this way; I can't spot why the
> > issue would only be "with certain devices". Yet that in turn doesn't look
> > to be very likely - pass-through use cases, in particular SR-IOV ones,
> > would certainly have noticed.
> 
> I agree. The MSI to PIRQ routing thing is *awful*, especially the way
> that Xen/QEMU snoops on writes to the MSI table while the target is
> *masked*, and then Xen unmasks the MSI instead of the guest doing so.
> 
> But it does work, and there are three implementations of it on the
> hypervisor side now (Xen itself, QEMU and the Nitro hypervisor).

There's only one implementation in Xen, that's split between the
hypervisor and QEMU.  IOW: it's not like the QEMU and the hypervisor
sides are independent.

It's also only used by Linux, I don't know of any other guest having
implemented support for them (thankfully I should say).

> We
> should fix the bug which is being reported, but I don't see why it's
> necessary to completely disable the feature.

It's not just this bug, the feature is fully undocumented, and every
time there's an issue reported against interrupt delivery on HVM Linux
we need to reverse engineer how this is supposed to work.

Not to mention the MSI breakage it introduces by re-using the MSI
data and address fields for Xen specific purposes.

Note that this commit is not removing the code, just disabling it by
default.  Users can still enable it using hvm_pirq option on xl.cfg
(and toolstacks can use the domctl create flag to enable it).

IMO if someone whats to make a case for having HVM PIRQs enabled by
default, we first need documentation about how it's supposed to work,
plus all the reported bugs fixed.  I have to admit I would probably be
reluctant to enable it by default even then.

Apart from the original issue, Xenia has also reported that when
having the option enabled they see some unexpected scheduling
imbalance, that's gone when the option is disabled:

https://lore.kernel.org/xen-devel/6fe776cd-3fa6-421f-9d02-9350e85d5612@xxxxxxx/

Regards, Roger.