Re: [PATCH v2 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest

[David Gibson]

On Thu, Dec 12, 2019 at 08:34:57AM +0100, Cédric Le Goater wrote:
> Hello Bharata,
> 
> 
> On 12/12/2019 06:50, Bharata B Rao wrote:
> > A pseries guest can be run as a secure guest on Ultravisor-enabled
> > POWER platforms. When such a secure guest is reset, we need to
> > release/reset a few resources both on ultravisor and hypervisor side.
> > This is achieved by invoking this new ioctl KVM_PPC_SVM_OFF from the
> > machine reset path.
> > 
> > As part of this ioctl, the secure guest is essentially transitioned
> > back to normal mode so that it can reboot like a regular guest and
> > become secure again.
> > 
> > This ioctl has no effect when invoked for a normal guest. If this ioctl
> > fails for a secure guest, the guest is terminated.
> 
> This looks OK. 
> 
> > Signed-off-by: Bharata B Rao <address@hidden>
> > ---
> >  hw/ppc/spapr.c       | 15 +++++++++++++++
> >  target/ppc/kvm.c     |  7 +++++++
> >  target/ppc/kvm_ppc.h |  6 ++++++
> >  3 files changed, 28 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index f11422fc41..25e1a3446e 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1597,6 +1597,21 @@ static void spapr_machine_reset(MachineState 
> > *machine)
> >      void *fdt;
> >      int rc;
> >  
> > +    /*
> > +     * KVM_PPC_SVM_OFF ioctl can fail for secure guests, check and
> > +     * exit in that case. However check for -ENOTTY explicitly
> > +     * to ensure that we don't terminate normal guests that are
> > +     * running on kernels which don't support this ioctl.
> > +     *
> > +     * Also, this ioctl returns 0 for normal guests on kernels where
> > +     * this ioctl is supported.
> > +     */
> > +    rc = kvmppc_svm_off();
> > +    if (rc && rc != -ENOTTY) {
> 
> I would put these low level tests under kvmppc_svm_off().
> 
> > +        error_report("Reset of secure guest failed, exiting...");
> > +        exit(EXIT_FAILURE);
> 
> The exit() could probably go under kvmppc_svm_off() also.

TBH, I don't think these details matter all that much.

But if I had to pick a preferred option here it would be:

int kvmppc_svm_off(Error **errp)

Which would set the errp with error_setg_errno() except in the case of
ENOTTY.  spapr_machine_reset() would call it with &error_fatal.  That
puts the analysis of whether the error is expected into
kvmppc_svm_off() - which is best equipped to know that, but the choice
of what to do about it (fail fatally) in the reset caller.

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

signature.asc
Description: PGP signature