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

[David Gibson <david@gibson.dropbear.id.au>]

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