Re: [PATCH] common: guest_physmap_add_page()'s return value needs checking

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.09.2021 11:20, Roger Pau Monné wrote:
> On Wed, Sep 01, 2021 at 06:06:37PM +0200, Jan Beulich wrote:
>> The function may fail; it is not correct to indicate "success" in this
>> case up the call stack. Mark the function must-check to prove all
>> cases have been caught (and no new ones will get introduced).
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks. Albeit strictly speaking an ack here isn't enough for the change
to go in, it would need to be R-b or come from a REST maintainer.

>> ---
>> In the grant-transfer case it is not really clear to me whether we can
>> stick to setting GTF_transfer_completed in the error case. Since a guest
>> may spin-wait for the flag to become set, simply not setting the flag is
>> not an option either. I was wondering whether we may want to slightly
>> alter (extend) the ABI and allow for a GTF_transfer_committed ->
>> GTF_transfer_completed transition (i.e. clearing GTF_transfer_committed
>> at the same time as setting GTF_transfer_completed).
>>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -2394,7 +2394,7 @@ gnttab_transfer(
>>          {
>>              grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, 
>> gop.ref);
>>  
>> -            guest_physmap_add_page(e, _gfn(sha->frame), mfn, 0);
>> +            rc = guest_physmap_add_page(e, _gfn(sha->frame), mfn, 0);
>>              if ( !paging_mode_translate(e) )
>>                  sha->frame = mfn_x(mfn);
>>          }
>> @@ -2402,7 +2402,7 @@ gnttab_transfer(
>>          {
>>              grant_entry_v2_t *sha = &shared_entry_v2(e->grant_table, 
>> gop.ref);
>>  
>> -            guest_physmap_add_page(e, _gfn(sha->full_page.frame), mfn, 0);
>> +            rc = guest_physmap_add_page(e, _gfn(sha->full_page.frame), mfn, 
>> 0);
>>              if ( !paging_mode_translate(e) )
>>                  sha->full_page.frame = mfn_x(mfn);
> 
> Is it fine to set the frame even if updating the physmap failed?

Well - the page is now owned by that domain, so there's nothing outright
wrong with reporting its MFN. guest_physmap_add_page() failing in the
!paging_mode_translate() is also only possible under obscure conditions,
with the guest guessing about MFNs it is in the process of getting
assigned.

>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -268,7 +268,8 @@ static void populate_physmap(struct memo
>>                  mfn = page_to_mfn(page);
>>              }
>>  
>> -            guest_physmap_add_page(d, _gfn(gpfn), mfn, a->extent_order);
>> +            if ( guest_physmap_add_page(d, _gfn(gpfn), mfn, 
>> a->extent_order) )
>> +                goto out;
>>  
>>              if ( !paging_mode_translate(d) &&
>>                   /* Inform the domain of the new page's machine address. */
>> @@ -765,8 +766,8 @@ static long memory_exchange(XEN_GUEST_HA
>>              }
>>  
>>              mfn = page_to_mfn(page);
>> -            guest_physmap_add_page(d, _gfn(gpfn), mfn,
>> -                                   exch.out.extent_order);
>> +            rc = guest_physmap_add_page(d, _gfn(gpfn), mfn,
>> +                                        exch.out.extent_order) ?: rc;
>>              if ( !paging_mode_translate(d) &&
>>                   __copy_mfn_to_guest_offset(exch.out.extent_start,
> 
> Would it be worth it setting the mfn on the guest output to
> INVALID_MFN or some such if the physmap addition failed?

Like above - the page is in possession of the guest now. Once it knows
of the MFN it may be able to do something to remedy the error (at the
very least: free the page again, e.g. via decrease-reservation, where
only the MFN is needed).

Of course in both cases a prereq requirement on guest behavior would
be that they consume the output field in the first place despite the
error, which in turn requires them to prefill the field with a sentinel
allowing them to recognize whether a valid MFN was passed back.

Jan