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

From: Andrew Cooper <amc96@cam.ac.uk>
To: Jan Beulich <jbeulich@suse.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>, "Julien Grall" <julien@xen.org>,
	"Paul Durrant" <paul@xen.org>,
	"Michał Leszczyński" <michal.leszczynski@cert.pl>,
	"Hubert Jasudowicz" <hubert.jasudowicz@cert.pl>,
	"Tamas K Lengyel" <tamas@tklengyel.com>,
	"Andrew Cooper" <amc96@cam.ac.uk>
Subject: Re: [PATCH v2 09/11] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource
Date: Mon, 11 Jan 2021 20:05:54 +0000	[thread overview]
Message-ID: <0462d837-947b-30a8-21ab-5a9a6b9ca4e1@cam.ac.uk> (raw)
In-Reply-To: <eb9768ad-b6cd-b01e-d689-63775f6e5914@suse.com>

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