ppc: fix ppc_set_compat() with KVM PR

[Qemu-devel] [PATCH] ppc: fix ppc_set_compat() with KVM PR

Posted by Greg Kurz 6 years, 8 months ago

When running in KVM PR mode, kvmppc_set_compat() always fail because the
current PR implementation doesn't handle KVM_REG_PPC_ARCH_COMPAT. Now that
the machine code inconditionally calls ppc_set_compat_all() at reset time
to restore the compat mode default value (commit 66d5c492dd3a9), it is
impossible to start a guest with PR:

qemu-system-ppc64: Unable to set CPU compatibility mode in KVM:
 Invalid argument

A tentative patch [1] was recently sent by Suraj to address the issue, but
it would prevent the compat mode to be turned off on reset. And we really
don't want to explicitely check for KVM PR. During the patch's review,
David suggested that we should only call the KVM ioctl() if the compat
PVR changes. This allows at least to run with KVM PR, provided no compat
mode is requested from the command line (which should be the case when
running PR nested). This is what this patch does.

While here, we also fix the side effect where KVM would fail but we would
change the CPU state in QEMU anyway.

[1] http://patchwork.ozlabs.org/patch/782039/

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 target/ppc/compat.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/ppc/compat.c b/target/ppc/compat.c
index f1b67faa97e3..f8729fe46d61 100644
--- a/target/ppc/compat.c
+++ b/target/ppc/compat.c
@@ -140,16 +140,17 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
 
     cpu_synchronize_state(CPU(cpu));
 
-    cpu->compat_pvr = compat_pvr;
-    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
-
-    if (kvm_enabled()) {
+    if (kvm_enabled() && cpu->compat_pvr != compat_pvr) {
         int ret = kvmppc_set_compat(cpu, cpu->compat_pvr);
         if (ret < 0) {
             error_setg_errno(errp, -ret,
                              "Unable to set CPU compatibility mode in KVM");
+            return;
         }
     }
+
+    cpu->compat_pvr = compat_pvr;
+    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
 }
 
 typedef struct {

Re: [Qemu-devel] [PATCH] ppc: fix ppc_set_compat() with KVM PR

Posted by Suraj Jitindar Singh 6 years, 8 months ago

On Mon, 2017-08-14 at 19:49 +0200, Greg Kurz wrote:
> When running in KVM PR mode, kvmppc_set_compat() always fail because
> the
> current PR implementation doesn't handle KVM_REG_PPC_ARCH_COMPAT. Now
> that
> the machine code inconditionally calls ppc_set_compat_all() at reset
> time
> to restore the compat mode default value (commit 66d5c492dd3a9), it
> is
> impossible to start a guest with PR:
> 
> qemu-system-ppc64: Unable to set CPU compatibility mode in KVM:
>  Invalid argument
> 
> A tentative patch [1] was recently sent by Suraj to address the
> issue, but
> it would prevent the compat mode to be turned off on reset. And we
> really
> don't want to explicitely check for KVM PR. During the patch's
> review,
> David suggested that we should only call the KVM ioctl() if the
> compat
> PVR changes. This allows at least to run with KVM PR, provided no
> compat
> mode is requested from the command line (which should be the case
> when
> running PR nested). This is what this patch does.
> 
> While here, we also fix the side effect where KVM would fail but we
> would
> change the CPU state in QEMU anyway.
> 
> [1] http://patchwork.ozlabs.org/patch/782039/
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

I meant to send a follow up but it fell on my priority list so thanks
for this

Looks good to me

Reviewed-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

> ---
>  target/ppc/compat.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/compat.c b/target/ppc/compat.c
> index f1b67faa97e3..f8729fe46d61 100644
> --- a/target/ppc/compat.c
> +++ b/target/ppc/compat.c
> @@ -140,16 +140,17 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t
> compat_pvr, Error **errp)
>  
>      cpu_synchronize_state(CPU(cpu));
>  
> -    cpu->compat_pvr = compat_pvr;
> -    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> -
> -    if (kvm_enabled()) {
> +    if (kvm_enabled() && cpu->compat_pvr != compat_pvr) {
>          int ret = kvmppc_set_compat(cpu, cpu->compat_pvr);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret,
>                               "Unable to set CPU compatibility mode
> in KVM");
> +            return;
>          }
>      }
> +
> +    cpu->compat_pvr = compat_pvr;
> +    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
>  }
>  
>  typedef struct {
>

Re: [Qemu-devel] [PATCH] ppc: fix ppc_set_compat() with KVM PR

Posted by Greg Kurz 6 years, 8 months ago

On Tue, 15 Aug 2017 11:40:34 +1000
Suraj Jitindar Singh <sjitindarsingh@gmail.com> wrote:

> On Mon, 2017-08-14 at 19:49 +0200, Greg Kurz wrote:
> > When running in KVM PR mode, kvmppc_set_compat() always fail because
> > the
> > current PR implementation doesn't handle KVM_REG_PPC_ARCH_COMPAT. Now
> > that
> > the machine code inconditionally calls ppc_set_compat_all() at reset
> > time
> > to restore the compat mode default value (commit 66d5c492dd3a9), it
> > is
> > impossible to start a guest with PR:
> > 
> > qemu-system-ppc64: Unable to set CPU compatibility mode in KVM:
> >  Invalid argument
> > 
> > A tentative patch [1] was recently sent by Suraj to address the
> > issue, but
> > it would prevent the compat mode to be turned off on reset. And we
> > really
> > don't want to explicitely check for KVM PR. During the patch's
> > review,
> > David suggested that we should only call the KVM ioctl() if the
> > compat
> > PVR changes. This allows at least to run with KVM PR, provided no
> > compat
> > mode is requested from the command line (which should be the case
> > when
> > running PR nested). This is what this patch does.
> > 
> > While here, we also fix the side effect where KVM would fail but we
> > would
> > change the CPU state in QEMU anyway.
> > 
> > [1] http://patchwork.ozlabs.org/patch/782039/
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> I meant to send a follow up but it fell on my priority list so thanks
> for this
> 

No problem :)

This is more a workaround than an actual fix since CAS will still want to
change the compat mode and fail, but at least QEMU doesn't exit right away.
I needed that to chase down another bug that breaks loadvm of a KVM PR guest.

> Looks good to me
> 
> Reviewed-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> 

Thanks!

> > ---
> >  target/ppc/compat.c |    9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target/ppc/compat.c b/target/ppc/compat.c
> > index f1b67faa97e3..f8729fe46d61 100644
> > --- a/target/ppc/compat.c
> > +++ b/target/ppc/compat.c
> > @@ -140,16 +140,17 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t
> > compat_pvr, Error **errp)
> >  
> >      cpu_synchronize_state(CPU(cpu));
> >  
> > -    cpu->compat_pvr = compat_pvr;
> > -    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> > -
> > -    if (kvm_enabled()) {
> > +    if (kvm_enabled() && cpu->compat_pvr != compat_pvr) {
> >          int ret = kvmppc_set_compat(cpu, cpu->compat_pvr);
> >          if (ret < 0) {
> >              error_setg_errno(errp, -ret,
> >                               "Unable to set CPU compatibility mode
> > in KVM");
> > +            return;
> >          }
> >      }
> > +
> > +    cpu->compat_pvr = compat_pvr;
> > +    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> >  }
> >  
> >  typedef struct {
> >

Re: [Qemu-devel] [PATCH] ppc: fix ppc_set_compat() with KVM PR

Posted by David Gibson 6 years, 8 months ago

On Mon, Aug 14, 2017 at 07:49:16PM +0200, Greg Kurz wrote:
> When running in KVM PR mode, kvmppc_set_compat() always fail because the
> current PR implementation doesn't handle KVM_REG_PPC_ARCH_COMPAT. Now that
> the machine code inconditionally calls ppc_set_compat_all() at reset time
> to restore the compat mode default value (commit 66d5c492dd3a9), it is
> impossible to start a guest with PR:
> 
> qemu-system-ppc64: Unable to set CPU compatibility mode in KVM:
>  Invalid argument
> 
> A tentative patch [1] was recently sent by Suraj to address the issue, but
> it would prevent the compat mode to be turned off on reset. And we really
> don't want to explicitely check for KVM PR. During the patch's review,
> David suggested that we should only call the KVM ioctl() if the compat
> PVR changes. This allows at least to run with KVM PR, provided no compat
> mode is requested from the command line (which should be the case when
> running PR nested). This is what this patch does.

I'm not sure that's true any more, since we now prefer compat modes
during CAS.  I guess we could exclude the compat modes from the
advertised list based on what KVM supports, but that seems pretty
awkward.

> While here, we also fix the side effect where KVM would fail but we would
> change the CPU state in QEMU anyway.
> 
> [1] http://patchwork.ozlabs.org/patch/782039/
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

In any case, this makes things better than they were, so I'm applying.

> ---
>  target/ppc/compat.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/compat.c b/target/ppc/compat.c
> index f1b67faa97e3..f8729fe46d61 100644
> --- a/target/ppc/compat.c
> +++ b/target/ppc/compat.c
> @@ -140,16 +140,17 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
>  
>      cpu_synchronize_state(CPU(cpu));
>  
> -    cpu->compat_pvr = compat_pvr;
> -    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> -
> -    if (kvm_enabled()) {
> +    if (kvm_enabled() && cpu->compat_pvr != compat_pvr) {
>          int ret = kvmppc_set_compat(cpu, cpu->compat_pvr);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret,
>                               "Unable to set CPU compatibility mode in KVM");
> +            return;
>          }
>      }
> +
> +    cpu->compat_pvr = compat_pvr;
> +    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
>  }
>  
>  typedef struct {
> 

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

Re: [Qemu-devel] [PATCH] ppc: fix ppc_set_compat() with KVM PR

Posted by Greg Kurz 6 years, 8 months ago

On Tue, 15 Aug 2017 14:04:09 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Aug 14, 2017 at 07:49:16PM +0200, Greg Kurz wrote:
> > When running in KVM PR mode, kvmppc_set_compat() always fail because the
> > current PR implementation doesn't handle KVM_REG_PPC_ARCH_COMPAT. Now that
> > the machine code inconditionally calls ppc_set_compat_all() at reset time
> > to restore the compat mode default value (commit 66d5c492dd3a9), it is
> > impossible to start a guest with PR:
> > 
> > qemu-system-ppc64: Unable to set CPU compatibility mode in KVM:
> >  Invalid argument
> > 
> > A tentative patch [1] was recently sent by Suraj to address the issue, but
> > it would prevent the compat mode to be turned off on reset. And we really
> > don't want to explicitely check for KVM PR. During the patch's review,
> > David suggested that we should only call the KVM ioctl() if the compat
> > PVR changes. This allows at least to run with KVM PR, provided no compat
> > mode is requested from the command line (which should be the case when
> > running PR nested). This is what this patch does.  
> 
> I'm not sure that's true any more, since we now prefer compat modes
> during CAS.  I guess we could exclude the compat modes from the

Oops, I wanted to talk about CAS and I... forgot :(

So, indeed, CAS fails to apply compat mode and immediately returns H_HARDWARE
to the guest, ie the rest of CAS isn't done :-\. Linux guests print a warning:

WARNING: ibm,client-architecture-support call FAILED!
 done

and (luckily?) continue to boot anyway (at least that's what happens with
RHEL7 guests).

> advertised list based on what KVM supports, but that seems pretty
> awkward.
> 

I'll re-read the details in LoPAPR and try to come up with something.

Cheers,

--
Greg

> > While here, we also fix the side effect where KVM would fail but we would
> > change the CPU state in QEMU anyway.
> > 
> > [1] http://patchwork.ozlabs.org/patch/782039/
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> In any case, this makes things better than they were, so I'm applying.
> 
> > ---
> >  target/ppc/compat.c |    9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target/ppc/compat.c b/target/ppc/compat.c
> > index f1b67faa97e3..f8729fe46d61 100644
> > --- a/target/ppc/compat.c
> > +++ b/target/ppc/compat.c
> > @@ -140,16 +140,17 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
> >  
> >      cpu_synchronize_state(CPU(cpu));
> >  
> > -    cpu->compat_pvr = compat_pvr;
> > -    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> > -
> > -    if (kvm_enabled()) {
> > +    if (kvm_enabled() && cpu->compat_pvr != compat_pvr) {
> >          int ret = kvmppc_set_compat(cpu, cpu->compat_pvr);
> >          if (ret < 0) {
> >              error_setg_errno(errp, -ret,
> >                               "Unable to set CPU compatibility mode in KVM");
> > +            return;
> >          }
> >      }
> > +
> > +    cpu->compat_pvr = compat_pvr;
> > +    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> >  }
> >  
> >  typedef struct {
> >   
>