| Message ID | 20191212055059.9399-3-bharata@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | ppc/spapr: Support reboot of secure pseries guest | expand |
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 <bharata@linux.ibm.com> > --- > 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, &tb_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) > { >
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 <bharata@linux.ibm.com> > > --- > > 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.
On Thu, 12 Dec 2019 11:20:59 +0530 Bharata B Rao <bharata@linux.ibm.com> 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 <bharata@linux.ibm.com> > --- > 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, &tb_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) > {
On Thu, 12 Dec 2019 14:23:43 +0530 Bharata B Rao <bharata@linux.ibm.com> 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 <bharata@linux.ibm.com> > > > --- > > > 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. > >
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.
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 <bharata@linux.ibm.com> > > --- > > 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.
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. >
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) { + 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, &tb_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) {
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 <bharata@linux.ibm.com> --- hw/ppc/spapr.c | 15 +++++++++++++++ target/ppc/kvm.c | 7 +++++++ target/ppc/kvm_ppc.h | 6 ++++++ 3 files changed, 28 insertions(+)