qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 0/2] Add QEMU support for Intel local MCE
@ 2016-06-03  6:09 Haozhong Zhang
  2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support Haozhong Zhang
                   ` (2 more replies)
  0 siblings, 3 replies; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-03  6:09 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Boris Petkov,
	Tony Luck, Andi Kleen, Ashok Raj, Haozhong Zhang

This is v3 of LMCE patch series. Previous ones can be found at
 v2: https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg01731.html
 v1: https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg01651.html
 v0: https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg01498.html

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 (sent
via another email with title "[PATCH v1] 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.

Haozhong Zhang (2):
  target-i386: KVM: add basic Intel LMCE support
  target-i386: add migration support for Intel LMCE

 include/hw/i386/pc.h  |  7 ++++++-
 target-i386/cpu.c     | 27 +++++++++++++++++++++++++++
 target-i386/cpu.h     | 18 +++++++++++++++++-
 target-i386/kvm.c     | 35 +++++++++++++++++++++++++++++++----
 target-i386/machine.c | 24 ++++++++++++++++++++++++
 5 files changed, 105 insertions(+), 6 deletions(-)

-- 
2.8.3

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-03  6:09 [Qemu-devel] [PATCH v3 0/2] Add QEMU support for Intel local MCE Haozhong Zhang
@ 2016-06-03  6:09 ` Haozhong Zhang
  2016-06-03 15:57   ` Radim Krčmář
                     ` (3 more replies)
  2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 2/2] target-i386: add migration support for Intel LMCE Haozhong Zhang
  2016-06-03  6:38 ` [Qemu-devel] [PATCH v3 0/2] Add QEMU support for Intel local MCE Haozhong Zhang
  2 siblings, 4 replies; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-03  6:09 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Boris Petkov,
	Tony Luck, Andi Kleen, Ashok Raj, Haozhong Zhang

This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
will be 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.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Boris Petkov <bp@suse.de>
Cc: kvm@vger.kernel.org
Cc: Tony Luck <tony.luck@intel.com>
Cc: Andi Kleen <andi.kleen@intel.com>
---
 target-i386/cpu.c | 26 ++++++++++++++++++++++++++
 target-i386/cpu.h | 13 ++++++++++++-
 target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
 3 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 895a386..9b4dbab 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2777,6 +2777,18 @@ 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;
@@ -2786,6 +2798,20 @@ static void mce_init(X86CPU *cpu)
         && (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()) {
+                cenv->mcg_cap |= MCG_LMCE_P;
+                cenv->msr_ia32_feature_control |=
+                    MSR_IA32_FEATURE_CONTROL_LMCE |
+                    MSR_IA32_FEATURE_CONTROL_LOCKED;
+            } else {
+                error_report("Warning: KVM unavailable or not support LMCE, "
+                             "LMCE disabled");
+                cpu->enable_lmce = false;
+            }
+        }
+
         cenv->mcg_ctl = ~(uint64_t)0;
         for (bank = 0; bank < MCE_BANKS_DEF; bank++) {
             cenv->mce_banks[bank * 4] = ~(uint64_t)0;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 0426459..2d411ba 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 */
@@ -325,6 +329,8 @@
 #define MSR_IA32_APICBASE_ENABLE        (1<<11)
 #define MSR_IA32_APICBASE_BASE          (0xfffffU<<12)
 #define MSR_IA32_FEATURE_CONTROL        0x0000003a
+#define MSR_IA32_FEATURE_CONTROL_LOCKED (1<<0)
+#define MSR_IA32_FEATURE_CONTROL_LMCE   (1<<20)
 #define MSR_TSC_ADJUST                  0x0000003b
 #define MSR_IA32_TSCDEADLINE            0x6e0
 
@@ -343,6 +349,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
 
@@ -1011,7 +1018,6 @@ typedef struct CPUX86State {
 
     uint64_t mcg_status;
     uint64_t msr_ia32_misc_enable;
-    uint64_t msr_ia32_feature_control;
 
     uint64_t msr_fixed_ctr_ctrl;
     uint64_t msr_global_ctrl;
@@ -1104,8 +1110,11 @@ typedef struct CPUX86State {
     int64_t user_tsc_khz; /* for sanity check only */
     void *kvm_xsave_buf;
 
+    uint64_t msr_ia32_feature_control;
+
     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 +1182,8 @@ struct X86CPU {
      */
     bool enable_pmu;
 
+    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.8.3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [Qemu-devel] [PATCH v3 2/2] target-i386: add migration support for Intel LMCE
  2016-06-03  6:09 [Qemu-devel] [PATCH v3 0/2] Add QEMU support for Intel local MCE Haozhong Zhang
  2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support Haozhong Zhang
@ 2016-06-03  6:09 ` Haozhong Zhang
  2016-06-07 20:18   ` Eduardo Habkost
  2016-06-08 11:36   ` Paolo Bonzini
  2016-06-03  6:38 ` [Qemu-devel] [PATCH v3 0/2] Add QEMU support for Intel local MCE Haozhong Zhang
  2 siblings, 2 replies; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-03  6:09 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Boris Petkov,
	Tony Luck, Andi Kleen, Ashok Raj, Haozhong Zhang

LMCE is disabled by default, but a cpu option 'lmce=on/off' is provided
to enable/disable it. Migration is only allowed between VCPUs with the
same lmce option.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Boris Petkov <bp@suse.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Andi Kleen <andi.kleen@intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
---
 include/hw/i386/pc.h  |  7 ++++++-
 target-i386/cpu.c     |  1 +
 target-i386/cpu.h     |  5 +++++
 target-i386/machine.c | 24 ++++++++++++++++++++++++
 4 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ca23609..058eef9 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -357,7 +357,12 @@ int e820_get_num_entries(void);
 bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
 #define PC_COMPAT_2_6 \
-    HW_COMPAT_2_6
+    HW_COMPAT_2_6 \
+    {\
+        .driver   = TYPE_X86_CPU,\
+        .property = "lmce",\
+        .value    = "off",\
+    },
 
 #define PC_COMPAT_2_5 \
     PC_COMPAT_2_6 \
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9b4dbab..c69cc17 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3232,6 +3232,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 2d411ba..b512fd6 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1182,6 +1182,11 @@ struct X86CPU {
      */
     bool enable_pmu;
 
+    /* Enable LMCE support which is set via cpu option 'lmce=on/off'. LMCE is
+     * disabled by default to avoid breaking the migration between QEMU with
+     * different LMCE support. Only migrating between QEMU with the same LMCE
+     * support is allowed.
+     */
     bool enable_lmce;
 
     /* in order to simplify APIC support, we leave this pointer to the
diff --git a/target-i386/machine.c b/target-i386/machine.c
index cb9adf2..b55d376 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -347,6 +347,11 @@ static int cpu_post_load(void *opaque, int version_id)
         return -EINVAL;
     }
 
+    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
+        error_report("LMCE not enabled");
+        return -EINVAL;
+    }
+
     /*
      * Real mode guest segments register DPL should be zero.
      * Older KVM version were setting it wrongly.
@@ -896,6 +901,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 +1045,7 @@ VMStateDescription vmstate_x86_cpu = {
 #ifdef TARGET_X86_64
         &vmstate_pkru,
 #endif
+        &vmstate_mcg_ext_ctl,
         NULL
     }
 };
-- 
2.8.3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/2] Add QEMU support for Intel local MCE
  2016-06-03  6:09 [Qemu-devel] [PATCH v3 0/2] Add QEMU support for Intel local MCE Haozhong Zhang
  2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support Haozhong Zhang
  2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 2/2] target-i386: add migration support for Intel LMCE Haozhong Zhang
@ 2016-06-03  6:38 ` Haozhong Zhang
  2 siblings, 0 replies; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-03  6:38 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Boris Petkov,
	Tony Luck, Andi Kleen, Ashok Raj

On 06/03/16 14:09, Haozhong Zhang wrote:
> This is v3 of LMCE patch series. Previous ones can be found at
>  v2: https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg01731.html
>  v1: https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg01651.html
>  v0: https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg01498.html
> 
> 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 (sent
> via another email with title "[PATCH v1] 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.
> 
> Haozhong Zhang (2):
>   target-i386: KVM: add basic Intel LMCE support
>   target-i386: add migration support for Intel LMCE

Forgot to note: Igor's patch "pc: add 2.7 machine" should be applied
before patch 2 "target-i386: add migration support for Intel LMCE".

Haozhong

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support Haozhong Zhang
@ 2016-06-03 15:57   ` Radim Krčmář
  2016-06-05 15:32     ` Haozhong Zhang
  2016-06-08 11:32     ` Paolo Bonzini
  2016-06-04 10:15   ` Boris Petkov
                     ` (2 subsequent siblings)
  3 siblings, 2 replies; 28+ messages in thread
From: Radim Krčmář @ 2016-06-03 15:57 UTC (permalink / raw)
  To: Haozhong Zhang
  Cc: qemu-devel, Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Boris Petkov,
	Tony Luck, Andi Kleen, Ashok Raj

2016-06-03 14:09+0800, Haozhong Zhang:
> This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> will be 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.
> 
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> @@ -2786,6 +2798,20 @@ static void mce_init(X86CPU *cpu)
>          && (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()) {
> +                cenv->mcg_cap |= MCG_LMCE_P;
> +                cenv->msr_ia32_feature_control |=
> +                    MSR_IA32_FEATURE_CONTROL_LMCE |
> +                    MSR_IA32_FEATURE_CONTROL_LOCKED;

Locking right from the start breaks nested KVM, because nested relies on
setting VMXON feature from inside of the guest.

Do we keep it unlocked, or move everything into QEMU?

(The latter seems simpler.)

> +            } else {
> +                error_report("Warning: KVM unavailable or not support LMCE, "
> +                             "LMCE disabled");
> +                cpu->enable_lmce = false;
> +            }
> +        }
> +
>          cenv->mcg_ctl = ~(uint64_t)0;
>          for (bank = 0; bank < MCE_BANKS_DEF; bank++) {
>              cenv->mce_banks[bank * 4] = ~(uint64_t)0;

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support Haozhong Zhang
  2016-06-03 15:57   ` Radim Krčmář
@ 2016-06-04 10:15   ` Boris Petkov
  2016-06-05 15:35     ` Haozhong Zhang
  2016-06-04 10:34   ` Boris Petkov
  2016-06-07 20:10   ` Eduardo Habkost
  3 siblings, 1 reply; 28+ messages in thread
From: Boris Petkov @ 2016-06-04 10:15 UTC (permalink / raw)
  To: Haozhong Zhang, qemu-devel
  Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Tony Luck, Andi Kleen,
	Ashok Raj

Haozhong Zhang <haozhong.zhang@intel.com> wrote:

>This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
>will be 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.
>
>Signed-off-by: Ashok Raj <ashok.raj@intel.com>
>Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>---
>Cc: Paolo Bonzini <pbonzini@redhat.com>
>Cc: Richard Henderson <rth@twiddle.net>
>Cc: Eduardo Habkost <ehabkost@redhat.com>
>Cc: Marcelo Tosatti <mtosatti@redhat.com>
>Cc: Boris Petkov <bp@suse.de>
>Cc: kvm@vger.kernel.org
>Cc: Tony Luck <tony.luck@intel.com>
>Cc: Andi Kleen <andi.kleen@intel.com>
>---
> target-i386/cpu.c | 26 ++++++++++++++++++++++++++
> target-i386/cpu.h | 13 ++++++++++++-
> target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
> 3 files changed, 69 insertions(+), 5 deletions(-)

...

>@@ -2786,6 +2798,20 @@ static void mce_init(X86CPU *cpu)
>         && (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()) {
>+                cenv->mcg_cap |= MCG_LMCE_P;
>+                cenv->msr_ia32_feature_control |=
>+                    MSR_IA32_FEATURE_CONTROL_LMCE |
>+                    MSR_IA32_FEATURE_CONTROL_LOCKED;
>+            } else {
>+                error_report("Warning: KVM unavailable or not support
>LMCE, "
>+                             "LMCE disabled");

"... or LMCE not supported..."

Also, do not split the string for easier grepping.

-- 
Sent from a small device: formatting sux and brevity is inevitable. 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support Haozhong Zhang
  2016-06-03 15:57   ` Radim Krčmář
  2016-06-04 10:15   ` Boris Petkov
@ 2016-06-04 10:34   ` Boris Petkov
  2016-06-04 21:03     ` Eduardo Habkost
  2016-06-05 15:41     ` Haozhong Zhang
  2016-06-07 20:10   ` Eduardo Habkost
  3 siblings, 2 replies; 28+ messages in thread
From: Boris Petkov @ 2016-06-04 10:34 UTC (permalink / raw)
  To: Haozhong Zhang, qemu-devel
  Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Tony Luck, Andi Kleen,
	Ashok Raj

Haozhong Zhang <haozhong.zhang@intel.com> wrote:

>This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
>will be 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.
>
>Signed-off-by: Ashok Raj <ashok.raj@intel.com>
>Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>---
>Cc: Paolo Bonzini <pbonzini@redhat.com>
>Cc: Richard Henderson <rth@twiddle.net>
>Cc: Eduardo Habkost <ehabkost@redhat.com>
>Cc: Marcelo Tosatti <mtosatti@redhat.com>
>Cc: Boris Petkov <bp@suse.de>
>Cc: kvm@vger.kernel.org
>Cc: Tony Luck <tony.luck@intel.com>
>Cc: Andi Kleen <andi.kleen@intel.com>
>---
> target-i386/cpu.c | 26 ++++++++++++++++++++++++++
> target-i386/cpu.h | 13 ++++++++++++-
> target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
> 3 files changed, 69 insertions(+), 5 deletions(-)

...

>@@ -1173,6 +1182,8 @@ struct X86CPU {
>      */
>     bool enable_pmu;
> 
>+    bool enable_lmce;

That struct would go fat pretty fast if it grows a bool per CPU feature. Perhaps a more clever, a-bit-per-featurebit scheme would be in order.

-- 
Sent from a small device: formatting sux and brevity is inevitable. 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-04 10:34   ` Boris Petkov
@ 2016-06-04 21:03     ` Eduardo Habkost
  2016-06-07  9:41       ` Haozhong Zhang
  2016-06-05 15:41     ` Haozhong Zhang
  1 sibling, 1 reply; 28+ messages in thread
From: Eduardo Habkost @ 2016-06-04 21:03 UTC (permalink / raw)
  To: Boris Petkov
  Cc: Haozhong Zhang, qemu-devel, Paolo Bonzini, Richard Henderson,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Tony Luck, Andi Kleen,
	Ashok Raj

On Sat, Jun 04, 2016 at 12:34:39PM +0200, Boris Petkov wrote:
> Haozhong Zhang <haozhong.zhang@intel.com> wrote:
> 
> >This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> >will be 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.
> >
> >Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> >Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >---
> >Cc: Paolo Bonzini <pbonzini@redhat.com>
> >Cc: Richard Henderson <rth@twiddle.net>
> >Cc: Eduardo Habkost <ehabkost@redhat.com>
> >Cc: Marcelo Tosatti <mtosatti@redhat.com>
> >Cc: Boris Petkov <bp@suse.de>
> >Cc: kvm@vger.kernel.org
> >Cc: Tony Luck <tony.luck@intel.com>
> >Cc: Andi Kleen <andi.kleen@intel.com>
> >---
> > target-i386/cpu.c | 26 ++++++++++++++++++++++++++
> > target-i386/cpu.h | 13 ++++++++++++-
> > target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
> > 3 files changed, 69 insertions(+), 5 deletions(-)
> 
> ...
> 
> >@@ -1173,6 +1182,8 @@ struct X86CPU {
> >      */
> >     bool enable_pmu;
> > 
> >+    bool enable_lmce;
> 
> That struct would go fat pretty fast if it grows a bool per CPU
> feature. Perhaps a more clever, a-bit-per-featurebit scheme
> would be in order.

We already have X86CPU.features, but it's specific for feature
flags appearing on CPUID. We can eventually extend
FeatureWord/FeatureWordInfo/x86_cpu_get_supported_feature_word()
to represent features that don't appear directly on CPUID.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-03 15:57   ` Radim Krčmář
@ 2016-06-05 15:32     ` Haozhong Zhang
  2016-06-08 11:32     ` Paolo Bonzini
  1 sibling, 0 replies; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-05 15:32 UTC (permalink / raw)
  To: Radim Krčmář
  Cc: qemu-devel, Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Boris Petkov,
	Tony Luck, Andi Kleen, Ashok Raj

On 06/03/16 17:57, Radim Krčmář wrote:
> 2016-06-03 14:09+0800, Haozhong Zhang:
> > This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> > will be 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.
> > 
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > @@ -2786,6 +2798,20 @@ static void mce_init(X86CPU *cpu)
> >          && (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()) {
> > +                cenv->mcg_cap |= MCG_LMCE_P;
> > +                cenv->msr_ia32_feature_control |=
> > +                    MSR_IA32_FEATURE_CONTROL_LMCE |
> > +                    MSR_IA32_FEATURE_CONTROL_LOCKED;
> 
> Locking right from the start breaks nested KVM, because nested relies on
> setting VMXON feature from inside of the guest.
> 
> Do we keep it unlocked, or move everything into QEMU?
> 
> (The latter seems simpler.)
>

Setting guest MSR_IA32_FEATURE_CONTROL is not necessary here, it's
instead the guest BIOS/OS duty to enable and lock corresponding
features. I'll remove this in the next version.

Thanks,
Haozhong

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-04 10:15   ` Boris Petkov
@ 2016-06-05 15:35     ` Haozhong Zhang
  0 siblings, 0 replies; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-05 15:35 UTC (permalink / raw)
  To: Boris Petkov
  Cc: qemu-devel, Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Tony Luck, Andi Kleen,
	Ashok Raj

On 06/04/16 12:15, Boris Petkov wrote:
> Haozhong Zhang <haozhong.zhang@intel.com> wrote:
> 
> >This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> >will be 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.
> >
> >Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> >Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >---
> >Cc: Paolo Bonzini <pbonzini@redhat.com>
> >Cc: Richard Henderson <rth@twiddle.net>
> >Cc: Eduardo Habkost <ehabkost@redhat.com>
> >Cc: Marcelo Tosatti <mtosatti@redhat.com>
> >Cc: Boris Petkov <bp@suse.de>
> >Cc: kvm@vger.kernel.org
> >Cc: Tony Luck <tony.luck@intel.com>
> >Cc: Andi Kleen <andi.kleen@intel.com>
> >---
> > target-i386/cpu.c | 26 ++++++++++++++++++++++++++
> > target-i386/cpu.h | 13 ++++++++++++-
> > target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
> > 3 files changed, 69 insertions(+), 5 deletions(-)
> 
> ...
> 
> >@@ -2786,6 +2798,20 @@ static void mce_init(X86CPU *cpu)
> >         && (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()) {
> >+                cenv->mcg_cap |= MCG_LMCE_P;
> >+                cenv->msr_ia32_feature_control |=
> >+                    MSR_IA32_FEATURE_CONTROL_LMCE |
> >+                    MSR_IA32_FEATURE_CONTROL_LOCKED;
> >+            } else {
> >+                error_report("Warning: KVM unavailable or not support
> >LMCE, "
> >+                             "LMCE disabled");
> 
> "... or LMCE not supported..."
>

will change

> Also, do not split the string for easier grepping.
>

OK, I was to avoid expiring 80 characters per line.

Thanks,
Haozhong

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-04 10:34   ` Boris Petkov
  2016-06-04 21:03     ` Eduardo Habkost
@ 2016-06-05 15:41     ` Haozhong Zhang
  2016-06-08 11:34       ` Paolo Bonzini
  1 sibling, 1 reply; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-05 15:41 UTC (permalink / raw)
  To: Boris Petkov
  Cc: qemu-devel, Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Tony Luck, Andi Kleen,
	Ashok Raj

On 06/04/16 12:34, Boris Petkov wrote:
> Haozhong Zhang <haozhong.zhang@intel.com> wrote:
> 
> >This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> >will be 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.
> >
> >Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> >Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >---
> >Cc: Paolo Bonzini <pbonzini@redhat.com>
> >Cc: Richard Henderson <rth@twiddle.net>
> >Cc: Eduardo Habkost <ehabkost@redhat.com>
> >Cc: Marcelo Tosatti <mtosatti@redhat.com>
> >Cc: Boris Petkov <bp@suse.de>
> >Cc: kvm@vger.kernel.org
> >Cc: Tony Luck <tony.luck@intel.com>
> >Cc: Andi Kleen <andi.kleen@intel.com>
> >---
> > target-i386/cpu.c | 26 ++++++++++++++++++++++++++
> > target-i386/cpu.h | 13 ++++++++++++-
> > target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
> > 3 files changed, 69 insertions(+), 5 deletions(-)
> 
> ...
> 
> >@@ -1173,6 +1182,8 @@ struct X86CPU {
> >      */
> >     bool enable_pmu;
> > 
> >+    bool enable_lmce;
> 
> That struct would go fat pretty fast if it grows a bool per CPU feature. Perhaps a more clever, a-bit-per-featurebit scheme would be in order.

OK, I'll use a 64-bit integer for current and future features.

Thanks,
Haozhong

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-04 21:03     ` Eduardo Habkost
@ 2016-06-07  9:41       ` Haozhong Zhang
  2016-06-07 11:47         ` Haozhong Zhang
  0 siblings, 1 reply; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-07  9:41 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Boris Petkov, qemu-devel, Paolo Bonzini, Richard Henderson,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Tony Luck, Andi Kleen,
	Ashok Raj

On 06/04/16 18:03, Eduardo Habkost wrote:
> On Sat, Jun 04, 2016 at 12:34:39PM +0200, Boris Petkov wrote:
> > Haozhong Zhang <haozhong.zhang@intel.com> wrote:
> > 
> > >This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> > >will be 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.
> > >
> > >Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > >Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > >---
> > >Cc: Paolo Bonzini <pbonzini@redhat.com>
> > >Cc: Richard Henderson <rth@twiddle.net>
> > >Cc: Eduardo Habkost <ehabkost@redhat.com>
> > >Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > >Cc: Boris Petkov <bp@suse.de>
> > >Cc: kvm@vger.kernel.org
> > >Cc: Tony Luck <tony.luck@intel.com>
> > >Cc: Andi Kleen <andi.kleen@intel.com>
> > >---
> > > target-i386/cpu.c | 26 ++++++++++++++++++++++++++
> > > target-i386/cpu.h | 13 ++++++++++++-
> > > target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
> > > 3 files changed, 69 insertions(+), 5 deletions(-)
> > 
> > ...
> > 
> > >@@ -1173,6 +1182,8 @@ struct X86CPU {
> > >      */
> > >     bool enable_pmu;
> > > 
> > >+    bool enable_lmce;
> > 
> > That struct would go fat pretty fast if it grows a bool per CPU
> > feature. Perhaps a more clever, a-bit-per-featurebit scheme
> > would be in order.
> 
> We already have X86CPU.features, but it's specific for feature
> flags appearing on CPUID. We can eventually extend
> FeatureWord/FeatureWordInfo/x86_cpu_get_supported_feature_word()
> to represent features that don't appear directly on CPUID.
>

You mean CPUX86State.features?

enable_lmce is also used in patch 2 to avoid migrating from
lmce-enabled qemu to lmce-disabled qemu, so I don't think
CPUX86State.features is the correct place for enable_lmce.

Or, we may append one or more bit words to X86CPU.filtered_features
for enable_pmu, enable_lmce and future features, e.g.

modified   target-i386/cpu.h
@@ -455,6 +455,14 @@ typedef enum FeatureWord {
 
 typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 
+typedef enum ExtFeatureBit {
+    EXT_FEAT_PMU,
+    EXT_FEAT_LMCE,
+    EXT_FEATURE_BITS,
+} ExtFeatureBit;
+
+#define EXT_FEATURE_WORDS ((EXT_FEATURE_BITS + 31) / 32)
+
+#define EXT_FEATURE_FILTER_ADD(words, feat)             \
+    do {                                                \
+        uint32_t *__words = (words);                    \
+        int __idx = (feat) / 32;                        \
+        int __oft = (feat) % 32;                        \
+        __words[__idx + FEATURE_WORDS] |= (1 << __oft); \
+    } while (0)
+
+#define EXT_FEATURE_FILTER_REMOVE(words, feat)           \
+    do {                                                 \
+        uint32_t *__words = (words);                     \
+        int __idx = (feat) / 32;                         \
+        int __oft = (feat) % 32;                         \
+        __words[__idx + FEATURE_WORDS] &= ~(1 << __oft); \
+    } while (0)
+
+#define EXT_FEATURE_FILTERED(words, feat)           \
+    ({                                              \
+        uint32_t *__words = (words);                \
+        int __idx = (feat) / 32;                    \
+        int __oft = (feat) % 32;                    \
+        __words[__idx + FEATURE_WORDS] & (1 << oft) \
+    })
+
 /* cpuid_features bits */
 #define CPUID_FP87 (1U << 0)
 #define CPUID_VME  (1U << 1)
@@ -1173,21 +1181,7 @@ struct X86CPU {
     /* Features that were filtered out because of missing host capabilities */
-    uint32_t filtered_features[FEATURE_WORDS];
+    /* Features that were filtered out because of missing host capabilities or
+       being disabled by default */
+    uint32_t filtered_features[FEATURE_WORDS + EXT_FEATURE_WORDS];
-
-    /* Enable PMU CPUID bits. This can't be enabled by default yet because
-     * it doesn't have ABI stability guarantees, as it passes all PMU CPUID
-     * bits returned by GET_SUPPORTED_CPUID (that depend on host CPU and kernel
-     * capabilities) directly to the guest.
-     */
-    bool enable_pmu;
-
-    /* Enable LMCE support which is set via cpu option 'lmce=on/off'. LMCE is
-     * disabled by default to avoid breaking the migration between QEMU with
-     * different LMCE support. Only migrating between QEMU with the same LMCE
-     * support is allowed.
-     */
-    bool enable_lmce;

Every place using X86CPU.enable_pmu and .enable_lmce is then replaced
by above macros EXT_FEATURE_FILTER_ADD, EXT_FEATURE_FILTER_REMOVE and
EXT_FEATURE_FILTERED. Of course, those bits in X86CPU.filtered_features
have the opposite meaning to original enable_lmce and enable_pmu.


Haozhong

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-07  9:41       ` Haozhong Zhang
@ 2016-06-07 11:47         ` Haozhong Zhang
  0 siblings, 0 replies; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-07 11:47 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Boris Petkov, qemu-devel, Paolo Bonzini, Richard Henderson,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Tony Luck, Andi Kleen,
	Ashok Raj

On 06/07/16 17:41, Haozhong Zhang wrote:
> On 06/04/16 18:03, Eduardo Habkost wrote:
> > On Sat, Jun 04, 2016 at 12:34:39PM +0200, Boris Petkov wrote:
> > > Haozhong Zhang <haozhong.zhang@intel.com> wrote:
> > > 
> > > >This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> > > >will be 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.
> > > >
> > > >Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > > >Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > >---
> > > >Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > >Cc: Richard Henderson <rth@twiddle.net>
> > > >Cc: Eduardo Habkost <ehabkost@redhat.com>
> > > >Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > > >Cc: Boris Petkov <bp@suse.de>
> > > >Cc: kvm@vger.kernel.org
> > > >Cc: Tony Luck <tony.luck@intel.com>
> > > >Cc: Andi Kleen <andi.kleen@intel.com>
> > > >---
> > > > target-i386/cpu.c | 26 ++++++++++++++++++++++++++
> > > > target-i386/cpu.h | 13 ++++++++++++-
> > > > target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
> > > > 3 files changed, 69 insertions(+), 5 deletions(-)
> > > 
> > > ...
> > > 
> > > >@@ -1173,6 +1182,8 @@ struct X86CPU {
> > > >      */
> > > >     bool enable_pmu;
> > > > 
> > > >+    bool enable_lmce;
> > > 
> > > That struct would go fat pretty fast if it grows a bool per CPU
> > > feature. Perhaps a more clever, a-bit-per-featurebit scheme
> > > would be in order.
> > 
> > We already have X86CPU.features, but it's specific for feature
> > flags appearing on CPUID. We can eventually extend
> > FeatureWord/FeatureWordInfo/x86_cpu_get_supported_feature_word()
> > to represent features that don't appear directly on CPUID.
> >
> 
> You mean CPUX86State.features?
> 
> enable_lmce is also used in patch 2 to avoid migrating from
> lmce-enabled qemu to lmce-disabled qemu, so I don't think
> CPUX86State.features is the correct place for enable_lmce.
>

Sorry, I got things wrong: I thought that CPUX86State.features on
destination is overwritten by the source one in the migration. As long
as it's in fact not, we may extend CPUX86State.features in a similar
way to I proposed for X86CPU.filtered_features to include features not
directly appear in cpuid.

Haozhong

> Or, we may append one or more bit words to X86CPU.filtered_features
> for enable_pmu, enable_lmce and future features, e.g.
> 
> modified   target-i386/cpu.h
> @@ -455,6 +455,14 @@ typedef enum FeatureWord {
>  
>  typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  
> +typedef enum ExtFeatureBit {
> +    EXT_FEAT_PMU,
> +    EXT_FEAT_LMCE,
> +    EXT_FEATURE_BITS,
> +} ExtFeatureBit;
> +
> +#define EXT_FEATURE_WORDS ((EXT_FEATURE_BITS + 31) / 32)
> +
> +#define EXT_FEATURE_FILTER_ADD(words, feat)             \
> +    do {                                                \
> +        uint32_t *__words = (words);                    \
> +        int __idx = (feat) / 32;                        \
> +        int __oft = (feat) % 32;                        \
> +        __words[__idx + FEATURE_WORDS] |= (1 << __oft); \
> +    } while (0)
> +
> +#define EXT_FEATURE_FILTER_REMOVE(words, feat)           \
> +    do {                                                 \
> +        uint32_t *__words = (words);                     \
> +        int __idx = (feat) / 32;                         \
> +        int __oft = (feat) % 32;                         \
> +        __words[__idx + FEATURE_WORDS] &= ~(1 << __oft); \
> +    } while (0)
> +
> +#define EXT_FEATURE_FILTERED(words, feat)           \
> +    ({                                              \
> +        uint32_t *__words = (words);                \
> +        int __idx = (feat) / 32;                    \
> +        int __oft = (feat) % 32;                    \
> +        __words[__idx + FEATURE_WORDS] & (1 << oft) \
> +    })
> +
>  /* cpuid_features bits */
>  #define CPUID_FP87 (1U << 0)
>  #define CPUID_VME  (1U << 1)
> @@ -1173,21 +1181,7 @@ struct X86CPU {
>      /* Features that were filtered out because of missing host capabilities */
> -    uint32_t filtered_features[FEATURE_WORDS];
> +    /* Features that were filtered out because of missing host capabilities or
> +       being disabled by default */
> +    uint32_t filtered_features[FEATURE_WORDS + EXT_FEATURE_WORDS];
> -
> -    /* Enable PMU CPUID bits. This can't be enabled by default yet because
> -     * it doesn't have ABI stability guarantees, as it passes all PMU CPUID
> -     * bits returned by GET_SUPPORTED_CPUID (that depend on host CPU and kernel
> -     * capabilities) directly to the guest.
> -     */
> -    bool enable_pmu;
> -
> -    /* Enable LMCE support which is set via cpu option 'lmce=on/off'. LMCE is
> -     * disabled by default to avoid breaking the migration between QEMU with
> -     * different LMCE support. Only migrating between QEMU with the same LMCE
> -     * support is allowed.
> -     */
> -    bool enable_lmce;
> 
> Every place using X86CPU.enable_pmu and .enable_lmce is then replaced
> by above macros EXT_FEATURE_FILTER_ADD, EXT_FEATURE_FILTER_REMOVE and
> EXT_FEATURE_FILTERED. Of course, those bits in X86CPU.filtered_features
> have the opposite meaning to original enable_lmce and enable_pmu.
> 
> 
> Haozhong
> --
> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support Haozhong Zhang
                     ` (2 preceding siblings ...)
  2016-06-04 10:34   ` Boris Petkov
@ 2016-06-07 20:10   ` Eduardo Habkost
  2016-06-08  1:43     ` Haozhong Zhang
  3 siblings, 1 reply; 28+ messages in thread
From: Eduardo Habkost @ 2016-06-07 20:10 UTC (permalink / raw)
  To: Haozhong Zhang
  Cc: qemu-devel, Paolo Bonzini, Richard Henderson, Marcelo Tosatti,
	Michael S . Tsirkin, kvm, Boris Petkov, Tony Luck, Andi Kleen,
	Ashok Raj

On Fri, Jun 03, 2016 at 02:09:43PM +0800, Haozhong Zhang wrote:
[...]
> +
> +        if (cpu->enable_lmce) {
> +            if (lmce_supported()) {
> +                cenv->mcg_cap |= MCG_LMCE_P;
> +                cenv->msr_ia32_feature_control |=
> +                    MSR_IA32_FEATURE_CONTROL_LMCE |
> +                    MSR_IA32_FEATURE_CONTROL_LOCKED;
> +            } else {
> +                error_report("Warning: KVM unavailable or not support LMCE, "
> +                             "LMCE disabled");
> +                cpu->enable_lmce = false;

Please don't do that. If the user explicitly asked for LMCE, you
should refuse to start if the host doesn't have the required
capabilities.


> +            }
> +        }
> +
>          cenv->mcg_ctl = ~(uint64_t)0;
>          for (bank = 0; bank < MCE_BANKS_DEF; bank++) {
>              cenv->mce_banks[bank * 4] = ~(uint64_t)0;
[...]

-- 
Eduardo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 2/2] target-i386: add migration support for Intel LMCE
  2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 2/2] target-i386: add migration support for Intel LMCE Haozhong Zhang
@ 2016-06-07 20:18   ` Eduardo Habkost
  2016-06-08  1:56     ` Haozhong Zhang
  2016-06-08 11:36   ` Paolo Bonzini
  1 sibling, 1 reply; 28+ messages in thread
From: Eduardo Habkost @ 2016-06-07 20:18 UTC (permalink / raw)
  To: Haozhong Zhang
  Cc: qemu-devel, Paolo Bonzini, Richard Henderson, Marcelo Tosatti,
	Michael S . Tsirkin, kvm, Boris Petkov, Tony Luck, Andi Kleen,
	Ashok Raj

On Fri, Jun 03, 2016 at 02:09:44PM +0800, Haozhong Zhang wrote:
> LMCE is disabled by default, but a cpu option 'lmce=on/off' is provided
> to enable/disable it. Migration is only allowed between VCPUs with the
> same lmce option.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Boris Petkov <bp@suse.de>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Andi Kleen <andi.kleen@intel.com>
> Cc: Ashok Raj <ashok.raj@intel.com>
> ---
>  include/hw/i386/pc.h  |  7 ++++++-
>  target-i386/cpu.c     |  1 +
>  target-i386/cpu.h     |  5 +++++
>  target-i386/machine.c | 24 ++++++++++++++++++++++++
>  4 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index ca23609..058eef9 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -357,7 +357,12 @@ int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
>  #define PC_COMPAT_2_6 \
> -    HW_COMPAT_2_6
> +    HW_COMPAT_2_6 \
> +    {\
> +        .driver   = TYPE_X86_CPU,\
> +        .property = "lmce",\
> +        .value    = "off",\
> +    },

You don't need this if lmce is disabled by default.

>  
>  #define PC_COMPAT_2_5 \
>      PC_COMPAT_2_6 \
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9b4dbab..c69cc17 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3232,6 +3232,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),

Maybe this belong to patch 1/2?

>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 2d411ba..b512fd6 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1182,6 +1182,11 @@ struct X86CPU {
>       */
>      bool enable_pmu;
>  
> +    /* Enable LMCE support which is set via cpu option 'lmce=on/off'. LMCE is
> +     * disabled by default to avoid breaking the migration between QEMU with
> +     * different LMCE support. Only migrating between QEMU with the same LMCE
> +     * support is allowed.
> +     */
>      bool enable_lmce;
>  
>      /* in order to simplify APIC support, we leave this pointer to the
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index cb9adf2..b55d376 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -347,6 +347,11 @@ static int cpu_post_load(void *opaque, int version_id)
>          return -EINVAL;
>      }
>  
> +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
> +        error_report("LMCE not enabled");
> +        return -EINVAL;
> +    }

Nice. But the error message could be clearer, to indicate that it
is about command-line configuration not being the same on both
sides. What about something like:
  config mismatch: VCPU has LMCE is enabled, but "lmce" option is disabled

> +
>      /*
>       * Real mode guest segments register DPL should be zero.
>       * Older KVM version were setting it wrongly.
> @@ -896,6 +901,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 +1045,7 @@ VMStateDescription vmstate_x86_cpu = {
>  #ifdef TARGET_X86_64
>          &vmstate_pkru,
>  #endif
> +        &vmstate_mcg_ext_ctl,
>          NULL
>      }
>  };
> -- 
> 2.8.3
> 

-- 
Eduardo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-07 20:10   ` Eduardo Habkost
@ 2016-06-08  1:43     ` Haozhong Zhang
  0 siblings, 0 replies; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-08  1:43 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: qemu-devel, Paolo Bonzini, Richard Henderson, Marcelo Tosatti,
	Michael S . Tsirkin, kvm, Boris Petkov, Tony Luck, Andi Kleen,
	Ashok Raj

On 06/07/16 17:10, Eduardo Habkost wrote:
> On Fri, Jun 03, 2016 at 02:09:43PM +0800, Haozhong Zhang wrote:
> [...]
> > +
> > +        if (cpu->enable_lmce) {
> > +            if (lmce_supported()) {
> > +                cenv->mcg_cap |= MCG_LMCE_P;
> > +                cenv->msr_ia32_feature_control |=
> > +                    MSR_IA32_FEATURE_CONTROL_LMCE |
> > +                    MSR_IA32_FEATURE_CONTROL_LOCKED;
> > +            } else {
> > +                error_report("Warning: KVM unavailable or not support LMCE, "
> > +                             "LMCE disabled");
> > +                cpu->enable_lmce = false;
> 
> Please don't do that. If the user explicitly asked for LMCE, you
> should refuse to start if the host doesn't have the required
> capabilities.
>

OK, I'll change in the next version.

Thanks,
Haozhong

> 
> > +            }
> > +        }
> > +
> >          cenv->mcg_ctl = ~(uint64_t)0;
> >          for (bank = 0; bank < MCE_BANKS_DEF; bank++) {
> >              cenv->mce_banks[bank * 4] = ~(uint64_t)0;
> [...]
> 
> -- 
> Eduardo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 2/2] target-i386: add migration support for Intel LMCE
  2016-06-07 20:18   ` Eduardo Habkost
@ 2016-06-08  1:56     ` Haozhong Zhang
  0 siblings, 0 replies; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-08  1:56 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: qemu-devel, Paolo Bonzini, Richard Henderson, Marcelo Tosatti,
	Michael S . Tsirkin, kvm, Boris Petkov, Tony Luck, Andi Kleen,
	Ashok Raj

On 06/07/16 17:18, Eduardo Habkost wrote:
> On Fri, Jun 03, 2016 at 02:09:44PM +0800, Haozhong Zhang wrote:
> > LMCE is disabled by default, but a cpu option 'lmce=on/off' is provided
> > to enable/disable it. Migration is only allowed between VCPUs with the
> > same lmce option.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Richard Henderson <rth@twiddle.net>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Cc: Boris Petkov <bp@suse.de>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Andi Kleen <andi.kleen@intel.com>
> > Cc: Ashok Raj <ashok.raj@intel.com>
> > ---
> >  include/hw/i386/pc.h  |  7 ++++++-
> >  target-i386/cpu.c     |  1 +
> >  target-i386/cpu.h     |  5 +++++
> >  target-i386/machine.c | 24 ++++++++++++++++++++++++
> >  4 files changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index ca23609..058eef9 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -357,7 +357,12 @@ int e820_get_num_entries(void);
> >  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> >  
> >  #define PC_COMPAT_2_6 \
> > -    HW_COMPAT_2_6
> > +    HW_COMPAT_2_6 \
> > +    {\
> > +        .driver   = TYPE_X86_CPU,\
> > +        .property = "lmce",\
> > +        .value    = "off",\
> > +    },
> 
> You don't need this if lmce is disabled by default.
>

Oh yes, I'll remove in the next version.

> >  
> >  #define PC_COMPAT_2_5 \
> >      PC_COMPAT_2_6 \
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 9b4dbab..c69cc17 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -3232,6 +3232,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),
> 
> Maybe this belong to patch 1/2?
>

I think it's better to not allow users to enable LMCE until we fix the
migration in patch 2, so I didn't put it in patch 1.

> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >  
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 2d411ba..b512fd6 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1182,6 +1182,11 @@ struct X86CPU {
> >       */
> >      bool enable_pmu;
> >  
> > +    /* Enable LMCE support which is set via cpu option 'lmce=on/off'. LMCE is
> > +     * disabled by default to avoid breaking the migration between QEMU with
> > +     * different LMCE support. Only migrating between QEMU with the same LMCE
> > +     * support is allowed.
> > +     */
> >      bool enable_lmce;
> >  
> >      /* in order to simplify APIC support, we leave this pointer to the
> > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > index cb9adf2..b55d376 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -347,6 +347,11 @@ static int cpu_post_load(void *opaque, int version_id)
> >          return -EINVAL;
> >      }
> >  
> > +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
> > +        error_report("LMCE not enabled");
> > +        return -EINVAL;
> > +    }
> 
> Nice. But the error message could be clearer, to indicate that it
> is about command-line configuration not being the same on both
> sides. What about something like:
>   config mismatch: VCPU has LMCE is enabled, but "lmce" option is disabled
>

Yes, yours is much clearer. I'll change in the next version.

Thanks,
Haozhong

> > +
> >      /*
> >       * Real mode guest segments register DPL should be zero.
> >       * Older KVM version were setting it wrongly.
> > @@ -896,6 +901,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 +1045,7 @@ VMStateDescription vmstate_x86_cpu = {
> >  #ifdef TARGET_X86_64
> >          &vmstate_pkru,
> >  #endif
> > +        &vmstate_mcg_ext_ctl,
> >          NULL
> >      }
> >  };
> > -- 
> > 2.8.3
> > 
> 
> -- 
> Eduardo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-03 15:57   ` Radim Krčmář
  2016-06-05 15:32     ` Haozhong Zhang
@ 2016-06-08 11:32     ` Paolo Bonzini
  2016-06-13  7:55       ` Haozhong Zhang
  1 sibling, 1 reply; 28+ messages in thread
From: Paolo Bonzini @ 2016-06-08 11:32 UTC (permalink / raw)
  To: Radim Krčmář, Haozhong Zhang
  Cc: qemu-devel, Richard Henderson, Eduardo Habkost, Marcelo Tosatti,
	Michael S . Tsirkin, kvm, Boris Petkov, Tony Luck, Andi Kleen,
	Ashok Raj



On 03/06/2016 17:57, Radim Krčmář wrote:
>> > +                cenv->msr_ia32_feature_control |=
>> > +                    MSR_IA32_FEATURE_CONTROL_LMCE |
>> > +                    MSR_IA32_FEATURE_CONTROL_LOCKED;
> Locking right from the start breaks nested KVM, because nested relies on
> setting VMXON feature from inside of the guest.
> 
> Do we keep it unlocked, or move everything into QEMU?
> 
> (The latter seems simpler.)

I think it should be moved into the firmware, with QEMU publishing the
desired setting via fw_cfg.  The same as what is done in real hardware,
that's the KVM mantra. :)

For v4 it's okay to just remove this.

Paolo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-05 15:41     ` Haozhong Zhang
@ 2016-06-08 11:34       ` Paolo Bonzini
  2016-06-09  6:52         ` Haozhong Zhang
  0 siblings, 1 reply; 28+ messages in thread
From: Paolo Bonzini @ 2016-06-08 11:34 UTC (permalink / raw)
  To: Boris Petkov, qemu-devel, Richard Henderson, Eduardo Habkost,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Tony Luck, Andi Kleen,
	Ashok Raj



On 05/06/2016 17:41, Haozhong Zhang wrote:
> On 06/04/16 12:34, Boris Petkov wrote:
>> Haozhong Zhang <haozhong.zhang@intel.com> wrote:
>>
>>> This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
>>> will be 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.
>>>
>>> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
>>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>> ---
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Richard Henderson <rth@twiddle.net>
>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>> Cc: Marcelo Tosatti <mtosatti@redhat.com>
>>> Cc: Boris Petkov <bp@suse.de>
>>> Cc: kvm@vger.kernel.org
>>> Cc: Tony Luck <tony.luck@intel.com>
>>> Cc: Andi Kleen <andi.kleen@intel.com>
>>> ---
>>> target-i386/cpu.c | 26 ++++++++++++++++++++++++++
>>> target-i386/cpu.h | 13 ++++++++++++-
>>> target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
>>> 3 files changed, 69 insertions(+), 5 deletions(-)
>>
>> ...
>>
>>> @@ -1173,6 +1182,8 @@ struct X86CPU {
>>>      */
>>>     bool enable_pmu;
>>>
>>> +    bool enable_lmce;
>>
>> That struct would go fat pretty fast if it grows a bool per CPU feature. Perhaps a more clever, a-bit-per-featurebit scheme would be in order.
> 
> OK, I'll use a 64-bit integer for current and future features.

No, please keep this as is for now.  It can be refactored later.

Paolo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 2/2] target-i386: add migration support for Intel LMCE
  2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 2/2] target-i386: add migration support for Intel LMCE Haozhong Zhang
  2016-06-07 20:18   ` Eduardo Habkost
@ 2016-06-08 11:36   ` Paolo Bonzini
  2016-06-09  7:16     ` Haozhong Zhang
  1 sibling, 1 reply; 28+ messages in thread
From: Paolo Bonzini @ 2016-06-08 11:36 UTC (permalink / raw)
  To: Haozhong Zhang, qemu-devel
  Cc: Richard Henderson, Eduardo Habkost, Marcelo Tosatti,
	Michael S . Tsirkin, kvm, Boris Petkov, Tony Luck, Andi Kleen,
	Ashok Raj



On 03/06/2016 08:09, Haozhong Zhang wrote:
> LMCE is disabled by default, but a cpu option 'lmce=on/off' is provided
> to enable/disable it. Migration is only allowed between VCPUs with the
> same lmce option.

This is not needed at all if you do the change in patch 1 that Eduardo
requested (refuse to start if the host doesn't have the required
capabilities).

So if you do that you can just move the lmce property to patch 1.

Thanks,

Paolo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-08 11:34       ` Paolo Bonzini
@ 2016-06-09  6:52         ` Haozhong Zhang
  0 siblings, 0 replies; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-09  6:52 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Boris Petkov, qemu-devel, Richard Henderson, Eduardo Habkost,
	Marcelo Tosatti, Michael S . Tsirkin, kvm, Tony Luck, Andi Kleen,
	Ashok Raj

On 06/08/16 13:34, Paolo Bonzini wrote:
> 
> 
> On 05/06/2016 17:41, Haozhong Zhang wrote:
> > On 06/04/16 12:34, Boris Petkov wrote:
> >> Haozhong Zhang <haozhong.zhang@intel.com> wrote:
> >>
> >>> This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> >>> will be 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.
> >>>
> >>> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> >>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >>> ---
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>> Cc: Richard Henderson <rth@twiddle.net>
> >>> Cc: Eduardo Habkost <ehabkost@redhat.com>
> >>> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> >>> Cc: Boris Petkov <bp@suse.de>
> >>> Cc: kvm@vger.kernel.org
> >>> Cc: Tony Luck <tony.luck@intel.com>
> >>> Cc: Andi Kleen <andi.kleen@intel.com>
> >>> ---
> >>> target-i386/cpu.c | 26 ++++++++++++++++++++++++++
> >>> target-i386/cpu.h | 13 ++++++++++++-
> >>> target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
> >>> 3 files changed, 69 insertions(+), 5 deletions(-)
> >>
> >> ...
> >>
> >>> @@ -1173,6 +1182,8 @@ struct X86CPU {
> >>>      */
> >>>     bool enable_pmu;
> >>>
> >>> +    bool enable_lmce;
> >>
> >> That struct would go fat pretty fast if it grows a bool per CPU feature. Perhaps a more clever, a-bit-per-featurebit scheme would be in order.
> > 
> > OK, I'll use a 64-bit integer for current and future features.
> 
> No, please keep this as is for now.  It can be refactored later.
>

OK

Thanks,
Haozhong

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 2/2] target-i386: add migration support for Intel LMCE
  2016-06-08 11:36   ` Paolo Bonzini
@ 2016-06-09  7:16     ` Haozhong Zhang
  2016-06-09  8:23       ` Paolo Bonzini
  0 siblings, 1 reply; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-09  7:16 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: qemu-devel, Richard Henderson, Eduardo Habkost, Marcelo Tosatti,
	Michael S . Tsirkin, kvm, Boris Petkov, Tony Luck, Andi Kleen,
	Ashok Raj

On 06/08/16 13:36, Paolo Bonzini wrote:
> 
> 
> On 03/06/2016 08:09, Haozhong Zhang wrote:
> > LMCE is disabled by default, but a cpu option 'lmce=on/off' is provided
> > to enable/disable it. Migration is only allowed between VCPUs with the
> > same lmce option.
> 
> This is not needed at all if you do the change in patch 1 that Eduardo
> requested (refuse to start if the host doesn't have the required
> capabilities).
> 
> So if you do that you can just move the lmce property to patch 1.
>

But it doesn't cover the migration from lmce-enabled qemu to
lmce-disabled qemu where KVM on both hosts support LMCE. In that case,
both qemu can start without failure, but the guest OS will run in a VM
with different configurations after migration. To avoid this, I didn't
leave the lmce property in patch 1, so that users have no way to
enable LMCE without this patch 2.

Thanks,
Haozhong

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 2/2] target-i386: add migration support for Intel LMCE
  2016-06-09  7:16     ` Haozhong Zhang
@ 2016-06-09  8:23       ` Paolo Bonzini
  0 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2016-06-09  8:23 UTC (permalink / raw)
  To: qemu-devel, Richard Henderson, Eduardo Habkost, Marcelo Tosatti,
	Michael S . Tsirkin, kvm, Boris Petkov, Tony Luck, Andi Kleen,
	Ashok Raj



On 09/06/2016 09:16, Haozhong Zhang wrote:
> On 06/08/16 13:36, Paolo Bonzini wrote:
>>
>>
>> On 03/06/2016 08:09, Haozhong Zhang wrote:
>>> LMCE is disabled by default, but a cpu option 'lmce=on/off' is provided
>>> to enable/disable it. Migration is only allowed between VCPUs with the
>>> same lmce option.
>>
>> This is not needed at all if you do the change in patch 1 that Eduardo
>> requested (refuse to start if the host doesn't have the required
>> capabilities).
>>
>> So if you do that you can just move the lmce property to patch 1.
>>
> 
> But it doesn't cover the migration from lmce-enabled qemu to
> lmce-disabled qemu where KVM on both hosts support LMCE.

That's a configuration problem; configuration is not migrated and is
assumed to be the same on the source and the destination.  You don't
need to test for this scenario.

Paolo

 In that case,
> both qemu can start without failure, but the guest OS will run in a VM
> with different configurations after migration. To avoid this, I didn't
> leave the lmce property in patch 1, so that users have no way to
> enable LMCE without this patch 2.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-08 11:32     ` Paolo Bonzini
@ 2016-06-13  7:55       ` Haozhong Zhang
  2016-06-13  8:33         ` Paolo Bonzini
  0 siblings, 1 reply; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-13  7:55 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Radim Krčmář, qemu-devel, Richard Henderson,
	Eduardo Habkost, Marcelo Tosatti, Michael S . Tsirkin, kvm,
	Boris Petkov, Tony Luck, Andi Kleen, Ashok Raj

On 06/08/16 13:32, Paolo Bonzini wrote:
> 
> 
> On 03/06/2016 17:57, Radim Krčmář wrote:
> >> > +                cenv->msr_ia32_feature_control |=
> >> > +                    MSR_IA32_FEATURE_CONTROL_LMCE |
> >> > +                    MSR_IA32_FEATURE_CONTROL_LOCKED;
> > Locking right from the start breaks nested KVM, because nested relies on
> > setting VMXON feature from inside of the guest.
> > 
> > Do we keep it unlocked, or move everything into QEMU?
> > 
> > (The latter seems simpler.)
> 
> I think it should be moved into the firmware, with QEMU publishing the
> desired setting via fw_cfg.  The same as what is done in real hardware,
> that's the KVM mantra. :)
> 
> For v4 it's okay to just remove this.
> 
> Paolo

Currently, only VMX bits (bit 1 & 2), LMCE bit (bit 20) as well as
lock bit (bit 0) in MSR_IA32_FEATURE_CONTROL are used for guest. The
availability of features indicated by those bits (except the lock bit)
can be discovered from cpuid and other MSR, so it looks not necessary
to publish them via fw_cfg. Or do you have other concerns?

Thanks,
Haozhong

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-13  7:55       ` Haozhong Zhang
@ 2016-06-13  8:33         ` Paolo Bonzini
  2016-06-13 10:01           ` Haozhong Zhang
  0 siblings, 1 reply; 28+ messages in thread
From: Paolo Bonzini @ 2016-06-13  8:33 UTC (permalink / raw)
  To: Radim Krčmář, qemu-devel, Richard Henderson,
	Eduardo Habkost, Marcelo Tosatti, Michael S . Tsirkin, kvm,
	Boris Petkov, Tony Luck, Andi Kleen, Ashok Raj



On 13/06/2016 09:55, Haozhong Zhang wrote:
> Currently, only VMX bits (bit 1 & 2), LMCE bit (bit 20) as well as
> lock bit (bit 0) in MSR_IA32_FEATURE_CONTROL are used for guest. The
> availability of features indicated by those bits (except the lock bit)
> can be discovered from cpuid and other MSR, so it looks not necessary
> to publish them via fw_cfg. Or do you have other concerns?

I would prefer to avoid having to change the firmware (SeaBIOS and OVMF)
every time a new bit is added.  Using fw_cfg makes it possible to
develop the feature in the firmware once and for all.

Paolo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-13  8:33         ` Paolo Bonzini
@ 2016-06-13 10:01           ` Haozhong Zhang
  2016-06-13 10:07             ` Paolo Bonzini
  0 siblings, 1 reply; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-13 10:01 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Radim Krčmář, qemu-devel, Richard Henderson,
	Eduardo Habkost, Marcelo Tosatti, Michael S . Tsirkin, kvm,
	Boris Petkov, Tony Luck, Andi Kleen, Ashok Raj

On 06/13/16 10:33, Paolo Bonzini wrote:
> 
> 
> On 13/06/2016 09:55, Haozhong Zhang wrote:
> > Currently, only VMX bits (bit 1 & 2), LMCE bit (bit 20) as well as
> > lock bit (bit 0) in MSR_IA32_FEATURE_CONTROL are used for guest. The
> > availability of features indicated by those bits (except the lock bit)
> > can be discovered from cpuid and other MSR, so it looks not necessary
> > to publish them via fw_cfg. Or do you have other concerns?
> 
> I would prefer to avoid having to change the firmware (SeaBIOS and OVMF)
> every time a new bit is added.  Using fw_cfg makes it possible to
> develop the feature in the firmware once and for all.
> 

Thanks for the explanation! Is it proper to add a key in fw_cfg for
this purpose, e.g FW_CFG_MSR_FEATURE_CONTROL to pass bits that are
supposed to be set by firmware?

Thanks,
Haozhong

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-13 10:01           ` Haozhong Zhang
@ 2016-06-13 10:07             ` Paolo Bonzini
  2016-06-13 10:09               ` Haozhong Zhang
  0 siblings, 1 reply; 28+ messages in thread
From: Paolo Bonzini @ 2016-06-13 10:07 UTC (permalink / raw)
  To: Radim Krčmář, qemu-devel, Richard Henderson,
	Eduardo Habkost, Marcelo Tosatti, Michael S . Tsirkin, kvm,
	Boris Petkov, Tony Luck, Andi Kleen, Ashok Raj



On 13/06/2016 12:01, Haozhong Zhang wrote:
> > I would prefer to avoid having to change the firmware (SeaBIOS and OVMF)
> > every time a new bit is added.  Using fw_cfg makes it possible to
> > develop the feature in the firmware once and for all.
> 
> Thanks for the explanation! Is it proper to add a key in fw_cfg for
> this purpose, e.g FW_CFG_MSR_FEATURE_CONTROL to pass bits that are
> supposed to be set by firmware?

We usually add new files now, so it would be a new file named
"etc/msr-feature-control".

Thanks,

Paolo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support
  2016-06-13 10:07             ` Paolo Bonzini
@ 2016-06-13 10:09               ` Haozhong Zhang
  0 siblings, 0 replies; 28+ messages in thread
From: Haozhong Zhang @ 2016-06-13 10:09 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Radim Krčmář, qemu-devel, Richard Henderson,
	Eduardo Habkost, Marcelo Tosatti, Michael S . Tsirkin, kvm,
	Boris Petkov, Tony Luck, Andi Kleen, Ashok Raj

On 06/13/16 12:07, Paolo Bonzini wrote:
> 
> 
> On 13/06/2016 12:01, Haozhong Zhang wrote:
> > > I would prefer to avoid having to change the firmware (SeaBIOS and OVMF)
> > > every time a new bit is added.  Using fw_cfg makes it possible to
> > > develop the feature in the firmware once and for all.
> > 
> > Thanks for the explanation! Is it proper to add a key in fw_cfg for
> > this purpose, e.g FW_CFG_MSR_FEATURE_CONTROL to pass bits that are
> > supposed to be set by firmware?
> 
> We usually add new files now, so it would be a new file named
> "etc/msr-feature-control".
> 

Got it, thanks!

Haozhong

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2016-06-13 10:10 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-03  6:09 [Qemu-devel] [PATCH v3 0/2] Add QEMU support for Intel local MCE Haozhong Zhang
2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support Haozhong Zhang
2016-06-03 15:57   ` Radim Krčmář
2016-06-05 15:32     ` Haozhong Zhang
2016-06-08 11:32     ` Paolo Bonzini
2016-06-13  7:55       ` Haozhong Zhang
2016-06-13  8:33         ` Paolo Bonzini
2016-06-13 10:01           ` Haozhong Zhang
2016-06-13 10:07             ` Paolo Bonzini
2016-06-13 10:09               ` Haozhong Zhang
2016-06-04 10:15   ` Boris Petkov
2016-06-05 15:35     ` Haozhong Zhang
2016-06-04 10:34   ` Boris Petkov
2016-06-04 21:03     ` Eduardo Habkost
2016-06-07  9:41       ` Haozhong Zhang
2016-06-07 11:47         ` Haozhong Zhang
2016-06-05 15:41     ` Haozhong Zhang
2016-06-08 11:34       ` Paolo Bonzini
2016-06-09  6:52         ` Haozhong Zhang
2016-06-07 20:10   ` Eduardo Habkost
2016-06-08  1:43     ` Haozhong Zhang
2016-06-03  6:09 ` [Qemu-devel] [PATCH v3 2/2] target-i386: add migration support for Intel LMCE Haozhong Zhang
2016-06-07 20:18   ` Eduardo Habkost
2016-06-08  1:56     ` Haozhong Zhang
2016-06-08 11:36   ` Paolo Bonzini
2016-06-09  7:16     ` Haozhong Zhang
2016-06-09  8:23       ` Paolo Bonzini
2016-06-03  6:38 ` [Qemu-devel] [PATCH v3 0/2] Add QEMU support for Intel local MCE Haozhong Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).