Re: [PATCH] x86/io-apic: fix directed EOI when using AMd-Vi interrupt remapping

["marmarek@invisiblethingslab.com" <marmarek@invisiblethingslab.com>]

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

On Mon, Oct 21, 2024 at 11:43:04AM +0000, Woodhouse, David wrote:
> On Sat, 2024-10-19 at 05:23 +0200, Marek Marczykowski-Górecki wrote:
> > On Fri, Oct 18, 2024 at 10:08:13AM +0200, Roger Pau Monne wrote:
> > > When using AMD-VI interrupt remapping the vector field in the IO-APIC RTE is
> > > repurposed to contain part of the offset into the remapping table.  Previous to
> > > 2ca9fbd739b8 Xen had logic so that the offset into the interrupt remapping
> > > table would match the vector.  Such logic was mandatory for end of interrupt to
> > > work, since the vector field (even when not containing a vector) is used by the
> > > IO-APIC to find for which pin the EOI must be performed.
> > > 
> > > Introduce a table to store the EOI handlers when using interrupt remapping, so
> > > that the IO-APIC driver can translate pins into EOI handlers without having to
> > > read the IO-APIC RTE entry.  Note that to simplify the logic such table is used
> > > unconditionally when interrupt remapping is enabled, even if strictly it would
> > > only be required for AMD-Vi.
> > > 
> > > Reported-by: Willi Junga <xenproject@ymy.be>
> > > Suggested-by: David Woodhouse <dwmw@amazon.co.uk>
> > > Fixes: 2ca9fbd739b8 ('AMD IOMMU: allocate IRTE entries instead of using a static mapping')
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > I can confirm it fixes touchpad issue on Framework 13 AMD,
> > it works without ioapic_ack=new now, thanks!
> > Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> 
> Thanks for testing. But... why did this work with the auto-EOI? That
> *should* have had exactly the same problem, surely? 
> 
> The problem fixed by this patch is that the directed EOI used the
> actual vector# and *not* the bits that the I/O APIC *thinks* are the
> vector#, which are actually the IRTE index#.
> 
> But if you let the CPU do its broadcast EOI then surely *that* is going
> to use the actual vector# too, and have precisely the same problem?
> 
> If you use the code prior to this patch, *without* ioapic_ack=new (i.e.
> the mode that was failing), what happens if you do this:
> 
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -595,7 +595,7 @@ void setup_local_APIC(void)
>      /*
>       * Enable directed EOI
>       */
> -    if ( directed_eoi_enabled )
> +    if ( 0 && directed_eoi_enabled )
>      {
>          value |= APIC_SPIV_DIRECTED_EOI;
>          apic_printk(APIC_VERBOSE, "Suppress EOI broadcast on CPU#%d\n",
> 
> 
> I'm guessing that 'fixes' it too? In which case, it looks like AMD has
> some undocumented hack in between its APIC and I/O APIC to let it
> magically auto-EOI the correct pin somehow?

So, this does _not_ fix the touchpad issue.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]