On Thu, 14 Jan 2021 10:58:05 +1100 David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxx> Unrelated. I've just spotted mdroth@xxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxxxxxxxx> > --- > 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;