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 <xenproj...@ymy.be>
>>>> Suggested-by: David Woodhouse <d...@amazon.co.uk>
>>>> Fixes: 2ca9fbd739b8 ('AMD IOMMU: allocate IRTE entries instead of using a
>>>> static mapping')
>>>> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
>>> 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
