Re: [Qemu-devel] [PATCH 5/6] PPC: Implement e500 (FSL) MMU

[Scott Wood]

On Fri, 6 May 2011 12:01:11 +0200
Alexander Graf <address@hidden> wrote:

> >> +    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?
> 
> I'm not sure - the spec isn't exactly obvious on this part. How do I find out 
> the mask from TLBnCFG?

I don't think the ISA specifies the hash at all.  The implementation manual
should specify it.  I think 7 bits is correct on all e500-family chips so
far, but it's really a function of the TLB geometry.  It may be desireable
in some situations for qemu to create a larger TLB than hardware actually
has, to reduce the TLB misses that the guest has to handle.

> > Better victim selection would check for empty ways, and perhaps keep
> > last_way on a per-set basis.
> 
> Does the hardware do this?

Hmm, I thought it did, but checking the docs it seems to be similar to
what you implemented.  I guess I was thinking of the I/D-cache victim
selection.

> >> +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).
> 
> Yeah, I've seen that mentioned as MAV 2.0 or so change. This is the exact 
> list of supported page sizes for an e500v2 though. Should we just support all 
> of the possible one and ignore the chip's deficiencies?

I'd say just support all of them -- it's undefined behavior if a MAV 1.0
guest sets that low bit, so this is as valid an implementation as any. :-)

> >> +static inline target_phys_addr_t e500_tlb_to_page_size(int size)
> > 
> > unsigned int
> 
> Why?

So you don't have to check size < 0.

> >> +    } 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.
> 
> Because that's what the spec says:
> 
> if category E.HV.LRAT supported & (MSRGS=1) & (MAS1V=1) then
>   rpn <- translate_logical_to_real(MAS7RPNU || MAS3RPNL, MAS8TLPID)
> else if MAS7 implemented then
>   rpn <- MAS7RPNU || MAS3RPNL
> else rpn <- 320 || MAS3RPNL

Sorry, I was thinking of MAS8[TGS], not MSR[GS].

> Yes, I need to think of a good way to generalize NV still. It isn't defined 
> that there's only a single NV available in the architecture either...

Architecturally, the NV algorithm is implementation-defined.

> >> @@ -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.
> 
> It's also in e500, right?

Yes.

> The flags there are a mask. Basically it means "this opcode is available
> on PPC_BOOKE and PPC2_BOOKE_FSL capable CPUs".

OK, it looked like it was being limited to only FSL.  I missed that
PPC_BOOKE and PPC2_BOOKE_FSL are the same "kind" of flags despite being in
different words.  Does PPC_BOOKE not refer to things that are common to all
booke?  Why do the individual implementations of booke need to be listed?

-Scott