Re: [Xen-devel] [PATCH for-4.13 2/2] x86/ioapic: don't use raw entry reads/writes in clear_IO_APIC_pin

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.11.2019 16:06, Roger Pau Monne wrote:
> clear_IO_APIC_pin can be called after the iommu has been enabled, and
> using raw entry reads and writes will result in a misconfiguration of
> the entries already setup to use the interrupt remapping table.

I'm afraid I don't understand this: Raw reads and writes don't even
go to the IOMMU interrupt remapping code, so how would the assertion
be triggered?

> (XEN) [   10.082154] ENABLING IO-APIC IRQs
> (XEN) [   10.087789]  -> Using new ACK method
> (XEN) [   10.093738] Assertion 'get_rte_index(rte) == offset' failed at 
> iommu_intr.c:328
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

"Reported-by: Sergey ..." ahead of this?

> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -514,13 +514,13 @@ static void clear_IO_APIC_pin(unsigned int apic, 
> unsigned int pin)
>          entry.mask = 1;
>          __ioapic_write_entry(apic, pin, false, entry);
>      }
> -    entry = __ioapic_read_entry(apic, pin, true);
> +    entry = __ioapic_read_entry(apic, pin, false);
>  
>      if (entry.irr) {
>          /* Make sure the trigger mode is set to level. */
>          if (!entry.trigger) {
>              entry.trigger = 1;
> -            __ioapic_write_entry(apic, pin, true, entry);
> +            __ioapic_write_entry(apic, pin, false, entry);
>          }

All we do here is set the trigger bit. No translation back and forth
of the RTE should be needed.

> @@ -530,9 +530,9 @@ static void clear_IO_APIC_pin(unsigned int apic, unsigned 
> int pin)
>       */
>      memset(&entry, 0, sizeof(entry));
>      entry.mask = 1;
> -    __ioapic_write_entry(apic, pin, true, entry);
> +    __ioapic_write_entry(apic, pin, false, entry);

I may be able to understand why this one can't use raw mode, but as
per above a better overall description is needed.

> -    entry = __ioapic_read_entry(apic, pin, true);
> +    entry = __ioapic_read_entry(apic, pin, false);
>      if (entry.irr)
>          printk(KERN_ERR "IO-APIC%02x-%u: Unable to reset IRR\n",
>                 IO_APIC_ID(apic), pin);

This read again shouldn't need conversion, as the IRR bit doesn't
get touched (I think) by the interrupt remapping code during the
translation it does.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel