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

[Jan Beulich <jbeulich@suse.com>]

On 01.09.2021 22:02, Andrew Cooper wrote:
> On 01/09/2021 17:06, 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@suse.com>
>> ---
>> 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).
> 
> Considering there are no production users of gnttab_transfer(), we can
> do what we want.  It was introduced for (IIRC) netlink2 and never got
> into production, and then we clobbered it almost entirely in an XSA
> several years ago by restricting steal_page() to PV guests only.
> 
> As a consequence, we can do anything which seems sensible, and does not
> necessarily need to be bound by a guest spinning on the bit.

Is this a "yes, let's go that route" then? Or rather leaving it to me,
i.e. translating "we can do anything which seems sensible" to "feel free
to do anything which seems sensible"? Which might as well be what is
there right now, and hence there could be the implied question of
whether your reply could be translated to an ack.

> The concept of gnttab_transfer() alone is crazy from an in-guest memory
> management point of view.  We could alternatively save our future selves
> more trouble by just Kconfig'ing it out now, deleting it in several
> releases time, and fogetting about the problem as nothing will break in
> practice.

I might ack such an initial patch. I might even consider making one
myself as long as it's agreed that the option will need to default to
y. I might also ack such a subsequent patch. But I would not want to
be the one to propose a patch removing functionality. I think I did say
more than once in the past that I don't think we can validly remove
anything from the public interface, as we will never be able to _prove_
there's no user anywhere. An exception might only be in cases where we
can prove certain functionality could never have been used successfully
for its intended or any other purpose. (For example, recently I've
[again] been considering to fully disable XENMEM_increase_reservation
for translated guests, rather than just leaving it useless by not
reporting back the allocated MFNs.)

Jan