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

["Daniel P. Berrangé" <berrange@redhat.com>]

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.

> +        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 :|