Re: [Xen-devel] [PATCH 19/19] tools/xen-mceinj: support injecting LMCE

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On 02/20/17 12:53 +0000, Wei Liu wrote:
> On Fri, Feb 17, 2017 at 02:39:36PM +0800, Haozhong Zhang wrote:
> > If option '-l' or '--lmce' is specified and the host supports LMCE,
> > xen-mceinj will inject LMCE to CPU specified by '-c' (or CPU0 if '-c'
> > is not present).
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@xxxxxxxxx>
> > ---
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> >  tools/libxc/include/xenctrl.h           |  1 +
> >  tools/libxc/xc_misc.c                   | 25 +++++++++++++++
> 
> I suggest you split out changes to libxc to a separate patch.
>

will do in the next version

> >  tools/tests/mce-test/tools/xen-mceinj.c | 57 
> > +++++++++++++++++++++++++++++++--
> >  3 files changed, 81 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > index 85d7fe5..2598952 100644
> > --- a/tools/libxc/include/xenctrl.h
> > +++ b/tools/libxc/include/xenctrl.h
> > @@ -1968,6 +1968,7 @@ int xc_cpuid_apply_policy(xc_interface *xch,
> >  void xc_cpuid_to_str(const unsigned int *regs,
> >                       char **strs); /* some strs[] may be NULL if ENOMEM */
> >  int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
> > +int xc_mca_op_cpumap(xc_interface *xch, struct xen_mc *mc, xc_cpumap_t 
> > cpumap);
> >  #endif
> >  
> >  struct xc_px_val {
> > diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
> > index 0fc6c22..24f7fdf 100644
> > --- a/tools/libxc/xc_misc.c
> > +++ b/tools/libxc/xc_misc.c
> > @@ -341,6 +341,31 @@ int xc_mca_op(xc_interface *xch, struct xen_mc *mc)
> >      xc_hypercall_bounce_post(xch, mc);
> >      return ret;
> >  }
> > +
> > +int xc_mca_op_cpumap(xc_interface *xch, struct xen_mc *mc, xc_cpumap_t 
> > cpumap)
> > +{
> > +    int ret;
> > +    DECLARE_HYPERCALL_BOUNCE(cpumap, 0, XC_HYPERCALL_BUFFER_BOUNCE_IN);
> > +
> > +    if ( cpumap )
> > +    {
> > +        HYPERCALL_BOUNCE_SET_SIZE(cpumap,
> > +                                  (mc->u.mc_inject_v2.cpumap.nr_bits + 7) 
> > / 8);
> > +        if ( xc_hypercall_bounce_pre(xch, cpumap) )
> > +        {
> > +            PERROR("Could not bouce cpumap memory buffer");
> > +            return -1;
> > +        }
> > +        set_xen_guest_handle(mc->u.mc_inject_v2.cpumap.bitmap, cpumap);
> > +    }
> > +
> > +    ret = xc_mca_op(xch, mc);
> > +
> > +    if ( cpumap )
> > +        xc_hypercall_bounce_post(xch, cpumap);
> > +
> > +    return ret;
> > +}
> 
> I kinda see why you did this: the bounce buffer infrastructure isn't
> available to userspace program (by design). But this API isn't nice. 
> 
> This function replaces part of the struct. I suggest you construct a
> xen_mc struct solely within this function, not doing part of it here and
> the other part in another place (inject_lmce below).  Then you also need
> to name it properly.
>

ditto

> >  #endif
> >  
> >  int xc_perfc_reset(xc_interface *xch)
> > diff --git a/tools/tests/mce-test/tools/xen-mceinj.c 
> > b/tools/tests/mce-test/tools/xen-mceinj.c
> > index 5f70a61..b2eb7d3 100644
> > --- a/tools/tests/mce-test/tools/xen-mceinj.c
> > +++ b/tools/tests/mce-test/tools/xen-mceinj.c
> > @@ -56,6 +56,8 @@
> >  #define MSR_IA32_MC0_MISC        0x00000403
> >  #define MSR_IA32_MC0_CTL2        0x00000280
> >  
> > +#define MCG_STATUS_LMCE          0x8
> > +
> >  struct mce_info {
> >      const char *description;
> >      uint8_t mcg_stat;
> > @@ -113,6 +115,7 @@ static struct mce_info mce_table[] = {
> >  #define LOGFILE stdout
> >  
> >  int dump;
> > +int lmce;
> >  struct xen_mc_msrinject msr_inj;
> >  
> >  static void Lprintf(const char *fmt, ...)
> > @@ -213,6 +216,42 @@ static int inject_mce(xc_interface *xc_handle, int 
> > cpu_nr)
> >      return xc_mca_op(xc_handle, &mc);
> >  }
> >  
> > +static int inject_lmce(xc_interface *xc_handle, uint32_t cpu_nr)
> > +{
> > +    struct xen_mc mc;
> > +    uint8_t *cpumap = NULL;
> > +    size_t cpumap_size, line, shift;
> > +    uint32_t nr_cpus;
> > +    int ret;
> > +
> > +    nr_cpus = mca_cpuinfo(xc_handle);
> > +    if ( !nr_cpus )
> > +        err(xc_handle, "Failed to get mca_cpuinfo");
> 
> Why xc_handle in err()?
>

err() needs to close xc_handle.

Thanks,
Haozhong

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