This patch adds a function, is_spar_system(), to check that s-Par
firmware is present, and then uses this function at the beginning of
each module to verify that the modules are being run on an s-Par system
before beginning initialization. If the firmware is not detected, the
module will return a failure code.
Checking for s-Par firmware uses the cpuid instruction to verify that
the processor is running with virtualization turned on, and then uses a
second cpuid instruction to check that the hypervisor ID is
"UnisysSpar64".
Additionally, some minor clean-up was done on copyright tags and
unnecessary messages were removed from the visorchipset_main() function.
This patch was tested with KVM to ensure that the modules do not load
when s-Par firmware is not present, and tested with s-Par 4.0.12 to
verify that the modules load correctly when the firmware is present.
Signed-off-by: Benjamin Romer <[email protected]>
---
diff --git a/drivers/staging/unisys/include/timskmodutils.h b/drivers/staging/unisys/include/timskmodutils.h
index 2d81d46..cc439d3 100644
--- a/drivers/staging/unisys/include/timskmodutils.h
+++ b/drivers/staging/unisys/include/timskmodutils.h
@@ -1,6 +1,6 @@
/* timskmodutils.h
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -71,5 +71,6 @@
u64 somethings, char *buf, size_t bufsize);
struct seq_file *visor_seq_file_new_buffer(void *buf, size_t buf_size);
void visor_seq_file_done_buffer(struct seq_file *m);
+int is_spar_system( void );
#endif
diff --git a/drivers/staging/unisys/uislib/uislib.c b/drivers/staging/unisys/uislib/uislib.c
index 8ea9c46..aa60ccb 100644
--- a/drivers/staging/unisys/uislib/uislib.c
+++ b/drivers/staging/unisys/uislib/uislib.c
@@ -1,6 +1,6 @@
/* uislib.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -2276,6 +2276,11 @@
static int __init
uislib_mod_init(void)
{
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( "s-Par not detected.\n" );
+ return -EPERM;
+ }
LOGINF("MONITORAPIS");
diff --git a/drivers/staging/unisys/virthba/virthba.c b/drivers/staging/unisys/virthba/virthba.c
index 817b11d..862509d 100644
--- a/drivers/staging/unisys/virthba/virthba.c
+++ b/drivers/staging/unisys/virthba/virthba.c
@@ -1,6 +1,6 @@
/* virthba.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -1691,6 +1691,12 @@
int error;
int i;
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( "s-Par not detected.\n" );
+ return -EPERM;
+ }
+
LOGINF("Entering virthba_mod_init...\n");
POSTCODE_LINUX_2(VHBA_CREATE_ENTRY_PC, POSTCODE_SEVERITY_INFO);
diff --git a/drivers/staging/unisys/virtpci/virtpci.c b/drivers/staging/unisys/virtpci/virtpci.c
index 8e34650..0d06f7e 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -1,6 +1,6 @@
/* virtpci.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -21,6 +21,7 @@
#ifdef CONFIG_MODVERSIONS
#include <config/modversions.h>
#endif
+#include "timskmodutils.h"
#include "uniklog.h"
#include "diagnostics/appos_subsystems.h"
#include "uisutils.h"
@@ -1686,6 +1687,11 @@
{
int ret;
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( "s-Par not detected.\n" );
+ return -EPERM;
+ }
LOGINF("Module build: Date:%s Time:%s...\n", __DATE__, __TIME__);
diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index 8252ca1..aa35aa2 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -1,6 +1,6 @@
/* visorchipset_main.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -2681,18 +2681,13 @@
struct proc_dir_entry *toolaction_file;
struct proc_dir_entry *bootToTool_file;
- LOGINF("chipset driver version %s loaded", VERSION);
- /* process module options */
- POSTCODE_LINUX_2(DRIVER_ENTRY_PC, POSTCODE_SEVERITY_INFO);
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( "s-Par not detected.\n" );
+ return -EPERM;
+ }
- LOGINF("option - testvnic=%d", visorchipset_testvnic);
- LOGINF("option - testvnicclient=%d", visorchipset_testvnicclient);
- LOGINF("option - testmsg=%d", visorchipset_testmsg);
- LOGINF("option - testteardown=%d", visorchipset_testteardown);
- LOGINF("option - major=%d", visorchipset_major);
- LOGINF("option - serverregwait=%d", visorchipset_serverregwait);
- LOGINF("option - clientregwait=%d", visorchipset_clientregwait);
- LOGINF("option - holdchipsetready=%d", visorchipset_holdchipsetready);
+ POSTCODE_LINUX_2(DRIVER_ENTRY_PC, POSTCODE_SEVERITY_INFO);
memset(&BusDev_Server_Notifiers, 0, sizeof(BusDev_Server_Notifiers));
memset(&BusDev_Client_Notifiers, 0, sizeof(BusDev_Client_Notifiers));
diff --git a/drivers/staging/unisys/visorutil/visorkmodutils.c b/drivers/staging/unisys/visorutil/visorkmodutils.c
index a7d1e94..764400f 100644
--- a/drivers/staging/unisys/visorutil/visorkmodutils.c
+++ b/drivers/staging/unisys/visorutil/visorkmodutils.c
@@ -1,6 +1,6 @@
/* timskmodutils.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -20,6 +20,21 @@
#define MYDRVNAME "timskmodutils"
+/* the s-Par firmware uses the virtualization hardware in the CPU to split up
+ * processors into partitions. The hypervisor ID can be found in the CPUID
+ * hypervisor feature leaf, encoded as a string "UnisysSpar64" across the
+ * returned ebx/ecx/edx registers.
+ */
+int is_spar_system() {
+ unsigned int eax, ebx, ecx, edx;
+
+ cpuid( 0x00000001, &eax, &ebx, &ecx, &edx ); /* check for virtual processor */
+ if( !(ecx & 0x80000000) ) return 0;
+ cpuid( 0x40000000, &eax, &ebx, &ecx, &edx ); /* check for s-Par id */
+ return (ebx == 0x73696e55) && (ecx == 0x70537379)
+ && (edx == 0x34367261);
+}
+
/** Callers to interfaces that set __GFP_NORETRY flag below
* must check for a NULL (error) result as we are telling the
* kernel interface that it is okay to fail.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, Apr 09, 2014 at 02:04:50PM -0500, Romer, Benjamin M wrote:
> This patch adds a function, is_spar_system(), to check that s-Par
> firmware is present, and then uses this function at the beginning of
> each module to verify that the modules are being run on an s-Par system
> before beginning initialization. If the firmware is not detected, the
> module will return a failure code.
>
> Checking for s-Par firmware uses the cpuid instruction to verify that
> the processor is running with virtualization turned on, and then uses a
> second cpuid instruction to check that the hypervisor ID is
> "UnisysSpar64".
Why not use the cpuid infrastructure to automatically load/bind your
code and not rely on it being loaded "by hand"? Doesn't that work for
CPU types already?
> Additionally, some minor clean-up was done on copyright tags and
> unnecessary messages were removed from the visorchipset_main() function.
Patches need to do only one thing, so can you please split this up in to
multiple patches, each one only doing one thing.
> This patch was tested with KVM to ensure that the modules do not load
> when s-Par firmware is not present, and tested with s-Par 4.0.12 to
> verify that the modules load correctly when the firmware is present.
>
> Signed-off-by: Benjamin Romer <[email protected]>
You forgot to add a "Reported-by:" line here, and possibly a
"Tested-by:" if someone tested it and reported that it solved the
problem. Proper attribution is very important.
thanks,
greg k-h
On Wed, Apr 09, 2014 at 02:04:50PM -0500, Romer, Benjamin M wrote:
> @@ -71,5 +71,6 @@
> u64 somethings, char *buf, size_t bufsize);
> struct seq_file *visor_seq_file_new_buffer(void *buf, size_t buf_size);
> void visor_seq_file_done_buffer(struct seq_file *m);
> +int is_spar_system( void );
That's a horrid function name to polute the global namespace, please be
more "unique".
>
> #endif
> diff --git a/drivers/staging/unisys/uislib/uislib.c b/drivers/staging/unisys/uislib/uislib.c
> index 8ea9c46..aa60ccb 100644
> --- a/drivers/staging/unisys/uislib/uislib.c
> +++ b/drivers/staging/unisys/uislib/uislib.c
> @@ -1,6 +1,6 @@
> /* uislib.c
> *
> - * Copyright � 2010 - 2013 UNISYS CORPORATION
> + * Copyright © 2010 - 2013 UNISYS CORPORATION
> * All rights reserved.
> *
> * This program is free software; you can redistribute it and/or modify
> @@ -2276,6 +2276,11 @@
> static int __init
> uislib_mod_init(void)
> {
> + /* check for s-Par support */
> + if( !is_spar_system() ) {
> + printk( "s-Par not detected.\n" );
> + return -EPERM;
> + }
Always run your patches through scripts/checkpatch.pl so I don't reject
them for the things it points out...
thanks,
greg k-h
This patch has a million checkpatch.pl warnings... We were so nice to
you on merging this driver directly into staging without commenting on
the style but YOU"RE IN THE ARMY NOW!!! Please, fix all the checkpatch
warnings and resend. :P
> diff --git a/drivers/staging/unisys/include/timskmodutils.h b/drivers/staging/unisys/include/timskmodutils.h
> index 2d81d46..cc439d3 100644
> --- a/drivers/staging/unisys/include/timskmodutils.h
> +++ b/drivers/staging/unisys/include/timskmodutils.h
> @@ -1,6 +1,6 @@
> /* timskmodutils.h
> *
> - * Copyright � 2010 - 2013 UNISYS CORPORATION
> + * Copyright © 2010 - 2013 UNISYS CORPORATION
> * All rights reserved.
> *
Send these typo fixes as a separate patch.
> @@ -2276,6 +2276,11 @@
> static int __init
> uislib_mod_init(void)
> {
> + /* check for s-Par support */
> + if( !is_spar_system() ) {
> + printk( "s-Par not detected.\n" );
> + return -EPERM;
EPERM isn't the right return code. Probably use ENODEV; By the way,
I'm confused why we have this check in so many places. Can't we just
check at module_init() or probe() or something? (I didn't try find the
answer to this question because I am a bad human being and lazy).
> @@ -2681,18 +2681,13 @@
> struct proc_dir_entry *toolaction_file;
> struct proc_dir_entry *bootToTool_file;
>
> - LOGINF("chipset driver version %s loaded", VERSION);
> - /* process module options */
> - POSTCODE_LINUX_2(DRIVER_ENTRY_PC, POSTCODE_SEVERITY_INFO);
> + /* check for s-Par support */
> + if( !is_spar_system() ) {
> + printk( "s-Par not detected.\n" );
> + return -EPERM;
> + }
>
> - LOGINF("option - testvnic=%d", visorchipset_testvnic);
> - LOGINF("option - testvnicclient=%d", visorchipset_testvnicclient);
> - LOGINF("option - testmsg=%d", visorchipset_testmsg);
> - LOGINF("option - testteardown=%d", visorchipset_testteardown);
> - LOGINF("option - major=%d", visorchipset_major);
> - LOGINF("option - serverregwait=%d", visorchipset_serverregwait);
> - LOGINF("option - clientregwait=%d", visorchipset_clientregwait);
> - LOGINF("option - holdchipsetready=%d", visorchipset_holdchipsetready);
> + POSTCODE_LINUX_2(DRIVER_ENTRY_PC, POSTCODE_SEVERITY_INFO);
Separate patch.
> +/* the s-Par firmware uses the virtualization hardware in the CPU to split up
> + * processors into partitions. The hypervisor ID can be found in the CPUID
> + * hypervisor feature leaf, encoded as a string "UnisysSpar64" across the
> + * returned ebx/ecx/edx registers.
> + */
> +int is_spar_system() {
> + unsigned int eax, ebx, ecx, edx;
> +
> + cpuid( 0x00000001, &eax, &ebx, &ecx, &edx ); /* check for virtual processor */
> + if( !(ecx & 0x80000000) ) return 0;
> + cpuid( 0x40000000, &eax, &ebx, &ecx, &edx ); /* check for s-Par id */
> + return (ebx == 0x73696e55) && (ecx == 0x70537379)
> + && (edx == 0x34367261);
> +}
Here is how to write this in kernel style:
int is_spar_system(void)
{
unsigned int eax, ebx, ecx, edx;
/* check for virtual processor */
cpuid(0x00000001, &eax, &ebx, &ecx, &edx);
if (!(ecx & 0x80000000))
return 0;
/* check for s-Par id */
cpuid(0x40000000, &eax, &ebx, &ecx, &edx);
return (ebx == 0x73696e55) && (ecx == 0x70537379) &&
(edx == 0x34367261);
}
Basically, any variation from that is going to make someone complain
about something... The void has to be there in the declaration. The
curly brace has to be on a line by itself. The comment has to be moved
to the line before so it doesn't go over 80 characters. The return has
to be on a line by itself. I put a blank line between the virt and the
spar checks, but that blank is optional. I moved the commen up a line
but that was a preference choice and reviewers are not allowed to
complain about matters of preference like that. The "&&" has to be at
the end of the line instead of the start of the line. The white space
on the last line is:
[tab][space][space][space][space][space][space][space](edx ==
Honestly, kernel coding is like that. Those are all rules.
Checkpatch.pl will help you with most of them. Good luck. :)
regards,
dan carpenter
On Wed, 2014-04-09 at 12:25 -0700, Greg Kroah-Hartman wrote:
> Patches need to do only one thing, so can you please split this up in to
> multiple patches, each one only doing one thing.
Sorry about that! I'll send another patch that fixes all of the
copyright statements at once then. :)
> You forgot to add a "Reported-by:" line here, and possibly a
> "Tested-by:" if someone tested it and reported that it solved the
> problem. Proper attribution is very important.
Will do! Thanks for all the feedback. I'll rework my patch. :)
-- Ben
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?