Patchwork [4/11] Add platform_has_feature()

login
register
mail settings
Submitter Nathan Fontenot
Date March 9, 2013, 4:02 a.m.
Message ID <513AB457.9000409@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/226301/
State Superseded, archived
Headers show

Comments

Nathan Fontenot - March 9, 2013, 4:02 a.m.
The firmware_has_feature() function makes it easy to check for supported
features of the hardware. There is not corresponding function to check for
features supported by the client architecture.

This patch adds a platform_has_feature() function to check features selected
by firmware and reported via the device tree 'ibm,architecture-vec5'
property. As part of this the #defines used for the architecture vector are
moved to prom.h and re-defined such that the vector 5 options have the vector
index and the feature bits encoded into them. This allows for callers of
platform_has_feature() to pass in a single pre-defined value.

Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/prom.h |   41 +++++++++++++++++++++++-----------------
 arch/powerpc/kernel/prom.c      |   19 ++++++++++++++++++
 arch/powerpc/kernel/prom_init.c |   14 +++++++------
 3 files changed, 51 insertions(+), 23 deletions(-)
Paul Mackerras - March 14, 2013, 8:56 a.m.
On Fri, Mar 08, 2013 at 10:02:31PM -0600, Nathan Fontenot wrote:
> The firmware_has_feature() function makes it easy to check for supported
> features of the hardware. There is not corresponding function to check for
> features supported by the client architecture.

Actually, firmware_has_feature checks for supported features of the
hypervisor, or in a sense the platform, rather than hardware.

> This patch adds a platform_has_feature() function to check features selected
> by firmware and reported via the device tree 'ibm,architecture-vec5'
> property. As part of this the #defines used for the architecture vector are
> moved to prom.h and re-defined such that the vector 5 options have the vector
> index and the feature bits encoded into them. This allows for callers of
> platform_has_feature() to pass in a single pre-defined value.

One other comment below...

>  /* PCIe/MSI support.  Without MSI full PCIe is not supported */
>  #ifdef CONFIG_PCI_MSI
> -#define OV5_MSI			0x01	/* PCIe/MSI support */
> +#define OV5_MSI			0x0201	/* PCIe/MSI support */
>  #else
> -#define OV5_MSI			0x00
> +#define OV5_MSI			0x0200
>  #endif /* CONFIG_PCI_MSI */

The #ifdef was done this way in order to control what ended up in the
option vector we pass to the platform firmware.  For checking what the
platform supports, wouldn't we want OV5_MSI to be 0x0201 always?
Similarly for OV5_CMO, OV5_XCMO, etc.?

Paul.
Paul Mackerras - March 14, 2013, 8:59 a.m.
On Fri, Mar 08, 2013 at 10:02:31PM -0600, Nathan Fontenot wrote:
> This patch adds a platform_has_feature() function to check features selected
> by firmware and reported via the device tree 'ibm,architecture-vec5'
> property. As part of this the #defines used for the architecture vector are
> moved to prom.h and re-defined such that the vector 5 options have the vector
> index and the feature bits encoded into them. This allows for callers of
> platform_has_feature() to pass in a single pre-defined value.

One other comment...

> +#if defined(CONFIG_PPC_PSERIES) || defined(CONFIG_PPC_POWERNV)
> +bool platform_has_feature(unsigned int feature)
> +{
> +	struct device_node *chosen;
> +	const char *vec5;
> +	bool has_option;
> +
> +	chosen = of_find_node_by_path("/chosen");
> +	if (!chosen)
> +		return false;
> +
> +	vec5 = of_get_property(chosen, "ibm,architecture-vec-5", NULL);
> +	has_option = vec5 && (vec5[OV5_INDX(feature)] & OV5_FEAT(feature));

You access vec5[index] without checking that the vector is at least
index+1 bytes long, according to either the length byte at the
beginning of the vector, or the total length of the property.
Checking both would be a good idea.

Paul.
Michael Ellerman - March 14, 2013, 1:42 p.m.
On Fri, Mar 08, 2013 at 10:02:31PM -0600, Nathan Fontenot wrote:
> The firmware_has_feature() function makes it easy to check for supported
> features of the hardware. There is not corresponding function to check for
> features supported by the client architecture.

Actually it doesn't tell you about features of the hardware, it tells
you about features of the firmware, or the platform ..

So I think you should really just be adding a new firmware feature flag,
and adding whatever glue code is required to set it based on what you
find in the device tree.

Also notice where you end up using it:

-       if (firmware_has_feature(FW_FEATURE_OPAL))
+       if (firmware_has_feature(FW_FEATURE_OPAL) ||
+           platform_has_feature(OV5_TYPE1_AFFINITY)) {
+               dbg("Using form 1 affinity\n");
		form1_affinity = 1;

Could be:

+       if (firmware_has_feature(FW_FEATURE_FORM1_AFFINITY) ||

cheers
Nathan Fontenot - March 19, 2013, 6:03 p.m.
On 03/14/2013 03:56 AM, Paul Mackerras wrote:
> On Fri, Mar 08, 2013 at 10:02:31PM -0600, Nathan Fontenot wrote:
>> The firmware_has_feature() function makes it easy to check for supported
>> features of the hardware. There is not corresponding function to check for
>> features supported by the client architecture.
> 
> Actually, firmware_has_feature checks for supported features of the
> hypervisor, or in a sense the platform, rather than hardware.

Ahh, thanks for clarifying that for me. I'll update the description.

> 
>> This patch adds a platform_has_feature() function to check features selected
>> by firmware and reported via the device tree 'ibm,architecture-vec5'
>> property. As part of this the #defines used for the architecture vector are
>> moved to prom.h and re-defined such that the vector 5 options have the vector
>> index and the feature bits encoded into them. This allows for callers of
>> platform_has_feature() to pass in a single pre-defined value.
> 
> One other comment below...
> 
>>  /* PCIe/MSI support.  Without MSI full PCIe is not supported */
>>  #ifdef CONFIG_PCI_MSI
>> -#define OV5_MSI			0x01	/* PCIe/MSI support */
>> +#define OV5_MSI			0x0201	/* PCIe/MSI support */
>>  #else
>> -#define OV5_MSI			0x00
>> +#define OV5_MSI			0x0200
>>  #endif /* CONFIG_PCI_MSI */
> 
> The #ifdef was done this way in order to control what ended up in the
> option vector we pass to the platform firmware.  For checking what the
> platform supports, wouldn't we want OV5_MSI to be 0x0201 always?
> Similarly for OV5_CMO, OV5_XCMO, etc.?

Yes, you're correct. I will update this.
Nathan Fontenot - March 19, 2013, 6:05 p.m.
On 03/14/2013 03:59 AM, Paul Mackerras wrote:
> On Fri, Mar 08, 2013 at 10:02:31PM -0600, Nathan Fontenot wrote:
>> This patch adds a platform_has_feature() function to check features selected
>> by firmware and reported via the device tree 'ibm,architecture-vec5'
>> property. As part of this the #defines used for the architecture vector are
>> moved to prom.h and re-defined such that the vector 5 options have the vector
>> index and the feature bits encoded into them. This allows for callers of
>> platform_has_feature() to pass in a single pre-defined value.
> 
> One other comment...
> 
>> +#if defined(CONFIG_PPC_PSERIES) || defined(CONFIG_PPC_POWERNV)
>> +bool platform_has_feature(unsigned int feature)
>> +{
>> +	struct device_node *chosen;
>> +	const char *vec5;
>> +	bool has_option;
>> +
>> +	chosen = of_find_node_by_path("/chosen");
>> +	if (!chosen)
>> +		return false;
>> +
>> +	vec5 = of_get_property(chosen, "ibm,architecture-vec-5", NULL);
>> +	has_option = vec5 && (vec5[OV5_INDX(feature)] & OV5_FEAT(feature));
> 
> You access vec5[index] without checking that the vector is at least
> index+1 bytes long, according to either the length byte at the
> beginning of the vector, or the total length of the property.
> Checking both would be a good idea.
> 

Yep. Thanks for letting me know.
Nathan Fontenot - March 19, 2013, 6:15 p.m.
On 03/14/2013 08:42 AM, Michael Ellerman wrote:
> On Fri, Mar 08, 2013 at 10:02:31PM -0600, Nathan Fontenot wrote:
>> The firmware_has_feature() function makes it easy to check for supported
>> features of the hardware. There is not corresponding function to check for
>> features supported by the client architecture.
> 
> Actually it doesn't tell you about features of the hardware, it tells
> you about features of the firmware, or the platform ..
> 
> So I think you should really just be adding a new firmware feature flag,
> and adding whatever glue code is required to set it based on what you
> find in the device tree.
> 
> Also notice where you end up using it:
> 
> -       if (firmware_has_feature(FW_FEATURE_OPAL))
> +       if (firmware_has_feature(FW_FEATURE_OPAL) ||
> +           platform_has_feature(OV5_TYPE1_AFFINITY)) {
> +               dbg("Using form 1 affinity\n");
> 		form1_affinity = 1;
> 
> Could be:
> 
> +       if (firmware_has_feature(FW_FEATURE_FORM1_AFFINITY) ||
> 

To make sure I understand what you're suggesting...

You think there should be a single firmware_has_feature() for all current
uses and also for checking items such as FORM1_AFFINITY and PRRN 
features as reported by the device tree for vector 5 portions of the
client architecture bits. I think this could be done by checking the
device tree ibm,architecture-vec-5 node for a specified feature and
setting a bit the appropriate bit in powerpc_firmware_features.

I like this more than separate firmware_has_feature() and platform_has_feature()
routines to check.
Michael Ellerman - March 22, 2013, 3:56 a.m.
On Tue, Mar 19, 2013 at 01:15:02PM -0500, Nathan Fontenot wrote:
> On 03/14/2013 08:42 AM, Michael Ellerman wrote:
> > On Fri, Mar 08, 2013 at 10:02:31PM -0600, Nathan Fontenot wrote:
> >> The firmware_has_feature() function makes it easy to check for supported
> >> features of the hardware. There is not corresponding function to check for
> >> features supported by the client architecture.
> > 
> > Actually it doesn't tell you about features of the hardware, it tells
> > you about features of the firmware, or the platform ..
> > 
> > So I think you should really just be adding a new firmware feature flag,
> > and adding whatever glue code is required to set it based on what you
> > find in the device tree.
> > 
> > Also notice where you end up using it:
> > 
> > -       if (firmware_has_feature(FW_FEATURE_OPAL))
> > +       if (firmware_has_feature(FW_FEATURE_OPAL) ||
> > +           platform_has_feature(OV5_TYPE1_AFFINITY)) {
> > +               dbg("Using form 1 affinity\n");
> > 		form1_affinity = 1;
> > 
> > Could be:
> > 
> > +       if (firmware_has_feature(FW_FEATURE_FORM1_AFFINITY) ||
> > 
> 
> To make sure I understand what you're suggesting...
> 
> You think there should be a single firmware_has_feature() for all current
> uses and also for checking items such as FORM1_AFFINITY and PRRN 
> features as reported by the device tree for vector 5 portions of the
> client architecture bits. I think this could be done by checking the
> device tree ibm,architecture-vec-5 node for a specified feature and
> setting a bit the appropriate bit in powerpc_firmware_features.

Yes that's right.

So you'd add a new FW_FEATURE_FORM1_AFFINITY, and set it depending on what
you find in "ibm,architecture-vec-5" on IBM pseries machines.

As a cleanup you'd also set it unconditionally on OPAL and then the
check above could be simply for FW_FEATURE_FORM1_AFFINITY.

cheers

Patch

Index: powerpc/arch/powerpc/include/asm/prom.h
===================================================================
--- powerpc.orig/arch/powerpc/include/asm/prom.h	2013-03-08 19:57:05.000000000 -0600
+++ powerpc/arch/powerpc/include/asm/prom.h	2013-03-08 19:57:14.000000000 -0600
@@ -111,31 +111,37 @@ 
 /* Option vector 4: IBM PAPR implementation */
 #define OV4_MIN_ENT_CAP		0x01	/* minimum VP entitled capacity */
 
-/* Option vector 5: PAPR/OF options supported */
-#define OV5_LPAR		0x80	/* logical partitioning supported */
-#define OV5_SPLPAR		0x40	/* shared-processor LPAR supported */
+/* Option vector 5: PAPR/OF options supported
+ * These bits are also used for the platform_has_feature() call so
+ * we encode the vector index in the define and use the OV5_FEAT()
+ * and OV5_INDX() macros to extract the desired information.
+ */
+#define OV5_FEAT(x)	((x) & 0xff)
+#define OV5_INDX(x)	((x) >> 8)
+#define OV5_LPAR		0x0280	/* logical partitioning supported */
+#define OV5_SPLPAR		0x0240	/* shared-processor LPAR supported */
 /* ibm,dynamic-reconfiguration-memory property supported */
-#define OV5_DRCONF_MEMORY	0x20
-#define OV5_LARGE_PAGES		0x10	/* large pages supported */
-#define OV5_DONATE_DEDICATE_CPU	0x02	/* donate dedicated CPU support */
+#define OV5_DRCONF_MEMORY	0x0220
+#define OV5_LARGE_PAGES		0x0210	/* large pages supported */
+#define OV5_DONATE_DEDICATE_CPU	0x0202	/* donate dedicated CPU support */
 /* PCIe/MSI support.  Without MSI full PCIe is not supported */
 #ifdef CONFIG_PCI_MSI
-#define OV5_MSI			0x01	/* PCIe/MSI support */
+#define OV5_MSI			0x0201	/* PCIe/MSI support */
 #else
-#define OV5_MSI			0x00
+#define OV5_MSI			0x0200
 #endif /* CONFIG_PCI_MSI */
 #ifdef CONFIG_PPC_SMLPAR
-#define OV5_CMO			0x80	/* Cooperative Memory Overcommitment */
-#define OV5_XCMO		0x40	/* Page Coalescing */
+#define OV5_CMO			0x0480	/* Cooperative Memory Overcommitment */
+#define OV5_XCMO		0x0440	/* Page Coalescing */
 #else
-#define OV5_CMO			0x00
-#define OV5_XCMO		0x00
+#define OV5_CMO			0x0400
+#define OV5_XCMO		0x0400
 #endif
-#define OV5_TYPE1_AFFINITY	0x80	/* Type 1 NUMA affinity */
-#define OV5_PFO_HW_RNG		0x80	/* PFO Random Number Generator */
-#define OV5_PFO_HW_842		0x40	/* PFO Compression Accelerator */
-#define OV5_PFO_HW_ENCR		0x20	/* PFO Encryption Accelerator */
-#define OV5_SUB_PROCESSORS	0x01	/* 1,2,or 4 Sub-Processors supported */
+#define OV5_TYPE1_AFFINITY	0x0580	/* Type 1 NUMA affinity */
+#define OV5_PFO_HW_RNG		0x0E80	/* PFO Random Number Generator */
+#define OV5_PFO_HW_842		0x0E40	/* PFO Compression Accelerator */
+#define OV5_PFO_HW_ENCR		0x0E20	/* PFO Encryption Accelerator */
+#define OV5_SUB_PROCESSORS	0x0F01	/* 1,2,or 4 Sub-Processors supported */
 
 /* Option Vector 6: IBM PAPR hints */
 #define OV6_LINUX		0x02	/* Linux is our OS */
@@ -145,6 +151,7 @@ 
  * followed by # option vectors - 1, followed by the option vectors.
  */
 extern unsigned char ibm_architecture_vec[];
+bool platform_has_feature(unsigned int);
 #endif
 
 /* These includes are put at the bottom because they may contain things
Index: powerpc/arch/powerpc/kernel/prom_init.c
===================================================================
--- powerpc.orig/arch/powerpc/kernel/prom_init.c	2013-03-08 19:57:05.000000000 -0600
+++ powerpc/arch/powerpc/kernel/prom_init.c	2013-03-08 19:57:14.000000000 -0600
@@ -684,11 +684,12 @@ 
 	/* option vector 5: PAPR/OF options */
 	19 - 2,				/* length */
 	0,				/* don't ignore, don't halt */
-	OV5_LPAR | OV5_SPLPAR | OV5_LARGE_PAGES | OV5_DRCONF_MEMORY |
-	OV5_DONATE_DEDICATE_CPU | OV5_MSI,
+	OV5_FEAT(OV5_LPAR) | OV5_FEAT(OV5_SPLPAR) | OV5_FEAT(OV5_LARGE_PAGES) |
+	OV5_FEAT(OV5_DRCONF_MEMORY) | OV5_FEAT(OV5_DONATE_DEDICATE_CPU) |
+	OV5_FEAT(OV5_MSI),
 	0,
-	OV5_CMO | OV5_XCMO,
-	OV5_TYPE1_AFFINITY,
+	OV5_FEAT(OV5_CMO) | OV5_FEAT(OV5_XCMO),
+	OV5_FEAT(OV5_TYPE1_AFFINITY),
 	0,
 	0,
 	0,
@@ -702,8 +703,9 @@ 
 	0,
 	0,
 	0,
-	OV5_PFO_HW_RNG | OV5_PFO_HW_ENCR | OV5_PFO_HW_842,
-	OV5_SUB_PROCESSORS,
+	OV5_FEAT(OV5_PFO_HW_RNG) | OV5_FEAT(OV5_PFO_HW_ENCR) |
+	OV5_FEAT(OV5_PFO_HW_842),
+	OV5_FEAT(OV5_SUB_PROCESSORS),
 	/* option vector 6: IBM PAPR hints */
 	4 - 2,				/* length */
 	0,
Index: powerpc/arch/powerpc/kernel/prom.c
===================================================================
--- powerpc.orig/arch/powerpc/kernel/prom.c	2013-03-08 19:23:06.000000000 -0600
+++ powerpc/arch/powerpc/kernel/prom.c	2013-03-08 19:57:14.000000000 -0600
@@ -871,6 +871,25 @@ 
 }
 EXPORT_SYMBOL(of_get_cpu_node);
 
+#if defined(CONFIG_PPC_PSERIES) || defined(CONFIG_PPC_POWERNV)
+bool platform_has_feature(unsigned int feature)
+{
+	struct device_node *chosen;
+	const char *vec5;
+	bool has_option;
+
+	chosen = of_find_node_by_path("/chosen");
+	if (!chosen)
+		return false;
+
+	vec5 = of_get_property(chosen, "ibm,architecture-vec-5", NULL);
+	has_option = vec5 && (vec5[OV5_INDX(feature)] & OV5_FEAT(feature));
+	of_node_put(chosen);
+
+	return has_option;
+}
+#endif
+
 #if defined(CONFIG_DEBUG_FS) && defined(DEBUG)
 static struct debugfs_blob_wrapper flat_dt_blob;