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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@intel.com>
> ---
>  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.

> +    }
> +}
> +
>  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.

> +        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.

~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;
>  };