* [RFC PATCH 1/4] confidential guest support: Add kvm_init() and kvm_reset() in class
2024-02-06 8:28 [RFC PATCH 0/4] Confidential Guest Support: Introduce kvm_init() and kvm_reset() virtual functions Xiaoyao Li
@ 2024-02-06 8:28 ` Xiaoyao Li
2024-02-06 14:14 ` Daniel P. Berrangé
2024-02-06 8:28 ` [RFC PATCH 2/4] i386/sev: Switch to use confidential_guest_kvm_init() Xiaoyao Li
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Xiaoyao Li @ 2024-02-06 8:28 UTC (permalink / raw)
To: Daniel P . Berrangé, Nicholas Piggin,
Daniel Henrique Barboza, Cédric Le Goater, David Gibson,
Harsh Prateek Bora, Halil Pasic, Christian Borntraeger,
Eric Farman, Richard Henderson, David Hildenbrand,
Ilya Leoshkevich, Thomas Huth, Paolo Bonzini, Marcelo Tosatti
Cc: qemu-devel, qemu-ppc, qemu-s390x
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>
---
include/exec/confidential-guest-support.h | 42 ++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
index ba2dd4b5dfc4..ff0bfb26ad7a 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,45 @@ 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;
+
+ if (!cgs) {
+ return 0;
+ }
+
+ 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;
+
+ if (!cgs) {
+ return 0;
+ }
+
+ 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [RFC PATCH 1/4] confidential guest support: Add kvm_init() and kvm_reset() in class
2024-02-06 8:28 ` [RFC PATCH 1/4] confidential guest support: Add kvm_init() and kvm_reset() in class Xiaoyao Li
@ 2024-02-06 14:14 ` Daniel P. Berrangé
0 siblings, 0 replies; 10+ messages in thread
From: Daniel P. Berrangé @ 2024-02-06 14:14 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Nicholas Piggin, Daniel Henrique Barboza, Cédric Le Goater,
David Gibson, Harsh Prateek Bora, Halil Pasic,
Christian Borntraeger, Eric Farman, Richard Henderson,
David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Paolo Bonzini,
Marcelo Tosatti, qemu-devel, qemu-ppc, qemu-s390x
On Tue, Feb 06, 2024 at 03:28:49AM -0500, Xiaoyao Li wrote:
> 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>
> ---
> include/exec/confidential-guest-support.h | 42 ++++++++++++++++++++++-
> 1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
> index ba2dd4b5dfc4..ff0bfb26ad7a 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,45 @@ 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;
> +
> + if (!cgs) {
> + return 0;
> + }
Typically I would expect any class/object methods to mandate a non-NULL
class/instance pointer.
IOW, the caller would generally be expected to check NULL and not
call this method in that case.
> +
> + 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;
> +
> + if (!cgs) {
> + return 0;
> + }
> +
> + 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
>
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 :|
^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC PATCH 2/4] i386/sev: Switch to use confidential_guest_kvm_init()
2024-02-06 8:28 [RFC PATCH 0/4] Confidential Guest Support: Introduce kvm_init() and kvm_reset() virtual functions Xiaoyao Li
2024-02-06 8:28 ` [RFC PATCH 1/4] confidential guest support: Add kvm_init() and kvm_reset() in class Xiaoyao Li
@ 2024-02-06 8:28 ` Xiaoyao Li
2024-02-06 14:16 ` Daniel P. Berrangé
2024-02-06 8:28 ` [RFC PATCH 3/4] ppc/pef: switch to use confidential_guest_kvm_init/reset() Xiaoyao Li
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Xiaoyao Li @ 2024-02-06 8:28 UTC (permalink / raw)
To: Daniel P . Berrangé, Nicholas Piggin,
Daniel Henrique Barboza, Cédric Le Goater, David Gibson,
Harsh Prateek Bora, Halil Pasic, Christian Borntraeger,
Eric Farman, Richard Henderson, David Hildenbrand,
Ilya Leoshkevich, Thomas Huth, Paolo Bonzini, Marcelo Tosatti
Cc: qemu-devel, qemu-ppc, qemu-s390x
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.
Clean up the sve-stub.c since it's not needed anymore.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
target/i386/kvm/kvm.c | 2 +-
target/i386/kvm/meson.build | 2 -
target/i386/kvm/sev-stub.c | 5 --
target/i386/sev.c | 120 +++++++++++++++++++-----------------
target/i386/sev.h | 2 -
5 files changed, 63 insertions(+), 68 deletions(-)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 76a66246eb72..bb63bba61fa1 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2534,7 +2534,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 = confidential_guest_kvm_init(ms->cgs, &local_err);
if (ret < 0) {
error_report_err(local_err);
return ret;
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
index 1be5341e8a6a..4a1560cf8ad7 100644
--- a/target/i386/kvm/sev-stub.c
+++ b/target/i386/kvm/sev-stub.c
@@ -14,8 +14,3 @@
#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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [RFC PATCH 2/4] i386/sev: Switch to use confidential_guest_kvm_init()
2024-02-06 8:28 ` [RFC PATCH 2/4] i386/sev: Switch to use confidential_guest_kvm_init() Xiaoyao Li
@ 2024-02-06 14:16 ` Daniel P. Berrangé
2024-02-07 7:10 ` Xiaoyao Li
0 siblings, 1 reply; 10+ messages in thread
From: Daniel P. Berrangé @ 2024-02-06 14:16 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Nicholas Piggin, Daniel Henrique Barboza, Cédric Le Goater,
David Gibson, Harsh Prateek Bora, Halil Pasic,
Christian Borntraeger, Eric Farman, Richard Henderson,
David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Paolo Bonzini,
Marcelo Tosatti, qemu-devel, qemu-ppc, qemu-s390x
On Tue, Feb 06, 2024 at 03:28:50AM -0500, Xiaoyao Li 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.
>
> Clean up the sve-stub.c since it's not needed anymore.
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> target/i386/kvm/kvm.c | 2 +-
> target/i386/kvm/meson.build | 2 -
> target/i386/kvm/sev-stub.c | 5 --
> target/i386/sev.c | 120 +++++++++++++++++++-----------------
> target/i386/sev.h | 2 -
> 5 files changed, 63 insertions(+), 68 deletions(-)
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 76a66246eb72..bb63bba61fa1 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2534,7 +2534,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 = confidential_guest_kvm_init(ms->cgs, &local_err);
If you agree with my comment in patch 1 about the API expecting non-NULL,
then this would need to be conditionalized (same for the 2 following
patches too)
if (ms->cgs) {
ret = confidential_guest_kvm_init(....)
if (ret < 0) {
....
}
}
> if (ret < 0) {
> error_report_err(local_err);
> return ret;
> 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
> index 1be5341e8a6a..4a1560cf8ad7 100644
> --- a/target/i386/kvm/sev-stub.c
> +++ b/target/i386/kvm/sev-stub.c
> @@ -14,8 +14,3 @@
> #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;
> -}
You can actually delete this entire file, since you removed the
only method in it, and stopped building it in the meson.build
patch above.
> 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
>
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 :|
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH 2/4] i386/sev: Switch to use confidential_guest_kvm_init()
2024-02-06 14:16 ` Daniel P. Berrangé
@ 2024-02-07 7:10 ` Xiaoyao Li
0 siblings, 0 replies; 10+ messages in thread
From: Xiaoyao Li @ 2024-02-07 7:10 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Nicholas Piggin, Daniel Henrique Barboza, Cédric Le Goater,
David Gibson, Harsh Prateek Bora, Halil Pasic,
Christian Borntraeger, Eric Farman, Richard Henderson,
David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Paolo Bonzini,
Marcelo Tosatti, qemu-devel, qemu-ppc, qemu-s390x
On 2/6/2024 10:16 PM, Daniel P. Berrangé wrote:
> On Tue, Feb 06, 2024 at 03:28:50AM -0500, Xiaoyao Li 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.
>>
>> Clean up the sve-stub.c since it's not needed anymore.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> target/i386/kvm/kvm.c | 2 +-
>> target/i386/kvm/meson.build | 2 -
>> target/i386/kvm/sev-stub.c | 5 --
>> target/i386/sev.c | 120 +++++++++++++++++++-----------------
>> target/i386/sev.h | 2 -
>> 5 files changed, 63 insertions(+), 68 deletions(-)
>>
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index 76a66246eb72..bb63bba61fa1 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -2534,7 +2534,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 = confidential_guest_kvm_init(ms->cgs, &local_err);
>
> If you agree with my comment in patch 1 about the API expecting non-NULL,
> then this would need to be conditionalized (same for the 2 following
> patches too)
sure. Will change.
> if (ms->cgs) {
> ret = confidential_guest_kvm_init(....)
> if (ret < 0) {
> ....
> }
> }
>
>> if (ret < 0) {
>> error_report_err(local_err);
>> return ret;
>> 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
>> index 1be5341e8a6a..4a1560cf8ad7 100644
>> --- a/target/i386/kvm/sev-stub.c
>> +++ b/target/i386/kvm/sev-stub.c
>> @@ -14,8 +14,3 @@
>> #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;
>> -}
>
> You can actually delete this entire file, since you removed the
> only method in it, and stopped building it in the meson.build
> patch above.
I intented to do it. Apprarently I missed it somehow and didn't catch it
before sending out.
will fix in next version.
>> 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
>>
>
> With regards,
> Daniel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC PATCH 3/4] ppc/pef: switch to use confidential_guest_kvm_init/reset()
2024-02-06 8:28 [RFC PATCH 0/4] Confidential Guest Support: Introduce kvm_init() and kvm_reset() virtual functions Xiaoyao Li
2024-02-06 8:28 ` [RFC PATCH 1/4] confidential guest support: Add kvm_init() and kvm_reset() in class Xiaoyao Li
2024-02-06 8:28 ` [RFC PATCH 2/4] i386/sev: Switch to use confidential_guest_kvm_init() Xiaoyao Li
@ 2024-02-06 8:28 ` Xiaoyao Li
2024-02-06 8:28 ` [RFC PATCH 4/4] s390: Switch to use confidential_guest_kvm_init() Xiaoyao Li
2024-02-06 14:19 ` [RFC PATCH 0/4] Confidential Guest Support: Introduce kvm_init() and kvm_reset() virtual functions Daniel P. Berrangé
4 siblings, 0 replies; 10+ messages in thread
From: Xiaoyao Li @ 2024-02-06 8:28 UTC (permalink / raw)
To: Daniel P . Berrangé, Nicholas Piggin,
Daniel Henrique Barboza, Cédric Le Goater, David Gibson,
Harsh Prateek Bora, Halil Pasic, Christian Borntraeger,
Eric Farman, Richard Henderson, David Hildenbrand,
Ilya Leoshkevich, Thomas Huth, Paolo Bonzini, Marcelo Tosatti
Cc: qemu-devel, qemu-ppc, qemu-s390x
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>
---
hw/ppc/pef.c | 9 ++++++---
hw/ppc/spapr.c | 6 +++---
include/hw/ppc/pef.h | 17 -----------------
3 files changed, 9 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 0d72d286d80f..d1459157e0cf 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,7 @@ 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);
+ confidential_guest_kvm_reset(machine->cgs, &error_fatal);
spapr_caps_apply(spapr);
first_ppc_cpu = POWERPC_CPU(first_cpu);
@@ -2811,7 +2811,7 @@ static void spapr_machine_init(MachineState *machine)
/*
* if Secure VM (PEF) support is configured, then initialize it
*/
- pef_kvm_init(machine->cgs, &error_fatal);
+ 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [RFC PATCH 4/4] s390: Switch to use confidential_guest_kvm_init()
2024-02-06 8:28 [RFC PATCH 0/4] Confidential Guest Support: Introduce kvm_init() and kvm_reset() virtual functions Xiaoyao Li
` (2 preceding siblings ...)
2024-02-06 8:28 ` [RFC PATCH 3/4] ppc/pef: switch to use confidential_guest_kvm_init/reset() Xiaoyao Li
@ 2024-02-06 8:28 ` Xiaoyao Li
2024-02-06 14:19 ` [RFC PATCH 0/4] Confidential Guest Support: Introduce kvm_init() and kvm_reset() virtual functions Daniel P. Berrangé
4 siblings, 0 replies; 10+ messages in thread
From: Xiaoyao Li @ 2024-02-06 8:28 UTC (permalink / raw)
To: Daniel P . Berrangé, Nicholas Piggin,
Daniel Henrique Barboza, Cédric Le Goater, David Gibson,
Harsh Prateek Bora, Halil Pasic, Christian Borntraeger,
Eric Farman, Richard Henderson, David Hildenbrand,
Ilya Leoshkevich, Thomas Huth, Paolo Bonzini, Marcelo Tosatti
Cc: qemu-devel, qemu-ppc, qemu-s390x
Use unified confidential_guest_kvm_init(), to avoid exposing specific
functions.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
hw/s390x/s390-virtio-ccw.c | 3 ++-
target/s390x/kvm/pv.c | 8 ++++++++
target/s390x/kvm/pv.h | 14 --------------
3 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 62804cc2281d..27096ccd9409 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,7 @@ 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);
+ 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [RFC PATCH 0/4] Confidential Guest Support: Introduce kvm_init() and kvm_reset() virtual functions
2024-02-06 8:28 [RFC PATCH 0/4] Confidential Guest Support: Introduce kvm_init() and kvm_reset() virtual functions Xiaoyao Li
` (3 preceding siblings ...)
2024-02-06 8:28 ` [RFC PATCH 4/4] s390: Switch to use confidential_guest_kvm_init() Xiaoyao Li
@ 2024-02-06 14:19 ` Daniel P. Berrangé
2024-02-07 7:29 ` Xiaoyao Li
4 siblings, 1 reply; 10+ messages in thread
From: Daniel P. Berrangé @ 2024-02-06 14:19 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Nicholas Piggin, Daniel Henrique Barboza, Cédric Le Goater,
David Gibson, Harsh Prateek Bora, Halil Pasic,
Christian Borntraeger, Eric Farman, Richard Henderson,
David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Paolo Bonzini,
Marcelo Tosatti, qemu-devel, qemu-ppc, qemu-s390x
On Tue, Feb 06, 2024 at 03:28:48AM -0500, Xiaoyao Li wrote:
> This series is inspired and suggested by Daniel:
> https://lore.kernel.org/qemu-devel/ZbfoQsEuv6_zwl3b@redhat.com/
>
> 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 RFC implements two helper functions confidential_guest_kvm_init()
> and confidential_guest_kvm_reset() in Patch 1. In the following patches,
> they are called in arch specific implementation. X86 will benefit more
> for the generic implementation when TDX support is added.
>
> 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 in their
> arch specific code.
>
> X86 fits it, however I'm not sure if ppc and s390 fit it as well.
> Because currently, ppc calls it in machine->init()
> and s390 calls in MachineClass->init(). I'm not sure if there is any
> order dependency.
IIUC that s390 call is still a machine->init method, rather than
class init.
I think this series is nice, but its up to the KVM maintainers
to decide...
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 :|
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH 0/4] Confidential Guest Support: Introduce kvm_init() and kvm_reset() virtual functions
2024-02-06 14:19 ` [RFC PATCH 0/4] Confidential Guest Support: Introduce kvm_init() and kvm_reset() virtual functions Daniel P. Berrangé
@ 2024-02-07 7:29 ` Xiaoyao Li
0 siblings, 0 replies; 10+ messages in thread
From: Xiaoyao Li @ 2024-02-07 7:29 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Nicholas Piggin, Daniel Henrique Barboza, Cédric Le Goater,
David Gibson, Harsh Prateek Bora, Halil Pasic,
Christian Borntraeger, Eric Farman, Richard Henderson,
David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Paolo Bonzini,
Marcelo Tosatti, qemu-devel, qemu-ppc, qemu-s390x
On 2/6/2024 10:19 PM, Daniel P. Berrangé wrote:
> On Tue, Feb 06, 2024 at 03:28:48AM -0500, Xiaoyao Li wrote:
>> This series is inspired and suggested by Daniel:
>> https://lore.kernel.org/qemu-devel/ZbfoQsEuv6_zwl3b@redhat.com/
>>
>> 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 RFC implements two helper functions confidential_guest_kvm_init()
>> and confidential_guest_kvm_reset() in Patch 1. In the following patches,
>> they are called in arch specific implementation. X86 will benefit more
>> for the generic implementation when TDX support is added.
>>
>> 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 in their
>> arch specific code.
>>
>> X86 fits it, however I'm not sure if ppc and s390 fit it as well.
>> Because currently, ppc calls it in machine->init()
>> and s390 calls in MachineClass->init(). I'm not sure if there is any
>> order dependency.
>
> IIUC that s390 call is still a machine->init method, rather than
> class init.
I double check the code again. Only struct MachineClass has .init()
function defined. And I find both ppc and s390 calls the
confidential_guest_kvm_init() (or their specific cgs kvm_init()) inside
their machine_class->init().
> I think this series is nice, but its up to the KVM maintainers
> to decide...
>
>
> With regards,
> Daniel
^ permalink raw reply [flat|nested] 10+ messages in thread