[PATCH v3 0/9] Generalize memory encryption models

[PATCH v3 0/9] Generalize memory encryption models

[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
"host-trust-limitation" 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.

Please apply.

Changes since RFCv2:
 * Rebased
 * Removed preliminary SEV cleanups (they've been merged)
 * Changed name to "host trust limitation"
 * Added migration blocker to the PEF code (based on SEV's version)
Changes since RFCv1:
 * Rebased
 * Fixed some errors pointed out by Dave Gilbert

David Gibson (9):
  host trust limitation: Introduce new host trust limitation interface
  host trust limitation: Handle memory encryption via interface
  host trust limitation: Move side effect out of
    machine_set_memory_encryption()
  host trust limitation: Rework the "memory-encryption" property
  host trust limitation: Decouple kvm_memcrypt_*() helpers from KVM
  host trust limitation: Add Error ** to HostTrustLimitation::kvm_init
  spapr: Add PEF based host trust limitation
  spapr: PEF: block migration
  host trust limitation: Alter virtio default properties for protected
    guests

 accel/kvm/kvm-all.c                  |  40 ++------
 accel/kvm/sev-stub.c                 |   7 +-
 accel/stubs/kvm-stub.c               |  10 --
 backends/Makefile.objs               |   2 +
 backends/host-trust-limitation.c     |  29 ++++++
 hw/core/machine.c                    |  61 +++++++++--
 hw/i386/pc_sysfw.c                   |   6 +-
 include/exec/host-trust-limitation.h |  72 +++++++++++++
 include/hw/boards.h                  |   2 +-
 include/qemu/typedefs.h              |   1 +
 include/sysemu/kvm.h                 |  17 ----
 include/sysemu/sev.h                 |   4 +-
 target/i386/sev.c                    | 146 ++++++++++++---------------
 target/ppc/Makefile.objs             |   2 +-
 target/ppc/pef.c                     |  89 ++++++++++++++++
 15 files changed, 325 insertions(+), 163 deletions(-)
 create mode 100644 backends/host-trust-limitation.c
 create mode 100644 include/exec/host-trust-limitation.h
 create mode 100644 target/ppc/pef.c

-- 
2.26.2

[PATCH v3 1/9] host trust limitation: Introduce new host trust limitation 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.

To (partially) unify handling for these, this introduces a new
HostTrustLimitation QOM interface.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 backends/Makefile.objs               |  2 ++
 backends/host-trust-limitation.c     | 29 ++++++++++++++++++++++++
 include/exec/host-trust-limitation.h | 33 ++++++++++++++++++++++++++++
 include/qemu/typedefs.h              |  1 +
 4 files changed, 65 insertions(+)
 create mode 100644 backends/host-trust-limitation.c
 create mode 100644 include/exec/host-trust-limitation.h

diff --git a/backends/Makefile.objs b/backends/Makefile.objs
index 28a847cd57..af761c9ab1 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 += host-trust-limitation.o
diff --git a/backends/host-trust-limitation.c b/backends/host-trust-limitation.c
new file mode 100644
index 0000000000..96a381cd8a
--- /dev/null
+++ b/backends/host-trust-limitation.c
@@ -0,0 +1,29 @@
+/*
+ * QEMU Host Trust Limitation 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/host-trust-limitation.h"
+
+static const TypeInfo host_trust_limitation_info = {
+    .name = TYPE_HOST_TRUST_LIMITATION,
+    .parent = TYPE_INTERFACE,
+    .class_size = sizeof(HostTrustLimitationClass),
+};
+
+static void host_trust_limitation_register_types(void)
+{
+    type_register_static(&host_trust_limitation_info);
+}
+
+type_init(host_trust_limitation_register_types)
diff --git a/include/exec/host-trust-limitation.h b/include/exec/host-trust-limitation.h
new file mode 100644
index 0000000000..03887b1be1
--- /dev/null
+++ b/include/exec/host-trust-limitation.h
@@ -0,0 +1,33 @@
+/*
+ * QEMU Host Trust Limitation 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_HOST_TRUST_LIMITATION_H
+#define QEMU_HOST_TRUST_LIMITATION_H
+
+#include "qom/object.h"
+
+#define TYPE_HOST_TRUST_LIMITATION "host-trust-limitation"
+#define HOST_TRUST_LIMITATION(obj)                                    \
+    INTERFACE_CHECK(HostTrustLimitation, (obj),                       \
+                    TYPE_HOST_TRUST_LIMITATION)
+#define HOST_TRUST_LIMITATION_CLASS(klass)                            \
+    OBJECT_CLASS_CHECK(HostTrustLimitationClass, (klass),             \
+                       TYPE_HOST_TRUST_LIMITATION)
+#define HOST_TRUST_LIMITATION_GET_CLASS(obj)                          \
+    OBJECT_GET_CLASS(HostTrustLimitationClass, (obj),                 \
+                     TYPE_HOST_TRUST_LIMITATION)
+
+typedef struct HostTrustLimitationClass {
+    InterfaceClass parent;
+} HostTrustLimitationClass;
+
+#endif /* QEMU_HOST_TRUST_LIMITATION_H */
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index ce4a78b687..f75c7eb2f2 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -51,6 +51,7 @@ typedef struct FWCfgIoState FWCfgIoState;
 typedef struct FWCfgMemState FWCfgMemState;
 typedef struct FWCfgState FWCfgState;
 typedef struct HostMemoryBackend HostMemoryBackend;
+typedef struct HostTrustLimitation HostTrustLimitation;
 typedef struct I2CBus I2CBus;
 typedef struct I2SCodec I2SCodec;
 typedef struct IOMMUMemoryRegion IOMMUMemoryRegion;
-- 
2.26.2

[PATCH v3 2/9] host trust limitation: Handle memory encryption 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 host trust
limitation, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/kvm/kvm-all.c                  |  38 ++++++---
 accel/kvm/sev-stub.c                 |   7 +-
 include/exec/host-trust-limitation.h |   3 +
 include/sysemu/sev.h                 |   4 +-
 target/i386/sev.c                    | 117 +++++++++++----------------
 5 files changed, 79 insertions(+), 90 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index f24d7da783..1e43e27f45 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -39,12 +39,12 @@
 #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"
 #include "qapi/qapi-visit-common.h"
 #include "sysemu/reset.h"
+#include "exec/host-trust-limitation.h"
 
 #include "hw/boards.h"
 
@@ -118,9 +118,8 @@ struct KVMState
     KVMMemoryListener memory_listener;
     QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus;
 
-    /* memory encryption */
-    void *memcrypt_handle;
-    int (*memcrypt_encrypt_data)(void *handle, uint8_t *ptr, uint64_t len);
+    /* host trust limitation (e.g. by guest memory encryption) */
+    HostTrustLimitation *htl;
 
     /* For "info mtree -f" to tell if an MR is registered in KVM */
     int nr_as;
@@ -222,7 +221,7 @@ int kvm_get_max_memslots(void)
 
 bool kvm_memcrypt_enabled(void)
 {
-    if (kvm_state && kvm_state->memcrypt_handle) {
+    if (kvm_state && kvm_state->htl) {
         return true;
     }
 
@@ -231,10 +230,12 @@ 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);
+    HostTrustLimitation *htl = kvm_state->htl;
+
+    if (htl) {
+        HostTrustLimitationClass *htlc = HOST_TRUST_LIMITATION_GET_CLASS(htl);
+
+        return htlc->encrypt_data(htl, ptr, len);
     }
 
     return 1;
@@ -2180,13 +2181,24 @@ 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) {
+        Object *obj = object_resolve_path_component(object_get_objects_root(),
+                                                    ms->memory_encryption);
+
+        if (object_dynamic_cast(obj, TYPE_HOST_TRUST_LIMITATION)) {
+            HostTrustLimitation *htl = HOST_TRUST_LIMITATION(obj);
+            HostTrustLimitationClass *htlc
+                = HOST_TRUST_LIMITATION_GET_CLASS(htl);
+
+            ret = htlc->kvm_init(htl);
+            if (ret < 0) {
+                goto err;
+            }
+
+            kvm_state->htl = htl;
+        } else {
             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..9c7c897593 100644
--- a/accel/kvm/sev-stub.c
+++ b/accel/kvm/sev-stub.c
@@ -15,12 +15,7 @@
 #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)
+HostTrustLimitation *sev_guest_init(const char *id)
 {
     return NULL;
 }
diff --git a/include/exec/host-trust-limitation.h b/include/exec/host-trust-limitation.h
index 03887b1be1..a19f12ae14 100644
--- a/include/exec/host-trust-limitation.h
+++ b/include/exec/host-trust-limitation.h
@@ -28,6 +28,9 @@
 
 typedef struct HostTrustLimitationClass {
     InterfaceClass parent;
+
+    int (*kvm_init)(HostTrustLimitation *);
+    int (*encrypt_data)(HostTrustLimitation *, uint8_t *, uint64_t);
 } HostTrustLimitationClass;
 
 #endif /* QEMU_HOST_TRUST_LIMITATION_H */
diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 98c1ec8d38..a4aee6a87d 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -16,6 +16,6 @@
 
 #include "sysemu/kvm.h"
 
-void *sev_guest_init(const char *id);
-int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
+HostTrustLimitation *sev_guest_init(const char *id);
+
 #endif
diff --git a/target/i386/sev.c b/target/i386/sev.c
index d273174ad3..052a05d15a 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/host-trust-limitation.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,40 +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)
-{
-    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)
 {
@@ -670,23 +617,15 @@ sev_vm_state_change(void *opaque, int running, RunState state)
     }
 }
 
-void *
-sev_guest_init(const char *id)
+static int sev_kvm_init(HostTrustLimitation *htl)
 {
-    SevGuestState *sev;
+    SevGuestState *sev = SEV_GUEST(htl);
     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;
 
@@ -748,16 +687,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 0;
 err:
     sev_guest = NULL;
-    return NULL;
+    return -1;
 }
 
-int
-sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
+static int
+sev_encrypt_data(HostTrustLimitation *opaque, uint8_t *ptr, uint64_t len)
 {
-    SevGuestState *sev = handle;
+    SevGuestState *sev = SEV_GUEST(opaque);
 
     assert(sev);
 
@@ -769,6 +708,46 @@ sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
     return 0;
 }
 
+static void
+sev_guest_class_init(ObjectClass *oc, void *data)
+{
+    HostTrustLimitationClass *htlc = HOST_TRUST_LIMITATION_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)");
+
+    htlc->kvm_init = sev_kvm_init;
+    htlc->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_HOST_TRUST_LIMITATION },
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
 static void
 sev_register_types(void)
 {
-- 
2.26.2

[PATCH v3 3/9] host trust limitation: 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.

More importantly, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 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 1d80ab0e1d..fdc0c7e038 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -435,14 +435,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)
@@ -1135,6 +1127,15 @@ void machine_run_board_init(MachineState *machine)
         }
     }
 
+    if (machine->memory_encryption) {
+        /*
+         * With host trust limitation, 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

[PATCH v3 4/9] host trust limitation: 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 "host-trust-limitation" 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 |  2 +-
 3 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 1e43e27f45..d8e8fa345e 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2180,25 +2180,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_HOST_TRUST_LIMITATION)) {
-            HostTrustLimitation *htl = HOST_TRUST_LIMITATION(obj);
-            HostTrustLimitationClass *htlc
-                = HOST_TRUST_LIMITATION_GET_CLASS(htl);
-
-            ret = htlc->kvm_init(htl);
-            if (ret < 0) {
-                goto err;
-            }
+    if (ms->htl) {
+        HostTrustLimitationClass *htlc =
+            HOST_TRUST_LIMITATION_GET_CLASS(ms->htl);
 
-            kvm_state->htl = htl;
-        } else {
-            ret = -1;
+        ret = htlc->kvm_init(ms->htl);
+        if (ret < 0) {
             goto err;
         }
+
+        kvm_state->htl = ms->htl;
     }
 
     ret = kvm_arch_init(ms, s);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index fdc0c7e038..a71792bc16 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/host-trust-limitation.h"
 
 GlobalProperty hw_compat_5_0[] = {
     { "virtio-balloon-device", "page-poison", "false" },
@@ -425,16 +426,37 @@ static char *machine_get_memory_encryption(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
 
-    return g_strdup(ms->memory_encryption);
+    if (ms->htl) {
+        return object_get_canonical_path_component(OBJECT(ms->htl));
+    }
+
+    return NULL;
 }
 
 static void machine_set_memory_encryption(Object *obj, const char *value,
                                         Error **errp)
 {
-    MachineState *ms = MACHINE(obj);
+    Object *htl =
+        object_resolve_path_component(object_get_objects_root(), value);
+
+    if (!htl) {
+        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, htl, "host-trust-limitation", errp);
+}
+
+static void machine_check_host_trust_limitation(const Object *obj,
+                                                const char *name,
+                                                Object *new_target,
+                                                Error **errp)
+{
+    /*
+     * So far the only constraint is that the target has the
+     * TYPE_HOST_TRUST_LIMITATION interface, and that's checked by the
+     * QOM core
+     */
 }
 
 static bool machine_get_nvdimm(Object *obj, Error **errp)
@@ -855,6 +877,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, "host-trust-limitation",
+                                   TYPE_HOST_TRUST_LIMITATION,
+                                   offsetof(MachineState, htl),
+                                   machine_check_host_trust_limitation,
+                                   OBJ_PROP_LINK_STRONG);
+    object_class_property_set_description(oc, "host-trust-limitation",
+        "Set host trust limitation 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",
@@ -1127,7 +1158,7 @@ void machine_run_board_init(MachineState *machine)
         }
     }
 
-    if (machine->memory_encryption) {
+    if (machine->htl) {
         /*
          * With host trust limitation, 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..a9f8444729 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -277,7 +277,7 @@ struct MachineState {
     bool suppress_vmdesc;
     bool enforce_config_section;
     bool enable_graphics;
-    char *memory_encryption;
+    HostTrustLimitation *htl;
     char *ram_memdev_id;
     /*
      * convenience alias to ram_memdev_id backend memory region
-- 
2.26.2

[PATCH v3 5/9] host trust limitation: 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 host-trust-limitation.h,
taking an explicit machine parameter.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/kvm/kvm-all.c                  | 27 ---------------------
 accel/stubs/kvm-stub.c               | 10 --------
 hw/i386/pc_sysfw.c                   |  6 +++--
 include/exec/host-trust-limitation.h | 36 ++++++++++++++++++++++++++++
 include/sysemu/kvm.h                 | 17 -------------
 5 files changed, 40 insertions(+), 56 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index d8e8fa345e..9645271ca5 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;
 
-    /* host trust limitation (e.g. by guest memory encryption) */
-    HostTrustLimitation *htl;
-
     /* For "info mtree -f" to tell if an MR is registered in KVM */
     int nr_as;
     struct KVMAs {
@@ -219,28 +216,6 @@ int kvm_get_max_memslots(void)
     return s->nr_slots;
 }
 
-bool kvm_memcrypt_enabled(void)
-{
-    if (kvm_state && kvm_state->htl) {
-        return true;
-    }
-
-    return false;
-}
-
-int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)
-{
-    HostTrustLimitation *htl = kvm_state->htl;
-
-    if (htl) {
-        HostTrustLimitationClass *htlc = HOST_TRUST_LIMITATION_GET_CLASS(htl);
-
-        return htlc->encrypt_data(htl, ptr, len);
-    }
-
-    return 1;
-}
-
 /* Called with KVMMemoryListener.slots_lock held */
 static KVMSlot *kvm_get_free_slot(KVMMemoryListener *kml)
 {
@@ -2188,8 +2163,6 @@ static int kvm_init(MachineState *ms)
         if (ret < 0) {
             goto err;
         }
-
-        kvm_state->htl = ms->htl;
     }
 
     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 ec2a3b3e7e..cab5ac5695 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/host-trust-limitation.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 (host_trust_limitation_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 = host_trust_limitation_encrypt(MACHINE(pcms),
+                                                    flash_ptr, flash_size);
                 if (ret) {
                     error_report("failed to encrypt pflash rom");
                     exit(1);
diff --git a/include/exec/host-trust-limitation.h b/include/exec/host-trust-limitation.h
index a19f12ae14..fc30ea3f78 100644
--- a/include/exec/host-trust-limitation.h
+++ b/include/exec/host-trust-limitation.h
@@ -14,6 +14,7 @@
 #define QEMU_HOST_TRUST_LIMITATION_H
 
 #include "qom/object.h"
+#include "hw/boards.h"
 
 #define TYPE_HOST_TRUST_LIMITATION "host-trust-limitation"
 #define HOST_TRUST_LIMITATION(obj)                                    \
@@ -33,4 +34,39 @@ typedef struct HostTrustLimitationClass {
     int (*encrypt_data)(HostTrustLimitation *, uint8_t *, uint64_t);
 } HostTrustLimitationClass;
 
+/**
+ * host_trust_limitation_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 host_trust_limitation_enabled(MachineState *machine)
+{
+    return !!machine->htl;
+}
+
+/**
+ * host_trust_limitation_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 host_trust_limitation_encrypt(MachineState *machine,
+                                                uint8_t *ptr, uint64_t len)
+{
+    HostTrustLimitation *htl = machine->htl;
+
+    if (htl) {
+        HostTrustLimitationClass *htlc = HOST_TRUST_LIMITATION_GET_CLASS(htl);
+
+        if (htlc->encrypt_data) {
+            return htlc->encrypt_data(htl, ptr, len);
+        }
+    }
+
+    return 1;
+}
+
 #endif /* QEMU_HOST_TRUST_LIMITATION_H */
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index b4174d941c..c7b9739609 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

[PATCH v3 6/9] host trust limitation: Add Error ** to HostTrustLimitation::kvm_init

[David Gibson]

This allows failures to be reported richly and idiomatically.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/kvm/kvm-all.c                  |  4 +++-
 include/exec/host-trust-limitation.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 9645271ca5..c236ebeae0 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2158,9 +2158,11 @@ static int kvm_init(MachineState *ms)
     if (ms->htl) {
         HostTrustLimitationClass *htlc =
             HOST_TRUST_LIMITATION_GET_CLASS(ms->htl);
+        Error *local_err = NULL;
 
-        ret = htlc->kvm_init(ms->htl);
+        ret = htlc->kvm_init(ms->htl, &local_err);
         if (ret < 0) {
+            error_report_err(local_err);
             goto err;
         }
     }
diff --git a/include/exec/host-trust-limitation.h b/include/exec/host-trust-limitation.h
index fc30ea3f78..d93b537280 100644
--- a/include/exec/host-trust-limitation.h
+++ b/include/exec/host-trust-limitation.h
@@ -30,7 +30,7 @@
 typedef struct HostTrustLimitationClass {
     InterfaceClass parent;
 
-    int (*kvm_init)(HostTrustLimitation *);
+    int (*kvm_init)(HostTrustLimitation *, Error **);
     int (*encrypt_data)(HostTrustLimitation *, uint8_t *, uint64_t);
 } HostTrustLimitationClass;
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 052a05d15a..829f78436a 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(HostTrustLimitation *htl)
+static int sev_kvm_init(HostTrustLimitation *htl, Error **errp)
 {
     SevGuestState *sev = SEV_GUEST(htl);
     char *devname;
@@ -633,14 +633,14 @@ static int sev_kvm_init(HostTrustLimitation *htl)
     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(HostTrustLimitation *htl)
     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(HostTrustLimitation *htl)
     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

[PATCH v3 7/9] spapr: Add PEF based host trust limitation

[David Gibson]

Some upcoming POWER machines have a system called PEF (Protected
Execution Facility) 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 having 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 host-trust-limitation 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.  Qemu has no directly way of knowing if the guest is in
secure mode, and certainly can't know until well after machine
creation time.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Acked-by: Ram Pai <linuxram@us.ibm.com>
---
 target/ppc/Makefile.objs |  2 +-
 target/ppc/pef.c         | 83 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 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..53a6af0347
--- /dev/null
+++ b/target/ppc/pef.c
@@ -0,0 +1,83 @@
+/*
+ * PEF (Protected Execution Facility) 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"
+
+#include "qapi/error.h"
+#include "qom/object_interfaces.h"
+#include "sysemu/kvm.h"
+#include "migration/blocker.h"
+#include "exec/host-trust-limitation.h"
+
+#define TYPE_PEF_GUEST "pef-guest"
+#define PEF_GUEST(obj)                                  \
+    OBJECT_CHECK(PefGuestState, (obj), 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 ...,host-trust-limitation=pef0
+ */
+struct PefGuestState {
+    Object parent_obj;
+};
+
+static int pef_kvm_init(HostTrustLimitation *gmpo, Error **errp)
+{
+    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)
+{
+    HostTrustLimitationClass *gmpc = HOST_TRUST_LIMITATION_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_HOST_TRUST_LIMITATION },
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
+static void
+pef_register_types(void)
+{
+    type_register_static(&pef_guest_info);
+}
+
+type_init(pef_register_types);
-- 
2.26.2

[PATCH v3 8/9] spapr: PEF: block migration

[David Gibson]

We haven't yet implemented the fairly involved handshaking that will be
needed to migrate PEF protected guests.  For now, just use a migration
blocker so we get a meaningful error if someone attempts this (this is the
same approach used by AMD SEV).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/pef.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/ppc/pef.c b/target/ppc/pef.c
index 53a6af0347..6a50efd580 100644
--- a/target/ppc/pef.c
+++ b/target/ppc/pef.c
@@ -36,6 +36,8 @@ struct PefGuestState {
     Object parent_obj;
 };
 
+static Error *pef_mig_blocker;
+
 static int pef_kvm_init(HostTrustLimitation *gmpo, Error **errp)
 {
     if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_SECURE_GUEST)) {
@@ -52,6 +54,10 @@ static int pef_kvm_init(HostTrustLimitation *gmpo, Error **errp)
         }
     }
 
+    /* add migration blocker */
+    error_setg(&pef_mig_blocker, "PEF: Migration is not implemented");
+    migrate_add_blocker(pef_mig_blocker, &error_abort);
+
     return 0;
 }
 
-- 
2.26.2

[PATCH v3 9/9] host trust limitation: 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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-pci.h"
 
 GlobalProperty hw_compat_5_0[] = {
     { "virtio-balloon-device", "page-poison", "false" },
@@ -1165,6 +1167,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: [PATCH v3 0/9] Generalize memory encryption models

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200619020602.118306-1-david@gibson.dropbear.id.au/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      contrib/vhost-user-input/main.o
  LINK    tests/qemu-iotests/socket_scm_helper
  GEN     docs/interop/qemu-qmp-ref.html
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  GEN     docs/interop/qemu-qmp-ref.txt
  GEN     docs/interop/qemu-qmp-ref.7
  CC      qga/commands.o
---
  SIGN    pc-bios/optionrom/pvh.bin
  LINK    elf2dmp
  CC      qemu-img.o
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    qemu-io
  LINK    qemu-edid
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    fsdev/virtfs-proxy-helper
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    scsi/qemu-pr-helper
  LINK    qemu-bridge-helper
  AR      libvhost-user.a
---
  GEN     docs/interop/qemu-ga-ref.txt
  GEN     docs/interop/qemu-ga-ref.7
  LINK    qemu-keymap
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    ivshmem-client
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    ivshmem-server
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    qemu-nbd
  LINK    qemu-storage-daemon
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    virtiofsd
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    vhost-user-input
  LINK    qemu-ga
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    qemu-img
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  GEN     x86_64-softmmu/hmp-commands.h
  GEN     x86_64-softmmu/config-devices.h
  GEN     x86_64-softmmu/hmp-commands-info.h
---
  CC      x86_64-softmmu/gdbstub-xml.o
  CC      x86_64-softmmu/trace/generated-helpers.o
  LINK    x86_64-softmmu/qemu-system-x86_64
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
common.rc: line 50: test: check: binary operator expected
(printf '#define QEMU_PKGVERSION ""\n'; printf '#define QEMU_FULL_VERSION "5.0.50"\n'; ) > qemu-version.h.tmp
make -C /tmp/qemu-test/src/slirp BUILD_DIR="/tmp/qemu-test/build/slirp" PKG_CONFIG="pkg-config" CC="clang" AR="ar"      LD="ld" RANLIB="ranlib" CFLAGS="-I/usr/include/pixman-1   -Werror -fsanitize=undefined -fsanitize=address  -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include  -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99  -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -fstack-protector-strong   -I/usr/include/p11-kit-1   -DSTRUCT_IOVEC_DEFINED  -I/usr/include/libpng16  -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/pixman-1   -I/tmp/qemu-test/src/tests -I/tmp/qemu-test/src/tests/qtest -g " LDFLAGS="-Wl,--warn-common -fsanitize=undefined -fsanitize=address -Wl,-z,relro -Wl,-z,now -pie -m64  -fstack-protector-strong"
---
clang -iquote /tmp/qemu-test/build/tests -iquote tests -iquote /tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem /tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote /tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote /tmp/qemu-test/src/disas/libvixl -I/usr/include/pixman-1   -Werror -fsanitize=undefined -fsanitize=address  -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include  -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99  -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -fstack-protector-strong   -I/usr/include/p11-kit-1   -DSTRUCT_IOVEC_DEFINED  -I/usr/include/libpng16  -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/pixman-1   -I/tmp/qemu-test/src/tests -I/tmp/qemu-test/src/tests/qtest -MMD -MP -MT tests/test-crypto-cipher.o -MF tests/test-crypto-cipher.d -g   -c -o tests/test-crypto-cipher.o /tmp/qemu-test/src/tests/test-crypto-cipher.c
clang -iquote /tmp/qemu-test/build/tests -iquote tests -iquote /tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem /tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote /tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote /tmp/qemu-test/src/disas/libvixl -I/usr/include/pixman-1   -Werror -fsanitize=undefined -fsanitize=address  -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include  -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99  -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -fstack-protector-strong   -I/usr/include/p11-kit-1   -DSTRUCT_IOVEC_DEFINED  -I/usr/include/libpng16  -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/pixman-1   -I/tmp/qemu-test/src/tests -I/tmp/qemu-test/src/tests/qtest -MMD -MP -MT tests/test-crypto-secret.o -MF tests/test-crypto-secret.d -g   -c -o tests/test-crypto-secret.o /tmp/qemu-test/src/tests/test-crypto-secret.c
clang -iquote /tmp/qemu-test/build/tests -iquote tests -iquote /tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem /tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote /tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote /tmp/qemu-test/src/disas/libvixl -I/usr/include/pixman-1   -Werror -fsanitize=undefined -fsanitize=address  -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include  -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99  -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -fstack-protector-strong   -I/usr/include/p11-kit-1   -DSTRUCT_IOVEC_DEFINED  -I/usr/include/libpng16  -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/pixman-1   -I/tmp/qemu-test/src/tests -I/tmp/qemu-test/src/tests/qtest -MMD -MP -MT tests/test-crypto-tlscredsx509.o -MF tests/test-crypto-tlscredsx509.d -g   -c -o tests/test-crypto-tlscredsx509.o /tmp/qemu-test/src/tests/test-crypto-tlscredsx509.c
/tmp/qemu-test/src/tests/qht-bench.c:287:29: error: implicit conversion from 'unsigned long' to 'double' changes value from 18446744073709551615 to 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion]
        *threshold = rate * UINT64_MAX;
                          ~ ^~~~~~~~~~
/usr/include/stdint.h:130:23: note: expanded from macro 'UINT64_MAX'
---
18446744073709551615UL
^~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: tests/qht-bench.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 669, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=0f30ada1843f4b7a8b54df19919b5462', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-wdzkmbob/src/docker-src.2020-06-18-22.37.15.1263:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=0f30ada1843f4b7a8b54df19919b5462
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-wdzkmbob/src'
make: *** [docker-run-test-debug@fedora] Error 2

real    5m41.189s
user    0m7.985s


The full log is available at
http://patchew.org/logs/20200619020602.118306-1-david@gibson.dropbear.id.au/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v3 0/9] Generalize memory encryption models

[David Hildenbrand]

On 19.06.20 04:05, 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.

Each architecture finds its own way to vandalize the original
architecture, some in more extreme/obscure ways than others. I guess in
the long term we'll regret most of that, but what do I know :)

> 
> 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
> "host-trust-limitation" property pointing to a platform specific
> object which configures and manages the specific details.
> 

I consider the property name sub-optimal. Yes, I am aware that there are
other approaches being discussed on the KVM list to disallow access to
guest memory without memory encryption. (most of them sound like people
are trying to convert KVM into XEN, but again, what do I know ... :)  )

"host-trust-limitation"  sounds like "I am the hypervisor, I configure
limited trust into myself". Also, "untrusted-host" would be a little bit
nicer (I think trust is a black/white thing).

However, once we have multiple options to protect a guest (memory
encryption, unmapping guest pages ,...) the name will no longer really
suffice to configure QEMU, no?

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

The only approach on s390x to not glue command line properties to the
cpu model would be to remove the CPU model feature and replace it by the
command line parameter. But that would, of course, be an incompatible break.

How do upper layers actually figure out if memory encryption etc is
available? on s390x, it's simply via the expanded host CPU model.

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 0/9] Generalize memory encryption models

[Cornelia Huck]

On Fri, 19 Jun 2020 10:28:22 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 19.06.20 04:05, 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.  
> 
> Each architecture finds its own way to vandalize the original
> architecture, some in more extreme/obscure ways than others. I guess in
> the long term we'll regret most of that, but what do I know :)
> 
> > 
> > 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
> > "host-trust-limitation" property pointing to a platform specific
> > object which configures and manages the specific details.
> >   
> 
> I consider the property name sub-optimal. Yes, I am aware that there are
> other approaches being discussed on the KVM list to disallow access to
> guest memory without memory encryption. (most of them sound like people
> are trying to convert KVM into XEN, but again, what do I know ... :)  )
> 
> "host-trust-limitation"  sounds like "I am the hypervisor, I configure
> limited trust into myself". Also, "untrusted-host" would be a little bit
> nicer (I think trust is a black/white thing).
> 
> However, once we have multiple options to protect a guest (memory
> encryption, unmapping guest pages ,...) the name will no longer really
> suffice to configure QEMU, no?

Hm... we could have a property that accepts bits indicating where the
actual limitation lies. Different parts of the code could then make
more fine-grained decisions of what needs to be done. Feels a bit
overengineered today; but maybe there's already stuff with different
semantics in the pipeline somewhere?

> 
> > 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.  
> 
> The only approach on s390x to not glue command line properties to the
> cpu model would be to remove the CPU model feature and replace it by the
> command line parameter. But that would, of course, be an incompatible break.

Yuck.

We still need to provide the cpu feature to the *guest* in any case, no?

> 
> How do upper layers actually figure out if memory encryption etc is
> available? on s390x, it's simply via the expanded host CPU model.
> 

Re: [PATCH v3 0/9] Generalize memory encryption models

[David Gibson]

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

On Fri, Jun 19, 2020 at 10:28:22AM +0200, David Hildenbrand wrote:
> On 19.06.20 04:05, 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.
> 
> Each architecture finds its own way to vandalize the original
> architecture, some in more extreme/obscure ways than others. I guess in
> the long term we'll regret most of that, but what do I know :)

Well, sure, but that's no *more* true if we start from a common point.

> > 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
> > "host-trust-limitation" property pointing to a platform specific
> > object which configures and manages the specific details.
> 
> I consider the property name sub-optimal. Yes, I am aware that there are
> other approaches being discussed on the KVM list to disallow access to
> guest memory without memory encryption. (most of them sound like people
> are trying to convert KVM into XEN, but again, what do I know ... :)  )

I don't love the name, it's just the best I've thought of so far.

> "host-trust-limitation"  sounds like "I am the hypervisor, I configure
> limited trust into myself".

Which is kind of... accurate...

> Also, "untrusted-host" would be a little bit
> nicer (I think trust is a black/white thing).

> However, once we have multiple options to protect a guest (memory
> encryption, unmapping guest pages ,...) the name will no longer really
> suffice to configure QEMU, no?

That's why it takes a parameter.  It points to an object which can
itself have more properties to configure the details.  SEV already
needs that set up, though for both PEF and s390 PV we could pre-create
a standard htl object.

> > 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.
> 
> The only approach on s390x to not glue command line properties to the
> cpu model would be to remove the CPU model feature and replace it by the
> command line parameter. But that would, of course, be an incompatible break.

I don't really understand why you're so against setting the cpu
default parameters from the machine.  The machine already sets basic
configuration for all sorts of devices in the VM, that's kind of what
it's for.

> How do upper layers actually figure out if memory encryption etc is
> available? on s390x, it's simply via the expanded host CPU model.

Haven't really tackled that yet.  But one way that works for multiple
systems has got to be better than a separate one for each, right?

-- 
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: [PATCH v3 0/9] Generalize memory encryption models

[David Hildenbrand]

>> "host-trust-limitation"  sounds like "I am the hypervisor, I configure
>> limited trust into myself". Also, "untrusted-host" would be a little bit
>> nicer (I think trust is a black/white thing).
>>
>> However, once we have multiple options to protect a guest (memory
>> encryption, unmapping guest pages ,...) the name will no longer really
>> suffice to configure QEMU, no?
> 
> Hm... we could have a property that accepts bits indicating where the
> actual limitation lies. Different parts of the code could then make
> more fine-grained decisions of what needs to be done. Feels a bit
> overengineered today; but maybe there's already stuff with different
> semantics in the pipeline somewhere?
> 
>>
>>> 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.  
>>
>> The only approach on s390x to not glue command line properties to the
>> cpu model would be to remove the CPU model feature and replace it by the
>> command line parameter. But that would, of course, be an incompatible break.
> 
> Yuck.
> 
> We still need to provide the cpu feature to the *guest* in any case, no?

Yeah, but that could be wired up internally. Wouldn't consider it clean,
though (I second the "overengineered" above).
-- 
Thanks,

David / dhildenb

Re: [PATCH v3 0/9] Generalize memory encryption models

[David Hildenbrand]

>> However, once we have multiple options to protect a guest (memory
>> encryption, unmapping guest pages ,...) the name will no longer really
>> suffice to configure QEMU, no?
> 
> That's why it takes a parameter.  It points to an object which can
> itself have more properties to configure the details.  SEV already
> needs that set up, though for both PEF and s390 PV we could pre-create
> a standard htl object.

Ah, okay, that's the "platform specific object which configures and
manages the specific details". It would have been nice in the cover
letter to show some examples of how that would look like.

So it's wrapping architecture-specific data in a common parameter. Hmm.

> 
>>> 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.
>>
>> The only approach on s390x to not glue command line properties to the
>> cpu model would be to remove the CPU model feature and replace it by the
>> command line parameter. But that would, of course, be an incompatible break.
> 
> I don't really understand why you're so against setting the cpu
> default parameters from the machine.  The machine already sets basic
> configuration for all sorts of devices in the VM, that's kind of what
> it's for.

It's a general design philosophy that the CPU model (especially the host
CPU model) does not depend on other command line parameters (except the
accelerator, and I think in corner cases on the machine). Necessary for
reliable host model probing by libvirt, for example.

We also don't have similar things for nested virt.

> 
>> How do upper layers actually figure out if memory encryption etc is
>> available? on s390x, it's simply via the expanded host CPU model.
> 
> Haven't really tackled that yet.  But one way that works for multiple
> systems has got to be better than a separate one for each, right?

I think that's an important piece. Especially once multiple different
approaches are theoretically available one wants to sense from upper layers.

At least on s390x, it really is like just another CPU-visible feature
that tells the guest that it can switch to protected mode.

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 0/9] Generalize memory encryption models

[Cornelia Huck]

On Fri, 19 Jun 2020 11:56:49 +0200
David Hildenbrand <david@redhat.com> wrote:

> >>> 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.    
> >>
> >> The only approach on s390x to not glue command line properties to the
> >> cpu model would be to remove the CPU model feature and replace it by the
> >> command line parameter. But that would, of course, be an incompatible break.  
> > 
> > Yuck.
> > 
> > We still need to provide the cpu feature to the *guest* in any case, no?  
> 
> Yeah, but that could be wired up internally. Wouldn't consider it clean,
> though (I second the "overengineered" above).

Could an internally wired-up cpu feature be introspected? Also, what
happens if new cpu features are introduced that have a dependency on or
a conflict with this one?

Re: [PATCH v3 0/9] Generalize memory encryption models

[David Hildenbrand]

On 19.06.20 12:05, Cornelia Huck wrote:
> On Fri, 19 Jun 2020 11:56:49 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>>>>> 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.    
>>>>
>>>> The only approach on s390x to not glue command line properties to the
>>>> cpu model would be to remove the CPU model feature and replace it by the
>>>> command line parameter. But that would, of course, be an incompatible break.  
>>>
>>> Yuck.
>>>
>>> We still need to provide the cpu feature to the *guest* in any case, no?  
>>
>> Yeah, but that could be wired up internally. Wouldn't consider it clean,
>> though (I second the "overengineered" above).
> 
> Could an internally wired-up cpu feature be introspected? Also, what

Nope. It would just be e.g., a "machine feature" indicated to the guest
via the STFL interface/instruction. I was tackling the introspect part
when asking David how to sense from upper layers. It would have to be
sense via a different interface as it would not longer be modeled as
part of CPU features in QEMU.

> happens if new cpu features are introduced that have a dependency on or
> a conflict with this one?

Conflict: bail out in QEMU when incompatible options are specified.
Dependency: warn and continue/fixup (e.g., mask off?)

Not clean I think.

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[Daniel P. Berrangé]

On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-pci.h"
>  
>  GlobalProperty hw_compat_5_0[] = {
>      { "virtio-balloon-device", "page-poison", "false" },
> @@ -1165,6 +1167,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");
>      }

Silently changing the user's request configuration like this is a bad idea.
The "disable-legacy" option in particular is undesirable as that switches
the device to virtio-1.0 only mode, which exposes a different PCI ID to
the guest.

If some options are incompatible with encryption, then we should raise a
fatal error at startup, so applications/admins are aware that their requested
config is broken.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[Michael S. Tsirkin]

On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > +#include "hw/virtio/virtio.h"
> > +#include "hw/virtio/virtio-pci.h"
> >  
> >  GlobalProperty hw_compat_5_0[] = {
> >      { "virtio-balloon-device", "page-poison", "false" },
> > @@ -1165,6 +1167,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");
> >      }
> 
> Silently changing the user's request configuration like this is a bad idea.
> The "disable-legacy" option in particular is undesirable as that switches
> the device to virtio-1.0 only mode, which exposes a different PCI ID to
> the guest.
> 
> If some options are incompatible with encryption, then we should raise a
> fatal error at startup, so applications/admins are aware that their requested
> config is broken.
> 
> Regards,
> Daniel

Agreed - my suggestion is an on/off/auto property, auto value
changes automatically, on/off is validated.


> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[Michael S. Tsirkin]

On Fri, Jun 19, 2020 at 07:46:14AM -0400, Michael S. Tsirkin wrote:
> On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > +#include "hw/virtio/virtio.h"
> > > +#include "hw/virtio/virtio-pci.h"
> > >  
> > >  GlobalProperty hw_compat_5_0[] = {
> > >      { "virtio-balloon-device", "page-poison", "false" },
> > > @@ -1165,6 +1167,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");
> > >      }
> > 
> > Silently changing the user's request configuration like this is a bad idea.
> > The "disable-legacy" option in particular is undesirable as that switches
> > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > the guest.
> > 
> > If some options are incompatible with encryption, then we should raise a
> > fatal error at startup, so applications/admins are aware that their requested
> > config is broken.
> > 
> > Regards,
> > Daniel
> 
> Agreed - my suggestion is an on/off/auto property, auto value
> changes automatically, on/off is validated.


In fact should we extend all bit properties to allow an auto value?

> 
> > -- 
> > |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> > |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> > |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[Daniel P. Berrangé]

On Fri, Jun 19, 2020 at 07:47:20AM -0400, Michael S. Tsirkin wrote:
> On Fri, Jun 19, 2020 at 07:46:14AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > > +#include "hw/virtio/virtio.h"
> > > > +#include "hw/virtio/virtio-pci.h"
> > > >  
> > > >  GlobalProperty hw_compat_5_0[] = {
> > > >      { "virtio-balloon-device", "page-poison", "false" },
> > > > @@ -1165,6 +1167,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");
> > > >      }
> > > 
> > > Silently changing the user's request configuration like this is a bad idea.
> > > The "disable-legacy" option in particular is undesirable as that switches
> > > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > > the guest.
> > > 
> > > If some options are incompatible with encryption, then we should raise a
> > > fatal error at startup, so applications/admins are aware that their requested
> > > config is broken.
> >
> > Agreed - my suggestion is an on/off/auto property, auto value
> > changes automatically, on/off is validated.
> 
> In fact should we extend all bit properties to allow an auto value?

If "auto" was made the default that creates a similar headache, as to
preserve existing configuration semantics we expose to apps, libvirt
would need to find all the properties changed to use "auto" and manually
set them back to on/off explicitly.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[David Gibson]

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

On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > +#include "hw/virtio/virtio.h"
> > +#include "hw/virtio/virtio-pci.h"
> >  
> >  GlobalProperty hw_compat_5_0[] = {
> >      { "virtio-balloon-device", "page-poison", "false" },
> > @@ -1165,6 +1167,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");
> >      }
> 
> Silently changing the user's request configuration like this

It doesn't, though.  register_sugar_prop() effectively registers a
default, so if the user has explicitly specified something, that will
take precedence.

> is a bad idea.
> The "disable-legacy" option in particular is undesirable as that switches
> the device to virtio-1.0 only mode, which exposes a different PCI ID to
> the guest.
> 
> If some options are incompatible with encryption, then we should raise a
> fatal error at startup, so applications/admins are aware that their requested
> config is broken.
> 
> Regards,
> Daniel

-- 
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: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[Daniel P. Berrangé]

On Sat, Jun 20, 2020 at 12:45:41AM +1000, David Gibson wrote:
> On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > +#include "hw/virtio/virtio.h"
> > > +#include "hw/virtio/virtio-pci.h"
> > >  
> > >  GlobalProperty hw_compat_5_0[] = {
> > >      { "virtio-balloon-device", "page-poison", "false" },
> > > @@ -1165,6 +1167,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");
> > >      }
> > 
> > Silently changing the user's request configuration like this
> 
> It doesn't, though.  register_sugar_prop() effectively registers a
> default, so if the user has explicitly specified something, that will
> take precedence.

Don't assume that the user has set "disable-legacy=off". People who want to
have a transtional device are almost certainly pasing "-device virtio-blk-pci",
because historical behaviour is that this is sufficient to give you a
transitional device. Changing the default of disable-legacy=on has not
honoured the users' requested config.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[Halil Pasic]

On Fri, 19 Jun 2020 13:16:38 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

[..]

> > > > > @@ -1165,6 +1167,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");
> > > > >      }  
> > > > 
> > > > Silently changing the user's request configuration like this is a bad idea.
> > > > The "disable-legacy" option in particular is undesirable as that switches
> > > > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > > > the guest.
> > > > 
> > > > If some options are incompatible with encryption, then we should raise a
> > > > fatal error at startup, so applications/admins are aware that their requested
> > > > config is broken.  
> > >
> > > Agreed - my suggestion is an on/off/auto property, auto value
> > > changes automatically, on/off is validated.  
> > 
> > In fact should we extend all bit properties to allow an auto value?  
> 
> If "auto" was made the default that creates a similar headache, as to
> preserve existing configuration semantics we expose to apps, libvirt
> would need to find all the properties changed to use "auto" and manually
> set them back to on/off explicitly.

Hm, does that mean we can't change the default for any qemu property?

I would expect that the defaults most remain invariant for any
particular machine version. Conceptually, IMHO the default could change
with a new machine version, but we would need some mechanism to ensure
compatibility for old machine versions.

Regards,
Halil

Re: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[David Gibson]

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

On Fri, Jun 19, 2020 at 04:05:56PM +0100, Daniel P. Berrangé wrote:
> On Sat, Jun 20, 2020 at 12:45:41AM +1000, David Gibson wrote:
> > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > > +#include "hw/virtio/virtio.h"
> > > > +#include "hw/virtio/virtio-pci.h"
> > > >  
> > > >  GlobalProperty hw_compat_5_0[] = {
> > > >      { "virtio-balloon-device", "page-poison", "false" },
> > > > @@ -1165,6 +1167,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");
> > > >      }
> > > 
> > > Silently changing the user's request configuration like this
> > 
> > It doesn't, though.  register_sugar_prop() effectively registers a
> > default, so if the user has explicitly specified something, that will
> > take precedence.
> 
> Don't assume that the user has set "disable-legacy=off". People who want to
> have a transtional device are almost certainly pasing "-device virtio-blk-pci",
> because historical behaviour is that this is sufficient to give you a
> transitional device. Changing the default of disable-legacy=on has not
> honoured the users' requested config.

Umm.. by this argument we can never change any default, ever.  But we
do that routinely with new machine versions.  How is changing based on
a machine option different from that?

-- 
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: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[Daniel P. Berrangé]

On Sat, Jun 20, 2020 at 06:24:27PM +1000, David Gibson wrote:
> On Fri, Jun 19, 2020 at 04:05:56PM +0100, Daniel P. Berrangé wrote:
> > On Sat, Jun 20, 2020 at 12:45:41AM +1000, David Gibson wrote:
> > > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > > > +#include "hw/virtio/virtio.h"
> > > > > +#include "hw/virtio/virtio-pci.h"
> > > > >  
> > > > >  GlobalProperty hw_compat_5_0[] = {
> > > > >      { "virtio-balloon-device", "page-poison", "false" },
> > > > > @@ -1165,6 +1167,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");
> > > > >      }
> > > > 
> > > > Silently changing the user's request configuration like this
> > > 
> > > It doesn't, though.  register_sugar_prop() effectively registers a
> > > default, so if the user has explicitly specified something, that will
> > > take precedence.
> > 
> > Don't assume that the user has set "disable-legacy=off". People who want to
> > have a transtional device are almost certainly pasing "-device virtio-blk-pci",
> > because historical behaviour is that this is sufficient to give you a
> > transitional device. Changing the default of disable-legacy=on has not
> > honoured the users' requested config.
> 
> Umm.. by this argument we can never change any default, ever.  But we
> do that routinely with new machine versions.  How is changing based on
> a machine option different from that?

It isn't really different. Most of the time we get away with it and no one
sees a problem. Some of the changes made though, do indeed break things,
and libvirt tries to override QEMU's changes in defaults where they are
especially at risk of causing breakage. The virtio device model is one such
change I'd consider especially risky as there are clear guest OS driver
support compatibility issues there, with it being a completely different
PCI device ID & impl.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/9] Generalize memory encryption models

[Cornelia Huck]

On Fri, 19 Jun 2020 12:10:13 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 19.06.20 12:05, Cornelia Huck wrote:
> > On Fri, 19 Jun 2020 11:56:49 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >>>>> 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.      
> >>>>
> >>>> The only approach on s390x to not glue command line properties to the
> >>>> cpu model would be to remove the CPU model feature and replace it by the
> >>>> command line parameter. But that would, of course, be an incompatible break.    
> >>>
> >>> Yuck.
> >>>
> >>> We still need to provide the cpu feature to the *guest* in any case, no?    
> >>
> >> Yeah, but that could be wired up internally. Wouldn't consider it clean,
> >> though (I second the "overengineered" above).  
> > 
> > Could an internally wired-up cpu feature be introspected? Also, what  
> 
> Nope. It would just be e.g., a "machine feature" indicated to the guest
> via the STFL interface/instruction. I was tackling the introspect part
> when asking David how to sense from upper layers. It would have to be
> sense via a different interface as it would not longer be modeled as
> part of CPU features in QEMU.
> 
> > happens if new cpu features are introduced that have a dependency on or
> > a conflict with this one?  
> 
> Conflict: bail out in QEMU when incompatible options are specified.
> Dependency: warn and continue/fixup (e.g., mask off?)

Masking off would likely be surprising to the user.

> Not clean I think.

I agree.

Still unsure how to bring this new machine property and the cpu feature
together. Would be great to have the same interface everywhere, but
having two distinct command line objects depend on each other sucks.
Automatically setting the feature bit if pv is supported complicates
things further.

(Is there any requirement that the machine object has been already set
up before the cpu features are processed? Or the other way around?)

Does this have any implications when probing with the 'none' machine?

Re: [PATCH v3 0/9] Generalize memory encryption models

[Christian Borntraeger]

On 19.06.20 04:05, 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
> "host-trust-limitation" 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.

Let me try to summarize what I understand what you try to achieve:
one command line parameter for all platforms that 

common across all platforms:
- disable KSM
- by default enables iommu_platform


per platform:
- setup the necessary encryption scheme when appropriate
- block migration
-....


The tricky part is certainly the per platform thing. For example on
s390 we just have a cpumodel flag that provides interfaces to the guest
to switch into protected mode via the ultravisor. This works perfectly
fine with the host model, so no need to configure anything.  The platform
code then disables KSM _on_switchover_ and not in general. Because the 
guest CAN switch into protected, but it does not have to.

So this feels really hard to do right. Would a virtual BoF on KVM forum
be too late? We had a BoF on protected guests last year and that was
valuable.

Re: [PATCH v3 0/9] Generalize memory encryption models

[Cornelia Huck]

On Mon, 22 Jun 2020 16:27:28 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 19.06.20 04:05, 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
> > "host-trust-limitation" 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.  
> 
> Let me try to summarize what I understand what you try to achieve:
> one command line parameter for all platforms that 
> 
> common across all platforms:
> - disable KSM
> - by default enables iommu_platform
> 
> 
> per platform:
> - setup the necessary encryption scheme when appropriate
> - block migration
> -....
> 
> 
> The tricky part is certainly the per platform thing. For example on
> s390 we just have a cpumodel flag that provides interfaces to the guest
> to switch into protected mode via the ultravisor. This works perfectly
> fine with the host model, so no need to configure anything.  The platform
> code then disables KSM _on_switchover_ and not in general. Because the 
> guest CAN switch into protected, but it does not have to.
> 
> So this feels really hard to do right. Would a virtual BoF on KVM forum
> be too late? We had a BoF on protected guests last year and that was
> valuable.

Maybe we can do some kind of call to discuss this earlier? (Maybe in
the KVM call slot on Tuesdays?) I think it would be really helpful if
everybody would have at least a general understanding about how
encryption/protection works on the different architectures.

Re: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[Michael S. Tsirkin]

On Fri, Jun 19, 2020 at 01:16:38PM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 19, 2020 at 07:47:20AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Jun 19, 2020 at 07:46:14AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > > > +#include "hw/virtio/virtio.h"
> > > > > +#include "hw/virtio/virtio-pci.h"
> > > > >  
> > > > >  GlobalProperty hw_compat_5_0[] = {
> > > > >      { "virtio-balloon-device", "page-poison", "false" },
> > > > > @@ -1165,6 +1167,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");
> > > > >      }
> > > > 
> > > > Silently changing the user's request configuration like this is a bad idea.
> > > > The "disable-legacy" option in particular is undesirable as that switches
> > > > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > > > the guest.
> > > > 
> > > > If some options are incompatible with encryption, then we should raise a
> > > > fatal error at startup, so applications/admins are aware that their requested
> > > > config is broken.
> > >
> > > Agreed - my suggestion is an on/off/auto property, auto value
> > > changes automatically, on/off is validated.
> > 
> > In fact should we extend all bit properties to allow an auto value?
> 
> If "auto" was made the default that creates a similar headache, as to
> preserve existing configuration semantics we expose to apps, libvirt
> would need to find all the properties changed to use "auto" and manually
> set them back to on/off explicitly.
> 
> Regards,
> Daniel

It's QEMU's job to try and have more or less consistent semantics across
versions. QEMU does not guarantee not to change any option defaults
though.

My point is to add ability to differentiate between property values
set by user and ones set by machine type for compatibility.


-- 
MST

Re: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[David Gibson]

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

On Wed, Jun 24, 2020 at 03:55:59AM -0400, Michael S. Tsirkin wrote:
> On Fri, Jun 19, 2020 at 01:16:38PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jun 19, 2020 at 07:47:20AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Jun 19, 2020 at 07:46:14AM -0400, Michael S. Tsirkin wrote:
> > > > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > > > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > > > > +#include "hw/virtio/virtio.h"
> > > > > > +#include "hw/virtio/virtio-pci.h"
> > > > > >  
> > > > > >  GlobalProperty hw_compat_5_0[] = {
> > > > > >      { "virtio-balloon-device", "page-poison", "false" },
> > > > > > @@ -1165,6 +1167,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");
> > > > > >      }
> > > > > 
> > > > > Silently changing the user's request configuration like this is a bad idea.
> > > > > The "disable-legacy" option in particular is undesirable as that switches
> > > > > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > > > > the guest.
> > > > > 
> > > > > If some options are incompatible with encryption, then we should raise a
> > > > > fatal error at startup, so applications/admins are aware that their requested
> > > > > config is broken.
> > > >
> > > > Agreed - my suggestion is an on/off/auto property, auto value
> > > > changes automatically, on/off is validated.
> > > 
> > > In fact should we extend all bit properties to allow an auto value?
> > 
> > If "auto" was made the default that creates a similar headache, as to
> > preserve existing configuration semantics we expose to apps, libvirt
> > would need to find all the properties changed to use "auto" and manually
> > set them back to on/off explicitly.
> > 
> > Regards,
> > Daniel
> 
> It's QEMU's job to try and have more or less consistent semantics across
> versions. QEMU does not guarantee not to change any option defaults
> though.
> 
> My point is to add ability to differentiate between property values
> set by user and ones set by machine type for compatibility.

At which point are you looking to differentiate these?  The use of
sugar_prop() in my draft code accomplishes this already for the
purposes of resolving a final property value within qemu (an explicit
user set one takes precedence).

-- 
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: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[David Gibson]

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

On Fri, Jun 19, 2020 at 07:46:10AM -0400, Michael S. Tsirkin wrote:
> On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > +#include "hw/virtio/virtio.h"
> > > +#include "hw/virtio/virtio-pci.h"
> > >  
> > >  GlobalProperty hw_compat_5_0[] = {
> > >      { "virtio-balloon-device", "page-poison", "false" },
> > > @@ -1165,6 +1167,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");
> > >      }
> > 
> > Silently changing the user's request configuration like this is a bad idea.
> > The "disable-legacy" option in particular is undesirable as that switches
> > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > the guest.
> > 
> > If some options are incompatible with encryption, then we should raise a
> > fatal error at startup, so applications/admins are aware that their requested
> > config is broken.
> > 
> > Regards,
> > Daniel
> 
> Agreed - my suggestion is an on/off/auto property, auto value
> changes automatically, on/off is validated.

So, I think you're specifically suggesting this for the
"iommu_platform" property, by delaying determining which mode to use
until the guest activates the device.  Is that correct?

That might work on s390, but I don't think it will work on POWER on at
least 2 counts:

1) qemu doesn't actually have a natural way of determining if a guest
   is in secure mode (that's handled directly between the guest and
   the ultravisor).  So even at driver init time, we won't know the
   right value.

2) for virtio-pci, iommu_platform=on requires a "modern" device, not a
   legacy or transitional one.  That changes the PCI ID, which means
   we can't delay deciding it until driver init

-- 
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: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[David Gibson]

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

On Mon, Jun 22, 2020 at 10:09:30AM +0100, Daniel P. Berrangé wrote:
> On Sat, Jun 20, 2020 at 06:24:27PM +1000, David Gibson wrote:
> > On Fri, Jun 19, 2020 at 04:05:56PM +0100, Daniel P. Berrangé wrote:
> > > On Sat, Jun 20, 2020 at 12:45:41AM +1000, David Gibson wrote:
> > > > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > > > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > > > > +#include "hw/virtio/virtio.h"
> > > > > > +#include "hw/virtio/virtio-pci.h"
> > > > > >  
> > > > > >  GlobalProperty hw_compat_5_0[] = {
> > > > > >      { "virtio-balloon-device", "page-poison", "false" },
> > > > > > @@ -1165,6 +1167,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");
> > > > > >      }
> > > > > 
> > > > > Silently changing the user's request configuration like this
> > > > 
> > > > It doesn't, though.  register_sugar_prop() effectively registers a
> > > > default, so if the user has explicitly specified something, that will
> > > > take precedence.
> > > 
> > > Don't assume that the user has set "disable-legacy=off". People who want to
> > > have a transtional device are almost certainly pasing "-device virtio-blk-pci",
> > > because historical behaviour is that this is sufficient to give you a
> > > transitional device. Changing the default of disable-legacy=on has not
> > > honoured the users' requested config.
> > 
> > Umm.. by this argument we can never change any default, ever.  But we
> > do that routinely with new machine versions.  How is changing based on
> > a machine option different from that?
> 
> It isn't really different. Most of the time we get away with it and no one
> sees a problem. Some of the changes made though, do indeed break things,
> and libvirt tries to override QEMU's changes in defaults where they are
> especially at risk of causing breakage. The virtio device model is one such
> change I'd consider especially risky as there are clear guest OS driver
> support compatibility issues there, with it being a completely different
> PCI device ID & impl.

If it were possible to drop an existing supported guest into secure VM
mode, that would make sense.  But AFAICT, a guest always need to be
aware of the secure mode - it certainly does on POWER.  Plus, support
for secure guest mode is way newer than support for modern virtio
devices.

Even then, I don't see that this is really anything new.  Updating
machine type version can absolutely change system devices in a way
which could break guests (though it mostly won't).  If you really want
stable support for a given guest, use a versioned machine type.  Doing
that will work just as well for the secure VM stuff as for anything
else.

-- 
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: [PATCH v3 0/9] Generalize memory encryption models

[David Gibson]

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

On Mon, Jun 22, 2020 at 02:02:54PM +0200, Cornelia Huck wrote:
> On Fri, 19 Jun 2020 12:10:13 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
> > On 19.06.20 12:05, Cornelia Huck wrote:
> > > On Fri, 19 Jun 2020 11:56:49 +0200
> > > David Hildenbrand <david@redhat.com> wrote:
> > >   
> > >>>>> 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.      
> > >>>>
> > >>>> The only approach on s390x to not glue command line properties to the
> > >>>> cpu model would be to remove the CPU model feature and replace it by the
> > >>>> command line parameter. But that would, of course, be an incompatible break.    
> > >>>
> > >>> Yuck.
> > >>>
> > >>> We still need to provide the cpu feature to the *guest* in any case, no?    
> > >>
> > >> Yeah, but that could be wired up internally. Wouldn't consider it clean,
> > >> though (I second the "overengineered" above).  
> > > 
> > > Could an internally wired-up cpu feature be introspected? Also, what  
> > 
> > Nope. It would just be e.g., a "machine feature" indicated to the guest
> > via the STFL interface/instruction. I was tackling the introspect part
> > when asking David how to sense from upper layers. It would have to be
> > sense via a different interface as it would not longer be modeled as
> > part of CPU features in QEMU.
> > 
> > > happens if new cpu features are introduced that have a dependency on or
> > > a conflict with this one?  
> > 
> > Conflict: bail out in QEMU when incompatible options are specified.
> > Dependency: warn and continue/fixup (e.g., mask off?)
> 
> Masking off would likely be surprising to the user.
> 
> > Not clean I think.
> 
> I agree.
> 
> Still unsure how to bring this new machine property and the cpu feature
> together. Would be great to have the same interface everywhere, but
> having two distinct command line objects depend on each other sucks.

Kinda, but the reality is that hardware - virtual and otherwise -
frequently doesn't have entirely orthogonal configuration for each of
its components.  This is by no means new in that regard.

> Automatically setting the feature bit if pv is supported complicates
> things further.

AIUI, on s390 the "unpack" feature is available by default on recent
models.  In that case you could do this:

 * Don't modify either cpu or HTL options based on each other
 * Bail out if the user specifies a non "unpack" secure CPU along with
   the HTL option

Cases of note:
 - User specifies an old CPU model + htl
   or explicitly sets unpack=off + htl
	=> fails with an error, correctly
 - User specifies modern/default cpu + htl, with secure aware guest
 	=> works as a secure guest
 - User specifies modern/default cpu + htl, with non secure aware guest
	=> works, though not secure (and maybe slower than neccessary)
 - User specifies modern/default cpu, no htl, with non-secure guest
 	=> works, "unpack" feature is present but unused
 - User specifies modern/default cpu, no htl, secure guest
  	=> this is the worst one.  It kind of works by accident if
	   you've also  manually specified whatever virtio (and
	   anything else) options are necessary. Ugly, but no
	   different from the situation right now, IIUC

> (Is there any requirement that the machine object has been already set
> up before the cpu features are processed? Or the other way around?)

CPUs are usually created by the machine, so I believe we can count on
the machine object being there first.

> Does this have any implications when probing with the 'none' machine?

I'm not sure.  In your case, I guess the cpu bit would still show up
as before, so it would tell you base feature availability, but not
whether you can use the new configuration option.

Since the HTL option is generic, you could still set it on the "none"
machine, though it wouldn't really have any effect.  That is, if you
could create a suitable object to point it at, which would depend on
... details.

-- 
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: [PATCH v3 0/9] Generalize memory encryption models

[David Gibson]

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

On Fri, Jun 19, 2020 at 12:04:25PM +0200, David Hildenbrand wrote:
> >> However, once we have multiple options to protect a guest (memory
> >> encryption, unmapping guest pages ,...) the name will no longer really
> >> suffice to configure QEMU, no?
> > 
> > That's why it takes a parameter.  It points to an object which can
> > itself have more properties to configure the details.  SEV already
> > needs that set up, though for both PEF and s390 PV we could pre-create
> > a standard htl object.
> 
> Ah, okay, that's the "platform specific object which configures and
> manages the specific details". It would have been nice in the cover
> letter to show some examples of how that would look like.

Ok, I can try to add some.

> So it's wrapping architecture-specific data in a common
> parameter. Hmm.

Well, I don't know I'd say "wrapping".  You have a common parameter
that points to an object with a well defined interface.  The available
implementations of that object will tend to be either zero or one per
architecture, but there's no theoretical reason it has to be.  Indeed
we expect at least 2 for x86 (SEV and the Intel one who's name I never
remember).  Extra ones are entirely plausible for POWER and maybe s390
too, when an updated version of PEF or PV inevitably rolls around.

Some sort of new HTL scheme which could work across multiple archs is
much less likely, but it's not totally impossible either.

> >>> 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.
> >>
> >> The only approach on s390x to not glue command line properties to the
> >> cpu model would be to remove the CPU model feature and replace it by the
> >> command line parameter. But that would, of course, be an incompatible break.
> > 
> > I don't really understand why you're so against setting the cpu
> > default parameters from the machine.  The machine already sets basic
> > configuration for all sorts of devices in the VM, that's kind of what
> > it's for.
> 
> It's a general design philosophy that the CPU model (especially the host
> CPU model) does not depend on other command line parameters (except the
> accelerator, and I think in corner cases on the machine). Necessary for
> reliable host model probing by libvirt, for example.

Ok, I've proposed a revision which doesn't require altering the CPU
model elsewhere in this thread.

> We also don't have similar things for nested virt.

I'm not sure what you're getting at there.

> >> How do upper layers actually figure out if memory encryption etc is
> >> available? on s390x, it's simply via the expanded host CPU model.
> > 
> > Haven't really tackled that yet.  But one way that works for multiple
> > systems has got to be better than a separate one for each, right?
> 
> I think that's an important piece. Especially once multiple different
> approaches are theoretically available one wants to sense from upper layers.

Fair point.

So... IIRC there's a general way of looking at available properties
for any object, including the machine.  So we can probe for
availability of the "host-trust-limitation" property itself easily
enough.

I guess we do need a way of probing for what implementations of the
htl interface are available.  And, if we go down that path, if there
are any pre-generated htl objects available.

> At least on s390x, it really is like just another CPU-visible feature
> that tells the guest that it can switch to protected mode.

Right.. which is great for you, since you already have a nice
orthogonal interface for that.   On POWER, (a) CPU model isn't enough
since you need a running ultravisor as well and (b) CPU feature
detection is already a real mess for.. reasons.

-- 
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: [PATCH v3 0/9] Generalize memory encryption models

[David Gibson]

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

On Mon, Jun 22, 2020 at 04:27:28PM +0200, Christian Borntraeger wrote:
> On 19.06.20 04:05, 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
> > "host-trust-limitation" 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.
> 
> Let me try to summarize what I understand what you try to achieve:
> one command line parameter for all platforms that 
> 
> common across all platforms:
> - disable KSM
> - by default enables iommu_platform

Pretty much, yes.  Plus, in future if we discover other things that
don't make sense in the context of a guest whose memory we can't
freely access, it can check for those and set sane defaults
accordingly.

> per platform:
> - setup the necessary encryption scheme when appropriate
> - block migration

That's true for now, but I believe there are plans to make secure
guests migratable, so that's not an inherent property.

> -....
> 
> 
> The tricky part is certainly the per platform thing. For example on
> s390 we just have a cpumodel flag that provides interfaces to the guest
> to switch into protected mode via the ultravisor. This works perfectly
> fine with the host model, so no need to configure anything.  The platform
> code then disables KSM _on_switchover_ and not in general.

Right, because your platform code is aware of the switchover.  On
POWER, we aren't.

> Because the 
> guest CAN switch into protected, but it does not have to.
> 
> So this feels really hard to do right. Would a virtual BoF on KVM forum
> be too late? We had a BoF on protected guests last year and that was
> valuable.

-- 
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: [PATCH v3 0/9] Generalize memory encryption models

[David Gibson]

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

On Wed, Jun 24, 2020 at 09:06:48AM +0200, Cornelia Huck wrote:
> On Mon, 22 Jun 2020 16:27:28 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > On 19.06.20 04:05, 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
> > > "host-trust-limitation" 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.  
> > 
> > Let me try to summarize what I understand what you try to achieve:
> > one command line parameter for all platforms that 
> > 
> > common across all platforms:
> > - disable KSM
> > - by default enables iommu_platform
> > 
> > 
> > per platform:
> > - setup the necessary encryption scheme when appropriate
> > - block migration
> > -....
> > 
> > 
> > The tricky part is certainly the per platform thing. For example on
> > s390 we just have a cpumodel flag that provides interfaces to the guest
> > to switch into protected mode via the ultravisor. This works perfectly
> > fine with the host model, so no need to configure anything.  The platform
> > code then disables KSM _on_switchover_ and not in general. Because the 
> > guest CAN switch into protected, but it does not have to.
> > 
> > So this feels really hard to do right. Would a virtual BoF on KVM forum
> > be too late? We had a BoF on protected guests last year and that was
> > valuable.
> 
> Maybe we can do some kind of call to discuss this earlier? (Maybe in
> the KVM call slot on Tuesdays?) I think it would be really helpful if
> everybody would have at least a general understanding about how
> encryption/protection works on the different architectures.

Yes, I think this would be a good idea.  KVM Forum is probably later
than we want, plus since it is virtual, I probably won't be shifting
into the right timezone to attend much of it.

I don't know when that Tuesday KVM call is.  Generally the best
available time for Australia/Europe meetings this time of year is 9am
CET / 5pm AEST.  As a once off I could go somewhat later into my
evening, 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: [PATCH v3 0/9] Generalize memory encryption models

[David Gibson]

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

On Thu, Jun 25, 2020 at 03:47:23PM +1000, David Gibson wrote:
> On Wed, Jun 24, 2020 at 09:06:48AM +0200, Cornelia Huck wrote:
> > On Mon, 22 Jun 2020 16:27:28 +0200
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> > 
> > > On 19.06.20 04:05, 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
> > > > "host-trust-limitation" 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.  
> > > 
> > > Let me try to summarize what I understand what you try to achieve:
> > > one command line parameter for all platforms that 
> > > 
> > > common across all platforms:
> > > - disable KSM
> > > - by default enables iommu_platform
> > > 
> > > 
> > > per platform:
> > > - setup the necessary encryption scheme when appropriate
> > > - block migration
> > > -....
> > > 
> > > 
> > > The tricky part is certainly the per platform thing. For example on
> > > s390 we just have a cpumodel flag that provides interfaces to the guest
> > > to switch into protected mode via the ultravisor. This works perfectly
> > > fine with the host model, so no need to configure anything.  The platform
> > > code then disables KSM _on_switchover_ and not in general. Because the 
> > > guest CAN switch into protected, but it does not have to.
> > > 
> > > So this feels really hard to do right. Would a virtual BoF on KVM forum
> > > be too late? We had a BoF on protected guests last year and that was
> > > valuable.
> > 
> > Maybe we can do some kind of call to discuss this earlier? (Maybe in
> > the KVM call slot on Tuesdays?) I think it would be really helpful if
> > everybody would have at least a general understanding about how
> > encryption/protection works on the different architectures.
> 
> Yes, I think this would be a good idea.  KVM Forum is probably later
> than we want, plus since it is virtual, I probably won't be shifting
> into the right timezone to attend much of it.
> 
> I don't know when that Tuesday KVM call is.  Generally the best
> available time for Australia/Europe meetings this time of year is 9am
> CET / 5pm AEST.  As a once off I could go somewhat later into my
> evening, though.

Oh.. plus I'm on holiday next week and the one after (27 Jun - 12 Jul).

-- 
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: [PATCH v3 0/9] Generalize memory encryption models

[David Hildenbrand]

> 
>> So it's wrapping architecture-specific data in a common
>> parameter. Hmm.
> 
> Well, I don't know I'd say "wrapping".  You have a common parameter
> that points to an object with a well defined interface.  The available
> implementations of that object will tend to be either zero or one per
> architecture, but there's no theoretical reason it has to be.  Indeed
> we expect at least 2 for x86 (SEV and the Intel one who's name I never
> remember).  Extra ones are entirely plausible for POWER and maybe s390
> too, when an updated version of PEF or PV inevitably rolls around.
> 
> Some sort of new HTL scheme which could work across multiple archs is
> much less likely, but it's not totally impossible either.
> 
>>>>> 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.
>>>>
>>>> The only approach on s390x to not glue command line properties to the
>>>> cpu model would be to remove the CPU model feature and replace it by the
>>>> command line parameter. But that would, of course, be an incompatible break.
>>>
>>> I don't really understand why you're so against setting the cpu
>>> default parameters from the machine.  The machine already sets basic
>>> configuration for all sorts of devices in the VM, that's kind of what
>>> it's for.
>>
>> It's a general design philosophy that the CPU model (especially the host
>> CPU model) does not depend on other command line parameters (except the
>> accelerator, and I think in corner cases on the machine). Necessary for
>> reliable host model probing by libvirt, for example.
> 
> Ok, I've proposed a revision which doesn't require altering the CPU
> model elsewhere in this thread.
> 
>> We also don't have similar things for nested virt.
> 
> I'm not sure what you're getting at there.

Sorry, back when we introduced nested virt there was a similar
(internal?) discussion, to enable/disable it via a machine flag and not
via 1..X CPU features. We went for the latter, because it matches the
actual architecture and allows for easy migration checks etc. Nested
virt also collides with some features currently (e.g., huge page backing
for the guest), but not as severe as encrypted virtualization.

> 
>>>> How do upper layers actually figure out if memory encryption etc is
>>>> available? on s390x, it's simply via the expanded host CPU model.
>>>
>>> Haven't really tackled that yet.  But one way that works for multiple
>>> systems has got to be better than a separate one for each, right?
>>
>> I think that's an important piece. Especially once multiple different
>> approaches are theoretically available one wants to sense from upper layers.
> 
> Fair point.
> 
> So... IIRC there's a general way of looking at available properties
> for any object, including the machine.  So we can probe for
> availability of the "host-trust-limitation" property itself easily
> enough.

You can have a look at how it's currently probed by libvirt in

https://www.redhat.com/archives/libvir-list/2020-June/msg00518.html

For now, the s390x check consists of
- checking if /sys/firmware/uv is available
- checking if the kernel cmdline contains 'prot_virt=1'

The sev check is
- checking if /sys/module/kvm_amd/parameters/sev contains the
   value '1'
- checking if /dev/sev

So at least libvirt does not sense via the CPU model on s390x yet.

> 
> I guess we do need a way of probing for what implementations of the
> htl interface are available.  And, if we go down that path, if there
> are any pre-generated htl objects available.
> 
>> At least on s390x, it really is like just another CPU-visible feature
>> that tells the guest that it can switch to protected mode.
> 
> Right.. which is great for you, since you already have a nice
> orthogonal interface for that.   On POWER, (a) CPU model isn't enough
> since you need a running ultravisor as well and (b) CPU feature
> detection is already a real mess for.. reasons.

I can understand the pain of the latter ... :)


-- 
Thanks,

David / dhildenb

Re: [PATCH v3 0/9] Generalize memory encryption models

[David Hildenbrand]

>> Still unsure how to bring this new machine property and the cpu feature
>> together. Would be great to have the same interface everywhere, but
>> having two distinct command line objects depend on each other sucks.
> 
> Kinda, but the reality is that hardware - virtual and otherwise -
> frequently doesn't have entirely orthogonal configuration for each of
> its components.  This is by no means new in that regard.
> 
>> Automatically setting the feature bit if pv is supported complicates
>> things further.
> 
> AIUI, on s390 the "unpack" feature is available by default on recent
> models.  In that case you could do this:
> 
>  * Don't modify either cpu or HTL options based on each other
>  * Bail out if the user specifies a non "unpack" secure CPU along with
>    the HTL option
> 
> Cases of note:
>  - User specifies an old CPU model + htl
>    or explicitly sets unpack=off + htl
> 	=> fails with an error, correctly
>  - User specifies modern/default cpu + htl, with secure aware guest
>  	=> works as a secure guest
>  - User specifies modern/default cpu + htl, with non secure aware guest
> 	=> works, though not secure (and maybe slower than neccessary)
>  - User specifies modern/default cpu, no htl, with non-secure guest
>  	=> works, "unpack" feature is present but unused
>  - User specifies modern/default cpu, no htl, secure guest
>   	=> this is the worst one.  It kind of works by accident if
> 	   you've also  manually specified whatever virtio (and
> 	   anything else) options are necessary. Ugly, but no
> 	   different from the situation right now, IIUC
> 
>> (Is there any requirement that the machine object has been already set
>> up before the cpu features are processed? Or the other way around?)
> 
> CPUs are usually created by the machine, so I believe we can count on
> the machine object being there first.

CPU model initialization is one of the first things machine
initialization code does on s390x.

static void ccw_init(MachineState *machine)
{
    [... memory init ...]
    s390_sclp_init();
    s390_memory_init(machine->ram);
    /* init CPUs (incl. CPU model) early so s390_has_feature() works */
    s390_init_cpus(machine);
    [...]
}

> 
>> Does this have any implications when probing with the 'none' machine?
> 
> I'm not sure.  In your case, I guess the cpu bit would still show up
> as before, so it would tell you base feature availability, but not
> whether you can use the new configuration option.
> 
> Since the HTL option is generic, you could still set it on the "none"
> machine, though it wouldn't really have any effect.  That is, if you
> could create a suitable object to point it at, which would depend on
> ... details.
> 

The important point is that we never want the (expanded) host cpu model
look different when either specifying or not specifying the HTL
property. We don't want to run into issues where libvirt probes and gets
host model X, but when using that probed model (automatically) for a
guest domain, we suddenly cannot run X anymore.

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 0/9] Generalize memory encryption models

[Cornelia Huck]

On Thu, 25 Jun 2020 08:59:00 +0200
David Hildenbrand <david@redhat.com> wrote:

> >>>> How do upper layers actually figure out if memory encryption etc is
> >>>> available? on s390x, it's simply via the expanded host CPU model.  
> >>>
> >>> Haven't really tackled that yet.  But one way that works for multiple
> >>> systems has got to be better than a separate one for each, right?  
> >>
> >> I think that's an important piece. Especially once multiple different
> >> approaches are theoretically available one wants to sense from upper layers.  
> > 
> > Fair point.
> > 
> > So... IIRC there's a general way of looking at available properties
> > for any object, including the machine.  So we can probe for
> > availability of the "host-trust-limitation" property itself easily
> > enough.  
> 
> You can have a look at how it's currently probed by libvirt in
> 
> https://www.redhat.com/archives/libvir-list/2020-June/msg00518.html
> 
> For now, the s390x check consists of
> - checking if /sys/firmware/uv is available
> - checking if the kernel cmdline contains 'prot_virt=1'
> 
> The sev check is
> - checking if /sys/module/kvm_amd/parameters/sev contains the
>    value '1'
> - checking if /dev/sev
> 
> So at least libvirt does not sense via the CPU model on s390x yet.

It checks for 158 (which is apparently 'host supports secure
execution'). IIUC, only 161 ('unpack facility') is relevant for the
guest... does that also show up on the host? (I guess it does, as it
describes an ultravisor feature, IIUC.) If it is always implied,
libvirt probably does not need an extra check.

Re: [PATCH v3 0/9] Generalize memory encryption models

[David Gibson]

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

On Thu, Jun 25, 2020 at 09:06:05AM +0200, David Hildenbrand wrote:
> >> Still unsure how to bring this new machine property and the cpu feature
> >> together. Would be great to have the same interface everywhere, but
> >> having two distinct command line objects depend on each other sucks.
> > 
> > Kinda, but the reality is that hardware - virtual and otherwise -
> > frequently doesn't have entirely orthogonal configuration for each of
> > its components.  This is by no means new in that regard.
> > 
> >> Automatically setting the feature bit if pv is supported complicates
> >> things further.
> > 
> > AIUI, on s390 the "unpack" feature is available by default on recent
> > models.  In that case you could do this:
> > 
> >  * Don't modify either cpu or HTL options based on each other
> >  * Bail out if the user specifies a non "unpack" secure CPU along with
> >    the HTL option
> > 
> > Cases of note:
> >  - User specifies an old CPU model + htl
> >    or explicitly sets unpack=off + htl
> > 	=> fails with an error, correctly
> >  - User specifies modern/default cpu + htl, with secure aware guest
> >  	=> works as a secure guest
> >  - User specifies modern/default cpu + htl, with non secure aware guest
> > 	=> works, though not secure (and maybe slower than neccessary)
> >  - User specifies modern/default cpu, no htl, with non-secure guest
> >  	=> works, "unpack" feature is present but unused
> >  - User specifies modern/default cpu, no htl, secure guest
> >   	=> this is the worst one.  It kind of works by accident if
> > 	   you've also  manually specified whatever virtio (and
> > 	   anything else) options are necessary. Ugly, but no
> > 	   different from the situation right now, IIUC
> > 
> >> (Is there any requirement that the machine object has been already set
> >> up before the cpu features are processed? Or the other way around?)
> > 
> > CPUs are usually created by the machine, so I believe we can count on
> > the machine object being there first.
> 
> CPU model initialization is one of the first things machine
> initialization code does on s390x.

As it is for most platforms, but still, the values of machine
properties are available to you at this point.

> static void ccw_init(MachineState *machine)
> {
>     [... memory init ...]
>     s390_sclp_init();
>     s390_memory_init(machine->ram);
>     /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>     s390_init_cpus(machine);
>     [...]
> }
> 
> > 
> >> Does this have any implications when probing with the 'none' machine?
> > 
> > I'm not sure.  In your case, I guess the cpu bit would still show up
> > as before, so it would tell you base feature availability, but not
> > whether you can use the new configuration option.
> > 
> > Since the HTL option is generic, you could still set it on the "none"
> > machine, though it wouldn't really have any effect.  That is, if you
> > could create a suitable object to point it at, which would depend on
> > ... details.
> > 
> 
> The important point is that we never want the (expanded) host cpu model
> look different when either specifying or not specifying the HTL
> property.

Ah, yes, I see your point.  So my current suggestion will satisfy
that, basically it is:

cpu has unpack (inc. by default) && htl specified
	=> works (allowing secure), as expected

!cpu has unpack && htl specified
	=> bails out with an error

!cpu has unpack && !htl specified
	=> works for a non-secure guest, as expected
	=> guest will fail if it attempts to go secure

cpu has unpack && !htl specified
	=> works as expected for a non-secure guest (unpack feature is
	   present, but unused)
	=> secure guest may work "by accident", but only if all virtio
	   properties have the right values, which is the user's
	   problem

That last case is kinda ugly, but I think it's tolerable.

> We don't want to run into issues where libvirt probes and gets
> host model X, but when using that probed model (automatically) for a
> guest domain, we suddenly cannot run X anymore.
> 

-- 
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: [PATCH v3 0/9] Generalize memory encryption models

[David Hildenbrand]

>>>> Does this have any implications when probing with the 'none' machine?
>>>
>>> I'm not sure.  In your case, I guess the cpu bit would still show up
>>> as before, so it would tell you base feature availability, but not
>>> whether you can use the new configuration option.
>>>
>>> Since the HTL option is generic, you could still set it on the "none"
>>> machine, though it wouldn't really have any effect.  That is, if you
>>> could create a suitable object to point it at, which would depend on
>>> ... details.
>>>
>>
>> The important point is that we never want the (expanded) host cpu model
>> look different when either specifying or not specifying the HTL
>> property.
> 
> Ah, yes, I see your point.  So my current suggestion will satisfy
> that, basically it is:
> 
> cpu has unpack (inc. by default) && htl specified
> 	=> works (allowing secure), as expected

ack

> 
> !cpu has unpack && htl specified
> 	=> bails out with an error

ack

> 
> !cpu has unpack && !htl specified
> 	=> works for a non-secure guest, as expected
> 	=> guest will fail if it attempts to go secure

ack, behavior just like running on older hw without unpack

> 
> cpu has unpack && !htl specified
> 	=> works as expected for a non-secure guest (unpack feature is
> 	   present, but unused)
> 	=> secure guest may work "by accident", but only if all virtio
> 	   properties have the right values, which is the user's
> 	   problem
> 
> That last case is kinda ugly, but I think it's tolerable.

Right, we must not affect non-secure guests, and existing secure setups
(e.g., older qemu machines). Will have to think about this some more,
but does not sound too crazy.

Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 0/9] Generalize memory encryption models

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2695 bytes --]

On 6/26/20 8:53 AM, David Hildenbrand wrote:
>>>>> Does this have any implications when probing with the 'none' machine?
>>>>
>>>> I'm not sure.  In your case, I guess the cpu bit would still show up
>>>> as before, so it would tell you base feature availability, but not
>>>> whether you can use the new configuration option.
>>>>
>>>> Since the HTL option is generic, you could still set it on the "none"
>>>> machine, though it wouldn't really have any effect.  That is, if you
>>>> could create a suitable object to point it at, which would depend on
>>>> ... details.
>>>>
>>>
>>> The important point is that we never want the (expanded) host cpu model
>>> look different when either specifying or not specifying the HTL
>>> property.
>>
>> Ah, yes, I see your point.  So my current suggestion will satisfy
>> that, basically it is:
>>
>> cpu has unpack (inc. by default) && htl specified
>> 	=> works (allowing secure), as expected
> 
> ack
> 
>>
>> !cpu has unpack && htl specified
>> 	=> bails out with an error
> 
> ack
> 
>>
>> !cpu has unpack && !htl specified
>> 	=> works for a non-secure guest, as expected
>> 	=> guest will fail if it attempts to go secure
> 
> ack, behavior just like running on older hw without unpack
> 
>>
>> cpu has unpack && !htl specified
>> 	=> works as expected for a non-secure guest (unpack feature is
>> 	   present, but unused)
>> 	=> secure guest may work "by accident", but only if all virtio
>> 	   properties have the right values, which is the user's
>> 	   problem
>>
>> That last case is kinda ugly, but I think it's tolerable.
> 
> Right, we must not affect non-secure guests, and existing secure setups
> (e.g., older qemu machines). Will have to think about this some more,
> but does not sound too crazy.

I severely dislike having to specify things to make PV work.
The IOMMU is already a thorn in our side and we're working on making the
whole ordeal completely transparent so the only requirement to make this
work is the right machine, kernel, qemu and kernel cmd line option
"prot_virt=1". That's why we do the reboot into PV mode in the first place.

I.e. the goal is that if customers convert compatible guests into
protected ones and start them up on a z15 on a distro with PV support
they can just use the guest without having to change XML or command line
parameters.

Internal customers have already created bugs because they did not follow
the documentation and the more cmd options we bring the more bugzillas
we'll get.

PV is already in the field/GA and can be ordered, as is our documentation.

@Christian: Please chime in here

> 
> Thanks!
> 



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

Re: [PATCH v3 0/9] Generalize memory encryption models

[Daniel P. Berrangé]

On Fri, Jun 26, 2020 at 11:01:58AM +0200, Janosch Frank wrote:
> On 6/26/20 8:53 AM, David Hildenbrand wrote:
> >>>>> Does this have any implications when probing with the 'none' machine?
> >>>>
> >>>> I'm not sure.  In your case, I guess the cpu bit would still show up
> >>>> as before, so it would tell you base feature availability, but not
> >>>> whether you can use the new configuration option.
> >>>>
> >>>> Since the HTL option is generic, you could still set it on the "none"
> >>>> machine, though it wouldn't really have any effect.  That is, if you
> >>>> could create a suitable object to point it at, which would depend on
> >>>> ... details.
> >>>>
> >>>
> >>> The important point is that we never want the (expanded) host cpu model
> >>> look different when either specifying or not specifying the HTL
> >>> property.
> >>
> >> Ah, yes, I see your point.  So my current suggestion will satisfy
> >> that, basically it is:
> >>
> >> cpu has unpack (inc. by default) && htl specified
> >> 	=> works (allowing secure), as expected
> > 
> > ack
> > 
> >>
> >> !cpu has unpack && htl specified
> >> 	=> bails out with an error
> > 
> > ack
> > 
> >>
> >> !cpu has unpack && !htl specified
> >> 	=> works for a non-secure guest, as expected
> >> 	=> guest will fail if it attempts to go secure
> > 
> > ack, behavior just like running on older hw without unpack
> > 
> >>
> >> cpu has unpack && !htl specified
> >> 	=> works as expected for a non-secure guest (unpack feature is
> >> 	   present, but unused)
> >> 	=> secure guest may work "by accident", but only if all virtio
> >> 	   properties have the right values, which is the user's
> >> 	   problem
> >>
> >> That last case is kinda ugly, but I think it's tolerable.
> > 
> > Right, we must not affect non-secure guests, and existing secure setups
> > (e.g., older qemu machines). Will have to think about this some more,
> > but does not sound too crazy.
> 
> I severely dislike having to specify things to make PV work.
> The IOMMU is already a thorn in our side and we're working on making the
> whole ordeal completely transparent so the only requirement to make this
> work is the right machine, kernel, qemu and kernel cmd line option
> "prot_virt=1". That's why we do the reboot into PV mode in the first place.
> 
> I.e. the goal is that if customers convert compatible guests into
> protected ones and start them up on a z15 on a distro with PV support
> they can just use the guest without having to change XML or command line
> parameters.

If you're exposing new features to the guest machine, then it is usually
to be expected that XML and QEMU command line will change. Some simple
things might be hidable behind a new QEMU machine type or CPU model, but
there's a limit to how much should be hidden that way while staying sane.

I'd really expect the configuration to change when switching a guest to
a new hardware platform and wanting major new functionality to be enabled.
The XML / QEMU config is a low level instantiation of a particular feature
set, optimized for a specific machine, rather than a high level description
of ideal "best" config independent of host machine.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/9] Generalize memory encryption models

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 4023 bytes --]

On 6/26/20 11:32 AM, Daniel P. Berrangé wrote:
> On Fri, Jun 26, 2020 at 11:01:58AM +0200, Janosch Frank wrote:
>> On 6/26/20 8:53 AM, David Hildenbrand wrote:
>>>>>>> Does this have any implications when probing with the 'none' machine?
>>>>>>
>>>>>> I'm not sure.  In your case, I guess the cpu bit would still show up
>>>>>> as before, so it would tell you base feature availability, but not
>>>>>> whether you can use the new configuration option.
>>>>>>
>>>>>> Since the HTL option is generic, you could still set it on the "none"
>>>>>> machine, though it wouldn't really have any effect.  That is, if you
>>>>>> could create a suitable object to point it at, which would depend on
>>>>>> ... details.
>>>>>>
>>>>>
>>>>> The important point is that we never want the (expanded) host cpu model
>>>>> look different when either specifying or not specifying the HTL
>>>>> property.
>>>>
>>>> Ah, yes, I see your point.  So my current suggestion will satisfy
>>>> that, basically it is:
>>>>
>>>> cpu has unpack (inc. by default) && htl specified
>>>> 	=> works (allowing secure), as expected
>>>
>>> ack
>>>
>>>>
>>>> !cpu has unpack && htl specified
>>>> 	=> bails out with an error
>>>
>>> ack
>>>
>>>>
>>>> !cpu has unpack && !htl specified
>>>> 	=> works for a non-secure guest, as expected
>>>> 	=> guest will fail if it attempts to go secure
>>>
>>> ack, behavior just like running on older hw without unpack
>>>
>>>>
>>>> cpu has unpack && !htl specified
>>>> 	=> works as expected for a non-secure guest (unpack feature is
>>>> 	   present, but unused)
>>>> 	=> secure guest may work "by accident", but only if all virtio
>>>> 	   properties have the right values, which is the user's
>>>> 	   problem
>>>>
>>>> That last case is kinda ugly, but I think it's tolerable.
>>>
>>> Right, we must not affect non-secure guests, and existing secure setups
>>> (e.g., older qemu machines). Will have to think about this some more,
>>> but does not sound too crazy.
>>
>> I severely dislike having to specify things to make PV work.
>> The IOMMU is already a thorn in our side and we're working on making the
>> whole ordeal completely transparent so the only requirement to make this
>> work is the right machine, kernel, qemu and kernel cmd line option
>> "prot_virt=1". That's why we do the reboot into PV mode in the first place.
>>
>> I.e. the goal is that if customers convert compatible guests into
>> protected ones and start them up on a z15 on a distro with PV support
>> they can just use the guest without having to change XML or command line
>> parameters.
> 
> If you're exposing new features to the guest machine, then it is usually
> to be expected that XML and QEMU command line will change. Some simple
> things might be hidable behind a new QEMU machine type or CPU model, but
> there's a limit to how much should be hidden that way while staying sane.
> 
> I'd really expect the configuration to change when switching a guest to
> a new hardware platform and wanting major new functionality to be enabled.
> The XML / QEMU config is a low level instantiation of a particular feature
> set, optimized for a specific machine, rather than a high level description
> of ideal "best" config independent of host machine.

You still have to set the host command line and make sure that unpack is
available. Currently you also have to specify the IOMMU which we like to
drop as a requirement. Everything else is dependent on runtime
information which tells us if we need to take a PV or non-PV branch.
Having the unpack facility should be enough to use the unpack facility.

Keep in mind that we have no real concept of a special protected VM to
begin with. If the VM never boots into a protected kernel it will never
be protected. On a reboot it drops from protected into unprotected mode
to execute the bios and boot loader and then may or may not move back
into a protected state.

> 
> Regards,
> Daniel
> 



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

Re: [PATCH v3 0/9] Generalize memory encryption models

[Dr. David Alan Gilbert]

* Janosch Frank (frankja@linux.ibm.com) wrote:
> On 6/26/20 11:32 AM, Daniel P. Berrangé wrote:
> > On Fri, Jun 26, 2020 at 11:01:58AM +0200, Janosch Frank wrote:
> >> On 6/26/20 8:53 AM, David Hildenbrand wrote:
> >>>>>>> Does this have any implications when probing with the 'none' machine?
> >>>>>>
> >>>>>> I'm not sure.  In your case, I guess the cpu bit would still show up
> >>>>>> as before, so it would tell you base feature availability, but not
> >>>>>> whether you can use the new configuration option.
> >>>>>>
> >>>>>> Since the HTL option is generic, you could still set it on the "none"
> >>>>>> machine, though it wouldn't really have any effect.  That is, if you
> >>>>>> could create a suitable object to point it at, which would depend on
> >>>>>> ... details.
> >>>>>>
> >>>>>
> >>>>> The important point is that we never want the (expanded) host cpu model
> >>>>> look different when either specifying or not specifying the HTL
> >>>>> property.
> >>>>
> >>>> Ah, yes, I see your point.  So my current suggestion will satisfy
> >>>> that, basically it is:
> >>>>
> >>>> cpu has unpack (inc. by default) && htl specified
> >>>> 	=> works (allowing secure), as expected
> >>>
> >>> ack
> >>>
> >>>>
> >>>> !cpu has unpack && htl specified
> >>>> 	=> bails out with an error
> >>>
> >>> ack
> >>>
> >>>>
> >>>> !cpu has unpack && !htl specified
> >>>> 	=> works for a non-secure guest, as expected
> >>>> 	=> guest will fail if it attempts to go secure
> >>>
> >>> ack, behavior just like running on older hw without unpack
> >>>
> >>>>
> >>>> cpu has unpack && !htl specified
> >>>> 	=> works as expected for a non-secure guest (unpack feature is
> >>>> 	   present, but unused)
> >>>> 	=> secure guest may work "by accident", but only if all virtio
> >>>> 	   properties have the right values, which is the user's
> >>>> 	   problem
> >>>>
> >>>> That last case is kinda ugly, but I think it's tolerable.
> >>>
> >>> Right, we must not affect non-secure guests, and existing secure setups
> >>> (e.g., older qemu machines). Will have to think about this some more,
> >>> but does not sound too crazy.
> >>
> >> I severely dislike having to specify things to make PV work.
> >> The IOMMU is already a thorn in our side and we're working on making the
> >> whole ordeal completely transparent so the only requirement to make this
> >> work is the right machine, kernel, qemu and kernel cmd line option
> >> "prot_virt=1". That's why we do the reboot into PV mode in the first place.
> >>
> >> I.e. the goal is that if customers convert compatible guests into
> >> protected ones and start them up on a z15 on a distro with PV support
> >> they can just use the guest without having to change XML or command line
> >> parameters.
> > 
> > If you're exposing new features to the guest machine, then it is usually
> > to be expected that XML and QEMU command line will change. Some simple
> > things might be hidable behind a new QEMU machine type or CPU model, but
> > there's a limit to how much should be hidden that way while staying sane.
> > 
> > I'd really expect the configuration to change when switching a guest to
> > a new hardware platform and wanting major new functionality to be enabled.
> > The XML / QEMU config is a low level instantiation of a particular feature
> > set, optimized for a specific machine, rather than a high level description
> > of ideal "best" config independent of host machine.
> 
> You still have to set the host command line and make sure that unpack is
> available. Currently you also have to specify the IOMMU which we like to
> drop as a requirement. Everything else is dependent on runtime
> information which tells us if we need to take a PV or non-PV branch.
> Having the unpack facility should be enough to use the unpack facility.
> 
> Keep in mind that we have no real concept of a special protected VM to
> begin with. If the VM never boots into a protected kernel it will never
> be protected. On a reboot it drops from protected into unprotected mode
> to execute the bios and boot loader and then may or may not move back
> into a protected state.

My worry isn't actually how painful adding all the iommu glue is, but
what happens when users forget; especially if they forget for one
device.

I could appreciate having a machine option to cause iommu to then get
turned on with all other devices; but I think also we could do with
something that failed with a nice error if an iommu flag was missing.
For SEV this could be done pretty early, but for power/s390 I guess
you'd have to do this when someone tried to enable secure mode, but
I'm not sure you can tell.

Dave


> > 
> > Regards,
> > Daniel
> > 
> 
> 



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

Re: [PATCH v3 8/9] spapr: PEF: block migration

[Dr. David Alan Gilbert]

* David Gibson (david@gibson.dropbear.id.au) wrote:
> We haven't yet implemented the fairly involved handshaking that will be
> needed to migrate PEF protected guests.  For now, just use a migration
> blocker so we get a meaningful error if someone attempts this (this is the
> same approach used by AMD SEV).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Do you expect this to happen if people run with -cpu host ?

Dave

> ---
>  target/ppc/pef.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target/ppc/pef.c b/target/ppc/pef.c
> index 53a6af0347..6a50efd580 100644
> --- a/target/ppc/pef.c
> +++ b/target/ppc/pef.c
> @@ -36,6 +36,8 @@ struct PefGuestState {
>      Object parent_obj;
>  };
>  
> +static Error *pef_mig_blocker;
> +
>  static int pef_kvm_init(HostTrustLimitation *gmpo, Error **errp)
>  {
>      if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_SECURE_GUEST)) {
> @@ -52,6 +54,10 @@ static int pef_kvm_init(HostTrustLimitation *gmpo, Error **errp)
>          }
>      }
>  
> +    /* add migration blocker */
> +    error_setg(&pef_mig_blocker, "PEF: Migration is not implemented");
> +    migrate_add_blocker(pef_mig_blocker, &error_abort);
> +
>      return 0;
>  }
>  
> -- 
> 2.26.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 0/9] Generalize memory encryption models

[Daniel P. Berrangé]

On Fri, Jun 26, 2020 at 11:29:03AM +0100, Dr. David Alan Gilbert wrote:
> * Janosch Frank (frankja@linux.ibm.com) wrote:
> > On 6/26/20 11:32 AM, Daniel P. Berrangé wrote:
> > > On Fri, Jun 26, 2020 at 11:01:58AM +0200, Janosch Frank wrote:
> > >> On 6/26/20 8:53 AM, David Hildenbrand wrote:
> > >>>>>>> Does this have any implications when probing with the 'none' machine?
> > >>>>>>
> > >>>>>> I'm not sure.  In your case, I guess the cpu bit would still show up
> > >>>>>> as before, so it would tell you base feature availability, but not
> > >>>>>> whether you can use the new configuration option.
> > >>>>>>
> > >>>>>> Since the HTL option is generic, you could still set it on the "none"
> > >>>>>> machine, though it wouldn't really have any effect.  That is, if you
> > >>>>>> could create a suitable object to point it at, which would depend on
> > >>>>>> ... details.
> > >>>>>>
> > >>>>>
> > >>>>> The important point is that we never want the (expanded) host cpu model
> > >>>>> look different when either specifying or not specifying the HTL
> > >>>>> property.
> > >>>>
> > >>>> Ah, yes, I see your point.  So my current suggestion will satisfy
> > >>>> that, basically it is:
> > >>>>
> > >>>> cpu has unpack (inc. by default) && htl specified
> > >>>> 	=> works (allowing secure), as expected
> > >>>
> > >>> ack
> > >>>
> > >>>>
> > >>>> !cpu has unpack && htl specified
> > >>>> 	=> bails out with an error
> > >>>
> > >>> ack
> > >>>
> > >>>>
> > >>>> !cpu has unpack && !htl specified
> > >>>> 	=> works for a non-secure guest, as expected
> > >>>> 	=> guest will fail if it attempts to go secure
> > >>>
> > >>> ack, behavior just like running on older hw without unpack
> > >>>
> > >>>>
> > >>>> cpu has unpack && !htl specified
> > >>>> 	=> works as expected for a non-secure guest (unpack feature is
> > >>>> 	   present, but unused)
> > >>>> 	=> secure guest may work "by accident", but only if all virtio
> > >>>> 	   properties have the right values, which is the user's
> > >>>> 	   problem
> > >>>>
> > >>>> That last case is kinda ugly, but I think it's tolerable.
> > >>>
> > >>> Right, we must not affect non-secure guests, and existing secure setups
> > >>> (e.g., older qemu machines). Will have to think about this some more,
> > >>> but does not sound too crazy.
> > >>
> > >> I severely dislike having to specify things to make PV work.
> > >> The IOMMU is already a thorn in our side and we're working on making the
> > >> whole ordeal completely transparent so the only requirement to make this
> > >> work is the right machine, kernel, qemu and kernel cmd line option
> > >> "prot_virt=1". That's why we do the reboot into PV mode in the first place.
> > >>
> > >> I.e. the goal is that if customers convert compatible guests into
> > >> protected ones and start them up on a z15 on a distro with PV support
> > >> they can just use the guest without having to change XML or command line
> > >> parameters.
> > > 
> > > If you're exposing new features to the guest machine, then it is usually
> > > to be expected that XML and QEMU command line will change. Some simple
> > > things might be hidable behind a new QEMU machine type or CPU model, but
> > > there's a limit to how much should be hidden that way while staying sane.
> > > 
> > > I'd really expect the configuration to change when switching a guest to
> > > a new hardware platform and wanting major new functionality to be enabled.
> > > The XML / QEMU config is a low level instantiation of a particular feature
> > > set, optimized for a specific machine, rather than a high level description
> > > of ideal "best" config independent of host machine.
> > 
> > You still have to set the host command line and make sure that unpack is
> > available. Currently you also have to specify the IOMMU which we like to
> > drop as a requirement. Everything else is dependent on runtime
> > information which tells us if we need to take a PV or non-PV branch.
> > Having the unpack facility should be enough to use the unpack facility.
> > 
> > Keep in mind that we have no real concept of a special protected VM to
> > begin with. If the VM never boots into a protected kernel it will never
> > be protected. On a reboot it drops from protected into unprotected mode
> > to execute the bios and boot loader and then may or may not move back
> > into a protected state.
> 
> My worry isn't actually how painful adding all the iommu glue is, but
> what happens when users forget; especially if they forget for one
> device.
> 
> I could appreciate having a machine option to cause iommu to then get
> turned on with all other devices; but I think also we could do with
> something that failed with a nice error if an iommu flag was missing.
> For SEV this could be done pretty early, but for power/s390 I guess
> you'd have to do this when someone tried to enable secure mode, but
> I'm not sure you can tell.

What is the cost / downside of turning on the iommu option for virtio
devices ? Is it something that is reasonable for a mgmt app todo
unconditionally, regardless of whether memory encryption is in use,
or will that have a negative impact on things ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 1/9] host trust limitation: Introduce new host trust limitation interface

[Dr. David Alan Gilbert]

* 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.
> 
> To (partially) unify handling for these, this introduces a new
> HostTrustLimitation QOM interface.

This does make some sense to me from a SEV point of view, so

Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  backends/Makefile.objs               |  2 ++
>  backends/host-trust-limitation.c     | 29 ++++++++++++++++++++++++
>  include/exec/host-trust-limitation.h | 33 ++++++++++++++++++++++++++++
>  include/qemu/typedefs.h              |  1 +
>  4 files changed, 65 insertions(+)
>  create mode 100644 backends/host-trust-limitation.c
>  create mode 100644 include/exec/host-trust-limitation.h
> 
> diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> index 28a847cd57..af761c9ab1 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 += host-trust-limitation.o
> diff --git a/backends/host-trust-limitation.c b/backends/host-trust-limitation.c
> new file mode 100644
> index 0000000000..96a381cd8a
> --- /dev/null
> +++ b/backends/host-trust-limitation.c
> @@ -0,0 +1,29 @@
> +/*
> + * QEMU Host Trust Limitation 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/host-trust-limitation.h"
> +
> +static const TypeInfo host_trust_limitation_info = {
> +    .name = TYPE_HOST_TRUST_LIMITATION,
> +    .parent = TYPE_INTERFACE,
> +    .class_size = sizeof(HostTrustLimitationClass),
> +};
> +
> +static void host_trust_limitation_register_types(void)
> +{
> +    type_register_static(&host_trust_limitation_info);
> +}
> +
> +type_init(host_trust_limitation_register_types)
> diff --git a/include/exec/host-trust-limitation.h b/include/exec/host-trust-limitation.h
> new file mode 100644
> index 0000000000..03887b1be1
> --- /dev/null
> +++ b/include/exec/host-trust-limitation.h
> @@ -0,0 +1,33 @@
> +/*
> + * QEMU Host Trust Limitation 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_HOST_TRUST_LIMITATION_H
> +#define QEMU_HOST_TRUST_LIMITATION_H
> +
> +#include "qom/object.h"
> +
> +#define TYPE_HOST_TRUST_LIMITATION "host-trust-limitation"
> +#define HOST_TRUST_LIMITATION(obj)                                    \
> +    INTERFACE_CHECK(HostTrustLimitation, (obj),                       \
> +                    TYPE_HOST_TRUST_LIMITATION)
> +#define HOST_TRUST_LIMITATION_CLASS(klass)                            \
> +    OBJECT_CLASS_CHECK(HostTrustLimitationClass, (klass),             \
> +                       TYPE_HOST_TRUST_LIMITATION)
> +#define HOST_TRUST_LIMITATION_GET_CLASS(obj)                          \
> +    OBJECT_GET_CLASS(HostTrustLimitationClass, (obj),                 \
> +                     TYPE_HOST_TRUST_LIMITATION)
> +
> +typedef struct HostTrustLimitationClass {
> +    InterfaceClass parent;
> +} HostTrustLimitationClass;
> +
> +#endif /* QEMU_HOST_TRUST_LIMITATION_H */
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index ce4a78b687..f75c7eb2f2 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -51,6 +51,7 @@ typedef struct FWCfgIoState FWCfgIoState;
>  typedef struct FWCfgMemState FWCfgMemState;
>  typedef struct FWCfgState FWCfgState;
>  typedef struct HostMemoryBackend HostMemoryBackend;
> +typedef struct HostTrustLimitation HostTrustLimitation;
>  typedef struct I2CBus I2CBus;
>  typedef struct I2SCodec I2SCodec;
>  typedef struct IOMMUMemoryRegion IOMMUMemoryRegion;
> -- 
> 2.26.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 0/9] Generalize memory encryption models

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 5719 bytes --]

On 6/26/20 12:58 PM, Daniel P. Berrangé wrote:
> On Fri, Jun 26, 2020 at 11:29:03AM +0100, Dr. David Alan Gilbert wrote:
>> * Janosch Frank (frankja@linux.ibm.com) wrote:
>>> On 6/26/20 11:32 AM, Daniel P. Berrangé wrote:
>>>> On Fri, Jun 26, 2020 at 11:01:58AM +0200, Janosch Frank wrote:
>>>>> On 6/26/20 8:53 AM, David Hildenbrand wrote:
>>>>>>>>>> Does this have any implications when probing with the 'none' machine?
>>>>>>>>>
>>>>>>>>> I'm not sure.  In your case, I guess the cpu bit would still show up
>>>>>>>>> as before, so it would tell you base feature availability, but not
>>>>>>>>> whether you can use the new configuration option.
>>>>>>>>>
>>>>>>>>> Since the HTL option is generic, you could still set it on the "none"
>>>>>>>>> machine, though it wouldn't really have any effect.  That is, if you
>>>>>>>>> could create a suitable object to point it at, which would depend on
>>>>>>>>> ... details.
>>>>>>>>>
>>>>>>>>
>>>>>>>> The important point is that we never want the (expanded) host cpu model
>>>>>>>> look different when either specifying or not specifying the HTL
>>>>>>>> property.
>>>>>>>
>>>>>>> Ah, yes, I see your point.  So my current suggestion will satisfy
>>>>>>> that, basically it is:
>>>>>>>
>>>>>>> cpu has unpack (inc. by default) && htl specified
>>>>>>> 	=> works (allowing secure), as expected
>>>>>>
>>>>>> ack
>>>>>>
>>>>>>>
>>>>>>> !cpu has unpack && htl specified
>>>>>>> 	=> bails out with an error
>>>>>>
>>>>>> ack
>>>>>>
>>>>>>>
>>>>>>> !cpu has unpack && !htl specified
>>>>>>> 	=> works for a non-secure guest, as expected
>>>>>>> 	=> guest will fail if it attempts to go secure
>>>>>>
>>>>>> ack, behavior just like running on older hw without unpack
>>>>>>
>>>>>>>
>>>>>>> cpu has unpack && !htl specified
>>>>>>> 	=> works as expected for a non-secure guest (unpack feature is
>>>>>>> 	   present, but unused)
>>>>>>> 	=> secure guest may work "by accident", but only if all virtio
>>>>>>> 	   properties have the right values, which is the user's
>>>>>>> 	   problem
>>>>>>>
>>>>>>> That last case is kinda ugly, but I think it's tolerable.
>>>>>>
>>>>>> Right, we must not affect non-secure guests, and existing secure setups
>>>>>> (e.g., older qemu machines). Will have to think about this some more,
>>>>>> but does not sound too crazy.
>>>>>
>>>>> I severely dislike having to specify things to make PV work.
>>>>> The IOMMU is already a thorn in our side and we're working on making the
>>>>> whole ordeal completely transparent so the only requirement to make this
>>>>> work is the right machine, kernel, qemu and kernel cmd line option
>>>>> "prot_virt=1". That's why we do the reboot into PV mode in the first place.
>>>>>
>>>>> I.e. the goal is that if customers convert compatible guests into
>>>>> protected ones and start them up on a z15 on a distro with PV support
>>>>> they can just use the guest without having to change XML or command line
>>>>> parameters.
>>>>
>>>> If you're exposing new features to the guest machine, then it is usually
>>>> to be expected that XML and QEMU command line will change. Some simple
>>>> things might be hidable behind a new QEMU machine type or CPU model, but
>>>> there's a limit to how much should be hidden that way while staying sane.
>>>>
>>>> I'd really expect the configuration to change when switching a guest to
>>>> a new hardware platform and wanting major new functionality to be enabled.
>>>> The XML / QEMU config is a low level instantiation of a particular feature
>>>> set, optimized for a specific machine, rather than a high level description
>>>> of ideal "best" config independent of host machine.
>>>
>>> You still have to set the host command line and make sure that unpack is
>>> available. Currently you also have to specify the IOMMU which we like to
>>> drop as a requirement. Everything else is dependent on runtime
>>> information which tells us if we need to take a PV or non-PV branch.
>>> Having the unpack facility should be enough to use the unpack facility.
>>>
>>> Keep in mind that we have no real concept of a special protected VM to
>>> begin with. If the VM never boots into a protected kernel it will never
>>> be protected. On a reboot it drops from protected into unprotected mode
>>> to execute the bios and boot loader and then may or may not move back
>>> into a protected state.
>>
>> My worry isn't actually how painful adding all the iommu glue is, but
>> what happens when users forget; especially if they forget for one
>> device.
>>
>> I could appreciate having a machine option to cause iommu to then get
>> turned on with all other devices; but I think also we could do with
>> something that failed with a nice error if an iommu flag was missing.
>> For SEV this could be done pretty early, but for power/s390 I guess
>> you'd have to do this when someone tried to enable secure mode, but
>> I'm not sure you can tell.
> 
> What is the cost / downside of turning on the iommu option for virtio
> devices ? Is it something that is reasonable for a mgmt app todo
> unconditionally, regardless of whether memory encryption is in use,
> or will that have a negative impact on things ?

speed, memory usage and compatibility problems.
There might also be a problem with s390 having to use <=2GB iommu areas
in the guest, I need to check with Halil if this is still true.

Also, if the default or specified IOMMU buffer size isn't big enough for
your IO workload the guest is gonna have a very bad time. I.e. if
somebody has an alternative implementation of bounce buffers we'd be
happy to take it :)

> 
> Regards,
> Daniel
> 



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

Re: [PATCH v3 0/9] Generalize memory encryption models

[Halil Pasic]

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

On Fri, 26 Jun 2020 14:49:37 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 6/26/20 12:58 PM, Daniel P. Berrangé wrote:
> > On Fri, Jun 26, 2020 at 11:29:03AM +0100, Dr. David Alan Gilbert wrote:
> >> * Janosch Frank (frankja@linux.ibm.com) wrote:
> >>> On 6/26/20 11:32 AM, Daniel P. Berrangé wrote:
> >>>> On Fri, Jun 26, 2020 at 11:01:58AM +0200, Janosch Frank wrote:
> >>>>> On 6/26/20 8:53 AM, David Hildenbrand wrote:
> >>>>>>>>>> Does this have any implications when probing with the 'none' machine?
> >>>>>>>>>
> >>>>>>>>> I'm not sure.  In your case, I guess the cpu bit would still show up
> >>>>>>>>> as before, so it would tell you base feature availability, but not
> >>>>>>>>> whether you can use the new configuration option.
> >>>>>>>>>
> >>>>>>>>> Since the HTL option is generic, you could still set it on the "none"
> >>>>>>>>> machine, though it wouldn't really have any effect.  That is, if you
> >>>>>>>>> could create a suitable object to point it at, which would depend on
> >>>>>>>>> ... details.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> The important point is that we never want the (expanded) host cpu model
> >>>>>>>> look different when either specifying or not specifying the HTL
> >>>>>>>> property.
> >>>>>>>
> >>>>>>> Ah, yes, I see your point.  So my current suggestion will satisfy
> >>>>>>> that, basically it is:
> >>>>>>>
> >>>>>>> cpu has unpack (inc. by default) && htl specified
> >>>>>>> 	=> works (allowing secure), as expected
> >>>>>>
> >>>>>> ack
> >>>>>>
> >>>>>>>
> >>>>>>> !cpu has unpack && htl specified
> >>>>>>> 	=> bails out with an error
> >>>>>>
> >>>>>> ack
> >>>>>>
> >>>>>>>
> >>>>>>> !cpu has unpack && !htl specified
> >>>>>>> 	=> works for a non-secure guest, as expected
> >>>>>>> 	=> guest will fail if it attempts to go secure
> >>>>>>
> >>>>>> ack, behavior just like running on older hw without unpack
> >>>>>>
> >>>>>>>
> >>>>>>> cpu has unpack && !htl specified
> >>>>>>> 	=> works as expected for a non-secure guest (unpack feature is
> >>>>>>> 	   present, but unused)
> >>>>>>> 	=> secure guest may work "by accident", but only if all virtio
> >>>>>>> 	   properties have the right values, which is the user's
> >>>>>>> 	   problem
> >>>>>>>
> >>>>>>> That last case is kinda ugly, but I think it's tolerable.
> >>>>>>
> >>>>>> Right, we must not affect non-secure guests, and existing secure setups
> >>>>>> (e.g., older qemu machines). Will have to think about this some more,
> >>>>>> but does not sound too crazy.
> >>>>>
> >>>>> I severely dislike having to specify things to make PV work.
> >>>>> The IOMMU is already a thorn in our side and we're working on making the
> >>>>> whole ordeal completely transparent so the only requirement to make this
> >>>>> work is the right machine, kernel, qemu and kernel cmd line option
> >>>>> "prot_virt=1". That's why we do the reboot into PV mode in the first place.
> >>>>>
> >>>>> I.e. the goal is that if customers convert compatible guests into
> >>>>> protected ones and start them up on a z15 on a distro with PV support
> >>>>> they can just use the guest without having to change XML or command line
> >>>>> parameters.
> >>>>
> >>>> If you're exposing new features to the guest machine, then it is usually
> >>>> to be expected that XML and QEMU command line will change. Some simple
> >>>> things might be hidable behind a new QEMU machine type or CPU model, but
> >>>> there's a limit to how much should be hidden that way while staying sane.
> >>>>
> >>>> I'd really expect the configuration to change when switching a guest to
> >>>> a new hardware platform and wanting major new functionality to be enabled.
> >>>> The XML / QEMU config is a low level instantiation of a particular feature
> >>>> set, optimized for a specific machine, rather than a high level description
> >>>> of ideal "best" config independent of host machine.
> >>>
> >>> You still have to set the host command line and make sure that unpack is
> >>> available. Currently you also have to specify the IOMMU which we like to
> >>> drop as a requirement. Everything else is dependent on runtime
> >>> information which tells us if we need to take a PV or non-PV branch.
> >>> Having the unpack facility should be enough to use the unpack facility.
> >>>
> >>> Keep in mind that we have no real concept of a special protected VM to
> >>> begin with. If the VM never boots into a protected kernel it will never
> >>> be protected. On a reboot it drops from protected into unprotected mode
> >>> to execute the bios and boot loader and then may or may not move back
> >>> into a protected state.
> >>
> >> My worry isn't actually how painful adding all the iommu glue is, but
> >> what happens when users forget; especially if they forget for one
> >> device.
> >>
> >> I could appreciate having a machine option to cause iommu to then get
> >> turned on with all other devices; but I think also we could do with
> >> something that failed with a nice error if an iommu flag was missing.
> >> For SEV this could be done pretty early, but for power/s390 I guess
> >> you'd have to do this when someone tried to enable secure mode, but
> >> I'm not sure you can tell.
> > 
> > What is the cost / downside of turning on the iommu option for virtio
> > devices ? Is it something that is reasonable for a mgmt app todo
> > unconditionally, regardless of whether memory encryption is in use,
> > or will that have a negative impact on things ?
> 
> speed, memory usage and compatibility problems.
> There might also be a problem with s390 having to use <=2GB iommu areas
> in the guest, I need to check with Halil if this is still true.

It is partially true. The coherent_dma_mask is 31 bit and the dma_mask
is 64. That means if iommu=on but !PV the coherent stuff will use <= 2GB
(that stuff allocated by virtio core, like virtqueues, CCWs, etc.) but
there will be no bounce buffering. We don't even initialize swiotlb if
!PV.

I agree with Janosch, we want iommu='on' only when really needed. I've
tried to make that point several times.

Regards,
Halil

> 
> Also, if the default or specified IOMMU buffer size isn't big enough for
> your IO workload the guest is gonna have a very bad time. I.e. if
> somebody has an alternative implementation of bounce buffers we'd be
> happy to take it :)
> 
> > 
> > Regards,
> > Daniel
> > 
> 
> 


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

Re: [PATCH v3 8/9] spapr: PEF: block migration

[David Gibson]

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

On Fri, Jun 26, 2020 at 11:33:03AM +0100, Dr. David Alan Gilbert wrote:
> * David Gibson (david@gibson.dropbear.id.au) wrote:
> > We haven't yet implemented the fairly involved handshaking that will be
> > needed to migrate PEF protected guests.  For now, just use a migration
> > blocker so we get a meaningful error if someone attempts this (this is the
> > same approach used by AMD SEV).
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Do you expect this to happen if people run with -cpu host ?

Uh.. I don't really understand the question.  What's the connection
between cpu model and migration blocking?

-- 
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: [PATCH v3 1/9] host trust limitation: Introduce new host trust limitation interface

[Richard Henderson]

On 6/18/20 7:05 PM, David Gibson 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.
> 
> To (partially) unify handling for these, this introduces a new
> HostTrustLimitation QOM interface.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  backends/Makefile.objs               |  2 ++
>  backends/host-trust-limitation.c     | 29 ++++++++++++++++++++++++
>  include/exec/host-trust-limitation.h | 33 ++++++++++++++++++++++++++++
>  include/qemu/typedefs.h              |  1 +
>  4 files changed, 65 insertions(+)
>  create mode 100644 backends/host-trust-limitation.c
>  create mode 100644 include/exec/host-trust-limitation.h

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

r~

Re: [PATCH v3 4/9] host trust limitation: Rework the "memory-encryption" property

[Richard Henderson]

On 6/18/20 7:05 PM, David Gibson wrote:
> 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 "host-trust-limitation" 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 |  2 +-
>  3 files changed, 44 insertions(+), 22 deletions(-)

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

r~