Re: [PATCH v2 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest
65;5803;1cOn Fri, Dec 13, 2019 at 09:34:38AM +0530, Bharata B Rao wrote:
> On Thu, Dec 12, 2019 at 01:27:23PM +0100, Greg Kurz wrote:
> > > 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) {
> >
> > This ioctl can also return -EINVAL if the ultravisor actually failed to move
> > the guest back to non-secure mode or -EBUSY if a vCPU is still running. I
> > agree that the former deserve the VM to be terminated. What about the
> > latter ?
> > Can this happen and if yes, why ? Should we try again as suggested by
> > Alexey ?
> > Could this reveal a bug in QEMU, in which case we should maybe abort ?
>
> We are in machine reset path, so all vcpus are already paused. So we don't
> expect any vcpus to be running to handle -EBUSY here. Neither do I see any
> sane recovery path from here.
Right. Because this path should only happen in the case of qemu (or
kernel) error, abort() would also be appropriate. However, it's not
worth making that a separate case from the other fatal errors.
>
> As Alexey mentioned earlier, may be we can just stop the VM?
> Do vm_stop() with RUN_STATE_PAUSED or some such reason?
>
> 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
Re: [PATCH v2 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest
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
> > ---
> > 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 _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
Re: [PATCH v2 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest
On Thu, Dec 12, 2019 at 01:27:23PM +0100, Greg Kurz wrote:
> > 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) {
>
> This ioctl can also return -EINVAL if the ultravisor actually failed to move
> the guest back to non-secure mode or -EBUSY if a vCPU is still running. I
> agree that the former deserve the VM to be terminated. What about the latter ?
> Can this happen and if yes, why ? Should we try again as suggested by Alexey ?
> Could this reveal a bug in QEMU, in which case we should maybe abort ?
We are in machine reset path, so all vcpus are already paused. So we don't
expect any vcpus to be running to handle -EBUSY here. Neither do I see any
sane recovery path from here.
As Alexey mentioned earlier, may be we can just stop the VM?
Do vm_stop() with RUN_STATE_PAUSED or some such reason?
Regards,
Bharata.
Re: [PATCH v2 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest
On Thu, 12 Dec 2019 14:23:43 +0530
Bharata B Rao wrote:
> 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
> > > ---
> > > 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().
>
> Makes sense.
>
> >
> > > +error_report("Reset of secure guest failed, exiting...");
> > > +exit(EXIT_FAILURE);
> >
> > The exit() could probably go under kvmppc_svm_off() also.
>
> May be not. Then error_report would have also have to go in.
> Doesn't make sense to print this error from there.
>
Why doesn't it make sense ? It seems there's a consensus that the
failure (at least the -EINVAL case) isn't recoverable in any way.
Are there cases where we would call this and the caller could
cope with an error ?
> Regards,
> Bharata.
>
>
Re: [PATCH v2 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest
On Thu, 12 Dec 2019 11:20:59 +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. If this ioctl
> fails for a secure guest, the guest is terminated.
>
> Signed-off-by: Bharata B Rao
> ---
> 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) {
This ioctl can also return -EINVAL if the ultravisor actually failed to move
the guest back to non-secure mode or -EBUSY if a vCPU is still running. I
agree that the former deserve the VM to be terminated. What about the latter ?
Can this happen and if yes, why ? Should we try again as suggested by Alexey ?
Could this reveal a bug in QEMU, in which case we should maybe abort ?
> +error_report("Reset of secure guest failed, exiting...");
> +exit(EXIT_FAILURE);
> +}
> +
> spapr_caps_apply(spapr);
>
> first_ppc_cpu = POWERPC_CPU(first_cpu);
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 7406d18945..1a86fa4f0c 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2900,3 +2900,10 @@ void kvmppc_set_reg_tb_offset(PowerPCCPU *cpu, int64_t
> tb_offset)
> kvm_set_one_reg(cs, KVM_REG_PPC_TB_OFFSET, _offset);
> }
> }
> +
> +int kvmppc_svm_off(void)
> +{
> +KVMState *s = KVM_STATE(current_machine->accelerator);
> +
> +return kvm_vm_ioctl(s, KVM_PPC_SVM_OFF);
> +}
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 47b08a4030..5cc812e486 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -37,6 +37,7 @@ int kvmppc_booke_watchdog_enable(PowerPCCPU *cpu);
> target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
> bool radix, bool gtse,
> uint64_t proc_tbl);
> +int kvmppc_svm_off(void);
> #ifndef CONFIG_USER_ONLY
> bool kvmppc_spapr_use_multitce(void);
> int kvmppc_spapr_enable_inkernel_multitce(void);
> @@ -201,6 +202,11 @@ static inline target_ulong
> kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
> return 0;
> }
>
> +static inline int kvmppc_svm_off(void)
> +{
> +return 0;
> +}
> +
> static inline void kvmppc_set_reg_ppc_online(PowerPCCPU *cpu,
> unsigned int online)
> {
Re: [PATCH v2 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest
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
> > ---
> > 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().
Makes sense.
>
> > +error_report("Reset of secure guest failed, exiting...");
> > +exit(EXIT_FAILURE);
>
> The exit() could probably go under kvmppc_svm_off() also.
May be not. Then error_report would have also have to go in.
Doesn't make sense to print this error from there.
Regards,
Bharata.
Re: [PATCH v2 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest
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
> ---
> 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.
C.
> +}
> +
> spapr_caps_apply(spapr);
>
> first_ppc_cpu = POWERPC_CPU(first_cpu);
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 7406d18945..1a86fa4f0c 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2900,3 +2900,10 @@ void kvmppc_set_reg_tb_offset(PowerPCCPU *cpu, int64_t
> tb_offset)
> kvm_set_one_reg(cs, KVM_REG_PPC_TB_OFFSET, _offset);
> }
> }
> +
> +int kvmppc_svm_off(void)
> +{
> +KVMState *s = KVM_STATE(current_machine->accelerator);
> +
> +return kvm_vm_ioctl(s, KVM_PPC_SVM_OFF);
> +}
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 47b08a4030..5cc812e486 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -37,6 +37,7 @@ int kvmppc_booke_watchdog_enable(PowerPCCPU *cpu);
> target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
> bool radix, bool gtse,
> uint64_t proc_tbl);
> +int kvmppc_svm_off(void);
> #ifndef CONFIG_USER_ONLY
> bool kvmppc_spapr_use_multitce(void);
> int kvmppc_spapr_enable_inkernel_multitce(void);
> @@ -201,6 +202,11 @@ static inline target_ulong
> kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
> return 0;
> }
>
> +static inline int kvmppc_svm_off(void)
> +{
> +return 0;
> +}
> +
> static inline void kvmppc_set_reg_ppc_online(PowerPCCPU *cpu,
> unsigned int online)
> {
>
