[RFC v2 00/18] Refactor configuration of guest memory protection

[RFC v2 00/18] Refactor configuration of guest memory protection

[David Gibson]

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.

Changes since v1:
 * Rebased
 * Fixed some errors pointed out by Dave Gilbert

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
  guest memory protection: Add Error ** to
    GuestMemoryProtection::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                      | 351 +++++++++++++------------
 target/i386/sev_i386.h                 |  49 ----
 target/ppc/Makefile.objs               |   2 +-
 target/ppc/pef.c                       |  81 ++++++
 15 files changed, 441 insertions(+), 299 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

[RFC v2 01/18] target/i386: sev: Remove unused QSevGuestInfoClass

[David Gibson]

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 51cdbe5496..2312510cf2 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -287,7 +287,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

[RFC v2 02/18] target/i386: sev: Move local structure definitions into .c file

[David Gibson]

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 2312510cf2..53def5f41a 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

[RFC v2 03/18] target/i386: sev: Rename QSevGuestInfo

[David Gibson]

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 53def5f41a..b6ed719fb5 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,82 +236,82 @@ 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);
     object_class_property_set_description(oc, "sev-device",
             "SEV device to use");
     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);
     object_class_property_set_description(oc, "dh-cert-file",
             "guest owners DH certificate (encoded with base64)");
     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);
     object_class_property_set_description(oc, "session-file",
             "guest owners session parameters (encoded with base64)");
 }
 
 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;
@@ -326,32 +327,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;
     }
@@ -510,7 +511,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;
 
@@ -696,7 +697,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;
     }
 
@@ -786,7 +787,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

[RFC v2 04/18] target/i386: sev: Embed SEVState in SevGuestState

[David Gibson]

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 b6ed719fb5..b4ab9720d6 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"
 
@@ -506,12 +509,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;
 
@@ -686,6 +689,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;
@@ -693,27 +697,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,"
@@ -723,7 +727,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__,
@@ -754,7 +758,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

[RFC v2 05/18] target/i386: sev: Partial cleanup to sev_state global

[David Gibson]

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 b4ab9720d6..9e8ab7b056 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
@@ -366,25 +366,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 *
@@ -393,15 +393,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;
@@ -550,7 +550,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;
@@ -563,7 +563,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;
@@ -575,7 +575,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'",
@@ -588,19 +588,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'",
@@ -612,7 +613,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'",
@@ -620,7 +621,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);
@@ -635,9 +636,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;
@@ -648,20 +649,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,
@@ -677,11 +679,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);
         }
     }
 }
@@ -704,7 +706,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);
@@ -766,23 +769,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

[RFC v2 06/18] target/i386: sev: Remove redundant cbitpos and reduced_phys_bits fields

[David Gibson]

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 9e8ab7b056..d25af37136 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;
@@ -378,13 +376,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 *
@@ -713,22 +711,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

[RFC v2 07/18] target/i386: sev: Remove redundant policy field

[David Gibson]

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 d25af37136..4b261beaa7 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;
@@ -397,7 +396,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;
     }
@@ -520,8 +519,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;
@@ -550,7 +548,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

[RFC v2 08/18] target/i386: sev: Remove redundant handle field

[David Gibson]

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 4b261beaa7..24e2dea9b8 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;
 };
 
@@ -398,7 +397,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;
@@ -517,8 +516,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) {
@@ -544,10 +542,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

[RFC v2 09/18] target/i386: sev: Unify SEVState and SevGuestState

[David Gibson]

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 24e2dea9b8..d273174ad3 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
@@ -368,7 +362,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
@@ -392,11 +386,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;
     }
 
@@ -507,7 +501,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;
@@ -535,7 +528,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));
@@ -566,7 +559,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'",
@@ -582,7 +575,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)) {
@@ -592,7 +584,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'",
@@ -604,7 +596,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'",
@@ -615,8 +607,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);
@@ -628,8 +620,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;
@@ -642,12 +634,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));
@@ -683,7 +674,6 @@ void *
 sev_guest_init(const char *id)
 {
     SevGuestState *sev;
-    SEVState *s;
     char *devname;
     int ret, fw_error;
     uint32_t ebx;
@@ -698,8 +688,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;
@@ -716,20 +705,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 "
@@ -737,12 +726,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

[RFC v2 10/18] guest memory protection: Add guest memory protection interface

[David Gibson]

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

[RFC v2 11/18] guest memory protection: Handle memory encrption via interface

[David Gibson]

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                      | 84 ++++++++++++++------------
 5 files changed, 63 insertions(+), 57 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index d06cc04079..40997de38c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -45,6 +45,7 @@
 #include "qapi/qapi-types-common.h"
 #include "qapi/qapi-visit-common.h"
 #include "sysemu/reset.h"
+#include "exec/guest-memory-protection.h"
 
 #include "hw/boards.h"
 
@@ -119,8 +120,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;
@@ -172,7 +172,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;
     }
 
@@ -181,10 +181,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;
@@ -2101,13 +2104,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 d273174ad3..986c2fee51 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,26 +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);
-    object_class_property_set_description(oc, "sev-device",
-            "SEV device to use");
-    object_class_property_add_str(oc, "dh-cert-file",
-                                  sev_guest_get_dh_cert_file,
-                                  sev_guest_set_dh_cert_file);
-    object_class_property_set_description(oc, "dh-cert-file",
-            "guest owners DH certificate (encoded with base64)");
-    object_class_property_add_str(oc, "session-file",
-                                  sev_guest_get_session_file,
-                                  sev_guest_set_session_file);
-    object_class_property_set_description(oc, "session-file",
-            "guest owners session parameters (encoded with base64)");
-}
-
 static void
 sev_guest_instance_init(Object *obj)
 {
@@ -319,20 +300,6 @@ sev_guest_instance_init(Object *obj)
                                    OBJ_PROP_FLAG_READWRITE);
 }
 
-/* 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)
 {
@@ -670,7 +637,7 @@ sev_vm_state_change(void *opaque, int running, RunState state)
     }
 }
 
-void *
+GuestMemoryProtection *
 sev_guest_init(const char *id)
 {
     SevGuestState *sev;
@@ -748,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);
 
@@ -769,6 +736,45 @@ 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);
+    object_class_property_set_description(oc, "sev-device",
+        "SEV device to use");
+    object_class_property_add_str(oc, "dh-cert-file",
+                                  sev_guest_get_dh_cert_file,
+                                  sev_guest_set_dh_cert_file);
+    object_class_property_set_description(oc, "dh-cert-file",
+        "guest owners DH certificate (encoded with base64)");
+    object_class_property_add_str(oc, "session-file",
+                                  sev_guest_get_session_file,
+                                  sev_guest_set_session_file);
+    object_class_property_set_description(oc, "session-file",
+        "guest owners session parameters (encoded with base64)");
+
+    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

[RFC v2 12/18] guest memory protection: Perform KVM init via interface

[David Gibson]

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 40997de38c..5cf1a397e3 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"
@@ -2104,8 +2103,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 986c2fee51..60e9d8c735 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);
 }
 
-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
@@ -757,6 +729,7 @@ sev_guest_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "session-file",
         "guest owners session parameters (encoded with base64)");
 
+    gmpc->kvm_init = sev_kvm_init;
     gmpc->encrypt_data = sev_encrypt_data;
 }
 
-- 
2.26.2

[RFC v2 13/18] guest memory protection: Move side effect out of machine_set_memory_encryption()

[David Gibson]

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 bb3a7b18b1..e75f0b73d0 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)
@@ -1129,6 +1121,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

[RFC v2 14/18] guest memory protection: Rework the "memory-encryption" property

[David Gibson]

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 5cf1a397e3..3588adf1e1 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2102,25 +2102,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 e75f0b73d0..88d699bceb 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)
@@ -849,6 +871,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");
 
+    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);
+    object_class_property_set_description(oc, "guest-memory-protection",
+        "Set guest memory protection object to use");
+
+    /* For compatibility */
     object_class_property_add_str(oc, "memory-encryption",
         machine_get_memory_encryption, machine_set_memory_encryption);
     object_class_property_set_description(oc, "memory-encryption",
@@ -1121,7 +1152,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

[RFC v2 15/18] guest memory protection: Decouple kvm_memcrypt_*() helpers from KVM

[David Gibson]

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 3588adf1e1..1b10e94222 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -118,9 +118,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 {
@@ -169,29 +166,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)
 {
@@ -2110,8 +2084,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 b8d8ef59eb..9cef5f7780 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
@@ -196,10 +197,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 3b2250471c..cfc4cee995 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

[RFC v2 16/18] guest memory protection: Add Error ** to GuestMemoryProtection::kvm_init

[David Gibson]

This allows failures to be reported richly and idiomatically.

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 1b10e94222..4011699736 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2079,9 +2079,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 60e9d8c735..6a56ec203b 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,19 @@ 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) {
+        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 +671,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

[RFC v2 17/18] spapr: Added PEF based guest memory protection

[David Gibson]

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

[RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[David Gibson]

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 88d699bceb..cb6580954e 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);
@@ -1159,6 +1161,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

Re: [RFC v2 11/18] guest memory protection: Handle memory encrption via interface

[Greg Kurz]

s/encrption/encryption

On Thu, 21 May 2020 13:42:57 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> 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                      | 84 ++++++++++++++------------
>  5 files changed, 63 insertions(+), 57 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index d06cc04079..40997de38c 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -45,6 +45,7 @@
>  #include "qapi/qapi-types-common.h"
>  #include "qapi/qapi-visit-common.h"
>  #include "sysemu/reset.h"
> +#include "exec/guest-memory-protection.h"
>  
>  #include "hw/boards.h"
>  
> @@ -119,8 +120,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;
> @@ -172,7 +172,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;
>      }
>  
> @@ -181,10 +181,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;
> @@ -2101,13 +2104,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;

This requires some extra care:

accel/kvm/sev-stub.c:18:7: error: conflicting types for ‘sev_guest_init’
 void *sev_guest_init(const char *id)
       ^~~~~~~~~~~~~~
In file included from accel/kvm/sev-stub.c:16:0:
include/sysemu/sev.h:21:24: note: previous declaration of ‘sev_guest_init’ was here
 GuestMemoryProtection *sev_guest_init(const char *id);
                        ^~~~~~~~~~~~~~
rules.mak:69: recipe for target 'accel/kvm/sev-stub.o' failed

> 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 d273174ad3..986c2fee51 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,26 +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);
> -    object_class_property_set_description(oc, "sev-device",
> -            "SEV device to use");
> -    object_class_property_add_str(oc, "dh-cert-file",
> -                                  sev_guest_get_dh_cert_file,
> -                                  sev_guest_set_dh_cert_file);
> -    object_class_property_set_description(oc, "dh-cert-file",
> -            "guest owners DH certificate (encoded with base64)");
> -    object_class_property_add_str(oc, "session-file",
> -                                  sev_guest_get_session_file,
> -                                  sev_guest_set_session_file);
> -    object_class_property_set_description(oc, "session-file",
> -            "guest owners session parameters (encoded with base64)");
> -}
> -
>  static void
>  sev_guest_instance_init(Object *obj)
>  {
> @@ -319,20 +300,6 @@ sev_guest_instance_init(Object *obj)
>                                     OBJ_PROP_FLAG_READWRITE);
>  }
>  
> -/* 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)
>  {
> @@ -670,7 +637,7 @@ sev_vm_state_change(void *opaque, int running, RunState state)
>      }
>  }
>  
> -void *
> +GuestMemoryProtection *
>  sev_guest_init(const char *id)
>  {
>      SevGuestState *sev;
> @@ -748,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);
>  
> @@ -769,6 +736,45 @@ 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);
> +    object_class_property_set_description(oc, "sev-device",
> +        "SEV device to use");
> +    object_class_property_add_str(oc, "dh-cert-file",
> +                                  sev_guest_get_dh_cert_file,
> +                                  sev_guest_set_dh_cert_file);
> +    object_class_property_set_description(oc, "dh-cert-file",
> +        "guest owners DH certificate (encoded with base64)");
> +    object_class_property_add_str(oc, "session-file",
> +                                  sev_guest_get_session_file,
> +                                  sev_guest_set_session_file);
> +    object_class_property_set_description(oc, "session-file",
> +        "guest owners session parameters (encoded with base64)");
> +
> +    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)
>  {

Re: [RFC v2 10/18] guest memory protection: Add guest memory protection interface

[Greg Kurz]

On Thu, 21 May 2020 13:42:56 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> 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 */
> +

Applying patch #1294935 using "git am -s -m"
Description: [RFC,v2,10/18] guest memory protection: Add guest memory protection
Applying: guest memory protection: Add guest memory protection interface
.git/rebase-apply/patch:95: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

Re: [RFC v2 17/18] spapr: Added PEF based guest memory protection

[Greg Kurz]

On Thu, 21 May 2020 13:43:03 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Some upcoming POWER machines have a system called PEF (Protected
> Execution Framework) which uses a small ultravisor to allow guests to

s/Framework/Facility

> 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

s/havint/having

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

Wrap line after "machine" maybe ?

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

Maybe worth mentioning that this is for KVM only. Also, this is
silently ignored with TCG since pef_kvm_init() isn't called in
this case. Would it make sense to print some warning like we
do for these spapr caps that we don't support with TCG ?

> 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

s/Framework/Facility

> + *
> + * 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"
> +

I had to include some more headers to build this.

#include "exec/guest-memory-protection.h"
#include "qapi/error.h"
#include "qom/object_interfaces.h"
#include "sysemu/kvm.h"

> +#define TYPE_PEF_GUEST "pef-guest"
> +#define PEF_GUEST(obj)                                  \
> +    OBJECT_CHECK(PefGuestState, (obj), TYPE_SEV_GUEST)

s/TYPE_SEV_GUEST/TYPE_PEF_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;

Unused.

> +
> +static int pef_kvm_init(GuestMemoryProtection *gmpo, Error **errp)
> +{
> +    PefGuestState *pef = PEF_GUEST(gmpo);

Unused.

> +
> +    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);

Re: [RFC v2 17/18] spapr: Added PEF based guest memory protection

[Ram Pai]

On Thu, May 21, 2020 at 01:43:03PM +1000, David Gibson wrote:
> Some upcoming POWER machines have a system called PEF (Protected
> Execution Framework) which uses a small ultravisor to allow guests to

Framework -> Facility

> 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

havint -> having

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

In fact, Qemu has no direct way of knowing if the guest has turned
secure or not, even after machine creation time. There are indirect ways
for Qemu to know that, but nothing informs Qemu explicitly about it. 

So maybe we should just say...

"..
 such that the guest still needs to talk to the ultravisor to enter
 secure mode, so we can't directly know if the guest actually is secure." 


> 
> 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;
> +}

This looks correct to me.

> +
> +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);

Acked-by: Ram Pai <linuxram@us.ibm.com>

Thanks for doing this!

BTW: Will there be a new machine type defined for running secure VMs?

RP

Re: [RFC v2 01/18] target/i386: sev: Remove unused QSevGuestInfoClass

[Philippe Mathieu-Daudé]

On 5/21/20 5:42 AM, David Gibson wrote:
> 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 51cdbe5496..2312510cf2 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -287,7 +287,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;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [RFC v2 02/18] target/i386: sev: Move local structure definitions into .c file

[Philippe Mathieu-Daudé]

On 5/21/20 5:42 AM, David Gibson wrote:
> 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [RFC v2 03/18] target/i386: sev: Rename QSevGuestInfo

[Philippe Mathieu-Daudé]

On 5/21/20 5:42 AM, David Gibson wrote:
> 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [RFC v2 04/18] target/i386: sev: Embed SEVState in SevGuestState

[Philippe Mathieu-Daudé]

On 5/21/20 5:42 AM, David Gibson wrote:
> 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 b6ed719fb5..b4ab9720d6 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;

Maybe typedef & declaration altogether.

> +
>  /**
>   * 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"
>  
> @@ -506,12 +509,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;
>  
> @@ -686,6 +689,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;
> @@ -693,27 +697,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;

I was going to suggest to clean that, but I see your next patch already
does the cleanup :)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>      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,"
> @@ -723,7 +727,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__,
> @@ -754,7 +758,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;
> 

Re: [RFC v2 06/18] target/i386: sev: Remove redundant cbitpos and reduced_phys_bits fields

[Philippe Mathieu-Daudé]

On 5/21/20 5:42 AM, David Gibson wrote:
> 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [RFC v2 09/18] target/i386: sev: Unify SEVState and SevGuestState

[Philippe Mathieu-Daudé]

On 5/21/20 5:42 AM, David Gibson wrote:
> 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(-)
[...]
>      sev_guest = sev;
> -    s = &sev->state;
> -    s->state = SEV_STATE_UNINIT;
> +    sev->state = SEV_STATE_UNINIT;

:)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [RFC v2 16/18] guest memory protection: Add Error ** to GuestMemoryProtection::kvm_init

[Philippe Mathieu-Daudé]

On 5/21/20 5:43 AM, David Gibson wrote:
> This allows failures to be reported richly and idiomatically.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  accel/kvm/kvm-all.c                    |  4 +++-
>  include/exec/guest-memory-protection.h |  2 +-
>  target/i386/sev.c                      | 31 +++++++++++++-------------
>  3 files changed, 19 insertions(+), 18 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Sean Christopherson]

On Thu, May 21, 2020 at 01:42:46PM +1000, David Gibson wrote:
> 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.

Many of the features are also going far beyond just protecting memory, so
even the "memory" part feels wrong.  Maybe something like protected-guest
or secure-guest?

A little imprecision isn't necessarily a bad thing, e.g. memory-encryption
is quite precise, but also wrong once it encompasses anything beyond plain
old encryption.

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Dr. David Alan Gilbert]

* Sean Christopherson (sean.j.christopherson@intel.com) wrote:
> On Thu, May 21, 2020 at 01:42:46PM +1000, David Gibson wrote:
> > 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.
> 
> Many of the features are also going far beyond just protecting memory, so
> even the "memory" part feels wrong.  Maybe something like protected-guest
> or secure-guest?
> 
> A little imprecision isn't necessarily a bad thing, e.g. memory-encryption
> is quite precise, but also wrong once it encompasses anything beyond plain
> old encryption.

The common thread I think is 'untrusted host' - but I don't know of a
better way to describe that.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC v2 10/18] guest memory protection: Add guest memory protection interface

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> @@ -0,0 +1,29 @@
> +#/*

Two extraneous # at the beginning of the new files.


r~

Re: [RFC v2 01/18] target/i386: sev: Remove unused QSevGuestInfoClass

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> 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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 02/18] target/i386: sev: Move local structure definitions into .c file

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> 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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 03/18] target/i386: sev: Rename QSevGuestInfo

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> 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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 04/18] target/i386: sev: Embed SEVState in SevGuestState

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> 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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 05/18] target/i386: sev: Partial cleanup to sev_state global

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> 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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 06/18] target/i386: sev: Remove redundant cbitpos and reduced_phys_bits fields

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> 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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 07/18] target/i386: sev: Remove redundant policy field

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> 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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 08/18] target/i386: sev: Remove redundant handle field

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> 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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 09/18] target/i386: sev: Unify SEVState and SevGuestState

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> 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(-)

Yay!

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 11/18] guest memory protection: Handle memory encrption via interface

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> 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                      | 84 ++++++++++++++------------
>  5 files changed, 63 insertions(+), 57 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 12/18] guest memory protection: Perform KVM init via interface

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> +        if (object_dynamic_cast(obj, TYPE_GUEST_MEMORY_PROTECTION)) {
> +            GuestMemoryProtection *gmpo = GUEST_MEMORY_PROTECTION(obj);

This duplicates the interface check.  You should use

  gmpo = (GuestMemoryProtection *)
    object_dynamic_cast(obj, TYPE_GUEST_MEMORY_PROTECTION);
  if (gmpo) {

AFICT.


r~

Re: [RFC v2 13/18] guest memory protection: Move side effect out of machine_set_memory_encryption()

[Richard Henderson]

On 5/20/20 8:42 PM, David Gibson wrote:
> 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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 12/18] guest memory protection: Perform KVM init via interface

[Richard Henderson]

On 6/1/20 8:39 PM, Richard Henderson wrote:
> On 5/20/20 8:42 PM, David Gibson wrote:
>> +        if (object_dynamic_cast(obj, TYPE_GUEST_MEMORY_PROTECTION)) {
>> +            GuestMemoryProtection *gmpo = GUEST_MEMORY_PROTECTION(obj);
> 
> This duplicates the interface check.  You should use
> 
>   gmpo = (GuestMemoryProtection *)
>     object_dynamic_cast(obj, TYPE_GUEST_MEMORY_PROTECTION);
>   if (gmpo) {
> 
> AFICT.

Or ignore this nit, since you clean it up in patch 14.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 14/18] guest memory protection: Rework the "memory-encryption" property

[Richard Henderson]

On 5/20/20 8:43 PM, David Gibson wrote:
> +++ b/include/hw/boards.h
> @@ -12,6 +12,8 @@
>  #include "qom/object.h"
>  #include "hw/core/cpu.h"
>  
> +typedef struct GuestMemoryProtection GuestMemoryProtection;
> +

I think this needs to be in include/qemu/typedefs.h,
and the other typedef in patch 10 needs to be moved there.

IIRC, clang warns about duplicate typedefs.


r~

Re: [RFC v2 15/18] guest memory protection: Decouple kvm_memcrypt_*() helpers from KVM

[Richard Henderson]

On 5/20/20 8:43 PM, David Gibson wrote:
> 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).

Yet?

I would expect TCG implementations of these interfaces eventually, for
simulation of such hardware.  Or are we expecting *this* interface to be used
only with kvm/any-other-hw-accel, as the nested guest inside of the outermost
tcg qemu that's simulating hardware that supports...

Anyway, the actual code looks ok.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 16/18] guest memory protection: Add Error ** to GuestMemoryProtection::kvm_init

[Richard Henderson]

On 5/20/20 8:43 PM, David Gibson wrote:
> This allows failures to be reported richly and idiomatically.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  accel/kvm/kvm-all.c                    |  4 +++-
>  include/exec/guest-memory-protection.h |  2 +-
>  target/i386/sev.c                      | 31 +++++++++++++-------------
>  3 files changed, 19 insertions(+), 18 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 10/18] guest memory protection: Add guest memory protection interface

[David Gibson]

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

On Mon, May 25, 2020 at 12:27:35PM +0200, Greg Kurz wrote:
> On Thu, 21 May 2020 13:42:56 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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 */
> > +
> 
> Applying patch #1294935 using "git am -s -m"
> Description: [RFC,v2,10/18] guest memory protection: Add guest memory protection
> Applying: guest memory protection: Add guest memory protection interface
> .git/rebase-apply/patch:95: new blank line at EOF.
> +
> warning: 1 line adds whitespace errors.

Oops, fixed.

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

Re: [RFC v2 10/18] guest memory protection: Add guest memory protection interface

[David Gibson]

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

On Mon, Jun 01, 2020 at 06:44:50PM -0700, Richard Henderson wrote:
> On 5/20/20 8:42 PM, David Gibson wrote:
> > @@ -0,0 +1,29 @@
> > +#/*
> 
> Two extraneous # at the beginning of the new files.

Huh, weird.  Fixed.

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

Re: [RFC v2 15/18] guest memory protection: Decouple kvm_memcrypt_*() helpers from KVM

[David Gibson]

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

On Mon, Jun 01, 2020 at 09:13:01PM -0700, Richard Henderson wrote:
> On 5/20/20 8:43 PM, David Gibson wrote:
> > 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).
> 
> Yet?
> 
> I would expect TCG implementations of these interfaces eventually, for
> simulation of such hardware.  Or are we expecting *this* interface to be used
> only with kvm/any-other-hw-accel, as the nested guest inside of the outermost
> tcg qemu that's simulating hardware that supports...

Yes, this.  The whole point of this stuff is that the guest is
protected *from the hypervisor*, and therefore qemu can't expect to
access guest memory.  It's hard to imagine any way TCG could deal with
that.

An implementation for a non-KVM accelerator is entirely plausible, on
the other hand.

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

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[David Gibson]

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

On Fri, May 29, 2020 at 03:19:26PM -0700, Sean Christopherson wrote:
> On Thu, May 21, 2020 at 01:42:46PM +1000, David Gibson wrote:
> > 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.
> 
> Many of the features are also going far beyond just protecting memory, so
> even the "memory" part feels wrong.  Maybe something like protected-guest
> or secure-guest?

I think those are too vague.  There are *heaps* of things related to
protecting or securing guests, the relevance of this stuff is that
it's protecting it from a compromised hypervisor.

> A little imprecision isn't necessarily a bad thing, e.g. memory-encryption
> is quite precise, but also wrong once it encompasses anything beyond plain
> old encryption.

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

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[David Gibson]

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

On Mon, Jun 01, 2020 at 10:16:18AM +0100, Dr. David Alan Gilbert wrote:
> * Sean Christopherson (sean.j.christopherson@intel.com) wrote:
> > On Thu, May 21, 2020 at 01:42:46PM +1000, David Gibson wrote:
> > > 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.
> > 
> > Many of the features are also going far beyond just protecting memory, so
> > even the "memory" part feels wrong.  Maybe something like protected-guest
> > or secure-guest?
> > 
> > A little imprecision isn't necessarily a bad thing, e.g. memory-encryption
> > is quite precise, but also wrong once it encompasses anything beyond plain
> > old encryption.
> 
> The common thread I think is 'untrusted host' - but I don't know of a
> better way to describe that.

Hrm..  UntrustedHost? CompromisedHostMitigation? HostTrustMitigation
(that way it has the same abbreviation as hardware transactional
memory for extra confusion)?  HypervisorPowerLimitation?

HostTrustLimitation? "HTL". That's not too bad, actually, I might go
with that unless someone suggests something better.

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

Re: [RFC v2 04/18] target/i386: sev: Embed SEVState in SevGuestState

[David Gibson]

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

On Fri, May 29, 2020 at 11:09:41AM +0200, Philippe Mathieu-Daudé wrote:
> On 5/21/20 5:42 AM, David Gibson wrote:
> > 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 b6ed719fb5..b4ab9720d6 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;
> 
> Maybe typedef & declaration altogether.

I'd prefer not - this makes it clearer that it's a pure code motion.
It's going to go away in a few patches time in any case.

> > +
> >  /**
> >   * 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"
> >  
> > @@ -506,12 +509,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;
> >  
> > @@ -686,6 +689,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;
> > @@ -693,27 +697,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;
> 
> I was going to suggest to clean that, but I see your next patch already
> does the cleanup :)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> >      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,"
> > @@ -723,7 +727,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__,
> > @@ -754,7 +758,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;
> > 
> 

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

Re: [RFC v2 11/18] guest memory protection: Handle memory encrption via interface

[David Gibson]

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

On Mon, May 25, 2020 at 12:26:55PM +0200, Greg Kurz wrote:
> 
> s/encrption/encryption

Fixed.

> On Thu, 21 May 2020 13:42:57 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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                      | 84 ++++++++++++++------------
> >  5 files changed, 63 insertions(+), 57 deletions(-)
> > 
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index d06cc04079..40997de38c 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -45,6 +45,7 @@
> >  #include "qapi/qapi-types-common.h"
> >  #include "qapi/qapi-visit-common.h"
> >  #include "sysemu/reset.h"
> > +#include "exec/guest-memory-protection.h"
> >  
> >  #include "hw/boards.h"
> >  
> > @@ -119,8 +120,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;
> > @@ -172,7 +172,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;
> >      }
> >  
> > @@ -181,10 +181,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;
> > @@ -2101,13 +2104,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;
> 
> This requires some extra care:
> 
> accel/kvm/sev-stub.c:18:7: error: conflicting types for ‘sev_guest_init’
>  void *sev_guest_init(const char *id)
>        ^~~~~~~~~~~~~~
> In file included from accel/kvm/sev-stub.c:16:0:
> include/sysemu/sev.h:21:24: note: previous declaration of ‘sev_guest_init’ was here
>  GuestMemoryProtection *sev_guest_init(const char *id);
>                         ^~~~~~~~~~~~~~
> rules.mak:69: recipe for target 'accel/kvm/sev-stub.o' failed

Oops, that's embarrassing - I forgot to do a build on ppc.

Fixed now.

> > 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 d273174ad3..986c2fee51 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,26 +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);
> > -    object_class_property_set_description(oc, "sev-device",
> > -            "SEV device to use");
> > -    object_class_property_add_str(oc, "dh-cert-file",
> > -                                  sev_guest_get_dh_cert_file,
> > -                                  sev_guest_set_dh_cert_file);
> > -    object_class_property_set_description(oc, "dh-cert-file",
> > -            "guest owners DH certificate (encoded with base64)");
> > -    object_class_property_add_str(oc, "session-file",
> > -                                  sev_guest_get_session_file,
> > -                                  sev_guest_set_session_file);
> > -    object_class_property_set_description(oc, "session-file",
> > -            "guest owners session parameters (encoded with base64)");
> > -}
> > -
> >  static void
> >  sev_guest_instance_init(Object *obj)
> >  {
> > @@ -319,20 +300,6 @@ sev_guest_instance_init(Object *obj)
> >                                     OBJ_PROP_FLAG_READWRITE);
> >  }
> >  
> > -/* 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)
> >  {
> > @@ -670,7 +637,7 @@ sev_vm_state_change(void *opaque, int running, RunState state)
> >      }
> >  }
> >  
> > -void *
> > +GuestMemoryProtection *
> >  sev_guest_init(const char *id)
> >  {
> >      SevGuestState *sev;
> > @@ -748,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);
> >  
> > @@ -769,6 +736,45 @@ 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);
> > +    object_class_property_set_description(oc, "sev-device",
> > +        "SEV device to use");
> > +    object_class_property_add_str(oc, "dh-cert-file",
> > +                                  sev_guest_get_dh_cert_file,
> > +                                  sev_guest_set_dh_cert_file);
> > +    object_class_property_set_description(oc, "dh-cert-file",
> > +        "guest owners DH certificate (encoded with base64)");
> > +    object_class_property_add_str(oc, "session-file",
> > +                                  sev_guest_get_session_file,
> > +                                  sev_guest_set_session_file);
> > +    object_class_property_set_description(oc, "session-file",
> > +        "guest owners session parameters (encoded with base64)");
> > +
> > +    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)
> >  {
> 

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

Re: [RFC v2 17/18] spapr: Added PEF based guest memory protection

[David Gibson]

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

On Fri, May 29, 2020 at 12:59:40AM -0700, Ram Pai wrote:
> On Thu, May 21, 2020 at 01:43:03PM +1000, David Gibson wrote:
> > Some upcoming POWER machines have a system called PEF (Protected
> > Execution Framework) which uses a small ultravisor to allow guests to
> 
> Framework -> Facility
> 
> > 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
> 
> havint -> having
> 
> > 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.
> 
> In fact, Qemu has no direct way of knowing if the guest has turned
> secure or not, even after machine creation time. There are indirect ways
> for Qemu to know that, but nothing informs Qemu explicitly about it. 
> 
> So maybe we should just say...
> 
> "..
>  such that the guest still needs to talk to the ultravisor to enter
>  secure mode, so we can't directly know if the guest actually is secure." 
> 
> 
> > 
> > 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;
> > +}
> 
> This looks correct to me.
> 
> > +
> > +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);
> 
> Acked-by: Ram Pai <linuxram@us.ibm.com>
> 
> Thanks for doing this!
> 
> BTW: Will there be a new machine type defined for running secure VMs?

I wasn't planning on it.  Part of the point of this unified
configuration is that we can reasonably have libvirt and upper layers
tell qemu to do this without needing specific machine type hacks.

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

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Thiago Jung Bauermann]

Hello David,

David Gibson <david@gibson.dropbear.id.au> writes:

> 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

Thank you very much for this series! Using a machine property is a nice
way of configuring this.

From an end-user perspective, `-M pseries,guest-memory-protection` in
the command line already expresses everything that QEMU needs to know,
so having to add `-object pef-guest,id=pef0` seems a bit redundant. Is
it possible to make QEMU create the pef-guest object behind the scenes
when the guest-memory-protection property is specified?

Regardless, I was able to successfuly launch POWER PEF guests using
these patches:

Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

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

Is "opaque guest memory" any better? It's slightly shorter, and slightly
more precise about what the main characteristic this guest property conveys.

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [RFC v2 14/18] guest memory protection: Rework the "memory-encryption" property

[David Gibson]

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

On Mon, Jun 01, 2020 at 08:54:42PM -0700, Richard Henderson wrote:
> On 5/20/20 8:43 PM, David Gibson wrote:
> > +++ b/include/hw/boards.h
> > @@ -12,6 +12,8 @@
> >  #include "qom/object.h"
> >  #include "hw/core/cpu.h"
> >  
> > +typedef struct GuestMemoryProtection GuestMemoryProtection;
> > +
> 
> I think this needs to be in include/qemu/typedefs.h,
> and the other typedef in patch 10 needs to be moved there.
> 
> IIRC, clang warns about duplicate typedefs.

Not, apparently, with the clang version I have, but I've made the
change anyway.

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

Re: [RFC v2 14/18] guest memory protection: Rework the "memory-encryption" property

[Thomas Huth]

On 04/06/2020 07.56, David Gibson wrote:
> On Mon, Jun 01, 2020 at 08:54:42PM -0700, Richard Henderson wrote:
>> On 5/20/20 8:43 PM, David Gibson wrote:
>>> +++ b/include/hw/boards.h
>>> @@ -12,6 +12,8 @@
>>>  #include "qom/object.h"
>>>  #include "hw/core/cpu.h"
>>>  
>>> +typedef struct GuestMemoryProtection GuestMemoryProtection;
>>> +
>>
>> I think this needs to be in include/qemu/typedefs.h,
>> and the other typedef in patch 10 needs to be moved there.
>>
>> IIRC, clang warns about duplicate typedefs.
> 
> Not, apparently, with the clang version I have, but I've made the
> change anyway.

FWIW, we got rid of that duplicated typedef problem in commit
e6e90feedb706b1b92, no need to worry about that anymore.

 Thomas

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[David Gibson]

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

On Thu, Jun 04, 2020 at 01:39:22AM -0300, Thiago Jung Bauermann wrote:
> 
> Hello David,
> 
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > 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
> 
> Thank you very much for this series! Using a machine property is a nice
> way of configuring this.
> 
> >From an end-user perspective, `-M pseries,guest-memory-protection` in
> the command line already expresses everything that QEMU needs to know,
> so having to add `-object pef-guest,id=pef0` seems a bit redundant. Is
> it possible to make QEMU create the pef-guest object behind the scenes
> when the guest-memory-protection property is specified?

Not exactly - the object needs to exist for the QOM core to resolve it
before we'd have a chance to look at the value to conditionally create
the object.

What we could do, however, is always create a PEF object in the
machine, and it would just have no effect if the machine parameter
wasn't specified.

I did consider that option, but left it this way for greater
consistency with AMD SEV - there the object can't be auto-created,
since it has mandatory parameters needed to configure the encryption.

I'm open to persuasion about changing that, though.

> Regardless, I was able to successfuly launch POWER PEF guests using
> these patches:
> 
> Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

Ah, great.

> > 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.
> 
> Is "opaque guest memory" any better? It's slightly shorter, and slightly
> more precise about what the main characteristic this guest property conveys.
> 

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

Re: [RFC v2 14/18] guest memory protection: Rework the "memory-encryption" property

[David Gibson]

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

On Thu, Jun 04, 2020 at 08:19:41AM +0200, Thomas Huth wrote:
> On 04/06/2020 07.56, David Gibson wrote:
> > On Mon, Jun 01, 2020 at 08:54:42PM -0700, Richard Henderson wrote:
> >> On 5/20/20 8:43 PM, David Gibson wrote:
> >>> +++ b/include/hw/boards.h
> >>> @@ -12,6 +12,8 @@
> >>>  #include "qom/object.h"
> >>>  #include "hw/core/cpu.h"
> >>>  
> >>> +typedef struct GuestMemoryProtection GuestMemoryProtection;
> >>> +
> >>
> >> I think this needs to be in include/qemu/typedefs.h,
> >> and the other typedef in patch 10 needs to be moved there.
> >>
> >> IIRC, clang warns about duplicate typedefs.
> > 
> > Not, apparently, with the clang version I have, but I've made the
> > change anyway.
> 
> FWIW, we got rid of that duplicated typedef problem in commit
> e6e90feedb706b1b92, no need to worry about that anymore.

Ah, right.  I think I'll leave it as is though - it kind of makes
sense to have all the incomplete structure typedefs in one place.

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

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[David Gibson]

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

On Thu, Jun 04, 2020 at 01:39:22AM -0300, Thiago Jung Bauermann wrote:
> 
> Hello David,
> 
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > 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
> 
> Thank you very much for this series! Using a machine property is a nice
> way of configuring this.
> 
> >From an end-user perspective, `-M pseries,guest-memory-protection` in
> the command line already expresses everything that QEMU needs to know,
> so having to add `-object pef-guest,id=pef0` seems a bit redundant. Is
> it possible to make QEMU create the pef-guest object behind the scenes
> when the guest-memory-protection property is specified?
> 
> Regardless, I was able to successfuly launch POWER PEF guests using
> these patches:
> 
> Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> 
> > 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.
> 
> Is "opaque guest memory" any better? It's slightly shorter, and slightly
> more precise about what the main characteristic this guest property conveys.

That's not a bad one, but for now I'm going with "host trust
limitation", since this might end up covering things other than just
memory protection.

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

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Greg Kurz]

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

On Thu, 4 Jun 2020 16:44:14 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 04, 2020 at 01:39:22AM -0300, Thiago Jung Bauermann wrote:
> > 
> > Hello David,
> > 
> > David Gibson <david@gibson.dropbear.id.au> writes:
> > 
> > > 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
> > 
> > Thank you very much for this series! Using a machine property is a nice
> > way of configuring this.
> > 
> > >From an end-user perspective, `-M pseries,guest-memory-protection` in
> > the command line already expresses everything that QEMU needs to know,
> > so having to add `-object pef-guest,id=pef0` seems a bit redundant. Is
> > it possible to make QEMU create the pef-guest object behind the scenes
> > when the guest-memory-protection property is specified?
> > 
> > Regardless, I was able to successfuly launch POWER PEF guests using
> > these patches:
> > 
> > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > 
> > > 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.
> > 
> > Is "opaque guest memory" any better? It's slightly shorter, and slightly
> > more precise about what the main characteristic this guest property conveys.
> 
> That's not a bad one, but for now I'm going with "host trust
> limitation", since this might end up covering things other than just
> memory protection.
> 

Any idea what these other things might be ? It seems a bit hard to
decide of a proper name without a broader picture at this point.

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

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Sean Christopherson]

On Thu, Jun 04, 2020 at 01:11:29PM +1000, David Gibson wrote:
> On Mon, Jun 01, 2020 at 10:16:18AM +0100, Dr. David Alan Gilbert wrote:
> > * Sean Christopherson (sean.j.christopherson@intel.com) wrote:
> > > On Thu, May 21, 2020 at 01:42:46PM +1000, David Gibson wrote:
> > > > 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.
> > > 
> > > Many of the features are also going far beyond just protecting memory, so
> > > even the "memory" part feels wrong.  Maybe something like protected-guest
> > > or secure-guest?
> > > 
> > > A little imprecision isn't necessarily a bad thing, e.g. memory-encryption
> > > is quite precise, but also wrong once it encompasses anything beyond plain
> > > old encryption.
> > 
> > The common thread I think is 'untrusted host' - but I don't know of a
> > better way to describe that.
> 
> Hrm..  UntrustedHost? CompromisedHostMitigation? HostTrustMitigation
> (that way it has the same abbreviation as hardware transactional
> memory for extra confusion)?  HypervisorPowerLimitation?

GuestWithTrustIssues?  Then we can shorten it to InsecureGuest and cause all
kinds of confusion :-D.

> HostTrustLimitation? "HTL". That's not too bad, actually, I might go
> with that unless someone suggests something better.

DePrivelegedHost?  "DPH".  The "de-privelege" phrase seems to be another
recurring theme.

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Thiago Jung Bauermann]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Thu, Jun 04, 2020 at 01:39:22AM -0300, Thiago Jung Bauermann wrote:
>> 
>> Hello David,
>> 
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > 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
>> 
>> Thank you very much for this series! Using a machine property is a nice
>> way of configuring this.
>> 
>> >From an end-user perspective, `-M pseries,guest-memory-protection` in
>> the command line already expresses everything that QEMU needs to know,
>> so having to add `-object pef-guest,id=pef0` seems a bit redundant. Is
>> it possible to make QEMU create the pef-guest object behind the scenes
>> when the guest-memory-protection property is specified?
>
> Not exactly - the object needs to exist for the QOM core to resolve it
> before we'd have a chance to look at the value to conditionally create
> the object.
>
> What we could do, however, is always create a PEF object in the
> machine, and it would just have no effect if the machine parameter
> wasn't specified.
>
> I did consider that option, but left it this way for greater
> consistency with AMD SEV - there the object can't be auto-created,
> since it has mandatory parameters needed to configure the encryption.
>
> I'm open to persuasion about changing that, though.

What about having it both ways?

QEMU could always create a PEF object, and if the command line defines
one, it will correspond to it. And if the command line doesn't define one,
then it would also work because the PEF object is already there.

That way, compatibility with AMD SEV is preserved but we also get
command line simplicity where it is not needed.


-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Paolo Bonzini]

On 04/06/20 23:54, Thiago Jung Bauermann wrote:
> QEMU could always create a PEF object, and if the command line defines
> one, it will correspond to it. And if the command line doesn't define one,
> then it would also work because the PEF object is already there.

How would you start a non-protected VM?  Currently it's the "-machine"
property that decides that, and the argument requires an id
corresponding to "-object".

Paolo

> That way, compatibility with AMD SEV is preserved but we also get
> command line simplicity where it is not needed.

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Thiago Jung Bauermann]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 04/06/20 23:54, Thiago Jung Bauermann wrote:
>> QEMU could always create a PEF object, and if the command line defines
>> one, it will correspond to it. And if the command line doesn't define one,
>> then it would also work because the PEF object is already there.
>
> How would you start a non-protected VM?

In the case of POWER PEF even with the machine property and the
pef-guest object it's not guaranteed that the VM will be protected. They
allow the possibility of the VM being protected. The decision lies with
the guest. The Linux kernel will request being moved to "secure memory"
when the `svm=on` parameter is passed in the kernel command line.

To start a VM that doesn't have the possibility of being protected, one
would simply not use the guest-memory-protection property (or
host-trust-limitation, if that ends up being its name). Regardless of
whether there's a pef-guest object.

Sorry if the above is pedantic. I just want to make sure we're
communicating clearly.

> Currently it's the "-machine"
> property that decides that, and the argument requires an id
> corresponding to "-object".

If there's only one object, there's no need to specify its id.

I have the feeling I didn't understand your point. I hope these answers
clarify what I'm suggesting.

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Paolo Bonzini]

On 05/06/20 01:30, Thiago Jung Bauermann wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>> On 04/06/20 23:54, Thiago Jung Bauermann wrote:
>>> QEMU could always create a PEF object, and if the command line defines
>>> one, it will correspond to it. And if the command line doesn't define one,
>>> then it would also work because the PEF object is already there.
>>
>> How would you start a non-protected VM?
>> Currently it's the "-machine"
>> property that decides that, and the argument requires an id
>> corresponding to "-object".
> 
> If there's only one object, there's no need to specify its id.

This answers my question.  However, the property is defined for all
machines (it's in the "machine" class), so if it takes the id for one
machine it does so for all of them.

Paolo

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[Cornelia Huck]

On Thu, 21 May 2020 13:43:04 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

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

cc: Halil, who had been looking at the interaction of virtio-ccw
devices and s390 protected virt.

(IIRC, we wanted to try with a on/off/auto property for virtio-ccw?)

> 
> 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 88d699bceb..cb6580954e 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);
> @@ -1159,6 +1161,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);

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Cornelia Huck]

On Thu, 21 May 2020 13:42:46 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

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

For s390, there's the 'unpack' cpu facility bit, which is indicated iff
the kernel indicates availability of the feature (depending on hardware
support). If that cpu facility is available, a guest can choose to
transition into protected mode. The current state (protected mode or
not) is tracked in the s390 ccw machine.

If I understand the series here correctly (I only did a quick
read-through), the user has to instruct QEMU to make protection
available, via a new machine property that links to an object?

> 
> 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.
> 
> Changes since v1:
>  * Rebased
>  * Fixed some errors pointed out by Dave Gilbert
> 
> 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
>   guest memory protection: Add Error ** to
>     GuestMemoryProtection::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                      | 351 +++++++++++++------------
>  target/i386/sev_i386.h                 |  49 ----
>  target/ppc/Makefile.objs               |   2 +-
>  target/ppc/pef.c                       |  81 ++++++
>  15 files changed, 441 insertions(+), 299 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
> 

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[Halil Pasic]

On Fri, 5 Jun 2020 12:45:35 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu, 21 May 2020 13:43:04 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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.
> 
> cc: Halil, who had been looking at the interaction of virtio-ccw
> devices and s390 protected virt.
> 
> (IIRC, we wanted to try with a on/off/auto property for virtio-ccw?)

Thanks Connie! I will definitely look into this. And regarding
on/off/auto I will try to send out something today.

Regards,
Halil

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Thiago Jung Bauermann]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 05/06/20 01:30, Thiago Jung Bauermann wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> On 04/06/20 23:54, Thiago Jung Bauermann wrote:
>>>> QEMU could always create a PEF object, and if the command line defines
>>>> one, it will correspond to it. And if the command line doesn't define one,
>>>> then it would also work because the PEF object is already there.
>>>
>>> How would you start a non-protected VM?
>>> Currently it's the "-machine"
>>> property that decides that, and the argument requires an id
>>> corresponding to "-object".
>>
>> If there's only one object, there's no need to specify its id.
>
> This answers my question.  However, the property is defined for all
> machines (it's in the "machine" class), so if it takes the id for one
> machine it does so for all of them.

I don't understand much about QEMU internals, so perhaps it's not
practical to implement but from an end-user perspective I think this
logic can apply to all architectures (since my understanding is that all
of them use only one object): make the id optional. If it's not
specified, then there must be only one object, and the property will
implicitly refer to it.

Then, if an architecture doesn't need to specify parameters at object
creation time, it can be implicitly created and the user doesn't have to
worry about this detail.

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[David Gibson]

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

On Fri, Jun 05, 2020 at 05:01:07PM -0300, Thiago Jung Bauermann wrote:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 05/06/20 01:30, Thiago Jung Bauermann wrote:
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >>> On 04/06/20 23:54, Thiago Jung Bauermann wrote:
> >>>> QEMU could always create a PEF object, and if the command line defines
> >>>> one, it will correspond to it. And if the command line doesn't define one,
> >>>> then it would also work because the PEF object is already there.
> >>>
> >>> How would you start a non-protected VM?
> >>> Currently it's the "-machine"
> >>> property that decides that, and the argument requires an id
> >>> corresponding to "-object".
> >>
> >> If there's only one object, there's no need to specify its id.
> >
> > This answers my question.  However, the property is defined for all
> > machines (it's in the "machine" class), so if it takes the id for one
> > machine it does so for all of them.
> 
> I don't understand much about QEMU internals, so perhaps it's not
> practical to implement but from an end-user perspective I think this
> logic can apply to all architectures (since my understanding is that all
> of them use only one object): make the id optional. If it's not
> specified, then there must be only one object, and the property will
> implicitly refer to it.
> 
> Then, if an architecture doesn't need to specify parameters at object
> creation time, it can be implicitly created and the user doesn't have to
> worry about this detail.

Seems overly complicated to me.  We could just have it always take an
ID, but for platforms with no extra configuration make the
pre-fabricated object available under a well-known name.

That's essentially the same as the way you can add a device to the
"pci.0" bus without having to instantiate and name that bus yourself.

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

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[David Gibson]

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

On Fri, Jun 05, 2020 at 12:55:05PM +0200, Cornelia Huck wrote:
> On Thu, 21 May 2020 13:42:46 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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.
> 
> For s390, there's the 'unpack' cpu facility bit, which is indicated iff
> the kernel indicates availability of the feature (depending on hardware
> support). If that cpu facility is available, a guest can choose to
> transition into protected mode. The current state (protected mode or
> not) is tracked in the s390 ccw machine.
> 
> If I understand the series here correctly (I only did a quick
> read-through), the user has to instruct QEMU to make protection
> available, via a new machine property that links to an object?

Correct.  We used to have basically the same model for POWER - the
guest just talks to the ultravisor to enter secure mode.  But we
realized that model is broken.  You're effectively advertising
availability of a guest hardware feature based on host kernel or
hardware properties.  That means if you try to migrate from a host
with the facility to one without, you won't know there's a problem
until too late.

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

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[David Gibson]

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

On Thu, Jun 04, 2020 at 11:08:21AM +0200, Greg Kurz wrote:
> On Thu, 4 Jun 2020 16:44:14 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 04, 2020 at 01:39:22AM -0300, Thiago Jung Bauermann wrote:
> > > 
> > > Hello David,
> > > 
> > > David Gibson <david@gibson.dropbear.id.au> writes:
> > > 
> > > > 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
> > > 
> > > Thank you very much for this series! Using a machine property is a nice
> > > way of configuring this.
> > > 
> > > >From an end-user perspective, `-M pseries,guest-memory-protection` in
> > > the command line already expresses everything that QEMU needs to know,
> > > so having to add `-object pef-guest,id=pef0` seems a bit redundant. Is
> > > it possible to make QEMU create the pef-guest object behind the scenes
> > > when the guest-memory-protection property is specified?
> > > 
> > > Regardless, I was able to successfuly launch POWER PEF guests using
> > > these patches:
> > > 
> > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > > 
> > > > 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.
> > > 
> > > Is "opaque guest memory" any better? It's slightly shorter, and slightly
> > > more precise about what the main characteristic this guest property conveys.
> > 
> > That's not a bad one, but for now I'm going with "host trust
> > limitation", since this might end up covering things other than just
> > memory protection.
> 
> Any idea what these other things might be ? It seems a bit hard to
> decide of a proper name without a broader picture at this point.

Well, at the very least there needs to be protection of the guest's
register state from the hypervisor (which may be indirectly implied by
protection of memory).

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

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[Michael S. Tsirkin]

On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:
> 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 88d699bceb..cb6580954e 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);
> @@ -1159,6 +1161,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");
>      }
>  

I think it's a reasonable way to address this overall.
As Cornelia has commented, addressing ccw as well as cases where user has
specified the property manually could be worth-while.

>      machine_class->init(machine);
> -- 
> 2.26.2

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[David Gibson]

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

On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:
> > 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 88d699bceb..cb6580954e 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);
> > @@ -1159,6 +1161,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");
> >      }
> >  
> 
> I think it's a reasonable way to address this overall.
> As Cornelia has commented, addressing ccw as well

Sure.  I was assuming somebody who actually knows ccw could do that as
a follow up.

> as cases where user has
> specified the property manually could be worth-while.

I don't really see what's to be done there.  I'm assuming that if the
user specifies it, they know what they're doing - particularly with
nonstandard guests there are some odd edge cases where those
combinations might work, they're just not very likely.

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

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Thiago Jung Bauermann]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Fri, Jun 05, 2020 at 05:01:07PM -0300, Thiago Jung Bauermann wrote:
>> 
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > On 05/06/20 01:30, Thiago Jung Bauermann wrote:
>> >> Paolo Bonzini <pbonzini@redhat.com> writes:
>> >>> On 04/06/20 23:54, Thiago Jung Bauermann wrote:
>> >>>> QEMU could always create a PEF object, and if the command line defines
>> >>>> one, it will correspond to it. And if the command line doesn't define one,
>> >>>> then it would also work because the PEF object is already there.
>> >>>
>> >>> How would you start a non-protected VM?
>> >>> Currently it's the "-machine"
>> >>> property that decides that, and the argument requires an id
>> >>> corresponding to "-object".
>> >>
>> >> If there's only one object, there's no need to specify its id.
>> >
>> > This answers my question.  However, the property is defined for all
>> > machines (it's in the "machine" class), so if it takes the id for one
>> > machine it does so for all of them.
>> 
>> I don't understand much about QEMU internals, so perhaps it's not
>> practical to implement but from an end-user perspective I think this
>> logic can apply to all architectures (since my understanding is that all
>> of them use only one object): make the id optional. If it's not
>> specified, then there must be only one object, and the property will
>> implicitly refer to it.
>> 
>> Then, if an architecture doesn't need to specify parameters at object
>> creation time, it can be implicitly created and the user doesn't have to
>> worry about this detail.
>
> Seems overly complicated to me.  We could just have it always take an
> ID, but for platforms with no extra configuration make the
> pre-fabricated object available under a well-known name.
>
> That's essentially the same as the way you can add a device to the
> "pci.0" bus without having to instantiate and name that bus yourself.

Ok, that sounds good to me.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[Halil Pasic]

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

On Sat, 6 Jun 2020 18:44:09 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jun 05, 2020 at 12:55:05PM +0200, Cornelia Huck wrote:
> > On Thu, 21 May 2020 13:42:46 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > 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.
> > 
> > For s390, there's the 'unpack' cpu facility bit, which is indicated iff
> > the kernel indicates availability of the feature (depending on hardware
> > support). If that cpu facility is available, a guest can choose to
> > transition into protected mode. The current state (protected mode or
> > not) is tracked in the s390 ccw machine.
> > 
> > If I understand the series here correctly (I only did a quick
> > read-through), the user has to instruct QEMU to make protection
> > available, via a new machine property that links to an object?
> 
> Correct.  We used to have basically the same model for POWER - the
> guest just talks to the ultravisor to enter secure mode.  But we
> realized that model is broken.  You're effectively advertising
> availability of a guest hardware feature based on host kernel or
> hardware properties.  That means if you try to migrate from a host
> with the facility to one without, you won't know there's a problem
> until too late.
> 

Sorry, I don't quite understand the migration problem described here. If
you have this modeled via a CPU model facility, then you can't migrate
from a host with the facility to one without, except if the user
specified CPU model does not include the facility in question. Or am I
missing something?

Regards,
Halil

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[Cornelia Huck]

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

On Sun, 7 Jun 2020 13:07:35 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:  
> > > 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 88d699bceb..cb6580954e 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);
> > > @@ -1159,6 +1161,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");
> > >      }
> > >    
> > 
> > I think it's a reasonable way to address this overall.
> > As Cornelia has commented, addressing ccw as well  
> 
> Sure.  I was assuming somebody who actually knows ccw could do that as
> a follow up.

FWIW, I think we could simply enable iommu_platform for protected
guests for ccw; no prereqs like pci's disable-legacy.

> 
> > as cases where user has
> > specified the property manually could be worth-while.  
> 
> I don't really see what's to be done there.  I'm assuming that if the
> user specifies it, they know what they're doing - particularly with
> nonstandard guests there are some odd edge cases where those
> combinations might work, they're just not very likely.

If I understood Halil correctly, devices without iommu_platform
apparently can crash protected guests on s390. Is that supposed to be a
"if it breaks, you get to keep the pieces" situation, or do we really
want to enforce iommu_platform?

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

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[Halil Pasic]

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

On Tue, 9 Jun 2020 12:16:41 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Sun, 7 Jun 2020 13:07:35 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:  
> > > > 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 88d699bceb..cb6580954e 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);
> > > > @@ -1159,6 +1161,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");
> > > >      }
> > > >    
> > > 
> > > I think it's a reasonable way to address this overall.
> > > As Cornelia has commented, addressing ccw as well  
> > 
> > Sure.  I was assuming somebody who actually knows ccw could do that as
> > a follow up.
> 
> FWIW, I think we could simply enable iommu_platform for protected
> guests for ccw; no prereqs like pci's disable-legacy.
> 

For s390x having a memory-encryption object is not prereq for doing
protected virtualization, so the scheme does not work for us right now.

I hope Jansoch will chime in after he is back from his vacation. IMHO
having a memory-protection object will come in handy for migration,
but the presence or absence of this object should be largely transparent
to the user (and not something that needs to be explicitly managed via
command line). AFAIU this object is in the end it is just QEMU plumbing.

> > 
> > > as cases where user has
> > > specified the property manually could be worth-while.  
> > 
> > I don't really see what's to be done there.  I'm assuming that if the
> > user specifies it, they know what they're doing - particularly with
> > nonstandard guests there are some odd edge cases where those
> > combinations might work, they're just not very likely.
> 
> If I understood Halil correctly, devices without iommu_platform
> apparently can crash protected guests on s390. Is that supposed to be a
> "if it breaks, you get to keep the pieces" situation, or do we really
> want to enforce iommu_platform?

I strongly oppose to adopting the "if it breaks, you get to keep the
pieces" strategy here. It is borderline acceptable on startup, although
IMHO not preferable, but a device hotplug bringing down a guest that is
already running userspace is not acceptable at all.

Regards,
Halil

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[Cornelia Huck]

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

On Tue, 9 Jun 2020 17:40:46 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Tue, 9 Jun 2020 12:16:41 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Sun, 7 Jun 2020 13:07:35 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:  
> > > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:    
> > > > > 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 88d699bceb..cb6580954e 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);
> > > > > @@ -1159,6 +1161,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");
> > > > >      }
> > > > >      
> > > > 
> > > > I think it's a reasonable way to address this overall.
> > > > As Cornelia has commented, addressing ccw as well    
> > > 
> > > Sure.  I was assuming somebody who actually knows ccw could do that as
> > > a follow up.  
> > 
> > FWIW, I think we could simply enable iommu_platform for protected
> > guests for ccw; no prereqs like pci's disable-legacy.
> >   
> 
> For s390x having a memory-encryption object is not prereq for doing
> protected virtualization, so the scheme does not work for us right now.

Yeah, that would obviously need to be added first.

> 
> I hope Jansoch will chime in after he is back from his vacation. IMHO
> having a memory-protection object will come in handy for migration,
> but the presence or absence of this object should be largely transparent
> to the user (and not something that needs to be explicitly managed via
> command line). AFAIU this object is in the end it is just QEMU plumbing.
> 
> > >   
> > > > as cases where user has
> > > > specified the property manually could be worth-while.    
> > > 
> > > I don't really see what's to be done there.  I'm assuming that if the
> > > user specifies it, they know what they're doing - particularly with
> > > nonstandard guests there are some odd edge cases where those
> > > combinations might work, they're just not very likely.  
> > 
> > If I understood Halil correctly, devices without iommu_platform
> > apparently can crash protected guests on s390. Is that supposed to be a
> > "if it breaks, you get to keep the pieces" situation, or do we really
> > want to enforce iommu_platform?  
> 
> I strongly oppose to adopting the "if it breaks, you get to keep the
> pieces" strategy here. It is borderline acceptable on startup, although
> IMHO not preferable, but a device hotplug bringing down a guest that is
> already running userspace is not acceptable at all.

There's still the option to fail to add such a device, though.

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

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[Michael S. Tsirkin]

On Tue, Jun 09, 2020 at 05:40:46PM +0200, Halil Pasic wrote:
> For s390x having a memory-encryption object is not prereq for doing
> protected virtualization, so the scheme does not work for us right now.

It does make things much easier implementation-wise while just
marginally harder to use though.

-- 
MST

Re: [RFC v2 00/18] Refactor configuration of guest memory protection

[David Gibson]

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

On Tue, Jun 09, 2020 at 12:11:05PM +0200, Halil Pasic wrote:
> On Sat, 6 Jun 2020 18:44:09 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jun 05, 2020 at 12:55:05PM +0200, Cornelia Huck wrote:
> > > On Thu, 21 May 2020 13:42:46 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > 
> > > > 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.
> > > 
> > > For s390, there's the 'unpack' cpu facility bit, which is indicated iff
> > > the kernel indicates availability of the feature (depending on hardware
> > > support). If that cpu facility is available, a guest can choose to
> > > transition into protected mode. The current state (protected mode or
> > > not) is tracked in the s390 ccw machine.
> > > 
> > > If I understand the series here correctly (I only did a quick
> > > read-through), the user has to instruct QEMU to make protection
> > > available, via a new machine property that links to an object?
> > 
> > Correct.  We used to have basically the same model for POWER - the
> > guest just talks to the ultravisor to enter secure mode.  But we
> > realized that model is broken.  You're effectively advertising
> > availability of a guest hardware feature based on host kernel or
> > hardware properties.  That means if you try to migrate from a host
> > with the facility to one without, you won't know there's a problem
> > until too late.
> > 
> 
> Sorry, I don't quite understand the migration problem described here. If
> you have this modeled via a CPU model facility, then you can't migrate
> from a host with the facility to one without, except if the user
> specified CPU model does not include the facility in question. Or am I
> missing something?

Ah, sorry, my mistake.  If it's all based on a cpu model facility that
must be explicitly selected by the user (not based on host
capbilities) then that's ok as well.

The problem comes if you base guest availability on host availability,
which early proposals for the POWER version did involve.

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

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[David Gibson]

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

On Tue, Jun 09, 2020 at 12:16:41PM +0200, Cornelia Huck wrote:
> On Sun, 7 Jun 2020 13:07:35 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:  
> > > > 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 88d699bceb..cb6580954e 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);
> > > > @@ -1159,6 +1161,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");
> > > >      }
> > > >    
> > > 
> > > I think it's a reasonable way to address this overall.
> > > As Cornelia has commented, addressing ccw as well  
> > 
> > Sure.  I was assuming somebody who actually knows ccw could do that as
> > a follow up.
> 
> FWIW, I think we could simply enable iommu_platform for protected
> guests for ccw; no prereqs like pci's disable-legacy.

Right, and the code above should in fact already do so, since it
applies that to TYPE_VIRTIO_DEVICE, which is common.  The
disable-legacy part should be harmless for s390, since this is
effectively just setting a default, and we don't expect any
TYPE_VIRTIO_PCI devices to be instantiated on z.

> > > as cases where user has
> > > specified the property manually could be worth-while.  
> > 
> > I don't really see what's to be done there.  I'm assuming that if the
> > user specifies it, they know what they're doing - particularly with
> > nonstandard guests there are some odd edge cases where those
> > combinations might work, they're just not very likely.
> 
> If I understood Halil correctly, devices without iommu_platform
> apparently can crash protected guests on s390. Is that supposed to be a
> "if it breaks, you get to keep the pieces" situation, or do we really
> want to enforce iommu_platform?

I actually think "if you broke it, keep the pieces" is an acceptable
approach here, but that doesn't preclude some further enforcement to
improve UX.

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

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[David Gibson]

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

On Tue, Jun 09, 2020 at 05:40:46PM +0200, Halil Pasic wrote:
> On Tue, 9 Jun 2020 12:16:41 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Sun, 7 Jun 2020 13:07:35 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> > > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:  
> > > > > 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 88d699bceb..cb6580954e 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);
> > > > > @@ -1159,6 +1161,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");
> > > > >      }
> > > > >    
> > > > 
> > > > I think it's a reasonable way to address this overall.
> > > > As Cornelia has commented, addressing ccw as well  
> > > 
> > > Sure.  I was assuming somebody who actually knows ccw could do that as
> > > a follow up.
> > 
> > FWIW, I think we could simply enable iommu_platform for protected
> > guests for ccw; no prereqs like pci's disable-legacy.
> 
> For s390x having a memory-encryption object is not prereq for doing
> protected virtualization, so the scheme does not work for us right now.

That's basically true for POWER as well - in our case the "memory
encrypt" object (called "host trust limitation" (HTL) object in the
latest version) is basically just a dummy with no parameters.  The
same should work for s390x.

I am considering having the machine always create the HTL object with
a well-known name (e.g. "pef0"), so you can just set the machine
property to it to enable PEF.  Again, that could also be done on
s390x.

Note also that anything could in principle implement the HTL
interface.  So you could have the machine object itelf, or the cpu
implement the interface to avoid creating a dummy object, though that
might get messier that just having a dummy in the long run.

> I hope Jansoch will chime in after he is back from his vacation. IMHO
> having a memory-protection object will come in handy for migration,
> but the presence or absence of this object should be largely transparent
> to the user (and not something that needs to be explicitly managed via
> command line). AFAIU this object is in the end it is just QEMU plumbing.

Yes.  However, if either POWER or z ever gets any configurable knobs
for their protection systems, it does provide an obvious place that we
can do that configuration.

> > > > as cases where user has
> > > > specified the property manually could be worth-while.  
> > > 
> > > I don't really see what's to be done there.  I'm assuming that if the
> > > user specifies it, they know what they're doing - particularly with
> > > nonstandard guests there are some odd edge cases where those
> > > combinations might work, they're just not very likely.
> > 
> > If I understood Halil correctly, devices without iommu_platform
> > apparently can crash protected guests on s390. Is that supposed to be a
> > "if it breaks, you get to keep the pieces" situation, or do we really
> > want to enforce iommu_platform?
> 
> I strongly oppose to adopting the "if it breaks, you get to keep the
> pieces" strategy here. It is borderline acceptable on startup, although
> IMHO not preferable, but a device hotplug bringing down a guest that is
> already running userspace is not acceptable at all.
> 
> Regards,
> Halil



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

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[Cornelia Huck]

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

On Wed, 10 Jun 2020 14:39:22 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jun 09, 2020 at 12:16:41PM +0200, Cornelia Huck wrote:
> > On Sun, 7 Jun 2020 13:07:35 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:  
> > > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:    
> > > > > 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 88d699bceb..cb6580954e 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);
> > > > > @@ -1159,6 +1161,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");
> > > > >      }
> > > > >      
> > > > 
> > > > I think it's a reasonable way to address this overall.
> > > > As Cornelia has commented, addressing ccw as well    
> > > 
> > > Sure.  I was assuming somebody who actually knows ccw could do that as
> > > a follow up.  
> > 
> > FWIW, I think we could simply enable iommu_platform for protected
> > guests for ccw; no prereqs like pci's disable-legacy.  
> 
> Right, and the code above should in fact already do so, since it
> applies that to TYPE_VIRTIO_DEVICE, which is common.  The
> disable-legacy part should be harmless for s390, since this is
> effectively just setting a default, and we don't expect any
> TYPE_VIRTIO_PCI devices to be instantiated on z.

Well, virtio-pci is available on s390, so people could try to use it --
however, forcing disable-legacy won't hurt in that case, as it won't
make the situation worse (I don't expect virtio-pci to work on s390
protected guests.)

> 
> > > > as cases where user has
> > > > specified the property manually could be worth-while.    
> > > 
> > > I don't really see what's to be done there.  I'm assuming that if the
> > > user specifies it, they know what they're doing - particularly with
> > > nonstandard guests there are some odd edge cases where those
> > > combinations might work, they're just not very likely.  
> > 
> > If I understood Halil correctly, devices without iommu_platform
> > apparently can crash protected guests on s390. Is that supposed to be a
> > "if it breaks, you get to keep the pieces" situation, or do we really
> > want to enforce iommu_platform?  
> 
> I actually think "if you broke it, keep the pieces" is an acceptable
> approach here, but that doesn't preclude some further enforcement to
> improve UX.

I'm worried about spreading dealing with this over too many code areas,
though.

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

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[David Gibson]

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

On Wed, Jun 10, 2020 at 10:48:42AM +0200, Cornelia Huck wrote:
> On Wed, 10 Jun 2020 14:39:22 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Jun 09, 2020 at 12:16:41PM +0200, Cornelia Huck wrote:
> > > On Sun, 7 Jun 2020 13:07:35 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:  
> > > > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:    
> > > > > > 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 88d699bceb..cb6580954e 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);
> > > > > > @@ -1159,6 +1161,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");
> > > > > >      }
> > > > > >      
> > > > > 
> > > > > I think it's a reasonable way to address this overall.
> > > > > As Cornelia has commented, addressing ccw as well    
> > > > 
> > > > Sure.  I was assuming somebody who actually knows ccw could do that as
> > > > a follow up.  
> > > 
> > > FWIW, I think we could simply enable iommu_platform for protected
> > > guests for ccw; no prereqs like pci's disable-legacy.  
> > 
> > Right, and the code above should in fact already do so, since it
> > applies that to TYPE_VIRTIO_DEVICE, which is common.  The
> > disable-legacy part should be harmless for s390, since this is
> > effectively just setting a default, and we don't expect any
> > TYPE_VIRTIO_PCI devices to be instantiated on z.
> 
> Well, virtio-pci is available on s390, so people could try to use it --
> however, forcing disable-legacy won't hurt in that case, as it won't
> make the situation worse (I don't expect virtio-pci to work on s390
> protected guests.)

Sure, and if by whatever chance it does work, then you'll need
iommu_platform, and therefore disable-legacy for it.

> > > > > as cases where user has
> > > > > specified the property manually could be worth-while.    
> > > > 
> > > > I don't really see what's to be done there.  I'm assuming that if the
> > > > user specifies it, they know what they're doing - particularly with
> > > > nonstandard guests there are some odd edge cases where those
> > > > combinations might work, they're just not very likely.  
> > > 
> > > If I understood Halil correctly, devices without iommu_platform
> > > apparently can crash protected guests on s390. Is that supposed to be a
> > > "if it breaks, you get to keep the pieces" situation, or do we really
> > > want to enforce iommu_platform?  
> > 
> > I actually think "if you broke it, keep the pieces" is an acceptable
> > approach here, but that doesn't preclude some further enforcement to
> > improve UX.
> 
> I'm worried about spreading dealing with this over too many code areas,
> though.



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

Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[Halil Pasic]

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

On Wed, 10 Jun 2020 10:48:42 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 10 Jun 2020 14:39:22 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Jun 09, 2020 at 12:16:41PM +0200, Cornelia Huck wrote:
> > > On Sun, 7 Jun 2020 13:07:35 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:  
> > > > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:    
> > > > > > 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 88d699bceb..cb6580954e 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);
> > > > > > @@ -1159,6 +1161,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");
> > > > > >      }
> > > > > >      
> > > > > 
> > > > > I think it's a reasonable way to address this overall.
> > > > > As Cornelia has commented, addressing ccw as well    
> > > > 
> > > > Sure.  I was assuming somebody who actually knows ccw could do that as
> > > > a follow up.  
> > > 
> > > FWIW, I think we could simply enable iommu_platform for protected
> > > guests for ccw; no prereqs like pci's disable-legacy.  
> > 
> > Right, and the code above should in fact already do so, since it
> > applies that to TYPE_VIRTIO_DEVICE, which is common.  The
> > disable-legacy part should be harmless for s390, since this is
> > effectively just setting a default, and we don't expect any
> > TYPE_VIRTIO_PCI devices to be instantiated on z.
> 
> Well, virtio-pci is available on s390, so people could try to use it --
> however, forcing disable-legacy won't hurt in that case, as it won't
> make the situation worse (I don't expect virtio-pci to work on s390
> protected guests.)

Currently we don't have to worry about virtio-pci when running in
protected mode, because PCI is not supported currently, and this is
enforced by the ultravisor which would inject operation exceptions should
a PV guest try to use our PCI instructions.

[..]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]