[PATCH v2] powerpc: Add POWER9 architected mode to cputable

[PATCH v2] powerpc: Add POWER9 architected mode to cputable

[Russell Currey]

PVR value of 0x0F000005 means we are arch v3.00 compliant (i.e. POWER9).

Acked-by: Michael Neuling <mikey@neuling.org>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
 arch/powerpc/kernel/cputable.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
index 6a82ef039c50..d23a54b09436 100644
--- a/arch/powerpc/kernel/cputable.c
+++ b/arch/powerpc/kernel/cputable.c
@@ -386,6 +386,25 @@ static struct cpu_spec __initdata cpu_specs[] = {
 		.machine_check_early	= __machine_check_early_realmode_p8,
 		.platform		= "power8",
 	},
+	{	/* 3.00-compliant processor, i.e. Power9 "architected" mode */
+		.pvr_mask		= 0xffffffff,
+		.pvr_value		= 0x0f000005,
+		.cpu_name		= "POWER9 (architected)",
+		.cpu_features		= CPU_FTRS_POWER9,
+		.cpu_user_features	= COMMON_USER_POWER9,
+		.cpu_user_features2	= COMMON_USER2_POWER9,
+		.mmu_features		= MMU_FTRS_POWER9,
+		.icache_bsize		= 128,
+		.dcache_bsize		= 128,
+		.num_pmcs		= 6,
+		.pmc_type		= PPC_PMC_IBM,
+		.oprofile_cpu_type	= "ppc64/ibm-compat-v1",
+		.oprofile_type		= PPC_OPROFILE_INVALID,
+		.cpu_setup		= __setup_cpu_power9,
+		.cpu_restore		= __restore_cpu_power9,
+		.flush_tlb		= __flush_tlb_power9,
+		.platform		= "power9",
+	},
 	{	/* Power7 */
 		.pvr_mask		= 0xffff0000,
 		.pvr_value		= 0x003f0000,
-- 
2.11.1

Re: [PATCH v2] powerpc: Add POWER9 architected mode to cputable

[Daniel Axtens]

Hi Russell,

This seems to go Power8, Power9, Power7 - is that intentional?

Regards,
Daniel

>  		.platform		= "power8",
...
> +		.platform		= "power9",
...
>  	{	/* Power7 */

Re: [PATCH v2] powerpc: Add POWER9 architected mode to cputable

[Michael Ellerman]

Daniel Axtens <dja@axtens.net> writes:

> Hi Russell,
>
> This seems to go Power8, Power9, Power7 - is that intentional?
>
> Regards,
> Daniel
>
>>  		.platform		= "power8",
> ...
>> +		.platform		= "power9",
> ...
>>  	{	/* Power7 */

It's because we have the architected PVRs and the raw ones, and the
ordering is a bit odd:

$ grep -n -e '(architected)' -e '(raw)' arch/powerpc/kernel/cputable.c
323:		.cpu_name		= "POWER6 (raw)",
343:		.cpu_name		= "POWER6 (architected)",
356:		.cpu_name		= "POWER7 (architected)",
374:		.cpu_name		= "POWER8 (architected)",
392:		.cpu_name		= "POWER9 (architected)",
411:		.cpu_name		= "POWER7 (raw)",
431:		.cpu_name		= "POWER7+ (raw)",
451:		.cpu_name		= "POWER8E (raw)",
471:		.cpu_name		= "POWER8NVL (raw)",
491:		.cpu_name		= "POWER8 (raw)",
511:		.cpu_name		= "POWER8 (raw)",
531:		.cpu_name		= "POWER9 (raw)",
550:		.cpu_name		= "POWER9 (raw)",

But I don't think it matters in practice. The architected entries use a
pvr_mask of 0xffffffff so it has to be an exact match.

cheers

Re: [PATCH v2] powerpc: Add POWER9 architected mode to cputable

[Michael Ellerman]

Russell Currey <ruscur@russell.cc> writes:

> diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
> index 6a82ef039c50..d23a54b09436 100644
> --- a/arch/powerpc/kernel/cputable.c
> +++ b/arch/powerpc/kernel/cputable.c
> @@ -386,6 +386,25 @@ static struct cpu_spec __initdata cpu_specs[] = {
>  		.machine_check_early	= __machine_check_early_realmode_p8,
>  		.platform		= "power8",
>  	},
> +	{	/* 3.00-compliant processor, i.e. Power9 "architected" mode */
> +		.pvr_mask		= 0xffffffff,
> +		.pvr_value		= 0x0f000005,
> +		.cpu_name		= "POWER9 (architected)",
> +		.cpu_features		= CPU_FTRS_POWER9,
> +		.cpu_user_features	= COMMON_USER_POWER9,
> +		.cpu_user_features2	= COMMON_USER2_POWER9,
> +		.mmu_features		= MMU_FTRS_POWER9,
> +		.icache_bsize		= 128,
> +		.dcache_bsize		= 128,
> +		.num_pmcs		= 6,

It's important *not* to set num_pmcs for the architected PVRs.

See the comment in setup_cpu_spec():

	/*
	 * If we are overriding a previous value derived from the real
	 * PVR with a new value obtained using a logical PVR value,
	 * don't modify the performance monitor fields.
	 */
	if (old.num_pmcs && !s->num_pmcs) {
		t->num_pmcs = old.num_pmcs;
		t->pmc_type = old.pmc_type;

I realise that having that requirement in the code is serious foot gun
material on our part, but c'est la vie.

The reason we do that is there's no "compat mode" for the PMU. So if you
boot on a Power9, and then the logical PVR says "actually pretend you're
on a Power8", we flip most of the cpu_spec to have the Power8 values,
but *not* the PMU fields. That way the Power9 PMU code will still detect
that it's on a Power9 and work correctly.

Possibly now that oprofile is more or less dead we can rip all that crap
out, and have perf just look at the PVR directly.

cheers

Re: [PATCH v2] powerpc: Add POWER9 architected mode to cputable

[Russell Currey]

On Fri, 2017-02-17 at 21:26 +1100, Michael Ellerman wrote:
> Russell Currey <ruscur@russell.cc> writes:
> 
> > diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
> > index 6a82ef039c50..d23a54b09436 100644
> > --- a/arch/powerpc/kernel/cputable.c
> > +++ b/arch/powerpc/kernel/cputable.c
> > @@ -386,6 +386,25 @@ static struct cpu_spec __initdata cpu_specs[] = {
> >  		.machine_check_early	=
> > __machine_check_early_realmode_p8,
> >  		.platform		= "power8",
> >  	},
> > +	{	/* 3.00-compliant processor, i.e. Power9 "architected"
> > mode */
> > +		.pvr_mask		= 0xffffffff,
> > +		.pvr_value		= 0x0f000005,
> > +		.cpu_name		= "POWER9 (architected)",
> > +		.cpu_features		= CPU_FTRS_POWER9,
> > +		.cpu_user_features	= COMMON_USER_POWER9,
> > +		.cpu_user_features2	= COMMON_USER2_POWER9,
> > +		.mmu_features		= MMU_FTRS_POWER9,
> > +		.icache_bsize		= 128,
> > +		.dcache_bsize		= 128,
> > +		.num_pmcs		= 6,
> 
> It's important *not* to set num_pmcs for the architected PVRs.
> 
> See the comment in setup_cpu_spec():
> 
> 	/*
> 	 * If we are overriding a previous value derived from the real
> 	 * PVR with a new value obtained using a logical PVR value,
> 	 * don't modify the performance monitor fields.
> 	 */
> 	if (old.num_pmcs && !s->num_pmcs) {
> 		t->num_pmcs = old.num_pmcs;
> 		t->pmc_type = old.pmc_type;
> 
> I realise that having that requirement in the code is serious foot gun
> material on our part, but c'est la vie.
> 
> The reason we do that is there's no "compat mode" for the PMU. So if you
> boot on a Power9, and then the logical PVR says "actually pretend you're
> on a Power8", we flip most of the cpu_spec to have the Power8 values,
> but *not* the PMU fields. That way the Power9 PMU code will still detect
> that it's on a Power9 and work correctly.
> 
> Possibly now that oprofile is more or less dead we can rip all that crap
> out, and have perf just look at the PVR directly.

Thanks a lot for explaining, that's interesting.  I thought it might just have
been an accidental omission in the architected entries but I should've dug
deeper.

> 
> cheers

Re: [PATCH v2] powerpc: Add POWER9 architected mode to cputable

[Russell Currey]

On Fri, 2017-02-17 at 21:26 +1100, Michael Ellerman wrote:
> Russell Currey <ruscur@russell.cc> writes:
> 
> > diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
> > index 6a82ef039c50..d23a54b09436 100644
> > --- a/arch/powerpc/kernel/cputable.c
> > +++ b/arch/powerpc/kernel/cputable.c
> > @@ -386,6 +386,25 @@ static struct cpu_spec __initdata cpu_specs[] = {
> >  		.machine_check_early	=
> > __machine_check_early_realmode_p8,
> >  		.platform		= "power8",
> >  	},
> > +	{	/* 3.00-compliant processor, i.e. Power9 "architected"
> > mode */
> > +		.pvr_mask		= 0xffffffff,
> > +		.pvr_value		= 0x0f000005,
> > +		.cpu_name		= "POWER9 (architected)",
> > +		.cpu_features		= CPU_FTRS_POWER9,
> > +		.cpu_user_features	= COMMON_USER_POWER9,
> > +		.cpu_user_features2	= COMMON_USER2_POWER9,
> > +		.mmu_features		= MMU_FTRS_POWER9,
> > +		.icache_bsize		= 128,
> > +		.dcache_bsize		= 128,
> > +		.num_pmcs		= 6,
> 
> It's important *not* to set num_pmcs for the architected PVRs.
> 
> See the comment in setup_cpu_spec():
> 
> 	/*
> 	 * If we are overriding a previous value derived from the real
> 	 * PVR with a new value obtained using a logical PVR value,
> 	 * don't modify the performance monitor fields.
> 	 */
> 	if (old.num_pmcs && !s->num_pmcs) {
> 		t->num_pmcs = old.num_pmcs;
> 		t->pmc_type = old.pmc_type;
> 
> I realise that having that requirement in the code is serious foot gun
> material on our part, but c'est la vie.
> 
> The reason we do that is there's no "compat mode" for the PMU. So if you
> boot on a Power9, and then the logical PVR says "actually pretend you're
> on a Power8", we flip most of the cpu_spec to have the Power8 values,
> but *not* the PMU fields. That way the Power9 PMU code will still detect
> that it's on a Power9 and work correctly.
> 
> Possibly now that oprofile is more or less dead we can rip all that crap
> out, and have perf just look at the PVR directly.

Oh and also, do you want me to respin or are you happy to drop it on your end?

- Russell

> 
> cheers

Re: [PATCH v2] powerpc: Add POWER9 architected mode to cputable

[Michael Ellerman]

Russell Currey <ruscur@russell.cc> writes:
> On Fri, 2017-02-17 at 21:26 +1100, Michael Ellerman wrote:
>> Russell Currey <ruscur@russell.cc> writes:
>> > diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cput=
able.c
>> > index 6a82ef039c50..d23a54b09436 100644
>> > --- a/arch/powerpc/kernel/cputable.c
>> > +++ b/arch/powerpc/kernel/cputable.c
>> > @@ -386,6 +386,25 @@ static struct cpu_spec __initdata cpu_specs[] =3D=
 {
>> > =C2=A0		.machine_check_early	=3D
>> > __machine_check_early_realmode_p8,
>> > =C2=A0		.platform		=3D "power8",
>> > =C2=A0	},
>> > +	{	/* 3.00-compliant processor, i.e. Power9 "architected"
>> > mode */
>> > +		.pvr_mask		=3D 0xffffffff,
>> > +		.pvr_value		=3D 0x0f000005,
>> > +		.cpu_name		=3D "POWER9 (architected)",
>> > +		.cpu_features		=3D CPU_FTRS_POWER9,
>> > +		.cpu_user_features	=3D COMMON_USER_POWER9,
>> > +		.cpu_user_features2	=3D COMMON_USER2_POWER9,
>> > +		.mmu_features		=3D MMU_FTRS_POWER9,
>> > +		.icache_bsize		=3D 128,
>> > +		.dcache_bsize		=3D 128,
>> > +		.num_pmcs		=3D 6,
>>=20
>> It's important *not* to set num_pmcs for the architected PVRs.
...
>
> Oh and also, do you want me to respin or are you happy to drop it on your=
 end?

I fixed it up:

commit 6ae3f8ad2017079292cb49c8959b527bcbcbefed
Author:     Russell Currey <ruscur@russell.cc>
AuthorDate: Fri Feb 17 13:01:35 2017 +1100
Commit:     Michael Ellerman <mpe@ellerman.id.au>
CommitDate: Fri Feb 17 21:48:56 2017 +1100

    powerpc: Add POWER9 architected mode to cputable
=20=20=20=20
    PVR value of 0x0F000005 means we are arch v3.00 compliant (i.e. POWER9).
=20=20=20=20
    Acked-by: Michael Neuling <mikey@neuling.org>
    Signed-off-by: Russell Currey <ruscur@russell.cc>
    [mpe: Don't set num_pmcs, so we keep the PMU fields from the raw entry]
    Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
index 6a82ef039c50..bb7a1890aeb7 100644
--- a/arch/powerpc/kernel/cputable.c
+++ b/arch/powerpc/kernel/cputable.c
@@ -386,6 +386,23 @@ static struct cpu_spec __initdata cpu_specs[] =3D {
 		.machine_check_early	=3D __machine_check_early_realmode_p8,
 		.platform		=3D "power8",
 	},
+	{	/* 3.00-compliant processor, i.e. Power9 "architected" mode */
+		.pvr_mask		=3D 0xffffffff,
+		.pvr_value		=3D 0x0f000005,
+		.cpu_name		=3D "POWER9 (architected)",
+		.cpu_features		=3D CPU_FTRS_POWER9,
+		.cpu_user_features	=3D COMMON_USER_POWER9,
+		.cpu_user_features2	=3D COMMON_USER2_POWER9,
+		.mmu_features		=3D MMU_FTRS_POWER9,
+		.icache_bsize		=3D 128,
+		.dcache_bsize		=3D 128,
+		.oprofile_type		=3D PPC_OPROFILE_INVALID,
+		.oprofile_cpu_type	=3D "ppc64/ibm-compat-v1",
+		.cpu_setup		=3D __setup_cpu_power9,
+		.cpu_restore		=3D __restore_cpu_power9,
+		.flush_tlb		=3D __flush_tlb_power9,
+		.platform		=3D "power9",
+	},
 	{	/* Power7 */
 		.pvr_mask		=3D 0xffff0000,
 		.pvr_value		=3D 0x003f0000,


cheers

Re: [v2] powerpc: Add POWER9 architected mode to cputable

[Michael Ellerman]

On Fri, 2017-02-17 at 02:01:35 UTC, Russell Currey wrote:
> PVR value of 0x0F000005 means we are arch v3.00 compliant (i.e. POWER9).
> 
> Acked-by: Michael Neuling <mikey@neuling.org>
> Signed-off-by: Russell Currey <ruscur@russell.cc>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/6ae3f8ad2017079292cb49c8959b52

cheers