[PATCH 0/4] Fix tools/xen-mceinj to handle >=4GB guest memory

[PATCH 0/4] Fix tools/xen-mceinj to handle >=4GB guest memory

[Haozhong Zhang]

The existing xen-mceinj can not inject MCE through MSR_MCI_ADDR to a
domain w/ more than 4GB memory, e.g. if domain 0 has more than 4GB
memory, the execution of the command
        xen-mceinj -d 0 -t 0 -p 0x2721a900
will fail w/ a message "Failed to get pfn list ffffffff: Operation not
supported".

The cause is that the hypercall XEN_DOMCTL_getmemlist used by
xen-mceinj to translate the guest physical address (argument of '-p')
to the host machine address always fails if the domain has more than
4GB memory due to the mitigation of XSA-74.

This patchset fixes this problem by moving the translation from
xen-mceinj to the hypervisor, so that it is not necessary to use
XEN_DOMCTL_getmemlist.

The first two patches just fix serval code-style issues, while the
other two are the actual fix.

Haozhong Zhang (4):
  x86/mce: Fix code style
  tools/mceinject: Fix code style
  x86/mce: Translate passed-in GPA to host machine address
  tools/xen-mceinj: Pass in GPA when injecting through MSR_MCI_ADDR

 tools/tests/mce-test/tools/xen-mceinj.c | 141 +++++---------------------------
 xen/arch/x86/cpu/mcheck/mce.c           |  43 ++++++++--
 xen/include/public/arch-x86/xen-mca.h   |  31 +++----
 3 files changed, 77 insertions(+), 138 deletions(-)

--
2.4.8

[PATCH 1/4] x86/mce: Fix code style

[Haozhong Zhang]

Remove trailing spaces and fix indentations in mce.c and xen-mca.h.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 xen/arch/x86/cpu/mcheck/mce.c         | 10 +++++-----
 xen/include/public/arch-x86/xen-mca.h | 28 ++++++++++++++--------------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 7c2cacc..561257d 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -105,7 +105,7 @@ void x86_mce_callback_register(x86_mce_callback_t cbfunc)
     mc_callback_bank_extended = cbfunc;
 }
 
-/* Machine check recoverable judgement callback handler
+/* Machine check recoverable judgement callback handler
  * It is used to judge whether an UC error is recoverable by software
  */
 static mce_recoverable_t mc_recoverable_scan = NULL;
@@ -160,9 +160,9 @@ static void mcabank_clear(int banknum)
 }
 
 /* Judging whether to Clear Machine Check error bank callback handler
- * According to Intel latest MCA OS Recovery Writer's Guide,
+ * According to Intel latest MCA OS Recovery Writer's Guide,
  * whether the error MCA bank needs to be cleared is decided by the mca_source
- * and MCi_status bit value.
+ * and MCi_status bit value.
  */
 static mce_need_clearbank_t mc_need_clearbank_scan = NULL;
 
@@ -535,7 +535,7 @@ void mcheck_cmn_handler(const struct cpu_user_regs *regs)
         }
         atomic_set(&found_error, 0);
     }
-    mce_barrier_exit(&mce_trap_bar);
+    mce_barrier_exit(&mce_trap_bar);
 
     /* Clear flags after above fatal check */
     mce_barrier_enter(&mce_trap_bar);
@@ -891,7 +891,7 @@ void x86_mcinfo_dump(struct mc_info *mi)
                "CPU%d: Machine Check Exception: %16"PRIx64"\n",
                mc_global->mc_coreid, mc_global->mc_gstatus);
     } else if (mc_global->mc_flags & MC_FLAG_CMCI) {
-        printk(XENLOG_WARNING "CMCI occurred on CPU %d.\n",
+        printk(XENLOG_WARNING "CMCI occurred on CPU %d.\n",
                mc_global->mc_coreid);
     } else if (mc_global->mc_flags & MC_FLAG_POLLED) {
         printk(XENLOG_WARNING "POLLED occurred on CPU %d.\n",
diff --git a/xen/include/public/arch-x86/xen-mca.h b/xen/include/public/arch-x86/xen-mca.h
index 04382ed..2422b76 100644
--- a/xen/include/public/arch-x86/xen-mca.h
+++ b/xen/include/public/arch-x86/xen-mca.h
@@ -1,11 +1,11 @@
 /******************************************************************************
  * arch-x86/mca.h
- *
+ *
  * Contributed by Advanced Micro Devices, Inc.
  * Author: Christoph Egger <Christoph.Egger@amd.com>
  *
  * Guest OS machine check interface to x86 Xen.
- *
+ *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to
  * deal in the Software without restriction, including without limitation the
@@ -156,7 +156,7 @@ struct mcinfo_msr {
 };
 
 /* contains mc information from other
- * or additional mc MSRs */
+ * or additional mc MSRs */
 struct mcinfo_extended {
     struct mcinfo_common common;
 
@@ -193,10 +193,10 @@ struct mcinfo_extended {
 /* L3 cache disable Action */
 #define MC_ACTION_CACHE_SHRINK (0x1 << 2)
 
-/* Below interface used between XEN/DOM0 for passing XEN's recovery action
- * information to DOM0.
+/* Below interface used between XEN/DOM0 for passing XEN's recovery action
+ * information to DOM0.
  * usage Senario: After offlining broken page, XEN might pass its page offline
- * recovery action result to DOM0. DOM0 will save the information in
+ * recovery action result to DOM0. DOM0 will save the information in
  * non-volatile memory for further proactive actions, such as offlining the
  * easy broken page earlier when doing next reboot.
 */
@@ -255,8 +255,8 @@ DEFINE_XEN_GUEST_HANDLE(mc_info_t);
 #define MC_CAPS_AMD_ECX	6	/* cpuid level 0x80000001 (%ecx) */
 
 struct mcinfo_logical_cpu {
-    uint32_t mc_cpunr;
-    uint32_t mc_chipid;
+    uint32_t mc_cpunr;
+    uint32_t mc_chipid;
     uint16_t mc_coreid;
     uint16_t mc_threadid;
     uint32_t mc_apicid;
@@ -281,7 +281,7 @@ typedef struct mcinfo_logical_cpu xen_mc_logical_cpu_t;
 DEFINE_XEN_GUEST_HANDLE(xen_mc_logical_cpu_t);
 
 
-/*
+/*
  * OS's should use these instead of writing their own lookup function
  * each with its own bugs and drawbacks.
  * We use macros instead of static inline functions to allow guests
@@ -389,11 +389,11 @@ struct xen_mc_physcpuinfo {
 #define MC_MSRINJ_MAXMSRS       8
 struct xen_mc_msrinject {
        /* IN */
-	uint32_t mcinj_cpunr;           /* target processor id */
-	uint32_t mcinj_flags;           /* see MC_MSRINJ_F_* below */
-	uint32_t mcinj_count;           /* 0 .. count-1 in array are valid */
-	uint32_t _pad0;
-	struct mcinfo_msr mcinj_msr[MC_MSRINJ_MAXMSRS];
+    uint32_t mcinj_cpunr;           /* target processor id */
+    uint32_t mcinj_flags;           /* see MC_MSRINJ_F_* below */
+    uint32_t mcinj_count;           /* 0 .. count-1 in array are valid */
+    uint32_t _pad0;
+    struct mcinfo_msr mcinj_msr[MC_MSRINJ_MAXMSRS];
 };
 
 /* Flags for mcinj_flags above; bits 16-31 are reserved */
--
2.4.8

Re: [PATCH 1/4] x86/mce: Fix code style

[Egger, Christoph]

On 2015/09/15 10:29, Haozhong Zhang wrote:
> Remove trailing spaces and fix indentations in mce.c and xen-mca.h.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Acked-by: Christoph Egger <chegger@amazon.de>

> ---
>  xen/arch/x86/cpu/mcheck/mce.c         | 10 +++++-----
>  xen/include/public/arch-x86/xen-mca.h | 28 ++++++++++++++--------------
>  2 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
> index 7c2cacc..561257d 100644
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -105,7 +105,7 @@ void x86_mce_callback_register(x86_mce_callback_t cbfunc)
>      mc_callback_bank_extended = cbfunc;
>  }
>  
> -/* Machine check recoverable judgement callback handler
> +/* Machine check recoverable judgement callback handler
>   * It is used to judge whether an UC error is recoverable by software
>   */
>  static mce_recoverable_t mc_recoverable_scan = NULL;
> @@ -160,9 +160,9 @@ static void mcabank_clear(int banknum)
>  }
>  
>  /* Judging whether to Clear Machine Check error bank callback handler
> - * According to Intel latest MCA OS Recovery Writer's Guide,
> + * According to Intel latest MCA OS Recovery Writer's Guide,
>   * whether the error MCA bank needs to be cleared is decided by the mca_source
> - * and MCi_status bit value.
> + * and MCi_status bit value.
>   */
>  static mce_need_clearbank_t mc_need_clearbank_scan = NULL;
>  
> @@ -535,7 +535,7 @@ void mcheck_cmn_handler(const struct cpu_user_regs *regs)
>          }
>          atomic_set(&found_error, 0);
>      }
> -    mce_barrier_exit(&mce_trap_bar);
> +    mce_barrier_exit(&mce_trap_bar);
>  
>      /* Clear flags after above fatal check */
>      mce_barrier_enter(&mce_trap_bar);
> @@ -891,7 +891,7 @@ void x86_mcinfo_dump(struct mc_info *mi)
>                 "CPU%d: Machine Check Exception: %16"PRIx64"\n",
>                 mc_global->mc_coreid, mc_global->mc_gstatus);
>      } else if (mc_global->mc_flags & MC_FLAG_CMCI) {
> -        printk(XENLOG_WARNING "CMCI occurred on CPU %d.\n",
> +        printk(XENLOG_WARNING "CMCI occurred on CPU %d.\n",
>                 mc_global->mc_coreid);
>      } else if (mc_global->mc_flags & MC_FLAG_POLLED) {
>          printk(XENLOG_WARNING "POLLED occurred on CPU %d.\n",
> diff --git a/xen/include/public/arch-x86/xen-mca.h b/xen/include/public/arch-x86/xen-mca.h
> index 04382ed..2422b76 100644
> --- a/xen/include/public/arch-x86/xen-mca.h
> +++ b/xen/include/public/arch-x86/xen-mca.h
> @@ -1,11 +1,11 @@
>  /******************************************************************************
>   * arch-x86/mca.h
> - *
> + *
>   * Contributed by Advanced Micro Devices, Inc.
>   * Author: Christoph Egger <Christoph.Egger@amd.com>
>   *
>   * Guest OS machine check interface to x86 Xen.
> - *
> + *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to
>   * deal in the Software without restriction, including without limitation the
> @@ -156,7 +156,7 @@ struct mcinfo_msr {
>  };
>  
>  /* contains mc information from other
> - * or additional mc MSRs */
> + * or additional mc MSRs */
>  struct mcinfo_extended {
>      struct mcinfo_common common;
>  
> @@ -193,10 +193,10 @@ struct mcinfo_extended {
>  /* L3 cache disable Action */
>  #define MC_ACTION_CACHE_SHRINK (0x1 << 2)
>  
> -/* Below interface used between XEN/DOM0 for passing XEN's recovery action
> - * information to DOM0.
> +/* Below interface used between XEN/DOM0 for passing XEN's recovery action
> + * information to DOM0.
>   * usage Senario: After offlining broken page, XEN might pass its page offline
> - * recovery action result to DOM0. DOM0 will save the information in
> + * recovery action result to DOM0. DOM0 will save the information in
>   * non-volatile memory for further proactive actions, such as offlining the
>   * easy broken page earlier when doing next reboot.
>  */
> @@ -255,8 +255,8 @@ DEFINE_XEN_GUEST_HANDLE(mc_info_t);
>  #define MC_CAPS_AMD_ECX	6	/* cpuid level 0x80000001 (%ecx) */
>  
>  struct mcinfo_logical_cpu {
> -    uint32_t mc_cpunr;
> -    uint32_t mc_chipid;
> +    uint32_t mc_cpunr;
> +    uint32_t mc_chipid;
>      uint16_t mc_coreid;
>      uint16_t mc_threadid;
>      uint32_t mc_apicid;
> @@ -281,7 +281,7 @@ typedef struct mcinfo_logical_cpu xen_mc_logical_cpu_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_mc_logical_cpu_t);
>  
>  
> -/*
> +/*
>   * OS's should use these instead of writing their own lookup function
>   * each with its own bugs and drawbacks.
>   * We use macros instead of static inline functions to allow guests
> @@ -389,11 +389,11 @@ struct xen_mc_physcpuinfo {
>  #define MC_MSRINJ_MAXMSRS       8
>  struct xen_mc_msrinject {
>         /* IN */
> -	uint32_t mcinj_cpunr;           /* target processor id */
> -	uint32_t mcinj_flags;           /* see MC_MSRINJ_F_* below */
> -	uint32_t mcinj_count;           /* 0 .. count-1 in array are valid */
> -	uint32_t _pad0;
> -	struct mcinfo_msr mcinj_msr[MC_MSRINJ_MAXMSRS];
> +    uint32_t mcinj_cpunr;           /* target processor id */
> +    uint32_t mcinj_flags;           /* see MC_MSRINJ_F_* below */
> +    uint32_t mcinj_count;           /* 0 .. count-1 in array are valid */
> +    uint32_t _pad0;
> +    struct mcinfo_msr mcinj_msr[MC_MSRINJ_MAXMSRS];
>  };
>  
>  /* Flags for mcinj_flags above; bits 16-31 are reserved */
> --
> 2.4.8
> 

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH 1/4] x86/mce: Fix code style

[Jan Beulich]

>>> On 15.09.15 at 10:29, <haozhong.zhang@intel.com> wrote:
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -105,7 +105,7 @@ void x86_mce_callback_register(x86_mce_callback_t cbfunc)
>      mc_callback_bank_extended = cbfunc;
>  }
>  
> -/* Machine check recoverable judgement callback handler
> +/* Machine check recoverable judgement callback handler

At the example of this one: I'm afraid your mailer ate the
trailing white space (what arrived in my inbox as well as what I
can see on the xen-devel archives has both lines without any
trailing white space). Which makes the submission unusable.
Please fix you mailer or send the patch as an attachment.

Jan

Re: [PATCH 1/4] x86/mce: Fix code style

[Haozhong Zhang]

On Tue, Sep 15, 2015 at 07:13:16AM -0600, Jan Beulich wrote:
> >>> On 15.09.15 at 10:29, <haozhong.zhang@intel.com> wrote:
> > --- a/xen/arch/x86/cpu/mcheck/mce.c
> > +++ b/xen/arch/x86/cpu/mcheck/mce.c
> > @@ -105,7 +105,7 @@ void x86_mce_callback_register(x86_mce_callback_t cbfunc)
> >      mc_callback_bank_extended = cbfunc;
> >  }
> >
> > -/* Machine check recoverable judgement callback handler
> > +/* Machine check recoverable judgement callback handler
>
> At the example of this one: I'm afraid your mailer ate the
> trailing white space (what arrived in my inbox as well as what I
> can see on the xen-devel archives has both lines without any
> trailing white space). Which makes the submission unusable.
> Please fix you mailer or send the patch as an attachment.
>

I'll resend correct ones in the next version. Thanks for the notification.

> Jan
>

[PATCH 2/4] tools/mceinject: Fix code style

[Haozhong Zhang]

Remove trailing spaces in xen-mceinj.c.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 tools/tests/mce-test/tools/xen-mceinj.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/tests/mce-test/tools/xen-mceinj.c b/tools/tests/mce-test/tools/xen-mceinj.c
index e2e49cb..0c2b640 100644
--- a/tools/tests/mce-test/tools/xen-mceinj.c
+++ b/tools/tests/mce-test/tools/xen-mceinj.c
@@ -13,7 +13,7 @@
  *
  * You should have received a copy of the GNU General Public License along with
  * this program; If not, see <http://www.gnu.org/licenses/>.
- * 
+ *
  * Authors: Yunhong Jiang <yunhong.jiang@intel.com>
  *          Haicheng Li <haicheng.li@intel.com>
  *          Xudong Hao <xudong.hao@intel.com>
@@ -300,7 +300,7 @@ static uint64_t guest_mfn(xc_interface *xc_handle,
         return MCE_INVALID_MFN;
 
     /* Get max gpfn */
-    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain, 
+    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
                                sizeof(domain)) + 1;
     if ( max_gpfn <= 0 )
         err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
@@ -360,7 +360,7 @@ static uint64_t mca_gpfn_to_mfn(xc_interface *xc_handle,
     if ( domain == DOMID_XEN )
         return gfn;
 
-    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain, 
+    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
                                sizeof(domain)) + 1;
     if ( max_gpfn <= 0 )
         err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
@@ -383,7 +383,7 @@ static int inject_mci_status(xc_interface *xc_handle,
                              uint64_t val)
 {
     return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
-                                    MCi_type_STATUS, bank, val); 
+                                    MCi_type_STATUS, bank, val);
 }
 
 static int inject_mci_misc(xc_interface *xc_handle,
@@ -392,7 +392,7 @@ static int inject_mci_misc(xc_interface *xc_handle,
                              uint64_t val)
 {
     return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
-                                    MCi_type_MISC, bank, val); 
+                                    MCi_type_MISC, bank, val);
 }
 
 static int inject_mci_addr(xc_interface *xc_handle,
@@ -401,7 +401,7 @@ static int inject_mci_addr(xc_interface *xc_handle,
                              uint64_t val)
 {
     return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
-                                    MCi_type_ADDR, bank, val); 
+                                    MCi_type_ADDR, bank, val);
 }
 
 static int inject(xc_interface *xc_handle, struct mce_info *mce,
@@ -553,7 +553,7 @@ int main(int argc, char *argv[])
             return 0;
         }
     }
-    
+
     if ( domid != DOMID_XEN ) {
         max_gpa = xs_get_dom_mem(domid);
         Lprintf("get domain %d max gpa is: 0x%lx", domid, max_gpa);
@@ -570,7 +570,7 @@ int main(int argc, char *argv[])
         haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
         if ( domid == DOMID_XEN )
             Lprintf("Xen: mfn=0x%lx, haddr=0x%lx", mfn, haddr);
-        else 
+        else
             Lprintf("Dom%d: gaddr=0x%lx, gpfn=0x%lx, mfn=0x%lx, haddr=0x%lx",
                     domid, gaddr, gpfn, mfn, haddr);
         goto out;
-- 
2.4.8

Re: [PATCH 2/4] tools/mceinject: Fix code style

[Egger, Christoph]

On 2015/09/15 10:29, Haozhong Zhang wrote:
> Remove trailing spaces in xen-mceinj.c.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Acked-by: Christoph Egger <chegger@amazon.de>

> ---
>  tools/tests/mce-test/tools/xen-mceinj.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/tests/mce-test/tools/xen-mceinj.c b/tools/tests/mce-test/tools/xen-mceinj.c
> index e2e49cb..0c2b640 100644
> --- a/tools/tests/mce-test/tools/xen-mceinj.c
> +++ b/tools/tests/mce-test/tools/xen-mceinj.c
> @@ -13,7 +13,7 @@
>   *
>   * You should have received a copy of the GNU General Public License along with
>   * this program; If not, see <http://www.gnu.org/licenses/>.
> - * 
> + *
>   * Authors: Yunhong Jiang <yunhong.jiang@intel.com>
>   *          Haicheng Li <haicheng.li@intel.com>
>   *          Xudong Hao <xudong.hao@intel.com>
> @@ -300,7 +300,7 @@ static uint64_t guest_mfn(xc_interface *xc_handle,
>          return MCE_INVALID_MFN;
>  
>      /* Get max gpfn */
> -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain, 
> +    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
>                                 sizeof(domain)) + 1;
>      if ( max_gpfn <= 0 )
>          err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
> @@ -360,7 +360,7 @@ static uint64_t mca_gpfn_to_mfn(xc_interface *xc_handle,
>      if ( domain == DOMID_XEN )
>          return gfn;
>  
> -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain, 
> +    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
>                                 sizeof(domain)) + 1;
>      if ( max_gpfn <= 0 )
>          err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
> @@ -383,7 +383,7 @@ static int inject_mci_status(xc_interface *xc_handle,
>                               uint64_t val)
>  {
>      return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> -                                    MCi_type_STATUS, bank, val); 
> +                                    MCi_type_STATUS, bank, val);
>  }
>  
>  static int inject_mci_misc(xc_interface *xc_handle,
> @@ -392,7 +392,7 @@ static int inject_mci_misc(xc_interface *xc_handle,
>                               uint64_t val)
>  {
>      return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> -                                    MCi_type_MISC, bank, val); 
> +                                    MCi_type_MISC, bank, val);
>  }
>  
>  static int inject_mci_addr(xc_interface *xc_handle,
> @@ -401,7 +401,7 @@ static int inject_mci_addr(xc_interface *xc_handle,
>                               uint64_t val)
>  {
>      return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> -                                    MCi_type_ADDR, bank, val); 
> +                                    MCi_type_ADDR, bank, val);
>  }
>  
>  static int inject(xc_interface *xc_handle, struct mce_info *mce,
> @@ -553,7 +553,7 @@ int main(int argc, char *argv[])
>              return 0;
>          }
>      }
> -    
> +
>      if ( domid != DOMID_XEN ) {
>          max_gpa = xs_get_dom_mem(domid);
>          Lprintf("get domain %d max gpa is: 0x%lx", domid, max_gpa);
> @@ -570,7 +570,7 @@ int main(int argc, char *argv[])
>          haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
>          if ( domid == DOMID_XEN )
>              Lprintf("Xen: mfn=0x%lx, haddr=0x%lx", mfn, haddr);
> -        else 
> +        else
>              Lprintf("Dom%d: gaddr=0x%lx, gpfn=0x%lx, mfn=0x%lx, haddr=0x%lx",
>                      domid, gaddr, gpfn, mfn, haddr);
>          goto out;
> 

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH 2/4] tools/mceinject: Fix code style

[Ian Campbell]

On Tue, 2015-09-15 at 10:48 +0200, Egger, Christoph wrote:
> On 2015/09/15 10:29, Haozhong Zhang wrote:
> > Remove trailing spaces in xen-mceinj.c.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> 
> Acked-by: Christoph Egger <chegger@amazon.de>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 2/4] tools/mceinject: Fix code style

[Wei Liu]

On Tue, Sep 15, 2015 at 04:29:38PM +0800, Haozhong Zhang wrote:
> Remove trailing spaces in xen-mceinj.c.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

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

[PATCH 3/4] x86/mce: Translate passed-in GPA to host machine address

[Haozhong Zhang]

This patch adds a new flag MC_MSRINJ_F_GPADDR to
xen_mc_msrinject.mcinj_flags, and makes do_mca() to translate the
guest physical address passed-in through
xen_mc_msrinject.mcinj_msr[i].value to the host machine address if
this flag is present.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 xen/arch/x86/cpu/mcheck/mce.c         | 33 +++++++++++++++++++++++++++++++++
 xen/include/public/arch-x86/xen-mca.h |  5 ++++-
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 561257d..343d9d2 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -21,6 +21,7 @@
 #include <asm/processor.h>
 #include <asm/system.h>
 #include <asm/msr.h>
+#include <asm/p2m.h>
 
 #include "mce.h"
 #include "barrier.h"
@@ -1422,6 +1423,38 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc)
         if (mc_msrinject->mcinj_count == 0)
             return 0;
 
+        if (mc_msrinject->mcinj_flags & MC_MSRINJ_F_GPADDR) {
+            struct domain *d;
+            struct mcinfo_msr *msr;
+            int i;
+            uint64_t gaddr, gpfn, mfn, haddr;
+            p2m_type_t t;
+
+            d = get_domain_by_id(mc_msrinject->mcinj_domid);
+            if (d == NULL)
+                return x86_mcerr("do_mca inject: illegal domain id", -EINVAL);
+
+            for (i = 0, msr = &mc_msrinject->mcinj_msr[0];
+                 i < mc_msrinject->mcinj_count; i++, msr++) {
+                gaddr = msr->value;
+                gpfn = gaddr >> PAGE_SHIFT;
+                mfn = mfn_x(get_gfn(d, gpfn, &t));
+
+                if (mfn == INVALID_MFN) {
+                    put_domain(d);
+                    return x86_mcerr("do_mca inject: illegal MSR value",
+                                     -EINVAL);
+                }
+
+                haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
+                msr->value = haddr;
+
+                put_gfn(d, gpfn);
+            }
+
+            put_domain(d);
+        }
+
         if (!x86_mc_msrinject_verify(mc_msrinject))
             return x86_mcerr("do_mca inject: illegal MSR", -EINVAL);
 
diff --git a/xen/include/public/arch-x86/xen-mca.h b/xen/include/public/arch-x86/xen-mca.h
index 2422b76..33c1a84 100644
--- a/xen/include/public/arch-x86/xen-mca.h
+++ b/xen/include/public/arch-x86/xen-mca.h
@@ -392,12 +392,15 @@ struct xen_mc_msrinject {
     uint32_t mcinj_cpunr;           /* target processor id */
     uint32_t mcinj_flags;           /* see MC_MSRINJ_F_* below */
     uint32_t mcinj_count;           /* 0 .. count-1 in array are valid */
-    uint32_t _pad0;
+    domid_t  mcinj_domid;           /* valid only if MC_MSRINJ_F_GPADDR presents
+                                       in mcinj_flags */
+    uint16_t _pad0;
     struct mcinfo_msr mcinj_msr[MC_MSRINJ_MAXMSRS];
 };
 
 /* Flags for mcinj_flags above; bits 16-31 are reserved */
 #define MC_MSRINJ_F_INTERPOSE   0x1
+#define MC_MSRINJ_F_GPADDR      0x2
 
 #define XEN_MC_mceinject    5
 struct xen_mc_mceinject {
-- 
2.4.8

Re: [PATCH 3/4] x86/mce: Translate passed-in GPA to host machine address

[Egger, Christoph]

On 2015/09/15 10:29, Haozhong Zhang wrote:
> This patch adds a new flag MC_MSRINJ_F_GPADDR to
> xen_mc_msrinject.mcinj_flags, and makes do_mca() to translate the
> guest physical address passed-in through
> xen_mc_msrinject.mcinj_msr[i].value to the host machine address if
> this flag is present.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Comments inline.

> ---
>  xen/arch/x86/cpu/mcheck/mce.c         | 33 +++++++++++++++++++++++++++++++++
>  xen/include/public/arch-x86/xen-mca.h |  5 ++++-
>  2 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
> index 561257d..343d9d2 100644
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -21,6 +21,7 @@
>  #include <asm/processor.h>
>  #include <asm/system.h>
>  #include <asm/msr.h>
> +#include <asm/p2m.h>
>  
>  #include "mce.h"
>  #include "barrier.h"
> @@ -1422,6 +1423,38 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc)
>          if (mc_msrinject->mcinj_count == 0)
>              return 0;
>  
> +        if (mc_msrinject->mcinj_flags & MC_MSRINJ_F_GPADDR) {
> +            struct domain *d;
> +            struct mcinfo_msr *msr;
> +            int i;
> +            uint64_t gaddr, gpfn, mfn, haddr;
> +            p2m_type_t t;
> +
> +            d = get_domain_by_id(mc_msrinject->mcinj_domid);
> +            if (d == NULL)
> +                return x86_mcerr("do_mca inject: illegal domain id", -EINVAL);
> +
> +            for (i = 0, msr = &mc_msrinject->mcinj_msr[0];
> +                 i < mc_msrinject->mcinj_count; i++, msr++) {
> +                gaddr = msr->value;
> +                gpfn = gaddr >> PAGE_SHIFT;
> +                mfn = mfn_x(get_gfn(d, gpfn, &t));
> +
> +                if (mfn == INVALID_MFN) {

                       put_gfn(d, gpfn);

> +                    put_domain(d);
> +                    return x86_mcerr("do_mca inject: illegal MSR value",
> +                                     -EINVAL);
> +                }
> +
> +                haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
> +                msr->value = haddr;
> +
> +                put_gfn(d, gpfn);
> +            }
> +
> +            put_domain(d);
> +        }
> +
>          if (!x86_mc_msrinject_verify(mc_msrinject))
>              return x86_mcerr("do_mca inject: illegal MSR", -EINVAL);
>  
> diff --git a/xen/include/public/arch-x86/xen-mca.h b/xen/include/public/arch-x86/xen-mca.h
> index 2422b76..33c1a84 100644
> --- a/xen/include/public/arch-x86/xen-mca.h
> +++ b/xen/include/public/arch-x86/xen-mca.h
> @@ -392,12 +392,15 @@ struct xen_mc_msrinject {
>      uint32_t mcinj_cpunr;           /* target processor id */
>      uint32_t mcinj_flags;           /* see MC_MSRINJ_F_* below */
>      uint32_t mcinj_count;           /* 0 .. count-1 in array are valid */
> -    uint32_t _pad0;
> +    domid_t  mcinj_domid;           /* valid only if MC_MSRINJ_F_GPADDR presents
> +                                       in mcinj_flags */

wording. I prefer "s/presents/is present/"

> +    uint16_t _pad0;
>      struct mcinfo_msr mcinj_msr[MC_MSRINJ_MAXMSRS];
>  };
>  
>  /* Flags for mcinj_flags above; bits 16-31 are reserved */
>  #define MC_MSRINJ_F_INTERPOSE   0x1
> +#define MC_MSRINJ_F_GPADDR      0x2
>  
>  #define XEN_MC_mceinject    5
>  struct xen_mc_mceinject {
> 

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH 3/4] x86/mce: Translate passed-in GPA to host machine address

[Haozhong Zhang]

On Tue, Sep 15, 2015 at 11:14:26AM +0200, Egger, Christoph wrote:
> On 2015/09/15 10:29, Haozhong Zhang wrote:
> > This patch adds a new flag MC_MSRINJ_F_GPADDR to
> > xen_mc_msrinject.mcinj_flags, and makes do_mca() to translate the
> > guest physical address passed-in through
> > xen_mc_msrinject.mcinj_msr[i].value to the host machine address if
> > this flag is present.
> >
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>
> Comments inline.
>
> > ---
> >  xen/arch/x86/cpu/mcheck/mce.c         | 33 +++++++++++++++++++++++++++++++++
> >  xen/include/public/arch-x86/xen-mca.h |  5 ++++-
> >  2 files changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
> > index 561257d..343d9d2 100644
> > --- a/xen/arch/x86/cpu/mcheck/mce.c
> > +++ b/xen/arch/x86/cpu/mcheck/mce.c
> > @@ -21,6 +21,7 @@
> >  #include <asm/processor.h>
> >  #include <asm/system.h>
> >  #include <asm/msr.h>
> > +#include <asm/p2m.h>
> >
> >  #include "mce.h"
> >  #include "barrier.h"
> > @@ -1422,6 +1423,38 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc)
> >          if (mc_msrinject->mcinj_count == 0)
> >              return 0;
> >
> > +        if (mc_msrinject->mcinj_flags & MC_MSRINJ_F_GPADDR) {
> > +            struct domain *d;
> > +            struct mcinfo_msr *msr;
> > +            int i;
> > +            uint64_t gaddr, gpfn, mfn, haddr;
> > +            p2m_type_t t;
> > +
> > +            d = get_domain_by_id(mc_msrinject->mcinj_domid);
> > +            if (d == NULL)
> > +                return x86_mcerr("do_mca inject: illegal domain id", -EINVAL);
> > +
> > +            for (i = 0, msr = &mc_msrinject->mcinj_msr[0];
> > +                 i < mc_msrinject->mcinj_count; i++, msr++) {
> > +                gaddr = msr->value;
> > +                gpfn = gaddr >> PAGE_SHIFT;
> > +                mfn = mfn_x(get_gfn(d, gpfn, &t));
> > +
> > +                if (mfn == INVALID_MFN) {
>
>                        put_gfn(d, gpfn);

Oh, I forgot it in this path and will add in next version.

>
> > +                    put_domain(d);
> > +                    return x86_mcerr("do_mca inject: illegal MSR value",
> > +                                     -EINVAL);
> > +                }
> > +
> > +                haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
> > +                msr->value = haddr;
> > +
> > +                put_gfn(d, gpfn);
> > +            }
> > +
> > +            put_domain(d);
> > +        }
> > +
> >          if (!x86_mc_msrinject_verify(mc_msrinject))
> >              return x86_mcerr("do_mca inject: illegal MSR", -EINVAL);
> >
> > diff --git a/xen/include/public/arch-x86/xen-mca.h b/xen/include/public/arch-x86/xen-mca.h
> > index 2422b76..33c1a84 100644
> > --- a/xen/include/public/arch-x86/xen-mca.h
> > +++ b/xen/include/public/arch-x86/xen-mca.h
> > @@ -392,12 +392,15 @@ struct xen_mc_msrinject {
> >      uint32_t mcinj_cpunr;           /* target processor id */
> >      uint32_t mcinj_flags;           /* see MC_MSRINJ_F_* below */
> >      uint32_t mcinj_count;           /* 0 .. count-1 in array are valid */
> > -    uint32_t _pad0;
> > +    domid_t  mcinj_domid;           /* valid only if MC_MSRINJ_F_GPADDR presents
> > +                                       in mcinj_flags */
>
> wording. I prefer "s/presents/is present/"
>

I will fix it in the next version.

> > +    uint16_t _pad0;
> >      struct mcinfo_msr mcinj_msr[MC_MSRINJ_MAXMSRS];
> >  };
> >
> >  /* Flags for mcinj_flags above; bits 16-31 are reserved */
> >  #define MC_MSRINJ_F_INTERPOSE   0x1
> > +#define MC_MSRINJ_F_GPADDR      0x2
> >
> >  #define XEN_MC_mceinject    5
> >  struct xen_mc_mceinject {
> >
>
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
> Ust-ID: DE289237879
> Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
>

Re: [PATCH 3/4] x86/mce: Translate passed-in GPA to host machine address

[Jan Beulich]

>>> On 15.09.15 at 10:29, <haozhong.zhang@intel.com> wrote:
> @@ -1422,6 +1423,38 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc)
>          if (mc_msrinject->mcinj_count == 0)
>              return 0;
>  
> +        if (mc_msrinject->mcinj_flags & MC_MSRINJ_F_GPADDR) {

Perhaps you earlier patch fixing coding style didn't go far enough:
Patch context as well as you additions make immediately clear that
this file (or just this function) doesn't follow either Xen or Linux
coding style. Going through the file as whole I see attempts to
follow Xen style, so please at least make sure new addition follow
that style rather than the surrounding broken one.

> +            struct domain *d;
> +            struct mcinfo_msr *msr;
> +            int i;

unsigned int

> +            uint64_t gaddr, gpfn, mfn, haddr;

paddr_t and unsigned long.

> +            p2m_type_t t;
> +
> +            d = get_domain_by_id(mc_msrinject->mcinj_domid);
> +            if (d == NULL)
> +                return x86_mcerr("do_mca inject: illegal domain id", -EINVAL);
> +
> +            for (i = 0, msr = &mc_msrinject->mcinj_msr[0];
> +                 i < mc_msrinject->mcinj_count; i++, msr++) {
> +                gaddr = msr->value;
> +                gpfn = gaddr >> PAGE_SHIFT;

PFN_DOWN()

> +                mfn = mfn_x(get_gfn(d, gpfn, &t));
> +
> +                if (mfn == INVALID_MFN) {
> +                    put_domain(d);
> +                    return x86_mcerr("do_mca inject: illegal MSR value",
> +                                     -EINVAL);

This message should be better distinguishable from the one further
down (or else it's pretty pointless) - perhaps you mean GPFN instead
of MSR?

> +                }
> +
> +                haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));

pfn_to_paddr()

> +                msr->value = haddr;

I also don't see the purpose of the intermediate variable here.

Jan

Re: [PATCH 3/4] x86/mce: Translate passed-in GPA to host machine address

[Haozhong Zhang]

On Tue, Sep 15, 2015 at 07:24:34AM -0600, Jan Beulich wrote:
> >>> On 15.09.15 at 10:29, <haozhong.zhang@intel.com> wrote:
> > @@ -1422,6 +1423,38 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc)
> >          if (mc_msrinject->mcinj_count == 0)
> >              return 0;
> >
> > +        if (mc_msrinject->mcinj_flags & MC_MSRINJ_F_GPADDR) {
>
> Perhaps you earlier patch fixing coding style didn't go far enough:
> Patch context as well as you additions make immediately clear that
> this file (or just this function) doesn't follow either Xen or Linux
> coding style. Going through the file as whole I see attempts to
> follow Xen style, so please at least make sure new addition follow
> that style rather than the surrounding broken one.
>

I was also confused by the mixed code style in this file and just
followed the surrounding one. I'll update my changes to follow Xen
style in the next version.

> > +            struct domain *d;
> > +            struct mcinfo_msr *msr;
> > +            int i;
>
> unsigned int
>

will fix in the next version

> > +            uint64_t gaddr, gpfn, mfn, haddr;
>
> paddr_t and unsigned long.

will fix in the next version

>
> > +            p2m_type_t t;
> > +
> > +            d = get_domain_by_id(mc_msrinject->mcinj_domid);
> > +            if (d == NULL)
> > +                return x86_mcerr("do_mca inject: illegal domain id", -EINVAL);
> > +
> > +            for (i = 0, msr = &mc_msrinject->mcinj_msr[0];
> > +                 i < mc_msrinject->mcinj_count; i++, msr++) {
> > +                gaddr = msr->value;
> > +                gpfn = gaddr >> PAGE_SHIFT;
>
> PFN_DOWN()
>

will fix in the next version

> > +                mfn = mfn_x(get_gfn(d, gpfn, &t));
> > +
> > +                if (mfn == INVALID_MFN) {
> > +                    put_domain(d);
> > +                    return x86_mcerr("do_mca inject: illegal MSR value",
> > +                                     -EINVAL);
>
> This message should be better distinguishable from the one further
> down (or else it's pretty pointless) - perhaps you mean GPFN instead
> of MSR?
>

Yes, I'll change it to "do_mca_inject: illegal GPFN".

> > +                }
> > +
> > +                haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
>
> pfn_to_paddr()
>

will fix in the next version

> > +                msr->value = haddr;
>
> I also don't see the purpose of the intermediate variable here.
>

will be merged with the previous line

> Jan
>

Re: [PATCH 3/4] x86/mce: Translate passed-in GPA to host machine address

[Jan Beulich]

>>> On 15.09.15 at 15:42, <haozhong.zhang@intel.com> wrote:
> On Tue, Sep 15, 2015 at 07:24:34AM -0600, Jan Beulich wrote:
>> >>> On 15.09.15 at 10:29, <haozhong.zhang@intel.com> wrote:
>> > +                mfn = mfn_x(get_gfn(d, gpfn, &t));
>> > +
>> > +                if (mfn == INVALID_MFN) {
>> > +                    put_domain(d);
>> > +                    return x86_mcerr("do_mca inject: illegal MSR value",
>> > +                                     -EINVAL);
>>
>> This message should be better distinguishable from the one further
>> down (or else it's pretty pointless) - perhaps you mean GPFN instead
>> of MSR?
>>
> 
> Yes, I'll change it to "do_mca_inject: illegal GPFN".

I'd suggest "wrong" or "bad" instead of "illegal".

Jan

Re: [PATCH 3/4] x86/mce: Translate passed-in GPA to host machine address

[Haozhong Zhang]

On Tue, Sep 15, 2015 at 07:47:49AM -0600, Jan Beulich wrote:
> >>> On 15.09.15 at 15:42, <haozhong.zhang@intel.com> wrote:
> > On Tue, Sep 15, 2015 at 07:24:34AM -0600, Jan Beulich wrote:
> >> >>> On 15.09.15 at 10:29, <haozhong.zhang@intel.com> wrote:
> >> > +                mfn = mfn_x(get_gfn(d, gpfn, &t));
> >> > +
> >> > +                if (mfn == INVALID_MFN) {
> >> > +                    put_domain(d);
> >> > +                    return x86_mcerr("do_mca inject: illegal MSR value",
> >> > +                                     -EINVAL);
> >>
> >> This message should be better distinguishable from the one further
> >> down (or else it's pretty pointless) - perhaps you mean GPFN instead
> >> of MSR?
> >>
> > 
> > Yes, I'll change it to "do_mca_inject: illegal GPFN".
> 
> I'd suggest "wrong" or "bad" instead of "illegal".
>

will change to "bad"

> Jan
> 
- Haozhong Zhang

Re: [PATCH 3/4] x86/mce: Translate passed-in GPA to host machine address

[Andrew Cooper]

On 15/09/15 14:42, Haozhong Zhang wrote:
>
>>> +                mfn = mfn_x(get_gfn(d, gpfn, &t));
>>> +
>>> +                if (mfn == INVALID_MFN) {
>>> +                    put_domain(d);
>>> +                    return x86_mcerr("do_mca inject: illegal MSR value",
>>> +                                     -EINVAL);
>> This message should be better distinguishable from the one further
>> down (or else it's pretty pointless) - perhaps you mean GPFN instead
>> of MSR?
>>
> Yes, I'll change it to "do_mca_inject: illegal GPFN".

Please always always always write useful error messages.  "illegal GPFN"
is useless without also stating which value is believed to be invalid.

Furthermore, per the clarification in c/s e758ed1, "gfn" is the correct
term to use here.

~Andrew

Re: [PATCH 3/4] x86/mce: Translate passed-in GPA to host machine address

[Haozhong Zhang]

On Tue, Sep 15, 2015 at 02:50:27PM +0100, Andrew Cooper wrote:
> On 15/09/15 14:42, Haozhong Zhang wrote:
> >
> >>> +                mfn = mfn_x(get_gfn(d, gpfn, &t));
> >>> +
> >>> +                if (mfn == INVALID_MFN) {
> >>> +                    put_domain(d);
> >>> +                    return x86_mcerr("do_mca inject: illegal MSR value",
> >>> +                                     -EINVAL);
> >> This message should be better distinguishable from the one further
> >> down (or else it's pretty pointless) - perhaps you mean GPFN instead
> >> of MSR?
> >>
> > Yes, I'll change it to "do_mca_inject: illegal GPFN".
> 
> Please always always always write useful error messages.  "illegal GPFN"
> is useless without also stating which value is believed to be invalid.
> 
> Furthermore, per the clarification in c/s e758ed1, "gfn" is the correct
> term to use here.
>

I'll update error messages in this patch to include more detailed
information.

> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

[PATCH 4/4] tools/xen-mceinj: Pass in GPA when injecting through MSR_MCI_ADDR

[Haozhong Zhang]

This patch removes the address translation in xen-mceinj which
translates the guest physical address passed-in through the argument
of '-p' to the host machine address.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 tools/tests/mce-test/tools/xen-mceinj.c | 135 +++++---------------------------
 1 file changed, 19 insertions(+), 116 deletions(-)

diff --git a/tools/tests/mce-test/tools/xen-mceinj.c b/tools/tests/mce-test/tools/xen-mceinj.c
index 0c2b640..7b9dd13 100644
--- a/tools/tests/mce-test/tools/xen-mceinj.c
+++ b/tools/tests/mce-test/tools/xen-mceinj.c
@@ -238,7 +238,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
                            uint32_t cpu_nr,
                            uint32_t flags,
                            uint64_t msr,
-                           uint64_t val)
+                           uint64_t val,
+                           domid_t domid)
 {
     uint32_t count;
 
@@ -256,6 +257,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
         msr_inj.mcinj_cpunr = cpu_nr;
         msr_inj.mcinj_flags = flags;
     }
+    if ( flags & MC_MSRINJ_F_GPADDR )
+        msr_inj.mcinj_domid = domid;
     msr_inj.mcinj_msr[count].reg = msr;
     msr_inj.mcinj_msr[count].value = val;
     msr_inj.mcinj_count++;
@@ -268,105 +271,15 @@ static int add_msr_bank_intpose(xc_interface *xc_handle,
                                 uint32_t flags,
                                 uint32_t type,
                                 uint32_t bank,
-                                uint64_t val)
+                                uint64_t val,
+                                domid_t domid)
 {
     uint64_t msr;
 
     msr = bank_addr(bank, type);
     if ( msr == INVALID_MSR )
         return -1;
-    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val);
-}
-
-#define MCE_INVALID_MFN ~0UL
-#define mfn_valid(_mfn) (_mfn != MCE_INVALID_MFN)
-#define mfn_to_pfn(_mfn) (live_m2p[(_mfn)])
-static uint64_t guest_mfn(xc_interface *xc_handle,
-                               uint32_t domain,
-                               uint64_t gpfn)
-{
-    xen_pfn_t *live_m2p = NULL;
-    int ret;
-    unsigned long hvirt_start;
-    unsigned int pt_levels;
-    uint64_t * pfn_buf = NULL;
-    unsigned long max_mfn = 0; /* max mfn of the whole machine */
-    unsigned long m2p_mfn0;
-    unsigned int guest_width;
-    long max_gpfn,i;
-    uint64_t mfn = MCE_INVALID_MFN;
-
-    if ( domain > DOMID_FIRST_RESERVED )
-        return MCE_INVALID_MFN;
-
-    /* Get max gpfn */
-    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
-                               sizeof(domain)) + 1;
-    if ( max_gpfn <= 0 )
-        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
-
-    Lprintf("Maxium gpfn for dom %d is 0x%lx", domain, max_gpfn);
-
-    /* Get max mfn */
-    if ( !get_platform_info(xc_handle, domain,
-                            &max_mfn, &hvirt_start,
-                            &pt_levels, &guest_width) )
-        err(xc_handle, "Failed to get platform information");
-
-    /* Get guest's pfn list */
-    pfn_buf = calloc(max_gpfn, sizeof(uint64_t));
-    if ( !pfn_buf )
-        err(xc_handle, "Failed to alloc pfn buf");
-
-    ret = xc_get_pfn_list(xc_handle, domain, pfn_buf, max_gpfn);
-    if ( ret < 0 ) {
-        free(pfn_buf);
-        err(xc_handle, "Failed to get pfn list %x", ret);
-    }
-
-    /* Now get the m2p table */
-    live_m2p = xc_map_m2p(xc_handle, max_mfn, PROT_READ, &m2p_mfn0);
-    if ( !live_m2p )
-        err(xc_handle, "Failed to map live M2P table");
-
-    /* match the mapping */
-    for ( i = 0; i < max_gpfn; i++ )
-    {
-        uint64_t tmp;
-        tmp = pfn_buf[i];
-
-        if (mfn_valid(tmp) &&  (mfn_to_pfn(tmp) == gpfn))
-        {
-            mfn = tmp;
-            Lprintf("We get the mfn 0x%lx for this injection", mfn);
-            break;
-        }
-    }
-
-    munmap(live_m2p, M2P_SIZE(max_mfn));
-
-    free(pfn_buf);
-    return mfn;
-}
-
-static uint64_t mca_gpfn_to_mfn(xc_interface *xc_handle,
-                                uint32_t domain,
-                                uint64_t gfn)
-{
-    uint64_t index;
-    long max_gpfn;
-
-    /* If domain is xen, means we want pass index directly */
-    if ( domain == DOMID_XEN )
-        return gfn;
-
-    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
-                               sizeof(domain)) + 1;
-    if ( max_gpfn <= 0 )
-        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
-    index = gfn % max_gpfn;
-
-    return guest_mfn(xc_handle, domain, index);
+    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val, domid);
 }
 
 static int inject_mcg_status(xc_interface *xc_handle,
@@ -374,7 +287,7 @@ static int inject_mcg_status(xc_interface *xc_handle,
                              uint64_t val)
 {
     return add_msr_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
-                               MSR_IA32_MCG_STATUS, val);
+                               MSR_IA32_MCG_STATUS, val, 0);
 }
 
 static int inject_mci_status(xc_interface *xc_handle,
@@ -383,7 +296,7 @@ static int inject_mci_status(xc_interface *xc_handle,
                              uint64_t val)
 {
     return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
-                                    MCi_type_STATUS, bank, val);
+                                    MCi_type_STATUS, bank, val, 0);
 }
 
 static int inject_mci_misc(xc_interface *xc_handle,
@@ -392,22 +305,23 @@ static int inject_mci_misc(xc_interface *xc_handle,
                              uint64_t val)
 {
     return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
-                                    MCi_type_MISC, bank, val);
+                                    MCi_type_MISC, bank, val, 0);
 }
 
 static int inject_mci_addr(xc_interface *xc_handle,
                              uint32_t cpu_nr,
                              uint64_t bank,
-                             uint64_t val)
+                             uint64_t val,
+                             domid_t domid)
 {
-    return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
-                                    MCi_type_ADDR, bank, val);
+    return add_msr_bank_intpose(xc_handle, cpu_nr,
+                                    MC_MSRINJ_F_INTERPOSE | MC_MSRINJ_F_GPADDR,
+                                    MCi_type_ADDR, bank, val, domid);
 }
 
 static int inject(xc_interface *xc_handle, struct mce_info *mce,
                   uint32_t cpu_nr, uint32_t domain, uint64_t gaddr)
 {
-    uint64_t gpfn, mfn, haddr;
     int ret = 0;
 
     ret = inject_mcg_status(xc_handle, cpu_nr, mce->mcg_stat);
@@ -424,12 +338,7 @@ static int inject(xc_interface *xc_handle, struct mce_info *mce,
     if ( ret )
         err(xc_handle, "Failed to inject MCi_MISC MSR");
 
-    gpfn = gaddr >> PAGE_SHIFT;
-    mfn = mca_gpfn_to_mfn(xc_handle, domain, gpfn);
-    if (!mfn_valid(mfn))
-        err(xc_handle, "The MFN is not valid");
-    haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
-    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, haddr);
+    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, gaddr, domain);
     if ( ret )
         err(xc_handle, "Failed to inject MCi_ADDR MSR");
 
@@ -507,7 +416,7 @@ int main(int argc, char *argv[])
     uint32_t domid;
     xc_interface *xc_handle;
     int cpu_nr;
-    int64_t gaddr, gpfn, mfn, haddr, max_gpa;
+    uint64_t gaddr, max_gpa;
 
     /* Default Value */
     domid = DOMID_XEN;
@@ -563,16 +472,10 @@ int main(int argc, char *argv[])
     Lprintf("get gaddr of error inject is: 0x%lx", gaddr);
 
     if ( dump ) {
-        gpfn = gaddr >> PAGE_SHIFT;
-        mfn = mca_gpfn_to_mfn(xc_handle, domid, gpfn);
-        if (!mfn_valid(mfn))
-            err(xc_handle, "The MFN is not valid");
-        haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
         if ( domid == DOMID_XEN )
-            Lprintf("Xen: mfn=0x%lx, haddr=0x%lx", mfn, haddr);
+            Lprintf("Xen: gaddr=0x%lx", gaddr);
         else
-            Lprintf("Dom%d: gaddr=0x%lx, gpfn=0x%lx, mfn=0x%lx, haddr=0x%lx",
-                    domid, gaddr, gpfn, mfn, haddr);
+            Lprintf("Dom%d: gaddr=0x%lx", domid, gaddr);
         goto out;
     }
 
-- 
2.4.8

Re: [PATCH 4/4] tools/xen-mceinj: Pass in GPA when injecting through MSR_MCI_ADDR

[Wei Liu]

I don't know this piece of code so my comments might be stupid.

On Tue, Sep 15, 2015 at 04:29:40PM +0800, Haozhong Zhang wrote:
> This patch removes the address translation in xen-mceinj which
> translates the guest physical address passed-in through the argument
> of '-p' to the host machine address.
> 

Is the translation functionality broken or superseded by hardware
support? What is the reason for removing this piece of (working?) code?

(I haven't looked at the code)

Wei.

Re: [PATCH 4/4] tools/xen-mceinj: Pass in GPA when injecting through MSR_MCI_ADDR

[Egger, Christoph]

On 2015/09/15 12:02, Wei Liu wrote:
> I don't know this piece of code so my comments might be stupid.
> 
> On Tue, Sep 15, 2015 at 04:29:40PM +0800, Haozhong Zhang wrote:
>> This patch removes the address translation in xen-mceinj which
>> translates the guest physical address passed-in through the argument
>> of '-p' to the host machine address.
>>
> 
> Is the translation functionality broken or superseded by hardware
> support? What is the reason for removing this piece of (working?) code?

Neither nor. The translation done in xen-mceinj.c doesn't deal with
memory addresses above 4G. The fix is to let the hypervisor do the
translation.

Christoph

> 
> (I haven't looked at the code)
> 
> Wei.
> 

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH 4/4] tools/xen-mceinj: Pass in GPA when injecting through MSR_MCI_ADDR

[Wei Liu]

On Tue, Sep 15, 2015 at 12:08:32PM +0200, Egger, Christoph wrote:
> On 2015/09/15 12:02, Wei Liu wrote:
> > I don't know this piece of code so my comments might be stupid.
> > 
> > On Tue, Sep 15, 2015 at 04:29:40PM +0800, Haozhong Zhang wrote:
> >> This patch removes the address translation in xen-mceinj which
> >> translates the guest physical address passed-in through the argument
> >> of '-p' to the host machine address.
> >>
> > 
> > Is the translation functionality broken or superseded by hardware
> > support? What is the reason for removing this piece of (working?) code?
> 
> Neither nor. The translation done in xen-mceinj.c doesn't deal with
> memory addresses above 4G. The fix is to let the hypervisor do the
> translation.
> 

Ah, I see. I missed patch 3.

Since this tool is distributed with HV so I don't think we chance of
breakage here.

The only concern is backward compatibility in parameter. But because it
is inside tools/tests I feel it's not a requirement to maintain backward
compatibility.

Christoph, since you're more familiar with this functionality could you
kindly have a look at this patch and possible give an ack if you're
happy with it?

Wei.

> Christoph
> 
> > 
> > (I haven't looked at the code)
> > 
> > Wei.
> > 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
> Ust-ID: DE289237879
> Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH 4/4] tools/xen-mceinj: Pass in GPA when injecting through MSR_MCI_ADDR

[Egger, Christoph]

On 2015/09/15 10:29, Haozhong Zhang wrote:
> This patch removes the address translation in xen-mceinj which
> translates the guest physical address passed-in through the argument
> of '-p' to the host machine address.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Comments inline.

> ---
>  tools/tests/mce-test/tools/xen-mceinj.c | 135 +++++---------------------------
>  1 file changed, 19 insertions(+), 116 deletions(-)
> 
> diff --git a/tools/tests/mce-test/tools/xen-mceinj.c b/tools/tests/mce-test/tools/xen-mceinj.c
> index 0c2b640..7b9dd13 100644
> --- a/tools/tests/mce-test/tools/xen-mceinj.c
> +++ b/tools/tests/mce-test/tools/xen-mceinj.c
> @@ -238,7 +238,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
>                             uint32_t cpu_nr,
>                             uint32_t flags,
>                             uint64_t msr,
> -                           uint64_t val)
> +                           uint64_t val,
> +                           domid_t domid)
>  {
>      uint32_t count;
>  
> @@ -256,6 +257,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
>          msr_inj.mcinj_cpunr = cpu_nr;
>          msr_inj.mcinj_flags = flags;
>      }
> +    if ( flags & MC_MSRINJ_F_GPADDR )
> +        msr_inj.mcinj_domid = domid;
>      msr_inj.mcinj_msr[count].reg = msr;
>      msr_inj.mcinj_msr[count].value = val;
>      msr_inj.mcinj_count++;
> @@ -268,105 +271,15 @@ static int add_msr_bank_intpose(xc_interface *xc_handle,
>                                  uint32_t flags,
>                                  uint32_t type,
>                                  uint32_t bank,
> -                                uint64_t val)
> +                                uint64_t val,
> +                                domid_t domid)
>  {
>      uint64_t msr;
>  
>      msr = bank_addr(bank, type);
>      if ( msr == INVALID_MSR )
>          return -1;
> -    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val);
> -}
> -
> -#define MCE_INVALID_MFN ~0UL
> -#define mfn_valid(_mfn) (_mfn != MCE_INVALID_MFN)
> -#define mfn_to_pfn(_mfn) (live_m2p[(_mfn)])
> -static uint64_t guest_mfn(xc_interface *xc_handle,
> -                               uint32_t domain,
> -                               uint64_t gpfn)
> -{
> -    xen_pfn_t *live_m2p = NULL;
> -    int ret;
> -    unsigned long hvirt_start;
> -    unsigned int pt_levels;
> -    uint64_t * pfn_buf = NULL;
> -    unsigned long max_mfn = 0; /* max mfn of the whole machine */
> -    unsigned long m2p_mfn0;
> -    unsigned int guest_width;
> -    long max_gpfn,i;
> -    uint64_t mfn = MCE_INVALID_MFN;
> -
> -    if ( domain > DOMID_FIRST_RESERVED )
> -        return MCE_INVALID_MFN;
> -
> -    /* Get max gpfn */
> -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
> -                               sizeof(domain)) + 1;
> -    if ( max_gpfn <= 0 )
> -        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
> -
> -    Lprintf("Maxium gpfn for dom %d is 0x%lx", domain, max_gpfn);
> -
> -    /* Get max mfn */
> -    if ( !get_platform_info(xc_handle, domain,
> -                            &max_mfn, &hvirt_start,
> -                            &pt_levels, &guest_width) )
> -        err(xc_handle, "Failed to get platform information");
> -
> -    /* Get guest's pfn list */
> -    pfn_buf = calloc(max_gpfn, sizeof(uint64_t));
> -    if ( !pfn_buf )
> -        err(xc_handle, "Failed to alloc pfn buf");
> -
> -    ret = xc_get_pfn_list(xc_handle, domain, pfn_buf, max_gpfn);
> -    if ( ret < 0 ) {
> -        free(pfn_buf);
> -        err(xc_handle, "Failed to get pfn list %x", ret);
> -    }
> -
> -    /* Now get the m2p table */
> -    live_m2p = xc_map_m2p(xc_handle, max_mfn, PROT_READ, &m2p_mfn0);
> -    if ( !live_m2p )
> -        err(xc_handle, "Failed to map live M2P table");
> -
> -    /* match the mapping */
> -    for ( i = 0; i < max_gpfn; i++ )
> -    {
> -        uint64_t tmp;
> -        tmp = pfn_buf[i];
> -
> -        if (mfn_valid(tmp) &&  (mfn_to_pfn(tmp) == gpfn))
> -        {
> -            mfn = tmp;
> -            Lprintf("We get the mfn 0x%lx for this injection", mfn);
> -            break;
> -        }
> -    }
> -
> -    munmap(live_m2p, M2P_SIZE(max_mfn));
> -
> -    free(pfn_buf);
> -    return mfn;
> -}
> -
> -static uint64_t mca_gpfn_to_mfn(xc_interface *xc_handle,
> -                                uint32_t domain,
> -                                uint64_t gfn)
> -{
> -    uint64_t index;
> -    long max_gpfn;
> -
> -    /* If domain is xen, means we want pass index directly */
> -    if ( domain == DOMID_XEN )
> -        return gfn;
> -
> -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
> -                               sizeof(domain)) + 1;
> -    if ( max_gpfn <= 0 )
> -        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
> -    index = gfn % max_gpfn;
> -
> -    return guest_mfn(xc_handle, domain, index);
> +    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val, domid);
>  }
>  
>  static int inject_mcg_status(xc_interface *xc_handle,
> @@ -374,7 +287,7 @@ static int inject_mcg_status(xc_interface *xc_handle,
>                               uint64_t val)
>  {
>      return add_msr_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> -                               MSR_IA32_MCG_STATUS, val);
> +                               MSR_IA32_MCG_STATUS, val, 0);

Why do you hardcode 0 here?
I think you should pass domid in here so that the DomU sees the right
status value.

>  }
>  
>  static int inject_mci_status(xc_interface *xc_handle,
> @@ -383,7 +296,7 @@ static int inject_mci_status(xc_interface *xc_handle,
>                               uint64_t val)
>  {
>      return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> -                                    MCi_type_STATUS, bank, val);
> +                                    MCi_type_STATUS, bank, val, 0);

Why do you hardcode 0 here?
I think you should pass domid in here so that the DomU sees the right
status value.

>  }
>  
>  static int inject_mci_misc(xc_interface *xc_handle,
> @@ -392,22 +305,23 @@ static int inject_mci_misc(xc_interface *xc_handle,
>                               uint64_t val)
>  {
>      return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> -                                    MCi_type_MISC, bank, val);
> +                                    MCi_type_MISC, bank, val, 0);
>  }
>  
>  static int inject_mci_addr(xc_interface *xc_handle,
>                               uint32_t cpu_nr,
>                               uint64_t bank,
> -                             uint64_t val)
> +                             uint64_t val,
> +                             domid_t domid)
>  {
> -    return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> -                                    MCi_type_ADDR, bank, val);
> +    return add_msr_bank_intpose(xc_handle, cpu_nr,
> +                                    MC_MSRINJ_F_INTERPOSE | MC_MSRINJ_F_GPADDR,
> +                                    MCi_type_ADDR, bank, val, domid);
>  }
>  
>  static int inject(xc_interface *xc_handle, struct mce_info *mce,
>                    uint32_t cpu_nr, uint32_t domain, uint64_t gaddr)
>  {
> -    uint64_t gpfn, mfn, haddr;
>      int ret = 0;
>  
>      ret = inject_mcg_status(xc_handle, cpu_nr, mce->mcg_stat);
> @@ -424,12 +338,7 @@ static int inject(xc_interface *xc_handle, struct mce_info *mce,
>      if ( ret )
>          err(xc_handle, "Failed to inject MCi_MISC MSR");
>  
> -    gpfn = gaddr >> PAGE_SHIFT;
> -    mfn = mca_gpfn_to_mfn(xc_handle, domain, gpfn);
> -    if (!mfn_valid(mfn))
> -        err(xc_handle, "The MFN is not valid");
> -    haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
> -    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, haddr);
> +    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, gaddr, domain);
>      if ( ret )
>          err(xc_handle, "Failed to inject MCi_ADDR MSR");
>  
> @@ -507,7 +416,7 @@ int main(int argc, char *argv[])
>      uint32_t domid;
>      xc_interface *xc_handle;
>      int cpu_nr;
> -    int64_t gaddr, gpfn, mfn, haddr, max_gpa;
> +    uint64_t gaddr, max_gpa;
>  
>      /* Default Value */
>      domid = DOMID_XEN;
> @@ -563,16 +472,10 @@ int main(int argc, char *argv[])
>      Lprintf("get gaddr of error inject is: 0x%lx", gaddr);
>  
>      if ( dump ) {
> -        gpfn = gaddr >> PAGE_SHIFT;
> -        mfn = mca_gpfn_to_mfn(xc_handle, domid, gpfn);
> -        if (!mfn_valid(mfn))
> -            err(xc_handle, "The MFN is not valid");
> -        haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
>          if ( domid == DOMID_XEN )
> -            Lprintf("Xen: mfn=0x%lx, haddr=0x%lx", mfn, haddr);
> +            Lprintf("Xen: gaddr=0x%lx", gaddr);
>          else
> -            Lprintf("Dom%d: gaddr=0x%lx, gpfn=0x%lx, mfn=0x%lx, haddr=0x%lx",
> -                    domid, gaddr, gpfn, mfn, haddr);
> +            Lprintf("Dom%d: gaddr=0x%lx", domid, gaddr);
>          goto out;
>      }
>  
> 

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH 4/4] tools/xen-mceinj: Pass in GPA when injecting through MSR_MCI_ADDR

[Haozhong Zhang]

On Tue, Sep 15, 2015 at 12:17:01PM +0200, Egger, Christoph wrote:
> On 2015/09/15 10:29, Haozhong Zhang wrote:
> > This patch removes the address translation in xen-mceinj which
> > translates the guest physical address passed-in through the argument
> > of '-p' to the host machine address.
> >
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>
> Comments inline.
>
> > ---
> >  tools/tests/mce-test/tools/xen-mceinj.c | 135 +++++---------------------------
> >  1 file changed, 19 insertions(+), 116 deletions(-)
> >
> > diff --git a/tools/tests/mce-test/tools/xen-mceinj.c b/tools/tests/mce-test/tools/xen-mceinj.c
> > index 0c2b640..7b9dd13 100644
> > --- a/tools/tests/mce-test/tools/xen-mceinj.c
> > +++ b/tools/tests/mce-test/tools/xen-mceinj.c
> > @@ -238,7 +238,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
> >                             uint32_t cpu_nr,
> >                             uint32_t flags,
> >                             uint64_t msr,
> > -                           uint64_t val)
> > +                           uint64_t val,
> > +                           domid_t domid)
> >  {
> >      uint32_t count;
> >
> > @@ -256,6 +257,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
> >          msr_inj.mcinj_cpunr = cpu_nr;
> >          msr_inj.mcinj_flags = flags;
> >      }
> > +    if ( flags & MC_MSRINJ_F_GPADDR )
> > +        msr_inj.mcinj_domid = domid;
> >      msr_inj.mcinj_msr[count].reg = msr;
> >      msr_inj.mcinj_msr[count].value = val;
> >      msr_inj.mcinj_count++;
> > @@ -268,105 +271,15 @@ static int add_msr_bank_intpose(xc_interface *xc_handle,
> >                                  uint32_t flags,
> >                                  uint32_t type,
> >                                  uint32_t bank,
> > -                                uint64_t val)
> > +                                uint64_t val,
> > +                                domid_t domid)
> >  {
> >      uint64_t msr;
> >
> >      msr = bank_addr(bank, type);
> >      if ( msr == INVALID_MSR )
> >          return -1;
> > -    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val);
> > -}
> > -
> > -#define MCE_INVALID_MFN ~0UL
> > -#define mfn_valid(_mfn) (_mfn != MCE_INVALID_MFN)
> > -#define mfn_to_pfn(_mfn) (live_m2p[(_mfn)])
> > -static uint64_t guest_mfn(xc_interface *xc_handle,
> > -                               uint32_t domain,
> > -                               uint64_t gpfn)
> > -{
> > -    xen_pfn_t *live_m2p = NULL;
> > -    int ret;
> > -    unsigned long hvirt_start;
> > -    unsigned int pt_levels;
> > -    uint64_t * pfn_buf = NULL;
> > -    unsigned long max_mfn = 0; /* max mfn of the whole machine */
> > -    unsigned long m2p_mfn0;
> > -    unsigned int guest_width;
> > -    long max_gpfn,i;
> > -    uint64_t mfn = MCE_INVALID_MFN;
> > -
> > -    if ( domain > DOMID_FIRST_RESERVED )
> > -        return MCE_INVALID_MFN;
> > -
> > -    /* Get max gpfn */
> > -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
> > -                               sizeof(domain)) + 1;
> > -    if ( max_gpfn <= 0 )
> > -        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
> > -
> > -    Lprintf("Maxium gpfn for dom %d is 0x%lx", domain, max_gpfn);
> > -
> > -    /* Get max mfn */
> > -    if ( !get_platform_info(xc_handle, domain,
> > -                            &max_mfn, &hvirt_start,
> > -                            &pt_levels, &guest_width) )
> > -        err(xc_handle, "Failed to get platform information");
> > -
> > -    /* Get guest's pfn list */
> > -    pfn_buf = calloc(max_gpfn, sizeof(uint64_t));
> > -    if ( !pfn_buf )
> > -        err(xc_handle, "Failed to alloc pfn buf");
> > -
> > -    ret = xc_get_pfn_list(xc_handle, domain, pfn_buf, max_gpfn);
> > -    if ( ret < 0 ) {
> > -        free(pfn_buf);
> > -        err(xc_handle, "Failed to get pfn list %x", ret);
> > -    }
> > -
> > -    /* Now get the m2p table */
> > -    live_m2p = xc_map_m2p(xc_handle, max_mfn, PROT_READ, &m2p_mfn0);
> > -    if ( !live_m2p )
> > -        err(xc_handle, "Failed to map live M2P table");
> > -
> > -    /* match the mapping */
> > -    for ( i = 0; i < max_gpfn; i++ )
> > -    {
> > -        uint64_t tmp;
> > -        tmp = pfn_buf[i];
> > -
> > -        if (mfn_valid(tmp) &&  (mfn_to_pfn(tmp) == gpfn))
> > -        {
> > -            mfn = tmp;
> > -            Lprintf("We get the mfn 0x%lx for this injection", mfn);
> > -            break;
> > -        }
> > -    }
> > -
> > -    munmap(live_m2p, M2P_SIZE(max_mfn));
> > -
> > -    free(pfn_buf);
> > -    return mfn;
> > -}
> > -
> > -static uint64_t mca_gpfn_to_mfn(xc_interface *xc_handle,
> > -                                uint32_t domain,
> > -                                uint64_t gfn)
> > -{
> > -    uint64_t index;
> > -    long max_gpfn;
> > -
> > -    /* If domain is xen, means we want pass index directly */
> > -    if ( domain == DOMID_XEN )
> > -        return gfn;
> > -
> > -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
> > -                               sizeof(domain)) + 1;
> > -    if ( max_gpfn <= 0 )
> > -        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
> > -    index = gfn % max_gpfn;
> > -
> > -    return guest_mfn(xc_handle, domain, index);
> > +    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val, domid);
> >  }
> >
> >  static int inject_mcg_status(xc_interface *xc_handle,
> > @@ -374,7 +287,7 @@ static int inject_mcg_status(xc_interface *xc_handle,
> >                               uint64_t val)
> >  {
> >      return add_msr_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> > -                               MSR_IA32_MCG_STATUS, val);
> > +                               MSR_IA32_MCG_STATUS, val, 0);
>
> Why do you hardcode 0 here?
> I think you should pass domid in here so that the DomU sees the right
> status value.

If the flag MC_MSRINJ_F_GPADDR is not given (which is the case here),
the domain id is not used when do_mca() handles the MCE injection, so
I just pass 0 as a placeholder here. But, yes, passing domid instead
keeps the right domain status. I'll change it in the next version.

>
> >  }
> >
> >  static int inject_mci_status(xc_interface *xc_handle,
> > @@ -383,7 +296,7 @@ static int inject_mci_status(xc_interface *xc_handle,
> >                               uint64_t val)
> >  {
> >      return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> > -                                    MCi_type_STATUS, bank, val);
> > +                                    MCi_type_STATUS, bank, val, 0);
>
> Why do you hardcode 0 here?
> I think you should pass domid in here so that the DomU sees the right
> status value.
>

the same as above

> >  }
> >
> >  static int inject_mci_misc(xc_interface *xc_handle,
> > @@ -392,22 +305,23 @@ static int inject_mci_misc(xc_interface *xc_handle,
> >                               uint64_t val)
> >  {
> >      return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> > -                                    MCi_type_MISC, bank, val);
> > +                                    MCi_type_MISC, bank, val, 0);
> >  }
> >
> >  static int inject_mci_addr(xc_interface *xc_handle,
> >                               uint32_t cpu_nr,
> >                               uint64_t bank,
> > -                             uint64_t val)
> > +                             uint64_t val,
> > +                             domid_t domid)
> >  {
> > -    return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> > -                                    MCi_type_ADDR, bank, val);
> > +    return add_msr_bank_intpose(xc_handle, cpu_nr,
> > +                                    MC_MSRINJ_F_INTERPOSE | MC_MSRINJ_F_GPADDR,
> > +                                    MCi_type_ADDR, bank, val, domid);
> >  }
> >
> >  static int inject(xc_interface *xc_handle, struct mce_info *mce,
> >                    uint32_t cpu_nr, uint32_t domain, uint64_t gaddr)
> >  {
> > -    uint64_t gpfn, mfn, haddr;
> >      int ret = 0;
> >
> >      ret = inject_mcg_status(xc_handle, cpu_nr, mce->mcg_stat);
> > @@ -424,12 +338,7 @@ static int inject(xc_interface *xc_handle, struct mce_info *mce,
> >      if ( ret )
> >          err(xc_handle, "Failed to inject MCi_MISC MSR");
> >
> > -    gpfn = gaddr >> PAGE_SHIFT;
> > -    mfn = mca_gpfn_to_mfn(xc_handle, domain, gpfn);
> > -    if (!mfn_valid(mfn))
> > -        err(xc_handle, "The MFN is not valid");
> > -    haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
> > -    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, haddr);
> > +    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, gaddr, domain);
> >      if ( ret )
> >          err(xc_handle, "Failed to inject MCi_ADDR MSR");
> >
> > @@ -507,7 +416,7 @@ int main(int argc, char *argv[])
> >      uint32_t domid;
> >      xc_interface *xc_handle;
> >      int cpu_nr;
> > -    int64_t gaddr, gpfn, mfn, haddr, max_gpa;
> > +    uint64_t gaddr, max_gpa;
> >
> >      /* Default Value */
> >      domid = DOMID_XEN;
> > @@ -563,16 +472,10 @@ int main(int argc, char *argv[])
> >      Lprintf("get gaddr of error inject is: 0x%lx", gaddr);
> >
> >      if ( dump ) {
> > -        gpfn = gaddr >> PAGE_SHIFT;
> > -        mfn = mca_gpfn_to_mfn(xc_handle, domid, gpfn);
> > -        if (!mfn_valid(mfn))
> > -            err(xc_handle, "The MFN is not valid");
> > -        haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
> >          if ( domid == DOMID_XEN )
> > -            Lprintf("Xen: mfn=0x%lx, haddr=0x%lx", mfn, haddr);
> > +            Lprintf("Xen: gaddr=0x%lx", gaddr);
> >          else
> > -            Lprintf("Dom%d: gaddr=0x%lx, gpfn=0x%lx, mfn=0x%lx, haddr=0x%lx",
> > -                    domid, gaddr, gpfn, mfn, haddr);
> > +            Lprintf("Dom%d: gaddr=0x%lx", domid, gaddr);
> >          goto out;
> >      }
> >
> >
>
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
> Ust-ID: DE289237879
> Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
>

Re: [PATCH 4/4] tools/xen-mceinj: Pass in GPA when injecting through MSR_MCI_ADDR

[Egger, Christoph]

On 2015/09/15 12:28, Haozhong Zhang wrote:
> On Tue, Sep 15, 2015 at 12:17:01PM +0200, Egger, Christoph wrote:
>> On 2015/09/15 10:29, Haozhong Zhang wrote:
>>> This patch removes the address translation in xen-mceinj which
>>> translates the guest physical address passed-in through the argument
>>> of '-p' to the host machine address.
>>>
>>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>
>> Comments inline.
>>
>>> ---
>>>  tools/tests/mce-test/tools/xen-mceinj.c | 135 +++++---------------------------
>>>  1 file changed, 19 insertions(+), 116 deletions(-)
>>>
>>> diff --git a/tools/tests/mce-test/tools/xen-mceinj.c b/tools/tests/mce-test/tools/xen-mceinj.c
>>> index 0c2b640..7b9dd13 100644
>>> --- a/tools/tests/mce-test/tools/xen-mceinj.c
>>> +++ b/tools/tests/mce-test/tools/xen-mceinj.c
>>> @@ -238,7 +238,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
>>>                             uint32_t cpu_nr,
>>>                             uint32_t flags,
>>>                             uint64_t msr,
>>> -                           uint64_t val)
>>> +                           uint64_t val,
>>> +                           domid_t domid)
>>>  {
>>>      uint32_t count;
>>>
>>> @@ -256,6 +257,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
>>>          msr_inj.mcinj_cpunr = cpu_nr;
>>>          msr_inj.mcinj_flags = flags;
>>>      }
>>> +    if ( flags & MC_MSRINJ_F_GPADDR )
>>> +        msr_inj.mcinj_domid = domid;
>>>      msr_inj.mcinj_msr[count].reg = msr;
>>>      msr_inj.mcinj_msr[count].value = val;
>>>      msr_inj.mcinj_count++;
>>> @@ -268,105 +271,15 @@ static int add_msr_bank_intpose(xc_interface *xc_handle,
>>>                                  uint32_t flags,
>>>                                  uint32_t type,
>>>                                  uint32_t bank,
>>> -                                uint64_t val)
>>> +                                uint64_t val,
>>> +                                domid_t domid)
>>>  {
>>>      uint64_t msr;
>>>
>>>      msr = bank_addr(bank, type);
>>>      if ( msr == INVALID_MSR )
>>>          return -1;
>>> -    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val);
>>> -}
>>> -
>>> -#define MCE_INVALID_MFN ~0UL
>>> -#define mfn_valid(_mfn) (_mfn != MCE_INVALID_MFN)
>>> -#define mfn_to_pfn(_mfn) (live_m2p[(_mfn)])
>>> -static uint64_t guest_mfn(xc_interface *xc_handle,
>>> -                               uint32_t domain,
>>> -                               uint64_t gpfn)
>>> -{
>>> -    xen_pfn_t *live_m2p = NULL;
>>> -    int ret;
>>> -    unsigned long hvirt_start;
>>> -    unsigned int pt_levels;
>>> -    uint64_t * pfn_buf = NULL;
>>> -    unsigned long max_mfn = 0; /* max mfn of the whole machine */
>>> -    unsigned long m2p_mfn0;
>>> -    unsigned int guest_width;
>>> -    long max_gpfn,i;
>>> -    uint64_t mfn = MCE_INVALID_MFN;
>>> -
>>> -    if ( domain > DOMID_FIRST_RESERVED )
>>> -        return MCE_INVALID_MFN;
>>> -
>>> -    /* Get max gpfn */
>>> -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
>>> -                               sizeof(domain)) + 1;
>>> -    if ( max_gpfn <= 0 )
>>> -        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
>>> -
>>> -    Lprintf("Maxium gpfn for dom %d is 0x%lx", domain, max_gpfn);
>>> -
>>> -    /* Get max mfn */
>>> -    if ( !get_platform_info(xc_handle, domain,
>>> -                            &max_mfn, &hvirt_start,
>>> -                            &pt_levels, &guest_width) )
>>> -        err(xc_handle, "Failed to get platform information");
>>> -
>>> -    /* Get guest's pfn list */
>>> -    pfn_buf = calloc(max_gpfn, sizeof(uint64_t));
>>> -    if ( !pfn_buf )
>>> -        err(xc_handle, "Failed to alloc pfn buf");
>>> -
>>> -    ret = xc_get_pfn_list(xc_handle, domain, pfn_buf, max_gpfn);
>>> -    if ( ret < 0 ) {
>>> -        free(pfn_buf);
>>> -        err(xc_handle, "Failed to get pfn list %x", ret);
>>> -    }
>>> -
>>> -    /* Now get the m2p table */
>>> -    live_m2p = xc_map_m2p(xc_handle, max_mfn, PROT_READ, &m2p_mfn0);
>>> -    if ( !live_m2p )
>>> -        err(xc_handle, "Failed to map live M2P table");
>>> -
>>> -    /* match the mapping */
>>> -    for ( i = 0; i < max_gpfn; i++ )
>>> -    {
>>> -        uint64_t tmp;
>>> -        tmp = pfn_buf[i];
>>> -
>>> -        if (mfn_valid(tmp) &&  (mfn_to_pfn(tmp) == gpfn))
>>> -        {
>>> -            mfn = tmp;
>>> -            Lprintf("We get the mfn 0x%lx for this injection", mfn);
>>> -            break;
>>> -        }
>>> -    }
>>> -
>>> -    munmap(live_m2p, M2P_SIZE(max_mfn));
>>> -
>>> -    free(pfn_buf);
>>> -    return mfn;
>>> -}
>>> -
>>> -static uint64_t mca_gpfn_to_mfn(xc_interface *xc_handle,
>>> -                                uint32_t domain,
>>> -                                uint64_t gfn)
>>> -{
>>> -    uint64_t index;
>>> -    long max_gpfn;
>>> -
>>> -    /* If domain is xen, means we want pass index directly */
>>> -    if ( domain == DOMID_XEN )
>>> -        return gfn;
>>> -
>>> -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
>>> -                               sizeof(domain)) + 1;
>>> -    if ( max_gpfn <= 0 )
>>> -        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
>>> -    index = gfn % max_gpfn;
>>> -
>>> -    return guest_mfn(xc_handle, domain, index);
>>> +    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val, domid);
>>>  }
>>>
>>>  static int inject_mcg_status(xc_interface *xc_handle,
>>> @@ -374,7 +287,7 @@ static int inject_mcg_status(xc_interface *xc_handle,
>>>                               uint64_t val)
>>>  {
>>>      return add_msr_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
>>> -                               MSR_IA32_MCG_STATUS, val);
>>> +                               MSR_IA32_MCG_STATUS, val, 0);
>>
>> Why do you hardcode 0 here?
>> I think you should pass domid in here so that the DomU sees the right
>> status value.
> 
> If the flag MC_MSRINJ_F_GPADDR is not given (which is the case here),
> the domain id is not used when do_mca() handles the MCE injection, so
> I just pass 0 as a placeholder here. But, yes, passing domid instead
> keeps the right domain status. I'll change it in the next version.

Thanks. This is much less error prone to future changes.

Christoph

> 
>>
>>>  }
>>>
>>>  static int inject_mci_status(xc_interface *xc_handle,
>>> @@ -383,7 +296,7 @@ static int inject_mci_status(xc_interface *xc_handle,
>>>                               uint64_t val)
>>>  {
>>>      return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
>>> -                                    MCi_type_STATUS, bank, val);
>>> +                                    MCi_type_STATUS, bank, val, 0);
>>
>> Why do you hardcode 0 here?
>> I think you should pass domid in here so that the DomU sees the right
>> status value.
>>
> 
> the same as above
> 
>>>  }
>>>
>>>  static int inject_mci_misc(xc_interface *xc_handle,
>>> @@ -392,22 +305,23 @@ static int inject_mci_misc(xc_interface *xc_handle,
>>>                               uint64_t val)
>>>  {
>>>      return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
>>> -                                    MCi_type_MISC, bank, val);
>>> +                                    MCi_type_MISC, bank, val, 0);
>>>  }
>>>
>>>  static int inject_mci_addr(xc_interface *xc_handle,
>>>                               uint32_t cpu_nr,
>>>                               uint64_t bank,
>>> -                             uint64_t val)
>>> +                             uint64_t val,
>>> +                             domid_t domid)
>>>  {
>>> -    return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
>>> -                                    MCi_type_ADDR, bank, val);
>>> +    return add_msr_bank_intpose(xc_handle, cpu_nr,
>>> +                                    MC_MSRINJ_F_INTERPOSE | MC_MSRINJ_F_GPADDR,
>>> +                                    MCi_type_ADDR, bank, val, domid);
>>>  }
>>>
>>>  static int inject(xc_interface *xc_handle, struct mce_info *mce,
>>>                    uint32_t cpu_nr, uint32_t domain, uint64_t gaddr)
>>>  {
>>> -    uint64_t gpfn, mfn, haddr;
>>>      int ret = 0;
>>>
>>>      ret = inject_mcg_status(xc_handle, cpu_nr, mce->mcg_stat);
>>> @@ -424,12 +338,7 @@ static int inject(xc_interface *xc_handle, struct mce_info *mce,
>>>      if ( ret )
>>>          err(xc_handle, "Failed to inject MCi_MISC MSR");
>>>
>>> -    gpfn = gaddr >> PAGE_SHIFT;
>>> -    mfn = mca_gpfn_to_mfn(xc_handle, domain, gpfn);
>>> -    if (!mfn_valid(mfn))
>>> -        err(xc_handle, "The MFN is not valid");
>>> -    haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
>>> -    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, haddr);
>>> +    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, gaddr, domain);
>>>      if ( ret )
>>>          err(xc_handle, "Failed to inject MCi_ADDR MSR");
>>>
>>> @@ -507,7 +416,7 @@ int main(int argc, char *argv[])
>>>      uint32_t domid;
>>>      xc_interface *xc_handle;
>>>      int cpu_nr;
>>> -    int64_t gaddr, gpfn, mfn, haddr, max_gpa;
>>> +    uint64_t gaddr, max_gpa;
>>>
>>>      /* Default Value */
>>>      domid = DOMID_XEN;
>>> @@ -563,16 +472,10 @@ int main(int argc, char *argv[])
>>>      Lprintf("get gaddr of error inject is: 0x%lx", gaddr);
>>>
>>>      if ( dump ) {
>>> -        gpfn = gaddr >> PAGE_SHIFT;
>>> -        mfn = mca_gpfn_to_mfn(xc_handle, domid, gpfn);
>>> -        if (!mfn_valid(mfn))
>>> -            err(xc_handle, "The MFN is not valid");
>>> -        haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
>>>          if ( domid == DOMID_XEN )
>>> -            Lprintf("Xen: mfn=0x%lx, haddr=0x%lx", mfn, haddr);
>>> +            Lprintf("Xen: gaddr=0x%lx", gaddr);
>>>          else
>>> -            Lprintf("Dom%d: gaddr=0x%lx, gpfn=0x%lx, mfn=0x%lx, haddr=0x%lx",
>>> -                    domid, gaddr, gpfn, mfn, haddr);
>>> +            Lprintf("Dom%d: gaddr=0x%lx", domid, gaddr);
>>>          goto out;
>>>      }
>>>
>>>

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH 4/4] tools/xen-mceinj: Pass in GPA when injecting through MSR_MCI_ADDR

[Ian Campbell]

On Tue, 2015-09-15 at 16:29 +0800, Haozhong Zhang wrote:
> This patch removes the address translation in xen-mceinj which
> translates the guest physical address passed-in through the argument
> of '-p' to the host machine address.

I think there is a missing/implied "... and replaces it with $something"
here. Can you make that explicit in the commit log please.

I think this also changes the semantics of -p, it would be useful to state
explicitly what the previous and new behaviour is in the commit message and
hopefully update the help text to say precisely what is now needed.

Ian.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  tools/tests/mce-test/tools/xen-mceinj.c | 135 +++++---------------------
> ------
>  1 file changed, 19 insertions(+), 116 deletions(-)
> 
> diff --git a/tools/tests/mce-test/tools/xen-mceinj.c b/tools/tests/mce
> -test/tools/xen-mceinj.c
> index 0c2b640..7b9dd13 100644
> --- a/tools/tests/mce-test/tools/xen-mceinj.c
> +++ b/tools/tests/mce-test/tools/xen-mceinj.c
> @@ -238,7 +238,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
>                             uint32_t cpu_nr,
>                             uint32_t flags,
>                             uint64_t msr,
> -                           uint64_t val)
> +                           uint64_t val,
> +                           domid_t domid)
>  {
>      uint32_t count;
>  
> @@ -256,6 +257,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
>          msr_inj.mcinj_cpunr = cpu_nr;
>          msr_inj.mcinj_flags = flags;
>      }
> +    if ( flags & MC_MSRINJ_F_GPADDR )
> +        msr_inj.mcinj_domid = domid;
>      msr_inj.mcinj_msr[count].reg = msr;
>      msr_inj.mcinj_msr[count].value = val;
>      msr_inj.mcinj_count++;
> @@ -268,105 +271,15 @@ static int add_msr_bank_intpose(xc_interface
> *xc_handle,
>                                  uint32_t flags,
>                                  uint32_t type,
>                                  uint32_t bank,
> -                                uint64_t val)
> +                                uint64_t val,
> +                                domid_t domid)
>  {
>      uint64_t msr;
>  
>      msr = bank_addr(bank, type);
>      if ( msr == INVALID_MSR )
>          return -1;
> -    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val);
> -}
> -
> -#define MCE_INVALID_MFN ~0UL
> -#define mfn_valid(_mfn) (_mfn != MCE_INVALID_MFN)
> -#define mfn_to_pfn(_mfn) (live_m2p[(_mfn)])
> -static uint64_t guest_mfn(xc_interface *xc_handle,
> -                               uint32_t domain,
> -                               uint64_t gpfn)
> -{
> -    xen_pfn_t *live_m2p = NULL;
> -    int ret;
> -    unsigned long hvirt_start;
> -    unsigned int pt_levels;
> -    uint64_t * pfn_buf = NULL;
> -    unsigned long max_mfn = 0; /* max mfn of the whole machine */
> -    unsigned long m2p_mfn0;
> -    unsigned int guest_width;
> -    long max_gpfn,i;
> -    uint64_t mfn = MCE_INVALID_MFN;
> -
> -    if ( domain > DOMID_FIRST_RESERVED )
> -        return MCE_INVALID_MFN;
> -
> -    /* Get max gpfn */
> -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
> -                               sizeof(domain)) + 1;
> -    if ( max_gpfn <= 0 )
> -        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
> -
> -    Lprintf("Maxium gpfn for dom %d is 0x%lx", domain, max_gpfn);
> -
> -    /* Get max mfn */
> -    if ( !get_platform_info(xc_handle, domain,
> -                            &max_mfn, &hvirt_start,
> -                            &pt_levels, &guest_width) )
> -        err(xc_handle, "Failed to get platform information");
> -
> -    /* Get guest's pfn list */
> -    pfn_buf = calloc(max_gpfn, sizeof(uint64_t));
> -    if ( !pfn_buf )
> -        err(xc_handle, "Failed to alloc pfn buf");
> -
> -    ret = xc_get_pfn_list(xc_handle, domain, pfn_buf, max_gpfn);
> -    if ( ret < 0 ) {
> -        free(pfn_buf);
> -        err(xc_handle, "Failed to get pfn list %x", ret);
> -    }
> -
> -    /* Now get the m2p table */
> -    live_m2p = xc_map_m2p(xc_handle, max_mfn, PROT_READ, &m2p_mfn0);
> -    if ( !live_m2p )
> -        err(xc_handle, "Failed to map live M2P table");
> -
> -    /* match the mapping */
> -    for ( i = 0; i < max_gpfn; i++ )
> -    {
> -        uint64_t tmp;
> -        tmp = pfn_buf[i];
> -
> -        if (mfn_valid(tmp) &&  (mfn_to_pfn(tmp) == gpfn))
> -        {
> -            mfn = tmp;
> -            Lprintf("We get the mfn 0x%lx for this injection", mfn);
> -            break;
> -        }
> -    }
> -
> -    munmap(live_m2p, M2P_SIZE(max_mfn));
> -
> -    free(pfn_buf);
> -    return mfn;
> -}
> -
> -static uint64_t mca_gpfn_to_mfn(xc_interface *xc_handle,
> -                                uint32_t domain,
> -                                uint64_t gfn)
> -{
> -    uint64_t index;
> -    long max_gpfn;
> -
> -    /* If domain is xen, means we want pass index directly */
> -    if ( domain == DOMID_XEN )
> -        return gfn;
> -
> -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
> -                               sizeof(domain)) + 1;
> -    if ( max_gpfn <= 0 )
> -        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
> -    index = gfn % max_gpfn;
> -
> -    return guest_mfn(xc_handle, domain, index);
> +    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val, domid);
>  }
>  
>  static int inject_mcg_status(xc_interface *xc_handle,
> @@ -374,7 +287,7 @@ static int inject_mcg_status(xc_interface *xc_handle,
>                               uint64_t val)
>  {
>      return add_msr_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> -                               MSR_IA32_MCG_STATUS, val);
> +                               MSR_IA32_MCG_STATUS, val, 0);
>  }
>  
>  static int inject_mci_status(xc_interface *xc_handle,
> @@ -383,7 +296,7 @@ static int inject_mci_status(xc_interface *xc_handle,
>                               uint64_t val)
>  {
>      return add_msr_bank_intpose(xc_handle, cpu_nr,
> MC_MSRINJ_F_INTERPOSE,
> -                                    MCi_type_STATUS, bank, val);
> +                                    MCi_type_STATUS, bank, val, 0);
>  }
>  
>  static int inject_mci_misc(xc_interface *xc_handle,
> @@ -392,22 +305,23 @@ static int inject_mci_misc(xc_interface *xc_handle,
>                               uint64_t val)
>  {
>      return add_msr_bank_intpose(xc_handle, cpu_nr,
> MC_MSRINJ_F_INTERPOSE,
> -                                    MCi_type_MISC, bank, val);
> +                                    MCi_type_MISC, bank, val, 0);
>  }
>  
>  static int inject_mci_addr(xc_interface *xc_handle,
>                               uint32_t cpu_nr,
>                               uint64_t bank,
> -                             uint64_t val)
> +                             uint64_t val,
> +                             domid_t domid)
>  {
> -    return add_msr_bank_intpose(xc_handle, cpu_nr,
> MC_MSRINJ_F_INTERPOSE,
> -                                    MCi_type_ADDR, bank, val);
> +    return add_msr_bank_intpose(xc_handle, cpu_nr,
> +                                    MC_MSRINJ_F_INTERPOSE |
> MC_MSRINJ_F_GPADDR,
> +                                    MCi_type_ADDR, bank, val, domid);
>  }
>  
>  static int inject(xc_interface *xc_handle, struct mce_info *mce,
>                    uint32_t cpu_nr, uint32_t domain, uint64_t gaddr)
>  {
> -    uint64_t gpfn, mfn, haddr;
>      int ret = 0;
>  
>      ret = inject_mcg_status(xc_handle, cpu_nr, mce->mcg_stat);
> @@ -424,12 +338,7 @@ static int inject(xc_interface *xc_handle, struct
> mce_info *mce,
>      if ( ret )
>          err(xc_handle, "Failed to inject MCi_MISC MSR");
>  
> -    gpfn = gaddr >> PAGE_SHIFT;
> -    mfn = mca_gpfn_to_mfn(xc_handle, domain, gpfn);
> -    if (!mfn_valid(mfn))
> -        err(xc_handle, "The MFN is not valid");
> -    haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
> -    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, haddr);
> +    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, gaddr, domain);
>      if ( ret )
>          err(xc_handle, "Failed to inject MCi_ADDR MSR");
>  
> @@ -507,7 +416,7 @@ int main(int argc, char *argv[])
>      uint32_t domid;
>      xc_interface *xc_handle;
>      int cpu_nr;
> -    int64_t gaddr, gpfn, mfn, haddr, max_gpa;
> +    uint64_t gaddr, max_gpa;
>  
>      /* Default Value */
>      domid = DOMID_XEN;
> @@ -563,16 +472,10 @@ int main(int argc, char *argv[])
>      Lprintf("get gaddr of error inject is: 0x%lx", gaddr);
>  
>      if ( dump ) {
> -        gpfn = gaddr >> PAGE_SHIFT;
> -        mfn = mca_gpfn_to_mfn(xc_handle, domid, gpfn);
> -        if (!mfn_valid(mfn))
> -            err(xc_handle, "The MFN is not valid");
> -        haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
>          if ( domid == DOMID_XEN )
> -            Lprintf("Xen: mfn=0x%lx, haddr=0x%lx", mfn, haddr);
> +            Lprintf("Xen: gaddr=0x%lx", gaddr);
>          else
> -            Lprintf("Dom%d: gaddr=0x%lx, gpfn=0x%lx, mfn=0x%lx,
> haddr=0x%lx",
> -                    domid, gaddr, gpfn, mfn, haddr);
> +            Lprintf("Dom%d: gaddr=0x%lx", domid, gaddr);
>          goto out;
>      }
>  

Re: [PATCH 4/4] tools/xen-mceinj: Pass in GPA when injecting through MSR_MCI_ADDR

[Haozhong Zhang]

On Tue, Sep 15, 2015 at 11:20:20AM +0100, Ian Campbell wrote:
> On Tue, 2015-09-15 at 16:29 +0800, Haozhong Zhang wrote:
> > This patch removes the address translation in xen-mceinj which
> > translates the guest physical address passed-in through the argument
> > of '-p' to the host machine address.
>
> I think there is a missing/implied "... and replaces it with $something"
> here. Can you make that explicit in the commit log please.
>
Yes, I'll add the missing part.

> I think this also changes the semantics of -p, it would be useful to state
> explicitly what the previous and new behaviour is in the commit message and
> hopefully update the help text to say precisely what is now needed.
>

The semantics of -p is not changed here. In both the original and the
patched code, it accepts the guest physical address. This patchset
just moves the address translation (to the host machine address) from
xen-mceinj to the hypervisor, and the functionality is unchanged.

> Ian.
> >
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  tools/tests/mce-test/tools/xen-mceinj.c | 135 +++++---------------------
> > ------
> >  1 file changed, 19 insertions(+), 116 deletions(-)
> >
> > diff --git a/tools/tests/mce-test/tools/xen-mceinj.c b/tools/tests/mce
> > -test/tools/xen-mceinj.c
> > index 0c2b640..7b9dd13 100644
> > --- a/tools/tests/mce-test/tools/xen-mceinj.c
> > +++ b/tools/tests/mce-test/tools/xen-mceinj.c
> > @@ -238,7 +238,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
> >                             uint32_t cpu_nr,
> >                             uint32_t flags,
> >                             uint64_t msr,
> > -                           uint64_t val)
> > +                           uint64_t val,
> > +                           domid_t domid)
> >  {
> >      uint32_t count;
> >
> > @@ -256,6 +257,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
> >          msr_inj.mcinj_cpunr = cpu_nr;
> >          msr_inj.mcinj_flags = flags;
> >      }
> > +    if ( flags & MC_MSRINJ_F_GPADDR )
> > +        msr_inj.mcinj_domid = domid;
> >      msr_inj.mcinj_msr[count].reg = msr;
> >      msr_inj.mcinj_msr[count].value = val;
> >      msr_inj.mcinj_count++;
> > @@ -268,105 +271,15 @@ static int add_msr_bank_intpose(xc_interface
> > *xc_handle,
> >                                  uint32_t flags,
> >                                  uint32_t type,
> >                                  uint32_t bank,
> > -                                uint64_t val)
> > +                                uint64_t val,
> > +                                domid_t domid)
> >  {
> >      uint64_t msr;
> >
> >      msr = bank_addr(bank, type);
> >      if ( msr == INVALID_MSR )
> >          return -1;
> > -    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val);
> > -}
> > -
> > -#define MCE_INVALID_MFN ~0UL
> > -#define mfn_valid(_mfn) (_mfn != MCE_INVALID_MFN)
> > -#define mfn_to_pfn(_mfn) (live_m2p[(_mfn)])
> > -static uint64_t guest_mfn(xc_interface *xc_handle,
> > -                               uint32_t domain,
> > -                               uint64_t gpfn)
> > -{
> > -    xen_pfn_t *live_m2p = NULL;
> > -    int ret;
> > -    unsigned long hvirt_start;
> > -    unsigned int pt_levels;
> > -    uint64_t * pfn_buf = NULL;
> > -    unsigned long max_mfn = 0; /* max mfn of the whole machine */
> > -    unsigned long m2p_mfn0;
> > -    unsigned int guest_width;
> > -    long max_gpfn,i;
> > -    uint64_t mfn = MCE_INVALID_MFN;
> > -
> > -    if ( domain > DOMID_FIRST_RESERVED )
> > -        return MCE_INVALID_MFN;
> > -
> > -    /* Get max gpfn */
> > -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
> > -                               sizeof(domain)) + 1;
> > -    if ( max_gpfn <= 0 )
> > -        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
> > -
> > -    Lprintf("Maxium gpfn for dom %d is 0x%lx", domain, max_gpfn);
> > -
> > -    /* Get max mfn */
> > -    if ( !get_platform_info(xc_handle, domain,
> > -                            &max_mfn, &hvirt_start,
> > -                            &pt_levels, &guest_width) )
> > -        err(xc_handle, "Failed to get platform information");
> > -
> > -    /* Get guest's pfn list */
> > -    pfn_buf = calloc(max_gpfn, sizeof(uint64_t));
> > -    if ( !pfn_buf )
> > -        err(xc_handle, "Failed to alloc pfn buf");
> > -
> > -    ret = xc_get_pfn_list(xc_handle, domain, pfn_buf, max_gpfn);
> > -    if ( ret < 0 ) {
> > -        free(pfn_buf);
> > -        err(xc_handle, "Failed to get pfn list %x", ret);
> > -    }
> > -
> > -    /* Now get the m2p table */
> > -    live_m2p = xc_map_m2p(xc_handle, max_mfn, PROT_READ, &m2p_mfn0);
> > -    if ( !live_m2p )
> > -        err(xc_handle, "Failed to map live M2P table");
> > -
> > -    /* match the mapping */
> > -    for ( i = 0; i < max_gpfn; i++ )
> > -    {
> > -        uint64_t tmp;
> > -        tmp = pfn_buf[i];
> > -
> > -        if (mfn_valid(tmp) &&  (mfn_to_pfn(tmp) == gpfn))
> > -        {
> > -            mfn = tmp;
> > -            Lprintf("We get the mfn 0x%lx for this injection", mfn);
> > -            break;
> > -        }
> > -    }
> > -
> > -    munmap(live_m2p, M2P_SIZE(max_mfn));
> > -
> > -    free(pfn_buf);
> > -    return mfn;
> > -}
> > -
> > -static uint64_t mca_gpfn_to_mfn(xc_interface *xc_handle,
> > -                                uint32_t domain,
> > -                                uint64_t gfn)
> > -{
> > -    uint64_t index;
> > -    long max_gpfn;
> > -
> > -    /* If domain is xen, means we want pass index directly */
> > -    if ( domain == DOMID_XEN )
> > -        return gfn;
> > -
> > -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
> > -                               sizeof(domain)) + 1;
> > -    if ( max_gpfn <= 0 )
> > -        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
> > -    index = gfn % max_gpfn;
> > -
> > -    return guest_mfn(xc_handle, domain, index);
> > +    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val, domid);
> >  }
> >
> >  static int inject_mcg_status(xc_interface *xc_handle,
> > @@ -374,7 +287,7 @@ static int inject_mcg_status(xc_interface *xc_handle,
> >                               uint64_t val)
> >  {
> >      return add_msr_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
> > -                               MSR_IA32_MCG_STATUS, val);
> > +                               MSR_IA32_MCG_STATUS, val, 0);
> >  }
> >
> >  static int inject_mci_status(xc_interface *xc_handle,
> > @@ -383,7 +296,7 @@ static int inject_mci_status(xc_interface *xc_handle,
> >                               uint64_t val)
> >  {
> >      return add_msr_bank_intpose(xc_handle, cpu_nr,
> > MC_MSRINJ_F_INTERPOSE,
> > -                                    MCi_type_STATUS, bank, val);
> > +                                    MCi_type_STATUS, bank, val, 0);
> >  }
> >
> >  static int inject_mci_misc(xc_interface *xc_handle,
> > @@ -392,22 +305,23 @@ static int inject_mci_misc(xc_interface *xc_handle,
> >                               uint64_t val)
> >  {
> >      return add_msr_bank_intpose(xc_handle, cpu_nr,
> > MC_MSRINJ_F_INTERPOSE,
> > -                                    MCi_type_MISC, bank, val);
> > +                                    MCi_type_MISC, bank, val, 0);
> >  }
> >
> >  static int inject_mci_addr(xc_interface *xc_handle,
> >                               uint32_t cpu_nr,
> >                               uint64_t bank,
> > -                             uint64_t val)
> > +                             uint64_t val,
> > +                             domid_t domid)
> >  {
> > -    return add_msr_bank_intpose(xc_handle, cpu_nr,
> > MC_MSRINJ_F_INTERPOSE,
> > -                                    MCi_type_ADDR, bank, val);
> > +    return add_msr_bank_intpose(xc_handle, cpu_nr,
> > +                                    MC_MSRINJ_F_INTERPOSE |
> > MC_MSRINJ_F_GPADDR,
> > +                                    MCi_type_ADDR, bank, val, domid);
> >  }
> >
> >  static int inject(xc_interface *xc_handle, struct mce_info *mce,
> >                    uint32_t cpu_nr, uint32_t domain, uint64_t gaddr)
> >  {
> > -    uint64_t gpfn, mfn, haddr;
> >      int ret = 0;
> >
> >      ret = inject_mcg_status(xc_handle, cpu_nr, mce->mcg_stat);
> > @@ -424,12 +338,7 @@ static int inject(xc_interface *xc_handle, struct
> > mce_info *mce,
> >      if ( ret )
> >          err(xc_handle, "Failed to inject MCi_MISC MSR");
> >
> > -    gpfn = gaddr >> PAGE_SHIFT;
> > -    mfn = mca_gpfn_to_mfn(xc_handle, domain, gpfn);
> > -    if (!mfn_valid(mfn))
> > -        err(xc_handle, "The MFN is not valid");
> > -    haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
> > -    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, haddr);
> > +    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, gaddr, domain);
> >      if ( ret )
> >          err(xc_handle, "Failed to inject MCi_ADDR MSR");
> >
> > @@ -507,7 +416,7 @@ int main(int argc, char *argv[])
> >      uint32_t domid;
> >      xc_interface *xc_handle;
> >      int cpu_nr;
> > -    int64_t gaddr, gpfn, mfn, haddr, max_gpa;
> > +    uint64_t gaddr, max_gpa;
> >
> >      /* Default Value */
> >      domid = DOMID_XEN;
> > @@ -563,16 +472,10 @@ int main(int argc, char *argv[])
> >      Lprintf("get gaddr of error inject is: 0x%lx", gaddr);
> >
> >      if ( dump ) {
> > -        gpfn = gaddr >> PAGE_SHIFT;
> > -        mfn = mca_gpfn_to_mfn(xc_handle, domid, gpfn);
> > -        if (!mfn_valid(mfn))
> > -            err(xc_handle, "The MFN is not valid");
> > -        haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
> >          if ( domid == DOMID_XEN )
> > -            Lprintf("Xen: mfn=0x%lx, haddr=0x%lx", mfn, haddr);
> > +            Lprintf("Xen: gaddr=0x%lx", gaddr);
> >          else
> > -            Lprintf("Dom%d: gaddr=0x%lx, gpfn=0x%lx, mfn=0x%lx,
> > haddr=0x%lx",
> > -                    domid, gaddr, gpfn, mfn, haddr);
> > +            Lprintf("Dom%d: gaddr=0x%lx", domid, gaddr);
> >          goto out;
> >      }
> >
- Haozhong Zhang