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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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.

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

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

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


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