On 24/10/2024 4:48 pm, 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]> > --- > Changes since v1: > - s/apic_pin_eoi/io_apic_pin_eoi/. > - Expand comment about io_apic_pin_eoi usage and layout. > - Use uint8_t instead of unsigned int as array type. > - Do not use a sentinel value. > --- > xen/arch/x86/io_apic.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c > index e40d2f7dbd75..e3cdfab6359a 100644 > --- a/xen/arch/x86/io_apic.c > +++ b/xen/arch/x86/io_apic.c > @@ -71,6 +71,24 @@ 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. > + * > + * The matrix is accessed as [#io-apic][#pin]. > + */ > +static uint8_t **io_apic_pin_eoi; > + > static void share_vector_maps(unsigned int src, unsigned int dst) > { > unsigned int pin; > @@ -273,6 +291,13 @@ void __ioapic_write_entry( > { > __io_apic_write(apic, 0x11 + 2 * pin, eu.w2); > __io_apic_write(apic, 0x10 + 2 * pin, eu.w1); > + /* > + * Called in clear_IO_APIC_pin() before io_apic_pin_eoi is allocated. > + * Entry will be updated once the array is allocated and there's a > + * write against the pin. > + */ > + if ( io_apic_pin_eoi ) > + io_apic_pin_eoi[apic][pin] = e.vector; > } > else > iommu_update_ire_from_apic(apic, pin, e.raw); > @@ -298,6 +323,9 @@ static void __io_apic_eoi(unsigned int apic, unsigned int > vector, unsigned int p > /* Prefer the use of the EOI register if available */ > if ( ioapic_has_eoi_reg(apic) ) > { > + if ( io_apic_pin_eoi ) > + vector = io_apic_pin_eoi[apic][pin]; > + > /* If vector is unknown, read it from the IO-APIC */ > if ( vector == IRQ_VECTOR_UNASSIGNED )
I'm not sure this works. https://godbolt.org/z/1a55PnKGq io_apic_pin_eoi[apic][pin] gets zero extended when assigning to vector, which can then never match IRQ_VECTOR_UNASSIGNED. Or doesn't this no longer matter in v2? ~Andrew
