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

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

On Mon, Oct 21, 2024 at 03:54:35PM +0100, David Woodhouse wrote:
> On Mon, 2024-10-21 at 15:51 +0100, Andrew Cooper wrote:
> > On 21/10/2024 3:06 pm, Roger Pau Monné wrote:
> > > On Mon, Oct 21, 2024 at 12:34:37PM +0100, David Woodhouse wrote:
> > > > On Fri, 2024-10-18 at 10:08 +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@xxxxxx>
> > > > > Suggested-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
> > > > > Fixes: 2ca9fbd739b8 ('AMD IOMMU: allocate IRTE entries instead of 
> > > > > using a static mapping')
> > > > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > > Hm, couldn't we just have used the pin#?
> > > Yes, but that would require a much bigger change that what's currently
> > > presented here, and for backport purposes I think it's better done
> > > this way for fixing this specific bug.
> > > 
> > > Changing to use pin# as the IR offset is worthwhile, but IMO needs to
> > > be done separated from the bugfix here.
> > > 
> > > > The AMD IOMMU has per-device IRTE, so you *know* you can just use IRTE
> > > > indices 0-23 for the I/O APIC pins.
> > > Aren't there IO-APICs with more than 24 pins?
> > 
> > Recent Intel SoCs have a single IO-APIC with 120 pins.
> 
> And Xen offers a 32-pin one to guests IIRC. I should have said. 'you
> can just use IRTE indices 0-(N-1) for the I/O APIC pins'.

Indeed, my comment was about the hardcoding of 24 pins, as I recall
seeing IO-APICs with more pins.

> The point is the IRTE is per-device, unless the platform has more than
> one I/O APIC with the *same* requester-id.

Yup, just wanted to clarify whether there was a reason for you to
mention 0-23 explicitly, didn't mean to be pedantic (but possibly
sounded like that).

Thanks, Roger