On 06.05.2011, at 19:40, Scott Wood wrote:
> On Fri, 6 May 2011 12:01:11 +0200
> Alexander Graf <ag...@suse.de> 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
