Re: [PATCH RFC 04/12] accel/kvm: Add CGS option to control in-place conversion support

[Markus Armbruster <armbru@redhat.com>]

Michael Roth <michael.roth@amd.com> writes:

> On Tue, Jun 02, 2026 at 10:23:40AM +0200, Markus Armbruster wrote:
>> Michael Roth <michael.roth@amd.com> writes:
>> 
>> > For confidential guests, guest_memfd is currently used only for private
>> > guest memory, and normal guest memory comes from the configured memory
>> > backend just as it does for a non-confidential guest. It is now possible
>> > to use the same physical memory to back a particular GPA regardless of
>> > whether it is in a shared or private state. This avoids the need to
>> > rely on discarding memory between shared/private conversions (to avoid
>> > doubled memory usage), and is intended to be the primary mode of using
>> > guest_memfd for confidential guests moving forward, and future features
>> > like hugepage support will likely require it.
>> >
>> > Add an option to enable this support. Since ConfidentialGuestSupport is
>> > already used to track some guest_memfd-related functionality (e.g.
>> > whether it is required for the configured machine), similarly introduce
>> > this option as a property of ConfidentialGuestSupport.
>> >
>> > Also add the KVM-specific checks to enable this support, but leave the
>> > option disabled until other required changes are implemented for
>> > CGS variants that intend to make use of KVM's in-place conversion
>> > support.
>> >
>> > Signed-off-by: Michael Roth <michael.roth@amd.com>
>> 
>> [...]
>> 
>> > diff --git a/qapi/qom.json b/qapi/qom.json
>> > index 502fafeb15..037c078799 100644
>> > --- a/qapi/qom.json
>> > +++ b/qapi/qom.json
>> > @@ -1014,6 +1014,21 @@
>> >    'if': 'CONFIG_IGVM',
>> >    'data': { 'file': 'str' } }
>> >  
>> > +##
>> > +# @ConfidentialGuestSupportProperties:
>> > +#
>> > +# Properties for ConfidentialGuestSupport base class.
>> > +#
>> > +# @convert-in-place: If true, the same physical pages are reused
>> > +#     when memory is converted between shared and private states.
>> > +#     If false (default), separate allocations are used depending
>> > +#     on whether the page is private or shared.
>> > +#
>> > +# Since: 11.1
>> > +##
>> > +{ 'struct': 'ConfidentialGuestSupportProperties',
>> > +  'data': { '*convert-in-place': 'bool' } }
>> > +
>> >  ##
>> >  # @SevCommonProperties:
>> >  #
>> > @@ -1038,6 +1053,7 @@
>> >  # Since: 9.1
>> >  ##
>> >  { 'struct': 'SevCommonProperties',
>> > +  'base': 'ConfidentialGuestSupportProperties',
>> >    'data': { '*sev-device': 'str',
>> >              '*cbitpos': 'uint32',
>> >              'reduced-phys-bits': 'uint32',
>> 
>> Why use a base type instead of simply adding @convert-in-place to
>> SevCommonProperties?
>> 
>
> My thinking was that TDX and other implementations would similarly enable
> this through their CGS implementation, so I went ahead and carved out a
> set of common properties that ConfidentialGuestSupport implementations
> could use the same ,convert-in-place=true option (or set it by default
> for newer implementations)

How confident are we in future reuse by TDX and others?

If there are doubts, refactoring for reuse when reuse happens would be
smarter.  The refactoring would be a bit of churn, but not all that
much.

If it's something like "pretty much inevitable", preparing the reuse now
saves us that churn, and makes sense.

Judgement call, i.e. you decide.

> It is sort of tied to the 'allow_convert_in_place' flag that is part of
> the common ConfidentialGuestSupport object struct, so the property
> handling is sort of tied to the common ConfidentialGuestSupport base
> class as well rather than something implementation-specific.

Valid point.  Not sure how much weight to assign to it, though.

> Not sure if there are better ways to handle all that though.

Work your rationale into the commit message, please.