[Qemu-devel] [PATCH 0/2] IOAPIC: clear remote IRR for edge interrupts

[Qemu-devel] [PATCH 0/2] IOAPIC: clear remote IRR for edge interrupts

[Peter Xu]

These two patches are seperated from v6 series of Intel IOMMU IR
support.

Existing read-only bits of IOAPIC registers are writable. We'd
better follow the spec to make it read-only. The first patch did
this.

The 2nd patch emulated real IOAPIC behavior that remote IRR bits are
cleared when configuring edge-triggered interrupts. This "feature"
is used by Linux kernel to do explicit EOI for 0x1X IOAPICs. For
more information, please refer to the comments in patch 2. This one
depends on patch 1 to work.

Peter Xu (2):
  ioapic: keep RO bits for IOAPIC entry
  ioapic: clear remote irr bit for edge-triggered interrupts

 hw/intc/ioapic.c                  | 33 +++++++++++++++++++++++++++++++++
 include/hw/i386/ioapic_internal.h |  5 +++++
 2 files changed, 38 insertions(+)

-- 
2.4.11

[Qemu-devel] [PATCH 1/2] ioapic: keep RO bits for IOAPIC entry

[Peter Xu]

Currently IOAPIC RO bits can be written. To be better aligned with
hardware, we should let them read-only.

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/ioapic.c                  | 4 ++++
 include/hw/i386/ioapic_internal.h | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 378e663..ef92673 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -280,6 +280,7 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
         default:
             index = (s->ioregsel - IOAPIC_REG_REDTBL_BASE) >> 1;
             if (index >= 0 && index < IOAPIC_NUM_PINS) {
+                uint64_t ro_bits = s->ioredtbl[index] & IOAPIC_RO_BITS;
                 if (s->ioregsel & 1) {
                     s->ioredtbl[index] &= 0xffffffff;
                     s->ioredtbl[index] |= (uint64_t)val << 32;
@@ -287,6 +288,9 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
                     s->ioredtbl[index] &= ~0xffffffffULL;
                     s->ioredtbl[index] |= val;
                 }
+                /* restore RO bits */
+                s->ioredtbl[index] &= IOAPIC_RW_BITS;
+                s->ioredtbl[index] |= ro_bits;
                 ioapic_service(s);
             }
         }
diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
index 797ed47..cab9e67 100644
--- a/include/hw/i386/ioapic_internal.h
+++ b/include/hw/i386/ioapic_internal.h
@@ -47,6 +47,11 @@
 #define IOAPIC_LVT_DEST_MODE            (1 << IOAPIC_LVT_DEST_MODE_SHIFT)
 #define IOAPIC_LVT_DELIV_MODE           (7 << IOAPIC_LVT_DELIV_MODE_SHIFT)
 
+/* Bits that are read-only for IOAPIC entry */
+#define IOAPIC_RO_BITS                  (IOAPIC_LVT_REMOTE_IRR | \
+                                         IOAPIC_LVT_DELIV_STATUS)
+#define IOAPIC_RW_BITS                  (~(uint64_t)IOAPIC_RO_BITS)
+
 #define IOAPIC_TRIGGER_EDGE             0
 #define IOAPIC_TRIGGER_LEVEL            1
 
-- 
2.4.11

[Qemu-devel] [PATCH 2/2] ioapic: clear remote irr bit for edge-triggered interrupts

[Peter Xu]

This is to better emulate IOAPIC version 0x1X hardware. Linux kernel
leveraged this "feature" to do explicit EOI since EOI register is still
not introduced at that time. This will also fix the issue that level
triggered interrupts failed to work when IR enabled (tested with Linux
kernel version 4.5).

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/ioapic.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index ef92673..51f1c0f 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -254,6 +254,34 @@ ioapic_mem_read(void *opaque, hwaddr addr, unsigned int size)
     return val;
 }
 
+/*
+ * This is to satisfy the hack in Linux kernel. One hack of it is to
+ * simulate clearing the Remote IRR bit of IOAPIC entry using the
+ * following:
+ *
+ * "For IO-APIC's with EOI register, we use that to do an explicit EOI.
+ * Otherwise, we simulate the EOI message manually by changing the trigger
+ * mode to edge and then back to level, with RTE being masked during
+ * this."
+ *
+ * (See linux kernel __eoi_ioapic_pin() comment in commit c0205701)
+ *
+ * This is based on the assumption that, Remote IRR bit will be
+ * cleared by IOAPIC hardware when configured as edge-triggered
+ * interrupts.
+ *
+ * Without this, level-triggered interrupts in IR mode might fail to
+ * work correctly.
+ */
+static inline void
+ioapic_fix_edge_remote_irr(uint64_t *entry)
+{
+    if (!(*entry & IOAPIC_LVT_TRIGGER_MODE)) {
+        /* Edge-triggered interrupts, make sure remote IRR is zero */
+        *entry &= ~((uint64_t)IOAPIC_LVT_REMOTE_IRR);
+    }
+}
+
 static void
 ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
                  unsigned int size)
@@ -291,6 +319,7 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
                 /* restore RO bits */
                 s->ioredtbl[index] &= IOAPIC_RW_BITS;
                 s->ioredtbl[index] |= ro_bits;
+                ioapic_fix_edge_remote_irr(&s->ioredtbl[index]);
                 ioapic_service(s);
             }
         }
-- 
2.4.11

Re: [Qemu-devel] [PATCH 0/2] IOAPIC: clear remote IRR for edge interrupts

[Paolo Bonzini]

On 10/05/2016 12:21, Peter Xu wrote:
> These two patches are seperated from v6 series of Intel IOMMU IR
> support.
> 
> Existing read-only bits of IOAPIC registers are writable. We'd
> better follow the spec to make it read-only. The first patch did
> this.
> 
> The 2nd patch emulated real IOAPIC behavior that remote IRR bits are
> cleared when configuring edge-triggered interrupts. This "feature"
> is used by Linux kernel to do explicit EOI for 0x1X IOAPICs. For
> more information, please refer to the comments in patch 2. This one
> depends on patch 1 to work.
> 
> Peter Xu (2):
>   ioapic: keep RO bits for IOAPIC entry
>   ioapic: clear remote irr bit for edge-triggered interrupts
> 
>  hw/intc/ioapic.c                  | 33 +++++++++++++++++++++++++++++++++
>  include/hw/i386/ioapic_internal.h |  5 +++++
>  2 files changed, 38 insertions(+)
> 

Queued for 2.7, thanks.

Paolo