[for-4.13,v2,2/2] x86/ioapic: fix clear_IO_APIC_pin write of raw entries

Commit Message

Roger Pau Monne Nov. 10, 2019, 9:25 a.m. UTC

clear_IO_APIC_pin can be called after the iommu has been enabled, and
using raw reads and writes to modify IO-APIC entries that have been
setup to use interrupt remapping can lead to issues as some of the
fields have different meaning when the IO-APIC entry is setup to point
to an interrupt remapping table entry.

The following ASSERT in AMD IOMMU code triggers afterwards as a result
of the raw changes to IO-APIC entries performed by clear_IO_APIC_pin.

(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

Fix this by making sure that modifications to entries are performed in
non raw mode.

Reported-by: Sergey Dyasli <[e-mail]>
Signed-off-by: Roger Pau Monné <[e-mail]>
---
Cc: Juergen Gross <[e-mail]>
---
Changes since v1:
 - Do not change all instances of raw reads.
 - Fix commit message
---
 xen/arch/x86/io_apic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Jan Beulich Nov. 11, 2019, 9:56 a.m. UTC | #1

On 10.11.2019 10:25, Roger Pau Monne wrote:
> clear_IO_APIC_pin can be called after the iommu has been enabled, and
> using raw reads and writes to modify IO-APIC entries that have been
> setup to use interrupt remapping can lead to issues as some of the
> fields have different meaning when the IO-APIC entry is setup to point
> to an interrupt remapping table entry.
> 
> The following ASSERT in AMD IOMMU code triggers afterwards as a result
> of the raw changes to IO-APIC entries performed by clear_IO_APIC_pin.
> 
> (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
> 
> Fix this by making sure that modifications to entries are performed in
> non raw mode.

... when fields are affected which may either have changed meaning
with interrupt remapping, or which may need mirroring into IRTEs.

> Reported-by: Sergey Dyasli <[e-mail]>
> Signed-off-by: Roger Pau Monné <[e-mail]>

With the above addition (or something substantially similar)
Reviewed-by: Jan Beulich <[e-mail]>
Of course the adjustment is easy enough to do while committing.

Jan

Roger Pau Monne Nov. 12, 2019, 9:34 a.m. UTC | #2

On Mon, Nov 11, 2019 at 10:56:21AM +0100, Jan Beulich wrote:
> On 10.11.2019 10:25, Roger Pau Monne wrote:
> > clear_IO_APIC_pin can be called after the iommu has been enabled, and
> > using raw reads and writes to modify IO-APIC entries that have been
> > setup to use interrupt remapping can lead to issues as some of the
> > fields have different meaning when the IO-APIC entry is setup to point
> > to an interrupt remapping table entry.
> > 
> > The following ASSERT in AMD IOMMU code triggers afterwards as a result
> > of the raw changes to IO-APIC entries performed by clear_IO_APIC_pin.
> > 
> > (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
> > 
> > Fix this by making sure that modifications to entries are performed in
> > non raw mode.
> 
> ... when fields are affected which may either have changed meaning
> with interrupt remapping, or which may need mirroring into IRTEs.
> 
> > Reported-by: Sergey Dyasli <[e-mail]>
> > Signed-off-by: Roger Pau Monné <[e-mail]>
> 
> With the above addition (or something substantially similar)
> Reviewed-by: Jan Beulich <[e-mail]>
> Of course the adjustment is easy enough to do while committing.

The adjustment LGTM, please do it at commit time unless there's
something else that requires a resend of the series.

Thanks, Roger.

Patch

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index b9c66acdb3..732b57995c 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -519,8 +519,9 @@  static void clear_IO_APIC_pin(unsigned int apic, unsigned int pin)
     if (entry.irr) {
         /* Make sure the trigger mode is set to level. */
         if (!entry.trigger) {
+            entry = __ioapic_read_entry(apic, pin, false);
             entry.trigger = 1;
-            __ioapic_write_entry(apic, pin, true, entry);
+            __ioapic_write_entry(apic, pin, false, entry);
         }
         __io_apic_eoi(apic, entry.vector, pin);
     }
@@ -530,7 +531,7 @@  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);
 
     entry = __ioapic_read_entry(apic, pin, true);
     if (entry.irr)