[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 11/17] target/ppc/POWER9: Update to new pte
|
From: |
Suraj Jitindar Singh |
|
Subject: |
Re: [Qemu-devel] [RFC PATCH 11/17] target/ppc/POWER9: Update to new pte format for POWER9 accesses |
|
Date: |
Fri, 10 Feb 2017 12:05:38 +1100 |
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 <address@hidden
> > > > > >
> > > > > ---
> > > > > 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 */
> > > > >