[PATCH 0/7] MCE code cleanup and bugfix

[PATCH 0/7] MCE code cleanup and bugfix

[Haozhong Zhang]

This patch series separates the uncommitted code cleanup and bugfix
patches from my previous patch series "[PATCH 00/19] MCE code cleanup and add LMCE support"
with other new fixes.

* This patch series is based on Jan's patch "x86/MCE: sanitize domain/vcpu ID handling".
* Patch 1 & 6 are new ones.
* Patch 2 is the previous patch 4 w/ the same title.
* Patch 3 - 5 are the previous patch 8 - 10 w/ the last one renamed.
* Patch 7 is the previous patch 11 w/ the same title.
* Individual changes are logged in each patch.

Haozhong Zhang (7):
  1/7 xen/mce: adjust comment of callback register functions
  2/7 xen/mce: remove unused x86_mcinfo_add()
  3/7 x86/mce: set mcinfo_comm.type and .size in x86_mcinfo_reserve()
  4/7 x86/vmce: fill MSR_IA32_MCG_STATUS on all vcpus in broadcast case
  5/7 x86/mce: clear MSR_IA32_MCG_STATUS by writing 0
  6/7 xen/mce: make ASSERT's about mce_dhandler_num in mce_action() Intel only
  7/7 tools/xen-mceinj: fix the type of cpu number

 tools/tests/mce-test/tools/xen-mceinj.c | 12 +++---
 xen/arch/x86/cpu/mcheck/mcaction.c      | 20 ++++-----
 xen/arch/x86/cpu/mcheck/mce.c           | 40 ++++++------------
 xen/arch/x86/cpu/mcheck/mce.h           | 24 +++++------
 xen/arch/x86/cpu/mcheck/mce_amd.c       |  4 +-
 xen/arch/x86/cpu/mcheck/mce_intel.c     |  6 +--
 xen/arch/x86/cpu/mcheck/vmce.c          | 74 +++++++++++++++++++++++----------
 xen/arch/x86/cpu/mcheck/vmce.h          |  2 +-
 8 files changed, 95 insertions(+), 87 deletions(-)

-- 
2.10.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/7] xen/mce: adjust comment of callback register functions

[Haozhong Zhang]

c/s e966818264908e842e2847f579ca4d94e586eaac added
mce_need_clearbank_register below the comment of
x86_mce_callback_register(). This commit (1) adjusts the first
paragraph of comment to be a general statement of all callback
register functions, and (2) moves the second paragraph to the
front of x86_mce_callback_register().

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Christoph Egger <chegger@amazon.de>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/cpu/mcheck/mce.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
index e83d431..42bb782 100644
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -128,24 +128,24 @@ extern void mcheck_mca_clearbanks(struct mca_banks *);
 extern mctelem_cookie_t mcheck_mca_logout(enum mca_source, struct mca_banks *,
     struct mca_summary *, struct mca_banks *);
 
-/* Register a callback to be made during bank telemetry logout.
- * This callback is only available to those machine check handlers
+/* Register callbacks to be made during bank telemetry logout.
+ * Those callbacks are only available to those machine check handlers
  * that call to the common mcheck_cmn_handler or who use the common
  * telemetry logout function mcheck_mca_logout in error polling.
- *
- * This can be used to collect additional information (typically non-
- * architectural) provided by newer CPU families/models without the need
- * to duplicate the whole handler resulting in various handlers each with
- * its own tweaks and bugs.  The callback receives an struct mc_info pointer
- * which it can use with x86_mcinfo_add to add additional telemetry,
- * the current MCA bank number we are reading telemetry from, and the
- * MCi_STATUS value for that bank.
  */
 
 /* Register a handler for judging whether the bank need to be cleared */
 typedef int (*mce_need_clearbank_t)(enum mca_source who, u64 status);
 extern void mce_need_clearbank_register(mce_need_clearbank_t);
 
+/* Register a callback to collect additional information (typically non-
+ * architectural) provided by newer CPU families/models without the need
+ * to duplicate the whole handler resulting in various handlers each with
+ * its own tweaks and bugs. The callback receives an struct mc_info pointer
+ * which it can use with x86_mcinfo_add to add additional telemetry,
+ * the current MCA bank number we are reading telemetry from, and the
+ * MCi_STATUS value for that bank.
+ */
 typedef struct mcinfo_extended *(*x86_mce_callback_t)
     (struct mc_info *, uint16_t, uint64_t);
 extern void x86_mce_callback_register(x86_mce_callback_t);
-- 
2.10.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/7] xen/mce: remove unused x86_mcinfo_add()

[Haozhong Zhang]

c/s 9d13fd9fd320a7740c6446c048ff6a2990095966 turned to update the
mcinfo buffer in-place instead of using x86_mcinfo_add(). The last
uses of x86_mcinfo_add() were removed by that commit as well.
Therefore, x86_mcinfo_add() was deprecated in fact.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Christoph Egger <chegger@amazon.de>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

Changes:
 * Explain the reason in the commit message.
 * Update the code comment mentioning x86_mcinfo_add.
---
 xen/arch/x86/cpu/mcheck/mce.c | 16 ----------------
 xen/arch/x86/cpu/mcheck/mce.h |  3 +--
 2 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index e41989d..aa9304f 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -835,22 +835,6 @@ void *x86_mcinfo_reserve(struct mc_info *mi, int size)
     return memset(mic_index, 0, size);
 }
 
-void *x86_mcinfo_add(struct mc_info *mi, void *mcinfo)
-{
-    struct mcinfo_common *mic, *buf;
-
-    mic = (struct mcinfo_common *)mcinfo;
-    buf = x86_mcinfo_reserve(mi, mic->size);
-
-    if ( !buf )
-        mce_printk(MCE_CRITICAL,
-                   "mcinfo_add: No space left in mc_info\n");
-    else
-        memcpy(buf, mic, mic->size);
-
-    return buf;
-}
-
 static void x86_mcinfo_apei_save(
     struct mcinfo_global *mc_global, struct mcinfo_bank *mc_bank)
 {
diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
index 42bb782..a28e862 100644
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -142,7 +142,7 @@ extern void mce_need_clearbank_register(mce_need_clearbank_t);
  * architectural) provided by newer CPU families/models without the need
  * to duplicate the whole handler resulting in various handlers each with
  * its own tweaks and bugs. The callback receives an struct mc_info pointer
- * which it can use with x86_mcinfo_add to add additional telemetry,
+ * which it can use with x86_mcinfo_reserve to add additional telemetry,
  * the current MCA bank number we are reading telemetry from, and the
  * MCi_STATUS value for that bank.
  */
@@ -150,7 +150,6 @@ typedef struct mcinfo_extended *(*x86_mce_callback_t)
     (struct mc_info *, uint16_t, uint64_t);
 extern void x86_mce_callback_register(x86_mce_callback_t);
 
-void *x86_mcinfo_add(struct mc_info *mi, void *mcinfo);
 void *x86_mcinfo_reserve(struct mc_info *mi, int size);
 void x86_mcinfo_dump(struct mc_info *mi);
 
-- 
2.10.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/7] x86/mce: set mcinfo_comm.type and .size in x86_mcinfo_reserve()

[Haozhong Zhang]

All existing calls to x86_mcinfo_reserve() are followed by statements
that set the size and the type of the reserved space, so move them into
x86_mcinfo_reserve() to simplify the code.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Christoph Egger <chegger@amazon.de>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

Changes:
 * Use "unsigned int" for arguments "size" and "type" of x86_mcinfo_reserve().
---
 xen/arch/x86/cpu/mcheck/mcaction.c  |  4 +---
 xen/arch/x86/cpu/mcheck/mce.c       | 17 +++++++++--------
 xen/arch/x86/cpu/mcheck/mce.h       |  3 ++-
 xen/arch/x86/cpu/mcheck/mce_amd.c   |  4 +---
 xen/arch/x86/cpu/mcheck/mce_intel.c |  6 +-----
 5 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mcaction.c b/xen/arch/x86/cpu/mcheck/mcaction.c
index 322163a..32056f2 100644
--- a/xen/arch/x86/cpu/mcheck/mcaction.c
+++ b/xen/arch/x86/cpu/mcheck/mcaction.c
@@ -13,14 +13,12 @@ mci_action_add_pageoffline(int bank, struct mc_info *mi,
     if (!mi)
         return NULL;
 
-    rec = x86_mcinfo_reserve(mi, sizeof(*rec));
+    rec = x86_mcinfo_reserve(mi, sizeof(*rec), MC_TYPE_RECOVERY);
     if (!rec) {
         mi->flags |= MCINFO_FLAGS_UNCOMPLETE;
         return NULL;
     }
 
-    rec->common.type = MC_TYPE_RECOVERY;
-    rec->common.size = sizeof(*rec);
     rec->mc_bank = bank;
     rec->action_types = MC_ACTION_PAGE_OFFLINE;
     rec->action_info.page_retire.mfn = mfn;
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index aa9304f..53ca29c 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -205,7 +205,7 @@ static void mca_init_bank(enum mca_source who,
     if (!mi)
         return;
 
-    mib = x86_mcinfo_reserve(mi, sizeof(*mib));
+    mib = x86_mcinfo_reserve(mi, sizeof(*mib), MC_TYPE_BANK);
     if (!mib)
     {
         mi->flags |= MCINFO_FLAGS_UNCOMPLETE;
@@ -214,8 +214,6 @@ static void mca_init_bank(enum mca_source who,
 
     mib->mc_status = mca_rdmsr(MSR_IA32_MCx_STATUS(bank));
 
-    mib->common.type = MC_TYPE_BANK;
-    mib->common.size = sizeof (struct mcinfo_bank);
     mib->mc_bank = bank;
     mib->mc_domid = DOMID_INVALID;
 
@@ -251,8 +249,6 @@ static int mca_init_global(uint32_t flags, struct mcinfo_global *mig)
     const struct vcpu *curr = current;
 
     /* Set global information */
-    mig->common.type = MC_TYPE_GLOBAL;
-    mig->common.size = sizeof (struct mcinfo_global);
     status = mca_rdmsr(MSR_IA32_MCG_STATUS);
     mig->mc_gstatus = status;
     mig->mc_domid = DOMID_INVALID;
@@ -349,7 +345,7 @@ mcheck_mca_logout(enum mca_source who, struct mca_banks *bankmask,
             if ( (mctc = mctelem_reserve(which)) != NULL ) {
                 mci = mctelem_dataptr(mctc);
                 mcinfo_clear(mci);
-                mig = x86_mcinfo_reserve(mci, sizeof(*mig));
+                mig = x86_mcinfo_reserve(mci, sizeof(*mig), MC_TYPE_GLOBAL);
                 /* mc_info should at least hold up the global information */
                 ASSERT(mig);
                 mca_init_global(mc_flags, mig);
@@ -805,7 +801,8 @@ static void mcinfo_clear(struct mc_info *mi)
     x86_mcinfo_nentries(mi) = 0;
 }
 
-void *x86_mcinfo_reserve(struct mc_info *mi, int size)
+void *x86_mcinfo_reserve(struct mc_info *mi,
+                         unsigned int size, unsigned int type)
 {
     int i;
     unsigned long end1, end2;
@@ -832,7 +829,11 @@ void *x86_mcinfo_reserve(struct mc_info *mi, int size)
     /* there's enough space. add entry. */
     x86_mcinfo_nentries(mi)++;
 
-    return memset(mic_index, 0, size);
+    memset(mic_index, 0, size);
+    mic_index->size = size;
+    mic_index->type = type;
+
+    return mic_index;
 }
 
 static void x86_mcinfo_apei_save(
diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
index a28e862..b479b20 100644
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -150,7 +150,8 @@ typedef struct mcinfo_extended *(*x86_mce_callback_t)
     (struct mc_info *, uint16_t, uint64_t);
 extern void x86_mce_callback_register(x86_mce_callback_t);
 
-void *x86_mcinfo_reserve(struct mc_info *mi, int size);
+void *x86_mcinfo_reserve(struct mc_info *mi,
+                         unsigned int size, unsigned int type);
 void x86_mcinfo_dump(struct mc_info *mi);
 
 static inline int mce_vendor_bank_msr(const struct vcpu *v, uint32_t msr)
diff --git a/xen/arch/x86/cpu/mcheck/mce_amd.c b/xen/arch/x86/cpu/mcheck/mce_amd.c
index 599e465..fe51be9 100644
--- a/xen/arch/x86/cpu/mcheck/mce_amd.c
+++ b/xen/arch/x86/cpu/mcheck/mce_amd.c
@@ -218,15 +218,13 @@ amd_f10_handler(struct mc_info *mi, uint16_t bank, uint64_t status)
     if ( !(status & MCi_STATUS_MISCV) )
         return NULL;
 
-    mc_ext = x86_mcinfo_reserve(mi, sizeof(*mc_ext));
+    mc_ext = x86_mcinfo_reserve(mi, sizeof(*mc_ext), MC_TYPE_EXTENDED);
     if ( !mc_ext )
     {
         mi->flags |= MCINFO_FLAGS_UNCOMPLETE;
         return NULL;
     }
 
-    mc_ext->common.type = MC_TYPE_EXTENDED;
-    mc_ext->common.size = sizeof(*mc_ext);
     mc_ext->mc_msrs = 3;
 
     mc_ext->mc_msr[0].reg = MSR_F10_MC4_MISC1;
diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c b/xen/arch/x86/cpu/mcheck/mce_intel.c
index 005e41d..189c1ed 100644
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -200,17 +200,13 @@ intel_get_extended_msrs(struct mcinfo_global *mig, struct mc_info *mi)
             !(mig->mc_gstatus & MCG_STATUS_EIPV))
         return NULL;
 
-    mc_ext = x86_mcinfo_reserve(mi, sizeof(*mc_ext));
+    mc_ext = x86_mcinfo_reserve(mi, sizeof(*mc_ext), MC_TYPE_EXTENDED);
     if (!mc_ext)
     {
         mi->flags |= MCINFO_FLAGS_UNCOMPLETE;
         return NULL;
     }
 
-    /* this function will called when CAP(9).MCG_EXT_P = 1 */
-    mc_ext->common.type = MC_TYPE_EXTENDED;
-    mc_ext->common.size = sizeof(struct mcinfo_extended);
-
     for (i = MSR_IA32_MCG_EAX; i <= MSR_IA32_MCG_MISC; i++)
         intel_get_extended_msr(mc_ext, i);
 
-- 
2.10.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 4/7] x86/vmce: fill MSR_IA32_MCG_STATUS on all vcpus in broadcast case

[Haozhong Zhang]

The current implementation only fills MC MSRs on vcpu0 and leaves MC
MSRs on other vcpus empty in the broadcast case. When guest reads 0
from MSR_IA32_MCG_STATUS on vcpuN (N > 0), it may think it's not
possible to recover the execution on that vcpu and then get panic,
although MSR_IA32_MCG_STATUS filled on vcpu0 may imply the injected
vMCE is actually recoverable. To avoid such unnecessary guest panic,
set MSR_IA32_MCG_STATUS on vcpuN (N > 0) to MCG_STATUS_MCIP|MCG_STATUS_RIPV.

In addition, fill_vmsr_data(mc_bank, ...) is changed to return -EINVAL
rather than 0, if an invalid domain ID is contained in mc_bank.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Christoph Egger <chegger@amazon.de>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

Changes:
 * Fix the return value check of fill_vmsr_data() in mc_memerr_dhandler().
 * Include the behavior change in the commit message.
 * Compare by vcpu id rather than vcpu structure in fill_vmsr_data()
 * Exlpain vMCEs injected to each vCPU in the code comment.
 * Update MC MSRs for *all* vCPUs and return the last error (if any).
 * Remove "goto" in fill_vmsr_data().
---
 xen/arch/x86/cpu/mcheck/mcaction.c | 16 ++++-----
 xen/arch/x86/cpu/mcheck/vmce.c     | 74 ++++++++++++++++++++++++++------------
 xen/arch/x86/cpu/mcheck/vmce.h     |  2 +-
 3 files changed, 60 insertions(+), 32 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mcaction.c b/xen/arch/x86/cpu/mcheck/mcaction.c
index 32056f2..dab9eac 100644
--- a/xen/arch/x86/cpu/mcheck/mcaction.c
+++ b/xen/arch/x86/cpu/mcheck/mcaction.c
@@ -88,22 +88,22 @@ mc_memerr_dhandler(struct mca_binfo *binfo,
                     goto vmce_failed;
                 }
 
+                if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
+                    global->mc_vcpuid == XEN_MC_VCPUID_INVALID)
+                    vmce_vcpuid = VMCE_INJECT_BROADCAST;
+                else
+                    vmce_vcpuid = global->mc_vcpuid;
+
                 bank->mc_addr = gfn << PAGE_SHIFT |
                   (bank->mc_addr & (PAGE_SIZE -1 ));
-                if ( fill_vmsr_data(bank, d,
-                      global->mc_gstatus) == -1 )
+                if (fill_vmsr_data(bank, d, global->mc_gstatus,
+                                   vmce_vcpuid == VMCE_INJECT_BROADCAST))
                 {
                     mce_printk(MCE_QUIET, "Fill vMCE# data for DOM%d "
                       "failed\n", bank->mc_domid);
                     goto vmce_failed;
                 }
 
-                if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
-                    global->mc_vcpuid == XEN_MC_VCPUID_INVALID)
-                    vmce_vcpuid = VMCE_INJECT_BROADCAST;
-                else
-                    vmce_vcpuid = global->mc_vcpuid;
-
                 /* We will inject vMCE to DOMU*/
                 if ( inject_vmce(d, vmce_vcpuid) < 0 )
                 {
diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
index 13b692c..2caccae 100644
--- a/xen/arch/x86/cpu/mcheck/vmce.c
+++ b/xen/arch/x86/cpu/mcheck/vmce.c
@@ -381,38 +381,66 @@ int inject_vmce(struct domain *d, int vcpu)
     return ret;
 }
 
-int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d,
-                   uint64_t gstatus)
+static int vcpu_fill_mc_msrs(struct vcpu *v, uint64_t mcg_status,
+                             uint64_t mci_status, uint64_t mci_addr,
+                             uint64_t mci_misc)
 {
-    struct vcpu *v = d->vcpu[0];
-
-    if ( mc_bank->mc_domid != DOMID_INVALID )
+    if ( v->arch.vmce.mcg_status & MCG_STATUS_MCIP )
     {
-        if ( v->arch.vmce.mcg_status & MCG_STATUS_MCIP )
-        {
-            mce_printk(MCE_QUIET, "MCE: guest has not handled previous"
-                       " vMCE yet!\n");
-            return -1;
-        }
+        mce_printk(MCE_QUIET, "MCE: %pv: guest has not handled previous"
+                   " vMCE yet!\n", v);
+        return -EBUSY;
+    }
 
-        spin_lock(&v->arch.vmce.lock);
+    spin_lock(&v->arch.vmce.lock);
 
-        v->arch.vmce.mcg_status = gstatus;
-        /*
-         * 1. Skip bank 0 to avoid 'bank 0 quirk' of old processors
-         * 2. Filter MCi_STATUS MSCOD model specific error code to guest
-         */
-        v->arch.vmce.bank[1].mci_status = mc_bank->mc_status &
-                                              MCi_STATUS_MSCOD_MASK;
-        v->arch.vmce.bank[1].mci_addr = mc_bank->mc_addr;
-        v->arch.vmce.bank[1].mci_misc = mc_bank->mc_misc;
+    v->arch.vmce.mcg_status = mcg_status;
+    /*
+     * 1. Skip bank 0 to avoid 'bank 0 quirk' of old processors
+     * 2. Filter MCi_STATUS MSCOD model specific error code to guest
+     */
+    v->arch.vmce.bank[1].mci_status = mci_status & MCi_STATUS_MSCOD_MASK;
+    v->arch.vmce.bank[1].mci_addr = mci_addr;
+    v->arch.vmce.bank[1].mci_misc = mci_misc;
 
-        spin_unlock(&v->arch.vmce.lock);
-    }
+    spin_unlock(&v->arch.vmce.lock);
 
     return 0;
 }
 
+int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d,
+                   uint64_t gstatus, bool broadcast)
+{
+    struct vcpu *v = d->vcpu[0];
+    int ret, err;
+
+    if ( mc_bank->mc_domid == DOMID_INVALID )
+        return -EINVAL;
+
+    /*
+     * vMCE with the actual error information is injected to vCPU0,
+     * and, if broadcast is required, we choose to inject less severe
+     * vMCEs to other vCPUs. Thus guest can always get the severest
+     * error (i.e. the actual one) on vCPU0. If guest can recover from
+     * the severest error on vCPU0, the less severe errors on other
+     * vCPUs will not prevent guest from recovering on those vCPUs.
+     */
+    ret = vcpu_fill_mc_msrs(v, gstatus, mc_bank->mc_status,
+                            mc_bank->mc_addr, mc_bank->mc_misc);
+    if ( broadcast )
+        for_each_vcpu ( d, v )
+        {
+            if ( !v->vcpu_id )
+                continue;
+            err = vcpu_fill_mc_msrs(v, MCG_STATUS_MCIP | MCG_STATUS_RIPV,
+                                    0, 0, 0);
+            if ( err )
+                ret = err;
+        }
+
+    return ret;
+}
+
 /* It's said some ram is setup as mmio_direct for UC cache attribute */
 #define P2M_UNMAP_TYPES (p2m_to_mask(p2m_ram_rw) \
                                 | p2m_to_mask(p2m_ram_logdirty) \
diff --git a/xen/arch/x86/cpu/mcheck/vmce.h b/xen/arch/x86/cpu/mcheck/vmce.h
index 163ce3c..74f6381 100644
--- a/xen/arch/x86/cpu/mcheck/vmce.h
+++ b/xen/arch/x86/cpu/mcheck/vmce.h
@@ -17,7 +17,7 @@ int vmce_amd_rdmsr(const struct vcpu *, uint32_t msr, uint64_t *val);
 int vmce_amd_wrmsr(struct vcpu *, uint32_t msr, uint64_t val);
 
 int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d,
-    uint64_t gstatus);
+                   uint64_t gstatus, bool broadcast);
 
 #define VMCE_INJECT_BROADCAST (-1)
 int inject_vmce(struct domain *d, int vcpu);
-- 
2.10.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 5/7] x86/mce: clear MSR_IA32_MCG_STATUS by writing 0

[Haozhong Zhang]

On Intel CPU, an attemp to write to MSR_IA32_MCG_STATUS with any
non-zero value would result in #GP.

This commit writes 0 on AMD CPU as well instead of just clearing MCIP
bit, because all non-reserved bits of MSR_IA32_MCG_STATUS have been
handled at this point.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Christoph Egger <chegger@amazon.de>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Changes:
 * Write 0 on AMD as well.
 * Change the patch title to reflect the above change.
---
 xen/arch/x86/cpu/mcheck/mce.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 53ca29c..5a7e2ba 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -539,7 +539,7 @@ void mcheck_cmn_handler(const struct cpu_user_regs *regs)
     gstatus = mca_rdmsr(MSR_IA32_MCG_STATUS);
     if ((gstatus & MCG_STATUS_MCIP) != 0) {
         mce_printk(MCE_CRITICAL, "MCE: Clear MCIP@ last step");
-        mca_wrmsr(MSR_IA32_MCG_STATUS, gstatus & ~MCG_STATUS_MCIP);
+        mca_wrmsr(MSR_IA32_MCG_STATUS, 0);
     }
     mce_barrier_exit(&mce_trap_bar);
 
-- 
2.10.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 6/7] xen/mce: make ASSERT's about mce_dhandler_num in mce_action() Intel only

[Haozhong Zhang]

mce_dhandlers[] and its array size mce_dhandler_num are not used and
set on non-Intel platforms, so the corresponding ASSERT's in
mce_action() should be valid only for Intel.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Christoph Egger <chegger@amazon.de>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/cpu/mcheck/mce.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 5a7e2ba..81d0902 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -1621,8 +1621,8 @@ static enum mce_result mce_action(const struct cpu_user_regs *regs,
         handlers = mce_uhandlers;
     }
 
-    /* At least a default handler should be registerd */
-    ASSERT(handler_num);
+    /* At least a default handler should be registered for Intel CPU. */
+    ASSERT(handler_num || boot_cpu_data.x86_vendor != X86_VENDOR_INTEL);
 
     local_mi = (struct mc_info*)mctelem_dataptr(mctc);
     x86_mcinfo_lookup(mic, local_mi, MC_TYPE_GLOBAL);
@@ -1656,7 +1656,8 @@ static enum mce_result mce_action(const struct cpu_user_regs *regs,
                 break;
             }
         }
-        ASSERT(i != handler_num);
+        ASSERT(i != handler_num ||
+               boot_cpu_data.x86_vendor != X86_VENDOR_INTEL);
     }
 
     return worst_result;
-- 
2.10.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 7/7] tools/xen-mceinj: fix the type of cpu number

[Haozhong Zhang]

Use "unsigned int" rather than "int" to align to the type "uint32_t"
of xen_mc_physcpuinfo.ncpus.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>

Changes:
 * Replace "uint32_t" by "unsigned int".
---
 tools/tests/mce-test/tools/xen-mceinj.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/tests/mce-test/tools/xen-mceinj.c b/tools/tests/mce-test/tools/xen-mceinj.c
index 51abc8a..bae5a46 100644
--- a/tools/tests/mce-test/tools/xen-mceinj.c
+++ b/tools/tests/mce-test/tools/xen-mceinj.c
@@ -161,7 +161,7 @@ static int flush_msr_inj(xc_interface *xc_handle)
     return xc_mca_op(xc_handle, &mc);
 }
 
-static int mca_cpuinfo(xc_interface *xc_handle)
+static unsigned int mca_cpuinfo(xc_interface *xc_handle)
 {
     struct xen_mc mc;
 
@@ -176,16 +176,18 @@ static int mca_cpuinfo(xc_interface *xc_handle)
         return 0;
 }
 
-static int inject_cmci(xc_interface *xc_handle, int cpu_nr)
+static int inject_cmci(xc_interface *xc_handle, unsigned int cpu_nr)
 {
     struct xen_mc mc;
-    int nr_cpus;
+    unsigned int nr_cpus;
 
     memset(&mc, 0, sizeof(struct xen_mc));
 
     nr_cpus = mca_cpuinfo(xc_handle);
     if (!nr_cpus)
         err(xc_handle, "Failed to get mca_cpuinfo");
+    if (cpu_nr >= nr_cpus)
+        err(xc_handle, "-c %u is larger than %u", cpu_nr, nr_cpus - 1);
 
     mc.cmd = XEN_MC_inject_v2;
     mc.interface_version = XEN_MCA_INTERFACE_VERSION;
@@ -420,7 +422,7 @@ int main(int argc, char *argv[])
     int c, opt_index;
     uint32_t domid;
     xc_interface *xc_handle;
-    int cpu_nr;
+    unsigned int cpu_nr;
     uint64_t gaddr, max_gpa;
 
     /* Default Value */
@@ -444,7 +446,7 @@ int main(int argc, char *argv[])
             dump=1;
             break;
         case 'c':
-            cpu_nr = strtol(optarg, &optarg, 10);
+            cpu_nr = strtoul(optarg, &optarg, 10);
             if ( strlen(optarg) != 0 )
                 err(xc_handle, "Please input a digit parameter for CPU");
             break;
-- 
2.10.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 5/7] x86/mce: clear MSR_IA32_MCG_STATUS by writing 0

[Boris Ostrovsky]

On 02/24/2017 05:52 AM, Haozhong Zhang wrote:
> On Intel CPU, an attemp to write to MSR_IA32_MCG_STATUS with any
> non-zero value would result in #GP.
>
> This commit writes 0 on AMD CPU as well instead of just clearing MCIP
> bit, because all non-reserved bits of MSR_IA32_MCG_STATUS have been
> handled at this point.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/7] xen/mce: remove unused x86_mcinfo_add()

[Jan Beulich]

>>> On 24.02.17 at 11:52, <haozhong.zhang@intel.com> wrote:
> c/s 9d13fd9fd320a7740c6446c048ff6a2990095966 turned to update the
> mcinfo buffer in-place instead of using x86_mcinfo_add(). The last
> uses of x86_mcinfo_add() were removed by that commit as well.
> Therefore, x86_mcinfo_add() was deprecated in fact.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/7] x86/mce: set mcinfo_comm.type and .size in x86_mcinfo_reserve()

[Jan Beulich]

>>> On 24.02.17 at 11:52, <haozhong.zhang@intel.com> wrote:
> All existing calls to x86_mcinfo_reserve() are followed by statements
> that set the size and the type of the reserved space, so move them into
> x86_mcinfo_reserve() to simplify the code.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/7] x86/vmce: fill MSR_IA32_MCG_STATUS on all vcpus in broadcast case

[Jan Beulich]

>>> On 24.02.17 at 11:52, <haozhong.zhang@intel.com> wrote:
> The current implementation only fills MC MSRs on vcpu0 and leaves MC
> MSRs on other vcpus empty in the broadcast case. When guest reads 0
> from MSR_IA32_MCG_STATUS on vcpuN (N > 0), it may think it's not
> possible to recover the execution on that vcpu and then get panic,
> although MSR_IA32_MCG_STATUS filled on vcpu0 may imply the injected
> vMCE is actually recoverable. To avoid such unnecessary guest panic,
> set MSR_IA32_MCG_STATUS on vcpuN (N > 0) to MCG_STATUS_MCIP|MCG_STATUS_RIPV.
> 
> In addition, fill_vmsr_data(mc_bank, ...) is changed to return -EINVAL
> rather than 0, if an invalid domain ID is contained in mc_bank.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 5/7] x86/mce: clear MSR_IA32_MCG_STATUS by writing 0

[Jan Beulich]

>>> On 24.02.17 at 11:52, <haozhong.zhang@intel.com> wrote:
> On Intel CPU, an attemp to write to MSR_IA32_MCG_STATUS with any
> non-zero value would result in #GP.
> 
> This commit writes 0 on AMD CPU as well instead of just clearing MCIP
> bit, because all non-reserved bits of MSR_IA32_MCG_STATUS have been
> handled at this point.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 6/7] xen/mce: make ASSERT's about mce_dhandler_num in mce_action() Intel only

[Jan Beulich]

>>> On 24.02.17 at 11:52, <haozhong.zhang@intel.com> wrote:
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -1621,8 +1621,8 @@ static enum mce_result mce_action(const struct cpu_user_regs *regs,
>          handlers = mce_uhandlers;
>      }
>  
> -    /* At least a default handler should be registerd */
> -    ASSERT(handler_num);
> +    /* At least a default handler should be registered for Intel CPU. */
> +    ASSERT(handler_num || boot_cpu_data.x86_vendor != X86_VENDOR_INTEL);
>  
>      local_mi = (struct mc_info*)mctelem_dataptr(mctc);
>      x86_mcinfo_lookup(mic, local_mi, MC_TYPE_GLOBAL);
> @@ -1656,7 +1656,8 @@ static enum mce_result mce_action(const struct cpu_user_regs *regs,
>                  break;
>              }
>          }
> -        ASSERT(i != handler_num);
> +        ASSERT(i != handler_num ||
> +               boot_cpu_data.x86_vendor != X86_VENDOR_INTEL);
>      }
>  
>      return worst_result;

I think both ASSERT()s should simply be removed: The code is fine
with the conditions they check violated, and debug builds can't have
worked due to their presence on AMD at all (yet production builds
would have been fine).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 7/7] tools/xen-mceinj: fix the type of cpu number

[Wei Liu]

On Fri, Feb 24, 2017 at 06:52:56PM +0800, Haozhong Zhang wrote:
> Use "unsigned int" rather than "int" to align to the type "uint32_t"
> of xen_mc_physcpuinfo.ncpus.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel