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

[David Gibson]

On Wed, Dec 11, 2019 at 10:38:24AM +0530, Bharata B Rao wrote:
> On Wed, Dec 11, 2019 at 10:41:32AM +1100, David Gibson wrote:
> > On Tue, Dec 10, 2019 at 12:20:07PM +0530, Bharata B Rao wrote:
> > > On Tue, Dec 10, 2019 at 04:05:36PM +1100, David Gibson wrote:
> > > > On Tue, Dec 10, 2019 at 03:03:01PM +1100, Alexey Kardashevskiy wrote:
> > > > > 
> > > > > 
> > > > > On 10/12/2019 14:50, Bharata B Rao wrote:
> > > > > > On Tue, Dec 10, 2019 at 02:28:51PM +1100, David Gibson wrote:
> > > > > >> On Mon, Dec 09, 2019 at 12:30:12PM +0530, 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.
> > > > > >>>
> > > > > >>> Signed-off-by: Bharata B Rao <address@hidden>
> > > > > >>> ---
> > > > > >>>  hw/ppc/spapr.c       | 1 +
> > > > > >>>  target/ppc/kvm.c     | 7 +++++++
> > > > > >>>  target/ppc/kvm_ppc.h | 6 ++++++
> > > > > >>>  3 files changed, 14 insertions(+)
> > > > > >>>
> > > > > >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > >>> index f11422fc41..4c7ad3400d 100644
> > > > > >>> --- a/hw/ppc/spapr.c
> > > > > >>> +++ b/hw/ppc/spapr.c
> > > > > >>> @@ -1597,6 +1597,7 @@ static void 
> > > > > >>> spapr_machine_reset(MachineState *machine)
> > > > > >>>      void *fdt;
> > > > > >>>      int rc;
> > > > > >>>  
> > > > > >>> +    kvmppc_svm_off();
> > > > > >>
> > > > > >> If you're going to have this return an error value, you should 
> > > > > >> really
> > > > > >> check it here.
> > > > > > 
> > > > > > I could, by spapr_machine_reset() and the callers don't propagate 
> > > > > > the
> > > > > > errors up. So may be I could print a warning instead when ioctl 
> > > > > > fails?
> > > > > 
> > > > > An error here means you cannot restart the machine and should probably
> > > > > suspend, or try until it is not EBUSY (==all threads have stopped?).
> > > > 
> > > > Right, if this fails, something has gone badly wrong.  You should
> > > > absolutely print a message, and in fact it might be appropriate to
> > > > quit outright.  IIUC the way PEF resets work, a failure here means you
> > > > won't be able to boot after the reset, since the guest memory will
> > > > still be inaccessible to the host.
> > > 
> > > Correct. I will send next version with a message and abort() added in
> > > the ioctl failure path.
> > 
> > abort() or assert() isn't right either - that's reserved for things
> > that are definitely caused by a qemu code bug.  This should be an
> > exit(EXIT_FAILURE).
> 
> Ok, but I see a problem with checking the return value of this
> ioctl from userspace. If this ioctl is run on older kernels that don't
> support this ioctl, we get -ENOTTY as return value. We shouldn't be
> exiting in that case.

Ah, right.  We'll need to check for -ENOTTY specifically and ignore
it, then.  We don't want this spewing warnings on every non-secure
guest.

> It looks like we may need a new KVM capability to advertise the presence
> of KVM_PPC_SVM_OFF ioctl (or more generally, to advertise host kernel's
> capability to support secure guests).

Actually, that's probably a better idea still.

> Paul - Do you think we should add such a KVM capability? Here is the
> summary of the problem:
> 
> 1. QEMU invokes KVM_PPC_SVM_OFF ioctl from machine reset path and currently
>    we don't check for its return value.
> 2. On host kernels that support secure guests,
>    2a. this ioctl returns 0 for regular guests and has no effect.
>    2b. the ioctl can fail for secure guests and here we could check the
>        return value and exit the guest right away.
> 3. On host kernels that don't support secure guests, ioctl returns -ENOTTY
>    but we ignore the return value and continue the reset as this is
>    for a non-secure guest.
> 
> If we have such a KVM capability, we could invoke the ioctl only if it
> is supported and handle the return value appropriately.
> 
> Regards,
> Bharata.
> 

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