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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.10.2024 09:57, Roger Pau Monne wrote:
> @@ -71,6 +72,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 breaks directed EOI, as the CPU vector no longer 
> matches
> + * the contents of the RTE vector field.  Add a translation cache so that
> + * directed EOI uses the value in the RTE vector field when interrupt 
> remapping
> + * is enabled.
> + *
> + * Intel VT-d Xen code still stores the CPU vector in the RTE vector field 
> when
> + * using the remapped format, but use the translation cache 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;

__ro_after_init?

> @@ -273,6 +292,17 @@ void __ioapic_write_entry(
>      {
>          __io_apic_write(apic, 0x11 + 2 * pin, eu.w2);
>          __io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
> +        /*
> +         * Might be called before io_apic_pin_eoi is allocated.  Entry will 
> be
> +         * initialized to the RTE value once the cache is allocated.

With the movement of the allocation to enable_IO_APIC(), isn't this part of
the comment stale now?

> +         * The vector field is only cached for raw RTE writes when using IR.
> +         * In that case the vector field might have been repurposed to store
> +         * something different than the CPU vector, and hence need to be 
> cached
> +         * for performing EOI.
> +         */
> +        if ( io_apic_pin_eoi )
> +            io_apic_pin_eoi[apic][pin] = e.vector;

The conditional here is necessary anyway, isn't it (for the allocation
being conditional itself)?

With the adjustments (or clarification of why they cannot be made)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

If the adjustments can be confirmed I'd also be happy to make them while
committing, to save another round-trip.

Jan