Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 29/07/2020 21:02, Jan Beulich wrote:
> On 28.07.2020 13:37, Andrew Cooper wrote:
>> The existing logic doesn't function in the general case for mapping a
>> guests
>> grant table, due to arbitrary 32 frame limit, and the default grant
>> table
>> limit being 64.
>>
>> In order to start addressing this, rework the existing grant table
>> logic by
>> implementing a single gnttab_acquire_resource().  This is far more
>> efficient
>> than the previous acquire_grant_table() in memory.c because it
>> doesn't take
>> the grant table write lock, and attempt to grow the table, for every
>> single
>> frame.
>
> Among the code you replace there is a comment "Iterate backwards in case
> table needs to grow" explaining why what you say about growing the grant
> table didn't actually happen.

What I have said is accurate.

Iterating backwards caused it to actually grow once, but every iteration
still takes the lock and attempts to grow the table.

>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct
>> domain *d,
>>       return 0;
>>   }
>>   +int gnttab_acquire_resource(
>> +    struct domain *d, unsigned int id, unsigned long frame,
>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>> +{
>> +    struct grant_table *gt = d->grant_table;
>> +    unsigned int i = nr_frames, tot_frames;
>> +    void **vaddrs;
>> +    int rc = 0;
>> +
>> +    /* Input sanity. */
>> +    if ( !nr_frames )
>> +        return -EINVAL;
>
> I can't seem to be able to find an equivalent of this in the old logic,
> and hence this looks like an unwarranted change in behavior to me. We
> have quite a few hypercall ops where some count being zero is simply
> a no-op, i.e. yielding success without doing anything.

There is no possible circumstance when getting here requesting 0 is
legitimate.

Tolerating a zero input for a mapping request is a very expensive way of
hiding caller bugs.

Most importantly however, the correctness of the logic does depends on
nr_frames being nonzero.

>
>> +    /* Overflow checks */
>> +    if ( frame + nr_frames < frame )
>> +        return -EINVAL;
>> +
>> +    tot_frames = frame + nr_frames;
>> +    if ( tot_frames != frame + nr_frames )
>> +        return -EINVAL;
>
> I find the naming here quite confusing. I realize part of this stems
> from the code you replace, but anyway: "unsigned long frame" typically
> represents a memory frame number of some sort, making the calculation
> look as if it was wrong. (Initially I merely meant to ask whether this
> check isn't redundant with the prior one, or vice versa.)

Both checks are strictly necessary.  The top one checks 64bit wrapping,
while the second is checking 32bit truncation.

This code is horrible, both in terms of the acquire_resource API, and
the grant API being off by one compared to normal, and needing the
max_frame index, not the number of frames.

~Andrew