Re: [PATCH v3 21/49] i386/sev: Introduce "sev-common" type to encapsulate common SEV state

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Mar 20, 2024 at 03:39:17AM -0500, Michael Roth wrote:
>> Currently all SEV/SEV-ES functionality is managed through a single
>> 'sev-guest' QOM type. With upcoming support for SEV-SNP, taking this
>> same approach won't work well since some of the properties/state
>> managed by 'sev-guest' is not applicable to SEV-SNP, which will instead
>> rely on a new QOM type with its own set of properties/state.
>> 
>> To prepare for this, this patch moves common state into an abstract
>> 'sev-common' parent type to encapsulate properties/state that are
>> common to both SEV/SEV-ES and SEV-SNP, leaving only SEV/SEV-ES-specific
>> properties/state in the current 'sev-guest' type. This should not
>> affect current behavior or command-line options.
>> 
>> As part of this patch, some related changes are also made:
>> 
>>   - a static 'sev_guest' variable is currently used to keep track of
>>     the 'sev-guest' instance. SEV-SNP would similarly introduce an
>>     'sev_snp_guest' static variable. But these instances are now
>>     available via qdev_get_machine()->cgs, so switch to using that
>>     instead and drop the static variable.
>> 
>>   - 'sev_guest' is currently used as the name for the static variable
>>     holding a pointer to the 'sev-guest' instance. Re-purpose the name
>>     as a local variable referring the 'sev-guest' instance, and use
>>     that consistently throughout the code so it can be easily
>>     distinguished from sev-common/sev-snp-guest instances.
>> 
>>   - 'sev' is generally used as the name for local variables holding a
>>     pointer to the 'sev-guest' instance. In cases where that now points
>>     to common state, use the name 'sev_common'; in cases where that now
>>     points to state specific to 'sev-guest' instance, use the name
>>     'sev_guest'
>> 
>> Signed-off-by: Michael Roth <michael.roth@amd.com>
>> ---
>>  qapi/qom.json     |  32 ++--
>>  target/i386/sev.c | 457 ++++++++++++++++++++++++++--------------------
>>  target/i386/sev.h |   3 +
>>  3 files changed, 281 insertions(+), 211 deletions(-)
>> 
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index baae3a183f..66b5781ca6 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -875,12 +875,29 @@
>>    'data': { '*filename': 'str' } }
>>  
>>  ##
>> -# @SevGuestProperties:
>> +# @SevCommonProperties:
>>  #
>> -# Properties for sev-guest objects.
>> +# Properties common to objects that are derivatives of sev-common.
>>  #
>>  # @sev-device: SEV device to use (default: "/dev/sev")
>>  #
>> +# @cbitpos: C-bit location in page table entry (default: 0)
>> +#
>> +# @reduced-phys-bits: number of bits in physical addresses that become
>> +#     unavailable when SEV is enabled
>> +#
>> +# Since: 2.12
>
> Not quite sure what we've done in this scenario before.
> It feels wierd to use '2.12' for the new base type, even
> though in effect the properties all existed since 2.12 in
> the sub-class.
>
> Perhaps 'Since: 9.1' for the type, but 'Since: 2.12' for the
> properties, along with an explanatory comment about stuff
> moving into the new base type ?
>
> Markus, opinions ?

The confusion is due to us documenting the schema instead of the
external interface defined by it.  Let me explain.

The external interface is commands and their arguments, ignoring
results, errors and events for brevity's sake.

We use types to define the arguments.  How exactly we use types is not
part of the interface.  This permits refactorings.  However, since the
documentation is attached to the types, refactorings can easily mess it
up.

I'd like to demonstrate this for a simpler command first, then return to
object-add.

Consider nbd-server-add.  It is documented to be since 1.3.

From now on, I'm abbreviating "documented to be since X.Y" to "since
X.Y".

Its arguments are the members of struct NbdServerAddOptions.

NbdServerAddOptions is since 5.0.  Its base BlockExportOptionsNbdBase is
since 5.2.

BlockExportOptionsNbdBase member @name is since 2.12, and @description
is since 5.0.

NbdServerAddOptions member @bitmap is since 4.0.  Members @device and
@writable have no "since" documented, so they inherit it from the
struct, i.e. 5.0.

So, it looks like the command is since 1.3, argument @name since 2.12,
@bitmap since 4.0, @description, @device, and @writable since 5.0.

Wrong!  Arguments @device and @writable have always been there,
i.e. since 1.3.  We ended up with documentation incorrectly claiming 5.0
via several refactorings.

Initially, the command arguments were defined right with the command.
They simply inherited the command's since 1.3.

Commit c62d24e906e (blockdev-nbd: Boxed argument type for
nbd-server-add) moved them to a struct type BlockExportNbd.  The new
struct type was since 5.0.  Newer arguments retained their "since" tags,
but the initial arguments @device and @writable remained without one.
Their documented "since" changed from 1.3 to 5.0.

Aside: the new struct was then used as a branch of union BlockExport,
which was incorrectly documented to be since 4.2.

Messing up "since" when factoring out arguments into a new type was
avoidable: either lie and claim the new type is as old as its members,
or add suitable since tags to its members.

Having a struct with members older than itself looks weird.  Of course,
when a struct came to be is *immaterial*.  How exactly we assemble the
arguments from types is not part of the interface.  We could omit
"since" for the struct, and then require it for all members.  We don't,
because having to think (and then argue!) whether we want a "since" or
not would be a waste of mental capacity.

Here's another refactoring where that may not be possible.  Say you
discover two structs share several members.  You decide to factor them
out into a common base type.  Won't affect the external interface.  But
what if one of these common members has conflicting "since"?  Either we
refrain from the refactoring, or we resort to something like "since
X1.Y1 when used for USER1, since X1.Y2 when used for USER2".  Which
*sucks* as external interface documentation.

Aside: documentation text could clash similarly.  Same code, different
meaning.

I've come to the conclusion that manually recording "since" in the
documentation is dumb.  One, because we mess it up.  Two, because not
messing it up involves either lies or oddities, or too much thought.
Three, because keeping it correct can interfere with refactorings.

Some time ago, Michael Tsirkin suggested to generate "since" information
automatically.  I like the idea.  We'd have to record the external
interface at release time.  To fully replace existing "since" tags, we'd
have to record for old versions, too.  I'd expect this to fix quite a
few doc bugs.

I hope "The confusion is due to us documenting the schema instead of the
external interface defined by it" is now more clear.  The external
interface is derived from the types.  How exactly we construct it from
types is invisible at the interface.  But since we document the
interface by documenting the types, the structure of our interface
documentation mirrors our use of types.  We succeed at shielding the
interface from how we use types, but we fail to shield the
documentation.

Back to your problem at hand.  The external interface is command
object-add.  The command is since 2.0.

It has common arguments and variant arguments depending on the value of
common argument @type.  We specify all that via union ObjectOptions, and
the variant arguments for @type value "sev-guest" via struct
SevGuestProperties.

Union ObjectOptions is since 6.0, but that's immaterial; the type isn't
part of the external interface, only its members are.

Its members are the common arguments.  Since they don't have their own
"since" tag, they inherit it from ObjectOptions, i.e. since 6.0.  That's
simply wrong; they exist since 2.0 just like object-add.

Struct SevGuestProperties is since 2.12, but that's also immaterial.

The members of SevGuestProperties are the variant arguments for @type
value "sev-guest".  Since they don't have their own "since" tag, they
inherit it from SevGuestProperties, i.e. since 2.12.

Your patch moves some of the members to new base type
SevCommonProperties.  As Daniel observed, you can either claim
SevCommonProperties is since 2.12 (which is a lie), or you claim 9.1 for
the type and 2.12 for all its members (which is odd).

I prefer oddities to lies.

I'm not sure we need a comment explaining the oddity.  If you think it's
useful, please make it a non-doc comment.  Reminder:

    ##
    # This is a doc comment.  It goes into generated documentation.
    ##

    # This is is not a doc comment.  It does not go into generated
    # documentation.

Comments?

[...]