Re: [Qemu-ppc] [PATCH] target-ppc/pseries: Clean up handling of KVM managed external HPTs

[David Gibson]

On Fri, Mar 04, 2016 at 01:40:42PM +1100, David Gibson wrote:
> fa48b43 "target-ppc: Remove hack for ppc_hash64_load_hpte*() with HV KVM"
> purports to remove a hack in the handling of hash page tables (HPTs)
> managed by KVM instead of qemu.  However, it makes the wrong call.
> 
> That patch requires anything looking for an external HPT (that is one not
> managed by the guest itself) to check both env->external_htab (for a qemu
> managed HPT) and kvmppc_kern_htab (for a KVM managed HPT).  That's a
> problem because kvmppc_kern_htab is local to mmu-hash64.c, but some places
> which need to check for an external HPT are outside that, such as
> kvm_arch_get_registers().  The latter was subtly broken by the earlier
> patch such that gdbstub can no longer access memory.
> 
> Basically a KVM managed HPT is much more like a qemu managed HPT than it is
> like a guest managed HPT, so the original "hack" was actually on the right
> track.
> 
> This partially reverts fa48b43, marking a KVM managed external HPT by
> putting a special but non-NULL value in env->external_htab.  It then goes
> further, using that marker to eliminate the kvmppc_kern_htab global
> entirely, and adding a ppc_hash64_set_external_hpt() helper function to
> reduce the amount of intimate knowledge of the cpu code that the pseries
> machine type needs to set this up correctly.
> 
> This also has some flow-on changes to the HPT access helpers, required by
> the above changes.
> 
> Reported-by: Greg Kurz <address@hidden>
> Signed-off-by: David Gibson <address@hidden>

Self NACK, sorry.  Realised this has a stupid omission (and also
partially overlaps with another patch I was working on).

> ---
>  hw/ppc/spapr.c          |  6 ++----
>  hw/ppc/spapr_hcall.c    | 10 +++++-----
>  target-ppc/mmu-hash64.c | 46 +++++++++++++++++++++++++---------------------
>  target-ppc/mmu-hash64.h |  9 ++++-----
>  4 files changed, 36 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e9d4abf..d8b749c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1091,7 +1091,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState 
> *spapr, int shift,
>          }
>  
>          spapr->htab_shift = shift;
> -        kvmppc_kern_htab = true;
> +        spapr->htab = NULL;
>      } else {
>          /* kernel-side HPT not needed, allocate in userspace instead */
>          size_t size = 1ULL << shift;
> @@ -1106,7 +1106,6 @@ static void spapr_reallocate_hpt(sPAPRMachineState 
> *spapr, int shift,
>  
>          memset(spapr->htab, 0, size);
>          spapr->htab_shift = shift;
> -        kvmppc_kern_htab = false;
>  
>          for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
>              DIRTY_HPTE(HPTE(spapr->htab, i));
> @@ -1196,8 +1195,7 @@ static void spapr_cpu_reset(void *opaque)
>  
>      env->spr[SPR_HIOR] = 0;
>  
> -    env->external_htab = (uint8_t *)spapr->htab;
> -    env->htab_base = -1;
> +    ppc_hash64_set_external_hpt(cpu, spapr->htab);
>      /*
>       * htab_mask is the mask used to normalize hash value to PTEG index.
>       * htab_shift is log2 of hash table size.
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 1733482..b2b1b93 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -122,17 +122,17 @@ static target_ulong h_enter(PowerPCCPU *cpu, 
> sPAPRMachineState *spapr,
>                  break;
>              }
>          }
> -        ppc_hash64_stop_access(token);
> +        ppc_hash64_stop_access(cpu, token);
>          if (index == 8) {
>              return H_PTEG_FULL;
>          }
>      } else {
>          token = ppc_hash64_start_access(cpu, pte_index);
>          if (ppc_hash64_load_hpte0(cpu, token, 0) & HPTE64_V_VALID) {
> -            ppc_hash64_stop_access(token);
> +            ppc_hash64_stop_access(cpu, token);
>              return H_PTEG_FULL;
>          }
> -        ppc_hash64_stop_access(token);
> +        ppc_hash64_stop_access(cpu, token);
>      }
>  
>      ppc_hash64_store_hpte(cpu, pte_index + index,
> @@ -165,7 +165,7 @@ static RemoveResult remove_hpte(PowerPCCPU *cpu, 
> target_ulong ptex,
>      token = ppc_hash64_start_access(cpu, ptex);
>      v = ppc_hash64_load_hpte0(cpu, token, 0);
>      r = ppc_hash64_load_hpte1(cpu, token, 0);
> -    ppc_hash64_stop_access(token);
> +    ppc_hash64_stop_access(cpu, token);
>  
>      if ((v & HPTE64_V_VALID) == 0 ||
>          ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> @@ -288,7 +288,7 @@ static target_ulong h_protect(PowerPCCPU *cpu, 
> sPAPRMachineState *spapr,
>      token = ppc_hash64_start_access(cpu, pte_index);
>      v = ppc_hash64_load_hpte0(cpu, token, 0);
>      r = ppc_hash64_load_hpte1(cpu, token, 0);
> -    ppc_hash64_stop_access(token);
> +    ppc_hash64_stop_access(cpu, token);
>  
>      if ((v & HPTE64_V_VALID) == 0 ||
>          ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 9c58fbf..88d4296 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -36,10 +36,11 @@
>  #endif
>  
>  /*
> - * Used to indicate whether we have allocated htab in the
> - * host kernel
> + * Used to indicate that a CPU has it's hash page table (HPT) managed
> + * within the host kernel
>   */
> -bool kvmppc_kern_htab;
> +#define MMU_HASH64_KVM_MANAGED_HPT      ((void *)-1)
> +
>  /*
>   * SLB handling
>   */
> @@ -259,6 +260,18 @@ target_ulong helper_load_slb_vsid(CPUPPCState *env, 
> target_ulong rb)
>   * 64-bit hash table MMU handling
>   */
>  
> +void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt)
> +{
> +    CPUPPCState *env = &cpu->env;
> +
> +    if (hpt) {
> +        env->external_htab = hpt;
> +    } else {
> +        env->external_htab = MMU_HASH64_KVM_MANAGED_HPT;
> +    }
> +    env->htab_base = -1;
> +}
> +
>  static int ppc_hash64_pte_prot(PowerPCCPU *cpu,
>                                 ppc_slb_t *slb, ppc_hash_pte64_t pte)
>  {
> @@ -353,25 +366,16 @@ uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, 
> target_ulong pte_index)
>      hwaddr pte_offset;
>  
>      pte_offset = pte_index * HASH_PTE_SIZE_64;
> -    if (kvmppc_kern_htab) {
> +    if (cpu->env.external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
>          /*
>           * HTAB is controlled by KVM. Fetch the PTEG into a new buffer.
>           */
>          token = kvmppc_hash64_read_pteg(cpu, pte_index);
> -        if (token) {
> -            return token;
> -        }
> +    } else if (cpu->env.external_htab) {
>          /*
> -         * pteg read failed, even though we have allocated htab via
> -         * kvmppc_reset_htab.
> +         * HTAB is controlled by QEMU. Just point to the internally
> +         * accessible PTEG.
>           */
> -        return 0;
> -    }
> -    /*
> -     * HTAB is controlled by QEMU. Just point to the internally
> -     * accessible PTEG.
> -     */
> -    if (cpu->env.external_htab) {
>          token = (uint64_t)(uintptr_t) cpu->env.external_htab + pte_offset;
>      } else if (cpu->env.htab_base) {
>          token = cpu->env.htab_base + pte_offset;
> @@ -379,9 +383,9 @@ uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, 
> target_ulong pte_index)
>      return token;
>  }
>  
> -void ppc_hash64_stop_access(uint64_t token)
> +void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token)
>  {
> -    if (kvmppc_kern_htab) {
> +    if (cpu->env.external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
>          kvmppc_hash64_free_pteg(token);
>      }
>  }
> @@ -410,11 +414,11 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, 
> hwaddr hash,
>              && HPTE64_V_COMPARE(pte0, ptem)) {
>              pte->pte0 = pte0;
>              pte->pte1 = pte1;
> -            ppc_hash64_stop_access(token);
> +            ppc_hash64_stop_access(cpu, token);
>              return (pte_index + i) * HASH_PTE_SIZE_64;
>          }
>      }
> -    ppc_hash64_stop_access(token);
> +    ppc_hash64_stop_access(cpu, token);
>      /*
>       * We didn't find a valid entry.
>       */
> @@ -729,7 +733,7 @@ void ppc_hash64_store_hpte(PowerPCCPU *cpu,
>  {
>      CPUPPCState *env = &cpu->env;
>  
> -    if (kvmppc_kern_htab) {
> +    if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
>          kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
>          return;
>      }
> diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
> index e7d9925..f7b1f0d 100644
> --- a/target-ppc/mmu-hash64.h
> +++ b/target-ppc/mmu-hash64.h
> @@ -90,10 +90,9 @@ unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
>  #define HPTE64_V_1TB_SEG        0x4000000000000000ULL
>  #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
>  
> -
> -extern bool kvmppc_kern_htab;
> +void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt);
>  uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_index);
> -void ppc_hash64_stop_access(uint64_t token);
> +void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token);
>  
>  static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
>                                                   uint64_t token, int index)
> @@ -102,7 +101,7 @@ static inline target_ulong 
> ppc_hash64_load_hpte0(PowerPCCPU *cpu,
>      uint64_t addr;
>  
>      addr = token + (index * HASH_PTE_SIZE_64);
> -    if (kvmppc_kern_htab || env->external_htab) {
> +    if (env->external_htab) {
>          return  ldq_p((const void *)(uintptr_t)addr);
>      } else {
>          return ldq_phys(CPU(cpu)->as, addr);
> @@ -116,7 +115,7 @@ static inline target_ulong 
> ppc_hash64_load_hpte1(PowerPCCPU *cpu,
>      uint64_t addr;
>  
>      addr = token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2;
> -    if (kvmppc_kern_htab || env->external_htab) {
> +    if (env->external_htab) {
>          return  ldq_p((const void *)(uintptr_t)addr);
>      } else {
>          return ldq_phys(CPU(cpu)->as, addr);

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