[RFC , PATCH] support for the ibm,pa_features cpu property

[RFC , PATCH] support for the ibm,pa_features cpu property

[Will Schmidt]

To determine if our processors support some features, such as large
pages, we should be using the ibm,pa_features property, rather than just
the PVR values.  
This is an initial pass at the functionality.  This has been tested in
the case where the property is missing, but still needs to be tested
against a system where the property actually exists.  :-o  



diff --git a/arch/powerpc/kernel/setup_64.c
b/arch/powerpc/kernel/setup_64.c
index 13e91c4..78ad054 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -106,6 +106,65 @@ static struct notifier_block ppc64_panic
 	.priority = INT_MIN /* may not return; must be done last */
 };
 
+/*
+ * ibm,pa-features is a per-cpu property that contains a 2 byte header
+ * plus up to 256 bytes worth of processor attributes.  First header
+ * byte specifies the number of bytes implemented by the platform.
+ * Second header byte is an "attribute-specifier" type, which should
+ * be zero.  Remainder of the data consists of ones and zeros.
+ * Implementation:  Pass in the byte and bit offset for the feature
+ * that we are interested in.  The function will return -1 if the
+ * pa-features property is missing, or a 1/0 to indicate if the feature
+ * is supported/not supported.
+ */
+
+static int get_pa_features(int pabyte,int pabit)
+{
+	struct device_node *cpu;
+	char *pa_feature_table;
+
+	cpu = of_find_node_by_type(NULL, "cpu");
+	pa_feature_table = 
+		(char *)get_property(cpu, "ibm,pa-features", NULL);
+
+	if ( pa_feature_table == NULL ) {
+		printk("ibm,pa-features property is missing.\n");
+		return -1;
+	}
+
+	/* sanity check */
+	if ( pabyte > pa_feature_table[0] ) {
+		printk("%s: %d out of range for table of size %d\n",
+			__FUNCTION__,pabyte,pa_feature_table[0]);
+		return -1;
+	}
+	
+	return pa_feature_table[2+pabyte*8+pabit];
+}
+
+/*
+ * set values within the cur_cpu_spec table according to
+ * the ibm,pa_features property. 
+ * potential entries include: 
+ * Byte 0, bit 1 - FPU available
+ * Byte 1, bit 2 - Large Pages 
+ * Byte 2, bit 3 - DAR set on alignment Interrupt. 
+ */
+static void add_cpu_features()
+{
+	/* if no property, bail early */
+	if (get_pa_features(0,0) == -1 ) return;
+
+	if (get_pa_features(1,2) ) {
+		printk("Adding CI_LARGE_PAGE to cur_cpu_spec \n");
+		cur_cpu_spec->cpu_features |= CPU_FTR_CI_LARGE_PAGE;
+	}
+
+	/* add more here... */
+
+}
+
+
 #ifdef CONFIG_SMP
 
 static int smt_enabled_cmdline;
@@ -425,6 +484,8 @@ void __init setup_system(void)
 
 	parse_early_param();
 
+	add_cpu_features();
+
 	check_smt_enabled();
 	smp_setup_cpu_maps();
 

Re: [RFC , PATCH] support for the ibm,pa_features cpu property

[Olof Johansson]

Hi Will,

Some general comments below. Not having seen the documentation for the
new property I obviously can't comment on the correctness from that
perspective.

Do you know why they went for this brand new extra architected bitfield
instead of continuing down the way that processor features always have
been documented before, by adding a property to the cpu device node?

That seems like a much more logical solution to me. It sort of sounds
like someone was bored one day and decided to get busy with architecting
yet another way to specify processor capabilities. :(

(Now, the naming convention of calling it a "pa feature" is unfortunate,
but nothing I can really complain about since our stuff is not yet in
tree.)


-Olof

On Fri, Apr 28, 2006 at 01:41:24PM -0500, Will Schmidt wrote:
> 
> To determine if our processors support some features, such as large
> pages, we should be using the ibm,pa_features property, rather than just
> the PVR values.  
> This is an initial pass at the functionality.  This has been tested in
> the case where the property is missing, but still needs to be tested
> against a system where the property actually exists.  :-o  
> 
> 
> 
> diff --git a/arch/powerpc/kernel/setup_64.c
> b/arch/powerpc/kernel/setup_64.c
> index 13e91c4..78ad054 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -106,6 +106,65 @@ static struct notifier_block ppc64_panic
>  	.priority = INT_MIN /* may not return; must be done last */
>  };
>  
> +/*
> + * ibm,pa-features is a per-cpu property that contains a 2 byte header
> + * plus up to 256 bytes worth of processor attributes.  First header
> + * byte specifies the number of bytes implemented by the platform.
> + * Second header byte is an "attribute-specifier" type, which should
> + * be zero.  Remainder of the data consists of ones and zeros.

So, essentially a bit array with one bit per feature with a 2-byte
header?

> + * Implementation:  Pass in the byte and bit offset for the feature
> + * that we are interested in.  The function will return -1 if the
> + * pa-features property is missing, or a 1/0 to indicate if the feature
> + * is supported/not supported.
> + */
> +
> +static int get_pa_features(int pabyte,int pabit)

So this is more of a check_pa_feature (int pabyte, int pabit)

(note space after comma, CodingStyle)

> +{
> +	struct device_node *cpu;
> +	char *pa_feature_table;
> +
> +	cpu = of_find_node_by_type(NULL, "cpu");
> +	pa_feature_table = 
> +		(char *)get_property(cpu, "ibm,pa-features", NULL);
> +	if ( pa_feature_table == NULL ) {

No spaces after ( and before )

> +		printk("ibm,pa-features property is missing.\n");
> +		return -1;
> +	}

This will read the property on every call. How about making the table
pointer static instead and keeping it cached?

> +
> +	/* sanity check */
> +	if ( pabyte > pa_feature_table[0] ) {
> +		printk("%s: %d out of range for table of size %d\n",
> +			__FUNCTION__,pabyte,pa_feature_table[0]);

This might pop on regular machines, so you might want to do away with
the printk by default, or add it under a DBG() macro that's not enabled
by default (see other powerpc files for examples)

> +		return -1;
> +	}
> +	
> +	return pa_feature_table[2+pabyte*8+pabit];
> +}

Not knowing the order of the bitfield, I'm guessing you might want this
instead:

	return pa_feature_table[2+pabyte] & (0x80 >> pabit);

I would prefer to see a 0/1 return. 1 if the feature is set, 0
otherwise. I.e. no -1 on failure.

> +/*
> + * set values within the cur_cpu_spec table according to
> + * the ibm,pa_features property. 
> + * potential entries include: 
> + * Byte 0, bit 1 - FPU available
> + * Byte 1, bit 2 - Large Pages 
> + * Byte 2, bit 3 - DAR set on alignment Interrupt. 

Wow, that's a really sparse array, 8 empty entries between each
allocated bit so far.

I would like to see these as #defines as well. It might make more sense
to have them as just bit number, and calculate the byte in the function
above instead. That way it's only one define per feature.

> + */
> +static void add_cpu_features()
> +{
> +	/* if no property, bail early */
> +	if (get_pa_features(0,0) == -1 ) return;

Probably no need to bail early, especially if the feature check function
just returns 0 if it's out-of-bounds (or no property exists)

> +
> +	if (get_pa_features(1,2) ) {
> +		printk("Adding CI_LARGE_PAGE to cur_cpu_spec \n");
> +		cur_cpu_spec->cpu_features |= CPU_FTR_CI_LARGE_PAGE;
> +	}
> +
> +	/* add more here... */
> +
> +}
> +
> +
>  #ifdef CONFIG_SMP
>  
>  static int smt_enabled_cmdline;
> @@ -425,6 +484,8 @@ void __init setup_system(void)
>  
>  	parse_early_param();
>  
> +	add_cpu_features();
> +
>  	check_smt_enabled();
>  	smp_setup_cpu_maps();
>  
> 
> 
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [RFC , PATCH] support for the ibm,pa_features cpu property

[Nathan Lynch]

Will Schmidt wrote:
> +static int get_pa_features(int pabyte,int pabit)
> +{
> +	struct device_node *cpu;
> +	char *pa_feature_table;
> +
> +	cpu = of_find_node_by_type(NULL, "cpu");
> +	pa_feature_table = 
> +		(char *)get_property(cpu, "ibm,pa-features", NULL);
> +
> +	if ( pa_feature_table == NULL ) {
> +		printk("ibm,pa-features property is missing.\n");
> +		return -1;
> +	}
> +
> +	/* sanity check */
> +	if ( pabyte > pa_feature_table[0] ) {
> +		printk("%s: %d out of range for table of size %d\n",
> +			__FUNCTION__,pabyte,pa_feature_table[0]);
> +		return -1;
> +	}
> +	
> +	return pa_feature_table[2+pabyte*8+pabit];
> +}

This function needs to of_node_put(cpu) before returning or we won't
be able to deconfigure the processor.

Re: [RFC , PATCH] support for the ibm,pa_features cpu property

[Will Schmidt]

Whoops.. funny thing occurred to me on my drive home..   comment inline

On Fri, 2006-04-28 at 13:41 -0500, Will Schmidt wrote:
> To determine if our processors support some features, such as large
> pages, we should be using the ibm,pa_features property, rather than just
> the PVR values.  
> This is an initial pass at the functionality.  This has been tested in
> the case where the property is missing, but still needs to be tested
> against a system where the property actually exists.  :-o  
> 
> 
> 
> diff --git a/arch/powerpc/kernel/setup_64.c
> b/arch/powerpc/kernel/setup_64.c
> index 13e91c4..78ad054 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -106,6 +106,65 @@ static struct notifier_block ppc64_panic
>  	.priority = INT_MIN /* may not return; must be done last */
>  };
> 
> +/*
> + * ibm,pa-features is a per-cpu property that contains a 2 byte header
> + * plus up to 256 bytes worth of processor attributes.  First header
> + * byte specifies the number of bytes implemented by the platform.
> + * Second header byte is an "attribute-specifier" type, which should
> + * be zero.  Remainder of the data consists of ones and zeros.
> + * Implementation:  Pass in the byte and bit offset for the feature
> + * that we are interested in.  The function will return -1 if the
> + * pa-features property is missing, or a 1/0 to indicate if the feature
> + * is supported/not supported.
> + */
> +
> +static int get_pa_features(int pabyte,int pabit)
> +{
> +	struct device_node *cpu;
> +	char *pa_feature_table;
> +
> +	cpu = of_find_node_by_type(NULL, "cpu");
> +	pa_feature_table = 
> +		(char *)get_property(cpu, "ibm,pa-features", NULL);
> +
> +	if ( pa_feature_table == NULL ) {
> +		printk("ibm,pa-features property is missing.\n");
> +		return -1;
> +	}
> +
> +	/* sanity check */
> +	if ( pabyte > pa_feature_table[0] ) {
> +		printk("%s: %d out of range for table of size %d\n",
> +			__FUNCTION__,pabyte,pa_feature_table[0]);
> +		return -1;
> +	}
> +	
> +	return pa_feature_table[2+pabyte*8+pabit];

pa_feature_table would be byte (char) addressable, while the fields that
we are interested with inside are actually bit values.  So this should
be something like

	return   (pa_feature_table[2+pabyte] & 1<<pabit);


> +}
> +
> +/*
> + * set values within the cur_cpu_spec table according to
> + * the ibm,pa_features property. 
> + * potential entries include: 
> + * Byte 0, bit 1 - FPU available
> + * Byte 1, bit 2 - Large Pages 
> + * Byte 2, bit 3 - DAR set on alignment Interrupt. 
> + */
> +static void add_cpu_features()
> +{
> +	/* if no property, bail early */
> +	if (get_pa_features(0,0) == -1 ) return;
> +
> +	if (get_pa_features(1,2) ) {
> +		printk("Adding CI_LARGE_PAGE to cur_cpu_spec \n");
> +		cur_cpu_spec->cpu_features |= CPU_FTR_CI_LARGE_PAGE;
> +	}
> +
> +	/* add more here... */
> +
> +}
> +
> +
>  #ifdef CONFIG_SMP
> 
>  static int smt_enabled_cmdline;
> @@ -425,6 +484,8 @@ void __init setup_system(void)
> 
>  	parse_early_param();
> 
> +	add_cpu_features();
> +
>  	check_smt_enabled();
>  	smp_setup_cpu_maps();
> 
> 
> 
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [RFC , PATCH] support for the ibm,pa_features cpu property

[Paul Mackerras]

Olof Johansson writes:

> Do you know why they went for this brand new extra architected bitfield
> instead of continuing down the way that processor features always have
> been documented before, by adding a property to the cpu device node?

They wanted to cover basically everything that we have CPU feature
bits for, plus some other things.  That would have been a lot of new
properties, so they went for one that had a bitmap in it, and made it
extensible in two different directions for good measure while they
were at it. :)

> (Now, the naming convention of calling it a "pa feature" is unfortunate,
> but nothing I can really complain about since our stuff is not yet in
> tree.)

The "pa" is just "processor architecture", nothing to do with your
employer. :)

Paul.

Re: [RFC , PATCH] support for the ibm,pa_features cpu property

[Paul Mackerras]

Will Schmidt writes:

> This is an initial pass at the functionality.  This has been tested in
> the case where the property is missing, but still needs to be tested
> against a system where the property actually exists.  :-o  

Some random comments, and a modified version of your patch...

- We may want to make a table giving the correspondence between
  pa-features byte/bit numbers and our feature bits, and then just
  have code that walks through the table and sets/clears feature bits
  as appropriate.

- If we are setting/clearing feature bits that are used with the asm
  feature macros, we'll have to do this much earlier, in prom.c,
  before the device tree is unflattened.

Paul.

diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 4467c49..022b9b3 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -106,6 +106,78 @@ static struct notifier_block ppc64_panic
 	.priority = INT_MIN /* may not return; must be done last */
 };
 
+/*
+ * ibm,pa-features is a per-cpu property that contains a string of
+ * attribute descriptors, each of which has a 2 byte header plus up
+ * to 254 bytes worth of processor attribute bits.  First header
+ * byte specifies the number of bytes following the header.
+ * Second header byte is an "attribute-specifier" type, of which
+ * zero is the only currently-defined value.
+ * Implementation:  Pass in the byte and bit offset for the feature
+ * that we are interested in.  The function will return -1 if the
+ * pa-features property is missing, or a 1/0 to indicate if the feature
+ * is supported/not supported.  Note that the bit numbers are
+ * big-endian to match the definition in PAPR.
+ */
+static int get_pa_feature(int pabyte, int pabit)
+{
+	struct device_node *cpu;
+	unsigned char *pa_feature_table;
+	int tablelen;
+	int ret = -1;
+
+	cpu = of_find_node_by_type(NULL, "cpu");
+	if (cpu == NULL)
+		return -1;
+
+	pa_feature_table = get_property(cpu, "ibm,pa-features", &tablelen);
+	if (pa_feature_table == NULL)
+		goto out;
+
+	/* find descriptor with type == 0 */
+	for (;;) {
+		if (tablelen < 3 || tablelen < 2 + pa_feature_table[1])
+			goto out;	/* not found */
+		if (pa_feature_table[0] == 0)
+			break;
+		tablelen -= 2 + pa_feature_table[1];
+		pa_feature_table += 2 + pa_feature_table[1];
+	}
+
+	/* check if the specifier is long enough */
+	if (pabyte >= pa_feature_table[1])
+		goto out;
+
+	ret = (pa_feature_table[2 + pabyte] >> (7 - pabit)) & 1;
+
+ out:
+	of_node_put(cpu);
+	return ret;
+}
+
+/*
+ * set values within the cur_cpu_spec table according to
+ * the ibm,pa_features property. 
+ * potential entries include: 
+ * Byte 0, bit 1 - FPU available
+ * Byte 1, bit 2 - cache-inhibited large pages supported
+ * Byte 2, bit 3 - DAR set on alignment interrupt. 
+ */
+static void check_cpu_features(void)
+{
+	int hasit;
+
+	/* test for support of cache-inhibited large pages */
+	hasit = get_pa_feature(1, 2);
+	if (hasit >= 0)
+		if (hasit)
+			cur_cpu_spec->cpu_features |= CPU_FTR_CI_LARGE_PAGE;
+		else
+			cur_cpu_spec->cpu_features &= ~CPU_FTR_CI_LARGE_PAGE;
+
+	/* add more here in future... */
+}
+
 #ifdef CONFIG_SMP
 
 static int smt_enabled_cmdline;
@@ -425,6 +497,8 @@ #endif
 
 	parse_early_param();
 
+	check_cpu_features();
+
 	check_smt_enabled();
 	smp_setup_cpu_maps();
 

Re: [RFC , PATCH] support for the ibm,pa_features cpu property

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

Hi Paul,

On Sat, 29 Apr 2006 12:20:42 +1000 Paul Mackerras <paulus@samba.org> wrote:
>
> +	/* find descriptor with type == 0 */
> +	for (;;) {
> +		if (tablelen < 3 || tablelen < 2 + pa_feature_table[1])
                                                                    ^
Shouldn't that be 0?

> +			goto out;	/* not found */
> +		if (pa_feature_table[0] == 0)
                                     ^
And this 1?

> +			break;
> +		tablelen -= 2 + pa_feature_table[1];
                                                 ^
0

> +		pa_feature_table += 2 + pa_feature_table[1];
                                                         ^
0

> +	}
> +
> +	/* check if the specifier is long enough */
> +	if (pabyte >= pa_feature_table[1])
                                       ^
0

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC , PATCH] support for the ibm,pa_features cpu property

[Olof Johansson]

On Sat, Apr 29, 2006 at 11:57:32AM +1000, Paul Mackerras wrote:
> Olof Johansson writes:
> 
> > Do you know why they went for this brand new extra architected bitfield
> > instead of continuing down the way that processor features always have
> > been documented before, by adding a property to the cpu device node?
> 
> They wanted to cover basically everything that we have CPU feature
> bits for, plus some other things.  That would have been a lot of new
> properties, so they went for one that had a bitmap in it, and made it
> extensible in two different directions for good measure while they
> were at it. :)

Sure, it might be a few, but we already have a handful. And having a
bitfield only gives you a chance to say here or not, no version numbers,
capacities, etc.

Anyway, I can't really debate it since I haven't seen the spec, just one
implementation.

> > (Now, the naming convention of calling it a "pa feature" is unfortunate,
> > but nothing I can really complain about since our stuff is not yet in
> > tree.)
> 
> The "pa" is just "processor architecture", nothing to do with your
> employer. :)

Yes, I know. Just saying. :)


-Olof