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 <bharata@linux.ibm.com>
---
 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();
     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)
 {
-- 
2.21.0

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 <bharata@linux.ibm.com>
> ---
>  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.

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

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

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 <bharata@linux.ibm.com>
> > ---
> >  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?

Regards,
Bharata.

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 <bharata@linux.ibm.com>
>>> ---
>>>  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?).


-- 
Alexey

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 <bharata@linux.ibm.com>
> >>> ---
> >>>  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.

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

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 <bharata@linux.ibm.com>
> > >>> ---
> > >>>  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.

Regards,
Bharata.

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 <bharata@linux.ibm.com>
> > > >>> ---
> > > >>>  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).

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

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 <bharata@linux.ibm.com>
> > > > >>> ---
> > > > >>>  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.

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

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.

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 <bharata@linux.ibm.com>
> > > > > >>> ---
> > > > > >>>  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

On Wed, Dec 11, 2019 at 04:27:42PM +1100, David Gibson wrote:
> 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.

I am posting v2 with explicit check for -ENOTTY.

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

If and when we decide to have this KVM capability and that goes upstream,
we can update the QEMU accordingly?

Regards,
Bharata.