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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Nov 04, 2024 at 10:56:36AM +0100, Jan Beulich wrote:
> 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?

Oh, yes indeed, allocations are static 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?

There are still paths that call __ioapic_write_entry() before
enable_IO_APIC().  See for example how x2apic_bsp_setup() makes use of
save_IO_APIC_setup() ahead of enable_IO_APIC().

> > +         * 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)?

Indeed, the matrix won't be allocated if interrupt remapping is not
enabled.

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

Thanks.

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

I agree with the __ro_after_init, see my reply to the code comment.

Regards, Roger.