Re: [PATCH v15 02/11] x86: add generic resource (e.g. MSR) access hypercall

[Konrad Rzeszutek Wilk <konrad@darnok.org>]

On Wed, Sep 10, 2014 at 10:55:12AM +0800, Chao Peng wrote:
> On Fri, Sep 05, 2014 at 11:59:11AM +0100, Andrew Cooper wrote:
> > On 05/09/14 09:37, Chao Peng wrote:
> > > Add a generic resource access hypercall for tool stack or other
> > > components, e.g., accessing MSR, port I/O, etc.
> > >
> > > 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 |   63 +++++++++++++++++++++++++++++++++++++
> > >  xen/include/public/platform.h     |   15 +++++++++
> > >  xen/include/xlat.lst              |    1 +
> > >  3 files changed, 79 insertions(+)
> > >
> > > diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
> > > index 2162811..e5ad4c9 100644
> > > --- a/xen/arch/x86/platform_hypercall.c
> > > +++ b/xen/arch/x86/platform_hypercall.c
> > > @@ -61,6 +61,42 @@ 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;
> > > +    struct xenpf_resource_op *data;
> > > +};
> > > +
> > > +static bool_t allow_access_msr(unsigned int msr)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > > +static void resource_access_one(void *info)
> > > +{
> > > +    struct xen_resource_access *ra = info;
> > > +    int ret = 0;
> > > +
> > > +    switch ( ra->data->cmd )
> > > +    {
> > > +    case XEN_RESOURCE_OP_MSR_READ:
> > > +    case XEN_RESOURCE_OP_MSR_WRITE:
> > > +        if ( ra->data->idx >> 32 )
> > > +            ret = -EINVAL;
> > > +        else if ( !allow_access_msr(ra->data->idx) )
> > > +            ret = -EACCES;
> > > +        else if ( ra->data->cmd == XEN_RESOURCE_OP_MSR_READ )
> > > +            ret = rdmsr_safe(ra->data->idx, ra->data->val);
> > > +        else
> > > +            ret = wrmsr_safe(ra->data->idx, ra->data->val);
> > > +        break;
> > > +    default:
> > > +        ret = -EINVAL;
> > > +        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 +637,33 @@ 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.data = rsc_op;
> > > +
> > > +        if ( rsc_op->cpu == cpu )
> > > +            resource_access_one(&ra);
> > > +        else if ( cpu_online(rsc_op->cpu) )
> > > +            on_selected_cpus(cpumask_of(rsc_op->cpu),
> > 
> > You must validate rsc_op->cpu before using it.  cpumask_of(something
> > large) will happily wander off the end of an array.
> cpu_online() should detect this.

Why would it? It just looks an array and checks to see if the bit is
set. (If you look at the ASSERT in 'cpumask_check' - the assert is not
part of non-debug build).

> > 
> > > +                         resource_access_one, &ra, 1);
> > > +        else
> > > +        {
> > > +            ret = -ENODEV;
> > > +            break;
> > > +        }
> > > +
> > > +        if ( ra.ret )
> > > +        {
> > > +            ret = ra.ret;
> > > +            break;
> > > +        }
> > > +    }
> > > +    break;
> > > +
> > >      default:
> > >          ret = -ENOSYS;
> > >          break;
> > > diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
> > > index 053b9fa..3ed50de 100644
> > > --- a/xen/include/public/platform.h
> > > +++ b/xen/include/public/platform.h
> > > @@ -527,6 +527,20 @@ 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
> > > +
> > > +#define XEN_RESOURCE_OP_MSR_READ  0
> > > +#define XEN_RESOURCE_OP_MSR_WRITE 1
> > > +
> > > +struct xenpf_resource_op {
> > > +    uint16_t cmd;       /* XEN_RESOURCE_OP_* */
> > 
> > There is an alignment issue here between 32 and 64bit. Putting an an
> > explicit unit16_t _resd; field should fix it.
> > 
> > ~Andrew
> OK, thanks.

You can also use 'pahole' on the Xen binary to figure these issues out.

> > 
> > > +    uint32_t cpu;
> > > +    uint64_t idx;
> > > +    uint64_t val;
> > > +};
> > > +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 +567,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;
> > >          uint8_t                        pad[128];
> > >      } u;
> > >  };
> > > diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
> > > index 9a35dd7..06ed7b9 100644
> > > --- a/xen/include/xlat.lst
> > > +++ b/xen/include/xlat.lst
> > > @@ -88,6 +88,7 @@
> > >  ?	xenpf_enter_acpi_sleep		platform.h
> > >  ?	xenpf_pcpuinfo			platform.h
> > >  ?	xenpf_pcpu_version		platform.h
> > > +?	xenpf_resource_op		platform.h
> > >  !	sched_poll			sched.h
> > >  ?	sched_remote_shutdown		sched.h
> > >  ?	sched_shutdown			sched.h
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel