| Message ID | 20210526091626.3388262-4-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc: LPCR synchronisation fixes | expand |
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; > }
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; }
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(-)