[RFC,11/17] target/ppc/POWER9: Update to new pte format for POWER9 accesses

Commit Message

Suraj Jitindar Singh Jan. 13, 2017, 6:28 a.m. UTC

The page table entry format was updated for the POWER9 processor.

It was decided that kernels would used the old format irrespective
with the translation occuring at the hypervisor level. Thus we convert
between the old and new format when accessing the ptes. Since we need the
whole pte to perform this conversion, we remove the old functions which
accessed either the first or second doubleword and introduce a new
functions which access the entire pte, returning the entry converted
back to the old format (if required). Update call sites accordingly.

Signed-off-by: Suraj Jitindar Singh <[e-mail]>
---
 hw/ppc/spapr_hcall.c    | 51 ++++++++++++++++++-----------------
 target/ppc/mmu-hash64.c | 13 +++++----
 target/ppc/mmu-hash64.h | 71 ++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 86 insertions(+), 49 deletions(-)

Comments

David Gibson Feb. 1, 2017, 4:28 a.m. UTC | #1

On Fri, Jan 13, 2017 at 05:28:17PM +1100, Suraj Jitindar Singh wrote:
> The page table entry format was updated for the POWER9 processor.
> 
> It was decided that kernels would used the old format irrespective
> with the translation occuring at the hypervisor level. Thus we convert
> between the old and new format when accessing the ptes. Since we need the
> whole pte to perform this conversion, we remove the old functions which
> accessed either the first or second doubleword and introduce a new
> functions which access the entire pte, returning the entry converted
> back to the old format (if required). Update call sites accordingly.
> 
> Signed-off-by: Suraj Jitindar Singh <[e-mail]>
> ---
>  hw/ppc/spapr_hcall.c    | 51 ++++++++++++++++++-----------------
>  target/ppc/mmu-hash64.c | 13 +++++----
>  target/ppc/mmu-hash64.h | 71 ++++++++++++++++++++++++++++++++++++-------------
>  3 files changed, 86 insertions(+), 49 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 9a9bedf..9f0c20d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -125,7 +125,8 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>          pte_index &= ~7ULL;
>          token = ppc_hash64_start_access(cpu, pte_index);
>          for (; index < 8; index++) {
> -            if (!(ppc_hash64_load_hpte0(cpu, token, index) & HPTE64_V_VALID)) {
> +            ppc_hash_pte64_t pte = ppc_hash64_load_hpte(cpu, token, index);
> +            if (!(pte.pte0 & HPTE64_V_VALID)) {
>                  break;
>              }
>          }
> @@ -134,8 +135,10 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>              return H_PTEG_FULL;
>          }
>      } else {
> +        ppc_hash_pte64_t pte;
>          token = ppc_hash64_start_access(cpu, pte_index);

IIRC the only reason for the clumsy start_access / stop_access stuff
was because we wanted to do the two loads separately (to avoid loading
word1 in cases we don't need it).  Since you're combining both loads
into a single helper, I think you can put the start_access /
stop_access into the same helper.

Or have I missed something.

> -        if (ppc_hash64_load_hpte0(cpu, token, 0) & HPTE64_V_VALID) {
> +        pte = ppc_hash64_load_hpte(cpu, token, 0);
> +        if (pte.pte0 & HPTE64_V_VALID) {
>              ppc_hash64_stop_access(cpu, token);
>              return H_PTEG_FULL;
>          }
> @@ -163,26 +166,25 @@ static RemoveResult remove_hpte(PowerPCCPU *cpu, target_ulong ptex,
>  {
>      CPUPPCState *env = &cpu->env;
>      uint64_t token;
> -    target_ulong v, r;
> +    ppc_hash_pte64_t pte;
>  
>      if (!valid_pte_index(env, 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);
> +    pte = ppc_hash64_load_hpte(cpu, token, 0);
>      ppc_hash64_stop_access(cpu, token);
>  
> -    if ((v & HPTE64_V_VALID) == 0 ||
> -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> -        ((flags & H_ANDCOND) && (v & avpn) != 0)) {
> +    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
> +        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn) ||
> +        ((flags & H_ANDCOND) && (pte.pte0 & avpn) != 0)) {
>          return REMOVE_NOT_FOUND;
>      }
> -    *vp = v;
> -    *rp = r;
> +    *vp = pte.pte0;
> +    *rp = pte.pte1;
>      ppc_hash64_store_hpte(cpu, ptex, HPTE64_V_HPTE_DIRTY, 0);
> -    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
> +    ppc_hash64_tlb_flush_hpte(cpu, ptex, pte.pte0, pte.pte1);
>      return REMOVE_SUCCESS;
>  }
>  
> @@ -293,35 +295,36 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      target_ulong flags = args[0];
>      target_ulong pte_index = args[1];
>      target_ulong avpn = args[2];
> +    ppc_hash_pte64_t pte;
>      uint64_t token;
> -    target_ulong v, r;
>  
>      if (!valid_pte_index(env, pte_index)) {
>          return H_PARAMETER;
>      }
>  
>      token = ppc_hash64_start_access(cpu, pte_index);
> -    v = ppc_hash64_load_hpte0(cpu, token, 0);
> -    r = ppc_hash64_load_hpte1(cpu, token, 0);
> +    pte = ppc_hash64_load_hpte(cpu, token, 0);
>      ppc_hash64_stop_access(cpu, token);
>  
> -    if ((v & HPTE64_V_VALID) == 0 ||
> -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> +    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
> +        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn)) {
>          return H_NOT_FOUND;
>      }
>  
> -    r &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> -           HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> -    r |= (flags << 55) & HPTE64_R_PP0;
> -    r |= (flags << 48) & HPTE64_R_KEY_HI;
> -    r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
> +    pte.pte1 &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> +                  HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> +    pte.pte1 |= (flags << 55) & HPTE64_R_PP0;
> +    pte.pte1 |= (flags << 48) & HPTE64_R_KEY_HI;
> +    pte.pte1 |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
>      ppc_hash64_store_hpte(cpu, pte_index,
> -                          (v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY, 0);
> -    ppc_hash64_tlb_flush_hpte(cpu, pte_index, v, r);
> +                          (pte.pte0 & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY,
> +                          0);
> +    ppc_hash64_tlb_flush_hpte(cpu, pte_index, pte.pte0, pte.pte1);
>      /* Flush the tlb */
>      check_tlb_flush(env, true);
>      /* Don't need a memory barrier, due to qemu's global lock */
> -    ppc_hash64_store_hpte(cpu, pte_index, v | HPTE64_V_HPTE_DIRTY, r);
> +    ppc_hash64_store_hpte(cpu, pte_index, pte.pte0 | HPTE64_V_HPTE_DIRTY,
> +                          pte.pte1);
>      return H_SUCCESS;
>  }
>  
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index b476b3f..03607d5 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -515,7 +515,6 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
>      CPUPPCState *env = &cpu->env;
>      int i;
>      uint64_t token;
> -    target_ulong pte0, pte1;
>      target_ulong pte_index;
>  
>      pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
> @@ -524,12 +523,11 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
>          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);
> +        ppc_hash_pte64_t entry = ppc_hash64_load_hpte(cpu, token, i);

Hm.  So the hypercalls using the access helpers which include format
conversion makes sense to me - the hypercalls are defined to use the
old format, even on POWER9 AIUI.

It doesn't really make sense to me here, in what is essentially the
physical implementation of the HPT lookup.  Shouldn't that be using
the new format natively?

Basically it seems odd that you store things in the new format in
memory, but convert to the old format on *all* accesses, not just the
hypercall ones which are defined that way.

>          /* This compares V, B, H (secondary) and the AVPN */
> -        if (HPTE64_V_COMPARE(pte0, ptem)) {
> -            *pshift = hpte_page_shift(sps, pte0, pte1);
> +        if (HPTE64_V_COMPARE(entry.pte0, ptem)) {
> +            *pshift = hpte_page_shift(sps, entry.pte0, entry.pte1);
>              /*
>               * If there is no match, ignore the PTE, it could simply
>               * be for a different segment size encoding and the
> @@ -543,8 +541,7 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
>              /* We don't do anything with pshift yet as qemu TLB only deals
>               * with 4K pages anyway
>               */
> -            pte->pte0 = pte0;
> -            pte->pte1 = pte1;
> +            *pte = entry;
>              ppc_hash64_stop_access(cpu, token);
>              return (pte_index + i) * HASH_PTE_SIZE_64;
>          }
> @@ -924,6 +921,8 @@ void ppc_hash64_store_hpte(PowerPCCPU *cpu,
>  {
>      CPUPPCState *env = &cpu->env;
>  
> +    ppc_hash64_hpte_old_to_new(env, &pte0, &pte1);
> +
>      if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
>          kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
>          return;
> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> index ab5d347..73d7ce4 100644
> --- a/target/ppc/mmu-hash64.h
> +++ b/target/ppc/mmu-hash64.h
> @@ -60,6 +60,7 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
>  #define HASH_PTE_SIZE_64        16
>  #define HASH_PTEG_SIZE_64       (HASH_PTE_SIZE_64 * HPTES_PER_GROUP)
>  
> +#define HPTE64_V_3_00_COMMON    0x000fffffffffffffULL
>  #define HPTE64_V_SSIZE_SHIFT    62
>  #define HPTE64_V_AVPN_SHIFT     7
>  #define HPTE64_V_AVPN           0x3fffffffffffff80ULL
> @@ -69,9 +70,12 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
>  #define HPTE64_V_SECONDARY      0x0000000000000002ULL
>  #define HPTE64_V_VALID          0x0000000000000001ULL
>  
> +#define HPTE64_R_3_00_COMMON    0xf1ffffffffffffffULL
>  #define HPTE64_R_PP0            0x8000000000000000ULL
>  #define HPTE64_R_TS             0x4000000000000000ULL
>  #define HPTE64_R_KEY_HI         0x3000000000000000ULL
> +#define HPTE64_R_SSIZE_SHIFT    58
> +#define HPTE64_R_SSIZE_MASK     (3ULL << HPTE64_R_SSIZE_SHIFT)
>  #define HPTE64_R_RPN_SHIFT      12
>  #define HPTE64_R_RPN            0x0ffffffffffff000ULL
>  #define HPTE64_R_FLAGS          0x00000000000003ffULL
> @@ -91,6 +95,10 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
>  #define HPTE64_V_1TB_SEG        0x4000000000000000ULL
>  #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
>  
> +typedef struct {
> +    uint64_t pte0, pte1;
> +} ppc_hash_pte64_t;
> +
>  void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
>                           Error **errp);
>  void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift,
> @@ -99,37 +107,64 @@ void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift,
>  uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_index);
>  void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token);
>  
> -static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
> -                                                 uint64_t token, int index)
> +static inline void ppc_hash64_hpte_old_to_new(CPUPPCState *env,
> +                                              target_ulong *pte0,
> +                                              target_ulong *pte1)
>  {
> -    CPUPPCState *env = &cpu->env;
> -    uint64_t addr;
> +    switch (env->mmu_model) {
> +    case POWERPC_MMU_3_00:
> +        /*
> +         * v3.00 of the ISA moved the B field to the second doubleword and
> +         * shortened the abbreviated virtual address and abbreviated real page
> +         * number fields
> +         */
> +        *pte1 = (*pte1 & HPTE64_R_3_00_COMMON) |
> +                ((*pte0 >> HPTE64_V_SSIZE_SHIFT) << HPTE64_R_SSIZE_SHIFT);
> +        *pte0 = *pte0 & HPTE64_V_3_00_COMMON;
> +    default:
> +        ;
> +    }
> +}
>  
> -    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);
> +static inline void ppc_hash64_hpte_new_to_old(CPUPPCState *env,
> +                                              target_ulong *pte0,
> +                                              target_ulong *pte1)
> +{
> +    switch (env->mmu_model) {
> +    case POWERPC_MMU_3_00:
> +        /*
> +         * v3.00 of the ISA moved the B field to the second doubleword and
> +         * shortened the abbreviated virtual address and abbreviated real page
> +         * number fields
> +         */
> +        *pte0 = (*pte0 & HPTE64_V_3_00_COMMON) | ((*pte1 & HPTE64_R_SSIZE_MASK)
> +                << (HPTE64_V_SSIZE_SHIFT - HPTE64_R_SSIZE_SHIFT));
> +        *pte1 = *pte1 & HPTE64_R_3_00_COMMON;
> +    default:
> +        ;
>      }
>  }
>  
> -static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU *cpu,
> -                                                 uint64_t token, int index)
> +static inline ppc_hash_pte64_t ppc_hash64_load_hpte(PowerPCCPU *cpu,
> +                                                    uint64_t token,
> +                                                    int index)
>  {
>      CPUPPCState *env = &cpu->env;
> +    ppc_hash_pte64_t pte;
>      uint64_t addr;
>  
> -    addr = token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2;
> +    addr = token + (index * HASH_PTE_SIZE_64);
>      if (env->external_htab) {
> -        return  ldq_p((const void *)(uintptr_t)addr);
> +        pte.pte0 = ldq_p((const void *)(uintptr_t)addr);
> +        pte.pte1 = ldq_p((const void *)(uintptr_t)addr + HASH_PTE_SIZE_64/2);
>      } else {
> -        return ldq_phys(CPU(cpu)->as, addr);
> +        pte.pte0 = ldq_phys(CPU(cpu)->as, addr);
> +        pte.pte1 = ldq_phys(CPU(cpu)->as, addr + HASH_PTE_SIZE_64/2);
>      }
> -}
>  
> -typedef struct {
> -    uint64_t pte0, pte1;
> -} ppc_hash_pte64_t;
> +    ppc_hash64_hpte_new_to_old(env, &pte.pte0, &pte.pte1);
> +    return pte;
> +}
>  
>  #endif /* CONFIG_USER_ONLY */
>

Suraj Jitindar Singh Feb. 9, 2017, 3:08 a.m. UTC | #2

On Wed, 2017-02-01 at 15:28 +1100, David Gibson wrote:
> On Fri, Jan 13, 2017 at 05:28:17PM +1100, Suraj Jitindar Singh wrote:
> > 
> > The page table entry format was updated for the POWER9 processor.
> > 
> > It was decided that kernels would used the old format irrespective
> > with the translation occuring at the hypervisor level. Thus we
> > convert
> > between the old and new format when accessing the ptes. Since we
> > need the
> > whole pte to perform this conversion, we remove the old functions
> > which
> > accessed either the first or second doubleword and introduce a new
> > functions which access the entire pte, returning the entry
> > converted
> > back to the old format (if required). Update call sites
> > accordingly.
> > 
> > Signed-off-by: Suraj Jitindar Singh <[e-mail]>
> > ---
> >  hw/ppc/spapr_hcall.c    | 51 ++++++++++++++++++-----------------
> >  target/ppc/mmu-hash64.c | 13 +++++----
> >  target/ppc/mmu-hash64.h | 71 ++++++++++++++++++++++++++++++++++++-
> > ------------
> >  3 files changed, 86 insertions(+), 49 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 9a9bedf..9f0c20d 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -125,7 +125,8 @@ static target_ulong h_enter(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr,
> >          pte_index &= ~7ULL;
> >          token = ppc_hash64_start_access(cpu, pte_index);
> >          for (; index < 8; index++) {
> > -            if (!(ppc_hash64_load_hpte0(cpu, token, index) &
> > HPTE64_V_VALID)) {
> > +            ppc_hash_pte64_t pte = ppc_hash64_load_hpte(cpu,
> > token, index);
> > +            if (!(pte.pte0 & HPTE64_V_VALID)) {
> >                  break;
> >              }
> >          }
> > @@ -134,8 +135,10 @@ static target_ulong h_enter(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr,
> >              return H_PTEG_FULL;
> >          }
> >      } else {
> > +        ppc_hash_pte64_t pte;
> >          token = ppc_hash64_start_access(cpu, pte_index);
> IIRC the only reason for the clumsy start_access / stop_access stuff
> was because we wanted to do the two loads separately (to avoid
> loading
> word1 in cases we don't need it).  Since you're combining both loads
> into a single helper, I think you can put the start_access /
> stop_access into the same helper.
> 
> Or have I missed something.
> 
Yeah these functions can probably be merged together...
> > 
> > -        if (ppc_hash64_load_hpte0(cpu, token, 0) & HPTE64_V_VALID)
> > {
> > +        pte = ppc_hash64_load_hpte(cpu, token, 0);
> > +        if (pte.pte0 & HPTE64_V_VALID) {
> >              ppc_hash64_stop_access(cpu, token);
> >              return H_PTEG_FULL;
> >          }
> > @@ -163,26 +166,25 @@ static RemoveResult remove_hpte(PowerPCCPU
> > *cpu, target_ulong ptex,
> >  {
> >      CPUPPCState *env = &cpu->env;
> >      uint64_t token;
> > -    target_ulong v, r;
> > +    ppc_hash_pte64_t pte;
> >  
> >      if (!valid_pte_index(env, 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);
> > +    pte = ppc_hash64_load_hpte(cpu, token, 0);
> >      ppc_hash64_stop_access(cpu, token);
> >  
> > -    if ((v & HPTE64_V_VALID) == 0 ||
> > -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> > -        ((flags & H_ANDCOND) && (v & avpn) != 0)) {
> > +    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
> > +        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn) ||
> > +        ((flags & H_ANDCOND) && (pte.pte0 & avpn) != 0)) {
> >          return REMOVE_NOT_FOUND;
> >      }
> > -    *vp = v;
> > -    *rp = r;
> > +    *vp = pte.pte0;
> > +    *rp = pte.pte1;
> >      ppc_hash64_store_hpte(cpu, ptex, HPTE64_V_HPTE_DIRTY, 0);
> > -    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
> > +    ppc_hash64_tlb_flush_hpte(cpu, ptex, pte.pte0, pte.pte1);
> >      return REMOVE_SUCCESS;
> >  }
> >  
> > @@ -293,35 +295,36 @@ static target_ulong h_protect(PowerPCCPU
> > *cpu, sPAPRMachineState *spapr,
> >      target_ulong flags = args[0];
> >      target_ulong pte_index = args[1];
> >      target_ulong avpn = args[2];
> > +    ppc_hash_pte64_t pte;
> >      uint64_t token;
> > -    target_ulong v, r;
> >  
> >      if (!valid_pte_index(env, pte_index)) {
> >          return H_PARAMETER;
> >      }
> >  
> >      token = ppc_hash64_start_access(cpu, pte_index);
> > -    v = ppc_hash64_load_hpte0(cpu, token, 0);
> > -    r = ppc_hash64_load_hpte1(cpu, token, 0);
> > +    pte = ppc_hash64_load_hpte(cpu, token, 0);
> >      ppc_hash64_stop_access(cpu, token);
> >  
> > -    if ((v & HPTE64_V_VALID) == 0 ||
> > -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> > +    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
> > +        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn)) {
> >          return H_NOT_FOUND;
> >      }
> >  
> > -    r &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> > -           HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> > -    r |= (flags << 55) & HPTE64_R_PP0;
> > -    r |= (flags << 48) & HPTE64_R_KEY_HI;
> > -    r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
> > +    pte.pte1 &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> > +                  HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> > +    pte.pte1 |= (flags << 55) & HPTE64_R_PP0;
> > +    pte.pte1 |= (flags << 48) & HPTE64_R_KEY_HI;
> > +    pte.pte1 |= flags & (HPTE64_R_PP | HPTE64_R_N |
> > HPTE64_R_KEY_LO);
> >      ppc_hash64_store_hpte(cpu, pte_index,
> > -                          (v & ~HPTE64_V_VALID) |
> > HPTE64_V_HPTE_DIRTY, 0);
> > -    ppc_hash64_tlb_flush_hpte(cpu, pte_index, v, r);
> > +                          (pte.pte0 & ~HPTE64_V_VALID) |
> > HPTE64_V_HPTE_DIRTY,
> > +                          0);
> > +    ppc_hash64_tlb_flush_hpte(cpu, pte_index, pte.pte0, pte.pte1);
> >      /* Flush the tlb */
> >      check_tlb_flush(env, true);
> >      /* Don't need a memory barrier, due to qemu's global lock */
> > -    ppc_hash64_store_hpte(cpu, pte_index, v | HPTE64_V_HPTE_DIRTY,
> > r);
> > +    ppc_hash64_store_hpte(cpu, pte_index, pte.pte0 |
> > HPTE64_V_HPTE_DIRTY,
> > +                          pte.pte1);
> >      return H_SUCCESS;
> >  }
> >  
> > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > index b476b3f..03607d5 100644
> > --- a/target/ppc/mmu-hash64.c
> > +++ b/target/ppc/mmu-hash64.c
> > @@ -515,7 +515,6 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU
> > *cpu, hwaddr hash,
> >      CPUPPCState *env = &cpu->env;
> >      int i;
> >      uint64_t token;
> > -    target_ulong pte0, pte1;
> >      target_ulong pte_index;
> >  
> >      pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
> > @@ -524,12 +523,11 @@ static hwaddr
> > ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
> >          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);
> > +        ppc_hash_pte64_t entry = ppc_hash64_load_hpte(cpu, token,
> > i);
> Hm.  So the hypercalls using the access helpers which include format
> conversion makes sense to me - the hypercalls are defined to use the
> old format, even on POWER9 AIUI.
> 
> It doesn't really make sense to me here, in what is essentially the
> physical implementation of the HPT lookup.  Shouldn't that be using
> the new format natively?
> 
> Basically it seems odd that you store things in the new format in
> memory, but convert to the old format on *all* accesses, not just the
> hypercall ones which are defined that way.
> 
It seemed easier to just do the conversion rather than updating the
code in all places. That said since this is modelling the hardware and
should probably use the new format instead of just converting it and
pretenting nothing changed.
> > 
> >          /* This compares V, B, H (secondary) and the AVPN */
> > -        if (HPTE64_V_COMPARE(pte0, ptem)) {
> > -            *pshift = hpte_page_shift(sps, pte0, pte1);
> > +        if (HPTE64_V_COMPARE(entry.pte0, ptem)) {
> > +            *pshift = hpte_page_shift(sps, entry.pte0,
> > entry.pte1);
> >              /*
> >               * If there is no match, ignore the PTE, it could
> > simply
> >               * be for a different segment size encoding and the
> > @@ -543,8 +541,7 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU
> > *cpu, hwaddr hash,
> >              /* We don't do anything with pshift yet as qemu TLB
> > only deals
> >               * with 4K pages anyway
> >               */
> > -            pte->pte0 = pte0;
> > -            pte->pte1 = pte1;
> > +            *pte = entry;
> >              ppc_hash64_stop_access(cpu, token);
> >              return (pte_index + i) * HASH_PTE_SIZE_64;
> >          }
> > @@ -924,6 +921,8 @@ void ppc_hash64_store_hpte(PowerPCCPU *cpu,
> >  {
> >      CPUPPCState *env = &cpu->env;
> >  
> > +    ppc_hash64_hpte_old_to_new(env, &pte0, &pte1);
> > +
> >      if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> >          kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
> >          return;
> > diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> > index ab5d347..73d7ce4 100644
> > --- a/target/ppc/mmu-hash64.h
> > +++ b/target/ppc/mmu-hash64.h
> > @@ -60,6 +60,7 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
> >  #define HASH_PTE_SIZE_64        16
> >  #define HASH_PTEG_SIZE_64       (HASH_PTE_SIZE_64 *
> > HPTES_PER_GROUP)
> >  
> > +#define HPTE64_V_3_00_COMMON    0x000fffffffffffffULL
> >  #define HPTE64_V_SSIZE_SHIFT    62
> >  #define HPTE64_V_AVPN_SHIFT     7
> >  #define HPTE64_V_AVPN           0x3fffffffffffff80ULL
> > @@ -69,9 +70,12 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
> >  #define HPTE64_V_SECONDARY      0x0000000000000002ULL
> >  #define HPTE64_V_VALID          0x0000000000000001ULL
> >  
> > +#define HPTE64_R_3_00_COMMON    0xf1ffffffffffffffULL
> >  #define HPTE64_R_PP0            0x8000000000000000ULL
> >  #define HPTE64_R_TS             0x4000000000000000ULL
> >  #define HPTE64_R_KEY_HI         0x3000000000000000ULL
> > +#define HPTE64_R_SSIZE_SHIFT    58
> > +#define HPTE64_R_SSIZE_MASK     (3ULL << HPTE64_R_SSIZE_SHIFT)
> >  #define HPTE64_R_RPN_SHIFT      12
> >  #define HPTE64_R_RPN            0x0ffffffffffff000ULL
> >  #define HPTE64_R_FLAGS          0x00000000000003ffULL
> > @@ -91,6 +95,10 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
> >  #define HPTE64_V_1TB_SEG        0x4000000000000000ULL
> >  #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
> >  
> > +typedef struct {
> > +    uint64_t pte0, pte1;
> > +} ppc_hash_pte64_t;
> > +
> >  void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
> >                           Error **errp);
> >  void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int
> > shift,
> > @@ -99,37 +107,64 @@ void ppc_hash64_set_external_hpt(PowerPCCPU
> > *cpu, void *hpt, int shift,
> >  uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong
> > pte_index);
> >  void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token);
> >  
> > -static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
> > -                                                 uint64_t token,
> > int index)
> > +static inline void ppc_hash64_hpte_old_to_new(CPUPPCState *env,
> > +                                              target_ulong *pte0,
> > +                                              target_ulong *pte1)
> >  {
> > -    CPUPPCState *env = &cpu->env;
> > -    uint64_t addr;
> > +    switch (env->mmu_model) {
> > +    case POWERPC_MMU_3_00:
> > +        /*
> > +         * v3.00 of the ISA moved the B field to the second
> > doubleword and
> > +         * shortened the abbreviated virtual address and
> > abbreviated real page
> > +         * number fields
> > +         */
> > +        *pte1 = (*pte1 & HPTE64_R_3_00_COMMON) |
> > +                ((*pte0 >> HPTE64_V_SSIZE_SHIFT) <<
> > HPTE64_R_SSIZE_SHIFT);
> > +        *pte0 = *pte0 & HPTE64_V_3_00_COMMON;
> > +    default:
> > +        ;
> > +    }
> > +}
> >  
> > -    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);
> > +static inline void ppc_hash64_hpte_new_to_old(CPUPPCState *env,
> > +                                              target_ulong *pte0,
> > +                                              target_ulong *pte1)
> > +{
> > +    switch (env->mmu_model) {
> > +    case POWERPC_MMU_3_00:
> > +        /*
> > +         * v3.00 of the ISA moved the B field to the second
> > doubleword and
> > +         * shortened the abbreviated virtual address and
> > abbreviated real page
> > +         * number fields
> > +         */
> > +        *pte0 = (*pte0 & HPTE64_V_3_00_COMMON) | ((*pte1 &
> > HPTE64_R_SSIZE_MASK)
> > +                << (HPTE64_V_SSIZE_SHIFT - HPTE64_R_SSIZE_SHIFT));
> > +        *pte1 = *pte1 & HPTE64_R_3_00_COMMON;
> > +    default:
> > +        ;
> >      }
> >  }
> >  
> > -static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU *cpu,
> > -                                                 uint64_t token,
> > int index)
> > +static inline ppc_hash_pte64_t ppc_hash64_load_hpte(PowerPCCPU
> > *cpu,
> > +                                                    uint64_t
> > token,
> > +                                                    int index)
> >  {
> >      CPUPPCState *env = &cpu->env;
> > +    ppc_hash_pte64_t pte;
> >      uint64_t addr;
> >  
> > -    addr = token + (index * HASH_PTE_SIZE_64) +
> > HASH_PTE_SIZE_64/2;
> > +    addr = token + (index * HASH_PTE_SIZE_64);
> >      if (env->external_htab) {
> > -        return  ldq_p((const void *)(uintptr_t)addr);
> > +        pte.pte0 = ldq_p((const void *)(uintptr_t)addr);
> > +        pte.pte1 = ldq_p((const void *)(uintptr_t)addr +
> > HASH_PTE_SIZE_64/2);
> >      } else {
> > -        return ldq_phys(CPU(cpu)->as, addr);
> > +        pte.pte0 = ldq_phys(CPU(cpu)->as, addr);
> > +        pte.pte1 = ldq_phys(CPU(cpu)->as, addr +
> > HASH_PTE_SIZE_64/2);
> >      }
> > -}
> >  
> > -typedef struct {
> > -    uint64_t pte0, pte1;
> > -} ppc_hash_pte64_t;
> > +    ppc_hash64_hpte_new_to_old(env, &pte.pte0, &pte.pte1);
> > +    return pte;
> > +}
> >  
> >  #endif /* CONFIG_USER_ONLY */
> >

Suraj Jitindar Singh Feb. 9, 2017, 11:47 p.m. UTC | #3

On Thu, 2017-02-09 at 14:08 +1100, Suraj Jitindar Singh wrote:
> On Wed, 2017-02-01 at 15:28 +1100, David Gibson wrote:
> > 
> > On Fri, Jan 13, 2017 at 05:28:17PM +1100, Suraj Jitindar Singh
> > wrote:
> > > 
> > > 
> > > The page table entry format was updated for the POWER9 processor.
> > > 
> > > It was decided that kernels would used the old format
> > > irrespective
> > > with the translation occuring at the hypervisor level. Thus we
> > > convert
> > > between the old and new format when accessing the ptes. Since we
> > > need the
> > > whole pte to perform this conversion, we remove the old functions
> > > which
> > > accessed either the first or second doubleword and introduce a
> > > new
> > > functions which access the entire pte, returning the entry
> > > converted
> > > back to the old format (if required). Update call sites
> > > accordingly.
> > > 
> > > Signed-off-by: Suraj Jitindar Singh <[e-mail]>
> > > ---
> > >  hw/ppc/spapr_hcall.c    | 51 ++++++++++++++++++-----------------
> > >  target/ppc/mmu-hash64.c | 13 +++++----
> > >  target/ppc/mmu-hash64.h | 71
> > > ++++++++++++++++++++++++++++++++++++-
> > > ------------
> > >  3 files changed, 86 insertions(+), 49 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > index 9a9bedf..9f0c20d 100644
> > > --- a/hw/ppc/spapr_hcall.c
> > > +++ b/hw/ppc/spapr_hcall.c
> > > @@ -125,7 +125,8 @@ static target_ulong h_enter(PowerPCCPU *cpu,
> > > sPAPRMachineState *spapr,
> > >          pte_index &= ~7ULL;
> > >          token = ppc_hash64_start_access(cpu, pte_index);
> > >          for (; index < 8; index++) {
> > > -            if (!(ppc_hash64_load_hpte0(cpu, token, index) &
> > > HPTE64_V_VALID)) {
> > > +            ppc_hash_pte64_t pte = ppc_hash64_load_hpte(cpu,
> > > token, index);
> > > +            if (!(pte.pte0 & HPTE64_V_VALID)) {
> > >                  break;
> > >              }
> > >          }
> > > @@ -134,8 +135,10 @@ static target_ulong h_enter(PowerPCCPU *cpu,
> > > sPAPRMachineState *spapr,
> > >              return H_PTEG_FULL;
> > >          }
> > >      } else {
> > > +        ppc_hash_pte64_t pte;
> > >          token = ppc_hash64_start_access(cpu, pte_index);
> > IIRC the only reason for the clumsy start_access / stop_access
> > stuff
> > was because we wanted to do the two loads separately (to avoid
> > loading
> > word1 in cases we don't need it).  Since you're combining both
> > loads
> > into a single helper, I think you can put the start_access /
> > stop_access into the same helper.
> > 
> > Or have I missed something.
> > 
> Yeah these functions can probably be merged together...
Actually, it looks like we need the start access and stop access stuff
for kvm-hv where we load the pte into a buffer on start access and free
it on stop access. So I think we still need to keep these two functions
separate.
> > 
> > > 
> > > 
> > > -        if (ppc_hash64_load_hpte0(cpu, token, 0) &
> > > HPTE64_V_VALID)
> > > {
> > > +        pte = ppc_hash64_load_hpte(cpu, token, 0);
> > > +        if (pte.pte0 & HPTE64_V_VALID) {
> > >              ppc_hash64_stop_access(cpu, token);
> > >              return H_PTEG_FULL;
> > >          }
> > > @@ -163,26 +166,25 @@ static RemoveResult remove_hpte(PowerPCCPU
> > > *cpu, target_ulong ptex,
> > >  {
> > >      CPUPPCState *env = &cpu->env;
> > >      uint64_t token;
> > > -    target_ulong v, r;
> > > +    ppc_hash_pte64_t pte;
> > >  
> > >      if (!valid_pte_index(env, 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);
> > > +    pte = ppc_hash64_load_hpte(cpu, token, 0);
> > >      ppc_hash64_stop_access(cpu, token);
> > >  
> > > -    if ((v & HPTE64_V_VALID) == 0 ||
> > > -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> > > -        ((flags & H_ANDCOND) && (v & avpn) != 0)) {
> > > +    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
> > > +        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn) ||
> > > +        ((flags & H_ANDCOND) && (pte.pte0 & avpn) != 0)) {
> > >          return REMOVE_NOT_FOUND;
> > >      }
> > > -    *vp = v;
> > > -    *rp = r;
> > > +    *vp = pte.pte0;
> > > +    *rp = pte.pte1;
> > >      ppc_hash64_store_hpte(cpu, ptex, HPTE64_V_HPTE_DIRTY, 0);
> > > -    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
> > > +    ppc_hash64_tlb_flush_hpte(cpu, ptex, pte.pte0, pte.pte1);
> > >      return REMOVE_SUCCESS;
> > >  }
> > >  
> > > @@ -293,35 +295,36 @@ static target_ulong h_protect(PowerPCCPU
> > > *cpu, sPAPRMachineState *spapr,
> > >      target_ulong flags = args[0];
> > >      target_ulong pte_index = args[1];
> > >      target_ulong avpn = args[2];
> > > +    ppc_hash_pte64_t pte;
> > >      uint64_t token;
> > > -    target_ulong v, r;
> > >  
> > >      if (!valid_pte_index(env, pte_index)) {
> > >          return H_PARAMETER;
> > >      }
> > >  
> > >      token = ppc_hash64_start_access(cpu, pte_index);
> > > -    v = ppc_hash64_load_hpte0(cpu, token, 0);
> > > -    r = ppc_hash64_load_hpte1(cpu, token, 0);
> > > +    pte = ppc_hash64_load_hpte(cpu, token, 0);
> > >      ppc_hash64_stop_access(cpu, token);
> > >  
> > > -    if ((v & HPTE64_V_VALID) == 0 ||
> > > -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> > > +    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
> > > +        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn)) {
> > >          return H_NOT_FOUND;
> > >      }
> > >  
> > > -    r &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> > > -           HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> > > -    r |= (flags << 55) & HPTE64_R_PP0;
> > > -    r |= (flags << 48) & HPTE64_R_KEY_HI;
> > > -    r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
> > > +    pte.pte1 &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> > > +                  HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> > > +    pte.pte1 |= (flags << 55) & HPTE64_R_PP0;
> > > +    pte.pte1 |= (flags << 48) & HPTE64_R_KEY_HI;
> > > +    pte.pte1 |= flags & (HPTE64_R_PP | HPTE64_R_N |
> > > HPTE64_R_KEY_LO);
> > >      ppc_hash64_store_hpte(cpu, pte_index,
> > > -                          (v & ~HPTE64_V_VALID) |
> > > HPTE64_V_HPTE_DIRTY, 0);
> > > -    ppc_hash64_tlb_flush_hpte(cpu, pte_index, v, r);
> > > +                          (pte.pte0 & ~HPTE64_V_VALID) |
> > > HPTE64_V_HPTE_DIRTY,
> > > +                          0);
> > > +    ppc_hash64_tlb_flush_hpte(cpu, pte_index, pte.pte0,
> > > pte.pte1);
> > >      /* Flush the tlb */
> > >      check_tlb_flush(env, true);
> > >      /* Don't need a memory barrier, due to qemu's global lock */
> > > -    ppc_hash64_store_hpte(cpu, pte_index, v |
> > > HPTE64_V_HPTE_DIRTY,
> > > r);
> > > +    ppc_hash64_store_hpte(cpu, pte_index, pte.pte0 |
> > > HPTE64_V_HPTE_DIRTY,
> > > +                          pte.pte1);
> > >      return H_SUCCESS;
> > >  }
> > >  
> > > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > > index b476b3f..03607d5 100644
> > > --- a/target/ppc/mmu-hash64.c
> > > +++ b/target/ppc/mmu-hash64.c
> > > @@ -515,7 +515,6 @@ static hwaddr
> > > ppc_hash64_pteg_search(PowerPCCPU
> > > *cpu, hwaddr hash,
> > >      CPUPPCState *env = &cpu->env;
> > >      int i;
> > >      uint64_t token;
> > > -    target_ulong pte0, pte1;
> > >      target_ulong pte_index;
> > >  
> > >      pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
> > > @@ -524,12 +523,11 @@ static hwaddr
> > > ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
> > >          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);
> > > +        ppc_hash_pte64_t entry = ppc_hash64_load_hpte(cpu,
> > > token,
> > > i);
> > Hm.  So the hypercalls using the access helpers which include
> > format
> > conversion makes sense to me - the hypercalls are defined to use
> > the
> > old format, even on POWER9 AIUI.
> > 
> > It doesn't really make sense to me here, in what is essentially the
> > physical implementation of the HPT lookup.  Shouldn't that be using
> > the new format natively?
> > 
> > Basically it seems odd that you store things in the new format in
> > memory, but convert to the old format on *all* accesses, not just
> > the
> > hypercall ones which are defined that way.
> > 
> It seemed easier to just do the conversion rather than updating the
> code in all places. That said since this is modelling the hardware
> and
> should probably use the new format instead of just converting it and
> pretenting nothing changed.
> > 
> > > 
> > > 
> > >          /* This compares V, B, H (secondary) and the AVPN */
> > > -        if (HPTE64_V_COMPARE(pte0, ptem)) {
> > > -            *pshift = hpte_page_shift(sps, pte0, pte1);
> > > +        if (HPTE64_V_COMPARE(entry.pte0, ptem)) {
> > > +            *pshift = hpte_page_shift(sps, entry.pte0,
> > > entry.pte1);
> > >              /*
> > >               * If there is no match, ignore the PTE, it could
> > > simply
> > >               * be for a different segment size encoding and the
> > > @@ -543,8 +541,7 @@ static hwaddr
> > > ppc_hash64_pteg_search(PowerPCCPU
> > > *cpu, hwaddr hash,
> > >              /* We don't do anything with pshift yet as qemu TLB
> > > only deals
> > >               * with 4K pages anyway
> > >               */
> > > -            pte->pte0 = pte0;
> > > -            pte->pte1 = pte1;
> > > +            *pte = entry;
> > >              ppc_hash64_stop_access(cpu, token);
> > >              return (pte_index + i) * HASH_PTE_SIZE_64;
> > >          }
> > > @@ -924,6 +921,8 @@ void ppc_hash64_store_hpte(PowerPCCPU *cpu,
> > >  {
> > >      CPUPPCState *env = &cpu->env;
> > >  
> > > +    ppc_hash64_hpte_old_to_new(env, &pte0, &pte1);
> > > +
> > >      if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> > >          kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
> > >          return;
> > > diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> > > index ab5d347..73d7ce4 100644
> > > --- a/target/ppc/mmu-hash64.h
> > > +++ b/target/ppc/mmu-hash64.h
> > > @@ -60,6 +60,7 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
> > >  #define HASH_PTE_SIZE_64        16
> > >  #define HASH_PTEG_SIZE_64       (HASH_PTE_SIZE_64 *
> > > HPTES_PER_GROUP)
> > >  
> > > +#define HPTE64_V_3_00_COMMON    0x000fffffffffffffULL
> > >  #define HPTE64_V_SSIZE_SHIFT    62
> > >  #define HPTE64_V_AVPN_SHIFT     7
> > >  #define HPTE64_V_AVPN           0x3fffffffffffff80ULL
> > > @@ -69,9 +70,12 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
> > >  #define HPTE64_V_SECONDARY      0x0000000000000002ULL
> > >  #define HPTE64_V_VALID          0x0000000000000001ULL
> > >  
> > > +#define HPTE64_R_3_00_COMMON    0xf1ffffffffffffffULL
> > >  #define HPTE64_R_PP0            0x8000000000000000ULL
> > >  #define HPTE64_R_TS             0x4000000000000000ULL
> > >  #define HPTE64_R_KEY_HI         0x3000000000000000ULL
> > > +#define HPTE64_R_SSIZE_SHIFT    58
> > > +#define HPTE64_R_SSIZE_MASK     (3ULL << HPTE64_R_SSIZE_SHIFT)
> > >  #define HPTE64_R_RPN_SHIFT      12
> > >  #define HPTE64_R_RPN            0x0ffffffffffff000ULL
> > >  #define HPTE64_R_FLAGS          0x00000000000003ffULL
> > > @@ -91,6 +95,10 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
> > >  #define HPTE64_V_1TB_SEG        0x4000000000000000ULL
> > >  #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
> > >  
> > > +typedef struct {
> > > +    uint64_t pte0, pte1;
> > > +} ppc_hash_pte64_t;
> > > +
> > >  void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
> > >                           Error **errp);
> > >  void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int
> > > shift,
> > > @@ -99,37 +107,64 @@ void ppc_hash64_set_external_hpt(PowerPCCPU
> > > *cpu, void *hpt, int shift,
> > >  uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong
> > > pte_index);
> > >  void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token);
> > >  
> > > -static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU
> > > *cpu,
> > > -                                                 uint64_t token,
> > > int index)
> > > +static inline void ppc_hash64_hpte_old_to_new(CPUPPCState *env,
> > > +                                              target_ulong
> > > *pte0,
> > > +                                              target_ulong
> > > *pte1)
> > >  {
> > > -    CPUPPCState *env = &cpu->env;
> > > -    uint64_t addr;
> > > +    switch (env->mmu_model) {
> > > +    case POWERPC_MMU_3_00:
> > > +        /*
> > > +         * v3.00 of the ISA moved the B field to the second
> > > doubleword and
> > > +         * shortened the abbreviated virtual address and
> > > abbreviated real page
> > > +         * number fields
> > > +         */
> > > +        *pte1 = (*pte1 & HPTE64_R_3_00_COMMON) |
> > > +                ((*pte0 >> HPTE64_V_SSIZE_SHIFT) <<
> > > HPTE64_R_SSIZE_SHIFT);
> > > +        *pte0 = *pte0 & HPTE64_V_3_00_COMMON;
> > > +    default:
> > > +        ;
> > > +    }
> > > +}
> > >  
> > > -    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);
> > > +static inline void ppc_hash64_hpte_new_to_old(CPUPPCState *env,
> > > +                                              target_ulong
> > > *pte0,
> > > +                                              target_ulong
> > > *pte1)
> > > +{
> > > +    switch (env->mmu_model) {
> > > +    case POWERPC_MMU_3_00:
> > > +        /*
> > > +         * v3.00 of the ISA moved the B field to the second
> > > doubleword and
> > > +         * shortened the abbreviated virtual address and
> > > abbreviated real page
> > > +         * number fields
> > > +         */
> > > +        *pte0 = (*pte0 & HPTE64_V_3_00_COMMON) | ((*pte1 &
> > > HPTE64_R_SSIZE_MASK)
> > > +                << (HPTE64_V_SSIZE_SHIFT -
> > > HPTE64_R_SSIZE_SHIFT));
> > > +        *pte1 = *pte1 & HPTE64_R_3_00_COMMON;
> > > +    default:
> > > +        ;
> > >      }
> > >  }
> > >  
> > > -static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU
> > > *cpu,
> > > -                                                 uint64_t token,
> > > int index)
> > > +static inline ppc_hash_pte64_t ppc_hash64_load_hpte(PowerPCCPU
> > > *cpu,
> > > +                                                    uint64_t
> > > token,
> > > +                                                    int index)
> > >  {
> > >      CPUPPCState *env = &cpu->env;
> > > +    ppc_hash_pte64_t pte;
> > >      uint64_t addr;
> > >  
> > > -    addr = token + (index * HASH_PTE_SIZE_64) +
> > > HASH_PTE_SIZE_64/2;
> > > +    addr = token + (index * HASH_PTE_SIZE_64);
> > >      if (env->external_htab) {
> > > -        return  ldq_p((const void *)(uintptr_t)addr);
> > > +        pte.pte0 = ldq_p((const void *)(uintptr_t)addr);
> > > +        pte.pte1 = ldq_p((const void *)(uintptr_t)addr +
> > > HASH_PTE_SIZE_64/2);
> > >      } else {
> > > -        return ldq_phys(CPU(cpu)->as, addr);
> > > +        pte.pte0 = ldq_phys(CPU(cpu)->as, addr);
> > > +        pte.pte1 = ldq_phys(CPU(cpu)->as, addr +
> > > HASH_PTE_SIZE_64/2);
> > >      }
> > > -}
> > >  
> > > -typedef struct {
> > > -    uint64_t pte0, pte1;
> > > -} ppc_hash_pte64_t;
> > > +    ppc_hash64_hpte_new_to_old(env, &pte.pte0, &pte.pte1);
> > > +    return pte;
> > > +}
> > >  
> > >  #endif /* CONFIG_USER_ONLY */
> > >

David Gibson Feb. 10, 2017, 12:21 a.m. UTC | #4

On Fri, Feb 10, 2017 at 10:47:15AM +1100, Suraj Jitindar Singh wrote:
> On Thu, 2017-02-09 at 14:08 +1100, Suraj Jitindar Singh wrote:
> > On Wed, 2017-02-01 at 15:28 +1100, David Gibson wrote:
> > > 
> > > On Fri, Jan 13, 2017 at 05:28:17PM +1100, Suraj Jitindar Singh
> > > wrote:
> > > > 
> > > > 
> > > > The page table entry format was updated for the POWER9 processor.
> > > > 
> > > > It was decided that kernels would used the old format
> > > > irrespective
> > > > with the translation occuring at the hypervisor level. Thus we
> > > > convert
> > > > between the old and new format when accessing the ptes. Since we
> > > > need the
> > > > whole pte to perform this conversion, we remove the old functions
> > > > which
> > > > accessed either the first or second doubleword and introduce a
> > > > new
> > > > functions which access the entire pte, returning the entry
> > > > converted
> > > > back to the old format (if required). Update call sites
> > > > accordingly.
> > > > 
> > > > Signed-off-by: Suraj Jitindar Singh <[e-mail]>
> > > > ---
> > > >  hw/ppc/spapr_hcall.c    | 51 ++++++++++++++++++-----------------
> > > >  target/ppc/mmu-hash64.c | 13 +++++----
> > > >  target/ppc/mmu-hash64.h | 71
> > > > ++++++++++++++++++++++++++++++++++++-
> > > > ------------
> > > >  3 files changed, 86 insertions(+), 49 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > > index 9a9bedf..9f0c20d 100644
> > > > --- a/hw/ppc/spapr_hcall.c
> > > > +++ b/hw/ppc/spapr_hcall.c
> > > > @@ -125,7 +125,8 @@ static target_ulong h_enter(PowerPCCPU *cpu,
> > > > sPAPRMachineState *spapr,
> > > >          pte_index &= ~7ULL;
> > > >          token = ppc_hash64_start_access(cpu, pte_index);
> > > >          for (; index < 8; index++) {
> > > > -            if (!(ppc_hash64_load_hpte0(cpu, token, index) &
> > > > HPTE64_V_VALID)) {
> > > > +            ppc_hash_pte64_t pte = ppc_hash64_load_hpte(cpu,
> > > > token, index);
> > > > +            if (!(pte.pte0 & HPTE64_V_VALID)) {
> > > >                  break;
> > > >              }
> > > >          }
> > > > @@ -134,8 +135,10 @@ static target_ulong h_enter(PowerPCCPU *cpu,
> > > > sPAPRMachineState *spapr,
> > > >              return H_PTEG_FULL;
> > > >          }
> > > >      } else {
> > > > +        ppc_hash_pte64_t pte;
> > > >          token = ppc_hash64_start_access(cpu, pte_index);
> > > IIRC the only reason for the clumsy start_access / stop_access
> > > stuff
> > > was because we wanted to do the two loads separately (to avoid
> > > loading
> > > word1 in cases we don't need it).  Since you're combining both
> > > loads
> > > into a single helper, I think you can put the start_access /
> > > stop_access into the same helper.
> > > 
> > > Or have I missed something.
> > > 
> > Yeah these functions can probably be merged together...
> Actually, it looks like we need the start access and stop access stuff
> for kvm-hv where we load the pte into a buffer on start access and free
> it on stop access. So I think we still need to keep these two functions
> separate.

Well, the question is are there any instances where we do more than
just a load word0 + load word1 within the start/stop pair.  I don't
remember off hand.

> > > 
> > > > 
> > > > 
> > > > -        if (ppc_hash64_load_hpte0(cpu, token, 0) &
> > > > HPTE64_V_VALID)
> > > > {
> > > > +        pte = ppc_hash64_load_hpte(cpu, token, 0);
> > > > +        if (pte.pte0 & HPTE64_V_VALID) {
> > > >              ppc_hash64_stop_access(cpu, token);
> > > >              return H_PTEG_FULL;
> > > >          }
> > > > @@ -163,26 +166,25 @@ static RemoveResult remove_hpte(PowerPCCPU
> > > > *cpu, target_ulong ptex,
> > > >  {
> > > >      CPUPPCState *env = &cpu->env;
> > > >      uint64_t token;
> > > > -    target_ulong v, r;
> > > > +    ppc_hash_pte64_t pte;
> > > >  
> > > >      if (!valid_pte_index(env, 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);
> > > > +    pte = ppc_hash64_load_hpte(cpu, token, 0);
> > > >      ppc_hash64_stop_access(cpu, token);
> > > >  
> > > > -    if ((v & HPTE64_V_VALID) == 0 ||
> > > > -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> > > > -        ((flags & H_ANDCOND) && (v & avpn) != 0)) {
> > > > +    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
> > > > +        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn) ||
> > > > +        ((flags & H_ANDCOND) && (pte.pte0 & avpn) != 0)) {
> > > >          return REMOVE_NOT_FOUND;
> > > >      }
> > > > -    *vp = v;
> > > > -    *rp = r;
> > > > +    *vp = pte.pte0;
> > > > +    *rp = pte.pte1;
> > > >      ppc_hash64_store_hpte(cpu, ptex, HPTE64_V_HPTE_DIRTY, 0);
> > > > -    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
> > > > +    ppc_hash64_tlb_flush_hpte(cpu, ptex, pte.pte0, pte.pte1);
> > > >      return REMOVE_SUCCESS;
> > > >  }
> > > >  
> > > > @@ -293,35 +295,36 @@ static target_ulong h_protect(PowerPCCPU
> > > > *cpu, sPAPRMachineState *spapr,
> > > >      target_ulong flags = args[0];
> > > >      target_ulong pte_index = args[1];
> > > >      target_ulong avpn = args[2];
> > > > +    ppc_hash_pte64_t pte;
> > > >      uint64_t token;
> > > > -    target_ulong v, r;
> > > >  
> > > >      if (!valid_pte_index(env, pte_index)) {
> > > >          return H_PARAMETER;
> > > >      }
> > > >  
> > > >      token = ppc_hash64_start_access(cpu, pte_index);
> > > > -    v = ppc_hash64_load_hpte0(cpu, token, 0);
> > > > -    r = ppc_hash64_load_hpte1(cpu, token, 0);
> > > > +    pte = ppc_hash64_load_hpte(cpu, token, 0);
> > > >      ppc_hash64_stop_access(cpu, token);
> > > >  
> > > > -    if ((v & HPTE64_V_VALID) == 0 ||
> > > > -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> > > > +    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
> > > > +        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn)) {
> > > >          return H_NOT_FOUND;
> > > >      }
> > > >  
> > > > -    r &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> > > > -           HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> > > > -    r |= (flags << 55) & HPTE64_R_PP0;
> > > > -    r |= (flags << 48) & HPTE64_R_KEY_HI;
> > > > -    r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
> > > > +    pte.pte1 &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> > > > +                  HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> > > > +    pte.pte1 |= (flags << 55) & HPTE64_R_PP0;
> > > > +    pte.pte1 |= (flags << 48) & HPTE64_R_KEY_HI;
> > > > +    pte.pte1 |= flags & (HPTE64_R_PP | HPTE64_R_N |
> > > > HPTE64_R_KEY_LO);
> > > >      ppc_hash64_store_hpte(cpu, pte_index,
> > > > -                          (v & ~HPTE64_V_VALID) |
> > > > HPTE64_V_HPTE_DIRTY, 0);
> > > > -    ppc_hash64_tlb_flush_hpte(cpu, pte_index, v, r);
> > > > +                          (pte.pte0 & ~HPTE64_V_VALID) |
> > > > HPTE64_V_HPTE_DIRTY,
> > > > +                          0);
> > > > +    ppc_hash64_tlb_flush_hpte(cpu, pte_index, pte.pte0,
> > > > pte.pte1);
> > > >      /* Flush the tlb */
> > > >      check_tlb_flush(env, true);
> > > >      /* Don't need a memory barrier, due to qemu's global lock */
> > > > -    ppc_hash64_store_hpte(cpu, pte_index, v |
> > > > HPTE64_V_HPTE_DIRTY,
> > > > r);
> > > > +    ppc_hash64_store_hpte(cpu, pte_index, pte.pte0 |
> > > > HPTE64_V_HPTE_DIRTY,
> > > > +                          pte.pte1);
> > > >      return H_SUCCESS;
> > > >  }
> > > >  
> > > > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > > > index b476b3f..03607d5 100644
> > > > --- a/target/ppc/mmu-hash64.c
> > > > +++ b/target/ppc/mmu-hash64.c
> > > > @@ -515,7 +515,6 @@ static hwaddr
> > > > ppc_hash64_pteg_search(PowerPCCPU
> > > > *cpu, hwaddr hash,
> > > >      CPUPPCState *env = &cpu->env;
> > > >      int i;
> > > >      uint64_t token;
> > > > -    target_ulong pte0, pte1;
> > > >      target_ulong pte_index;
> > > >  
> > > >      pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
> > > > @@ -524,12 +523,11 @@ static hwaddr
> > > > ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
> > > >          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);
> > > > +        ppc_hash_pte64_t entry = ppc_hash64_load_hpte(cpu,
> > > > token,
> > > > i);
> > > Hm.  So the hypercalls using the access helpers which include
> > > format
> > > conversion makes sense to me - the hypercalls are defined to use
> > > the
> > > old format, even on POWER9 AIUI.
> > > 
> > > It doesn't really make sense to me here, in what is essentially the
> > > physical implementation of the HPT lookup.  Shouldn't that be using
> > > the new format natively?
> > > 
> > > Basically it seems odd that you store things in the new format in
> > > memory, but convert to the old format on *all* accesses, not just
> > > the
> > > hypercall ones which are defined that way.
> > > 
> > It seemed easier to just do the conversion rather than updating the
> > code in all places. That said since this is modelling the hardware
> > and
> > should probably use the new format instead of just converting it and
> > pretenting nothing changed.
> > > 
> > > > 
> > > > 
> > > >          /* This compares V, B, H (secondary) and the AVPN */
> > > > -        if (HPTE64_V_COMPARE(pte0, ptem)) {
> > > > -            *pshift = hpte_page_shift(sps, pte0, pte1);
> > > > +        if (HPTE64_V_COMPARE(entry.pte0, ptem)) {
> > > > +            *pshift = hpte_page_shift(sps, entry.pte0,
> > > > entry.pte1);
> > > >              /*
> > > >               * If there is no match, ignore the PTE, it could
> > > > simply
> > > >               * be for a different segment size encoding and the
> > > > @@ -543,8 +541,7 @@ static hwaddr
> > > > ppc_hash64_pteg_search(PowerPCCPU
> > > > *cpu, hwaddr hash,
> > > >              /* We don't do anything with pshift yet as qemu TLB
> > > > only deals
> > > >               * with 4K pages anyway
> > > >               */
> > > > -            pte->pte0 = pte0;
> > > > -            pte->pte1 = pte1;
> > > > +            *pte = entry;
> > > >              ppc_hash64_stop_access(cpu, token);
> > > >              return (pte_index + i) * HASH_PTE_SIZE_64;
> > > >          }
> > > > @@ -924,6 +921,8 @@ void ppc_hash64_store_hpte(PowerPCCPU *cpu,
> > > >  {
> > > >      CPUPPCState *env = &cpu->env;
> > > >  
> > > > +    ppc_hash64_hpte_old_to_new(env, &pte0, &pte1);
> > > > +
> > > >      if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> > > >          kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
> > > >          return;
> > > > diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> > > > index ab5d347..73d7ce4 100644
> > > > --- a/target/ppc/mmu-hash64.h
> > > > +++ b/target/ppc/mmu-hash64.h
> > > > @@ -60,6 +60,7 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
> > > >  #define HASH_PTE_SIZE_64        16
> > > >  #define HASH_PTEG_SIZE_64       (HASH_PTE_SIZE_64 *
> > > > HPTES_PER_GROUP)
> > > >  
> > > > +#define HPTE64_V_3_00_COMMON    0x000fffffffffffffULL
> > > >  #define HPTE64_V_SSIZE_SHIFT    62
> > > >  #define HPTE64_V_AVPN_SHIFT     7
> > > >  #define HPTE64_V_AVPN           0x3fffffffffffff80ULL
> > > > @@ -69,9 +70,12 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
> > > >  #define HPTE64_V_SECONDARY      0x0000000000000002ULL
> > > >  #define HPTE64_V_VALID          0x0000000000000001ULL
> > > >  
> > > > +#define HPTE64_R_3_00_COMMON    0xf1ffffffffffffffULL
> > > >  #define HPTE64_R_PP0            0x8000000000000000ULL
> > > >  #define HPTE64_R_TS             0x4000000000000000ULL
> > > >  #define HPTE64_R_KEY_HI         0x3000000000000000ULL
> > > > +#define HPTE64_R_SSIZE_SHIFT    58
> > > > +#define HPTE64_R_SSIZE_MASK     (3ULL << HPTE64_R_SSIZE_SHIFT)
> > > >  #define HPTE64_R_RPN_SHIFT      12
> > > >  #define HPTE64_R_RPN            0x0ffffffffffff000ULL
> > > >  #define HPTE64_R_FLAGS          0x00000000000003ffULL
> > > > @@ -91,6 +95,10 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
> > > >  #define HPTE64_V_1TB_SEG        0x4000000000000000ULL
> > > >  #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
> > > >  
> > > > +typedef struct {
> > > > +    uint64_t pte0, pte1;
> > > > +} ppc_hash_pte64_t;
> > > > +
> > > >  void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
> > > >                           Error **errp);
> > > >  void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int
> > > > shift,
> > > > @@ -99,37 +107,64 @@ void ppc_hash64_set_external_hpt(PowerPCCPU
> > > > *cpu, void *hpt, int shift,
> > > >  uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong
> > > > pte_index);
> > > >  void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token);
> > > >  
> > > > -static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU
> > > > *cpu,
> > > > -                                                 uint64_t token,
> > > > int index)
> > > > +static inline void ppc_hash64_hpte_old_to_new(CPUPPCState *env,
> > > > +                                              target_ulong
> > > > *pte0,
> > > > +                                              target_ulong
> > > > *pte1)
> > > >  {
> > > > -    CPUPPCState *env = &cpu->env;
> > > > -    uint64_t addr;
> > > > +    switch (env->mmu_model) {
> > > > +    case POWERPC_MMU_3_00:
> > > > +        /*
> > > > +         * v3.00 of the ISA moved the B field to the second
> > > > doubleword and
> > > > +         * shortened the abbreviated virtual address and
> > > > abbreviated real page
> > > > +         * number fields
> > > > +         */
> > > > +        *pte1 = (*pte1 & HPTE64_R_3_00_COMMON) |
> > > > +                ((*pte0 >> HPTE64_V_SSIZE_SHIFT) <<
> > > > HPTE64_R_SSIZE_SHIFT);
> > > > +        *pte0 = *pte0 & HPTE64_V_3_00_COMMON;
> > > > +    default:
> > > > +        ;
> > > > +    }
> > > > +}
> > > >  
> > > > -    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);
> > > > +static inline void ppc_hash64_hpte_new_to_old(CPUPPCState *env,
> > > > +                                              target_ulong
> > > > *pte0,
> > > > +                                              target_ulong
> > > > *pte1)
> > > > +{
> > > > +    switch (env->mmu_model) {
> > > > +    case POWERPC_MMU_3_00:
> > > > +        /*
> > > > +         * v3.00 of the ISA moved the B field to the second
> > > > doubleword and
> > > > +         * shortened the abbreviated virtual address and
> > > > abbreviated real page
> > > > +         * number fields
> > > > +         */
> > > > +        *pte0 = (*pte0 & HPTE64_V_3_00_COMMON) | ((*pte1 &
> > > > HPTE64_R_SSIZE_MASK)
> > > > +                << (HPTE64_V_SSIZE_SHIFT -
> > > > HPTE64_R_SSIZE_SHIFT));
> > > > +        *pte1 = *pte1 & HPTE64_R_3_00_COMMON;
> > > > +    default:
> > > > +        ;
> > > >      }
> > > >  }
> > > >  
> > > > -static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU
> > > > *cpu,
> > > > -                                                 uint64_t token,
> > > > int index)
> > > > +static inline ppc_hash_pte64_t ppc_hash64_load_hpte(PowerPCCPU
> > > > *cpu,
> > > > +                                                    uint64_t
> > > > token,
> > > > +                                                    int index)
> > > >  {
> > > >      CPUPPCState *env = &cpu->env;
> > > > +    ppc_hash_pte64_t pte;
> > > >      uint64_t addr;
> > > >  
> > > > -    addr = token + (index * HASH_PTE_SIZE_64) +
> > > > HASH_PTE_SIZE_64/2;
> > > > +    addr = token + (index * HASH_PTE_SIZE_64);
> > > >      if (env->external_htab) {
> > > > -        return  ldq_p((const void *)(uintptr_t)addr);
> > > > +        pte.pte0 = ldq_p((const void *)(uintptr_t)addr);
> > > > +        pte.pte1 = ldq_p((const void *)(uintptr_t)addr +
> > > > HASH_PTE_SIZE_64/2);
> > > >      } else {
> > > > -        return ldq_phys(CPU(cpu)->as, addr);
> > > > +        pte.pte0 = ldq_phys(CPU(cpu)->as, addr);
> > > > +        pte.pte1 = ldq_phys(CPU(cpu)->as, addr +
> > > > HASH_PTE_SIZE_64/2);
> > > >      }
> > > > -}
> > > >  
> > > > -typedef struct {
> > > > -    uint64_t pte0, pte1;
> > > > -} ppc_hash_pte64_t;
> > > > +    ppc_hash64_hpte_new_to_old(env, &pte.pte0, &pte.pte1);
> > > > +    return pte;
> > > > +}
> > > >  
> > > >  #endif /* CONFIG_USER_ONLY */
> > > >  
>

Suraj Jitindar Singh Feb. 10, 2017, 1:05 a.m. UTC | #5

On Fri, 2017-02-10 at 11:21 +1100, David Gibson wrote:
> On Fri, Feb 10, 2017 at 10:47:15AM +1100, Suraj Jitindar Singh wrote:
> > 
> > On Thu, 2017-02-09 at 14:08 +1100, Suraj Jitindar Singh wrote:
> > > 
> > > On Wed, 2017-02-01 at 15:28 +1100, David Gibson wrote:
> > > > 
> > > > 
> > > > On Fri, Jan 13, 2017 at 05:28:17PM +1100, Suraj Jitindar Singh
> > > > wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > The page table entry format was updated for the POWER9
> > > > > processor.
> > > > > 
> > > > > It was decided that kernels would used the old format
> > > > > irrespective
> > > > > with the translation occuring at the hypervisor level. Thus
> > > > > we
> > > > > convert
> > > > > between the old and new format when accessing the ptes. Since
> > > > > we
> > > > > need the
> > > > > whole pte to perform this conversion, we remove the old
> > > > > functions
> > > > > which
> > > > > accessed either the first or second doubleword and introduce
> > > > > a
> > > > > new
> > > > > functions which access the entire pte, returning the entry
> > > > > converted
> > > > > back to the old format (if required). Update call sites
> > > > > accordingly.
> > > > > 
> > > > > Signed-off-by: Suraj Jitindar Singh <[e-mail]
> > > > > >
> > > > > ---
> > > > >  hw/ppc/spapr_hcall.c    | 51 ++++++++++++++++++-------------
> > > > > ----
> > > > >  target/ppc/mmu-hash64.c | 13 +++++----
> > > > >  target/ppc/mmu-hash64.h | 71
> > > > > ++++++++++++++++++++++++++++++++++++-
> > > > > ------------
> > > > >  3 files changed, 86 insertions(+), 49 deletions(-)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > > > index 9a9bedf..9f0c20d 100644
> > > > > --- a/hw/ppc/spapr_hcall.c
> > > > > +++ b/hw/ppc/spapr_hcall.c
> > > > > @@ -125,7 +125,8 @@ static target_ulong h_enter(PowerPCCPU
> > > > > *cpu,
> > > > > sPAPRMachineState *spapr,
> > > > >          pte_index &= ~7ULL;
> > > > >          token = ppc_hash64_start_access(cpu, pte_index);
> > > > >          for (; index < 8; index++) {
> > > > > -            if (!(ppc_hash64_load_hpte0(cpu, token, index) &
> > > > > HPTE64_V_VALID)) {
> > > > > +            ppc_hash_pte64_t pte = ppc_hash64_load_hpte(cpu,
> > > > > token, index);
> > > > > +            if (!(pte.pte0 & HPTE64_V_VALID)) {
> > > > >                  break;
> > > > >              }
> > > > >          }
> > > > > @@ -134,8 +135,10 @@ static target_ulong h_enter(PowerPCCPU
> > > > > *cpu,
> > > > > sPAPRMachineState *spapr,
> > > > >              return H_PTEG_FULL;
> > > > >          }
> > > > >      } else {
> > > > > +        ppc_hash_pte64_t pte;
> > > > >          token = ppc_hash64_start_access(cpu, pte_index);
> > > > IIRC the only reason for the clumsy start_access / stop_access
> > > > stuff
> > > > was because we wanted to do the two loads separately (to avoid
> > > > loading
> > > > word1 in cases we don't need it).  Since you're combining both
> > > > loads
> > > > into a single helper, I think you can put the start_access /
> > > > stop_access into the same helper.
> > > > 
> > > > Or have I missed something.
> > > > 
> > > Yeah these functions can probably be merged together...
> > Actually, it looks like we need the start access and stop access
> > stuff
> > for kvm-hv where we load the pte into a buffer on start access and
> > free
> > it on stop access. So I think we still need to keep these two
> > functions
> > separate.
> Well, the question is are there any instances where we do more than
> just a load word0 + load word1 within the start/stop pair.  I don't
> remember off hand.
> 
Yeah, so the start/stop access gives you a whole page table entry group
and so on KVM this loads the whole pteg into a buffer. We then access
any number of the ptes within that group, meaning we need the
start/stop because otherwise we would be calling into kvm and copying
the ptes 8 times (once per pte in the group) to read a whole group
rather than read it once, access all 8 ptes and then free it.

I can't see a nice way of just having the load without the start/stop
access which doesn't require either multiple calls into kvm to read a
whole group or which requires you to keep a pointer to the group buffer
which is read in on the first access and freed on the last. But this
causes problems when you only access one pte from the group instead of
reading every single one, which does happen, so I think we really need
to leave it as is.
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > -        if (ppc_hash64_load_hpte0(cpu, token, 0) &
> > > > > HPTE64_V_VALID)
> > > > > {
> > > > > +        pte = ppc_hash64_load_hpte(cpu, token, 0);
> > > > > +        if (pte.pte0 & HPTE64_V_VALID) {
> > > > >              ppc_hash64_stop_access(cpu, token);
> > > > >              return H_PTEG_FULL;
> > > > >          }
> > > > > @@ -163,26 +166,25 @@ static RemoveResult
> > > > > remove_hpte(PowerPCCPU
> > > > > *cpu, target_ulong ptex,
> > > > >  {
> > > > >      CPUPPCState *env = &cpu->env;
> > > > >      uint64_t token;
> > > > > -    target_ulong v, r;
> > > > > +    ppc_hash_pte64_t pte;
> > > > >  
> > > > >      if (!valid_pte_index(env, 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);
> > > > > +    pte = ppc_hash64_load_hpte(cpu, token, 0);
> > > > >      ppc_hash64_stop_access(cpu, token);
> > > > >  
> > > > > -    if ((v & HPTE64_V_VALID) == 0 ||
> > > > > -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> > > > > -        ((flags & H_ANDCOND) && (v & avpn) != 0)) {
> > > > > +    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
> > > > > +        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn)
> > > > > ||
> > > > > +        ((flags & H_ANDCOND) && (pte.pte0 & avpn) != 0)) {
> > > > >          return REMOVE_NOT_FOUND;
> > > > >      }
> > > > > -    *vp = v;
> > > > > -    *rp = r;
> > > > > +    *vp = pte.pte0;
> > > > > +    *rp = pte.pte1;
> > > > >      ppc_hash64_store_hpte(cpu, ptex, HPTE64_V_HPTE_DIRTY,
> > > > > 0);
> > > > > -    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
> > > > > +    ppc_hash64_tlb_flush_hpte(cpu, ptex, pte.pte0,
> > > > > pte.pte1);
> > > > >      return REMOVE_SUCCESS;
> > > > >  }
> > > > >  
> > > > > @@ -293,35 +295,36 @@ static target_ulong
> > > > > h_protect(PowerPCCPU
> > > > > *cpu, sPAPRMachineState *spapr,
> > > > >      target_ulong flags = args[0];
> > > > >      target_ulong pte_index = args[1];
> > > > >      target_ulong avpn = args[2];
> > > > > +    ppc_hash_pte64_t pte;
> > > > >      uint64_t token;
> > > > > -    target_ulong v, r;
> > > > >  
> > > > >      if (!valid_pte_index(env, pte_index)) {
> > > > >          return H_PARAMETER;
> > > > >      }
> > > > >  
> > > > >      token = ppc_hash64_start_access(cpu, pte_index);
> > > > > -    v = ppc_hash64_load_hpte0(cpu, token, 0);
> > > > > -    r = ppc_hash64_load_hpte1(cpu, token, 0);
> > > > > +    pte = ppc_hash64_load_hpte(cpu, token, 0);
> > > > >      ppc_hash64_stop_access(cpu, token);
> > > > >  
> > > > > -    if ((v & HPTE64_V_VALID) == 0 ||
> > > > > -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> > > > > +    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
> > > > > +        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn))
> > > > > {
> > > > >          return H_NOT_FOUND;
> > > > >      }
> > > > >  
> > > > > -    r &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> > > > > -           HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> > > > > -    r |= (flags << 55) & HPTE64_R_PP0;
> > > > > -    r |= (flags << 48) & HPTE64_R_KEY_HI;
> > > > > -    r |= flags & (HPTE64_R_PP | HPTE64_R_N |
> > > > > HPTE64_R_KEY_LO);
> > > > > +    pte.pte1 &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> > > > > +                  HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> > > > > +    pte.pte1 |= (flags << 55) & HPTE64_R_PP0;
> > > > > +    pte.pte1 |= (flags << 48) & HPTE64_R_KEY_HI;
> > > > > +    pte.pte1 |= flags & (HPTE64_R_PP | HPTE64_R_N |
> > > > > HPTE64_R_KEY_LO);
> > > > >      ppc_hash64_store_hpte(cpu, pte_index,
> > > > > -                          (v & ~HPTE64_V_VALID) |
> > > > > HPTE64_V_HPTE_DIRTY, 0);
> > > > > -    ppc_hash64_tlb_flush_hpte(cpu, pte_index, v, r);
> > > > > +                          (pte.pte0 & ~HPTE64_V_VALID) |
> > > > > HPTE64_V_HPTE_DIRTY,
> > > > > +                          0);
> > > > > +    ppc_hash64_tlb_flush_hpte(cpu, pte_index, pte.pte0,
> > > > > pte.pte1);
> > > > >      /* Flush the tlb */
> > > > >      check_tlb_flush(env, true);
> > > > >      /* Don't need a memory barrier, due to qemu's global
> > > > > lock */
> > > > > -    ppc_hash64_store_hpte(cpu, pte_index, v |
> > > > > HPTE64_V_HPTE_DIRTY,
> > > > > r);
> > > > > +    ppc_hash64_store_hpte(cpu, pte_index, pte.pte0 |
> > > > > HPTE64_V_HPTE_DIRTY,
> > > > > +                          pte.pte1);
> > > > >      return H_SUCCESS;
> > > > >  }
> > > > >  
> > > > > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-
> > > > > hash64.c
> > > > > index b476b3f..03607d5 100644
> > > > > --- a/target/ppc/mmu-hash64.c
> > > > > +++ b/target/ppc/mmu-hash64.c
> > > > > @@ -515,7 +515,6 @@ static hwaddr
> > > > > ppc_hash64_pteg_search(PowerPCCPU
> > > > > *cpu, hwaddr hash,
> > > > >      CPUPPCState *env = &cpu->env;
> > > > >      int i;
> > > > >      uint64_t token;
> > > > > -    target_ulong pte0, pte1;
> > > > >      target_ulong pte_index;
> > > > >  
> > > > >      pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
> > > > > @@ -524,12 +523,11 @@ static hwaddr
> > > > > ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
> > > > >          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);
> > > > > +        ppc_hash_pte64_t entry = ppc_hash64_load_hpte(cpu,
> > > > > token,
> > > > > i);
> > > > Hm.  So the hypercalls using the access helpers which include
> > > > format
> > > > conversion makes sense to me - the hypercalls are defined to
> > > > use
> > > > the
> > > > old format, even on POWER9 AIUI.
> > > > 
> > > > It doesn't really make sense to me here, in what is essentially
> > > > the
> > > > physical implementation of the HPT lookup.  Shouldn't that be
> > > > using
> > > > the new format natively?
> > > > 
> > > > Basically it seems odd that you store things in the new format
> > > > in
> > > > memory, but convert to the old format on *all* accesses, not
> > > > just
> > > > the
> > > > hypercall ones which are defined that way.
> > > > 
> > > It seemed easier to just do the conversion rather than updating
> > > the
> > > code in all places. That said since this is modelling the
> > > hardware
> > > and
> > > should probably use the new format instead of just converting it
> > > and
> > > pretenting nothing changed.
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > >          /* This compares V, B, H (secondary) and the AVPN */
> > > > > -        if (HPTE64_V_COMPARE(pte0, ptem)) {
> > > > > -            *pshift = hpte_page_shift(sps, pte0, pte1);
> > > > > +        if (HPTE64_V_COMPARE(entry.pte0, ptem)) {
> > > > > +            *pshift = hpte_page_shift(sps, entry.pte0,
> > > > > entry.pte1);
> > > > >              /*
> > > > >               * If there is no match, ignore the PTE, it
> > > > > could
> > > > > simply
> > > > >               * be for a different segment size encoding and
> > > > > the
> > > > > @@ -543,8 +541,7 @@ static hwaddr
> > > > > ppc_hash64_pteg_search(PowerPCCPU
> > > > > *cpu, hwaddr hash,
> > > > >              /* We don't do anything with pshift yet as qemu
> > > > > TLB
> > > > > only deals
> > > > >               * with 4K pages anyway
> > > > >               */
> > > > > -            pte->pte0 = pte0;
> > > > > -            pte->pte1 = pte1;
> > > > > +            *pte = entry;
> > > > >              ppc_hash64_stop_access(cpu, token);
> > > > >              return (pte_index + i) * HASH_PTE_SIZE_64;
> > > > >          }
> > > > > @@ -924,6 +921,8 @@ void ppc_hash64_store_hpte(PowerPCCPU
> > > > > *cpu,
> > > > >  {
> > > > >      CPUPPCState *env = &cpu->env;
> > > > >  
> > > > > +    ppc_hash64_hpte_old_to_new(env, &pte0, &pte1);
> > > > > +
> > > > >      if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> > > > >          kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
> > > > >          return;
> > > > > diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-
> > > > > hash64.h
> > > > > index ab5d347..73d7ce4 100644
> > > > > --- a/target/ppc/mmu-hash64.h
> > > > > +++ b/target/ppc/mmu-hash64.h
> > > > > @@ -60,6 +60,7 @@ void ppc_hash64_update_rmls(CPUPPCState
> > > > > *env);
> > > > >  #define HASH_PTE_SIZE_64        16
> > > > >  #define HASH_PTEG_SIZE_64       (HASH_PTE_SIZE_64 *
> > > > > HPTES_PER_GROUP)
> > > > >  
> > > > > +#define HPTE64_V_3_00_COMMON    0x000fffffffffffffULL
> > > > >  #define HPTE64_V_SSIZE_SHIFT    62
> > > > >  #define HPTE64_V_AVPN_SHIFT     7
> > > > >  #define HPTE64_V_AVPN           0x3fffffffffffff80ULL
> > > > > @@ -69,9 +70,12 @@ void ppc_hash64_update_rmls(CPUPPCState
> > > > > *env);
> > > > >  #define HPTE64_V_SECONDARY      0x0000000000000002ULL
> > > > >  #define HPTE64_V_VALID          0x0000000000000001ULL
> > > > >  
> > > > > +#define HPTE64_R_3_00_COMMON    0xf1ffffffffffffffULL
> > > > >  #define HPTE64_R_PP0            0x8000000000000000ULL
> > > > >  #define HPTE64_R_TS             0x4000000000000000ULL
> > > > >  #define HPTE64_R_KEY_HI         0x3000000000000000ULL
> > > > > +#define HPTE64_R_SSIZE_SHIFT    58
> > > > > +#define HPTE64_R_SSIZE_MASK     (3ULL <<
> > > > > HPTE64_R_SSIZE_SHIFT)
> > > > >  #define HPTE64_R_RPN_SHIFT      12
> > > > >  #define HPTE64_R_RPN            0x0ffffffffffff000ULL
> > > > >  #define HPTE64_R_FLAGS          0x00000000000003ffULL
> > > > > @@ -91,6 +95,10 @@ void ppc_hash64_update_rmls(CPUPPCState
> > > > > *env);
> > > > >  #define HPTE64_V_1TB_SEG        0x4000000000000000ULL
> > > > >  #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
> > > > >  
> > > > > +typedef struct {
> > > > > +    uint64_t pte0, pte1;
> > > > > +} ppc_hash_pte64_t;
> > > > > +
> > > > >  void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong
> > > > > value,
> > > > >                           Error **errp);
> > > > >  void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt,
> > > > > int
> > > > > shift,
> > > > > @@ -99,37 +107,64 @@ void
> > > > > ppc_hash64_set_external_hpt(PowerPCCPU
> > > > > *cpu, void *hpt, int shift,
> > > > >  uint64_t ppc_hash64_start_access(PowerPCCPU *cpu,
> > > > > target_ulong
> > > > > pte_index);
> > > > >  void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t
> > > > > token);
> > > > >  
> > > > > -static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU
> > > > > *cpu,
> > > > > -                                                 uint64_t
> > > > > token,
> > > > > int index)
> > > > > +static inline void ppc_hash64_hpte_old_to_new(CPUPPCState
> > > > > *env,
> > > > > +                                              target_ulong
> > > > > *pte0,
> > > > > +                                              target_ulong
> > > > > *pte1)
> > > > >  {
> > > > > -    CPUPPCState *env = &cpu->env;
> > > > > -    uint64_t addr;
> > > > > +    switch (env->mmu_model) {
> > > > > +    case POWERPC_MMU_3_00:
> > > > > +        /*
> > > > > +         * v3.00 of the ISA moved the B field to the second
> > > > > doubleword and
> > > > > +         * shortened the abbreviated virtual address and
> > > > > abbreviated real page
> > > > > +         * number fields
> > > > > +         */
> > > > > +        *pte1 = (*pte1 & HPTE64_R_3_00_COMMON) |
> > > > > +                ((*pte0 >> HPTE64_V_SSIZE_SHIFT) <<
> > > > > HPTE64_R_SSIZE_SHIFT);
> > > > > +        *pte0 = *pte0 & HPTE64_V_3_00_COMMON;
> > > > > +    default:
> > > > > +        ;
> > > > > +    }
> > > > > +}
> > > > >  
> > > > > -    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);
> > > > > +static inline void ppc_hash64_hpte_new_to_old(CPUPPCState
> > > > > *env,
> > > > > +                                              target_ulong
> > > > > *pte0,
> > > > > +                                              target_ulong
> > > > > *pte1)
> > > > > +{
> > > > > +    switch (env->mmu_model) {
> > > > > +    case POWERPC_MMU_3_00:
> > > > > +        /*
> > > > > +         * v3.00 of the ISA moved the B field to the second
> > > > > doubleword and
> > > > > +         * shortened the abbreviated virtual address and
> > > > > abbreviated real page
> > > > > +         * number fields
> > > > > +         */
> > > > > +        *pte0 = (*pte0 & HPTE64_V_3_00_COMMON) | ((*pte1 &
> > > > > HPTE64_R_SSIZE_MASK)
> > > > > +                << (HPTE64_V_SSIZE_SHIFT -
> > > > > HPTE64_R_SSIZE_SHIFT));
> > > > > +        *pte1 = *pte1 & HPTE64_R_3_00_COMMON;
> > > > > +    default:
> > > > > +        ;
> > > > >      }
> > > > >  }
> > > > >  
> > > > > -static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU
> > > > > *cpu,
> > > > > -                                                 uint64_t
> > > > > token,
> > > > > int index)
> > > > > +static inline ppc_hash_pte64_t
> > > > > ppc_hash64_load_hpte(PowerPCCPU
> > > > > *cpu,
> > > > > +                                                    uint64_t
> > > > > token,
> > > > > +                                                    int
> > > > > index)
> > > > >  {
> > > > >      CPUPPCState *env = &cpu->env;
> > > > > +    ppc_hash_pte64_t pte;
> > > > >      uint64_t addr;
> > > > >  
> > > > > -    addr = token + (index * HASH_PTE_SIZE_64) +
> > > > > HASH_PTE_SIZE_64/2;
> > > > > +    addr = token + (index * HASH_PTE_SIZE_64);
> > > > >      if (env->external_htab) {
> > > > > -        return  ldq_p((const void *)(uintptr_t)addr);
> > > > > +        pte.pte0 = ldq_p((const void *)(uintptr_t)addr);
> > > > > +        pte.pte1 = ldq_p((const void *)(uintptr_t)addr +
> > > > > HASH_PTE_SIZE_64/2);
> > > > >      } else {
> > > > > -        return ldq_phys(CPU(cpu)->as, addr);
> > > > > +        pte.pte0 = ldq_phys(CPU(cpu)->as, addr);
> > > > > +        pte.pte1 = ldq_phys(CPU(cpu)->as, addr +
> > > > > HASH_PTE_SIZE_64/2);
> > > > >      }
> > > > > -}
> > > > >  
> > > > > -typedef struct {
> > > > > -    uint64_t pte0, pte1;
> > > > > -} ppc_hash_pte64_t;
> > > > > +    ppc_hash64_hpte_new_to_old(env, &pte.pte0, &pte.pte1);
> > > > > +    return pte;
> > > > > +}
> > > > >  
> > > > >  #endif /* CONFIG_USER_ONLY */
> > > > >

David Gibson Feb. 10, 2017, 2:24 a.m. UTC | #6

On Fri, Feb 10, 2017 at 12:05:38PM +1100, Suraj Jitindar Singh wrote:
> On Fri, 2017-02-10 at 11:21 +1100, David Gibson wrote:
> > On Fri, Feb 10, 2017 at 10:47:15AM +1100, Suraj Jitindar Singh wrote:
> > > 
> > > On Thu, 2017-02-09 at 14:08 +1100, Suraj Jitindar Singh wrote:
> > > > 
> > > > On Wed, 2017-02-01 at 15:28 +1100, David Gibson wrote:
> > > > > 
> > > > > 
> > > > > On Fri, Jan 13, 2017 at 05:28:17PM +1100, Suraj Jitindar Singh
> > > > > wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > The page table entry format was updated for the POWER9
> > > > > > processor.
> > > > > > 
> > > > > > It was decided that kernels would used the old format
> > > > > > irrespective
> > > > > > with the translation occuring at the hypervisor level. Thus
> > > > > > we
> > > > > > convert
> > > > > > between the old and new format when accessing the ptes. Since
> > > > > > we
> > > > > > need the
> > > > > > whole pte to perform this conversion, we remove the old
> > > > > > functions
> > > > > > which
> > > > > > accessed either the first or second doubleword and introduce
> > > > > > a
> > > > > > new
> > > > > > functions which access the entire pte, returning the entry
> > > > > > converted
> > > > > > back to the old format (if required). Update call sites
> > > > > > accordingly.
> > > > > > 
> > > > > > Signed-off-by: Suraj Jitindar Singh <[e-mail]
> > > > > > >
> > > > > > ---
> > > > > >  hw/ppc/spapr_hcall.c    | 51 ++++++++++++++++++-------------
> > > > > > ----
> > > > > >  target/ppc/mmu-hash64.c | 13 +++++----
> > > > > >  target/ppc/mmu-hash64.h | 71
> > > > > > ++++++++++++++++++++++++++++++++++++-
> > > > > > ------------
> > > > > >  3 files changed, 86 insertions(+), 49 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > > > > index 9a9bedf..9f0c20d 100644
> > > > > > --- a/hw/ppc/spapr_hcall.c
> > > > > > +++ b/hw/ppc/spapr_hcall.c
> > > > > > @@ -125,7 +125,8 @@ static target_ulong h_enter(PowerPCCPU
> > > > > > *cpu,
> > > > > > sPAPRMachineState *spapr,
> > > > > >          pte_index &= ~7ULL;
> > > > > >          token = ppc_hash64_start_access(cpu, pte_index);
> > > > > >          for (; index < 8; index++) {
> > > > > > -            if (!(ppc_hash64_load_hpte0(cpu, token, index) &
> > > > > > HPTE64_V_VALID)) {
> > > > > > +            ppc_hash_pte64_t pte = ppc_hash64_load_hpte(cpu,
> > > > > > token, index);
> > > > > > +            if (!(pte.pte0 & HPTE64_V_VALID)) {
> > > > > >                  break;
> > > > > >              }
> > > > > >          }
> > > > > > @@ -134,8 +135,10 @@ static target_ulong h_enter(PowerPCCPU
> > > > > > *cpu,
> > > > > > sPAPRMachineState *spapr,
> > > > > >              return H_PTEG_FULL;
> > > > > >          }
> > > > > >      } else {
> > > > > > +        ppc_hash_pte64_t pte;
> > > > > >          token = ppc_hash64_start_access(cpu, pte_index);
> > > > > IIRC the only reason for the clumsy start_access / stop_access
> > > > > stuff
> > > > > was because we wanted to do the two loads separately (to avoid
> > > > > loading
> > > > > word1 in cases we don't need it).  Since you're combining both
> > > > > loads
> > > > > into a single helper, I think you can put the start_access /
> > > > > stop_access into the same helper.
> > > > > 
> > > > > Or have I missed something.
> > > > > 
> > > > Yeah these functions can probably be merged together...
> > > Actually, it looks like we need the start access and stop access
> > > stuff
> > > for kvm-hv where we load the pte into a buffer on start access and
> > > free
> > > it on stop access. So I think we still need to keep these two
> > > functions
> > > separate.
> > Well, the question is are there any instances where we do more than
> > just a load word0 + load word1 within the start/stop pair.  I don't
> > remember off hand.
> > 
> Yeah, so the start/stop access gives you a whole page table entry group
> and so on KVM this loads the whole pteg into a buffer. We then access
> any number of the ptes within that group, meaning we need the
> start/stop because otherwise we would be calling into kvm and copying
> the ptes 8 times (once per pte in the group) to read a whole group
> rather than read it once, access all 8 ptes and then free it.
> 
> I can't see a nice way of just having the load without the start/stop
> access which doesn't require either multiple calls into kvm to read a
> whole group or which requires you to keep a pointer to the group buffer
> which is read in on the first access and freed on the last. But this
> causes problems when you only access one pte from the group instead of
> reading every single one, which does happen, so I think we really need
> to leave it as is.

Ok, fair enough.

> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > -        if (ppc_hash64_load_hpte0(cpu, token, 0) &
> > > > > > HPTE64_V_VALID)
> > > > > > {
> > > > > > +        pte = ppc_hash64_load_hpte(cpu, token, 0);
> > > > > > +        if (pte.pte0 & HPTE64_V_VALID) {
> > > > > >              ppc_hash64_stop_access(cpu, token);
> > > > > >              return H_PTEG_FULL;
> > > > > >          }
> > > > > > @@ -163,26 +166,25 @@ static RemoveResult
> > > > > > remove_hpte(PowerPCCPU
> > > > > > *cpu, target_ulong ptex,
> > > > > >  {
> > > > > >      CPUPPCState *env = &cpu->env;
> > > > > >      uint64_t token;
> > > > > > -    target_ulong v, r;
> > > > > > +    ppc_hash_pte64_t pte;
> > > > > >  
> > > > > >      if (!valid_pte_index(env, 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);
> > > > > > +    pte = ppc_hash64_load_hpte(cpu, token, 0);
> > > > > >      ppc_hash64_stop_access(cpu, token);
> > > > > >  
> > > > > > -    if ((v & HPTE64_V_VALID) == 0 ||
> > > > > > -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> > > > > > -        ((flags & H_ANDCOND) && (v & avpn) != 0)) {
> > > > > > +    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
> > > > > > +        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn)
> > > > > > ||
> > > > > > +        ((flags & H_ANDCOND) && (pte.pte0 & avpn) != 0)) {
> > > > > >          return REMOVE_NOT_FOUND;
> > > > > >      }
> > > > > > -    *vp = v;
> > > > > > -    *rp = r;
> > > > > > +    *vp = pte.pte0;
> > > > > > +    *rp = pte.pte1;
> > > > > >      ppc_hash64_store_hpte(cpu, ptex, HPTE64_V_HPTE_DIRTY,
> > > > > > 0);
> > > > > > -    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
> > > > > > +    ppc_hash64_tlb_flush_hpte(cpu, ptex, pte.pte0,
> > > > > > pte.pte1);
> > > > > >      return REMOVE_SUCCESS;
> > > > > >  }
> > > > > >  
> > > > > > @@ -293,35 +295,36 @@ static target_ulong
> > > > > > h_protect(PowerPCCPU
> > > > > > *cpu, sPAPRMachineState *spapr,
> > > > > >      target_ulong flags = args[0];
> > > > > >      target_ulong pte_index = args[1];
> > > > > >      target_ulong avpn = args[2];
> > > > > > +    ppc_hash_pte64_t pte;
> > > > > >      uint64_t token;
> > > > > > -    target_ulong v, r;
> > > > > >  
> > > > > >      if (!valid_pte_index(env, pte_index)) {
> > > > > >          return H_PARAMETER;
> > > > > >      }
> > > > > >  
> > > > > >      token = ppc_hash64_start_access(cpu, pte_index);
> > > > > > -    v = ppc_hash64_load_hpte0(cpu, token, 0);
> > > > > > -    r = ppc_hash64_load_hpte1(cpu, token, 0);
> > > > > > +    pte = ppc_hash64_load_hpte(cpu, token, 0);
> > > > > >      ppc_hash64_stop_access(cpu, token);
> > > > > >  
> > > > > > -    if ((v & HPTE64_V_VALID) == 0 ||
> > > > > > -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> > > > > > +    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
> > > > > > +        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn))
> > > > > > {
> > > > > >          return H_NOT_FOUND;
> > > > > >      }
> > > > > >  
> > > > > > -    r &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> > > > > > -           HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> > > > > > -    r |= (flags << 55) & HPTE64_R_PP0;
> > > > > > -    r |= (flags << 48) & HPTE64_R_KEY_HI;
> > > > > > -    r |= flags & (HPTE64_R_PP | HPTE64_R_N |
> > > > > > HPTE64_R_KEY_LO);
> > > > > > +    pte.pte1 &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> > > > > > +                  HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> > > > > > +    pte.pte1 |= (flags << 55) & HPTE64_R_PP0;
> > > > > > +    pte.pte1 |= (flags << 48) & HPTE64_R_KEY_HI;
> > > > > > +    pte.pte1 |= flags & (HPTE64_R_PP | HPTE64_R_N |
> > > > > > HPTE64_R_KEY_LO);
> > > > > >      ppc_hash64_store_hpte(cpu, pte_index,
> > > > > > -                          (v & ~HPTE64_V_VALID) |
> > > > > > HPTE64_V_HPTE_DIRTY, 0);
> > > > > > -    ppc_hash64_tlb_flush_hpte(cpu, pte_index, v, r);
> > > > > > +                          (pte.pte0 & ~HPTE64_V_VALID) |
> > > > > > HPTE64_V_HPTE_DIRTY,
> > > > > > +                          0);
> > > > > > +    ppc_hash64_tlb_flush_hpte(cpu, pte_index, pte.pte0,
> > > > > > pte.pte1);
> > > > > >      /* Flush the tlb */
> > > > > >      check_tlb_flush(env, true);
> > > > > >      /* Don't need a memory barrier, due to qemu's global
> > > > > > lock */
> > > > > > -    ppc_hash64_store_hpte(cpu, pte_index, v |
> > > > > > HPTE64_V_HPTE_DIRTY,
> > > > > > r);
> > > > > > +    ppc_hash64_store_hpte(cpu, pte_index, pte.pte0 |
> > > > > > HPTE64_V_HPTE_DIRTY,
> > > > > > +                          pte.pte1);
> > > > > >      return H_SUCCESS;
> > > > > >  }
> > > > > >  
> > > > > > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-
> > > > > > hash64.c
> > > > > > index b476b3f..03607d5 100644
> > > > > > --- a/target/ppc/mmu-hash64.c
> > > > > > +++ b/target/ppc/mmu-hash64.c
> > > > > > @@ -515,7 +515,6 @@ static hwaddr
> > > > > > ppc_hash64_pteg_search(PowerPCCPU
> > > > > > *cpu, hwaddr hash,
> > > > > >      CPUPPCState *env = &cpu->env;
> > > > > >      int i;
> > > > > >      uint64_t token;
> > > > > > -    target_ulong pte0, pte1;
> > > > > >      target_ulong pte_index;
> > > > > >  
> > > > > >      pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
> > > > > > @@ -524,12 +523,11 @@ static hwaddr
> > > > > > ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
> > > > > >          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);
> > > > > > +        ppc_hash_pte64_t entry = ppc_hash64_load_hpte(cpu,
> > > > > > token,
> > > > > > i);
> > > > > Hm.  So the hypercalls using the access helpers which include
> > > > > format
> > > > > conversion makes sense to me - the hypercalls are defined to
> > > > > use
> > > > > the
> > > > > old format, even on POWER9 AIUI.
> > > > > 
> > > > > It doesn't really make sense to me here, in what is essentially
> > > > > the
> > > > > physical implementation of the HPT lookup.  Shouldn't that be
> > > > > using
> > > > > the new format natively?
> > > > > 
> > > > > Basically it seems odd that you store things in the new format
> > > > > in
> > > > > memory, but convert to the old format on *all* accesses, not
> > > > > just
> > > > > the
> > > > > hypercall ones which are defined that way.
> > > > > 
> > > > It seemed easier to just do the conversion rather than updating
> > > > the
> > > > code in all places. That said since this is modelling the
> > > > hardware
> > > > and
> > > > should probably use the new format instead of just converting it
> > > > and
> > > > pretenting nothing changed.
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > >          /* This compares V, B, H (secondary) and the AVPN */
> > > > > > -        if (HPTE64_V_COMPARE(pte0, ptem)) {
> > > > > > -            *pshift = hpte_page_shift(sps, pte0, pte1);
> > > > > > +        if (HPTE64_V_COMPARE(entry.pte0, ptem)) {
> > > > > > +            *pshift = hpte_page_shift(sps, entry.pte0,
> > > > > > entry.pte1);
> > > > > >              /*
> > > > > >               * If there is no match, ignore the PTE, it
> > > > > > could
> > > > > > simply
> > > > > >               * be for a different segment size encoding and
> > > > > > the
> > > > > > @@ -543,8 +541,7 @@ static hwaddr
> > > > > > ppc_hash64_pteg_search(PowerPCCPU
> > > > > > *cpu, hwaddr hash,
> > > > > >              /* We don't do anything with pshift yet as qemu
> > > > > > TLB
> > > > > > only deals
> > > > > >               * with 4K pages anyway
> > > > > >               */
> > > > > > -            pte->pte0 = pte0;
> > > > > > -            pte->pte1 = pte1;
> > > > > > +            *pte = entry;
> > > > > >              ppc_hash64_stop_access(cpu, token);
> > > > > >              return (pte_index + i) * HASH_PTE_SIZE_64;
> > > > > >          }
> > > > > > @@ -924,6 +921,8 @@ void ppc_hash64_store_hpte(PowerPCCPU
> > > > > > *cpu,
> > > > > >  {
> > > > > >      CPUPPCState *env = &cpu->env;
> > > > > >  
> > > > > > +    ppc_hash64_hpte_old_to_new(env, &pte0, &pte1);
> > > > > > +
> > > > > >      if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
> > > > > >          kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
> > > > > >          return;
> > > > > > diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-
> > > > > > hash64.h
> > > > > > index ab5d347..73d7ce4 100644
> > > > > > --- a/target/ppc/mmu-hash64.h
> > > > > > +++ b/target/ppc/mmu-hash64.h
> > > > > > @@ -60,6 +60,7 @@ void ppc_hash64_update_rmls(CPUPPCState
> > > > > > *env);
> > > > > >  #define HASH_PTE_SIZE_64        16
> > > > > >  #define HASH_PTEG_SIZE_64       (HASH_PTE_SIZE_64 *
> > > > > > HPTES_PER_GROUP)
> > > > > >  
> > > > > > +#define HPTE64_V_3_00_COMMON    0x000fffffffffffffULL
> > > > > >  #define HPTE64_V_SSIZE_SHIFT    62
> > > > > >  #define HPTE64_V_AVPN_SHIFT     7
> > > > > >  #define HPTE64_V_AVPN           0x3fffffffffffff80ULL
> > > > > > @@ -69,9 +70,12 @@ void ppc_hash64_update_rmls(CPUPPCState
> > > > > > *env);
> > > > > >  #define HPTE64_V_SECONDARY      0x0000000000000002ULL
> > > > > >  #define HPTE64_V_VALID          0x0000000000000001ULL
> > > > > >  
> > > > > > +#define HPTE64_R_3_00_COMMON    0xf1ffffffffffffffULL
> > > > > >  #define HPTE64_R_PP0            0x8000000000000000ULL
> > > > > >  #define HPTE64_R_TS             0x4000000000000000ULL
> > > > > >  #define HPTE64_R_KEY_HI         0x3000000000000000ULL
> > > > > > +#define HPTE64_R_SSIZE_SHIFT    58
> > > > > > +#define HPTE64_R_SSIZE_MASK     (3ULL <<
> > > > > > HPTE64_R_SSIZE_SHIFT)
> > > > > >  #define HPTE64_R_RPN_SHIFT      12
> > > > > >  #define HPTE64_R_RPN            0x0ffffffffffff000ULL
> > > > > >  #define HPTE64_R_FLAGS          0x00000000000003ffULL
> > > > > > @@ -91,6 +95,10 @@ void ppc_hash64_update_rmls(CPUPPCState
> > > > > > *env);
> > > > > >  #define HPTE64_V_1TB_SEG        0x4000000000000000ULL
> > > > > >  #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
> > > > > >  
> > > > > > +typedef struct {
> > > > > > +    uint64_t pte0, pte1;
> > > > > > +} ppc_hash_pte64_t;
> > > > > > +
> > > > > >  void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong
> > > > > > value,
> > > > > >                           Error **errp);
> > > > > >  void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt,
> > > > > > int
> > > > > > shift,
> > > > > > @@ -99,37 +107,64 @@ void
> > > > > > ppc_hash64_set_external_hpt(PowerPCCPU
> > > > > > *cpu, void *hpt, int shift,
> > > > > >  uint64_t ppc_hash64_start_access(PowerPCCPU *cpu,
> > > > > > target_ulong
> > > > > > pte_index);
> > > > > >  void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t
> > > > > > token);
> > > > > >  
> > > > > > -static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU
> > > > > > *cpu,
> > > > > > -                                                 uint64_t
> > > > > > token,
> > > > > > int index)
> > > > > > +static inline void ppc_hash64_hpte_old_to_new(CPUPPCState
> > > > > > *env,
> > > > > > +                                              target_ulong
> > > > > > *pte0,
> > > > > > +                                              target_ulong
> > > > > > *pte1)
> > > > > >  {
> > > > > > -    CPUPPCState *env = &cpu->env;
> > > > > > -    uint64_t addr;
> > > > > > +    switch (env->mmu_model) {
> > > > > > +    case POWERPC_MMU_3_00:
> > > > > > +        /*
> > > > > > +         * v3.00 of the ISA moved the B field to the second
> > > > > > doubleword and
> > > > > > +         * shortened the abbreviated virtual address and
> > > > > > abbreviated real page
> > > > > > +         * number fields
> > > > > > +         */
> > > > > > +        *pte1 = (*pte1 & HPTE64_R_3_00_COMMON) |
> > > > > > +                ((*pte0 >> HPTE64_V_SSIZE_SHIFT) <<
> > > > > > HPTE64_R_SSIZE_SHIFT);
> > > > > > +        *pte0 = *pte0 & HPTE64_V_3_00_COMMON;
> > > > > > +    default:
> > > > > > +        ;
> > > > > > +    }
> > > > > > +}
> > > > > >  
> > > > > > -    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);
> > > > > > +static inline void ppc_hash64_hpte_new_to_old(CPUPPCState
> > > > > > *env,
> > > > > > +                                              target_ulong
> > > > > > *pte0,
> > > > > > +                                              target_ulong
> > > > > > *pte1)
> > > > > > +{
> > > > > > +    switch (env->mmu_model) {
> > > > > > +    case POWERPC_MMU_3_00:
> > > > > > +        /*
> > > > > > +         * v3.00 of the ISA moved the B field to the second
> > > > > > doubleword and
> > > > > > +         * shortened the abbreviated virtual address and
> > > > > > abbreviated real page
> > > > > > +         * number fields
> > > > > > +         */
> > > > > > +        *pte0 = (*pte0 & HPTE64_V_3_00_COMMON) | ((*pte1 &
> > > > > > HPTE64_R_SSIZE_MASK)
> > > > > > +                << (HPTE64_V_SSIZE_SHIFT -
> > > > > > HPTE64_R_SSIZE_SHIFT));
> > > > > > +        *pte1 = *pte1 & HPTE64_R_3_00_COMMON;
> > > > > > +    default:
> > > > > > +        ;
> > > > > >      }
> > > > > >  }
> > > > > >  
> > > > > > -static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU
> > > > > > *cpu,
> > > > > > -                                                 uint64_t
> > > > > > token,
> > > > > > int index)
> > > > > > +static inline ppc_hash_pte64_t
> > > > > > ppc_hash64_load_hpte(PowerPCCPU
> > > > > > *cpu,
> > > > > > +                                                    uint64_t
> > > > > > token,
> > > > > > +                                                    int
> > > > > > index)
> > > > > >  {
> > > > > >      CPUPPCState *env = &cpu->env;
> > > > > > +    ppc_hash_pte64_t pte;
> > > > > >      uint64_t addr;
> > > > > >  
> > > > > > -    addr = token + (index * HASH_PTE_SIZE_64) +
> > > > > > HASH_PTE_SIZE_64/2;
> > > > > > +    addr = token + (index * HASH_PTE_SIZE_64);
> > > > > >      if (env->external_htab) {
> > > > > > -        return  ldq_p((const void *)(uintptr_t)addr);
> > > > > > +        pte.pte0 = ldq_p((const void *)(uintptr_t)addr);
> > > > > > +        pte.pte1 = ldq_p((const void *)(uintptr_t)addr +
> > > > > > HASH_PTE_SIZE_64/2);
> > > > > >      } else {
> > > > > > -        return ldq_phys(CPU(cpu)->as, addr);
> > > > > > +        pte.pte0 = ldq_phys(CPU(cpu)->as, addr);
> > > > > > +        pte.pte1 = ldq_phys(CPU(cpu)->as, addr +
> > > > > > HASH_PTE_SIZE_64/2);
> > > > > >      }
> > > > > > -}
> > > > > >  
> > > > > > -typedef struct {
> > > > > > -    uint64_t pte0, pte1;
> > > > > > -} ppc_hash_pte64_t;
> > > > > > +    ppc_hash64_hpte_new_to_old(env, &pte.pte0, &pte.pte1);
> > > > > > +    return pte;
> > > > > > +}
> > > > > >  
> > > > > >  #endif /* CONFIG_USER_ONLY */
> > > > > >  
>

Patch

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 9a9bedf..9f0c20d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -125,7 +125,8 @@  static target_ulong h_enter(PowerPCCPU *cpu, sPAPRMachineState *spapr,
         pte_index &= ~7ULL;
         token = ppc_hash64_start_access(cpu, pte_index);
         for (; index < 8; index++) {
-            if (!(ppc_hash64_load_hpte0(cpu, token, index) & HPTE64_V_VALID)) {
+            ppc_hash_pte64_t pte = ppc_hash64_load_hpte(cpu, token, index);
+            if (!(pte.pte0 & HPTE64_V_VALID)) {
                 break;
             }
         }
@@ -134,8 +135,10 @@  static target_ulong h_enter(PowerPCCPU *cpu, sPAPRMachineState *spapr,
             return H_PTEG_FULL;
         }
     } else {
+        ppc_hash_pte64_t pte;
         token = ppc_hash64_start_access(cpu, pte_index);
-        if (ppc_hash64_load_hpte0(cpu, token, 0) & HPTE64_V_VALID) {
+        pte = ppc_hash64_load_hpte(cpu, token, 0);
+        if (pte.pte0 & HPTE64_V_VALID) {
             ppc_hash64_stop_access(cpu, token);
             return H_PTEG_FULL;
         }
@@ -163,26 +166,25 @@  static RemoveResult remove_hpte(PowerPCCPU *cpu, target_ulong ptex,
 {
     CPUPPCState *env = &cpu->env;
     uint64_t token;
-    target_ulong v, r;
+    ppc_hash_pte64_t pte;
 
     if (!valid_pte_index(env, 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);
+    pte = ppc_hash64_load_hpte(cpu, token, 0);
     ppc_hash64_stop_access(cpu, token);
 
-    if ((v & HPTE64_V_VALID) == 0 ||
-        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
-        ((flags & H_ANDCOND) && (v & avpn) != 0)) {
+    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
+        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn) ||
+        ((flags & H_ANDCOND) && (pte.pte0 & avpn) != 0)) {
         return REMOVE_NOT_FOUND;
     }
-    *vp = v;
-    *rp = r;
+    *vp = pte.pte0;
+    *rp = pte.pte1;
     ppc_hash64_store_hpte(cpu, ptex, HPTE64_V_HPTE_DIRTY, 0);
-    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
+    ppc_hash64_tlb_flush_hpte(cpu, ptex, pte.pte0, pte.pte1);
     return REMOVE_SUCCESS;
 }
 
@@ -293,35 +295,36 @@  static target_ulong h_protect(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     target_ulong flags = args[0];
     target_ulong pte_index = args[1];
     target_ulong avpn = args[2];
+    ppc_hash_pte64_t pte;
     uint64_t token;
-    target_ulong v, r;
 
     if (!valid_pte_index(env, pte_index)) {
         return H_PARAMETER;
     }
 
     token = ppc_hash64_start_access(cpu, pte_index);
-    v = ppc_hash64_load_hpte0(cpu, token, 0);
-    r = ppc_hash64_load_hpte1(cpu, token, 0);
+    pte = ppc_hash64_load_hpte(cpu, token, 0);
     ppc_hash64_stop_access(cpu, token);
 
-    if ((v & HPTE64_V_VALID) == 0 ||
-        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
+    if ((pte.pte0 & HPTE64_V_VALID) == 0 ||
+        ((flags & H_AVPN) && (pte.pte0 & ~0x7fULL) != avpn)) {
         return H_NOT_FOUND;
     }
 
-    r &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
-           HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
-    r |= (flags << 55) & HPTE64_R_PP0;
-    r |= (flags << 48) & HPTE64_R_KEY_HI;
-    r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
+    pte.pte1 &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
+                  HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
+    pte.pte1 |= (flags << 55) & HPTE64_R_PP0;
+    pte.pte1 |= (flags << 48) & HPTE64_R_KEY_HI;
+    pte.pte1 |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
     ppc_hash64_store_hpte(cpu, pte_index,
-                          (v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY, 0);
-    ppc_hash64_tlb_flush_hpte(cpu, pte_index, v, r);
+                          (pte.pte0 & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY,
+                          0);
+    ppc_hash64_tlb_flush_hpte(cpu, pte_index, pte.pte0, pte.pte1);
     /* Flush the tlb */
     check_tlb_flush(env, true);
     /* Don't need a memory barrier, due to qemu's global lock */
-    ppc_hash64_store_hpte(cpu, pte_index, v | HPTE64_V_HPTE_DIRTY, r);
+    ppc_hash64_store_hpte(cpu, pte_index, pte.pte0 | HPTE64_V_HPTE_DIRTY,
+                          pte.pte1);
     return H_SUCCESS;
 }
 
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index b476b3f..03607d5 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -515,7 +515,6 @@  static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
     CPUPPCState *env = &cpu->env;
     int i;
     uint64_t token;
-    target_ulong pte0, pte1;
     target_ulong pte_index;
 
     pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
@@ -524,12 +523,11 @@  static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
         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);
+        ppc_hash_pte64_t entry = ppc_hash64_load_hpte(cpu, token, i);
 
         /* This compares V, B, H (secondary) and the AVPN */
-        if (HPTE64_V_COMPARE(pte0, ptem)) {
-            *pshift = hpte_page_shift(sps, pte0, pte1);
+        if (HPTE64_V_COMPARE(entry.pte0, ptem)) {
+            *pshift = hpte_page_shift(sps, entry.pte0, entry.pte1);
             /*
              * If there is no match, ignore the PTE, it could simply
              * be for a different segment size encoding and the
@@ -543,8 +541,7 @@  static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
             /* We don't do anything with pshift yet as qemu TLB only deals
              * with 4K pages anyway
              */
-            pte->pte0 = pte0;
-            pte->pte1 = pte1;
+            *pte = entry;
             ppc_hash64_stop_access(cpu, token);
             return (pte_index + i) * HASH_PTE_SIZE_64;
         }
@@ -924,6 +921,8 @@  void ppc_hash64_store_hpte(PowerPCCPU *cpu,
 {
     CPUPPCState *env = &cpu->env;
 
+    ppc_hash64_hpte_old_to_new(env, &pte0, &pte1);
+
     if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
         kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
         return;
diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
index ab5d347..73d7ce4 100644
--- a/target/ppc/mmu-hash64.h
+++ b/target/ppc/mmu-hash64.h
@@ -60,6 +60,7 @@  void ppc_hash64_update_rmls(CPUPPCState *env);
 #define HASH_PTE_SIZE_64        16
 #define HASH_PTEG_SIZE_64       (HASH_PTE_SIZE_64 * HPTES_PER_GROUP)
 
+#define HPTE64_V_3_00_COMMON    0x000fffffffffffffULL
 #define HPTE64_V_SSIZE_SHIFT    62
 #define HPTE64_V_AVPN_SHIFT     7
 #define HPTE64_V_AVPN           0x3fffffffffffff80ULL
@@ -69,9 +70,12 @@  void ppc_hash64_update_rmls(CPUPPCState *env);
 #define HPTE64_V_SECONDARY      0x0000000000000002ULL
 #define HPTE64_V_VALID          0x0000000000000001ULL
 
+#define HPTE64_R_3_00_COMMON    0xf1ffffffffffffffULL
 #define HPTE64_R_PP0            0x8000000000000000ULL
 #define HPTE64_R_TS             0x4000000000000000ULL
 #define HPTE64_R_KEY_HI         0x3000000000000000ULL
+#define HPTE64_R_SSIZE_SHIFT    58
+#define HPTE64_R_SSIZE_MASK     (3ULL << HPTE64_R_SSIZE_SHIFT)
 #define HPTE64_R_RPN_SHIFT      12
 #define HPTE64_R_RPN            0x0ffffffffffff000ULL
 #define HPTE64_R_FLAGS          0x00000000000003ffULL
@@ -91,6 +95,10 @@  void ppc_hash64_update_rmls(CPUPPCState *env);
 #define HPTE64_V_1TB_SEG        0x4000000000000000ULL
 #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
 
+typedef struct {
+    uint64_t pte0, pte1;
+} ppc_hash_pte64_t;
+
 void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
                          Error **errp);
 void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift,
@@ -99,37 +107,64 @@  void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift,
 uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_index);
 void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token);
 
-static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
-                                                 uint64_t token, int index)
+static inline void ppc_hash64_hpte_old_to_new(CPUPPCState *env,
+                                              target_ulong *pte0,
+                                              target_ulong *pte1)
 {
-    CPUPPCState *env = &cpu->env;
-    uint64_t addr;
+    switch (env->mmu_model) {
+    case POWERPC_MMU_3_00:
+        /*
+         * v3.00 of the ISA moved the B field to the second doubleword and
+         * shortened the abbreviated virtual address and abbreviated real page
+         * number fields
+         */
+        *pte1 = (*pte1 & HPTE64_R_3_00_COMMON) |
+                ((*pte0 >> HPTE64_V_SSIZE_SHIFT) << HPTE64_R_SSIZE_SHIFT);
+        *pte0 = *pte0 & HPTE64_V_3_00_COMMON;
+    default:
+        ;
+    }
+}
 
-    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);
+static inline void ppc_hash64_hpte_new_to_old(CPUPPCState *env,
+                                              target_ulong *pte0,
+                                              target_ulong *pte1)
+{
+    switch (env->mmu_model) {
+    case POWERPC_MMU_3_00:
+        /*
+         * v3.00 of the ISA moved the B field to the second doubleword and
+         * shortened the abbreviated virtual address and abbreviated real page
+         * number fields
+         */
+        *pte0 = (*pte0 & HPTE64_V_3_00_COMMON) | ((*pte1 & HPTE64_R_SSIZE_MASK)
+                << (HPTE64_V_SSIZE_SHIFT - HPTE64_R_SSIZE_SHIFT));
+        *pte1 = *pte1 & HPTE64_R_3_00_COMMON;
+    default:
+        ;
     }
 }
 
-static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU *cpu,
-                                                 uint64_t token, int index)
+static inline ppc_hash_pte64_t ppc_hash64_load_hpte(PowerPCCPU *cpu,
+                                                    uint64_t token,
+                                                    int index)
 {
     CPUPPCState *env = &cpu->env;
+    ppc_hash_pte64_t pte;
     uint64_t addr;
 
-    addr = token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2;
+    addr = token + (index * HASH_PTE_SIZE_64);
     if (env->external_htab) {
-        return  ldq_p((const void *)(uintptr_t)addr);
+        pte.pte0 = ldq_p((const void *)(uintptr_t)addr);
+        pte.pte1 = ldq_p((const void *)(uintptr_t)addr + HASH_PTE_SIZE_64/2);
     } else {
-        return ldq_phys(CPU(cpu)->as, addr);
+        pte.pte0 = ldq_phys(CPU(cpu)->as, addr);
+        pte.pte1 = ldq_phys(CPU(cpu)->as, addr + HASH_PTE_SIZE_64/2);
     }
-}
 
-typedef struct {
-    uint64_t pte0, pte1;
-} ppc_hash_pte64_t;
+    ppc_hash64_hpte_new_to_old(env, &pte.pte0, &pte.pte1);
+    return pte;
+}
 
 #endif /* CONFIG_USER_ONLY */