Re: [PATCH v4 18/18] KVM: guest_memfd: Combine .gmem_prepare()+.gmem_invalidate() into .gmem_convert()

[Ackerley Tng]

Fuad Tabba <fuad.tabba@xxxxxxxxx> writes:

>
> [...snip...]
>

Thanks for bringing [1] up, I didn't make the link when this change was
being discussed.

pKVM definitely needs a callback when the folio is released from
guest_memfd's ownership.

>
> free_folio is the only to_private=false caller today, but the cover
> letter sketches the other one, a private->shared conversion through
> kvm_gmem_make_shared(inode, start, end). Once that lands both reach
> the hook with to_private=false, and as far as I can tell, the only
> thing separating them is kvm == NULL / INVALID_GPA. That isn't

On the folio teardown path kvm has to be non-NULL so that SNP can find
out which VM to unassign from, so you're right that INVALID_GPA won't be
sufficient to separate conversion to private and teardown, for pKVM,
from the arch perspective.

> documented as a contract, and nothing in-tree reads it on that branch
> (the kvm/gpa check is under if (to_private), and
> sev_gmem_make_shared() doesn't take a kvm at all), so nothing would
> catch it regressing. The sketch is also inode-scoped, so I'm not sure
> the conversion path passes a non-NULL kvm at all; if it doesn't, the
> two aren't distinguishable.
>
>>
>> [...snip...]
>>
>
> The Kconfig now couples them too: an arch can't take teardown without also
> taking conversion. pKVM wants teardown and not conversion, so that's a problem
> for us today, not only once the second caller lands.
>
> This is the same ground as v8 15/46, which ended with the two split under
> separate Kconfigs. Suzuki confirmed CCA disables both; pKVM wants teardown only,
> since our conversions are guest-initiated and EL2 has already done the
> transition by the time the host runs the generic path [1].
>
> I'd rather keep the free path on its own callback. If you want a single hook,
> passing the caller explicitly instead of leaving it implied by kvm would work
> too. Merging the two conversion directions is a genuine cleanup, it's the free
> path I'd like to keep separate.
>

Is it okay if later, we add a .gmem_teardown callback with a new CONFIG
flag that pKVM can use?

If that's okay, this merge (of function names and of the patch series)
can proceed. It's also nicer that the new callback and CONFIG are
explicitly (re?)-introduced with pKVM support so the introduction is
chronologically linked with pKVM support too :)

> Thanks,
> /fuad
>
> [1] https://lore.kernel.org/all/CA+EHjTwLRzrq0vY0-OosENJ2Gkknerux53aVBj0zGRQUkRQP=A@xxxxxxxxxxxxxx/
>
>>
>> [...snip...]
>>