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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.10.2024 12:03, 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.

In here I think you mean "handle" when you use "handler"? Plus with what you 
said
earlier about vector vs EOI handle, and with the code using "vector" all over 
the
place, their (non-)relationship could also do with clarifying (perhaps better in
a code comment in __io_apic_eoi()).

> @@ -273,6 +293,13 @@ void __ioapic_write_entry(
>      {
>          __io_apic_write(apic, 0x11 + 2 * pin, eu.w2);
>          __io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
> +        /*
> +         * Called in clear_IO_APIC_pin() before io_apic_pin_eoi is allocated.
> +         * Entry will be updated once the array is allocated and there's a
> +         * write against the pin.
> +         */
> +        if ( io_apic_pin_eoi )
> +            io_apic_pin_eoi[apic][pin] = e.vector;

The comment here looks a little misleading to me. clear_IO_APIC_pin() calls
here to, in particular, set the mask bit. With the mask bit the vector isn't
meaningful anyway (and indeed clear_IO_APIC_pin() sets it to zero, at which
point recording IRQ_VECTOR_UNASSIGNED might be better than the bogus vector
0x00).

> @@ -298,9 +325,17 @@ static void __io_apic_eoi(unsigned int apic, unsigned 
> int vector, unsigned int p
>      /* Prefer the use of the EOI register if available */
>      if ( ioapic_has_eoi_reg(apic) )
>      {
> +        if ( io_apic_pin_eoi )
> +            vector = io_apic_pin_eoi[apic][pin];
> +
>          /* If vector is unknown, read it from the IO-APIC */
>          if ( vector == IRQ_VECTOR_UNASSIGNED )
> +        {
>              vector = __ioapic_read_entry(apic, pin, true).vector;

Related to my comment higher up regarding vector vs EOI handle: Here we're
doing a raw read, i.e. we don't really fetch the vector but the EOI handle
in the AMD case. Why is it that this isn't sufficient for directed EOI to
work (perhaps with the conditional adjusted)?

Then again - are we ever taking this path? Certainly not when coming from
clear_IO_APIC_pin(), hence ...

> +            if ( io_apic_pin_eoi )

... I'm unconvinced this conditional is needed.

> +                /* Update cached value so further EOI don't need to fetch 
> it. */
> +                io_apic_pin_eoi[apic][pin] = vector;
> +        }
>  
>          *(IO_APIC_BASE(apic)+16) = vector;
>      }
> @@ -1022,8 +1057,27 @@ static void __init setup_IO_APIC_irqs(void)
>  
>      apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
>  
> +    if ( iommu_intremap )
> +    {
> +        io_apic_pin_eoi = xmalloc_array(typeof(*io_apic_pin_eoi), 
> nr_ioapics);

Nit: Strictly speaking this and ...

> +        BUG_ON(!io_apic_pin_eoi);
> +    }
> +
>      for (apic = 0; apic < nr_ioapics; apic++) {
> -        for (pin = 0; pin < nr_ioapic_entries[apic]; pin++) {
> +        const unsigned int nr_entries = nr_ioapic_entries[apic];
> +
> +        if ( iommu_intremap )
> +        {
> +            io_apic_pin_eoi[apic] = xmalloc_array(typeof(**io_apic_pin_eoi),
> +                                                  nr_entries);

... and this should be xvmalloc_array() in new code.

Also this 2nd conditional may better use io_apic_pin_eoi, such that the two
conditionals don't need keeping in sync. Note also how Andrew previously
pointed out that both conditionals aren't Misra-compliant right now.

Jan