On Wed, Mar 20, 2024 at 12:10:04PM +0000, Daniel P. Berrangé wrote:
> On Wed, Mar 20, 2024 at 03:39:27AM -0500, Michael Roth wrote:
> > Most of the current 'query-sev' command is relevant to both legacy
> > SEV/SEV-ES guests and SEV-SNP guests, with 2 exceptions:
> >
> > - 'policy' is a 64-bit field for SEV-SNP, not 32-bit, and
> > the meaning of the bit positions has changed
> > - 'handle' is not relevant to SEV-SNP
> >
> > To address this, this patch adds a new 'sev-type' field that can be
> > used as a discriminator to select between SEV and SEV-SNP-specific
> > fields/formats without breaking compatibility for existing management
> > tools (so long as management tools that add support for launching
> > SEV-SNP guest update their handling of query-sev appropriately).
> >
> > The corresponding HMP command has also been fixed up similarly.
> >
> > Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> > ---
> > qapi/misc-target.json | 71 ++++++++++++++++++++++++++++++++++---------
> > target/i386/sev.c | 50 ++++++++++++++++++++----------
> > target/i386/sev.h | 3 ++
> > 3 files changed, 94 insertions(+), 30 deletions(-)
> >
> > diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> > index 4e0a6492a9..daceb85d95 100644
> > --- a/qapi/misc-target.json
> > +++ b/qapi/misc-target.json
> > @@ -47,6 +47,49 @@
> > 'send-update', 'receive-update' ],
> > 'if': 'TARGET_I386' }
> >
> > +##
> > +# @SevGuestType:
> > +#
> > +# An enumeration indicating the type of SEV guest being run.
> > +#
> > +# @sev: The guest is a legacy SEV or SEV-ES guest.
> > +# @sev-snp: The guest is an SEV-SNP guest.
> > +#
> > +# Since: 6.2
>
> Now 9.1 at the earliest.
>
> > +##
> > +{ 'enum': 'SevGuestType',
> > + 'data': [ 'sev', 'sev-snp' ],
> > + 'if': 'TARGET_I386' }
> > +
> > +##
> > +# @SevGuestInfo:
> > +#
> > +# Information specific to legacy SEV/SEV-ES guests.
> > +#
> > +# @policy: SEV policy value
> > +#
> > +# @handle: SEV firmware handle
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'struct': 'SevGuestInfo',
> > + 'data': { 'policy': 'uint32',
> > + 'handle': 'uint32' },
> > + 'if': 'TARGET_I386' }
> > +
> > +##
> > +# @SevSnpGuestInfo:
> > +#
> > +# Information specific to SEV-SNP guests.
> > +#
> > +# @snp-policy: SEV-SNP policy value
> > +#
> > +# Since: 6.2
> > +##
> > +{ 'struct': 'SevSnpGuestInfo',
> > + 'data': { 'snp-policy': 'uint64' },
> > + 'if': 'TARGET_I386' }
>
> IMHO it can just be called 'policy' still, since
> it is implicitly within a 'Snp' specific type.
>
>
> > +
> > ##
> > # @SevInfo:
> > #
> > @@ -60,25 +103,25 @@
> > #
> > # @build-id: SEV FW build id
> > #
> > -# @policy: SEV policy value
> > -#
> > # @state: SEV guest state
> > #
> > -# @handle: SEV firmware handle
> > +# @sev-type: Type of SEV guest being run
> > #
> > # Since: 2.12
> > ##
> > -{ 'struct': 'SevInfo',
> > - 'data': { 'enabled': 'bool',
> > - 'api-major': 'uint8',
> > - 'api-minor' : 'uint8',
> > - 'build-id' : 'uint8',
> > - 'policy' : 'uint32',
> > - 'state' : 'SevState',
> > - 'handle' : 'uint32'
> > - },
> > - 'if': 'TARGET_I386'
> > -}
> > +{ 'union': 'SevInfo',
> > + 'base': { 'enabled': 'bool',
> > + 'api-major': 'uint8',
> > + 'api-minor' : 'uint8',
> > + 'build-id' : 'uint8',
> > + 'state' : 'SevState',
> > + 'sev-type' : 'SevGuestType' },
> > + 'discriminator': 'sev-type',
> > + 'data': {
> > + 'sev': 'SevGuestInfo',
> > + 'sev-snp': 'SevSnpGuestInfo' },
> > + 'if': 'TARGET_I386' }
> > +
> >
> > ##
> > # @query-sev:
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 43e6c0172f..b03d70a3d1 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -353,25 +353,27 @@ static SevInfo *sev_get_info(void)
> > {
> > SevInfo *info;
> > SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs);
> > - SevGuestState *sev_guest =
> > - (SevGuestState *)object_dynamic_cast(OBJECT(sev_common),
> > - TYPE_SEV_GUEST);
> >
> > info = g_new0(SevInfo, 1);
> > info->enabled = sev_enabled();
> >
> > if (info->enabled) {
> > - if (sev_guest) {
> > - info->handle = sev_guest->handle;
> > - }
> > info->api_major = sev_common->api_major;
> > info->api_minor = sev_common->api_minor;
> > info->build_id = sev_common->build_id;
> > info->state = sev_common->state;
> > - /* we only report the lower 32-bits of policy for SNP, ok for now... */
> > - info->policy =
> > - (uint32_t)object_property_get_uint(OBJECT(sev_common),
> > - "policy", NULL);
> > +
> > + if (sev_snp_enabled()) {
> > + info->sev_type = SEV_GUEST_TYPE_SEV_SNP;
> > + info->u.sev_snp.snp_policy =
> > + object_property_get_uint(OBJECT(sev_common), "policy", NULL);
> > + } else {
> > + info->sev_type = SEV_GUEST_TYPE_SEV;
> > + info->u.sev.handle = SEV_GUEST(sev_common)->handle;
> > + info->u.sev.policy =
> > + (uint32_t)object_property_get_uint(OBJECT(sev_common),
> > + "policy", NULL);
> > + }
> > }
>
> Ok, this is fixing the issues I reported earlier.
>
> For 'policy' I do wonder if we really need to push it into the
> type specific part of the union, as oppposed to having a common
> 'policy' field that is uint64 in size.
>
> As mentioned earlier, on the wire there's no distinction between
> int32/int64s, so there's no compat issues with changing it from
> int32 to int64.
Mentioned this in response to your earlier reply, but the renaming of
'policy' to 'snp_policy' was in response to Dave's query about old
management tools who don't understand/parse the new sev-type field
and blinding misinterpret an SNP 'policy' as an SEV one:
https://lore.kernel.org/kvm/YTdSlg5NymDQex5T@work-vm/T/#mac6758af9bfc41ee49ff3ef5c3ec3779ec275ff9
I think the thinking is that it's better that they see nothing at all
(likely treating as policy 0x0) versus getting unexpected values and
reporting random policy features.
>
> >
> > return info;
> > @@ -394,20 +396,36 @@ void hmp_info_sev(Monitor *mon, const QDict *qdict)
> > {
> > SevInfo *info = sev_get_info();
> >
> > - if (info && info->enabled) {
> > - monitor_printf(mon, "handle: %d\n", info->handle);
> > + if (!info || !info->enabled) {
> > + monitor_printf(mon, "SEV is not enabled\n");
> > + goto out;
> > + }
> > +
> > + if (sev_snp_enabled()) {
> > monitor_printf(mon, "state: %s\n", SevState_str(info->state));
> > monitor_printf(mon, "build: %d\n", info->build_id);
> > monitor_printf(mon, "api version: %d.%d\n",
> > info->api_major, info->api_minor);
> > monitor_printf(mon, "debug: %s\n",
> > - info->policy & SEV_POLICY_NODBG ? "off" : "on");
> > - monitor_printf(mon, "key-sharing: %s\n",
> > - info->policy & SEV_POLICY_NOKS ? "off" : "on");
> > + info->u.sev_snp.snp_policy & SEV_SNP_POLICY_DBG ? "on"
> > + : "off");
> > + monitor_printf(mon, "SMT allowed: %s\n",
> > + info->u.sev_snp.snp_policy & SEV_SNP_POLICY_SMT ? "on"
> > + : "off");
> > } else {
> > - monitor_printf(mon, "SEV is not enabled\n");
> > + monitor_printf(mon, "handle: %d\n", info->u.sev.handle);
> > + monitor_printf(mon, "state: %s\n", SevState_str(info->state));
> > + monitor_printf(mon, "build: %d\n", info->build_id);
> > + monitor_printf(mon, "api version: %d.%d\n",
> > + info->api_major, info->api_minor);
>
> This set of three fields is identical in both branches, so the duplication
> in printing it should be eliminated.
Makes sense.
>
> > + monitor_printf(mon, "debug: %s\n",
> > + info->u.sev.policy & SEV_POLICY_NODBG ? "off" : "on");
> > + monitor_printf(mon, "key-sharing: %s\n",
> > + info->u.sev.policy & SEV_POLICY_NOKS ? "off" : "on");
> > }
> > + monitor_printf(mon, "SEV type: %s\n", SevGuestType_str(info->sev_type));
>
> I'd say 'SEV type' should be printed before everything else, since
> that value is the discriminator for interpreting the other data that
> is printed.
Will do.
Thanks,
Mike
>
> >
> > +out:
> > qapi_free_SevInfo(info);
> > }
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
