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

Hm, do you mean to change IRQ_VECTOR_UNASSIGNED to be 0 instead of -1?
Those vectors are equally invalid to be used for external interrupts,
and hence should never be in an irq_desc.

Otherwise just using 0 for the EOI table sentinel will make the code a
bit more ugly IMO:

if ( apic_pin_eoi )
    vector = apic_pin_eoi[apic][pin] ?: IRQ_VECTOR_UNASSIGNED;

/* If vector is unknown, read it from the IO-APIC */
if ( vector == IRQ_VECTOR_UNASSIGNED )
{
    vector = __ioapic_read_entry(apic, pin, true).vector;
    if ( apic_pin_eoi )
        /* Update cached value so further EOI don't need to fetch it. */
        apic_pin_eoi[apic][pin] = vector;
}

Thanks, Roger.