On Thu, 17 Dec 2020 15:15:30 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Thu, 17 Dec 2020 12:38:42 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Thu, 17 Dec 2020 16:47:36 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Mon, Dec 14, 2020 at 06:22:40PM +0100, Cornelia Huck wrote:  
> > > > On Fri,  4 Dec 2020 16:44:13 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > We haven't yet implemented the fairly involved handshaking that will be
> > > > > needed to migrate PEF protected guests.  For now, just use a migration
> > > > > blocker so we get a meaningful error if someone attempts this (this is the
> > > > > same approach used by AMD SEV).
> > > > > 
> > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > ---
> > > > >  hw/ppc/pef.c | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/hw/ppc/pef.c b/hw/ppc/pef.c
> > > > > index 3ae3059cfe..edc3e744ba 100644
> > > > > --- a/hw/ppc/pef.c
> > > > > +++ b/hw/ppc/pef.c
> > > > > @@ -38,7 +38,11 @@ struct PefGuestState {
> > > > >  };
> > > > >  
> > > > >  #ifdef CONFIG_KVM
> > > > > +static Error *pef_mig_blocker;
> > > > > +
> > > > >  static int kvmppc_svm_init(Error **errp)    
> > > > 
> > > > This looks weird?    
> > > 
> > > Oops.  Not sure how that made it past even my rudimentary compile
> > > testing.
> > >   
> > > > > +
> > > > > +int kvmppc_svm_init(SecurableGuestMemory *sgm, Error **errp)
> > > > >  {
> > > > >      if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_SECURABLE_GUEST)) {
> > > > >          error_setg(errp,
> > > > > @@ -54,6 +58,11 @@ static int kvmppc_svm_init(Error **errp)
> > > > >          }
> > > > >      }
> > > > >  
> > > > > +    /* add migration blocker */
> > > > > +    error_setg(&pef_mig_blocker, "PEF: Migration is not implemented");
> > > > > +    /* NB: This can fail if --only-migratable is used */
> > > > > +    migrate_add_blocker(pef_mig_blocker, &error_fatal);    
> > > > 
> > > > Just so that I understand: is PEF something that is enabled by the host
> > > > (and the guest is either secured or doesn't start), or is it using a
> > > > model like s390x PV where the guest initiates the transition into
> > > > secured mode?    
> > > 
> > > Like s390x PV it's initiated by the guest.
> > >   
> > > > Asking because s390x adds the migration blocker only when the
> > > > transition is actually happening (i.e. guests that do not transition
> > > > into secure mode remain migratable.) This has the side effect that you
> > > > might be able to start a machine with --only-migratable that
> > > > transitions into a non-migratable machine via a guest action, if I'm
> > > > not mistaken. Without the new object, I don't see a way to block with
> > > > --only-migratable; with it, we should be able to do that. Not sure what
> > > > the desirable behaviour is here.    
> > >   
> 
> The purpose of --only-migratable is specifically to prevent the machine
> to transition to a non-migrate state IIUC. The guest transition to
> secure mode should be nacked in this case.

Yes, that's what happens for s390x: The guest tries to transition, QEMU
can't add a migration blocker and fails the instruction used for
transitioning, the guest sees the error.

The drawback is that we see the failure only when we already launched
the machine and the guest tries to transition. If I start QEMU with
--only-migratable, it will refuse to start when non-migratable devices
are configured in the command line, so I see the issue right from the
start. (For s390x, that would possibly mean that we should not even
present the cpu feature bit when only_migratable is set?)

> 
> > > Hm, I'm not sure what the best option is here either.  
> > 
> > If we agree on anything, it should be as consistent across
> > architectures as possible :)
> > 
> > If we want to add the migration blocker to s390x even before the guest
> > transitions, it needs to be tied to the new object; if we'd make it
> > dependent on the cpu feature bit, we'd block migration of all machines
> > on hardware with SE and a recent kernel.
> > 
> > Is there a convenient point in time when PEF guests transition where
> > QEMU can add a blocker?
> >   
> > >   
> > > >     
> > > > > +
> > > > >      return 0;
> > > > >  }
> > > > >      
> > > >     
> > >   
> >   
>