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

[Andrew Cooper <amc96@xxxxxxxxx>]

On 28/09/2020 10:37, Jan Beulich wrote:
> On 22.09.2020 20:24, Andrew Cooper wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4632,7 +4632,6 @@ int arch_acquire_resource(struct domain *d, unsigned 
>> int type,
>>          if ( id != (unsigned int)ioservid )
>>              break;
>>  
>> -        rc = 0;
>>          for ( i = 0; i < nr_frames; i++ )
>>          {
>>              mfn_t mfn;
>> @@ -4643,6 +4642,9 @@ int arch_acquire_resource(struct domain *d, unsigned 
>> int type,
>>  
>>              mfn_list[i] = mfn_x(mfn);
>>          }
>> +        if ( i == nr_frames )
>> +            /* Success.  Passed nr_frames back to the caller. */
>> +            rc = nr_frames;
> With this, shouldn't the return type of the function be changed to
> "long"? I realize that's no an issue with XENMEM_resource_ioreq_server
> specifically, but I mean the general case.

That would require going back in time and making a more sane ABI for
struct xen_mem_acquire_resource

We really do have a 32bit nr_frames field, and a 64bit "where to
continue from" field.

>> --- 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.

>
>> --- 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.

>
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -1027,17 +1027,31 @@ static unsigned int resource_max_frames(struct 
>> domain *d,
>>      }
>>  }
>>  
>> +/*
>> + * Returns -errno on error, or positive in the range [1, nr_frames] on
>> + * success.  Returning less than nr_frames contitutes a request for a
>> + * continuation.
>> + */
>> +static int _acquire_resource(
>> +    struct domain *d, unsigned int type, unsigned int id, unsigned long 
>> frame,
>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
> As per the comment the return type may again want to be "long" here.
> Albeit I realize the restriction to (UINT_MAX >> MEMOP_EXTENT_SHIFT)
> makes this (and the other place above) only a latent issue for now,
> so it may well be fine to be left as is.

Hmm yes - it should be long, because per the ABI we still should be able
to return 0xffffffff to a caller in the success case.

I'll update.

~Andrew