[Qemu-devel] [PATCH 0/3] x86: ioapic: fix fail migrate when irqchip=split

[Qemu-devel] [PATCH 0/3] x86: ioapic: fix fail migrate when irqchip=split

[Peter Xu]

Split irqchip works based on the fact that we kept the first 24 gsi
routing entries inside KVM for userspace ioapic's use. When system
boot, we'll reserve these MSI routing entries before hand. However,
after migration, we forgot to re-configure it up in the destination
side. The result is, we'll get invalid gsi routing entries after
migration (all empty), and we get interrupts with vector=0, then
strange things happen, like keyboard hang.

The solution is simple - we update them after migration, which is a
one line fix in patch 3.

The first two patches are things I used for debugging this issue.
Though it might not be directly related, I still think it's something
good to have in case we encounter similar interrupt issues in the
future.

Please review, thanks,

Peter Xu (3):
  x86: ioapic: add traces for ioapic
  x86: ioapic: dump version for "info ioapic"
  x86: ioapic: fix fail migration when irqchip=split

 hw/intc/ioapic.c        | 22 ++++++++++++++++++++--
 hw/intc/ioapic_common.c |  3 ++-
 hw/intc/trace-events    |  7 +++++++
 3 files changed, 29 insertions(+), 3 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] x86: ioapic: add traces for ioapic

[Peter Xu]

>From time to time, there are issues with ioapic, either on guest side or
on hypervisor side. Good to have some persistent traces for better
triaging and debugging.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/ioapic.c     | 17 +++++++++++++++--
 hw/intc/trace-events |  7 +++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index ea7ea0b..d1254f8 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -33,6 +33,7 @@
 #include "target/i386/cpu.h"
 #include "hw/i386/apic-msidef.h"
 #include "hw/i386/x86-iommu.h"
+#include "trace.h"
 
 //#define DEBUG_IOAPIC
 
@@ -115,6 +116,7 @@ static void ioapic_service(IOAPICCommonState *s)
                     s->irr &= ~mask;
                 } else {
                     coalesce = s->ioredtbl[i] & IOAPIC_LVT_REMOTE_IRR;
+                    trace_ioapic_set_remote_irr(i);
                     s->ioredtbl[i] |= IOAPIC_LVT_REMOTE_IRR;
                 }
 
@@ -220,6 +222,8 @@ void ioapic_eoi_broadcast(int vector)
     uint64_t entry;
     int i, n;
 
+    trace_ioapic_eoi_broadcast(vector);
+
     for (i = 0; i < MAX_IOAPICS; i++) {
         s = ioapics[i];
         if (!s) {
@@ -229,6 +233,7 @@ void ioapic_eoi_broadcast(int vector)
             entry = s->ioredtbl[n];
             if ((entry & IOAPIC_LVT_REMOTE_IRR)
                 && (entry & IOAPIC_VECTOR_MASK) == vector) {
+                trace_ioapic_clear_remote_irr(n, vector);
                 s->ioredtbl[n] = entry & ~IOAPIC_LVT_REMOTE_IRR;
                 if (!(entry & IOAPIC_LVT_MASKED) && (s->irr & (1 << n))) {
                     ioapic_service(s);
@@ -256,7 +261,9 @@ ioapic_mem_read(void *opaque, hwaddr addr, unsigned int size)
     int index;
     uint32_t val = 0;
 
-    switch (addr & 0xff) {
+    addr &= 0xff;
+
+    switch (addr) {
     case IOAPIC_IOREGSEL:
         val = s->ioregsel;
         break;
@@ -286,6 +293,9 @@ ioapic_mem_read(void *opaque, hwaddr addr, unsigned int size)
         DPRINTF("read: %08x = %08x\n", s->ioregsel, val);
         break;
     }
+
+    trace_ioapic_mem_read(addr, size, val);
+
     return val;
 }
 
@@ -324,7 +334,10 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
     IOAPICCommonState *s = opaque;
     int index;
 
-    switch (addr & 0xff) {
+    addr &= 0xff;
+    trace_ioapic_mem_write(addr, size, val);
+
+    switch (addr) {
     case IOAPIC_IOREGSEL:
         s->ioregsel = val;
         break;
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 340f617..180b893 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -14,6 +14,13 @@ apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode, uint8_t
 apic_mem_readl(uint64_t addr, uint32_t val)  "%"PRIx64" = %08x"
 apic_mem_writel(uint64_t addr, uint32_t val) "%"PRIx64" = %08x"
 
+# hw/intc/ioapic.c
+ioapic_set_remote_irr(int n) "set remote irr for pin %d"
+ioapic_clear_remote_irr(int n, int vector) "clear remote irr for pin %d vector %d"
+ioapic_eoi_broadcast(int vector) "EOI broadcast for vector %d"
+ioapic_mem_read(uint8_t addr, uint8_t size, uint32_t val) "ioapic mem read addr 0x%"PRIx8" size 0x%"PRIx8" retval 0x%"PRIx32
+ioapic_mem_write(uint8_t addr, uint8_t size, uint32_t val) "ioapic mem write addr 0x%"PRIx8" size 0x%"PRIx8" val 0x%"PRIx32
+
 # hw/intc/slavio_intctl.c
 slavio_intctl_mem_readl(uint32_t cpu, uint64_t addr, uint32_t ret) "read cpu %d reg 0x%"PRIx64" = %x"
 slavio_intctl_mem_writel(uint32_t cpu, uint64_t addr, uint32_t val) "write cpu %d reg 0x%"PRIx64" = %x"
-- 
2.7.4

[Qemu-devel] [PATCH 2/3] x86: ioapic: dump version for "info ioapic"

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/ioapic_common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
index 1b7ec5e..97c4f9c 100644
--- a/hw/intc/ioapic_common.c
+++ b/hw/intc/ioapic_common.c
@@ -58,7 +58,8 @@ void ioapic_print_redtbl(Monitor *mon, IOAPICCommonState *s)
     uint32_t remote_irr = 0;
     int i;
 
-    monitor_printf(mon, "ioapic id=0x%02x sel=0x%02x", s->id, s->ioregsel);
+    monitor_printf(mon, "ioapic ver=0x%x id=0x%02x sel=0x%02x",
+                   s->version, s->id, s->ioregsel);
     if (s->ioregsel) {
         monitor_printf(mon, " (redir[%u])\n",
                        (s->ioregsel - IOAPIC_REG_REDTBL_BASE) >> 1);
-- 
2.7.4

[Qemu-devel] [PATCH 3/3] x86: ioapic: fix fail migration when irqchip=split

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/ioapic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index d1254f8..9047b89 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -439,6 +439,11 @@ static void ioapic_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     k->realize = ioapic_realize;
+    /*
+     * If APIC is in kernel, we need to update the kernel cache after
+     * migration, otherwise first 24 gsi routes will be invalid.
+     */
+    k->post_load = ioapic_update_kvm_routes;
     dc->reset = ioapic_reset_common;
     dc->props = ioapic_properties;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/3] x86: ioapic: fix fail migrate when irqchip=split

[Paolo Bonzini]

On 09/01/2017 09:55, Peter Xu wrote:
> Split irqchip works based on the fact that we kept the first 24 gsi
> routing entries inside KVM for userspace ioapic's use. When system
> boot, we'll reserve these MSI routing entries before hand. However,
> after migration, we forgot to re-configure it up in the destination
> side. The result is, we'll get invalid gsi routing entries after
> migration (all empty), and we get interrupts with vector=0, then
> strange things happen, like keyboard hang.
> 
> The solution is simple - we update them after migration, which is a
> one line fix in patch 3.
> 
> The first two patches are things I used for debugging this issue.
> Though it might not be directly related, I still think it's something
> good to have in case we encounter similar interrupt issues in the
> future.
> 
> Please review, thanks,
> 
> Peter Xu (3):
>   x86: ioapic: add traces for ioapic
>   x86: ioapic: dump version for "info ioapic"
>   x86: ioapic: fix fail migration when irqchip=split
> 
>  hw/intc/ioapic.c        | 22 ++++++++++++++++++++--
>  hw/intc/ioapic_common.c |  3 ++-
>  hw/intc/trace-events    |  7 +++++++
>  3 files changed, 29 insertions(+), 3 deletions(-)
> 

Thanks, queued.

Paolo