Hi,
On 7/10/22 18:00, Jithu Joseph wrote:
> Existing implementation limits IFS images to be loaded only from
> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
>
> But there are situations where there may be multiple scan files
> that can be run on a particular system stored in /lib/firmware/intel/ifs
>
> E.g.
> 1. Because test contents are larger than the memory reserved for IFS by BIOS
> 2. To provide increased test coverage
> 3. Custom test files to debug certain specific issues in the field
>
> Renaming each of these to ff-mm-ss.scan and then loading might be
> possible in some environments. But on systems where /lib is read-only
> this is not a practical solution.
>
> Modify the semantics of the driver file
> /sys/devices/virtual/misc/intel_ifs_0/reload such that,
> it interprets the input as the filename to be loaded.
Much better, but I do wonder about the behavior of still loading
the default filename at module-init? If there can be multiple
different "test-patterns" then does it not make more sense to
let the user always load the desired pattern before testing first?
Not doing the initial load at module-load time will also speed-up
the module initialization and thus booting the system. Especially
on many-core servers this might make a measurable difference
in module-init time.
Regards,
Hans
>
> Signed-off-by: Jithu Joseph <jithu.joseph@xxxxxxxxx>
> ---
> Changes in v2
> - drop treating "1" specially, i.e treat everything as a file-name
>
> drivers/platform/x86/intel/ifs/ifs.h | 11 ++++----
> drivers/platform/x86/intel/ifs/core.c | 2 +-
> drivers/platform/x86/intel/ifs/load.c | 25 +++++++++++++------
> drivers/platform/x86/intel/ifs/sysfs.c | 13 +++-------
> .../ABI/testing/sysfs-platform-intel-ifs | 3 +--
> 5 files changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 73c8e91cf144..577cee7db86a 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -34,12 +34,13 @@
> * socket in a two step process using writes to MSRs to first load the
> * SHA hashes for the test. Then the tests themselves. Status MSRs provide
> * feedback on the success/failure of these steps. When a new test file
> - * is installed it can be loaded by writing to the driver reload file::
> + * is installed it can be loaded by writing the filename to the driver reload file::
> *
> - * # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload
> + * # echo mytest > /sys/devices/virtual/misc/intel_ifs_0/reload
> *
> - * Similar to microcode, the current version of the scan tests is stored
> - * in a fixed location: /lib/firmware/intel/ifs.0/family-model-stepping.scan
> + * The file will be loaded from /lib/firmware/intel/ifs/mytest
> + * The default file /lib/firmware/intel/ifs/family-model-stepping.scan
> + * will be loaded during module insertion.
> *
> * Running tests
> * -------------
> @@ -225,7 +226,7 @@ static inline struct ifs_data *ifs_get_data(struct device *dev)
> return &d->data;
> }
>
> -void ifs_load_firmware(struct device *dev);
> +int ifs_load_firmware(struct device *dev, const char *file_name);
> int do_core_test(int cpu, struct device *dev);
> const struct attribute_group **ifs_get_groups(void);
>
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 27204e3d674d..9c319ada62d8 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -53,7 +53,7 @@ static int __init ifs_init(void)
> 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);
> + ifs_load_firmware(ifs_device.misc.this_device, NULL);
> up(&ifs_sem);
> return 0;
> }
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index d056617ddc85..89d76bd8b40a 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -232,17 +232,27 @@ static bool ifs_image_sanity_check(struct device *dev, const struct microcode_he
>
> /*
> * 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}.
> + * in the folder /lib/firmware/intel/ifs/
> */
> -void ifs_load_firmware(struct device *dev)
> +int ifs_load_firmware(struct device *dev, const char *file_name)
> {
> struct ifs_data *ifsd = ifs_get_data(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);
> + char scan_path[64];
> + int ret = -EINVAL;
> + int file_name_len;
> +
> + if (!file_name) {
> + 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);
> + } else {
> + if (strchr(file_name, '/'))
> + goto done;
> + file_name_len = strchrnul(file_name, '\n') - file_name;
> + if (snprintf(scan_path, sizeof(scan_path), "intel/ifs/%.*s",
> + file_name_len, file_name) >= sizeof(scan_path))
> + goto done;
> + }
>
> ret = request_firmware_direct(&fw, scan_path, dev);
> if (ret) {
> @@ -263,4 +273,5 @@ void ifs_load_firmware(struct device *dev)
> release_firmware(fw);
> done:
> ifsd->loaded = (ret == 0);
> + return ret;
> }
> diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
> index 37d8380d6fa8..b4716b7d36aa 100644
> --- a/drivers/platform/x86/intel/ifs/sysfs.c
> +++ b/drivers/platform/x86/intel/ifs/sysfs.c
> @@ -94,23 +94,16 @@ static ssize_t reload_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - struct ifs_data *ifsd = ifs_get_data(dev);
> - bool res;
> -
> -
> - if (kstrtobool(buf, &res))
> - return -EINVAL;
> - if (!res)
> - return count;
> + int ret;
>
> if (down_interruptible(&ifs_sem))
> return -EINTR;
>
> - ifs_load_firmware(dev);
> + ret = ifs_load_firmware(dev, buf);
>
> up(&ifs_sem);
>
> - return ifsd->loaded ? count : -ENODEV;
> + return ret ? ret : count;
> }
>
> static DEVICE_ATTR_WO(reload);
> diff --git a/Documentation/ABI/testing/sysfs-platform-intel-ifs b/Documentation/ABI/testing/sysfs-platform-intel-ifs
> index 486d6d2ff8a0..0b373f73a2b6 100644
> --- a/Documentation/ABI/testing/sysfs-platform-intel-ifs
> +++ b/Documentation/ABI/testing/sysfs-platform-intel-ifs
> @@ -35,5 +35,4 @@ What: /sys/devices/virtual/misc/intel_ifs_<N>/reload
> Date: April 21 2022
> KernelVersion: 5.19
> Contact: "Jithu Joseph" <jithu.joseph@xxxxxxxxx>
> -Description: Write "1" (or "y" or "Y") to reload the IFS image from
> - /lib/firmware/intel/ifs/ff-mm-ss.scan.
> +Description: Write <file_name> to reload the IFS image from /lib/firmware/intel/<file_name>.
>
> base-commit: 88084a3df1672e131ddc1b4e39eeacfd39864acf
