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

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

On Tue, Sep 02, 2014 at 09:52:21AM +0100, Jan Beulich wrote:
> >>> On 02.09.14 at 10:33, <chao.p.peng@linux.intel.com> wrote:
> > 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:
> >> > +    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.
> 
> You can't add any parameter to do_platform_op(), and I also
> don't see why you'd need to.
> 
> Jan
Let me shed more light on this.

The 'i' we want to pass to next iteration is saved in guest_cpu_user_regs in
hypercall_create_continuation(), which will be used as parameter for
the re-execution of do_platform_op(). Take do_hvm_op() as example,

 6199        rc = hypercall_create_continuation(__HYPERVISOR_hvm_op,"lh",
 6120                                            op | start_iter, arg);

It reuses several bits in existed param 'op' to pass the start_iter and then
the start_iter is obtained in the second call of do_hvm_op():

 5478     unsigned long start_iter = op & ~HVMOP_op_mask;

For our case we only save the 'i' into guest_cpu_user_regs but we lack of
way to accept it.

However, the method used in do_hvm_op() does not work for us as do_platform_op()
only has one point type param which we can't safely reuse any bit.

Thanks,
Chao