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 <npig...@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
signature.asc
Description: PGP signature
