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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Oct 21, 2024 at 12:38:13PM +0100, Andrew Cooper wrote:
> On 21/10/2024 12:10 pm, Andrew Cooper wrote:
> > On 18/10/2024 9:08 am, 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>
> > Yet more fallout from the multi-MSI work.  That really has been a giant
> > source of bugs.
> >
> >> ---
> >>  xen/arch/x86/io_apic.c | 47 ++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 47 insertions(+)
> >>
> >> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> >> index e40d2f7dbd75..8856eb29d275 100644
> >> --- a/xen/arch/x86/io_apic.c
> >> +++ b/xen/arch/x86/io_apic.c
> >> @@ -71,6 +71,22 @@ static int apic_pin_2_gsi_irq(int apic, int pin);
> >>  
> >>  static vmask_t *__read_mostly vector_map[MAX_IO_APICS];
> >>  
> >> +/*
> >> + * Store the EOI handle when using interrupt remapping.
> >> + *
> >> + * If using AMD-Vi interrupt remapping the IO-APIC redirection entry remapped
> >> + * format repurposes the vector field to store the offset into the Interrupt
> >> + * Remap table.  This causes directed EOI to longer work, as the CPU vector no
> >> + * longer matches the contents of the RTE vector field.  Add a translation
> >> + * table so that directed EOI uses the value in the RTE vector field when
> >> + * interrupt remapping is enabled.
> >> + *
> >> + * Note Intel VT-d Xen code still stores the CPU vector in the RTE vector field
> >> + * when using the remapped format, but use the translation table uniformly in
> >> + * order to avoid extra logic to differentiate between VT-d and AMD-Vi.
> >> + */
> >> +static unsigned int **apic_pin_eoi;
> > I think we can get away with this being uint8_t rather than unsigned
> > int, especially as we're allocating memory when not strictly necessary.
> >
> > The only sentinel value we use is IRQ_VECTOR_UNASSIGNED which is -1.
> >
> > Vector 0xff is strictly SPIV and not allocated for anything else, so can
> > be reused as a suitable sentinel here.
> 
> Actually, vectors 0 thru 0x0f are also strictly invalid, and could be
> used as sentinels.  That's probably better than trying to play integer
> promotion games between IRQ_VECTOR_UNASSIGNED and uint8_t.

I've been giving some thought about this further, and I don't think
the above is accurate.  While vectors 0 thru 0x0f are strictly
invalid, the EOI handle in AMD-Vi is not a vector, but an offset into
the IR table.  Hence the range of valid handles is 0 to 0xff.

So the type of apic_pin_eoi needs to account for 0 to 0xff plus one
sentinel.  We could use uint16_t or int16_t, but at that point it
might be better to just use unsigned int?

Thanks, Roger.