| Message ID | 20210113235811.1909610-8-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generalize memory encryption models | expand |
On Thu, 14 Jan 2021 10:58:05 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > The platform specific details of mechanisms for implementing > confidential guest support may require setup at various points during > initialization. Thus, it's not really feasible to have a single cgs > initialization hook, but instead each mechanism needs its own > initialization calls in arch or machine specific code. > > However, to make it harder to have a bug where a mechanism isn't > properly initialized under some circumstances, we want to have a > common place, relatively late in boot, where we verify that cgs has > been initialized if it was requested. > > This patch introduces a ready flag to the ConfidentialGuestSupport > base type to accomplish this, which we verify just before the machine > specific initialization function. > Since this is a strong requirement for any new cgs implementation, I guess it could be advertised a bit more with some extra documentation in the confidential-guest-support.h header file as well. Anyway, Reviewed-by: Greg Kurz <groug@kaod.org> Unrelated. I've just spotted mdroth@linux.vnet.ibm.com in the Cc list of this thread, but, as you know, Mike is now working on other topics at AMD :) > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/core/machine.c | 8 ++++++++ > include/exec/confidential-guest-support.h | 2 ++ > target/i386/sev.c | 2 ++ > 3 files changed, 12 insertions(+) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 94194ab82d..5a7433332b 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -1190,6 +1190,14 @@ void machine_run_board_init(MachineState *machine) > } > > if (machine->cgs) { > + /* > + * Where confidential guest support is initialized depends on > + * the specific mechanism in use. But, we need to make sure > + * it's ready by now. If it isn't, that's a bug in the > + * implementation of that cgs mechanism. > + */ > + assert(machine->cgs->ready); > + > /* > * With confidential guests, the host can't see the real > * contents of RAM, so there's no point in it trying to merge > diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h > index 5f131023ba..bcaf6c9f49 100644 > --- a/include/exec/confidential-guest-support.h > +++ b/include/exec/confidential-guest-support.h > @@ -27,6 +27,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, CONFIDENTIAL_GUEST_SUPPORT) > > struct ConfidentialGuestSupport { > Object parent; > + > + bool ready; > }; > > typedef struct ConfidentialGuestSupportClass { > diff --git a/target/i386/sev.c b/target/i386/sev.c > index e2b41ef342..3d94635397 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -737,6 +737,8 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > qemu_add_machine_init_done_notifier(&sev_machine_done_notify); > qemu_add_vm_change_state_handler(sev_vm_state_change, sev); > > + cgs->ready = true; > + > return 0; > err: > sev_guest = NULL;
On Thu, 14 Jan 2021 10:58:05 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > The platform specific details of mechanisms for implementing > confidential guest support may require setup at various points during > initialization. Thus, it's not really feasible to have a single cgs > initialization hook, but instead each mechanism needs its own > initialization calls in arch or machine specific code. > > However, to make it harder to have a bug where a mechanism isn't > properly initialized under some circumstances, we want to have a > common place, relatively late in boot, where we verify that cgs has > been initialized if it was requested. > > This patch introduces a ready flag to the ConfidentialGuestSupport > base type to accomplish this, which we verify just before the machine > specific initialization function. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/core/machine.c | 8 ++++++++ > include/exec/confidential-guest-support.h | 2 ++ > target/i386/sev.c | 2 ++ > 3 files changed, 12 insertions(+) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
* David Gibson (david@gibson.dropbear.id.au) wrote: > The platform specific details of mechanisms for implementing > confidential guest support may require setup at various points during > initialization. Thus, it's not really feasible to have a single cgs > initialization hook, but instead each mechanism needs its own > initialization calls in arch or machine specific code. > > However, to make it harder to have a bug where a mechanism isn't > properly initialized under some circumstances, we want to have a > common place, relatively late in boot, where we verify that cgs has > been initialized if it was requested. > > This patch introduces a ready flag to the ConfidentialGuestSupport > base type to accomplish this, which we verify just before the machine > specific initialization function. You may find you need to define 'ready' and the answer might be a bit variable; for example, on SEV there's a setup bit and then you may end up doing an attestation and receiving some data before you actaully let the guest execute code. Is it ready before it's received the attestation response or only when it can run code? Is a Power or 390 machine 'ready' before it's executed the magic instruction to enter the confidential mode? Dave > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/core/machine.c | 8 ++++++++ > include/exec/confidential-guest-support.h | 2 ++ > target/i386/sev.c | 2 ++ > 3 files changed, 12 insertions(+) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 94194ab82d..5a7433332b 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -1190,6 +1190,14 @@ void machine_run_board_init(MachineState *machine) > } > > if (machine->cgs) { > + /* > + * Where confidential guest support is initialized depends on > + * the specific mechanism in use. But, we need to make sure > + * it's ready by now. If it isn't, that's a bug in the > + * implementation of that cgs mechanism. > + */ > + assert(machine->cgs->ready); > + > /* > * With confidential guests, the host can't see the real > * contents of RAM, so there's no point in it trying to merge > diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h > index 5f131023ba..bcaf6c9f49 100644 > --- a/include/exec/confidential-guest-support.h > +++ b/include/exec/confidential-guest-support.h > @@ -27,6 +27,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, CONFIDENTIAL_GUEST_SUPPORT) > > struct ConfidentialGuestSupport { > Object parent; > + > + bool ready; > }; > > typedef struct ConfidentialGuestSupportClass { > diff --git a/target/i386/sev.c b/target/i386/sev.c > index e2b41ef342..3d94635397 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -737,6 +737,8 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > qemu_add_machine_init_done_notifier(&sev_machine_done_notify); > qemu_add_vm_change_state_handler(sev_vm_state_change, sev); > > + cgs->ready = true; > + > return 0; > err: > sev_guest = NULL; > -- > 2.29.2 >
On Mon, 18 Jan 2021 19:47:30 +0000 "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > The platform specific details of mechanisms for implementing > > confidential guest support may require setup at various points during > > initialization. Thus, it's not really feasible to have a single cgs > > initialization hook, but instead each mechanism needs its own > > initialization calls in arch or machine specific code. > > > > However, to make it harder to have a bug where a mechanism isn't > > properly initialized under some circumstances, we want to have a > > common place, relatively late in boot, where we verify that cgs has > > been initialized if it was requested. > > > > This patch introduces a ready flag to the ConfidentialGuestSupport > > base type to accomplish this, which we verify just before the machine > > specific initialization function. > > You may find you need to define 'ready' and the answer might be a bit > variable; for example, on SEV there's a setup bit and then you may end > up doing an attestation and receiving some data before you actaully let > the guest execute code. Is it ready before it's received the > attestation response or only when it can run code? > Is a Power or 390 machine 'ready' before it's executed the magic > instruction to enter the confidential mode? I would consider those machines where the guest makes the transition itself "ready" as soon as everything is set up so that the guest can actually initiate the transition. Otherwise, those machines would never be "ready" if the guest does not transition. Maybe we can define "ready" as "the guest can start to execute in secure mode", where "guest" includes the bootloader and everything that runs in a guest context, and "start to execute" implies that some setup may be done only after the guest has kicked it off?
On Tue, Jan 19, 2021 at 09:16:08AM +0100, Cornelia Huck wrote: > On Mon, 18 Jan 2021 19:47:30 +0000 > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > The platform specific details of mechanisms for implementing > > > confidential guest support may require setup at various points during > > > initialization. Thus, it's not really feasible to have a single cgs > > > initialization hook, but instead each mechanism needs its own > > > initialization calls in arch or machine specific code. > > > > > > However, to make it harder to have a bug where a mechanism isn't > > > properly initialized under some circumstances, we want to have a > > > common place, relatively late in boot, where we verify that cgs has > > > been initialized if it was requested. > > > > > > This patch introduces a ready flag to the ConfidentialGuestSupport > > > base type to accomplish this, which we verify just before the machine > > > specific initialization function. > > > > You may find you need to define 'ready' and the answer might be a bit > > variable; for example, on SEV there's a setup bit and then you may end > > up doing an attestation and receiving some data before you actaully let > > the guest execute code. Is it ready before it's received the > > attestation response or only when it can run code? > > Is a Power or 390 machine 'ready' before it's executed the magic > > instruction to enter the confidential mode? > > I would consider those machines where the guest makes the transition > itself "ready" as soon as everything is set up so that the guest can > actually initiate the transition. Otherwise, those machines would never > be "ready" if the guest does not transition. > > Maybe we can define "ready" as "the guest can start to execute in > secure mode", where "guest" includes the bootloader and everything that > runs in a guest context, and "start to execute" implies that some setup > may be done only after the guest has kicked it off? That was pretty much my intention. I've put a big comment on the field definition and tweaked things around a bit in the hopes of making that clearer for the next spin.
diff --git a/hw/core/machine.c b/hw/core/machine.c index 94194ab82d..5a7433332b 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -1190,6 +1190,14 @@ void machine_run_board_init(MachineState *machine) } if (machine->cgs) { + /* + * Where confidential guest support is initialized depends on + * the specific mechanism in use. But, we need to make sure + * it's ready by now. If it isn't, that's a bug in the + * implementation of that cgs mechanism. + */ + assert(machine->cgs->ready); + /* * With confidential guests, the host can't see the real * contents of RAM, so there's no point in it trying to merge diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h index 5f131023ba..bcaf6c9f49 100644 --- a/include/exec/confidential-guest-support.h +++ b/include/exec/confidential-guest-support.h @@ -27,6 +27,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, CONFIDENTIAL_GUEST_SUPPORT) struct ConfidentialGuestSupport { Object parent; + + bool ready; }; typedef struct ConfidentialGuestSupportClass { diff --git a/target/i386/sev.c b/target/i386/sev.c index e2b41ef342..3d94635397 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -737,6 +737,8 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) qemu_add_machine_init_done_notifier(&sev_machine_done_notify); qemu_add_vm_change_state_handler(sev_vm_state_change, sev); + cgs->ready = true; + return 0; err: sev_guest = NULL;
The platform specific details of mechanisms for implementing confidential guest support may require setup at various points during initialization. Thus, it's not really feasible to have a single cgs initialization hook, but instead each mechanism needs its own initialization calls in arch or machine specific code. However, to make it harder to have a bug where a mechanism isn't properly initialized under some circumstances, we want to have a common place, relatively late in boot, where we verify that cgs has been initialized if it was requested. This patch introduces a ready flag to the ConfidentialGuestSupport base type to accomplish this, which we verify just before the machine specific initialization function. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/core/machine.c | 8 ++++++++ include/exec/confidential-guest-support.h | 2 ++ target/i386/sev.c | 2 ++ 3 files changed, 12 insertions(+)