| Message ID | 20220422200219.2843823-5-tony.luck@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Introduce In Field Scan driver | expand |
On Fri, Apr 22, 2022 at 01:02:13PM -0700, Tony Luck wrote: > From: Jithu Joseph <jithu.joseph@intel.com> > > Driver probe routine allocates structure to communicate status > and parameters between functions in the driver. Also call > load_ifs_binary() to load the scan image file. > > There is a separate scan image file for each processor family, > model, stepping combination. This is read from the static path: > > /lib/firmware/intel/ifs/{ff-mm-ss}.scan > > Step 1 in loading is to generate the correct path and use > request_firmware_direct() to load into memory. > > Subsequent patches will use the IFS MSR interfaces to copy > the image to BIOS reserved memory and validate the SHA256 > checksums. > > Signed-off-by: Jithu Joseph <jithu.joseph@intel.com> > Co-developed-by: Tony Luck <tony.luck@intel.com> > Signed-off-by: Tony Luck <tony.luck@intel.com> > --- > drivers/platform/x86/intel/ifs/Makefile | 2 +- > drivers/platform/x86/intel/ifs/core.c | 36 ++++++++++++++++++++++++- > drivers/platform/x86/intel/ifs/ifs.h | 25 +++++++++++++++++ > drivers/platform/x86/intel/ifs/load.c | 28 +++++++++++++++++++ > 4 files changed, 89 insertions(+), 2 deletions(-) > create mode 100644 drivers/platform/x86/intel/ifs/ifs.h > create mode 100644 drivers/platform/x86/intel/ifs/load.c > > diff --git a/drivers/platform/x86/intel/ifs/Makefile b/drivers/platform/x86/intel/ifs/Makefile > index af904880e959..98b6fde15689 100644 > --- a/drivers/platform/x86/intel/ifs/Makefile > +++ b/drivers/platform/x86/intel/ifs/Makefile > @@ -1,3 +1,3 @@ > obj-$(CONFIG_INTEL_IFS) += intel_ifs.o > > -intel_ifs-objs := core.o > +intel_ifs-objs := core.o load.o > diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c > index 5713e6ee90f0..ed4ded6755b2 100644 > --- a/drivers/platform/x86/intel/ifs/core.c > +++ b/drivers/platform/x86/intel/ifs/core.c > @@ -6,6 +6,8 @@ > > #include <asm/cpu_device_id.h> > > +#include "ifs.h" > + > enum test_types { > IFS_SAF, > }; > @@ -20,10 +22,27 @@ static const struct x86_cpu_id ifs_cpu_ids[] __initconst = { > }; > MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids); > > +static struct ifs_device ifs_devices[] = { > + [IFS_SAF] = { > + .data = { > + .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT, > + }, > + .misc = { > + .name = "intel_ifs_0", > + .nodename = "intel_ifs/0", > + .minor = MISC_DYNAMIC_MINOR, > + }, > + }, > +}; > + > +#define IFS_NUMTESTS ARRAY_SIZE(ifs_devices) Cute way to do this, but I don't see you ever have any more devices added to this list in this series. Did I miss them? If not, why all the overhead and complexity involved here for just a single misc device? thanks, greg k-h
On Tue, Apr 26, 2022 at 12:45:40PM +0200, Greg KH wrote: > On Fri, Apr 22, 2022 at 01:02:13PM -0700, Tony Luck wrote: > > drivers/platform/x86/intel/ifs/Makefile | 2 +- > > drivers/platform/x86/intel/ifs/core.c | 36 ++++++++++++++++++++++++- > > drivers/platform/x86/intel/ifs/ifs.h | 25 +++++++++++++++++ > > drivers/platform/x86/intel/ifs/load.c | 28 +++++++++++++++++++ You haven't commented on the source tree location. With the change to use misc_register() this isn't a "platform" device anymore. Should I move to "drivers/misc/"? Or is there some better spot that preseves the detail that this is an x86/intel driver in the path? > > +static struct ifs_device ifs_devices[] = { > > + [IFS_SAF] = { > > + .data = { > > + .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT, > > + }, > > + .misc = { > > + .name = "intel_ifs_0", > > + .nodename = "intel_ifs/0", > > + .minor = MISC_DYNAMIC_MINOR, > > + }, > > + }, > > +}; > > + > > +#define IFS_NUMTESTS ARRAY_SIZE(ifs_devices) > > Cute way to do this, but I don't see you ever have any more devices > added to this list in this series. Did I miss them? That's in part 11/10 ... I have hardware, so I'm pretty sure that this is a real thing. Just not ready to post until Intel announces the details of the new test type. > If not, why all the overhead and complexity involved here for just a > single misc device? It didn't seem like a lot of complexity here. It makes the changes to this file to add an extra test trivial (just a new name in the "enum" and a new initializer in ifs_devices[]). Obviously some more code in load.c and runtest.c to handle the new test type. If it really is too much now, I can rip it out from this submission and add it back when the second test is ready for public view. -Tony
On Tue, Apr 26, 2022 at 09:12:37AM -0700, Luck, Tony wrote: > On Tue, Apr 26, 2022 at 12:45:40PM +0200, Greg KH wrote: > > On Fri, Apr 22, 2022 at 01:02:13PM -0700, Tony Luck wrote: > > > drivers/platform/x86/intel/ifs/Makefile | 2 +- > > > drivers/platform/x86/intel/ifs/core.c | 36 ++++++++++++++++++++++++- > > > drivers/platform/x86/intel/ifs/ifs.h | 25 +++++++++++++++++ > > > drivers/platform/x86/intel/ifs/load.c | 28 +++++++++++++++++++ > > You haven't commented on the source tree location. With the change > to use misc_register() this isn't a "platform" device anymore. > > Should I move to "drivers/misc/"? Or is there some better spot that > preseves the detail that this is an x86/intel driver in the path? There's misc_register() users all over the tree, no need for it to be in drivers/misc/ at all, especially if this really is a platform device as this one is. It's fine here. > > > +static struct ifs_device ifs_devices[] = { > > > + [IFS_SAF] = { > > > + .data = { > > > + .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT, > > > + }, > > > + .misc = { > > > + .name = "intel_ifs_0", > > > + .nodename = "intel_ifs/0", > > > + .minor = MISC_DYNAMIC_MINOR, > > > + }, > > > + }, > > > +}; > > > + > > > +#define IFS_NUMTESTS ARRAY_SIZE(ifs_devices) > > > > Cute way to do this, but I don't see you ever have any more devices > > added to this list in this series. Did I miss them? > > That's in part 11/10 ... I have hardware, so I'm pretty sure that this > is a real thing. Just not ready to post until Intel announces the > details of the new test type. Let's not over-engineer for anything we can not review today please. > > If not, why all the overhead and complexity involved here for just a > > single misc device? > > It didn't seem like a lot of complexity here. It makes the changes to > this file to add an extra test trivial (just a new name in the "enum" > and a new initializer in ifs_devices[]). > > Obviously some more code in load.c and runtest.c to handle the new > test type. > > If it really is too much now, I can rip it out from this submission > and add it back when the second test is ready for public view. Please do, thanks. greg k-h
On Tue, Apr 26, 2022 at 06:36:46PM +0200, Greg KH wrote: > On Tue, Apr 26, 2022 at 09:12:37AM -0700, Luck, Tony wrote: > > If it really is too much now, I can rip it out from this submission > > and add it back when the second test is ready for public view. > > Please do, thanks. Hmmm ... maybe there were more bits than I thought. 1 file changed, 19 insertions(+), 36 deletions(-) core.c is now down to just 80 lines ... so that was a significant fraction of the file. Net change below (I'll thread it back into the patch series before reposting). Any other comments on the series? -Tony diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c index 317ed3225307..489b77645b5e 100644 --- a/drivers/platform/x86/intel/ifs/core.c +++ b/drivers/platform/x86/intel/ifs/core.c @@ -9,10 +9,6 @@ #include "ifs.h" -enum test_types { - IFS_SAF, -}; - #define X86_MATCH(model) \ X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6, \ INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL) @@ -23,27 +19,21 @@ static const struct x86_cpu_id ifs_cpu_ids[] __initconst = { }; MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids); -static struct ifs_device ifs_devices[] = { - [IFS_SAF] = { - .data = { - .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT, - }, - .misc = { - .name = "intel_ifs_0", - .nodename = "intel_ifs/0", - .minor = MISC_DYNAMIC_MINOR, - }, +static struct ifs_device ifs_device = { + .data = { + .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT, + }, + .misc = { + .name = "intel_ifs_0", + .nodename = "intel_ifs/0", + .minor = MISC_DYNAMIC_MINOR, }, }; -#define IFS_NUMTESTS ARRAY_SIZE(ifs_devices) - static int __init ifs_init(void) { const struct x86_cpu_id *m; - int ndevices = 0; u64 msrval; - int i; m = x86_match_cpu(ifs_cpu_ids); if (!m) @@ -61,32 +51,25 @@ static int __init ifs_init(void) if (ifs_setup_wq()) return -ENOMEM; - for (i = 0; i < IFS_NUMTESTS; i++) { - if (!(msrval & BIT(ifs_devices[i].data.integrity_cap_bit))) - continue; - - ifs_devices[i].misc.groups = ifs_get_groups(); - if (!misc_register(&ifs_devices[i].misc)) { - ndevices++; - down(&ifs_sem); - ifs_load_firmware(ifs_devices[i].misc.this_device); - up(&ifs_sem); - } - } + ifs_device.misc.groups = ifs_get_groups(); - if (!ndevices) + if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) && + !misc_register(&ifs_device.misc)) { + down(&ifs_sem); + ifs_load_firmware(ifs_device.misc.this_device); + up(&ifs_sem); + } else { ifs_destroy_wq(); + return -ENODEV; + } - return ndevices ? 0 : -ENODEV; + return 0; } static void __exit ifs_exit(void) { - int i; - for (i = 0; i < IFS_NUMTESTS; i++) - if (ifs_devices[i].misc.this_device) - misc_deregister(&ifs_devices[i].misc); + misc_deregister(&ifs_device.misc); ifs_destroy_wq(); }
diff --git a/drivers/platform/x86/intel/ifs/Makefile b/drivers/platform/x86/intel/ifs/Makefile index af904880e959..98b6fde15689 100644 --- a/drivers/platform/x86/intel/ifs/Makefile +++ b/drivers/platform/x86/intel/ifs/Makefile @@ -1,3 +1,3 @@ obj-$(CONFIG_INTEL_IFS) += intel_ifs.o -intel_ifs-objs := core.o +intel_ifs-objs := core.o load.o diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c index 5713e6ee90f0..ed4ded6755b2 100644 --- a/drivers/platform/x86/intel/ifs/core.c +++ b/drivers/platform/x86/intel/ifs/core.c @@ -6,6 +6,8 @@ #include <asm/cpu_device_id.h> +#include "ifs.h" + enum test_types { IFS_SAF, }; @@ -20,10 +22,27 @@ static const struct x86_cpu_id ifs_cpu_ids[] __initconst = { }; MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids); +static struct ifs_device ifs_devices[] = { + [IFS_SAF] = { + .data = { + .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT, + }, + .misc = { + .name = "intel_ifs_0", + .nodename = "intel_ifs/0", + .minor = MISC_DYNAMIC_MINOR, + }, + }, +}; + +#define IFS_NUMTESTS ARRAY_SIZE(ifs_devices) + static int __init ifs_init(void) { const struct x86_cpu_id *m; + int ndevices = 0; u64 msrval; + int i; m = x86_match_cpu(ifs_cpu_ids); if (!m) @@ -38,11 +57,26 @@ static int __init ifs_init(void) if (rdmsrl_safe(MSR_INTEGRITY_CAPS, &msrval)) return -ENODEV; - return 0; + for (i = 0; i < IFS_NUMTESTS; i++) { + if (!(msrval & BIT(ifs_devices[i].data.integrity_cap_bit))) + continue; + + if (!misc_register(&ifs_devices[i].misc)) { + ndevices++; + ifs_load_firmware(ifs_devices[i].misc.this_device); + } + } + + return ndevices ? 0 : -ENODEV; } static void __exit ifs_exit(void) { + int i; + + for (i = 0; i < IFS_NUMTESTS; i++) + if (ifs_devices[i].misc.this_device) + misc_deregister(&ifs_devices[i].misc); } module_init(ifs_init); diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h new file mode 100644 index 000000000000..9a0f8e2077e2 --- /dev/null +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* Copyright(c) 2022 Intel Corporation. */ + +#ifndef _IFS_H_ +#define _IFS_H_ + +#include <linux/device.h> +#include <linux/miscdevice.h> + +/** + * struct ifs_data - attributes related to intel IFS driver + * @integrity_cap_bit - MSR_INTEGRITY_CAPS bit enumerating this test + */ +struct ifs_data { + int integrity_cap_bit; +}; + +struct ifs_device { + struct ifs_data data; + struct miscdevice misc; +}; + +void ifs_load_firmware(struct device *dev); + +#endif diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c new file mode 100644 index 000000000000..9fb71d38c819 --- /dev/null +++ b/drivers/platform/x86/intel/ifs/load.c @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright(c) 2022 Intel Corporation. */ + +#include <linux/firmware.h> + +#include "ifs.h" + +/* + * Load ifs image. Before loading ifs module, the ifs image must be located + * in /lib/firmware/intel/ifs and named as {family/model/stepping}.{testname}. + */ +void ifs_load_firmware(struct device *dev) +{ + const struct firmware *fw; + char scan_path[32]; + int ret; + + snprintf(scan_path, sizeof(scan_path), "intel/ifs/%02x-%02x-%02x.scan", + boot_cpu_data.x86, boot_cpu_data.x86_model, boot_cpu_data.x86_stepping); + + ret = request_firmware_direct(&fw, scan_path, dev); + if (ret) { + dev_err(dev, "ifs file %s load failed\n", scan_path); + return; + } + + release_firmware(fw); +}