[PATCH v4 0/3] Add QEMU support for Intel local MCE

[PATCH v4 0/3] Add QEMU support for Intel local MCE

[Haozhong Zhang]

Changes in v4:
 * Abort starting QEMU if lmce option is present but host does support
   LMCE. (Eduardo Habkost)
 * Remove setting MSR_IA32_FEATURE_CONTROL, which should be left to
   guest. (Radim Krčmá, Paolo Bonzini
 * Adjust error messages in mce_init(). (Boris Petkov)
 * Move adding option 'lmce' to patch 1. (Eduardo Habkost, Paolo Bonzini)
 * Adjust LMCE error message in cpu_post_load(). (Eduardo Habkost)
 * (Patch 3) Add a fw_cfg file 'etc/msr_feature_control' to advise
   bits should be set in MSR_IA32_FEATURE_CONTROL. (Paolo Bonzini)
 * Fix SOB chain in patch 1.

Changes in v3:
 * LMCE can be enabled only for non-intel guests.
 * LMCE is disabled by default and a cpu option 'lmce=on/off' is added
   to explicitly enable/disable LMCE.
 * LMCE is disabled if KVM does not support (even though 'lmce=on').
 * VM on LMCE-enabled QEMU can be only migrated to LMCE-enabled QEMU.
 * MCG_LMCE_P is not included in MCE_CAP_DEF and instead added to
   env->mcg_cap if LMCE is enabled.
 * Code style fix.

This QEMU patch series along with the corresponding KVM patch series
(sent via another email with title "[PATCH v2 0/3] Add KVM support for
Intel local MCE") enables Intel local MCE feature for guest.

Intel Local MCE (LMCE) is a feature on Intel Skylake Server CPU that
can deliver MCE to a single processor thread instead of broadcasting
to all threads, which can reduce software's load when processing MCE
on machines with a large number of processor threads.

The technical details of LMCE can be found in Intel SDM Vol 3, Chapter
"Machine-Check Architecture" (search for 'LMCE'). Basically,
 * The capability of LMCE is indicated by bit 27 (MCG_LMCE_P) of
   MSR_IA32_MCG_CAP.
 * LMCE is enabled by setting bit 20 (MSR_IA32_FEATURE_CONTROL_LMCE)
   of MSR_IA32_FEATURE_CONTROL and bit 0 (MCG_EXT_CTL_LMCE_EN) of
   MSR_IA32_MCG_EXT_CTL.
 * Software can determine if a MCE is local to the current processor
   thread by checking bit 2 (MCG_STATUS_LMCE) of MSR_IA32_MCG_STATUS.

Ashok Raj (1):
  target-i386: KVM: add basic Intel LMCE support

Haozhong Zhang (2):
  target-i386: add migration support for Intel LMCE
  i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

 hw/i386/pc.c          | 28 ++++++++++++++++++++++++++++
 target-i386/cpu.c     | 23 +++++++++++++++++++++++
 target-i386/cpu.h     | 16 ++++++++++++++++
 target-i386/kvm.c     | 35 +++++++++++++++++++++++++++++++----
 target-i386/machine.c | 25 +++++++++++++++++++++++++
 5 files changed, 123 insertions(+), 4 deletions(-)

-- 
2.9.0

[Qemu-devel] [PATCH v4 0/3] Add QEMU support for Intel local MCE

[Haozhong Zhang]

Changes in v4:
 * Abort starting QEMU if lmce option is present but host does support
   LMCE. (Eduardo Habkost)
 * Remove setting MSR_IA32_FEATURE_CONTROL, which should be left to
   guest. (Radim Krčmá, Paolo Bonzini
 * Adjust error messages in mce_init(). (Boris Petkov)
 * Move adding option 'lmce' to patch 1. (Eduardo Habkost, Paolo Bonzini)
 * Adjust LMCE error message in cpu_post_load(). (Eduardo Habkost)
 * (Patch 3) Add a fw_cfg file 'etc/msr_feature_control' to advise
   bits should be set in MSR_IA32_FEATURE_CONTROL. (Paolo Bonzini)
 * Fix SOB chain in patch 1.

Changes in v3:
 * LMCE can be enabled only for non-intel guests.
 * LMCE is disabled by default and a cpu option 'lmce=on/off' is added
   to explicitly enable/disable LMCE.
 * LMCE is disabled if KVM does not support (even though 'lmce=on').
 * VM on LMCE-enabled QEMU can be only migrated to LMCE-enabled QEMU.
 * MCG_LMCE_P is not included in MCE_CAP_DEF and instead added to
   env->mcg_cap if LMCE is enabled.
 * Code style fix.

This QEMU patch series along with the corresponding KVM patch series
(sent via another email with title "[PATCH v2 0/3] Add KVM support for
Intel local MCE") enables Intel local MCE feature for guest.

Intel Local MCE (LMCE) is a feature on Intel Skylake Server CPU that
can deliver MCE to a single processor thread instead of broadcasting
to all threads, which can reduce software's load when processing MCE
on machines with a large number of processor threads.

The technical details of LMCE can be found in Intel SDM Vol 3, Chapter
"Machine-Check Architecture" (search for 'LMCE'). Basically,
 * The capability of LMCE is indicated by bit 27 (MCG_LMCE_P) of
   MSR_IA32_MCG_CAP.
 * LMCE is enabled by setting bit 20 (MSR_IA32_FEATURE_CONTROL_LMCE)
   of MSR_IA32_FEATURE_CONTROL and bit 0 (MCG_EXT_CTL_LMCE_EN) of
   MSR_IA32_MCG_EXT_CTL.
 * Software can determine if a MCE is local to the current processor
   thread by checking bit 2 (MCG_STATUS_LMCE) of MSR_IA32_MCG_STATUS.

Ashok Raj (1):
  target-i386: KVM: add basic Intel LMCE support

Haozhong Zhang (2):
  target-i386: add migration support for Intel LMCE
  i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

 hw/i386/pc.c          | 28 ++++++++++++++++++++++++++++
 target-i386/cpu.c     | 23 +++++++++++++++++++++++
 target-i386/cpu.h     | 16 ++++++++++++++++
 target-i386/kvm.c     | 35 +++++++++++++++++++++++++++++++----
 target-i386/machine.c | 25 +++++++++++++++++++++++++
 5 files changed, 123 insertions(+), 4 deletions(-)

-- 
2.9.0

[PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Haozhong Zhang]

From: Ashok Raj <ashok.raj@intel.com>

This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
are injected to only one VCPU rather than broadcast to all VCPUs. As KVM
reports LMCE support on Intel platforms, this features is only available
on Intel platforms.

LMCE is disabled by default and can be enabled/disabled by cpu option
'lmce=on/off'.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
[Haozhong: Enable LMCE only on Intel platforms
           Disable LMCE by default and add a cpu option 'lmce'
	   Disable LMCE if missing KVM support
	   Remove MCG_LMCE_P from MCE_CAP_DEF
	   Minor code style changes]
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 target-i386/cpu.c | 23 +++++++++++++++++++++++
 target-i386/cpu.h | 12 ++++++++++++
 target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 895a386..bd35db2 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2777,15 +2777,37 @@ static void x86_cpu_machine_reset_cb(void *opaque)
 }
 #endif
 
+static bool lmce_supported(void)
+{
+    uint64_t mce_cap;
+
+    if (!kvm_enabled() ||
+        kvm_ioctl(kvm_state, KVM_X86_GET_MCE_CAP_SUPPORTED, &mce_cap) < 0) {
+        return false;
+    }
+
+    return !!(mce_cap & MCG_LMCE_P);
+}
+
 static void mce_init(X86CPU *cpu)
 {
     CPUX86State *cenv = &cpu->env;
     unsigned int bank;
+    Error *local_err = NULL;
 
     if (((cenv->cpuid_version >> 8) & 0xf) >= 6
         && (cenv->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
             (CPUID_MCE | CPUID_MCA)) {
         cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
+
+        if (cpu->enable_lmce) {
+            if (!lmce_supported()) {
+                error_setg(&local_err, "KVM unavailable or LMCE not supported");
+                error_propagate(&error_abort, local_err);
+            }
+            cenv->mcg_cap |= MCG_LMCE_P;
+        }
+
         cenv->mcg_ctl = ~(uint64_t)0;
         for (bank = 0; bank < MCE_BANKS_DEF; bank++) {
             cenv->mce_banks[bank * 4] = ~(uint64_t)0;
@@ -3206,6 +3228,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
     DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
     DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
+    DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 0426459..f0cb04f 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -292,6 +292,7 @@
 
 #define MCG_CTL_P       (1ULL<<8)   /* MCG_CAP register available */
 #define MCG_SER_P       (1ULL<<24) /* MCA recovery/new status bits */
+#define MCG_LMCE_P      (1ULL<<27) /* Local Machine Check Supported */
 
 #define MCE_CAP_DEF     (MCG_CTL_P|MCG_SER_P)
 #define MCE_BANKS_DEF   10
@@ -301,6 +302,9 @@
 #define MCG_STATUS_RIPV (1ULL<<0)   /* restart ip valid */
 #define MCG_STATUS_EIPV (1ULL<<1)   /* ip points to correct instruction */
 #define MCG_STATUS_MCIP (1ULL<<2)   /* machine check in progress */
+#define MCG_STATUS_LMCE (1ULL<<3)   /* Local MCE signaled */
+
+#define MCG_EXT_CTL_LMCE_EN (1ULL<<0) /* Local MCE enabled */
 
 #define MCI_STATUS_VAL   (1ULL<<63)  /* valid error */
 #define MCI_STATUS_OVER  (1ULL<<62)  /* previous errors lost */
@@ -343,6 +347,7 @@
 #define MSR_MCG_CAP                     0x179
 #define MSR_MCG_STATUS                  0x17a
 #define MSR_MCG_CTL                     0x17b
+#define MSR_MCG_EXT_CTL                 0x4d0
 
 #define MSR_P6_EVNTSEL0                 0x186
 
@@ -1106,6 +1111,7 @@ typedef struct CPUX86State {
 
     uint64_t mcg_cap;
     uint64_t mcg_ctl;
+    uint64_t mcg_ext_ctl;
     uint64_t mce_banks[MCE_BANKS_DEF*4];
 
     uint64_t tsc_aux;
@@ -1173,6 +1179,12 @@ struct X86CPU {
      */
     bool enable_pmu;
 
+    /* LMCE support can be enabled/disabled via cpu option 'lmce=on/off'. It is
+     * disabled by default to avoid breaking migration between QEMU with
+     * different LMCE configurations.
+     */
+    bool enable_lmce;
+
     /* in order to simplify APIC support, we leave this pointer to the
        user */
     struct DeviceState *apic_state;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index abf50e6..ea442b3 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -107,6 +107,8 @@ static int has_xsave;
 static int has_xcrs;
 static int has_pit_state2;
 
+static bool has_msr_mcg_ext_ctl;
+
 int kvm_has_pit_state2(void)
 {
     return has_pit_state2;
@@ -378,10 +380,12 @@ static int kvm_get_mce_cap_supported(KVMState *s, uint64_t *mce_cap,
 
 static void kvm_mce_inject(X86CPU *cpu, hwaddr paddr, int code)
 {
+    CPUState *cs = CPU(cpu);
     CPUX86State *env = &cpu->env;
     uint64_t status = MCI_STATUS_VAL | MCI_STATUS_UC | MCI_STATUS_EN |
                       MCI_STATUS_MISCV | MCI_STATUS_ADDRV | MCI_STATUS_S;
     uint64_t mcg_status = MCG_STATUS_MCIP;
+    int flags = 0;
 
     if (code == BUS_MCEERR_AR) {
         status |= MCI_STATUS_AR | 0x134;
@@ -390,10 +394,19 @@ static void kvm_mce_inject(X86CPU *cpu, hwaddr paddr, int code)
         status |= 0xc0;
         mcg_status |= MCG_STATUS_RIPV;
     }
+
+    flags = cpu_x86_support_mca_broadcast(env) ? MCE_INJECT_BROADCAST : 0;
+    /* We need to read back the value of MSR_EXT_MCG_CTL that was set by the
+     * guest kernel back into env->mcg_ext_ctl.
+     */
+    cpu_synchronize_state(cs);
+    if (env->mcg_ext_ctl & MCG_EXT_CTL_LMCE_EN) {
+        mcg_status |= MCG_STATUS_LMCE;
+        flags = 0;
+    }
+
     cpu_x86_inject_mce(NULL, cpu, 9, status, mcg_status, paddr,
-                       (MCM_ADDR_PHYS << 6) | 0xc,
-                       cpu_x86_support_mca_broadcast(env) ?
-                       MCE_INJECT_BROADCAST : 0);
+                       (MCM_ADDR_PHYS << 6) | 0xc, flags);
 }
 
 static void hardware_memory_error(void)
@@ -878,7 +891,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
     c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
     if (c) {
         has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
-                                  !!(c->ecx & CPUID_EXT_SMX);
+                                  !!(c->ecx & CPUID_EXT_SMX) ||
+                                  !!(env->mcg_cap & MCG_LMCE_P);
+    }
+
+    if (has_msr_feature_control && (env->mcg_cap & MCG_LMCE_P)) {
+        has_msr_mcg_ext_ctl = true;
     }
 
     c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
@@ -1702,6 +1720,9 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
 
         kvm_msr_entry_add(cpu, MSR_MCG_STATUS, env->mcg_status);
         kvm_msr_entry_add(cpu, MSR_MCG_CTL, env->mcg_ctl);
+        if (has_msr_mcg_ext_ctl) {
+            kvm_msr_entry_add(cpu, MSR_MCG_EXT_CTL, env->mcg_ext_ctl);
+        }
         for (i = 0; i < (env->mcg_cap & 0xff) * 4; i++) {
             kvm_msr_entry_add(cpu, MSR_MC0_CTL + i, env->mce_banks[i]);
         }
@@ -2005,6 +2026,9 @@ static int kvm_get_msrs(X86CPU *cpu)
     if (env->mcg_cap) {
         kvm_msr_entry_add(cpu, MSR_MCG_STATUS, 0);
         kvm_msr_entry_add(cpu, MSR_MCG_CTL, 0);
+        if (has_msr_mcg_ext_ctl) {
+            kvm_msr_entry_add(cpu, MSR_MCG_EXT_CTL, 0);
+        }
         for (i = 0; i < (env->mcg_cap & 0xff) * 4; i++) {
             kvm_msr_entry_add(cpu, MSR_MC0_CTL + i, 0);
         }
@@ -2133,6 +2157,9 @@ static int kvm_get_msrs(X86CPU *cpu)
         case MSR_MCG_CTL:
             env->mcg_ctl = msrs[i].data;
             break;
+        case MSR_MCG_EXT_CTL:
+            env->mcg_ext_ctl = msrs[i].data;
+            break;
         case MSR_IA32_MISC_ENABLE:
             env->msr_ia32_misc_enable = msrs[i].data;
             break;
-- 
2.9.0

[Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Haozhong Zhang]

From: Ashok Raj <ashok.raj@intel.com>

This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
are injected to only one VCPU rather than broadcast to all VCPUs. As KVM
reports LMCE support on Intel platforms, this features is only available
on Intel platforms.

LMCE is disabled by default and can be enabled/disabled by cpu option
'lmce=on/off'.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
[Haozhong: Enable LMCE only on Intel platforms
           Disable LMCE by default and add a cpu option 'lmce'
	   Disable LMCE if missing KVM support
	   Remove MCG_LMCE_P from MCE_CAP_DEF
	   Minor code style changes]
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 target-i386/cpu.c | 23 +++++++++++++++++++++++
 target-i386/cpu.h | 12 ++++++++++++
 target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 895a386..bd35db2 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2777,15 +2777,37 @@ static void x86_cpu_machine_reset_cb(void *opaque)
 }
 #endif
 
+static bool lmce_supported(void)
+{
+    uint64_t mce_cap;
+
+    if (!kvm_enabled() ||
+        kvm_ioctl(kvm_state, KVM_X86_GET_MCE_CAP_SUPPORTED, &mce_cap) < 0) {
+        return false;
+    }
+
+    return !!(mce_cap & MCG_LMCE_P);
+}
+
 static void mce_init(X86CPU *cpu)
 {
     CPUX86State *cenv = &cpu->env;
     unsigned int bank;
+    Error *local_err = NULL;
 
     if (((cenv->cpuid_version >> 8) & 0xf) >= 6
         && (cenv->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
             (CPUID_MCE | CPUID_MCA)) {
         cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
+
+        if (cpu->enable_lmce) {
+            if (!lmce_supported()) {
+                error_setg(&local_err, "KVM unavailable or LMCE not supported");
+                error_propagate(&error_abort, local_err);
+            }
+            cenv->mcg_cap |= MCG_LMCE_P;
+        }
+
         cenv->mcg_ctl = ~(uint64_t)0;
         for (bank = 0; bank < MCE_BANKS_DEF; bank++) {
             cenv->mce_banks[bank * 4] = ~(uint64_t)0;
@@ -3206,6 +3228,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
     DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
     DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
+    DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 0426459..f0cb04f 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -292,6 +292,7 @@
 
 #define MCG_CTL_P       (1ULL<<8)   /* MCG_CAP register available */
 #define MCG_SER_P       (1ULL<<24) /* MCA recovery/new status bits */
+#define MCG_LMCE_P      (1ULL<<27) /* Local Machine Check Supported */
 
 #define MCE_CAP_DEF     (MCG_CTL_P|MCG_SER_P)
 #define MCE_BANKS_DEF   10
@@ -301,6 +302,9 @@
 #define MCG_STATUS_RIPV (1ULL<<0)   /* restart ip valid */
 #define MCG_STATUS_EIPV (1ULL<<1)   /* ip points to correct instruction */
 #define MCG_STATUS_MCIP (1ULL<<2)   /* machine check in progress */
+#define MCG_STATUS_LMCE (1ULL<<3)   /* Local MCE signaled */
+
+#define MCG_EXT_CTL_LMCE_EN (1ULL<<0) /* Local MCE enabled */
 
 #define MCI_STATUS_VAL   (1ULL<<63)  /* valid error */
 #define MCI_STATUS_OVER  (1ULL<<62)  /* previous errors lost */
@@ -343,6 +347,7 @@
 #define MSR_MCG_CAP                     0x179
 #define MSR_MCG_STATUS                  0x17a
 #define MSR_MCG_CTL                     0x17b
+#define MSR_MCG_EXT_CTL                 0x4d0
 
 #define MSR_P6_EVNTSEL0                 0x186
 
@@ -1106,6 +1111,7 @@ typedef struct CPUX86State {
 
     uint64_t mcg_cap;
     uint64_t mcg_ctl;
+    uint64_t mcg_ext_ctl;
     uint64_t mce_banks[MCE_BANKS_DEF*4];
 
     uint64_t tsc_aux;
@@ -1173,6 +1179,12 @@ struct X86CPU {
      */
     bool enable_pmu;
 
+    /* LMCE support can be enabled/disabled via cpu option 'lmce=on/off'. It is
+     * disabled by default to avoid breaking migration between QEMU with
+     * different LMCE configurations.
+     */
+    bool enable_lmce;
+
     /* in order to simplify APIC support, we leave this pointer to the
        user */
     struct DeviceState *apic_state;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index abf50e6..ea442b3 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -107,6 +107,8 @@ static int has_xsave;
 static int has_xcrs;
 static int has_pit_state2;
 
+static bool has_msr_mcg_ext_ctl;
+
 int kvm_has_pit_state2(void)
 {
     return has_pit_state2;
@@ -378,10 +380,12 @@ static int kvm_get_mce_cap_supported(KVMState *s, uint64_t *mce_cap,
 
 static void kvm_mce_inject(X86CPU *cpu, hwaddr paddr, int code)
 {
+    CPUState *cs = CPU(cpu);
     CPUX86State *env = &cpu->env;
     uint64_t status = MCI_STATUS_VAL | MCI_STATUS_UC | MCI_STATUS_EN |
                       MCI_STATUS_MISCV | MCI_STATUS_ADDRV | MCI_STATUS_S;
     uint64_t mcg_status = MCG_STATUS_MCIP;
+    int flags = 0;
 
     if (code == BUS_MCEERR_AR) {
         status |= MCI_STATUS_AR | 0x134;
@@ -390,10 +394,19 @@ static void kvm_mce_inject(X86CPU *cpu, hwaddr paddr, int code)
         status |= 0xc0;
         mcg_status |= MCG_STATUS_RIPV;
     }
+
+    flags = cpu_x86_support_mca_broadcast(env) ? MCE_INJECT_BROADCAST : 0;
+    /* We need to read back the value of MSR_EXT_MCG_CTL that was set by the
+     * guest kernel back into env->mcg_ext_ctl.
+     */
+    cpu_synchronize_state(cs);
+    if (env->mcg_ext_ctl & MCG_EXT_CTL_LMCE_EN) {
+        mcg_status |= MCG_STATUS_LMCE;
+        flags = 0;
+    }
+
     cpu_x86_inject_mce(NULL, cpu, 9, status, mcg_status, paddr,
-                       (MCM_ADDR_PHYS << 6) | 0xc,
-                       cpu_x86_support_mca_broadcast(env) ?
-                       MCE_INJECT_BROADCAST : 0);
+                       (MCM_ADDR_PHYS << 6) | 0xc, flags);
 }
 
 static void hardware_memory_error(void)
@@ -878,7 +891,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
     c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
     if (c) {
         has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
-                                  !!(c->ecx & CPUID_EXT_SMX);
+                                  !!(c->ecx & CPUID_EXT_SMX) ||
+                                  !!(env->mcg_cap & MCG_LMCE_P);
+    }
+
+    if (has_msr_feature_control && (env->mcg_cap & MCG_LMCE_P)) {
+        has_msr_mcg_ext_ctl = true;
     }
 
     c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
@@ -1702,6 +1720,9 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
 
         kvm_msr_entry_add(cpu, MSR_MCG_STATUS, env->mcg_status);
         kvm_msr_entry_add(cpu, MSR_MCG_CTL, env->mcg_ctl);
+        if (has_msr_mcg_ext_ctl) {
+            kvm_msr_entry_add(cpu, MSR_MCG_EXT_CTL, env->mcg_ext_ctl);
+        }
         for (i = 0; i < (env->mcg_cap & 0xff) * 4; i++) {
             kvm_msr_entry_add(cpu, MSR_MC0_CTL + i, env->mce_banks[i]);
         }
@@ -2005,6 +2026,9 @@ static int kvm_get_msrs(X86CPU *cpu)
     if (env->mcg_cap) {
         kvm_msr_entry_add(cpu, MSR_MCG_STATUS, 0);
         kvm_msr_entry_add(cpu, MSR_MCG_CTL, 0);
+        if (has_msr_mcg_ext_ctl) {
+            kvm_msr_entry_add(cpu, MSR_MCG_EXT_CTL, 0);
+        }
         for (i = 0; i < (env->mcg_cap & 0xff) * 4; i++) {
             kvm_msr_entry_add(cpu, MSR_MC0_CTL + i, 0);
         }
@@ -2133,6 +2157,9 @@ static int kvm_get_msrs(X86CPU *cpu)
         case MSR_MCG_CTL:
             env->mcg_ctl = msrs[i].data;
             break;
+        case MSR_MCG_EXT_CTL:
+            env->mcg_ext_ctl = msrs[i].data;
+            break;
         case MSR_IA32_MISC_ENABLE:
             env->msr_ia32_misc_enable = msrs[i].data;
             break;
-- 
2.9.0

[PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Haozhong Zhang]

Migration is only allowed between VCPUs with the same lmce option.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 target-i386/machine.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/target-i386/machine.c b/target-i386/machine.c
index cb9adf2..00375a3 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -347,6 +347,12 @@ static int cpu_post_load(void *opaque, int version_id)
         return -EINVAL;
     }
 
+    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
+        error_report("Config mismatch: VCPU has LMCE enabled, "
+                     "but \"lmce\" option is disabled");
+        return -EINVAL;
+    }
+
     /*
      * Real mode guest segments register DPL should be zero.
      * Older KVM version were setting it wrongly.
@@ -896,6 +902,24 @@ static const VMStateDescription vmstate_tsc_khz = {
     }
 };
 
+static bool mcg_ext_ctl_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+    return cpu->enable_lmce && env->mcg_ext_ctl;
+}
+
+static const VMStateDescription vmstate_mcg_ext_ctl = {
+    .name = "cpu/mcg_ext_ctl",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = mcg_ext_ctl_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(env.mcg_ext_ctl, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
     .version_id = 12,
@@ -1022,6 +1046,7 @@ VMStateDescription vmstate_x86_cpu = {
 #ifdef TARGET_X86_64
         &vmstate_pkru,
 #endif
+        &vmstate_mcg_ext_ctl,
         NULL
     }
 };
-- 
2.9.0

[Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Haozhong Zhang]

Migration is only allowed between VCPUs with the same lmce option.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 target-i386/machine.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/target-i386/machine.c b/target-i386/machine.c
index cb9adf2..00375a3 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -347,6 +347,12 @@ static int cpu_post_load(void *opaque, int version_id)
         return -EINVAL;
     }
 
+    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
+        error_report("Config mismatch: VCPU has LMCE enabled, "
+                     "but \"lmce\" option is disabled");
+        return -EINVAL;
+    }
+
     /*
      * Real mode guest segments register DPL should be zero.
      * Older KVM version were setting it wrongly.
@@ -896,6 +902,24 @@ static const VMStateDescription vmstate_tsc_khz = {
     }
 };
 
+static bool mcg_ext_ctl_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+    return cpu->enable_lmce && env->mcg_ext_ctl;
+}
+
+static const VMStateDescription vmstate_mcg_ext_ctl = {
+    .name = "cpu/mcg_ext_ctl",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = mcg_ext_ctl_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(env.mcg_ext_ctl, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
     .version_id = 12,
@@ -1022,6 +1046,7 @@ VMStateDescription vmstate_x86_cpu = {
 #ifdef TARGET_X86_64
         &vmstate_pkru,
 #endif
+        &vmstate_mcg_ext_ctl,
         NULL
     }
 };
-- 
2.9.0

[PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Haozhong Zhang]

It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
be set before some features (e.g. VMX and LMCE) can be used, which is
usually done by the firmware. This patch adds a fw_cfg file
"etc/msr_feature_control" which contains the advised value of
MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 hw/i386/pc.c      | 28 ++++++++++++++++++++++++++++
 target-i386/cpu.h |  4 ++++
 2 files changed, 32 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7198ed5..d8178a5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1147,6 +1147,33 @@ void pc_cpus_init(PCMachineState *pcms)
     smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
 }
 
+static void pc_build_feature_control_file(PCMachineState *pcms)
+{
+    X86CPU *cpu = X86_CPU(pcms->possible_cpus->cpus[0].cpu);
+    CPUX86State *env = &cpu->env;
+    uint32_t unused, ecx, edx, feature_control_bits = 0;
+    uint32_t *val;
+
+    cpu_x86_cpuid(env, 1, 0, &unused, &unused, &ecx, &edx);
+    if (ecx & CPUID_EXT_VMX) {
+        feature_control_bits |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
+    }
+
+    if ((edx & (CPUID_EXT2_MCE | CPUID_EXT2_MCA)) ==
+        (CPUID_EXT2_MCE | CPUID_EXT2_MCA) &&
+        (env->mcg_cap & MCG_LMCE_P)) {
+        feature_control_bits |= FEATURE_CONTROL_LMCE;
+    }
+
+    if (!feature_control_bits) {
+        return;
+    }
+
+    val = g_malloc(sizeof(*val));
+    *val = feature_control_bits | FEATURE_CONTROL_LOCKED;
+    fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
+}
+
 static
 void pc_machine_done(Notifier *notifier, void *data)
 {
@@ -1174,6 +1201,7 @@ void pc_machine_done(Notifier *notifier, void *data)
     acpi_setup();
     if (pcms->fw_cfg) {
         pc_build_smbios(pcms->fw_cfg);
+        pc_build_feature_control_file(pcms);
     }
 }
 
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index f0cb04f..5e07c7a 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -332,6 +332,10 @@
 #define MSR_TSC_ADJUST                  0x0000003b
 #define MSR_IA32_TSCDEADLINE            0x6e0
 
+#define FEATURE_CONTROL_LOCKED                    (1<<0)
+#define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX (1<<2)
+#define FEATURE_CONTROL_LMCE                      (1<<20)
+
 #define MSR_P6_PERFCTR0                 0xc1
 
 #define MSR_IA32_SMBASE                 0x9e
-- 
2.9.0

[Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Haozhong Zhang]

It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
be set before some features (e.g. VMX and LMCE) can be used, which is
usually done by the firmware. This patch adds a fw_cfg file
"etc/msr_feature_control" which contains the advised value of
MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 hw/i386/pc.c      | 28 ++++++++++++++++++++++++++++
 target-i386/cpu.h |  4 ++++
 2 files changed, 32 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7198ed5..d8178a5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1147,6 +1147,33 @@ void pc_cpus_init(PCMachineState *pcms)
     smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
 }
 
+static void pc_build_feature_control_file(PCMachineState *pcms)
+{
+    X86CPU *cpu = X86_CPU(pcms->possible_cpus->cpus[0].cpu);
+    CPUX86State *env = &cpu->env;
+    uint32_t unused, ecx, edx, feature_control_bits = 0;
+    uint32_t *val;
+
+    cpu_x86_cpuid(env, 1, 0, &unused, &unused, &ecx, &edx);
+    if (ecx & CPUID_EXT_VMX) {
+        feature_control_bits |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
+    }
+
+    if ((edx & (CPUID_EXT2_MCE | CPUID_EXT2_MCA)) ==
+        (CPUID_EXT2_MCE | CPUID_EXT2_MCA) &&
+        (env->mcg_cap & MCG_LMCE_P)) {
+        feature_control_bits |= FEATURE_CONTROL_LMCE;
+    }
+
+    if (!feature_control_bits) {
+        return;
+    }
+
+    val = g_malloc(sizeof(*val));
+    *val = feature_control_bits | FEATURE_CONTROL_LOCKED;
+    fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
+}
+
 static
 void pc_machine_done(Notifier *notifier, void *data)
 {
@@ -1174,6 +1201,7 @@ void pc_machine_done(Notifier *notifier, void *data)
     acpi_setup();
     if (pcms->fw_cfg) {
         pc_build_smbios(pcms->fw_cfg);
+        pc_build_feature_control_file(pcms);
     }
 }
 
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index f0cb04f..5e07c7a 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -332,6 +332,10 @@
 #define MSR_TSC_ADJUST                  0x0000003b
 #define MSR_IA32_TSCDEADLINE            0x6e0
 
+#define FEATURE_CONTROL_LOCKED                    (1<<0)
+#define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX (1<<2)
+#define FEATURE_CONTROL_LMCE                      (1<<20)
+
 #define MSR_P6_PERFCTR0                 0xc1
 
 #define MSR_IA32_SMBASE                 0x9e
-- 
2.9.0

Re: [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Paolo Bonzini]

On 16/06/2016 08:06, Haozhong Zhang wrote:
> +            if (!lmce_supported()) {
> +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> +                error_propagate(&error_abort, local_err);
> +            }

Aborts should never be triggered by user input.  The error instead
should propagate from mce_init to its caller with a new errp argument
(i.e. error_setg(errp, "KVM unavailable or LMCE not supported")).

x86_cpu_realizefn can pass &local_err and check the outcome through
local_err != NULL.  See the existing call to x86_cpu_apic_create, right
above the call to mce_init.

> @@ -878,7 +891,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
>      if (c) {
>          has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
> -                                  !!(c->ecx & CPUID_EXT_SMX);
> +                                  !!(c->ecx & CPUID_EXT_SMX) ||
> +                                  !!(env->mcg_cap & MCG_LMCE_P);

This part is wrong; env->mcg_cap is independent from CPUID[1].ECX.

> +    }
> +
> +    if (has_msr_feature_control && (env->mcg_cap & MCG_LMCE_P)) {

Don't test has_msr_feature_control here, instead set it to true inside
the "if".

> +        has_msr_mcg_ext_ctl = true;
>      }
>  
>      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);

Which silicon has LMCE?  We may want to enable the property for some CPU
models.  Apart from this, the patch is pretty much okay.

Paolo

Re: [Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Paolo Bonzini]

On 16/06/2016 08:06, Haozhong Zhang wrote:
> +            if (!lmce_supported()) {
> +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> +                error_propagate(&error_abort, local_err);
> +            }

Aborts should never be triggered by user input.  The error instead
should propagate from mce_init to its caller with a new errp argument
(i.e. error_setg(errp, "KVM unavailable or LMCE not supported")).

x86_cpu_realizefn can pass &local_err and check the outcome through
local_err != NULL.  See the existing call to x86_cpu_apic_create, right
above the call to mce_init.

> @@ -878,7 +891,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
>      if (c) {
>          has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
> -                                  !!(c->ecx & CPUID_EXT_SMX);
> +                                  !!(c->ecx & CPUID_EXT_SMX) ||
> +                                  !!(env->mcg_cap & MCG_LMCE_P);

This part is wrong; env->mcg_cap is independent from CPUID[1].ECX.

> +    }
> +
> +    if (has_msr_feature_control && (env->mcg_cap & MCG_LMCE_P)) {

Don't test has_msr_feature_control here, instead set it to true inside
the "if".

> +        has_msr_mcg_ext_ctl = true;
>      }
>  
>      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);

Which silicon has LMCE?  We may want to enable the property for some CPU
models.  Apart from this, the patch is pretty much okay.

Paolo

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Paolo Bonzini]

On 16/06/2016 08:06, Haozhong Zhang wrote:
> Migration is only allowed between VCPUs with the same lmce option.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  target-i386/machine.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index cb9adf2..00375a3 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -347,6 +347,12 @@ static int cpu_post_load(void *opaque, int version_id)
>          return -EINVAL;
>      }
>  
> +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
> +        error_report("Config mismatch: VCPU has LMCE enabled, "
> +                     "but \"lmce\" option is disabled");
> +        return -EINVAL;
> +    }
> +

I think this is unnecessary.  Apart from this, the patch is good and can
be squashed in patch 1 for v5.

Paolo

>      /*
>       * Real mode guest segments register DPL should be zero.
>       * Older KVM version were setting it wrongly.
> @@ -896,6 +902,24 @@ static const VMStateDescription vmstate_tsc_khz = {
>      }
>  };
>  
> +static bool mcg_ext_ctl_needed(void *opaque)
> +{
> +    X86CPU *cpu = opaque;
> +    CPUX86State *env = &cpu->env;
> +    return cpu->enable_lmce && env->mcg_ext_ctl;
> +}
> +
> +static const VMStateDescription vmstate_mcg_ext_ctl = {
> +    .name = "cpu/mcg_ext_ctl",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = mcg_ext_ctl_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(env.mcg_ext_ctl, X86CPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  VMStateDescription vmstate_x86_cpu = {
>      .name = "cpu",
>      .version_id = 12,
> @@ -1022,6 +1046,7 @@ VMStateDescription vmstate_x86_cpu = {
>  #ifdef TARGET_X86_64
>          &vmstate_pkru,
>  #endif
> +        &vmstate_mcg_ext_ctl,
>          NULL
>      }
>  };
> 

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Paolo Bonzini]

On 16/06/2016 08:06, Haozhong Zhang wrote:
> Migration is only allowed between VCPUs with the same lmce option.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  target-i386/machine.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index cb9adf2..00375a3 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -347,6 +347,12 @@ static int cpu_post_load(void *opaque, int version_id)
>          return -EINVAL;
>      }
>  
> +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
> +        error_report("Config mismatch: VCPU has LMCE enabled, "
> +                     "but \"lmce\" option is disabled");
> +        return -EINVAL;
> +    }
> +

I think this is unnecessary.  Apart from this, the patch is good and can
be squashed in patch 1 for v5.

Paolo

>      /*
>       * Real mode guest segments register DPL should be zero.
>       * Older KVM version were setting it wrongly.
> @@ -896,6 +902,24 @@ static const VMStateDescription vmstate_tsc_khz = {
>      }
>  };
>  
> +static bool mcg_ext_ctl_needed(void *opaque)
> +{
> +    X86CPU *cpu = opaque;
> +    CPUX86State *env = &cpu->env;
> +    return cpu->enable_lmce && env->mcg_ext_ctl;
> +}
> +
> +static const VMStateDescription vmstate_mcg_ext_ctl = {
> +    .name = "cpu/mcg_ext_ctl",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = mcg_ext_ctl_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(env.mcg_ext_ctl, X86CPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  VMStateDescription vmstate_x86_cpu = {
>      .name = "cpu",
>      .version_id = 12,
> @@ -1022,6 +1046,7 @@ VMStateDescription vmstate_x86_cpu = {
>  #ifdef TARGET_X86_64
>          &vmstate_pkru,
>  #endif
> +        &vmstate_mcg_ext_ctl,
>          NULL
>      }
>  };
> 

Re: [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Paolo Bonzini]

On 16/06/2016 08:06, Haozhong Zhang wrote:
> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
> be set before some features (e.g. VMX and LMCE) can be used, which is
> usually done by the firmware. This patch adds a fw_cfg file
> "etc/msr_feature_control" which contains the advised value of
> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  hw/i386/pc.c      | 28 ++++++++++++++++++++++++++++
>  target-i386/cpu.h |  4 ++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7198ed5..d8178a5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1147,6 +1147,33 @@ void pc_cpus_init(PCMachineState *pcms)
>      smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
>  }
>  
> +static void pc_build_feature_control_file(PCMachineState *pcms)
> +{
> +    X86CPU *cpu = X86_CPU(pcms->possible_cpus->cpus[0].cpu);
> +    CPUX86State *env = &cpu->env;
> +    uint32_t unused, ecx, edx, feature_control_bits = 0;
> +    uint32_t *val;
> +
> +    cpu_x86_cpuid(env, 1, 0, &unused, &unused, &ecx, &edx);
> +    if (ecx & CPUID_EXT_VMX) {
> +        feature_control_bits |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
> +    }
> +
> +    if ((edx & (CPUID_EXT2_MCE | CPUID_EXT2_MCA)) ==
> +        (CPUID_EXT2_MCE | CPUID_EXT2_MCA) &&
> +        (env->mcg_cap & MCG_LMCE_P)) {
> +        feature_control_bits |= FEATURE_CONTROL_LMCE;
> +    }
> +
> +    if (!feature_control_bits) {
> +        return;
> +    }
> +
> +    val = g_malloc(sizeof(*val));
> +    *val = feature_control_bits | FEATURE_CONTROL_LOCKED;
> +    fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
> +}
> +
>  static
>  void pc_machine_done(Notifier *notifier, void *data)
>  {
> @@ -1174,6 +1201,7 @@ void pc_machine_done(Notifier *notifier, void *data)
>      acpi_setup();
>      if (pcms->fw_cfg) {
>          pc_build_smbios(pcms->fw_cfg);
> +        pc_build_feature_control_file(pcms);
>      }
>  }
>  
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index f0cb04f..5e07c7a 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -332,6 +332,10 @@
>  #define MSR_TSC_ADJUST                  0x0000003b
>  #define MSR_IA32_TSCDEADLINE            0x6e0
>  
> +#define FEATURE_CONTROL_LOCKED                    (1<<0)
> +#define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX (1<<2)
> +#define FEATURE_CONTROL_LMCE                      (1<<20)
> +
>  #define MSR_P6_PERFCTR0                 0xc1
>  
>  #define MSR_IA32_SMBASE                 0x9e
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Have you prepared a patch for SeaBIOS already?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Paolo Bonzini]

On 16/06/2016 08:06, Haozhong Zhang wrote:
> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
> be set before some features (e.g. VMX and LMCE) can be used, which is
> usually done by the firmware. This patch adds a fw_cfg file
> "etc/msr_feature_control" which contains the advised value of
> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  hw/i386/pc.c      | 28 ++++++++++++++++++++++++++++
>  target-i386/cpu.h |  4 ++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7198ed5..d8178a5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1147,6 +1147,33 @@ void pc_cpus_init(PCMachineState *pcms)
>      smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
>  }
>  
> +static void pc_build_feature_control_file(PCMachineState *pcms)
> +{
> +    X86CPU *cpu = X86_CPU(pcms->possible_cpus->cpus[0].cpu);
> +    CPUX86State *env = &cpu->env;
> +    uint32_t unused, ecx, edx, feature_control_bits = 0;
> +    uint32_t *val;
> +
> +    cpu_x86_cpuid(env, 1, 0, &unused, &unused, &ecx, &edx);
> +    if (ecx & CPUID_EXT_VMX) {
> +        feature_control_bits |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
> +    }
> +
> +    if ((edx & (CPUID_EXT2_MCE | CPUID_EXT2_MCA)) ==
> +        (CPUID_EXT2_MCE | CPUID_EXT2_MCA) &&
> +        (env->mcg_cap & MCG_LMCE_P)) {
> +        feature_control_bits |= FEATURE_CONTROL_LMCE;
> +    }
> +
> +    if (!feature_control_bits) {
> +        return;
> +    }
> +
> +    val = g_malloc(sizeof(*val));
> +    *val = feature_control_bits | FEATURE_CONTROL_LOCKED;
> +    fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
> +}
> +
>  static
>  void pc_machine_done(Notifier *notifier, void *data)
>  {
> @@ -1174,6 +1201,7 @@ void pc_machine_done(Notifier *notifier, void *data)
>      acpi_setup();
>      if (pcms->fw_cfg) {
>          pc_build_smbios(pcms->fw_cfg);
> +        pc_build_feature_control_file(pcms);
>      }
>  }
>  
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index f0cb04f..5e07c7a 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -332,6 +332,10 @@
>  #define MSR_TSC_ADJUST                  0x0000003b
>  #define MSR_IA32_TSCDEADLINE            0x6e0
>  
> +#define FEATURE_CONTROL_LOCKED                    (1<<0)
> +#define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX (1<<2)
> +#define FEATURE_CONTROL_LMCE                      (1<<20)
> +
>  #define MSR_P6_PERFCTR0                 0xc1
>  
>  #define MSR_IA32_SMBASE                 0x9e
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Have you prepared a patch for SeaBIOS already?

Thanks,

Paolo

Re: [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Haozhong Zhang]

On 06/16/16 11:50, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 08:06, Haozhong Zhang wrote:
> > +            if (!lmce_supported()) {
> > +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> > +                error_propagate(&error_abort, local_err);
> > +            }
> 
> Aborts should never be triggered by user input.  The error instead
> should propagate from mce_init to its caller with a new errp argument
> (i.e. error_setg(errp, "KVM unavailable or LMCE not supported")).
> 
> x86_cpu_realizefn can pass &local_err and check the outcome through
> local_err != NULL.  See the existing call to x86_cpu_apic_create, right
> above the call to mce_init.
>

Ah yes, I'll pass that local_err into mce_init() in the next version.

> > @@ -878,7 +891,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >      c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
> >      if (c) {
> >          has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
> > -                                  !!(c->ecx & CPUID_EXT_SMX);
> > +                                  !!(c->ecx & CPUID_EXT_SMX) ||
> > +                                  !!(env->mcg_cap & MCG_LMCE_P);
> 
> This part is wrong; env->mcg_cap is independent from CPUID[1].ECX.
>

Along with your next comment, I'll set it in the next if.

> > +    }
> > +
> > +    if (has_msr_feature_control && (env->mcg_cap & MCG_LMCE_P)) {
> 
> Don't test has_msr_feature_control here, instead set it to true inside
> the "if".
> 
> > +        has_msr_mcg_ext_ctl = true;
> >      }
> >  
> >      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> 
> Which silicon has LMCE?  We may want to enable the property for some CPU
> models.  Apart from this, the patch is pretty much okay.
>

Skylake-EX

Thanks,
Haozhong

Re: [Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Haozhong Zhang]

On 06/16/16 11:50, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 08:06, Haozhong Zhang wrote:
> > +            if (!lmce_supported()) {
> > +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> > +                error_propagate(&error_abort, local_err);
> > +            }
> 
> Aborts should never be triggered by user input.  The error instead
> should propagate from mce_init to its caller with a new errp argument
> (i.e. error_setg(errp, "KVM unavailable or LMCE not supported")).
> 
> x86_cpu_realizefn can pass &local_err and check the outcome through
> local_err != NULL.  See the existing call to x86_cpu_apic_create, right
> above the call to mce_init.
>

Ah yes, I'll pass that local_err into mce_init() in the next version.

> > @@ -878,7 +891,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >      c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
> >      if (c) {
> >          has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
> > -                                  !!(c->ecx & CPUID_EXT_SMX);
> > +                                  !!(c->ecx & CPUID_EXT_SMX) ||
> > +                                  !!(env->mcg_cap & MCG_LMCE_P);
> 
> This part is wrong; env->mcg_cap is independent from CPUID[1].ECX.
>

Along with your next comment, I'll set it in the next if.

> > +    }
> > +
> > +    if (has_msr_feature_control && (env->mcg_cap & MCG_LMCE_P)) {
> 
> Don't test has_msr_feature_control here, instead set it to true inside
> the "if".
> 
> > +        has_msr_mcg_ext_ctl = true;
> >      }
> >  
> >      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> 
> Which silicon has LMCE?  We may want to enable the property for some CPU
> models.  Apart from this, the patch is pretty much okay.
>

Skylake-EX

Thanks,
Haozhong

Re: [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Paolo Bonzini]

On 16/06/2016 12:16, Haozhong Zhang wrote:
> > 
> > > +        has_msr_mcg_ext_ctl = true;
> > >      }
> > >  
> > >      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> > 
> > Which silicon has LMCE?  We may want to enable the property for some CPU
> > models.  Apart from this, the patch is pretty much okay.
>
> Skylake-EX

... However, all virtual CPUs can use LMCE because the rendez-vous is
done in the host.  Is this correct?

Paolo

Re: [Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Paolo Bonzini]

On 16/06/2016 12:16, Haozhong Zhang wrote:
> > 
> > > +        has_msr_mcg_ext_ctl = true;
> > >      }
> > >  
> > >      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> > 
> > Which silicon has LMCE?  We may want to enable the property for some CPU
> > models.  Apart from this, the patch is pretty much okay.
>
> Skylake-EX

... However, all virtual CPUs can use LMCE because the rendez-vous is
done in the host.  Is this correct?

Paolo

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Haozhong Zhang]

On 06/16/16 11:51, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 08:06, Haozhong Zhang wrote:
> > Migration is only allowed between VCPUs with the same lmce option.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/machine.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > index cb9adf2..00375a3 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -347,6 +347,12 @@ static int cpu_post_load(void *opaque, int version_id)
> >          return -EINVAL;
> >      }
> >  
> > +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
> > +        error_report("Config mismatch: VCPU has LMCE enabled, "
> > +                     "but \"lmce\" option is disabled");
> > +        return -EINVAL;
> > +    }
> > +
> 
> I think this is unnecessary.  Apart from this, the patch is good and can
> be squashed in patch 1 for v5.
>

Without this check, the migration from LMCE enabled QEMU to LMCE
disabled QEMU will not fail. Is such configuration change considered
be error? If not, I will remove the error report and return, but add a
fix to remove MCG_LMCE_P from env->mcg_cap in this check.

Haozhong

> 
> >      /*
> >       * Real mode guest segments register DPL should be zero.
> >       * Older KVM version were setting it wrongly.
> > @@ -896,6 +902,24 @@ static const VMStateDescription vmstate_tsc_khz = {
> >      }
> >  };
> >  
> > +static bool mcg_ext_ctl_needed(void *opaque)
> > +{
> > +    X86CPU *cpu = opaque;
> > +    CPUX86State *env = &cpu->env;
> > +    return cpu->enable_lmce && env->mcg_ext_ctl;
> > +}
> > +
> > +static const VMStateDescription vmstate_mcg_ext_ctl = {
> > +    .name = "cpu/mcg_ext_ctl",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = mcg_ext_ctl_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT64(env.mcg_ext_ctl, X86CPU),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  VMStateDescription vmstate_x86_cpu = {
> >      .name = "cpu",
> >      .version_id = 12,
> > @@ -1022,6 +1046,7 @@ VMStateDescription vmstate_x86_cpu = {
> >  #ifdef TARGET_X86_64
> >          &vmstate_pkru,
> >  #endif
> > +        &vmstate_mcg_ext_ctl,
> >          NULL
> >      }
> >  };
> > 

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Haozhong Zhang]

On 06/16/16 11:51, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 08:06, Haozhong Zhang wrote:
> > Migration is only allowed between VCPUs with the same lmce option.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/machine.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > index cb9adf2..00375a3 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -347,6 +347,12 @@ static int cpu_post_load(void *opaque, int version_id)
> >          return -EINVAL;
> >      }
> >  
> > +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
> > +        error_report("Config mismatch: VCPU has LMCE enabled, "
> > +                     "but \"lmce\" option is disabled");
> > +        return -EINVAL;
> > +    }
> > +
> 
> I think this is unnecessary.  Apart from this, the patch is good and can
> be squashed in patch 1 for v5.
>

Without this check, the migration from LMCE enabled QEMU to LMCE
disabled QEMU will not fail. Is such configuration change considered
be error? If not, I will remove the error report and return, but add a
fix to remove MCG_LMCE_P from env->mcg_cap in this check.

Haozhong

> 
> >      /*
> >       * Real mode guest segments register DPL should be zero.
> >       * Older KVM version were setting it wrongly.
> > @@ -896,6 +902,24 @@ static const VMStateDescription vmstate_tsc_khz = {
> >      }
> >  };
> >  
> > +static bool mcg_ext_ctl_needed(void *opaque)
> > +{
> > +    X86CPU *cpu = opaque;
> > +    CPUX86State *env = &cpu->env;
> > +    return cpu->enable_lmce && env->mcg_ext_ctl;
> > +}
> > +
> > +static const VMStateDescription vmstate_mcg_ext_ctl = {
> > +    .name = "cpu/mcg_ext_ctl",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = mcg_ext_ctl_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT64(env.mcg_ext_ctl, X86CPU),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  VMStateDescription vmstate_x86_cpu = {
> >      .name = "cpu",
> >      .version_id = 12,
> > @@ -1022,6 +1046,7 @@ VMStateDescription vmstate_x86_cpu = {
> >  #ifdef TARGET_X86_64
> >          &vmstate_pkru,
> >  #endif
> > +        &vmstate_mcg_ext_ctl,
> >          NULL
> >      }
> >  };
> > 

Re: [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Haozhong Zhang]

On 06/16/16 12:23, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 12:16, Haozhong Zhang wrote:
> > > 
> > > > +        has_msr_mcg_ext_ctl = true;
> > > >      }
> > > >  
> > > >      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> > > 
> > > Which silicon has LMCE?  We may want to enable the property for some CPU
> > > models.  Apart from this, the patch is pretty much okay.
> >
> > Skylake-EX
> 
> ... However, all virtual CPUs can use LMCE because the rendez-vous is
> done in the host.  Is this correct?
>

Yes, if it does not confuse the guest which sees LMCE available on
lower end or earlier CPUs (though I think someone would feel
happy). Or do we add it only to qemu64 and kvm64?

Haozhong

Re: [Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Haozhong Zhang]

On 06/16/16 12:23, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 12:16, Haozhong Zhang wrote:
> > > 
> > > > +        has_msr_mcg_ext_ctl = true;
> > > >      }
> > > >  
> > > >      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> > > 
> > > Which silicon has LMCE?  We may want to enable the property for some CPU
> > > models.  Apart from this, the patch is pretty much okay.
> >
> > Skylake-EX
> 
> ... However, all virtual CPUs can use LMCE because the rendez-vous is
> done in the host.  Is this correct?
>

Yes, if it does not confuse the guest which sees LMCE available on
lower end or earlier CPUs (though I think someone would feel
happy). Or do we add it only to qemu64 and kvm64?

Haozhong

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Paolo Bonzini]

On 16/06/2016 12:29, Haozhong Zhang wrote:
> On 06/16/16 11:51, Paolo Bonzini wrote:
>>
>>
>> On 16/06/2016 08:06, Haozhong Zhang wrote:
>>> Migration is only allowed between VCPUs with the same lmce option.
>>>
>>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>> ---
>>>  target-i386/machine.c | 25 +++++++++++++++++++++++++
>>>  1 file changed, 25 insertions(+)
>>>
>>> diff --git a/target-i386/machine.c b/target-i386/machine.c
>>> index cb9adf2..00375a3 100644
>>> --- a/target-i386/machine.c
>>> +++ b/target-i386/machine.c
>>> @@ -347,6 +347,12 @@ static int cpu_post_load(void *opaque, int version_id)
>>>          return -EINVAL;
>>>      }
>>>  
>>> +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
>>> +        error_report("Config mismatch: VCPU has LMCE enabled, "
>>> +                     "but \"lmce\" option is disabled");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>
>> I think this is unnecessary.  Apart from this, the patch is good and can
>> be squashed in patch 1 for v5.
>>
> 
> Without this check, the migration from LMCE enabled QEMU to LMCE
> disabled QEMU will not fail. Is such configuration change considered
> be error? If not, I will remove the error report and return, but add a
> fix to remove MCG_LMCE_P from env->mcg_cap in this check.

It's considered a user error.  You can skip the "if" completely.

Paolo

> Haozhong
> 
>>
>>>      /*
>>>       * Real mode guest segments register DPL should be zero.
>>>       * Older KVM version were setting it wrongly.
>>> @@ -896,6 +902,24 @@ static const VMStateDescription vmstate_tsc_khz = {
>>>      }
>>>  };
>>>  
>>> +static bool mcg_ext_ctl_needed(void *opaque)
>>> +{
>>> +    X86CPU *cpu = opaque;
>>> +    CPUX86State *env = &cpu->env;
>>> +    return cpu->enable_lmce && env->mcg_ext_ctl;
>>> +}
>>> +
>>> +static const VMStateDescription vmstate_mcg_ext_ctl = {
>>> +    .name = "cpu/mcg_ext_ctl",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .needed = mcg_ext_ctl_needed,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_UINT64(env.mcg_ext_ctl, X86CPU),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>>  VMStateDescription vmstate_x86_cpu = {
>>>      .name = "cpu",
>>>      .version_id = 12,
>>> @@ -1022,6 +1046,7 @@ VMStateDescription vmstate_x86_cpu = {
>>>  #ifdef TARGET_X86_64
>>>          &vmstate_pkru,
>>>  #endif
>>> +        &vmstate_mcg_ext_ctl,
>>>          NULL
>>>      }
>>>  };
>>>

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Paolo Bonzini]

On 16/06/2016 12:29, Haozhong Zhang wrote:
> On 06/16/16 11:51, Paolo Bonzini wrote:
>>
>>
>> On 16/06/2016 08:06, Haozhong Zhang wrote:
>>> Migration is only allowed between VCPUs with the same lmce option.
>>>
>>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>> ---
>>>  target-i386/machine.c | 25 +++++++++++++++++++++++++
>>>  1 file changed, 25 insertions(+)
>>>
>>> diff --git a/target-i386/machine.c b/target-i386/machine.c
>>> index cb9adf2..00375a3 100644
>>> --- a/target-i386/machine.c
>>> +++ b/target-i386/machine.c
>>> @@ -347,6 +347,12 @@ static int cpu_post_load(void *opaque, int version_id)
>>>          return -EINVAL;
>>>      }
>>>  
>>> +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
>>> +        error_report("Config mismatch: VCPU has LMCE enabled, "
>>> +                     "but \"lmce\" option is disabled");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>
>> I think this is unnecessary.  Apart from this, the patch is good and can
>> be squashed in patch 1 for v5.
>>
> 
> Without this check, the migration from LMCE enabled QEMU to LMCE
> disabled QEMU will not fail. Is such configuration change considered
> be error? If not, I will remove the error report and return, but add a
> fix to remove MCG_LMCE_P from env->mcg_cap in this check.

It's considered a user error.  You can skip the "if" completely.

Paolo

> Haozhong
> 
>>
>>>      /*
>>>       * Real mode guest segments register DPL should be zero.
>>>       * Older KVM version were setting it wrongly.
>>> @@ -896,6 +902,24 @@ static const VMStateDescription vmstate_tsc_khz = {
>>>      }
>>>  };
>>>  
>>> +static bool mcg_ext_ctl_needed(void *opaque)
>>> +{
>>> +    X86CPU *cpu = opaque;
>>> +    CPUX86State *env = &cpu->env;
>>> +    return cpu->enable_lmce && env->mcg_ext_ctl;
>>> +}
>>> +
>>> +static const VMStateDescription vmstate_mcg_ext_ctl = {
>>> +    .name = "cpu/mcg_ext_ctl",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .needed = mcg_ext_ctl_needed,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_UINT64(env.mcg_ext_ctl, X86CPU),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>>  VMStateDescription vmstate_x86_cpu = {
>>>      .name = "cpu",
>>>      .version_id = 12,
>>> @@ -1022,6 +1046,7 @@ VMStateDescription vmstate_x86_cpu = {
>>>  #ifdef TARGET_X86_64
>>>          &vmstate_pkru,
>>>  #endif
>>> +        &vmstate_mcg_ext_ctl,
>>>          NULL
>>>      }
>>>  };
>>>

Re: [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Paolo Bonzini]

On 16/06/2016 12:34, Haozhong Zhang wrote:
> On 06/16/16 12:23, Paolo Bonzini wrote:
>>
>>
>> On 16/06/2016 12:16, Haozhong Zhang wrote:
>>>>
>>>>> +        has_msr_mcg_ext_ctl = true;
>>>>>      }
>>>>>  
>>>>>      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
>>>>
>>>> Which silicon has LMCE?  We may want to enable the property for some CPU
>>>> models.  Apart from this, the patch is pretty much okay.
>>>
>>> Skylake-EX
>>
>> ... However, all virtual CPUs can use LMCE because the rendez-vous is
>> done in the host.  Is this correct?
>>
> 
> Yes, if it does not confuse the guest which sees LMCE available on
> lower end or earlier CPUs (though I think someone would feel
> happy).

Yes, that's what I expect too.  No confusion, and some happiness. :)

> Or do we add it only to qemu64 and kvm64?

I'm not sure where to add it, actually. :(  Let's wait for Eduardo's
opinion.

Paolo

Re: [Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Paolo Bonzini]

On 16/06/2016 12:34, Haozhong Zhang wrote:
> On 06/16/16 12:23, Paolo Bonzini wrote:
>>
>>
>> On 16/06/2016 12:16, Haozhong Zhang wrote:
>>>>
>>>>> +        has_msr_mcg_ext_ctl = true;
>>>>>      }
>>>>>  
>>>>>      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
>>>>
>>>> Which silicon has LMCE?  We may want to enable the property for some CPU
>>>> models.  Apart from this, the patch is pretty much okay.
>>>
>>> Skylake-EX
>>
>> ... However, all virtual CPUs can use LMCE because the rendez-vous is
>> done in the host.  Is this correct?
>>
> 
> Yes, if it does not confuse the guest which sees LMCE available on
> lower end or earlier CPUs (though I think someone would feel
> happy).

Yes, that's what I expect too.  No confusion, and some happiness. :)

> Or do we add it only to qemu64 and kvm64?

I'm not sure where to add it, actually. :(  Let's wait for Eduardo's
opinion.

Paolo

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Haozhong Zhang]

On 06/16/16 12:41, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 12:29, Haozhong Zhang wrote:
> > On 06/16/16 11:51, Paolo Bonzini wrote:
> >>
> >>
> >> On 16/06/2016 08:06, Haozhong Zhang wrote:
> >>> Migration is only allowed between VCPUs with the same lmce option.
> >>>
> >>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >>> ---
> >>>  target-i386/machine.c | 25 +++++++++++++++++++++++++
> >>>  1 file changed, 25 insertions(+)
> >>>
> >>> diff --git a/target-i386/machine.c b/target-i386/machine.c
> >>> index cb9adf2..00375a3 100644
> >>> --- a/target-i386/machine.c
> >>> +++ b/target-i386/machine.c
> >>> @@ -347,6 +347,12 @@ static int cpu_post_load(void *opaque, int version_id)
> >>>          return -EINVAL;
> >>>      }
> >>>  
> >>> +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
> >>> +        error_report("Config mismatch: VCPU has LMCE enabled, "
> >>> +                     "but \"lmce\" option is disabled");
> >>> +        return -EINVAL;
> >>> +    }
> >>> +
> >>
> >> I think this is unnecessary.  Apart from this, the patch is good and can
> >> be squashed in patch 1 for v5.
> >>
> > 
> > Without this check, the migration from LMCE enabled QEMU to LMCE
> > disabled QEMU will not fail. Is such configuration change considered
> > be error? If not, I will remove the error report and return, but add a
> > fix to remove MCG_LMCE_P from env->mcg_cap in this check.
> 
> It's considered a user error.  You can skip the "if" completely.
>

Eduardo said nice for this part in previous version [1], so we may wait
for his comments?

[1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html

Thanks,
Haozhong

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Haozhong Zhang]

On 06/16/16 12:41, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 12:29, Haozhong Zhang wrote:
> > On 06/16/16 11:51, Paolo Bonzini wrote:
> >>
> >>
> >> On 16/06/2016 08:06, Haozhong Zhang wrote:
> >>> Migration is only allowed between VCPUs with the same lmce option.
> >>>
> >>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >>> ---
> >>>  target-i386/machine.c | 25 +++++++++++++++++++++++++
> >>>  1 file changed, 25 insertions(+)
> >>>
> >>> diff --git a/target-i386/machine.c b/target-i386/machine.c
> >>> index cb9adf2..00375a3 100644
> >>> --- a/target-i386/machine.c
> >>> +++ b/target-i386/machine.c
> >>> @@ -347,6 +347,12 @@ static int cpu_post_load(void *opaque, int version_id)
> >>>          return -EINVAL;
> >>>      }
> >>>  
> >>> +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
> >>> +        error_report("Config mismatch: VCPU has LMCE enabled, "
> >>> +                     "but \"lmce\" option is disabled");
> >>> +        return -EINVAL;
> >>> +    }
> >>> +
> >>
> >> I think this is unnecessary.  Apart from this, the patch is good and can
> >> be squashed in patch 1 for v5.
> >>
> > 
> > Without this check, the migration from LMCE enabled QEMU to LMCE
> > disabled QEMU will not fail. Is such configuration change considered
> > be error? If not, I will remove the error report and return, but add a
> > fix to remove MCG_LMCE_P from env->mcg_cap in this check.
> 
> It's considered a user error.  You can skip the "if" completely.
>

Eduardo said nice for this part in previous version [1], so we may wait
for his comments?

[1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html

Thanks,
Haozhong

Re: [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Haozhong Zhang]

On 06/16/16 11:52, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 08:06, Haozhong Zhang wrote:
> > It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
> > be set before some features (e.g. VMX and LMCE) can be used, which is
> > usually done by the firmware. This patch adds a fw_cfg file
> > "etc/msr_feature_control" which contains the advised value of
> > MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
> > 
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  hw/i386/pc.c      | 28 ++++++++++++++++++++++++++++
> >  target-i386/cpu.h |  4 ++++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 7198ed5..d8178a5 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1147,6 +1147,33 @@ void pc_cpus_init(PCMachineState *pcms)
> >      smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
> >  }
> >  
> > +static void pc_build_feature_control_file(PCMachineState *pcms)
> > +{
> > +    X86CPU *cpu = X86_CPU(pcms->possible_cpus->cpus[0].cpu);
> > +    CPUX86State *env = &cpu->env;
> > +    uint32_t unused, ecx, edx, feature_control_bits = 0;
> > +    uint32_t *val;
> > +
> > +    cpu_x86_cpuid(env, 1, 0, &unused, &unused, &ecx, &edx);
> > +    if (ecx & CPUID_EXT_VMX) {
> > +        feature_control_bits |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
> > +    }
> > +
> > +    if ((edx & (CPUID_EXT2_MCE | CPUID_EXT2_MCA)) ==
> > +        (CPUID_EXT2_MCE | CPUID_EXT2_MCA) &&
> > +        (env->mcg_cap & MCG_LMCE_P)) {
> > +        feature_control_bits |= FEATURE_CONTROL_LMCE;
> > +    }
> > +
> > +    if (!feature_control_bits) {
> > +        return;
> > +    }
> > +
> > +    val = g_malloc(sizeof(*val));
> > +    *val = feature_control_bits | FEATURE_CONTROL_LOCKED;
> > +    fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
> > +}
> > +
> >  static
> >  void pc_machine_done(Notifier *notifier, void *data)
> >  {
> > @@ -1174,6 +1201,7 @@ void pc_machine_done(Notifier *notifier, void *data)
> >      acpi_setup();
> >      if (pcms->fw_cfg) {
> >          pc_build_smbios(pcms->fw_cfg);
> > +        pc_build_feature_control_file(pcms);
> >      }
> >  }
> >  
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index f0cb04f..5e07c7a 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -332,6 +332,10 @@
> >  #define MSR_TSC_ADJUST                  0x0000003b
> >  #define MSR_IA32_TSCDEADLINE            0x6e0
> >  
> > +#define FEATURE_CONTROL_LOCKED                    (1<<0)
> > +#define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX (1<<2)
> > +#define FEATURE_CONTROL_LMCE                      (1<<20)
> > +
> >  #define MSR_P6_PERFCTR0                 0xc1
> >  
> >  #define MSR_IA32_SMBASE                 0x9e
> > 
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Have you prepared a patch for SeaBIOS already?

Yes, I'll send it after I fix the type error (uint32_t => uint64_t) in
next version.

Thanks,
Haozhong

Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Haozhong Zhang]

On 06/16/16 11:52, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 08:06, Haozhong Zhang wrote:
> > It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
> > be set before some features (e.g. VMX and LMCE) can be used, which is
> > usually done by the firmware. This patch adds a fw_cfg file
> > "etc/msr_feature_control" which contains the advised value of
> > MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
> > 
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  hw/i386/pc.c      | 28 ++++++++++++++++++++++++++++
> >  target-i386/cpu.h |  4 ++++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 7198ed5..d8178a5 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1147,6 +1147,33 @@ void pc_cpus_init(PCMachineState *pcms)
> >      smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
> >  }
> >  
> > +static void pc_build_feature_control_file(PCMachineState *pcms)
> > +{
> > +    X86CPU *cpu = X86_CPU(pcms->possible_cpus->cpus[0].cpu);
> > +    CPUX86State *env = &cpu->env;
> > +    uint32_t unused, ecx, edx, feature_control_bits = 0;
> > +    uint32_t *val;
> > +
> > +    cpu_x86_cpuid(env, 1, 0, &unused, &unused, &ecx, &edx);
> > +    if (ecx & CPUID_EXT_VMX) {
> > +        feature_control_bits |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
> > +    }
> > +
> > +    if ((edx & (CPUID_EXT2_MCE | CPUID_EXT2_MCA)) ==
> > +        (CPUID_EXT2_MCE | CPUID_EXT2_MCA) &&
> > +        (env->mcg_cap & MCG_LMCE_P)) {
> > +        feature_control_bits |= FEATURE_CONTROL_LMCE;
> > +    }
> > +
> > +    if (!feature_control_bits) {
> > +        return;
> > +    }
> > +
> > +    val = g_malloc(sizeof(*val));
> > +    *val = feature_control_bits | FEATURE_CONTROL_LOCKED;
> > +    fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
> > +}
> > +
> >  static
> >  void pc_machine_done(Notifier *notifier, void *data)
> >  {
> > @@ -1174,6 +1201,7 @@ void pc_machine_done(Notifier *notifier, void *data)
> >      acpi_setup();
> >      if (pcms->fw_cfg) {
> >          pc_build_smbios(pcms->fw_cfg);
> > +        pc_build_feature_control_file(pcms);
> >      }
> >  }
> >  
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index f0cb04f..5e07c7a 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -332,6 +332,10 @@
> >  #define MSR_TSC_ADJUST                  0x0000003b
> >  #define MSR_IA32_TSCDEADLINE            0x6e0
> >  
> > +#define FEATURE_CONTROL_LOCKED                    (1<<0)
> > +#define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX (1<<2)
> > +#define FEATURE_CONTROL_LMCE                      (1<<20)
> > +
> >  #define MSR_P6_PERFCTR0                 0xc1
> >  
> >  #define MSR_IA32_SMBASE                 0x9e
> > 
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Have you prepared a patch for SeaBIOS already?

Yes, I'll send it after I fix the type error (uint32_t => uint64_t) in
next version.

Thanks,
Haozhong

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Eduardo Habkost]

On Thu, Jun 16, 2016 at 06:55:29PM +0800, Haozhong Zhang wrote:
> On 06/16/16 12:41, Paolo Bonzini wrote:
> > 
> > 
> > On 16/06/2016 12:29, Haozhong Zhang wrote:
> > > On 06/16/16 11:51, Paolo Bonzini wrote:
> > >>
> > >>
> > >> On 16/06/2016 08:06, Haozhong Zhang wrote:
> > >>> Migration is only allowed between VCPUs with the same lmce option.
> > >>>
> > >>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > >>> ---
> > >>>  target-i386/machine.c | 25 +++++++++++++++++++++++++
> > >>>  1 file changed, 25 insertions(+)
> > >>>
> > >>> diff --git a/target-i386/machine.c b/target-i386/machine.c
> > >>> index cb9adf2..00375a3 100644
> > >>> --- a/target-i386/machine.c
> > >>> +++ b/target-i386/machine.c
> > >>> @@ -347,6 +347,12 @@ static int cpu_post_load(void *opaque, int version_id)
> > >>>          return -EINVAL;
> > >>>      }
> > >>>  
> > >>> +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
> > >>> +        error_report("Config mismatch: VCPU has LMCE enabled, "
> > >>> +                     "but \"lmce\" option is disabled");
> > >>> +        return -EINVAL;
> > >>> +    }
> > >>> +
> > >>
> > >> I think this is unnecessary.  Apart from this, the patch is good and can
> > >> be squashed in patch 1 for v5.
> > >>
> > > 
> > > Without this check, the migration from LMCE enabled QEMU to LMCE
> > > disabled QEMU will not fail. Is such configuration change considered
> > > be error? If not, I will remove the error report and return, but add a
> > > fix to remove MCG_LMCE_P from env->mcg_cap in this check.
> > 
> > It's considered a user error.  You can skip the "if" completely.
> >
> 
> Eduardo said nice for this part in previous version [1], so we may wait
> for his comments?
> 
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html

I agree we don't need this check, but I still believe it is a
nice thing to have.

In addition to detecting user errors, they don't hurt and are
useful for things like "-cpu host", that don't guarantee
live-migration compatibility but still allow migration if you
ensure host capabilities are the same on both sides.

(I was going to suggest enabling lmce automatically on "-cpu
host" as a follow-up patch, BTW.)

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Eduardo Habkost]

On Thu, Jun 16, 2016 at 06:55:29PM +0800, Haozhong Zhang wrote:
> On 06/16/16 12:41, Paolo Bonzini wrote:
> > 
> > 
> > On 16/06/2016 12:29, Haozhong Zhang wrote:
> > > On 06/16/16 11:51, Paolo Bonzini wrote:
> > >>
> > >>
> > >> On 16/06/2016 08:06, Haozhong Zhang wrote:
> > >>> Migration is only allowed between VCPUs with the same lmce option.
> > >>>
> > >>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > >>> ---
> > >>>  target-i386/machine.c | 25 +++++++++++++++++++++++++
> > >>>  1 file changed, 25 insertions(+)
> > >>>
> > >>> diff --git a/target-i386/machine.c b/target-i386/machine.c
> > >>> index cb9adf2..00375a3 100644
> > >>> --- a/target-i386/machine.c
> > >>> +++ b/target-i386/machine.c
> > >>> @@ -347,6 +347,12 @@ static int cpu_post_load(void *opaque, int version_id)
> > >>>          return -EINVAL;
> > >>>      }
> > >>>  
> > >>> +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
> > >>> +        error_report("Config mismatch: VCPU has LMCE enabled, "
> > >>> +                     "but \"lmce\" option is disabled");
> > >>> +        return -EINVAL;
> > >>> +    }
> > >>> +
> > >>
> > >> I think this is unnecessary.  Apart from this, the patch is good and can
> > >> be squashed in patch 1 for v5.
> > >>
> > > 
> > > Without this check, the migration from LMCE enabled QEMU to LMCE
> > > disabled QEMU will not fail. Is such configuration change considered
> > > be error? If not, I will remove the error report and return, but add a
> > > fix to remove MCG_LMCE_P from env->mcg_cap in this check.
> > 
> > It's considered a user error.  You can skip the "if" completely.
> >
> 
> Eduardo said nice for this part in previous version [1], so we may wait
> for his comments?
> 
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html

I agree we don't need this check, but I still believe it is a
nice thing to have.

In addition to detecting user errors, they don't hurt and are
useful for things like "-cpu host", that don't guarantee
live-migration compatibility but still allow migration if you
ensure host capabilities are the same on both sides.

(I was going to suggest enabling lmce automatically on "-cpu
host" as a follow-up patch, BTW.)

-- 
Eduardo

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Paolo Bonzini]

On 16/06/2016 19:36, Eduardo Habkost wrote:
>> > 
>> > Eduardo said nice for this part in previous version [1], so we may wait
>> > for his comments?
>> > 
>> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html
> I agree we don't need this check, but I still believe it is a
> nice thing to have.
> 
> In addition to detecting user errors, they don't hurt and are
> useful for things like "-cpu host", that don't guarantee
> live-migration compatibility but still allow migration if you
> ensure host capabilities are the same on both sides.

On the other hand we don't check for this on any other property, either
CPU or device, do we?  Considering "lmce=on" always breaks on an old
kernel (i.e. there's no need for an explicit ",enforce" on the -cpu
flag), I think it's unnecessary and makes things inconsistent.

> (I was going to suggest enabling lmce automatically on "-cpu
> host" as a follow-up patch, BTW.)

Interesting.  Technically it comes from the host kernel, not from the
host CPU.  But it does sounds like a good idea; -cpu host pretty much
implies the same kernel (in addition to the same processor) on both
sides of the migration.

Paolo

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Paolo Bonzini]

On 16/06/2016 19:36, Eduardo Habkost wrote:
>> > 
>> > Eduardo said nice for this part in previous version [1], so we may wait
>> > for his comments?
>> > 
>> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html
> I agree we don't need this check, but I still believe it is a
> nice thing to have.
> 
> In addition to detecting user errors, they don't hurt and are
> useful for things like "-cpu host", that don't guarantee
> live-migration compatibility but still allow migration if you
> ensure host capabilities are the same on both sides.

On the other hand we don't check for this on any other property, either
CPU or device, do we?  Considering "lmce=on" always breaks on an old
kernel (i.e. there's no need for an explicit ",enforce" on the -cpu
flag), I think it's unnecessary and makes things inconsistent.

> (I was going to suggest enabling lmce automatically on "-cpu
> host" as a follow-up patch, BTW.)

Interesting.  Technically it comes from the host kernel, not from the
host CPU.  But it does sounds like a good idea; -cpu host pretty much
implies the same kernel (in addition to the same processor) on both
sides of the migration.

Paolo

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Eduardo Habkost]

On Thu, Jun 16, 2016 at 07:40:20PM +0200, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 19:36, Eduardo Habkost wrote:
> >> > 
> >> > Eduardo said nice for this part in previous version [1], so we may wait
> >> > for his comments?
> >> > 
> >> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html
> > I agree we don't need this check, but I still believe it is a
> > nice thing to have.
> > 
> > In addition to detecting user errors, they don't hurt and are
> > useful for things like "-cpu host", that don't guarantee
> > live-migration compatibility but still allow migration if you
> > ensure host capabilities are the same on both sides.
> 
> On the other hand we don't check for this on any other property, either
> CPU or device, do we?  Considering "lmce=on" always breaks on an old
> kernel (i.e. there's no need for an explicit ",enforce" on the -cpu
> flag), I think it's unnecessary and makes things inconsistent.

We don't check that because we normally can't: we usually don't
send any configuration data (or anything that could be used to
detect configuration mismatches) to the destination. When we do,
it's often by accident.

In this case, it looks like we never needed to send mcg_cap in
the migration stream. But we already send it, so let's use it for
something useful.

I believe we should have more checks like these, when possible. I
have been planning for a while to send CPUID data in the
migration stream, to detect migration compatibility errors
(either user errors or QEMU bugs).

In theory, those checks should never be necessary. In practice I
believe they would be very useful.

> 
> > (I was going to suggest enabling lmce automatically on "-cpu
> > host" as a follow-up patch, BTW.)
> 
> Interesting.  Technically it comes from the host kernel, not from the
> host CPU.  But it does sounds like a good idea; -cpu host pretty much
> implies the same kernel (in addition to the same processor) on both
> sides of the migration.

"-cpu host" already means "whatever is allowed by the host [CPU
and/or kernel]", not just "host CPU". It enables x2apic on all
hosts, for example.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Eduardo Habkost]

On Thu, Jun 16, 2016 at 07:40:20PM +0200, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 19:36, Eduardo Habkost wrote:
> >> > 
> >> > Eduardo said nice for this part in previous version [1], so we may wait
> >> > for his comments?
> >> > 
> >> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html
> > I agree we don't need this check, but I still believe it is a
> > nice thing to have.
> > 
> > In addition to detecting user errors, they don't hurt and are
> > useful for things like "-cpu host", that don't guarantee
> > live-migration compatibility but still allow migration if you
> > ensure host capabilities are the same on both sides.
> 
> On the other hand we don't check for this on any other property, either
> CPU or device, do we?  Considering "lmce=on" always breaks on an old
> kernel (i.e. there's no need for an explicit ",enforce" on the -cpu
> flag), I think it's unnecessary and makes things inconsistent.

We don't check that because we normally can't: we usually don't
send any configuration data (or anything that could be used to
detect configuration mismatches) to the destination. When we do,
it's often by accident.

In this case, it looks like we never needed to send mcg_cap in
the migration stream. But we already send it, so let's use it for
something useful.

I believe we should have more checks like these, when possible. I
have been planning for a while to send CPUID data in the
migration stream, to detect migration compatibility errors
(either user errors or QEMU bugs).

In theory, those checks should never be necessary. In practice I
believe they would be very useful.

> 
> > (I was going to suggest enabling lmce automatically on "-cpu
> > host" as a follow-up patch, BTW.)
> 
> Interesting.  Technically it comes from the host kernel, not from the
> host CPU.  But it does sounds like a good idea; -cpu host pretty much
> implies the same kernel (in addition to the same processor) on both
> sides of the migration.

"-cpu host" already means "whatever is allowed by the host [CPU
and/or kernel]", not just "host CPU". It enables x2apic on all
hosts, for example.

-- 
Eduardo

Re: [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Eduardo Habkost]

On Thu, Jun 16, 2016 at 12:42:19PM +0200, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 12:34, Haozhong Zhang wrote:
> > On 06/16/16 12:23, Paolo Bonzini wrote:
> >>
> >>
> >> On 16/06/2016 12:16, Haozhong Zhang wrote:
> >>>>
> >>>>> +        has_msr_mcg_ext_ctl = true;
> >>>>>      }
> >>>>>  
> >>>>>      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> >>>>
> >>>> Which silicon has LMCE?  We may want to enable the property for some CPU
> >>>> models.  Apart from this, the patch is pretty much okay.
> >>>
> >>> Skylake-EX
> >>
> >> ... However, all virtual CPUs can use LMCE because the rendez-vous is
> >> done in the host.  Is this correct?
> >>
> > 
> > Yes, if it does not confuse the guest which sees LMCE available on
> > lower end or earlier CPUs (though I think someone would feel
> > happy).
> 
> Yes, that's what I expect too.  No confusion, and some happiness. :)
> 
> > Or do we add it only to qemu64 and kvm64?
> 
> I'm not sure where to add it, actually. :(  Let's wait for Eduardo's
> opinion.

Unfortunately we can't enable it by default to any existing CPU
model, or we break the machine-type-runnability rule
(machine-type version changes in an existing runnable VM
configuration should never make the VM not runnable in the same
host).

If one day we decide that QEMU as a whole can require a newer
kernel, then we can enable it by default on all CPU models.

What's the first kernel release where LMCE is enabled in
KVM_X86_GET_MCE_CAP_SUPPORTED, BTW?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Eduardo Habkost]

On Thu, Jun 16, 2016 at 12:42:19PM +0200, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 12:34, Haozhong Zhang wrote:
> > On 06/16/16 12:23, Paolo Bonzini wrote:
> >>
> >>
> >> On 16/06/2016 12:16, Haozhong Zhang wrote:
> >>>>
> >>>>> +        has_msr_mcg_ext_ctl = true;
> >>>>>      }
> >>>>>  
> >>>>>      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> >>>>
> >>>> Which silicon has LMCE?  We may want to enable the property for some CPU
> >>>> models.  Apart from this, the patch is pretty much okay.
> >>>
> >>> Skylake-EX
> >>
> >> ... However, all virtual CPUs can use LMCE because the rendez-vous is
> >> done in the host.  Is this correct?
> >>
> > 
> > Yes, if it does not confuse the guest which sees LMCE available on
> > lower end or earlier CPUs (though I think someone would feel
> > happy).
> 
> Yes, that's what I expect too.  No confusion, and some happiness. :)
> 
> > Or do we add it only to qemu64 and kvm64?
> 
> I'm not sure where to add it, actually. :(  Let's wait for Eduardo's
> opinion.

Unfortunately we can't enable it by default to any existing CPU
model, or we break the machine-type-runnability rule
(machine-type version changes in an existing runnable VM
configuration should never make the VM not runnable in the same
host).

If one day we decide that QEMU as a whole can require a newer
kernel, then we can enable it by default on all CPU models.

What's the first kernel release where LMCE is enabled in
KVM_X86_GET_MCE_CAP_SUPPORTED, BTW?

-- 
Eduardo

Re: [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Paolo Bonzini]

On 16/06/2016 20:05, Eduardo Habkost wrote:
> On Thu, Jun 16, 2016 at 12:42:19PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 16/06/2016 12:34, Haozhong Zhang wrote:
>>> On 06/16/16 12:23, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 16/06/2016 12:16, Haozhong Zhang wrote:
>>>>>>
>>>>>>> +        has_msr_mcg_ext_ctl = true;
>>>>>>>      }
>>>>>>>  
>>>>>>>      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
>>>>>>
>>>>>> Which silicon has LMCE?  We may want to enable the property for some CPU
>>>>>> models.  Apart from this, the patch is pretty much okay.
>>>>>
>>>>> Skylake-EX
>>>>
>>>> ... However, all virtual CPUs can use LMCE because the rendez-vous is
>>>> done in the host.  Is this correct?
>>>>
>>>
>>> Yes, if it does not confuse the guest which sees LMCE available on
>>> lower end or earlier CPUs (though I think someone would feel
>>> happy).
>>
>> Yes, that's what I expect too.  No confusion, and some happiness. :)
>>
>>> Or do we add it only to qemu64 and kvm64?
>>
>> I'm not sure where to add it, actually. :(  Let's wait for Eduardo's
>> opinion.
> 
> Unfortunately we can't enable it by default to any existing CPU
> model, or we break the machine-type-runnability rule
> (machine-type version changes in an existing runnable VM
> configuration should never make the VM not runnable in the same
> host).
> 
> If one day we decide that QEMU as a whole can require a newer
> kernel, then we can enable it by default on all CPU models.
> 
> What's the first kernel release where LMCE is enabled in
> KVM_X86_GET_MCE_CAP_SUPPORTED, BTW?

It will presumably be 4.8.

Paolo

Re: [Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Paolo Bonzini]

On 16/06/2016 20:05, Eduardo Habkost wrote:
> On Thu, Jun 16, 2016 at 12:42:19PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 16/06/2016 12:34, Haozhong Zhang wrote:
>>> On 06/16/16 12:23, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 16/06/2016 12:16, Haozhong Zhang wrote:
>>>>>>
>>>>>>> +        has_msr_mcg_ext_ctl = true;
>>>>>>>      }
>>>>>>>  
>>>>>>>      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
>>>>>>
>>>>>> Which silicon has LMCE?  We may want to enable the property for some CPU
>>>>>> models.  Apart from this, the patch is pretty much okay.
>>>>>
>>>>> Skylake-EX
>>>>
>>>> ... However, all virtual CPUs can use LMCE because the rendez-vous is
>>>> done in the host.  Is this correct?
>>>>
>>>
>>> Yes, if it does not confuse the guest which sees LMCE available on
>>> lower end or earlier CPUs (though I think someone would feel
>>> happy).
>>
>> Yes, that's what I expect too.  No confusion, and some happiness. :)
>>
>>> Or do we add it only to qemu64 and kvm64?
>>
>> I'm not sure where to add it, actually. :(  Let's wait for Eduardo's
>> opinion.
> 
> Unfortunately we can't enable it by default to any existing CPU
> model, or we break the machine-type-runnability rule
> (machine-type version changes in an existing runnable VM
> configuration should never make the VM not runnable in the same
> host).
> 
> If one day we decide that QEMU as a whole can require a newer
> kernel, then we can enable it by default on all CPU models.
> 
> What's the first kernel release where LMCE is enabled in
> KVM_X86_GET_MCE_CAP_SUPPORTED, BTW?

It will presumably be 4.8.

Paolo

Re: [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Eduardo Habkost]

On Thu, Jun 16, 2016 at 02:06:19PM +0800, Haozhong Zhang wrote:
> From: Ashok Raj <ashok.raj@intel.com>
> 
> This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> are injected to only one VCPU rather than broadcast to all VCPUs. As KVM
> reports LMCE support on Intel platforms, this features is only available
> on Intel platforms.
> 
> LMCE is disabled by default and can be enabled/disabled by cpu option
> 'lmce=on/off'.
> 
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> [Haozhong: Enable LMCE only on Intel platforms
>            Disable LMCE by default and add a cpu option 'lmce'
> 	   Disable LMCE if missing KVM support
> 	   Remove MCG_LMCE_P from MCE_CAP_DEF
> 	   Minor code style changes]

You are mixing tabs and spaces above.

> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  target-i386/cpu.c | 23 +++++++++++++++++++++++
>  target-i386/cpu.h | 12 ++++++++++++
>  target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
>  3 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 895a386..bd35db2 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2777,15 +2777,37 @@ static void x86_cpu_machine_reset_cb(void *opaque)
>  }
>  #endif
>  
> +static bool lmce_supported(void)
> +{
> +    uint64_t mce_cap;
> +
> +    if (!kvm_enabled() ||
> +        kvm_ioctl(kvm_state, KVM_X86_GET_MCE_CAP_SUPPORTED, &mce_cap) < 0) {
> +        return false;
> +    }
> +
> +    return !!(mce_cap & MCG_LMCE_P);
> +}
> +
>  static void mce_init(X86CPU *cpu)
>  {
>      CPUX86State *cenv = &cpu->env;
>      unsigned int bank;
> +    Error *local_err = NULL;
>  
>      if (((cenv->cpuid_version >> 8) & 0xf) >= 6
>          && (cenv->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
>              (CPUID_MCE | CPUID_MCA)) {
>          cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
> +
> +        if (cpu->enable_lmce) {
> +            if (!lmce_supported()) {
> +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> +                error_propagate(&error_abort, local_err);
> +            }
> +            cenv->mcg_cap |= MCG_LMCE_P;
> +        }
> +

This duplicates the existing check in kvm_arch_init_vcpu(). The
difference is that the existing code is KVM-specific and doesn't
stop initialization when capabilities are missing. We can unify
them into a single mcg_cap-checking function as a follow-up.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Eduardo Habkost]

On Thu, Jun 16, 2016 at 02:06:19PM +0800, Haozhong Zhang wrote:
> From: Ashok Raj <ashok.raj@intel.com>
> 
> This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> are injected to only one VCPU rather than broadcast to all VCPUs. As KVM
> reports LMCE support on Intel platforms, this features is only available
> on Intel platforms.
> 
> LMCE is disabled by default and can be enabled/disabled by cpu option
> 'lmce=on/off'.
> 
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> [Haozhong: Enable LMCE only on Intel platforms
>            Disable LMCE by default and add a cpu option 'lmce'
> 	   Disable LMCE if missing KVM support
> 	   Remove MCG_LMCE_P from MCE_CAP_DEF
> 	   Minor code style changes]

You are mixing tabs and spaces above.

> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  target-i386/cpu.c | 23 +++++++++++++++++++++++
>  target-i386/cpu.h | 12 ++++++++++++
>  target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
>  3 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 895a386..bd35db2 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2777,15 +2777,37 @@ static void x86_cpu_machine_reset_cb(void *opaque)
>  }
>  #endif
>  
> +static bool lmce_supported(void)
> +{
> +    uint64_t mce_cap;
> +
> +    if (!kvm_enabled() ||
> +        kvm_ioctl(kvm_state, KVM_X86_GET_MCE_CAP_SUPPORTED, &mce_cap) < 0) {
> +        return false;
> +    }
> +
> +    return !!(mce_cap & MCG_LMCE_P);
> +}
> +
>  static void mce_init(X86CPU *cpu)
>  {
>      CPUX86State *cenv = &cpu->env;
>      unsigned int bank;
> +    Error *local_err = NULL;
>  
>      if (((cenv->cpuid_version >> 8) & 0xf) >= 6
>          && (cenv->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
>              (CPUID_MCE | CPUID_MCA)) {
>          cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
> +
> +        if (cpu->enable_lmce) {
> +            if (!lmce_supported()) {
> +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> +                error_propagate(&error_abort, local_err);
> +            }
> +            cenv->mcg_cap |= MCG_LMCE_P;
> +        }
> +

This duplicates the existing check in kvm_arch_init_vcpu(). The
difference is that the existing code is KVM-specific and doesn't
stop initialization when capabilities are missing. We can unify
them into a single mcg_cap-checking function as a follow-up.

-- 
Eduardo

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Eduardo Habkost]

On Thu, Jun 16, 2016 at 07:40:20PM +0200, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 19:36, Eduardo Habkost wrote:
> >> > 
> >> > Eduardo said nice for this part in previous version [1], so we may wait
> >> > for his comments?
> >> > 
> >> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html
> > I agree we don't need this check, but I still believe it is a
> > nice thing to have.
> > 
> > In addition to detecting user errors, they don't hurt and are
> > useful for things like "-cpu host", that don't guarantee
> > live-migration compatibility but still allow migration if you
> > ensure host capabilities are the same on both sides.
> 
> On the other hand we don't check for this on any other property, either
> CPU or device, do we?  Considering "lmce=on" always breaks on an old
> kernel (i.e. there's no need for an explicit ",enforce" on the -cpu
> flag), I think it's unnecessary and makes things inconsistent.

BTW, just found another case where we check for migration mismatches: TSC
frequency.

    if (env->tsc_khz && env->user_tsc_khz &&
        env->tsc_khz != env->user_tsc_khz) {
        error_report("Mismatch between user-specified TSC frequency and "
                     "migrated TSC frequency");
        return -EINVAL;
    }

We can do that because tsc_khz is unusual like mcg_cap: it can be
configured by the user but is also included in the migration
stream.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Eduardo Habkost]

On Thu, Jun 16, 2016 at 07:40:20PM +0200, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 19:36, Eduardo Habkost wrote:
> >> > 
> >> > Eduardo said nice for this part in previous version [1], so we may wait
> >> > for his comments?
> >> > 
> >> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html
> > I agree we don't need this check, but I still believe it is a
> > nice thing to have.
> > 
> > In addition to detecting user errors, they don't hurt and are
> > useful for things like "-cpu host", that don't guarantee
> > live-migration compatibility but still allow migration if you
> > ensure host capabilities are the same on both sides.
> 
> On the other hand we don't check for this on any other property, either
> CPU or device, do we?  Considering "lmce=on" always breaks on an old
> kernel (i.e. there's no need for an explicit ",enforce" on the -cpu
> flag), I think it's unnecessary and makes things inconsistent.

BTW, just found another case where we check for migration mismatches: TSC
frequency.

    if (env->tsc_khz && env->user_tsc_khz &&
        env->tsc_khz != env->user_tsc_khz) {
        error_report("Mismatch between user-specified TSC frequency and "
                     "migrated TSC frequency");
        return -EINVAL;
    }

We can do that because tsc_khz is unusual like mcg_cap: it can be
configured by the user but is also included in the migration
stream.

-- 
Eduardo

Re: [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Haozhong Zhang]

On 06/16/16 16:37, Eduardo Habkost wrote:
> On Thu, Jun 16, 2016 at 02:06:19PM +0800, Haozhong Zhang wrote:
> > From: Ashok Raj <ashok.raj@intel.com>
> > 
> > This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> > are injected to only one VCPU rather than broadcast to all VCPUs. As KVM
> > reports LMCE support on Intel platforms, this features is only available
> > on Intel platforms.
> > 
> > LMCE is disabled by default and can be enabled/disabled by cpu option
> > 'lmce=on/off'.
> > 
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > [Haozhong: Enable LMCE only on Intel platforms
> >            Disable LMCE by default and add a cpu option 'lmce'
> > 	   Disable LMCE if missing KVM support
> > 	   Remove MCG_LMCE_P from MCE_CAP_DEF
> > 	   Minor code style changes]
>
> You are mixing tabs and spaces above.
>

Oops, I missed to take care tabs in the commit message. will fix

> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/cpu.c | 23 +++++++++++++++++++++++
> >  target-i386/cpu.h | 12 ++++++++++++
> >  target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
> >  3 files changed, 66 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 895a386..bd35db2 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2777,15 +2777,37 @@ static void x86_cpu_machine_reset_cb(void *opaque)
> >  }
> >  #endif
> >  
> > +static bool lmce_supported(void)
> > +{
> > +    uint64_t mce_cap;
> > +
> > +    if (!kvm_enabled() ||
> > +        kvm_ioctl(kvm_state, KVM_X86_GET_MCE_CAP_SUPPORTED, &mce_cap) < 0) {
> > +        return false;
> > +    }
> > +
> > +    return !!(mce_cap & MCG_LMCE_P);
> > +}
> > +
> >  static void mce_init(X86CPU *cpu)
> >  {
> >      CPUX86State *cenv = &cpu->env;
> >      unsigned int bank;
> > +    Error *local_err = NULL;
> >  
> >      if (((cenv->cpuid_version >> 8) & 0xf) >= 6
> >          && (cenv->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
> >              (CPUID_MCE | CPUID_MCA)) {
> >          cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
> > +
> > +        if (cpu->enable_lmce) {
> > +            if (!lmce_supported()) {
> > +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> > +                error_propagate(&error_abort, local_err);
> > +            }
> > +            cenv->mcg_cap |= MCG_LMCE_P;
> > +        }
> > +
> 
> This duplicates the existing check in kvm_arch_init_vcpu(). The
> difference is that the existing code is KVM-specific and doesn't
> stop initialization when capabilities are missing. We can unify
> them into a single mcg_cap-checking function as a follow-up.
>

If I reuse the existing MCE capability check in kvm_arch_init_vcpu(),
is it reasonable to make change to stop initialization if missing
capabilities? Or should we stop only for missing newly added capabilities
(e.g. LMCE) in order to keep backwards compatibility?

Thanks,
Haozhong

Re: [Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Haozhong Zhang]

On 06/16/16 16:37, Eduardo Habkost wrote:
> On Thu, Jun 16, 2016 at 02:06:19PM +0800, Haozhong Zhang wrote:
> > From: Ashok Raj <ashok.raj@intel.com>
> > 
> > This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> > are injected to only one VCPU rather than broadcast to all VCPUs. As KVM
> > reports LMCE support on Intel platforms, this features is only available
> > on Intel platforms.
> > 
> > LMCE is disabled by default and can be enabled/disabled by cpu option
> > 'lmce=on/off'.
> > 
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > [Haozhong: Enable LMCE only on Intel platforms
> >            Disable LMCE by default and add a cpu option 'lmce'
> > 	   Disable LMCE if missing KVM support
> > 	   Remove MCG_LMCE_P from MCE_CAP_DEF
> > 	   Minor code style changes]
>
> You are mixing tabs and spaces above.
>

Oops, I missed to take care tabs in the commit message. will fix

> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/cpu.c | 23 +++++++++++++++++++++++
> >  target-i386/cpu.h | 12 ++++++++++++
> >  target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
> >  3 files changed, 66 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 895a386..bd35db2 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2777,15 +2777,37 @@ static void x86_cpu_machine_reset_cb(void *opaque)
> >  }
> >  #endif
> >  
> > +static bool lmce_supported(void)
> > +{
> > +    uint64_t mce_cap;
> > +
> > +    if (!kvm_enabled() ||
> > +        kvm_ioctl(kvm_state, KVM_X86_GET_MCE_CAP_SUPPORTED, &mce_cap) < 0) {
> > +        return false;
> > +    }
> > +
> > +    return !!(mce_cap & MCG_LMCE_P);
> > +}
> > +
> >  static void mce_init(X86CPU *cpu)
> >  {
> >      CPUX86State *cenv = &cpu->env;
> >      unsigned int bank;
> > +    Error *local_err = NULL;
> >  
> >      if (((cenv->cpuid_version >> 8) & 0xf) >= 6
> >          && (cenv->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
> >              (CPUID_MCE | CPUID_MCA)) {
> >          cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
> > +
> > +        if (cpu->enable_lmce) {
> > +            if (!lmce_supported()) {
> > +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> > +                error_propagate(&error_abort, local_err);
> > +            }
> > +            cenv->mcg_cap |= MCG_LMCE_P;
> > +        }
> > +
> 
> This duplicates the existing check in kvm_arch_init_vcpu(). The
> difference is that the existing code is KVM-specific and doesn't
> stop initialization when capabilities are missing. We can unify
> them into a single mcg_cap-checking function as a follow-up.
>

If I reuse the existing MCE capability check in kvm_arch_init_vcpu(),
is it reasonable to make change to stop initialization if missing
capabilities? Or should we stop only for missing newly added capabilities
(e.g. LMCE) in order to keep backwards compatibility?

Thanks,
Haozhong

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Haozhong Zhang]

On 06/16/16 14:58, Eduardo Habkost wrote:
> On Thu, Jun 16, 2016 at 07:40:20PM +0200, Paolo Bonzini wrote:
> > 
> > 
> > On 16/06/2016 19:36, Eduardo Habkost wrote:
> > >> > 
> > >> > Eduardo said nice for this part in previous version [1], so we may wait
> > >> > for his comments?
> > >> > 
> > >> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html
> > > I agree we don't need this check, but I still believe it is a
> > > nice thing to have.
> > > 
> > > In addition to detecting user errors, they don't hurt and are
> > > useful for things like "-cpu host", that don't guarantee
> > > live-migration compatibility but still allow migration if you
> > > ensure host capabilities are the same on both sides.
> > 
> > On the other hand we don't check for this on any other property, either
> > CPU or device, do we?  Considering "lmce=on" always breaks on an old
> > kernel (i.e. there's no need for an explicit ",enforce" on the -cpu
> > flag), I think it's unnecessary and makes things inconsistent.
> 
> We don't check that because we normally can't: we usually don't
> send any configuration data (or anything that could be used to
> detect configuration mismatches) to the destination. When we do,
> it's often by accident.
> 
> In this case, it looks like we never needed to send mcg_cap in
> the migration stream. But we already send it, so let's use it for
> something useful.
> 
> I believe we should have more checks like these, when possible. I
> have been planning for a while to send CPUID data in the
> migration stream, to detect migration compatibility errors
> (either user errors or QEMU bugs).
> 
> In theory, those checks should never be necessary. In practice I
> believe they would be very useful.
>

Hi Eduardo and Paolo,

What will be the conclusion? Do we still need this check?

I'm fine to remove this check if we normally didn't make such kind of
checks and require users to avoid configuration mismatch.

> > 
> > > (I was going to suggest enabling lmce automatically on "-cpu
> > > host" as a follow-up patch, BTW.)
> > 
> > Interesting.  Technically it comes from the host kernel, not from the
> > host CPU.  But it does sounds like a good idea; -cpu host pretty much
> > implies the same kernel (in addition to the same processor) on both
> > sides of the migration.
> 
> "-cpu host" already means "whatever is allowed by the host [CPU
> and/or kernel]", not just "host CPU". It enables x2apic on all
> hosts, for example.
>

Does that mean we can automatically enable LMCE for "-cpu host"?

Thanks,
Haozhong

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Haozhong Zhang]

On 06/16/16 14:58, Eduardo Habkost wrote:
> On Thu, Jun 16, 2016 at 07:40:20PM +0200, Paolo Bonzini wrote:
> > 
> > 
> > On 16/06/2016 19:36, Eduardo Habkost wrote:
> > >> > 
> > >> > Eduardo said nice for this part in previous version [1], so we may wait
> > >> > for his comments?
> > >> > 
> > >> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html
> > > I agree we don't need this check, but I still believe it is a
> > > nice thing to have.
> > > 
> > > In addition to detecting user errors, they don't hurt and are
> > > useful for things like "-cpu host", that don't guarantee
> > > live-migration compatibility but still allow migration if you
> > > ensure host capabilities are the same on both sides.
> > 
> > On the other hand we don't check for this on any other property, either
> > CPU or device, do we?  Considering "lmce=on" always breaks on an old
> > kernel (i.e. there's no need for an explicit ",enforce" on the -cpu
> > flag), I think it's unnecessary and makes things inconsistent.
> 
> We don't check that because we normally can't: we usually don't
> send any configuration data (or anything that could be used to
> detect configuration mismatches) to the destination. When we do,
> it's often by accident.
> 
> In this case, it looks like we never needed to send mcg_cap in
> the migration stream. But we already send it, so let's use it for
> something useful.
> 
> I believe we should have more checks like these, when possible. I
> have been planning for a while to send CPUID data in the
> migration stream, to detect migration compatibility errors
> (either user errors or QEMU bugs).
> 
> In theory, those checks should never be necessary. In practice I
> believe they would be very useful.
>

Hi Eduardo and Paolo,

What will be the conclusion? Do we still need this check?

I'm fine to remove this check if we normally didn't make such kind of
checks and require users to avoid configuration mismatch.

> > 
> > > (I was going to suggest enabling lmce automatically on "-cpu
> > > host" as a follow-up patch, BTW.)
> > 
> > Interesting.  Technically it comes from the host kernel, not from the
> > host CPU.  But it does sounds like a good idea; -cpu host pretty much
> > implies the same kernel (in addition to the same processor) on both
> > sides of the migration.
> 
> "-cpu host" already means "whatever is allowed by the host [CPU
> and/or kernel]", not just "host CPU". It enables x2apic on all
> hosts, for example.
>

Does that mean we can automatically enable LMCE for "-cpu host"?

Thanks,
Haozhong

Re: [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Eduardo Habkost]

On Fri, Jun 17, 2016 at 09:26:57AM +0800, Haozhong Zhang wrote:
[...]
> > >  static void mce_init(X86CPU *cpu)
> > >  {
> > >      CPUX86State *cenv = &cpu->env;
> > >      unsigned int bank;
> > > +    Error *local_err = NULL;
> > >  
> > >      if (((cenv->cpuid_version >> 8) & 0xf) >= 6
> > >          && (cenv->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
> > >              (CPUID_MCE | CPUID_MCA)) {
> > >          cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
> > > +
> > > +        if (cpu->enable_lmce) {
> > > +            if (!lmce_supported()) {
> > > +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> > > +                error_propagate(&error_abort, local_err);
> > > +            }
> > > +            cenv->mcg_cap |= MCG_LMCE_P;
> > > +        }
> > > +
> > 
> > This duplicates the existing check in kvm_arch_init_vcpu(). The
> > difference is that the existing code is KVM-specific and doesn't
> > stop initialization when capabilities are missing. We can unify
> > them into a single mcg_cap-checking function as a follow-up.
> >
> 
> If I reuse the existing MCE capability check in kvm_arch_init_vcpu(),
> is it reasonable to make change to stop initialization if missing
> capabilities? Or should we stop only for missing newly added capabilities
> (e.g. LMCE) in order to keep backwards compatibility?

Ideally, yes. But in practice we need to check if we won't break
existing setups that were working. If all kernel versions we care
about always MCG_CTL_P|MCG_SER_P + 10 banks as supported, we can
make all bits mandatory.

I need to re-read the thread were kvm_get_mce_cap_supported() was
discussed, to refresh my memory.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Eduardo Habkost]

On Fri, Jun 17, 2016 at 09:26:57AM +0800, Haozhong Zhang wrote:
[...]
> > >  static void mce_init(X86CPU *cpu)
> > >  {
> > >      CPUX86State *cenv = &cpu->env;
> > >      unsigned int bank;
> > > +    Error *local_err = NULL;
> > >  
> > >      if (((cenv->cpuid_version >> 8) & 0xf) >= 6
> > >          && (cenv->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
> > >              (CPUID_MCE | CPUID_MCA)) {
> > >          cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
> > > +
> > > +        if (cpu->enable_lmce) {
> > > +            if (!lmce_supported()) {
> > > +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> > > +                error_propagate(&error_abort, local_err);
> > > +            }
> > > +            cenv->mcg_cap |= MCG_LMCE_P;
> > > +        }
> > > +
> > 
> > This duplicates the existing check in kvm_arch_init_vcpu(). The
> > difference is that the existing code is KVM-specific and doesn't
> > stop initialization when capabilities are missing. We can unify
> > them into a single mcg_cap-checking function as a follow-up.
> >
> 
> If I reuse the existing MCE capability check in kvm_arch_init_vcpu(),
> is it reasonable to make change to stop initialization if missing
> capabilities? Or should we stop only for missing newly added capabilities
> (e.g. LMCE) in order to keep backwards compatibility?

Ideally, yes. But in practice we need to check if we won't break
existing setups that were working. If all kernel versions we care
about always MCG_CTL_P|MCG_SER_P + 10 banks as supported, we can
make all bits mandatory.

I need to re-read the thread were kvm_get_mce_cap_supported() was
discussed, to refresh my memory.

-- 
Eduardo

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Eduardo Habkost]

On Fri, Jun 17, 2016 at 10:01:05AM +0800, Haozhong Zhang wrote:
> On 06/16/16 14:58, Eduardo Habkost wrote:
> > On Thu, Jun 16, 2016 at 07:40:20PM +0200, Paolo Bonzini wrote:
> > > 
> > > 
> > > On 16/06/2016 19:36, Eduardo Habkost wrote:
> > > >> > 
> > > >> > Eduardo said nice for this part in previous version [1], so we may wait
> > > >> > for his comments?
> > > >> > 
> > > >> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html
> > > > I agree we don't need this check, but I still believe it is a
> > > > nice thing to have.
> > > > 
> > > > In addition to detecting user errors, they don't hurt and are
> > > > useful for things like "-cpu host", that don't guarantee
> > > > live-migration compatibility but still allow migration if you
> > > > ensure host capabilities are the same on both sides.
> > > 
> > > On the other hand we don't check for this on any other property, either
> > > CPU or device, do we?  Considering "lmce=on" always breaks on an old
> > > kernel (i.e. there's no need for an explicit ",enforce" on the -cpu
> > > flag), I think it's unnecessary and makes things inconsistent.
> > 
> > We don't check that because we normally can't: we usually don't
> > send any configuration data (or anything that could be used to
> > detect configuration mismatches) to the destination. When we do,
> > it's often by accident.
> > 
> > In this case, it looks like we never needed to send mcg_cap in
> > the migration stream. But we already send it, so let's use it for
> > something useful.
> > 
> > I believe we should have more checks like these, when possible. I
> > have been planning for a while to send CPUID data in the
> > migration stream, to detect migration compatibility errors
> > (either user errors or QEMU bugs).
> > 
> > In theory, those checks should never be necessary. In practice I
> > believe they would be very useful.
> >
> 
> Hi Eduardo and Paolo,
> 
> What will be the conclusion? Do we still need this check?
> 
> I'm fine to remove this check if we normally didn't make such kind of
> checks and require users to avoid configuration mismatch.

I don't know yet if Paolo is convinced that the check is still
useful. :)

I suggest doing it as a separate patch, so we can apply the rest
of the series now and discuss/apply the check later.

> 
> > > 
> > > > (I was going to suggest enabling lmce automatically on "-cpu
> > > > host" as a follow-up patch, BTW.)
> > > 
> > > Interesting.  Technically it comes from the host kernel, not from the
> > > host CPU.  But it does sounds like a good idea; -cpu host pretty much
> > > implies the same kernel (in addition to the same processor) on both
> > > sides of the migration.
> > 
> > "-cpu host" already means "whatever is allowed by the host [CPU
> > and/or kernel]", not just "host CPU". It enables x2apic on all
> > hosts, for example.
> >
> 
> Does that mean we can automatically enable LMCE for "-cpu host"?

We can automatically enable LMCE for "-cpu host" if and only if
the host kernel supports LMCE.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Eduardo Habkost]

On Fri, Jun 17, 2016 at 10:01:05AM +0800, Haozhong Zhang wrote:
> On 06/16/16 14:58, Eduardo Habkost wrote:
> > On Thu, Jun 16, 2016 at 07:40:20PM +0200, Paolo Bonzini wrote:
> > > 
> > > 
> > > On 16/06/2016 19:36, Eduardo Habkost wrote:
> > > >> > 
> > > >> > Eduardo said nice for this part in previous version [1], so we may wait
> > > >> > for his comments?
> > > >> > 
> > > >> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html
> > > > I agree we don't need this check, but I still believe it is a
> > > > nice thing to have.
> > > > 
> > > > In addition to detecting user errors, they don't hurt and are
> > > > useful for things like "-cpu host", that don't guarantee
> > > > live-migration compatibility but still allow migration if you
> > > > ensure host capabilities are the same on both sides.
> > > 
> > > On the other hand we don't check for this on any other property, either
> > > CPU or device, do we?  Considering "lmce=on" always breaks on an old
> > > kernel (i.e. there's no need for an explicit ",enforce" on the -cpu
> > > flag), I think it's unnecessary and makes things inconsistent.
> > 
> > We don't check that because we normally can't: we usually don't
> > send any configuration data (or anything that could be used to
> > detect configuration mismatches) to the destination. When we do,
> > it's often by accident.
> > 
> > In this case, it looks like we never needed to send mcg_cap in
> > the migration stream. But we already send it, so let's use it for
> > something useful.
> > 
> > I believe we should have more checks like these, when possible. I
> > have been planning for a while to send CPUID data in the
> > migration stream, to detect migration compatibility errors
> > (either user errors or QEMU bugs).
> > 
> > In theory, those checks should never be necessary. In practice I
> > believe they would be very useful.
> >
> 
> Hi Eduardo and Paolo,
> 
> What will be the conclusion? Do we still need this check?
> 
> I'm fine to remove this check if we normally didn't make such kind of
> checks and require users to avoid configuration mismatch.

I don't know yet if Paolo is convinced that the check is still
useful. :)

I suggest doing it as a separate patch, so we can apply the rest
of the series now and discuss/apply the check later.

> 
> > > 
> > > > (I was going to suggest enabling lmce automatically on "-cpu
> > > > host" as a follow-up patch, BTW.)
> > > 
> > > Interesting.  Technically it comes from the host kernel, not from the
> > > host CPU.  But it does sounds like a good idea; -cpu host pretty much
> > > implies the same kernel (in addition to the same processor) on both
> > > sides of the migration.
> > 
> > "-cpu host" already means "whatever is allowed by the host [CPU
> > and/or kernel]", not just "host CPU". It enables x2apic on all
> > hosts, for example.
> >
> 
> Does that mean we can automatically enable LMCE for "-cpu host"?

We can automatically enable LMCE for "-cpu host" if and only if
the host kernel supports LMCE.

-- 
Eduardo

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Paolo Bonzini]

On 17/06/2016 19:20, Eduardo Habkost wrote:
>> > 
>> > What will be the conclusion? Do we still need this check?
>> > 
>> > I'm fine to remove this check if we normally didn't make such kind of
>> > checks and require users to avoid configuration mismatch.
> 
> I don't know yet if Paolo is convinced that the check is still
> useful. :)

I'm not. :)

> > Does that mean we can automatically enable LMCE for "-cpu host"?
>
> We can automatically enable LMCE for "-cpu host" if and only if
> the host kernel supports LMCE.

Yes, I agree here.  It's a start.

Paolo

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Paolo Bonzini]

On 17/06/2016 19:20, Eduardo Habkost wrote:
>> > 
>> > What will be the conclusion? Do we still need this check?
>> > 
>> > I'm fine to remove this check if we normally didn't make such kind of
>> > checks and require users to avoid configuration mismatch.
> 
> I don't know yet if Paolo is convinced that the check is still
> useful. :)

I'm not. :)

> > Does that mean we can automatically enable LMCE for "-cpu host"?
>
> We can automatically enable LMCE for "-cpu host" if and only if
> the host kernel supports LMCE.

Yes, I agree here.  It's a start.

Paolo

Re: [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Laszlo Ersek]

Hi Haozhong,

On 06/16/16 13:19, Haozhong Zhang wrote:
> On 06/16/16 11:52, Paolo Bonzini wrote:
>>
>>
>> On 16/06/2016 08:06, Haozhong Zhang wrote:
>>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
>>> be set before some features (e.g. VMX and LMCE) can be used, which is
>>> usually done by the firmware. This patch adds a fw_cfg file
>>> "etc/msr_feature_control" which contains the advised value of
>>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
>>>
>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>> ---
>>>  hw/i386/pc.c      | 28 ++++++++++++++++++++++++++++
>>>  target-i386/cpu.h |  4 ++++
>>>  2 files changed, 32 insertions(+)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 7198ed5..d8178a5 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -1147,6 +1147,33 @@ void pc_cpus_init(PCMachineState *pcms)
>>>      smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
>>>  }
>>>  
>>> +static void pc_build_feature_control_file(PCMachineState *pcms)
>>> +{
>>> +    X86CPU *cpu = X86_CPU(pcms->possible_cpus->cpus[0].cpu);
>>> +    CPUX86State *env = &cpu->env;
>>> +    uint32_t unused, ecx, edx, feature_control_bits = 0;
>>> +    uint32_t *val;
>>> +
>>> +    cpu_x86_cpuid(env, 1, 0, &unused, &unused, &ecx, &edx);
>>> +    if (ecx & CPUID_EXT_VMX) {
>>> +        feature_control_bits |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
>>> +    }
>>> +
>>> +    if ((edx & (CPUID_EXT2_MCE | CPUID_EXT2_MCA)) ==
>>> +        (CPUID_EXT2_MCE | CPUID_EXT2_MCA) &&
>>> +        (env->mcg_cap & MCG_LMCE_P)) {
>>> +        feature_control_bits |= FEATURE_CONTROL_LMCE;
>>> +    }
>>> +
>>> +    if (!feature_control_bits) {
>>> +        return;
>>> +    }
>>> +
>>> +    val = g_malloc(sizeof(*val));
>>> +    *val = feature_control_bits | FEATURE_CONTROL_LOCKED;
>>> +    fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
>>> +}
>>> +
>>>  static
>>>  void pc_machine_done(Notifier *notifier, void *data)
>>>  {
>>> @@ -1174,6 +1201,7 @@ void pc_machine_done(Notifier *notifier, void *data)
>>>      acpi_setup();
>>>      if (pcms->fw_cfg) {
>>>          pc_build_smbios(pcms->fw_cfg);
>>> +        pc_build_feature_control_file(pcms);
>>>      }
>>>  }
>>>  
>>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>>> index f0cb04f..5e07c7a 100644
>>> --- a/target-i386/cpu.h
>>> +++ b/target-i386/cpu.h
>>> @@ -332,6 +332,10 @@
>>>  #define MSR_TSC_ADJUST                  0x0000003b
>>>  #define MSR_IA32_TSCDEADLINE            0x6e0
>>>  
>>> +#define FEATURE_CONTROL_LOCKED                    (1<<0)
>>> +#define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX (1<<2)
>>> +#define FEATURE_CONTROL_LMCE                      (1<<20)
>>> +
>>>  #define MSR_P6_PERFCTR0                 0xc1
>>>  
>>>  #define MSR_IA32_SMBASE                 0x9e
>>>
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Have you prepared a patch for SeaBIOS already?
> 
> Yes, I'll send it after I fix the type error (uint32_t => uint64_t) in
> next version.

This should be supported by OVMF as well (thanks Paolo for the heads-up).

I'm glad to code that up, but I'd like to ask you to file an RFE
(Request For Enhancement) in the upstream edk2 tracker for OVMF:

https://github.com/tianocore/edk2/issues/new

In the RFE,
- the subject line should include "OvmfPkg",
- please describe what the feature is good for (going into the details
  of a specific use case is welcome),
- please specify the pathname and the internal format of the fw_cfg
  file,
- please clarify if this MSR is considered part of the "chipset state"
  that the firmware is responsible for at *every* boot. In particular
  whether you expect that the firmware program this MSR at S3 resume as
  well.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Laszlo Ersek]

Hi Haozhong,

On 06/16/16 13:19, Haozhong Zhang wrote:
> On 06/16/16 11:52, Paolo Bonzini wrote:
>>
>>
>> On 16/06/2016 08:06, Haozhong Zhang wrote:
>>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
>>> be set before some features (e.g. VMX and LMCE) can be used, which is
>>> usually done by the firmware. This patch adds a fw_cfg file
>>> "etc/msr_feature_control" which contains the advised value of
>>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
>>>
>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>> ---
>>>  hw/i386/pc.c      | 28 ++++++++++++++++++++++++++++
>>>  target-i386/cpu.h |  4 ++++
>>>  2 files changed, 32 insertions(+)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 7198ed5..d8178a5 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -1147,6 +1147,33 @@ void pc_cpus_init(PCMachineState *pcms)
>>>      smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
>>>  }
>>>  
>>> +static void pc_build_feature_control_file(PCMachineState *pcms)
>>> +{
>>> +    X86CPU *cpu = X86_CPU(pcms->possible_cpus->cpus[0].cpu);
>>> +    CPUX86State *env = &cpu->env;
>>> +    uint32_t unused, ecx, edx, feature_control_bits = 0;
>>> +    uint32_t *val;
>>> +
>>> +    cpu_x86_cpuid(env, 1, 0, &unused, &unused, &ecx, &edx);
>>> +    if (ecx & CPUID_EXT_VMX) {
>>> +        feature_control_bits |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
>>> +    }
>>> +
>>> +    if ((edx & (CPUID_EXT2_MCE | CPUID_EXT2_MCA)) ==
>>> +        (CPUID_EXT2_MCE | CPUID_EXT2_MCA) &&
>>> +        (env->mcg_cap & MCG_LMCE_P)) {
>>> +        feature_control_bits |= FEATURE_CONTROL_LMCE;
>>> +    }
>>> +
>>> +    if (!feature_control_bits) {
>>> +        return;
>>> +    }
>>> +
>>> +    val = g_malloc(sizeof(*val));
>>> +    *val = feature_control_bits | FEATURE_CONTROL_LOCKED;
>>> +    fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
>>> +}
>>> +
>>>  static
>>>  void pc_machine_done(Notifier *notifier, void *data)
>>>  {
>>> @@ -1174,6 +1201,7 @@ void pc_machine_done(Notifier *notifier, void *data)
>>>      acpi_setup();
>>>      if (pcms->fw_cfg) {
>>>          pc_build_smbios(pcms->fw_cfg);
>>> +        pc_build_feature_control_file(pcms);
>>>      }
>>>  }
>>>  
>>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>>> index f0cb04f..5e07c7a 100644
>>> --- a/target-i386/cpu.h
>>> +++ b/target-i386/cpu.h
>>> @@ -332,6 +332,10 @@
>>>  #define MSR_TSC_ADJUST                  0x0000003b
>>>  #define MSR_IA32_TSCDEADLINE            0x6e0
>>>  
>>> +#define FEATURE_CONTROL_LOCKED                    (1<<0)
>>> +#define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX (1<<2)
>>> +#define FEATURE_CONTROL_LMCE                      (1<<20)
>>> +
>>>  #define MSR_P6_PERFCTR0                 0xc1
>>>  
>>>  #define MSR_IA32_SMBASE                 0x9e
>>>
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Have you prepared a patch for SeaBIOS already?
> 
> Yes, I'll send it after I fix the type error (uint32_t => uint64_t) in
> next version.

This should be supported by OVMF as well (thanks Paolo for the heads-up).

I'm glad to code that up, but I'd like to ask you to file an RFE
(Request For Enhancement) in the upstream edk2 tracker for OVMF:

https://github.com/tianocore/edk2/issues/new

In the RFE,
- the subject line should include "OvmfPkg",
- please describe what the feature is good for (going into the details
  of a specific use case is welcome),
- please specify the pathname and the internal format of the fw_cfg
  file,
- please clarify if this MSR is considered part of the "chipset state"
  that the firmware is responsible for at *every* boot. In particular
  whether you expect that the firmware program this MSR at S3 resume as
  well.

Thanks!
Laszlo

Re: [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Raj, Ashok]

On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek wrote:
> >>
> >> On 16/06/2016 08:06, Haozhong Zhang wrote:
> >>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
> >>> be set before some features (e.g. VMX and LMCE) can be used, which is
> >>> usually done by the firmware. This patch adds a fw_cfg file
> >>> "etc/msr_feature_control" which contains the advised value of
> >>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
> >>>

I'm sorry i'm joining this discussion a bit late returning from vacation. 
In a real platform supporting LMCE, BIOS is responsible for setting the bits 
for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the 
BIOS to play this role.

in a virtualized environment, do we really have to push the same requirement
or would it suffice to just emulate it as we did in the early patches.

Not sure what exact problem is created by just simply supporting it within
kvm/qemu and not needing the bios for the guest to also adapt these changes.

Cheers,
Ashok

Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Raj, Ashok]

On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek wrote:
> >>
> >> On 16/06/2016 08:06, Haozhong Zhang wrote:
> >>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
> >>> be set before some features (e.g. VMX and LMCE) can be used, which is
> >>> usually done by the firmware. This patch adds a fw_cfg file
> >>> "etc/msr_feature_control" which contains the advised value of
> >>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
> >>>

I'm sorry i'm joining this discussion a bit late returning from vacation. 
In a real platform supporting LMCE, BIOS is responsible for setting the bits 
for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the 
BIOS to play this role.

in a virtualized environment, do we really have to push the same requirement
or would it suffice to just emulate it as we did in the early patches.

Not sure what exact problem is created by just simply supporting it within
kvm/qemu and not needing the bios for the guest to also adapt these changes.

Cheers,
Ashok

Re: [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Laszlo Ersek]

On 06/17/16 22:21, Raj, Ashok wrote:
> On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek wrote:
>>>>
>>>> On 16/06/2016 08:06, Haozhong Zhang wrote:
>>>>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
>>>>> be set before some features (e.g. VMX and LMCE) can be used, which is
>>>>> usually done by the firmware. This patch adds a fw_cfg file
>>>>> "etc/msr_feature_control" which contains the advised value of
>>>>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
>>>>>
> 
> I'm sorry i'm joining this discussion a bit late returning from vacation. 
> In a real platform supporting LMCE, BIOS is responsible for setting the bits 
> for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the 
> BIOS to play this role.
> 
> in a virtualized environment, do we really have to push the same requirement
> or would it suffice to just emulate it as we did in the early patches.
> 
> Not sure what exact problem is created by just simply supporting it within
> kvm/qemu and not needing the bios for the guest to also adapt these changes.

At the moment, my understanding of this feature is superficial, but the
mechanisms involved in it don't seem complex. I don't expect
difficulties implementing it, I just need the details that I asked for
spelled out for me.

As to why we should be doing this in the guest firmware(s) -- "because
that's what happens on physical machines too" :) Following the phys
world to the letter in virt is not always a goal, but it's never wrong.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Laszlo Ersek]

On 06/17/16 22:21, Raj, Ashok wrote:
> On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek wrote:
>>>>
>>>> On 16/06/2016 08:06, Haozhong Zhang wrote:
>>>>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
>>>>> be set before some features (e.g. VMX and LMCE) can be used, which is
>>>>> usually done by the firmware. This patch adds a fw_cfg file
>>>>> "etc/msr_feature_control" which contains the advised value of
>>>>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
>>>>>
> 
> I'm sorry i'm joining this discussion a bit late returning from vacation. 
> In a real platform supporting LMCE, BIOS is responsible for setting the bits 
> for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the 
> BIOS to play this role.
> 
> in a virtualized environment, do we really have to push the same requirement
> or would it suffice to just emulate it as we did in the early patches.
> 
> Not sure what exact problem is created by just simply supporting it within
> kvm/qemu and not needing the bios for the guest to also adapt these changes.

At the moment, my understanding of this feature is superficial, but the
mechanisms involved in it don't seem complex. I don't expect
difficulties implementing it, I just need the details that I asked for
spelled out for me.

As to why we should be doing this in the guest firmware(s) -- "because
that's what happens on physical machines too" :) Following the phys
world to the letter in virt is not always a goal, but it's never wrong.

Thanks
Laszlo

Re: [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Raj, Ashok]

On Fri, Jun 17, 2016 at 10:48:17PM +0200, Laszlo Ersek wrote:
> On 06/17/16 22:21, Raj, Ashok wrote:
> > On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek wrote:
> >>>>
> >>>> On 16/06/2016 08:06, Haozhong Zhang wrote:
> >>>>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
> >>>>> be set before some features (e.g. VMX and LMCE) can be used, which is
> >>>>> usually done by the firmware. This patch adds a fw_cfg file
> >>>>> "etc/msr_feature_control" which contains the advised value of
> >>>>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
> >>>>>
> > 
> > I'm sorry i'm joining this discussion a bit late returning from vacation. 
> > In a real platform supporting LMCE, BIOS is responsible for setting the bits 
> > for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the 
> > BIOS to play this role.
> > 
> > in a virtualized environment, do we really have to push the same requirement
> > or would it suffice to just emulate it as we did in the early patches.
> > 
> > Not sure what exact problem is created by just simply supporting it within
> > kvm/qemu and not needing the bios for the guest to also adapt these changes.
> 
> At the moment, my understanding of this feature is superficial, but the
> mechanisms involved in it don't seem complex. I don't expect
> difficulties implementing it, I just need the details that I asked for
> spelled out for me.
> 
> As to why we should be doing this in the guest firmware(s) -- "because
> that's what happens on physical machines too" :) Following the phys
> world to the letter in virt is not always a goal, but it's never wrong.

But the guest bios does nothing like the BIOS in the real platform.

for e.g. a real bios would have SMM handlers to work for implementing firmware
first mechanisms before notifying the OS. None of these exist in the 
virtalized world.

Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Raj, Ashok]

On Fri, Jun 17, 2016 at 10:48:17PM +0200, Laszlo Ersek wrote:
> On 06/17/16 22:21, Raj, Ashok wrote:
> > On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek wrote:
> >>>>
> >>>> On 16/06/2016 08:06, Haozhong Zhang wrote:
> >>>>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
> >>>>> be set before some features (e.g. VMX and LMCE) can be used, which is
> >>>>> usually done by the firmware. This patch adds a fw_cfg file
> >>>>> "etc/msr_feature_control" which contains the advised value of
> >>>>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
> >>>>>
> > 
> > I'm sorry i'm joining this discussion a bit late returning from vacation. 
> > In a real platform supporting LMCE, BIOS is responsible for setting the bits 
> > for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the 
> > BIOS to play this role.
> > 
> > in a virtualized environment, do we really have to push the same requirement
> > or would it suffice to just emulate it as we did in the early patches.
> > 
> > Not sure what exact problem is created by just simply supporting it within
> > kvm/qemu and not needing the bios for the guest to also adapt these changes.
> 
> At the moment, my understanding of this feature is superficial, but the
> mechanisms involved in it don't seem complex. I don't expect
> difficulties implementing it, I just need the details that I asked for
> spelled out for me.
> 
> As to why we should be doing this in the guest firmware(s) -- "because
> that's what happens on physical machines too" :) Following the phys
> world to the letter in virt is not always a goal, but it's never wrong.

But the guest bios does nothing like the BIOS in the real platform.

for e.g. a real bios would have SMM handlers to work for implementing firmware
first mechanisms before notifying the OS. None of these exist in the 
virtalized world.

Re: [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Laszlo Ersek]

On 06/17/16 22:55, Raj, Ashok wrote:
> On Fri, Jun 17, 2016 at 10:48:17PM +0200, Laszlo Ersek wrote:
>> On 06/17/16 22:21, Raj, Ashok wrote:
>>> On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek wrote:
>>>>>>
>>>>>> On 16/06/2016 08:06, Haozhong Zhang wrote:
>>>>>>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
>>>>>>> be set before some features (e.g. VMX and LMCE) can be used, which is
>>>>>>> usually done by the firmware. This patch adds a fw_cfg file
>>>>>>> "etc/msr_feature_control" which contains the advised value of
>>>>>>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
>>>>>>>
>>>
>>> I'm sorry i'm joining this discussion a bit late returning from vacation. 
>>> In a real platform supporting LMCE, BIOS is responsible for setting the bits 
>>> for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the 
>>> BIOS to play this role.
>>>
>>> in a virtualized environment, do we really have to push the same requirement
>>> or would it suffice to just emulate it as we did in the early patches.
>>>
>>> Not sure what exact problem is created by just simply supporting it within
>>> kvm/qemu and not needing the bios for the guest to also adapt these changes.
>>
>> At the moment, my understanding of this feature is superficial, but the
>> mechanisms involved in it don't seem complex. I don't expect
>> difficulties implementing it, I just need the details that I asked for
>> spelled out for me.
>>
>> As to why we should be doing this in the guest firmware(s) -- "because
>> that's what happens on physical machines too" :) Following the phys
>> world to the letter in virt is not always a goal, but it's never wrong.
> 
> But the guest bios does nothing like the BIOS in the real platform.

That's overstated. The guest firmwares do a lot of things they also do
on physical hardware. PCI enumeration / resource assignment, for example.

> for e.g. a real bios would have SMM handlers to work for implementing firmware
> first mechanisms before notifying the OS. None of these exist in the 
> virtalized world.

Both SeaBIOS and OVMF utilize SMM, for various purposes.

OVMF's goals with SMM are briefly documented here:
<https://github.com/tianocore/edk2/blob/master/OvmfPkg/README#L121>.

For SMM support, all of KVM, QEMU, and OVMF needed (many) patches.


Anyway, I'm neutral on this. If the consensus is that the MSR at hand is
none of the guest firmware's business, I won't object -- hey, it's only
less work for me. OTOH, if the consensus is that SeaBIOS should be aware
of the MSR, then it follows that so should OVMF.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Laszlo Ersek]

On 06/17/16 22:55, Raj, Ashok wrote:
> On Fri, Jun 17, 2016 at 10:48:17PM +0200, Laszlo Ersek wrote:
>> On 06/17/16 22:21, Raj, Ashok wrote:
>>> On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek wrote:
>>>>>>
>>>>>> On 16/06/2016 08:06, Haozhong Zhang wrote:
>>>>>>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
>>>>>>> be set before some features (e.g. VMX and LMCE) can be used, which is
>>>>>>> usually done by the firmware. This patch adds a fw_cfg file
>>>>>>> "etc/msr_feature_control" which contains the advised value of
>>>>>>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
>>>>>>>
>>>
>>> I'm sorry i'm joining this discussion a bit late returning from vacation. 
>>> In a real platform supporting LMCE, BIOS is responsible for setting the bits 
>>> for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the 
>>> BIOS to play this role.
>>>
>>> in a virtualized environment, do we really have to push the same requirement
>>> or would it suffice to just emulate it as we did in the early patches.
>>>
>>> Not sure what exact problem is created by just simply supporting it within
>>> kvm/qemu and not needing the bios for the guest to also adapt these changes.
>>
>> At the moment, my understanding of this feature is superficial, but the
>> mechanisms involved in it don't seem complex. I don't expect
>> difficulties implementing it, I just need the details that I asked for
>> spelled out for me.
>>
>> As to why we should be doing this in the guest firmware(s) -- "because
>> that's what happens on physical machines too" :) Following the phys
>> world to the letter in virt is not always a goal, but it's never wrong.
> 
> But the guest bios does nothing like the BIOS in the real platform.

That's overstated. The guest firmwares do a lot of things they also do
on physical hardware. PCI enumeration / resource assignment, for example.

> for e.g. a real bios would have SMM handlers to work for implementing firmware
> first mechanisms before notifying the OS. None of these exist in the 
> virtalized world.

Both SeaBIOS and OVMF utilize SMM, for various purposes.

OVMF's goals with SMM are briefly documented here:
<https://github.com/tianocore/edk2/blob/master/OvmfPkg/README#L121>.

For SMM support, all of KVM, QEMU, and OVMF needed (many) patches.


Anyway, I'm neutral on this. If the consensus is that the MSR at hand is
none of the guest firmware's business, I won't object -- hey, it's only
less work for me. OTOH, if the consensus is that SeaBIOS should be aware
of the MSR, then it follows that so should OVMF.

Thanks
Laszlo

Re: [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Haozhong Zhang]

On 06/17/16 13:20, Eduardo Habkost wrote:
> On Fri, Jun 17, 2016 at 09:26:57AM +0800, Haozhong Zhang wrote:
> [...]
> > > >  static void mce_init(X86CPU *cpu)
> > > >  {
> > > >      CPUX86State *cenv = &cpu->env;
> > > >      unsigned int bank;
> > > > +    Error *local_err = NULL;
> > > >  
> > > >      if (((cenv->cpuid_version >> 8) & 0xf) >= 6
> > > >          && (cenv->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
> > > >              (CPUID_MCE | CPUID_MCA)) {
> > > >          cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
> > > > +
> > > > +        if (cpu->enable_lmce) {
> > > > +            if (!lmce_supported()) {
> > > > +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> > > > +                error_propagate(&error_abort, local_err);
> > > > +            }
> > > > +            cenv->mcg_cap |= MCG_LMCE_P;
> > > > +        }
> > > > +
> > > 
> > > This duplicates the existing check in kvm_arch_init_vcpu(). The
> > > difference is that the existing code is KVM-specific and doesn't
> > > stop initialization when capabilities are missing. We can unify
> > > them into a single mcg_cap-checking function as a follow-up.
> > >
> > 
> > If I reuse the existing MCE capability check in kvm_arch_init_vcpu(),
> > is it reasonable to make change to stop initialization if missing
> > capabilities? Or should we stop only for missing newly added capabilities
> > (e.g. LMCE) in order to keep backwards compatibility?
> 
> Ideally, yes. But in practice we need to check if we won't break
> existing setups that were working. If all kernel versions we care
> about always MCG_CTL_P|MCG_SER_P + 10 banks as supported, we can
> make all bits mandatory.
>

Let's stop only for LMCE in this patch series. Other bits may be
changed in future after the kernel support is clarified.

Thanks,
Haozhong

> I need to re-read the thread were kvm_get_mce_cap_supported() was
> discussed, to refresh my memory.
>
> -- 
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Haozhong Zhang]

On 06/17/16 13:20, Eduardo Habkost wrote:
> On Fri, Jun 17, 2016 at 09:26:57AM +0800, Haozhong Zhang wrote:
> [...]
> > > >  static void mce_init(X86CPU *cpu)
> > > >  {
> > > >      CPUX86State *cenv = &cpu->env;
> > > >      unsigned int bank;
> > > > +    Error *local_err = NULL;
> > > >  
> > > >      if (((cenv->cpuid_version >> 8) & 0xf) >= 6
> > > >          && (cenv->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
> > > >              (CPUID_MCE | CPUID_MCA)) {
> > > >          cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
> > > > +
> > > > +        if (cpu->enable_lmce) {
> > > > +            if (!lmce_supported()) {
> > > > +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> > > > +                error_propagate(&error_abort, local_err);
> > > > +            }
> > > > +            cenv->mcg_cap |= MCG_LMCE_P;
> > > > +        }
> > > > +
> > > 
> > > This duplicates the existing check in kvm_arch_init_vcpu(). The
> > > difference is that the existing code is KVM-specific and doesn't
> > > stop initialization when capabilities are missing. We can unify
> > > them into a single mcg_cap-checking function as a follow-up.
> > >
> > 
> > If I reuse the existing MCE capability check in kvm_arch_init_vcpu(),
> > is it reasonable to make change to stop initialization if missing
> > capabilities? Or should we stop only for missing newly added capabilities
> > (e.g. LMCE) in order to keep backwards compatibility?
> 
> Ideally, yes. But in practice we need to check if we won't break
> existing setups that were working. If all kernel versions we care
> about always MCG_CTL_P|MCG_SER_P + 10 banks as supported, we can
> make all bits mandatory.
>

Let's stop only for LMCE in this patch series. Other bits may be
changed in future after the kernel support is clarified.

Thanks,
Haozhong

> I need to re-read the thread were kvm_get_mce_cap_supported() was
> discussed, to refresh my memory.
>
> -- 
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Haozhong Zhang]

On 06/17/16 14:20, Eduardo Habkost wrote:
> On Fri, Jun 17, 2016 at 10:01:05AM +0800, Haozhong Zhang wrote:
> > On 06/16/16 14:58, Eduardo Habkost wrote:
> > > On Thu, Jun 16, 2016 at 07:40:20PM +0200, Paolo Bonzini wrote:
> > > > 
> > > > 
> > > > On 16/06/2016 19:36, Eduardo Habkost wrote:
> > > > >> > 
> > > > >> > Eduardo said nice for this part in previous version [1], so we may wait
> > > > >> > for his comments?
> > > > >> > 
> > > > >> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html
> > > > > I agree we don't need this check, but I still believe it is a
> > > > > nice thing to have.
> > > > > 
> > > > > In addition to detecting user errors, they don't hurt and are
> > > > > useful for things like "-cpu host", that don't guarantee
> > > > > live-migration compatibility but still allow migration if you
> > > > > ensure host capabilities are the same on both sides.
> > > > 
> > > > On the other hand we don't check for this on any other property, either
> > > > CPU or device, do we?  Considering "lmce=on" always breaks on an old
> > > > kernel (i.e. there's no need for an explicit ",enforce" on the -cpu
> > > > flag), I think it's unnecessary and makes things inconsistent.
> > > 
> > > We don't check that because we normally can't: we usually don't
> > > send any configuration data (or anything that could be used to
> > > detect configuration mismatches) to the destination. When we do,
> > > it's often by accident.
> > > 
> > > In this case, it looks like we never needed to send mcg_cap in
> > > the migration stream. But we already send it, so let's use it for
> > > something useful.
> > > 
> > > I believe we should have more checks like these, when possible. I
> > > have been planning for a while to send CPUID data in the
> > > migration stream, to detect migration compatibility errors
> > > (either user errors or QEMU bugs).
> > > 
> > > In theory, those checks should never be necessary. In practice I
> > > believe they would be very useful.
> > >
> > 
> > Hi Eduardo and Paolo,
> > 
> > What will be the conclusion? Do we still need this check?
> > 
> > I'm fine to remove this check if we normally didn't make such kind of
> > checks and require users to avoid configuration mismatch.
> 
> I don't know yet if Paolo is convinced that the check is still
> useful. :)
> 
> I suggest doing it as a separate patch, so we can apply the rest
> of the series now and discuss/apply the check later.
>

Yes, I'll move the check to a separate patch so that we can easily
drop it if not necessary. Thanks for the suggestion!

> > 
> > > > 
> > > > > (I was going to suggest enabling lmce automatically on "-cpu
> > > > > host" as a follow-up patch, BTW.)
> > > > 
> > > > Interesting.  Technically it comes from the host kernel, not from the
> > > > host CPU.  But it does sounds like a good idea; -cpu host pretty much
> > > > implies the same kernel (in addition to the same processor) on both
> > > > sides of the migration.
> > > 
> > > "-cpu host" already means "whatever is allowed by the host [CPU
> > > and/or kernel]", not just "host CPU". It enables x2apic on all
> > > hosts, for example.
> > >
> > 
> > Does that mean we can automatically enable LMCE for "-cpu host"?
> 
> We can automatically enable LMCE for "-cpu host" if and only if
> the host kernel supports LMCE.
>

According to our discussion for KVM Patch 3, we may have to disable it
by default by -cpu host, so that pc-2.7 will not require new kernels
unless LMCE is required explicitly by users.

Thanks,
Haozhong

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Haozhong Zhang]

On 06/17/16 14:20, Eduardo Habkost wrote:
> On Fri, Jun 17, 2016 at 10:01:05AM +0800, Haozhong Zhang wrote:
> > On 06/16/16 14:58, Eduardo Habkost wrote:
> > > On Thu, Jun 16, 2016 at 07:40:20PM +0200, Paolo Bonzini wrote:
> > > > 
> > > > 
> > > > On 16/06/2016 19:36, Eduardo Habkost wrote:
> > > > >> > 
> > > > >> > Eduardo said nice for this part in previous version [1], so we may wait
> > > > >> > for his comments?
> > > > >> > 
> > > > >> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg01992.html
> > > > > I agree we don't need this check, but I still believe it is a
> > > > > nice thing to have.
> > > > > 
> > > > > In addition to detecting user errors, they don't hurt and are
> > > > > useful for things like "-cpu host", that don't guarantee
> > > > > live-migration compatibility but still allow migration if you
> > > > > ensure host capabilities are the same on both sides.
> > > > 
> > > > On the other hand we don't check for this on any other property, either
> > > > CPU or device, do we?  Considering "lmce=on" always breaks on an old
> > > > kernel (i.e. there's no need for an explicit ",enforce" on the -cpu
> > > > flag), I think it's unnecessary and makes things inconsistent.
> > > 
> > > We don't check that because we normally can't: we usually don't
> > > send any configuration data (or anything that could be used to
> > > detect configuration mismatches) to the destination. When we do,
> > > it's often by accident.
> > > 
> > > In this case, it looks like we never needed to send mcg_cap in
> > > the migration stream. But we already send it, so let's use it for
> > > something useful.
> > > 
> > > I believe we should have more checks like these, when possible. I
> > > have been planning for a while to send CPUID data in the
> > > migration stream, to detect migration compatibility errors
> > > (either user errors or QEMU bugs).
> > > 
> > > In theory, those checks should never be necessary. In practice I
> > > believe they would be very useful.
> > >
> > 
> > Hi Eduardo and Paolo,
> > 
> > What will be the conclusion? Do we still need this check?
> > 
> > I'm fine to remove this check if we normally didn't make such kind of
> > checks and require users to avoid configuration mismatch.
> 
> I don't know yet if Paolo is convinced that the check is still
> useful. :)
> 
> I suggest doing it as a separate patch, so we can apply the rest
> of the series now and discuss/apply the check later.
>

Yes, I'll move the check to a separate patch so that we can easily
drop it if not necessary. Thanks for the suggestion!

> > 
> > > > 
> > > > > (I was going to suggest enabling lmce automatically on "-cpu
> > > > > host" as a follow-up patch, BTW.)
> > > > 
> > > > Interesting.  Technically it comes from the host kernel, not from the
> > > > host CPU.  But it does sounds like a good idea; -cpu host pretty much
> > > > implies the same kernel (in addition to the same processor) on both
> > > > sides of the migration.
> > > 
> > > "-cpu host" already means "whatever is allowed by the host [CPU
> > > and/or kernel]", not just "host CPU". It enables x2apic on all
> > > hosts, for example.
> > >
> > 
> > Does that mean we can automatically enable LMCE for "-cpu host"?
> 
> We can automatically enable LMCE for "-cpu host" if and only if
> the host kernel supports LMCE.
>

According to our discussion for KVM Patch 3, we may have to disable it
by default by -cpu host, so that pc-2.7 will not require new kernels
unless LMCE is required explicitly by users.

Thanks,
Haozhong

Re: [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Haozhong Zhang]

On 06/17/16 13:21, Raj, Ashok wrote:
> On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek wrote:
> > >>
> > >> On 16/06/2016 08:06, Haozhong Zhang wrote:
> > >>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
> > >>> be set before some features (e.g. VMX and LMCE) can be used, which is
> > >>> usually done by the firmware. This patch adds a fw_cfg file
> > >>> "etc/msr_feature_control" which contains the advised value of
> > >>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
> > >>>
> 
> I'm sorry i'm joining this discussion a bit late returning from vacation. 
> In a real platform supporting LMCE, BIOS is responsible for setting the bits 
> for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the 
> BIOS to play this role.
> 
> in a virtualized environment, do we really have to push the same requirement
> or would it suffice to just emulate it as we did in the early patches.
> 
> Not sure what exact problem is created by just simply supporting it within
> kvm/qemu and not needing the bios for the guest to also adapt these changes.
> 

In the current nested VMX implementation in QEMU, setup
MSR_IA32_FEATURE_CONTROL is left to guest. So I think, for LMCE which
is another feature involving MSR_IA32_FEATURE_CONTROL, we may follow
the existing code.

Paolo and Radim, is there any case that objects to setting
MSR_IA32_FEATURE_CONTROL in QEMU?

Thanks,
Haozhong

Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Haozhong Zhang]

On 06/17/16 13:21, Raj, Ashok wrote:
> On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek wrote:
> > >>
> > >> On 16/06/2016 08:06, Haozhong Zhang wrote:
> > >>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
> > >>> be set before some features (e.g. VMX and LMCE) can be used, which is
> > >>> usually done by the firmware. This patch adds a fw_cfg file
> > >>> "etc/msr_feature_control" which contains the advised value of
> > >>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
> > >>>
> 
> I'm sorry i'm joining this discussion a bit late returning from vacation. 
> In a real platform supporting LMCE, BIOS is responsible for setting the bits 
> for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the 
> BIOS to play this role.
> 
> in a virtualized environment, do we really have to push the same requirement
> or would it suffice to just emulate it as we did in the early patches.
> 
> Not sure what exact problem is created by just simply supporting it within
> kvm/qemu and not needing the bios for the guest to also adapt these changes.
> 

In the current nested VMX implementation in QEMU, setup
MSR_IA32_FEATURE_CONTROL is left to guest. So I think, for LMCE which
is another feature involving MSR_IA32_FEATURE_CONTROL, we may follow
the existing code.

Paolo and Radim, is there any case that objects to setting
MSR_IA32_FEATURE_CONTROL in QEMU?

Thanks,
Haozhong

Re: [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Paolo Bonzini]

On 20/06/2016 05:09, Haozhong Zhang wrote:
> In the current nested VMX implementation in QEMU, setup
> MSR_IA32_FEATURE_CONTROL is left to guest. So I think, for LMCE which
> is another feature involving MSR_IA32_FEATURE_CONTROL, we may follow
> the existing code.
> 
> Paolo and Radim, is there any case that objects to setting
> MSR_IA32_FEATURE_CONTROL in QEMU?

If the SDM says that the reset state of the MSR is zero, QEMU should
initialize it to zero.

Paolo

Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Paolo Bonzini]

On 20/06/2016 05:09, Haozhong Zhang wrote:
> In the current nested VMX implementation in QEMU, setup
> MSR_IA32_FEATURE_CONTROL is left to guest. So I think, for LMCE which
> is another feature involving MSR_IA32_FEATURE_CONTROL, we may follow
> the existing code.
> 
> Paolo and Radim, is there any case that objects to setting
> MSR_IA32_FEATURE_CONTROL in QEMU?

If the SDM says that the reset state of the MSR is zero, QEMU should
initialize it to zero.

Paolo

Re: [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Paolo Bonzini]

On 20/06/2016 04:11, Haozhong Zhang wrote:
>>> > > Does that mean we can automatically enable LMCE for "-cpu host"?
>> > 
>> > We can automatically enable LMCE for "-cpu host" if and only if
>> > the host kernel supports LMCE.
>> >
> According to our discussion for KVM Patch 3, we may have to disable it
> by default by -cpu host, so that pc-2.7 will not require new kernels
> unless LMCE is required explicitly by users.

-cpu host is a bit special, it requires the same processor and kernel on
both sides of a migration.  So it can enable LMCE.

Paolo

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Paolo Bonzini]

On 20/06/2016 04:11, Haozhong Zhang wrote:
>>> > > Does that mean we can automatically enable LMCE for "-cpu host"?
>> > 
>> > We can automatically enable LMCE for "-cpu host" if and only if
>> > the host kernel supports LMCE.
>> >
> According to our discussion for KVM Patch 3, we may have to disable it
> by default by -cpu host, so that pc-2.7 will not require new kernels
> unless LMCE is required explicitly by users.

-cpu host is a bit special, it requires the same processor and kernel on
both sides of a migration.  So it can enable LMCE.

Paolo

Re: [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Haozhong Zhang]

On 06/20/16 08:56, Paolo Bonzini wrote:
> 
> 
> On 20/06/2016 05:09, Haozhong Zhang wrote:
> > In the current nested VMX implementation in QEMU, setup
> > MSR_IA32_FEATURE_CONTROL is left to guest. So I think, for LMCE which
> > is another feature involving MSR_IA32_FEATURE_CONTROL, we may follow
> > the existing code.
> > 
> > Paolo and Radim, is there any case that objects to setting
> > MSR_IA32_FEATURE_CONTROL in QEMU?
> 
> If the SDM says that the reset state of the MSR is zero, QEMU should
> initialize it to zero.
> 

Yes, SDM says "This MSR is cleared to zero when a logical processor is
reset" (in section "Enabling and Entering VMX operation"), so we
should do so in qemu as well.

Thanks,
Haozhong

Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Haozhong Zhang]

On 06/20/16 08:56, Paolo Bonzini wrote:
> 
> 
> On 20/06/2016 05:09, Haozhong Zhang wrote:
> > In the current nested VMX implementation in QEMU, setup
> > MSR_IA32_FEATURE_CONTROL is left to guest. So I think, for LMCE which
> > is another feature involving MSR_IA32_FEATURE_CONTROL, we may follow
> > the existing code.
> > 
> > Paolo and Radim, is there any case that objects to setting
> > MSR_IA32_FEATURE_CONTROL in QEMU?
> 
> If the SDM says that the reset state of the MSR is zero, QEMU should
> initialize it to zero.
> 

Yes, SDM says "This MSR is cleared to zero when a logical processor is
reset" (in section "Enabling and Entering VMX operation"), so we
should do so in qemu as well.

Thanks,
Haozhong

Re: [Qemu-devel] [PATCH v4 2/3] target-i386: add migration support for Intel LMCE

[Haozhong Zhang]

On 06/20/16 08:58, Paolo Bonzini wrote:
> 
> 
> On 20/06/2016 04:11, Haozhong Zhang wrote:
> >>> > > Does that mean we can automatically enable LMCE for "-cpu host"?
> >> > 
> >> > We can automatically enable LMCE for "-cpu host" if and only if
> >> > the host kernel supports LMCE.
> >> >
> > According to our discussion for KVM Patch 3, we may have to disable it
> > by default by -cpu host, so that pc-2.7 will not require new kernels
> > unless LMCE is required explicitly by users.
> 
> -cpu host is a bit special, it requires the same processor and kernel on
> both sides of a migration.  So it can enable LMCE.
> 

OK, I'll make a separate patch in the next version to enable LMCE for
-cpu host.

Thanks,
Haozhong

Re: [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Haozhong Zhang]

On 06/17/16 19:31, Laszlo Ersek wrote:
> Hi Haozhong,
> 
> On 06/16/16 13:19, Haozhong Zhang wrote:
> > On 06/16/16 11:52, Paolo Bonzini wrote:
[..]
> >> Have you prepared a patch for SeaBIOS already?
> > 
> > Yes, I'll send it after I fix the type error (uint32_t => uint64_t) in
> > next version.
> 
> This should be supported by OVMF as well (thanks Paolo for the heads-up).
> 
> I'm glad to code that up, but I'd like to ask you to file an RFE
> (Request For Enhancement) in the upstream edk2 tracker for OVMF:
> 
> https://github.com/tianocore/edk2/issues/new
> 
> In the RFE,
> - the subject line should include "OvmfPkg",
> - please describe what the feature is good for (going into the details
>   of a specific use case is welcome),
> - please specify the pathname and the internal format of the fw_cfg
>   file,
> - please clarify if this MSR is considered part of the "chipset state"
>   that the firmware is responsible for at *every* boot. In particular
>   whether you expect that the firmware program this MSR at S3 resume as
>   well.
> 

Done. Pls check https://github.com/tianocore/edk2/issues/97.

Thanks,
Haozhong

Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Haozhong Zhang]

On 06/17/16 19:31, Laszlo Ersek wrote:
> Hi Haozhong,
> 
> On 06/16/16 13:19, Haozhong Zhang wrote:
> > On 06/16/16 11:52, Paolo Bonzini wrote:
[..]
> >> Have you prepared a patch for SeaBIOS already?
> > 
> > Yes, I'll send it after I fix the type error (uint32_t => uint64_t) in
> > next version.
> 
> This should be supported by OVMF as well (thanks Paolo for the heads-up).
> 
> I'm glad to code that up, but I'd like to ask you to file an RFE
> (Request For Enhancement) in the upstream edk2 tracker for OVMF:
> 
> https://github.com/tianocore/edk2/issues/new
> 
> In the RFE,
> - the subject line should include "OvmfPkg",
> - please describe what the feature is good for (going into the details
>   of a specific use case is welcome),
> - please specify the pathname and the internal format of the fw_cfg
>   file,
> - please clarify if this MSR is considered part of the "chipset state"
>   that the firmware is responsible for at *every* boot. In particular
>   whether you expect that the firmware program this MSR at S3 resume as
>   well.
> 

Done. Pls check https://github.com/tianocore/edk2/issues/97.

Thanks,
Haozhong

Re: [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Laszlo Ersek]

On 06/22/16 12:18, Haozhong Zhang wrote:
> On 06/17/16 19:31, Laszlo Ersek wrote:
>> Hi Haozhong,
>>
>> On 06/16/16 13:19, Haozhong Zhang wrote:
>>> On 06/16/16 11:52, Paolo Bonzini wrote:
> [..]
>>>> Have you prepared a patch for SeaBIOS already?
>>>
>>> Yes, I'll send it after I fix the type error (uint32_t => uint64_t) in
>>> next version.
>>
>> This should be supported by OVMF as well (thanks Paolo for the heads-up).
>>
>> I'm glad to code that up, but I'd like to ask you to file an RFE
>> (Request For Enhancement) in the upstream edk2 tracker for OVMF:
>>
>> https://github.com/tianocore/edk2/issues/new
>>
>> In the RFE,
>> - the subject line should include "OvmfPkg",
>> - please describe what the feature is good for (going into the details
>>   of a specific use case is welcome),
>> - please specify the pathname and the internal format of the fw_cfg
>>   file,
>> - please clarify if this MSR is considered part of the "chipset state"
>>   that the firmware is responsible for at *every* boot. In particular
>>   whether you expect that the firmware program this MSR at S3 resume as
>>   well.
>>
> 
> Done. Pls check https://github.com/tianocore/edk2/issues/97.

Thanks. Looks good. I asked a few more questions in there. Especially if
the APs are involved in this, then the feature is going to be
significantly more complex than I expected.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Laszlo Ersek]

On 06/22/16 12:18, Haozhong Zhang wrote:
> On 06/17/16 19:31, Laszlo Ersek wrote:
>> Hi Haozhong,
>>
>> On 06/16/16 13:19, Haozhong Zhang wrote:
>>> On 06/16/16 11:52, Paolo Bonzini wrote:
> [..]
>>>> Have you prepared a patch for SeaBIOS already?
>>>
>>> Yes, I'll send it after I fix the type error (uint32_t => uint64_t) in
>>> next version.
>>
>> This should be supported by OVMF as well (thanks Paolo for the heads-up).
>>
>> I'm glad to code that up, but I'd like to ask you to file an RFE
>> (Request For Enhancement) in the upstream edk2 tracker for OVMF:
>>
>> https://github.com/tianocore/edk2/issues/new
>>
>> In the RFE,
>> - the subject line should include "OvmfPkg",
>> - please describe what the feature is good for (going into the details
>>   of a specific use case is welcome),
>> - please specify the pathname and the internal format of the fw_cfg
>>   file,
>> - please clarify if this MSR is considered part of the "chipset state"
>>   that the firmware is responsible for at *every* boot. In particular
>>   whether you expect that the firmware program this MSR at S3 resume as
>>   well.
>>
> 
> Done. Pls check https://github.com/tianocore/edk2/issues/97.

Thanks. Looks good. I asked a few more questions in there. Especially if
the APs are involved in this, then the feature is going to be
significantly more complex than I expected.

Thanks
Laszlo