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

[Michael Roth]

On Wed, Mar 20, 2024 at 11:47:28AM +0000, Daniel P. Berrangé wrote:
> 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(-)
> > 
> 
> >  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) {
> > -        info->api_major = sev_guest->api_major;
> > -        info->api_minor = sev_guest->api_minor;
> > -        info->build_id = sev_guest->build_id;
> > -        info->policy = sev_guest->policy;
> > -        info->state = sev_guest->state;
> > -        info->handle = sev_guest->handle;
> > +        if (sev_guest) {
> > +            info->handle = sev_guest->handle;
> > +        }
> 
> If we're not going to provide a value for 'handle', then
> we should update the QAPI for this to mark the property
> as optional, which would then require doing
> 
>   info->has_handle = true;
> 
> inside this 'if' block.

I think this is another temporarily-awkward case that gets resolved
with:

  i386/sev: Update query-sev QAPI format to handle SEV-SNP

With that patch 'handle' is always available for SEV guests, and never
available for SNP, and that's managed through a discriminated union
type. I think that info->handle should be treated the same as the
other fields as part of this patch and any changes in how they are
reported should be kept in the above-mentioned patch.

This might be another artifact from v2's handling. Will get this fixed
up.

-Mike

> > +        }

> 
> > +        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);
> >      }
> 
> 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 :|
>