Re: [V2, 68/68] powerpc/mm/radix: Use firmware feature to disable radix

[Michael Neuling]

On Wed, 2016-04-20 at 12:59 +1000, Michael Ellerman wrote:

> On Sat, 2016-09-04 at 06:14:04 UTC, "Aneesh Kumar K.V" wrote:

> > We can depend on ibm,pa-features to enable/disable radix. This gives us
> > a nice way to test p9 hash config, by changing device tree property.
> 
> I think we might want to be more careful here.
> 
> You set MMU_FTR_RADIX in the cputable entry. So it's on by default on P9 cpus.
> 
> Then if there is an ibm,pa-features property *and* it is >= 41 bytes long, the
> below feature entry will hit. In that case the firmware controls whether it's 
> on
> or off.
> 
> I think it would be clearer if we removed RADIX from the cputable, and the 
> below
> became the only way to turn it on. Would that break anything?

I don't think it'll break anything.  FWIW I'm good with this approach.

We'll need to teach firmware about this feature, but I think we should
do that anyway.  skiboot is a bit of a mess WRT to this property with
HDAT vs device tree, but we can clean that up there.

Mikey


> 

> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> > index 7030b035905d..a4d1f44364b8 100644
> > --- a/arch/powerpc/kernel/prom.c
> > +++ b/arch/powerpc/kernel/prom.c
> > @@ -165,6 +165,7 @@ static struct ibm_pa_feature {
> >  * which is 0 if the kernel doesn't support TM.
> >  */
> > {CPU_FTR_TM_COMP, 0, 0, 22, 0, 0},
> > +   {0, MMU_FTR_RADIX, 0,   40, 0, 0},
> 
> So that says bit 0 of byte 40 enables MMU_FTR_RADIX. Where is that documented?
> 
> cheers
> ___
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [V2, 68/68] powerpc/mm/radix: Use firmware feature to disable radix

[Aneesh Kumar K.V]

Michael Ellerman  writes:

> On Sat, 2016-09-04 at 06:14:04 UTC, "Aneesh Kumar K.V" wrote:
>> We can depend on ibm,pa-features to enable/disable radix. This gives us
>> a nice way to test p9 hash config, by changing device tree property.
>
> I think we might want to be more careful here.
>
> You set MMU_FTR_RADIX in the cputable entry. So it's on by default on P9 cpus.
>
> Then if there is an ibm,pa-features property *and* it is >= 41 bytes long, the
> below feature entry will hit. In that case the firmware controls whether it's 
> on
> or off.
>
> I think it would be clearer if we removed RADIX from the cputable, and the 
> below
> became the only way to turn it on. Would that break anything?

I was thinking we want to use RADIX by default on Power9. Hence the idea
of clearing feature bit. Now we will be using this to switch
between Radix and hash segment table. We are not really going to use
this to switch between the long tested SLB hash to the newly
added Power9 Radix. So it is not really there to make sure we run safely
with old code by default.

>
>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> index 7030b035905d..a4d1f44364b8 100644
>> --- a/arch/powerpc/kernel/prom.c
>> +++ b/arch/powerpc/kernel/prom.c
>> @@ -165,6 +165,7 @@ static struct ibm_pa_feature {
>>   * which is 0 if the kernel doesn't support TM.
>>   */
>>  {CPU_FTR_TM_COMP, 0, 0, 22, 0, 0},
>> +{0, MMU_FTR_RADIX, 0,   40, 0, 0},
>
> So that says bit 0 of byte 40 enables MMU_FTR_RADIX. Where is that documented?
>

In PAPR document.

-aneehs

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [V2, 68/68] powerpc/mm/radix: Use firmware feature to disable radix

[Michael Ellerman]

On Sat, 2016-09-04 at 06:14:04 UTC, "Aneesh Kumar K.V" wrote:
> We can depend on ibm,pa-features to enable/disable radix. This gives us
> a nice way to test p9 hash config, by changing device tree property.

I think we might want to be more careful here.

You set MMU_FTR_RADIX in the cputable entry. So it's on by default on P9 cpus.

Then if there is an ibm,pa-features property *and* it is >= 41 bytes long, the
below feature entry will hit. In that case the firmware controls whether it's on
or off.

I think it would be clearer if we removed RADIX from the cputable, and the below
became the only way to turn it on. Would that break anything?

> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 7030b035905d..a4d1f44364b8 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -165,6 +165,7 @@ static struct ibm_pa_feature {
>* which is 0 if the kernel doesn't support TM.
>*/
>   {CPU_FTR_TM_COMP, 0, 0, 22, 0, 0},
> + {0, MMU_FTR_RADIX, 0,   40, 0, 0},

So that says bit 0 of byte 40 enables MMU_FTR_RADIX. Where is that documented?

cheers
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH V2 68/68] powerpc/mm/radix: Use firmware feature to disable radix

[Aneesh Kumar K.V]

We can depend on ibm,pa-features to enable/disable radix. This gives us
a nice way to test p9 hash config, by changing device tree property.

Signed-off-by: Aneesh Kumar K.V 
---
 arch/powerpc/kernel/prom.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 7030b035905d..a4d1f44364b8 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -165,6 +165,7 @@ static struct ibm_pa_feature {
 * which is 0 if the kernel doesn't support TM.
 */
{CPU_FTR_TM_COMP, 0, 0, 22, 0, 0},
+   {0, MMU_FTR_RADIX, 0,   40, 0, 0},
 };
 
 static void __init scan_features(unsigned long node, const unsigned char *ftrs,
-- 
2.5.0

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev