Patchwork arch/powerpc: advertise ISA2.07, HTM, DSCR, EBB and ISEL bits in HWCAP2

login
register
mail settings
Submitter Nishanth Aravamudan
Date May 3, 2013, 11:19 p.m.
Message ID <20130503231933.GA29436@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/241410/
State Superseded
Headers show

Comments

Nishanth Aravamudan - May 3, 2013, 11:19 p.m.
Now that we have AT_HWCAP2 support, start exposing some of the new
POWER8 features via it.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>

---
Note: there are, I think, some Freescale processors that also should be
updated to indicate they support ISEL, but I don't know which ones.
Since this is a new feature bit (and vector), it seems like we can fix
that up in a follow-on patch. Also, this is my first patch trying to
manipulate these bits, so please let me know if I'm doing something
wrong (for instance, I don't see any particular order to the bits in
PPC_FEATURE_*)
Benjamin Herrenschmidt - May 3, 2013, 11:23 p.m.
On Fri, 2013-05-03 at 16:19 -0700, Nishanth Aravamudan wrote:
> +/* in AT_HWCAP2 */
> +#define PPC_FEATURE2_ARCH_2_07         0x80000000
> +#define PPC_FEATURE2_HTM               0x40000000
> +#define PPC_FEATURE2_DSCR              0x20000000
> +#define PPC_FEATURE2_EBB               0x10000000
> +#define PPC_FEATURE2_ISEL              0x08000000

Should we "adjust" (ie filter out) some of these based
on CONFIG_ options (such as transactional memory enabled,
EBB supported by the hypervisor, etc...) ?

Cheers,
Ben.
Nishanth Aravamudan - May 3, 2013, 11:40 p.m.
On 04.05.2013 [09:23:51 +1000], Benjamin Herrenschmidt wrote:
> On Fri, 2013-05-03 at 16:19 -0700, Nishanth Aravamudan wrote:
> > +/* in AT_HWCAP2 */
> > +#define PPC_FEATURE2_ARCH_2_07         0x80000000
> > +#define PPC_FEATURE2_HTM               0x40000000
> > +#define PPC_FEATURE2_DSCR              0x20000000
> > +#define PPC_FEATURE2_EBB               0x10000000
> > +#define PPC_FEATURE2_ISEL              0x08000000
> 
> Should we "adjust" (ie filter out) some of these based
> on CONFIG_ options (such as transactional memory enabled,
> EBB supported by the hypervisor, etc...) ?

Err, yeah, that seems reasonable :) However, it seems like glibc uses
these values rather directly so it knows what bits to check for each
feature. Therefore, it seems like it would be better to do the
ifdeffery/checking in the user in cputable.c, but that seems like it
could get quite complicated.

Would it be ok (I guess I'm asking Ryan & co. here) to have an #ifdef in
the definition that may or may not mean the bit is set in the aux
vector, but the bit, if set, would always be the same bit?

-Nish
Michael Neuling - May 4, 2013, 12:04 a.m.
Nishanth Aravamudan <nacc@linux.vnet.ibm.com> wrote:

> Now that we have AT_HWCAP2 support, start exposing some of the new
> POWER8 features via it.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>

A few points:

We need a TAR bit as well, although this could be covered in 207?

For TM we need to turn it off if CONFIG_PPC_TRANSACTIONAL_MEM is not
set.  Look at PPC_FEATURE_HAS_ALTIVEC_COMP for how we do that and follow
the bouncing ball.  

Please add ISEL on other processors.

Please add DSCR on other processors.

EBB can be reserved, but there is no enablement at this point so don't
turn it on yet.  We'll turn it on when we send the perf API.

Mikey

> ---
> Note: there are, I think, some Freescale processors that also should be
> updated to indicate they support ISEL, but I don't know which ones.
> Since this is a new feature bit (and vector), it seems like we can fix
> that up in a follow-on patch. Also, this is my first patch trying to
> manipulate these bits, so please let me know if I'm doing something
> wrong (for instance, I don't see any particular order to the bits in
> PPC_FEATURE_*)
> 
> diff --git a/arch/powerpc/include/uapi/asm/cputable.h b/arch/powerpc/include/uapi/asm/cputable.h
> index ed9dd81..78db4e2 100644
> --- a/arch/powerpc/include/uapi/asm/cputable.h
> +++ b/arch/powerpc/include/uapi/asm/cputable.h
> @@ -1,6 +1,7 @@
>  #ifndef _UAPI__ASM_POWERPC_CPUTABLE_H
>  #define _UAPI__ASM_POWERPC_CPUTABLE_H
>  
> +/* in AT_HWCAP */
>  #define PPC_FEATURE_32			0x80000000
>  #define PPC_FEATURE_64			0x40000000
>  #define PPC_FEATURE_601_INSTR		0x20000000
> @@ -33,4 +34,11 @@
>  #define PPC_FEATURE_TRUE_LE		0x00000002
>  #define PPC_FEATURE_PPC_LE		0x00000001
>  
> +/* in AT_HWCAP2 */
> +#define PPC_FEATURE2_ARCH_2_07		0x80000000
> +#define PPC_FEATURE2_HTM		0x40000000
> +#define PPC_FEATURE2_DSCR		0x20000000
> +#define PPC_FEATURE2_EBB		0x10000000
> +#define PPC_FEATURE2_ISEL		0x08000000
> +
>  #endif /* _UAPI__ASM_POWERPC_CPUTABLE_H */
> diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
> index ae9f433..871c741 100644
> --- a/arch/powerpc/kernel/cputable.c
> +++ b/arch/powerpc/kernel/cputable.c
> @@ -102,6 +102,9 @@ extern void __restore_cpu_e6500(void);
>  				 PPC_FEATURE_SMT | PPC_FEATURE_ICACHE_SNOOP | \
>  				 PPC_FEATURE_TRUE_LE | \
>  				 PPC_FEATURE_PSERIES_PERFMON_COMPAT)
> +#define COMMON_USER2_POWER8	(PPC_FEATURE2_ARCH_2_07 | PPC_FEATURE2_HTM | \
> +				 PPC_FEATURE2_DSCR | PPC_FEATURE2_EBB | \
> +				 PPC_FEATURE2_ISEL)
>  #define COMMON_USER_PA6T	(COMMON_USER_PPC64 | PPC_FEATURE_PA6T |\
>  				 PPC_FEATURE_TRUE_LE | \
>  				 PPC_FEATURE_HAS_ALTIVEC_COMP)
> @@ -443,6 +446,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
>  		.cpu_name		= "POWER8 (architected)",
>  		.cpu_features		= CPU_FTRS_POWER8,
>  		.cpu_user_features	= COMMON_USER_POWER8,
> +		.cpu_user_features2	= COMMON_USER2_POWER8,
>  		.mmu_features		= MMU_FTRS_POWER8,
>  		.icache_bsize		= 128,
>  		.dcache_bsize		= 128,
> @@ -492,6 +496,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
>  		.cpu_name		= "POWER8 (raw)",
>  		.cpu_features		= CPU_FTRS_POWER8,
>  		.cpu_user_features	= COMMON_USER_POWER8,
> +		.cpu_user_features2	= COMMON_USER2_POWER8,
>  		.mmu_features		= MMU_FTRS_POWER8,
>  		.icache_bsize		= 128,
>  		.dcache_bsize		= 128,
>
Ryan Arnold - May 6, 2013, 2:38 p.m.
Nishanth Aravamudan <nacc@linux.vnet.ibm.com> wrote on 05/03/2013 06:40:19
PM:

> Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> 05/03/2013 06:40 PM
>
> To
>
> Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> cc
>
> Michael Neuling <michael.neuling@au1.ibm.com>, Michael R Meissner/
> Cambridge/IBM@IBMUS, Steve Munroe/Rochester/IBM@IBMUS, Peter
> Bergner/Rochester/IBM@IBMUS, Ryan Arnold/Rochester/IBM@IBMUS,
> linuxppc-dev@lists.ozlabs.org
>
> Subject
>
> Re: [PATCH] arch/powerpc: advertise ISA2.07, HTM, DSCR, EBB and ISEL
> bits in HWCAP2
>
> On 04.05.2013 [09:23:51 +1000], Benjamin Herrenschmidt wrote:
> > On Fri, 2013-05-03 at 16:19 -0700, Nishanth Aravamudan wrote:
> > > +/* in AT_HWCAP2 */
> > > +#define PPC_FEATURE2_ARCH_2_07         0x80000000
> > > +#define PPC_FEATURE2_HTM               0x40000000
> > > +#define PPC_FEATURE2_DSCR              0x20000000
> > > +#define PPC_FEATURE2_EBB               0x10000000
> > > +#define PPC_FEATURE2_ISEL              0x08000000
> >
> > Should we "adjust" (ie filter out) some of these based
> > on CONFIG_ options (such as transactional memory enabled,
> > EBB supported by the hypervisor, etc...) ?
>
> Err, yeah, that seems reasonable :) However, it seems like glibc uses
> these values rather directly so it knows what bits to check for each
> feature. Therefore, it seems like it would be better to do the
> ifdeffery/checking in the user in cputable.c, but that seems like it
> could get quite complicated.
>
> Would it be ok (I guess I'm asking Ryan & co. here) to have an #ifdef in
> the definition that may or may not mean the bit is set in the aux
> vector, but the bit, if set, would always be the same bit?

My understanding was that these bits being 'on' is an indication of what
features the hardware supports (or what the kernel emulates) and a not an
indication of whether that facility is currently enabled or not.  If the
hardware supports a particular feature but it is not enabled I'd expect
that user-space usage of that feature would cause the kernel to trap on a
facility availability exception (which is how Altivec/VMX is implemented,
being defaulted to turned off).

Otherwise there's no way I could know whether an ISA [optional] feature is
actually available on a particular machine.

And yes, the bits can't change.  My usage of hwcap.h has to coincide with
the kernel's asm/cputable.h
Ryan
Benjamin Herrenschmidt - May 6, 2013, 9:37 p.m.
On Mon, 2013-05-06 at 09:38 -0500, Ryan Arnold wrote:
> My understanding was that these bits being 'on' is an indication of
> what features the hardware supports (or what the kernel emulates) and
> a not an indication of whether that facility is currently enabled or
> not.  If the hardware supports a particular feature but it is not
> enabled I'd expect that user-space usage of that feature would cause
> the kernel to trap on a facility availability exception (which is how
> Altivec/VMX is implemented, being defaulted to turned off).

Right but the discussion is about whether we should expose the bits
when the kernel doesn't have the ability to handle the feature :-)

IE. We need to remove the HTM feature if the kernel is compiled without
transactional memory support.

Similarily, Nish, you may need to check that we remove those bits if
pHyp has the partition in a mode that doesn't support them (P7
compatibility for example) for migration purposes.

Cheers,
Ben.

> Otherwise there's no way I could know whether an ISA [optional]
> feature is actually available on a particular machine.
> 
> And yes, the bits can't change.  My usage of hwcap.h has to coincide
> with the kernel's asm/cputable.h
Ryan Arnold - May 7, 2013, 3:11 p.m.
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 05/06/2013
04:37:08 PM:

> Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 05/06/2013 04:37 PM
>
> To
>
> Ryan Arnold/Rochester/IBM@IBMUS
>
> cc
>
> Nishanth Aravamudan <nacc@linux.vnet.ibm.com>, linuxppc-
> dev@lists.ozlabs.org, Michael R Meissner/Cambridge/IBM@IBMUS,
> Michael Neuling <michael.neuling@au1.ibm.com>, Peter Bergner/
> Rochester/IBM@IBMUS, Steve Munroe/Rochester/IBM@IBMUS
>
> Subject
>
> Re: [PATCH] arch/powerpc: advertise ISA2.07, HTM, DSCR, EBB and ISEL
> bits in HWCAP2
>
> On Mon, 2013-05-06 at 09:38 -0500, Ryan Arnold wrote:
> > My understanding was that these bits being 'on' is an indication of
> > what features the hardware supports (or what the kernel emulates) and
> > a not an indication of whether that facility is currently enabled or
> > not.  If the hardware supports a particular feature but it is not
> > enabled I'd expect that user-space usage of that feature would cause
> > the kernel to trap on a facility availability exception (which is how
> > Altivec/VMX is implemented, being defaulted to turned off).
>
> Right but the discussion is about whether we should expose the bits
> when the kernel doesn't have the ability to handle the feature :-)
>
> IE. We need to remove the HTM feature if the kernel is compiled without
> transactional memory support.

Thanks for explaining. This is exactly how it should work.

Ryan
Nishanth Aravamudan - May 7, 2013, 8:33 p.m.
On 07.05.2013 [07:37:08 +1000], Benjamin Herrenschmidt wrote:
> On Mon, 2013-05-06 at 09:38 -0500, Ryan Arnold wrote:
> > My understanding was that these bits being 'on' is an indication of
> > what features the hardware supports (or what the kernel emulates) and
> > a not an indication of whether that facility is currently enabled or
> > not.  If the hardware supports a particular feature but it is not
> > enabled I'd expect that user-space usage of that feature would cause
> > the kernel to trap on a facility availability exception (which is how
> > Altivec/VMX is implemented, being defaulted to turned off).
> 
> Right but the discussion is about whether we should expose the bits
> when the kernel doesn't have the ability to handle the feature :-)
> 
> IE. We need to remove the HTM feature if the kernel is compiled without
> transactional memory support.
> 
> Similarily, Nish, you may need to check that we remove those bits if
> pHyp has the partition in a mode that doesn't support them (P7
> compatibility for example) for migration purposes.

Yep, I'll need to talk with Mikey about this part. Will be a follow-on
patch if needed. Minimally, the bit defines will stay the same, which is
the important part to get going right now.

Thanks,
Nish
Benjamin Herrenschmidt - May 7, 2013, 8:49 p.m.
On Tue, 2013-05-07 at 13:33 -0700, Nishanth Aravamudan wrote:
> > Similarily, Nish, you may need to check that we remove those bits if
> > pHyp has the partition in a mode that doesn't support them (P7
> > compatibility for example) for migration purposes.
> 
> Yep, I'll need to talk with Mikey about this part. Will be a follow-on
> patch if needed. Minimally, the bit defines will stay the same, which is
> the important part to get going right now.

Actually in such a mode we'd get a back-version architected PVR so we
should be fine now that I think twice, but of course that need to be
tested.

Cheers,
Ben.
Nishanth Aravamudan - May 7, 2013, 9:11 p.m.
On 08.05.2013 [06:49:38 +1000], Benjamin Herrenschmidt wrote:
> On Tue, 2013-05-07 at 13:33 -0700, Nishanth Aravamudan wrote:
> > > Similarily, Nish, you may need to check that we remove those bits if
> > > pHyp has the partition in a mode that doesn't support them (P7
> > > compatibility for example) for migration purposes.
> > 
> > Yep, I'll need to talk with Mikey about this part. Will be a follow-on
> > patch if needed. Minimally, the bit defines will stay the same, which is
> > the important part to get going right now.
> 
> Actually in such a mode we'd get a back-version architected PVR so we
> should be fine now that I think twice, but of course that need to be
> tested.

True, I'll make sure it does get tested.

-Nish

Patch

diff --git a/arch/powerpc/include/uapi/asm/cputable.h b/arch/powerpc/include/uapi/asm/cputable.h
index ed9dd81..78db4e2 100644
--- a/arch/powerpc/include/uapi/asm/cputable.h
+++ b/arch/powerpc/include/uapi/asm/cputable.h
@@ -1,6 +1,7 @@ 
 #ifndef _UAPI__ASM_POWERPC_CPUTABLE_H
 #define _UAPI__ASM_POWERPC_CPUTABLE_H
 
+/* in AT_HWCAP */
 #define PPC_FEATURE_32			0x80000000
 #define PPC_FEATURE_64			0x40000000
 #define PPC_FEATURE_601_INSTR		0x20000000
@@ -33,4 +34,11 @@ 
 #define PPC_FEATURE_TRUE_LE		0x00000002
 #define PPC_FEATURE_PPC_LE		0x00000001
 
+/* in AT_HWCAP2 */
+#define PPC_FEATURE2_ARCH_2_07		0x80000000
+#define PPC_FEATURE2_HTM		0x40000000
+#define PPC_FEATURE2_DSCR		0x20000000
+#define PPC_FEATURE2_EBB		0x10000000
+#define PPC_FEATURE2_ISEL		0x08000000
+
 #endif /* _UAPI__ASM_POWERPC_CPUTABLE_H */
diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
index ae9f433..871c741 100644
--- a/arch/powerpc/kernel/cputable.c
+++ b/arch/powerpc/kernel/cputable.c
@@ -102,6 +102,9 @@  extern void __restore_cpu_e6500(void);
 				 PPC_FEATURE_SMT | PPC_FEATURE_ICACHE_SNOOP | \
 				 PPC_FEATURE_TRUE_LE | \
 				 PPC_FEATURE_PSERIES_PERFMON_COMPAT)
+#define COMMON_USER2_POWER8	(PPC_FEATURE2_ARCH_2_07 | PPC_FEATURE2_HTM | \
+				 PPC_FEATURE2_DSCR | PPC_FEATURE2_EBB | \
+				 PPC_FEATURE2_ISEL)
 #define COMMON_USER_PA6T	(COMMON_USER_PPC64 | PPC_FEATURE_PA6T |\
 				 PPC_FEATURE_TRUE_LE | \
 				 PPC_FEATURE_HAS_ALTIVEC_COMP)
@@ -443,6 +446,7 @@  static struct cpu_spec __initdata cpu_specs[] = {
 		.cpu_name		= "POWER8 (architected)",
 		.cpu_features		= CPU_FTRS_POWER8,
 		.cpu_user_features	= COMMON_USER_POWER8,
+		.cpu_user_features2	= COMMON_USER2_POWER8,
 		.mmu_features		= MMU_FTRS_POWER8,
 		.icache_bsize		= 128,
 		.dcache_bsize		= 128,
@@ -492,6 +496,7 @@  static struct cpu_spec __initdata cpu_specs[] = {
 		.cpu_name		= "POWER8 (raw)",
 		.cpu_features		= CPU_FTRS_POWER8,
 		.cpu_user_features	= COMMON_USER_POWER8,
+		.cpu_user_features2	= COMMON_USER2_POWER8,
 		.mmu_features		= MMU_FTRS_POWER8,
 		.icache_bsize		= 128,
 		.dcache_bsize		= 128,