[Qemu-devel] [PATCH uq/master 0/3] Fix MSI injection at load time

[Qemu-devel] [PATCH uq/master 0/3] Fix MSI injection at load time

[Paolo Bonzini]

Alexander Larsson reported a migration bug where after migration
the Windows virtio-serial driver was not able to read anymore, not seeing
the data from the host.  He debugged it and noticed that after migration
the virtio-serial driver ddid not respond to any irqs.

During restore we virtio_notify() on the serial device, which eventually
raises the pci irq level to 1. However, the driver is never notified
and thus never responds to this by reading the VIRTIO_PCI_ISR, so the
irq level is never cleared, and all later virtio_notify() do nothing.

A simplified reproducer (that doesn't hang Linux,
but shows the message) is to start the VM without a backend for the
virtserialport, and to resume it with a backend, for example

$ qemu-system-x86_64 -device virtio-serial-pci -device virtserialport test.img --enable-kvm -m 512
$ qemu-system-x86_64 -device virtio-serial-pci -chardev stdio,id=vs0 -device virtserialport,chardev=vs0 test.img --enable-kvm -m 512 -incoming 'exec:cat foo.ckp'

In fact, interrupt injection fails and reports correctly "KVM: injection
failed, MSI lost".  The reason for the failure is that the LAPIC doesn't
think it's enabled, which in turn is because the LAPIC is restored after
the CPU and, when restoring the CPU, a dummy post-reset state is passed
to the in-kernel APIC.

The fix for this is to let the APIC update its in-kernel counterpart
after loading.  Patches 1 and 2 change the hard-coded references to
kvm_get_apic_state and kvm_put_apic_state to methods in APICCommonClass.
This is useful because it lets APICCommon force an update of the in-kernel
state after load (patch 3).

Patches 4 and 5 similarly add get/put methods to the IOAPIC hierarchy,
which replace pre_save/post_load.

Paolo

Paolo Bonzini (5):
  kvm: move KVM_GET_LAPIC/KVM_SET_LAPIC to hw/kvm/apic.c
  apic: add get/put methods
  apic: always update the in-kernel status after loading
  ioapic: change pre_save/post_load methods to get/put
  ioapic: unify reset callbacks

 hw/apic.h            |    2 +
 hw/apic_common.c     |   33 ++++++++++++++++++++
 hw/apic_internal.h   |    2 +
 hw/ioapic.c          |    2 -
 hw/ioapic_common.c   |   42 +++++++++++++++++---------
 hw/ioapic_internal.h |    6 +--
 hw/kvm/apic.c        |   80 ++++++++++++++++++++++++++++----------------------
 hw/kvm/ioapic.c      |   13 +-------
 kvm.h                |    3 --
 target-i386/kvm.c    |   34 +--------------------
 10 files changed, 115 insertions(+), 102 deletions(-)

[Qemu-devel] [PATCH 1/3] kvm: move KVM_GET_LAPIC/KVM_SET_LAPIC to hw/kvm/apic.c

[Paolo Bonzini]

Leave knowledge of the KVM in-kernel LAPIC ioctls to hw/kvm/apic.c.
The CPU doesn't need to know anything about kvm_lapic_state.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/kvm/apic.c     |   76 ++++++++++++++++++++++++++++++-----------------------
 kvm.h             |    4 +-
 target-i386/kvm.c |   14 +--------
 3 files changed, 47 insertions(+), 47 deletions(-)

diff --git a/hw/kvm/apic.c b/hw/kvm/apic.c
index dbac7ff..ddf6b7d 100644
--- a/hw/kvm/apic.c
+++ b/hw/kvm/apic.c
@@ -25,62 +25,72 @@ static inline uint32_t kvm_apic_get_reg(struct kvm_lapic_state *kapic,
     return *((uint32_t *)(kapic->regs + (reg_id << 4)));
 }
 
-void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
+int kvm_put_apic_state(DeviceState *d)
 {
     APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    struct kvm_lapic_state kapic;
     int i;
 
-    memset(kapic, 0, sizeof(*kapic));
-    kvm_apic_set_reg(kapic, 0x2, s->id << 24);
-    kvm_apic_set_reg(kapic, 0x8, s->tpr);
-    kvm_apic_set_reg(kapic, 0xd, s->log_dest << 24);
-    kvm_apic_set_reg(kapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
-    kvm_apic_set_reg(kapic, 0xf, s->spurious_vec);
+    memset(&kapic, 0, sizeof(kapic));
+    kvm_apic_set_reg(&kapic, 0x2, s->id << 24);
+    kvm_apic_set_reg(&kapic, 0x8, s->tpr);
+    kvm_apic_set_reg(&kapic, 0xd, s->log_dest << 24);
+    kvm_apic_set_reg(&kapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
+    kvm_apic_set_reg(&kapic, 0xf, s->spurious_vec);
     for (i = 0; i < 8; i++) {
-        kvm_apic_set_reg(kapic, 0x10 + i, s->isr[i]);
-        kvm_apic_set_reg(kapic, 0x18 + i, s->tmr[i]);
-        kvm_apic_set_reg(kapic, 0x20 + i, s->irr[i]);
+        kvm_apic_set_reg(&kapic, 0x10 + i, s->isr[i]);
+        kvm_apic_set_reg(&kapic, 0x18 + i, s->tmr[i]);
+        kvm_apic_set_reg(&kapic, 0x20 + i, s->irr[i]);
     }
-    kvm_apic_set_reg(kapic, 0x28, s->esr);
-    kvm_apic_set_reg(kapic, 0x30, s->icr[0]);
-    kvm_apic_set_reg(kapic, 0x31, s->icr[1]);
+    kvm_apic_set_reg(&kapic, 0x28, s->esr);
+    kvm_apic_set_reg(&kapic, 0x30, s->icr[0]);
+    kvm_apic_set_reg(&kapic, 0x31, s->icr[1]);
     for (i = 0; i < APIC_LVT_NB; i++) {
-        kvm_apic_set_reg(kapic, 0x32 + i, s->lvt[i]);
+        kvm_apic_set_reg(&kapic, 0x32 + i, s->lvt[i]);
     }
-    kvm_apic_set_reg(kapic, 0x38, s->initial_count);
-    kvm_apic_set_reg(kapic, 0x3e, s->divide_conf);
+    kvm_apic_set_reg(&kapic, 0x38, s->initial_count);
+    kvm_apic_set_reg(&kapic, 0x3e, s->divide_conf);
+
+    return kvm_vcpu_ioctl(s->cpu_env, KVM_SET_LAPIC, &kapic);
 }
 
-void kvm_get_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
+int kvm_get_apic_state(DeviceState *d)
 {
     APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
-    int i, v;
-
-    s->id = kvm_apic_get_reg(kapic, 0x2) >> 24;
-    s->tpr = kvm_apic_get_reg(kapic, 0x8);
-    s->arb_id = kvm_apic_get_reg(kapic, 0x9);
-    s->log_dest = kvm_apic_get_reg(kapic, 0xd) >> 24;
-    s->dest_mode = kvm_apic_get_reg(kapic, 0xe) >> 28;
-    s->spurious_vec = kvm_apic_get_reg(kapic, 0xf);
+    struct kvm_lapic_state kapic;
+    int i, v, ret;
+
+    ret = kvm_vcpu_ioctl(s->cpu_env, KVM_GET_LAPIC, &kapic);
+    if (ret < 0) {
+        return ret;
+    }
+
+    s->id = kvm_apic_get_reg(&kapic, 0x2) >> 24;
+    s->tpr = kvm_apic_get_reg(&kapic, 0x8);
+    s->arb_id = kvm_apic_get_reg(&kapic, 0x9);
+    s->log_dest = kvm_apic_get_reg(&kapic, 0xd) >> 24;
+    s->dest_mode = kvm_apic_get_reg(&kapic, 0xe) >> 28;
+    s->spurious_vec = kvm_apic_get_reg(&kapic, 0xf);
     for (i = 0; i < 8; i++) {
-        s->isr[i] = kvm_apic_get_reg(kapic, 0x10 + i);
-        s->tmr[i] = kvm_apic_get_reg(kapic, 0x18 + i);
-        s->irr[i] = kvm_apic_get_reg(kapic, 0x20 + i);
+        s->isr[i] = kvm_apic_get_reg(&kapic, 0x10 + i);
+        s->tmr[i] = kvm_apic_get_reg(&kapic, 0x18 + i);
+        s->irr[i] = kvm_apic_get_reg(&kapic, 0x20 + i);
     }
-    s->esr = kvm_apic_get_reg(kapic, 0x28);
-    s->icr[0] = kvm_apic_get_reg(kapic, 0x30);
-    s->icr[1] = kvm_apic_get_reg(kapic, 0x31);
+    s->esr = kvm_apic_get_reg(&kapic, 0x28);
+    s->icr[0] = kvm_apic_get_reg(&kapic, 0x30);
+    s->icr[1] = kvm_apic_get_reg(&kapic, 0x31);
     for (i = 0; i < APIC_LVT_NB; i++) {
-        s->lvt[i] = kvm_apic_get_reg(kapic, 0x32 + i);
+        s->lvt[i] = kvm_apic_get_reg(&kapic, 0x32 + i);
     }
-    s->initial_count = kvm_apic_get_reg(kapic, 0x38);
-    s->divide_conf = kvm_apic_get_reg(kapic, 0x3e);
+    s->initial_count = kvm_apic_get_reg(&kapic, 0x38);
+    s->divide_conf = kvm_apic_get_reg(&kapic, 0x3e);
 
     v = (s->divide_conf & 3) | ((s->divide_conf >> 1) & 4);
     s->count_shift = (v + 1) & 7;
 
     s->initial_count_load_time = qemu_get_clock_ns(vm_clock);
     apic_next_timer(s, s->initial_count_load_time);
+    return 0;
 }
 
 static void kvm_apic_set_base(APICCommonState *s, uint64_t val)
diff --git a/kvm.h b/kvm.h
index 2b26dcb..0056f92 100644
--- a/kvm.h
+++ b/kvm.h
@@ -191,8 +191,8 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
 
 void kvm_irqchip_add_irq_route(KVMState *s, int gsi, int irqchip, int pin);
 
-void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic);
-void kvm_get_apic_state(DeviceState *d, struct kvm_lapic_state *kapic);
+int kvm_put_apic_state(DeviceState *d);
+int kvm_get_apic_state(DeviceState *d);
 
 struct kvm_guest_debug;
 struct kvm_debug_exit_arch;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 3aa62b2..092d4f1 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1384,16 +1384,9 @@ static int kvm_get_mp_state(CPUX86State *env)
 static int kvm_get_apic(CPUX86State *env)
 {
     DeviceState *apic = env->apic_state;
-    struct kvm_lapic_state kapic;
-    int ret;
 
     if (apic && kvm_irqchip_in_kernel()) {
-        ret = kvm_vcpu_ioctl(env, KVM_GET_LAPIC, &kapic);
-        if (ret < 0) {
-            return ret;
-        }
-
-        kvm_get_apic_state(apic, &kapic);
+        return kvm_get_apic_state(apic);
     }
     return 0;
 }
@@ -1401,12 +1394,9 @@ static int kvm_get_apic(CPUX86State *env)
 static int kvm_put_apic(CPUX86State *env)
 {
     DeviceState *apic = env->apic_state;
-    struct kvm_lapic_state kapic;
 
     if (apic && kvm_irqchip_in_kernel()) {
-        kvm_put_apic_state(apic, &kapic);
-
-        return kvm_vcpu_ioctl(env, KVM_SET_LAPIC, &kapic);
+        return kvm_put_apic_state(apic);
     }
     return 0;
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/3] kvm: move KVM_GET_LAPIC/KVM_SET_LAPIC to hw/kvm/apic.c

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 7114 bytes --]

On 2012-10-30 13:16, Paolo Bonzini wrote:
> Leave knowledge of the KVM in-kernel LAPIC ioctls to hw/kvm/apic.c.
> The CPU doesn't need to know anything about kvm_lapic_state.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/kvm/apic.c     |   76 ++++++++++++++++++++++++++++++-----------------------
>  kvm.h             |    4 +-
>  target-i386/kvm.c |   14 +--------
>  3 files changed, 47 insertions(+), 47 deletions(-)
> 
> diff --git a/hw/kvm/apic.c b/hw/kvm/apic.c
> index dbac7ff..ddf6b7d 100644
> --- a/hw/kvm/apic.c
> +++ b/hw/kvm/apic.c
> @@ -25,62 +25,72 @@ static inline uint32_t kvm_apic_get_reg(struct kvm_lapic_state *kapic,
>      return *((uint32_t *)(kapic->regs + (reg_id << 4)));
>  }
>  
> -void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
> +int kvm_put_apic_state(DeviceState *d)
>  {
>      APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
> +    struct kvm_lapic_state kapic;
>      int i;
>  
> -    memset(kapic, 0, sizeof(*kapic));
> -    kvm_apic_set_reg(kapic, 0x2, s->id << 24);
> -    kvm_apic_set_reg(kapic, 0x8, s->tpr);
> -    kvm_apic_set_reg(kapic, 0xd, s->log_dest << 24);
> -    kvm_apic_set_reg(kapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
> -    kvm_apic_set_reg(kapic, 0xf, s->spurious_vec);
> +    memset(&kapic, 0, sizeof(kapic));
> +    kvm_apic_set_reg(&kapic, 0x2, s->id << 24);
> +    kvm_apic_set_reg(&kapic, 0x8, s->tpr);
> +    kvm_apic_set_reg(&kapic, 0xd, s->log_dest << 24);
> +    kvm_apic_set_reg(&kapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
> +    kvm_apic_set_reg(&kapic, 0xf, s->spurious_vec);
>      for (i = 0; i < 8; i++) {
> -        kvm_apic_set_reg(kapic, 0x10 + i, s->isr[i]);
> -        kvm_apic_set_reg(kapic, 0x18 + i, s->tmr[i]);
> -        kvm_apic_set_reg(kapic, 0x20 + i, s->irr[i]);
> +        kvm_apic_set_reg(&kapic, 0x10 + i, s->isr[i]);
> +        kvm_apic_set_reg(&kapic, 0x18 + i, s->tmr[i]);
> +        kvm_apic_set_reg(&kapic, 0x20 + i, s->irr[i]);
>      }
> -    kvm_apic_set_reg(kapic, 0x28, s->esr);
> -    kvm_apic_set_reg(kapic, 0x30, s->icr[0]);
> -    kvm_apic_set_reg(kapic, 0x31, s->icr[1]);
> +    kvm_apic_set_reg(&kapic, 0x28, s->esr);
> +    kvm_apic_set_reg(&kapic, 0x30, s->icr[0]);
> +    kvm_apic_set_reg(&kapic, 0x31, s->icr[1]);
>      for (i = 0; i < APIC_LVT_NB; i++) {
> -        kvm_apic_set_reg(kapic, 0x32 + i, s->lvt[i]);
> +        kvm_apic_set_reg(&kapic, 0x32 + i, s->lvt[i]);
>      }
> -    kvm_apic_set_reg(kapic, 0x38, s->initial_count);
> -    kvm_apic_set_reg(kapic, 0x3e, s->divide_conf);
> +    kvm_apic_set_reg(&kapic, 0x38, s->initial_count);
> +    kvm_apic_set_reg(&kapic, 0x3e, s->divide_conf);
> +
> +    return kvm_vcpu_ioctl(s->cpu_env, KVM_SET_LAPIC, &kapic);
>  }
>  
> -void kvm_get_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
> +int kvm_get_apic_state(DeviceState *d)
>  {
>      APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
> -    int i, v;
> -
> -    s->id = kvm_apic_get_reg(kapic, 0x2) >> 24;
> -    s->tpr = kvm_apic_get_reg(kapic, 0x8);
> -    s->arb_id = kvm_apic_get_reg(kapic, 0x9);
> -    s->log_dest = kvm_apic_get_reg(kapic, 0xd) >> 24;
> -    s->dest_mode = kvm_apic_get_reg(kapic, 0xe) >> 28;
> -    s->spurious_vec = kvm_apic_get_reg(kapic, 0xf);
> +    struct kvm_lapic_state kapic;
> +    int i, v, ret;
> +
> +    ret = kvm_vcpu_ioctl(s->cpu_env, KVM_GET_LAPIC, &kapic);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    s->id = kvm_apic_get_reg(&kapic, 0x2) >> 24;
> +    s->tpr = kvm_apic_get_reg(&kapic, 0x8);
> +    s->arb_id = kvm_apic_get_reg(&kapic, 0x9);
> +    s->log_dest = kvm_apic_get_reg(&kapic, 0xd) >> 24;
> +    s->dest_mode = kvm_apic_get_reg(&kapic, 0xe) >> 28;
> +    s->spurious_vec = kvm_apic_get_reg(&kapic, 0xf);
>      for (i = 0; i < 8; i++) {
> -        s->isr[i] = kvm_apic_get_reg(kapic, 0x10 + i);
> -        s->tmr[i] = kvm_apic_get_reg(kapic, 0x18 + i);
> -        s->irr[i] = kvm_apic_get_reg(kapic, 0x20 + i);
> +        s->isr[i] = kvm_apic_get_reg(&kapic, 0x10 + i);
> +        s->tmr[i] = kvm_apic_get_reg(&kapic, 0x18 + i);
> +        s->irr[i] = kvm_apic_get_reg(&kapic, 0x20 + i);
>      }
> -    s->esr = kvm_apic_get_reg(kapic, 0x28);
> -    s->icr[0] = kvm_apic_get_reg(kapic, 0x30);
> -    s->icr[1] = kvm_apic_get_reg(kapic, 0x31);
> +    s->esr = kvm_apic_get_reg(&kapic, 0x28);
> +    s->icr[0] = kvm_apic_get_reg(&kapic, 0x30);
> +    s->icr[1] = kvm_apic_get_reg(&kapic, 0x31);
>      for (i = 0; i < APIC_LVT_NB; i++) {
> -        s->lvt[i] = kvm_apic_get_reg(kapic, 0x32 + i);
> +        s->lvt[i] = kvm_apic_get_reg(&kapic, 0x32 + i);
>      }
> -    s->initial_count = kvm_apic_get_reg(kapic, 0x38);
> -    s->divide_conf = kvm_apic_get_reg(kapic, 0x3e);
> +    s->initial_count = kvm_apic_get_reg(&kapic, 0x38);
> +    s->divide_conf = kvm_apic_get_reg(&kapic, 0x3e);
>  
>      v = (s->divide_conf & 3) | ((s->divide_conf >> 1) & 4);
>      s->count_shift = (v + 1) & 7;
>  
>      s->initial_count_load_time = qemu_get_clock_ns(vm_clock);
>      apic_next_timer(s, s->initial_count_load_time);
> +    return 0;
>  }
>  
>  static void kvm_apic_set_base(APICCommonState *s, uint64_t val)
> diff --git a/kvm.h b/kvm.h
> index 2b26dcb..0056f92 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -191,8 +191,8 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>  
>  void kvm_irqchip_add_irq_route(KVMState *s, int gsi, int irqchip, int pin);
>  
> -void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic);
> -void kvm_get_apic_state(DeviceState *d, struct kvm_lapic_state *kapic);
> +int kvm_put_apic_state(DeviceState *d);
> +int kvm_get_apic_state(DeviceState *d);
>  
>  struct kvm_guest_debug;
>  struct kvm_debug_exit_arch;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 3aa62b2..092d4f1 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -1384,16 +1384,9 @@ static int kvm_get_mp_state(CPUX86State *env)
>  static int kvm_get_apic(CPUX86State *env)
>  {
>      DeviceState *apic = env->apic_state;
> -    struct kvm_lapic_state kapic;
> -    int ret;
>  
>      if (apic && kvm_irqchip_in_kernel()) {
> -        ret = kvm_vcpu_ioctl(env, KVM_GET_LAPIC, &kapic);
> -        if (ret < 0) {
> -            return ret;
> -        }
> -
> -        kvm_get_apic_state(apic, &kapic);
> +        return kvm_get_apic_state(apic);
>      }
>      return 0;
>  }
> @@ -1401,12 +1394,9 @@ static int kvm_get_apic(CPUX86State *env)
>  static int kvm_put_apic(CPUX86State *env)
>  {
>      DeviceState *apic = env->apic_state;
> -    struct kvm_lapic_state kapic;
>  
>      if (apic && kvm_irqchip_in_kernel()) {
> -        kvm_put_apic_state(apic, &kapic);
> -
> -        return kvm_vcpu_ioctl(env, KVM_SET_LAPIC, &kapic);
> +        return kvm_put_apic_state(apic);
>      }
>      return 0;
>  }
> 

This refactoring is fine with me.

Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] [PATCH 2/3] apic: add get/put methods

[Paolo Bonzini]

Change the hard-coded references to kvm_get_apic_state and
kvm_put_apic_state to methods in APICCommonClass.  This makes it possible
to reuse the methods in common code that cannot assume the presence
of KVM.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/apic.h          |    2 ++
 hw/apic_common.c   |   32 ++++++++++++++++++++++++++++++++
 hw/apic_internal.h |    2 ++
 hw/kvm/apic.c      |    8 ++++----
 kvm.h              |    3 ---
 target-i386/kvm.c  |   24 ++----------------------
 6 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/hw/apic.h b/hw/apic.h
index 1d48e02..f15d100 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -12,6 +12,8 @@ void apic_deliver_nmi(DeviceState *d);
 int apic_get_interrupt(DeviceState *s);
 void apic_reset_irq_delivered(void);
 int apic_get_irq_delivered(void);
+int cpu_get_apic_state(DeviceState *d);
+int cpu_put_apic_state(DeviceState *d);
 void cpu_set_apic_base(DeviceState *s, uint64_t val);
 uint64_t cpu_get_apic_base(DeviceState *s);
 void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
diff --git a/hw/apic_common.c b/hw/apic_common.c
index d68116d..f373ba8 100644
--- a/hw/apic_common.c
+++ b/hw/apic_common.c
@@ -312,6 +312,38 @@ static int apic_init_common(SysBusDevice *dev)
     return 0;
 }
 
+int cpu_get_apic_state(DeviceState *apic)
+{
+    APICCommonState *s;
+    APICCommonClass *info;
+    if (!apic) {
+        return 0;
+    }
+
+    s = APIC_COMMON(apic);
+    info = APIC_COMMON_GET_CLASS(s);
+    if (info->get) {
+        return info->get(s);
+    }
+    return 0;
+}
+
+int cpu_put_apic_state(DeviceState *apic)
+{
+    APICCommonState *s;
+    APICCommonClass *info;
+    if (!apic) {
+        return 0;
+    }
+
+    s = APIC_COMMON(apic);
+    info = APIC_COMMON_GET_CLASS(s);
+    if (info->put) {
+        return info->put(s);
+    }
+    return 0;
+}
+
 static void apic_dispatch_pre_save(void *opaque)
 {
     APICCommonState *s = APIC_COMMON(opaque);
diff --git a/hw/apic_internal.h b/hw/apic_internal.h
index 30932a3..256fb1a 100644
--- a/hw/apic_internal.h
+++ b/hw/apic_internal.h
@@ -89,6 +89,8 @@ typedef struct APICCommonClass
     void (*enable_tpr_reporting)(APICCommonState *s, bool enable);
     void (*vapic_base_update)(APICCommonState *s);
     void (*external_nmi)(APICCommonState *s);
+    int (*get)(APICCommonState *s);
+    int (*put)(APICCommonState *s);
     void (*pre_save)(APICCommonState *s);
     void (*post_load)(APICCommonState *s);
 } APICCommonClass;
diff --git a/hw/kvm/apic.c b/hw/kvm/apic.c
index ddf6b7d..35afe0c 100644
--- a/hw/kvm/apic.c
+++ b/hw/kvm/apic.c
@@ -25,9 +25,8 @@ static inline uint32_t kvm_apic_get_reg(struct kvm_lapic_state *kapic,
     return *((uint32_t *)(kapic->regs + (reg_id << 4)));
 }
 
-int kvm_put_apic_state(DeviceState *d)
+static int kvm_put_apic_state(APICCommonState *s)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
     struct kvm_lapic_state kapic;
     int i;
 
@@ -54,9 +53,8 @@ int kvm_put_apic_state(DeviceState *d)
     return kvm_vcpu_ioctl(s->cpu_env, KVM_SET_LAPIC, &kapic);
 }
 
-int kvm_get_apic_state(DeviceState *d)
+static int kvm_get_apic_state(APICCommonState *s)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
     struct kvm_lapic_state kapic;
     int i, v, ret;
 
@@ -202,6 +200,8 @@ static void kvm_apic_class_init(ObjectClass *klass, void *data)
     k->enable_tpr_reporting = kvm_apic_enable_tpr_reporting;
     k->vapic_base_update = kvm_apic_vapic_base_update;
     k->external_nmi = kvm_apic_external_nmi;
+    k->get = kvm_get_apic_state;
+    k->put = kvm_put_apic_state;
 }
 
 static TypeInfo kvm_apic_info = {
diff --git a/kvm.h b/kvm.h
index 0056f92..83f7b05 100644
--- a/kvm.h
+++ b/kvm.h
@@ -191,9 +191,6 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
 
 void kvm_irqchip_add_irq_route(KVMState *s, int gsi, int irqchip, int pin);
 
-int kvm_put_apic_state(DeviceState *d);
-int kvm_get_apic_state(DeviceState *d);
-
 struct kvm_guest_debug;
 struct kvm_debug_exit_arch;
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 092d4f1..0912e15 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1381,26 +1381,6 @@ static int kvm_get_mp_state(CPUX86State *env)
     return 0;
 }
 
-static int kvm_get_apic(CPUX86State *env)
-{
-    DeviceState *apic = env->apic_state;
-
-    if (apic && kvm_irqchip_in_kernel()) {
-        return kvm_get_apic_state(apic);
-    }
-    return 0;
-}
-
-static int kvm_put_apic(CPUX86State *env)
-{
-    DeviceState *apic = env->apic_state;
-
-    if (apic && kvm_irqchip_in_kernel()) {
-        return kvm_put_apic_state(apic);
-    }
-    return 0;
-}
-
 static int kvm_put_vcpu_events(CPUX86State *env, int level)
 {
     struct kvm_vcpu_events events;
@@ -1576,7 +1556,7 @@ int kvm_arch_put_registers(CPUX86State *env, int level)
         if (ret < 0) {
             return ret;
         }
-        ret = kvm_put_apic(env);
+        ret = cpu_put_apic_state(env->apic_state);
         if (ret < 0) {
             return ret;
         }
@@ -1627,7 +1607,7 @@ int kvm_arch_get_registers(CPUX86State *env)
     if (ret < 0) {
         return ret;
     }
-    ret = kvm_get_apic(env);
+    ret = cpu_get_apic_state(env->apic_state);
     if (ret < 0) {
         return ret;
     }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/3] apic: add get/put methods

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 6110 bytes --]

On 2012-10-30 13:16, Paolo Bonzini wrote:
> Change the hard-coded references to kvm_get_apic_state and
> kvm_put_apic_state to methods in APICCommonClass.  This makes it possible
> to reuse the methods in common code that cannot assume the presence
> of KVM.

The effect of patch 3 can be achieved using existing callbacks, and I
fail to see how this is nicer (the concept of "get" and "put" device
model states is not really generic). So I would skip this refactoring.

Jan

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/apic.h          |    2 ++
>  hw/apic_common.c   |   32 ++++++++++++++++++++++++++++++++
>  hw/apic_internal.h |    2 ++
>  hw/kvm/apic.c      |    8 ++++----
>  kvm.h              |    3 ---
>  target-i386/kvm.c  |   24 ++----------------------
>  6 files changed, 42 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/apic.h b/hw/apic.h
> index 1d48e02..f15d100 100644
> --- a/hw/apic.h
> +++ b/hw/apic.h
> @@ -12,6 +12,8 @@ void apic_deliver_nmi(DeviceState *d);
>  int apic_get_interrupt(DeviceState *s);
>  void apic_reset_irq_delivered(void);
>  int apic_get_irq_delivered(void);
> +int cpu_get_apic_state(DeviceState *d);
> +int cpu_put_apic_state(DeviceState *d);
>  void cpu_set_apic_base(DeviceState *s, uint64_t val);
>  uint64_t cpu_get_apic_base(DeviceState *s);
>  void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
> diff --git a/hw/apic_common.c b/hw/apic_common.c
> index d68116d..f373ba8 100644
> --- a/hw/apic_common.c
> +++ b/hw/apic_common.c
> @@ -312,6 +312,38 @@ static int apic_init_common(SysBusDevice *dev)
>      return 0;
>  }
>  
> +int cpu_get_apic_state(DeviceState *apic)
> +{
> +    APICCommonState *s;
> +    APICCommonClass *info;
> +    if (!apic) {
> +        return 0;
> +    }
> +
> +    s = APIC_COMMON(apic);
> +    info = APIC_COMMON_GET_CLASS(s);
> +    if (info->get) {
> +        return info->get(s);
> +    }
> +    return 0;
> +}
> +
> +int cpu_put_apic_state(DeviceState *apic)
> +{
> +    APICCommonState *s;
> +    APICCommonClass *info;
> +    if (!apic) {
> +        return 0;
> +    }
> +
> +    s = APIC_COMMON(apic);
> +    info = APIC_COMMON_GET_CLASS(s);
> +    if (info->put) {
> +        return info->put(s);
> +    }
> +    return 0;
> +}
> +
>  static void apic_dispatch_pre_save(void *opaque)
>  {
>      APICCommonState *s = APIC_COMMON(opaque);
> diff --git a/hw/apic_internal.h b/hw/apic_internal.h
> index 30932a3..256fb1a 100644
> --- a/hw/apic_internal.h
> +++ b/hw/apic_internal.h
> @@ -89,6 +89,8 @@ typedef struct APICCommonClass
>      void (*enable_tpr_reporting)(APICCommonState *s, bool enable);
>      void (*vapic_base_update)(APICCommonState *s);
>      void (*external_nmi)(APICCommonState *s);
> +    int (*get)(APICCommonState *s);
> +    int (*put)(APICCommonState *s);
>      void (*pre_save)(APICCommonState *s);
>      void (*post_load)(APICCommonState *s);
>  } APICCommonClass;
> diff --git a/hw/kvm/apic.c b/hw/kvm/apic.c
> index ddf6b7d..35afe0c 100644
> --- a/hw/kvm/apic.c
> +++ b/hw/kvm/apic.c
> @@ -25,9 +25,8 @@ static inline uint32_t kvm_apic_get_reg(struct kvm_lapic_state *kapic,
>      return *((uint32_t *)(kapic->regs + (reg_id << 4)));
>  }
>  
> -int kvm_put_apic_state(DeviceState *d)
> +static int kvm_put_apic_state(APICCommonState *s)
>  {
> -    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
>      struct kvm_lapic_state kapic;
>      int i;
>  
> @@ -54,9 +53,8 @@ int kvm_put_apic_state(DeviceState *d)
>      return kvm_vcpu_ioctl(s->cpu_env, KVM_SET_LAPIC, &kapic);
>  }
>  
> -int kvm_get_apic_state(DeviceState *d)
> +static int kvm_get_apic_state(APICCommonState *s)
>  {
> -    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
>      struct kvm_lapic_state kapic;
>      int i, v, ret;
>  
> @@ -202,6 +200,8 @@ static void kvm_apic_class_init(ObjectClass *klass, void *data)
>      k->enable_tpr_reporting = kvm_apic_enable_tpr_reporting;
>      k->vapic_base_update = kvm_apic_vapic_base_update;
>      k->external_nmi = kvm_apic_external_nmi;
> +    k->get = kvm_get_apic_state;
> +    k->put = kvm_put_apic_state;
>  }
>  
>  static TypeInfo kvm_apic_info = {
> diff --git a/kvm.h b/kvm.h
> index 0056f92..83f7b05 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -191,9 +191,6 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>  
>  void kvm_irqchip_add_irq_route(KVMState *s, int gsi, int irqchip, int pin);
>  
> -int kvm_put_apic_state(DeviceState *d);
> -int kvm_get_apic_state(DeviceState *d);
> -
>  struct kvm_guest_debug;
>  struct kvm_debug_exit_arch;
>  
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 092d4f1..0912e15 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -1381,26 +1381,6 @@ static int kvm_get_mp_state(CPUX86State *env)
>      return 0;
>  }
>  
> -static int kvm_get_apic(CPUX86State *env)
> -{
> -    DeviceState *apic = env->apic_state;
> -
> -    if (apic && kvm_irqchip_in_kernel()) {
> -        return kvm_get_apic_state(apic);
> -    }
> -    return 0;
> -}
> -
> -static int kvm_put_apic(CPUX86State *env)
> -{
> -    DeviceState *apic = env->apic_state;
> -
> -    if (apic && kvm_irqchip_in_kernel()) {
> -        return kvm_put_apic_state(apic);
> -    }
> -    return 0;
> -}
> -
>  static int kvm_put_vcpu_events(CPUX86State *env, int level)
>  {
>      struct kvm_vcpu_events events;
> @@ -1576,7 +1556,7 @@ int kvm_arch_put_registers(CPUX86State *env, int level)
>          if (ret < 0) {
>              return ret;
>          }
> -        ret = kvm_put_apic(env);
> +        ret = cpu_put_apic_state(env->apic_state);
>          if (ret < 0) {
>              return ret;
>          }
> @@ -1627,7 +1607,7 @@ int kvm_arch_get_registers(CPUX86State *env)
>      if (ret < 0) {
>          return ret;
>      }
> -    ret = kvm_get_apic(env);
> +    ret = cpu_get_apic_state(env->apic_state);
>      if (ret < 0) {
>          return ret;
>      }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] [PATCH 3/3] apic: always update the in-kernel status after loading

[Paolo Bonzini]

The LAPIC is loaded separately from the rest of the VCPU state.  Thus,
when restoring the CPU the dummy post-reset state is passed to the
in-kernel APIC.

This can cause MSI injection to fail if attempted during the restore of
another device, because the LAPIC believes it's not enabled.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/apic_common.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/apic_common.c b/hw/apic_common.c
index f373ba8..1ef52b2 100644
--- a/hw/apic_common.c
+++ b/hw/apic_common.c
@@ -362,6 +362,7 @@ static int apic_dispatch_post_load(void *opaque, int version_id)
     if (info->post_load) {
         info->post_load(s);
     }
+    cpu_put_apic_state(DEVICE(s));
     return 0;
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/3] apic: always update the in-kernel status after loading

[Avi Kivity]

On 10/30/2012 02:16 PM, Paolo Bonzini wrote:
> The LAPIC is loaded separately from the rest of the VCPU state.  Thus,
> when restoring the CPU the dummy post-reset state is passed to the
> in-kernel APIC.
> 
> This can cause MSI injection to fail if attempted during the restore of
> another device, because the LAPIC believes it's not enabled.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/apic_common.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/apic_common.c b/hw/apic_common.c
> index f373ba8..1ef52b2 100644
> --- a/hw/apic_common.c
> +++ b/hw/apic_common.c
> @@ -362,6 +362,7 @@ static int apic_dispatch_post_load(void *opaque, int version_id)
>      if (info->post_load) {
>          info->post_load(s);
>      }
> +    cpu_put_apic_state(DEVICE(s));
>      return 0;
>  }

Aren't we still dependent on the order of processing?  If the APIC is
restored after the device, won't we get the same problem?


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 3/3] apic: always update the in-kernel status after loading

[Paolo Bonzini]

Il 30/10/2012 13:38, Avi Kivity ha scritto:
> On 10/30/2012 02:16 PM, Paolo Bonzini wrote:
>> The LAPIC is loaded separately from the rest of the VCPU state.  Thus,
>> when restoring the CPU the dummy post-reset state is passed to the
>> in-kernel APIC.
>>
>> This can cause MSI injection to fail if attempted during the restore of
>> another device, because the LAPIC believes it's not enabled.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/apic_common.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/apic_common.c b/hw/apic_common.c
>> index f373ba8..1ef52b2 100644
>> --- a/hw/apic_common.c
>> +++ b/hw/apic_common.c
>> @@ -362,6 +362,7 @@ static int apic_dispatch_post_load(void *opaque, int version_id)
>>      if (info->post_load) {
>>          info->post_load(s);
>>      }
>> +    cpu_put_apic_state(DEVICE(s));
>>      return 0;
>>  }
> 
> Aren't we still dependent on the order of processing?  If the APIC is
> restored after the device, won't we get the same problem?

Strictly speaking yes, but CPUs and APICs are always the first devices
to be saved.

Paolo

Re: [Qemu-devel] [PATCH 3/3] apic: always update the in-kernel status after loading

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]

On 2012-10-30 15:16, Paolo Bonzini wrote:
> Il 30/10/2012 13:38, Avi Kivity ha scritto:
>> On 10/30/2012 02:16 PM, Paolo Bonzini wrote:
>>> The LAPIC is loaded separately from the rest of the VCPU state.  Thus,
>>> when restoring the CPU the dummy post-reset state is passed to the
>>> in-kernel APIC.
>>>
>>> This can cause MSI injection to fail if attempted during the restore of
>>> another device, because the LAPIC believes it's not enabled.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  hw/apic_common.c |    1 +
>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/apic_common.c b/hw/apic_common.c
>>> index f373ba8..1ef52b2 100644
>>> --- a/hw/apic_common.c
>>> +++ b/hw/apic_common.c
>>> @@ -362,6 +362,7 @@ static int apic_dispatch_post_load(void *opaque, int version_id)
>>>      if (info->post_load) {
>>>          info->post_load(s);
>>>      }
>>> +    cpu_put_apic_state(DEVICE(s));
>>>      return 0;
>>>  }
>>
>> Aren't we still dependent on the order of processing?  If the APIC is
>> restored after the device, won't we get the same problem?
> 
> Strictly speaking yes, but CPUs and APICs are always the first devices
> to be saved.

Hmm, thinking about this again: Why is the MSI event injected at all
during restore, specifically while the device models are in transitional
state. Can you explain this? Does the same pattern then also apply on
INTx injection?

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] [PATCH 3/3] apic: always update the in-kernel status after loading

[Paolo Bonzini]

Il 30/10/2012 19:21, Jan Kiszka ha scritto:
> > > Aren't we still dependent on the order of processing?  If the APIC is
> > > restored after the device, won't we get the same problem?
> > 
> > Strictly speaking yes, but CPUs and APICs are always the first devices
> > to be saved.
> Hmm, thinking about this again: Why is the MSI event injected at all
> during restore, specifically while the device models are in transitional
> state. Can you explain this?

Because the (virtio-serial) port was connected on the source and
disconnected on the destination, or vice versa.

In my simplified reproducer, I'm really using different command-lines on
the source and destination, but it is not necessary.  For example, if
you have a socket backend, the destination will usually be disconnected
at the time the machine loads.

One alternative fix is a vm_clock timer that expires immediately.  It
would fix both MSI and INTx, on the other hand I thought it was an APIC
bug because the QEMU APIC works nicely.

> Does the same pattern then also apply on INTx injection?

Yes.

Paolo

Re: [Qemu-devel] [PATCH 3/3] apic: always update the in-kernel status after loading

[Jan Kiszka]

On 2012-11-02 15:53, Paolo Bonzini wrote:
> Il 30/10/2012 19:21, Jan Kiszka ha scritto:
>>>> Aren't we still dependent on the order of processing?  If the APIC is
>>>> restored after the device, won't we get the same problem?
>>>
>>> Strictly speaking yes, but CPUs and APICs are always the first devices
>>> to be saved.
>> Hmm, thinking about this again: Why is the MSI event injected at all
>> during restore, specifically while the device models are in transitional
>> state. Can you explain this?
> 
> Because the (virtio-serial) port was connected on the source and
> disconnected on the destination, or vice versa.
> 
> In my simplified reproducer, I'm really using different command-lines on
> the source and destination, but it is not necessary.  For example, if
> you have a socket backend, the destination will usually be disconnected
> at the time the machine loads.
> 
> One alternative fix is a vm_clock timer that expires immediately.  It
> would fix both MSI and INTx, on the other hand I thought it was an APIC
> bug because the QEMU APIC works nicely.

I think deferring IRQ events to the point when the complete vmstate is
loaded is the cleaner and more robust approach.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 3/3] apic: always update the in-kernel status after loading

[Gerd Hoffmann]

  Hi,

> I think deferring IRQ events to the point when the complete vmstate is
> loaded is the cleaner and more robust approach.

Agree.  Just schedule a bh in post_load.
See also a229c0535bd336efaec786dd6e352a54e0a8187d

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/3] apic: always update the in-kernel status after loading

[Paolo Bonzini]

> Hi,
> 
> > I think deferring IRQ events to the point when the complete vmstate
> > is
> > loaded is the cleaner and more robust approach.
> 
> Agree.  Just schedule a bh in post_load.
> See also a229c0535bd336efaec786dd6e352a54e0a8187d

No, it cannot a bh.  Right now incoming migration is blocking,
but this will change in 1.3.  There is no guarantee that a
bottom half will run after migration has completed.

Paolo

Re: [Qemu-devel] [PATCH 3/3] apic: always update the in-kernel status after loading

[Gerd Hoffmann]

On 11/02/12 16:13, Paolo Bonzini wrote:
>> Hi,
>>
>>> I think deferring IRQ events to the point when the complete vmstate
>>> is
>>> loaded is the cleaner and more robust approach.
>>
>> Agree.  Just schedule a bh in post_load.
>> See also a229c0535bd336efaec786dd6e352a54e0a8187d
> 
> No, it cannot a bh.  Right now incoming migration is blocking,
> but this will change in 1.3.  There is no guarantee that a
> bottom half will run after migration has completed.

Then we'll need some new way to do this, maybe a new post_load handler
which is called once _all_ state is loaded.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/3] apic: always update the in-kernel status after loading

[Paolo Bonzini]

Il 02/11/2012 16:17, Gerd Hoffmann ha scritto:
> On 11/02/12 16:13, Paolo Bonzini wrote:
>>> >> Hi,
>>> >>
>>>> >>> I think deferring IRQ events to the point when the complete vmstate
>>>> >>> is loaded is the cleaner and more robust approach.
>>> >>
>>> >> Agree.  Just schedule a bh in post_load.
>>> >> See also a229c0535bd336efaec786dd6e352a54e0a8187d
>> > 
>> > No, it cannot a bh.  Right now incoming migration is blocking,
>> > but this will change in 1.3.  There is no guarantee that a
>> > bottom half will run after migration has completed.
> Then we'll need some new way to do this, maybe a new post_load handler
> which is called once _all_ state is loaded.

The simplest is a vm_clock timer that expires at time 0.

Paolo

Re: [Qemu-devel] [PATCH 3/3] apic: always update the in-kernel status after loading

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

On 2012-10-30 13:16, Paolo Bonzini wrote:
> The LAPIC is loaded separately from the rest of the VCPU state.  Thus,
> when restoring the CPU the dummy post-reset state is passed to the
> in-kernel APIC.
> 
> This can cause MSI injection to fail if attempted during the restore of
> another device, because the LAPIC believes it's not enabled.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/apic_common.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/apic_common.c b/hw/apic_common.c
> index f373ba8..1ef52b2 100644
> --- a/hw/apic_common.c
> +++ b/hw/apic_common.c
> @@ -362,6 +362,7 @@ static int apic_dispatch_post_load(void *opaque, int version_id)
>      if (info->post_load) {
>          info->post_load(s);
>      }
> +    cpu_put_apic_state(DEVICE(s));

Just implement a post_load handler for the KVM APIC and trigger putting
from there.

Jan

>      return 0;
>  }
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] [PATCH 4/3] ioapic: change pre_save/post_load methods to get/put

[Paolo Bonzini]

Similar to the APIC, add get/put methods that can be called from common
IOAPIC code.  Use them already for pre_save/post_load, since they are
exact replacements.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ioapic_common.c   |   40 +++++++++++++++++++++++++---------------
 hw/ioapic_internal.h |    4 ++--
 hw/kvm/ioapic.c      |    4 ++--
 3 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/hw/ioapic_common.c b/hw/ioapic_common.c
index 653eef2..1f3ea37 100644
--- a/hw/ioapic_common.c
+++ b/hw/ioapic_common.c
@@ -23,7 +23,25 @@
 #include "ioapic_internal.h"
 #include "sysbus.h"
 
-void ioapic_reset_common(DeviceState *dev)
+static void ioapic_get(IOAPICCommonState *s)
+{
+    IOAPICCommonClass *info = IOAPIC_COMMON_GET_CLASS(s);
+
+    if (info->get) {
+        info->get(s);
+    }
+}
+
+static void ioapic_put(IOAPICCommonState *s)
+{
+    IOAPICCommonClass *info = IOAPIC_COMMON_GET_CLASS(s);
+
+    if (info->put) {
+        info->put(s);
+    }
+}
+
+static void ioapic_reset_common(DeviceState *dev)
 {
     IOAPICCommonState *s = IOAPIC_COMMON(dev);
     int i;
@@ -36,24 +54,16 @@ void ioapic_reset_common(DeviceState *dev)
     }
 }
 
-static void ioapic_dispatch_pre_save(void *opaque)
+static void ioapic_pre_save(void *opaque)
 {
     IOAPICCommonState *s = IOAPIC_COMMON(opaque);
-    IOAPICCommonClass *info = IOAPIC_COMMON_GET_CLASS(s);
-
-    if (info->pre_save) {
-        info->pre_save(s);
-    }
+    ioapic_get(s);
 }
 
-static int ioapic_dispatch_post_load(void *opaque, int version_id)
+static int ioapic_post_load(void *opaque, int version_id)
 {
     IOAPICCommonState *s = IOAPIC_COMMON(opaque);
-    IOAPICCommonClass *info = IOAPIC_COMMON_GET_CLASS(s);
-
-    if (info->post_load) {
-        info->post_load(s);
-    }
+    ioapic_put(s);
     return 0;
 }
 
@@ -81,8 +91,8 @@ static const VMStateDescription vmstate_ioapic_common = {
     .version_id = 3,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
-    .pre_save = ioapic_dispatch_pre_save,
-    .post_load = ioapic_dispatch_post_load,
+    .pre_save = ioapic_pre_save,
+    .post_load = ioapic_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(id, IOAPICCommonState),
         VMSTATE_UINT8(ioregsel, IOAPICCommonState),
diff --git a/hw/ioapic_internal.h b/hw/ioapic_internal.h
index e04c9f3..7311ad0 100644
--- a/hw/ioapic_internal.h
+++ b/hw/ioapic_internal.h
@@ -84,8 +84,8 @@ typedef struct IOAPICCommonState IOAPICCommonState;
 typedef struct IOAPICCommonClass {
     SysBusDeviceClass parent_class;
     void (*init)(IOAPICCommonState *s, int instance_no);
-    void (*pre_save)(IOAPICCommonState *s);
-    void (*post_load)(IOAPICCommonState *s);
+    void (*get)(IOAPICCommonState *s);
+    void (*put)(IOAPICCommonState *s);
 } IOAPICCommonClass;
 
 struct IOAPICCommonState {
diff --git a/hw/kvm/ioapic.c b/hw/kvm/ioapic.c
index 6c3b8fe..03cb36c 100644
--- a/hw/kvm/ioapic.c
+++ b/hw/kvm/ioapic.c
@@ -104,8 +104,8 @@ static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     k->init      = kvm_ioapic_init;
-    k->pre_save  = kvm_ioapic_get;
-    k->post_load = kvm_ioapic_put;
+    k->get       = kvm_ioapic_get;
+    k->put       = kvm_ioapic_put;
     dc->reset    = kvm_ioapic_reset;
     dc->props    = kvm_ioapic_properties;
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 4/3] ioapic: change pre_save/post_load methods to get/put

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3823 bytes --]

On 2012-10-30 13:16, Paolo Bonzini wrote:
> Similar to the APIC, add get/put methods that can be called from common
> IOAPIC code.  Use them already for pre_save/post_load, since they are
> exact replacements.

Also here: I don't see a benefit and prefer the current style.

Jan

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ioapic_common.c   |   40 +++++++++++++++++++++++++---------------
>  hw/ioapic_internal.h |    4 ++--
>  hw/kvm/ioapic.c      |    4 ++--
>  3 files changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/ioapic_common.c b/hw/ioapic_common.c
> index 653eef2..1f3ea37 100644
> --- a/hw/ioapic_common.c
> +++ b/hw/ioapic_common.c
> @@ -23,7 +23,25 @@
>  #include "ioapic_internal.h"
>  #include "sysbus.h"
>  
> -void ioapic_reset_common(DeviceState *dev)
> +static void ioapic_get(IOAPICCommonState *s)
> +{
> +    IOAPICCommonClass *info = IOAPIC_COMMON_GET_CLASS(s);
> +
> +    if (info->get) {
> +        info->get(s);
> +    }
> +}
> +
> +static void ioapic_put(IOAPICCommonState *s)
> +{
> +    IOAPICCommonClass *info = IOAPIC_COMMON_GET_CLASS(s);
> +
> +    if (info->put) {
> +        info->put(s);
> +    }
> +}
> +
> +static void ioapic_reset_common(DeviceState *dev)
>  {
>      IOAPICCommonState *s = IOAPIC_COMMON(dev);
>      int i;
> @@ -36,24 +54,16 @@ void ioapic_reset_common(DeviceState *dev)
>      }
>  }
>  
> -static void ioapic_dispatch_pre_save(void *opaque)
> +static void ioapic_pre_save(void *opaque)
>  {
>      IOAPICCommonState *s = IOAPIC_COMMON(opaque);
> -    IOAPICCommonClass *info = IOAPIC_COMMON_GET_CLASS(s);
> -
> -    if (info->pre_save) {
> -        info->pre_save(s);
> -    }
> +    ioapic_get(s);
>  }
>  
> -static int ioapic_dispatch_post_load(void *opaque, int version_id)
> +static int ioapic_post_load(void *opaque, int version_id)
>  {
>      IOAPICCommonState *s = IOAPIC_COMMON(opaque);
> -    IOAPICCommonClass *info = IOAPIC_COMMON_GET_CLASS(s);
> -
> -    if (info->post_load) {
> -        info->post_load(s);
> -    }
> +    ioapic_put(s);
>      return 0;
>  }
>  
> @@ -81,8 +91,8 @@ static const VMStateDescription vmstate_ioapic_common = {
>      .version_id = 3,
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
> -    .pre_save = ioapic_dispatch_pre_save,
> -    .post_load = ioapic_dispatch_post_load,
> +    .pre_save = ioapic_pre_save,
> +    .post_load = ioapic_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(id, IOAPICCommonState),
>          VMSTATE_UINT8(ioregsel, IOAPICCommonState),
> diff --git a/hw/ioapic_internal.h b/hw/ioapic_internal.h
> index e04c9f3..7311ad0 100644
> --- a/hw/ioapic_internal.h
> +++ b/hw/ioapic_internal.h
> @@ -84,8 +84,8 @@ typedef struct IOAPICCommonState IOAPICCommonState;
>  typedef struct IOAPICCommonClass {
>      SysBusDeviceClass parent_class;
>      void (*init)(IOAPICCommonState *s, int instance_no);
> -    void (*pre_save)(IOAPICCommonState *s);
> -    void (*post_load)(IOAPICCommonState *s);
> +    void (*get)(IOAPICCommonState *s);
> +    void (*put)(IOAPICCommonState *s);
>  } IOAPICCommonClass;
>  
>  struct IOAPICCommonState {
> diff --git a/hw/kvm/ioapic.c b/hw/kvm/ioapic.c
> index 6c3b8fe..03cb36c 100644
> --- a/hw/kvm/ioapic.c
> +++ b/hw/kvm/ioapic.c
> @@ -104,8 +104,8 @@ static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      k->init      = kvm_ioapic_init;
> -    k->pre_save  = kvm_ioapic_get;
> -    k->post_load = kvm_ioapic_put;
> +    k->get       = kvm_ioapic_get;
> +    k->put       = kvm_ioapic_put;
>      dc->reset    = kvm_ioapic_reset;
>      dc->props    = kvm_ioapic_properties;
>  }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] [PATCH 5/3] ioapic: unify reset callbacks

[Paolo Bonzini]

The reset callback of the in-kernel ioapic has to update the kernel
state.  This can be done for all "models" using ioapic_put.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ioapic.c          |    2 --
 hw/ioapic_common.c   |    2 ++
 hw/ioapic_internal.h |    2 --
 hw/kvm/ioapic.c      |    9 ---------
 4 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/hw/ioapic.c b/hw/ioapic.c
index 7273095..c9f5993 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -238,10 +238,8 @@ static void ioapic_init(IOAPICCommonState *s, int instance_no)
 static void ioapic_class_init(ObjectClass *klass, void *data)
 {
     IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
-    DeviceClass *dc = DEVICE_CLASS(klass);
 
     k->init = ioapic_init;
-    dc->reset = ioapic_reset_common;
 }
 
 static TypeInfo ioapic_info = {
diff --git a/hw/ioapic_common.c b/hw/ioapic_common.c
index 1f3ea37..37715ab 100644
--- a/hw/ioapic_common.c
+++ b/hw/ioapic_common.c
@@ -52,6 +52,7 @@ static void ioapic_reset_common(DeviceState *dev)
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         s->ioredtbl[i] = 1 << IOAPIC_LVT_MASKED_SHIFT;
     }
+    ioapic_put(s);
 }
 
 static void ioapic_pre_save(void *opaque)
@@ -109,6 +110,7 @@ static void ioapic_common_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     sc->init = ioapic_init_common;
+    dc->reset = ioapic_reset_common;
     dc->vmsd = &vmstate_ioapic_common;
     dc->no_user = 1;
 }
diff --git a/hw/ioapic_internal.h b/hw/ioapic_internal.h
index 7311ad0..f8861a2 100644
--- a/hw/ioapic_internal.h
+++ b/hw/ioapic_internal.h
@@ -97,6 +97,4 @@ struct IOAPICCommonState {
     uint64_t ioredtbl[IOAPIC_NUM_PINS];
 };
 
-void ioapic_reset_common(DeviceState *dev);
-
 #endif /* !QEMU_IOAPIC_INTERNAL_H */
diff --git a/hw/kvm/ioapic.c b/hw/kvm/ioapic.c
index 03cb36c..41cfa08 100644
--- a/hw/kvm/ioapic.c
+++ b/hw/kvm/ioapic.c
@@ -69,14 +69,6 @@ static void kvm_ioapic_put(IOAPICCommonState *s)
     }
 }
 
-static void kvm_ioapic_reset(DeviceState *dev)
-{
-    IOAPICCommonState *s = DO_UPCAST(IOAPICCommonState, busdev.qdev, dev);
-
-    ioapic_reset_common(dev);
-    kvm_ioapic_put(s);
-}
-
 static void kvm_ioapic_set_irq(void *opaque, int irq, int level)
 {
     KVMIOAPICState *s = opaque;
@@ -106,7 +98,6 @@ static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
     k->init      = kvm_ioapic_init;
     k->get       = kvm_ioapic_get;
     k->put       = kvm_ioapic_put;
-    dc->reset    = kvm_ioapic_reset;
     dc->props    = kvm_ioapic_properties;
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH uq/master 0/3] Fix MSI injection at load time

[Paolo Bonzini]

Il 30/10/2012 13:16, Paolo Bonzini ha scritto:
> A simplified reproducer (that doesn't hang Linux,
> but shows the message) is to start the VM without a backend for the
> virtserialport, and to resume it with a backend, for example
> 
> $ qemu-system-x86_64 -device virtio-serial-pci -device virtserialport test.img --enable-kvm -m 512
> $ qemu-system-x86_64 -device virtio-serial-pci -chardev stdio,id=vs0 -device virtserialport,chardev=vs0 test.img --enable-kvm -m 512 -incoming 'exec:cat foo.ckp'

Jan, Amit,

the same bug is also happening without MSI.  The reproducer is the same
as above, but with pci=nomsi for Linux guests.  After migration, "cat
/dev/vport0p1" will not block, and if you have a NIC on the same line as
virtio-serial it will also not work.

Do any of you have some time to look at it?

Paolo

Re: [Qemu-devel] [PATCH uq/master 0/3] Fix MSI injection at load time

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

On 2012-10-30 17:47, Paolo Bonzini wrote:
> Il 30/10/2012 13:16, Paolo Bonzini ha scritto:
>> A simplified reproducer (that doesn't hang Linux,
>> but shows the message) is to start the VM without a backend for the
>> virtserialport, and to resume it with a backend, for example
>>
>> $ qemu-system-x86_64 -device virtio-serial-pci -device virtserialport test.img --enable-kvm -m 512
>> $ qemu-system-x86_64 -device virtio-serial-pci -chardev stdio,id=vs0 -device virtserialport,chardev=vs0 test.img --enable-kvm -m 512 -incoming 'exec:cat foo.ckp'
> 
> Jan, Amit,
> 
> the same bug is also happening without MSI.  The reproducer is the same
> as above, but with pci=nomsi for Linux guests.  After migration, "cat
> /dev/vport0p1" will not block, and if you have a NIC on the same line as
> virtio-serial it will also not work.
> 
> Do any of you have some time to look at it?

Likely not the very next days. :-/

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]