kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC 00/18] Refactor configuration of guest memory protection
@ 2020-05-14  6:41 David Gibson
  2020-05-14  6:41 ` [RFC 01/18] target/i386: sev: Remove unused QSevGuestInfoClass David Gibson
                   ` (17 more replies)
  0 siblings, 18 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

A number of hardware platforms are implementing mechanisms whereby the
hypervisor does not have unfettered access to guest memory, in order
to mitigate the security impact of a compromised hypervisor.

AMD's SEV implements this with in-cpu memory encryption, and Intel has
its own memory encryption mechanism.  POWER has an upcoming mechanism
to accomplish this in a different way, using a new memory protection
level plus a small trusted ultravisor.  s390 also has a protected
execution environment.

The current code (committed or draft) for these features has each
platform's version configured entirely differently.  That doesn't seem
ideal for users, or particularly for management layers.

AMD SEV introduces a notionally generic machine option
"machine-encryption", but it doesn't actually cover any cases other
than SEV.

This series is a proposal to at least partially unify configuration
for these mechanisms, by renaming and generalizing AMD's
"memory-encryption" property.  It is replaced by a
"guest-memory-protection" property pointing to a platform specific
object which configures and manages the specific details.

For now this series covers just AMD SEV and POWER PEF.  I'm hoping it
can be extended to cover the Intel and s390 mechanisms as well,
though.

Note: I'm using the term "guest memory protection" throughout to refer
to mechanisms like this.  I don't particular like the term, it's both
long and not really precise.  If someone can think of a succinct way
of saying "a means of protecting guest memory from a possibly
compromised hypervisor", I'd be grateful for the suggestion.

David Gibson (18):
  target/i386: sev: Remove unused QSevGuestInfoClass
  target/i386: sev: Move local structure definitions into .c file
  target/i386: sev: Rename QSevGuestInfo
  target/i386: sev: Embed SEVState in SevGuestState
  target/i386: sev: Partial cleanup to sev_state global
  target/i386: sev: Remove redundant cbitpos and reduced_phys_bits
    fields
  target/i386: sev: Remove redundant policy field
  target/i386: sev: Remove redundant handle field
  target/i386: sev: Unify SEVState and SevGuestState
  guest memory protection: Add guest memory protection interface
  guest memory protection: Handle memory encrption via interface
  guest memory protection: Perform KVM init via interface
  guest memory protection: Move side effect out of
    machine_set_memory_encryption()
  guest memory protection: Rework the "memory-encryption" property
  guest memory protection: Decouple kvm_memcrypt_*() helpers from KVM
  use errp for gmpo kvm_init
  spapr: Added PEF based guest memory protection
  guest memory protection: Alter virtio default properties for protected
    guests

 accel/kvm/kvm-all.c                    |  40 +--
 accel/kvm/sev-stub.c                   |   5 -
 accel/stubs/kvm-stub.c                 |  10 -
 backends/Makefile.objs                 |   2 +
 backends/guest-memory-protection.c     |  29 ++
 hw/core/machine.c                      |  61 ++++-
 hw/i386/pc_sysfw.c                     |   6 +-
 include/exec/guest-memory-protection.h |  77 ++++++
 include/hw/boards.h                    |   4 +-
 include/sysemu/kvm.h                   |  17 --
 include/sysemu/sev.h                   |   6 +-
 target/i386/sev.c                      | 358 +++++++++++++------------
 target/i386/sev_i386.h                 |  49 ----
 target/ppc/Makefile.objs               |   2 +-
 target/ppc/pef.c                       |  81 ++++++
 15 files changed, 445 insertions(+), 302 deletions(-)
 create mode 100644 backends/guest-memory-protection.c
 create mode 100644 include/exec/guest-memory-protection.h
 create mode 100644 target/ppc/pef.c

-- 
2.26.2


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [RFC 01/18] target/i386: sev: Remove unused QSevGuestInfoClass
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 02/18] target/i386: sev: Move local structure definitions into .c file David Gibson
                   ` (16 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

This structure is nothing but an empty wrapper around the parent class,
which by QOM conventions means we don't need it at all.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/i386/sev.c      | 1 -
 target/i386/sev_i386.h | 5 -----
 2 files changed, 6 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 846018a12d..d73d53d558 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -290,7 +290,6 @@ static const TypeInfo qsev_guest_info = {
     .name = TYPE_QSEV_GUEST_INFO,
     .instance_size = sizeof(QSevGuestInfo),
     .instance_finalize = qsev_guest_finalize,
-    .class_size = sizeof(QSevGuestInfoClass),
     .class_init = qsev_guest_class_init,
     .instance_init = qsev_guest_init,
     .interfaces = (InterfaceInfo[]) {
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 8ada9d385d..4f193642ac 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -41,7 +41,6 @@ extern char *sev_get_launch_measurement(void);
 extern SevCapability *sev_get_capabilities(void);
 
 typedef struct QSevGuestInfo QSevGuestInfo;
-typedef struct QSevGuestInfoClass QSevGuestInfoClass;
 
 /**
  * QSevGuestInfo:
@@ -64,10 +63,6 @@ struct QSevGuestInfo {
     uint32_t reduced_phys_bits;
 };
 
-struct QSevGuestInfoClass {
-    ObjectClass parent_class;
-};
-
 struct SEVState {
     QSevGuestInfo *sev_info;
     uint8_t api_major;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 02/18] target/i386: sev: Move local structure definitions into .c file
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
  2020-05-14  6:41 ` [RFC 01/18] target/i386: sev: Remove unused QSevGuestInfoClass David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 03/18] target/i386: sev: Rename QSevGuestInfo David Gibson
                   ` (15 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

Neither QSevGuestInfo nor SEVState (not to be confused with SevState) is
used anywhere outside target/i386/sev.c, so they might as well live in
there rather than in a (somewhat) exposed header.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/i386/sev.c      | 44 ++++++++++++++++++++++++++++++++++++++++++
 target/i386/sev_i386.h | 44 ------------------------------------------
 2 files changed, 44 insertions(+), 44 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index d73d53d558..c7a6e3f6d2 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -29,6 +29,50 @@
 #include "trace.h"
 #include "migration/blocker.h"
 
+#define TYPE_QSEV_GUEST_INFO "sev-guest"
+#define QSEV_GUEST_INFO(obj)                  \
+    OBJECT_CHECK(QSevGuestInfo, (obj), TYPE_QSEV_GUEST_INFO)
+
+typedef struct QSevGuestInfo QSevGuestInfo;
+
+/**
+ * QSevGuestInfo:
+ *
+ * The QSevGuestInfo object is used for creating a SEV guest.
+ *
+ * # $QEMU \
+ *         -object sev-guest,id=sev0 \
+ *         -machine ...,memory-encryption=sev0
+ */
+struct QSevGuestInfo {
+    Object parent_obj;
+
+    char *sev_device;
+    uint32_t policy;
+    uint32_t handle;
+    char *dh_cert_file;
+    char *session_file;
+    uint32_t cbitpos;
+    uint32_t reduced_phys_bits;
+};
+
+struct SEVState {
+    QSevGuestInfo *sev_info;
+    uint8_t api_major;
+    uint8_t api_minor;
+    uint8_t build_id;
+    uint32_t policy;
+    uint64_t me_mask;
+    uint32_t cbitpos;
+    uint32_t reduced_phys_bits;
+    uint32_t handle;
+    int sev_fd;
+    SevState state;
+    gchar *measurement;
+};
+
+typedef struct SEVState SEVState;
+
 #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
 #define DEFAULT_SEV_DEVICE      "/dev/sev"
 
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 4f193642ac..8eb7de1bef 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -28,10 +28,6 @@
 #define SEV_POLICY_DOMAIN       0x10
 #define SEV_POLICY_SEV          0x20
 
-#define TYPE_QSEV_GUEST_INFO "sev-guest"
-#define QSEV_GUEST_INFO(obj)                  \
-    OBJECT_CHECK(QSevGuestInfo, (obj), TYPE_QSEV_GUEST_INFO)
-
 extern bool sev_enabled(void);
 extern uint64_t sev_get_me_mask(void);
 extern SevInfo *sev_get_info(void);
@@ -40,44 +36,4 @@ extern uint32_t sev_get_reduced_phys_bits(void);
 extern char *sev_get_launch_measurement(void);
 extern SevCapability *sev_get_capabilities(void);
 
-typedef struct QSevGuestInfo QSevGuestInfo;
-
-/**
- * QSevGuestInfo:
- *
- * The QSevGuestInfo object is used for creating a SEV guest.
- *
- * # $QEMU \
- *         -object sev-guest,id=sev0 \
- *         -machine ...,memory-encryption=sev0
- */
-struct QSevGuestInfo {
-    Object parent_obj;
-
-    char *sev_device;
-    uint32_t policy;
-    uint32_t handle;
-    char *dh_cert_file;
-    char *session_file;
-    uint32_t cbitpos;
-    uint32_t reduced_phys_bits;
-};
-
-struct SEVState {
-    QSevGuestInfo *sev_info;
-    uint8_t api_major;
-    uint8_t api_minor;
-    uint8_t build_id;
-    uint32_t policy;
-    uint64_t me_mask;
-    uint32_t cbitpos;
-    uint32_t reduced_phys_bits;
-    uint32_t handle;
-    int sev_fd;
-    SevState state;
-    gchar *measurement;
-};
-
-typedef struct SEVState SEVState;
-
 #endif
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 03/18] target/i386: sev: Rename QSevGuestInfo
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
  2020-05-14  6:41 ` [RFC 01/18] target/i386: sev: Remove unused QSevGuestInfoClass David Gibson
  2020-05-14  6:41 ` [RFC 02/18] target/i386: sev: Move local structure definitions into .c file David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 04/18] target/i386: sev: Embed SEVState in SevGuestState David Gibson
                   ` (14 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

At the moment this is a purely passive object which is just a container for
information used elsewhere, hence the name.  I'm going to change that
though, so as a preliminary rename it to SevGuestState.

That name risks confusion with both SEVState and SevState, but I'll be
working on that in following patches.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/i386/sev.c | 87 ++++++++++++++++++++++++-----------------------
 1 file changed, 44 insertions(+), 43 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index c7a6e3f6d2..0f7abe134a 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -29,22 +29,23 @@
 #include "trace.h"
 #include "migration/blocker.h"
 
-#define TYPE_QSEV_GUEST_INFO "sev-guest"
-#define QSEV_GUEST_INFO(obj)                  \
-    OBJECT_CHECK(QSevGuestInfo, (obj), TYPE_QSEV_GUEST_INFO)
+#define TYPE_SEV_GUEST "sev-guest"
+#define SEV_GUEST(obj)                                          \
+    OBJECT_CHECK(SevGuestState, (obj), TYPE_SEV_GUEST)
 
-typedef struct QSevGuestInfo QSevGuestInfo;
+typedef struct SevGuestState SevGuestState;
 
 /**
- * QSevGuestInfo:
+ * SevGuestState:
  *
- * The QSevGuestInfo object is used for creating a SEV guest.
+ * The SevGuestState object is used for creating and managing a SEV
+ * guest.
  *
  * # $QEMU \
  *         -object sev-guest,id=sev0 \
  *         -machine ...,memory-encryption=sev0
  */
-struct QSevGuestInfo {
+struct SevGuestState {
     Object parent_obj;
 
     char *sev_device;
@@ -57,7 +58,7 @@ struct QSevGuestInfo {
 };
 
 struct SEVState {
-    QSevGuestInfo *sev_info;
+    SevGuestState *sev_info;
     uint8_t api_major;
     uint8_t api_minor;
     uint8_t build_id;
@@ -235,85 +236,85 @@ static struct RAMBlockNotifier sev_ram_notifier = {
 };
 
 static void
-qsev_guest_finalize(Object *obj)
+sev_guest_finalize(Object *obj)
 {
 }
 
 static char *
-qsev_guest_get_session_file(Object *obj, Error **errp)
+sev_guest_get_session_file(Object *obj, Error **errp)
 {
-    QSevGuestInfo *s = QSEV_GUEST_INFO(obj);
+    SevGuestState *s = SEV_GUEST(obj);
 
     return s->session_file ? g_strdup(s->session_file) : NULL;
 }
 
 static void
-qsev_guest_set_session_file(Object *obj, const char *value, Error **errp)
+sev_guest_set_session_file(Object *obj, const char *value, Error **errp)
 {
-    QSevGuestInfo *s = QSEV_GUEST_INFO(obj);
+    SevGuestState *s = SEV_GUEST(obj);
 
     s->session_file = g_strdup(value);
 }
 
 static char *
-qsev_guest_get_dh_cert_file(Object *obj, Error **errp)
+sev_guest_get_dh_cert_file(Object *obj, Error **errp)
 {
-    QSevGuestInfo *s = QSEV_GUEST_INFO(obj);
+    SevGuestState *s = SEV_GUEST(obj);
 
     return g_strdup(s->dh_cert_file);
 }
 
 static void
-qsev_guest_set_dh_cert_file(Object *obj, const char *value, Error **errp)
+sev_guest_set_dh_cert_file(Object *obj, const char *value, Error **errp)
 {
-    QSevGuestInfo *s = QSEV_GUEST_INFO(obj);
+    SevGuestState *s = SEV_GUEST(obj);
 
     s->dh_cert_file = g_strdup(value);
 }
 
 static char *
-qsev_guest_get_sev_device(Object *obj, Error **errp)
+sev_guest_get_sev_device(Object *obj, Error **errp)
 {
-    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
+    SevGuestState *sev = SEV_GUEST(obj);
 
     return g_strdup(sev->sev_device);
 }
 
 static void
-qsev_guest_set_sev_device(Object *obj, const char *value, Error **errp)
+sev_guest_set_sev_device(Object *obj, const char *value, Error **errp)
 {
-    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
+    SevGuestState *sev = SEV_GUEST(obj);
 
     sev->sev_device = g_strdup(value);
 }
 
 static void
-qsev_guest_class_init(ObjectClass *oc, void *data)
+sev_guest_class_init(ObjectClass *oc, void *data)
 {
     object_class_property_add_str(oc, "sev-device",
-                                  qsev_guest_get_sev_device,
-                                  qsev_guest_set_sev_device,
+                                  sev_guest_get_sev_device,
+                                  sev_guest_set_sev_device,
                                   NULL);
     object_class_property_set_description(oc, "sev-device",
             "SEV device to use", NULL);
     object_class_property_add_str(oc, "dh-cert-file",
-                                  qsev_guest_get_dh_cert_file,
-                                  qsev_guest_set_dh_cert_file,
+                                  sev_guest_get_dh_cert_file,
+                                  sev_guest_set_dh_cert_file,
                                   NULL);
     object_class_property_set_description(oc, "dh-cert-file",
             "guest owners DH certificate (encoded with base64)", NULL);
     object_class_property_add_str(oc, "session-file",
-                                  qsev_guest_get_session_file,
-                                  qsev_guest_set_session_file,
+                                  sev_guest_get_session_file,
+                                  sev_guest_set_session_file,
                                   NULL);
     object_class_property_set_description(oc, "session-file",
             "guest owners session parameters (encoded with base64)", NULL);
 }
 
 static void
-qsev_guest_init(Object *obj)
+sev_guest_instance_init(Object *obj)
 {
-    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
+    SevGuestState *sev = SEV_GUEST(obj);
 
     sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
     sev->policy = DEFAULT_GUEST_POLICY;
@@ -329,32 +330,32 @@ qsev_guest_init(Object *obj)
 }
 
 /* sev guest info */
-static const TypeInfo qsev_guest_info = {
+static const TypeInfo sev_guest_info = {
     .parent = TYPE_OBJECT,
-    .name = TYPE_QSEV_GUEST_INFO,
-    .instance_size = sizeof(QSevGuestInfo),
-    .instance_finalize = qsev_guest_finalize,
-    .class_init = qsev_guest_class_init,
-    .instance_init = qsev_guest_init,
+    .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 QSevGuestInfo *
+static SevGuestState *
 lookup_sev_guest_info(const char *id)
 {
     Object *obj;
-    QSevGuestInfo *info;
+    SevGuestState *info;
 
     obj = object_resolve_path_component(object_get_objects_root(), id);
     if (!obj) {
         return NULL;
     }
 
-    info = (QSevGuestInfo *)
-            object_dynamic_cast(obj, TYPE_QSEV_GUEST_INFO);
+    info = (SevGuestState *)
+            object_dynamic_cast(obj, TYPE_SEV_GUEST);
     if (!info) {
         return NULL;
     }
@@ -513,7 +514,7 @@ sev_launch_start(SEVState *s)
     gsize sz;
     int ret = 1;
     int fw_error, rc;
-    QSevGuestInfo *sev = s->sev_info;
+    SevGuestState *sev = s->sev_info;
     struct kvm_sev_launch_start *start;
     guchar *session = NULL, *dh_cert = NULL;
 
@@ -699,7 +700,7 @@ sev_guest_init(const char *id)
     s->sev_info = lookup_sev_guest_info(id);
     if (!s->sev_info) {
         error_report("%s: '%s' is not a valid '%s' object",
-                     __func__, id, TYPE_QSEV_GUEST_INFO);
+                     __func__, id, TYPE_SEV_GUEST);
         goto err;
     }
 
@@ -789,7 +790,7 @@ sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
 static void
 sev_register_types(void)
 {
-    type_register_static(&qsev_guest_info);
+    type_register_static(&sev_guest_info);
 }
 
 type_init(sev_register_types);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 04/18] target/i386: sev: Embed SEVState in SevGuestState
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (2 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 03/18] target/i386: sev: Rename QSevGuestInfo David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 05/18] target/i386: sev: Partial cleanup to sev_state global David Gibson
                   ` (13 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

Currently SevGuestState contains only configuration information.  For
runtime state another non-QOM struct SEVState is allocated separately.

Simplify things by instead embedding the SEVState structure in
SevGuestState.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/i386/sev.c | 54 +++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 0f7abe134a..89138a7507 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -35,6 +35,22 @@
 
 typedef struct SevGuestState SevGuestState;
 
+struct SEVState {
+    uint8_t api_major;
+    uint8_t api_minor;
+    uint8_t build_id;
+    uint32_t policy;
+    uint64_t me_mask;
+    uint32_t cbitpos;
+    uint32_t reduced_phys_bits;
+    uint32_t handle;
+    int sev_fd;
+    SevState state;
+    gchar *measurement;
+};
+
+typedef struct SEVState SEVState;
+
 /**
  * SevGuestState:
  *
@@ -48,6 +64,7 @@ typedef struct SevGuestState SevGuestState;
 struct SevGuestState {
     Object parent_obj;
 
+    /* configuration parameters */
     char *sev_device;
     uint32_t policy;
     uint32_t handle;
@@ -55,25 +72,11 @@ struct SevGuestState {
     char *session_file;
     uint32_t cbitpos;
     uint32_t reduced_phys_bits;
-};
 
-struct SEVState {
-    SevGuestState *sev_info;
-    uint8_t api_major;
-    uint8_t api_minor;
-    uint8_t build_id;
-    uint32_t policy;
-    uint64_t me_mask;
-    uint32_t cbitpos;
-    uint32_t reduced_phys_bits;
-    uint32_t handle;
-    int sev_fd;
-    SevState state;
-    gchar *measurement;
+    /* runtime state */
+    SEVState state;
 };
 
-typedef struct SEVState SEVState;
-
 #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
 #define DEFAULT_SEV_DEVICE      "/dev/sev"
 
@@ -509,12 +512,12 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len)
 }
 
 static int
-sev_launch_start(SEVState *s)
+sev_launch_start(SevGuestState *sev)
 {
+    SEVState *s = &sev->state;
     gsize sz;
     int ret = 1;
     int fw_error, rc;
-    SevGuestState *sev = s->sev_info;
     struct kvm_sev_launch_start *start;
     guchar *session = NULL, *dh_cert = NULL;
 
@@ -689,6 +692,7 @@ sev_vm_state_change(void *opaque, int running, RunState state)
 void *
 sev_guest_init(const char *id)
 {
+    SevGuestState *sev;
     SEVState *s;
     char *devname;
     int ret, fw_error;
@@ -696,27 +700,27 @@ sev_guest_init(const char *id)
     uint32_t host_cbitpos;
     struct sev_user_data_status status = {};
 
-    sev_state = s = g_new0(SEVState, 1);
-    s->sev_info = lookup_sev_guest_info(id);
-    if (!s->sev_info) {
+    sev = lookup_sev_guest_info(id);
+    if (!sev) {
         error_report("%s: '%s' is not a valid '%s' object",
                      __func__, id, TYPE_SEV_GUEST);
         goto err;
     }
 
+    sev_state = s = &sev->state;
     s->state = SEV_STATE_UNINIT;
 
     host_cpuid(0x8000001F, 0, NULL, &ebx, NULL, NULL);
     host_cbitpos = ebx & 0x3f;
 
-    s->cbitpos = object_property_get_int(OBJECT(s->sev_info), "cbitpos", NULL);
+    s->cbitpos = object_property_get_int(OBJECT(sev), "cbitpos", NULL);
     if (host_cbitpos != s->cbitpos) {
         error_report("%s: cbitpos check failed, host '%d' requested '%d'",
                      __func__, host_cbitpos, s->cbitpos);
         goto err;
     }
 
-    s->reduced_phys_bits = object_property_get_int(OBJECT(s->sev_info),
+    s->reduced_phys_bits = object_property_get_int(OBJECT(sev),
                                         "reduced-phys-bits", NULL);
     if (s->reduced_phys_bits < 1) {
         error_report("%s: reduced_phys_bits check failed, it should be >=1,"
@@ -726,7 +730,7 @@ sev_guest_init(const char *id)
 
     s->me_mask = ~(1UL << s->cbitpos);
 
-    devname = object_property_get_str(OBJECT(s->sev_info), "sev-device", NULL);
+    devname = object_property_get_str(OBJECT(sev), "sev-device", NULL);
     s->sev_fd = open(devname, O_RDWR);
     if (s->sev_fd < 0) {
         error_report("%s: Failed to open %s '%s'", __func__,
@@ -757,7 +761,7 @@ sev_guest_init(const char *id)
         goto err;
     }
 
-    ret = sev_launch_start(s);
+    ret = sev_launch_start(sev);
     if (ret) {
         error_report("%s: failed to create encryption context", __func__);
         goto err;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 05/18] target/i386: sev: Partial cleanup to sev_state global
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (3 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 04/18] target/i386: sev: Embed SEVState in SevGuestState David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 06/18] target/i386: sev: Remove redundant cbitpos and reduced_phys_bits fields David Gibson
                   ` (12 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

The SEV code uses a pretty ugly global to access its internal state.  Now
that SEVState is embedded in SevGuestState, we can avoid accessing it via
the global in some cases.  In the remaining cases use a new global
referencing the containing SevGuestState which will simplify some future
transformations.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/i386/sev.c | 92 ++++++++++++++++++++++++-----------------------
 1 file changed, 48 insertions(+), 44 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 89138a7507..5f0c38da95 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -80,7 +80,7 @@ struct SevGuestState {
 #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
 #define DEFAULT_SEV_DEVICE      "/dev/sev"
 
-static SEVState *sev_state;
+static SevGuestState *sev_guest;
 static Error *sev_mig_blocker;
 
 static const char *const sev_fw_errlist[] = {
@@ -159,21 +159,21 @@ fw_error_to_str(int code)
 }
 
 static bool
-sev_check_state(SevState state)
+sev_check_state(const SevGuestState *sev, SevState state)
 {
-    assert(sev_state);
-    return sev_state->state == state ? true : false;
+    assert(sev);
+    return sev->state.state == state ? true : false;
 }
 
 static void
-sev_set_guest_state(SevState new_state)
+sev_set_guest_state(SevGuestState *sev, SevState new_state)
 {
     assert(new_state < SEV_STATE__MAX);
-    assert(sev_state);
+    assert(sev);
 
-    trace_kvm_sev_change_state(SevState_str(sev_state->state),
+    trace_kvm_sev_change_state(SevState_str(sev->state.state),
                                SevState_str(new_state));
-    sev_state->state = new_state;
+    sev->state.state = new_state;
 }
 
 static void
@@ -369,25 +369,25 @@ lookup_sev_guest_info(const char *id)
 bool
 sev_enabled(void)
 {
-    return sev_state ? true : false;
+    return !!sev_guest;
 }
 
 uint64_t
 sev_get_me_mask(void)
 {
-    return sev_state ? sev_state->me_mask : ~0;
+    return sev_guest ? sev_guest->state.me_mask : ~0;
 }
 
 uint32_t
 sev_get_cbit_position(void)
 {
-    return sev_state ? sev_state->cbitpos : 0;
+    return sev_guest ? sev_guest->state.cbitpos : 0;
 }
 
 uint32_t
 sev_get_reduced_phys_bits(void)
 {
-    return sev_state ? sev_state->reduced_phys_bits : 0;
+    return sev_guest ? sev_guest->state.reduced_phys_bits : 0;
 }
 
 SevInfo *
@@ -396,15 +396,15 @@ sev_get_info(void)
     SevInfo *info;
 
     info = g_new0(SevInfo, 1);
-    info->enabled = sev_state ? true : false;
+    info->enabled = sev_enabled();
 
     if (info->enabled) {
-        info->api_major = sev_state->api_major;
-        info->api_minor = sev_state->api_minor;
-        info->build_id = sev_state->build_id;
-        info->policy = sev_state->policy;
-        info->state = sev_state->state;
-        info->handle = sev_state->handle;
+        info->api_major = sev_guest->state.api_major;
+        info->api_minor = sev_guest->state.api_minor;
+        info->build_id = sev_guest->state.build_id;
+        info->policy = sev_guest->state.policy;
+        info->state = sev_guest->state.state;
+        info->handle = sev_guest->state.handle;
     }
 
     return info;
@@ -553,7 +553,7 @@ sev_launch_start(SevGuestState *sev)
 
     object_property_set_int(OBJECT(sev), start->handle, "handle",
                             &error_abort);
-    sev_set_guest_state(SEV_STATE_LAUNCH_UPDATE);
+    sev_set_guest_state(sev, SEV_STATE_LAUNCH_UPDATE);
     s->handle = start->handle;
     s->policy = start->policy;
     ret = 0;
@@ -566,7 +566,7 @@ out:
 }
 
 static int
-sev_launch_update_data(uint8_t *addr, uint64_t len)
+sev_launch_update_data(SevGuestState *sev, uint8_t *addr, uint64_t len)
 {
     int ret, fw_error;
     struct kvm_sev_launch_update_data update;
@@ -578,7 +578,7 @@ sev_launch_update_data(uint8_t *addr, uint64_t len)
     update.uaddr = (__u64)(unsigned long)addr;
     update.len = len;
     trace_kvm_sev_launch_update_data(addr, len);
-    ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_LAUNCH_UPDATE_DATA,
+    ret = sev_ioctl(sev->state.sev_fd, KVM_SEV_LAUNCH_UPDATE_DATA,
                     &update, &fw_error);
     if (ret) {
         error_report("%s: LAUNCH_UPDATE ret=%d fw_error=%d '%s'",
@@ -591,19 +591,20 @@ sev_launch_update_data(uint8_t *addr, uint64_t len)
 static void
 sev_launch_get_measure(Notifier *notifier, void *unused)
 {
+    SevGuestState *sev = sev_guest;
     int ret, error;
     guchar *data;
-    SEVState *s = sev_state;
+    SEVState *s = &sev->state;
     struct kvm_sev_launch_measure *measurement;
 
-    if (!sev_check_state(SEV_STATE_LAUNCH_UPDATE)) {
+    if (!sev_check_state(sev, SEV_STATE_LAUNCH_UPDATE)) {
         return;
     }
 
     measurement = g_new0(struct kvm_sev_launch_measure, 1);
 
     /* query the measurement blob length */
-    ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_LAUNCH_MEASURE,
+    ret = sev_ioctl(sev->state.sev_fd, KVM_SEV_LAUNCH_MEASURE,
                     measurement, &error);
     if (!measurement->len) {
         error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'",
@@ -615,7 +616,7 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
     measurement->uaddr = (unsigned long)data;
 
     /* get the measurement blob */
-    ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_LAUNCH_MEASURE,
+    ret = sev_ioctl(sev->state.sev_fd, KVM_SEV_LAUNCH_MEASURE,
                     measurement, &error);
     if (ret) {
         error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'",
@@ -623,7 +624,7 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
         goto free_data;
     }
 
-    sev_set_guest_state(SEV_STATE_LAUNCH_SECRET);
+    sev_set_guest_state(sev, SEV_STATE_LAUNCH_SECRET);
 
     /* encode the measurement value and emit the event */
     s->measurement = g_base64_encode(data, measurement->len);
@@ -638,9 +639,9 @@ free_measurement:
 char *
 sev_get_launch_measurement(void)
 {
-    if (sev_state &&
-        sev_state->state >= SEV_STATE_LAUNCH_SECRET) {
-        return g_strdup(sev_state->measurement);
+    if (sev_guest &&
+        sev_guest->state.state >= SEV_STATE_LAUNCH_SECRET) {
+        return g_strdup(sev_guest->state.measurement);
     }
 
     return NULL;
@@ -651,20 +652,21 @@ static Notifier sev_machine_done_notify = {
 };
 
 static void
-sev_launch_finish(SEVState *s)
+sev_launch_finish(SevGuestState *sev)
 {
+    SEVState *s = &sev->state;
     int ret, error;
     Error *local_err = NULL;
 
     trace_kvm_sev_launch_finish();
-    ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_LAUNCH_FINISH, 0, &error);
+    ret = sev_ioctl(s->sev_fd, KVM_SEV_LAUNCH_FINISH, 0, &error);
     if (ret) {
         error_report("%s: LAUNCH_FINISH ret=%d fw_error=%d '%s'",
                      __func__, ret, error, fw_error_to_str(error));
         exit(1);
     }
 
-    sev_set_guest_state(SEV_STATE_RUNNING);
+    sev_set_guest_state(sev, SEV_STATE_RUNNING);
 
     /* add migration blocker */
     error_setg(&sev_mig_blocker,
@@ -680,11 +682,11 @@ sev_launch_finish(SEVState *s)
 static void
 sev_vm_state_change(void *opaque, int running, RunState state)
 {
-    SEVState *s = opaque;
+    SevGuestState *sev = opaque;
 
     if (running) {
-        if (!sev_check_state(SEV_STATE_RUNNING)) {
-            sev_launch_finish(s);
+        if (!sev_check_state(sev, SEV_STATE_RUNNING)) {
+            sev_launch_finish(sev);
         }
     }
 }
@@ -707,7 +709,8 @@ sev_guest_init(const char *id)
         goto err;
     }
 
-    sev_state = s = &sev->state;
+    sev_guest = sev;
+    s = &sev->state;
     s->state = SEV_STATE_UNINIT;
 
     host_cpuid(0x8000001F, 0, NULL, &ebx, NULL, NULL);
@@ -769,23 +772,24 @@ sev_guest_init(const char *id)
 
     ram_block_notifier_add(&sev_ram_notifier);
     qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
-    qemu_add_vm_change_state_handler(sev_vm_state_change, s);
+    qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
 
-    return s;
+    return sev;
 err:
-    g_free(sev_state);
-    sev_state = NULL;
+    sev_guest = NULL;
     return NULL;
 }
 
 int
 sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
 {
-    assert(handle);
+    SevGuestState *sev = handle;
+
+    assert(sev);
 
     /* if SEV is in update state then encrypt the data else do nothing */
-    if (sev_check_state(SEV_STATE_LAUNCH_UPDATE)) {
-        return sev_launch_update_data(ptr, len);
+    if (sev_check_state(sev, SEV_STATE_LAUNCH_UPDATE)) {
+        return sev_launch_update_data(sev, ptr, len);
     }
 
     return 0;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 06/18] target/i386: sev: Remove redundant cbitpos and reduced_phys_bits fields
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (4 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 05/18] target/i386: sev: Partial cleanup to sev_state global David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 07/18] target/i386: sev: Remove redundant policy field David Gibson
                   ` (11 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

The SEVState structure has cbitpos and reduced_phys_bits fields which are
simply copied from the SevGuestState structure and never changed.  Now that
SEVState is embedded in SevGuestState we can just access the original copy
directly.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/i386/sev.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 5f0c38da95..c85f59d78f 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -41,8 +41,6 @@ struct SEVState {
     uint8_t build_id;
     uint32_t policy;
     uint64_t me_mask;
-    uint32_t cbitpos;
-    uint32_t reduced_phys_bits;
     uint32_t handle;
     int sev_fd;
     SevState state;
@@ -381,13 +379,13 @@ sev_get_me_mask(void)
 uint32_t
 sev_get_cbit_position(void)
 {
-    return sev_guest ? sev_guest->state.cbitpos : 0;
+    return sev_guest ? sev_guest->cbitpos : 0;
 }
 
 uint32_t
 sev_get_reduced_phys_bits(void)
 {
-    return sev_guest ? sev_guest->state.reduced_phys_bits : 0;
+    return sev_guest ? sev_guest->reduced_phys_bits : 0;
 }
 
 SevInfo *
@@ -716,22 +714,19 @@ sev_guest_init(const char *id)
     host_cpuid(0x8000001F, 0, NULL, &ebx, NULL, NULL);
     host_cbitpos = ebx & 0x3f;
 
-    s->cbitpos = object_property_get_int(OBJECT(sev), "cbitpos", NULL);
-    if (host_cbitpos != s->cbitpos) {
+    if (host_cbitpos != sev->cbitpos) {
         error_report("%s: cbitpos check failed, host '%d' requested '%d'",
-                     __func__, host_cbitpos, s->cbitpos);
+                     __func__, host_cbitpos, sev->cbitpos);
         goto err;
     }
 
-    s->reduced_phys_bits = object_property_get_int(OBJECT(sev),
-                                        "reduced-phys-bits", NULL);
-    if (s->reduced_phys_bits < 1) {
+    if (sev->reduced_phys_bits < 1) {
         error_report("%s: reduced_phys_bits check failed, it should be >=1,"
-                     " requested '%d'", __func__, s->reduced_phys_bits);
+                     " requested '%d'", __func__, sev->reduced_phys_bits);
         goto err;
     }
 
-    s->me_mask = ~(1UL << s->cbitpos);
+    s->me_mask = ~(1UL << sev->cbitpos);
 
     devname = object_property_get_str(OBJECT(sev), "sev-device", NULL);
     s->sev_fd = open(devname, O_RDWR);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 07/18] target/i386: sev: Remove redundant policy field
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (5 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 06/18] target/i386: sev: Remove redundant cbitpos and reduced_phys_bits fields David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 08/18] target/i386: sev: Remove redundant handle field David Gibson
                   ` (10 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

SEVState::policy is set from the final value of the policy field in the
parameter structure for the KVM_SEV_LAUNCH_START ioctl().  But, AFAICT
that ioctl() won't ever change it from the original supplied value which
comes from SevGuestState::policy.

So, remove this field and just use SevGuestState::policy directly.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/i386/sev.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index c85f59d78f..1ba05cd2a6 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -39,7 +39,6 @@ struct SEVState {
     uint8_t api_major;
     uint8_t api_minor;
     uint8_t build_id;
-    uint32_t policy;
     uint64_t me_mask;
     uint32_t handle;
     int sev_fd;
@@ -400,7 +399,7 @@ sev_get_info(void)
         info->api_major = sev_guest->state.api_major;
         info->api_minor = sev_guest->state.api_minor;
         info->build_id = sev_guest->state.build_id;
-        info->policy = sev_guest->state.policy;
+        info->policy = sev_guest->policy;
         info->state = sev_guest->state.state;
         info->handle = sev_guest->state.handle;
     }
@@ -523,8 +522,7 @@ sev_launch_start(SevGuestState *sev)
 
     start->handle = object_property_get_int(OBJECT(sev), "handle",
                                             &error_abort);
-    start->policy = object_property_get_int(OBJECT(sev), "policy",
-                                            &error_abort);
+    start->policy = sev->policy;
     if (sev->session_file) {
         if (sev_read_file_base64(sev->session_file, &session, &sz) < 0) {
             goto out;
@@ -553,7 +551,6 @@ sev_launch_start(SevGuestState *sev)
                             &error_abort);
     sev_set_guest_state(sev, SEV_STATE_LAUNCH_UPDATE);
     s->handle = start->handle;
-    s->policy = start->policy;
     ret = 0;
 
 out:
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 08/18] target/i386: sev: Remove redundant handle field
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (6 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 07/18] target/i386: sev: Remove redundant policy field David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 09/18] target/i386: sev: Unify SEVState and SevGuestState David Gibson
                   ` (9 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

The user can explicitly specify a handle via the "handle" property wired
to SevGuestState::handle.  That gets passed to the KVM_SEV_LAUNCH_START
ioctl() which may update it, the final value being copied back to both
SevGuestState::handle and SEVState::handle.

AFAICT, nothing will be looking SEVState::handle before it and
SevGuestState::handle have been updated from the ioctl().  So, remove the
field and just use SevGuestState::handle directly.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/i386/sev.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 1ba05cd2a6..11034ed356 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -40,7 +40,6 @@ struct SEVState {
     uint8_t api_minor;
     uint8_t build_id;
     uint64_t me_mask;
-    uint32_t handle;
     int sev_fd;
     SevState state;
     gchar *measurement;
@@ -64,13 +63,13 @@ struct SevGuestState {
     /* configuration parameters */
     char *sev_device;
     uint32_t policy;
-    uint32_t handle;
     char *dh_cert_file;
     char *session_file;
     uint32_t cbitpos;
     uint32_t reduced_phys_bits;
 
     /* runtime state */
+    uint32_t handle;
     SEVState state;
 };
 
@@ -401,7 +400,7 @@ sev_get_info(void)
         info->build_id = sev_guest->state.build_id;
         info->policy = sev_guest->policy;
         info->state = sev_guest->state.state;
-        info->handle = sev_guest->state.handle;
+        info->handle = sev_guest->handle;
     }
 
     return info;
@@ -520,8 +519,7 @@ sev_launch_start(SevGuestState *sev)
 
     start = g_new0(struct kvm_sev_launch_start, 1);
 
-    start->handle = object_property_get_int(OBJECT(sev), "handle",
-                                            &error_abort);
+    start->handle = sev->handle;
     start->policy = sev->policy;
     if (sev->session_file) {
         if (sev_read_file_base64(sev->session_file, &session, &sz) < 0) {
@@ -547,10 +545,8 @@ sev_launch_start(SevGuestState *sev)
         goto out;
     }
 
-    object_property_set_int(OBJECT(sev), start->handle, "handle",
-                            &error_abort);
     sev_set_guest_state(sev, SEV_STATE_LAUNCH_UPDATE);
-    s->handle = start->handle;
+    sev->handle = start->handle;
     ret = 0;
 
 out:
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 09/18] target/i386: sev: Unify SEVState and SevGuestState
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (7 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 08/18] target/i386: sev: Remove redundant handle field David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 10/18] guest memory protection: Add guest memory protection interface David Gibson
                   ` (8 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

SEVState is contained with SevGuestState.  We've now fixed redundancies
and name conflicts, so there's no real point to the nested structure.  Just
move all the fields of SEVState into SevGuestState.

This eliminates the SEVState structure, which as a bonus removes the
confusion with the SevState enum.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/i386/sev.c | 79 ++++++++++++++++++++---------------------------
 1 file changed, 34 insertions(+), 45 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 11034ed356..d7c2032989 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -35,18 +35,6 @@
 
 typedef struct SevGuestState SevGuestState;
 
-struct SEVState {
-    uint8_t api_major;
-    uint8_t api_minor;
-    uint8_t build_id;
-    uint64_t me_mask;
-    int sev_fd;
-    SevState state;
-    gchar *measurement;
-};
-
-typedef struct SEVState SEVState;
-
 /**
  * SevGuestState:
  *
@@ -70,7 +58,13 @@ struct SevGuestState {
 
     /* runtime state */
     uint32_t handle;
-    SEVState state;
+    uint8_t api_major;
+    uint8_t api_minor;
+    uint8_t build_id;
+    uint64_t me_mask;
+    int sev_fd;
+    SevState state;
+    gchar *measurement;
 };
 
 #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
@@ -158,7 +152,7 @@ static bool
 sev_check_state(const SevGuestState *sev, SevState state)
 {
     assert(sev);
-    return sev->state.state == state ? true : false;
+    return sev->state == state ? true : false;
 }
 
 static void
@@ -167,9 +161,9 @@ sev_set_guest_state(SevGuestState *sev, SevState new_state)
     assert(new_state < SEV_STATE__MAX);
     assert(sev);
 
-    trace_kvm_sev_change_state(SevState_str(sev->state.state),
+    trace_kvm_sev_change_state(SevState_str(sev->state),
                                SevState_str(new_state));
-    sev->state.state = new_state;
+    sev->state = new_state;
 }
 
 static void
@@ -371,7 +365,7 @@ sev_enabled(void)
 uint64_t
 sev_get_me_mask(void)
 {
-    return sev_guest ? sev_guest->state.me_mask : ~0;
+    return sev_guest ? sev_guest->me_mask : ~0;
 }
 
 uint32_t
@@ -395,11 +389,11 @@ sev_get_info(void)
     info->enabled = sev_enabled();
 
     if (info->enabled) {
-        info->api_major = sev_guest->state.api_major;
-        info->api_minor = sev_guest->state.api_minor;
-        info->build_id = sev_guest->state.build_id;
+        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.state;
+        info->state = sev_guest->state;
         info->handle = sev_guest->handle;
     }
 
@@ -510,7 +504,6 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len)
 static int
 sev_launch_start(SevGuestState *sev)
 {
-    SEVState *s = &sev->state;
     gsize sz;
     int ret = 1;
     int fw_error, rc;
@@ -538,7 +531,7 @@ sev_launch_start(SevGuestState *sev)
     }
 
     trace_kvm_sev_launch_start(start->policy, session, dh_cert);
-    rc = sev_ioctl(s->sev_fd, KVM_SEV_LAUNCH_START, start, &fw_error);
+    rc = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_START, start, &fw_error);
     if (rc < 0) {
         error_report("%s: LAUNCH_START ret=%d fw_error=%d '%s'",
                 __func__, ret, fw_error, fw_error_to_str(fw_error));
@@ -569,7 +562,7 @@ sev_launch_update_data(SevGuestState *sev, uint8_t *addr, uint64_t len)
     update.uaddr = (__u64)(unsigned long)addr;
     update.len = len;
     trace_kvm_sev_launch_update_data(addr, len);
-    ret = sev_ioctl(sev->state.sev_fd, KVM_SEV_LAUNCH_UPDATE_DATA,
+    ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_UPDATE_DATA,
                     &update, &fw_error);
     if (ret) {
         error_report("%s: LAUNCH_UPDATE ret=%d fw_error=%d '%s'",
@@ -585,7 +578,6 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
     SevGuestState *sev = sev_guest;
     int ret, error;
     guchar *data;
-    SEVState *s = &sev->state;
     struct kvm_sev_launch_measure *measurement;
 
     if (!sev_check_state(sev, SEV_STATE_LAUNCH_UPDATE)) {
@@ -595,7 +587,7 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
     measurement = g_new0(struct kvm_sev_launch_measure, 1);
 
     /* query the measurement blob length */
-    ret = sev_ioctl(sev->state.sev_fd, KVM_SEV_LAUNCH_MEASURE,
+    ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_MEASURE,
                     measurement, &error);
     if (!measurement->len) {
         error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'",
@@ -607,7 +599,7 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
     measurement->uaddr = (unsigned long)data;
 
     /* get the measurement blob */
-    ret = sev_ioctl(sev->state.sev_fd, KVM_SEV_LAUNCH_MEASURE,
+    ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_MEASURE,
                     measurement, &error);
     if (ret) {
         error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'",
@@ -618,8 +610,8 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
     sev_set_guest_state(sev, SEV_STATE_LAUNCH_SECRET);
 
     /* encode the measurement value and emit the event */
-    s->measurement = g_base64_encode(data, measurement->len);
-    trace_kvm_sev_launch_measurement(s->measurement);
+    sev->measurement = g_base64_encode(data, measurement->len);
+    trace_kvm_sev_launch_measurement(sev->measurement);
 
 free_data:
     g_free(data);
@@ -631,8 +623,8 @@ char *
 sev_get_launch_measurement(void)
 {
     if (sev_guest &&
-        sev_guest->state.state >= SEV_STATE_LAUNCH_SECRET) {
-        return g_strdup(sev_guest->state.measurement);
+        sev_guest->state >= SEV_STATE_LAUNCH_SECRET) {
+        return g_strdup(sev_guest->measurement);
     }
 
     return NULL;
@@ -645,12 +637,11 @@ static Notifier sev_machine_done_notify = {
 static void
 sev_launch_finish(SevGuestState *sev)
 {
-    SEVState *s = &sev->state;
     int ret, error;
     Error *local_err = NULL;
 
     trace_kvm_sev_launch_finish();
-    ret = sev_ioctl(s->sev_fd, KVM_SEV_LAUNCH_FINISH, 0, &error);
+    ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_FINISH, 0, &error);
     if (ret) {
         error_report("%s: LAUNCH_FINISH ret=%d fw_error=%d '%s'",
                      __func__, ret, error, fw_error_to_str(error));
@@ -686,7 +677,6 @@ void *
 sev_guest_init(const char *id)
 {
     SevGuestState *sev;
-    SEVState *s;
     char *devname;
     int ret, fw_error;
     uint32_t ebx;
@@ -701,8 +691,7 @@ sev_guest_init(const char *id)
     }
 
     sev_guest = sev;
-    s = &sev->state;
-    s->state = SEV_STATE_UNINIT;
+    sev->state = SEV_STATE_UNINIT;
 
     host_cpuid(0x8000001F, 0, NULL, &ebx, NULL, NULL);
     host_cbitpos = ebx & 0x3f;
@@ -719,20 +708,20 @@ sev_guest_init(const char *id)
         goto err;
     }
 
-    s->me_mask = ~(1UL << sev->cbitpos);
+    sev->me_mask = ~(1UL << sev->cbitpos);
 
     devname = object_property_get_str(OBJECT(sev), "sev-device", NULL);
-    s->sev_fd = open(devname, O_RDWR);
-    if (s->sev_fd < 0) {
+    sev->sev_fd = open(devname, O_RDWR);
+    if (sev->sev_fd < 0) {
         error_report("%s: Failed to open %s '%s'", __func__,
                      devname, strerror(errno));
     }
     g_free(devname);
-    if (s->sev_fd < 0) {
+    if (sev->sev_fd < 0) {
         goto err;
     }
 
-    ret = sev_platform_ioctl(s->sev_fd, SEV_PLATFORM_STATUS, &status,
+    ret = sev_platform_ioctl(sev->sev_fd, SEV_PLATFORM_STATUS, &status,
                              &fw_error);
     if (ret) {
         error_report("%s: failed to get platform status ret=%d "
@@ -740,12 +729,12 @@ sev_guest_init(const char *id)
                      fw_error_to_str(fw_error));
         goto err;
     }
-    s->build_id = status.build;
-    s->api_major = status.api_major;
-    s->api_minor = status.api_minor;
+    sev->build_id = status.build;
+    sev->api_major = status.api_major;
+    sev->api_minor = status.api_minor;
 
     trace_kvm_sev_init();
-    ret = sev_ioctl(s->sev_fd, KVM_SEV_INIT, NULL, &fw_error);
+    ret = sev_ioctl(sev->sev_fd, KVM_SEV_INIT, NULL, &fw_error);
     if (ret) {
         error_report("%s: failed to initialize ret=%d fw_error=%d '%s'",
                      __func__, ret, fw_error, fw_error_to_str(fw_error));
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 10/18] guest memory protection: Add guest memory protection interface
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (8 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 09/18] target/i386: sev: Unify SEVState and SevGuestState David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 11/18] guest memory protection: Handle memory encrption via interface David Gibson
                   ` (7 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

Several architectures have mechanisms which are designed to protect guest
memory from interference or eavesdropping by a compromised hypervisor.  AMD
SEV does this with in-chip memory encryption and Intel has a similar
mechanism.  POWER's Protected Execution Framework (PEF) accomplishes a
similar goal using an ultravisor and new memory protection features,
instead of encryption.

This introduces a new GuestMemoryProtection QOM interface which we'll use
to (partially) unify handling of these various mechanisms.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 backends/Makefile.objs                 |  2 ++
 backends/guest-memory-protection.c     | 29 +++++++++++++++++++++
 include/exec/guest-memory-protection.h | 36 ++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)
 create mode 100644 backends/guest-memory-protection.c
 create mode 100644 include/exec/guest-memory-protection.h

diff --git a/backends/Makefile.objs b/backends/Makefile.objs
index 28a847cd57..e4fb4f5280 100644
--- a/backends/Makefile.objs
+++ b/backends/Makefile.objs
@@ -21,3 +21,5 @@ common-obj-$(CONFIG_LINUX) += hostmem-memfd.o
 common-obj-$(CONFIG_GIO) += dbus-vmstate.o
 dbus-vmstate.o-cflags = $(GIO_CFLAGS)
 dbus-vmstate.o-libs = $(GIO_LIBS)
+
+common-obj-y += guest-memory-protection.o
diff --git a/backends/guest-memory-protection.c b/backends/guest-memory-protection.c
new file mode 100644
index 0000000000..7e538214f7
--- /dev/null
+++ b/backends/guest-memory-protection.c
@@ -0,0 +1,29 @@
+#/*
+ * QEMU Guest Memory Protection interface
+ *
+ * Copyright: David Gibson, Red Hat Inc. 2020
+ *
+ * Authors:
+ *  David Gibson <david@gibson.dropbear.id.au>
+ *
+ * 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 "exec/guest-memory-protection.h"
+
+static const TypeInfo guest_memory_protection_info = {
+    .name = TYPE_GUEST_MEMORY_PROTECTION,
+    .parent = TYPE_INTERFACE,
+    .class_size = sizeof(GuestMemoryProtectionClass),
+};
+
+static void guest_memory_protection_register_types(void)
+{
+    type_register_static(&guest_memory_protection_info);
+}
+
+type_init(guest_memory_protection_register_types)
diff --git a/include/exec/guest-memory-protection.h b/include/exec/guest-memory-protection.h
new file mode 100644
index 0000000000..38e9b01667
--- /dev/null
+++ b/include/exec/guest-memory-protection.h
@@ -0,0 +1,36 @@
+#/*
+ * QEMU Guest Memory Protection interface
+ *
+ * Copyright: David Gibson, Red Hat Inc. 2020
+ *
+ * Authors:
+ *  David Gibson <david@gibson.dropbear.id.au>
+ *
+ * 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 QEMU_GUEST_MEMORY_PROTECTION_H
+#define QEMU_GUEST_MEMORY_PROTECTION_H
+
+#include "qom/object.h"
+
+typedef struct GuestMemoryProtection GuestMemoryProtection;
+
+#define TYPE_GUEST_MEMORY_PROTECTION "guest-memory-protection"
+#define GUEST_MEMORY_PROTECTION(obj)                                    \
+    INTERFACE_CHECK(GuestMemoryProtection, (obj),                       \
+                    TYPE_GUEST_MEMORY_PROTECTION)
+#define GUEST_MEMORY_PROTECTION_CLASS(klass)                            \
+    OBJECT_CLASS_CHECK(GuestMemoryProtectionClass, (klass),             \
+                       TYPE_GUEST_MEMORY_PROTECTION)
+#define GUEST_MEMORY_PROTECTION_GET_CLASS(obj)                          \
+    OBJECT_GET_CLASS(GuestMemoryProtectionClass, (obj),                 \
+                     TYPE_GUEST_MEMORY_PROTECTION)
+
+typedef struct GuestMemoryProtectionClass {
+    InterfaceClass parent;
+} GuestMemoryProtectionClass;
+
+#endif /* QEMU_GUEST_MEMORY_PROTECTION_H */
+
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 11/18] guest memory protection: Handle memory encrption via interface
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (9 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 10/18] guest memory protection: Add guest memory protection interface David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 12/18] guest memory protection: Perform KVM init " David Gibson
                   ` (6 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

At the moment AMD SEV sets a special function pointer, plus an opaque
handle in KVMState to let things know how to encrypt guest memory.

Now that we have a QOM interface for handling things related to guest
memory protection, use a QOM method on that interface, rather than a bare
function pointer for this.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 accel/kvm/kvm-all.c                    | 23 +++----
 accel/kvm/sev-stub.c                   |  5 --
 include/exec/guest-memory-protection.h |  2 +
 include/sysemu/sev.h                   |  6 +-
 target/i386/sev.c                      | 90 ++++++++++++++------------
 5 files changed, 66 insertions(+), 60 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 439a4efe52..47d7142aa1 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -44,6 +44,7 @@
 #include "qapi/visitor.h"
 #include "qapi/qapi-types-common.h"
 #include "qapi/qapi-visit-common.h"
+#include "exec/guest-memory-protection.h"
 
 #include "hw/boards.h"
 
@@ -118,8 +119,7 @@ struct KVMState
     QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus;
 
     /* memory encryption */
-    void *memcrypt_handle;
-    int (*memcrypt_encrypt_data)(void *handle, uint8_t *ptr, uint64_t len);
+    GuestMemoryProtection *guest_memory_protection;
 
     /* For "info mtree -f" to tell if an MR is registered in KVM */
     int nr_as;
@@ -171,7 +171,7 @@ int kvm_get_max_memslots(void)
 
 bool kvm_memcrypt_enabled(void)
 {
-    if (kvm_state && kvm_state->memcrypt_handle) {
+    if (kvm_state && kvm_state->guest_memory_protection) {
         return true;
     }
 
@@ -180,10 +180,13 @@ bool kvm_memcrypt_enabled(void)
 
 int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)
 {
-    if (kvm_state->memcrypt_handle &&
-        kvm_state->memcrypt_encrypt_data) {
-        return kvm_state->memcrypt_encrypt_data(kvm_state->memcrypt_handle,
-                                              ptr, len);
+    GuestMemoryProtection *gmpo = kvm_state->guest_memory_protection;
+
+    if (gmpo) {
+        GuestMemoryProtectionClass *gmpc =
+            GUEST_MEMORY_PROTECTION_GET_CLASS(gmpo);
+
+        return gmpc->encrypt_data(gmpo, ptr, len);
     }
 
     return 1;
@@ -2067,13 +2070,11 @@ static int kvm_init(MachineState *ms)
      * encryption context.
      */
     if (ms->memory_encryption) {
-        kvm_state->memcrypt_handle = sev_guest_init(ms->memory_encryption);
-        if (!kvm_state->memcrypt_handle) {
+        kvm_state->guest_memory_protection = sev_guest_init(ms->memory_encryption);
+        if (!kvm_state->guest_memory_protection) {
             ret = -1;
             goto err;
         }
-
-        kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
     }
 
     ret = kvm_arch_init(ms, s);
diff --git a/accel/kvm/sev-stub.c b/accel/kvm/sev-stub.c
index 4f97452585..4a5cc5569e 100644
--- a/accel/kvm/sev-stub.c
+++ b/accel/kvm/sev-stub.c
@@ -15,11 +15,6 @@
 #include "qemu-common.h"
 #include "sysemu/sev.h"
 
-int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
-{
-    abort();
-}
-
 void *sev_guest_init(const char *id)
 {
     return NULL;
diff --git a/include/exec/guest-memory-protection.h b/include/exec/guest-memory-protection.h
index 38e9b01667..eb712a5804 100644
--- a/include/exec/guest-memory-protection.h
+++ b/include/exec/guest-memory-protection.h
@@ -30,6 +30,8 @@ typedef struct GuestMemoryProtection GuestMemoryProtection;
 
 typedef struct GuestMemoryProtectionClass {
     InterfaceClass parent;
+
+    int (*encrypt_data)(GuestMemoryProtection *, uint8_t *, uint64_t);
 } GuestMemoryProtectionClass;
 
 #endif /* QEMU_GUEST_MEMORY_PROTECTION_H */
diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 98c1ec8d38..7735a7942e 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -16,6 +16,8 @@
 
 #include "sysemu/kvm.h"
 
-void *sev_guest_init(const char *id);
-int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
+typedef struct GuestMemoryProtection GuestMemoryProtection;
+
+GuestMemoryProtection *sev_guest_init(const char *id);
+
 #endif
diff --git a/target/i386/sev.c b/target/i386/sev.c
index d7c2032989..d9c17af514 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -28,6 +28,7 @@
 #include "sysemu/runstate.h"
 #include "trace.h"
 #include "migration/blocker.h"
+#include "exec/guest-memory-protection.h"
 
 #define TYPE_SEV_GUEST "sev-guest"
 #define SEV_GUEST(obj)                                          \
@@ -281,29 +282,6 @@ sev_guest_set_sev_device(Object *obj, const char *value, Error **errp)
     sev->sev_device = g_strdup(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,
-                                  NULL);
-    object_class_property_set_description(oc, "sev-device",
-            "SEV device to use", NULL);
-    object_class_property_add_str(oc, "dh-cert-file",
-                                  sev_guest_get_dh_cert_file,
-                                  sev_guest_set_dh_cert_file,
-                                  NULL);
-    object_class_property_set_description(oc, "dh-cert-file",
-            "guest owners DH certificate (encoded with base64)", NULL);
-    object_class_property_add_str(oc, "session-file",
-                                  sev_guest_get_session_file,
-                                  sev_guest_set_session_file,
-                                  NULL);
-    object_class_property_set_description(oc, "session-file",
-            "guest owners session parameters (encoded with base64)", NULL);
-}
-
 static void
 sev_guest_instance_init(Object *obj)
 {
@@ -322,20 +300,6 @@ sev_guest_instance_init(Object *obj)
                                    OBJ_PROP_FLAG_READWRITE, NULL);
 }
 
-/* sev guest info */
-static const TypeInfo sev_guest_info = {
-    .parent = TYPE_OBJECT,
-    .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 SevGuestState *
 lookup_sev_guest_info(const char *id)
 {
@@ -673,7 +637,7 @@ sev_vm_state_change(void *opaque, int running, RunState state)
     }
 }
 
-void *
+GuestMemoryProtection *
 sev_guest_init(const char *id)
 {
     SevGuestState *sev;
@@ -751,16 +715,16 @@ sev_guest_init(const char *id)
     qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
     qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
 
-    return sev;
+    return GUEST_MEMORY_PROTECTION(sev);
 err:
     sev_guest = NULL;
     return NULL;
 }
 
-int
-sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
+static int
+sev_encrypt_data(GuestMemoryProtection *opaque, uint8_t *ptr, uint64_t len)
 {
-    SevGuestState *sev = handle;
+    SevGuestState *sev = SEV_GUEST(opaque);
 
     assert(sev);
 
@@ -772,6 +736,48 @@ sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
     return 0;
 }
 
+static void
+sev_guest_class_init(ObjectClass *oc, void *data)
+{
+    GuestMemoryProtectionClass *gmpc = GUEST_MEMORY_PROTECTION_CLASS(oc);
+
+    object_class_property_add_str(oc, "sev-device",
+                                  sev_guest_get_sev_device,
+                                  sev_guest_set_sev_device,
+                                  NULL);
+    object_class_property_set_description(oc, "sev-device",
+            "SEV device to use", NULL);
+    object_class_property_add_str(oc, "dh-cert-file",
+                                  sev_guest_get_dh_cert_file,
+                                  sev_guest_set_dh_cert_file,
+                                  NULL);
+    object_class_property_set_description(oc, "dh-cert-file",
+            "guest owners DH certificate (encoded with base64)", NULL);
+    object_class_property_add_str(oc, "session-file",
+                                  sev_guest_get_session_file,
+                                  sev_guest_set_session_file,
+                                  NULL);
+    object_class_property_set_description(oc, "session-file",
+            "guest owners session parameters (encoded with base64)", NULL);
+
+    gmpc->encrypt_data = sev_encrypt_data;
+}
+
+/* sev guest info */
+static const TypeInfo sev_guest_info = {
+    .parent = TYPE_OBJECT,
+    .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_GUEST_MEMORY_PROTECTION },
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
 static void
 sev_register_types(void)
 {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 12/18] guest memory protection: Perform KVM init via interface
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (10 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 11/18] guest memory protection: Handle memory encrption via interface David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 13/18] guest memory protection: Move side effect out of machine_set_memory_encryption() David Gibson
                   ` (5 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

Currently the "memory-encryption" machine option is notionally generic,
but in fact is only used for AMD SEV setups.  Make another step towards it
being actually generic, but having using the GuestMemoryProtection QOM
interface to dispatch the initial setup, rather than directly calling
sev_guest_init() from kvm_init().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 accel/kvm/kvm-all.c                    | 18 ++++++++++---
 include/exec/guest-memory-protection.h |  1 +
 target/i386/sev.c                      | 37 ++++----------------------
 3 files changed, 21 insertions(+), 35 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 47d7142aa1..9b4863aced 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -39,7 +39,6 @@
 #include "qemu/main-loop.h"
 #include "trace.h"
 #include "hw/irq.h"
-#include "sysemu/sev.h"
 #include "sysemu/balloon.h"
 #include "qapi/visitor.h"
 #include "qapi/qapi-types-common.h"
@@ -2070,8 +2069,21 @@ static int kvm_init(MachineState *ms)
      * encryption context.
      */
     if (ms->memory_encryption) {
-        kvm_state->guest_memory_protection = sev_guest_init(ms->memory_encryption);
-        if (!kvm_state->guest_memory_protection) {
+        Object *obj = object_resolve_path_component(object_get_objects_root(),
+                                                    ms->memory_encryption);
+
+        if (object_dynamic_cast(obj, TYPE_GUEST_MEMORY_PROTECTION)) {
+            GuestMemoryProtection *gmpo = GUEST_MEMORY_PROTECTION(obj);
+            GuestMemoryProtectionClass *gmpc =
+                GUEST_MEMORY_PROTECTION_GET_CLASS(gmpo);
+
+            ret = gmpc->kvm_init(gmpo);
+            if (ret < 0) {
+                goto err;
+            }
+
+            kvm_state->guest_memory_protection = gmpo;
+        } else {
             ret = -1;
             goto err;
         }
diff --git a/include/exec/guest-memory-protection.h b/include/exec/guest-memory-protection.h
index eb712a5804..3707b96515 100644
--- a/include/exec/guest-memory-protection.h
+++ b/include/exec/guest-memory-protection.h
@@ -31,6 +31,7 @@ typedef struct GuestMemoryProtection GuestMemoryProtection;
 typedef struct GuestMemoryProtectionClass {
     InterfaceClass parent;
 
+    int (*kvm_init)(GuestMemoryProtection *);
     int (*encrypt_data)(GuestMemoryProtection *, uint8_t *, uint64_t);
 } GuestMemoryProtectionClass;
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index d9c17af514..2051fae0c1 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -300,26 +300,6 @@ sev_guest_instance_init(Object *obj)
                                    OBJ_PROP_FLAG_READWRITE, NULL);
 }
 
-static SevGuestState *
-lookup_sev_guest_info(const char *id)
-{
-    Object *obj;
-    SevGuestState *info;
-
-    obj = object_resolve_path_component(object_get_objects_root(), id);
-    if (!obj) {
-        return NULL;
-    }
-
-    info = (SevGuestState *)
-            object_dynamic_cast(obj, TYPE_SEV_GUEST);
-    if (!info) {
-        return NULL;
-    }
-
-    return info;
-}
-
 bool
 sev_enabled(void)
 {
@@ -637,23 +617,15 @@ sev_vm_state_change(void *opaque, int running, RunState state)
     }
 }
 
-GuestMemoryProtection *
-sev_guest_init(const char *id)
+static int sev_kvm_init(GuestMemoryProtection *gmpo)
 {
-    SevGuestState *sev;
+    SevGuestState *sev = SEV_GUEST(gmpo);
     char *devname;
     int ret, fw_error;
     uint32_t ebx;
     uint32_t host_cbitpos;
     struct sev_user_data_status status = {};
 
-    sev = lookup_sev_guest_info(id);
-    if (!sev) {
-        error_report("%s: '%s' is not a valid '%s' object",
-                     __func__, id, TYPE_SEV_GUEST);
-        goto err;
-    }
-
     sev_guest = sev;
     sev->state = SEV_STATE_UNINIT;
 
@@ -715,10 +687,10 @@ sev_guest_init(const char *id)
     qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
     qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
 
-    return GUEST_MEMORY_PROTECTION(sev);
+    return 0;
 err:
     sev_guest = NULL;
-    return NULL;
+    return -1;
 }
 
 static int
@@ -760,6 +732,7 @@ sev_guest_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "session-file",
             "guest owners session parameters (encoded with base64)", NULL);
 
+    gmpc->kvm_init = sev_kvm_init;
     gmpc->encrypt_data = sev_encrypt_data;
 }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 13/18] guest memory protection: Move side effect out of machine_set_memory_encryption()
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (11 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 12/18] guest memory protection: Perform KVM init " David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 14/18] guest memory protection: Rework the "memory-encryption" property David Gibson
                   ` (4 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

When the "memory-encryption" property is set, we also disable KSM
merging for the guest, since it won't accomplish anything.

We want that, but doing it in the property set function itself is
thereoretically incorrect, in the unlikely event of some configuration
environment that set the property then cleared it again before
constructing the guest.

But more important, it makes some other cleanups we want more
difficult.  So, instead move this logic to machine_run_board_init()
conditional on the final value of the property.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/core/machine.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 7a50dd518f..a50ba82d74 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -429,14 +429,6 @@ static void machine_set_memory_encryption(Object *obj, const char *value,
 
     g_free(ms->memory_encryption);
     ms->memory_encryption = g_strdup(value);
-
-    /*
-     * With memory encryption, the host can't see the real contents of RAM,
-     * so there's no point in it trying to merge areas.
-     */
-    if (value) {
-        machine_set_mem_merge(obj, false, errp);
-    }
 }
 
 static bool machine_get_nvdimm(Object *obj, Error **errp)
@@ -1140,6 +1132,15 @@ void machine_run_board_init(MachineState *machine)
         }
     }
 
+    if (machine->memory_encryption) {
+        /*
+         * With guest memory protection, the host can't see the real
+         * contents of RAM, so there's no point in it trying to merge
+         * areas.
+         */
+        machine_set_mem_merge(OBJECT(machine), false, &error_abort);
+    }
+
     machine_class->init(machine);
 }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 14/18] guest memory protection: Rework the "memory-encryption" property
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (12 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 13/18] guest memory protection: Move side effect out of machine_set_memory_encryption() David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 15/18] guest memory protection: Decouple kvm_memcrypt_*() helpers from KVM David Gibson
                   ` (3 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

Currently the "memory-encryption" property is only looked at once we get to
kvm_init().  Although protection of guest memory from the hypervisor isn't
something that could really ever work with TCG, it's not conceptually tied
to the KVM accelerator.

In addition, the way the string property is resolved to an object is
almost identical to how a QOM link property is handled.

So, create a new "guest-memory-protection" link property which sets
this QOM interface link directly in the machine.  For compatibility we
keep the "memory-encryption" property, but now implemented in terms of
the new property.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 accel/kvm/kvm-all.c | 23 +++++++----------------
 hw/core/machine.c   | 41 ++++++++++++++++++++++++++++++++++++-----
 include/hw/boards.h |  4 +++-
 3 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 9b4863aced..60c4fe326b 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2068,25 +2068,16 @@ static int kvm_init(MachineState *ms)
      * if memory encryption object is specified then initialize the memory
      * encryption context.
      */
-    if (ms->memory_encryption) {
-        Object *obj = object_resolve_path_component(object_get_objects_root(),
-                                                    ms->memory_encryption);
-
-        if (object_dynamic_cast(obj, TYPE_GUEST_MEMORY_PROTECTION)) {
-            GuestMemoryProtection *gmpo = GUEST_MEMORY_PROTECTION(obj);
-            GuestMemoryProtectionClass *gmpc =
-                GUEST_MEMORY_PROTECTION_GET_CLASS(gmpo);
-
-            ret = gmpc->kvm_init(gmpo);
-            if (ret < 0) {
-                goto err;
-            }
+    if (ms->gmpo) {
+        GuestMemoryProtectionClass *gmpc =
+            GUEST_MEMORY_PROTECTION_GET_CLASS(ms->gmpo);
 
-            kvm_state->guest_memory_protection = gmpo;
-        } else {
-            ret = -1;
+        ret = gmpc->kvm_init(ms->gmpo);
+        if (ret < 0) {
             goto err;
         }
+
+        kvm_state->guest_memory_protection = ms->gmpo;
     }
 
     ret = kvm_arch_init(ms, s);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index a50ba82d74..37d9f7f85c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -27,6 +27,7 @@
 #include "hw/pci/pci.h"
 #include "hw/mem/nvdimm.h"
 #include "migration/vmstate.h"
+#include "exec/guest-memory-protection.h"
 
 GlobalProperty hw_compat_5_0[] = {};
 const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
@@ -419,16 +420,37 @@ static char *machine_get_memory_encryption(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
 
-    return g_strdup(ms->memory_encryption);
+    if (ms->gmpo) {
+        return object_get_canonical_path_component(OBJECT(ms->gmpo));
+    }
+
+    return NULL;
 }
 
 static void machine_set_memory_encryption(Object *obj, const char *value,
                                         Error **errp)
 {
-    MachineState *ms = MACHINE(obj);
+    Object *gmpo =
+        object_resolve_path_component(object_get_objects_root(), value);
+
+    if (!gmpo) {
+        error_setg(errp, "No such memory encryption object '%s'", value);
+        return;
+    }
 
-    g_free(ms->memory_encryption);
-    ms->memory_encryption = g_strdup(value);
+    object_property_set_link(obj, gmpo, "guest-memory-protection", errp);
+}
+
+static void machine_check_guest_memory_protection(const Object *obj,
+                                                  const char *name,
+                                                  Object *new_target,
+                                                  Error **errp)
+{
+    /*
+     * So far the only constraint is that the target has the
+     * TYPE_GUEST_MEMORY_PROTECTION interface, and that's checked by
+     * the QOM core
+     */
 }
 
 static bool machine_get_nvdimm(Object *obj, Error **errp)
@@ -853,6 +875,15 @@ static void machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "enforce-config-section",
         "Set on to enforce configuration section migration", &error_abort);
 
+    object_class_property_add_link(oc, "guest-memory-protection",
+                                   TYPE_GUEST_MEMORY_PROTECTION,
+                                   offsetof(MachineState, gmpo),
+                                   machine_check_guest_memory_protection,
+                                   OBJ_PROP_LINK_STRONG, &error_abort);
+    object_class_property_set_description(oc, "guest-memory-protection",
+        "Set guest memory protection object to use", &error_abort);
+
+    /* For compatibility */
     object_class_property_add_str(oc, "memory-encryption",
         machine_get_memory_encryption, machine_set_memory_encryption,
         &error_abort);
@@ -1132,7 +1163,7 @@ void machine_run_board_init(MachineState *machine)
         }
     }
 
-    if (machine->memory_encryption) {
+    if (machine->gmpo) {
         /*
          * With guest memory protection, the host can't see the real
          * contents of RAM, so there's no point in it trying to merge
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 18815d9be2..19bf2c38fc 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -12,6 +12,8 @@
 #include "qom/object.h"
 #include "hw/core/cpu.h"
 
+typedef struct GuestMemoryProtection GuestMemoryProtection;
+
 #define TYPE_MACHINE_SUFFIX "-machine"
 
 /* Machine class name that needs to be used for class-name-based machine
@@ -277,7 +279,7 @@ struct MachineState {
     bool suppress_vmdesc;
     bool enforce_config_section;
     bool enable_graphics;
-    char *memory_encryption;
+    GuestMemoryProtection *gmpo;
     char *ram_memdev_id;
     /*
      * convenience alias to ram_memdev_id backend memory region
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 15/18] guest memory protection: Decouple kvm_memcrypt_*() helpers from KVM
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (13 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 14/18] guest memory protection: Rework the "memory-encryption" property David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 16/18] use errp for gmpo kvm_init David Gibson
                   ` (2 subsequent siblings)
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

The kvm_memcrypt_enabled() and kvm_memcrypt_encrypt_data() helper functions
don't conceptually have any connection to KVM (although it's not possible
in practice to use them without it).

They also rely on looking at the global KVMState.  But the same information
is available from the machine, and the only existing callers have natural
access to the machine state.

Therefore, move and rename them to helpers in guest-memory-protection.h,
taking an explicit machine parameter.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 accel/kvm/kvm-all.c                    | 28 -------------------
 accel/stubs/kvm-stub.c                 | 10 -------
 hw/i386/pc_sysfw.c                     |  6 ++--
 include/exec/guest-memory-protection.h | 38 ++++++++++++++++++++++++++
 include/sysemu/kvm.h                   | 17 ------------
 5 files changed, 42 insertions(+), 57 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 60c4fe326b..5451728425 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -117,9 +117,6 @@ struct KVMState
     KVMMemoryListener memory_listener;
     QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus;
 
-    /* memory encryption */
-    GuestMemoryProtection *guest_memory_protection;
-
     /* For "info mtree -f" to tell if an MR is registered in KVM */
     int nr_as;
     struct KVMAs {
@@ -168,29 +165,6 @@ int kvm_get_max_memslots(void)
     return s->nr_slots;
 }
 
-bool kvm_memcrypt_enabled(void)
-{
-    if (kvm_state && kvm_state->guest_memory_protection) {
-        return true;
-    }
-
-    return false;
-}
-
-int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)
-{
-    GuestMemoryProtection *gmpo = kvm_state->guest_memory_protection;
-
-    if (gmpo) {
-        GuestMemoryProtectionClass *gmpc =
-            GUEST_MEMORY_PROTECTION_GET_CLASS(gmpo);
-
-        return gmpc->encrypt_data(gmpo, ptr, len);
-    }
-
-    return 1;
-}
-
 /* Called with KVMMemoryListener.slots_lock held */
 static KVMSlot *kvm_get_free_slot(KVMMemoryListener *kml)
 {
@@ -2076,8 +2050,6 @@ static int kvm_init(MachineState *ms)
         if (ret < 0) {
             goto err;
         }
-
-        kvm_state->guest_memory_protection = ms->gmpo;
     }
 
     ret = kvm_arch_init(ms, s);
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 82f118d2df..78b3eef117 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -104,16 +104,6 @@ int kvm_on_sigbus(int code, void *addr)
     return 1;
 }
 
-bool kvm_memcrypt_enabled(void)
-{
-    return false;
-}
-
-int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)
-{
-  return 1;
-}
-
 #ifndef CONFIG_USER_ONLY
 int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
 {
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index f5f3f466b0..4b015085f6 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -38,6 +38,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/block/flash.h"
 #include "sysemu/kvm.h"
+#include "exec/guest-memory-protection.h"
 
 /*
  * We don't have a theoretically justifiable exact lower bound on the base
@@ -197,10 +198,11 @@ static void pc_system_flash_map(PCMachineState *pcms,
             pc_isa_bios_init(rom_memory, flash_mem, size);
 
             /* Encrypt the pflash boot ROM */
-            if (kvm_memcrypt_enabled()) {
+            if (guest_memory_protection_enabled(MACHINE(pcms))) {
                 flash_ptr = memory_region_get_ram_ptr(flash_mem);
                 flash_size = memory_region_size(flash_mem);
-                ret = kvm_memcrypt_encrypt_data(flash_ptr, flash_size);
+                ret = guest_memory_protection_encrypt(MACHINE(pcms),
+                                                      flash_ptr, flash_size);
                 if (ret) {
                     error_report("failed to encrypt pflash rom");
                     exit(1);
diff --git a/include/exec/guest-memory-protection.h b/include/exec/guest-memory-protection.h
index 3707b96515..7d959b4910 100644
--- a/include/exec/guest-memory-protection.h
+++ b/include/exec/guest-memory-protection.h
@@ -14,6 +14,7 @@
 #define QEMU_GUEST_MEMORY_PROTECTION_H
 
 #include "qom/object.h"
+#include "hw/boards.h"
 
 typedef struct GuestMemoryProtection GuestMemoryProtection;
 
@@ -35,5 +36,42 @@ typedef struct GuestMemoryProtectionClass {
     int (*encrypt_data)(GuestMemoryProtection *, uint8_t *, uint64_t);
 } GuestMemoryProtectionClass;
 
+/**
+ * guest_memory_protection_enabled - return whether guest memory is
+ *                                   protected from hypervisor access
+ *                                   (with memory encryption or
+ *                                   otherwise)
+ * Returns: true guest memory is not directly accessible to qemu
+ *          false guest memory is directly accessible to qemu
+ */
+static inline bool guest_memory_protection_enabled(MachineState *machine)
+{
+    return !!machine->gmpo;
+}
+
+/**
+ * guest_memory_protection_encrypt: encrypt the memory range to make
+ *                                  it guest accessible
+ *
+ * Return: 1 failed to encrypt the range
+ *         0 succesfully encrypted memory region
+ */
+static inline int guest_memory_protection_encrypt(MachineState *machine,
+                                                  uint8_t *ptr, uint64_t len)
+{
+    GuestMemoryProtection *gmpo = machine->gmpo;
+
+    if (gmpo) {
+        GuestMemoryProtectionClass *gmpc =
+            GUEST_MEMORY_PROTECTION_GET_CLASS(gmpo);
+
+        if (gmpc->encrypt_data) {
+            return gmpc->encrypt_data(gmpo, ptr, len);
+        }
+    }
+
+    return 1;
+}
+
 #endif /* QEMU_GUEST_MEMORY_PROTECTION_H */
 
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 141342de98..656b122f8a 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -231,23 +231,6 @@ int kvm_destroy_vcpu(CPUState *cpu);
  */
 bool kvm_arm_supports_user_irq(void);
 
-/**
- * kvm_memcrypt_enabled - return boolean indicating whether memory encryption
- *                        is enabled
- * Returns: 1 memory encryption is enabled
- *          0 memory encryption is disabled
- */
-bool kvm_memcrypt_enabled(void);
-
-/**
- * kvm_memcrypt_encrypt_data: encrypt the memory range
- *
- * Return: 1 failed to encrypt the range
- *         0 succesfully encrypted memory region
- */
-int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len);
-
-
 #ifdef NEED_CPU_H
 #include "cpu.h"
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 16/18] use errp for gmpo kvm_init
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (14 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 15/18] guest memory protection: Decouple kvm_memcrypt_*() helpers from KVM David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14 17:09   ` Dr. David Alan Gilbert
  2020-05-14  6:41 ` [RFC 17/18] spapr: Added PEF based guest memory protection David Gibson
  2020-05-14  6:41 ` [RFC 18/18] guest memory protection: Alter virtio default properties for protected guests David Gibson
  17 siblings, 1 reply; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

---
 accel/kvm/kvm-all.c                    |  4 +++-
 include/exec/guest-memory-protection.h |  2 +-
 target/i386/sev.c                      | 32 +++++++++++++-------------
 3 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 5451728425..392ab02867 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2045,9 +2045,11 @@ static int kvm_init(MachineState *ms)
     if (ms->gmpo) {
         GuestMemoryProtectionClass *gmpc =
             GUEST_MEMORY_PROTECTION_GET_CLASS(ms->gmpo);
+        Error *local_err = NULL;
 
-        ret = gmpc->kvm_init(ms->gmpo);
+        ret = gmpc->kvm_init(ms->gmpo, &local_err);
         if (ret < 0) {
+            error_report_err(local_err);
             goto err;
         }
     }
diff --git a/include/exec/guest-memory-protection.h b/include/exec/guest-memory-protection.h
index 7d959b4910..2a88475136 100644
--- a/include/exec/guest-memory-protection.h
+++ b/include/exec/guest-memory-protection.h
@@ -32,7 +32,7 @@ typedef struct GuestMemoryProtection GuestMemoryProtection;
 typedef struct GuestMemoryProtectionClass {
     InterfaceClass parent;
 
-    int (*kvm_init)(GuestMemoryProtection *);
+    int (*kvm_init)(GuestMemoryProtection *, Error **);
     int (*encrypt_data)(GuestMemoryProtection *, uint8_t *, uint64_t);
 } GuestMemoryProtectionClass;
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 2051fae0c1..82f16b2f3b 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -617,7 +617,7 @@ sev_vm_state_change(void *opaque, int running, RunState state)
     }
 }
 
-static int sev_kvm_init(GuestMemoryProtection *gmpo)
+static int sev_kvm_init(GuestMemoryProtection *gmpo, Error **errp)
 {
     SevGuestState *sev = SEV_GUEST(gmpo);
     char *devname;
@@ -633,14 +633,14 @@ static int sev_kvm_init(GuestMemoryProtection *gmpo)
     host_cbitpos = ebx & 0x3f;
 
     if (host_cbitpos != sev->cbitpos) {
-        error_report("%s: cbitpos check failed, host '%d' requested '%d'",
-                     __func__, host_cbitpos, sev->cbitpos);
+        error_setg(errp, "%s: cbitpos check failed, host '%d' requested '%d'",
+                   __func__, host_cbitpos, sev->cbitpos);
         goto err;
     }
 
     if (sev->reduced_phys_bits < 1) {
-        error_report("%s: reduced_phys_bits check failed, it should be >=1,"
-                     " requested '%d'", __func__, sev->reduced_phys_bits);
+        error_setg(errp, "%s: reduced_phys_bits check failed, it should be >=1,"
+                   " requested '%d'", __func__, sev->reduced_phys_bits);
         goto err;
     }
 
@@ -649,20 +649,20 @@ static int sev_kvm_init(GuestMemoryProtection *gmpo)
     devname = object_property_get_str(OBJECT(sev), "sev-device", NULL);
     sev->sev_fd = open(devname, O_RDWR);
     if (sev->sev_fd < 0) {
-        error_report("%s: Failed to open %s '%s'", __func__,
-                     devname, strerror(errno));
-    }
-    g_free(devname);
-    if (sev->sev_fd < 0) {
+        g_free(devname);
+        error_setg(errp, "%s: Failed to open %s '%s'", __func__,
+                   devname, strerror(errno));
+        g_free(devname);
         goto err;
     }
+    g_free(devname);
 
     ret = sev_platform_ioctl(sev->sev_fd, SEV_PLATFORM_STATUS, &status,
                              &fw_error);
     if (ret) {
-        error_report("%s: failed to get platform status ret=%d "
-                     "fw_error='%d: %s'", __func__, ret, fw_error,
-                     fw_error_to_str(fw_error));
+        error_setg(errp, "%s: failed to get platform status ret=%d "
+                   "fw_error='%d: %s'", __func__, ret, fw_error,
+                   fw_error_to_str(fw_error));
         goto err;
     }
     sev->build_id = status.build;
@@ -672,14 +672,14 @@ static int sev_kvm_init(GuestMemoryProtection *gmpo)
     trace_kvm_sev_init();
     ret = sev_ioctl(sev->sev_fd, KVM_SEV_INIT, NULL, &fw_error);
     if (ret) {
-        error_report("%s: failed to initialize ret=%d fw_error=%d '%s'",
-                     __func__, ret, fw_error, fw_error_to_str(fw_error));
+        error_setg(errp, "%s: failed to initialize ret=%d fw_error=%d '%s'",
+                   __func__, ret, fw_error, fw_error_to_str(fw_error));
         goto err;
     }
 
     ret = sev_launch_start(sev);
     if (ret) {
-        error_report("%s: failed to create encryption context", __func__);
+        error_setg(errp, "%s: failed to create encryption context", __func__);
         goto err;
     }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 17/18] spapr: Added PEF based guest memory protection
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (15 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 16/18] use errp for gmpo kvm_init David Gibson
@ 2020-05-14  6:41 ` David Gibson
  2020-05-14  6:41 ` [RFC 18/18] guest memory protection: Alter virtio default properties for protected guests David Gibson
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

Some upcoming POWER machines have a system called PEF (Protected
Execution Framework) which uses a small ultravisor to allow guests to
run in a way that they can't be eavesdropped by the hypervisor.  The
effect is roughly similar to AMD SEV, although the mechanisms are
quite different.

Most of the work of this is done between the guest, KVM and the
ultravisor, with little need for involvement by qemu.  However qemu
does need to tell KVM to allow secure VMs.

Because the availability of secure mode is a guest visible difference
which depends on havint the right hardware and firmware, we don't
enable this by default.  In order to run a secure guest you need to
create a "pef-guest" object and set the guest-memory-protection machine property to point to it.

Note that this just *allows* secure guests, the architecture of PEF is
such that the guest still needs to talk to the ultravisor to enter
secure mode, so we can't know if the guest actually is secure until
well after machine creation time.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/Makefile.objs |  2 +-
 target/ppc/pef.c         | 81 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 1 deletion(-)
 create mode 100644 target/ppc/pef.c

diff --git a/target/ppc/Makefile.objs b/target/ppc/Makefile.objs
index e8fa18ce13..ac93b9700e 100644
--- a/target/ppc/Makefile.objs
+++ b/target/ppc/Makefile.objs
@@ -6,7 +6,7 @@ obj-y += machine.o mmu_helper.o mmu-hash32.o monitor.o arch_dump.o
 obj-$(TARGET_PPC64) += mmu-hash64.o mmu-book3s-v3.o compat.o
 obj-$(TARGET_PPC64) += mmu-radix64.o
 endif
-obj-$(CONFIG_KVM) += kvm.o
+obj-$(CONFIG_KVM) += kvm.o pef.o
 obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
 obj-y += dfp_helper.o
 obj-y += excp_helper.o
diff --git a/target/ppc/pef.c b/target/ppc/pef.c
new file mode 100644
index 0000000000..823daf3e9c
--- /dev/null
+++ b/target/ppc/pef.c
@@ -0,0 +1,81 @@
+/*
+ * PEF (Protected Execution Framework) for POWER support
+ *
+ * Copyright David Gibson, Redhat Inc. 2020
+ *
+ * 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"
+
+#define TYPE_PEF_GUEST "pef-guest"
+#define PEF_GUEST(obj)                                  \
+    OBJECT_CHECK(PefGuestState, (obj), TYPE_SEV_GUEST)
+
+typedef struct PefGuestState PefGuestState;
+
+/**
+ * PefGuestState:
+ *
+ * The PefGuestState object is used for creating and managing a PEF
+ * guest.
+ *
+ * # $QEMU \
+ *         -object pef-guest,id=pef0 \
+ *         -machine ...,guest-memory-protection=pef0
+ */
+struct PefGuestState {
+    Object parent_obj;
+};
+
+static Error *pef_mig_blocker;
+
+static int pef_kvm_init(GuestMemoryProtection *gmpo, Error **errp)
+{
+    PefGuestState *pef = PEF_GUEST(gmpo);
+
+    if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_SECURE_GUEST)) {
+        error_setg(errp,
+                   "KVM implementation does not support Secure VMs (is an ultravisor running?)");
+        return -1;
+    } else {
+        int ret = kvm_vm_enable_cap(kvm_state, KVM_CAP_PPC_SECURE_GUEST, 0, 1);
+
+        if (ret < 0) {
+            error_setg(errp,
+                       "Error enabling PEF with KVM");
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+static void pef_guest_class_init(ObjectClass *oc, void *data)
+{
+    GuestMemoryProtectionClass *gmpc = GUEST_MEMORY_PROTECTION_CLASS(oc);
+
+    gmpc->kvm_init = pef_kvm_init;
+}
+
+static const TypeInfo pef_guest_info = {
+    .parent = TYPE_OBJECT,
+    .name = TYPE_PEF_GUEST,
+    .instance_size = sizeof(PefGuestState),
+    .class_init = pef_guest_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_GUEST_MEMORY_PROTECTION },
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
+static void
+pef_register_types(void)
+{
+    type_register_static(&pef_guest_info);
+}
+
+type_init(pef_register_types);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC 18/18] guest memory protection: Alter virtio default properties for protected guests
  2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
                   ` (16 preceding siblings ...)
  2020-05-14  6:41 ` [RFC 17/18] spapr: Added PEF based guest memory protection David Gibson
@ 2020-05-14  6:41 ` David Gibson
  17 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-14  6:41 UTC (permalink / raw)
  To: dgilbert, frankja, pair, qemu-devel, brijesh.singh
  Cc: kvm, qemu-ppc, David Gibson, Richard Henderson, cohuck,
	Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
	Eduardo Habkost, qemu-devel, mdroth

The default behaviour for virtio devices is not to use the platforms normal
DMA paths, but instead to use the fact that it's running in a hypervisor
to directly access guest memory.  That doesn't work if the guest's memory
is protected from hypervisor access, such as with AMD's SEV or POWER's PEF.

So, if a guest memory protection mechanism is enabled, then apply the
iommu_platform=on option so it will go through normal DMA mechanisms.
Those will presumably have some way of marking memory as shared with the
hypervisor or hardware so that DMA will work.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/core/machine.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 37d9f7f85c..373a144171 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -28,6 +28,8 @@
 #include "hw/mem/nvdimm.h"
 #include "migration/vmstate.h"
 #include "exec/guest-memory-protection.h"
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-pci.h"
 
 GlobalProperty hw_compat_5_0[] = {};
 const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
@@ -1170,6 +1172,15 @@ void machine_run_board_init(MachineState *machine)
          * areas.
          */
         machine_set_mem_merge(OBJECT(machine), false, &error_abort);
+
+        /*
+         * Virtio devices can't count on directly accessing guest
+         * memory, so they need iommu_platform=on to use normal DMA
+         * mechanisms.  That requires disabling legacy virtio support
+         * for virtio pci devices
+         */
+        object_register_sugar_prop(TYPE_VIRTIO_PCI, "disable-legacy", "on");
+        object_register_sugar_prop(TYPE_VIRTIO_DEVICE, "iommu_platform", "on");
     }
 
     machine_class->init(machine);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [RFC 16/18] use errp for gmpo kvm_init
  2020-05-14  6:41 ` [RFC 16/18] use errp for gmpo kvm_init David Gibson
@ 2020-05-14 17:09   ` Dr. David Alan Gilbert
  2020-05-15  0:14     ` David Gibson
  2020-05-15  0:20     ` David Gibson
  0 siblings, 2 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2020-05-14 17:09 UTC (permalink / raw)
  To: David Gibson
  Cc: frankja, qemu-devel, brijesh.singh, kvm, qemu-ppc,
	Richard Henderson, cohuck, Paolo Bonzini, Marcel Apfelbaum,
	Michael S. Tsirkin, Eduardo Habkost, mdroth

Dave:
  You've got some screwy mail headers here, the qemu-devel@nongnu.-rg is
the best one anmd the pair@us.redhat.com is weird as well.

* David Gibson (david@gibson.dropbear.id.au) wrote:
> ---
>  accel/kvm/kvm-all.c                    |  4 +++-
>  include/exec/guest-memory-protection.h |  2 +-
>  target/i386/sev.c                      | 32 +++++++++++++-------------
>  3 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 5451728425..392ab02867 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2045,9 +2045,11 @@ static int kvm_init(MachineState *ms)
>      if (ms->gmpo) {
>          GuestMemoryProtectionClass *gmpc =
>              GUEST_MEMORY_PROTECTION_GET_CLASS(ms->gmpo);
> +        Error *local_err = NULL;
>  
> -        ret = gmpc->kvm_init(ms->gmpo);
> +        ret = gmpc->kvm_init(ms->gmpo, &local_err);
>          if (ret < 0) {
> +            error_report_err(local_err);
>              goto err;
>          }
>      }
> diff --git a/include/exec/guest-memory-protection.h b/include/exec/guest-memory-protection.h
> index 7d959b4910..2a88475136 100644
> --- a/include/exec/guest-memory-protection.h
> +++ b/include/exec/guest-memory-protection.h
> @@ -32,7 +32,7 @@ typedef struct GuestMemoryProtection GuestMemoryProtection;
>  typedef struct GuestMemoryProtectionClass {
>      InterfaceClass parent;
>  
> -    int (*kvm_init)(GuestMemoryProtection *);
> +    int (*kvm_init)(GuestMemoryProtection *, Error **);
>      int (*encrypt_data)(GuestMemoryProtection *, uint8_t *, uint64_t);
>  } GuestMemoryProtectionClass;
>  
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 2051fae0c1..82f16b2f3b 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -617,7 +617,7 @@ sev_vm_state_change(void *opaque, int running, RunState state)
>      }
>  }
>  
> -static int sev_kvm_init(GuestMemoryProtection *gmpo)
> +static int sev_kvm_init(GuestMemoryProtection *gmpo, Error **errp)
>  {
>      SevGuestState *sev = SEV_GUEST(gmpo);
>      char *devname;
> @@ -633,14 +633,14 @@ static int sev_kvm_init(GuestMemoryProtection *gmpo)
>      host_cbitpos = ebx & 0x3f;
>  
>      if (host_cbitpos != sev->cbitpos) {
> -        error_report("%s: cbitpos check failed, host '%d' requested '%d'",
> -                     __func__, host_cbitpos, sev->cbitpos);
> +        error_setg(errp, "%s: cbitpos check failed, host '%d' requested '%d'",
> +                   __func__, host_cbitpos, sev->cbitpos);
>          goto err;
>      }
>  
>      if (sev->reduced_phys_bits < 1) {
> -        error_report("%s: reduced_phys_bits check failed, it should be >=1,"
> -                     " requested '%d'", __func__, sev->reduced_phys_bits);
> +        error_setg(errp, "%s: reduced_phys_bits check failed, it should be >=1,"
> +                   " requested '%d'", __func__, sev->reduced_phys_bits);
>          goto err;
>      }
>  
> @@ -649,20 +649,20 @@ static int sev_kvm_init(GuestMemoryProtection *gmpo)
>      devname = object_property_get_str(OBJECT(sev), "sev-device", NULL);
>      sev->sev_fd = open(devname, O_RDWR);
>      if (sev->sev_fd < 0) {
> -        error_report("%s: Failed to open %s '%s'", __func__,
> -                     devname, strerror(errno));
> -    }
> -    g_free(devname);
> -    if (sev->sev_fd < 0) {
> +        g_free(devname);
> +        error_setg(errp, "%s: Failed to open %s '%s'", __func__,
> +                   devname, strerror(errno));
> +        g_free(devname);

You seem to have double free'd devname - would g_autofree work here?

other than that, looks OK to me.

Dave

>          goto err;
>      }
> +    g_free(devname);
>  
>      ret = sev_platform_ioctl(sev->sev_fd, SEV_PLATFORM_STATUS, &status,
>                               &fw_error);
>      if (ret) {
> -        error_report("%s: failed to get platform status ret=%d "
> -                     "fw_error='%d: %s'", __func__, ret, fw_error,
> -                     fw_error_to_str(fw_error));
> +        error_setg(errp, "%s: failed to get platform status ret=%d "
> +                   "fw_error='%d: %s'", __func__, ret, fw_error,
> +                   fw_error_to_str(fw_error));
>          goto err;
>      }
>      sev->build_id = status.build;
> @@ -672,14 +672,14 @@ static int sev_kvm_init(GuestMemoryProtection *gmpo)
>      trace_kvm_sev_init();
>      ret = sev_ioctl(sev->sev_fd, KVM_SEV_INIT, NULL, &fw_error);
>      if (ret) {
> -        error_report("%s: failed to initialize ret=%d fw_error=%d '%s'",
> -                     __func__, ret, fw_error, fw_error_to_str(fw_error));
> +        error_setg(errp, "%s: failed to initialize ret=%d fw_error=%d '%s'",
> +                   __func__, ret, fw_error, fw_error_to_str(fw_error));
>          goto err;
>      }
>  
>      ret = sev_launch_start(sev);
>      if (ret) {
> -        error_report("%s: failed to create encryption context", __func__);
> +        error_setg(errp, "%s: failed to create encryption context", __func__);
>          goto err;
>      }
>  
> -- 
> 2.26.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC 16/18] use errp for gmpo kvm_init
  2020-05-14 17:09   ` Dr. David Alan Gilbert
@ 2020-05-15  0:14     ` David Gibson
  2020-05-15  0:20     ` David Gibson
  1 sibling, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-15  0:14 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: frankja, qemu-devel, brijesh.singh, kvm, qemu-ppc,
	Richard Henderson, cohuck, Paolo Bonzini, Marcel Apfelbaum,
	Michael S. Tsirkin, Eduardo Habkost, mdroth

[-- Attachment #1: Type: text/plain, Size: 5801 bytes --]

On Thu, May 14, 2020 at 06:09:46PM +0100, Dr. David Alan Gilbert wrote:
> Dave:
>   You've got some screwy mail headers here, the qemu-devel@nongnu.-rg is
> the best one anmd the pair@us.redhat.com is weird as well.

Yeah, apparently I forgot how to type when I entered my git-publish
command line :/.

> 
> * David Gibson (david@gibson.dropbear.id.au) wrote:
> > ---
> >  accel/kvm/kvm-all.c                    |  4 +++-
> >  include/exec/guest-memory-protection.h |  2 +-
> >  target/i386/sev.c                      | 32 +++++++++++++-------------
> >  3 files changed, 20 insertions(+), 18 deletions(-)
> > 
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index 5451728425..392ab02867 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -2045,9 +2045,11 @@ static int kvm_init(MachineState *ms)
> >      if (ms->gmpo) {
> >          GuestMemoryProtectionClass *gmpc =
> >              GUEST_MEMORY_PROTECTION_GET_CLASS(ms->gmpo);
> > +        Error *local_err = NULL;
> >  
> > -        ret = gmpc->kvm_init(ms->gmpo);
> > +        ret = gmpc->kvm_init(ms->gmpo, &local_err);
> >          if (ret < 0) {
> > +            error_report_err(local_err);
> >              goto err;
> >          }
> >      }
> > diff --git a/include/exec/guest-memory-protection.h b/include/exec/guest-memory-protection.h
> > index 7d959b4910..2a88475136 100644
> > --- a/include/exec/guest-memory-protection.h
> > +++ b/include/exec/guest-memory-protection.h
> > @@ -32,7 +32,7 @@ typedef struct GuestMemoryProtection GuestMemoryProtection;
> >  typedef struct GuestMemoryProtectionClass {
> >      InterfaceClass parent;
> >  
> > -    int (*kvm_init)(GuestMemoryProtection *);
> > +    int (*kvm_init)(GuestMemoryProtection *, Error **);
> >      int (*encrypt_data)(GuestMemoryProtection *, uint8_t *, uint64_t);
> >  } GuestMemoryProtectionClass;
> >  
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 2051fae0c1..82f16b2f3b 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -617,7 +617,7 @@ sev_vm_state_change(void *opaque, int running, RunState state)
> >      }
> >  }
> >  
> > -static int sev_kvm_init(GuestMemoryProtection *gmpo)
> > +static int sev_kvm_init(GuestMemoryProtection *gmpo, Error **errp)
> >  {
> >      SevGuestState *sev = SEV_GUEST(gmpo);
> >      char *devname;
> > @@ -633,14 +633,14 @@ static int sev_kvm_init(GuestMemoryProtection *gmpo)
> >      host_cbitpos = ebx & 0x3f;
> >  
> >      if (host_cbitpos != sev->cbitpos) {
> > -        error_report("%s: cbitpos check failed, host '%d' requested '%d'",
> > -                     __func__, host_cbitpos, sev->cbitpos);
> > +        error_setg(errp, "%s: cbitpos check failed, host '%d' requested '%d'",
> > +                   __func__, host_cbitpos, sev->cbitpos);
> >          goto err;
> >      }
> >  
> >      if (sev->reduced_phys_bits < 1) {
> > -        error_report("%s: reduced_phys_bits check failed, it should be >=1,"
> > -                     " requested '%d'", __func__, sev->reduced_phys_bits);
> > +        error_setg(errp, "%s: reduced_phys_bits check failed, it should be >=1,"
> > +                   " requested '%d'", __func__, sev->reduced_phys_bits);
> >          goto err;
> >      }
> >  
> > @@ -649,20 +649,20 @@ static int sev_kvm_init(GuestMemoryProtection *gmpo)
> >      devname = object_property_get_str(OBJECT(sev), "sev-device", NULL);
> >      sev->sev_fd = open(devname, O_RDWR);
> >      if (sev->sev_fd < 0) {
> > -        error_report("%s: Failed to open %s '%s'", __func__,
> > -                     devname, strerror(errno));
> > -    }
> > -    g_free(devname);
> > -    if (sev->sev_fd < 0) {
> > +        g_free(devname);
> > +        error_setg(errp, "%s: Failed to open %s '%s'", __func__,
> > +                   devname, strerror(errno));
> > +        g_free(devname);
> 
> You seem to have double free'd devname - would g_autofree work here?
> 
> other than that, looks OK to me.
> 
> Dave
> 
> >          goto err;
> >      }
> > +    g_free(devname);
> >  
> >      ret = sev_platform_ioctl(sev->sev_fd, SEV_PLATFORM_STATUS, &status,
> >                               &fw_error);
> >      if (ret) {
> > -        error_report("%s: failed to get platform status ret=%d "
> > -                     "fw_error='%d: %s'", __func__, ret, fw_error,
> > -                     fw_error_to_str(fw_error));
> > +        error_setg(errp, "%s: failed to get platform status ret=%d "
> > +                   "fw_error='%d: %s'", __func__, ret, fw_error,
> > +                   fw_error_to_str(fw_error));
> >          goto err;
> >      }
> >      sev->build_id = status.build;
> > @@ -672,14 +672,14 @@ static int sev_kvm_init(GuestMemoryProtection *gmpo)
> >      trace_kvm_sev_init();
> >      ret = sev_ioctl(sev->sev_fd, KVM_SEV_INIT, NULL, &fw_error);
> >      if (ret) {
> > -        error_report("%s: failed to initialize ret=%d fw_error=%d '%s'",
> > -                     __func__, ret, fw_error, fw_error_to_str(fw_error));
> > +        error_setg(errp, "%s: failed to initialize ret=%d fw_error=%d '%s'",
> > +                   __func__, ret, fw_error, fw_error_to_str(fw_error));
> >          goto err;
> >      }
> >  
> >      ret = sev_launch_start(sev);
> >      if (ret) {
> > -        error_report("%s: failed to create encryption context", __func__);
> > +        error_setg(errp, "%s: failed to create encryption context", __func__);
> >          goto err;
> >      }
> >  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC 16/18] use errp for gmpo kvm_init
  2020-05-14 17:09   ` Dr. David Alan Gilbert
  2020-05-15  0:14     ` David Gibson
@ 2020-05-15  0:20     ` David Gibson
  1 sibling, 0 replies; 22+ messages in thread
From: David Gibson @ 2020-05-15  0:20 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: frankja, qemu-devel, brijesh.singh, kvm, qemu-ppc,
	Richard Henderson, cohuck, Paolo Bonzini, Marcel Apfelbaum,
	Michael S. Tsirkin, Eduardo Habkost, mdroth

[-- Attachment #1: Type: text/plain, Size: 2789 bytes --]

On Thu, May 14, 2020 at 06:09:46PM +0100, Dr. David Alan Gilbert wrote:
> * David Gibson (david@gibson.dropbear.id.au) wrote:
[snip]
> > @@ -649,20 +649,20 @@ static int sev_kvm_init(GuestMemoryProtection *gmpo)
> >      devname = object_property_get_str(OBJECT(sev), "sev-device", NULL);
> >      sev->sev_fd = open(devname, O_RDWR);
> >      if (sev->sev_fd < 0) {
> > -        error_report("%s: Failed to open %s '%s'", __func__,
> > -                     devname, strerror(errno));
> > -    }
> > -    g_free(devname);
> > -    if (sev->sev_fd < 0) {
> > +        g_free(devname);
> > +        error_setg(errp, "%s: Failed to open %s '%s'", __func__,
> > +                   devname, strerror(errno));
> > +        g_free(devname);
> 
> You seem to have double free'd devname - would g_autofree work here?

Oops, fixed.  I'm not really familiar with the g_autofree stuff as
yet, so, maybe?

I also entirely forgot to write a non-placeholder commit message for
this patch.  Oops.

> other than that, looks OK to me.



> 
> Dave
> 
> >          goto err;
> >      }
> > +    g_free(devname);
> >  
> >      ret = sev_platform_ioctl(sev->sev_fd, SEV_PLATFORM_STATUS, &status,
> >                               &fw_error);
> >      if (ret) {
> > -        error_report("%s: failed to get platform status ret=%d "
> > -                     "fw_error='%d: %s'", __func__, ret, fw_error,
> > -                     fw_error_to_str(fw_error));
> > +        error_setg(errp, "%s: failed to get platform status ret=%d "
> > +                   "fw_error='%d: %s'", __func__, ret, fw_error,
> > +                   fw_error_to_str(fw_error));
> >          goto err;
> >      }
> >      sev->build_id = status.build;
> > @@ -672,14 +672,14 @@ static int sev_kvm_init(GuestMemoryProtection *gmpo)
> >      trace_kvm_sev_init();
> >      ret = sev_ioctl(sev->sev_fd, KVM_SEV_INIT, NULL, &fw_error);
> >      if (ret) {
> > -        error_report("%s: failed to initialize ret=%d fw_error=%d '%s'",
> > -                     __func__, ret, fw_error, fw_error_to_str(fw_error));
> > +        error_setg(errp, "%s: failed to initialize ret=%d fw_error=%d '%s'",
> > +                   __func__, ret, fw_error, fw_error_to_str(fw_error));
> >          goto err;
> >      }
> >  
> >      ret = sev_launch_start(sev);
> >      if (ret) {
> > -        error_report("%s: failed to create encryption context", __func__);
> > +        error_setg(errp, "%s: failed to create encryption context", __func__);
> >          goto err;
> >      }
> >  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2020-05-15  1:53 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-14  6:41 [RFC 00/18] Refactor configuration of guest memory protection David Gibson
2020-05-14  6:41 ` [RFC 01/18] target/i386: sev: Remove unused QSevGuestInfoClass David Gibson
2020-05-14  6:41 ` [RFC 02/18] target/i386: sev: Move local structure definitions into .c file David Gibson
2020-05-14  6:41 ` [RFC 03/18] target/i386: sev: Rename QSevGuestInfo David Gibson
2020-05-14  6:41 ` [RFC 04/18] target/i386: sev: Embed SEVState in SevGuestState David Gibson
2020-05-14  6:41 ` [RFC 05/18] target/i386: sev: Partial cleanup to sev_state global David Gibson
2020-05-14  6:41 ` [RFC 06/18] target/i386: sev: Remove redundant cbitpos and reduced_phys_bits fields David Gibson
2020-05-14  6:41 ` [RFC 07/18] target/i386: sev: Remove redundant policy field David Gibson
2020-05-14  6:41 ` [RFC 08/18] target/i386: sev: Remove redundant handle field David Gibson
2020-05-14  6:41 ` [RFC 09/18] target/i386: sev: Unify SEVState and SevGuestState David Gibson
2020-05-14  6:41 ` [RFC 10/18] guest memory protection: Add guest memory protection interface David Gibson
2020-05-14  6:41 ` [RFC 11/18] guest memory protection: Handle memory encrption via interface David Gibson
2020-05-14  6:41 ` [RFC 12/18] guest memory protection: Perform KVM init " David Gibson
2020-05-14  6:41 ` [RFC 13/18] guest memory protection: Move side effect out of machine_set_memory_encryption() David Gibson
2020-05-14  6:41 ` [RFC 14/18] guest memory protection: Rework the "memory-encryption" property David Gibson
2020-05-14  6:41 ` [RFC 15/18] guest memory protection: Decouple kvm_memcrypt_*() helpers from KVM David Gibson
2020-05-14  6:41 ` [RFC 16/18] use errp for gmpo kvm_init David Gibson
2020-05-14 17:09   ` Dr. David Alan Gilbert
2020-05-15  0:14     ` David Gibson
2020-05-15  0:20     ` David Gibson
2020-05-14  6:41 ` [RFC 17/18] spapr: Added PEF based guest memory protection David Gibson
2020-05-14  6:41 ` [RFC 18/18] guest memory protection: Alter virtio default properties for protected guests David Gibson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).