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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.09.14 at 10:33, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> On Fri, Aug 29, 2014 at 04:40:52PM +0100, Jan Beulich wrote:
>> >>> On 28.08.14 at 09:43, <chao.p.peng@xxxxxxxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel