Re: [PATCH v6 05/13] confidential guest support: Rework the "memory-encryption" property

2021-01-13 Thread Dr. David Alan Gilbert
* David Gibson (da...@gibson.dropbear.id.au) wrote:
> On Tue, Jan 12, 2021 at 11:59:59AM +0100, Greg Kurz wrote:
> > On Tue, 12 Jan 2021 15:45:00 +1100
> > 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 "confidential-guest-support" 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.
> > 
> > Do we really want to keep "memory-encryption" in the long term ? If
> > not, then maybe engage the deprecation process and add a warning in
> > machine_set_memory_encryption() ?
> 
> Hmm.. I kind of think that's up to the SEV people to decide on the
> timetable (if any) for deprecation - it's their existing option.  In
> any case I'd prefer to leave that to a separate patch.
> 
> Dave (Gilbert), any opinions?

Well, the first thing would be to get libvirt to know about the new
mechanism; only when it's happy can we even think about deprecating the
old one;  but yes in the long term it makes sense.

Dave

> > Apart from that, LGTM:
> > 
> > Reviewed-by: Greg Kurz 
> > 
> > > Signed-off-by: David Gibson 
> > > ---
> > >  accel/kvm/kvm-all.c  |  5 +++--
> > >  accel/kvm/sev-stub.c |  5 +++--
> > >  hw/core/machine.c| 43 +--
> > >  include/hw/boards.h  |  2 +-
> > >  include/sysemu/sev.h |  2 +-
> > >  target/i386/sev.c| 32 ++--
> > >  6 files changed, 47 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > > index 260ed73ffe..28ab126f70 100644
> > > --- a/accel/kvm/kvm-all.c
> > > +++ b/accel/kvm/kvm-all.c
> > > @@ -2181,8 +2181,9 @@ static int kvm_init(MachineState *ms)
> > >   * if memory encryption object is specified then initialize the 
> > > memory
> > >   * encryption context.
> > >   */
> > > -if (ms->memory_encryption) {
> > > -ret = sev_guest_init(ms->memory_encryption);
> > > +if (ms->cgs) {
> > > +/* FIXME handle mechanisms other than SEV */
> > > +ret = sev_kvm_init(ms->cgs);
> > >  if (ret < 0) {
> > >  goto err;
> > >  }
> > > diff --git a/accel/kvm/sev-stub.c b/accel/kvm/sev-stub.c
> > > index 5db9ab8f00..3d4787ae4a 100644
> > > --- a/accel/kvm/sev-stub.c
> > > +++ b/accel/kvm/sev-stub.c
> > > @@ -15,7 +15,8 @@
> > >  #include "qemu-common.h"
> > >  #include "sysemu/sev.h"
> > >  
> > > -int sev_guest_init(const char *id)
> > > +int sev_kvm_init(ConfidentialGuestSupport *cgs)
> > >  {
> > > -return -1;
> > > +/* SEV can't be selected if it's not compiled */
> > > +g_assert_not_reached();
> > >  }
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index 8909117d80..94194ab82d 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -32,6 +32,7 @@
> > >  #include "hw/mem/nvdimm.h"
> > >  #include "migration/global_state.h"
> > >  #include "migration/vmstate.h"
> > > +#include "exec/confidential-guest-support.h"
> > >  
> > >  GlobalProperty hw_compat_5_2[] = {};
> > >  const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
> > > @@ -427,16 +428,37 @@ static char *machine_get_memory_encryption(Object 
> > > *obj, Error **errp)
> > >  {
> > >  MachineState *ms = MACHINE(obj);
> > >  
> > > -return g_strdup(ms->memory_encryption);
> > > +if (ms->cgs) {
> > > +return 
> > > g_strdup(object_get_canonical_path_component(OBJECT(ms->cgs)));
> > > +}
> > > +
> > > +return NULL;
> > >  }
> > >  
> > >  static void machine_set_memory_encryption(Object *obj, const char *value,
> > >  Error **errp)
> > >  {
> > > -MachineState *ms = MACHINE(obj);
> > > +Object *cgs =
> > > +object_resolve_path_component(object_get_objects_root(), value);
> > > +
> > > +if (!cgs) {
> > > +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, "confidential-guest-support", cgs, 
> > > errp);
> > > +}
> > > +
> > > +static void machine_check_confidential_guest_support(const Object *obj,
> > > + const char *name,
> > > + Object *new_target,
> > > + Error **errp)
> > > +{
> > > +/*
> > > + 

Re: [PATCH v6 05/13] confidential guest support: Rework the "memory-encryption" property

2021-01-12 Thread David Gibson
On Tue, Jan 12, 2021 at 11:59:59AM +0100, Greg Kurz wrote:
> On Tue, 12 Jan 2021 15:45:00 +1100
> 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 "confidential-guest-support" 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.
> 
> Do we really want to keep "memory-encryption" in the long term ? If
> not, then maybe engage the deprecation process and add a warning in
> machine_set_memory_encryption() ?

Hmm.. I kind of think that's up to the SEV people to decide on the
timetable (if any) for deprecation - it's their existing option.  In
any case I'd prefer to leave that to a separate patch.

Dave (Gilbert), any opinions?

> Apart from that, LGTM:
> 
> Reviewed-by: Greg Kurz 
> 
> > Signed-off-by: David Gibson 
> > ---
> >  accel/kvm/kvm-all.c  |  5 +++--
> >  accel/kvm/sev-stub.c |  5 +++--
> >  hw/core/machine.c| 43 +--
> >  include/hw/boards.h  |  2 +-
> >  include/sysemu/sev.h |  2 +-
> >  target/i386/sev.c| 32 ++--
> >  6 files changed, 47 insertions(+), 42 deletions(-)
> > 
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index 260ed73ffe..28ab126f70 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -2181,8 +2181,9 @@ static int kvm_init(MachineState *ms)
> >   * if memory encryption object is specified then initialize the memory
> >   * encryption context.
> >   */
> > -if (ms->memory_encryption) {
> > -ret = sev_guest_init(ms->memory_encryption);
> > +if (ms->cgs) {
> > +/* FIXME handle mechanisms other than SEV */
> > +ret = sev_kvm_init(ms->cgs);
> >  if (ret < 0) {
> >  goto err;
> >  }
> > diff --git a/accel/kvm/sev-stub.c b/accel/kvm/sev-stub.c
> > index 5db9ab8f00..3d4787ae4a 100644
> > --- a/accel/kvm/sev-stub.c
> > +++ b/accel/kvm/sev-stub.c
> > @@ -15,7 +15,8 @@
> >  #include "qemu-common.h"
> >  #include "sysemu/sev.h"
> >  
> > -int sev_guest_init(const char *id)
> > +int sev_kvm_init(ConfidentialGuestSupport *cgs)
> >  {
> > -return -1;
> > +/* SEV can't be selected if it's not compiled */
> > +g_assert_not_reached();
> >  }
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 8909117d80..94194ab82d 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -32,6 +32,7 @@
> >  #include "hw/mem/nvdimm.h"
> >  #include "migration/global_state.h"
> >  #include "migration/vmstate.h"
> > +#include "exec/confidential-guest-support.h"
> >  
> >  GlobalProperty hw_compat_5_2[] = {};
> >  const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
> > @@ -427,16 +428,37 @@ static char *machine_get_memory_encryption(Object 
> > *obj, Error **errp)
> >  {
> >  MachineState *ms = MACHINE(obj);
> >  
> > -return g_strdup(ms->memory_encryption);
> > +if (ms->cgs) {
> > +return 
> > g_strdup(object_get_canonical_path_component(OBJECT(ms->cgs)));
> > +}
> > +
> > +return NULL;
> >  }
> >  
> >  static void machine_set_memory_encryption(Object *obj, const char *value,
> >  Error **errp)
> >  {
> > -MachineState *ms = MACHINE(obj);
> > +Object *cgs =
> > +object_resolve_path_component(object_get_objects_root(), value);
> > +
> > +if (!cgs) {
> > +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, "confidential-guest-support", cgs, errp);
> > +}
> > +
> > +static void machine_check_confidential_guest_support(const Object *obj,
> > + const char *name,
> > + Object *new_target,
> > + Error **errp)
> > +{
> > +/*
> > + * So far the only constraint is that the target has the
> > + * TYPE_CONFIDENTIAL_GUEST_SUPPORT interface, and that's checked
> > + * by the QOM core
> > + */
> >  }
> >  
> >  static bool machine_get_nvdimm(Object *obj, Error **errp)
> > @@ -836,6 +858,15 @@ static void machine_class_init(ObjectClass *oc, void 
> > *data)
> >  object_class_property_set_description(oc, "suppress-vmdesc",
> >  "Set on to disable self-describing migration");
> >  
> > +

Re: [PATCH v6 05/13] confidential guest support: Rework the "memory-encryption" property

2021-01-12 Thread Greg Kurz
On Tue, 12 Jan 2021 15:45:00 +1100
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 "confidential-guest-support" 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.
> 

Do we really want to keep "memory-encryption" in the long term ? If
not, then maybe engage the deprecation process and add a warning in
machine_set_memory_encryption() ?

Apart from that, LGTM:

Reviewed-by: Greg Kurz 

> Signed-off-by: David Gibson 
> ---
>  accel/kvm/kvm-all.c  |  5 +++--
>  accel/kvm/sev-stub.c |  5 +++--
>  hw/core/machine.c| 43 +--
>  include/hw/boards.h  |  2 +-
>  include/sysemu/sev.h |  2 +-
>  target/i386/sev.c| 32 ++--
>  6 files changed, 47 insertions(+), 42 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 260ed73ffe..28ab126f70 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2181,8 +2181,9 @@ static int kvm_init(MachineState *ms)
>   * if memory encryption object is specified then initialize the memory
>   * encryption context.
>   */
> -if (ms->memory_encryption) {
> -ret = sev_guest_init(ms->memory_encryption);
> +if (ms->cgs) {
> +/* FIXME handle mechanisms other than SEV */
> +ret = sev_kvm_init(ms->cgs);
>  if (ret < 0) {
>  goto err;
>  }
> diff --git a/accel/kvm/sev-stub.c b/accel/kvm/sev-stub.c
> index 5db9ab8f00..3d4787ae4a 100644
> --- a/accel/kvm/sev-stub.c
> +++ b/accel/kvm/sev-stub.c
> @@ -15,7 +15,8 @@
>  #include "qemu-common.h"
>  #include "sysemu/sev.h"
>  
> -int sev_guest_init(const char *id)
> +int sev_kvm_init(ConfidentialGuestSupport *cgs)
>  {
> -return -1;
> +/* SEV can't be selected if it's not compiled */
> +g_assert_not_reached();
>  }
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 8909117d80..94194ab82d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -32,6 +32,7 @@
>  #include "hw/mem/nvdimm.h"
>  #include "migration/global_state.h"
>  #include "migration/vmstate.h"
> +#include "exec/confidential-guest-support.h"
>  
>  GlobalProperty hw_compat_5_2[] = {};
>  const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
> @@ -427,16 +428,37 @@ static char *machine_get_memory_encryption(Object *obj, 
> Error **errp)
>  {
>  MachineState *ms = MACHINE(obj);
>  
> -return g_strdup(ms->memory_encryption);
> +if (ms->cgs) {
> +return 
> g_strdup(object_get_canonical_path_component(OBJECT(ms->cgs)));
> +}
> +
> +return NULL;
>  }
>  
>  static void machine_set_memory_encryption(Object *obj, const char *value,
>  Error **errp)
>  {
> -MachineState *ms = MACHINE(obj);
> +Object *cgs =
> +object_resolve_path_component(object_get_objects_root(), value);
> +
> +if (!cgs) {
> +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, "confidential-guest-support", cgs, errp);
> +}
> +
> +static void machine_check_confidential_guest_support(const Object *obj,
> + const char *name,
> + Object *new_target,
> + Error **errp)
> +{
> +/*
> + * So far the only constraint is that the target has the
> + * TYPE_CONFIDENTIAL_GUEST_SUPPORT interface, and that's checked
> + * by the QOM core
> + */
>  }
>  
>  static bool machine_get_nvdimm(Object *obj, Error **errp)
> @@ -836,6 +858,15 @@ static void machine_class_init(ObjectClass *oc, void 
> *data)
>  object_class_property_set_description(oc, "suppress-vmdesc",
>  "Set on to disable self-describing migration");
>  
> +object_class_property_add_link(oc, "confidential-guest-support",
> +   TYPE_CONFIDENTIAL_GUEST_SUPPORT,
> +   offsetof(MachineState, cgs),
> +   machine_check_confidential_guest_support,
> +   OBJ_PROP_LINK_STRONG);
> +object_class_property_set_description(oc, "confidential-guest-support",
> +  "Set confidential guest scheme to 
> support");
> +
> +/* For compatibility */
>   

[PATCH v6 05/13] confidential guest support: Rework the "memory-encryption" property

2021-01-11 Thread 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 "confidential-guest-support" 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 
---
 accel/kvm/kvm-all.c  |  5 +++--
 accel/kvm/sev-stub.c |  5 +++--
 hw/core/machine.c| 43 +--
 include/hw/boards.h  |  2 +-
 include/sysemu/sev.h |  2 +-
 target/i386/sev.c| 32 ++--
 6 files changed, 47 insertions(+), 42 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 260ed73ffe..28ab126f70 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2181,8 +2181,9 @@ static int kvm_init(MachineState *ms)
  * if memory encryption object is specified then initialize the memory
  * encryption context.
  */
-if (ms->memory_encryption) {
-ret = sev_guest_init(ms->memory_encryption);
+if (ms->cgs) {
+/* FIXME handle mechanisms other than SEV */
+ret = sev_kvm_init(ms->cgs);
 if (ret < 0) {
 goto err;
 }
diff --git a/accel/kvm/sev-stub.c b/accel/kvm/sev-stub.c
index 5db9ab8f00..3d4787ae4a 100644
--- a/accel/kvm/sev-stub.c
+++ b/accel/kvm/sev-stub.c
@@ -15,7 +15,8 @@
 #include "qemu-common.h"
 #include "sysemu/sev.h"
 
-int sev_guest_init(const char *id)
+int sev_kvm_init(ConfidentialGuestSupport *cgs)
 {
-return -1;
+/* SEV can't be selected if it's not compiled */
+g_assert_not_reached();
 }
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 8909117d80..94194ab82d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -32,6 +32,7 @@
 #include "hw/mem/nvdimm.h"
 #include "migration/global_state.h"
 #include "migration/vmstate.h"
+#include "exec/confidential-guest-support.h"
 
 GlobalProperty hw_compat_5_2[] = {};
 const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
@@ -427,16 +428,37 @@ static char *machine_get_memory_encryption(Object *obj, 
Error **errp)
 {
 MachineState *ms = MACHINE(obj);
 
-return g_strdup(ms->memory_encryption);
+if (ms->cgs) {
+return g_strdup(object_get_canonical_path_component(OBJECT(ms->cgs)));
+}
+
+return NULL;
 }
 
 static void machine_set_memory_encryption(Object *obj, const char *value,
 Error **errp)
 {
-MachineState *ms = MACHINE(obj);
+Object *cgs =
+object_resolve_path_component(object_get_objects_root(), value);
+
+if (!cgs) {
+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, "confidential-guest-support", cgs, errp);
+}
+
+static void machine_check_confidential_guest_support(const Object *obj,
+ const char *name,
+ Object *new_target,
+ Error **errp)
+{
+/*
+ * So far the only constraint is that the target has the
+ * TYPE_CONFIDENTIAL_GUEST_SUPPORT interface, and that's checked
+ * by the QOM core
+ */
 }
 
 static bool machine_get_nvdimm(Object *obj, Error **errp)
@@ -836,6 +858,15 @@ static void machine_class_init(ObjectClass *oc, void *data)
 object_class_property_set_description(oc, "suppress-vmdesc",
 "Set on to disable self-describing migration");
 
+object_class_property_add_link(oc, "confidential-guest-support",
+   TYPE_CONFIDENTIAL_GUEST_SUPPORT,
+   offsetof(MachineState, cgs),
+   machine_check_confidential_guest_support,
+   OBJ_PROP_LINK_STRONG);
+object_class_property_set_description(oc, "confidential-guest-support",
+  "Set confidential guest scheme to 
support");
+
+/* 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",
@@ -1158,9 +1189,9 @@ void machine_run_board_init(MachineState *machine)
 cc->deprecation_note);
 }
 
-if (machine->memory_encryption) {
+if (machine->cgs) {
 /*
- * With memory encryption, the host can't see the real
+ * With confidential guests, the host can't see the real