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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/10/2024 6:00 pm, Roger Pau Monné wrote:
> 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.
> 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.

Yeah, that occurred to me after sending.  With IR active, it's no longer
an architectural vector, and can have any 8-bit value.

> 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?

Either of those are still half the allocated memory vs unsigned int, so
worth it IMO.

~Andrew