Re: [PATCH v3 5/12] Update firmware_has_feature() to check architecture bits
On 04/22/2013 08:50 PM, Stephen Rothwell wrote:
Hi Nathan,
On Mon, 22 Apr 2013 13:38:47 -0500 Nathan Fontenot nf...@linux.vnet.ibm.com
wrote:
-/* Option vector 5: PAPR/OF options supported */
-#define OV5_LPAR0x80/* logical partitioning supported */
-#define OV5_SPLPAR 0x40/* shared-processor LPAR supported */
+/* Option vector 5: PAPR/OF options supported
+ * Thses bits are also used for the platform_has_feature() call so
^
typo
will fix.
+ * 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_LPAR0x0280 /* logical partitioning supported */
+#define OV5_SPLPAR 0x0240 /* shared-processor LPAR supported */
Wouldn't it be clearer to say
#define OV5_LPAR (OV5_INDX(0x2) | OV5_FEAT(0x80))
The defines won't work the way you used them, they were designed to take the
combined value, i.e. 0x0280, and parse out the index and the feature.
I do think having macros to create the actual values as your example does is
easier
to read. We could do something like...
#define OV5_FEAT(x) ((x) 0xff)
#define OV5_SETINDX(x) ((x) 8)
#define OV5_GETINDX(x) ((x) 8)
#define OV5_LPAR(OV5_SETINDX(0x2) | OV5_FEAT(0x80))
Thoughts?
etc?
@@ -145,6 +141,7 @@
* followed by # option vectors - 1, followed by the option vectors.
*/
extern unsigned char ibm_architecture_vec[];
+bool platform_has_feature(unsigned int);
extern, please (if nothing else, for consistency).
That shouldn't really be there, its an artifact from a previous patch. I'll
remove it.
+static __initdata struct vec5_fw_feature
+vec5_fw_features_table[FIRMWARE_MAX_FEATURES] = {
Why make this array FIRMWARE_MAX_FEATURES (63) long? You could just
restrict the for loop below to ARRAY_SIZE(vec5_fw_features_table).
+{FW_FEATURE_TYPE1_AFFINITY, OV5_TYPE1_AFFINITY},
+};
+
+void __init fw_vec5_feature_init(const char *vec5, unsigned long len)
+{
+unsigned int index, feat;
+int i;
+
+pr_debug( - fw_vec5_feature_init()\n);
+
+for (i = 0; i FIRMWARE_MAX_FEATURES; i++) {
+if (!vec5_fw_features_table[i].feature)
+continue;
And this test could go away.
I realise that you have just copied the existing code, but you should not
do that blindly. Maybe you could even add an (earlier) patch that fixes
the existing code.
I think that could be done easily enough.
Thanks for looking,
-Nathan
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v3 5/12] Update firmware_has_feature() to check architecture bits
The firmware_has_feature() function makes it easy to check for supported features of the hypervisor. This patch extends the capability of the firmware_has_feature() function to include checking for specified bits in vector 5 of the architecture vector as is reported in the device tree. As part of this the #defines used for the architecture vector are re-defined such that the vector 5 options have the vector index and the feature bits encoded into them. This makes for a much simpler design to update firmware_has_feature() to check for bits in the architecture vector. Signed-off-by: Nathan Fontenot nf...@linux.vnet.ibm.com --- arch/powerpc/include/asm/firmware.h |4 +- arch/powerpc/include/asm/prom.h | 45 --- arch/powerpc/kernel/prom_init.c | 23 ++ arch/powerpc/platforms/pseries/firmware.c | 49 +- arch/powerpc/platforms/pseries/pseries.h |5 ++- arch/powerpc/platforms/pseries/setup.c| 40 6 files changed, 113 insertions(+), 53 deletions(-) Index: powerpc/arch/powerpc/include/asm/prom.h === --- powerpc.orig/arch/powerpc/include/asm/prom.h2013-04-17 13:43:13.0 -0500 +++ powerpc/arch/powerpc/include/asm/prom.h 2013-04-17 13:51:46.0 -0500 @@ -111,31 +111,27 @@ /* Option vector 4: IBM PAPR implementation */ #define OV4_MIN_ENT_CAP0x01/* 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 + * Thses 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_PAGES0x10/* large pages supported */ -#define OV5_DONATE_DEDICATE_CPU0x02/* donate dedicated CPU support */ -/* PCIe/MSI support. Without MSI full PCIe is not supported */ -#ifdef CONFIG_PCI_MSI -#define OV5_MSI0x01/* PCIe/MSI support */ -#else -#define OV5_MSI0x00 -#endif /* CONFIG_PCI_MSI */ -#ifdef CONFIG_PPC_SMLPAR -#define OV5_CMO0x80/* Cooperative Memory Overcommitment */ -#define OV5_XCMO 0x40/* Page Coalescing */ -#else -#define OV5_CMO0x00 -#define OV5_XCMO 0x00 -#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_ENCR0x20/* PFO Encryption Accelerator */ -#define OV5_SUB_PROCESSORS 0x01/* 1,2,or 4 Sub-Processors supported */ +#define OV5_DRCONF_MEMORY 0x0220 +#define OV5_LARGE_PAGES0x0210 /* large pages supported */ +#define OV5_DONATE_DEDICATE_CPU0x0202 /* donate dedicated CPU support */ +#define OV5_MSI0x0201 /* PCIe/MSI support */ +#define OV5_CMO0x0480 /* Cooperative Memory Overcommitment */ +#define OV5_XCMO 0x0440 /* Page Coalescing */ +#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_ENCR0x0E20 /* 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 +141,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.c2013-04-17 13:43:13.0 -0500 +++ powerpc/arch/powerpc/kernel/prom_init.c 2013-04-17 13:51:46.0 -0500 @@ -684,11 +684,21 @@ /* option vector 5: PAPR/OF options */ 19 - 2, /* length */ 0, /*
Re: [PATCH v3 5/12] Update firmware_has_feature() to check architecture bits
Hi Nathan,
On Mon, 22 Apr 2013 13:38:47 -0500 Nathan Fontenot nf...@linux.vnet.ibm.com
wrote:
-/* 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
+ * Thses bits are also used for the platform_has_feature() call so
^
typo
+ * 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 */
Wouldn't it be clearer to say
#define OV5_LPAR(OV5_INDX(0x2) | OV5_FEAT(0x80))
etc?
@@ -145,6 +141,7 @@
* followed by # option vectors - 1, followed by the option vectors.
*/
extern unsigned char ibm_architecture_vec[];
+bool platform_has_feature(unsigned int);
extern, please (if nothing else, for consistency).
+static __initdata struct vec5_fw_feature
+vec5_fw_features_table[FIRMWARE_MAX_FEATURES] = {
Why make this array FIRMWARE_MAX_FEATURES (63) long? You could just
restrict the for loop below to ARRAY_SIZE(vec5_fw_features_table).
+ {FW_FEATURE_TYPE1_AFFINITY, OV5_TYPE1_AFFINITY},
+};
+
+void __init fw_vec5_feature_init(const char *vec5, unsigned long len)
+{
+ unsigned int index, feat;
+ int i;
+
+ pr_debug( - fw_vec5_feature_init()\n);
+
+ for (i = 0; i FIRMWARE_MAX_FEATURES; i++) {
+ if (!vec5_fw_features_table[i].feature)
+ continue;
And this test could go away.
I realise that you have just copied the existing code, but you should not
do that blindly. Maybe you could even add an (earlier) patch that fixes
the existing code.
--
Cheers,
Stephen Rothwells...@canb.auug.org.au
pgpwjjkEJ0Fy3.pgp
Description: PGP signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 5/12] Update firmware_has_feature() to check architecture bits
Hi Nathan, On Mon, 22 Apr 2013 13:38:47 -0500 Nathan Fontenot nf...@linux.vnet.ibm.com wrote: +/* Option vector 5: PAPR/OF options supported + * Thses bits are also used for the platform_has_feature() call so You talk about platform_has_feature(), but that does not exist (I assume it existed in a previous version of the patch set). +bool platform_has_feature(unsigned int); Ditto. -- Cheers, Stephen Rothwells...@canb.auug.org.au pgp5P_HIWmupA.pgp Description: PGP signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
