[PATCH 0/4] Confidential Guest Support: Introduce kvm_init() and kvm_reset() virtual functions

[PATCH 0/4] Confidential Guest Support: Introduce kvm_init() and kvm_reset() virtual functions

[Xiaoyao Li]

This series was inspired and suggested by Daniel:
https://lore.kernel.org/qemu-devel/ZbfoQsEuv6_zwl3b@redhat.com/

I remove the rfc tag in this version, to get more attention.

Currently, different confidential VMs in different architectures have
their own specific *_kvm_init() (and some have *_kvm_reset()) exposed
for KVM stuff when it's a confidential VM. e.g., sev_kmv_init() for x86
SEV, pef_kvm_init() and pef_kvm_reset() for PPC PEF, and s390_pv_init()
for s390 PV VMs.

Introduce a generic .kvm_init() and .kvm_reset() functions in
ConfidentialGuestSupportClass, so that different cgs technologies in
different architectures can implement their own, while common interface
of cgs can be used.

This series implements two helper functions confidential_guest_kvm_init()
and confidential_guest_kvm_reset() in Patch 1. In the following patches,
they are used in each arch to replace specific exposed functions.
X86 will benefit more from the new generic interface  when TDX support is
added later.

There is one step forward possible, that calling
confidential_guest_kvm_init() before kvm_arch_init() in kvm_int() in
accel/kvm/kvm-all.c. This way, each arch doesn't need to call their
specific code in arch-specific code.
	
X86 fits it, however I'm not sure if ppc and s390 fit it as well.
Because currently, both ppc and s390 calls it in MachineClass->init().
I'm not sure if there is any order dependency and need input from
corresponding arch folks.


===
changes from RFC v1:
- remove RFC tag;
- move the !NULL check of ms->cgs into callers;
- delete whole sev-stub.c;

RFC v1:
https://lore.kernel.org/qemu-devel/20240206082852.3333299-1-xiaoyao.li@intel.com/

Xiaoyao Li (4):
  confidential guest support: Add kvm_init() and kvm_reset() in class
  i386/sev: Switch to use confidential_guest_kvm_init()
  ppc/pef: switch to use confidential_guest_kvm_init/reset()
  s390: Switch to use confidential_guest_kvm_init()

 hw/ppc/pef.c                              |   9 +-
 hw/ppc/spapr.c                            |  10 +-
 hw/s390x/s390-virtio-ccw.c                |   5 +-
 include/exec/confidential-guest-support.h |  34 +++++-
 include/hw/ppc/pef.h                      |  17 ---
 target/i386/kvm/kvm.c                     |  10 +-
 target/i386/kvm/meson.build               |   2 -
 target/i386/kvm/sev-stub.c                |  21 ----
 target/i386/sev.c                         | 120 +++++++++++-----------
 target/i386/sev.h                         |   2 -
 target/s390x/kvm/pv.c                     |   8 ++
 target/s390x/kvm/pv.h                     |  14 ---
 12 files changed, 126 insertions(+), 126 deletions(-)
 delete mode 100644 include/hw/ppc/pef.h
 delete mode 100644 target/i386/kvm/sev-stub.c

-- 
2.34.1

[PATCH 1/4] confidential guest support: Add kvm_init() and kvm_reset() in class

[Xiaoyao Li]

Different confidential VMs in different architectures all have the same
needs to do their specific initialization (and maybe resetting) stuffs
with KVM. Currently each of them exposes individual *_kvm_init()
functions and let machine code or kvm code to call it.

To make it more object oriented, add two virtual functions, kvm_init()
and kvm_reset() in ConfidentialGuestSupportClass, and expose two helpers
functions for invodking them.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Changes since rfc v1:
- Drop the NULL check and rely the check from the caller;
---
 include/exec/confidential-guest-support.h | 34 ++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
index ba2dd4b5dfc4..e5b188cffbfd 100644
--- a/include/exec/confidential-guest-support.h
+++ b/include/exec/confidential-guest-support.h
@@ -23,7 +23,10 @@
 #include "qom/object.h"
 
 #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
-OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, CONFIDENTIAL_GUEST_SUPPORT)
+OBJECT_DECLARE_TYPE(ConfidentialGuestSupport,
+                    ConfidentialGuestSupportClass,
+                    CONFIDENTIAL_GUEST_SUPPORT)
+
 
 struct ConfidentialGuestSupport {
     Object parent;
@@ -55,8 +58,37 @@ struct ConfidentialGuestSupport {
 
 typedef struct ConfidentialGuestSupportClass {
     ObjectClass parent;
+
+    int (*kvm_init)(ConfidentialGuestSupport *cgs, Error **errp);
+    int (*kvm_reset)(ConfidentialGuestSupport *cgs, Error **errp);
 } ConfidentialGuestSupportClass;
 
+static inline int confidential_guest_kvm_init(ConfidentialGuestSupport *cgs,
+                                              Error **errp)
+{
+    ConfidentialGuestSupportClass *klass;
+
+    klass = CONFIDENTIAL_GUEST_SUPPORT_GET_CLASS(cgs);
+    if (klass->kvm_init) {
+        return klass->kvm_init(cgs, errp);
+    }
+
+    return 0;
+}
+
+static inline int confidential_guest_kvm_reset(ConfidentialGuestSupport *cgs,
+                                               Error **errp)
+{
+    ConfidentialGuestSupportClass *klass;
+
+    klass = CONFIDENTIAL_GUEST_SUPPORT_GET_CLASS(cgs);
+    if (klass->kvm_reset) {
+        return klass->kvm_reset(cgs, errp);
+    }
+
+    return 0;
+}
+
 #endif /* !CONFIG_USER_ONLY */
 
 #endif /* QEMU_CONFIDENTIAL_GUEST_SUPPORT_H */
-- 
2.34.1

[PATCH 2/4] i386/sev: Switch to use confidential_guest_kvm_init()

[Xiaoyao Li]

Use confidential_guest_kvm_init() instead of calling SEV specific
sev_kvm_init(). As a bouns, it fits to future TDX when TDX implements
its own confidential_guest_support and .kvm_init().

Move the "TypeInfo sev_guest_info" definition and related functions to
the end of the file, to avoid declaring the sev_kvm_init() ahead.

Delete the sve-stub.c since it's not needed anymore.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Changes from rfc v1:
- check ms->cgs not NULL before calling confidential_guest_kvm_init();
- delete the sev-stub.c;
---
 target/i386/kvm/kvm.c       |  10 +--
 target/i386/kvm/meson.build |   2 -
 target/i386/kvm/sev-stub.c  |  21 -------
 target/i386/sev.c           | 120 +++++++++++++++++++-----------------
 target/i386/sev.h           |   2 -
 5 files changed, 68 insertions(+), 87 deletions(-)
 delete mode 100644 target/i386/kvm/sev-stub.c

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 42970ab046fa..ca4e1fb72dd9 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2531,10 +2531,12 @@ 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);
-    if (ret < 0) {
-        error_report_err(local_err);
-        return ret;
+    if (ms->cgs) {
+        ret = confidential_guest_kvm_init(ms->cgs, &local_err);
+        if (ret < 0) {
+            error_report_err(local_err);
+            return ret;
+        }
     }
 
     has_xcrs = kvm_check_extension(s, KVM_CAP_XCRS);
diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
index 84d9143e6029..e7850981e62d 100644
--- a/target/i386/kvm/meson.build
+++ b/target/i386/kvm/meson.build
@@ -7,8 +7,6 @@ i386_kvm_ss.add(files(
 
 i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files('xen-emu.c'))
 
-i386_kvm_ss.add(when: 'CONFIG_SEV', if_false: files('sev-stub.c'))
-
 i386_system_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), if_false: files('hyperv-stub.c'))
 
 i386_system_ss.add_all(when: 'CONFIG_KVM', if_true: i386_kvm_ss)
diff --git a/target/i386/kvm/sev-stub.c b/target/i386/kvm/sev-stub.c
deleted file mode 100644
index 1be5341e8a6a..000000000000
--- a/target/i386/kvm/sev-stub.c
+++ /dev/null
@@ -1,21 +0,0 @@
-/*
- * QEMU SEV stub
- *
- * Copyright Advanced Micro Devices 2018
- *
- * Authors:
- *      Brijesh Singh <brijesh.singh@amd.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-
-#include "qemu/osdep.h"
-#include "sev.h"
-
-int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
-{
-    /* If we get here, cgs must be some non-SEV thing */
-    return 0;
-}
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 173de91afe7d..19e79d3631d0 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -353,63 +353,6 @@ static void sev_guest_set_kernel_hashes(Object *obj, bool value, Error **errp)
     sev->kernel_hashes = value;
 }
 
-static void
-sev_guest_class_init(ObjectClass *oc, void *data)
-{
-    object_class_property_add_str(oc, "sev-device",
-                                  sev_guest_get_sev_device,
-                                  sev_guest_set_sev_device);
-    object_class_property_set_description(oc, "sev-device",
-            "SEV device to use");
-    object_class_property_add_str(oc, "dh-cert-file",
-                                  sev_guest_get_dh_cert_file,
-                                  sev_guest_set_dh_cert_file);
-    object_class_property_set_description(oc, "dh-cert-file",
-            "guest owners DH certificate (encoded with base64)");
-    object_class_property_add_str(oc, "session-file",
-                                  sev_guest_get_session_file,
-                                  sev_guest_set_session_file);
-    object_class_property_set_description(oc, "session-file",
-            "guest owners session parameters (encoded with base64)");
-    object_class_property_add_bool(oc, "kernel-hashes",
-                                   sev_guest_get_kernel_hashes,
-                                   sev_guest_set_kernel_hashes);
-    object_class_property_set_description(oc, "kernel-hashes",
-            "add kernel hashes to guest firmware for measured Linux boot");
-}
-
-static void
-sev_guest_instance_init(Object *obj)
-{
-    SevGuestState *sev = SEV_GUEST(obj);
-
-    sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
-    sev->policy = DEFAULT_GUEST_POLICY;
-    object_property_add_uint32_ptr(obj, "policy", &sev->policy,
-                                   OBJ_PROP_FLAG_READWRITE);
-    object_property_add_uint32_ptr(obj, "handle", &sev->handle,
-                                   OBJ_PROP_FLAG_READWRITE);
-    object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos,
-                                   OBJ_PROP_FLAG_READWRITE);
-    object_property_add_uint32_ptr(obj, "reduced-phys-bits",
-                                   &sev->reduced_phys_bits,
-                                   OBJ_PROP_FLAG_READWRITE);
-}
-
-/* sev guest info */
-static const TypeInfo sev_guest_info = {
-    .parent = TYPE_CONFIDENTIAL_GUEST_SUPPORT,
-    .name = TYPE_SEV_GUEST,
-    .instance_size = sizeof(SevGuestState),
-    .instance_finalize = sev_guest_finalize,
-    .class_init = sev_guest_class_init,
-    .instance_init = sev_guest_instance_init,
-    .interfaces = (InterfaceInfo[]) {
-        { TYPE_USER_CREATABLE },
-        { }
-    }
-};
-
 bool
 sev_enabled(void)
 {
@@ -906,7 +849,7 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
     }
 }
 
-int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
+static int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
 {
     SevGuestState *sev
         = (SevGuestState *)object_dynamic_cast(OBJECT(cgs), TYPE_SEV_GUEST);
@@ -1383,6 +1326,67 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     return ret;
 }
 
+static void
+sev_guest_class_init(ObjectClass *oc, void *data)
+{
+    ConfidentialGuestSupportClass *klass = CONFIDENTIAL_GUEST_SUPPORT_CLASS(oc);
+
+    klass->kvm_init = sev_kvm_init;
+
+    object_class_property_add_str(oc, "sev-device",
+                                  sev_guest_get_sev_device,
+                                  sev_guest_set_sev_device);
+    object_class_property_set_description(oc, "sev-device",
+            "SEV device to use");
+    object_class_property_add_str(oc, "dh-cert-file",
+                                  sev_guest_get_dh_cert_file,
+                                  sev_guest_set_dh_cert_file);
+    object_class_property_set_description(oc, "dh-cert-file",
+            "guest owners DH certificate (encoded with base64)");
+    object_class_property_add_str(oc, "session-file",
+                                  sev_guest_get_session_file,
+                                  sev_guest_set_session_file);
+    object_class_property_set_description(oc, "session-file",
+            "guest owners session parameters (encoded with base64)");
+    object_class_property_add_bool(oc, "kernel-hashes",
+                                   sev_guest_get_kernel_hashes,
+                                   sev_guest_set_kernel_hashes);
+    object_class_property_set_description(oc, "kernel-hashes",
+            "add kernel hashes to guest firmware for measured Linux boot");
+}
+
+static void
+sev_guest_instance_init(Object *obj)
+{
+    SevGuestState *sev = SEV_GUEST(obj);
+
+    sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
+    sev->policy = DEFAULT_GUEST_POLICY;
+    object_property_add_uint32_ptr(obj, "policy", &sev->policy,
+                                   OBJ_PROP_FLAG_READWRITE);
+    object_property_add_uint32_ptr(obj, "handle", &sev->handle,
+                                   OBJ_PROP_FLAG_READWRITE);
+    object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos,
+                                   OBJ_PROP_FLAG_READWRITE);
+    object_property_add_uint32_ptr(obj, "reduced-phys-bits",
+                                   &sev->reduced_phys_bits,
+                                   OBJ_PROP_FLAG_READWRITE);
+}
+
+/* sev guest info */
+static const TypeInfo sev_guest_info = {
+    .parent = TYPE_CONFIDENTIAL_GUEST_SUPPORT,
+    .name = TYPE_SEV_GUEST,
+    .instance_size = sizeof(SevGuestState),
+    .instance_finalize = sev_guest_finalize,
+    .class_init = sev_guest_class_init,
+    .instance_init = sev_guest_instance_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
 static void
 sev_register_types(void)
 {
diff --git a/target/i386/sev.h b/target/i386/sev.h
index e7499c95b1e8..9e10d09539a7 100644
--- a/target/i386/sev.h
+++ b/target/i386/sev.h
@@ -57,6 +57,4 @@ int sev_inject_launch_secret(const char *hdr, const char *secret,
 int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
 void sev_es_set_reset_vector(CPUState *cpu);
 
-int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
-
 #endif
-- 
2.34.1

Re: [PATCH 2/4] i386/sev: Switch to use confidential_guest_kvm_init()

[Paolo Bonzini]

On Thu, Feb 29, 2024 at 7:01 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>
> Use confidential_guest_kvm_init() instead of calling SEV specific
> sev_kvm_init(). As a bouns, it fits to future TDX when TDX implements
> its own confidential_guest_support and .kvm_init().
>
> Move the "TypeInfo sev_guest_info" definition and related functions to
> the end of the file, to avoid declaring the sev_kvm_init() ahead.
>
> Delete the sve-stub.c since it's not needed anymore.
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> Changes from rfc v1:
> - check ms->cgs not NULL before calling confidential_guest_kvm_init();
> - delete the sev-stub.c;

Queued, with just one small simplification that can be done on top:

diff --git a/target/i386/sev.c b/target/i386/sev.c
index e89d64fa52..b8f79d34d1 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -851,18 +851,13 @@ sev_vm_state_change(void *opaque, bool running,
RunState state)

 static int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
 {
-    SevGuestState *sev
-        = (SevGuestState *)object_dynamic_cast(OBJECT(cgs), TYPE_SEV_GUEST);
+    SevGuestState *sev = SEV_GUEST(cgs);
     char *devname;
     int ret, fw_error, cmd;
     uint32_t ebx;
     uint32_t host_cbitpos;
     struct sev_user_data_status status = {};

-    if (!sev) {
-        return 0;
-    }
-
     ret = ram_block_discard_disable(true);
     if (ret) {
         error_report("%s: cannot disable RAM discard", __func__);

Thanks!

Paolo

> ---
>  target/i386/kvm/kvm.c       |  10 +--
>  target/i386/kvm/meson.build |   2 -
>  target/i386/kvm/sev-stub.c  |  21 -------
>  target/i386/sev.c           | 120 +++++++++++++++++++-----------------
>  target/i386/sev.h           |   2 -
>  5 files changed, 68 insertions(+), 87 deletions(-)
>  delete mode 100644 target/i386/kvm/sev-stub.c
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 42970ab046fa..ca4e1fb72dd9 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2531,10 +2531,12 @@ 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);
> -    if (ret < 0) {
> -        error_report_err(local_err);
> -        return ret;
> +    if (ms->cgs) {
> +        ret = confidential_guest_kvm_init(ms->cgs, &local_err);
> +        if (ret < 0) {
> +            error_report_err(local_err);
> +            return ret;
> +        }
>      }
>
>      has_xcrs = kvm_check_extension(s, KVM_CAP_XCRS);
> diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
> index 84d9143e6029..e7850981e62d 100644
> --- a/target/i386/kvm/meson.build
> +++ b/target/i386/kvm/meson.build
> @@ -7,8 +7,6 @@ i386_kvm_ss.add(files(
>
>  i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files('xen-emu.c'))
>
> -i386_kvm_ss.add(when: 'CONFIG_SEV', if_false: files('sev-stub.c'))
> -
>  i386_system_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), if_false: files('hyperv-stub.c'))
>
>  i386_system_ss.add_all(when: 'CONFIG_KVM', if_true: i386_kvm_ss)
> diff --git a/target/i386/kvm/sev-stub.c b/target/i386/kvm/sev-stub.c
> deleted file mode 100644
> index 1be5341e8a6a..000000000000
> --- a/target/i386/kvm/sev-stub.c
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -/*
> - * QEMU SEV stub
> - *
> - * Copyright Advanced Micro Devices 2018
> - *
> - * Authors:
> - *      Brijesh Singh <brijesh.singh@amd.com>
> - *
> - * This work is licensed under the terms of the GNU GPL, version 2 or later.
> - * See the COPYING file in the top-level directory.
> - *
> - */
> -
> -#include "qemu/osdep.h"
> -#include "sev.h"
> -
> -int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> -{
> -    /* If we get here, cgs must be some non-SEV thing */
> -    return 0;
> -}
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 173de91afe7d..19e79d3631d0 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -353,63 +353,6 @@ static void sev_guest_set_kernel_hashes(Object *obj, bool value, Error **errp)
>      sev->kernel_hashes = value;
>  }
>
> -static void
> -sev_guest_class_init(ObjectClass *oc, void *data)
> -{
> -    object_class_property_add_str(oc, "sev-device",
> -                                  sev_guest_get_sev_device,
> -                                  sev_guest_set_sev_device);
> -    object_class_property_set_description(oc, "sev-device",
> -            "SEV device to use");
> -    object_class_property_add_str(oc, "dh-cert-file",
> -                                  sev_guest_get_dh_cert_file,
> -                                  sev_guest_set_dh_cert_file);
> -    object_class_property_set_description(oc, "dh-cert-file",
> -            "guest owners DH certificate (encoded with base64)");
> -    object_class_property_add_str(oc, "session-file",
> -                                  sev_guest_get_session_file,
> -                                  sev_guest_set_session_file);
> -    object_class_property_set_description(oc, "session-file",
> -            "guest owners session parameters (encoded with base64)");
> -    object_class_property_add_bool(oc, "kernel-hashes",
> -                                   sev_guest_get_kernel_hashes,
> -                                   sev_guest_set_kernel_hashes);
> -    object_class_property_set_description(oc, "kernel-hashes",
> -            "add kernel hashes to guest firmware for measured Linux boot");
> -}
> -
> -static void
> -sev_guest_instance_init(Object *obj)
> -{
> -    SevGuestState *sev = SEV_GUEST(obj);
> -
> -    sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
> -    sev->policy = DEFAULT_GUEST_POLICY;
> -    object_property_add_uint32_ptr(obj, "policy", &sev->policy,
> -                                   OBJ_PROP_FLAG_READWRITE);
> -    object_property_add_uint32_ptr(obj, "handle", &sev->handle,
> -                                   OBJ_PROP_FLAG_READWRITE);
> -    object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos,
> -                                   OBJ_PROP_FLAG_READWRITE);
> -    object_property_add_uint32_ptr(obj, "reduced-phys-bits",
> -                                   &sev->reduced_phys_bits,
> -                                   OBJ_PROP_FLAG_READWRITE);
> -}
> -
> -/* sev guest info */
> -static const TypeInfo sev_guest_info = {
> -    .parent = TYPE_CONFIDENTIAL_GUEST_SUPPORT,
> -    .name = TYPE_SEV_GUEST,
> -    .instance_size = sizeof(SevGuestState),
> -    .instance_finalize = sev_guest_finalize,
> -    .class_init = sev_guest_class_init,
> -    .instance_init = sev_guest_instance_init,
> -    .interfaces = (InterfaceInfo[]) {
> -        { TYPE_USER_CREATABLE },
> -        { }
> -    }
> -};
> -
>  bool
>  sev_enabled(void)
>  {
> @@ -906,7 +849,7 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
>      }
>  }
>
> -int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> +static int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>  {
>      SevGuestState *sev
>          = (SevGuestState *)object_dynamic_cast(OBJECT(cgs), TYPE_SEV_GUEST);
> @@ -1383,6 +1326,67 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
>      return ret;
>  }
>
> +static void
> +sev_guest_class_init(ObjectClass *oc, void *data)
> +{
> +    ConfidentialGuestSupportClass *klass = CONFIDENTIAL_GUEST_SUPPORT_CLASS(oc);
> +
> +    klass->kvm_init = sev_kvm_init;
> +
> +    object_class_property_add_str(oc, "sev-device",
> +                                  sev_guest_get_sev_device,
> +                                  sev_guest_set_sev_device);
> +    object_class_property_set_description(oc, "sev-device",
> +            "SEV device to use");
> +    object_class_property_add_str(oc, "dh-cert-file",
> +                                  sev_guest_get_dh_cert_file,
> +                                  sev_guest_set_dh_cert_file);
> +    object_class_property_set_description(oc, "dh-cert-file",
> +            "guest owners DH certificate (encoded with base64)");
> +    object_class_property_add_str(oc, "session-file",
> +                                  sev_guest_get_session_file,
> +                                  sev_guest_set_session_file);
> +    object_class_property_set_description(oc, "session-file",
> +            "guest owners session parameters (encoded with base64)");
> +    object_class_property_add_bool(oc, "kernel-hashes",
> +                                   sev_guest_get_kernel_hashes,
> +                                   sev_guest_set_kernel_hashes);
> +    object_class_property_set_description(oc, "kernel-hashes",
> +            "add kernel hashes to guest firmware for measured Linux boot");
> +}
> +
> +static void
> +sev_guest_instance_init(Object *obj)
> +{
> +    SevGuestState *sev = SEV_GUEST(obj);
> +
> +    sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
> +    sev->policy = DEFAULT_GUEST_POLICY;
> +    object_property_add_uint32_ptr(obj, "policy", &sev->policy,
> +                                   OBJ_PROP_FLAG_READWRITE);
> +    object_property_add_uint32_ptr(obj, "handle", &sev->handle,
> +                                   OBJ_PROP_FLAG_READWRITE);
> +    object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos,
> +                                   OBJ_PROP_FLAG_READWRITE);
> +    object_property_add_uint32_ptr(obj, "reduced-phys-bits",
> +                                   &sev->reduced_phys_bits,
> +                                   OBJ_PROP_FLAG_READWRITE);
> +}
> +
> +/* sev guest info */
> +static const TypeInfo sev_guest_info = {
> +    .parent = TYPE_CONFIDENTIAL_GUEST_SUPPORT,
> +    .name = TYPE_SEV_GUEST,
> +    .instance_size = sizeof(SevGuestState),
> +    .instance_finalize = sev_guest_finalize,
> +    .class_init = sev_guest_class_init,
> +    .instance_init = sev_guest_instance_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_USER_CREATABLE },
> +        { }
> +    }
> +};
> +
>  static void
>  sev_register_types(void)
>  {
> diff --git a/target/i386/sev.h b/target/i386/sev.h
> index e7499c95b1e8..9e10d09539a7 100644
> --- a/target/i386/sev.h
> +++ b/target/i386/sev.h
> @@ -57,6 +57,4 @@ int sev_inject_launch_secret(const char *hdr, const char *secret,
>  int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
>  void sev_es_set_reset_vector(CPUState *cpu);
>
> -int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
> -
>  #endif
> --
> 2.34.1
>

Re: [PATCH 2/4] i386/sev: Switch to use confidential_guest_kvm_init()

[Xiaoyao Li]

On 3/19/2024 5:51 AM, Paolo Bonzini wrote:
> On Thu, Feb 29, 2024 at 7:01 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>>
>> Use confidential_guest_kvm_init() instead of calling SEV specific
>> sev_kvm_init(). As a bouns, it fits to future TDX when TDX implements
>> its own confidential_guest_support and .kvm_init().
>>
>> Move the "TypeInfo sev_guest_info" definition and related functions to
>> the end of the file, to avoid declaring the sev_kvm_init() ahead.
>>
>> Delete the sve-stub.c since it's not needed anymore.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> Changes from rfc v1:
>> - check ms->cgs not NULL before calling confidential_guest_kvm_init();
>> - delete the sev-stub.c;
> 
> Queued, with just one small simplification that can be done on top:

thank you, Paolo!

> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index e89d64fa52..b8f79d34d1 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -851,18 +851,13 @@ sev_vm_state_change(void *opaque, bool running,
> RunState state)
> 
>   static int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>   {
> -    SevGuestState *sev
> -        = (SevGuestState *)object_dynamic_cast(OBJECT(cgs), TYPE_SEV_GUEST);
> +    SevGuestState *sev = SEV_GUEST(cgs);
>       char *devname;
>       int ret, fw_error, cmd;
>       uint32_t ebx;
>       uint32_t host_cbitpos;
>       struct sev_user_data_status status = {};
> 
> -    if (!sev) {
> -        return 0;
> -    }
> -
>       ret = ram_block_discard_disable(true);
>       if (ret) {
>           error_report("%s: cannot disable RAM discard", __func__);

It looks good.

> Thanks!
> 
> Paolo

[PATCH 3/4] ppc/pef: switch to use confidential_guest_kvm_init/reset()

[Xiaoyao Li]

Use the unified interface to call confidential guest related kvm_init()
and kvm_reset(), to avoid exposing pef specific functions.

remove perf.h since it is now blank..

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Changes from rfc v1:
 - check machine->cgs not NULL before callling
   confidential_guest_kvm_init/reset();
---
 hw/ppc/pef.c         |  9 ++++++---
 hw/ppc/spapr.c       | 10 +++++++---
 include/hw/ppc/pef.h | 17 -----------------
 3 files changed, 13 insertions(+), 23 deletions(-)
 delete mode 100644 include/hw/ppc/pef.h

diff --git a/hw/ppc/pef.c b/hw/ppc/pef.c
index d28ed3ba7333..47553348b1e7 100644
--- a/hw/ppc/pef.c
+++ b/hw/ppc/pef.c
@@ -15,7 +15,6 @@
 #include "sysemu/kvm.h"
 #include "migration/blocker.h"
 #include "exec/confidential-guest-support.h"
-#include "hw/ppc/pef.h"
 
 #define TYPE_PEF_GUEST "pef-guest"
 OBJECT_DECLARE_SIMPLE_TYPE(PefGuest, PEF_GUEST)
@@ -93,7 +92,7 @@ static int kvmppc_svm_off(Error **errp)
 #endif
 }
 
-int pef_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
+static int pef_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
 {
     if (!object_dynamic_cast(OBJECT(cgs), TYPE_PEF_GUEST)) {
         return 0;
@@ -107,7 +106,7 @@ int pef_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     return kvmppc_svm_init(cgs, errp);
 }
 
-int pef_kvm_reset(ConfidentialGuestSupport *cgs, Error **errp)
+static int pef_kvm_reset(ConfidentialGuestSupport *cgs, Error **errp)
 {
     if (!object_dynamic_cast(OBJECT(cgs), TYPE_PEF_GUEST)) {
         return 0;
@@ -131,6 +130,10 @@ OBJECT_DEFINE_TYPE_WITH_INTERFACES(PefGuest,
 
 static void pef_guest_class_init(ObjectClass *oc, void *data)
 {
+    ConfidentialGuestSupportClass *klass = CONFIDENTIAL_GUEST_SUPPORT_CLASS(oc);
+
+    klass->kvm_init = pef_kvm_init;
+    klass->kvm_reset = pef_kvm_reset;
 }
 
 static void pef_guest_init(Object *obj)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 55263f0815ed..544b5f5c1690 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -74,6 +74,7 @@
 #include "hw/virtio/vhost-scsi-common.h"
 
 #include "exec/ram_addr.h"
+#include "exec/confidential-guest-support.h"
 #include "hw/usb.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
@@ -86,7 +87,6 @@
 #include "hw/ppc/spapr_tpm_proxy.h"
 #include "hw/ppc/spapr_nvdimm.h"
 #include "hw/ppc/spapr_numa.h"
-#include "hw/ppc/pef.h"
 
 #include "monitor/monitor.h"
 
@@ -1687,7 +1687,9 @@ static void spapr_machine_reset(MachineState *machine, ShutdownCause reason)
         qemu_guest_getrandom_nofail(spapr->fdt_rng_seed, 32);
     }
 
-    pef_kvm_reset(machine->cgs, &error_fatal);
+    if (machine->cgs) {
+        confidential_guest_kvm_reset(machine->cgs, &error_fatal);
+    }
     spapr_caps_apply(spapr);
 
     first_ppc_cpu = POWERPC_CPU(first_cpu);
@@ -2811,7 +2813,9 @@ static void spapr_machine_init(MachineState *machine)
     /*
      * if Secure VM (PEF) support is configured, then initialize it
      */
-    pef_kvm_init(machine->cgs, &error_fatal);
+    if (machine->cgs) {
+        confidential_guest_kvm_init(machine->cgs, &error_fatal);
+    }
 
     msi_nonbroken = true;
 
diff --git a/include/hw/ppc/pef.h b/include/hw/ppc/pef.h
deleted file mode 100644
index 707dbe524c42..000000000000
--- a/include/hw/ppc/pef.h
+++ /dev/null
@@ -1,17 +0,0 @@
-/*
- * PEF (Protected Execution Facility) for POWER support
- *
- * Copyright Red Hat.
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-
-#ifndef HW_PPC_PEF_H
-#define HW_PPC_PEF_H
-
-int pef_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
-int pef_kvm_reset(ConfidentialGuestSupport *cgs, Error **errp);
-
-#endif /* HW_PPC_PEF_H */
-- 
2.34.1

[PATCH 4/4] s390: Switch to use confidential_guest_kvm_init()

[Xiaoyao Li]

Use unified confidential_guest_kvm_init(), to avoid exposing specific
functions.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Changes from rfc v1:
 - check machine->cgs not NULL before calling confidential_guest_kvm_init();
---
 hw/s390x/s390-virtio-ccw.c |  5 ++++-
 target/s390x/kvm/pv.c      |  8 ++++++++
 target/s390x/kvm/pv.h      | 14 --------------
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 62804cc2281d..9b7564bbd4d2 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -14,6 +14,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "exec/ram_addr.h"
+#include "exec/confidential-guest-support.h"
 #include "hw/s390x/s390-virtio-hcall.h"
 #include "hw/s390x/sclp.h"
 #include "hw/s390x/s390_flic.h"
@@ -260,7 +261,9 @@ static void ccw_init(MachineState *machine)
     s390_init_cpus(machine);
 
     /* Need CPU model to be determined before we can set up PV */
-    s390_pv_init(machine->cgs, &error_fatal);
+    if (machine->cgs) {
+        confidential_guest_kvm_init(machine->cgs, &error_fatal);
+    }
 
     s390_flic_init();
 
diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
index 7ca7faec73e9..c04d53753bfa 100644
--- a/target/s390x/kvm/pv.c
+++ b/target/s390x/kvm/pv.c
@@ -340,6 +340,11 @@ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
         return 0;
     }
 
+    if (!kvm_enabled()) {
+        error_setg(errp, "Protected Virtualization requires KVM");
+        return -1;
+    }
+
     if (!s390_has_feat(S390_FEAT_UNPACK)) {
         error_setg(errp,
                    "CPU model does not support Protected Virtualization");
@@ -364,6 +369,9 @@ OBJECT_DEFINE_TYPE_WITH_INTERFACES(S390PVGuest,
 
 static void s390_pv_guest_class_init(ObjectClass *oc, void *data)
 {
+    ConfidentialGuestSupportClass *klass = CONFIDENTIAL_GUEST_SUPPORT_CLASS(oc);
+
+    klass->kvm_init = s390_pv_kvm_init;
 }
 
 static void s390_pv_guest_init(Object *obj)
diff --git a/target/s390x/kvm/pv.h b/target/s390x/kvm/pv.h
index 5877d28ff10a..4b408174391a 100644
--- a/target/s390x/kvm/pv.h
+++ b/target/s390x/kvm/pv.h
@@ -80,18 +80,4 @@ static inline int kvm_s390_dump_mem_state(uint64_t addr, size_t len,
 static inline int kvm_s390_dump_completion_data(void *buff) { return 0; }
 #endif /* CONFIG_KVM */
 
-int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
-static inline int s390_pv_init(ConfidentialGuestSupport *cgs, Error **errp)
-{
-    if (!cgs) {
-        return 0;
-    }
-    if (kvm_enabled()) {
-        return s390_pv_kvm_init(cgs, errp);
-    }
-
-    error_setg(errp, "Protected Virtualization requires KVM");
-    return -1;
-}
-
 #endif /* HW_S390_PV_H */
-- 
2.34.1