Message ID | 4F8AF3E6.3080501@suse.de |
---|---|
State | New |
Headers | show |
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
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
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
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
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
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 --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);