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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.10.14 at 12:04, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> On Tue, Sep 30, 2014 at 01:12:42PM +0100, Jan Beulich wrote:
>> >>> On 30.09.14 at 12:49, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
>> > +    case XENPF_resource_op:
>> > +    {
>> > +        struct xen_resource_access ra;
>> > +        uint32_t cpu;
>> > +        XEN_GUEST_HANDLE(xenpf_resource_entry_t) guest_entries;
>> > +
>> > +        ra.nr_entries = op->u.resource_op.nr_entries;
>> > +        if ( ra.nr_entries == 0 || ra.nr_entries > 
>> > RESOURCE_ACCESS_MAX_ENTRIES )
>> > +        {
>> > +            ret = -EINVAL;
>> 
>> I don't think ra.nr_entries == 0 is a reason to fail the hypercall.
> Do you mean 'ret = 0' ?

Sure.

>> > +            break;
>> > +        }
>> > +
>> > +        ra.entries = xmalloc_array(xenpf_resource_entry_t, ra.nr_entries);
>> > +        if ( !ra.entries )
>> > +        {
>> > +            ret = -ENOMEM;
>> > +            break;
>> > +        }
>> > +
>> > +        guest_from_compat_handle(guest_entries, 
>> > op->u.resource_op.entries);
>> > +
>> > +        if ( copy_from_guest(ra.entries, guest_entries, ra.nr_entries) )
>> > +        {
>> > +            xfree(ra.entries);
>> > +            ret = -EFAULT;
>> > +            break;
>> > +        }
>> > +
>> > +        /* Do sanity check earlier to omit the potential IPI overhead. */
>> > +        if ( check_resource_access(&ra) < ra.nr_entries )
>> > +        {
>> > +            /* Copy the return value for failed entry. */
>> > +            if ( __copy_to_guest_offset(guest_entries, ret,
>> > +                                        ra.entries + ret, 1) )
>> > +                ret = -EFAULT;
>> > +            else
>> > +                ret = 0;
>> 
>> This should be the index of the failed entry. I guess it would be
>> easier and more consistent if check_resource_access() too used
>> ra.ret for passing back the failed index (which btw should be
>> renamed to e.g. "done" - "ret" is no longer a suitable name).
> 
> I agree to use ra.ret for check_resource_access(). But I insist that return 
> 0
> here is more reasonable. As we use positive return value to indicate the
> number of successful operations. But here we just passed some check and
> have even not performed the access. Return index of the failed entry
> will lead the caller to think the data for entries earlier is valid.

No - really correct behavior would be to carry out the operations
that passed the validation checks rather than exiting early here.

>> > +            on_selected_cpus(cpumask_of(cpu), resource_access, &ra, 1);
>> > +        else
>> > +        {
>> > +            xfree(ra.entries);
>> > +            ret = -ENODEV;
>> > +            break;
>> > +        }
>> > +
>> > +        /* Copy all if succeeded or up to the failed entry. */
>> > +        if ( __copy_to_guest_offset(guest_entries, 0, ra.entries,
>> > +                                    min(ra.nr_entries, ra.ret + 1)) )
>> 
>> I don't see a need for min() here - ra.ret mustn't be out of range.
>> If you're concerned, add an ASSERT().
> 
> For fully-succeeded case, ra.ret will be ra.nr_entries and ra.ret + 1 is out
> of range.

I don't think I understand what you're trying to tell me.

Jan


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