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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 29.09.14 at 20:52, <konrad@darnok.org> wrote:
> 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).

Indeed - this needs to be a range check followed by cpu_online().

Jan