kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: Greg Kurz <groug@kaod.org>
Cc: pasic@linux.ibm.com, brijesh.singh@amd.com, pair@us.ibm.com,
	dgilbert@redhat.com, qemu-devel@nongnu.org, andi.kleen@intel.com,
	qemu-ppc@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com>,
	"Marcelo Tosatti" <mtosatti@redhat.com>,
	frankja@linux.ibm.com, thuth@redhat.com,
	"Christian Borntraeger" <borntraeger@de.ibm.com>,
	mdroth@linux.vnet.ibm.com, richard.henderson@linaro.org,
	kvm@vger.kernel.org, "Daniel P. Berrangé" <berrange@redhat.com>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	david@redhat.com, "Cornelia Huck" <cohuck@redhat.com>,
	mst@redhat.com, qemu-s390x@nongnu.org,
	pragyansri.pathi@intel.com, jun.nakajima@intel.com
Subject: Re: [PATCH v6 05/13] confidential guest support: Rework the "memory-encryption" property
Date: Wed, 13 Jan 2021 11:50:32 +1100	[thread overview]
Message-ID: <20210113005032.GA435587@yekko.fritz.box> (raw)
In-Reply-To: <20210112115959.2c042dbb@bahia.lan>

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

On Tue, Jan 12, 2021 at 11:59:59AM +0100, Greg Kurz wrote:
> On Tue, 12 Jan 2021 15:45:00 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org>
> 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  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
> >           * contents of RAM, so there's no point in it trying to merge
> >           * areas.
> >           */
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 17b1f3f0b9..1acd662fa5 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -270,7 +270,7 @@ struct MachineState {
> >      bool iommu;
> >      bool suppress_vmdesc;
> >      bool enable_graphics;
> > -    char *memory_encryption;
> > +    ConfidentialGuestSupport *cgs;
> >      char *ram_memdev_id;
> >      /*
> >       * convenience alias to ram_memdev_id backend memory region
> > diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
> > index 7335e59867..3b5b1aacf1 100644
> > --- a/include/sysemu/sev.h
> > +++ b/include/sysemu/sev.h
> > @@ -16,7 +16,7 @@
> >  
> >  #include "sysemu/kvm.h"
> >  
> > -int sev_guest_init(const char *id);
> > +int sev_kvm_init(ConfidentialGuestSupport *cgs);
> >  int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
> >  int sev_inject_launch_secret(const char *hdr, const char *secret,
> >                               uint64_t gpa, Error **errp);
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 2a4b2187d6..5399a136ad 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -335,26 +335,6 @@ static const TypeInfo sev_guest_info = {
> >      }
> >  };
> >  
> > -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)
> >  {
> > @@ -682,10 +662,9 @@ sev_vm_state_change(void *opaque, int running, RunState state)
> >      }
> >  }
> >  
> > -int
> > -sev_guest_init(const char *id)
> > +int sev_kvm_init(ConfidentialGuestSupport *cgs)
> >  {
> > -    SevGuestState *sev;
> > +    SevGuestState *sev = SEV_GUEST(cgs);
> >      char *devname;
> >      int ret, fw_error;
> >      uint32_t ebx;
> > @@ -698,13 +677,6 @@ sev_guest_init(const char *id)
> >          return -1;
> >      }
> >  
> > -    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;
> >  
> 

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

  parent reply	other threads:[~2021-01-13  1:02 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-12  4:44 [PATCH v6 00/13] Generalize memory encryption models David Gibson
2021-01-12  4:44 ` [PATCH v6 01/13] qom: Allow optional sugar props David Gibson
2021-01-12  4:44 ` [PATCH v6 02/13] confidential guest support: Introduce new confidential guest support class David Gibson
2021-01-12  9:46   ` Daniel P. Berrangé
2021-01-13  2:09     ` David Gibson
2021-01-12  4:44 ` [PATCH v6 03/13] sev: Remove false abstraction of flash encryption David Gibson
2021-01-12  4:44 ` [PATCH v6 04/13] confidential guest support: Move side effect out of machine_set_memory_encryption() David Gibson
2021-01-12  4:45 ` [PATCH v6 05/13] confidential guest support: Rework the "memory-encryption" property David Gibson
     [not found]   ` <20210112115959.2c042dbb@bahia.lan>
2021-01-13  0:50     ` David Gibson [this message]
2021-01-13 12:03       ` Dr. David Alan Gilbert
2021-01-12  4:45 ` [PATCH v6 06/13] sev: Add Error ** to sev_kvm_init() David Gibson
2021-01-12  4:45 ` [PATCH v6 07/13] confidential guest support: Introduce cgs "ready" flag David Gibson
2021-01-12  4:45 ` [PATCH v6 08/13] confidential guest support: Move SEV initialization into arch specific code David Gibson
2021-01-12  4:45 ` [PATCH v6 09/13] confidential guest support: Update documentation David Gibson
2021-01-12  4:45 ` [PATCH v6 10/13] spapr: Add PEF based confidential guest support David Gibson
2021-01-12  7:56   ` Christian Borntraeger
2021-01-12  8:36     ` David Gibson
2021-01-12  9:52   ` Daniel P. Berrangé
2021-01-12  9:56   ` Daniel P. Berrangé
2021-01-13  0:52     ` David Gibson
     [not found]   ` <20210112122750.5dcd995c@bahia.lan>
2021-01-13  0:56     ` David Gibson
2021-01-12  4:45 ` [PATCH v6 11/13] spapr: PEF: prevent migration David Gibson
2021-01-12  4:45 ` [PATCH v6 12/13] confidential guest support: Alter virtio default properties for protected guests David Gibson
2021-01-12  4:45 ` [PATCH v6 13/13] s390: Recognize confidential-guest-support option David Gibson
2021-01-12  8:15   ` Christian Borntraeger
2021-01-12 11:36     ` Cornelia Huck
2021-01-12 11:48       ` Christian Borntraeger
2021-01-12 11:49       ` Daniel P. Berrangé
2021-01-13  0:57       ` David Gibson
2021-01-13  6:57         ` Christian Borntraeger
2021-01-13 23:56           ` David Gibson
2021-01-12  9:54   ` Daniel P. Berrangé

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210113005032.GA435587@yekko.fritz.box \
    --to=david@gibson.dropbear.id.au \
    --cc=andi.kleen@intel.com \
    --cc=berrange@redhat.com \
    --cc=borntraeger@de.ibm.com \
    --cc=brijesh.singh@amd.com \
    --cc=cohuck@redhat.com \
    --cc=david@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=frankja@linux.ibm.com \
    --cc=groug@kaod.org \
    --cc=jun.nakajima@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=marcel.apfelbaum@gmail.com \
    --cc=mdroth@linux.vnet.ibm.com \
    --cc=mst@redhat.com \
    --cc=mtosatti@redhat.com \
    --cc=pair@us.ibm.com \
    --cc=pasic@linux.ibm.com \
    --cc=pbonzini@redhat.com \
    --cc=pragyansri.pathi@intel.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    --cc=qemu-s390x@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --cc=thuth@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).