[PATCH v1 0/3] ppc: LPCR synchronisation fixes

[PATCH v1 0/3] ppc: LPCR synchronisation fixes

[Nicholas Piggin]

These incoherencies have been around for a while, but they've started
to get noticed now because recent Linux guests crash when LPCR[AIL] is
not set correctly (scv requires it). This series fixes at least AIL and
DPFD incoherency between KVM and QEMU, and between different vCPUs.

Thanks,
Nick

Nicholas Piggin (3):
  spapr: Remove stale comment about power-saving LPCR bits
  spapr: Set LPCR to current AIL mode when starting a new CPU
  target/ppc: Synchronize with KVM's LPCR value when creating a vCPU

 hw/ppc/spapr_cpu_core.c |  9 ++++++---
 hw/ppc/spapr_rtas.c     | 15 +++++++++------
 target/ppc/kvm.c        | 34 ++++++++++++++++++++--------------
 3 files changed, 35 insertions(+), 23 deletions(-)

-- 
2.23.0

[PATCH v1 1/3] spapr: Remove stale comment about power-saving LPCR bits

[Nicholas Piggin]

Commit 47a9b551547 ("spapr: Clean up handling of LPCR power-saving exit
bits") moved this logic but did not remove the comment from the
previous location.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr_rtas.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 03355b4c0a..63d96955c0 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -164,7 +164,6 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
     env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
     hreg_compute_hflags(env);
 
-    /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
     lpcr = env->spr[SPR_LPCR];
     if (!pcc->interrupts_big_endian(callcpu)) {
         lpcr |= LPCR_ILE;
-- 
2.23.0

Re: [PATCH v1 1/3] spapr: Remove stale comment about power-saving LPCR bits

[Cédric Le Goater]

On 5/26/21 11:16 AM, Nicholas Piggin wrote:
> Commit 47a9b551547 ("spapr: Clean up handling of LPCR power-saving exit
> bits") moved this logic but did not remove the comment from the
> previous location.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> ---
>  hw/ppc/spapr_rtas.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 03355b4c0a..63d96955c0 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -164,7 +164,6 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
>      env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>      hreg_compute_hflags(env);
>  
> -    /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
>      lpcr = env->spr[SPR_LPCR];
>      if (!pcc->interrupts_big_endian(callcpu)) {
>          lpcr |= LPCR_ILE;
> 

Re: [PATCH v1 1/3] spapr: Remove stale comment about power-saving LPCR bits

[Greg Kurz]

On Wed, 26 May 2021 19:16:24 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:

> Commit 47a9b551547 ("spapr: Clean up handling of LPCR power-saving exit
> bits") moved this logic but did not remove the comment from the
> previous location.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_rtas.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 03355b4c0a..63d96955c0 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -164,7 +164,6 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
>      env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>      hreg_compute_hflags(env);
>  
> -    /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
>      lpcr = env->spr[SPR_LPCR];
>      if (!pcc->interrupts_big_endian(callcpu)) {
>          lpcr |= LPCR_ILE;

Re: [PATCH v1 1/3] spapr: Remove stale comment about power-saving LPCR bits

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]

On Wed, May 26, 2021 at 07:16:24PM +1000, Nicholas Piggin wrote:
> Commit 47a9b551547 ("spapr: Clean up handling of LPCR power-saving exit
> bits") moved this logic but did not remove the comment from the
> previous location.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to ppc-for-6.1, thanks.

> ---
>  hw/ppc/spapr_rtas.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 03355b4c0a..63d96955c0 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -164,7 +164,6 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
>      env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>      hreg_compute_hflags(env);
>  
> -    /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
>      lpcr = env->spr[SPR_LPCR];
>      if (!pcc->interrupts_big_endian(callcpu)) {
>          lpcr |= LPCR_ILE;

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 2/3] spapr: Set LPCR to current AIL mode when starting a new CPU

[Nicholas Piggin]

TCG does not keep track of AIL mode in a central place, it's based on
the current LPCR[AIL] bits. Synchronize the new CPU's LPCR to the
current LPCR in rtas_start_cpu(), similarly to the way the ILE bit is
synchronized.

Open-code the ILE setting as well now that the caller's LPCR is
available directly, there is no need for the indirection.

Without this, under both TCG and KVM, adding a POWER8/9/10 class CPU
with a new core ID after a modern Linux has booted results in the new
CPU's LPCR missing the LPCR[AIL]=0b11 setting that the other CPUs have.
This can cause crashes and unexpected behaviour.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr_rtas.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 63d96955c0..b476382ae6 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -132,8 +132,8 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
     target_ulong id, start, r3;
     PowerPCCPU *newcpu;
     CPUPPCState *env;
-    PowerPCCPUClass *pcc;
     target_ulong lpcr;
+    target_ulong caller_lpcr;
 
     if (nargs != 3 || nret != 1) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
@@ -152,7 +152,6 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
     }
 
     env = &newcpu->env;
-    pcc = POWERPC_CPU_GET_CLASS(newcpu);
 
     if (!CPU(newcpu)->halted) {
         rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
@@ -164,10 +163,15 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
     env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
     hreg_compute_hflags(env);
 
+    caller_lpcr = callcpu->env.spr[SPR_LPCR];
     lpcr = env->spr[SPR_LPCR];
-    if (!pcc->interrupts_big_endian(callcpu)) {
-        lpcr |= LPCR_ILE;
-    }
+
+    /* Set ILE the same way */
+    lpcr = (lpcr & ~LPCR_ILE) | (caller_lpcr & LPCR_ILE);
+
+    /* Set AIL the same way */
+    lpcr = (lpcr & ~LPCR_AIL) | (caller_lpcr & LPCR_AIL);
+
     if (env->mmu_model == POWERPC_MMU_3_00) {
         /*
          * New cpus are expected to start in the same radix/hash mode
-- 
2.23.0

Re: [PATCH v1 2/3] spapr: Set LPCR to current AIL mode when starting a new CPU

[Cédric Le Goater]

On 5/26/21 11:16 AM, Nicholas Piggin wrote:
> TCG does not keep track of AIL mode in a central place, it's based on
> the current LPCR[AIL] bits. Synchronize the new CPU's LPCR to the
> current LPCR in rtas_start_cpu(), similarly to the way the ILE bit is
> synchronized.
> 
> Open-code the ILE setting as well now that the caller's LPCR is
> available directly, there is no need for the indirection.

That's two patches in one but no big deal.

> 
> Without this, under both TCG and KVM, adding a POWER8/9/10 class CPU
> with a new core ID after a modern Linux has booted results in the new
> CPU's LPCR missing the LPCR[AIL]=0b11 setting that the other CPUs have.
> This can cause crashes and unexpected behaviour.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

C.

> ---
>  hw/ppc/spapr_rtas.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 63d96955c0..b476382ae6 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -132,8 +132,8 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
>      target_ulong id, start, r3;
>      PowerPCCPU *newcpu;
>      CPUPPCState *env;
> -    PowerPCCPUClass *pcc;
>      target_ulong lpcr;
> +    target_ulong caller_lpcr;
>  
>      if (nargs != 3 || nret != 1) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -152,7 +152,6 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
>      }
>  
>      env = &newcpu->env;
> -    pcc = POWERPC_CPU_GET_CLASS(newcpu);
>  
>      if (!CPU(newcpu)->halted) {
>          rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> @@ -164,10 +163,15 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
>      env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>      hreg_compute_hflags(env);
>  
> +    caller_lpcr = callcpu->env.spr[SPR_LPCR];
>      lpcr = env->spr[SPR_LPCR];
> -    if (!pcc->interrupts_big_endian(callcpu)) {
> -        lpcr |= LPCR_ILE;
> -    }
> +
> +    /* Set ILE the same way */
> +    lpcr = (lpcr & ~LPCR_ILE) | (caller_lpcr & LPCR_ILE);
> +
> +    /* Set AIL the same way */
> +    lpcr = (lpcr & ~LPCR_AIL) | (caller_lpcr & LPCR_AIL);
> +
>      if (env->mmu_model == POWERPC_MMU_3_00) {
>          /*
>           * New cpus are expected to start in the same radix/hash mode
> 

Re: [PATCH v1 2/3] spapr: Set LPCR to current AIL mode when starting a new CPU

[Greg Kurz]

On Wed, 26 May 2021 19:16:25 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:

> TCG does not keep track of AIL mode in a central place, it's based on
> the current LPCR[AIL] bits. Synchronize the new CPU's LPCR to the
> current LPCR in rtas_start_cpu(), similarly to the way the ILE bit is
> synchronized.
> 
> Open-code the ILE setting as well now that the caller's LPCR is
> available directly, there is no need for the indirection.
> 
> Without this, under both TCG and KVM, adding a POWER8/9/10 class CPU
> with a new core ID after a modern Linux has booted results in the new
> CPU's LPCR missing the LPCR[AIL]=0b11 setting that the other CPUs have.
> This can cause crashes and unexpected behaviour.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_rtas.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 63d96955c0..b476382ae6 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -132,8 +132,8 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
>      target_ulong id, start, r3;
>      PowerPCCPU *newcpu;
>      CPUPPCState *env;
> -    PowerPCCPUClass *pcc;
>      target_ulong lpcr;
> +    target_ulong caller_lpcr;
>  
>      if (nargs != 3 || nret != 1) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -152,7 +152,6 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
>      }
>  
>      env = &newcpu->env;
> -    pcc = POWERPC_CPU_GET_CLASS(newcpu);
>  
>      if (!CPU(newcpu)->halted) {
>          rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> @@ -164,10 +163,15 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
>      env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>      hreg_compute_hflags(env);
>  
> +    caller_lpcr = callcpu->env.spr[SPR_LPCR];
>      lpcr = env->spr[SPR_LPCR];
> -    if (!pcc->interrupts_big_endian(callcpu)) {
> -        lpcr |= LPCR_ILE;
> -    }
> +
> +    /* Set ILE the same way */
> +    lpcr = (lpcr & ~LPCR_ILE) | (caller_lpcr & LPCR_ILE);
> +

Unrelated change as Cedric already noted but that's nice :)

/me starting to think we might do the same elsewhere and
maybe get rid of PowerPCCPUClass::interrupts_big_endian()

> +    /* Set AIL the same way */
> +    lpcr = (lpcr & ~LPCR_AIL) | (caller_lpcr & LPCR_AIL);
> +

It seems better indeed to rely on the calling CPU here rather
than the arbitrary first_cpu in the hotplug handler.

Reviewed-by: Greg Kurz <groug@kaod.org>

>      if (env->mmu_model == POWERPC_MMU_3_00) {
>          /*
>           * New cpus are expected to start in the same radix/hash mode

Re: [PATCH v1 2/3] spapr: Set LPCR to current AIL mode when starting a new CPU

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3242 bytes --]

On Wed, May 26, 2021 at 06:03:09PM +0200, Greg Kurz wrote:
> On Wed, 26 May 2021 19:16:25 +1000
> Nicholas Piggin <npiggin@gmail.com> wrote:
> 
> > TCG does not keep track of AIL mode in a central place, it's based on
> > the current LPCR[AIL] bits. Synchronize the new CPU's LPCR to the
> > current LPCR in rtas_start_cpu(), similarly to the way the ILE bit is
> > synchronized.
> > 
> > Open-code the ILE setting as well now that the caller's LPCR is
> > available directly, there is no need for the indirection.
> > 
> > Without this, under both TCG and KVM, adding a POWER8/9/10 class CPU
> > with a new core ID after a modern Linux has booted results in the new
> > CPU's LPCR missing the LPCR[AIL]=0b11 setting that the other CPUs have.
> > This can cause crashes and unexpected behaviour.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  hw/ppc/spapr_rtas.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 63d96955c0..b476382ae6 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -132,8 +132,8 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
> >      target_ulong id, start, r3;
> >      PowerPCCPU *newcpu;
> >      CPUPPCState *env;
> > -    PowerPCCPUClass *pcc;
> >      target_ulong lpcr;
> > +    target_ulong caller_lpcr;
> >  
> >      if (nargs != 3 || nret != 1) {
> >          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > @@ -152,7 +152,6 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
> >      }
> >  
> >      env = &newcpu->env;
> > -    pcc = POWERPC_CPU_GET_CLASS(newcpu);
> >  
> >      if (!CPU(newcpu)->halted) {
> >          rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > @@ -164,10 +163,15 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, SpaprMachineState *spapr,
> >      env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> >      hreg_compute_hflags(env);
> >  
> > +    caller_lpcr = callcpu->env.spr[SPR_LPCR];
> >      lpcr = env->spr[SPR_LPCR];
> > -    if (!pcc->interrupts_big_endian(callcpu)) {
> > -        lpcr |= LPCR_ILE;
> > -    }
> > +
> > +    /* Set ILE the same way */
> > +    lpcr = (lpcr & ~LPCR_ILE) | (caller_lpcr & LPCR_ILE);
> > +
> 
> Unrelated change as Cedric already noted but that's nice :)
> 
> /me starting to think we might do the same elsewhere and
> maybe get rid of PowerPCCPUClass::interrupts_big_endian()

Yes, that's a nice thought.

> > +    /* Set AIL the same way */
> > +    lpcr = (lpcr & ~LPCR_AIL) | (caller_lpcr & LPCR_AIL);
> > +
> 
> It seems better indeed to rely on the calling CPU here rather
> than the arbitrary first_cpu in the hotplug handler.

I agree.

> Reviewed-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-6.1, thanks.

> 
> >      if (env->mmu_model == POWERPC_MMU_3_00) {
> >          /*
> >           * New cpus are expected to start in the same radix/hash mode
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 3/3] target/ppc: Synchronize with KVM's LPCR value when creating a vCPU

[Nicholas Piggin]

Despite the suggestion from the comment, LPCR value set by KVM does not
get propagated to QEMU SPR values. Instead, the KVM LPCR register is set
from the inital QEMU values, of which KVM allows the DPFD, ILE, TC, AIL,
LD fields to be modified. For the most part these get fixed up, but at
least the DPFD value set by KVM gets lost.

Fix this by reading the KVM LPCR when initialising a vCPU and reading
registers. The hack for setting the LPCR LD bit can be removed.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr_cpu_core.c |  9 ++++++---
 target/ppc/kvm.c        | 34 ++++++++++++++++++++--------------
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 4f316a6f9d..91193b4bba 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -40,9 +40,12 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
 
     lpcr = env->spr[SPR_LPCR];
 
-    /* Set emulated LPCR to not send interrupts to hypervisor. Note that
-     * under KVM, the actual HW LPCR will be set differently by KVM itself,
-     * the settings below ensure proper operations with TCG in absence of
+    /*
+     * Set emulated LPCR to not send interrupts to hypervisor. Note that
+     * under KVM, the actual HW LPCR will be set differently by KVM itself
+     * and that gets loaded by kvm_arch_get_registers and kvm_arch_init_vcpu.
+     *
+     * The LPCR settings below ensure proper operations with TCG in absence of
      * a real hypervisor.
      *
      * Disable Power-saving mode Exit Cause exceptions for the CPU, so
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 104a308abb..ec269e38f8 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -104,6 +104,7 @@ static bool kvmppc_is_pr(KVMState *ks)
     return kvm_vm_check_extension(ks, KVM_CAP_PPC_GET_PVINFO) != 0;
 }
 
+static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr);
 static int kvm_ppc_register_host_cpu_type(void);
 static void kvmppc_get_cpu_characteristics(KVMState *s);
 static int kvmppc_get_dec_bits(void);
@@ -477,6 +478,16 @@ int kvm_arch_init_vcpu(CPUState *cs)
         return ret;
     }
 
+    /*
+     * As explained in spapr_reset_vcpu, a KVM guest needs to synchronize
+     * to the LPCR value set by KVM.
+     */
+#ifdef TARGET_PPC64
+    kvm_get_one_spr(cs, KVM_REG_PPC_LPCR_64, SPR_LPCR);
+#else
+    kvm_get_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
+#endif
+
     switch (cenv->mmu_model) {
     case POWERPC_MMU_BOOKE206:
         /* This target supports access to KVM's guest TLB */
@@ -1307,6 +1318,7 @@ int kvm_arch_get_registers(CPUState *cs)
 
         kvm_get_one_reg(cs, KVM_REG_PPC_TB_OFFSET, &env->tb_env->tb_offset);
         kvm_get_one_spr(cs, KVM_REG_PPC_DPDES, SPR_DPDES);
+        kvm_get_one_spr(cs, KVM_REG_PPC_LPCR_64, SPR_LPCR);
 #endif
     }
 
@@ -2529,24 +2541,18 @@ int kvmppc_get_cap_large_decr(void)
 
 int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
 {
+    CPUPPCState *env = &cpu->env;
     CPUState *cs = CPU(cpu);
     uint64_t lpcr;
 
-    kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
-    /* Do we need to modify the LPCR? */
-    if (!!(lpcr & LPCR_LD) != !!enable) {
-        if (enable) {
-            lpcr |= LPCR_LD;
-        } else {
-            lpcr &= ~LPCR_LD;
-        }
-        kvm_set_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
-        kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
-
-        if (!!(lpcr & LPCR_LD) != !!enable) {
-            return -1;
-        }
+    cpu_synchronize_state(cs);
+    lpcr = env->spr[SPR_LPCR];
+    if (enable) {
+        lpcr |= LPCR_LD;
+    } else {
+        lpcr &= ~LPCR_LD;
     }
+    ppc_store_lpcr(cpu, lpcr);
 
     return 0;
 }
-- 
2.23.0

Re: [PATCH v1 3/3] target/ppc: Synchronize with KVM's LPCR value when creating a vCPU

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5196 bytes --]

On Wed, May 26, 2021 at 07:16:26PM +1000, Nicholas Piggin wrote:
> Despite the suggestion from the comment, LPCR value set by KVM does not
> get propagated to QEMU SPR values. Instead, the KVM LPCR register is set
> from the inital QEMU values, of which KVM allows the DPFD, ILE, TC, AIL,
> LD fields to be modified. For the most part these get fixed up, but at
> least the DPFD value set by KVM gets lost.
> 
> Fix this by reading the KVM LPCR when initialising a vCPU and reading
> registers. The hack for setting the LPCR LD bit can be removed.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Uh.. few things I want to straighten out before applying this.

> ---
>  hw/ppc/spapr_cpu_core.c |  9 ++++++---
>  target/ppc/kvm.c        | 34 ++++++++++++++++++++--------------
>  2 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 4f316a6f9d..91193b4bba 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -40,9 +40,12 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>  
>      lpcr = env->spr[SPR_LPCR];
>  
> -    /* Set emulated LPCR to not send interrupts to hypervisor. Note that
> -     * under KVM, the actual HW LPCR will be set differently by KVM itself,
> -     * the settings below ensure proper operations with TCG in absence of
> +    /*
> +     * Set emulated LPCR to not send interrupts to hypervisor. Note that
> +     * under KVM, the actual HW LPCR will be set differently by KVM itself
> +     * and that gets loaded by kvm_arch_get_registers and kvm_arch_init_vcpu.
> +     *
> +     * The LPCR settings below ensure proper operations with TCG in absence of
>       * a real hypervisor.
>       *
>       * Disable Power-saving mode Exit Cause exceptions for the CPU, so
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 104a308abb..ec269e38f8 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -104,6 +104,7 @@ static bool kvmppc_is_pr(KVMState *ks)
>      return kvm_vm_check_extension(ks, KVM_CAP_PPC_GET_PVINFO) != 0;
>  }
>  
> +static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr);
>  static int kvm_ppc_register_host_cpu_type(void);
>  static void kvmppc_get_cpu_characteristics(KVMState *s);
>  static int kvmppc_get_dec_bits(void);
> @@ -477,6 +478,16 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          return ret;
>      }
>  
> +    /*
> +     * As explained in spapr_reset_vcpu, a KVM guest needs to synchronize
> +     * to the LPCR value set by KVM.
> +     */

We need this stuff before we do our first full
kvm_arch_get_registers(), so I guess we need this.

> +#ifdef TARGET_PPC64
> +    kvm_get_one_spr(cs, KVM_REG_PPC_LPCR_64, SPR_LPCR);
> +#else
> +    kvm_get_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);

But, I don't think the LPCR is a thing on any 32-bit CPUs, so I don't
think we need this.  Certainly we only register the LPCR for POWER5+
and later.

> +#endif
> +
>      switch (cenv->mmu_model) {
>      case POWERPC_MMU_BOOKE206:
>          /* This target supports access to KVM's guest TLB */
> @@ -1307,6 +1318,7 @@ int kvm_arch_get_registers(CPUState *cs)
>  
>          kvm_get_one_reg(cs, KVM_REG_PPC_TB_OFFSET, &env->tb_env->tb_offset);
>          kvm_get_one_spr(cs, KVM_REG_PPC_DPDES, SPR_DPDES);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_LPCR_64, SPR_LPCR);

This one however confuses me... as do several of the existing
kvm_get_one_spr() calls there.  AFAICT we should be getting all the
SPRs from KVM with code earlier in kvm_arch_get_registers().

       for (i = 0; i < 1024; i++) {
            uint64_t id = env->spr_cb[i].one_reg_id;

            if (id != 0) {
                kvm_get_one_spr(cs, id, i);
            }
        }

So, I'm wondering if the other explicit calls to kvm_get_one_spr() are
just working around places where we haven't properly set the KVM reg
id at register_spr() time.

>  #endif
>      }
>  
> @@ -2529,24 +2541,18 @@ int kvmppc_get_cap_large_decr(void)
>  
>  int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
>  {
> +    CPUPPCState *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
>      uint64_t lpcr;
>  
> -    kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
> -    /* Do we need to modify the LPCR? */
> -    if (!!(lpcr & LPCR_LD) != !!enable) {
> -        if (enable) {
> -            lpcr |= LPCR_LD;
> -        } else {
> -            lpcr &= ~LPCR_LD;
> -        }
> -        kvm_set_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
> -        kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
> -
> -        if (!!(lpcr & LPCR_LD) != !!enable) {
> -            return -1;
> -        }
> +    cpu_synchronize_state(cs);
> +    lpcr = env->spr[SPR_LPCR];
> +    if (enable) {
> +        lpcr |= LPCR_LD;
> +    } else {
> +        lpcr &= ~LPCR_LD;
>      }
> +    ppc_store_lpcr(cpu, lpcr);
>  
>      return 0;
>  }

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]