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

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

On Fri, Aug 29, 2014 at 04:40:52PM +0100, Jan Beulich wrote:
> >>> On 28.08.14 at 09:43, <chao.p.peng@linux.intel.com> wrote:
> > +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;
> > +        if ( !allow_access_msr(ra->data.idx) )
> > +            ret = -EACCES;
> > +        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;
> 
> Did you mean these latter tow if()-s perhaps be "else if"?
It is true, thanks.
> 
> > +    case XENPF_resource_op:
> > +    {
> > +        struct xen_resource_access ra;
> > +        struct xenpf_resource_op *rsc_op = &op->u.resource_op;
> > +        unsigned int i, j = 0, cpu = smp_processor_id();
> > +
> > +        for ( i = 0; i < rsc_op->nr; i++ )
> > +        {
> > +            if ( copy_from_guest_offset(&ra.data, rsc_op->data, i, 1) )
> > +            {
> > +                ret = -EFAULT;
> > +                break;
> > +            }
> > +
> > +            if ( ra.data.cpu == cpu )
> > +                resource_access_one(&ra);
> > +            else if ( cpu_online(ra.data.cpu) )
> > +                on_selected_cpus(cpumask_of(ra.data.cpu),
> > +                                 resource_access_one, &ra, 1);
> > +            else
> > +            {
> > +                ret = -ENODEV;
> > +                break;
> > +            }
> > +
> > +            if ( ra.ret )
> > +            {
> > +                ret = ra.ret;
> > +                break;
> > +            }
> > +
> > +            if ( copy_to_guest_offset(rsc_op->data, i, &ra.data, 1) )
> > +            {
> > +                ret = -EFAULT;
> > +                break;
> > +            }
> > +
> > +            /* Find the start point that requires no preemption */
> > +            if ( ra.data.flag && j == 0 )
> > +                j = i;
> > +            /* Set j = 0 when walking out of the non-preemption area */
> > +            if ( ra.data.flag == 0 )
> > +                j = 0;
> > +            if ( hypercall_preempt_check() )
> > +            {
> > +                ret = hypercall_create_continuation(
> > +                    __HYPERVISOR_platform_op, "ih",
> > +                    ra.data.flag ? j : i, u_xenpf_op);
> 
> Which means everything starting from j will be re-executed
> another time when continuing. That creates three problems: You
> can't guarantee forwards progress, you may do something
> having side effects more than once, and you break the operation
> in a place that was requested to not be preemptible.
I saw the problem here. Actually the j or i here will not be passed to
next iteration successfully. Possibly a 'count' param is needed to be
added to do_platform_op() for this purpose.
> 
> > +struct xenpf_resource_data {
> > +    uint16_t cmd;       /* XEN_RESOURCE_OP_* */
> > +    uint16_t flag;      /* avoid preemption between certain iterations */
> 
> You want to use just one bit out of this field (and verify all other bits
> are zero, so they can get assigned a meaning later).
OK, I will add a one-bit flag macro to indicate this.
> 
> Jan
> 
> > +    uint32_t cpu;
> > +    uint64_t idx;
> > +    uint64_t val;
> > +};