Patchwork target-ppc: Init dcache and icache size for e500 user mode

login
register
mail settings
Submitter Meador Inge
Date April 10, 2012, 8:04 p.m.
Message ID <1334088263-10530-1-git-send-email-meadori@codesourcery.com>
Download mbox | patch
Permalink /patch/151689/
State New
Headers show

Comments

Meador Inge - April 10, 2012, 8:04 p.m.
commit f7aa558396dd0f6b7a2b22c05cb503c655854102 pulled the dcache and icache
line size initialization inside of a '#if !defined(CONFIG_USER_ONLY)' block.
This is not correct because instructions like 'dcbz' need the dcache size
initialized even for user mode.

Signed-off-by: Meador Inge <meadori@codesourcery.com>
---
 target-ppc/translate_init.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)
Andreas Färber - April 12, 2012, 4:59 p.m.
Am 10.04.2012 22:04, schrieb Meador Inge:
> commit f7aa558396dd0f6b7a2b22c05cb503c655854102 pulled the dcache and icache
> line size initialization inside of a '#if !defined(CONFIG_USER_ONLY)' block.
> This is not correct because instructions like 'dcbz' need the dcache size
> initialized even for user mode.
> 
> Signed-off-by: Meador Inge <meadori@codesourcery.com>

Looks okay and compiles,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Scott, are you planning to review this e500 patch? Or should I go ahead
and apply?

Andreas

> ---
>  target-ppc/translate_init.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index b1f8785..ef8735a 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -4466,25 +4466,32 @@ static void init_proc_e500 (CPUPPCState *env, int version)
>      env->nb_pids = 3;
>      env->nb_ways = 2;
>      env->id_tlbs = 0;
> +#endif
>      switch (version) {
>      case fsl_e500v1:
>          /* e500v1 */
> +#if !defined(CONFIG_USER_ONLY)
>          tlbncfg[0] = gen_tlbncfg(2, 1, 1, 0, 256);
>          tlbncfg[1] = gen_tlbncfg(16, 1, 9, TLBnCFG_AVAIL | TLBnCFG_IPROT, 16);
> +#endif
>          env->dcache_line_size = 32;
>          env->icache_line_size = 32;
>          break;
>      case fsl_e500v2:
>          /* e500v2 */
> +#if !defined(CONFIG_USER_ONLY)
>          tlbncfg[0] = gen_tlbncfg(4, 1, 1, 0, 512);
>          tlbncfg[1] = gen_tlbncfg(16, 1, 12, TLBnCFG_AVAIL | TLBnCFG_IPROT, 16);
> +#endif
>          env->dcache_line_size = 32;
>          env->icache_line_size = 32;
>          break;
>      case fsl_e500mc:
>          /* e500mc */
> +#if !defined(CONFIG_USER_ONLY)
>          tlbncfg[0] = gen_tlbncfg(4, 1, 1, 0, 512);
>          tlbncfg[1] = gen_tlbncfg(64, 1, 12, TLBnCFG_AVAIL | TLBnCFG_IPROT, 64);
> +#endif
>          env->dcache_line_size = 64;
>          env->icache_line_size = 64;
>          l1cfg0 |= 0x1000000; /* 64 byte cache block size */
> @@ -4492,7 +4499,6 @@ static void init_proc_e500 (CPUPPCState *env, int version)
>      default:
>          cpu_abort(env, "Unknown CPU: " TARGET_FMT_lx "\n", env->spr[SPR_PVR]);
>      }
> -#endif
>      gen_spr_BookE206(env, 0x000000DF, tlbncfg);
>      /* XXX : not implemented */
>      spr_register(env, SPR_HID0, "HID0",
Scott Wood - April 12, 2012, 5:24 p.m.
On 04/12/2012 11:59 AM, Andreas Färber wrote:
> Am 10.04.2012 22:04, schrieb Meador Inge:
>> commit f7aa558396dd0f6b7a2b22c05cb503c655854102 pulled the dcache and icache
>> line size initialization inside of a '#if !defined(CONFIG_USER_ONLY)' block.
>> This is not correct because instructions like 'dcbz' need the dcache size
>> initialized even for user mode.
>>
>> Signed-off-by: Meador Inge <meadori@codesourcery.com>
> 
> Looks okay and compiles,
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> 
> Scott, are you planning to review this e500 patch? Or should I go ahead
> and apply?

I'm OK with it, though it may make more sense for USER_ONLY to just pick
an arbitrary cache line size (probably 32) than to try to imitate a
specific core.

-Scott
Andreas Färber - April 13, 2012, 11:40 a.m.
Am 12.04.2012 19:24, schrieb Scott Wood:
> On 04/12/2012 11:59 AM, Andreas Färber wrote:
>> Am 10.04.2012 22:04, schrieb Meador Inge:
>>> commit f7aa558396dd0f6b7a2b22c05cb503c655854102 pulled the dcache and icache
>>> line size initialization inside of a '#if !defined(CONFIG_USER_ONLY)' block.
>>> This is not correct because instructions like 'dcbz' need the dcache size
>>> initialized even for user mode.
>>>
>>> Signed-off-by: Meador Inge <meadori@codesourcery.com>
>>
>> Looks okay and compiles,
>>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>
>> Scott, are you planning to review this e500 patch? Or should I go ahead
>> and apply?
> 
> I'm OK with it, though it may make more sense for USER_ONLY to just pick
> an arbitrary cache line size (probably 32) than to try to imitate a
> specific core.

Meador, what was your specific problem: The value being zero or not
matching real hardware?

Scott's suggestion would avoid some #ifdef'ery so I'd prefer that if
possible. I'm planning for a PULL later today, so let me know.

Andreas
Meador Inge - April 13, 2012, noon
On 04/13/2012 06:40 AM, Andreas Färber wrote:

> Am 12.04.2012 19:24, schrieb Scott Wood:
>> On 04/12/2012 11:59 AM, Andreas Färber wrote:
>>> Am 10.04.2012 22:04, schrieb Meador Inge:
>>>> commit f7aa558396dd0f6b7a2b22c05cb503c655854102 pulled the dcache and icache
>>>> line size initialization inside of a '#if !defined(CONFIG_USER_ONLY)' block.
>>>> This is not correct because instructions like 'dcbz' need the dcache size
>>>> initialized even for user mode.
>>>>
>>>> Signed-off-by: Meador Inge <meadori@codesourcery.com>
>>>
>>> Looks okay and compiles,
>>>
>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>
>>> Scott, are you planning to review this e500 patch? Or should I go ahead
>>> and apply?
>>
>> I'm OK with it, though it may make more sense for USER_ONLY to just pick
>> an arbitrary cache line size (probably 32) than to try to imitate a
>> specific core.
> 
> Meador, what was your specific problem: The value being zero or not
> matching real hardware?

My specific problem was the value being zero.  Therefore nothing actually got
cleared because the 'dcbz' implementation uses 'dcache_line_size' as an upper
bound on a loop.

On a side note, the way I actually ran into this was by running a application
linked against an optimized version of glibc for e500 that uses 'dcbz' in
'memset'.  ld.so wasn't correctly clearing .bss as a result.  That was fun to
debug :-)

> Scott's suggestion would avoid some #ifdef'ery so I'd prefer that if
> possible. I'm planning for a PULL later today, so let me know.

Sounds good to me.  I think the #ifdef stuff is gross, but I wasn't sure of way
around it.

Patch

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index b1f8785..ef8735a 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -4466,25 +4466,32 @@  static void init_proc_e500 (CPUPPCState *env, int version)
     env->nb_pids = 3;
     env->nb_ways = 2;
     env->id_tlbs = 0;
+#endif
     switch (version) {
     case fsl_e500v1:
         /* e500v1 */
+#if !defined(CONFIG_USER_ONLY)
         tlbncfg[0] = gen_tlbncfg(2, 1, 1, 0, 256);
         tlbncfg[1] = gen_tlbncfg(16, 1, 9, TLBnCFG_AVAIL | TLBnCFG_IPROT, 16);
+#endif
         env->dcache_line_size = 32;
         env->icache_line_size = 32;
         break;
     case fsl_e500v2:
         /* e500v2 */
+#if !defined(CONFIG_USER_ONLY)
         tlbncfg[0] = gen_tlbncfg(4, 1, 1, 0, 512);
         tlbncfg[1] = gen_tlbncfg(16, 1, 12, TLBnCFG_AVAIL | TLBnCFG_IPROT, 16);
+#endif
         env->dcache_line_size = 32;
         env->icache_line_size = 32;
         break;
     case fsl_e500mc:
         /* e500mc */
+#if !defined(CONFIG_USER_ONLY)
         tlbncfg[0] = gen_tlbncfg(4, 1, 1, 0, 512);
         tlbncfg[1] = gen_tlbncfg(64, 1, 12, TLBnCFG_AVAIL | TLBnCFG_IPROT, 64);
+#endif
         env->dcache_line_size = 64;
         env->icache_line_size = 64;
         l1cfg0 |= 0x1000000; /* 64 byte cache block size */
@@ -4492,7 +4499,6 @@  static void init_proc_e500 (CPUPPCState *env, int version)
     default:
         cpu_abort(env, "Unknown CPU: " TARGET_FMT_lx "\n", env->spr[SPR_PVR]);
     }
-#endif
     gen_spr_BookE206(env, 0x000000DF, tlbncfg);
     /* XXX : not implemented */
     spr_register(env, SPR_HID0, "HID0",