| Message ID | 20130503231933.GA29436@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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.
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
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, >
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
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
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
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
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.
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
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,
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_*)