Re: [PATCH v16 01/10] x86: add generic resource (e.g. MSR) access hypercall

[Chao Peng <chao.p.peng@linux.intel.com>]

On Thu, Sep 25, 2014 at 04:12:18PM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Sep 25, 2014 at 08:57:38PM +0100, Andrew Cooper wrote:
> > On 25/09/14 11:19, Chao Peng wrote:
> > > Add a generic resource access hypercall for tool stack or other
> > > components, e.g., accessing MSR, port I/O, etc.
> > >
> 
> You should include a bit more information in the description.
> Please give a bit information on what kind of parameters the
> hypercall is to have.
ok
> 
> > > Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> > > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > > ---
> > >  xen/arch/x86/platform_hypercall.c        |   90 ++++++++++++++++++++++++++++++
> > >  xen/arch/x86/x86_64/platform_hypercall.c |    4 ++
> > >  xen/include/public/platform.h            |   23 ++++++++
> > >  xen/include/xlat.lst                     |    2 +
> > >  4 files changed, 119 insertions(+)
> > >
> > > diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
> > > index 2162811..081d9f5 100644
> > > --- a/xen/arch/x86/platform_hypercall.c
> > > +++ b/xen/arch/x86/platform_hypercall.c
> > > @@ -61,6 +61,68 @@ long cpu_down_helper(void *data);
> > >  long core_parking_helper(void *data);
> > >  uint32_t get_cur_idle_nums(void);
> > >  
> > > +struct xen_resource_access {
> > > +    int32_t ret;
> > > +    uint32_t nr;
> > > +    XEN_GUEST_HANDLE(xenpf_resource_data_t) data;
> > > +};
> > > +
> > > +static bool_t allow_access_msr(unsigned int msr)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > > +static void resource_access(void *info)
> > > +{
> > > +    struct xen_resource_access *ra = info;
> > > +    xenpf_resource_data_t data;
> > > +    int ret = 0;
> > > +    unsigned int i;
> > > +
> > > +    for ( i = 0; i < ra->nr; i++ )
> > > +    {
> > > +        if ( copy_from_guest_offset(&data, ra->data, i, 1) )
> > 
> > You cannot use copy_{to,from}_guest() here.  You are almost certainly on
> > the wrong cpu, and running on the wrong set of pagetables.
> > 
> > At best, you will copy bogus data from the wrong process/domain, and
> > likely corrupt it when copying back.
> > 
> > > +        {
> > > +            ret = -EFAULT;
> > > +            break;
> > > +        }
> > > +
> > > +        if ( data.rsvd ) {
> 
> That '{' needs to be on its own line.
> 
> > > +            ret = -EINVAL;
> > > +            break;
> > > +        }
> > > +
> > > +        switch ( data.cmd )
> > > +        {
> > > +        case XEN_RESOURCE_OP_MSR_READ:
> > > +        case XEN_RESOURCE_OP_MSR_WRITE:
> > > +            if ( data.idx >> 32 )
> > > +                ret = -EINVAL;
> > > +            else if ( !allow_access_msr(data.idx) )
> > > +                ret = -EACCES;
> > > +            else if ( data.cmd == XEN_RESOURCE_OP_MSR_READ )
> > > +                ret = rdmsr_safe(data.idx, data.val);
> > > +            else
> > > +                ret = wrmsr_safe(data.idx, data.val);
> > > +            break;
> > > +        default:
> > > +            ret = -EINVAL;
> > > +            break;
> > > +        }
> > > +
> > > +        if ( ret )
> > > +            break;
> > > +
> > > +        if ( copy_to_guest_offset(ra->data, i, &data, 1) )
> > > +        {
> > > +            ret = -EFAULT;
> > > +            break;
> > > +        }
> > > +    }
> > > +
> > > +    ra->ret = ret;
> > > +}
> > > +
> > >  ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
> > >  {
> > >      ret_t ret = 0;
> > > @@ -601,6 +663,34 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
> > >      }
> > >      break;
> > >  
> > > +    case XENPF_resource_op:
> > > +    {
> > > +        struct xen_resource_access ra;
> > > +        struct xenpf_resource_op *rsc_op = &op->u.resource_op;
> > > +        unsigned int cpu = smp_processor_id();
> > > +
> > > +        ra.nr = rsc_op->nr;
> > > +        ra.data = rsc_op->data;
> > 
> > You must do all copy_{from,to}_user() here, and strictly only pass Xen
> > pointers to resource_access().
> > 
> > This means you will need to xmalloc() yourself some space for the
> > xenpf_resource_data_t array.
> > 
> > 
> > On a different note, you need to enforce a maximum resource_op.nr of
> > something rather low to (16/32 perhaps?) to prevent a toolstack asking
> > for 0xffffffff non-preemptible operations.
> 
> The toolstack has:
> 
> int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops)
> {
>     if ( nr_ops == 1 )
>         return xc_resource_op_one(xch, ops);
> 
> And with that expectation we ought to have a similar check here, in the form
> of:
> 
> 	if ( ra.nr != 1 )
> 		return -EINVAL;
> 
The 'nr' here is not the 'nr_ops', but the 'nr_entries' contained in an 'op'.
This hypercall can only process only one 'op' at a time, multiple 'ops'
are processed by multicall then pass each 'op' down to this hypercall.

While I think I can improve the name here(nr => nr_entries).
Thanks.

> > 
> > ~Andrew
> > 
> > > +
> > > +        if ( rsc_op->cpu == cpu )
> > > +            resource_access(&ra);
> > > +        else if ( cpu_online(rsc_op->cpu) )
> > > +            on_selected_cpus(cpumask_of(rsc_op->cpu),
> > > +                         resource_access, &ra, 1);
> > > +        else
> > > +        {
> > > +            ret = -ENODEV;
> > > +            break;
> > > +        }
> > > +
> > > +        if ( ra.ret )
> > > +        {
> > > +            ret = ra.ret;
> > > +            break;
> > > +        }
> > > +    }
> > > +    break;
> > > +
> > >      default:
> > >          ret = -ENOSYS;
> > >          break;
> > > diff --git a/xen/arch/x86/x86_64/platform_hypercall.c b/xen/arch/x86/x86_64/platform_hypercall.c
> > > index b6f380e..4db6622 100644
> > > --- a/xen/arch/x86/x86_64/platform_hypercall.c
> > > +++ b/xen/arch/x86/x86_64/platform_hypercall.c
> > > @@ -32,6 +32,10 @@ CHECK_pf_pcpu_version;
> > >  CHECK_pf_enter_acpi_sleep;
> > >  #undef xen_pf_enter_acpi_sleep
> > >  
> > > +#define xen_pf_resource_data xenpf_resource_data
> > > +CHECK_pf_resource_data;
> > > +#undef xen_pf_resource_data
> > > +
> > >  #define COMPAT
> > >  #define _XEN_GUEST_HANDLE(t) XEN_GUEST_HANDLE(t)
> > >  #define _XEN_GUEST_HANDLE_PARAM(t) XEN_GUEST_HANDLE_PARAM(t)
> > > diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
> > > index 053b9fa..e4d9091 100644
> > > --- a/xen/include/public/platform.h
> > > +++ b/xen/include/public/platform.h
> > > @@ -527,6 +527,28 @@ struct xenpf_core_parking {
> > >  typedef struct xenpf_core_parking xenpf_core_parking_t;
> > >  DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t);
> > >  
> > > +#define XENPF_resource_op   61
> 
> More details please.
ok
> > > +
> > > +#define XEN_RESOURCE_OP_MSR_READ  0
> > > +#define XEN_RESOURCE_OP_MSR_WRITE 1
> > > +
> > > +struct xenpf_resource_data {
> > > +    uint32_t cmd;       /* XEN_RESOURCE_OP_* */
> > > +    uint32_t rsvd;
> > > +    uint64_t idx;
> > > +    uint64_t val;
> 
> More details please. Pls say what the 'rsvd' is for, what
> the expected values are for 'idx', and 'val'.
> 
> Do also say which ones are IN or OUT.
> 
> Put yourself in the mindset of somebody who wants to use this
> and does not want to dive in the hypervisor to figure this out.
> Give as much information as possible in the headers.
> 
Good suggestion, I will follow this.
> > > +typedef struct xenpf_resource_data xenpf_resource_data_t;
> > > +DEFINE_XEN_GUEST_HANDLE(xenpf_resource_data_t);
> > > +
> > > +struct xenpf_resource_op {
> > > +    uint32_t nr;    /* number of data entry */
> > > +    uint32_t cpu;   /* which cpu to run */
> > > +    XEN_GUEST_HANDLE(xenpf_resource_data_t) data;
> > > +};
> > > +typedef struct xenpf_resource_op xenpf_resource_op_t;
> > > +DEFINE_XEN_GUEST_HANDLE(xenpf_resource_op_t);
> > > +
> > >  /*
> > >   * ` enum neg_errnoval
> > >   * ` HYPERVISOR_platform_op(const struct xen_platform_op*);
> > > @@ -553,6 +575,7 @@ struct xen_platform_op {
> > >          struct xenpf_cpu_hotadd        cpu_add;
> > >          struct xenpf_mem_hotadd        mem_add;
> > >          struct xenpf_core_parking      core_parking;
> > > +        struct xenpf_resource_op       resource_op;
> 
> resource_op?  I would really call this 'msr' or 'msr_data'
So you have known that this is not just for msr.
> 
> 
> > >          uint8_t                        pad[128];
> > >      } u;
> > >  };
> > > diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
> > > index 9a35dd7..100fcf5 100644
> > > --- a/xen/include/xlat.lst
> > > +++ b/xen/include/xlat.lst
> > > @@ -88,6 +88,8 @@
> > >  ?	xenpf_enter_acpi_sleep		platform.h
> > >  ?	xenpf_pcpuinfo			platform.h
> > >  ?	xenpf_pcpu_version		platform.h
> > > +?	xenpf_resource_op		platform.h
> > > +?	xenpf_resource_data		platform.h
> > >  !	sched_poll			sched.h
> > >  ?	sched_remote_shutdown		sched.h
> > >  ?	sched_shutdown			sched.h
> > 
> >