diff mbox

[Qemu-ppc] target-ppc: Init dcache and icache size for e500 user mode

Message ID 4F8AF3E6.3080501@suse.de
State New
Headers show

Commit Message

Andreas Färber April 15, 2012, 4:14 p.m. UTC 
Am 13.04.2012 14:00, schrieb Meador Inge:
> 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.
[...]
>> 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.

Not seeing a follow-up yet I've applied the following change to ppc-next:

-#endif
         env->dcache_line_size = 64;
         env->icache_line_size = 64;
         l1cfg0 |= 0x1000000; /* 64 byte cache block size */
@@ -4499,6 +4495,7 @@ 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",

http://repo.or.cz/w/qemu/agraf.git/shortlog/refs/heads/ppc-next

Thanks everyone,

Andreas

Comments

Alexander Graf April 18, 2012, 3:40 p.m. UTC | #1 
On 04/15/2012 06:14 PM, Andreas Färber wrote:
> Am 13.04.2012 14:00, schrieb Meador Inge:
>> 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.
> [...]
>>> 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.
> Not seeing a follow-up yet I've applied the following change to ppc-next:
>
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 86a915c..ba4b84d 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -4462,36 +4462,32 @@ static void init_proc_e500 (CPUPPCState *env,
> int version)
>                    &spr_read_spefscr,&spr_write_spefscr,
>                    0x00000000);
>       /* Memory management */
> -#if !defined(CONFIG_USER_ONLY)
> +#if defined(CONFIG_USER_ONLY)
> +    env->dcache_line_size = 32;
> +    env->icache_line_size = 32;
> +#else /* !defined(CONFIG_USER_ONLY) */

I don't think I like this one. If you're running linux-user with a 
specific CPU (not the generic "I am the common ground" CPU) then you're 
asking for behavior of that exact CPU. So the cache line size has to be 
exposed to the program.


Alex
Scott Wood April 18, 2012, 4:27 p.m. UTC | #2 
On 04/18/2012 10:40 AM, Alexander Graf wrote:
> On 04/15/2012 06:14 PM, Andreas Färber wrote:
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index 86a915c..ba4b84d 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -4462,36 +4462,32 @@ static void init_proc_e500 (CPUPPCState *env,
>> int version)
>>                    &spr_read_spefscr,&spr_write_spefscr,
>>                    0x00000000);
>>       /* Memory management */
>> -#if !defined(CONFIG_USER_ONLY)
>> +#if defined(CONFIG_USER_ONLY)
>> +    env->dcache_line_size = 32;
>> +    env->icache_line_size = 32;
>> +#else /* !defined(CONFIG_USER_ONLY) */
> 
> I don't think I like this one. If you're running linux-user with a
> specific CPU (not the generic "I am the common ground" CPU)

But how much value is there really in doing that, that justifies a bunch
of ifdefs in the per-CPU code?

-Scott
Andreas Färber April 18, 2012, 8:16 p.m. UTC | #3 
Am 18.04.2012 17:40, schrieb Alexander Graf:
> On 04/15/2012 06:14 PM, Andreas Färber wrote:
>> Am 13.04.2012 14:00, schrieb Meador Inge:
>>> 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.
>> [...]
>>>> 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.
>> Not seeing a follow-up yet I've applied the following change to ppc-next:
>>
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index 86a915c..ba4b84d 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -4462,36 +4462,32 @@ static void init_proc_e500 (CPUPPCState *env,
>> int version)
>>                    &spr_read_spefscr,&spr_write_spefscr,
>>                    0x00000000);
>>       /* Memory management */
>> -#if !defined(CONFIG_USER_ONLY)
>> +#if defined(CONFIG_USER_ONLY)
>> +    env->dcache_line_size = 32;
>> +    env->icache_line_size = 32;
>> +#else /* !defined(CONFIG_USER_ONLY) */
> 
> I don't think I like this one. If you're running linux-user with a
> specific CPU (not the generic "I am the common ground" CPU) then you're
> asking for behavior of that exact CPU. So the cache line size has to be
> exposed to the program.

Scott, Meador and me disliked the density of #ifdefs though. If you have
a better idea to structure the code feel free to change it.

Andreas
Alexander Graf April 18, 2012, 9:24 p.m. UTC | #4 
On 18.04.2012, at 18:27, Scott Wood wrote:

> On 04/18/2012 10:40 AM, Alexander Graf wrote:
>> On 04/15/2012 06:14 PM, Andreas Färber wrote:
>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>>> index 86a915c..ba4b84d 100644
>>> --- a/target-ppc/translate_init.c
>>> +++ b/target-ppc/translate_init.c
>>> @@ -4462,36 +4462,32 @@ static void init_proc_e500 (CPUPPCState *env,
>>> int version)
>>>                   &spr_read_spefscr,&spr_write_spefscr,
>>>                   0x00000000);
>>>      /* Memory management */
>>> -#if !defined(CONFIG_USER_ONLY)
>>> +#if defined(CONFIG_USER_ONLY)
>>> +    env->dcache_line_size = 32;
>>> +    env->icache_line_size = 32;
>>> +#else /* !defined(CONFIG_USER_ONLY) */
>> 
>> I don't think I like this one. If you're running linux-user with a
>> specific CPU (not the generic "I am the common ground" CPU)
> 
> But how much value is there really in doing that, that justifies a bunch
> of ifdefs in the per-CPU code?

As much value as in allowing specific CPUs to be selected in the first place. If you want the behavior of a specific CPU, we should give you the behavior of that specific CPU. That includes the dcbz zeroing length imho.


Alex
Alexander Graf April 18, 2012, 9:25 p.m. UTC | #5 
On 18.04.2012, at 22:16, Andreas Färber wrote:

> Am 18.04.2012 17:40, schrieb Alexander Graf:
>> On 04/15/2012 06:14 PM, Andreas Färber wrote:
>>> Am 13.04.2012 14:00, schrieb Meador Inge:
>>>> 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.
>>> [...]
>>>>> 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.
>>> Not seeing a follow-up yet I've applied the following change to ppc-next:
>>> 
>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>>> index 86a915c..ba4b84d 100644
>>> --- a/target-ppc/translate_init.c
>>> +++ b/target-ppc/translate_init.c
>>> @@ -4462,36 +4462,32 @@ static void init_proc_e500 (CPUPPCState *env,
>>> int version)
>>>                   &spr_read_spefscr,&spr_write_spefscr,
>>>                   0x00000000);
>>>      /* Memory management */
>>> -#if !defined(CONFIG_USER_ONLY)
>>> +#if defined(CONFIG_USER_ONLY)
>>> +    env->dcache_line_size = 32;
>>> +    env->icache_line_size = 32;
>>> +#else /* !defined(CONFIG_USER_ONLY) */
>> 
>> I don't think I like this one. If you're running linux-user with a
>> specific CPU (not the generic "I am the common ground" CPU) then you're
>> asking for behavior of that exact CPU. So the cache line size has to be
>> exposed to the program.
> 
> Scott, Meador and me disliked the density of #ifdefs though. If you have
> a better idea to structure the code feel free to change it.

Sure - just do 2 switch statements :).


Alex
Scott Wood April 18, 2012, 9:26 p.m. UTC | #6 
On 04/18/2012 04:24 PM, Alexander Graf wrote:
> 
> On 18.04.2012, at 18:27, Scott Wood wrote:
> 
>> On 04/18/2012 10:40 AM, Alexander Graf wrote:
>>> On 04/15/2012 06:14 PM, Andreas Färber wrote:
>>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>>>> index 86a915c..ba4b84d 100644
>>>> --- a/target-ppc/translate_init.c
>>>> +++ b/target-ppc/translate_init.c
>>>> @@ -4462,36 +4462,32 @@ static void init_proc_e500 (CPUPPCState *env,
>>>> int version)
>>>>                   &spr_read_spefscr,&spr_write_spefscr,
>>>>                   0x00000000);
>>>>      /* Memory management */
>>>> -#if !defined(CONFIG_USER_ONLY)
>>>> +#if defined(CONFIG_USER_ONLY)
>>>> +    env->dcache_line_size = 32;
>>>> +    env->icache_line_size = 32;
>>>> +#else /* !defined(CONFIG_USER_ONLY) */
>>>
>>> I don't think I like this one. If you're running linux-user with a
>>> specific CPU (not the generic "I am the common ground" CPU)
>>
>> But how much value is there really in doing that, that justifies a bunch
>> of ifdefs in the per-CPU code?
> 
> As much value as in allowing specific CPUs to be selected in the first place.

Right, I was wondering how much sense it makes to select a particular
CPU for user-only, or whether it might make more sense to just select
particular user-visible features.

The right answer is probably just to rework all this so that it's more
data-driven and we don't need so many ifdefs regardless.

-Scott
diff mbox

Patch

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 86a915c..ba4b84d 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -4462,36 +4462,32 @@  static void init_proc_e500 (CPUPPCState *env,
int version)
                  &spr_read_spefscr, &spr_write_spefscr,
                  0x00000000);
     /* Memory management */
-#if !defined(CONFIG_USER_ONLY)
+#if defined(CONFIG_USER_ONLY)
+    env->dcache_line_size = 32;
+    env->icache_line_size = 32;
+#else /* !defined(CONFIG_USER_ONLY) */
     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);