Re: [PATCH v2 09/11] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.01.2021 21:05, Andrew Cooper wrote:
> On 28/09/2020 10:37, Jan Beulich wrote:
>> On 22.09.2020 20:24, Andrew Cooper wrote:
>>> --- a/xen/common/compat/memory.c
>>> +++ b/xen/common/compat/memory.c
>>> @@ -636,15 +662,45 @@ int compat_memory_op(unsigned int cmd, 
>>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>>                      compat_frame_list[i] = frame;
>>>                  }
>>>  
>>> -                if ( __copy_to_compat_offset(cmp.mar.frame_list, 0,
>>> -                                             compat_frame_list,
>>> -                                             cmp.mar.nr_frames) )
>>> +                if ( __copy_to_compat_offset(
>>> +                         cmp.mar.frame_list, start_extent,
>>> +                         compat_frame_list, done) )
>>>                      return -EFAULT;
>>>              }
>>> -            break;
>>> +
>>> +            start_extent += done;
>>> +
>>> +            /* Completely done. */
>>> +            if ( start_extent == cmp.mar.nr_frames )
>>> +                break;
>>> +
>>> +            /*
>>> +             * Done a "full" batch, but we were limited by space in the 
>>> xlat
>>> +             * area.  Go around the loop again without necesserily 
>>> returning
>>> +             * to guest context.
>>> +             */
>>> +            if ( done == nat.mar->nr_frames )
>>> +            {
>>> +                split = 1;
>>> +                break;
>>> +            }
>>> +
>>> +            /* Explicit continuation request from a higher level. */
>>> +            if ( done < nat.mar->nr_frames )
>>> +                return hypercall_create_continuation(
>>> +                    __HYPERVISOR_memory_op, "ih",
>>> +                    op | (start_extent << MEMOP_EXTENT_SHIFT), compat);
>>> +
>>> +            /*
>>> +             * Well... Somethings gone wrong with the two levels of 
>>> chunking.
>>> +             * My condolences to whomever next has to debug this mess.
>>> +             */
>> Any suggestion how to overcome this "mess"?
> 
> The double level of array handling is what makes it so complicated. 
> There are enough cases in compat_memory_op() alone which can't
> 
> We've got two cases in practice.  A singleton object needing conversion,
> or a large array of them.  I'm quite certain we'd have less code and
> less complexity by having copy_$OJBECT_{to,from}_guest() helpers which
> dealt with compat internally as appropriate.
> 
> We don't care about the performance of 32bit hypercalls, but not doing
> batch conversions of 1020/etc objects in the compat layer will probably
> result in better performance overall, as we don't throw away the work as
> we batch things at smaller increments higher up the stack.

I've put this on my todo list to investigate. Maybe nowadays we
really don't care all this much about 32-bit hypercall performance.
The picture was surely different when the compat layer was
introduced.

>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -4105,6 +4105,9 @@ int gnttab_acquire_resource(
>>>      for ( i = 0; i < nr_frames; ++i )
>>>          mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
>>>  
>>> +    /* Success.  Passed nr_frames back to the caller. */
>> Nit: "Pass"?
> 
> We have already passed them back to the caller.  "Pass" is the wrong
> tense to use.

Hmm, interesting view. I personally wouldn't consider the
passing back to have completed before we've returned.

Jan