Patchwork [7/8] PPC: booke206: Check for min/max TLB entry size

login
register
mail settings
Submitter Alexander Graf
Date Jan. 21, 2012, 4:15 a.m.
Message ID <1327119330-29304-8-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/137158/
State New
Headers show

Comments

Alexander Graf - Jan. 21, 2012, 4:15 a.m.
When setting a TLB entry, we need to check if the TLB we're putting it in
actually supports the given size. According to the 2.06 PowerPC ISA, a
value that's out of range can either be redefined to something implementation
dependent or we can raise an illegal opcode exception. We do the latter.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - fix min/max check
  - use mav 2.0 prepared code
  - raise exception on invalid page size
---
 target-ppc/op_helper.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
Scott Wood - Jan. 23, 2012, 5:32 p.m.
On 01/20/2012 10:15 PM, Alexander Graf wrote:
> @@ -4273,6 +4274,16 @@ void helper_booke206_tlbwe(void)
>          tlb->mas1 &= ~MAS1_IPROT;
>      }
>  
> +    /* check that we support the targeted size */
> +    size_tlb = (tlb->mas1 & MAS1_TSIZE_MASK) >> MAS1_TSIZE_SHIFT;
> +    size_ps = booke206_tlbnps(env, tlbn);
> +    if ((tlb->mas1 & MAS1_VALID) && (tlbncfg & TLBnCFG_AVAIL) &&
> +        !(size_ps & (1 << size_tlb))) {
> +        helper_raise_exception_err(POWERPC_EXCP_PROGRAM,
> +                                   POWERPC_EXCP_INVAL |
> +                                   POWERPC_EXCP_INVAL_INVAL);
> +    }
> +
>      if (booke206_tlb_to_page_size(env, tlb) == TARGET_PAGE_SIZE) {
>          tlb_flush_page(env, tlb->mas2 & MAS2_EPN_MASK);
>      } else {

For tlb0 on e500 and derivatives, tsize is explicitly documented as
ignored.  Software may rely on this.

-Scott
Alexander Graf - Jan. 23, 2012, 5:33 p.m.
On 01/23/2012 06:32 PM, Scott Wood wrote:
> On 01/20/2012 10:15 PM, Alexander Graf wrote:
>> @@ -4273,6 +4274,16 @@ void helper_booke206_tlbwe(void)
>>           tlb->mas1&= ~MAS1_IPROT;
>>       }
>>
>> +    /* check that we support the targeted size */
>> +    size_tlb = (tlb->mas1&  MAS1_TSIZE_MASK)>>  MAS1_TSIZE_SHIFT;
>> +    size_ps = booke206_tlbnps(env, tlbn);
>> +    if ((tlb->mas1&  MAS1_VALID)&&  (tlbncfg&  TLBnCFG_AVAIL)&&
>> +        !(size_ps&  (1<<  size_tlb))) {
>> +        helper_raise_exception_err(POWERPC_EXCP_PROGRAM,
>> +                                   POWERPC_EXCP_INVAL |
>> +                                   POWERPC_EXCP_INVAL_INVAL);
>> +    }
>> +
>>       if (booke206_tlb_to_page_size(env, tlb) == TARGET_PAGE_SIZE) {
>>           tlb_flush_page(env, tlb->mas2&  MAS2_EPN_MASK);
>>       } else {
> 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.


Alex
Scott Wood - Jan. 23, 2012, 6:19 p.m.
On 01/23/2012 11:33 AM, Alexander Graf wrote:
> On 01/23/2012 06:32 PM, Scott Wood wrote:
>> On 01/20/2012 10:15 PM, Alexander Graf wrote:
>>> @@ -4273,6 +4274,16 @@ void helper_booke206_tlbwe(void)
>>>           tlb->mas1&= ~MAS1_IPROT;
>>>       }
>>>
>>> +    /* check that we support the targeted size */
>>> +    size_tlb = (tlb->mas1&  MAS1_TSIZE_MASK)>>  MAS1_TSIZE_SHIFT;
>>> +    size_ps = booke206_tlbnps(env, tlbn);
>>> +    if ((tlb->mas1&  MAS1_VALID)&&  (tlbncfg&  TLBnCFG_AVAIL)&&
>>> +        !(size_ps&  (1<<  size_tlb))) {
>>> +        helper_raise_exception_err(POWERPC_EXCP_PROGRAM,
>>> +                                   POWERPC_EXCP_INVAL |
>>> +                                   POWERPC_EXCP_INVAL_INVAL);
>>> +    }
>>> +
>>>       if (booke206_tlb_to_page_size(env, tlb) == TARGET_PAGE_SIZE) {
>>>           tlb_flush_page(env, tlb->mas2&  MAS2_EPN_MASK);
>>>       } else {
>> 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

Where do we check whether the TLB exists at all?

-Scott
Alexander Graf - Jan. 23, 2012, 6:41 p.m.
On 01/23/2012 07:19 PM, Scott Wood wrote:
> On 01/23/2012 11:33 AM, Alexander Graf wrote:
>> On 01/23/2012 06:32 PM, Scott Wood wrote:
>>> On 01/20/2012 10:15 PM, Alexander Graf wrote:
>>>> @@ -4273,6 +4274,16 @@ void helper_booke206_tlbwe(void)
>>>>            tlb->mas1&= ~MAS1_IPROT;
>>>>        }
>>>>
>>>> +    /* check that we support the targeted size */
>>>> +    size_tlb = (tlb->mas1&   MAS1_TSIZE_MASK)>>   MAS1_TSIZE_SHIFT;
>>>> +    size_ps = booke206_tlbnps(env, tlbn);
>>>> +    if ((tlb->mas1&   MAS1_VALID)&&   (tlbncfg&   TLBnCFG_AVAIL)&&
>>>> +        !(size_ps&   (1<<   size_tlb))) {
>>>> +        helper_raise_exception_err(POWERPC_EXCP_PROGRAM,
>>>> +                                   POWERPC_EXCP_INVAL |
>>>> +                                   POWERPC_EXCP_INVAL_INVAL);
>>>> +    }
>>>> +
>>>>        if (booke206_tlb_to_page_size(env, tlb) == TARGET_PAGE_SIZE) {
>>>>            tlb_flush_page(env, tlb->mas2&   MAS2_EPN_MASK);
>>>>        } else {
>>> 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
>
> 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.

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


Alex
Scott Wood - Jan. 23, 2012, 6:49 p.m.
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.

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

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

-Scott
Alexander Graf - Jan. 23, 2012, 8:03 p.m.
On 23.01.2012, at 19:49, Scott Wood <scottwood@freescale.com> 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
>
Scott Wood - Jan. 23, 2012, 8:10 p.m.
On 01/23/2012 02:03 PM, Alexander Graf wrote:
> 
> 
> On 23.01.2012, at 19:49, Scott Wood <scottwood@freescale.com> 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 :)

If TLB0 has TLBnCFG[AVAIL] set, then with this patch you'll be raising
an exception rather than setting the size to the minimum.

If TLB0 does not have TLBnCFG[AVAIL] set, you'll be letting the user set
whatever size they want.

In either case, you seem to be letting the user write whatever the want
to the TLB array, and only afterward check whether to send an exception.

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

I'd have booke206_get_tlbm() check and return NULL, with callers
checking for that.  Optimization can come later, if/when it's shown to
be a bottleneck.

-Scott
Alexander Graf - Jan. 23, 2012, 9:29 p.m.
On 23.01.2012, at 21:10, Scott Wood <scottwood@freescale.com> wrote:

> On 01/23/2012 02:03 PM, Alexander Graf wrote:
>> 
>> 
>> On 23.01.2012, at 19:49, Scott Wood <scottwood@freescale.com> 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 :)
> 
> If TLB0 has TLBnCFG[AVAIL] set, then with this patch you'll be raising
> an exception rather than setting the size to the minimum.
> 
> If TLB0 does not have TLBnCFG[AVAIL] set, you'll be letting the user set
> whatever size they want.
> 
> In either case, you seem to be letting the user write whatever the want
> to the TLB array, and only afterward check whether to send an exception.

Yes, for !AVAIL we simply override the page size on qemu tlb miss iirc.

Is that wrong? Does tlbwe;tlbre result in different tsize values?

> 
>> 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...
> 
> I'd have booke206_get_tlbm() check and return NULL, with callers
> checking for that.  Optimization can come later, if/when it's shown to
> be a bottleneck.

It's more about not missing any cases :). But yeah, it's probably best to just change the semantics.

Alex
Scott Wood - Jan. 23, 2012, 9:41 p.m.
On 01/23/2012 03:29 PM, Alexander Graf wrote:
> 
> 
> On 23.01.2012, at 21:10, Scott Wood <scottwood@freescale.com> wrote:
> 
>> If TLB0 has TLBnCFG[AVAIL] set, then with this patch you'll be raising
>> an exception rather than setting the size to the minimum.
>>
>> If TLB0 does not have TLBnCFG[AVAIL] set, you'll be letting the user set
>> whatever size they want.
>>
>> In either case, you seem to be letting the user write whatever the want
>> to the TLB array, and only afterward check whether to send an exception.
> 
> Yes, for !AVAIL we simply override the page size on qemu tlb miss iirc.

Ah.  That seems like a hotter path than tlbwe, and you could still
insert an invalid entry into tlb1 (you'd get an exception, but the entry
would be there).

> Is that wrong? Does tlbwe;tlbre result in different tsize values?

e500mc manual (table 6-6, "MMU Assist Register Field Updates") says
tlbre returns a tsize of 1 for tlb0 -- it doesn't store tsize.  The KVM
MMU API also requires that tsize be stored as a valid value, to simplify
the code that operates on the TLB.  The TLB dump code depends on this
(could be fixed of course, but simpler to fix it once in tlbwe).

>>> 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...
>>
>> I'd have booke206_get_tlbm() check and return NULL, with callers
>> checking for that.  Optimization can come later, if/when it's shown to
>> be a bottleneck.
> 
> It's more about not missing any cases :). But yeah, it's probably best to just change the semantics.

At least a NULL deference will be more noticeable than an array overrun...

-Scott

Patch

diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 6339c95..2c8a96f 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -4228,6 +4228,7 @@  void helper_booke206_tlbwe(void)
 {
     uint32_t tlbncfg, tlbn;
     ppcmas_tlb_t *tlb;
+    uint32_t size_tlb, size_ps;
 
     switch (env->spr[SPR_BOOKE_MAS0] & MAS0_WQ_MASK) {
     case MAS0_WQ_ALWAYS:
@@ -4273,6 +4274,16 @@  void helper_booke206_tlbwe(void)
         tlb->mas1 &= ~MAS1_IPROT;
     }
 
+    /* check that we support the targeted size */
+    size_tlb = (tlb->mas1 & MAS1_TSIZE_MASK) >> MAS1_TSIZE_SHIFT;
+    size_ps = booke206_tlbnps(env, tlbn);
+    if ((tlb->mas1 & MAS1_VALID) && (tlbncfg & TLBnCFG_AVAIL) &&
+        !(size_ps & (1 << size_tlb))) {
+        helper_raise_exception_err(POWERPC_EXCP_PROGRAM,
+                                   POWERPC_EXCP_INVAL |
+                                   POWERPC_EXCP_INVAL_INVAL);
+    }
+
     if (booke206_tlb_to_page_size(env, tlb) == TARGET_PAGE_SIZE) {
         tlb_flush_page(env, tlb->mas2 & MAS2_EPN_MASK);
     } else {