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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Nov 07, 2019 at 04:28:56PM +0100, Jan Beulich wrote:
> 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?

Because the code does something like:

memset(&rte, 0, ...);
...
__ioapic_write_entry(apic, pin, true, rte);

At which point you misconfigure an ioapic entry that was already setup
to point to an interrupt remapping entry, and the AMD IOMMU code
chokes in the assert below.

> 
> > (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?

Oh yes.

> > --- 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.

Yes, this is the one that's actually incorrect, but see my reasoning
below.

> 
> > -    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.

TBH, I think raw mode should only be used by the iommu code in order
to setup the entries to point to the interrupt remapping table,
everything else shouldn't be using raw mode. While it's true that some
of the cases here are safe to use raw mode I would discourage it's
usage as it can lead to issues, and this is not a performance critical
path anyway.

Thanks, Roger.

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