[Qemu-devel] [PATCH v2 0/2] kvm: fix incorrect LAPIC reset after reboot in x2apic mode

[Qemu-devel] [PATCH v2 0/2] kvm: fix incorrect LAPIC reset after reboot in x2apic mode

[Paolo Bonzini]

Linux 4.8 introduces changes that really really require you to set
the APIC base (via either KVM_SET_SREGS or KVM_SET_MSRS) before the
KVM_SET_LAPIC ioctl, unless KVM_SET_LAPIC is immediately preceded by
KVM_GET_LAPIC.  This is because in x2apic mode KVM now moves the APIC id
from bits 31-24 of the KVM_SET_LAPIC argument to bits 7-0 of the APIC_ID
register (0x20).  If QEMU does not nudge the APIC out of x2apic at reset
time, the subsequent KVM_SET_LAPIC incorrectly shifts the APIC id right
and Linux sees (in bits 31-24) a zero id.

While QEMU has always done the right thing, the recent commit 78d6a05
("x86/lapic: Load LAPIC state at post_load", 2016-09-13) changed
that, causing a bug on newer kernels.  Fix this by setting the APIC
base in kvm_apic_put.  Another workaround could be to enable
KVM_X2APIC_API_USE_32BIT_IDS, which lets KVM not mangle the APIC ID.

Despite the "don't break userspace" mantra, I believe that this can be
considered a QEMU bug.

I checked kvmtool and it only calls KVM_SET_LAPIC after a KVM_GET_LAPIC,
which avoids the issue in the first place.

Dr. David Alan Gilbert (1):
  kvm: apic: set APIC base as part of kvm_apic_put

Paolo Bonzini (1):
  target-i386: introduce kvm_put_one_msr

v1->v2: send the right patches

 hw/i386/kvm/apic.c     |  2 ++
 target-i386/kvm.c      | 31 ++++++++++++++++++++++---------
 target-i386/kvm_i386.h |  2 ++
 3 files changed, 26 insertions(+), 9 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] target-i386: introduce kvm_put_one_msr

[Paolo Bonzini]

Avoid further code duplication in the next patch.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/kvm.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f1ad805..38609fd 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1532,6 +1532,14 @@ static void kvm_msr_entry_add(X86CPU *cpu, uint32_t index, uint64_t value)
     msrs->nmsrs++;
 }
 
+static int kvm_put_one_msr(X86CPU *cpu, int index, uint64_t value)
+{
+    kvm_msr_buf_reset(cpu);
+    kvm_msr_entry_add(cpu, index, value);
+
+    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, cpu->kvm_msr_buf);
+}
+
 static int kvm_put_tscdeadline_msr(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
@@ -1541,10 +1551,7 @@ static int kvm_put_tscdeadline_msr(X86CPU *cpu)
         return 0;
     }
 
-    kvm_msr_buf_reset(cpu);
-    kvm_msr_entry_add(cpu, MSR_IA32_TSCDEADLINE, env->tsc_deadline);
-
-    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, cpu->kvm_msr_buf);
+    ret = kvm_put_one_msr(cpu, MSR_IA32_TSCDEADLINE, env->tsc_deadline);
     if (ret < 0) {
         return ret;
     }
@@ -1567,11 +1574,8 @@ static int kvm_put_msr_feature_control(X86CPU *cpu)
         return 0;
     }
 
-    kvm_msr_buf_reset(cpu);
-    kvm_msr_entry_add(cpu, MSR_IA32_FEATURE_CONTROL,
-                      cpu->env.msr_ia32_feature_control);
-
-    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, cpu->kvm_msr_buf);
+    ret = kvm_put_one_msr(cpu, MSR_IA32_FEATURE_CONTROL,
+                          cpu->env.msr_ia32_feature_control);
     if (ret < 0) {
         return ret;
     }
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] kvm: apic: set APIC base as part of kvm_apic_put

[Paolo Bonzini]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The parsing of KVM_SET_LAPIC's input depends on the current value of the
APIC base MSR---which indeed is stored in APICCommonState---but for historical
reasons APIC base is set through KVM_SET_SREGS together with cr8 (which is
really just the APIC TPR) and the actual "special CPU registers".

APIC base must now be set before the actual LAPIC registers, so do that
in kvm_apic_put.  It will be set again to the same value with KVM_SET_SREGS,
but that's not a big issue.

This only happens since Linux 4.8, which checks for x2apic mode in
KVM_SET_LAPIC.  However it's really a QEMU bug; until the recent
commit 78d6a05 ("x86/lapic: Load LAPIC state at post_load", 2016-09-13)
QEMU was indeed setting APIC base (via KVM_SET_SREGS) before the other
LAPIC registers.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/kvm/apic.c     | 2 ++
 target-i386/kvm.c      | 9 +++++++++
 target-i386/kvm_i386.h | 2 ++
 3 files changed, 13 insertions(+)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index feb0002..f57fed1 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -15,6 +15,7 @@
 #include "hw/i386/apic_internal.h"
 #include "hw/pci/msi.h"
 #include "sysemu/kvm.h"
+#include "target-i386/kvm_i386.h"
 
 static inline void kvm_apic_set_reg(struct kvm_lapic_state *kapic,
                                     int reg_id, uint32_t val)
@@ -130,6 +131,7 @@ static void kvm_apic_put(void *data)
     struct kvm_lapic_state kapic;
     int ret;
 
+    kvm_put_apicbase(s->cpu, s->apicbase);
     kvm_put_apic_state(s, &kapic);
 
     ret = kvm_vcpu_ioctl(CPU(s->cpu), KVM_SET_LAPIC, &kapic);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 38609fd..59242e4 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1542,6 +1542,14 @@ static int kvm_put_one_msr(X86CPU *cpu, int index, uint64_t value)
     return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, cpu->kvm_msr_buf);
 }
 
+void kvm_put_apicbase(X86CPU *cpu, uint64_t value)
+{
+    int ret;
+
+    ret = kvm_put_one_msr(cpu, MSR_IA32_APICBASE, value);
+    assert(ret == 1);
+}
+
 static int kvm_put_tscdeadline_msr(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
index 42b00af..36407e0 100644
--- a/target-i386/kvm_i386.h
+++ b/target-i386/kvm_i386.h
@@ -41,4 +41,6 @@ int kvm_device_msix_set_vector(KVMState *s, uint32_t dev_id, uint32_t vector,
 int kvm_device_msix_assign(KVMState *s, uint32_t dev_id);
 int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
 
+void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
+
 #endif
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 0/2] kvm: fix incorrect LAPIC reset after reboot in x2apic mode

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Linux 4.8 introduces changes that really really require you to set
> the APIC base (via either KVM_SET_SREGS or KVM_SET_MSRS) before the
> KVM_SET_LAPIC ioctl, unless KVM_SET_LAPIC is immediately preceded by
> KVM_GET_LAPIC.  This is because in x2apic mode KVM now moves the APIC id
> from bits 31-24 of the KVM_SET_LAPIC argument to bits 7-0 of the APIC_ID
> register (0x20).  If QEMU does not nudge the APIC out of x2apic at reset
> time, the subsequent KVM_SET_LAPIC incorrectly shifts the APIC id right
> and Linux sees (in bits 31-24) a zero id.
> 
> While QEMU has always done the right thing, the recent commit 78d6a05
> ("x86/lapic: Load LAPIC state at post_load", 2016-09-13) changed
> that, causing a bug on newer kernels.  Fix this by setting the APIC
> base in kvm_apic_put.  Another workaround could be to enable
> KVM_X2APIC_API_USE_32BIT_IDS, which lets KVM not mangle the APIC ID.
> 
> Despite the "don't break userspace" mantra, I believe that this can be
> considered a QEMU bug.
> 
> I checked kvmtool and it only calls KVM_SET_LAPIC after a KVM_GET_LAPIC,
> which avoids the issue in the first place.
> 
> Dr. David Alan Gilbert (1):
>   kvm: apic: set APIC base as part of kvm_apic_put
> 
> Paolo Bonzini (1):
>   target-i386: introduce kvm_put_one_msr
> 
> v1->v2: send the right patches

Yep, that's better - that corresponds to the version you pastebin'd
earlier and it seems to work nicely.

Dave

> 
>  hw/i386/kvm/apic.c     |  2 ++
>  target-i386/kvm.c      | 31 ++++++++++++++++++++++---------
>  target-i386/kvm_i386.h |  2 ++
>  3 files changed, 26 insertions(+), 9 deletions(-)
> 
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK