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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/09/2020 11:47, Paul Durrant wrote:
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index e82307bdae..8628f51402 100644
>> --- 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. */
> Nit: s/passed/pass

That would change the meaning of the comment, and it would no longer be
accurate.

>
>> +            rc = nr_frames;
>>          break;
>>      }
>>  #endif
>> diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
>> index 834c5e19d1..17619f26ed 100644
>> --- a/xen/common/compat/memory.c
>> +++ b/xen/common/compat/memory.c
>> @@ -611,6 +622,21 @@ int compat_memory_op(unsigned int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>                  break;
>>              }
>>
>> +            if ( split < 0 )
>> +            {
>> +                /* Contintuation occured. */
>> +                ASSERT(rc != XENMEM_acquire_resource);
> Do you mean "op !=" here?

No.  Such an assertion had better trigger every time we hit this path.

rc is the continuation information that we're in the middle of
processing.  See also the comment about this being a giant undebuggable
mess.

~Andrew