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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew