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 <[email protected]> > >> Suggested-by: David Woodhouse <[email protected]> > >> Fixes: 2ca9fbd739b8 ('AMD IOMMU: allocate IRTE entries instead of using a > >> static mapping') > >> Signed-off-by: Roger Pau Monné <[email protected]> > > 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.
