Re: [Qemu-ppc] [PATCH 7/8] PPC: booke206: Check for min/max TLB entry size

[Alexander Graf]

On 23.01.2012, at 19:49, Scott Wood <address@hidden> wrote:

> On 01/23/2012 12:41 PM, Alexander Graf wrote:
>>>> For tlb0 on e500 and derivatives, tsize is explicitly documented as
>>>> ignored.  Software may rely on this.
>>> Yup, that's why there's the check for TLBnCG_AVAIL, which indicates that
>>> a TLB has dynamic page size capabilities, which TLB0 does not have.
>> Silly me, thinking "avail" meant "this TLB is available" instead of
>> looking up the actual meaning. :-P
> 
> But where do we fill in the size if TLBnCFG_AVAIL is not set?  If this
> is TLB0 on e500, we can't trust that the target code provided a valid
> size -- we need to force to 4K.

TLB0 has min=max=4k :)

> 
>>> Where do we check whether the TLB exists at all?
>> 
>> We don't. Eventually TLB access goes through:
>> 
>> static inline ppcmas_tlb_t *booke206_get_tlbm(CPUState *env, const int
>> tlbn,
>>                                              target_ulong ea, int way)
>> {
>>    int r;
>>    uint32_t ways = booke206_tlb_ways(env, tlbn);
>>    int ways_bits = ffs(ways) - 1;
>>    int tlb_bits = ffs(booke206_tlb_size(env, tlbn)) - 1;
>>    int i;
>> 
>>    way &= ways - 1;
>>    ea >>= MAS2_EPN_SHIFT;
>>    ea &= (1 << (tlb_bits - ways_bits)) - 1;
>>    r = (ea << ways_bits) | way;
>> 
>>    /* bump up to tlbn index */
>>    for (i = 0; i < tlbn; i++) {
>>        r += booke206_tlb_size(env, i);
>>    }
>> 
>>    return &env->tlb.tlbm[r];
>> }
>> 
>> Since unavailable TLBs have ways set to 0 and tlb_size is 0, we always
>> end up with the last TLB entry that's available.
> 
> I think you end up with the first entry beyond the end of the array,
> actually.

Yikes. Yeah :(

> 
>> So if you do a tlbwe on tlbn=5 on TLB2, you write to the last entry of
>> TLB1. Which actually is fine according to the spec:
>> 
>> If an invalid value is specified for MAS0TLBSEL
>> MAS0ESEL or MAS2EPN, either no TLB entry is written
>> by the tlbwe, or the tlbwe is performed as if some
>> implementation-dependent, valid value were substi-
>> tuted for the invalid value, or an Illegal Instruction
>> exception occurs.
>> 
>> We substitute it with a valid value :)
> 
> Even if I'm reading it wrong and you do somehow end up with the last
> element of the array, how do you know it's valid to write this entry
> there?  You haven't been checking that array's page size restrictions,
> or way/set geometry.

True. Maybe we should just always reserve a surplus TLB entry and have the 
current code work, basically making it be a nop?

Or we could add checks everywhere...

Alex

> 
> -Scott
>