[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/6] PPC: Implement e500 (FSL) MMU
|
From: |
Scott Wood |
|
Subject: |
Re: [Qemu-devel] [PATCH 5/6] PPC: Implement e500 (FSL) MMU |
|
Date: |
Tue, 3 May 2011 14:25:45 -0500 |
On Mon, 2 May 2011 17:03:21 +0200
Alexander Graf <address@hidden> wrote:
> Most of the code to support e500 style MMUs is already in place, but
> we're missing on some of the special TLB0-TLB1 handling code and slightly
> different TLB modification.
>
> This patch adds support for the FSL style MMU.
>
> Signed-off-by: Alexander Graf <address@hidden>
You beat me to (part of) it... :-)
How is this going to interact with the KVM MMU API, where it was suggested
that qemu use that representation as its native structure? Should we just
change the representation at that point, for both types of booke mmu (so 4xx
would be represented with MAS registers internally, even though a
different interface is exposed to the guest)?
> @@ -678,8 +887,9 @@ struct CPUPPCState {
> int nb_BATs;
> target_ulong DBAT[2][8];
> target_ulong IBAT[2][8];
> - /* PowerPC TLB registers (for 4xx and 60x software driven TLBs) */
> + /* PowerPC TLB registers (for 4xx, e500 and 60x software driven TLBs) */
> int nb_tlb; /* Total number of TLB
> */
> + int nb_tlbs[4]; /* Number of respective TLB entries (e500)
> */
Looks like "number of tlbs", not "number of tlb entries"... Was less
confusing when there was only one tlb array.
> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> index 5e4030b..261c1a9 100644
> --- a/target-ppc/helper.c
> +++ b/target-ppc/helper.c
> @@ -1153,48 +1153,144 @@ void store_40x_sler (CPUPPCState *env, uint32_t val)
> env->spr[SPR_405_SLER] = val;
> }
>
> +static inline int mmubooke_check_tlb (CPUState *env, ppcemb_tlb_t *tlb,
> + target_phys_addr_t *raddr, int *prot,
> + target_ulong address, int rw,
> + int access_type, int i)
> +{
> + int ret, _prot;
> +
> + if (ppcemb_tlb_check(env, tlb, raddr, address,
> + env->spr[SPR_BOOKE_PID],
> + !env->nb_pids, i) >= 0) {
> + goto found_tlb;
> + }
> +
> + if ((env->nb_pids > 1) && env->spr[SPR_BOOKE_PID1] &&
> + ppcemb_tlb_check(env, tlb, raddr, address,
> + env->spr[SPR_BOOKE_PID1], 0, i) >= 0) {
> + goto found_tlb;
> + }
> +
> + if ((env->nb_pids > 2) && env->spr[SPR_BOOKE_PID2] &&
> + ppcemb_tlb_check(env, tlb, raddr, address,
> + env->spr[SPR_BOOKE_PID2], 0, i) >= 0) {
> + goto found_tlb;
> + }
Maybe PID1/PID2 should be moved into ppcemb_tlb_check, and eliminate the
nb_pids checks by putting zero in PID1/PID2 on chips that don't have it?
I'm assuming this is performance sensitive for non-KVM.
> +static int mmue500_get_physical_address (CPUState *env, mmu_ctx_t *ctx,
> + target_ulong address, int rw,
> + int access_type)
> +{
> + ppcemb_tlb_t *tlb;
> + target_phys_addr_t raddr;
> + int i, ret;
> + uint32_t ea = (address >> MAS2_EPN_SHIFT) & 0x7f;
> +
> + ret = -1;
> + raddr = (target_phys_addr_t)-1ULL;
> +
> + /* check TLB1 - hits often because the kernel is here */
I'd optimize for userspace instead -- that's where the real work is more
likely to happen. Plus, compare what's likely to be 5-6 iterations before
finding the entry for a kernel large-page entry if we check TLB0 first,
versus 17+ iterations (65+ on e500mc) for finding a TLB0 entry if TLB1 is
checked first -- based on Linux's TLB1 usage patterns.
> + for (i = env->nb_tlbs[0]; i < env->nb_tlb; i++) {
> + tlb = &env->tlb[i].tlbe;
> + ret = mmubooke_check_tlb(env, tlb, &raddr, &ctx->prot, address, rw,
> + access_type, i);
> + if (!ret) {
> + goto found_tlb;
> }
> }
> - if (ret >= 0)
> +
> + /* check possible TLB0 entries */
> + for (i = 0; i < env->nb_ways; i++) {
> + tlb = &env->tlb[ea | (i << 7)].tlbe;
Do we have to hardcode 7 (and 0x7f) here?
And if possible I'd rearrange the tlb so that ways within a set are
contiguous. Better for cache utilization, and is what the KVM MMU API will
want.
> @@ -1348,6 +1446,44 @@ target_phys_addr_t cpu_get_phys_page_debug (CPUState
> *env, target_ulong addr)
> return ctx.raddr & TARGET_PAGE_MASK;
> }
>
> +static void e500_update_mas_tlb_miss(CPUState *env, target_ulong address,
> + int rw)
> +{
> + env->spr[SPR_BOOKE_MAS0] = env->spr[SPR_BOOKE_MAS4] & MAS4_TLBSELD_MASK;
> + env->spr[SPR_BOOKE_MAS1] = env->spr[SPR_BOOKE_MAS4] & MAS4_TSIZED_MASK;
> + env->spr[SPR_BOOKE_MAS2] = env->spr[SPR_BOOKE_MAS4] & MAS4_WIMGED_MASK;
> + env->spr[SPR_BOOKE_MAS6] = 0;
> +
> + /* AS */
> + if (((rw == 2) && msr_ir) || ((rw != 2) && msr_dr)) {
> + env->spr[SPR_BOOKE_MAS1] |= MAS1_TS;
> + env->spr[SPR_BOOKE_MAS6] |= MAS6_SAS;
> + }
> +
> + env->spr[SPR_BOOKE_MAS1] |= MAS1_VALID;
> + env->spr[SPR_BOOKE_MAS2] |= address & MAS2_EPN_MASK;
> +
> + switch (env->spr[SPR_BOOKE_MAS4] & MAS4_TIDSELD_PIDZ) {
> + case MAS4_TIDSELD_PID0:
> + env->spr[SPR_BOOKE_MAS1] |= env->spr[SPR_BOOKE_PID] <<
> MAS1_TID_SHIFT;
> + break;
> + case MAS4_TIDSELD_PID1:
> + env->spr[SPR_BOOKE_MAS1] |= env->spr[SPR_BOOKE_PID1] <<
> MAS1_TID_SHIFT;
> + break;
> + case MAS4_TIDSELD_PID2:
> + env->spr[SPR_BOOKE_MAS1] |= env->spr[SPR_BOOKE_PID2] <<
> MAS1_TID_SHIFT;
> + break;
> + }
> +
> + env->spr[SPR_BOOKE_MAS6] |= env->spr[SPR_BOOKE_PID] << 16;
> +
> + /* next victim logic */
> + env->spr[SPR_BOOKE_MAS0] |= env->last_way << MAS0_ESEL_SHIFT;
> + env->last_way++;
> + env->last_way &= 3;
> + env->spr[SPR_BOOKE_MAS0] |= env->last_way << MAS0_NV_SHIFT;
> +}
MAS3/7 should be zeroed according to the architecture.
Better victim selection would check for empty ways, and perhaps keep
last_way on a per-set basis.
> @@ -1820,12 +1956,8 @@ void ppc_tlb_invalidate_all (CPUPPCState *env)
> cpu_abort(env, "MPC8xx MMU model is not implemented\n");
> break;
> case POWERPC_MMU_BOOKE:
> - tlb_flush(env, 1);
> - break;
> case POWERPC_MMU_BOOKE_FSL:
> - /* XXX: TODO */
> - if (!kvm_enabled())
> - cpu_abort(env, "BookE MMU model is not implemented\n");
> + tlb_flush(env, 1);
> break;
> case POWERPC_MMU_32B:
> case POWERPC_MMU_601:
This flushes env->tlb_table, but don't we still need to clear out env->tlb?
> diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
> index d5db484..3c0b061 100644
> --- a/target-ppc/op_helper.c
> +++ b/target-ppc/op_helper.c
> @@ -4206,4 +4206,422 @@ target_ulong helper_440_tlbsx (target_ulong address)
> return ppcemb_tlb_search(env, address, env->spr[SPR_440_MMUCR] & 0xFF);
> }
>
> +/* PowerPC e500 TLB management */
> +
> +static inline int e500_tlb_num(CPUState *env, ppcemb_tlb_t *tlb)
> +{
> + ulong tlbel = (ulong)tlb;
> + ulong tlbl = (ulong)env->tlb;
> +
> + return (tlbel - tlbl) / sizeof(env->tlb[0]);
> +}
> +
> +static inline ppcemb_tlb_t *e500_cur_tlb(CPUState *env)
This is a bit big to be inlined - let the compiler decide such things.
> +{
> + uint32_t tlbncfg = 0;
> + int esel = (env->spr[SPR_BOOKE_MAS0] & MAS0_ESEL_MASK) >>
> MAS0_ESEL_SHIFT;
> + int ea = (env->spr[SPR_BOOKE_MAS2] & MAS2_EPN_MASK) >> MAS2_EPN_SHIFT;
> + int tlb;
> + int r;
> +
> + tlb = (env->spr[SPR_BOOKE_MAS0] & MAS0_TLBSEL_MASK) >> MAS0_TLBSEL_SHIFT;
> + tlbncfg = env->spr[SPR_BOOKE_TLB0CFG + tlb];
> +
> + if ((tlbncfg & TLBnCFG_HES) && (env->spr[SPR_BOOKE_MAS0] & MAS0_HES)) {
> + cpu_abort(env, "we don't support HES yet\n");
> + }
> +
> + if (tlb == 1) {
> + r = env->nb_tlbs[0] + (esel % env->nb_tlbs[1]);
Ouch, division by non-constant.
There's no need for it anyway; esel >= env_nb_tlbs[1] is undefined
behavior. Just crop the value arbitrarily (or error out, if possible) to
avoid dereferencing a bad array index.
> + } else {
> + esel &= env->nb_ways - 1;
> + esel <<= 7;
> + ea &= 0x7f;
> + r = (esel | ea) % env->nb_tlbs[0];
More unnecessary division -- use "& (env->nb_tlbs[0] - 1)", or variable-ize
the 7 and 0x7f so that we know that we won't produce an out-of-bounds value.
> +static const target_phys_addr_t e500_tlb_sizes[] = {
> + 0,
> + 4 * 1024,
> + 16 * 1024,
> + 64 * 1024,
> + 256 * 1024,
> + 1024 * 1024,
> + 4 * 1024 * 1024,
> + 16 * 1024 * 1024,
> + 64 * 1024 * 1024,
> + 256 * 1024 * 1024,
> + 1024 * 1024 * 1024,
> + (target_phys_addr_t)(4ULL * 1024ULL * 1024ULL * 1024ULL),
> +};
FWIW, power-of-2 page sizes are architected, and may appear in future
e500-family chips. The TSIZE field is extended by one bit on the right
(previously reserved and should-be-zero).
> +static inline target_phys_addr_t e500_tlb_to_page_size(int size)
unsigned int
> +{
> + target_phys_addr_t r;
> +
> + if (size > 11) {
if (size >= ARRAY_SIZE(e500_tlb_sizes)) {
> + /* should not happen */
> + r = 1024 * 1024 * 1024;
Error message?
+ switch (env->spr[SPR_BOOKE_MAS0] & MAS0_WQ_MASK) {
+ case MAS0_WQ_ALWAYS:
+ /* good to go, write that entry */
+ break;
+ case MAS0_WQ_COND:
+ /* XXX check if reserved */
+ if (0) {
+ return;
+ }
+ break;
+ case MAS0_WQ_CLR_RSRV:
+ /* XXX clear entry */
+ return;
+ default:
+ /* no idea what to do */
+ return;
+ }
If the plan is to support such things, might want a more generic name than
"e500" -- we're really talking about isa 2.06 book3e. The first chip that
implements this stuff will probably not be from Freescale, much less
called "e500". :-)
> + if (msr_gs) {
> + cpu_abort(env, "missing HV implementation\n");
What's the policy on aborting versus error-message versus no-op if the
guest invokes undefined/unimplemented behavior?
> + } else {
> + rpn = ((uint64_t)env->spr[SPR_BOOKE_MAS7] << 32) |
> + (env->spr[SPR_BOOKE_MAS3] & 0xfffff000);
> + }
Not sure why this is in an else-branch versus msr_gs.
> + tlb->RPN = rpn;
> +
> + tlb->PID = (env->spr[SPR_BOOKE_MAS1] & MAS1_TID_MASK) >> MAS1_TID_SHIFT;
> + tlb->size = e500_tlb_to_page_size((env->spr[SPR_BOOKE_MAS1]
> + & MAS1_TSIZE_MASK) >> MAS1_TSIZE_SHIFT);
e500 manuals document that tsize is ignored in tlb0.
> + tlb->EPN = (uint32_t)(env->spr[SPR_BOOKE_MAS2] & MAS2_EPN_MASK);
> + tlb->attr = env->spr[SPR_BOOKE_MAS2] & (MAS2_ACM | MAS2_VLE | MAS2_W |
> + MAS2_I | MAS2_M | MAS2_G |
> MAS2_E)
> + << 1;
> + tlb->attr |= env->spr[SPR_BOOKE_MAS1] & MAS1_IPROT;
> + tlb->attr |= (env->spr[SPR_BOOKE_MAS3] &
> + ((MAS3_U0 | MAS3_U1 | MAS3_U2 | MAS3_U3)) << 8);
Might be nice to #define the encoding of these bits into attr/etc, versus
magic shifts (though it's moot if we're going to switch to a MAS-based
representation).
> + if (tlb->size == TARGET_PAGE_SIZE) {
> + tlb_flush_page(env, tlb->EPN);
> + } else {
> + tlb_flush(env, 1);
> + }
Need to check whether the previous entry was a large page needing a full
flush.
> +void helper_e500_tlbre(void)
> +{
> + ppcemb_tlb_t *tlb = NULL;
> +
> + tlb = e500_cur_tlb(env);
> + e500_tlb_to_mas(env, tlb, e500_tlb_num(env, tlb));
> +}
> +
> +/* Generic TLB check function for embedded PowerPC implementations */
> +static inline int ppcemb_tlb_check(CPUState *env, ppcemb_tlb_t *tlb,
> + target_phys_addr_t *raddrp,
> + target_ulong address, uint32_t pid, int
> ext,
> + int i)
Why is this both here and in helper.c?
> +void helper_e500_tlbsx(target_ulong address_hi, target_ulong address_lo)
> +{
> + ppcemb_tlb_t *tlb = NULL;
> + int i;
> + target_phys_addr_t raddr;
> + uint32_t spid, sas;
> + uint32_t ea = (address_lo >> MAS2_EPN_SHIFT) & 0x7f;
> +
> + spid = (env->spr[SPR_BOOKE_MAS6] & MAS6_SPID_MASK) >> MAS6_SPID_SHIFT;
> + sas = env->spr[SPR_BOOKE_MAS6] & MAS6_SAS;
> +
> + /* check possible TLB0 entries */
> + for (i = 0; i < env->nb_ways; i++) {
> + tlb = &env->tlb[ea | (i << 7)].tlbe;
> +
> + if (ppcemb_tlb_check(env, tlb, &raddr, address_lo, spid, 0, i) < 0) {
> + continue;
> + }
> +
> + if (sas != (tlb->attr & MAS6_SAS)) {
> + continue;
> + }
> +
> + e500_tlb_to_mas(env, tlb, ea | (i << 7));
> + return;
> + }
> +
> + /* check TLB1 */
> + for (i = env->nb_tlbs[0]; i < (env->nb_tlbs[0] + env->nb_tlbs[1]); i++) {
> + tlb = &env->tlb[i].tlbe;
> +
> + if (ppcemb_tlb_check(env, tlb, &raddr, address_lo, spid, 0, i) < 0) {
> + continue;
> + }
> +
> + if (sas != (tlb->attr & MAS6_SAS)) {
> + continue;
> + }
> +
> + e500_tlb_to_mas(env, tlb, i);
> + return;
> + }
This has a lot of overlap with mmue500_get_physical_address()...
> +
> + /* no entry found, fill with defaults */
> + env->spr[SPR_BOOKE_MAS0] = env->spr[SPR_BOOKE_MAS4] & MAS4_TLBSELD_MASK;
> + env->spr[SPR_BOOKE_MAS1] = env->spr[SPR_BOOKE_MAS4] & MAS4_TSIZED_MASK;
> + env->spr[SPR_BOOKE_MAS2] = env->spr[SPR_BOOKE_MAS4] & MAS4_WIMGED_MASK;
> +
> + if (env->spr[SPR_BOOKE_MAS6] & MAS6_SAS) {
> + env->spr[SPR_BOOKE_MAS1] |= MAS1_TS;
> + }
> +
> + env->spr[SPR_BOOKE_MAS1] |= (env->spr[SPR_BOOKE_MAS6] >> 16)
> + << MAS1_TID_SHIFT;
> +
> + /* next victim logic */
> + env->spr[SPR_BOOKE_MAS0] |= env->last_way << MAS0_ESEL_SHIFT;
> + env->last_way++;
> + env->last_way &= 3;
> + env->spr[SPR_BOOKE_MAS0] |= env->last_way << MAS0_NV_SHIFT;
...and this with e500_update_mas_tlb_miss().
> @@ -8433,7 +8505,7 @@ GEN_HANDLER2(icbt_40x, "icbt", 0x1F, 0x06, 0x08,
> 0x03E00001, PPC_40x_ICBT),
> GEN_HANDLER(iccci, 0x1F, 0x06, 0x1E, 0x00000001, PPC_4xx_COMMON),
> GEN_HANDLER(icread, 0x1F, 0x06, 0x1F, 0x03E00001, PPC_4xx_COMMON),
> GEN_HANDLER2(rfci_40x, "rfci", 0x13, 0x13, 0x01, 0x03FF8001, PPC_40x_EXCP),
> -GEN_HANDLER(rfci, 0x13, 0x13, 0x01, 0x03FF8001, PPC_BOOKE),
> +GEN_HANDLER_E(rfci, 0x13, 0x13, 0x01, 0x03FF8001, PPC_BOOKE, PPC2_BOOKE_FSL),
What is PPC2_BOOKE_FSL supposed to indicate?
rfci is basic booke. It's in the 440.
> +GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
> + PPC_BOOKE, PPC2_BOOKE_FSL),
"icbt_440" is FSL-specific? :-)
> +static void spr_write_booke_pid (void *opaque, int sprn, int gprn)
> +{
> + gen_store_spr(sprn, cpu_gpr[gprn]);
> + /* switching context, so need to flush tlb */
> + gen_helper_tlbia();
> +}
Well, we want to flush the non-guest-visible TLB that doesn't understand
PIDs -- but I'd expect "tlbia" to flush the architected TLB. 8xx, at
least, has both tlbia and an architected TLB.
Plus, we need ppc_tlb_invalidate_all() to clear the architected TLB, unless
we call something different on reset.
> @@ -1578,45 +1614,38 @@ static void gen_spr_BookE_FSL (CPUPPCState *env,
> uint32_t mas_mask)
> SPR_NOACCESS, SPR_NOACCESS,
> &spr_read_generic, SPR_NOACCESS,
> 0x00000000); /* TOFIX */
> - /* XXX : not implemented */
> - spr_register(env, SPR_MMUCSR0, "MMUCSR0",
> - SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> - 0x00000000); /* TOFIX */
> switch (env->nb_ways) {
> case 4:
> - /* XXX : not implemented */
> spr_register(env, SPR_BOOKE_TLB3CFG, "TLB3CFG",
> SPR_NOACCESS, SPR_NOACCESS,
> &spr_read_generic, SPR_NOACCESS,
> - 0x00000000); /* TOFIX */
> + tlbncfg[3]);
> /* Fallthru */
> case 3:
> - /* XXX : not implemented */
> spr_register(env, SPR_BOOKE_TLB2CFG, "TLB2CFG",
> SPR_NOACCESS, SPR_NOACCESS,
> &spr_read_generic, SPR_NOACCESS,
> - 0x00000000); /* TOFIX */
> + tlbncfg[2]);
> /* Fallthru */
In some places you use nb_ways as the associativity of TLB0. But here it
seems to be the number of TLB arrays?
> @@ -4334,8 +4373,23 @@ static void init_proc_e500 (CPUPPCState *env)
> /* Memory management */
> #if !defined(CONFIG_USER_ONLY)
> env->nb_pids = 3;
> + env->nb_ways = 2;
> + env->id_tlbs = 0;
> + if ((env->spr[SPR_PVR] & 0x00010000)) {
> + /* e500v2 */
> + env->nb_tlbs[0] = 512;
> + tlbncfg[0] = gen_tlbncfg(4, 1, 1, 0, env->nb_tlbs[0]);
> + tlbncfg[1] = gen_tlbncfg(16, 1, 12, TLBnCFG_AVAIL | TLBnCFG_IPROT,
> 16);
> + } else {
> + /* e500v1 */
> + env->nb_tlbs[0] = 256;
> + tlbncfg[0] = gen_tlbncfg(2, 1, 1, 0, env->nb_tlbs[0]);
> + tlbncfg[1] = gen_tlbncfg(16, 1, 9, TLBnCFG_AVAIL | TLBnCFG_IPROT,
> 16);
> + }
It would be nice to be more precise with PVR testing, and complain if an
unrecognized PVR is used (e.g. e500mc).
-Scott
- [Qemu-devel] [PATCH 0/6] PPC: Add FSL (e500) MMU emulation v3, Alexander Graf, 2011/05/02
- [Qemu-devel] [PATCH 2/6] PPC: Make MPC8544DS emulation work w/o KVM, Alexander Graf, 2011/05/02
- [Qemu-devel] [PATCH 1/6] PPC: Make MPC8544DS obey -cpu switch, Alexander Graf, 2011/05/02
- [Qemu-devel] [PATCH 3/6] PPC: Add GS MSR definition, Alexander Graf, 2011/05/02
- [Qemu-devel] [PATCH 6/6] PPC: Qdev'ify e500 pci, Alexander Graf, 2011/05/02
- [Qemu-devel] [PATCH 5/6] PPC: Implement e500 (FSL) MMU, Alexander Graf, 2011/05/02
- Re: [Qemu-devel] [PATCH 5/6] PPC: Implement e500 (FSL) MMU,
Scott Wood <=
[Qemu-devel] [PATCH 4/6] PPC: Add another 64 bits to instruction feature mask, Alexander Graf, 2011/05/02
Re: [Qemu-devel] [PATCH 0/6] PPC: Add FSL (e500) MMU emulation v3, Blue Swirl, 2011/05/04