[4/6] target/ppc: Cleanup HPTE accessors for 64-bit hash MMU

Accesses to the hashed page table (HPT) are complicated by the fact that
the HPT could be in one of three places:
   1) Within guest memory - when we're emulating a full guest CPU at the
      hardware level (e.g. powernv, mac99, g3beige)
   2) Within qemu, but outside guest memory - when we're emulating user and
      supervisor instructions within TCG, but instead of emulating
      the CPU's hypervisor mode, we just emulate a hypervisor's behaviour
      (pseries in TCG)
   3) Within KVM - a pseries machine using KVM acceleration.  Mostly
      accesses to the HPT are handled by KVM, but there are a few cases
      where qemu needs to access it via a special fd for the purpose.

In order to batch accesses to the fd in case (3), we use a somewhat awkward
ppc_hash64_start_access() / ppc_hash64_stop_access() pair, which for case
(3) reads / releases a whole PTEG from the kernel.  For cases (1) & (2)
it just returns an address value.  The actual HPTE load helpers then need
to interpret the returned token differently in the 3 cases.

This patch keeps the same basic structure, but simplfiies the details.
First start_access() / stop_access() are renamed to get_pteg() and
put_pteg() to make their operation more obvious.  Second, read_pteg() now
always returns a qemu pointer, which can always be used in the same way
by the load_hpte() helpers.  In case (1) it comes from address_space_map()
in case (2) directly from qemu's HPT buffer and in case (3) from a
temporary buffer read from the KVM fd.

While we're at it, make things a bit more consistent in terms of types and
variable names: avoid variables named 'index' (it shadows index(3) which
can lead to confusing results), use 'hwaddr ptex' for HPTE indices and
uint64_t for each of the HPTE words, use ptex throughout the call stack
instead of pte_offset in some places (we still need that at the bottom
layer, but nowhere else).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_hcall.c    | 36 +++++++++---------
 target/ppc/cpu.h        |  3 +-
 target/ppc/kvm.c        | 25 ++++++-------
 target/ppc/kvm_ppc.h    | 43 ++++++++++------------
 target/ppc/mmu-hash64.c | 98 ++++++++++++++++++++++++++-----------------------
 target/ppc/mmu-hash64.h | 46 ++++++++---------------
 6 files changed, 119 insertions(+), 132 deletions(-)

On 23/02/17 13:09, David Gibson wrote:
> Accesses to the hashed page table (HPT) are complicated by the fact that
> the HPT could be in one of three places:
>    1) Within guest memory - when we're emulating a full guest CPU at the
>       hardware level (e.g. powernv, mac99, g3beige)
>    2) Within qemu, but outside guest memory - when we're emulating user and
>       supervisor instructions within TCG, but instead of emulating
>       the CPU's hypervisor mode, we just emulate a hypervisor's behaviour
>       (pseries in TCG)
>    3) Within KVM - a pseries machine using KVM acceleration.  Mostly
>       accesses to the HPT are handled by KVM, but there are a few cases
>       where qemu needs to access it via a special fd for the purpose.
> 
> In order to batch accesses to the fd in case (3), we use a somewhat awkward
> ppc_hash64_start_access() / ppc_hash64_stop_access() pair, which for case
> (3) reads / releases a whole PTEG from the kernel.  For cases (1) & (2)
> it just returns an address value.  The actual HPTE load helpers then need
> to interpret the returned token differently in the 3 cases.
> 
> This patch keeps the same basic structure, but simplfiies the details.
> First start_access() / stop_access() are renamed to get_pteg() and
> put_pteg() to make their operation more obvious.


They are renamed to map_hptes/unmap_hptes actually.


> Second, read_pteg() now
> always returns a qemu pointer, which can always be used in the same way
> by the load_hpte() helpers.  In case (1) it comes from address_space_map()
> in case (2) directly from qemu's HPT buffer and in case (3) from a
> temporary buffer read from the KVM fd.
> 
> While we're at it, make things a bit more consistent in terms of types and
> variable names: avoid variables named 'index' (it shadows index(3) which
> can lead to confusing results), use 'hwaddr ptex' for HPTE indices and
> uint64_t for each of the HPTE words, use ptex throughout the call stack
> instead of pte_offset in some places (we still need that at the bottom
> layer, but nowhere else).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_hcall.c    | 36 +++++++++---------
>  target/ppc/cpu.h        |  3 +-
>  target/ppc/kvm.c        | 25 ++++++-------
>  target/ppc/kvm_ppc.h    | 43 ++++++++++------------
>  target/ppc/mmu-hash64.c | 98 ++++++++++++++++++++++++++-----------------------
>  target/ppc/mmu-hash64.h | 46 ++++++++---------------
>  6 files changed, 119 insertions(+), 132 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 3298a14..fd961b5 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -84,7 +84,7 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      unsigned apshift;
>      target_ulong raddr;
>      target_ulong slot;
> -    uint64_t token;
> +    const ppc_hash_pte64_t *hptes;
>  
>      apshift = ppc_hash64_hpte_page_shift_noslb(cpu, pteh, ptel);
>      if (!apshift) {
> @@ -123,23 +123,23 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      ptex = ptex & ~7ULL;
>  
>      if (likely((flags & H_EXACT) == 0)) {
> -        token = ppc_hash64_start_access(cpu, ptex);
> +        hptes = ppc_hash64_map_hptes(cpu, ptex, HPTES_PER_GROUP);
>          for (slot = 0; slot < 8; slot++) {
> -            if (!(ppc_hash64_load_hpte0(cpu, token, slot) & HPTE64_V_VALID)) {
> +            if (!(ppc_hash64_hpte0(cpu, hptes, slot) & HPTE64_V_VALID)) {
>                  break;
>              }
>          }
> -        ppc_hash64_stop_access(cpu, token);
> +        ppc_hash64_unmap_hptes(cpu, hptes, ptex, HPTES_PER_GROUP);
>          if (slot == 8) {
>              return H_PTEG_FULL;
>          }
>      } else {
> -        token = ppc_hash64_start_access(cpu, ptex);
> -        if (ppc_hash64_load_hpte0(cpu, token, 0) & HPTE64_V_VALID) {
> -            ppc_hash64_stop_access(cpu, token);
> +        hptes = ppc_hash64_map_hptes(cpu, ptex + slot, 1);
> +        if (ppc_hash64_hpte0(cpu, hptes, 0) & HPTE64_V_VALID) {
> +            ppc_hash64_unmap_hptes(cpu, hptes, ptex + slot, 1);
>              return H_PTEG_FULL;
>          }
> -        ppc_hash64_stop_access(cpu, token);
> +        ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
>      }
>  
>      ppc_hash64_store_hpte(cpu, ptex + slot, pteh | HPTE64_V_HPTE_DIRTY, ptel);
> @@ -160,17 +160,17 @@ static RemoveResult remove_hpte(PowerPCCPU *cpu, target_ulong ptex,
>                                  target_ulong flags,
>                                  target_ulong *vp, target_ulong *rp)
>  {
> -    uint64_t token;
> +    const ppc_hash_pte64_t *hptes;
>      target_ulong v, r;
>  
>      if (!valid_ptex(cpu, ptex)) {
>          return REMOVE_PARM;
>      }
>  
> -    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(cpu, token);
> +    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
> +    v = ppc_hash64_hpte0(cpu, hptes, 0);
> +    r = ppc_hash64_hpte1(cpu, hptes, 0);
> +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
>  
>      if ((v & HPTE64_V_VALID) == 0 ||
>          ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> @@ -291,17 +291,17 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      target_ulong flags = args[0];
>      target_ulong ptex = args[1];
>      target_ulong avpn = args[2];
> -    uint64_t token;
> +    const ppc_hash_pte64_t *hptes;
>      target_ulong v, r;
>  
>      if (!valid_ptex(cpu, ptex)) {
>          return H_PARAMETER;
>      }
>  
> -    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(cpu, token);
> +    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
> +    v = ppc_hash64_hpte0(cpu, hptes, 0);
> +    r = ppc_hash64_hpte1(cpu, hptes, 0);
> +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
>  
>      if ((v & HPTE64_V_VALID) == 0 ||
>          ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index f99bcae..c89973e 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -223,11 +223,12 @@ enum {
>  typedef struct opc_handler_t opc_handler_t;
>  
>  /*****************************************************************************/
> -/* Types used to describe some PowerPC registers */
> +/* Types used to describe some PowerPC registers etc. */
>  typedef struct DisasContext DisasContext;
>  typedef struct ppc_spr_t ppc_spr_t;
>  typedef union ppc_avr_t ppc_avr_t;
>  typedef union ppc_tlb_t ppc_tlb_t;
> +typedef struct ppc_hash_pte64 ppc_hash_pte64_t;
>  
>  /* SPR access micro-ops generations callbacks */
>  struct ppc_spr_t {
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 52bbea5..9d3e57e 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2601,17 +2601,17 @@ struct kvm_get_htab_buf {
>      /*
>       * We require one extra byte for read
>       */
> -    target_ulong hpte[(HPTES_PER_GROUP * 2) + 1];
> +    ppc_hash_pte64_t hpte[HPTES_PER_GROUP];


The "one extra byte" (which was ulong) is not needed any more why?


>  };
>  
> -uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong pte_index)
> +const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex, int n)


This "int n" is ignored here by a reason?


>  {
>      int htab_fd;
>      struct kvm_get_htab_fd ghf;
> -    struct kvm_get_htab_buf  *hpte_buf;
> +    struct kvm_get_htab_buf *hpte_buf;
>  
>      ghf.flags = 0;
> -    ghf.start_index = pte_index;
> +    ghf.start_index = ptex;
>      htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
>      if (htab_fd < 0) {
>          goto error_out;
> @@ -2626,7 +2626,7 @@ uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong pte_index)
>      }
>  
>      close(htab_fd);
> -    return (uint64_t)(uintptr_t) hpte_buf->hpte;
> +    return hpte_buf->hpte;
>  
>  out_close:
>      g_free(hpte_buf);
> @@ -2635,18 +2635,15 @@ error_out:
>      return 0;
>  }
>  
> -void kvmppc_hash64_free_pteg(uint64_t token)
> +void kvmppc_unmap_hptes(const ppc_hash_pte64_t *hptes, hwaddr ptex, int n)
>  {
>      struct kvm_get_htab_buf *htab_buf;
>  
> -    htab_buf = container_of((void *)(uintptr_t) token, struct kvm_get_htab_buf,
> -                            hpte);
> +    htab_buf = container_of((void *)hptes, struct kvm_get_htab_buf, hpte);
>      g_free(htab_buf);
> -    return;
>  }
>  
> -void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
> -                             target_ulong pte0, target_ulong pte1)
> +void kvmppc_hash64_write_pte(hwaddr ptex, uint64_t pte0, uint64_t pte1)
>  {
>      int htab_fd;
>      struct kvm_get_htab_fd ghf;
> @@ -2661,9 +2658,9 @@ void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
>  
>      hpte_buf.header.n_valid = 1;
>      hpte_buf.header.n_invalid = 0;
> -    hpte_buf.header.index = pte_index;
> -    hpte_buf.hpte[0] = pte0;
> -    hpte_buf.hpte[1] = pte1;
> +    hpte_buf.header.index = ptex;
> +    hpte_buf.hpte[0].pte0 = pte0;
> +    hpte_buf.hpte[0].pte1 = pte1;
>      /*
>       * Write the hpte entry.
>       * CAUTION: write() has the warn_unused_result attribute. Hence we
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 8da2ee4..3f8fccd 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -41,6 +41,10 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int *pfd,
>  int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t window_size);
>  int kvmppc_reset_htab(int shift_hint);
>  uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
> +const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex, int n);
> +void kvmppc_unmap_hptes(const ppc_hash_pte64_t *hptes, hwaddr ptex, int n);
> +
> +void kvmppc_hash64_write_pte(hwaddr ptex, uint64_t pte0, uint64_t pte1);
>  #endif /* !CONFIG_USER_ONLY */
>  bool kvmppc_has_cap_epr(void);
>  int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function);
> @@ -49,11 +53,6 @@ int kvmppc_get_htab_fd(bool write);
>  int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
>  int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>                             uint16_t n_valid, uint16_t n_invalid);
> -uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong pte_index);
> -void kvmppc_hash64_free_pteg(uint64_t token);
> -
> -void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
> -                             target_ulong pte0, target_ulong pte1);
>  bool kvmppc_has_cap_fixup_hcalls(void);
>  bool kvmppc_has_cap_htm(void);
>  int kvmppc_enable_hwrng(void);
> @@ -199,6 +198,22 @@ static inline bool kvmppc_is_mem_backend_page_size_ok(char *obj_path)
>      return true;
>  }
>  
> +static inline const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex, int n)
> +{
> +    abort();
> +}
> +
> +static inline void kvmppc_unmap_hptes(const ppc_hash_pte64_t *hptes,
> +                                      hwaddr ptex, int n)
> +{
> +    abort();
> +}
> +
> +static inline void kvmppc_hash64_write_pte(hwaddr ptex,
> +                                           uint64_t pte0, uint64_t pte1)
> +{
> +    abort();
> +}
>  #endif /* !CONFIG_USER_ONLY */
>  
>  static inline bool kvmppc_has_cap_epr(void)
> @@ -234,24 +249,6 @@ static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>      abort();
>  }
>  
> -static inline uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu,
> -                                               target_ulong pte_index)
> -{
> -    abort();
> -}
> -
> -static inline void kvmppc_hash64_free_pteg(uint64_t token)
> -{
> -    abort();
> -}
> -
> -static inline void kvmppc_hash64_write_pte(CPUPPCState *env,
> -                                           target_ulong pte_index,
> -                                           target_ulong pte0, target_ulong pte1)
> -{
> -    abort();
> -}
> -
>  static inline bool kvmppc_has_cap_fixup_hcalls(void)
>  {
>      abort();
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 76669ed..c59db47 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -27,6 +27,7 @@
>  #include "kvm_ppc.h"
>  #include "mmu-hash64.h"
>  #include "exec/log.h"
> +#include "hw/hw.h"
>  
>  //#define DEBUG_SLB
>  
> @@ -431,33 +432,42 @@ static int ppc_hash64_amr_prot(PowerPCCPU *cpu, ppc_hash_pte64_t pte)
>      return prot;
>  }
>  
> -uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_index)
> +const ppc_hash_pte64_t *ppc_hash64_map_hptes(PowerPCCPU *cpu,
> +                                             hwaddr ptex, int n)
>  {
> -    uint64_t token = 0;
> -    hwaddr pte_offset;
> +    const ppc_hash_pte64_t *hptes = NULL;
> +    hwaddr pte_offset = ptex * HASH_PTE_SIZE_64;
>  
> -    pte_offset = pte_index * HASH_PTE_SIZE_64;
>      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);
> +        hptes = kvmppc_map_hptes(ptex, n);
>      } else if (cpu->env.external_htab) {
>          /*
>           * HTAB is controlled by QEMU. Just point to the internally
>           * accessible PTEG.
>           */
> -        token = (uint64_t)(uintptr_t) cpu->env.external_htab + pte_offset;
> +        hptes = (ppc_hash_pte64_t *)(cpu->env.external_htab + pte_offset);
>      } else if (cpu->env.htab_base) {
> -        token = cpu->env.htab_base + pte_offset;
> +        hwaddr plen = n * HASH_PTE_SIZE_64;
> +        hptes = address_space_map(CPU(cpu)->as, cpu->env.htab_base + pte_offset,
> +                                 &plen, false);
> +        if (plen < (n * HASH_PTE_SIZE_64)) {
> +            hw_error("%s: Unable to map all requested HPTEs\n", __FUNCTION__);
> +        }
>      }
> -    return token;
> +    return hptes;
>  }
>  
> -void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token)
> +void ppc_hash64_unmap_hptes(PowerPCCPU *cpu, const ppc_hash_pte64_t *hptes,
> +                            hwaddr ptex, int n)
>  {
>      if (cpu->env.external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> -        kvmppc_hash64_free_pteg(token);
> +        kvmppc_unmap_hptes(hptes, ptex, n);
> +    } else if (!cpu->env.external_htab) {
> +        address_space_unmap(CPU(cpu)->as, (void *)hptes, n * HASH_PTE_SIZE_64,
> +                            false, n * HASH_PTE_SIZE_64);
>      }
>  }
>  
> @@ -505,18 +515,18 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
>  {
>      CPUPPCState *env = &cpu->env;
>      int i;
> -    uint64_t token;
> +    const ppc_hash_pte64_t *pteg;
>      target_ulong pte0, pte1;
> -    target_ulong pte_index;
> +    target_ulong ptex;
>  
> -    pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
> -    token = ppc_hash64_start_access(cpu, pte_index);
> -    if (!token) {
> +    ptex = (hash & env->htab_mask) * HPTES_PER_GROUP;
> +    pteg = ppc_hash64_map_hptes(cpu, ptex, HPTES_PER_GROUP);
> +    if (!pteg) {
>          return -1;
>      }
>      for (i = 0; i < HPTES_PER_GROUP; i++) {
> -        pte0 = ppc_hash64_load_hpte0(cpu, token, i);
> -        pte1 = ppc_hash64_load_hpte1(cpu, token, i);
> +        pte0 = ppc_hash64_hpte0(cpu, pteg, i);
> +        pte1 = ppc_hash64_hpte1(cpu, pteg, i);
>  
>          /* This compares V, B, H (secondary) and the AVPN */
>          if (HPTE64_V_COMPARE(pte0, ptem)) {
> @@ -536,11 +546,11 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
>               */
>              pte->pte0 = pte0;
>              pte->pte1 = pte1;
> -            ppc_hash64_stop_access(cpu, token);
> -            return (pte_index + i) * HASH_PTE_SIZE_64;
> +            ppc_hash64_unmap_hptes(cpu, pteg, ptex, HPTES_PER_GROUP);
> +            return ptex + i;
>          }
>      }
> -    ppc_hash64_stop_access(cpu, token);
> +    ppc_hash64_unmap_hptes(cpu, pteg, ptex, HPTES_PER_GROUP);
>      /*
>       * We didn't find a valid entry.
>       */
> @@ -552,8 +562,7 @@ static hwaddr ppc_hash64_htab_lookup(PowerPCCPU *cpu,
>                                       ppc_hash_pte64_t *pte, unsigned *pshift)
>  {
>      CPUPPCState *env = &cpu->env;
> -    hwaddr pte_offset;
> -    hwaddr hash;
> +    hwaddr hash, ptex;
>      uint64_t vsid, epnmask, epn, ptem;
>      const struct ppc_one_seg_page_size *sps = slb->sps;
>  
> @@ -596,9 +605,9 @@ static hwaddr ppc_hash64_htab_lookup(PowerPCCPU *cpu,
>              " vsid=" TARGET_FMT_lx " ptem=" TARGET_FMT_lx
>              " hash=" TARGET_FMT_plx "\n",
>              env->htab_base, env->htab_mask, vsid, ptem,  hash);
> -    pte_offset = ppc_hash64_pteg_search(cpu, hash, sps, ptem, pte, pshift);
> +    ptex = ppc_hash64_pteg_search(cpu, hash, sps, ptem, pte, pshift);
>  
> -    if (pte_offset == -1) {
> +    if (ptex == -1) {
>          /* Secondary PTEG lookup */
>          ptem |= HPTE64_V_SECONDARY;
>          qemu_log_mask(CPU_LOG_MMU,
> @@ -607,10 +616,10 @@ static hwaddr ppc_hash64_htab_lookup(PowerPCCPU *cpu,
>                  " hash=" TARGET_FMT_plx "\n", env->htab_base,
>                  env->htab_mask, vsid, ptem, ~hash);
>  
> -        pte_offset = ppc_hash64_pteg_search(cpu, ~hash, sps, ptem, pte, pshift);
> +        ptex = ppc_hash64_pteg_search(cpu, ~hash, sps, ptem, pte, pshift);
>      }
>  
> -    return pte_offset;
> +    return ptex;
>  }
>  
>  unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
> @@ -708,7 +717,7 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
>      CPUPPCState *env = &cpu->env;
>      ppc_slb_t *slb;
>      unsigned apshift;
> -    hwaddr pte_offset;
> +    hwaddr ptex;
>      ppc_hash_pte64_t pte;
>      int pp_prot, amr_prot, prot;
>      uint64_t new_pte1, dsisr;
> @@ -792,8 +801,8 @@ skip_slb_search:
>      }
>  
>      /* 4. Locate the PTE in the hash table */
> -    pte_offset = ppc_hash64_htab_lookup(cpu, slb, eaddr, &pte, &apshift);
> -    if (pte_offset == -1) {
> +    ptex = ppc_hash64_htab_lookup(cpu, slb, eaddr, &pte, &apshift);
> +    if (ptex == -1) {
>          dsisr = 0x40000000;
>          if (rwx == 2) {
>              ppc_hash64_set_isi(cs, env, dsisr);
> @@ -806,7 +815,7 @@ skip_slb_search:
>          return 1;
>      }
>      qemu_log_mask(CPU_LOG_MMU,
> -                "found PTE at offset %08" HWADDR_PRIx "\n", pte_offset);
> +                  "found PTE at index %08" HWADDR_PRIx "\n", ptex);
>  
>      /* 5. Check access permissions */
>  
> @@ -849,8 +858,7 @@ skip_slb_search:
>      }
>  
>      if (new_pte1 != pte.pte1) {
> -        ppc_hash64_store_hpte(cpu, pte_offset / HASH_PTE_SIZE_64,
> -                              pte.pte0, new_pte1);
> +        ppc_hash64_store_hpte(cpu, ptex, pte.pte0, new_pte1);
>      }
>  
>      /* 7. Determine the real address from the PTE */
> @@ -867,7 +875,7 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
>  {
>      CPUPPCState *env = &cpu->env;
>      ppc_slb_t *slb;
> -    hwaddr pte_offset, raddr;
> +    hwaddr ptex, raddr;
>      ppc_hash_pte64_t pte;
>      unsigned apshift;
>  
> @@ -900,8 +908,8 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
>          }
>      }
>  
> -    pte_offset = ppc_hash64_htab_lookup(cpu, slb, addr, &pte, &apshift);
> -    if (pte_offset == -1) {
> +    ptex = ppc_hash64_htab_lookup(cpu, slb, addr, &pte, &apshift);
> +    if (ptex == -1) {
>          return -1;
>      }
>  
> @@ -909,30 +917,28 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
>          & TARGET_PAGE_MASK;
>  }
>  
> -void ppc_hash64_store_hpte(PowerPCCPU *cpu,
> -                           target_ulong pte_index,
> -                           target_ulong pte0, target_ulong pte1)
> +void ppc_hash64_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
> +                           uint64_t pte0, uint64_t pte1)
>  {
>      CPUPPCState *env = &cpu->env;
> +    hwaddr offset = ptex * HASH_PTE_SIZE_64;
>  
>      if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> -        kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
> +        kvmppc_hash64_write_pte(ptex, pte0, pte1);
>          return;
>      }
>  
> -    pte_index *= HASH_PTE_SIZE_64;
>      if (env->external_htab) {
> -        stq_p(env->external_htab + pte_index, pte0);
> -        stq_p(env->external_htab + pte_index + HASH_PTE_SIZE_64 / 2, pte1);
> +        stq_p(env->external_htab + offset, pte0);
> +        stq_p(env->external_htab + offset + HASH_PTE_SIZE_64 / 2, pte1);
>      } else {
> -        stq_phys(CPU(cpu)->as, env->htab_base + pte_index, pte0);
> +        stq_phys(CPU(cpu)->as, env->htab_base + offset, pte0);
>          stq_phys(CPU(cpu)->as,
> -                 env->htab_base + pte_index + HASH_PTE_SIZE_64 / 2, pte1);
> +                 env->htab_base + offset + HASH_PTE_SIZE_64 / 2, pte1);
>      }
>  }
>  
> -void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
> -                               target_ulong pte_index,
> +void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
>                                 target_ulong pte0, target_ulong pte1)
>  {
>      /*
> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> index 7a0b7fc..8637fe4 100644
> --- a/target/ppc/mmu-hash64.h
> +++ b/target/ppc/mmu-hash64.h
> @@ -10,8 +10,8 @@ int ppc_store_slb(PowerPCCPU *cpu, target_ulong slot,
>  hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr);
>  int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr address, int rw,
>                                  int mmu_idx);
> -void ppc_hash64_store_hpte(PowerPCCPU *cpu, target_ulong index,
> -                           target_ulong pte0, target_ulong pte1);
> +void ppc_hash64_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
> +                           uint64_t pte0, uint64_t pte1);
>  void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
>                                 target_ulong pte_index,
>                                 target_ulong pte0, target_ulong pte1);
> @@ -96,41 +96,27 @@ void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
>  void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift,
>                                   Error **errp);
>  
> -uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_index);
> -void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token);
> +struct ppc_hash_pte64 {
> +    uint64_t pte0, pte1;
> +};
> +
> +const ppc_hash_pte64_t *ppc_hash64_map_hptes(PowerPCCPU *cpu,\


You do not need the trailing '\'.


> +                                             hwaddr ptex, int n);
> +void ppc_hash64_unmap_hptes(PowerPCCPU *cpu, const ppc_hash_pte64_t *hptes,
> +                            hwaddr ptex, int n);
>  
> -static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
> -                                                 uint64_t token, int index)
> +static inline uint64_t ppc_hash64_hpte0(PowerPCCPU *cpu,
> +                                        const ppc_hash_pte64_t *hptes, int i)
>  {
> -    CPUPPCState *env = &cpu->env;
> -    uint64_t addr;
> -
> -    addr = token + (index * HASH_PTE_SIZE_64);
> -    if (env->external_htab) {
> -        return  ldq_p((const void *)(uintptr_t)addr);
> -    } else {
> -        return ldq_phys(CPU(cpu)->as, addr);
> -    }
> +    return ldq_p(&(hptes[i].pte0));
>  }
>  
> -static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU *cpu,
> -                                                 uint64_t token, int index)
> +static inline uint64_t ppc_hash64_hpte1(PowerPCCPU *cpu,
> +                                        const ppc_hash_pte64_t *hptes, int i)
>  {
> -    CPUPPCState *env = &cpu->env;
> -    uint64_t addr;
> -
> -    addr = token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2;
> -    if (env->external_htab) {
> -        return  ldq_p((const void *)(uintptr_t)addr);
> -    } else {
> -        return ldq_phys(CPU(cpu)->as, addr);
> -    }
> +    return ldq_p(&(hptes[i].pte1));
>  }
>  
> -typedef struct {
> -    uint64_t pte0, pte1;
> -} ppc_hash_pte64_t;
> -
>  #endif /* CONFIG_USER_ONLY */
>  
>  #endif /* MMU_HASH64_H */
>

On Thu, Feb 23, 2017 at 04:02:54PM +1100, Alexey Kardashevskiy wrote:
> On 23/02/17 13:09, David Gibson wrote:
> > Accesses to the hashed page table (HPT) are complicated by the fact that
> > the HPT could be in one of three places:
> >    1) Within guest memory - when we're emulating a full guest CPU at the
> >       hardware level (e.g. powernv, mac99, g3beige)
> >    2) Within qemu, but outside guest memory - when we're emulating user and
> >       supervisor instructions within TCG, but instead of emulating
> >       the CPU's hypervisor mode, we just emulate a hypervisor's behaviour
> >       (pseries in TCG)
> >    3) Within KVM - a pseries machine using KVM acceleration.  Mostly
> >       accesses to the HPT are handled by KVM, but there are a few cases
> >       where qemu needs to access it via a special fd for the purpose.
> > 
> > In order to batch accesses to the fd in case (3), we use a somewhat awkward
> > ppc_hash64_start_access() / ppc_hash64_stop_access() pair, which for case
> > (3) reads / releases a whole PTEG from the kernel.  For cases (1) & (2)
> > it just returns an address value.  The actual HPTE load helpers then need
> > to interpret the returned token differently in the 3 cases.
> > 
> > This patch keeps the same basic structure, but simplfiies the details.
> > First start_access() / stop_access() are renamed to get_pteg() and
> > put_pteg() to make their operation more obvious.
> 
> 
> They are renamed to map_hptes/unmap_hptes actually.

Oops.  Adjusted.

> 
> 
> > Second, read_pteg() now
> > always returns a qemu pointer, which can always be used in the same way
> > by the load_hpte() helpers.  In case (1) it comes from address_space_map()
> > in case (2) directly from qemu's HPT buffer and in case (3) from a
> > temporary buffer read from the KVM fd.
> > 
> > While we're at it, make things a bit more consistent in terms of types and
> > variable names: avoid variables named 'index' (it shadows index(3) which
> > can lead to confusing results), use 'hwaddr ptex' for HPTE indices and
> > uint64_t for each of the HPTE words, use ptex throughout the call stack
> > instead of pte_offset in some places (we still need that at the bottom
> > layer, but nowhere else).
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_hcall.c    | 36 +++++++++---------
> >  target/ppc/cpu.h        |  3 +-
> >  target/ppc/kvm.c        | 25 ++++++-------
> >  target/ppc/kvm_ppc.h    | 43 ++++++++++------------
> >  target/ppc/mmu-hash64.c | 98 ++++++++++++++++++++++++++-----------------------
> >  target/ppc/mmu-hash64.h | 46 ++++++++---------------
> >  6 files changed, 119 insertions(+), 132 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 3298a14..fd961b5 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -84,7 +84,7 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >      unsigned apshift;
> >      target_ulong raddr;
> >      target_ulong slot;
> > -    uint64_t token;
> > +    const ppc_hash_pte64_t *hptes;
> >  
> >      apshift = ppc_hash64_hpte_page_shift_noslb(cpu, pteh, ptel);
> >      if (!apshift) {
> > @@ -123,23 +123,23 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >      ptex = ptex & ~7ULL;
> >  
> >      if (likely((flags & H_EXACT) == 0)) {
> > -        token = ppc_hash64_start_access(cpu, ptex);
> > +        hptes = ppc_hash64_map_hptes(cpu, ptex, HPTES_PER_GROUP);
> >          for (slot = 0; slot < 8; slot++) {
> > -            if (!(ppc_hash64_load_hpte0(cpu, token, slot) & HPTE64_V_VALID)) {
> > +            if (!(ppc_hash64_hpte0(cpu, hptes, slot) & HPTE64_V_VALID)) {
> >                  break;
> >              }
> >          }
> > -        ppc_hash64_stop_access(cpu, token);
> > +        ppc_hash64_unmap_hptes(cpu, hptes, ptex, HPTES_PER_GROUP);
> >          if (slot == 8) {
> >              return H_PTEG_FULL;
> >          }
> >      } else {
> > -        token = ppc_hash64_start_access(cpu, ptex);
> > -        if (ppc_hash64_load_hpte0(cpu, token, 0) & HPTE64_V_VALID) {
> > -            ppc_hash64_stop_access(cpu, token);
> > +        hptes = ppc_hash64_map_hptes(cpu, ptex + slot, 1);
> > +        if (ppc_hash64_hpte0(cpu, hptes, 0) & HPTE64_V_VALID) {
> > +            ppc_hash64_unmap_hptes(cpu, hptes, ptex + slot, 1);
> >              return H_PTEG_FULL;
> >          }
> > -        ppc_hash64_stop_access(cpu, token);
> > +        ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
> >      }
> >  
> >      ppc_hash64_store_hpte(cpu, ptex + slot, pteh | HPTE64_V_HPTE_DIRTY, ptel);
> > @@ -160,17 +160,17 @@ static RemoveResult remove_hpte(PowerPCCPU *cpu, target_ulong ptex,
> >                                  target_ulong flags,
> >                                  target_ulong *vp, target_ulong *rp)
> >  {
> > -    uint64_t token;
> > +    const ppc_hash_pte64_t *hptes;
> >      target_ulong v, r;
> >  
> >      if (!valid_ptex(cpu, ptex)) {
> >          return REMOVE_PARM;
> >      }
> >  
> > -    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(cpu, token);
> > +    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
> > +    v = ppc_hash64_hpte0(cpu, hptes, 0);
> > +    r = ppc_hash64_hpte1(cpu, hptes, 0);
> > +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
> >  
> >      if ((v & HPTE64_V_VALID) == 0 ||
> >          ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> > @@ -291,17 +291,17 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >      target_ulong flags = args[0];
> >      target_ulong ptex = args[1];
> >      target_ulong avpn = args[2];
> > -    uint64_t token;
> > +    const ppc_hash_pte64_t *hptes;
> >      target_ulong v, r;
> >  
> >      if (!valid_ptex(cpu, ptex)) {
> >          return H_PARAMETER;
> >      }
> >  
> > -    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(cpu, token);
> > +    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
> > +    v = ppc_hash64_hpte0(cpu, hptes, 0);
> > +    r = ppc_hash64_hpte1(cpu, hptes, 0);
> > +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
> >  
> >      if ((v & HPTE64_V_VALID) == 0 ||
> >          ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index f99bcae..c89973e 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -223,11 +223,12 @@ enum {
> >  typedef struct opc_handler_t opc_handler_t;
> >  
> >  /*****************************************************************************/
> > -/* Types used to describe some PowerPC registers */
> > +/* Types used to describe some PowerPC registers etc. */
> >  typedef struct DisasContext DisasContext;
> >  typedef struct ppc_spr_t ppc_spr_t;
> >  typedef union ppc_avr_t ppc_avr_t;
> >  typedef union ppc_tlb_t ppc_tlb_t;
> > +typedef struct ppc_hash_pte64 ppc_hash_pte64_t;
> >  
> >  /* SPR access micro-ops generations callbacks */
> >  struct ppc_spr_t {
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 52bbea5..9d3e57e 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -2601,17 +2601,17 @@ struct kvm_get_htab_buf {
> >      /*
> >       * We require one extra byte for read
> >       */
> > -    target_ulong hpte[(HPTES_PER_GROUP * 2) + 1];
> > +    ppc_hash_pte64_t hpte[HPTES_PER_GROUP];
> 
> 
> The "one extra byte" (which was ulong) is not needed any more why?

Ah.. good question.  A better one is why was it needed in the first
place, to which I don't have a good answer :/.

> 
> 
> >  };
> >  
> > -uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong pte_index)
> > +const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex, int n)
> 
> 
> This "int n" is ignored here by a reason?

Uh.. only by reason that I forgot to get around to handling that
properly.  Actually.. I rather suspect there are several things broken
with this function in any case (and equally it's predecessor) - I
think it will return bogus things if there are any invalid HPTEs in
the region read.

I think we're getting away with it because this interface is never
actually used: if we have KVM then we don't ever enter the MMU code
paths that could call this.  I should probably just change the KVM
mode hooks to abort()s.
> 
> 
> >  {
> >      int htab_fd;
> >      struct kvm_get_htab_fd ghf;
> > -    struct kvm_get_htab_buf  *hpte_buf;
> > +    struct kvm_get_htab_buf *hpte_buf;
> >  
> >      ghf.flags = 0;
> > -    ghf.start_index = pte_index;
> > +    ghf.start_index = ptex;
> >      htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
> >      if (htab_fd < 0) {
> >          goto error_out;
> > @@ -2626,7 +2626,7 @@ uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong pte_index)
> >      }
> >  
> >      close(htab_fd);
> > -    return (uint64_t)(uintptr_t) hpte_buf->hpte;
> > +    return hpte_buf->hpte;
> >  
> >  out_close:
> >      g_free(hpte_buf);
> > @@ -2635,18 +2635,15 @@ error_out:
> >      return 0;
> >  }
> >  
> > -void kvmppc_hash64_free_pteg(uint64_t token)
> > +void kvmppc_unmap_hptes(const ppc_hash_pte64_t *hptes, hwaddr ptex, int n)
> >  {
> >      struct kvm_get_htab_buf *htab_buf;
> >  
> > -    htab_buf = container_of((void *)(uintptr_t) token, struct kvm_get_htab_buf,
> > -                            hpte);
> > +    htab_buf = container_of((void *)hptes, struct kvm_get_htab_buf, hpte);
> >      g_free(htab_buf);
> > -    return;
> >  }
> >  
> > -void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
> > -                             target_ulong pte0, target_ulong pte1)
> > +void kvmppc_hash64_write_pte(hwaddr ptex, uint64_t pte0, uint64_t pte1)
> >  {
> >      int htab_fd;
> >      struct kvm_get_htab_fd ghf;
> > @@ -2661,9 +2658,9 @@ void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
> >  
> >      hpte_buf.header.n_valid = 1;
> >      hpte_buf.header.n_invalid = 0;
> > -    hpte_buf.header.index = pte_index;
> > -    hpte_buf.hpte[0] = pte0;
> > -    hpte_buf.hpte[1] = pte1;
> > +    hpte_buf.header.index = ptex;
> > +    hpte_buf.hpte[0].pte0 = pte0;
> > +    hpte_buf.hpte[0].pte1 = pte1;
> >      /*
> >       * Write the hpte entry.
> >       * CAUTION: write() has the warn_unused_result attribute. Hence we
> > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> > index 8da2ee4..3f8fccd 100644
> > --- a/target/ppc/kvm_ppc.h
> > +++ b/target/ppc/kvm_ppc.h
> > @@ -41,6 +41,10 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int *pfd,
> >  int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t window_size);
> >  int kvmppc_reset_htab(int shift_hint);
> >  uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
> > +const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex, int n);
> > +void kvmppc_unmap_hptes(const ppc_hash_pte64_t *hptes, hwaddr ptex, int n);
> > +
> > +void kvmppc_hash64_write_pte(hwaddr ptex, uint64_t pte0, uint64_t pte1);
> >  #endif /* !CONFIG_USER_ONLY */
> >  bool kvmppc_has_cap_epr(void);
> >  int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function);
> > @@ -49,11 +53,6 @@ int kvmppc_get_htab_fd(bool write);
> >  int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
> >  int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
> >                             uint16_t n_valid, uint16_t n_invalid);
> > -uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong pte_index);
> > -void kvmppc_hash64_free_pteg(uint64_t token);
> > -
> > -void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
> > -                             target_ulong pte0, target_ulong pte1);
> >  bool kvmppc_has_cap_fixup_hcalls(void);
> >  bool kvmppc_has_cap_htm(void);
> >  int kvmppc_enable_hwrng(void);
> > @@ -199,6 +198,22 @@ static inline bool kvmppc_is_mem_backend_page_size_ok(char *obj_path)
> >      return true;
> >  }
> >  
> > +static inline const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex, int n)
> > +{
> > +    abort();
> > +}
> > +
> > +static inline void kvmppc_unmap_hptes(const ppc_hash_pte64_t *hptes,
> > +                                      hwaddr ptex, int n)
> > +{
> > +    abort();
> > +}
> > +
> > +static inline void kvmppc_hash64_write_pte(hwaddr ptex,
> > +                                           uint64_t pte0, uint64_t pte1)
> > +{
> > +    abort();
> > +}
> >  #endif /* !CONFIG_USER_ONLY */
> >  
> >  static inline bool kvmppc_has_cap_epr(void)
> > @@ -234,24 +249,6 @@ static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
> >      abort();
> >  }
> >  
> > -static inline uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu,
> > -                                               target_ulong pte_index)
> > -{
> > -    abort();
> > -}
> > -
> > -static inline void kvmppc_hash64_free_pteg(uint64_t token)
> > -{
> > -    abort();
> > -}
> > -
> > -static inline void kvmppc_hash64_write_pte(CPUPPCState *env,
> > -                                           target_ulong pte_index,
> > -                                           target_ulong pte0, target_ulong pte1)
> > -{
> > -    abort();
> > -}
> > -
> >  static inline bool kvmppc_has_cap_fixup_hcalls(void)
> >  {
> >      abort();
> > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > index 76669ed..c59db47 100644
> > --- a/target/ppc/mmu-hash64.c
> > +++ b/target/ppc/mmu-hash64.c
> > @@ -27,6 +27,7 @@
> >  #include "kvm_ppc.h"
> >  #include "mmu-hash64.h"
> >  #include "exec/log.h"
> > +#include "hw/hw.h"
> >  
> >  //#define DEBUG_SLB
> >  
> > @@ -431,33 +432,42 @@ static int ppc_hash64_amr_prot(PowerPCCPU *cpu, ppc_hash_pte64_t pte)
> >      return prot;
> >  }
> >  
> > -uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_index)
> > +const ppc_hash_pte64_t *ppc_hash64_map_hptes(PowerPCCPU *cpu,
> > +                                             hwaddr ptex, int n)
> >  {
> > -    uint64_t token = 0;
> > -    hwaddr pte_offset;
> > +    const ppc_hash_pte64_t *hptes = NULL;
> > +    hwaddr pte_offset = ptex * HASH_PTE_SIZE_64;
> >  
> > -    pte_offset = pte_index * HASH_PTE_SIZE_64;
> >      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);
> > +        hptes = kvmppc_map_hptes(ptex, n);
> >      } else if (cpu->env.external_htab) {
> >          /*
> >           * HTAB is controlled by QEMU. Just point to the internally
> >           * accessible PTEG.
> >           */
> > -        token = (uint64_t)(uintptr_t) cpu->env.external_htab + pte_offset;
> > +        hptes = (ppc_hash_pte64_t *)(cpu->env.external_htab + pte_offset);
> >      } else if (cpu->env.htab_base) {
> > -        token = cpu->env.htab_base + pte_offset;
> > +        hwaddr plen = n * HASH_PTE_SIZE_64;
> > +        hptes = address_space_map(CPU(cpu)->as, cpu->env.htab_base + pte_offset,
> > +                                 &plen, false);
> > +        if (plen < (n * HASH_PTE_SIZE_64)) {
> > +            hw_error("%s: Unable to map all requested HPTEs\n", __FUNCTION__);
> > +        }
> >      }
> > -    return token;
> > +    return hptes;
> >  }
> >  
> > -void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token)
> > +void ppc_hash64_unmap_hptes(PowerPCCPU *cpu, const ppc_hash_pte64_t *hptes,
> > +                            hwaddr ptex, int n)
> >  {
> >      if (cpu->env.external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> > -        kvmppc_hash64_free_pteg(token);
> > +        kvmppc_unmap_hptes(hptes, ptex, n);
> > +    } else if (!cpu->env.external_htab) {
> > +        address_space_unmap(CPU(cpu)->as, (void *)hptes, n * HASH_PTE_SIZE_64,
> > +                            false, n * HASH_PTE_SIZE_64);
> >      }
> >  }
> >  
> > @@ -505,18 +515,18 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
> >  {
> >      CPUPPCState *env = &cpu->env;
> >      int i;
> > -    uint64_t token;
> > +    const ppc_hash_pte64_t *pteg;
> >      target_ulong pte0, pte1;
> > -    target_ulong pte_index;
> > +    target_ulong ptex;
> >  
> > -    pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
> > -    token = ppc_hash64_start_access(cpu, pte_index);
> > -    if (!token) {
> > +    ptex = (hash & env->htab_mask) * HPTES_PER_GROUP;
> > +    pteg = ppc_hash64_map_hptes(cpu, ptex, HPTES_PER_GROUP);
> > +    if (!pteg) {
> >          return -1;
> >      }
> >      for (i = 0; i < HPTES_PER_GROUP; i++) {
> > -        pte0 = ppc_hash64_load_hpte0(cpu, token, i);
> > -        pte1 = ppc_hash64_load_hpte1(cpu, token, i);
> > +        pte0 = ppc_hash64_hpte0(cpu, pteg, i);
> > +        pte1 = ppc_hash64_hpte1(cpu, pteg, i);
> >  
> >          /* This compares V, B, H (secondary) and the AVPN */
> >          if (HPTE64_V_COMPARE(pte0, ptem)) {
> > @@ -536,11 +546,11 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
> >               */
> >              pte->pte0 = pte0;
> >              pte->pte1 = pte1;
> > -            ppc_hash64_stop_access(cpu, token);
> > -            return (pte_index + i) * HASH_PTE_SIZE_64;
> > +            ppc_hash64_unmap_hptes(cpu, pteg, ptex, HPTES_PER_GROUP);
> > +            return ptex + i;
> >          }
> >      }
> > -    ppc_hash64_stop_access(cpu, token);
> > +    ppc_hash64_unmap_hptes(cpu, pteg, ptex, HPTES_PER_GROUP);
> >      /*
> >       * We didn't find a valid entry.
> >       */
> > @@ -552,8 +562,7 @@ static hwaddr ppc_hash64_htab_lookup(PowerPCCPU *cpu,
> >                                       ppc_hash_pte64_t *pte, unsigned *pshift)
> >  {
> >      CPUPPCState *env = &cpu->env;
> > -    hwaddr pte_offset;
> > -    hwaddr hash;
> > +    hwaddr hash, ptex;
> >      uint64_t vsid, epnmask, epn, ptem;
> >      const struct ppc_one_seg_page_size *sps = slb->sps;
> >  
> > @@ -596,9 +605,9 @@ static hwaddr ppc_hash64_htab_lookup(PowerPCCPU *cpu,
> >              " vsid=" TARGET_FMT_lx " ptem=" TARGET_FMT_lx
> >              " hash=" TARGET_FMT_plx "\n",
> >              env->htab_base, env->htab_mask, vsid, ptem,  hash);
> > -    pte_offset = ppc_hash64_pteg_search(cpu, hash, sps, ptem, pte, pshift);
> > +    ptex = ppc_hash64_pteg_search(cpu, hash, sps, ptem, pte, pshift);
> >  
> > -    if (pte_offset == -1) {
> > +    if (ptex == -1) {
> >          /* Secondary PTEG lookup */
> >          ptem |= HPTE64_V_SECONDARY;
> >          qemu_log_mask(CPU_LOG_MMU,
> > @@ -607,10 +616,10 @@ static hwaddr ppc_hash64_htab_lookup(PowerPCCPU *cpu,
> >                  " hash=" TARGET_FMT_plx "\n", env->htab_base,
> >                  env->htab_mask, vsid, ptem, ~hash);
> >  
> > -        pte_offset = ppc_hash64_pteg_search(cpu, ~hash, sps, ptem, pte, pshift);
> > +        ptex = ppc_hash64_pteg_search(cpu, ~hash, sps, ptem, pte, pshift);
> >      }
> >  
> > -    return pte_offset;
> > +    return ptex;
> >  }
> >  
> >  unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
> > @@ -708,7 +717,7 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
> >      CPUPPCState *env = &cpu->env;
> >      ppc_slb_t *slb;
> >      unsigned apshift;
> > -    hwaddr pte_offset;
> > +    hwaddr ptex;
> >      ppc_hash_pte64_t pte;
> >      int pp_prot, amr_prot, prot;
> >      uint64_t new_pte1, dsisr;
> > @@ -792,8 +801,8 @@ skip_slb_search:
> >      }
> >  
> >      /* 4. Locate the PTE in the hash table */
> > -    pte_offset = ppc_hash64_htab_lookup(cpu, slb, eaddr, &pte, &apshift);
> > -    if (pte_offset == -1) {
> > +    ptex = ppc_hash64_htab_lookup(cpu, slb, eaddr, &pte, &apshift);
> > +    if (ptex == -1) {
> >          dsisr = 0x40000000;
> >          if (rwx == 2) {
> >              ppc_hash64_set_isi(cs, env, dsisr);
> > @@ -806,7 +815,7 @@ skip_slb_search:
> >          return 1;
> >      }
> >      qemu_log_mask(CPU_LOG_MMU,
> > -                "found PTE at offset %08" HWADDR_PRIx "\n", pte_offset);
> > +                  "found PTE at index %08" HWADDR_PRIx "\n", ptex);
> >  
> >      /* 5. Check access permissions */
> >  
> > @@ -849,8 +858,7 @@ skip_slb_search:
> >      }
> >  
> >      if (new_pte1 != pte.pte1) {
> > -        ppc_hash64_store_hpte(cpu, pte_offset / HASH_PTE_SIZE_64,
> > -                              pte.pte0, new_pte1);
> > +        ppc_hash64_store_hpte(cpu, ptex, pte.pte0, new_pte1);
> >      }
> >  
> >      /* 7. Determine the real address from the PTE */
> > @@ -867,7 +875,7 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
> >  {
> >      CPUPPCState *env = &cpu->env;
> >      ppc_slb_t *slb;
> > -    hwaddr pte_offset, raddr;
> > +    hwaddr ptex, raddr;
> >      ppc_hash_pte64_t pte;
> >      unsigned apshift;
> >  
> > @@ -900,8 +908,8 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
> >          }
> >      }
> >  
> > -    pte_offset = ppc_hash64_htab_lookup(cpu, slb, addr, &pte, &apshift);
> > -    if (pte_offset == -1) {
> > +    ptex = ppc_hash64_htab_lookup(cpu, slb, addr, &pte, &apshift);
> > +    if (ptex == -1) {
> >          return -1;
> >      }
> >  
> > @@ -909,30 +917,28 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
> >          & TARGET_PAGE_MASK;
> >  }
> >  
> > -void ppc_hash64_store_hpte(PowerPCCPU *cpu,
> > -                           target_ulong pte_index,
> > -                           target_ulong pte0, target_ulong pte1)
> > +void ppc_hash64_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
> > +                           uint64_t pte0, uint64_t pte1)
> >  {
> >      CPUPPCState *env = &cpu->env;
> > +    hwaddr offset = ptex * HASH_PTE_SIZE_64;
> >  
> >      if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> > -        kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
> > +        kvmppc_hash64_write_pte(ptex, pte0, pte1);
> >          return;
> >      }
> >  
> > -    pte_index *= HASH_PTE_SIZE_64;
> >      if (env->external_htab) {
> > -        stq_p(env->external_htab + pte_index, pte0);
> > -        stq_p(env->external_htab + pte_index + HASH_PTE_SIZE_64 / 2, pte1);
> > +        stq_p(env->external_htab + offset, pte0);
> > +        stq_p(env->external_htab + offset + HASH_PTE_SIZE_64 / 2, pte1);
> >      } else {
> > -        stq_phys(CPU(cpu)->as, env->htab_base + pte_index, pte0);
> > +        stq_phys(CPU(cpu)->as, env->htab_base + offset, pte0);
> >          stq_phys(CPU(cpu)->as,
> > -                 env->htab_base + pte_index + HASH_PTE_SIZE_64 / 2, pte1);
> > +                 env->htab_base + offset + HASH_PTE_SIZE_64 / 2, pte1);
> >      }
> >  }
> >  
> > -void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
> > -                               target_ulong pte_index,
> > +void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
> >                                 target_ulong pte0, target_ulong pte1)
> >  {
> >      /*
> > diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> > index 7a0b7fc..8637fe4 100644
> > --- a/target/ppc/mmu-hash64.h
> > +++ b/target/ppc/mmu-hash64.h
> > @@ -10,8 +10,8 @@ int ppc_store_slb(PowerPCCPU *cpu, target_ulong slot,
> >  hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr);
> >  int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr address, int rw,
> >                                  int mmu_idx);
> > -void ppc_hash64_store_hpte(PowerPCCPU *cpu, target_ulong index,
> > -                           target_ulong pte0, target_ulong pte1);
> > +void ppc_hash64_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
> > +                           uint64_t pte0, uint64_t pte1);
> >  void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
> >                                 target_ulong pte_index,
> >                                 target_ulong pte0, target_ulong pte1);
> > @@ -96,41 +96,27 @@ void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
> >  void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift,
> >                                   Error **errp);
> >  
> > -uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_index);
> > -void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token);
> > +struct ppc_hash_pte64 {
> > +    uint64_t pte0, pte1;
> > +};
> > +
> > +const ppc_hash_pte64_t *ppc_hash64_map_hptes(PowerPCCPU *cpu,\
> 
> 
> You do not need the trailing '\'.
> 
> 
> > +                                             hwaddr ptex, int n);
> > +void ppc_hash64_unmap_hptes(PowerPCCPU *cpu, const ppc_hash_pte64_t *hptes,
> > +                            hwaddr ptex, int n);
> >  
> > -static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
> > -                                                 uint64_t token, int index)
> > +static inline uint64_t ppc_hash64_hpte0(PowerPCCPU *cpu,
> > +                                        const ppc_hash_pte64_t *hptes, int i)
> >  {
> > -    CPUPPCState *env = &cpu->env;
> > -    uint64_t addr;
> > -
> > -    addr = token + (index * HASH_PTE_SIZE_64);
> > -    if (env->external_htab) {
> > -        return  ldq_p((const void *)(uintptr_t)addr);
> > -    } else {
> > -        return ldq_phys(CPU(cpu)->as, addr);
> > -    }
> > +    return ldq_p(&(hptes[i].pte0));
> >  }
> >  
> > -static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU *cpu,
> > -                                                 uint64_t token, int index)
> > +static inline uint64_t ppc_hash64_hpte1(PowerPCCPU *cpu,
> > +                                        const ppc_hash_pte64_t *hptes, int i)
> >  {
> > -    CPUPPCState *env = &cpu->env;
> > -    uint64_t addr;
> > -
> > -    addr = token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2;
> > -    if (env->external_htab) {
> > -        return  ldq_p((const void *)(uintptr_t)addr);
> > -    } else {
> > -        return ldq_phys(CPU(cpu)->as, addr);
> > -    }
> > +    return ldq_p(&(hptes[i].pte1));
> >  }
> >  
> > -typedef struct {
> > -    uint64_t pte0, pte1;
> > -} ppc_hash_pte64_t;
> > -
> >  #endif /* CONFIG_USER_ONLY */
> >  
> >  #endif /* MMU_HASH64_H */
> > 
> 
>

On Thu, 2017-02-23 at 13:09 +1100, David Gibson wrote:
> Accesses to the hashed page table (HPT) are complicated by the fact
> that
> the HPT could be in one of three places:
>    1) Within guest memory - when we're emulating a full guest CPU at
> the
>       hardware level (e.g. powernv, mac99, g3beige)
>    2) Within qemu, but outside guest memory - when we're emulating
> user and
>       supervisor instructions within TCG, but instead of emulating
>       the CPU's hypervisor mode, we just emulate a hypervisor's
> behaviour
>       (pseries in TCG)
>    3) Within KVM - a pseries machine using KVM acceleration.  Mostly
>       accesses to the HPT are handled by KVM, but there are a few
> cases
>       where qemu needs to access it via a special fd for the purpose.

Should you clarify that this is the case for KVM-HV with KVM-PR the
same as (2)?

> 
> In order to batch accesses to the fd in case (3), we use a somewhat
> awkward
> ppc_hash64_start_access() / ppc_hash64_stop_access() pair, which for
> case
> (3) reads / releases a whole PTEG from the kernel.  For cases (1) &
> (2)
> it just returns an address value.  The actual HPTE load helpers then
> need
> to interpret the returned token differently in the 3 cases.
> 
> This patch keeps the same basic structure, but simplfiies the
> details.
> First start_access() / stop_access() are renamed to get_pteg() and
> put_pteg() to make their operation more obvious.  Second, read_pteg()

Here you say they've been renamed to get/put/read_pteg, but in the code
they're called map/unmap_hptes and it looks like map_hptes does both
the get and the read?

> now
> always returns a qemu pointer, which can always be used in the same
> way
> by the load_hpte() helpers.  In case (1) it comes from
> address_space_map()
> in case (2) directly from qemu's HPT buffer and in case (3) from a
> temporary buffer read from the KVM fd.
> 
> While we're at it, make things a bit more consistent in terms of
> types and
> variable names: avoid variables named 'index' (it shadows index(3)
> which
> can lead to confusing results), use 'hwaddr ptex' for HPTE indices
> and
> uint64_t for each of the HPTE words, use ptex throughout the call
> stack
> instead of pte_offset in some places (we still need that at the
> bottom
> layer, but nowhere else).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Other than the commit message:

Reviewed-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

> ---
>  hw/ppc/spapr_hcall.c    | 36 +++++++++---------
>  target/ppc/cpu.h        |  3 +-
>  target/ppc/kvm.c        | 25 ++++++-------
>  target/ppc/kvm_ppc.h    | 43 ++++++++++------------
>  target/ppc/mmu-hash64.c | 98 ++++++++++++++++++++++++++-------------
> ----------
>  target/ppc/mmu-hash64.h | 46 ++++++++---------------
>  6 files changed, 119 insertions(+), 132 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 3298a14..fd961b5 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -84,7 +84,7 @@ static target_ulong h_enter(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
>      unsigned apshift;
>      target_ulong raddr;
>      target_ulong slot;
> -    uint64_t token;
> +    const ppc_hash_pte64_t *hptes;
>  
>      apshift = ppc_hash64_hpte_page_shift_noslb(cpu, pteh, ptel);
>      if (!apshift) {
> @@ -123,23 +123,23 @@ static target_ulong h_enter(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
>      ptex = ptex & ~7ULL;
>  
>      if (likely((flags & H_EXACT) == 0)) {
> -        token = ppc_hash64_start_access(cpu, ptex);
> +        hptes = ppc_hash64_map_hptes(cpu, ptex, HPTES_PER_GROUP);
>          for (slot = 0; slot < 8; slot++) {
> -            if (!(ppc_hash64_load_hpte0(cpu, token, slot) &
> HPTE64_V_VALID)) {
> +            if (!(ppc_hash64_hpte0(cpu, hptes, slot) &
> HPTE64_V_VALID)) {
>                  break;
>              }
>          }
> -        ppc_hash64_stop_access(cpu, token);
> +        ppc_hash64_unmap_hptes(cpu, hptes, ptex, HPTES_PER_GROUP);
>          if (slot == 8) {
>              return H_PTEG_FULL;
>          }
>      } else {
> -        token = ppc_hash64_start_access(cpu, ptex);
> -        if (ppc_hash64_load_hpte0(cpu, token, 0) & HPTE64_V_VALID) {
> -            ppc_hash64_stop_access(cpu, token);
> +        hptes = ppc_hash64_map_hptes(cpu, ptex + slot, 1);
> +        if (ppc_hash64_hpte0(cpu, hptes, 0) & HPTE64_V_VALID) {
> +            ppc_hash64_unmap_hptes(cpu, hptes, ptex + slot, 1);
>              return H_PTEG_FULL;
>          }
> -        ppc_hash64_stop_access(cpu, token);
> +        ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
>      }
>  
>      ppc_hash64_store_hpte(cpu, ptex + slot, pteh |
> HPTE64_V_HPTE_DIRTY, ptel);
> @@ -160,17 +160,17 @@ static RemoveResult remove_hpte(PowerPCCPU
> *cpu, target_ulong ptex,
>                                  target_ulong flags,
>                                  target_ulong *vp, target_ulong *rp)
>  {
> -    uint64_t token;
> +    const ppc_hash_pte64_t *hptes;
>      target_ulong v, r;
>  
>      if (!valid_ptex(cpu, ptex)) {
>          return REMOVE_PARM;
>      }
>  
> -    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(cpu, token);
> +    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
> +    v = ppc_hash64_hpte0(cpu, hptes, 0);
> +    r = ppc_hash64_hpte1(cpu, hptes, 0);
> +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
>  
>      if ((v & HPTE64_V_VALID) == 0 ||
>          ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> @@ -291,17 +291,17 @@ static target_ulong h_protect(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
>      target_ulong flags = args[0];
>      target_ulong ptex = args[1];
>      target_ulong avpn = args[2];
> -    uint64_t token;
> +    const ppc_hash_pte64_t *hptes;
>      target_ulong v, r;
>  
>      if (!valid_ptex(cpu, ptex)) {
>          return H_PARAMETER;
>      }
>  
> -    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(cpu, token);
> +    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
> +    v = ppc_hash64_hpte0(cpu, hptes, 0);
> +    r = ppc_hash64_hpte1(cpu, hptes, 0);
> +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
>  
>      if ((v & HPTE64_V_VALID) == 0 ||
>          ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index f99bcae..c89973e 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -223,11 +223,12 @@ enum {
>  typedef struct opc_handler_t opc_handler_t;
>  
>  /*******************************************************************
> **********/
> -/* Types used to describe some PowerPC registers */
> +/* Types used to describe some PowerPC registers etc. */
>  typedef struct DisasContext DisasContext;
>  typedef struct ppc_spr_t ppc_spr_t;
>  typedef union ppc_avr_t ppc_avr_t;
>  typedef union ppc_tlb_t ppc_tlb_t;
> +typedef struct ppc_hash_pte64 ppc_hash_pte64_t;
>  
>  /* SPR access micro-ops generations callbacks */
>  struct ppc_spr_t {
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 52bbea5..9d3e57e 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2601,17 +2601,17 @@ struct kvm_get_htab_buf {
>      /*
>       * We require one extra byte for read
>       */
> -    target_ulong hpte[(HPTES_PER_GROUP * 2) + 1];
> +    ppc_hash_pte64_t hpte[HPTES_PER_GROUP];
>  };
>  
> -uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong
> pte_index)
> +const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex, int n)
>  {
>      int htab_fd;
>      struct kvm_get_htab_fd ghf;
> -    struct kvm_get_htab_buf  *hpte_buf;
> +    struct kvm_get_htab_buf *hpte_buf;
>  
>      ghf.flags = 0;
> -    ghf.start_index = pte_index;
> +    ghf.start_index = ptex;
>      htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
>      if (htab_fd < 0) {
>          goto error_out;
> @@ -2626,7 +2626,7 @@ uint64_t kvmppc_hash64_read_pteg(PowerPCCPU
> *cpu, target_ulong pte_index)
>      }
>  
>      close(htab_fd);
> -    return (uint64_t)(uintptr_t) hpte_buf->hpte;
> +    return hpte_buf->hpte;
>  
>  out_close:
>      g_free(hpte_buf);
> @@ -2635,18 +2635,15 @@ error_out:
>      return 0;
>  }
>  
> -void kvmppc_hash64_free_pteg(uint64_t token)
> +void kvmppc_unmap_hptes(const ppc_hash_pte64_t *hptes, hwaddr ptex,
> int n)
>  {
>      struct kvm_get_htab_buf *htab_buf;
>  
> -    htab_buf = container_of((void *)(uintptr_t) token, struct
> kvm_get_htab_buf,
> -                            hpte);
> +    htab_buf = container_of((void *)hptes, struct kvm_get_htab_buf,
> hpte);
>      g_free(htab_buf);
> -    return;
>  }
>  
> -void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong
> pte_index,
> -                             target_ulong pte0, target_ulong pte1)
> +void kvmppc_hash64_write_pte(hwaddr ptex, uint64_t pte0, uint64_t
> pte1)
>  {
>      int htab_fd;
>      struct kvm_get_htab_fd ghf;
> @@ -2661,9 +2658,9 @@ void kvmppc_hash64_write_pte(CPUPPCState *env,
> target_ulong pte_index,
>  
>      hpte_buf.header.n_valid = 1;
>      hpte_buf.header.n_invalid = 0;
> -    hpte_buf.header.index = pte_index;
> -    hpte_buf.hpte[0] = pte0;
> -    hpte_buf.hpte[1] = pte1;
> +    hpte_buf.header.index = ptex;
> +    hpte_buf.hpte[0].pte0 = pte0;
> +    hpte_buf.hpte[0].pte1 = pte1;
>      /*
>       * Write the hpte entry.
>       * CAUTION: write() has the warn_unused_result attribute. Hence
> we
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 8da2ee4..3f8fccd 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -41,6 +41,10 @@ void *kvmppc_create_spapr_tce(uint32_t liobn,
> uint32_t window_size, int *pfd,
>  int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t
> window_size);
>  int kvmppc_reset_htab(int shift_hint);
>  uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int
> hash_shift);
> +const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex, int n);
> +void kvmppc_unmap_hptes(const ppc_hash_pte64_t *hptes, hwaddr ptex,
> int n);
> +
> +void kvmppc_hash64_write_pte(hwaddr ptex, uint64_t pte0, uint64_t
> pte1);
>  #endif /* !CONFIG_USER_ONLY */
>  bool kvmppc_has_cap_epr(void);
>  int kvmppc_define_rtas_kernel_token(uint32_t token, const char
> *function);
> @@ -49,11 +53,6 @@ int kvmppc_get_htab_fd(bool write);
>  int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t
> max_ns);
>  int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>                             uint16_t n_valid, uint16_t n_invalid);
> -uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong
> pte_index);
> -void kvmppc_hash64_free_pteg(uint64_t token);
> -
> -void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong
> pte_index,
> -                             target_ulong pte0, target_ulong pte1);
>  bool kvmppc_has_cap_fixup_hcalls(void);
>  bool kvmppc_has_cap_htm(void);
>  int kvmppc_enable_hwrng(void);
> @@ -199,6 +198,22 @@ static inline bool
> kvmppc_is_mem_backend_page_size_ok(char *obj_path)
>      return true;
>  }
>  
> +static inline const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex,
> int n)
> +{
> +    abort();
> +}
> +
> +static inline void kvmppc_unmap_hptes(const ppc_hash_pte64_t *hptes,
> +                                      hwaddr ptex, int n)
> +{
> +    abort();
> +}
> +
> +static inline void kvmppc_hash64_write_pte(hwaddr ptex,
> +                                           uint64_t pte0, uint64_t
> pte1)
> +{
> +    abort();
> +}
>  #endif /* !CONFIG_USER_ONLY */
>  
>  static inline bool kvmppc_has_cap_epr(void)
> @@ -234,24 +249,6 @@ static inline int
> kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>      abort();
>  }
>  
> -static inline uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu,
> -                                               target_ulong
> pte_index)
> -{
> -    abort();
> -}
> -
> -static inline void kvmppc_hash64_free_pteg(uint64_t token)
> -{
> -    abort();
> -}
> -
> -static inline void kvmppc_hash64_write_pte(CPUPPCState *env,
> -                                           target_ulong pte_index,
> -                                           target_ulong pte0,
> target_ulong pte1)
> -{
> -    abort();
> -}
> -
>  static inline bool kvmppc_has_cap_fixup_hcalls(void)
>  {
>      abort();
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 76669ed..c59db47 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -27,6 +27,7 @@
>  #include "kvm_ppc.h"
>  #include "mmu-hash64.h"
>  #include "exec/log.h"
> +#include "hw/hw.h"
>  
>  //#define DEBUG_SLB
>  
> @@ -431,33 +432,42 @@ static int ppc_hash64_amr_prot(PowerPCCPU *cpu,
> ppc_hash_pte64_t pte)
>      return prot;
>  }
>  
> -uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong
> pte_index)
> +const ppc_hash_pte64_t *ppc_hash64_map_hptes(PowerPCCPU *cpu,
> +                                             hwaddr ptex, int n)
>  {
> -    uint64_t token = 0;
> -    hwaddr pte_offset;
> +    const ppc_hash_pte64_t *hptes = NULL;
> +    hwaddr pte_offset = ptex * HASH_PTE_SIZE_64;
>  
> -    pte_offset = pte_index * HASH_PTE_SIZE_64;
>      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);
> +        hptes = kvmppc_map_hptes(ptex, n);
>      } else if (cpu->env.external_htab) {
>          /*
>           * HTAB is controlled by QEMU. Just point to the internally
>           * accessible PTEG.
>           */
> -        token = (uint64_t)(uintptr_t) cpu->env.external_htab +
> pte_offset;
> +        hptes = (ppc_hash_pte64_t *)(cpu->env.external_htab +
> pte_offset);
>      } else if (cpu->env.htab_base) {
> -        token = cpu->env.htab_base + pte_offset;
> +        hwaddr plen = n * HASH_PTE_SIZE_64;
> +        hptes = address_space_map(CPU(cpu)->as, cpu->env.htab_base +
> pte_offset,
> +                                 &plen, false);
> +        if (plen < (n * HASH_PTE_SIZE_64)) {
> +            hw_error("%s: Unable to map all requested HPTEs\n",
> __FUNCTION__);
> +        }
>      }
> -    return token;
> +    return hptes;
>  }
>  
> -void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token)
> +void ppc_hash64_unmap_hptes(PowerPCCPU *cpu, const ppc_hash_pte64_t
> *hptes,
> +                            hwaddr ptex, int n)
>  {
>      if (cpu->env.external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> -        kvmppc_hash64_free_pteg(token);
> +        kvmppc_unmap_hptes(hptes, ptex, n);
> +    } else if (!cpu->env.external_htab) {
> +        address_space_unmap(CPU(cpu)->as, (void *)hptes, n *
> HASH_PTE_SIZE_64,
> +                            false, n * HASH_PTE_SIZE_64);
>      }
>  }
>  
> @@ -505,18 +515,18 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU
> *cpu, hwaddr hash,
>  {
>      CPUPPCState *env = &cpu->env;
>      int i;
> -    uint64_t token;
> +    const ppc_hash_pte64_t *pteg;
>      target_ulong pte0, pte1;
> -    target_ulong pte_index;
> +    target_ulong ptex;
>  
> -    pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
> -    token = ppc_hash64_start_access(cpu, pte_index);
> -    if (!token) {
> +    ptex = (hash & env->htab_mask) * HPTES_PER_GROUP;
> +    pteg = ppc_hash64_map_hptes(cpu, ptex, HPTES_PER_GROUP);
> +    if (!pteg) {
>          return -1;
>      }
>      for (i = 0; i < HPTES_PER_GROUP; i++) {
> -        pte0 = ppc_hash64_load_hpte0(cpu, token, i);
> -        pte1 = ppc_hash64_load_hpte1(cpu, token, i);
> +        pte0 = ppc_hash64_hpte0(cpu, pteg, i);
> +        pte1 = ppc_hash64_hpte1(cpu, pteg, i);
>  
>          /* This compares V, B, H (secondary) and the AVPN */
>          if (HPTE64_V_COMPARE(pte0, ptem)) {
> @@ -536,11 +546,11 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU
> *cpu, hwaddr hash,
>               */
>              pte->pte0 = pte0;
>              pte->pte1 = pte1;
> -            ppc_hash64_stop_access(cpu, token);
> -            return (pte_index + i) * HASH_PTE_SIZE_64;
> +            ppc_hash64_unmap_hptes(cpu, pteg, ptex,
> HPTES_PER_GROUP);
> +            return ptex + i;
>          }
>      }
> -    ppc_hash64_stop_access(cpu, token);
> +    ppc_hash64_unmap_hptes(cpu, pteg, ptex, HPTES_PER_GROUP);
>      /*
>       * We didn't find a valid entry.
>       */
> @@ -552,8 +562,7 @@ static hwaddr ppc_hash64_htab_lookup(PowerPCCPU
> *cpu,
>                                       ppc_hash_pte64_t *pte, unsigned
> *pshift)
>  {
>      CPUPPCState *env = &cpu->env;
> -    hwaddr pte_offset;
> -    hwaddr hash;
> +    hwaddr hash, ptex;
>      uint64_t vsid, epnmask, epn, ptem;
>      const struct ppc_one_seg_page_size *sps = slb->sps;
>  
> @@ -596,9 +605,9 @@ static hwaddr ppc_hash64_htab_lookup(PowerPCCPU
> *cpu,
>              " vsid=" TARGET_FMT_lx " ptem=" TARGET_FMT_lx
>              " hash=" TARGET_FMT_plx "\n",
>              env->htab_base, env->htab_mask, vsid, ptem,  hash);
> -    pte_offset = ppc_hash64_pteg_search(cpu, hash, sps, ptem, pte,
> pshift);
> +    ptex = ppc_hash64_pteg_search(cpu, hash, sps, ptem, pte,
> pshift);
>  
> -    if (pte_offset == -1) {
> +    if (ptex == -1) {
>          /* Secondary PTEG lookup */
>          ptem |= HPTE64_V_SECONDARY;
>          qemu_log_mask(CPU_LOG_MMU,
> @@ -607,10 +616,10 @@ static hwaddr ppc_hash64_htab_lookup(PowerPCCPU
> *cpu,
>                  " hash=" TARGET_FMT_plx "\n", env->htab_base,
>                  env->htab_mask, vsid, ptem, ~hash);
>  
> -        pte_offset = ppc_hash64_pteg_search(cpu, ~hash, sps, ptem,
> pte, pshift);
> +        ptex = ppc_hash64_pteg_search(cpu, ~hash, sps, ptem, pte,
> pshift);
>      }
>  
> -    return pte_offset;
> +    return ptex;
>  }
>  
>  unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
> @@ -708,7 +717,7 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu,
> vaddr eaddr,
>      CPUPPCState *env = &cpu->env;
>      ppc_slb_t *slb;
>      unsigned apshift;
> -    hwaddr pte_offset;
> +    hwaddr ptex;
>      ppc_hash_pte64_t pte;
>      int pp_prot, amr_prot, prot;
>      uint64_t new_pte1, dsisr;
> @@ -792,8 +801,8 @@ skip_slb_search:
>      }
>  
>      /* 4. Locate the PTE in the hash table */
> -    pte_offset = ppc_hash64_htab_lookup(cpu, slb, eaddr, &pte,
> &apshift);
> -    if (pte_offset == -1) {
> +    ptex = ppc_hash64_htab_lookup(cpu, slb, eaddr, &pte, &apshift);
> +    if (ptex == -1) {
>          dsisr = 0x40000000;
>          if (rwx == 2) {
>              ppc_hash64_set_isi(cs, env, dsisr);
> @@ -806,7 +815,7 @@ skip_slb_search:
>          return 1;
>      }
>      qemu_log_mask(CPU_LOG_MMU,
> -                "found PTE at offset %08" HWADDR_PRIx "\n",
> pte_offset);
> +                  "found PTE at index %08" HWADDR_PRIx "\n", ptex);
>  
>      /* 5. Check access permissions */
>  
> @@ -849,8 +858,7 @@ skip_slb_search:
>      }
>  
>      if (new_pte1 != pte.pte1) {
> -        ppc_hash64_store_hpte(cpu, pte_offset / HASH_PTE_SIZE_64,
> -                              pte.pte0, new_pte1);
> +        ppc_hash64_store_hpte(cpu, ptex, pte.pte0, new_pte1);
>      }
>  
>      /* 7. Determine the real address from the PTE */
> @@ -867,7 +875,7 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU
> *cpu, target_ulong addr)
>  {
>      CPUPPCState *env = &cpu->env;
>      ppc_slb_t *slb;
> -    hwaddr pte_offset, raddr;
> +    hwaddr ptex, raddr;
>      ppc_hash_pte64_t pte;
>      unsigned apshift;
>  
> @@ -900,8 +908,8 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU
> *cpu, target_ulong addr)
>          }
>      }
>  
> -    pte_offset = ppc_hash64_htab_lookup(cpu, slb, addr, &pte,
> &apshift);
> -    if (pte_offset == -1) {
> +    ptex = ppc_hash64_htab_lookup(cpu, slb, addr, &pte, &apshift);
> +    if (ptex == -1) {
>          return -1;
>      }
>  
> @@ -909,30 +917,28 @@ hwaddr
> ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
>          & TARGET_PAGE_MASK;
>  }
>  
> -void ppc_hash64_store_hpte(PowerPCCPU *cpu,
> -                           target_ulong pte_index,
> -                           target_ulong pte0, target_ulong pte1)
> +void ppc_hash64_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
> +                           uint64_t pte0, uint64_t pte1)
>  {
>      CPUPPCState *env = &cpu->env;
> +    hwaddr offset = ptex * HASH_PTE_SIZE_64;
>  
>      if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> -        kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
> +        kvmppc_hash64_write_pte(ptex, pte0, pte1);
>          return;
>      }
>  
> -    pte_index *= HASH_PTE_SIZE_64;
>      if (env->external_htab) {
> -        stq_p(env->external_htab + pte_index, pte0);
> -        stq_p(env->external_htab + pte_index + HASH_PTE_SIZE_64 / 2,
> pte1);
> +        stq_p(env->external_htab + offset, pte0);
> +        stq_p(env->external_htab + offset + HASH_PTE_SIZE_64 / 2,
> pte1);
>      } else {
> -        stq_phys(CPU(cpu)->as, env->htab_base + pte_index, pte0);
> +        stq_phys(CPU(cpu)->as, env->htab_base + offset, pte0);
>          stq_phys(CPU(cpu)->as,
> -                 env->htab_base + pte_index + HASH_PTE_SIZE_64 / 2,
> pte1);
> +                 env->htab_base + offset + HASH_PTE_SIZE_64 / 2,
> pte1);
>      }
>  }
>  
> -void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
> -                               target_ulong pte_index,
> +void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
>                                 target_ulong pte0, target_ulong pte1)
>  {
>      /*
> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> index 7a0b7fc..8637fe4 100644
> --- a/target/ppc/mmu-hash64.h
> +++ b/target/ppc/mmu-hash64.h
> @@ -10,8 +10,8 @@ int ppc_store_slb(PowerPCCPU *cpu, target_ulong
> slot,
>  hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong
> addr);
>  int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr address, int
> rw,
>                                  int mmu_idx);
> -void ppc_hash64_store_hpte(PowerPCCPU *cpu, target_ulong index,
> -                           target_ulong pte0, target_ulong pte1);
> +void ppc_hash64_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
> +                           uint64_t pte0, uint64_t pte1);
>  void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
>                                 target_ulong pte_index,
>                                 target_ulong pte0, target_ulong
> pte1);
> @@ -96,41 +96,27 @@ void ppc_hash64_set_sdr1(PowerPCCPU *cpu,
> target_ulong value,
>  void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int
> shift,
>                                   Error **errp);
>  
> -uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong
> pte_index);
> -void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token);
> +struct ppc_hash_pte64 {
> +    uint64_t pte0, pte1;
> +};
> +
> +const ppc_hash_pte64_t *ppc_hash64_map_hptes(PowerPCCPU *cpu,\
> +                                             hwaddr ptex, int n);
> +void ppc_hash64_unmap_hptes(PowerPCCPU *cpu, const ppc_hash_pte64_t
> *hptes,
> +                            hwaddr ptex, int n);
>  
> -static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
> -                                                 uint64_t token, int
> index)
> +static inline uint64_t ppc_hash64_hpte0(PowerPCCPU *cpu,
> +                                        const ppc_hash_pte64_t
> *hptes, int i)
>  {
> -    CPUPPCState *env = &cpu->env;
> -    uint64_t addr;
> -
> -    addr = token + (index * HASH_PTE_SIZE_64);
> -    if (env->external_htab) {
> -        return  ldq_p((const void *)(uintptr_t)addr);
> -    } else {
> -        return ldq_phys(CPU(cpu)->as, addr);
> -    }
> +    return ldq_p(&(hptes[i].pte0));
>  }
>  
> -static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU *cpu,
> -                                                 uint64_t token, int
> index)
> +static inline uint64_t ppc_hash64_hpte1(PowerPCCPU *cpu,
> +                                        const ppc_hash_pte64_t
> *hptes, int i)
>  {
> -    CPUPPCState *env = &cpu->env;
> -    uint64_t addr;
> -
> -    addr = token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2;
> -    if (env->external_htab) {
> -        return  ldq_p((const void *)(uintptr_t)addr);
> -    } else {
> -        return ldq_phys(CPU(cpu)->as, addr);
> -    }
> +    return ldq_p(&(hptes[i].pte1));
>  }
>  
> -typedef struct {
> -    uint64_t pte0, pte1;
> -} ppc_hash_pte64_t;
> -
>  #endif /* CONFIG_USER_ONLY */
>  
>  #endif /* MMU_HASH64_H */

On Thu, Feb 23, 2017 at 04:37:00PM +1100, Suraj Jitindar Singh wrote:
> On Thu, 2017-02-23 at 13:09 +1100, David Gibson wrote:
> > Accesses to the hashed page table (HPT) are complicated by the fact
> > that
> > the HPT could be in one of three places:
> >    1) Within guest memory - when we're emulating a full guest CPU at
> > the
> >       hardware level (e.g. powernv, mac99, g3beige)
> >    2) Within qemu, but outside guest memory - when we're emulating
> > user and
> >       supervisor instructions within TCG, but instead of emulating
> >       the CPU's hypervisor mode, we just emulate a hypervisor's
> > behaviour
> >       (pseries in TCG)
> >    3) Within KVM - a pseries machine using KVM acceleration.  Mostly
> >       accesses to the HPT are handled by KVM, but there are a few
> > cases
> >       where qemu needs to access it via a special fd for the purpose.
> 
> Should you clarify that this is the case for KVM-HV with KVM-PR the
> same as (2)?

Ah, good idea.

> 
> > 
> > In order to batch accesses to the fd in case (3), we use a somewhat
> > awkward
> > ppc_hash64_start_access() / ppc_hash64_stop_access() pair, which for
> > case
> > (3) reads / releases a whole PTEG from the kernel.  For cases (1) &
> > (2)
> > it just returns an address value.  The actual HPTE load helpers then
> > need
> > to interpret the returned token differently in the 3 cases.
> > 
> > This patch keeps the same basic structure, but simplfiies the
> > details.
> > First start_access() / stop_access() are renamed to get_pteg() and
> > put_pteg() to make their operation more obvious.  Second, read_pteg()
> 
> Here you say they've been renamed to get/put/read_pteg, but in the code
> they're called map/unmap_hptes and it looks like map_hptes does both
> the get and the read?

Oops, I'll update.

> 
> > now
> > always returns a qemu pointer, which can always be used in the same
> > way
> > by the load_hpte() helpers.  In case (1) it comes from
> > address_space_map()
> > in case (2) directly from qemu's HPT buffer and in case (3) from a
> > temporary buffer read from the KVM fd.
> > 
> > While we're at it, make things a bit more consistent in terms of
> > types and
> > variable names: avoid variables named 'index' (it shadows index(3)
> > which
> > can lead to confusing results), use 'hwaddr ptex' for HPTE indices
> > and
> > uint64_t for each of the HPTE words, use ptex throughout the call
> > stack
> > instead of pte_offset in some places (we still need that at the
> > bottom
> > layer, but nowhere else).
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Other than the commit message:
> 
> Reviewed-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> 
> > ---
> >  hw/ppc/spapr_hcall.c    | 36 +++++++++---------
> >  target/ppc/cpu.h        |  3 +-
> >  target/ppc/kvm.c        | 25 ++++++-------
> >  target/ppc/kvm_ppc.h    | 43 ++++++++++------------
> >  target/ppc/mmu-hash64.c | 98 ++++++++++++++++++++++++++-------------
> > ----------
> >  target/ppc/mmu-hash64.h | 46 ++++++++---------------
> >  6 files changed, 119 insertions(+), 132 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 3298a14..fd961b5 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -84,7 +84,7 @@ static target_ulong h_enter(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr,
> >      unsigned apshift;
> >      target_ulong raddr;
> >      target_ulong slot;
> > -    uint64_t token;
> > +    const ppc_hash_pte64_t *hptes;
> >  
> >      apshift = ppc_hash64_hpte_page_shift_noslb(cpu, pteh, ptel);
> >      if (!apshift) {
> > @@ -123,23 +123,23 @@ static target_ulong h_enter(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr,
> >      ptex = ptex & ~7ULL;
> >  
> >      if (likely((flags & H_EXACT) == 0)) {
> > -        token = ppc_hash64_start_access(cpu, ptex);
> > +        hptes = ppc_hash64_map_hptes(cpu, ptex, HPTES_PER_GROUP);
> >          for (slot = 0; slot < 8; slot++) {
> > -            if (!(ppc_hash64_load_hpte0(cpu, token, slot) &
> > HPTE64_V_VALID)) {
> > +            if (!(ppc_hash64_hpte0(cpu, hptes, slot) &
> > HPTE64_V_VALID)) {
> >                  break;
> >              }
> >          }
> > -        ppc_hash64_stop_access(cpu, token);
> > +        ppc_hash64_unmap_hptes(cpu, hptes, ptex, HPTES_PER_GROUP);
> >          if (slot == 8) {
> >              return H_PTEG_FULL;
> >          }
> >      } else {
> > -        token = ppc_hash64_start_access(cpu, ptex);
> > -        if (ppc_hash64_load_hpte0(cpu, token, 0) & HPTE64_V_VALID) {
> > -            ppc_hash64_stop_access(cpu, token);
> > +        hptes = ppc_hash64_map_hptes(cpu, ptex + slot, 1);
> > +        if (ppc_hash64_hpte0(cpu, hptes, 0) & HPTE64_V_VALID) {
> > +            ppc_hash64_unmap_hptes(cpu, hptes, ptex + slot, 1);
> >              return H_PTEG_FULL;
> >          }
> > -        ppc_hash64_stop_access(cpu, token);
> > +        ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
> >      }
> >  
> >      ppc_hash64_store_hpte(cpu, ptex + slot, pteh |
> > HPTE64_V_HPTE_DIRTY, ptel);
> > @@ -160,17 +160,17 @@ static RemoveResult remove_hpte(PowerPCCPU
> > *cpu, target_ulong ptex,
> >                                  target_ulong flags,
> >                                  target_ulong *vp, target_ulong *rp)
> >  {
> > -    uint64_t token;
> > +    const ppc_hash_pte64_t *hptes;
> >      target_ulong v, r;
> >  
> >      if (!valid_ptex(cpu, ptex)) {
> >          return REMOVE_PARM;
> >      }
> >  
> > -    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(cpu, token);
> > +    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
> > +    v = ppc_hash64_hpte0(cpu, hptes, 0);
> > +    r = ppc_hash64_hpte1(cpu, hptes, 0);
> > +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
> >  
> >      if ((v & HPTE64_V_VALID) == 0 ||
> >          ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> > @@ -291,17 +291,17 @@ static target_ulong h_protect(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr,
> >      target_ulong flags = args[0];
> >      target_ulong ptex = args[1];
> >      target_ulong avpn = args[2];
> > -    uint64_t token;
> > +    const ppc_hash_pte64_t *hptes;
> >      target_ulong v, r;
> >  
> >      if (!valid_ptex(cpu, ptex)) {
> >          return H_PARAMETER;
> >      }
> >  
> > -    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(cpu, token);
> > +    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
> > +    v = ppc_hash64_hpte0(cpu, hptes, 0);
> > +    r = ppc_hash64_hpte1(cpu, hptes, 0);
> > +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
> >  
> >      if ((v & HPTE64_V_VALID) == 0 ||
> >          ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index f99bcae..c89973e 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -223,11 +223,12 @@ enum {
> >  typedef struct opc_handler_t opc_handler_t;
> >  
> >  /*******************************************************************
> > **********/
> > -/* Types used to describe some PowerPC registers */
> > +/* Types used to describe some PowerPC registers etc. */
> >  typedef struct DisasContext DisasContext;
> >  typedef struct ppc_spr_t ppc_spr_t;
> >  typedef union ppc_avr_t ppc_avr_t;
> >  typedef union ppc_tlb_t ppc_tlb_t;
> > +typedef struct ppc_hash_pte64 ppc_hash_pte64_t;
> >  
> >  /* SPR access micro-ops generations callbacks */
> >  struct ppc_spr_t {
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 52bbea5..9d3e57e 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -2601,17 +2601,17 @@ struct kvm_get_htab_buf {
> >      /*
> >       * We require one extra byte for read
> >       */
> > -    target_ulong hpte[(HPTES_PER_GROUP * 2) + 1];
> > +    ppc_hash_pte64_t hpte[HPTES_PER_GROUP];
> >  };
> >  
> > -uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong
> > pte_index)
> > +const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex, int n)
> >  {
> >      int htab_fd;
> >      struct kvm_get_htab_fd ghf;
> > -    struct kvm_get_htab_buf  *hpte_buf;
> > +    struct kvm_get_htab_buf *hpte_buf;
> >  
> >      ghf.flags = 0;
> > -    ghf.start_index = pte_index;
> > +    ghf.start_index = ptex;
> >      htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
> >      if (htab_fd < 0) {
> >          goto error_out;
> > @@ -2626,7 +2626,7 @@ uint64_t kvmppc_hash64_read_pteg(PowerPCCPU
> > *cpu, target_ulong pte_index)
> >      }
> >  
> >      close(htab_fd);
> > -    return (uint64_t)(uintptr_t) hpte_buf->hpte;
> > +    return hpte_buf->hpte;
> >  
> >  out_close:
> >      g_free(hpte_buf);
> > @@ -2635,18 +2635,15 @@ error_out:
> >      return 0;
> >  }
> >  
> > -void kvmppc_hash64_free_pteg(uint64_t token)
> > +void kvmppc_unmap_hptes(const ppc_hash_pte64_t *hptes, hwaddr ptex,
> > int n)
> >  {
> >      struct kvm_get_htab_buf *htab_buf;
> >  
> > -    htab_buf = container_of((void *)(uintptr_t) token, struct
> > kvm_get_htab_buf,
> > -                            hpte);
> > +    htab_buf = container_of((void *)hptes, struct kvm_get_htab_buf,
> > hpte);
> >      g_free(htab_buf);
> > -    return;
> >  }
> >  
> > -void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong
> > pte_index,
> > -                             target_ulong pte0, target_ulong pte1)
> > +void kvmppc_hash64_write_pte(hwaddr ptex, uint64_t pte0, uint64_t
> > pte1)
> >  {
> >      int htab_fd;
> >      struct kvm_get_htab_fd ghf;
> > @@ -2661,9 +2658,9 @@ void kvmppc_hash64_write_pte(CPUPPCState *env,
> > target_ulong pte_index,
> >  
> >      hpte_buf.header.n_valid = 1;
> >      hpte_buf.header.n_invalid = 0;
> > -    hpte_buf.header.index = pte_index;
> > -    hpte_buf.hpte[0] = pte0;
> > -    hpte_buf.hpte[1] = pte1;
> > +    hpte_buf.header.index = ptex;
> > +    hpte_buf.hpte[0].pte0 = pte0;
> > +    hpte_buf.hpte[0].pte1 = pte1;
> >      /*
> >       * Write the hpte entry.
> >       * CAUTION: write() has the warn_unused_result attribute. Hence
> > we
> > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> > index 8da2ee4..3f8fccd 100644
> > --- a/target/ppc/kvm_ppc.h
> > +++ b/target/ppc/kvm_ppc.h
> > @@ -41,6 +41,10 @@ void *kvmppc_create_spapr_tce(uint32_t liobn,
> > uint32_t window_size, int *pfd,
> >  int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t
> > window_size);
> >  int kvmppc_reset_htab(int shift_hint);
> >  uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int
> > hash_shift);
> > +const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex, int n);
> > +void kvmppc_unmap_hptes(const ppc_hash_pte64_t *hptes, hwaddr ptex,
> > int n);
> > +
> > +void kvmppc_hash64_write_pte(hwaddr ptex, uint64_t pte0, uint64_t
> > pte1);
> >  #endif /* !CONFIG_USER_ONLY */
> >  bool kvmppc_has_cap_epr(void);
> >  int kvmppc_define_rtas_kernel_token(uint32_t token, const char
> > *function);
> > @@ -49,11 +53,6 @@ int kvmppc_get_htab_fd(bool write);
> >  int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t
> > max_ns);
> >  int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
> >                             uint16_t n_valid, uint16_t n_invalid);
> > -uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong
> > pte_index);
> > -void kvmppc_hash64_free_pteg(uint64_t token);
> > -
> > -void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong
> > pte_index,
> > -                             target_ulong pte0, target_ulong pte1);
> >  bool kvmppc_has_cap_fixup_hcalls(void);
> >  bool kvmppc_has_cap_htm(void);
> >  int kvmppc_enable_hwrng(void);
> > @@ -199,6 +198,22 @@ static inline bool
> > kvmppc_is_mem_backend_page_size_ok(char *obj_path)
> >      return true;
> >  }
> >  
> > +static inline const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex,
> > int n)
> > +{
> > +    abort();
> > +}
> > +
> > +static inline void kvmppc_unmap_hptes(const ppc_hash_pte64_t *hptes,
> > +                                      hwaddr ptex, int n)
> > +{
> > +    abort();
> > +}
> > +
> > +static inline void kvmppc_hash64_write_pte(hwaddr ptex,
> > +                                           uint64_t pte0, uint64_t
> > pte1)
> > +{
> > +    abort();
> > +}
> >  #endif /* !CONFIG_USER_ONLY */
> >  
> >  static inline bool kvmppc_has_cap_epr(void)
> > @@ -234,24 +249,6 @@ static inline int
> > kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
> >      abort();
> >  }
> >  
> > -static inline uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu,
> > -                                               target_ulong
> > pte_index)
> > -{
> > -    abort();
> > -}
> > -
> > -static inline void kvmppc_hash64_free_pteg(uint64_t token)
> > -{
> > -    abort();
> > -}
> > -
> > -static inline void kvmppc_hash64_write_pte(CPUPPCState *env,
> > -                                           target_ulong pte_index,
> > -                                           target_ulong pte0,
> > target_ulong pte1)
> > -{
> > -    abort();
> > -}
> > -
> >  static inline bool kvmppc_has_cap_fixup_hcalls(void)
> >  {
> >      abort();
> > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > index 76669ed..c59db47 100644
> > --- a/target/ppc/mmu-hash64.c
> > +++ b/target/ppc/mmu-hash64.c
> > @@ -27,6 +27,7 @@
> >  #include "kvm_ppc.h"
> >  #include "mmu-hash64.h"
> >  #include "exec/log.h"
> > +#include "hw/hw.h"
> >  
> >  //#define DEBUG_SLB
> >  
> > @@ -431,33 +432,42 @@ static int ppc_hash64_amr_prot(PowerPCCPU *cpu,
> > ppc_hash_pte64_t pte)
> >      return prot;
> >  }
> >  
> > -uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong
> > pte_index)
> > +const ppc_hash_pte64_t *ppc_hash64_map_hptes(PowerPCCPU *cpu,
> > +                                             hwaddr ptex, int n)
> >  {
> > -    uint64_t token = 0;
> > -    hwaddr pte_offset;
> > +    const ppc_hash_pte64_t *hptes = NULL;
> > +    hwaddr pte_offset = ptex * HASH_PTE_SIZE_64;
> >  
> > -    pte_offset = pte_index * HASH_PTE_SIZE_64;
> >      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);
> > +        hptes = kvmppc_map_hptes(ptex, n);
> >      } else if (cpu->env.external_htab) {
> >          /*
> >           * HTAB is controlled by QEMU. Just point to the internally
> >           * accessible PTEG.
> >           */
> > -        token = (uint64_t)(uintptr_t) cpu->env.external_htab +
> > pte_offset;
> > +        hptes = (ppc_hash_pte64_t *)(cpu->env.external_htab +
> > pte_offset);
> >      } else if (cpu->env.htab_base) {
> > -        token = cpu->env.htab_base + pte_offset;
> > +        hwaddr plen = n * HASH_PTE_SIZE_64;
> > +        hptes = address_space_map(CPU(cpu)->as, cpu->env.htab_base +
> > pte_offset,
> > +                                 &plen, false);
> > +        if (plen < (n * HASH_PTE_SIZE_64)) {
> > +            hw_error("%s: Unable to map all requested HPTEs\n",
> > __FUNCTION__);
> > +        }
> >      }
> > -    return token;
> > +    return hptes;
> >  }
> >  
> > -void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token)
> > +void ppc_hash64_unmap_hptes(PowerPCCPU *cpu, const ppc_hash_pte64_t
> > *hptes,
> > +                            hwaddr ptex, int n)
> >  {
> >      if (cpu->env.external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> > -        kvmppc_hash64_free_pteg(token);
> > +        kvmppc_unmap_hptes(hptes, ptex, n);
> > +    } else if (!cpu->env.external_htab) {
> > +        address_space_unmap(CPU(cpu)->as, (void *)hptes, n *
> > HASH_PTE_SIZE_64,
> > +                            false, n * HASH_PTE_SIZE_64);
> >      }
> >  }
> >  
> > @@ -505,18 +515,18 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU
> > *cpu, hwaddr hash,
> >  {
> >      CPUPPCState *env = &cpu->env;
> >      int i;
> > -    uint64_t token;
> > +    const ppc_hash_pte64_t *pteg;
> >      target_ulong pte0, pte1;
> > -    target_ulong pte_index;
> > +    target_ulong ptex;
> >  
> > -    pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
> > -    token = ppc_hash64_start_access(cpu, pte_index);
> > -    if (!token) {
> > +    ptex = (hash & env->htab_mask) * HPTES_PER_GROUP;
> > +    pteg = ppc_hash64_map_hptes(cpu, ptex, HPTES_PER_GROUP);
> > +    if (!pteg) {
> >          return -1;
> >      }
> >      for (i = 0; i < HPTES_PER_GROUP; i++) {
> > -        pte0 = ppc_hash64_load_hpte0(cpu, token, i);
> > -        pte1 = ppc_hash64_load_hpte1(cpu, token, i);
> > +        pte0 = ppc_hash64_hpte0(cpu, pteg, i);
> > +        pte1 = ppc_hash64_hpte1(cpu, pteg, i);
> >  
> >          /* This compares V, B, H (secondary) and the AVPN */
> >          if (HPTE64_V_COMPARE(pte0, ptem)) {
> > @@ -536,11 +546,11 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU
> > *cpu, hwaddr hash,
> >               */
> >              pte->pte0 = pte0;
> >              pte->pte1 = pte1;
> > -            ppc_hash64_stop_access(cpu, token);
> > -            return (pte_index + i) * HASH_PTE_SIZE_64;
> > +            ppc_hash64_unmap_hptes(cpu, pteg, ptex,
> > HPTES_PER_GROUP);
> > +            return ptex + i;
> >          }
> >      }
> > -    ppc_hash64_stop_access(cpu, token);
> > +    ppc_hash64_unmap_hptes(cpu, pteg, ptex, HPTES_PER_GROUP);
> >      /*
> >       * We didn't find a valid entry.
> >       */
> > @@ -552,8 +562,7 @@ static hwaddr ppc_hash64_htab_lookup(PowerPCCPU
> > *cpu,
> >                                       ppc_hash_pte64_t *pte, unsigned
> > *pshift)
> >  {
> >      CPUPPCState *env = &cpu->env;
> > -    hwaddr pte_offset;
> > -    hwaddr hash;
> > +    hwaddr hash, ptex;
> >      uint64_t vsid, epnmask, epn, ptem;
> >      const struct ppc_one_seg_page_size *sps = slb->sps;
> >  
> > @@ -596,9 +605,9 @@ static hwaddr ppc_hash64_htab_lookup(PowerPCCPU
> > *cpu,
> >              " vsid=" TARGET_FMT_lx " ptem=" TARGET_FMT_lx
> >              " hash=" TARGET_FMT_plx "\n",
> >              env->htab_base, env->htab_mask, vsid, ptem,  hash);
> > -    pte_offset = ppc_hash64_pteg_search(cpu, hash, sps, ptem, pte,
> > pshift);
> > +    ptex = ppc_hash64_pteg_search(cpu, hash, sps, ptem, pte,
> > pshift);
> >  
> > -    if (pte_offset == -1) {
> > +    if (ptex == -1) {
> >          /* Secondary PTEG lookup */
> >          ptem |= HPTE64_V_SECONDARY;
> >          qemu_log_mask(CPU_LOG_MMU,
> > @@ -607,10 +616,10 @@ static hwaddr ppc_hash64_htab_lookup(PowerPCCPU
> > *cpu,
> >                  " hash=" TARGET_FMT_plx "\n", env->htab_base,
> >                  env->htab_mask, vsid, ptem, ~hash);
> >  
> > -        pte_offset = ppc_hash64_pteg_search(cpu, ~hash, sps, ptem,
> > pte, pshift);
> > +        ptex = ppc_hash64_pteg_search(cpu, ~hash, sps, ptem, pte,
> > pshift);
> >      }
> >  
> > -    return pte_offset;
> > +    return ptex;
> >  }
> >  
> >  unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
> > @@ -708,7 +717,7 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu,
> > vaddr eaddr,
> >      CPUPPCState *env = &cpu->env;
> >      ppc_slb_t *slb;
> >      unsigned apshift;
> > -    hwaddr pte_offset;
> > +    hwaddr ptex;
> >      ppc_hash_pte64_t pte;
> >      int pp_prot, amr_prot, prot;
> >      uint64_t new_pte1, dsisr;
> > @@ -792,8 +801,8 @@ skip_slb_search:
> >      }
> >  
> >      /* 4. Locate the PTE in the hash table */
> > -    pte_offset = ppc_hash64_htab_lookup(cpu, slb, eaddr, &pte,
> > &apshift);
> > -    if (pte_offset == -1) {
> > +    ptex = ppc_hash64_htab_lookup(cpu, slb, eaddr, &pte, &apshift);
> > +    if (ptex == -1) {
> >          dsisr = 0x40000000;
> >          if (rwx == 2) {
> >              ppc_hash64_set_isi(cs, env, dsisr);
> > @@ -806,7 +815,7 @@ skip_slb_search:
> >          return 1;
> >      }
> >      qemu_log_mask(CPU_LOG_MMU,
> > -                "found PTE at offset %08" HWADDR_PRIx "\n",
> > pte_offset);
> > +                  "found PTE at index %08" HWADDR_PRIx "\n", ptex);
> >  
> >      /* 5. Check access permissions */
> >  
> > @@ -849,8 +858,7 @@ skip_slb_search:
> >      }
> >  
> >      if (new_pte1 != pte.pte1) {
> > -        ppc_hash64_store_hpte(cpu, pte_offset / HASH_PTE_SIZE_64,
> > -                              pte.pte0, new_pte1);
> > +        ppc_hash64_store_hpte(cpu, ptex, pte.pte0, new_pte1);
> >      }
> >  
> >      /* 7. Determine the real address from the PTE */
> > @@ -867,7 +875,7 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU
> > *cpu, target_ulong addr)
> >  {
> >      CPUPPCState *env = &cpu->env;
> >      ppc_slb_t *slb;
> > -    hwaddr pte_offset, raddr;
> > +    hwaddr ptex, raddr;
> >      ppc_hash_pte64_t pte;
> >      unsigned apshift;
> >  
> > @@ -900,8 +908,8 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU
> > *cpu, target_ulong addr)
> >          }
> >      }
> >  
> > -    pte_offset = ppc_hash64_htab_lookup(cpu, slb, addr, &pte,
> > &apshift);
> > -    if (pte_offset == -1) {
> > +    ptex = ppc_hash64_htab_lookup(cpu, slb, addr, &pte, &apshift);
> > +    if (ptex == -1) {
> >          return -1;
> >      }
> >  
> > @@ -909,30 +917,28 @@ hwaddr
> > ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
> >          & TARGET_PAGE_MASK;
> >  }
> >  
> > -void ppc_hash64_store_hpte(PowerPCCPU *cpu,
> > -                           target_ulong pte_index,
> > -                           target_ulong pte0, target_ulong pte1)
> > +void ppc_hash64_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
> > +                           uint64_t pte0, uint64_t pte1)
> >  {
> >      CPUPPCState *env = &cpu->env;
> > +    hwaddr offset = ptex * HASH_PTE_SIZE_64;
> >  
> >      if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> > -        kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
> > +        kvmppc_hash64_write_pte(ptex, pte0, pte1);
> >          return;
> >      }
> >  
> > -    pte_index *= HASH_PTE_SIZE_64;
> >      if (env->external_htab) {
> > -        stq_p(env->external_htab + pte_index, pte0);
> > -        stq_p(env->external_htab + pte_index + HASH_PTE_SIZE_64 / 2,
> > pte1);
> > +        stq_p(env->external_htab + offset, pte0);
> > +        stq_p(env->external_htab + offset + HASH_PTE_SIZE_64 / 2,
> > pte1);
> >      } else {
> > -        stq_phys(CPU(cpu)->as, env->htab_base + pte_index, pte0);
> > +        stq_phys(CPU(cpu)->as, env->htab_base + offset, pte0);
> >          stq_phys(CPU(cpu)->as,
> > -                 env->htab_base + pte_index + HASH_PTE_SIZE_64 / 2,
> > pte1);
> > +                 env->htab_base + offset + HASH_PTE_SIZE_64 / 2,
> > pte1);
> >      }
> >  }
> >  
> > -void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
> > -                               target_ulong pte_index,
> > +void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
> >                                 target_ulong pte0, target_ulong pte1)
> >  {
> >      /*
> > diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> > index 7a0b7fc..8637fe4 100644
> > --- a/target/ppc/mmu-hash64.h
> > +++ b/target/ppc/mmu-hash64.h
> > @@ -10,8 +10,8 @@ int ppc_store_slb(PowerPCCPU *cpu, target_ulong
> > slot,
> >  hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong
> > addr);
> >  int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr address, int
> > rw,
> >                                  int mmu_idx);
> > -void ppc_hash64_store_hpte(PowerPCCPU *cpu, target_ulong index,
> > -                           target_ulong pte0, target_ulong pte1);
> > +void ppc_hash64_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
> > +                           uint64_t pte0, uint64_t pte1);
> >  void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
> >                                 target_ulong pte_index,
> >                                 target_ulong pte0, target_ulong
> > pte1);
> > @@ -96,41 +96,27 @@ void ppc_hash64_set_sdr1(PowerPCCPU *cpu,
> > target_ulong value,
> >  void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int
> > shift,
> >                                   Error **errp);
> >  
> > -uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong
> > pte_index);
> > -void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token);
> > +struct ppc_hash_pte64 {
> > +    uint64_t pte0, pte1;
> > +};
> > +
> > +const ppc_hash_pte64_t *ppc_hash64_map_hptes(PowerPCCPU *cpu,\
> > +                                             hwaddr ptex, int n);
> > +void ppc_hash64_unmap_hptes(PowerPCCPU *cpu, const ppc_hash_pte64_t
> > *hptes,
> > +                            hwaddr ptex, int n);
> >  
> > -static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
> > -                                                 uint64_t token, int
> > index)
> > +static inline uint64_t ppc_hash64_hpte0(PowerPCCPU *cpu,
> > +                                        const ppc_hash_pte64_t
> > *hptes, int i)
> >  {
> > -    CPUPPCState *env = &cpu->env;
> > -    uint64_t addr;
> > -
> > -    addr = token + (index * HASH_PTE_SIZE_64);
> > -    if (env->external_htab) {
> > -        return  ldq_p((const void *)(uintptr_t)addr);
> > -    } else {
> > -        return ldq_phys(CPU(cpu)->as, addr);
> > -    }
> > +    return ldq_p(&(hptes[i].pte0));
> >  }
> >  
> > -static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU *cpu,
> > -                                                 uint64_t token, int
> > index)
> > +static inline uint64_t ppc_hash64_hpte1(PowerPCCPU *cpu,
> > +                                        const ppc_hash_pte64_t
> > *hptes, int i)
> >  {
> > -    CPUPPCState *env = &cpu->env;
> > -    uint64_t addr;
> > -
> > -    addr = token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2;
> > -    if (env->external_htab) {
> > -        return  ldq_p((const void *)(uintptr_t)addr);
> > -    } else {
> > -        return ldq_phys(CPU(cpu)->as, addr);
> > -    }
> > +    return ldq_p(&(hptes[i].pte1));
> >  }
> >  
> > -typedef struct {
> > -    uint64_t pte0, pte1;
> > -} ppc_hash_pte64_t;
> > -
> >  #endif /* CONFIG_USER_ONLY */
> >  
> >  #endif /* MMU_HASH64_H */
>

On Thu, Feb 23, 2017 at 04:02:54PM +1100, Alexey Kardashevskiy wrote:
> On 23/02/17 13:09, David Gibson wrote:
> > Accesses to the hashed page table (HPT) are complicated by the fact that
> > the HPT could be in one of three places:
> >    1) Within guest memory - when we're emulating a full guest CPU at the
> >       hardware level (e.g. powernv, mac99, g3beige)
> >    2) Within qemu, but outside guest memory - when we're emulating user and
> >       supervisor instructions within TCG, but instead of emulating
> >       the CPU's hypervisor mode, we just emulate a hypervisor's behaviour
> >       (pseries in TCG)
> >    3) Within KVM - a pseries machine using KVM acceleration.  Mostly
> >       accesses to the HPT are handled by KVM, but there are a few cases
> >       where qemu needs to access it via a special fd for the purpose.
> > 
> > In order to batch accesses to the fd in case (3), we use a somewhat awkward
> > ppc_hash64_start_access() / ppc_hash64_stop_access() pair, which for case
> > (3) reads / releases a whole PTEG from the kernel.  For cases (1) & (2)
> > it just returns an address value.  The actual HPTE load helpers then need
> > to interpret the returned token differently in the 3 cases.
> > 
> > This patch keeps the same basic structure, but simplfiies the details.
> > First start_access() / stop_access() are renamed to get_pteg() and
> > put_pteg() to make their operation more obvious.

[snip]
> >  /*****************************************************************************/
> > -/* Types used to describe some PowerPC registers */
> > +/* Types used to describe some PowerPC registers etc. */
> >  typedef struct DisasContext DisasContext;
> >  typedef struct ppc_spr_t ppc_spr_t;
> >  typedef union ppc_avr_t ppc_avr_t;
> >  typedef union ppc_tlb_t ppc_tlb_t;
> > +typedef struct ppc_hash_pte64 ppc_hash_pte64_t;
> >  
> >  /* SPR access micro-ops generations callbacks */
> >  struct ppc_spr_t {
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 52bbea5..9d3e57e 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -2601,17 +2601,17 @@ struct kvm_get_htab_buf {
> >      /*
> >       * We require one extra byte for read
> >       */
> > -    target_ulong hpte[(HPTES_PER_GROUP * 2) + 1];
> > +    ppc_hash_pte64_t hpte[HPTES_PER_GROUP];
> 
> 
> The "one extra byte" (which was ulong) is not needed any more why?
> 
> 
> >  };
> >  
> > -uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong pte_index)
> > +const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex, int n)
> 
> 
> This "int n" is ignored here by a reason?

So looking at these comments, I realized the current code for reading
the HPTEs from KVM is just broken.  So, for v2, I've prepended a patch
to fix that first, after which I don't need to touch the KVM side for
the rest of the series.

[snip]
> > +const ppc_hash_pte64_t *ppc_hash64_map_hptes(PowerPCCPU *cpu,\
> 
> 
> You do not need the trailing '\'.

Missed this comment on my first pass, fixed now, thanks.