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 <[email protected]>
>
> Acked-by: Roger Pau Monné <[email protected]>
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
