| Message ID | 513AB457.9000409@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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.
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.
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
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.
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.
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.
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
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;
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(-)