Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <535ccbeb-b6e5-b7ef-47b4-894af24c00b0@redhat.com>
Date:   Sun, 10 Jul 2022 18:12:44 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for
 IFS image
Content-Language: en-US
To:     Jithu Joseph <jithu.joseph@intel.com>, markgross@kernel.org
Cc:     ashok.raj@intel.com, tony.luck@intel.com,
        gregkh@linuxfoundation.org, ravi.v.shankar@intel.com,
        linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        patches@lists.linux.dev
References: <20220710160011.995800-1-jithu.joseph@intel.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20220710160011.995800-1-jithu.joseph@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@intel.com>
> ---
> 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@intel.com>
> -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