Re: [PATCH v4 13/66] target/i386: Introduce kvm_confidential_guest_init()

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

On Wed, Jan 24, 2024 at 10:22:35PM -0500, Xiaoyao Li wrote:
> Introduce a separate function kvm_confidential_guest_init(), which
> dispatches specific confidential guest initialization function by
> ms->cgs type.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  target/i386/kvm/kvm.c | 11 ++++++++++-
>  target/i386/sev.c     |  1 -
>  target/i386/sev.h     |  2 ++
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index c961846777cc..f9a774925cf6 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2541,6 +2541,15 @@ int kvm_arch_get_default_type(MachineState *ms)
>      return 0;
>  }
>  
> +static int kvm_confidential_guest_init(MachineState *ms, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(ms->cgs), TYPE_SEV_GUEST)) {
> +        return sev_kvm_init(ms->cgs, errp);
> +    }
> +
> +    return 0;
> +}

if/else ladders checking object subclass type and then invoking a
subclass specific method are quite an object oriented code
anti-pattern. I think this suggests that ConfidentialGuestSupportClass
should gain a member

   void (*kvm_init)(ConfidentialGuestSpport *cgs, Error **errp);

and then an impl

   void confidential_guest_kvm_init(ConfidentialGuestSupport *cgs, Error *errp) {
       ConfidentialGuestSupportClass *klass = CONFIDENTIAL_GUEST_SUPPORT_GET_CLASS(cgs);
       klass->kvm_init(cgs, errp)
   }

that way, kvm.c arch code doesn't get directly bound to the APIs
for the different CVM technologies, the ConfidentialGuestSupport
object will act as a proper isolation layer. 

This is likely to apply in other parts of KVM code that need to
call into SEV/TDX specific functions too.

> +
>  int kvm_arch_init(MachineState *ms, KVMState *s)
>  {
>      uint64_t identity_base = 0xfffbc000;
> @@ -2561,7 +2570,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>       * mechanisms are supported in future (e.g. TDX), they'll need
>       * their own initialization either here or elsewhere.
>       */
> -    ret = sev_kvm_init(ms->cgs, &local_err);
> +    ret = kvm_confidential_guest_init(ms, &local_err);
>      if (ret < 0) {
>          error_report_err(local_err);
>          return ret;
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 173de91afe7d..27d58702d6dc 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -39,7 +39,6 @@
>  #include "hw/i386/pc.h"
>  #include "exec/address-spaces.h"
>  
> -#define TYPE_SEV_GUEST "sev-guest"
>  OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
>  
>  
> diff --git a/target/i386/sev.h b/target/i386/sev.h
> index e7499c95b1e8..1fe25d096dc4 100644
> --- a/target/i386/sev.h
> +++ b/target/i386/sev.h
> @@ -20,6 +20,8 @@
>  
>  #include "exec/confidential-guest-support.h"
>  
> +#define TYPE_SEV_GUEST "sev-guest"
> +
>  #define SEV_POLICY_NODBG        0x1
>  #define SEV_POLICY_NOKS         0x2
>  #define SEV_POLICY_ES           0x4
> -- 
> 2.34.1
> 

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