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

[Greg Kurz]

On Tue, 15 Aug 2017 14:04:09 +1000
David Gibson  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   
> 
> 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 {
> >   
> 



pgpYFhxJWm3lm.pgp
Description: OpenPGP digital signature

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

[Greg Kurz]

On Tue, 15 Aug 2017 11:40:34 +1000
Suraj Jitindar Singh  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   
> 
> 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 
> 

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



pgpnggAhms9ep.pgp
Description: OpenPGP digital signature

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

[David Gibson]

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 

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


signature.asc
Description: PGP signature

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

[Suraj Jitindar Singh]

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 

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 

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