Re: [Xen-devel] [PATCH v11 1/9] x86: add generic MSR access hypercall

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Friday, June 20, 2014 10:57 PM
> To: Xu, Dongxiao; xen-devel@xxxxxxxxxxxxx
> Cc: keir@xxxxxxx; JBeulich@xxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx;
> Ian.Campbell@xxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx;
> George.Dunlap@xxxxxxxxxxxxx
> Subject: Re: [PATCH v11 1/9] x86: add generic MSR access hypercall
> 
> On 20/06/14 15:31, Dongxiao Xu wrote:
> > Add a generic MSR access hypercall for tool stack or other components to
> > read or write certain system MSRs.
> >
> > Signed-off-by: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> > ---
> >  xen/arch/x86/sysctl.c       | 54
> +++++++++++++++++++++++++++++++++++++++++++++
> >  xen/include/public/sysctl.h | 25 +++++++++++++++++++++
> >  2 files changed, 79 insertions(+)
> >
> > diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
> > index 15d4b91..49f95e4 100644
> > --- a/xen/arch/x86/sysctl.c
> > +++ b/xen/arch/x86/sysctl.c
> > @@ -66,10 +66,27 @@ void arch_do_physinfo(xen_sysctl_physinfo_t *pi)
> >          pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm_directio;
> >  }
> >
> > +static void msr_access_helper(void *data)
> > +{
> > +    struct msr_access_info *info = data;
> > +    xen_sysctl_msr_data_t *msr_data;
> > +    unsigned int i;
> > +
> > +    for ( i = 0; i < info->nr_ops; i++ )
> > +    {
> > +        msr_data = &info->msr_data[i];
> > +        if ( msr_data->cmd == XEN_SYSCTL_MSR_read )
> > +            rdmsrl(msr_data->msr, msr_data->val);
> > +        else if ( msr_data->cmd == XEN_SYSCTL_MSR_write )
> > +            wrmsrl(msr_data->msr, msr_data->val);
> 
> These must be the _safe() variants, otherwise it is trivial for a
> privileged domain to cause Xen to die with #GP faults.

Okay, thanks!

> 
> > +    }
> > +}
> > +
> >  long arch_do_sysctl(
> >      struct xen_sysctl *sysctl, XEN_GUEST_HANDLE_PARAM(xen_sysctl_t)
> u_sysctl)
> >  {
> >      long ret = 0;
> > +    bool_t copyback = 0;
> >
> >      switch ( sysctl->cmd )
> >      {
> > @@ -101,11 +118,48 @@ long arch_do_sysctl(
> >      }
> >      break;
> >
> > +    case XEN_SYSCTL_msr_op:
> > +    {
> > +        unsigned int cpu = sysctl->u.msr_op.cpu;
> > +        struct msr_access_info info;
> > +
> > +        info.nr_ops = sysctl->u.msr_op.nr_ops;
> > +        info.msr_data = xzalloc_array(xen_sysctl_msr_data_t, info.nr_ops);
> 
> This memory allocation can be avoided.  Do a copy_from_user() on the
> msr_op structure and pass the guest handle and nr into msr_access_helper().
> 
> It can then do copy_to/from_user on the individual entries.

This implementation pack all operations together towards one CPU. Jan is 
suggesting if we can specify one CPU for each operation.
If that's the case, I will think about whether we can eliminate the memory 
allocation here.

> 
> > +        if ( !info.msr_data )
> > +            return -ENOMEM;
> > +
> > +        if ( copy_from_guest(info.msr_data, sysctl->u.msr_op.msr_data,
> > +                             info.nr_ops) )
> > +        {
> > +            xfree(info.msr_data);
> > +            return -EFAULT;
> > +        }
> > +
> > +        if ( cpu == smp_processor_id() )
> > +            msr_access_helper(&info);
> > +        else
> > +            on_selected_cpus(cpumask_of(cpu), msr_access_helper, &info,
> 1);
> > +
> > +        if ( copy_to_guest(sysctl->u.msr_op.msr_data, info.msr_data,
> > +                           info.nr_ops ) )
> > +        {
> > +            xfree(info.msr_data);
> > +            return -EFAULT;
> > +        }
> > +
> > +        xfree(info.msr_data);
> > +        copyback = 1;
> > +    }
> > +    break;
> > +
> >      default:
> >          ret = -ENOSYS;
> >          break;
> >      }
> >
> > +    if ( copyback && __copy_to_guest(u_sysctl, sysctl, 1) )
> > +        ret = -EFAULT;
> > +
> >      return ret;
> >  }
> >
> > diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> > index 3588698..fd042bb 100644
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -636,6 +636,29 @@ struct xen_sysctl_coverage_op {
> >  typedef struct xen_sysctl_coverage_op xen_sysctl_coverage_op_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
> >
> > +#define XEN_SYSCTL_MSR_read                0
> > +#define XEN_SYSCTL_MSR_write               1
> > +
> > +struct xen_sysctl_msr_data {
> > +    uint32_t cmd;       /* XEN_SYSCTL_MSR_* */
> > +    uint32_t msr;
> > +    uint64_t val;
> > +};
> > +typedef struct xen_sysctl_msr_data xen_sysctl_msr_data_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_msr_data_t);
> > +struct xen_sysctl_msr_op {
> > +    uint32_t nr_ops;
> > +    uint32_t cpu;
> > +    XEN_GUEST_HANDLE_64(xen_sysctl_msr_data_t) msr_data;
> > +};
> > +typedef struct xen_sysctl_msr_op xen_sysctl_msr_op_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_msr_op_t);
> > +
> > +struct msr_access_info {
> > +    uint32_t nr_ops;
> > +    xen_sysctl_msr_data_t *msr_data;
> > +};
> 
> This is private to Xen.  It should not be here in a public header.

Okay, thanks!

Dongxiao

> 
> ~Andrew
> 
> > +
> >
> >  struct xen_sysctl {
> >      uint32_t cmd;
> > @@ -658,6 +681,7 @@ struct xen_sysctl {
> >  #define XEN_SYSCTL_cpupool_op                    18
> >  #define XEN_SYSCTL_scheduler_op                  19
> >  #define XEN_SYSCTL_coverage_op                   20
> > +#define XEN_SYSCTL_msr_op                        21
> >      uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
> >      union {
> >          struct xen_sysctl_readconsole       readconsole;
> > @@ -679,6 +703,7 @@ struct xen_sysctl {
> >          struct xen_sysctl_cpupool_op        cpupool_op;
> >          struct xen_sysctl_scheduler_op      scheduler_op;
> >          struct xen_sysctl_coverage_op       coverage_op;
> > +        struct xen_sysctl_msr_op            msr_op;
> >          uint8_t                             pad[128];
> >      } u;
> >  };


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