On Thu, 17 Dec 2020 16:38:20 +1100 David Gibson wrote: > On Mon, Dec 14, 2020 at 06:00:36PM +0100, Cornelia Huck wrote: > > On Fri, 4 Dec 2020 16:44:10 +1100 > > David Gibson wrote: > > > > > The platform specific details of mechanisms for implementing securable > > > guest memory may require setup at various points during initialization. > > > Thus, it's not really feasible to have a single sgm 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 sgm has been initialized if > > > it was requested. > > > > > > This patch introduces a ready flag to the SecurableGuestMemory base type > > > to accomplish this, which we verify just before the machine specific > > > initialization function. > > > > > > Signed-off-by: David Gibson > > > --- > > > hw/core/machine.c | 8 ++++++++ > > > include/exec/securable-guest-memory.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 816ea3ae3e..a67a27d03c 100644 > > > --- a/hw/core/machine.c > > > +++ b/hw/core/machine.c > > > @@ -1155,6 +1155,14 @@ void machine_run_board_init(MachineState *machine) > > > } > > > > > > if (machine->sgm) { > > > + /* > > > + * Where securable guest memory 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 sgm mechanism. > > > + */ > > > + assert(machine->sgm->ready); > > > > Under which circumstances might we arrive here with 'ready' not set? > > > > - programming error, setup is happening too late -> assert() seems > > appropriate > > Yes, this is designed to catch programming errors. In particular I'm > concerned about: > * Re-arranging the init code, and either entirely forgetting the sgm > setup, or accidentally moving it too late > * The sgm setup is buried in the machine setup code, conditional on > various things, and changes mean we no longer either call it or > (correctly) fail > * User has specified an sgm scheme designed for a machine type other > than the one they selected. The arch/machine init code hasn't > correctly accounted for that possibility and ignores it, instead > of correctly throwing an error > > > - we tried to set it up, but some error happened -> should we rely on > > the setup code to error out first? (i.e. we won't end up here, unless > > there's a programming error, in which case the assert() looks > > fine) > > Yes, that's my intention. > > > Is there a possible use case for "we could not set it up, but we > > support an unsecured guest (as long as it is clear what happens)"? > > I don't think so. My feeling is that if you specify that you want the > feature, qemu needs to either give it to you, or fail, not silently > degrade the features presented to the guest. Yes, that should align with what QEMU is doing elsewhere. > > > Likely only for guests that transition themselves, but one could > > argue that QEMU should simply be invoked a second time without the > > sgm stuff being specified in the error case. > > Right - I think whatever error we give here is likely to be easier to > diagnose than the guest itself throwing an error when it fails to > transition to secure mode (plus we should catch it always, rather than > only if we run a guest which tries to go secure). Yes, that makes sense.