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

[Alexander Graf]

On 06.05.2011, at 19:40, Scott Wood wrote:

> 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.

Yup, changed that one :).

> 
>>> 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. :-)

I have a nice algorithm for MAV 1.0 now - when 2.0 comes along we can check 
again and see what fits.

> 
>>>> +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.

Ah, I see. That one's moot by now since we don't look up things in an array 
anymore.

> 
>>>> +    } 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.

Meh :).

> 
>>>> @@ -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?

Well, the PPC_BOOKE flag was also used for the 440 tlb modification opcodes, 
which are different for 2.06. So I just introduced a new one :). We could of 
course also introduce yet another flag for 440s and define specific 
instructions individually, but I don't really see it buying us too much for now.


Alex