[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: |
David Gibson |
|
Subject: |
Re: [Qemu-devel] [RFC PATCH 11/17] target/ppc/POWER9: Update to new pte format for POWER9 accesses |
|
Date: |
Fri, 10 Feb 2017 13:24:42 +1100 |
|
User-agent: |
Mutt/1.7.1 (2016-10-04) |
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 <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.
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 */
> > > > > >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature