Initial implementation of IFS driver assumed a single IFS test image
file with a fixed name.
Subsequently, it became evident that supporting more than one
test image file is needed to provide more comprehensive
test coverage. (Test coverage in this scenario refers to testing
more transistors in the core to identify faults).
This series makes the driver aware of multiple scan test image files,
modifies IFS test image file headers to make it fully compatible
with microcode headers and adds a few other bug fixes and changes.
Patch organization:
Patches 1, 2, and 3: bug fixes
Patch 4: Removes image loading during init path
Patch 5, 6 and 7: exports a couple of microcode/intel.c functions
for use by driver
Patch 8: Adds Meta-data support in microcode file
Rest of them are IFS driver changes
Patches 9 and 10: IFS header format changes to make it fully compatible
to intel microcode header format
Patches 11, 12 and 13: native support for multiple scan test image files
Patch 14: reverts the broken flag
Ashok Raj (1):
x86/microcode/intel: Meta-data support in microcode file
Jithu Joseph (13):
platform/x86/intel/ifs: Remove unused selection
platform/x86/intel/ifs: Propagate load failure error code
platform/x86/intel/ifs: return a more appropriate Error code
platform/x86/intel/ifs: Remove image loading during init
x86/microcode/intel: Expose find_matching_signature() for IFS
x86/microcode/intel: Use appropriate type in microcode_sanity_check()
x86/microcode/intel: Expose microcode_sanity_check()
platform/x86/intel/ifs: Use generic microcode headers and functions
platform/x86/intel/ifs: Add metadata validation
platform/x86/intel/ifs: Remove reload sysfs entry
platform/x86/intel/ifs: Add current_batch sysfs entry
Documentation/ABI: Update IFS ABI doc
Revert "platform/x86/intel/ifs: Mark as BROKEN"
arch/x86/include/asm/microcode_intel.h | 25 ++-
drivers/platform/x86/intel/ifs/ifs.h | 27 ++-
arch/x86/kernel/cpu/microcode/intel.c | 81 ++++++--
drivers/platform/x86/intel/ifs/core.c | 7 +-
drivers/platform/x86/intel/ifs/load.c | 174 +++++++++---------
drivers/platform/x86/intel/ifs/runtest.c | 10 +-
drivers/platform/x86/intel/ifs/sysfs.c | 41 +++--
.../ABI/testing/sysfs-platform-intel-ifs | 30 +--
drivers/platform/x86/intel/ifs/Kconfig | 4 -
9 files changed, 243 insertions(+), 156 deletions(-)
base-commit: f76349cf41451c5c42a99f18a9163377e4b364ff
--
2.25.1
scan_chunks_sanity_check() was returning -ENOMEM if it encounters
an error while copying IFS test image from memory to Secure Memory.
Return -EIO in this scenario, as it is more appropriate.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/load.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index ebaa1d6a2b18..b88db0765311 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -157,8 +157,10 @@ static int scan_chunks_sanity_check(struct device *dev)
INIT_WORK(&local_work.w, copy_hashes_authenticate_chunks);
schedule_work_on(cpu, &local_work.w);
wait_for_completion(&ifs_done);
- if (ifsd->loading_error)
+ if (ifsd->loading_error) {
+ ret = -EIO;
goto out;
+ }
package_authenticated[curr_pkg] = 1;
}
ret = 0;
--
2.25.1
Existing implementation was returning fixed error code to user space
regardless of the load failure encountered.
Modify this to propagate the actual error code to user space.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/ifs.h | 2 +-
drivers/platform/x86/intel/ifs/load.c | 4 +++-
drivers/platform/x86/intel/ifs/sysfs.c | 7 +++----
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 73c8e91cf144..782bcc039ddb 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -225,7 +225,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);
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/load.c b/drivers/platform/x86/intel/ifs/load.c
index d056617ddc85..ebaa1d6a2b18 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -234,7 +234,7 @@ 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}.
*/
-void ifs_load_firmware(struct device *dev)
+int ifs_load_firmware(struct device *dev)
{
struct ifs_data *ifsd = ifs_get_data(dev);
const struct firmware *fw;
@@ -263,4 +263,6 @@ 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..4af4e1bea98d 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -94,9 +94,8 @@ 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;
-
+ int rc;
if (kstrtobool(buf, &res))
return -EINVAL;
@@ -106,11 +105,11 @@ static ssize_t reload_store(struct device *dev,
if (down_interruptible(&ifs_sem))
return -EINTR;
- ifs_load_firmware(dev);
+ rc = ifs_load_firmware(dev);
up(&ifs_sem);
- return ifsd->loaded ? count : -ENODEV;
+ return (rc == 0) ? count : rc;
}
static DEVICE_ATTR_WO(reload);
--
2.25.1
From: Ashok Raj <[email protected]>
Intel has made extensions to the microcode file format so that it
can also be used for In Field Scan. One of the existing reserved fields
has been allocated to indicate the size of the region in the file allocated
for metadata structures.
Microcode Format
+----------------------+ Base
|Header Version |
+----------------------+
|Update revision |
+----------------------+
|Date DDMMYYYY |
+----------------------+
|Sig |
+----------------------+
|Checksum |
+----------------------+
|Loader Version |
+----------------------+
|Processor Flags |
+----------------------+
|Data Size |
+----------------------+
|Total Size |
+----------------------+
|Meta Size |
+----------------------+
|Reserved |
+----------------------+
|Reserved |
+----------------------+ Base+48
| |
| |
| |
| |
| Microcode |
| |
| Data |
| |
| |
+----------------------+ Base+48+data_size-
| | meta_size
| Meta Data |
| |
+----------------------+ Base+48+data_size
| Extended Signature |
| Table |
| |
| |
| |
| |
| |
+----------------------+ Base+total_size
In subsequent patches IFS test image file (which reuse microcode header
format) will make use of metadata section. Though IFS is the first
consumer of this metadata within microcode file, it is expected that
this will be used for supporting upcoming microcode update
related enhancements.
Also add an accessor function which will return a pointer to the
start of a specific meta_type being queried.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Ashok Raj <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
arch/x86/include/asm/microcode_intel.h | 19 +++++++++-
arch/x86/kernel/cpu/microcode/intel.c | 48 ++++++++++++++++++++++++--
2 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 27eba991c6b6..dcbc377f67d1 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -14,7 +14,8 @@ struct microcode_header_intel {
unsigned int pf;
unsigned int datasize;
unsigned int totalsize;
- unsigned int reserved[3];
+ unsigned int metasize;
+ unsigned int reserved[2];
};
struct microcode_intel {
@@ -36,6 +37,18 @@ struct extended_sigtable {
struct extended_signature sigs[];
};
+#define META_TYPE_END (0)
+
+struct metadata_header {
+ unsigned int meta_type;
+ unsigned int meta_blk_size;
+};
+
+struct metadata_intel {
+ struct metadata_header meta_hdr;
+ unsigned int meta_bits[];
+};
+
#define DEFAULT_UCODE_DATASIZE (2000)
#define MC_HEADER_SIZE (sizeof(struct microcode_header_intel))
#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
@@ -76,6 +89,7 @@ extern int __init save_microcode_in_initrd_intel(void);
void reload_ucode_intel(void);
int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf);
int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver);
+struct metadata_header *microcode_intel_find_meta_data(void *ucode, unsigned int meta_type);
#else
static inline __init void load_ucode_intel_bsp(void) {}
static inline void load_ucode_intel_ap(void) {}
@@ -86,6 +100,9 @@ static inline int microcode_intel_find_matching_signature(void *mc, unsigned int
{ return 0; }
static inline int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
{ return -EINVAL; }
+static inline struct metadata_header *microcode_intel_find_meta_data(void *ucode,
+ unsigned int meta_type)
+ { return NULL; }
#endif
#endif /* _ASM_X86_MICROCODE_INTEL_H */
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index bc3f33a25d7a..179ca345bc06 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -168,14 +168,17 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
{
- unsigned long total_size, data_size, ext_table_size;
+ unsigned long total_size, data_size, ext_table_size, total_meta;
struct microcode_header_intel *mc_header = mc;
struct extended_sigtable *ext_header = NULL;
u32 sum, orig_sum, ext_sigcount = 0, i;
struct extended_signature *ext_sig;
+ struct metadata_header *meta_header;
+ unsigned long meta_size = 0;
total_size = get_totalsize(mc_header);
data_size = get_datasize(mc_header);
+ total_meta = mc_header->metasize;
if (data_size + MC_HEADER_SIZE > total_size) {
if (print_err)
@@ -245,7 +248,7 @@ int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
}
if (!ext_table_size)
- return 0;
+ goto check_meta;
/*
* Check extended signature checksum: 0 => valid.
@@ -262,6 +265,22 @@ int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
return -EINVAL;
}
}
+
+check_meta:
+ if (!total_meta)
+ return 0;
+
+ meta_header = (mc + MC_HEADER_SIZE + data_size) - total_meta;
+ while (meta_header->meta_type != META_TYPE_END) {
+ meta_size += meta_header->meta_blk_size;
+ if (!meta_header->meta_blk_size || meta_size > total_meta) {
+ if (print_err) {
+ pr_err("Bad value for metadata size, aborting.\n");
+ return -EINVAL;
+ }
+ }
+ meta_header = (void *)meta_header + meta_header->meta_blk_size;
+ }
return 0;
}
EXPORT_SYMBOL_GPL(microcode_intel_sanity_check);
@@ -967,3 +986,28 @@ struct microcode_ops * __init init_intel_microcode(void)
return µcode_intel_ops;
}
+
+struct metadata_header *microcode_intel_find_meta_data(void *ucode, unsigned int meta_type)
+{
+ struct metadata_header *meta_header;
+ unsigned long data_size, total_meta;
+ unsigned long meta_size = 0;
+
+ data_size = get_datasize(ucode);
+ total_meta = ((struct microcode_intel *)ucode)->hdr.metasize;
+
+ if (!total_meta)
+ return NULL;
+
+ meta_header = (ucode + MC_HEADER_SIZE + data_size) - total_meta;
+
+ while ((meta_header->meta_type != META_TYPE_END) && meta_size < total_meta) {
+ meta_size += meta_header->meta_blk_size;
+ if (meta_header->meta_type == meta_type)
+ return meta_header;
+
+ meta_header = (void *)meta_header + meta_header->meta_blk_size;
+ }
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(microcode_intel_find_meta_data);
--
2.25.1
Initial implementation assumed a single IFS test image file with a
fixed name ff-mm-ss.scan. (where ff, mm, ss refers to family, model
and stepping of the core)
Subsequently, it became evident that supporting more than one
test image file is needed to provide more comprehensive
test coverage. (Test coverage in this scenario refers to testing
more transistors in the core to identify faults)
The other alternative of increasing the size of a single scan test image
file would not work as the upper bound is limited by the size of memory
area reserved by BIOS for loading IFS test image.
Introduce "current_batch" file which accepts a number. Writing a
number to the current_batch file would load the test image file by name
ff-mm-ss-<xy>.scan, where <xy> is the number written to the
"current_batch" file in hex. Range check of the input is done to verify
it not greater than 0xff.
For e.g if the scan test image comprises of 6 files, they would be named
as show below:
06-8f-06-01.scan
06-8f-06-02.scan
06-8f-06-03.scan
06-8f-06-04.scan
06-8f-06-05.scan
06-8f-06-06.scan
And writing 3 to current_batch would result in loading 06-8f-06-03.scan
in the above e.g. The file can also be read to know the currently loaded
file.
And testing a system looks like:
for each scan file
do
load the IFS test image file (write to the batch file)
for each core
do
test the core with this set of tests
done
done
Qualify few error messages with the test image file suffix to
provide better context.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/ifs.h | 21 +++++++++----
drivers/platform/x86/intel/ifs/core.c | 1 +
drivers/platform/x86/intel/ifs/load.c | 14 +++++----
drivers/platform/x86/intel/ifs/runtest.c | 10 ++++---
drivers/platform/x86/intel/ifs/sysfs.c | 38 ++++++++++++++++++++++++
5 files changed, 70 insertions(+), 14 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index bb43fd65d2d2..7651558c218e 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -33,13 +33,23 @@
* The driver loads the tests into memory reserved BIOS local to each CPU
* 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::
+ * feedback on the success/failure of these steps.
*
- * # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload
+ * The test files are kept in a fixed location: /lib/firmware/intel/ifs_0/
+ * For e.g if there are 3 test files, they would be named in the following
+ * fashion:
+ * ff-mm-ss-01.scan
+ * ff-mm-ss-02.scan
+ * ff-mm-ss-03.scan
+ * (where ff refers to family, mm indicates model and ss indicates stepping)
*
- * 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
+ * A different testfile can be loaded by writing the numerical portion
+ * (e.g 1, 2 or 3 in the above scenario) into the curent_batch file.
+ * To load ff-mm-ss-02.scan, the following command can be used::
+ *
+ * # echo 2 > /sys/devices/virtual/misc/intel_ifs_0/current_batch
+ *
+ * The above file can also be read to know the currently loaded image.
*
* Running tests
* -------------
@@ -207,6 +217,7 @@ struct ifs_data {
int status;
u64 scan_details;
int cur_batch;
+ int test_num;
};
struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 5fb7f655c291..1f040837e8eb 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -22,6 +22,7 @@ MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
static struct ifs_device ifs_device = {
.data = {
.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
+ .test_num = 0,
},
.misc = {
.name = "intel_ifs_0",
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index d300cf3ce5bd..abc217da3e55 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -225,17 +225,18 @@ static int ifs_image_sanity_check(struct device *dev, const struct microcode_hea
/*
* 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 /lib/firmware/intel/ifs_x/ and named as family-model-stepping-02x.{testname}.
*/
int ifs_load_firmware(struct device *dev)
{
struct ifs_data *ifsd = ifs_get_data(dev);
const struct firmware *fw;
- char scan_path[32];
- int ret;
+ char scan_path[64];
+ int ret = -EINVAL;
- 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);
+ snprintf(scan_path, sizeof(scan_path), "intel/ifs_%d/%02x-%02x-%02x-%02x.scan",
+ ifsd->test_num, boot_cpu_data.x86, boot_cpu_data.x86_model,
+ boot_cpu_data.x86_stepping, ifsd->cur_batch);
ret = request_firmware_direct(&fw, scan_path, dev);
if (ret) {
@@ -251,6 +252,9 @@ int ifs_load_firmware(struct device *dev)
ifs_hash_ptr = (u64)(ifs_header_ptr + 1);
ret = scan_chunks_sanity_check(dev);
+ if (ret)
+ dev_err(dev, "Load failure for batch: %02x\n", ifsd->cur_batch);
+
release:
release_firmware(fw);
done:
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index b2ca2bb4501f..0bfd8fcdd7e8 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -78,14 +78,16 @@ static void message_not_tested(struct device *dev, int cpu, union ifs_status sta
static void message_fail(struct device *dev, int cpu, union ifs_status status)
{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+
/*
* control_error is set when the microcode runs into a problem
* loading the image from the reserved BIOS memory, or it has
* been corrupted. Reloading the image may fix this issue.
*/
if (status.control_error) {
- dev_err(dev, "CPU(s) %*pbl: could not execute from loaded scan image\n",
- cpumask_pr_args(cpu_smt_mask(cpu)));
+ dev_err(dev, "CPU(s) %*pbl: could not execute from loaded scan image. Batch: %02x version: 0x%x\n",
+ cpumask_pr_args(cpu_smt_mask(cpu)), ifsd->cur_batch, ifsd->loaded_version);
}
/*
@@ -96,8 +98,8 @@ static void message_fail(struct device *dev, int cpu, union ifs_status status)
* the core being tested.
*/
if (status.signature_error) {
- dev_err(dev, "CPU(s) %*pbl: test signature incorrect.\n",
- cpumask_pr_args(cpu_smt_mask(cpu)));
+ dev_err(dev, "CPU(s) %*pbl: test signature incorrect. Batch: %02x version: 0x%x\n",
+ cpumask_pr_args(cpu_smt_mask(cpu)), ifsd->cur_batch, ifsd->loaded_version);
}
}
diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
index e077910c5d28..e6e5fb2b7fc0 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -87,6 +87,43 @@ static ssize_t run_test_store(struct device *dev,
static DEVICE_ATTR_WO(run_test);
+static ssize_t current_batch_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+ int cur_batch;
+ int rc;
+
+ rc = kstrtouint(buf, 0, &cur_batch);
+ if (rc < 0 || cur_batch > 0xff)
+ return -EINVAL;
+
+ if (down_interruptible(&ifs_sem))
+ return -EINTR;
+
+ ifsd->cur_batch = cur_batch;
+
+ rc = ifs_load_firmware(dev);
+
+ up(&ifs_sem);
+
+ return (rc == 0) ? count : rc;
+}
+
+static ssize_t current_batch_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+
+ if (!ifsd->loaded)
+ return sysfs_emit(buf, "%s\n", "none");
+ else
+ return sysfs_emit(buf, "0x%02x\n", ifsd->cur_batch);
+}
+
+static DEVICE_ATTR_RW(current_batch);
+
/*
* Display currently loaded IFS image version.
*/
@@ -108,6 +145,7 @@ static struct attribute *plat_ifs_attrs[] = {
&dev_attr_details.attr,
&dev_attr_status.attr,
&dev_attr_run_test.attr,
+ &dev_attr_current_batch.attr,
&dev_attr_image_version.attr,
NULL
};
--
2.25.1
The data type of print_err parameter used by microcode_sanity_check()
is int. In prepration for exporting this function, to be used by
the IFS driver, Convert it to a more appropriate bool type for readability.
No functional change intended.
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Ashok Raj <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index f0cc60d92dfc..5473b094baee 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -164,7 +164,7 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
intel_ucode_patch = p->data;
}
-static int microcode_sanity_check(void *mc, int print_err)
+static int microcode_sanity_check(void *mc, bool print_err)
{
unsigned long total_size, data_size, ext_table_size;
struct microcode_header_intel *mc_header = mc;
@@ -282,7 +282,7 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
mc_size = get_totalsize(mc_header);
if (!mc_size ||
mc_size > size ||
- microcode_sanity_check(data, 0) < 0)
+ microcode_sanity_check(data, false) < 0)
break;
size -= mc_size;
@@ -821,7 +821,7 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
memcpy(mc, &mc_header, sizeof(mc_header));
data = mc + sizeof(mc_header);
if (!copy_from_iter_full(data, data_size, iter) ||
- microcode_sanity_check(mc, 1) < 0) {
+ microcode_sanity_check(mc, true) < 0) {
break;
}
--
2.25.1
IFS uses 'scan test images' provided by Intel that can be regarded as
firmware. IFS test image carries microcode header with extended signature
table.
Expose find_matching_signature() for verifying if the test image
header or the extended signature table indicate whether an IFS test image
is fit to run on a system. Add microcode_intel_ prefix to the
function name.
No functional change
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Ashok Raj <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
arch/x86/include/asm/microcode_intel.h | 3 +++
arch/x86/kernel/cpu/microcode/intel.c | 19 ++++++++++---------
2 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 4c92cea7e4b5..33db2a62ed34 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -74,12 +74,15 @@ extern void load_ucode_intel_ap(void);
extern void show_ucode_info_early(void);
extern int __init save_microcode_in_initrd_intel(void);
void reload_ucode_intel(void);
+int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf);
#else
static inline __init void load_ucode_intel_bsp(void) {}
static inline void load_ucode_intel_ap(void) {}
static inline void show_ucode_info_early(void) {}
static inline int __init save_microcode_in_initrd_intel(void) { return -EINVAL; }
static inline void reload_ucode_intel(void) {}
+static inline int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf)
+ { return 0; }
#endif
#endif /* _ASM_X86_MICROCODE_INTEL_H */
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 025c8f0cd948..f0cc60d92dfc 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -48,7 +48,7 @@ static int llc_size_per_core;
/*
* Returns 1 if update has been found, 0 otherwise.
*/
-static int find_matching_signature(void *mc, unsigned int csig, int cpf)
+int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf)
{
struct microcode_header_intel *mc_hdr = mc;
struct extended_sigtable *ext_hdr;
@@ -72,6 +72,7 @@ static int find_matching_signature(void *mc, unsigned int csig, int cpf)
}
return 0;
}
+EXPORT_SYMBOL_GPL(microcode_intel_find_matching_signature);
/*
* Returns 1 if update has been found, 0 otherwise.
@@ -83,7 +84,7 @@ static int has_newer_microcode(void *mc, unsigned int csig, int cpf, int new_rev
if (mc_hdr->rev <= new_rev)
return 0;
- return find_matching_signature(mc, csig, cpf);
+ return microcode_intel_find_matching_signature(mc, csig, cpf);
}
static struct ucode_patch *memdup_patch(void *data, unsigned int size)
@@ -117,7 +118,7 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
sig = mc_saved_hdr->sig;
pf = mc_saved_hdr->pf;
- if (find_matching_signature(data, sig, pf)) {
+ if (microcode_intel_find_matching_signature(data, sig, pf)) {
prev_found = true;
if (mc_hdr->rev <= mc_saved_hdr->rev)
@@ -149,7 +150,7 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
if (!p)
return;
- if (!find_matching_signature(p->data, uci->cpu_sig.sig, uci->cpu_sig.pf))
+ if (!microcode_intel_find_matching_signature(p->data, uci->cpu_sig.sig, uci->cpu_sig.pf))
return;
/*
@@ -286,8 +287,8 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
size -= mc_size;
- if (!find_matching_signature(data, uci->cpu_sig.sig,
- uci->cpu_sig.pf)) {
+ if (!microcode_intel_find_matching_signature(data, uci->cpu_sig.sig,
+ uci->cpu_sig.pf)) {
data += mc_size;
continue;
}
@@ -652,9 +653,9 @@ static struct microcode_intel *find_patch(struct ucode_cpu_info *uci)
if (phdr->rev <= uci->cpu_sig.rev)
continue;
- if (!find_matching_signature(phdr,
- uci->cpu_sig.sig,
- uci->cpu_sig.pf))
+ if (!microcode_intel_find_matching_signature(phdr,
+ uci->cpu_sig.sig,
+ uci->cpu_sig.pf))
continue;
return iter->data;
--
2.25.1
IFS test image carries the same microcode header as regular Intel
microcode blobs. Microcode blobs use header version of 1,
whereas IFS test images will use header version of 2.
microcode_sanity_check() can be used by IFS driver to perform
sanity check of the IFS test images too.
Refactor header version as a parameter and expose this function.
Qualify the function name with intel.
No functional change intended.
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Ashok Raj <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
arch/x86/include/asm/microcode_intel.h | 3 +++
arch/x86/kernel/cpu/microcode/intel.c | 14 +++++++++-----
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 33db2a62ed34..27eba991c6b6 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -75,6 +75,7 @@ extern void show_ucode_info_early(void);
extern int __init save_microcode_in_initrd_intel(void);
void reload_ucode_intel(void);
int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf);
+int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver);
#else
static inline __init void load_ucode_intel_bsp(void) {}
static inline void load_ucode_intel_ap(void) {}
@@ -83,6 +84,8 @@ static inline int __init save_microcode_in_initrd_intel(void) { return -EINVAL;
static inline void reload_ucode_intel(void) {}
static inline int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf)
{ return 0; }
+static inline int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
+ { return -EINVAL; }
#endif
#endif /* _ASM_X86_MICROCODE_INTEL_H */
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 5473b094baee..bc3f33a25d7a 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -37,6 +37,8 @@
#include <asm/setup.h>
#include <asm/msr.h>
+#define MICROCODE_HEADER_VER 1
+
static const char ucode_path[] = "kernel/x86/microcode/GenuineIntel.bin";
/* Current microcode patch used in early patching on the APs. */
@@ -164,7 +166,7 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
intel_ucode_patch = p->data;
}
-static int microcode_sanity_check(void *mc, bool print_err)
+int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
{
unsigned long total_size, data_size, ext_table_size;
struct microcode_header_intel *mc_header = mc;
@@ -181,9 +183,10 @@ static int microcode_sanity_check(void *mc, bool print_err)
return -EINVAL;
}
- if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
+ if (mc_header->ldrver != 1 || mc_header->hdrver != hdr_ver) {
if (print_err)
- pr_err("Error: invalid/unknown microcode update format.\n");
+ pr_err("Error: invalid/unknown microcode update format. Header version %d\n",
+ mc_header->hdrver);
return -EINVAL;
}
@@ -261,6 +264,7 @@ static int microcode_sanity_check(void *mc, bool print_err)
}
return 0;
}
+EXPORT_SYMBOL_GPL(microcode_intel_sanity_check);
/*
* Get microcode matching with BSP's model. Only CPUs with the same model as
@@ -282,7 +286,7 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
mc_size = get_totalsize(mc_header);
if (!mc_size ||
mc_size > size ||
- microcode_sanity_check(data, false) < 0)
+ microcode_intel_sanity_check(data, false, MICROCODE_HEADER_VER) < 0)
break;
size -= mc_size;
@@ -821,7 +825,7 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
memcpy(mc, &mc_header, sizeof(mc_header));
data = mc + sizeof(mc_header);
if (!copy_from_iter_full(data, data_size, iter) ||
- microcode_sanity_check(mc, true) < 0) {
+ microcode_intel_sanity_check(mc, true, MICROCODE_HEADER_VER) < 0) {
break;
}
--
2.25.1
Remove reload documentation and add current_batch documentation.
Update the kernel version and date for all the entries.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
.../ABI/testing/sysfs-platform-intel-ifs | 30 ++++++++++---------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-platform-intel-ifs b/Documentation/ABI/testing/sysfs-platform-intel-ifs
index 486d6d2ff8a0..f74df3abee57 100644
--- a/Documentation/ABI/testing/sysfs-platform-intel-ifs
+++ b/Documentation/ABI/testing/sysfs-platform-intel-ifs
@@ -1,39 +1,41 @@
What: /sys/devices/virtual/misc/intel_ifs_<N>/run_test
-Date: April 21 2022
-KernelVersion: 5.19
+Date: Sept 30 2022
+KernelVersion: 6.2
Contact: "Jithu Joseph" <[email protected]>
Description: Write <cpu#> to trigger IFS test for one online core.
Note that the test is per core. The cpu# can be
for any thread on the core. Running on one thread
completes the test for the core containing that thread.
Example: to test the core containing cpu5: echo 5 >
- /sys/devices/platform/intel_ifs.<N>/run_test
+ /sys/devices/virtual/misc/intel_ifs_<N>/run_test
What: /sys/devices/virtual/misc/intel_ifs_<N>/status
-Date: April 21 2022
-KernelVersion: 5.19
+Date: Sept 30 2022
+KernelVersion: 6.2
Contact: "Jithu Joseph" <[email protected]>
Description: The status of the last test. It can be one of "pass", "fail"
or "untested".
What: /sys/devices/virtual/misc/intel_ifs_<N>/details
-Date: April 21 2022
-KernelVersion: 5.19
+Date: Sept 30 2022
+KernelVersion: 6.2
Contact: "Jithu Joseph" <[email protected]>
Description: Additional information regarding the last test. The details file reports
the hex value of the SCAN_STATUS MSR. Note that the error_code field
may contain driver defined software code not defined in the Intel SDM.
What: /sys/devices/virtual/misc/intel_ifs_<N>/image_version
-Date: April 21 2022
-KernelVersion: 5.19
+Date: Sept 30 2022
+KernelVersion: 6.2
Contact: "Jithu Joseph" <[email protected]>
Description: Version (hexadecimal) of loaded IFS binary image. If no scan image
is loaded reports "none".
-What: /sys/devices/virtual/misc/intel_ifs_<N>/reload
-Date: April 21 2022
-KernelVersion: 5.19
+What: /sys/devices/virtual/misc/intel_ifs_<N>/current_batch
+Date: Sept 30 2022
+KernelVersion: 6.2
Contact: "Jithu Joseph" <[email protected]>
-Description: Write "1" (or "y" or "Y") to reload the IFS image from
- /lib/firmware/intel/ifs/ff-mm-ss.scan.
+Description: Write a number less than or equal to 0xff to load an IFS test image.
+ The number written treated as the 2 digit suffix in the following file name:
+ /lib/firmware/intel/ifs_<N>/ff-mm-ss-02x.scan
+ Reading the file will provide the suffix of the currently loaded IFS test image.
--
2.25.1
Existing implementation loads IFS test image during the driver
init flow.
Dropping test image loading from the init path improves
module load time.
Prior to starting IFS tests, the user has to load one of
the IFS test images by writing to the current_batch sysfs file.
Removing IFS test image loading during init also allows us to
make ifs_sem static as it is used only within sysfs.c.
Reviewed-by: Tony Luck <[email protected]>
Suggested-by: Hans de Goede <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/ifs.h | 2 --
drivers/platform/x86/intel/ifs/core.c | 6 +-----
drivers/platform/x86/intel/ifs/sysfs.c | 2 +-
3 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 782bcc039ddb..be37512535f2 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -229,6 +229,4 @@ int ifs_load_firmware(struct device *dev);
int do_core_test(int cpu, struct device *dev);
const struct attribute_group **ifs_get_groups(void);
-extern struct semaphore ifs_sem;
-
#endif
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 27204e3d674d..5fb7f655c291 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -51,12 +51,8 @@ static int __init ifs_init(void)
ifs_device.misc.groups = ifs_get_groups();
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);
+ !misc_register(&ifs_device.misc))
return 0;
- }
return -ENODEV;
}
diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
index 4af4e1bea98d..766cee651bd6 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -13,7 +13,7 @@
* Protects against simultaneous tests on multiple cores, or
* reloading can file while a test is in progress
*/
-DEFINE_SEMAPHORE(ifs_sem);
+static DEFINE_SEMAPHORE(ifs_sem);
/*
* The sysfs interface to check additional details of last test
--
2.25.1
Reload sysfs entry is replaced by current_batch, drop it.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/sysfs.c | 28 --------------------------
1 file changed, 28 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
index 766cee651bd6..e077910c5d28 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -87,33 +87,6 @@ static ssize_t run_test_store(struct device *dev,
static DEVICE_ATTR_WO(run_test);
-/*
- * Reload the IFS image. When user wants to install new IFS image
- */
-static ssize_t reload_store(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
-{
- bool res;
- int rc;
-
- if (kstrtobool(buf, &res))
- return -EINVAL;
- if (!res)
- return count;
-
- if (down_interruptible(&ifs_sem))
- return -EINTR;
-
- rc = ifs_load_firmware(dev);
-
- up(&ifs_sem);
-
- return (rc == 0) ? count : rc;
-}
-
-static DEVICE_ATTR_WO(reload);
-
/*
* Display currently loaded IFS image version.
*/
@@ -135,7 +108,6 @@ static struct attribute *plat_ifs_attrs[] = {
&dev_attr_details.attr,
&dev_attr_status.attr,
&dev_attr_run_test.attr,
- &dev_attr_reload.attr,
&dev_attr_image_version.attr,
NULL
};
--
2.25.1
Existing implementation (broken) of IFS used a header format (for IFS
test images) which was very similar to microcode format, but didn’t
accommodate extended signatures. This meant same IFS test image had to
be duplicated for different steppings and the validation code in the
driver was only looking at the primary header parameters. Going forward
IFS test image headers has been tweaked to become fully compatible with
microcode format.
Newer IFS test image headers will use microcode_header_intel->hdrver = 2,
so as to distinguish it from microcode images and older IFS test images.
In light of the above, use struct microcode_header_intel directly in
IFS driver instead of duplicating into a separate ifs_header structure.
Further use existing microcode_sanity_check() and find_matching_signature()
directly instead of implementing separate ones within the driver.
More IFS specific checks will be added subsequently.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/load.c | 102 +++++---------------------
1 file changed, 19 insertions(+), 83 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index b88db0765311..3cb13a7aa74b 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -8,22 +8,9 @@
#include "ifs.h"
-struct ifs_header {
- u32 header_ver;
- u32 blob_revision;
- u32 date;
- u32 processor_sig;
- u32 check_sum;
- u32 loader_rev;
- u32 processor_flags;
- u32 metadata_size;
- u32 total_size;
- u32 fusa_info;
- u64 reserved;
-};
-
-#define IFS_HEADER_SIZE (sizeof(struct ifs_header))
-static struct ifs_header *ifs_header_ptr; /* pointer to the ifs image header */
+#define IFS_HEADER_SIZE (sizeof(struct microcode_header_intel))
+#define IFS_HEADER_VER 2
+static struct microcode_header_intel *ifs_header_ptr; /* pointer to the ifs image header */
static u64 ifs_hash_ptr; /* Address of ifs metadata (hash) */
static u64 ifs_test_image_ptr; /* 256B aligned address of test pattern */
static DECLARE_COMPLETION(ifs_done);
@@ -118,33 +105,18 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
*/
static int scan_chunks_sanity_check(struct device *dev)
{
- int metadata_size, curr_pkg, cpu, ret = -ENOMEM;
struct ifs_data *ifsd = ifs_get_data(dev);
+ int curr_pkg, cpu, ret = -ENOMEM;
bool *package_authenticated;
struct ifs_work local_work;
- char *test_ptr;
package_authenticated = kcalloc(topology_max_packages(), sizeof(bool), GFP_KERNEL);
if (!package_authenticated)
return ret;
- metadata_size = ifs_header_ptr->metadata_size;
- /* Spec says that if the Meta Data Size = 0 then it should be treated as 2000 */
- if (metadata_size == 0)
- metadata_size = 2000;
-
- /* Scan chunk start must be 256 byte aligned */
- if ((metadata_size + IFS_HEADER_SIZE) % 256) {
- dev_err(dev, "Scan pattern offset within the binary is not 256 byte aligned\n");
- return -EINVAL;
- }
-
- test_ptr = (char *)ifs_header_ptr + IFS_HEADER_SIZE + metadata_size;
ifsd->loading_error = false;
-
- ifs_test_image_ptr = (u64)test_ptr;
- ifsd->loaded_version = ifs_header_ptr->blob_revision;
+ ifsd->loaded_version = ifs_header_ptr->rev;
/* copy the scan hash and authenticate per package */
cpus_read_lock();
@@ -171,67 +143,32 @@ static int scan_chunks_sanity_check(struct device *dev)
return ret;
}
-static int ifs_sanity_check(struct device *dev,
- const struct microcode_header_intel *mc_header)
+static int ifs_image_sanity_check(struct device *dev, const struct microcode_header_intel *data)
{
- unsigned long total_size, data_size;
- u32 sum, *mc;
-
- total_size = get_totalsize(mc_header);
- data_size = get_datasize(mc_header);
+ struct ucode_cpu_info uci;
- if ((data_size + MC_HEADER_SIZE > total_size) || (total_size % sizeof(u32))) {
- dev_err(dev, "bad ifs data file size.\n");
+ if (data->hdrver != IFS_HEADER_VER) {
+ dev_err(dev, "Header version %d not supported\n", data->hdrver);
return -EINVAL;
}
- if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
- dev_err(dev, "invalid/unknown ifs update format.\n");
+ if (microcode_intel_sanity_check((void *)data, true, IFS_HEADER_VER)) {
+ dev_err(dev, "sanity check failed\n");
return -EINVAL;
}
- mc = (u32 *)mc_header;
- sum = 0;
- for (int i = 0; i < total_size / sizeof(u32); i++)
- sum += mc[i];
+ intel_cpu_collect_info(&uci);
- if (sum) {
- dev_err(dev, "bad ifs data checksum, aborting.\n");
+ if (!microcode_intel_find_matching_signature((void *)data,
+ uci.cpu_sig.sig,
+ uci.cpu_sig.pf)) {
+ dev_err(dev, "cpu signature, pf not matching\n");
return -EINVAL;
}
return 0;
}
-static bool find_ifs_matching_signature(struct device *dev, struct ucode_cpu_info *uci,
- const struct microcode_header_intel *shdr)
-{
- unsigned int mc_size;
-
- mc_size = get_totalsize(shdr);
-
- if (!mc_size || ifs_sanity_check(dev, shdr) < 0) {
- dev_err(dev, "ifs sanity check failure\n");
- return false;
- }
-
- if (!intel_cpu_signatures_match(uci->cpu_sig.sig, uci->cpu_sig.pf, shdr->sig, shdr->pf)) {
- dev_err(dev, "ifs signature, pf not matching\n");
- return false;
- }
-
- return true;
-}
-
-static bool ifs_image_sanity_check(struct device *dev, const struct microcode_header_intel *data)
-{
- struct ucode_cpu_info uci;
-
- intel_cpu_collect_info(&uci);
-
- return find_ifs_matching_signature(dev, &uci, data);
-}
-
/*
* 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}.
@@ -252,12 +189,11 @@ int ifs_load_firmware(struct device *dev)
goto done;
}
- if (!ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data)) {
- dev_err(dev, "ifs header sanity check failed\n");
+ ret = ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
+ if (ret)
goto release;
- }
- ifs_header_ptr = (struct ifs_header *)fw->data;
+ ifs_header_ptr = (struct microcode_header_intel *)fw->data;
ifs_hash_ptr = (u64)(ifs_header_ptr + 1);
ret = scan_chunks_sanity_check(dev);
--
2.25.1
The data portion of IFS test image file contains a meta-data
structure in addition to test data and hashes.
Introduce the layout of this meta_data structure and validate
the sanity of certain fields of the new-image before loading.
Tweak references to IFS test image chunks to reflect the updated
layout of the test image.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/ifs.h | 2 +
drivers/platform/x86/intel/ifs/load.c | 54 +++++++++++++++++++++++++++
2 files changed, 56 insertions(+)
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index be37512535f2..bb43fd65d2d2 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -196,6 +196,7 @@ union ifs_status {
* @valid_chunks: number of chunks which could be validated.
* @status: it holds simple status pass/fail/untested
* @scan_details: opaque scan status code from h/w
+ * @cur_batch: suffix indicating the currently loaded test file
*/
struct ifs_data {
int integrity_cap_bit;
@@ -205,6 +206,7 @@ struct ifs_data {
int valid_chunks;
int status;
u64 scan_details;
+ int cur_batch;
};
struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 3cb13a7aa74b..d300cf3ce5bd 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -8,8 +8,24 @@
#include "ifs.h"
+struct meta_data {
+ unsigned int meta_type; // metadata type
+ unsigned int meta_size; // size of this entire struct including hdrs.
+ unsigned int test_type; // IFS test type
+ unsigned int fusa_info; // Fusa info
+ unsigned int total_images; // Total number of images
+ unsigned int current_image; // Current Image #
+ unsigned int total_chunks; // Total number of chunks in this image
+ unsigned int starting_chunk; // Starting chunk number in this image
+ unsigned int size_per_chunk; // size of each chunk
+ unsigned int chunks_per_stride; // number of chunks in a stride
+ unsigned int reserved[54]; // Align to 256 bytes for chunk alignment.
+};
+
#define IFS_HEADER_SIZE (sizeof(struct microcode_header_intel))
#define IFS_HEADER_VER 2
+#define META_TYPE_IFS 1
+#define IFS_CHUNK_ALIGNMENT 256
static struct microcode_header_intel *ifs_header_ptr; /* pointer to the ifs image header */
static u64 ifs_hash_ptr; /* Address of ifs metadata (hash) */
static u64 ifs_test_image_ptr; /* 256B aligned address of test pattern */
@@ -98,6 +114,41 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
complete(&ifs_done);
}
+static int validate_ifs_metadata(struct device *dev)
+{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+ struct meta_data *ifs_meta;
+ char test_file[64];
+ int ret = -EINVAL;
+
+ snprintf(test_file, sizeof(test_file), "%02x-%02x-%02x-%02x.scan",
+ boot_cpu_data.x86, boot_cpu_data.x86_model,
+ boot_cpu_data.x86_stepping, ifsd->cur_batch);
+
+ ifs_meta = (struct meta_data *)microcode_intel_find_meta_data(ifs_header_ptr,
+ META_TYPE_IFS);
+ if (!ifs_meta) {
+ dev_err(dev, "IFS Metadata missing in file %s\n", test_file);
+ return ret;
+ }
+
+ ifs_test_image_ptr = (u64)ifs_meta + sizeof(struct meta_data);
+
+ /* Scan chunk start must be 256 byte aligned */
+ if (!IS_ALIGNED(ifs_test_image_ptr, IFS_CHUNK_ALIGNMENT)) {
+ dev_err(dev, "Scan pattern offset is not 256 byte aligned in %s\n", test_file);
+ return ret;
+ }
+
+ if (ifs_meta->current_image != ifsd->cur_batch) {
+ dev_warn(dev, "Suffix metadata is not matching with filename %s(0x%02x)\n",
+ test_file, ifs_meta->current_image);
+ return ret;
+ }
+
+ return 0;
+}
+
/*
* IFS requires scan chunks authenticated per each socket in the platform.
* Once the test chunk is authenticated, it is automatically copied to secured memory
@@ -114,6 +165,9 @@ static int scan_chunks_sanity_check(struct device *dev)
if (!package_authenticated)
return ret;
+ ret = validate_ifs_metadata(dev);
+ if (ret)
+ return ret;
ifsd->loading_error = false;
ifsd->loaded_version = ifs_header_ptr->rev;
--
2.25.1
Issues with user interface [1] to load scan test images
has been addressed, so the following can be reverted.
commit c483e7ea10fa ("platform/x86/intel/ifs: Mark as BROKEN")
Link: https://lore.kernel.org/lkml/[email protected]/ [1]
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/Kconfig | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig
index 89152d46deee..3eded966757e 100644
--- a/drivers/platform/x86/intel/ifs/Kconfig
+++ b/drivers/platform/x86/intel/ifs/Kconfig
@@ -1,9 +1,6 @@
config INTEL_IFS
tristate "Intel In Field Scan"
depends on X86 && CPU_SUP_INTEL && 64BIT && SMP
- # Discussion on the list has shown that the sysfs API needs a bit
- # more work, mark this as broken for now
- depends on BROKEN
help
Enable support for the In Field Scan capability in select
CPUs. The capability allows for running low level tests via
--
2.25.1
CONFIG_INTEL_IFS_DEVICE is not used anywhere. The selection in
Kconfig is therefore pointless. Delete it.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig
index c341a27cc1a3..89152d46deee 100644
--- a/drivers/platform/x86/intel/ifs/Kconfig
+++ b/drivers/platform/x86/intel/ifs/Kconfig
@@ -4,7 +4,6 @@ config INTEL_IFS
# Discussion on the list has shown that the sysfs API needs a bit
# more work, mark this as broken for now
depends on BROKEN
- select INTEL_IFS_DEVICE
help
Enable support for the In Field Scan capability in select
CPUs. The capability allows for running low level tests via
--
2.25.1
Hi Jithu,
On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> Existing implementation was returning fixed error code to user space
> regardless of the load failure encountered.
>
The tense is a bit confusing here. Also, 'Existing implementation' is
typically implied and unnecessary. Would something like this be better?
The reload operation returns a fixed error code to user space regardless
of the load failure encountered.
Modify..
> Modify this to propagate the actual error code to user space.
>
...
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index d056617ddc85..ebaa1d6a2b18 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -234,7 +234,7 @@ 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}.
> */
> -void ifs_load_firmware(struct device *dev)
> +int ifs_load_firmware(struct device *dev)
> {
> struct ifs_data *ifsd = ifs_get_data(dev);
> const struct firmware *fw;
> @@ -263,4 +263,6 @@ 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..4af4e1bea98d 100644
> --- a/drivers/platform/x86/intel/ifs/sysfs.c
> +++ b/drivers/platform/x86/intel/ifs/sysfs.c
> @@ -94,9 +94,8 @@ 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;
> -
> + int rc;
>
Does rc refer to return code? The other IFS functions like above use the
commonly used 'ret' variable. Any specific reason for the inconsistency?
Also, patch 11 completely removes the reload_store() function. Should we
avoid a separate patch to fix something that is going to be removed
soon? Would re-ordering the patches help in that regard?
> if (kstrtobool(buf, &res))
> return -EINVAL;
> @@ -106,11 +105,11 @@ static ssize_t reload_store(struct device *dev,
> if (down_interruptible(&ifs_sem))
> return -EINTR;
>
> - ifs_load_firmware(dev);
> + rc = ifs_load_firmware(dev);
>
> up(&ifs_sem);
>
> - return ifsd->loaded ? count : -ENODEV;
> + return (rc == 0) ? count : rc;
> }
>
> static DEVICE_ATTR_WO(reload);
Sohil
On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> scan_chunks_sanity_check() was returning -ENOMEM if it encounters
> an error while copying IFS test image from memory to Secure Memory.
>
Same as before:
s/was returning/returns
> Return -EIO in this scenario, as it is more appropriate.
>
Do the first 3 patches need a 'Fixes' tag? Or is the idea here that the
feature isn't truly enabled so everything before removing the BROKEN tag
will be considered together?
> Do the first 3 patches need a 'Fixes' tag? Or is the idea here that the
> feature isn't truly enabled so everything before removing the BROKEN tag
> will be considered together?
No point in back porting a "Fix" to a stable release that still marks the
driver as BROKEN
-Tony
On 10/24/2022 3:52 PM, Sohil Mehta wrote:
> Hi Jithu,
>
> On 10/21/2022 1:34 PM, Jithu Joseph wrote:
>> Existing implementation was returning fixed error code to user space
>> regardless of the load failure encountered.
>>
>
> The tense is a bit confusing here. Also, 'Existing implementation' is typically implied and unnecessary. Would something like this be better?
>
> The reload operation returns a fixed error code to user space regardless of the load failure encountered.
Thanks Sohil for the review, will reword as you suggest above
>
> Modify..
>
>> Modify this to propagate the actual error code to user space.
>>
>
> ...
>
>> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
>> index d056617ddc85..ebaa1d6a2b18 100644
>> --- a/drivers/platform/x86/intel/ifs/load.c
>> +++ b/drivers/platform/x86/intel/ifs/load.c
>> @@ -234,7 +234,7 @@ 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}.
>> */
>> -void ifs_load_firmware(struct device *dev)
>> +int ifs_load_firmware(struct device *dev)
>> {
>> struct ifs_data *ifsd = ifs_get_data(dev);
>> const struct firmware *fw;
>> @@ -263,4 +263,6 @@ void ifs_load_firmware(struct device *dev)
>> release_firmware(fw);
>> done:
>> ifsd->loaded = (ret == 0);
>> +
>> + return ret;
>> }
This change is still needed by the new code, more below
>> diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
>> index 37d8380d6fa8..4af4e1bea98d 100644
>> --- a/drivers/platform/x86/intel/ifs/sysfs.c
>> +++ b/drivers/platform/x86/intel/ifs/sysfs.c
>> @@ -94,9 +94,8 @@ 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;
>> -
>> + int rc;
>>
>
> Does rc refer to return code? The other IFS functions like above use the commonly used 'ret' variable. Any specific reason for the inconsistency?
>
> Also, patch 11 completely removes the reload_store() function. Should we avoid a separate patch to fix something that is going to be removed soon? Would re-ordering the patches help in that regard?
You are right that reload is removed subsequently, only the ifs_load_firmware() part above is needed for the new code . Will move the above to a new patch between current patches 11 and 12 (and drop this patch)
Jithu
On 10/24/2022 4:50 PM, Sohil Mehta wrote:
> On 10/21/2022 1:34 PM, Jithu Joseph wrote:
>> Existing implementation loads IFS test image during the driver
>> init flow.
>>
>> Dropping test image loading from the init path improves
>> module load time.
>>
>> Prior to starting IFS tests, the user has to load one of
>> the IFS test images by writing to the current_batch sysfs file.
>>
>
> The kernel is still unaware of the 'current_batch' sysfs file. It will be introduced in patch 12. You can avoid the reference here.
ok will remove the current_batch reference and mention that user has to explicitly load test images
>
>> Removing IFS test image loading during init also allows us to
>> make ifs_sem static as it is used only within sysfs.c.
>>
>
> Does something like this sound better?
>
> IFS test image is unnecessarily loaded during driver initialization. The user has to load one when starting the tests.
>
> Drop image loading during ifs_init() and improve module load time. As a consequence, make ifs_sem static as it is only used within sysfs.c
Will adopt this, Thanks
>
>> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
>> index 27204e3d674d..5fb7f655c291 100644
>> --- a/drivers/platform/x86/intel/ifs/core.c
>> +++ b/drivers/platform/x86/intel/ifs/core.c
>> @@ -51,12 +51,8 @@ static int __init ifs_init(void)
>> ifs_device.misc.groups = ifs_get_groups();
>> if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) &&
>
> Is there a reason to store the integrity cap constant in the device.data global structure?
>
> .data = {
> .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
> },
This was originally added so that, when future additional tests are supported by the driver, support can be checked using the same if ((msrval & BIT(ifs_device.data.integrity_cap_bit)
Different tests would have different integrity_cap_bit assigned in the ifs_device[] array (today it is just a single element and not an array
Note that the current series doesn't introduce this
Jithu
On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> Existing implementation loads IFS test image during the driver
> init flow.
>
> Dropping test image loading from the init path improves
> module load time.
>
> Prior to starting IFS tests, the user has to load one of
> the IFS test images by writing to the current_batch sysfs file.
>
The kernel is still unaware of the 'current_batch' sysfs file. It will
be introduced in patch 12. You can avoid the reference here.
> Removing IFS test image loading during init also allows us to
> make ifs_sem static as it is used only within sysfs.c.
>
Does something like this sound better?
IFS test image is unnecessarily loaded during driver initialization. The
user has to load one when starting the tests.
Drop image loading during ifs_init() and improve module load time. As a
consequence, make ifs_sem static as it is only used within sysfs.c
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 27204e3d674d..5fb7f655c291 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -51,12 +51,8 @@ static int __init ifs_init(void)
> ifs_device.misc.groups = ifs_get_groups();
>
> if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) &&
Is there a reason to store the integrity cap constant in the device.data
global structure?
.data = {
.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
},
IIUC, you are basically reading an MSR and testing a bit within? You
already have an BIT macro (unused) for that.
#define MSR_INTEGRITY_CAPS_PERIODIC_BIST
BIT(MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT)
> - !misc_register(&ifs_device.misc)) {
> - down(&ifs_sem);
> - ifs_load_firmware(ifs_device.misc.this_device);
> - up(&ifs_sem);
> + !misc_register(&ifs_device.misc))
> return 0;
> - }
>
> return -ENODEV;
> }
Sohil
On 10/24/2022 5:41 PM, Joseph, Jithu wrote:
>>> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
>>> index 27204e3d674d..5fb7f655c291 100644
>>> --- a/drivers/platform/x86/intel/ifs/core.c
>>> +++ b/drivers/platform/x86/intel/ifs/core.c
>>> @@ -51,12 +51,8 @@ static int __init ifs_init(void)
>>> ifs_device.misc.groups = ifs_get_groups();
>>> if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) &&
>>
>> Is there a reason to store the integrity cap constant in the device.data global structure?
>>
>> .data = {
>> .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
>> },
>
> This was originally added so that, when future additional tests are supported by the driver, support can be checked using the same if ((msrval & BIT(ifs_device.data.integrity_cap_bit)
> Different tests would have different integrity_cap_bit assigned in the ifs_device[] array (today it is just a single element and not an array
>
> Note that the current series doesn't introduce this
>
>
Sorry, I am still not able to follow.
Let's say you have an ifs_device[] array which has different integrity
capabilities, there would still need to be some logic in the init code
to differentiate between the resulting action that needs to be taken?
Currently, the init function only registers the device. Maybe some
example/code might be helpful to drive the point.
Also, are the future additional tests expected to be added soon? If not,
maybe we can defer this optimization until the need arrives.
Sohil
>
> Jithu
On 10/24/2022 11:06 PM, Sohil Mehta wrote:
> On 10/24/2022 5:41 PM, Joseph, Jithu wrote:
>>>> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
>>>> index 27204e3d674d..5fb7f655c291 100644
>>>> --- a/drivers/platform/x86/intel/ifs/core.c
>>>> +++ b/drivers/platform/x86/intel/ifs/core.c
>>>> @@ -51,12 +51,8 @@ static int __init ifs_init(void)
>>>> ifs_device.misc.groups = ifs_get_groups();
>>>> if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) &&
>>>
>>> Is there a reason to store the integrity cap constant in the device.data global structure?
>>>
>>> .data = {
>>> .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
>>> },
>>
>> This was originally added so that, when future additional tests are supported by the driver, support can be checked using the same if ((msrval & BIT(ifs_device.data.integrity_cap_bit)
>> Different tests would have different integrity_cap_bit assigned in the ifs_device[] array (today it is just a single element and not an array
>>
>> Note that the current series doesn't introduce this
>>
>>
>
> Sorry, I am still not able to follow.
>
> Let's say you have an ifs_device[] array which has different integrity capabilities, there would still need to be some logic in the init code to differentiate between the resulting action that needs to be taken? Currently, the init function only registers the device. Maybe some example/code might be helpful to drive the point.
multiple devices will be created if support for more than one is advertised by MSR_INTEGRITY_CAPS as shown below
static int __init ifs_init(void)
{
....
if (rdmsrl_safe(MSR_INTEGRITY_CAPS, &msrval))
return -ENODEV;
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++;
}
}
return ndevices ? 0 : -ENODEV;
}
At the handler side we can branch to the corresponding handler by looking at this bit
Say for e.g the following function in runtest.c could be extended to something like
int do_core_test(int cpu, struct device *dev)
{
struct ifs_data *ifsd = ifs_get_data(dev);
.....
switch (ifsd->integrity_cap_bit) {
case MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT:
ifs_test_core(cpu, dev);
break;
case MSR_INTEGRITY_CAPS_TEST2
ifs_do_test2(cpu, dev);
break;
case MSR_INTEGRITY_CAPS_TEST3
ifs_do_test3(cpu, dev);
break;
default:
return -EINVAL;
}
....
}
Jithu
Thanks for the psuedo code. I think I understand the reasoning now.
There would be an IFS device array created for each type of test that
exists. Based on the capabilities supported in MSR_INTEGRITY_CAPS the
specific IFS devices would be created to run the tests.
> multiple devices will be created if support for more than one is advertised by MSR_INTEGRITY_CAPS as shown below
>
Well, it would also depend on whether the currently running kernel has
enumerated that test. IIUC, older kernels running on newer hardware
would only create ifs test devices they are aware of.
It would have been great if the above statement would be true as is :)
> static int __init ifs_init(void)
> {
>
> ....
> if (rdmsrl_safe(MSR_INTEGRITY_CAPS, &msrval))
> return -ENODEV;
>
> 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++;
> }
> }
>
> return ndevices ? 0 : -ENODEV;
> }
>
Nit:
The _BIT extension is probably unnecessary. How about?
.data = {
.integrity_cap = MSR_INTEGRITY_CAPS_PERIODIC_BIST,
},
On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> Refactor header version as a parameter and expose this function.
Isn't the header version part of the microcode data itself?
Microcode Format
+----------------------+ Base
|Header Version |
+----------------------+
|Update revision |
+----------------------+
If so, why the need to pass it as a parameter to sanity_check()?
>
> No functional change intended.
Maybe skip this statement. Apart from adding a parameter to an newly
exported function, there is a change in an error print as well.
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 5473b094baee..bc3f33a25d7a 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -37,6 +37,8 @@
> #include <asm/setup.h>
> #include <asm/msr.h>
>
> +#define MICROCODE_HEADER_VER 1
> +
Should this define be in a central location, like microcode_intel.h?
You would soon be adding a define for IFS_HEADER_VER. Having them
defined together would make it easier to follow.
Sohil
How about?
x86/microcode/intel: Add metadata support
> +struct metadata_header {
> + unsigned int meta_type;
> + unsigned int meta_blk_size;
> +};
> +
> +struct metadata_intel {
> + struct metadata_header meta_hdr;
> + unsigned int meta_bits[];
> +};
> +
Can we avoid the meta_ prefixes in the struct variables since the struct
name already includes meta?
> #define DEFAULT_UCODE_DATASIZE (2000)
> #define MC_HEADER_SIZE (sizeof(struct microcode_header_intel))
> #define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
> @@ -76,6 +89,7 @@ extern int __init save_microcode_in_initrd_intel(void);
> void reload_ucode_intel(void);
> int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf);
> int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver);
> +struct metadata_header *microcode_intel_find_meta_data(void *ucode, unsigned int meta_type);
Is there a difference between "ucode" and "mc"? They seem to be used
interchangeably all over.
At least to keep it consistent across the exported functions, should the
parameter be named "mc"?
> int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
> {
> - unsigned long total_size, data_size, ext_table_size;
> + unsigned long total_size, data_size, ext_table_size, total_meta;
> struct microcode_header_intel *mc_header = mc;
> struct extended_sigtable *ext_header = NULL;
> u32 sum, orig_sum, ext_sigcount = 0, i;
> struct extended_signature *ext_sig;
> + struct metadata_header *meta_header;
> + unsigned long meta_size = 0;
>
> total_size = get_totalsize(mc_header);
> data_size = get_datasize(mc_header);
> + total_meta = mc_header->metasize;
>
> if (data_size + MC_HEADER_SIZE > total_size) {
> if (print_err)
> @@ -245,7 +248,7 @@ int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
> }
>
> if (!ext_table_size)
> - return 0;
> + goto check_meta;
>
The code flow in this function seems a bit confusing. Can we avoid the
goto and make this a bit cleaner?
There is already a check for ext_table_size above. Can the extended
signature checking be merged with that?
> /*
> * Check extended signature checksum: 0 => valid.
> @@ -262,6 +265,22 @@ int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
> return -EINVAL;
> }
> }
> +
> +check_meta:
> + if (!total_meta)
> + return 0;
> +
> + meta_header = (mc + MC_HEADER_SIZE + data_size) - total_meta;
> + while (meta_header->meta_type != META_TYPE_END) {
> + meta_size += meta_header->meta_blk_size;
> + if (!meta_header->meta_blk_size || meta_size > total_meta) {
> + if (print_err) {
> + pr_err("Bad value for metadata size, aborting.\n");
> + return -EINVAL;
> + }
This seems to be returning an error only when print_err is enabled.
Otherwise, it treats as a success.
> + }
> + meta_header = (void *)meta_header + meta_header->meta_blk_size;
> + }
> return 0;
> }
> EXPORT_SYMBOL_GPL(microcode_intel_sanity_check);
> @@ -967,3 +986,28 @@ struct microcode_ops * __init init_intel_microcode(void)
>
> return µcode_intel_ops;
> }
> +
Sohil
On 11/1/2022 1:51 AM, Sohil Mehta wrote:
> How about?
>
> x86/microcode/intel: Add metadata support
Will reword as you suggest above
>
>> +struct metadata_header {
>> + unsigned int meta_type;
>> + unsigned int meta_blk_size;
>> +};
>> +
>> +struct metadata_intel {
>> + struct metadata_header meta_hdr;
>> + unsigned int meta_bits[];
>> +};
>> +
>
> Can we avoid the meta_ prefixes in the struct variables since the struct name already includes meta?
Will do
>
>> #define DEFAULT_UCODE_DATASIZE (2000)
>> #define MC_HEADER_SIZE (sizeof(struct microcode_header_intel))
>> #define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
>> @@ -76,6 +89,7 @@ extern int __init save_microcode_in_initrd_intel(void);
>> void reload_ucode_intel(void);
>> int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf);
>> int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver);
>> +struct metadata_header *microcode_intel_find_meta_data(void *ucode, unsigned int meta_type);
>
> Is there a difference between "ucode" and "mc"? They seem to be used interchangeably all over.
>
> At least to keep it consistent across the exported functions, should the parameter be named "mc"?
Will change the parameter to mc
>
>> int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
>> {
>> - unsigned long total_size, data_size, ext_table_size;
>> + unsigned long total_size, data_size, ext_table_size, total_meta;
>> struct microcode_header_intel *mc_header = mc;
>> struct extended_sigtable *ext_header = NULL;
>> u32 sum, orig_sum, ext_sigcount = 0, i;
>> struct extended_signature *ext_sig;
>> + struct metadata_header *meta_header;
>> + unsigned long meta_size = 0;
>> total_size = get_totalsize(mc_header);
>> data_size = get_datasize(mc_header);
>> + total_meta = mc_header->metasize;
>> if (data_size + MC_HEADER_SIZE > total_size) {
>> if (print_err)
>> @@ -245,7 +248,7 @@ int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
>> }
>> if (!ext_table_size)
>> - return 0;
>> + goto check_meta;
>>
>
> The code flow in this function seems a bit confusing. Can we avoid the goto and make this a bit cleaner?
>
> There is already a check for ext_table_size above. Can the extended signature checking be merged with that?
Will modify the flow as below
- if (!ext_table_size)
- goto check_meta;
-
+ if (ext_table_size) {
/*
* Check extended signature checksum: 0 => valid.
*/
for( ...) {
return -EINVAL;
}
}
+ }
>> +
>> +check_meta:
>> + if (!total_meta)
>> + return 0;
>> +
>> + meta_header = (mc + MC_HEADER_SIZE + data_size) - total_meta;
>> + while (meta_header->meta_type != META_TYPE_END) {
>> + meta_size += meta_header->meta_blk_size;
>> + if (!meta_header->meta_blk_size || meta_size > total_meta) {
>> + if (print_err) {
>> + pr_err("Bad value for metadata size, aborting.\n");
>> + return -EINVAL;
>> + }
>
> This seems to be returning an error only when print_err is enabled. Otherwise, it treats as a success.
>
Thanks for pointing this, will remove the {} following the "if (print_err)"
Jithu
On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> Newer IFS test image headers will use microcode_header_intel->hdrver = 2,
> so as to distinguish it from microcode images and older IFS test images.
>
IIUC, older IFS test images would no longer be supported. Have they been
released publicly?
What would happen if someone tries to load one? I am guessing one of the
error checks would catch it. It might be useful to describe this error
signature in the commit message.
>
> - if ((data_size + MC_HEADER_SIZE > total_size) || (total_size % sizeof(u32))) {
> - dev_err(dev, "bad ifs data file size.\n");
> + if (data->hdrver != IFS_HEADER_VER) {
> + dev_err(dev, "Header version %d not supported\n", data->hdrver);
> return -EINVAL;
> }
>
> - if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
> - dev_err(dev, "invalid/unknown ifs update format.\n");
> + if (microcode_intel_sanity_check((void *)data, true, IFS_HEADER_VER)) {
I referred to this in a another patch. The data->hdrver is already
verified above, why is there a need to pass it as a parameter as well.
> + dev_err(dev, "sanity check failed\n");
> return -EINVAL;
> }
>
> - mc = (u32 *)mc_header;
> - sum = 0;
> - for (int i = 0; i < total_size / sizeof(u32); i++)
> - sum += mc[i];
> + intel_cpu_collect_info(&uci);
>
> - if (sum) {
> - dev_err(dev, "bad ifs data checksum, aborting.\n");
> + if (!microcode_intel_find_matching_signature((void *)data,
> + uci.cpu_sig.sig,
> + uci.cpu_sig.pf)) {
> + dev_err(dev, "cpu signature, pf not matching\n");
What does pf stand for? It would be good to avoid abbreviations for
error logging.
> /*
> * 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}.
> @@ -252,12 +189,11 @@ int ifs_load_firmware(struct device *dev)
> goto done;
> }
>
> - if (!ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data)) {
> - dev_err(dev, "ifs header sanity check failed\n");
> + ret = ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
> + if (ret)
> goto release;
> - }
>
> - ifs_header_ptr = (struct ifs_header *)fw->data;
> + ifs_header_ptr = (struct microcode_header_intel *)fw->data;
The use of a global ifs_header_ptr seems problematic. The semaphore
operation before calling ifs_load_firmware() makes it seem concurrency
is expected. Can ifs_load_firmware() really be called concurrently?
If that is not true can we use a mutex for synchronization?
Sohil
On 11/1/2022 12:28 AM, Sohil Mehta wrote:
> On 10/21/2022 1:34 PM, Jithu Joseph wrote:
>
>> Refactor header version as a parameter and expose this function.
>
> Isn't the header version part of the microcode data itself?
>
Header forms the first 48 bytes in a microcode file followed by contents
> Microcode Format
> +----------------------+ Base
> |Header Version |
> +----------------------+
> |Update revision |
> +----------------------+
>
> If so, why the need to pass it as a parameter to sanity_check()?
>
It is for sanity checking if the contents of the input file matches with the usage scenario.
All microcode files have 1 in the header and IFS test images will use 2.
Existing usages check if the microcode file has Header version 1. This (along with other sanity checks) is done by the below code
microcode_intel_sanity_check(data, false, MICROCODE_HEADER_VER)
and IFS usages check whether the file has Header version 2 (along with other sanity checks shared with microcode files)
microcode_intel_sanity_check(data, true, IFS_HEADER_VER))
In other words the sanity check will flag somebody mistakenly passing in a microcode file as IFS test image (and vice-versa)
>>
>> No functional change intended.
>
> Maybe skip this statement. Apart from adding a parameter to an newly exported function, there is a change in an error print as well.
>
>> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
>> index 5473b094baee..bc3f33a25d7a 100644
>> --- a/arch/x86/kernel/cpu/microcode/intel.c
>> +++ b/arch/x86/kernel/cpu/microcode/intel.c
>> @@ -37,6 +37,8 @@
>> #include <asm/setup.h>
>> #include <asm/msr.h>
>> +#define MICROCODE_HEADER_VER 1
>> +
>
> Should this define be in a central location, like microcode_intel.h?
>
It can be moved there
> You would soon be adding a define for IFS_HEADER_VER. Having them defined together would make it easier to follow.
>
> Sohil
Jithu
On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> The data portion of IFS test image file contains a meta-data
Can we be consistent with meta-data/metadata usage? Multiple patches
have this dual usage.
The popular usage in the kernel seems to be "metadata". I would suggest:
s/meta-data/metadata
> structure in addition to test data and hashes.
>
> Introduce the layout of this meta_data structure and validate
> the sanity of certain fields of the new-image before loading.
>
s/new-image/new image
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index be37512535f2..bb43fd65d2d2 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -196,6 +196,7 @@ union ifs_status {
> * @valid_chunks: number of chunks which could be validated.
> * @status: it holds simple status pass/fail/untested
> * @scan_details: opaque scan status code from h/w
> + * @cur_batch: suffix indicating the currently loaded test file
What does "suffix" refer to here? I feel how you derive the current
batch information shouldn't really matter.
> */
> struct ifs_data {
> int integrity_cap_bit;
> @@ -205,6 +206,7 @@ struct ifs_data {
> int valid_chunks;
> int status;
> u64 scan_details;
> + int cur_batch;
> };
>
...
> #define IFS_HEADER_SIZE (sizeof(struct microcode_header_intel))
> #define IFS_HEADER_VER 2
> +#define META_TYPE_IFS 1
What namespace does this meta type belong to? Is the expectation here
that IFS will have different meta types? Or in the generic microcode
header IFS meta can be found using this type?
I am asking since microcode_intel_find_meta_data() would be eventually
called from other non-ifs places as well.
Can you please point me to the architecture documentation that describes
this?
> +static int validate_ifs_metadata(struct device *dev)
> +{
> + struct ifs_data *ifsd = ifs_get_data(dev);
> + struct meta_data *ifs_meta;
> + char test_file[64];
> + int ret = -EINVAL;
> +
> + snprintf(test_file, sizeof(test_file), "%02x-%02x-%02x-%02x.scan",
> + boot_cpu_data.x86, boot_cpu_data.x86_model,
> + boot_cpu_data.x86_stepping, ifsd->cur_batch);
> +
There are multiple usages of ifsd->cur_batch in this patch. AFAIU, the
variable is still uninitialized. Would this validation patch make more
sense after patch 12? The "cur_batch" terminology is introduced there
and the initialization happens there as well.
> +
> + if (ifs_meta->current_image != ifsd->cur_batch) {
> + dev_warn(dev, "Suffix metadata is not matching with filename %s(0x%02x)\n",
What does "suffix metadata" mean? How about:
Currently loaded filename %s(0x%02x) doesn't match with the information
in the metadata.
Sohil
On 11/1/2022 11:37 AM, Sohil Mehta wrote:
> On 10/21/2022 1:34 PM, Jithu Joseph wrote:
>> Newer IFS test image headers will use microcode_header_intel->hdrver = 2,
>> so as to distinguish it from microcode images and older IFS test images.
>>
>
> IIUC, older IFS test images would no longer be supported. Have they been released publicly?
This is true. The modified driver would need compatible images. It has not been widely release (note that the driver is under CONFIG_BROKEN today)
>
> What would happen if someone tries to load one? I am guessing one of the error checks would catch it. It might be useful to describe this error signature in the commit message.
This will be caught by the sanity_check()
if (microcode_intel_sanity_check((void *)data, true, IFS_HEADER_VER)) {
dev_err(dev, "sanity check failed\n");
return -EINVAL;
}
Further the version mismatch dev_err from microcode_intel_sanity_check() would also be visible
>
>> - if ((data_size + MC_HEADER_SIZE > total_size) || (total_size % sizeof(u32))) {
>> - dev_err(dev, "bad ifs data file size.\n");
>> + if (data->hdrver != IFS_HEADER_VER) {
>> + dev_err(dev, "Header version %d not supported\n", data->hdrver);
>> return -EINVAL;
>> }
>> - if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
>> - dev_err(dev, "invalid/unknown ifs update format.\n");
>> + if (microcode_intel_sanity_check((void *)data, true, IFS_HEADER_VER)) {
>
> I referred to this in a another patch. The data->hdrver is already verified above, why is there a need to pass it as a parameter as well.
Yes, I noted the rationale in my response
>
>> + dev_err(dev, "sanity check failed\n");
>> return -EINVAL;
>> }
>> - mc = (u32 *)mc_header;
>> - sum = 0;
>> - for (int i = 0; i < total_size / sizeof(u32); i++)
>> - sum += mc[i];
>> + intel_cpu_collect_info(&uci);
>> - if (sum) {
>> - dev_err(dev, "bad ifs data checksum, aborting.\n");
>> + if (!microcode_intel_find_matching_signature((void *)data,
>> + uci.cpu_sig.sig,
>> + uci.cpu_sig.pf)) {
>> + dev_err(dev, "cpu signature, pf not matching\n");
>
> What does pf stand for? It would be good to avoid abbreviations for error logging.
>
intel_cpu_collect_info() comments call it as "processor flags from MSR 0x17" ... I will
expand "pf" to "processor flags" in the above message
>
>> /*
>> * 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}.
>> @@ -252,12 +189,11 @@ int ifs_load_firmware(struct device *dev)
>> goto done;
>> }
>> - if (!ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data)) {
>> - dev_err(dev, "ifs header sanity check failed\n");
>> + ret = ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
>> + if (ret)
>> goto release;
>> - }
>> - ifs_header_ptr = (struct ifs_header *)fw->data;
>> + ifs_header_ptr = (struct microcode_header_intel *)fw->data;
>
> The use of a global ifs_header_ptr seems problematic. The semaphore operation before calling ifs_load_firmware() makes it seem concurrency is expected. Can ifs_load_firmware() really be called concurrently?
Multiple simultaneous loads or simultaneous loads and run_tests should not be allowed from IFS device standpoint
Synchronization in the form of "down_interruptible(&ifs_sem)" is in place at the sysfs entry points (run_test_store() and current_batch_store()). If that was not present there is nothing preventing multiple loads (current_batch_store()) or runtests from being called concurrently.
>
> If that is not true can we use a mutex for synchronization?
Since we are using the semaphore initalized to 1 (using DEFINE_SEMAPHORE), I believe it can be replaced by mutex APIs (mutex_lock_interruptible() in place of aforementioned down_interruptible()) . However I feel this change can be taken up separately as current series doesn't introduce any synchronization mechanisms and the existing locking in place seems sufficient for the synchronization needs of the device and its operations.
Jithu
On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> Initial implementation assumed a single IFS test image file with a
> fixed name ff-mm-ss.scan. (where ff, mm, ss refers to family, model
> and stepping of the core)
>
> Subsequently, it became evident that supporting more than one
> test image file is needed to provide more comprehensive
> test coverage. (Test coverage in this scenario refers to testing
> more transistors in the core to identify faults)
>
> The other alternative of increasing the size of a single scan test image
> file would not work as the upper bound is limited by the size of memory
> area reserved by BIOS for loading IFS test image.
>
> Introduce "current_batch" file which accepts a number. Writing a
> number to the current_batch file would load the test image file by name
> ff-mm-ss-<xy>.scan, where <xy> is the number written to the
> "current_batch" file in hex.
Any specific reasoning why the name "current_batch" was chosen? To me,
batch seems to suggest multiple or a group of files. But in reality only
one test file is loaded at a time.
Naming can sometimes be quite subjective so it might be useful to get
multiple opinions here.
As per my understanding, there is sysfs file called run_test which runs
a loaded test. Instead of current_batch how about the name load_test (or
maybe current_test)?
load_test - Write a number less than or equal to 0xff to load an IFS
test image. (Description as-is from the documentation patch)
> * Running tests
> * -------------
> @@ -207,6 +217,7 @@ struct ifs_data {
> int status;
> u64 scan_details;
> int cur_batch;
> + int test_num;
> };
>
> struct ifs_work {
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 5fb7f655c291..1f040837e8eb 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -22,6 +22,7 @@ MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
> static struct ifs_device ifs_device = {
> .data = {
> .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
> + .test_num = 0,
Is this initialization really needed? Wouldn't it default to 0?
Maybe if you explain what does test_num refer to it might answer the above?
> +static ssize_t current_batch_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct ifs_data *ifsd = ifs_get_data(dev);
> +
> + if (!ifsd->loaded)
> + return sysfs_emit(buf, "%s\n", "none");
Why not:
sysfs_emit(buf, "none\n");
> + else
> + return sysfs_emit(buf, "0x%02x\n", ifsd->cur_batch);
> +}
Sohil
On 11/1/2022 3:34 PM, Sohil Mehta wrote:
> On 10/21/2022 1:34 PM, Jithu Joseph wrote:
>> -What: /sys/devices/virtual/misc/intel_ifs_<N>/reload
>> -Date: April 21 2022
>> -KernelVersion: 5.19
>> +What: /sys/devices/virtual/misc/intel_ifs_<N>/current_batch
>> +Date: Sept 30 2022
>> +KernelVersion: 6.2
>> Contact: "Jithu Joseph" <[email protected]>
>> -Description: Write "1" (or "y" or "Y") to reload the IFS image from
>> - /lib/firmware/intel/ifs/ff-mm-ss.scan.
>> +Description: Write a number less than or equal to 0xff to load an IFS test image.
>> + The number written treated as the 2 digit suffix in the following file name:
>> + /lib/firmware/intel/ifs_<N>/ff-mm-ss-02x.scan
>> + Reading the file will provide the suffix of the currently loaded IFS test image.
>
> A few places in the code use the following string: family-model-stepping-02x.{testname}
>
> Does that mean that the in future there might be other extensions apart from .scan?
yes as when when support for additional tests are added
>
> If so, current_batch would let the user identify the 2 digit suffix but how would they identify which testname is current loaded?
The testname would be inferred based the sysfs file context (the<N> in /sys/devices/virtual/misc/intel_ifs_<N>/current_batch) from which the operation is triggered.
Meaning if the user writes to /sys/devices/virtual/misc/intel_ifs_0/current_batch it would look for ff-mm-ss.scan and if they write to
/sys/devices/virtual/misc/intel_ifs_2/current_batch it would look for ff-mm-ss.<test_type_2>
Jithu
On 11/1/2022 3:48 PM, Joseph, Jithu wrote:
> The testname would be inferred based the sysfs file context (the<N> in /sys/devices/virtual/misc/intel_ifs_<N>/current_batch) from which the operation is triggered.
>
> Meaning if the user writes to /sys/devices/virtual/misc/intel_ifs_0/current_batch it would look for ff-mm-ss.scan and if they write to
> /sys/devices/virtual/misc/intel_ifs_2/current_batch it would look for ff-mm-ss.<test_type_2>
>
So intel_ifs_<N> corresponds to <test_type> i.e. intel_ifs_0 maps to .scan
Is the N to test_type mapping visible to the user somewhere? If not, how
would the user infer that?
Would it be useful to name the misc device as intel_ifs_<test_type>?
On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> -What: /sys/devices/virtual/misc/intel_ifs_<N>/reload
> -Date: April 21 2022
> -KernelVersion: 5.19
> +What: /sys/devices/virtual/misc/intel_ifs_<N>/current_batch
> +Date: Sept 30 2022
> +KernelVersion: 6.2
> Contact: "Jithu Joseph" <[email protected]>
> -Description: Write "1" (or "y" or "Y") to reload the IFS image from
> - /lib/firmware/intel/ifs/ff-mm-ss.scan.
> +Description: Write a number less than or equal to 0xff to load an IFS test image.
> + The number written treated as the 2 digit suffix in the following file name:
> + /lib/firmware/intel/ifs_<N>/ff-mm-ss-02x.scan
> + Reading the file will provide the suffix of the currently loaded IFS test image.
A few places in the code use the following string:
family-model-stepping-02x.{testname}
Does that mean that the in future there might be other extensions apart
from .scan?
If so, current_batch would let the user identify the 2 digit suffix but
how would they identify which testname is current loaded?
On 11/1/2022 3:26 PM, Sohil Mehta wrote:
> On 10/21/2022 1:34 PM, Jithu Joseph wrote:
>> Initial implementation assumed a single IFS test image file with a
>> fixed name ff-mm-ss.scan. (where ff, mm, ss refers to family, model
>> and stepping of the core)
>>
>> Subsequently, it became evident that supporting more than one
>> test image file is needed to provide more comprehensive
>> test coverage. (Test coverage in this scenario refers to testing
>> more transistors in the core to identify faults)
>>
>> The other alternative of increasing the size of a single scan test image
>> file would not work as the upper bound is limited by the size of memory
>> area reserved by BIOS for loading IFS test image.
>>
>> Introduce "current_batch" file which accepts a number. Writing a
>> number to the current_batch file would load the test image file by name
>> ff-mm-ss-<xy>.scan, where <xy> is the number written to the
>> "current_batch" file in hex.
>
> Any specific reasoning why the name "current_batch" was chosen? To me, batch seems to suggest multiple or a group of files. But in reality only one test file is loaded at a time.
There would be a batch of test files (6 in the e.g described in the commit message). At a time you can load one by writing its suffix into "current_batch". So "current_batch" was more like a short form for "current_file_in_the_batch_of_files"
>
> Naming can sometimes be quite subjective so it might be useful to get multiple opinions here.
>
> As per my understanding, there is sysfs file called run_test which runs a loaded test. Instead of current_batch how about the name load_test (or maybe current_test)?
Each file would contain multiple tests. All these tests contained within a file would be executed when you write 1 to "runtest".
Given this load_test too would be confusing (more appropriate than "load_test" would be "load_test_file" or "load_file" or "current_file")
>
> load_test - Write a number less than or equal to 0xff to load an IFS test image. (Description as-is from the documentation patch)
>
>
>> * Running tests
>> * -------------
>> @@ -207,6 +217,7 @@ struct ifs_data {
>> int status;
>> u64 scan_details;
>> int cur_batch;
>> + int test_num;
>> };
>> struct ifs_work {
>> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
>> index 5fb7f655c291..1f040837e8eb 100644
>> --- a/drivers/platform/x86/intel/ifs/core.c
>> +++ b/drivers/platform/x86/intel/ifs/core.c
>> @@ -22,6 +22,7 @@ MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
>> static struct ifs_device ifs_device = {
>> .data = {
>> .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
>> + .test_num = 0,
>
> Is this initialization really needed? Wouldn't it default to 0?
>
Not strictly needed when we have a single test
> Maybe if you explain what does test_num refer to it might answer the above?
It is currently used to form the path in ifs_load_firmware() for the test image to be loaded
>
>
>> +static ssize_t current_batch_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct ifs_data *ifsd = ifs_get_data(dev);
>> +
>> + if (!ifsd->loaded)
>> + return sysfs_emit(buf, "%s\n", "none");
>
> Why not:
>
> sysfs_emit(buf, "none\n");
Will change
Jithu
On Fri, Oct 21, 2022 at 01:34:04PM -0700, Jithu Joseph wrote:
> IFS uses 'scan test images' provided by Intel that can be regarded as
> firmware. IFS test image carries microcode header with extended signature
> table.
>
> Expose find_matching_signature() for verifying if the test image
> header or the extended signature table indicate whether an IFS test image
> is fit to run on a system. Add microcode_intel_ prefix to the
> function name.
This doesn't look like the right design to me:
If this is going to be generic CPU-vendor related code which other
facilities like the microcode loader can use, then the prefix should be
intel_<bla>. Just like intel_cpu_signatures_match().
Then that code should either be in a lib-like compilation unit or simply
in arch/x86/kernel/cpu/intel.c. Just like intel_cpu_signatures_match().
Ok?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 11/2/2022 12:03 PM, Borislav Petkov wrote:
> On Fri, Oct 21, 2022 at 01:34:04PM -0700, Jithu Joseph wrote:
>> IFS uses 'scan test images' provided by Intel that can be regarded as
>> firmware. IFS test image carries microcode header with extended signature
>> table.
>>
>> Expose find_matching_signature() for verifying if the test image
>> header or the extended signature table indicate whether an IFS test image
>> is fit to run on a system. Add microcode_intel_ prefix to the
>> function name.
>
> This doesn't look like the right design to me:
>
> If this is going to be generic CPU-vendor related code which other
> facilities like the microcode loader can use, then the prefix should be
> intel_<bla>. Just like intel_cpu_signatures_match().
>
> Then that code should either be in a lib-like compilation unit or simply
> in arch/x86/kernel/cpu/intel.c. Just like intel_cpu_signatures_match().
Will rename the function to intel_find_matching_signature() and move it to
to arch/x86/kernel/cpu/intel.c as you suggest above and add its declaration
to arch/x86/include/asm/cpu.h (where intel_cpu_signatures_match() is defined)
Jithu
On 11/1/2022 3:59 PM, Sohil Mehta wrote:
> On 11/1/2022 3:48 PM, Joseph, Jithu wrote:
>> The testname would be inferred based the sysfs file context (the<N> in /sys/devices/virtual/misc/intel_ifs_<N>/current_batch) from which the operation is triggered.
>>
>> Meaning if the user writes to /sys/devices/virtual/misc/intel_ifs_0/current_batch it would look for ff-mm-ss.scan and if they write to
>> /sys/devices/virtual/misc/intel_ifs_2/current_batch it would look for ff-mm-ss.<test_type_2>
>>
>
> So intel_ifs_<N> corresponds to <test_type> i.e. intel_ifs_0 maps to .scan
>
> Is the N to test_type mapping visible to the user somewhere? If not, how would the user infer that?
>
> Would it be useful to name the misc device as intel_ifs_<test_type>?
>
There is a mapping based on the device number, for e.g if the user tries to use the below sysfs file for loading
/sys/devices/virtual/misc/intel_ifs_<N>/current_batch
The driver will look for the below test image for loading
/lib/firmware/intel/ifs_<N>/ff-mm-ss-02x.<test_type>
The number<N> is same in both the above path.
Prior to issuing the load command, user has to simply copy the supplied test image to the corresponding /lib/firmware path
Jithu
On 11/2/2022 3:10 PM, Joseph, Jithu wrote:
>
> There is a mapping based on the device number, for e.g if the user tries to use the below sysfs file for loading
> /sys/devices/virtual/misc/intel_ifs_<N>/current_batch
>
> The driver will look for the below test image for loading
> /lib/firmware/intel/ifs_<N>/ff-mm-ss-02x.<test_type>
>
> The number<N> is same in both the above path.
I guess that is OK. Adding the test name to the device path could
potentially make the interface more restrictive.
Sohil
On 11/1/2022 4:27 PM, Joseph, Jithu wrote:
> Each file would contain multiple tests. All these tests contained within a file would be executed when you write 1 to "runtest".
> Given this load_test too would be confusing (more appropriate than "load_test" would be "load_test_file" or "load_file" or "current_file")
>
Yeah, not sure if any of them are more suitable in that case. Maybe you
can leave it as is until someone suggests a better name or has a strong
preference for one of the above.
(Probably you can add "load_batch" to mix as well.)
Sohil
Hi Jithu,
On 10/21/2022 1:33 PM, Jithu Joseph wrote:
> This series makes the driver aware of multiple scan test image files,
> modifies IFS test image file headers to make it fully compatible
> with microcode headers and adds a few other bug fixes and changes.
>
I am done with my review comments. In general, the patches look OK to me
apart from the minor changes that we discussed.
(If some of my suggestions seem out of scope for this series then maybe
you can follow-up with a separate patch later to avoid complicating this
series.)
Thanks,
Sohil
On Fri, Oct 21, 2022 at 01:34:06PM -0700, Jithu Joseph wrote:
> IFS test image carries the same microcode header as regular Intel
> microcode blobs. Microcode blobs use header version of 1,
> whereas IFS test images will use header version of 2.
>
> microcode_sanity_check() can be used by IFS driver to perform
> sanity check of the IFS test images too.
So I'm very sceptical about such reuse.
The moment we decide to change something in the microcode loader, we're
going to have to
* test the IFS driver too
* and I suspect we won't even be able to because we'd probably need
special hardware and those special blobs which you probably don't
distribute freely.
And yes, right now that function should be doing the SDM-sanctioned
dance about verifying the table and thus should also be generic but
judging from past experience, things do get different in time and
implementations do get changed so even if it is a trivial change to
microcode_sanity_check(), we would still need to test the IFS driver
too.
So I'm wondering if it wouldn't be simply easier on everyone involved if
you just copy the bits you need into your driver and use them there as
you wish.
Then the testing burden won't be an issue and there won't be any
potential cross-breakages.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Oct 21, 2022 at 01:34:07PM -0700, Jithu Joseph wrote:
> From: Ashok Raj <[email protected]>
>
> Intel has made extensions to the microcode file format so that it
> can also be used for In Field Scan.
And this basically confirms what I just said in my previous mail: you
want to do IFS-specific changes already.
So please copy all code into your driver and reuse as you like so that
there are no cross-breakages.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Nov 03, 2022 at 12:33:50PM +0100, Borislav Petkov wrote:
> On Fri, Oct 21, 2022 at 01:34:06PM -0700, Jithu Joseph wrote:
> > IFS test image carries the same microcode header as regular Intel
> > microcode blobs. Microcode blobs use header version of 1,
> > whereas IFS test images will use header version of 2.
> >
> > microcode_sanity_check() can be used by IFS driver to perform
> > sanity check of the IFS test images too.
>
> So I'm very sceptical about such reuse.
The generic parts of the microcode is completely common between IFS and CPU
microcode. The parts that differ are in the meta-data.
CPU microcode has stayed the same for over 15 years roughly. Changing
formats isn't something that's done lightly especially since there is cost
of enabling in all OS's.
Both are in the same class of microcode, consumed by the CPU. Inside Intel
the format change is also pretty strict that they have several levels of
approval within the HW teams and also needs review and approval from SW
teams.
What probably is best is that we should document that the external formats
are shared in the header and probably in the shared routines. Changing
names as you suggested to be more generic is in the right track.
> Then the testing burden won't be an issue and there won't be any
> potential cross-breakages.
Even if its only a few hundred lines of code, it seems odd to simply
duplicate for the sake of fear of testing.
On Thu, Nov 03, 2022 at 12:25:32PM -0700, Ashok Raj wrote:
> CPU microcode has stayed the same for over 15 years roughly. Changing
> formats isn't something that's done lightly especially since there is cost
> of enabling in all OS's.
Yeah, because it has been that way for the last 15 years and that means
it'll not change in the future either. Yeah, right.
> Even if its only a few hundred lines of code, it seems odd to simply
> duplicate for the sake of fear of testing.
I don't think I ever said "fear" but rather that it would be real hard
to test. But it's not like you're reading my mails properly.
But don't worry about it. We'll change the microcode loader and if we
break the IFS thing in the process, then, oh well, sh*t happens.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 11/3/2022 4:33 AM, Borislav Petkov wrote:
> The moment we decide to change something in the microcode loader, we're
> going to have to
>
> * test the IFS driver too
>
> * and I suspect we won't even be able to because we'd probably need
> special hardware and those special blobs which you probably don't
> distribute freely.
We hear your concern that IFS test images are not yet publicly available for testing.
We are working on it and it should be available in the near future.
>
> And yes, right now that function should be doing the SDM-sanctioned
> dance about verifying the table and thus should also be generic but
> judging from past experience, things do get different in time and
> implementations do get changed so even if it is a trivial change to
> microcode_sanity_check(), we would still need to test the IFS driver
> too.
>
> So I'm wondering if it wouldn't be simply easier on everyone involved if
> you just copy the bits you need into your driver and use them there as
> you wish.
>
> Then the testing burden won't be an issue and there won't be any
> potential cross-breakages.
>
We further hear your concern of potential cross-breakages due to IFS and
Intel Microcode update driver sharing common functionality (like find_matching_signature()
and microcode_sanity_check()). Within Intel, microcode and IFS folks do work closely,
limiting the chances of this being introduced from Intel side.
If these doesn’t alleviate your concern, I will post v2 without exporting
the aforementioned functions and implementing them separately in IFS driver as you suggested.
Jithu
On Thu, Nov 03, 2022 at 11:15:12PM -0700, Joseph, Jithu wrote:
> If these doesn’t alleviate your concern, I will post v2 without
> exporting the aforementioned functions and implementing them
> separately in IFS driver as you suggested.
So tglx persuaded me yesterday that we should do code sharing after
all, so that all blob loading remains consistent. So let's try the
cpu/intel.c thing and see what breaks, how and when.
As to patch 8, that metadata checking should not be part of
microcode_intel_sanity_check() but a separate function. Along with
microcode_intel_find_meta_data() - all those should go into the IFS
thing.
When microcode loading ends up really needing metadata, *then*
that functionality should be lifted into a more fitting place like
cpu/intel.c
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 11/4/2022 3:50 AM, Borislav Petkov wrote:
> As to patch 8, that metadata checking should not be part of
> microcode_intel_sanity_check() but a separate function. Along with
> microcode_intel_find_meta_data() - all those should go into the IFS
> thing.
>
> When microcode loading ends up really needing metadata, *then*
> that functionality should be lifted into a more fitting place like
> cpu/intel.c
Thanks for the pointer. I will move the microcode_intel_find_meta_data()
function into IFS driver and also drop the metadata checking from the exported
microcode_intel_sanity_check()
Wanted to check with you, if it is okay to rename the first "reserved" field
in microcode_header_intel to "metasize" today (as shown in the diff below)
or would you prefer to do that too at a later point ? (doing so today will help to
avoid redefining an IFS specific header struct, with this as the only change )
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index a4d2ed43193c..bec23c11ca52 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -14,7 +14,8 @@ struct microcode_header_intel {
unsigned int pf;
unsigned int datasize;
unsigned int totalsize;
- unsigned int reserved[3];
+ unsigned int metasize;
+ unsigned int reserved[2];
};
Jithu
On Fri, Nov 04, 2022 at 03:02:42PM -0700, Joseph, Jithu wrote:
> Wanted to check with you, if it is okay to rename the first "reserved"
> field in microcode_header_intel to "metasize" today (as shown in the
> diff below) or would you prefer to do that too at a later point ?
> (doing so today will help to avoid redefining an IFS specific header
> struct, with this as the only change )
No objections - it is not used by the microcode loader anyway.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi,
On 10/21/22 22:33, Jithu Joseph wrote:
> Initial implementation of IFS driver assumed a single IFS test image
> file with a fixed name.
>
> Subsequently, it became evident that supporting more than one
> test image file is needed to provide more comprehensive
> test coverage. (Test coverage in this scenario refers to testing
> more transistors in the core to identify faults).
>
> This series makes the driver aware of multiple scan test image files,
> modifies IFS test image file headers to make it fully compatible
> with microcode headers and adds a few other bug fixes and changes.
>
> Patch organization:
> Patches 1, 2, and 3: bug fixes
> Patch 4: Removes image loading during init path
> Patch 5, 6 and 7: exports a couple of microcode/intel.c functions
> for use by driver
> Patch 8: Adds Meta-data support in microcode file
>
> Rest of them are IFS driver changes
> Patches 9 and 10: IFS header format changes to make it fully compatible
> to intel microcode header format
> Patches 11, 12 and 13: native support for multiple scan test image files
> Patch 14: reverts the broken flag
Thank you for your work on this. Due to personal circumstances I have
been unable to do any pdx86 patch review for the last 2 weeks.
I see that there have been lots of comments on this series already please
send a new version 2 addressing all the existing comments. Then I will
review version 2 once posted.
Regards,
Hans
>
> Ashok Raj (1):
> x86/microcode/intel: Meta-data support in microcode file
>
> Jithu Joseph (13):
> platform/x86/intel/ifs: Remove unused selection
> platform/x86/intel/ifs: Propagate load failure error code
> platform/x86/intel/ifs: return a more appropriate Error code
> platform/x86/intel/ifs: Remove image loading during init
> x86/microcode/intel: Expose find_matching_signature() for IFS
> x86/microcode/intel: Use appropriate type in microcode_sanity_check()
> x86/microcode/intel: Expose microcode_sanity_check()
> platform/x86/intel/ifs: Use generic microcode headers and functions
> platform/x86/intel/ifs: Add metadata validation
> platform/x86/intel/ifs: Remove reload sysfs entry
> platform/x86/intel/ifs: Add current_batch sysfs entry
> Documentation/ABI: Update IFS ABI doc
> Revert "platform/x86/intel/ifs: Mark as BROKEN"
>
> arch/x86/include/asm/microcode_intel.h | 25 ++-
> drivers/platform/x86/intel/ifs/ifs.h | 27 ++-
> arch/x86/kernel/cpu/microcode/intel.c | 81 ++++++--
> drivers/platform/x86/intel/ifs/core.c | 7 +-
> drivers/platform/x86/intel/ifs/load.c | 174 +++++++++---------
> drivers/platform/x86/intel/ifs/runtest.c | 10 +-
> drivers/platform/x86/intel/ifs/sysfs.c | 41 +++--
> .../ABI/testing/sysfs-platform-intel-ifs | 30 +--
> drivers/platform/x86/intel/ifs/Kconfig | 4 -
> 9 files changed, 243 insertions(+), 156 deletions(-)
>
>
> base-commit: f76349cf41451c5c42a99f18a9163377e4b364ff
Changes in v2
- Rebased ontop of v6.1-rc4
Boris
- Moved exported functions (microcode_sanity_check(),
find_matching_signature ) from microcode/intel.c to cpu/intel.c
(patch4,6)
- Removed microcode metadata specific code changes to
microcode_sanity_check() (patch6)
- Moved find_meta_data() from common to IFS driver (Patch 8)
Sohil
- Dropped portions of Patch2 from v1 and folded remaining to Patch12
- Rewording multiple commits
- Avoid meta prefix in fields of metadata_header (patch8)
- Defining MICROCODE_HEADER_VER* into common location (patch6, 9)
- Elaborating error messages w.r.t processor flags (patch9)
- Changed sysfs_emit() parameter (patch 11)
v1 patches available from
Link: https://lore.kernel.org/lkml/[email protected]/
Initial implementation of IFS driver assumed a single IFS test image
file with a fixed name.
Subsequently, it became evident that supporting more than one
test image file is needed to provide more comprehensive
test coverage. (Test coverage in this scenario refers to testing
more transistors in the core to identify faults).
This series makes the driver aware of multiple scan test image files,
modifies IFS test image file headers to make it fully compatible
with microcode headers and adds a few other bug fixes and changes.
Patch organization:
Patches 1, and 2: bug fixes
Patch 3: Removes image loading during init path
Patch 4, 5 and 6: exports a couple of microcode/intel.c functions
for use by driver
Rest of them are IFS driver changes
Patches 9: IFS header format changes to make it fully compatible
to intel microcode header format
Patch 7,8 and 10: microcode/IFS metadata section related
Patches 11, 12 and 13: native support for multiple scan test image files
Patch 14: reverts the broken flag
Ashok Raj (1):
platform/x86/intel/ifs: Add metadata support
Jithu Joseph (13):
platform/x86/intel/ifs: Remove unused selection
platform/x86/intel/ifs: return a more appropriate Error code
platform/x86/intel/ifs: Remove image loading during init
x86/microcode/intel: Expose find_matching_signature() for IFS
x86/microcode/intel: Use appropriate type in microcode_sanity_check()
x86/microcode/intel: Expose microcode_sanity_check()
x86/microcode/intel: Use a reserved field for metasize
platform/x86/intel/ifs: Use generic microcode headers and functions
platform/x86/intel/ifs: Add metadata validation
platform/x86/intel/ifs: Remove reload sysfs entry
platform/x86/intel/ifs: Add current_batch sysfs entry
Documentation/ABI: Update IFS ABI doc
Revert "platform/x86/intel/ifs: Mark as BROKEN"
arch/x86/include/asm/cpu.h | 2 +
arch/x86/include/asm/microcode_intel.h | 5 +-
drivers/platform/x86/intel/ifs/ifs.h | 27 ++-
arch/x86/kernel/cpu/intel.c | 129 +++++++++++
arch/x86/kernel/cpu/microcode/intel.c | 146 +------------
drivers/platform/x86/intel/ifs/core.c | 7 +-
drivers/platform/x86/intel/ifs/load.c | 205 ++++++++++--------
drivers/platform/x86/intel/ifs/runtest.c | 10 +-
drivers/platform/x86/intel/ifs/sysfs.c | 41 ++--
.../ABI/testing/sysfs-platform-intel-ifs | 30 +--
drivers/platform/x86/intel/ifs/Kconfig | 4 -
11 files changed, 330 insertions(+), 276 deletions(-)
base-commit: f0c4d9fc9cc9462659728d168387191387e903cc
--
2.25.1
Intel is using microcode file format for IFS test images too.
IFS test images use one of the existing reserved fields in microcode
header to indicate the size of the region in the file allocated for
metadata structures.
In prepration for this, rename first of the existing reserved fields
in microcode header to metasize. In subsequent patches IFS specific
code will make use of this field while parsing IFS images.
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Ashok Raj <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
arch/x86/include/asm/microcode_intel.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 6626744c577b..0ff4545f72d2 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -14,7 +14,8 @@ struct microcode_header_intel {
unsigned int pf;
unsigned int datasize;
unsigned int totalsize;
- unsigned int reserved[3];
+ unsigned int metasize;
+ unsigned int reserved[2];
};
struct microcode_intel {
--
2.25.1
IFS test image is unnecessarily loaded during driver initialization.
Drop image loading during ifs_init() and improve module load time.
With this change, user has to load one when starting the tests.
As a consequence, make ifs_sem static as it is only used within sysfs.c
Reviewed-by: Tony Luck <[email protected]>
Suggested-by: Hans de Goede <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/ifs.h | 2 --
drivers/platform/x86/intel/ifs/core.c | 6 +-----
drivers/platform/x86/intel/ifs/sysfs.c | 2 +-
3 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 73c8e91cf144..3ff1d9aaeaa9 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -229,6 +229,4 @@ void ifs_load_firmware(struct device *dev);
int do_core_test(int cpu, struct device *dev);
const struct attribute_group **ifs_get_groups(void);
-extern struct semaphore ifs_sem;
-
#endif
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 27204e3d674d..5fb7f655c291 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -51,12 +51,8 @@ static int __init ifs_init(void)
ifs_device.misc.groups = ifs_get_groups();
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);
+ !misc_register(&ifs_device.misc))
return 0;
- }
return -ENODEV;
}
diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
index 37d8380d6fa8..65dd6fea5342 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -13,7 +13,7 @@
* Protects against simultaneous tests on multiple cores, or
* reloading can file while a test is in progress
*/
-DEFINE_SEMAPHORE(ifs_sem);
+static DEFINE_SEMAPHORE(ifs_sem);
/*
* The sysfs interface to check additional details of last test
--
2.25.1
Issues with user interface [1] to load scan test images
has been addressed, so the following can be reverted.
commit c483e7ea10fa ("platform/x86/intel/ifs: Mark as BROKEN")
Link: https://lore.kernel.org/lkml/[email protected]/ [1]
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/Kconfig | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig
index 89152d46deee..3eded966757e 100644
--- a/drivers/platform/x86/intel/ifs/Kconfig
+++ b/drivers/platform/x86/intel/ifs/Kconfig
@@ -1,9 +1,6 @@
config INTEL_IFS
tristate "Intel In Field Scan"
depends on X86 && CPU_SUP_INTEL && 64BIT && SMP
- # Discussion on the list has shown that the sysfs API needs a bit
- # more work, mark this as broken for now
- depends on BROKEN
help
Enable support for the In Field Scan capability in select
CPUs. The capability allows for running low level tests via
--
2.25.1
Initial implementation assumed a single IFS test image file with a
fixed name ff-mm-ss.scan. (where ff, mm, ss refers to family, model
and stepping of the core)
Subsequently, it became evident that supporting more than one
test image file is needed to provide more comprehensive
test coverage. (Test coverage in this scenario refers to testing
more transistors in the core to identify faults)
The other alternative of increasing the size of a single scan test image
file would not work as the upper bound is limited by the size of memory
area reserved by BIOS for loading IFS test image.
Introduce "current_batch" file which accepts a number. Writing a
number to the current_batch file would load the test image file by name
ff-mm-ss-<xy>.scan, where <xy> is the number written to the
"current_batch" file in hex. Range check of the input is done to verify
it not greater than 0xff.
For e.g if the scan test image comprises of 6 files, they would be named
as show below:
06-8f-06-01.scan
06-8f-06-02.scan
06-8f-06-03.scan
06-8f-06-04.scan
06-8f-06-05.scan
06-8f-06-06.scan
And writing 3 to current_batch would result in loading 06-8f-06-03.scan
in the above e.g. The file can also be read to know the currently loaded
file.
And testing a system looks like:
for each scan file
do
load the IFS test image file (write to the batch file)
for each core
do
test the core with this set of tests
done
done
Qualify few error messages with the test image file suffix to
provide better context.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/ifs.h | 23 ++++++++++----
drivers/platform/x86/intel/ifs/core.c | 1 +
drivers/platform/x86/intel/ifs/load.c | 18 +++++++----
drivers/platform/x86/intel/ifs/runtest.c | 10 ++++---
drivers/platform/x86/intel/ifs/sysfs.c | 38 ++++++++++++++++++++++++
5 files changed, 74 insertions(+), 16 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 98ca91bdd5ca..390e508faf57 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -33,13 +33,23 @@
* The driver loads the tests into memory reserved BIOS local to each CPU
* 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::
+ * feedback on the success/failure of these steps.
*
- * # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload
+ * The test files are kept in a fixed location: /lib/firmware/intel/ifs_0/
+ * For e.g if there are 3 test files, they would be named in the following
+ * fashion:
+ * ff-mm-ss-01.scan
+ * ff-mm-ss-02.scan
+ * ff-mm-ss-03.scan
+ * (where ff refers to family, mm indicates model and ss indicates stepping)
*
- * 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
+ * A different testfile can be loaded by writing the numerical portion
+ * (e.g 1, 2 or 3 in the above scenario) into the curent_batch file.
+ * To load ff-mm-ss-02.scan, the following command can be used::
+ *
+ * # echo 2 > /sys/devices/virtual/misc/intel_ifs_0/current_batch
+ *
+ * The above file can also be read to know the currently loaded image.
*
* Running tests
* -------------
@@ -207,6 +217,7 @@ struct ifs_data {
int status;
u64 scan_details;
int cur_batch;
+ int test_num;
};
struct ifs_work {
@@ -227,7 +238,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);
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 5fb7f655c291..1f040837e8eb 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -22,6 +22,7 @@ MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
static struct ifs_device ifs_device = {
.data = {
.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
+ .test_num = 0,
},
.misc = {
.name = "intel_ifs_0",
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index f361fd42a320..c6414958a691 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -256,17 +256,18 @@ static int ifs_image_sanity_check(struct device *dev, const struct microcode_hea
/*
* 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 /lib/firmware/intel/ifs_x/ and named as family-model-stepping-02x.{testname}.
*/
-void ifs_load_firmware(struct device *dev)
+int ifs_load_firmware(struct device *dev)
{
struct ifs_data *ifsd = ifs_get_data(dev);
const struct firmware *fw;
- char scan_path[32];
- int ret;
+ char scan_path[64];
+ int ret = -EINVAL;
- 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);
+ snprintf(scan_path, sizeof(scan_path), "intel/ifs_%d/%02x-%02x-%02x-%02x.scan",
+ ifsd->test_num, boot_cpu_data.x86, boot_cpu_data.x86_model,
+ boot_cpu_data.x86_stepping, ifsd->cur_batch);
ret = request_firmware_direct(&fw, scan_path, dev);
if (ret) {
@@ -282,8 +283,13 @@ void ifs_load_firmware(struct device *dev)
ifs_hash_ptr = (u64)(ifs_header_ptr + 1);
ret = scan_chunks_sanity_check(dev);
+ if (ret)
+ dev_err(dev, "Load failure for batch: %02x\n", ifsd->cur_batch);
+
release:
release_firmware(fw);
done:
ifsd->loaded = (ret == 0);
+
+ return ret;
}
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index b2ca2bb4501f..0bfd8fcdd7e8 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -78,14 +78,16 @@ static void message_not_tested(struct device *dev, int cpu, union ifs_status sta
static void message_fail(struct device *dev, int cpu, union ifs_status status)
{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+
/*
* control_error is set when the microcode runs into a problem
* loading the image from the reserved BIOS memory, or it has
* been corrupted. Reloading the image may fix this issue.
*/
if (status.control_error) {
- dev_err(dev, "CPU(s) %*pbl: could not execute from loaded scan image\n",
- cpumask_pr_args(cpu_smt_mask(cpu)));
+ dev_err(dev, "CPU(s) %*pbl: could not execute from loaded scan image. Batch: %02x version: 0x%x\n",
+ cpumask_pr_args(cpu_smt_mask(cpu)), ifsd->cur_batch, ifsd->loaded_version);
}
/*
@@ -96,8 +98,8 @@ static void message_fail(struct device *dev, int cpu, union ifs_status status)
* the core being tested.
*/
if (status.signature_error) {
- dev_err(dev, "CPU(s) %*pbl: test signature incorrect.\n",
- cpumask_pr_args(cpu_smt_mask(cpu)));
+ dev_err(dev, "CPU(s) %*pbl: test signature incorrect. Batch: %02x version: 0x%x\n",
+ cpumask_pr_args(cpu_smt_mask(cpu)), ifsd->cur_batch, ifsd->loaded_version);
}
}
diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
index e077910c5d28..d2eeeb04d760 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -87,6 +87,43 @@ static ssize_t run_test_store(struct device *dev,
static DEVICE_ATTR_WO(run_test);
+static ssize_t current_batch_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+ int cur_batch;
+ int rc;
+
+ rc = kstrtouint(buf, 0, &cur_batch);
+ if (rc < 0 || cur_batch > 0xff)
+ return -EINVAL;
+
+ if (down_interruptible(&ifs_sem))
+ return -EINTR;
+
+ ifsd->cur_batch = cur_batch;
+
+ rc = ifs_load_firmware(dev);
+
+ up(&ifs_sem);
+
+ return (rc == 0) ? count : rc;
+}
+
+static ssize_t current_batch_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+
+ if (!ifsd->loaded)
+ return sysfs_emit(buf, "none\n");
+ else
+ return sysfs_emit(buf, "0x%02x\n", ifsd->cur_batch);
+}
+
+static DEVICE_ATTR_RW(current_batch);
+
/*
* Display currently loaded IFS image version.
*/
@@ -108,6 +145,7 @@ static struct attribute *plat_ifs_attrs[] = {
&dev_attr_details.attr,
&dev_attr_status.attr,
&dev_attr_run_test.attr,
+ &dev_attr_current_batch.attr,
&dev_attr_image_version.attr,
NULL
};
--
2.25.1
IFS uses 'scan test images' provided by Intel that can be regarded as
firmware. IFS test image carries microcode header with extended signature
table.
Expose find_matching_signature() for verifying if the test image
header or the extended signature table indicate whether an IFS test image
is fit to run on a system. Move the function to cpu/intel.c and
add intel_ prefix to the function name.
No functional change
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Ashok Raj <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
arch/x86/include/asm/cpu.h | 1 +
arch/x86/kernel/cpu/intel.c | 29 ++++++++++++++++++
arch/x86/kernel/cpu/microcode/intel.c | 44 +++++----------------------
3 files changed, 38 insertions(+), 36 deletions(-)
diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index b472ef76826a..e853440b5c65 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -95,5 +95,6 @@ static inline bool intel_cpu_signatures_match(unsigned int s1, unsigned int p1,
}
extern u64 x86_read_arch_cap_msr(void);
+int intel_find_matching_signature(void *mc, unsigned int csig, int cpf);
#endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 2d7ea5480ec3..b6f9210fb31a 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -216,6 +216,35 @@ int intel_cpu_collect_info(struct ucode_cpu_info *uci)
}
EXPORT_SYMBOL_GPL(intel_cpu_collect_info);
+/*
+ * Returns 1 if update has been found, 0 otherwise.
+ */
+int intel_find_matching_signature(void *mc, unsigned int csig, int cpf)
+{
+ struct microcode_header_intel *mc_hdr = mc;
+ struct extended_sigtable *ext_hdr;
+ struct extended_signature *ext_sig;
+ int i;
+
+ if (intel_cpu_signatures_match(csig, cpf, mc_hdr->sig, mc_hdr->pf))
+ return 1;
+
+ /* Look for ext. headers: */
+ if (get_totalsize(mc_hdr) <= get_datasize(mc_hdr) + MC_HEADER_SIZE)
+ return 0;
+
+ ext_hdr = mc + get_datasize(mc_hdr) + MC_HEADER_SIZE;
+ ext_sig = (void *)ext_hdr + EXT_HEADER_SIZE;
+
+ for (i = 0; i < ext_hdr->count; i++) {
+ if (intel_cpu_signatures_match(csig, cpf, ext_sig->sig, ext_sig->pf))
+ return 1;
+ ext_sig++;
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(intel_find_matching_signature);
+
static void early_init_intel(struct cpuinfo_x86 *c)
{
u64 misc_enable;
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 1fcbd671f1df..4e611a708718 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -45,34 +45,6 @@ static struct microcode_intel *intel_ucode_patch;
/* last level cache size per core */
static int llc_size_per_core;
-/*
- * Returns 1 if update has been found, 0 otherwise.
- */
-static int find_matching_signature(void *mc, unsigned int csig, int cpf)
-{
- struct microcode_header_intel *mc_hdr = mc;
- struct extended_sigtable *ext_hdr;
- struct extended_signature *ext_sig;
- int i;
-
- if (intel_cpu_signatures_match(csig, cpf, mc_hdr->sig, mc_hdr->pf))
- return 1;
-
- /* Look for ext. headers: */
- if (get_totalsize(mc_hdr) <= get_datasize(mc_hdr) + MC_HEADER_SIZE)
- return 0;
-
- ext_hdr = mc + get_datasize(mc_hdr) + MC_HEADER_SIZE;
- ext_sig = (void *)ext_hdr + EXT_HEADER_SIZE;
-
- for (i = 0; i < ext_hdr->count; i++) {
- if (intel_cpu_signatures_match(csig, cpf, ext_sig->sig, ext_sig->pf))
- return 1;
- ext_sig++;
- }
- return 0;
-}
-
/*
* Returns 1 if update has been found, 0 otherwise.
*/
@@ -83,7 +55,7 @@ static int has_newer_microcode(void *mc, unsigned int csig, int cpf, int new_rev
if (mc_hdr->rev <= new_rev)
return 0;
- return find_matching_signature(mc, csig, cpf);
+ return intel_find_matching_signature(mc, csig, cpf);
}
static struct ucode_patch *memdup_patch(void *data, unsigned int size)
@@ -117,7 +89,7 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
sig = mc_saved_hdr->sig;
pf = mc_saved_hdr->pf;
- if (find_matching_signature(data, sig, pf)) {
+ if (intel_find_matching_signature(data, sig, pf)) {
prev_found = true;
if (mc_hdr->rev <= mc_saved_hdr->rev)
@@ -149,7 +121,7 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
if (!p)
return;
- if (!find_matching_signature(p->data, uci->cpu_sig.sig, uci->cpu_sig.pf))
+ if (!intel_find_matching_signature(p->data, uci->cpu_sig.sig, uci->cpu_sig.pf))
return;
/*
@@ -286,8 +258,8 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
size -= mc_size;
- if (!find_matching_signature(data, uci->cpu_sig.sig,
- uci->cpu_sig.pf)) {
+ if (!intel_find_matching_signature(data, uci->cpu_sig.sig,
+ uci->cpu_sig.pf)) {
data += mc_size;
continue;
}
@@ -652,9 +624,9 @@ static struct microcode_intel *find_patch(struct ucode_cpu_info *uci)
if (phdr->rev <= uci->cpu_sig.rev)
continue;
- if (!find_matching_signature(phdr,
- uci->cpu_sig.sig,
- uci->cpu_sig.pf))
+ if (!intel_find_matching_signature(phdr,
+ uci->cpu_sig.sig,
+ uci->cpu_sig.pf))
continue;
return iter->data;
--
2.25.1
Reload sysfs entry will be replaced by current_batch, drop it.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/sysfs.c | 29 --------------------------
1 file changed, 29 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
index 65dd6fea5342..e077910c5d28 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -87,34 +87,6 @@ static ssize_t run_test_store(struct device *dev,
static DEVICE_ATTR_WO(run_test);
-/*
- * Reload the IFS image. When user wants to install new IFS image
- */
-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;
-
- if (down_interruptible(&ifs_sem))
- return -EINTR;
-
- ifs_load_firmware(dev);
-
- up(&ifs_sem);
-
- return ifsd->loaded ? count : -ENODEV;
-}
-
-static DEVICE_ATTR_WO(reload);
-
/*
* Display currently loaded IFS image version.
*/
@@ -136,7 +108,6 @@ static struct attribute *plat_ifs_attrs[] = {
&dev_attr_details.attr,
&dev_attr_status.attr,
&dev_attr_run_test.attr,
- &dev_attr_reload.attr,
&dev_attr_image_version.attr,
NULL
};
--
2.25.1
CONFIG_INTEL_IFS_DEVICE is not used anywhere. The selection in
Kconfig is therefore pointless. Delete it.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig
index c341a27cc1a3..89152d46deee 100644
--- a/drivers/platform/x86/intel/ifs/Kconfig
+++ b/drivers/platform/x86/intel/ifs/Kconfig
@@ -4,7 +4,6 @@ config INTEL_IFS
# Discussion on the list has shown that the sysfs API needs a bit
# more work, mark this as broken for now
depends on BROKEN
- select INTEL_IFS_DEVICE
help
Enable support for the In Field Scan capability in select
CPUs. The capability allows for running low level tests via
--
2.25.1
On 11/7/2022 1:24 AM, Hans de Goede wrote:
> I see that there have been lots of comments on this series already please
> send a new version 2 addressing all the existing comments. Then I will
> review version 2 once posted.
Thanks for getting back Hans. I just posted updated v2 version of the series.
Jithu
scan_chunks_sanity_check() returns -ENOMEM if it encounters
an error while copying IFS test image from memory to Secure Memory.
Return -EIO in this scenario, as it is more appropriate.
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/load.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index d056617ddc85..89ce265887ea 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -157,8 +157,10 @@ static int scan_chunks_sanity_check(struct device *dev)
INIT_WORK(&local_work.w, copy_hashes_authenticate_chunks);
schedule_work_on(cpu, &local_work.w);
wait_for_completion(&ifs_done);
- if (ifsd->loading_error)
+ if (ifsd->loading_error) {
+ ret = -EIO;
goto out;
+ }
package_authenticated[curr_pkg] = 1;
}
ret = 0;
--
2.25.1
On 11/7/2022 2:53 PM, Jithu Joseph wrote:
> CONFIG_INTEL_IFS_DEVICE is not used anywhere. The selection in
> Kconfig is therefore pointless. Delete it.
>
> Reviewed-by: Tony Luck <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Reviewed-by: Sohil Mehta <[email protected]>
A nit:
s/return/Return
It might be better to just say:
platform/x86/intel/ifs: Return appropriate error code
On 11/7/2022 2:53 PM, Jithu Joseph wrote:
> scan_chunks_sanity_check() returns -ENOMEM if it encounters
> an error while copying IFS test image from memory to Secure Memory.
>
> Return -EIO in this scenario, as it is more appropriate.
>
> Reviewed-by: Tony Luck <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Reviewed-by: Sohil Mehta <[email protected]>
On 11/7/2022 2:53 PM, Jithu Joseph wrote:
> IFS test image is unnecessarily loaded during driver initialization.
> Drop image loading during ifs_init() and improve module load time.
> With this change, user has to load one when starting the tests.
>
> As a consequence, make ifs_sem static as it is only used within sysfs.c
>
> Reviewed-by: Tony Luck <[email protected]>
> Suggested-by: Hans de Goede <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Reviewed-by: Sohil Mehta <[email protected]>
On 11/7/2022 2:53 PM, Jithu Joseph wrote:
> IFS uses 'scan test images' provided by Intel that can be regarded as
> firmware. IFS test image carries microcode header with extended signature
> table.
>
> Expose find_matching_signature() for verifying if the test image
> header or the extended signature table indicate whether an IFS test image
> is fit to run on a system. Move the function to cpu/intel.c and
> add intel_ prefix to the function name.
>
> No functional change
>
> Reviewed-by: Tony Luck <[email protected]>
> Reviewed-by: Ashok Raj <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Reviewed-by: Sohil Mehta <[email protected]>
On 11/7/2022 2:53 PM, Jithu Joseph wrote:
> Intel is using microcode file format for IFS test images too.
>
> IFS test images use one of the existing reserved fields in microcode
> header to indicate the size of the region in the file allocated for
> metadata structures.
>
> In prepration for this, rename first of the existing reserved fields
> in microcode header to metasize. In subsequent patches IFS specific
Nit:
s/In subsequent patches/Upcoming
> code will make use of this field while parsing IFS images.
>
> Reviewed-by: Tony Luck <[email protected]>
> Reviewed-by: Ashok Raj <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Reviewed-by: Sohil Mehta <[email protected]>
On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> The data portion of IFS test image file contains a meta-data
> structure in addition to test data and hashes.
>
> Introduce the layout of this meta_data structure and validate
> the sanity of certain fields of the new-image before loading.
>
> Tweak references to IFS test image chunks to reflect the updated
> layout of the test image.
>
Can you provide some more information on the updated layout of the
metadata structure?
Some parts of validate_ifs_metadata() like the ifs_test_image_ptr
calculation would be easier to follow with that.
On 11/7/2022 2:53 PM, Jithu Joseph wrote:
> Reload sysfs entry will be replaced by current_batch, drop it.
>
> Reviewed-by: Tony Luck <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Reviewed-by: Sohil Mehta <[email protected]>
On 11/7/2022 2:53 PM, Jithu Joseph wrote:
> Issues with user interface [1] to load scan test images
> has been addressed, so the following can be reverted.
> commit c483e7ea10fa ("platform/x86/intel/ifs: Mark as BROKEN")
>
> Link: https://lore.kernel.org/lkml/[email protected]/ [1]
>
> Reviewed-by: Tony Luck <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Reviewed-by: Sohil Mehta <[email protected]>
On 11/7/2022 2:53 PM, Jithu Joseph wrote:
> Reviewed-by: Tony Luck <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
> ---
> drivers/platform/x86/intel/ifs/ifs.h | 23 ++++++++++----
> drivers/platform/x86/intel/ifs/core.c | 1 +
> drivers/platform/x86/intel/ifs/load.c | 18 +++++++----
> drivers/platform/x86/intel/ifs/runtest.c | 10 ++++---
> drivers/platform/x86/intel/ifs/sysfs.c | 38 ++++++++++++++++++++++++
> 5 files changed, 74 insertions(+), 16 deletions(-)
>
Reviewed-by: Sohil Mehta <[email protected]>
A Nit below.
> +static ssize_t current_batch_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ifs_data *ifsd = ifs_get_data(dev);
> + int cur_batch;
> + int rc;
> +
> + rc = kstrtouint(buf, 0, &cur_batch);
Should this be kstrtoint() since cur_batch is defined as int? Or maybe
it might be better to define cur_batch as unsigned int to avoid the
negative input?
> + if (rc < 0 || cur_batch > 0xff)
> + return -EINVAL;
> +
> + if (down_interruptible(&ifs_sem))
> + return -EINTR;
> +
> + ifsd->cur_batch = cur_batch;
> +
Sohil
On Mon, Nov 07, 2022 at 02:53:09PM -0800, Jithu Joseph wrote:
> Changes in v2
> - Rebased ontop of v6.1-rc4
> Boris
> - Moved exported functions (microcode_sanity_check(),
> find_matching_signature ) from microcode/intel.c to cpu/intel.c
> (patch4,6)
> - Removed microcode metadata specific code changes to
> microcode_sanity_check() (patch6)
> - Moved find_meta_data() from common to IFS driver (Patch 8)
What's the upstreaming plan here - I'm assuming I should take the
microcode patches through the tip tree?
Or should I take the whole thing through tip so that there's no
confusion and having to sync and share branches between trees?
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi,
On 11/7/22 23:53, Jithu Joseph wrote:
> scan_chunks_sanity_check() returns -ENOMEM if it encounters
> an error while copying IFS test image from memory to Secure Memory.
>
> Return -EIO in this scenario, as it is more appropriate.
>
> Reviewed-by: Tony Luck <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <[email protected]>
Regards,
Hans
> ---
> drivers/platform/x86/intel/ifs/load.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index d056617ddc85..89ce265887ea 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -157,8 +157,10 @@ static int scan_chunks_sanity_check(struct device *dev)
> INIT_WORK(&local_work.w, copy_hashes_authenticate_chunks);
> schedule_work_on(cpu, &local_work.w);
> wait_for_completion(&ifs_done);
> - if (ifsd->loading_error)
> + if (ifsd->loading_error) {
> + ret = -EIO;
> goto out;
> + }
> package_authenticated[curr_pkg] = 1;
> }
> ret = 0;
Hi,
On 11/7/22 23:53, Jithu Joseph wrote:
> IFS test image is unnecessarily loaded during driver initialization.
> Drop image loading during ifs_init() and improve module load time.
> With this change, user has to load one when starting the tests.
>
> As a consequence, make ifs_sem static as it is only used within sysfs.c
>
> Reviewed-by: Tony Luck <[email protected]>
> Suggested-by: Hans de Goede <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <[email protected]>
Regards,
Hans
> ---
> drivers/platform/x86/intel/ifs/ifs.h | 2 --
> drivers/platform/x86/intel/ifs/core.c | 6 +-----
> drivers/platform/x86/intel/ifs/sysfs.c | 2 +-
> 3 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 73c8e91cf144..3ff1d9aaeaa9 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -229,6 +229,4 @@ void ifs_load_firmware(struct device *dev);
> int do_core_test(int cpu, struct device *dev);
> const struct attribute_group **ifs_get_groups(void);
>
> -extern struct semaphore ifs_sem;
> -
> #endif
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 27204e3d674d..5fb7f655c291 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -51,12 +51,8 @@ static int __init ifs_init(void)
> ifs_device.misc.groups = ifs_get_groups();
>
> 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);
> + !misc_register(&ifs_device.misc))
> return 0;
> - }
>
> return -ENODEV;
> }
> diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
> index 37d8380d6fa8..65dd6fea5342 100644
> --- a/drivers/platform/x86/intel/ifs/sysfs.c
> +++ b/drivers/platform/x86/intel/ifs/sysfs.c
> @@ -13,7 +13,7 @@
> * Protects against simultaneous tests on multiple cores, or
> * reloading can file while a test is in progress
> */
> -DEFINE_SEMAPHORE(ifs_sem);
> +static DEFINE_SEMAPHORE(ifs_sem);
>
> /*
> * The sysfs interface to check additional details of last test
Hi,
On 11/7/22 23:53, Jithu Joseph wrote:
> Issues with user interface [1] to load scan test images
> has been addressed, so the following can be reverted.
> commit c483e7ea10fa ("platform/x86/intel/ifs: Mark as BROKEN")
>
> Link: https://lore.kernel.org/lkml/[email protected]/ [1]
>
> Reviewed-by: Tony Luck <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <[email protected]>
Regards,
Hans
> ---
> drivers/platform/x86/intel/ifs/Kconfig | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig
> index 89152d46deee..3eded966757e 100644
> --- a/drivers/platform/x86/intel/ifs/Kconfig
> +++ b/drivers/platform/x86/intel/ifs/Kconfig
> @@ -1,9 +1,6 @@
> config INTEL_IFS
> tristate "Intel In Field Scan"
> depends on X86 && CPU_SUP_INTEL && 64BIT && SMP
> - # Discussion on the list has shown that the sysfs API needs a bit
> - # more work, mark this as broken for now
> - depends on BROKEN
> help
> Enable support for the In Field Scan capability in select
> CPUs. The capability allows for running low level tests via
Hi,
On 11/7/22 23:53, Jithu Joseph wrote:
> Reload sysfs entry will be replaced by current_batch, drop it.
>
> Reviewed-by: Tony Luck <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <[email protected]>
Regards,
Hans
> ---
> drivers/platform/x86/intel/ifs/sysfs.c | 29 --------------------------
> 1 file changed, 29 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
> index 65dd6fea5342..e077910c5d28 100644
> --- a/drivers/platform/x86/intel/ifs/sysfs.c
> +++ b/drivers/platform/x86/intel/ifs/sysfs.c
> @@ -87,34 +87,6 @@ static ssize_t run_test_store(struct device *dev,
>
> static DEVICE_ATTR_WO(run_test);
>
> -/*
> - * Reload the IFS image. When user wants to install new IFS image
> - */
> -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;
> -
> - if (down_interruptible(&ifs_sem))
> - return -EINTR;
> -
> - ifs_load_firmware(dev);
> -
> - up(&ifs_sem);
> -
> - return ifsd->loaded ? count : -ENODEV;
> -}
> -
> -static DEVICE_ATTR_WO(reload);
> -
> /*
> * Display currently loaded IFS image version.
> */
> @@ -136,7 +108,6 @@ static struct attribute *plat_ifs_attrs[] = {
> &dev_attr_details.attr,
> &dev_attr_status.attr,
> &dev_attr_run_test.attr,
> - &dev_attr_reload.attr,
> &dev_attr_image_version.attr,
> NULL
> };
Hi,
On 11/7/22 23:53, Jithu Joseph wrote:
> Initial implementation assumed a single IFS test image file with a
> fixed name ff-mm-ss.scan. (where ff, mm, ss refers to family, model
> and stepping of the core)
>
> Subsequently, it became evident that supporting more than one
> test image file is needed to provide more comprehensive
> test coverage. (Test coverage in this scenario refers to testing
> more transistors in the core to identify faults)
>
> The other alternative of increasing the size of a single scan test image
> file would not work as the upper bound is limited by the size of memory
> area reserved by BIOS for loading IFS test image.
>
> Introduce "current_batch" file which accepts a number. Writing a
> number to the current_batch file would load the test image file by name
> ff-mm-ss-<xy>.scan, where <xy> is the number written to the
> "current_batch" file in hex. Range check of the input is done to verify
> it not greater than 0xff.
>
> For e.g if the scan test image comprises of 6 files, they would be named
> as show below:
> 06-8f-06-01.scan
> 06-8f-06-02.scan
> 06-8f-06-03.scan
> 06-8f-06-04.scan
> 06-8f-06-05.scan
> 06-8f-06-06.scan
>
> And writing 3 to current_batch would result in loading 06-8f-06-03.scan
> in the above e.g. The file can also be read to know the currently loaded
> file.
>
> And testing a system looks like:
> for each scan file
> do
> load the IFS test image file (write to the batch file)
> for each core
> do
> test the core with this set of tests
> done
> done
>
> Qualify few error messages with the test image file suffix to
> provide better context.
>
> Reviewed-by: Tony Luck <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <[email protected]>
Regards,
Hans
> ---
> drivers/platform/x86/intel/ifs/ifs.h | 23 ++++++++++----
> drivers/platform/x86/intel/ifs/core.c | 1 +
> drivers/platform/x86/intel/ifs/load.c | 18 +++++++----
> drivers/platform/x86/intel/ifs/runtest.c | 10 ++++---
> drivers/platform/x86/intel/ifs/sysfs.c | 38 ++++++++++++++++++++++++
> 5 files changed, 74 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 98ca91bdd5ca..390e508faf57 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -33,13 +33,23 @@
> * The driver loads the tests into memory reserved BIOS local to each CPU
> * 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::
> + * feedback on the success/failure of these steps.
> *
> - * # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload
> + * The test files are kept in a fixed location: /lib/firmware/intel/ifs_0/
> + * For e.g if there are 3 test files, they would be named in the following
> + * fashion:
> + * ff-mm-ss-01.scan
> + * ff-mm-ss-02.scan
> + * ff-mm-ss-03.scan
> + * (where ff refers to family, mm indicates model and ss indicates stepping)
> *
> - * 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
> + * A different testfile can be loaded by writing the numerical portion
> + * (e.g 1, 2 or 3 in the above scenario) into the curent_batch file.
> + * To load ff-mm-ss-02.scan, the following command can be used::
> + *
> + * # echo 2 > /sys/devices/virtual/misc/intel_ifs_0/current_batch
> + *
> + * The above file can also be read to know the currently loaded image.
> *
> * Running tests
> * -------------
> @@ -207,6 +217,7 @@ struct ifs_data {
> int status;
> u64 scan_details;
> int cur_batch;
> + int test_num;
> };
>
> struct ifs_work {
> @@ -227,7 +238,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);
> 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 5fb7f655c291..1f040837e8eb 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -22,6 +22,7 @@ MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
> static struct ifs_device ifs_device = {
> .data = {
> .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
> + .test_num = 0,
> },
> .misc = {
> .name = "intel_ifs_0",
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index f361fd42a320..c6414958a691 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -256,17 +256,18 @@ static int ifs_image_sanity_check(struct device *dev, const struct microcode_hea
>
> /*
> * 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 /lib/firmware/intel/ifs_x/ and named as family-model-stepping-02x.{testname}.
> */
> -void ifs_load_firmware(struct device *dev)
> +int ifs_load_firmware(struct device *dev)
> {
> struct ifs_data *ifsd = ifs_get_data(dev);
> const struct firmware *fw;
> - char scan_path[32];
> - int ret;
> + char scan_path[64];
> + int ret = -EINVAL;
>
> - 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);
> + snprintf(scan_path, sizeof(scan_path), "intel/ifs_%d/%02x-%02x-%02x-%02x.scan",
> + ifsd->test_num, boot_cpu_data.x86, boot_cpu_data.x86_model,
> + boot_cpu_data.x86_stepping, ifsd->cur_batch);
>
> ret = request_firmware_direct(&fw, scan_path, dev);
> if (ret) {
> @@ -282,8 +283,13 @@ void ifs_load_firmware(struct device *dev)
> ifs_hash_ptr = (u64)(ifs_header_ptr + 1);
>
> ret = scan_chunks_sanity_check(dev);
> + if (ret)
> + dev_err(dev, "Load failure for batch: %02x\n", ifsd->cur_batch);
> +
> release:
> release_firmware(fw);
> done:
> ifsd->loaded = (ret == 0);
> +
> + return ret;
> }
> diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
> index b2ca2bb4501f..0bfd8fcdd7e8 100644
> --- a/drivers/platform/x86/intel/ifs/runtest.c
> +++ b/drivers/platform/x86/intel/ifs/runtest.c
> @@ -78,14 +78,16 @@ static void message_not_tested(struct device *dev, int cpu, union ifs_status sta
>
> static void message_fail(struct device *dev, int cpu, union ifs_status status)
> {
> + struct ifs_data *ifsd = ifs_get_data(dev);
> +
> /*
> * control_error is set when the microcode runs into a problem
> * loading the image from the reserved BIOS memory, or it has
> * been corrupted. Reloading the image may fix this issue.
> */
> if (status.control_error) {
> - dev_err(dev, "CPU(s) %*pbl: could not execute from loaded scan image\n",
> - cpumask_pr_args(cpu_smt_mask(cpu)));
> + dev_err(dev, "CPU(s) %*pbl: could not execute from loaded scan image. Batch: %02x version: 0x%x\n",
> + cpumask_pr_args(cpu_smt_mask(cpu)), ifsd->cur_batch, ifsd->loaded_version);
> }
>
> /*
> @@ -96,8 +98,8 @@ static void message_fail(struct device *dev, int cpu, union ifs_status status)
> * the core being tested.
> */
> if (status.signature_error) {
> - dev_err(dev, "CPU(s) %*pbl: test signature incorrect.\n",
> - cpumask_pr_args(cpu_smt_mask(cpu)));
> + dev_err(dev, "CPU(s) %*pbl: test signature incorrect. Batch: %02x version: 0x%x\n",
> + cpumask_pr_args(cpu_smt_mask(cpu)), ifsd->cur_batch, ifsd->loaded_version);
> }
> }
>
> diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
> index e077910c5d28..d2eeeb04d760 100644
> --- a/drivers/platform/x86/intel/ifs/sysfs.c
> +++ b/drivers/platform/x86/intel/ifs/sysfs.c
> @@ -87,6 +87,43 @@ static ssize_t run_test_store(struct device *dev,
>
> static DEVICE_ATTR_WO(run_test);
>
> +static ssize_t current_batch_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ifs_data *ifsd = ifs_get_data(dev);
> + int cur_batch;
> + int rc;
> +
> + rc = kstrtouint(buf, 0, &cur_batch);
> + if (rc < 0 || cur_batch > 0xff)
> + return -EINVAL;
> +
> + if (down_interruptible(&ifs_sem))
> + return -EINTR;
> +
> + ifsd->cur_batch = cur_batch;
> +
> + rc = ifs_load_firmware(dev);
> +
> + up(&ifs_sem);
> +
> + return (rc == 0) ? count : rc;
> +}
> +
> +static ssize_t current_batch_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct ifs_data *ifsd = ifs_get_data(dev);
> +
> + if (!ifsd->loaded)
> + return sysfs_emit(buf, "none\n");
> + else
> + return sysfs_emit(buf, "0x%02x\n", ifsd->cur_batch);
> +}
> +
> +static DEVICE_ATTR_RW(current_batch);
> +
> /*
> * Display currently loaded IFS image version.
> */
> @@ -108,6 +145,7 @@ static struct attribute *plat_ifs_attrs[] = {
> &dev_attr_details.attr,
> &dev_attr_status.attr,
> &dev_attr_run_test.attr,
> + &dev_attr_current_batch.attr,
> &dev_attr_image_version.attr,
> NULL
> };
On 11/10/2022 1:37 PM, Hans de Goede wrote:
> Hi Boris,
>
> On 11/10/22 10:59, Borislav Petkov wrote:
>> On Mon, Nov 07, 2022 at 02:53:09PM -0800, Jithu Joseph wrote:
>>> Changes in v2
>>> - Rebased ontop of v6.1-rc4
>>> Boris
>>> - Moved exported functions (microcode_sanity_check(),
>>> find_matching_signature ) from microcode/intel.c to cpu/intel.c
>>> (patch4,6)
>>> - Removed microcode metadata specific code changes to
>>> microcode_sanity_check() (patch6)
>>> - Moved find_meta_data() from common to IFS driver (Patch 8)
>>
>> What's the upstreaming plan here - I'm assuming I should take the
>> microcode patches through the tip tree?
>>
>> Or should I take the whole thing through tip so that there's no
>> confusion and having to sync and share branches between trees?
>
> I have just reviewed all the platform/x86/intel/ifs changes
> and they all look good to me.
>
Hans, Thanks for reviewing.
Boris, I have fixes from Sohil's comments (mainly for patch 06/14). Let me know
if you have any other issues that need touching up in x86 parts (04 - 07).
I will post a cleaned-up series next week.
Jithu
Hi,
On 11/7/22 23:53, Jithu Joseph wrote:
> CONFIG_INTEL_IFS_DEVICE is not used anywhere. The selection in
> Kconfig is therefore pointless. Delete it.
>
> Reviewed-by: Tony Luck <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <[email protected]>
Regards,
Hans
> ---
> drivers/platform/x86/intel/ifs/Kconfig | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig
> index c341a27cc1a3..89152d46deee 100644
> --- a/drivers/platform/x86/intel/ifs/Kconfig
> +++ b/drivers/platform/x86/intel/ifs/Kconfig
> @@ -4,7 +4,6 @@ config INTEL_IFS
> # Discussion on the list has shown that the sysfs API needs a bit
> # more work, mark this as broken for now
> depends on BROKEN
> - select INTEL_IFS_DEVICE
> help
> Enable support for the In Field Scan capability in select
> CPUs. The capability allows for running low level tests via
Hi Boris,
On 11/10/22 10:59, Borislav Petkov wrote:
> On Mon, Nov 07, 2022 at 02:53:09PM -0800, Jithu Joseph wrote:
>> Changes in v2
>> - Rebased ontop of v6.1-rc4
>> Boris
>> - Moved exported functions (microcode_sanity_check(),
>> find_matching_signature ) from microcode/intel.c to cpu/intel.c
>> (patch4,6)
>> - Removed microcode metadata specific code changes to
>> microcode_sanity_check() (patch6)
>> - Moved find_meta_data() from common to IFS driver (Patch 8)
>
> What's the upstreaming plan here - I'm assuming I should take the
> microcode patches through the tip tree?
>
> Or should I take the whole thing through tip so that there's no
> confusion and having to sync and share branches between trees?
I have just reviewed all the platform/x86/intel/ifs changes
and they all look good to me.
I think it is the best and easiest if you just take the whole
branch.
I don't have any changes pending under drivers/platform/x86/intel/ifs
so there should not be any conflicts.
Regards,
Hans
On Mon, Nov 07, 2022 at 02:53:13PM -0800, Jithu Joseph wrote:
> IFS uses 'scan test images' provided by Intel that can be regarded as
Why is that in '' quotes?
> firmware. IFS test image carries microcode header with extended signature
> table.
>
> Expose find_matching_signature() for verifying if the test image
Yeah, not "expose" but "reuse".
> header or the extended signature table indicate whether an IFS test image
> is fit to run on a system.
> Move the function to cpu/intel.c and
> add intel_ prefix to the function name.
That is not needed in a commit message - it is visible from the diff
itself.
Please remove the "what" in all your commit messages.
...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Nov 07, 2022 at 02:53:16PM -0800, Jithu Joseph wrote:
> Intel is using microcode file format for IFS test images too.
>
> IFS test images use one of the existing reserved fields in microcode
> header to indicate the size of the region in the file allocated for
> metadata structures.
>
> In prepration for this, rename first of the existing reserved fields
Unknown word [prepration] in commit message.
Suggestions: ['preparation', 'peroration', 'prep ration', 'prep-ration', 'preparations', 'reparation', 'perspiration', "preparation's", 'proportion']
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Nov 07, 2022 at 02:53:21PM -0800, Jithu Joseph wrote:
> Initial implementation assumed a single IFS test image file with a
> fixed name ff-mm-ss.scan. (where ff, mm, ss refers to family, model
> and stepping of the core)
>
> Subsequently, it became evident that supporting more than one
> test image file is needed to provide more comprehensive
> test coverage. (Test coverage in this scenario refers to testing
> more transistors in the core to identify faults)
>
> The other alternative of increasing the size of a single scan test image
> file would not work as the upper bound is limited by the size of memory
> area reserved by BIOS for loading IFS test image.
>
> Introduce "current_batch" file which accepts a number. Writing a
> number to the current_batch file would load the test image file by name
> ff-mm-ss-<xy>.scan, where <xy> is the number written to the
> "current_batch" file in hex. Range check of the input is done to verify
> it not greater than 0xff.
Dunno - sounds silly to me. Means one needs to go and look up which
files are there and echo those batch numbers into sysfs and so on.
What I would do is make it real trivial for the user so that latter can
simply do:
for f in $(ls /lib/firmware/intel/ifs_0/*.scan);
do
echo $f > /sys/devices/virtual/misc/intel_ifs_0/test_file
done
and simply supply the full filename.
There will be no requirement on the naming - only on the filename length
and it should be in that directory /lib/firmware/intel/ifs_0/
(btw, what's that appended "_0" supposed to mean there?)
So the kernel would simply open it, sanity-check it, if it passes, it
would run it - otherwise it would ignore it.
A usability win-win.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Saturday, 12 November 2022 08:26:28 PST Borislav Petkov wrote:
> > Introduce "current_batch" file which accepts a number. Writing a
> > number to the current_batch file would load the test image file by name
> > ff-mm-ss-<xy>.scan, where <xy> is the number written to the
> > "current_batch" file in hex. Range check of the input is done to verify
> > it not greater than 0xff.
>
> Dunno - sounds silly to me. Means one needs to go and look up which
> files are there and echo those batch numbers into sysfs and so on.
Not exactly. That's what this file is there for. It allows the algorithm to
read the current batch file, add 1, then echo back. If the load succeeds, the
the batch exists; if not, then the algorithm should simply go back to 0.
That's what we're implementing here:
https://github.com/opendcdiag/opendcdiag/pull/163
> What I would do is make it real trivial for the user so that latter can
> simply do:
>
> for f in $(ls /lib/firmware/intel/ifs_0/*.scan);
> do
> echo $f > /sys/devices/virtual/misc/intel_ifs_0/test_file
> done
>
> and simply supply the full filename.
Unfortunately, there are other limitations that make such a simple algorithm
not possible in the first place.
First, there's the question of the ability to see into /lib/firmware. I'm not a
kernel dev but I'm told that request_firmware() only operates on the root
container's filesystem view. We're expecting that the application may get
deployed as a container (with full privileges so it can write to /sys, sure),
so it won't be able to see the host system's /lib to know what files are
available. It could "guess" at the file names, based on the current processor's
family/model/stepping and a natural number, but that's sub-optimal.
Unless the driver were allowed to load any file named by the application, from
its own view of the filesystem, permitting the firmware files being distributed
inside the container.
Second, for electrical reasons, we expect that certain processor generations
will need a timeout between tests before testing can be done again on a given
core, whether the same batch or the next one. This time out can be in the
order of many minutes, which is longer than any hyperscaler is willing to
allocate for a system self-test hogging a core or the whole system, just
waiting. For example, let's say that the timeout is 15 minutes and there are 4
batches: this means the whole testing procedure takes one hour, even though
the actual downtime for each core was less than 1 second. This is lost
revenue.
Instead, they wish the next available maintenance window to simply resume
testing at the point where the last one stopped. These windows need not be
scheduled; they can also be opportunistic, when the orchestrator determines
the machine or a subset of one is going to be idle. That's what the algorithm
in the pull request above implements: if the current_batch's result was
"untested", it is attempted again, otherwise it tries the next one, rolling
back to 0 if the loading failed. This removes the need to know anything about
the timeout on the current processor or even whether there is one, or how many
batches there are.242
> So the kernel would simply open it, sanity-check it, if it passes, it
> would run it - otherwise it would ignore it.
>
> A usability win-win.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Cloud Software Architect - Intel DCAI Cloud Engineering
> Dunno - sounds silly to me. Means one needs to go and look up which
> files are there and echo those batch numbers into sysfs and so on.
>
> What I would do is make it real trivial for the user so that latter can
> simply do:
>
> for f in $(ls /lib/firmware/intel/ifs_0/*.scan);
> do
> echo $f > /sys/devices/virtual/misc/intel_ifs_0/test_file
> done
>
> and simply supply the full filename.
We tried the full file name in an earlier version. GregKH was unimpressed. But that was when we were trying to overload the meaning of the “reload” file.
Is it different now?
-Tony
On Sat, Nov 12, 2022 at 10:21:35AM -0800, Thiago Macieira wrote:
> Not exactly. That's what this file is there for. It allows the algorithm to
> read the current batch file, add 1, then echo back. If the load succeeds, the
> the batch exists; if not, then the algorithm should simply go back to 0.
This sounds to me like there's a special order in which those batches
should be executed?
I thought they're simply collections of test sequences which can be run
in any order...
> First, there's the question of the ability to see into /lib/firmware. I'm not a
> kernel dev but I'm told that request_firmware() only operates on the root
> container's filesystem view. We're expecting that the application may get
> deployed as a container (with full privileges so it can write to /sys, sure),
> so it won't be able to see the host system's /lib to know what files are
> available. It could "guess" at the file names, based on the current processor's
> family/model/stepping and a natural number, but that's sub-optimal.
It is not about seeing - you simply give it the filename -
request_firmware* does the "seeing". Either the file's there or it
isn't.
> Unless the driver were allowed to load any file named by the application, from
> its own view of the filesystem, permitting the firmware files being distributed
> inside the container.
There's a reason I wrote:
"There will be no requirement on the naming - only on the filename
length and it should be in that directory /lib/firmware/intel/ifs_0/"
Of course the driver should load only from that directory.
> Second, for electrical reasons, we expect that certain processor generations
> will need a timeout between tests before testing can be done again on a given
> core, whether the same batch or the next one. This time out can be in the
> order of many minutes, which is longer than any hyperscaler is willing to
> allocate for a system self-test hogging a core or the whole system, just
> waiting. For example, let's say that the timeout is 15 minutes and there are 4
> batches: this means the whole testing procedure takes one hour, even though
> the actual downtime for each core was less than 1 second. This is lost
> revenue.
All that doesn't matter - if the CPU *must* wait 15 minutes between
batches, then that should be enforced by the driver and not relied upon
by userspace to DTRT.
> Instead, they wish the next available maintenance window to simply resume
> testing at the point where the last one stopped. These windows need not be
> scheduled; they can also be opportunistic, when the orchestrator determines
> the machine or a subset of one is going to be idle. That's what the algorithm
> in the pull request above implements: if the current_batch's result was
> "untested", it is attempted again, otherwise it tries the next one, rolling
> back to 0 if the loading failed. This removes the need to know anything about
> the timeout on the current processor or even whether there is one, or how many
> batches there are.242
This all has nothing to do with whether you give it a number or a
filename. How you glue your testing around it together is a userspace
issue - all the kernel driver needs to be able to do is load the
sequence and execute it.
Echoing filenames into sysfs is no different from echoing numbers into
it - former is simpler. If the CPU says it cannot execute the sequence
currently, you have to think about how you retry that sequence. How you
specify it doesn't matter.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sat, Nov 12, 2022 at 06:33:58PM +0000, Luck, Tony wrote:
> We tried the full file name in an earlier version. GregKH was
> unimpressed. But that was when we were trying to overload the meaning
> of the “reload” file.
I'm guessing the background of his lack of "impressiveness" was
something along the lines of:
"Attributes should be ASCII text files, preferably with only one value
per file. It is noted that it may not be efficient to contain only one
value per file, so it is socially acceptable to express an array of
values of the same type."
Question is, is a filename string one value per file or strings don't
count?
Suspend allows it:
# echo "shutdown" > /sys/power/disk
# echo "disk" > /sys/power/state
Btw, do you guys really want to run this in production?
As in, are you really that confident that all those test sequence
executions really do not affect other CPU execution at all?
Because if this is going to be run during downtime, as Thiago says, then
you can just as well use debugfs for this. And then there's no need to
cast any API in stone and so on.
Hmmm.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sat, Nov 12, 2022 at 08:20:30PM +0100, Borislav Petkov wrote:
> On Sat, Nov 12, 2022 at 10:21:35AM -0800, Thiago Macieira wrote:
> > Not exactly. That's what this file is there for. It allows the algorithm to
> > read the current batch file, add 1, then echo back. If the load succeeds, the
> > the batch exists; if not, then the algorithm should simply go back to 0.
>
> This sounds to me like there's a special order in which those batches
> should be executed?
There is no special sequence required. Idea is that user would like to run all
tests, so simply going through them works. User has no idea what the test
content is for each file, unless there is a case where they like to repeat
a specific batch on one core, they can simply skip and do one specific
batch, they don't need to go through in sequence.
>
> I thought they're simply collections of test sequences which can be run
> in any order...
>
> > First, there's the question of the ability to see into /lib/firmware. I'm not a
> > kernel dev but I'm told that request_firmware() only operates on the root
> > container's filesystem view. We're expecting that the application may get
> > deployed as a container (with full privileges so it can write to /sys, sure),
> > so it won't be able to see the host system's /lib to know what files are
> > available. It could "guess" at the file names, based on the current processor's
> > family/model/stepping and a natural number, but that's sub-optimal.
>
> It is not about seeing - you simply give it the filename -
> request_firmware* does the "seeing". Either the file's there or it
> isn't.
Link to Tony's last post before changing the file formats to accomodate
Greg's feedback.
https://lore.kernel.org/lkml/SJ1PR11MB6083263E8EEBF106B6B61B24FC969@SJ1PR11MB6083.namprd11.prod.outlook.com/
Cheers,
Ashok
> Btw, do you guys really want to run this in production?
Absolutely yes.
> As in, are you really that confident that all those test sequence
> executions really do not affect other CPU execution at all?
Tests may draw a lot of power, so prevent
other cores going into turbo frequency states.
But otherwise no effect.
> Because if this is going to be run during downtime, as Thiago says, then
> you can just as well use debugfs for this. And then there's no need to
> cast any API in stone and so on.
Did Thiago say “during downtime”? I think
he talked about some users opportunistic
use of scan tests. But that’s far from only
during downtime. We fully expect CSPs to
run these scans periodically on production
machines.
-Tony
On Saturday, 12 November 2022 11:20:30 PST Borislav Petkov wrote:
> This sounds to me like there's a special order in which those batches
> should be executed?
>
> I thought they're simply collections of test sequences which can be run
> in any order...
As Ashok replied, no, they are not ordered. But running them from first to last
is the simplest algorithm to code.
If we did support file names, then the directory order would be also as good as
any (unsorted).
> It is not about seeing - you simply give it the filename -
> request_firmware* does the "seeing". Either the file's there or it
> isn't.
I meant knowing which files are there. If they don't form a specific pattern,
then it's impossible to know what they have been named. And if they do form a
specific pattern, what's the harm in just using the sequence number? It's much
easier for the kernel to remember a single 8-bit number than a file name.
It also allows for new techniques like deploying a single cpio or tarball with
everything with an update to the kernel, without having userspace have to
update to match.
> There's a reason I wrote:
>
> "There will be no requirement on the naming - only on the filename
> length and it should be in that directory /lib/firmware/intel/ifs_0/"
>
> Of course the driver should load only from that directory.
Thank you for that explanation. But my argument was that the application
driving this might be deployed as a container, as part of a container-
orchestration and scheduling system, while the firmware files would already be
pre-installed on the machine and updated with the regular firmware update
mechanism. If the container can't see what files are there, it would have to
resort to the guessing I mentioned above. CSPs could avoid this by bind-
mounting /lib/firmware into the container, but we'd prefer not to add yet
another constraint.
> All that doesn't matter - if the CPU *must* wait 15 minutes between
> batches, then that should be enforced by the driver and not relied upon
> by userspace to DTRT.
If's enforced by the CPU today. How it determines when a test can be run is
besides the point; the driver will ask it to run and the CPU will reply it
couldn't. That information is conveyed back to userspace by changing the
"status" back to "untested".
> This all has nothing to do with whether you give it a number or a
> filename. How you glue your testing around it together is a userspace
> issue - all the kernel driver needs to be able to do is load the
> sequence and execute it.
>
> Echoing filenames into sysfs is no different from echoing numbers into
> it - former is simpler. If the CPU says it cannot execute the sequence
> currently, you have to think about how you retry that sequence. How you
> specify it doesn't matter.
Right, it's no different from echoing file names, but it's much simpler to
increment a number than do a directory listing and sort the file names, so it
can pick up from where it left off.
I mentioned this complication to explain why the userspace won't be able to
simply echo each file name and expect things to have reached full coverage.
Unfortunately, userspace needs to cope with the fact that there will be a
timeout for certain generations. It's not what we'd have preferred; it's a
hardware constraint we have to adapt to.
WIth this constraint in mind, having a single number simplifies userspace. You
can't do it with a three-line shell script, but we're not expecting that shell
scripts are the means to use this feature in the first place.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Cloud Software Architect - Intel DCAI Cloud Engineering
On Saturday, 12 November 2022 15:32:47 PST Luck, Tony wrote:
> > Because if this is going to be run during downtime, as Thiago says, then
> > you can just as well use debugfs for this. And then there's no need to
> > cast any API in stone and so on.
>
> Did Thiago say “during downtime”? I think
> he talked about some users opportunistic
> use of scan tests. But that’s far from only
> during downtime. We fully expect CSPs to
> run these scans periodically on production
> machines.
Let me clarify. I did not mean full system downtime for maintenance, but I did
mean that there's a gap in consumer workload, for both threads of one or more
cores. As Tony said, it should have little observable effect on any other core,
meaning an IFS run can be scheduled *as* any other workload (albeit a
privileged one) for a subset of the machine, while the rest of the system
remains in production. This allows them a lot of flexibility and is the reason
I am talking about containers, with the implied constraint that the
container's view of the filesystem is narrower than the kernel's.
There'll be some coordination required to get all cores to have run all tests,
but it should be doable over a period of time, and I'm thinking days, not
years. This should still be short enough to reveal if the system can detect a
defect or wear-out before any real workload is impacted by it.
If an issue is detected, the admin can decide whether to offline the core(s)
reporting problems but keep the rest serving workloads and generating revenue,
or offline the entire machine for full maintenance and to run more invasive and
time-consuming tests.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Cloud Software Architect - Intel DCAI Cloud Engineering
On Sat, Nov 12, 2022 at 06:33:58PM +0000, Luck, Tony wrote:
>
> > Dunno - sounds silly to me. Means one needs to go and look up which
> > files are there and echo those batch numbers into sysfs and so on.
> >
> > What I would do is make it real trivial for the user so that latter can
> > simply do:
> >
> > for f in $(ls /lib/firmware/intel/ifs_0/*.scan);
> > do
> > echo $f > /sys/devices/virtual/misc/intel_ifs_0/test_file
> > done
> >
> > and simply supply the full filename.
>
> We tried the full file name in an earlier version. GregKH was unimpressed. But that was when we were trying to overload the meaning of the “reload” file.
>
> Is it different now?
No, please do not force the driver to resolve a filename path in the
kernel.
Replying to both with one mail because it still feels like there's a
misunderstanding.
On Sun, Nov 13, 2022 at 08:37:32AM +0100, [email protected] wrote:
> No, please do not force the driver to resolve a filename path in the
> kernel.
No, I don't mean to do any filename path resolving - all I suggest is to
echo into sysfs the full filename instead of the number. I.e., this:
for i in $(ls /lib/firmware/intel/ifs_0/*.scan);
do
echo $i /sys/devices/virtual/misc/intel_ifs_0/current_batch
done
What you end up echoing inside is only the full filename - *not* the
absolute filename - instead of a number. So those in a succession:
06-8f-06-00.scan
06-8f-06-01.scan
06-8f-06-02.scan
06-8f-06-03.scan
06-8f-06-04.scan
06-8f-06-05.scan
The advantage being, you don't need to remember which file sequence and
which family/model/stepping.
For all Intel employees here on the thread, there's a world outside
Intel and people do not talk (family model stepping) tuples like we do.
All they wanna do is run their damn tests.
So instead of what the code does now:
+ snprintf(scan_path, sizeof(scan_path), "intel/ifs_%d/%02x-%02x-%02x-%02x.scan",
+ ifsd->test_num, boot_cpu_data.x86, boot_cpu_data.x86_model,
+ boot_cpu_data.x86_stepping, ifsd->cur_batch);
It would still use the *same* scan_path - /lib/firmware/intel/ifs_0/ -
no one is proposing to give a full path name - it would only use the
filename string - 06-8f-06-00.scan, for example - instead of the "0" in
it to build that string.
And, ofcourse it would check the format of that string against family,
model, stepping and sequence number (btw this way you drop your
limitation of 256 for the sequence number which you don't really need
either).
And then if the format passes, it would check the headers.
And only if those pass too, then it would load.
> Right, it's no different from echoing file names, but it's much
> simpler to increment a number than do a directory listing and sort the
> file names, so it can pick up from where it left off.
It is no different - you still need to remember where you are in the
sequence wrt to testing.
So if you want to deal with the timeout, that same script above will
check the status and wait. Not rocket science.
As to this Thiago:
> You can't do it with a three-line shell script, but we're not
> expecting that shell scripts are the means to use this feature in the
> first place.
I consider it a serious design mistake of having to have a *special*
tool. A special tool is *always* a burden. You need to build it, supply
it, make sure it is installable on the target system and so on.
And I'm telling you this with my Linux distributor hat on. It is always
a pain - trust me.
For example, there's a reason why you can still control ftrace from the
command line and you don't need any tool. You *can* use a tool but you
don't have to. IOW, the KISS philosophy.
IOW, I really think that designing our interfaces with user friendliness
in mind should be of much more higher importance. And requiring the user
to remember or figure out anything she doesn't really need to, in order
to run the test is a burden.
Just look at microcode loading: early loading works automatically - you
only need to install the blobs in /lib/firmware/intel-ucode/. The initrd
builder figures out which to add to the image.
And not even that is required - I have a script which adds *all* blobs
to my image. It still loads the right one.
Late loading works also trivially:
echo 1 > /sys/devices/system/cpu/microcode/reload
And it goes and builds the filename from f/m/s and loads it from the
hardcoded path - no filename resolving.
But it doesn't ask the user to give a f/m/s or a sequence number.
I sincerely hope that makes more sense.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sun, Nov 13, 2022 at 12:48:40PM +0100, Borislav Petkov wrote:
> Replying to both with one mail because it still feels like there's a
> misunderstanding.
>
> On Sun, Nov 13, 2022 at 08:37:32AM +0100, [email protected] wrote:
> > No, please do not force the driver to resolve a filename path in the
> > kernel.
>
> No, I don't mean to do any filename path resolving - all I suggest is to
> echo into sysfs the full filename instead of the number. I.e., this:
>
> for i in $(ls /lib/firmware/intel/ifs_0/*.scan);
> do
> echo $i /sys/devices/virtual/misc/intel_ifs_0/current_batch
> done
>
> What you end up echoing inside is only the full filename - *not* the
> absolute filename - instead of a number. So those in a succession:
>
> 06-8f-06-00.scan
> 06-8f-06-01.scan
> 06-8f-06-02.scan
> 06-8f-06-03.scan
> 06-8f-06-04.scan
> 06-8f-06-05.scan
Do you expect the /lib/firmware/intel/ifs_0/ to contain *ONLY* files for
this platform? For microcode we have everything in the public release
included here.
In the above proposal, you can *ONLY* put files for this platform unlike simply
copying everything released and let the kernel pick the right one since it
does the ff-mm-ss-*.scan lookup. Only the batch number is supplied from
user space.
>
> The advantage being, you don't need to remember which file sequence and
> which family/model/stepping.
Even in the current implementation, user doesn't need to know f/m/s. That's
something the driver selects automatically, just like what microcode does
for reload.
>
> For all Intel employees here on the thread, there's a world outside
> Intel and people do not talk (family model stepping) tuples like we do.
> All they wanna do is run their damn tests.
>
> So instead of what the code does now:
>
> + snprintf(scan_path, sizeof(scan_path), "intel/ifs_%d/%02x-%02x-%02x-%02x.scan",
> + ifsd->test_num, boot_cpu_data.x86, boot_cpu_data.x86_model,
> + boot_cpu_data.x86_stepping, ifsd->cur_batch);
>
> It would still use the *same* scan_path - /lib/firmware/intel/ifs_0/ -
> no one is proposing to give a full path name - it would only use the
> filename string - 06-8f-06-00.scan, for example - instead of the "0" in
> it to build that string.
>
> And, ofcourse it would check the format of that string against family,
> model, stepping and sequence number (btw this way you drop your
> limitation of 256 for the sequence number which you don't really need
> either).
>
> And then if the format passes, it would check the headers.
>
> And only if those pass too, then it would load.
Isn't it simple now? No need to check if user supplied the right f/m/s
since its not a user input, kernel composes that automatically.
> > Right, it's no different from echoing file names, but it's much
> > simpler to increment a number than do a directory listing and sort the
> > file names, so it can pick up from where it left off.
>
> It is no different - you still need to remember where you are in the
> sequence wrt to testing.
>
> So if you want to deal with the timeout, that same script above will
> check the status and wait. Not rocket science.
>
> As to this Thiago:
>
> > You can't do it with a three-line shell script, but we're not
> > expecting that shell scripts are the means to use this feature in the
> > first place.
>
> I consider it a serious design mistake of having to have a *special*
> tool. A special tool is *always* a burden. You need to build it, supply
> it, make sure it is installable on the target system and so on.
>
> And I'm telling you this with my Linux distributor hat on. It is always
> a pain - trust me.
>
> For example, there's a reason why you can still control ftrace from the
> command line and you don't need any tool. You *can* use a tool but you
> don't have to. IOW, the KISS philosophy.
The tool we have is not for a simple test run. That can easily be done with
a shell script. The tool does a bit more like handling retries if the test
was not scheduled. The fundamental pass/fail simply output is what the
kernel provides.
>
> IOW, I really think that designing our interfaces with user friendliness
> in mind should be of much more higher importance. And requiring the user
> to remember or figure out anything she doesn't really need to, in order
> to run the test is a burden.
>
> Just look at microcode loading: early loading works automatically - you
> only need to install the blobs in /lib/firmware/intel-ucode/. The initrd
> builder figures out which to add to the image.
>
> And not even that is required - I have a script which adds *all* blobs
> to my image. It still loads the right one.
>
> Late loading works also trivially:
>
> echo 1 > /sys/devices/system/cpu/microcode/reload
>
> And it goes and builds the filename from f/m/s and loads it from the
> hardcoded path - no filename resolving.
>
> But it doesn't ask the user to give a f/m/s or a sequence number.
I don't think the current proposed interface expects a f/m/s. The entire
IFS design was sort of mimicking the microcode interface.
The V1 submission did pretty much that. But it required copying files from
some place to /lib/firmware so they can echo 1 > reload.
Instead of copying files over that *did* sound like a hack, the batch was
introduced and that's the only thing needed from user input.
The utility is more like icing, to run a simple test all you need is a
simple script. It is not a baseline requirement.
Cheers,
Ashok
On Sun, Nov 13, 2022 at 07:15:03AM -0800, Ashok Raj wrote:
> Do you expect the /lib/firmware/intel/ifs_0/ to contain *ONLY* files for
> this platform? For microcode we have everything in the public release
> included here.
Same as microcode, as I said further down in my mail:
"And, ofcourse it would check the format of that string against family,
model, stepping and sequence number (btw this way you drop your
limitation of 256 for the sequence number which you don't really need
either)."
> In the above proposal, you can *ONLY* put files for this platform
> unlike simply copying everything released and let the kernel pick the
> right one since it does the ff-mm-ss-*.scan lookup. Only the batch
> number is supplied from user space.
No, see above. You check the filename against the current f/m/s. Just
like microcode.
> Even in the current implementation, user doesn't need to know f/m/s.
> That's something the driver selects automatically, just like what
> microcode does for reload.
Basically what I'm saying all this time.
> Isn't it simple now? No need to check if user supplied the right f/m/s
> since its not a user input, kernel composes that automatically.
Let's see
* try echoing a magic number into some sysfs file
vs
* simply try *all* files in a directory
Latter is even simpler because you don't have to explain anything about
sequence numbers - the user doesn't need to know.
> The tool we have is not for a simple test run. That can easily be done with
> a shell script. The tool does a bit more like handling retries if the test
> was not scheduled. The fundamental pass/fail simply output is what the
> kernel provides.
Because a simple script can't handle retries based on the values read
from that sysfs file?
Yeah, right.
> I don't think the current proposed interface expects a f/m/s. The entire
> IFS design was sort of mimicking the microcode interface.
Ashok, you prove for the nth time that you don't really read my emails.
Sorry, try again.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 11/13/2022 3:48 AM, Borislav Petkov wrote:
>
> So instead of what the code does now:
>
> + snprintf(scan_path, sizeof(scan_path), "intel/ifs_%d/%02x-%02x-%02x-%02x.scan",
> + ifsd->test_num, boot_cpu_data.x86, boot_cpu_data.x86_model,
> + boot_cpu_data.x86_stepping, ifsd->cur_batch);
>
> It would still use the *same* scan_path - /lib/firmware/intel/ifs_0/ -
> no one is proposing to give a full path name - it would only use the
> filename string - 06-8f-06-00.scan, for example - instead of the "0" in
> it to build that string.
>
> And, ofcourse it would check the format of that string against family,
> model, stepping and sequence number (btw this way you drop your
> limitation of 256 for the sequence number which you don't really need
> either).
>
> And then if the format passes, it would check the headers.
Do you think it is better to restrict filename input to confirm to
ff-mm-ss-xy.<test> format rather than accepting any string and treating it as
a file-name and trying to load it, if it is present ?
(Given that, before loading, We do intel_find_matching_signature(), which validates if the
signature/pf entries in header confirms to the machine we are on before loading)
We did accepting file-name as input before [1] (except for validating if the filename confirms to ff-mm-ss format)
>
> And only if those pass too, then it would load.
>
[1] https://lore.kernel.org/lkml/[email protected]/
On Sun, Nov 13, 2022 at 08:41:47AM -0800, Joseph, Jithu wrote:
> Do you think it is better to restrict filename input to confirm
> to ff-mm-ss-xy.<test> format rather than accepting any string and
> treating it as a file-name and trying to load it, if it is present ?
Yap, that way you pre-filter filenames. Yeah, the header checks *must*
absolutely happen too but it would be a simple first test.
Also, you can check for:
ff-mm-ss-X.<test>
where X is a [0-9]+ of arbitrary length and this way won't have the
artificial 256 limit you have now.
> (Given that, before loading, We do intel_find_matching_signature(),
> which validates if the signature/pf entries in header confirms to the
> machine we are on before loading)
>
> We did accepting file-name as input before [1] (except for validating
> if the filename confirms to ff-mm-ss format)
Yeah, except now you want to do multiple sets of scan files.
But yeah, you can extend the interface even more:
echo <fname> /sysfs/file
* if f/m/s matches, you execute
if still within the timeout, you return -EAGAIN from
current_batch_store() to tell userspace, take a nap and try again.
* if the f/m/s doesn't match you return -EINVAL to say, wrong filename,
try the next one.
And this way, the user script can simply look at the retvals and act
accordingly - it won't even need to parse dmesg or wnatnot.
For example.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sun, Nov 13, 2022 at 04:58:52PM +0100, Borislav Petkov wrote:
> On Sun, Nov 13, 2022 at 07:15:03AM -0800, Ashok Raj wrote:
> > Do you expect the /lib/firmware/intel/ifs_0/ to contain *ONLY* files for
> > this platform? For microcode we have everything in the public release
> > included here.
>
> Same as microcode, as I said further down in my mail:
>
> "And, ofcourse it would check the format of that string against family,
-----------------^^ It is user space or the kernel driver?
> model, stepping and sequence number (btw this way you drop your
> limitation of 256 for the sequence number which you don't really need
> either)."
>
> > In the above proposal, you can *ONLY* put files for this platform
> > unlike simply copying everything released and let the kernel pick the
> > right one since it does the ff-mm-ss-*.scan lookup. Only the batch
> > number is supplied from user space.
>
> No, see above. You check the filename against the current f/m/s. Just
> like microcode.
If it's Ok to ask a question. "You" above is the kernel?
Microcode has no such functionality today right? User space
never inputs a filename, only performs echo 1 > reload.
If a file name composed by the kernel exists, then it checks the header
validity before proceeding.
Apologize if I misunderstood you.
>
> > Even in the current implementation, user doesn't need to know f/m/s.
> > That's something the driver selects automatically, just like what
> > microcode does for reload.
>
> Basically what I'm saying all this time.
>
> > Isn't it simple now? No need to check if user supplied the right f/m/s
> > since its not a user input, kernel composes that automatically.
>
> Let's see
>
> * try echoing a magic number into some sysfs file
>
> vs
>
> * simply try *all* files in a directory
>
> Latter is even simpler because you don't have to explain anything about
> sequence numbers - the user doesn't need to know.
Ok, so can you take through how that is going to work..
for i in *
do
echo $i > batch
done
If some of the files are for a different fms, kernel will check the format
and ignores the input.
So some of the files will work, some will fail, and user space doesn't
care?
Please pardon if I misunderstood you.
> Ashok, you prove for the nth time that you don't really read my emails.
> Sorry, try again.
I absolutely read your emails Boris. But when I have a misunderstanding its
an iterative process.
Asking a question or responding to your email doesn't mean I'm willfully
ignoring, or have a ill/malice intend.
You are reviewing the code and I'm simply discussing what each person
means. I hope its OK to have a dialog.
Cheers,
Ashok
On 11/13/2022 8:58 AM, Borislav Petkov wrote:
> On Sun, Nov 13, 2022 at 08:41:47AM -0800, Joseph, Jithu wrote:
>> Do you think it is better to restrict filename input to confirm
>> to ff-mm-ss-xy.<test> format rather than accepting any string and
>> treating it as a file-name and trying to load it, if it is present ?
>
> Yap, that way you pre-filter filenames. Yeah, the header checks *must*
> absolutely happen too but it would be a simple first test.
>
> Also, you can check for:
>
> ff-mm-ss-X.<test>
>
> where X is a [0-9]+ of arbitrary length and this way won't have the
> artificial 256 limit you have now.
>
Thanks for clarifying
>> (Given that, before loading, We do intel_find_matching_signature(),
>> which validates if the signature/pf entries in header confirms to the
>> machine we are on before loading)
>>
>> We did accepting file-name as input before [1] (except for validating
>> if the filename confirms to ff-mm-ss format)
>
> Yeah, except now you want to do multiple sets of scan files.
The earlier change (which modified reload sysfs file from accepting a file-name instead of 1) was for
allowing multiple test files (i.e no different from what we are doing now)
Then we were told not to specify a filename via sysfs file (apologies for being repetitive)
Jithu
[1] https://lore.kernel.org/lkml/[email protected]/
On Sun, Nov 13, 2022 at 09:01:32AM -0800, Ashok Raj wrote:
> If it's Ok to ask a question. "You" above is the kernel?
Of course the kernel. If you think about it, it makes sense only for the
kernel to do any checking. As it is enforcing that only the proper blobs
are loaded. Just like microcode.
Userspace is only doing the triggering of the actions.
> Microcode has no such functionality today right? User space
> never inputs a filename, only performs echo 1 > reload.
Yes, because it is as user-friendly as possible. Users should not care
about filenames. But microcode needs only a single file.
If you have multiple files like IFS, you could just as well supply them
and the kernel would check every aspect before loading them.
> If a file name composed by the kernel exists, then it checks the header
> validity before proceeding.
Yes.
> So some of the files will work, some will fail, and user space doesn't
> care?
See my reply to Jithu:
https://lore.kernel.org/r/[email protected]
> You are reviewing the code and I'm simply discussing what each person
> means. I hope its OK to have a dialog.
I say
| Late loading works also trivially:
|
| echo 1 > /sys/devices/system/cpu/microcode/reload
|
| And it goes and builds the filename from f/m/s and loads it from the
| hardcoded path - no filename resolving.
|
| But it doesn't ask the user to give a f/m/s or a sequence number.
You reply with
| I don't think the current proposed interface expects a f/m/s. The
| entire IFS design was sort of mimicking the microcode interface."
|
| and you go on to explain what it used to do. I read what it used to do.
So how does your reply have any relevance to what I'm saying?
I go and give the full spiel on how it is important to support command
line loading and how you don't really need a special tool, you say
|The utility is more like icing, to run a simple test all you need is a
|simple script. It is not a baseline requirement."
which feels like you didn't read this part *at* *all*:
| A special tool is *always* a burden. You need to build it, supply
| it, make sure it is installable on the target system and so on.
|
| And I'm telling you this with my Linux distributor hat on. It is always
| a pain - trust me.
|
| For example, there's a reason why you can still control ftrace from the
| command line and you don't need any tool. You *can* use a tool but you
| don't have to. IOW, the KISS philosophy.
So now I ended up pasting practically the most of my text again.
Why?
Because your reply doesn't give me *any* signs that you actually read
what I said.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sun, Nov 13, 2022 at 09:55:00AM -0800, Joseph, Jithu wrote:
> Then we were told not to specify a filename via sysfs file (apologies
> for being repetitive)
Yeah, I'm unclear on why that is either and am hoping that Greg will
clarify. He fears that there will be file path resolution which I'm not
even thinking about.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sunday, 13 November 2022 08:58:17 PST Borislav Petkov wrote:
> * if f/m/s matches, you execute
>
> if still within the timeout, you return -EAGAIN from
> current_batch_store() to tell userspace, take a nap and try again.
>
> * if the f/m/s doesn't match you return -EINVAL to say, wrong filename,
> try the next one.
But if it matches but is corrupt or the HW fails to accept it, you also get an
error. So you now need to differentiate a failure to load a candidate file and
an attempt to load a non-candidate.
I'm assuming that the kernel would provide different error conditions for
those. But handling those in shell scripting is very difficult: you'd need to
start a subshell and parse stderr. It's MUCH easier in C, of course, but
incrementing a number is magnitudes easier in C than performing a globbing.
Either way, incrementing a number in shell is pretty easy too. The simplest
script with just the numbers would be:
i=0
while echo $i > /sys/devices/virtual/misc/intel_ifs_0/current_batch; do
test_all_cpus
done
It's four lines and does not need to know about where the scan files are
located, how they're named and if some files it may find are not to be used. But
I've hidden a lot of complexity in the test_all_cpus shell function, which
would be common to either solution of how we specify the batch to be loaded.
And this is part of my argument of why it's unlikely people will use their
shells to do this. That shell function is easily another 10 lines of
scripting, if it's meant to do its job properly. To make that easier, we've
developed two tools, one of them the OpenDCDiag tool I linked to, but both
just happen to be written in C and C++ instead of shell.
> For all Intel employees here on the thread, there's a world outside
> Intel and people do not talk (family model stepping) tuples like we do.
> All they wanna do is run their damn tests.
Indeed they do. I have personally been in contact with the few that will
represent over 90% of the deployment of this feature for the next few years.
They want this functionality to integrate with their existing health-check
scanning methodology. This is where OpenDCDiag comes in, because it does
integrate with their workflows, like logging. For another example, it obeys the
cpuset that the parent process may have set with sched_setaffinity() or alike
tools (taskset(1) or schedtool(8)). I have zero clue how to do that with shell
scripting.
Which actually means I am the maintainer of the tool that is going to be
driving 99% or more of all scans (that's why I was cc'ed in the submission). I
am your user.
I'm not saying I am the only user. I definitely want to see the best interface
so that others could write tools too if they want to. And I don't want there
to be a kludge that we need to keep compatibility with for a decade, or to set
a bad precedent. But I am giving you the constraints that I need to work under
and the kernel interface to support me. I am telling you this is very good
right now and your proposal makes it worse for me, not better, for little
apparent gain.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Cloud Software Architect - Intel DCAI Cloud Engineering
On Sun, Nov 13, 2022 at 07:27:26PM +0100, Borislav Petkov wrote:
> On Sun, Nov 13, 2022 at 09:55:00AM -0800, Joseph, Jithu wrote:
> > Then we were told not to specify a filename via sysfs file (apologies
> > for being repetitive)
>
> Yeah, I'm unclear on why that is either and am hoping that Greg will
> clarify. He fears that there will be file path resolution which I'm not
> even thinking about.
Summarizing the competing proposals here:
Option 1 (patches as currently posted)
User writes the batch number to the sysfs file:
# echo 4 > /sys/devices/virtual/misc/intel_ifs_0/current_batch
Driver turns that into a *partial* path (with test type, family-model-stepping
and batch number filled in):
"intel/ifs_%d/%02x-%02x-%02x-%02x.scan"
Feeds that to request_firmware_direct() (which looks in /lib/firmware)
Option 2 (proposed by Boris)
User writes a filename to the sysfs file:
# echo 06-8f-06-04.scan > /sys/devices/virtual/misc/intel_ifs_0/current_batch
Driver parses that:
If family-mode-stepping does not match current CPU, then
fail with -EINVAL
If filename doesn't end with a ".scan" suffix, also
fails with -EINVAL
Otherwise proceeds in similar manner to above. Constructs partial
pathname (just fills in test type and filename:
"intel/ifs_%d/%s"
Feeds that to request_firmware_direct() (which looks in /lib/firmware)
IMHO option 1 is following the microcode precedent of having the kernel
construct the filename based on the {x86,model,stepping} fields of
struct cpuinfo_x86.
I think option 2 isn't really doing the user any favors. Having them
feed all the *.scan files they find in /lib/firmware/intel/ifs_0 to the
driver to see which ones work becomes progressively worse in every CPU
generation. Any script/app running tests is likely to do the ff-mm-ss
filter itself ... so why have the kernel do it too?
-Tony
On Sunday, 13 November 2022 07:58:52 PST Borislav Petkov wrote:
> * simply try *all* files in a directory
By the way, we don't want that.
It's possible that different steppings of the same generation will have the
same test scan files, with the extended signature informing that they are valid
for this stepping too (see find_matching_signature())), because these files are
going to be pretty big, in the order of a hundred MB each. That means we will
either see symlinked or hardlinked files in the directory.
If you blindly try them all, you're going to spend twice or three times as
long as necessary to complete the scan. With the timeout in question for at
least Sapphire Rapids, we're talking about a difference measured in hours.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Cloud Software Architect - Intel DCAI Cloud Engineering
On Sunday, 13 November 2022 03:48:40 PST Borislav Petkov wrote:
> > You can't do it with a three-line shell script, but we're not
> > expecting that shell scripts are the means to use this feature in the
> > first place.
>
> I consider it a serious design mistake of having to have a *special*
> tool. A special tool is *always* a burden. You need to build it, supply
> it, make sure it is installable on the target system and so on.
>
> And I'm telling you this with my Linux distributor hat on. It is always
> a pain - trust me.
>
[cut]
> IOW, I really think that designing our interfaces with user friendliness
> in mind should be of much more higher importance. And requiring the user
> to remember or figure out anything she doesn't really need to, in order
> to run the test is a burden.
We agree that it should be operable without a tool. If nothing else, we will
run into situations where we need to debug what's happening and the tool is
not going to work for those conditions. Having a direct access to the API
right there in /sys is great and is one of the things that Linux excels at and
differentiates itself from the competition with.
However, I am saying that we shouldn't have to go out of our way to make the
extreme corner case easy if it comes to the detriment of the 99% case.
Anyway, we can update the tool to print "%02x-%02x-%02x-%02x.%s" instead of
"%d". That's trivial to us. I just don't think it's a worthwhile change,
because four of the five placeholders there are enforced by the kernel and
therefore the kernel must check them again (maybe it does so anyway when it
opens and validates the file).
What REALLY matters to me is that the current_batch file be readable and I can
get the last batch that was loaded, in a well-known format. Without this, we
go from "inconvenience" to "major design change, must talk to customers and
customers must adapt their workloads". Please help me out here.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Cloud Software Architect - Intel DCAI Cloud Engineering
On Sun, Nov 13, 2022 at 01:21:41PM -0800, Thiago Macieira wrote:
> I'm assuming that the kernel would provide different error conditions for
> those. But handling those in shell scripting is very difficult: you'd need to
> start a subshell and parse stderr.
You call that "very difficult"?!
> And this is part of my argument of why it's unlikely people will use their
> shells to do this. That shell function is easily another 10 lines of
> scripting, if it's meant to do its job properly. To make that easier, we've
> developed two tools, one of them the OpenDCDiag tool I linked to, but both
> just happen to be written in C and C++ instead of shell.
I've told you already that having a special tool is always more
problematic. I guess you'll make your own experience distributing it but
if you see pushback and people start complaining, you shouldn't wonder
why.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sun, Nov 13, 2022 at 01:33:50PM -0800, Tony Luck wrote:
> IMHO option 1 is following the microcode precedent of having the kernel
> construct the filename based on the {x86,model,stepping} fields of
> struct cpuinfo_x86.
You can't follow the microcode precedent if you have more than one file.
> I think option 2 isn't really doing the user any favors. Having them
> feed all the *.scan files they find in /lib/firmware/intel/ifs_0 to the
> driver to see which ones work becomes progressively worse in every CPU
> generation. Any script/app running tests is likely to do the ff-mm-ss
> filter itself ... so why have the kernel do it too?
So you think that feeding a sequence number is more intuitive?
I guess I've given all my arguments here but you folks don't see things
this way.
I guess time will show whether the sequence number thing was a good
idea.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sun, Nov 13, 2022 at 01:40:56PM -0800, Thiago Macieira wrote:
> On Sunday, 13 November 2022 07:58:52 PST Borislav Petkov wrote:
> > * simply try *all* files in a directory
>
> By the way, we don't want that.
>
> It's possible that different steppings of the same generation will have the
> same test scan files, with the extended signature informing that they are valid
> for this stepping too (see find_matching_signature())), because these files are
> going to be pretty big, in the order of a hundred MB each. That means we will
> either see symlinked or hardlinked files in the directory.
>
> If you blindly try them all, you're going to spend twice or three times as
> long as necessary to complete the scan. With the timeout in question for at
> least Sapphire Rapids, we're talking about a difference measured in hours.
You either have files which are valid and which will get run on the CPU
or those which will fail the header check. The header check happens in
msecs.
So I have no clue what "hours" you're talking about here.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sun, Nov 13, 2022 at 01:51:56PM -0800, Thiago Macieira wrote:
> Anyway, we can update the tool to print "%02x-%02x-%02x-%02x.%s" instead of
> "%d". That's trivial to us. I just don't think it's a worthwhile change,
As I wrote to Tony, "I guess time will show whether the sequence number
thing was a good idea."
Just remember that changing a user-visible interface after the fact is a
lot harder.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sun, Nov 13, 2022 at 12:48:40PM +0100, Borislav Petkov wrote:
> Replying to both with one mail because it still feels like there's a
> misunderstanding.
>
> On Sun, Nov 13, 2022 at 08:37:32AM +0100, [email protected] wrote:
> > No, please do not force the driver to resolve a filename path in the
> > kernel.
>
> No, I don't mean to do any filename path resolving - all I suggest is to
> echo into sysfs the full filename instead of the number. I.e., this:
>
> for i in $(ls /lib/firmware/intel/ifs_0/*.scan);
> do
> echo $i /sys/devices/virtual/misc/intel_ifs_0/current_batch
> done
Sorry, yes, that is fine, I was objecting to the previous "write any
path/file to the sysfs entry and the kernel will parse it" that was
happening in the original series. A filename, without a path, that
always loads from the existing in-kernel firmware path locations is
fine.
thanks,
greg k-h
Hi,
On 11/14/22 00:05, Borislav Petkov wrote:
> On Sun, Nov 13, 2022 at 01:51:56PM -0800, Thiago Macieira wrote:
>> Anyway, we can update the tool to print "%02x-%02x-%02x-%02x.%s" instead of
>> "%d". That's trivial to us. I just don't think it's a worthwhile change,
>
> As I wrote to Tony, "I guess time will show whether the sequence number
> thing was a good idea."
Just for the record: I've read the entire thread and I'm fine with doing
things either way.
If I understand this last email correctly then the plan is to move
ahead with the patches as-is, with writing only the batch-number and
have the kernel create the filename. This was and still is fine with me.
Regards,
Hans
On Mon, Nov 14, 2022 at 08:15:58AM +0100, [email protected] wrote:
> On Sun, Nov 13, 2022 at 12:48:40PM +0100, Borislav Petkov wrote:
> > Replying to both with one mail because it still feels like there's a
> > misunderstanding.
> >
> > On Sun, Nov 13, 2022 at 08:37:32AM +0100, [email protected] wrote:
> > > No, please do not force the driver to resolve a filename path in the
> > > kernel.
> >
> > No, I don't mean to do any filename path resolving - all I suggest is to
> > echo into sysfs the full filename instead of the number. I.e., this:
> >
> > for i in $(ls /lib/firmware/intel/ifs_0/*.scan);
> > do
> > echo $i /sys/devices/virtual/misc/intel_ifs_0/current_batch
Bug ... $i is a full path here. I think Boris meant:
echo ${i##*/} > /sys/devices/virtual/misc/intel_ifs_0/current_batch
> > done
>
> Sorry, yes, that is fine, I was objecting to the previous "write any
> path/file to the sysfs entry and the kernel will parse it" that was
> happening in the original series. A filename, without a path, that
> always loads from the existing in-kernel firmware path locations is
> fine.
Just to set the record straight, the previous patch did *not* allow any
path/file. It seems that you misread that original patch.
Whole thing is here:
https://lore.kernel.org/all/[email protected]/
But the important bits are in the commit comment:
Change the semantics of the "reload" file. Writing "1" keeps the legacy
behavior to reload from the default "ff-mm-ss.scan" file, but now interpret
other strings as a filename to be loaded from the /lib/firmware/intel/ifs
directory.
and the code:
+ if (strchr(file_name, '/'))
+ goto done;
Is there some other function/macro that we should have used to check
that user input was a filename and not a pathname that would have made
this clearer?
I do agree that overloading semantics of the "reload" was icky. Changing
the name of the sysfs file and dropping the "1" means reload default has
made this a better interface.
-Tony
On Mon, Nov 14, 2022 at 07:33:40AM -0800, Tony Luck wrote:
> Bug ... $i is a full path here. I think Boris meant:
Kinda.
I meant:
for i in $(ls /lib/firmware/intel/ifs_0/) ...
initially. Note the missing glob at the end. This gives the filenames
only, without the path. But then I added "*.scan" without testing it.
The "*.scan" thing would've been redundant too if we did what I was
suggesting...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
> So, what's the point of the sscanf() to check the *filename* other than
> saving some potentially expensive request_firmware() calls?
Not much point. There are two subsequent checks. First the driver checks
the F-M-S in the header of the file ... so your dastardly user will be thwarted
by this check.
If an even more dastardly user edited the file to have the right F-M-S (and
recomputed the file checksum) then the driver would be fooled, but microcode
would see that the signed binary portion is not for this CPU and reject it.
-Tony
On 11/13/22 07:58, Borislav Petkov wrote:
> On Sun, Nov 13, 2022 at 07:15:03AM -0800, Ashok Raj wrote:
>> Do you expect the /lib/firmware/intel/ifs_0/ to contain *ONLY* files for
>> this platform? For microcode we have everything in the public release
>> included here.
> Same as microcode, as I said further down in my mail:
>
> "And, ofcourse it would check the format of that string against family,
> model, stepping and sequence number (btw this way you drop your
> limitation of 256 for the sequence number which you don't really need
> either)."
Maybe dumb question, but what's the point of even checking the
filenames? They're meaningless.
Let's say we're on model=1,family=2,stepping=3. We try to load test #99:
01-02-03-99.scan
The kernel goes and does the sscanf() and checks "01", "02", etc...
Everything is fine. The header checks on the .scan file are OK. Life
is good. No harm no foul.
Then, some dastardly user does this:
mv 04-05-06-99.scan 01-02-03-99.scan
Taking an evil model=4,family=5,stepping=6 .scan file and trying to load
it. It will *pass* the sscanf() checks. But, will fail the metadata
checks. The kernel wasted the effort of requesting the file, but
there's no harm to anything.
So, what's the point of the sscanf() to check the *filename* other than
saving some potentially expensive request_firmware() calls?
> Now someone comes along and changes them all to x-y, where both x and y
> are > 6. Or removes the sequence numbers completely.
While there are system admins who might want to deliberately sabotage the
system they are responsible for ... let's not worry too much about them. They
have root access and can find a million other ways to break things.
-Tony
On Mon, Nov 14, 2022 at 07:07:47PM +0000, Luck, Tony wrote:
> > Now someone comes along and changes them all to x-y, where both x and y
> > are > 6. Or removes the sequence numbers completely.
>
> While there are system admins who might want to deliberately sabotage the
> system they are responsible for ... let's not worry too much about them. They
> have root access and can find a million other ways to break things.
It doesn't have to be a deliberate sabotage but just plain old
sloppiness. Or some wild renaming when handing in the directory into
guests with bind mounts or some weird setup or whatnot.
You're making these sequence numbers unnecessarily magical.
And they don't need to be.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Nov 14, 2022 at 10:13:44AM -0800, Dave Hansen wrote:
> So, what's the point of the sscanf() to check the *filename* other than
> saving some potentially expensive request_firmware() calls?
The point is to do a first sanity check. The other tests are of course
mandatory.
Which brings me to another bastardly idea:
Let's say you have sequence numbers 0-6.
Now someone comes along and changes them all to x-y, where both x and y
are > 6. Or removes the sequence numbers completely.
Now you go and echo your sequence number into current_batch and nothing
happens. But the sequences *are* there - just not named in a magic way.
Now *my* idea would work because you don't care what the filenames are.
But you don't want that and you want do some silly sequence numbers
which the user has to go and connect with what's in the directory.
If this were microcode and arch/x86/, I would've never accepted it.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
> You're making these sequence numbers unnecessarily magical.
> And they don't need to be.
I see them as opaque tokens. User asks to load "test set 2" with "echo 2 > current_batch"
and the driver finds the file for the ff-mm-ss that contains batch 2 of tests.
Numbers are plausible tokens - except for the sequence implication ... you don't
have to start at batch 1, and you don't need to go in any particular order. But realistically
people will run {1..N} in order, and there's no harm if they think they have to do that.
For Thiago's use case, numbers are better than filenames because the container running
the test may not have access to the directory to find the filenames.
But if this is the only roadblock to taking this patch series, then we can switch to filenames
and make Thiago find some way for the container to be given the list of tests to run in the
form of filenames.
-Tony
On Mon, Nov 14, 2022 at 07:38:35PM +0000, Luck, Tony wrote:
> But if this is the only roadblock to taking this patch series,
> then we can switch to filenames and make Thiago find some way for
> the container to be given the list of tests to run in the form of
> filenames.
I'm not putting any roadblocks as this is not the area I maintain. I'm
just saying that this whole scheme looks fragile to me.
I.e., I'm just playing the devil's advocate and pointing out the issues
this scheme could have. It might not but it could - we can't know of all
the possible future use cases.
So this is all your call as far as I'm concerned. Hans was fine either
way.
I'll take the next revision through tip as we agreed with Hans and after
the other, minor issues I've pointed out have been taken care of.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Changes in v3
- Rebased ontop of v6.1-rc5
- Added reviewed-by tags from Sohil and Hans
Boris
- Moved dynamic memory allocation from scan_chunks_sanity_check()
to driver init (patch 4)
- Split v2's Expose microcode_sanity_check(), into 2 separate patches
(patch 7 and patch8).
- Add kerneldoc style comment to intel_microcode_sanity_check() and
change parameter name to hdr_type instead of hdr_ver (patch 8)
- Remove ifs_ prefix from static functions (patch 10, patch11)
- Rename macro names more appropriately (patch10, patch8, patch12)
- Remove obvious "what" from certain commit messages
- Fix typos and wordings
Dave
- Use union to describe ifs meta_data structure and use u32 types
Sohil
- Use unsigned int to store current_batch (patch 11, patch 14)
- Fix an inadvertent mistake in microcode_sanity_check (patch 7)
- Fix wordings
v2 patches available from
Link: https://lore.kernel.org/lkml/[email protected]/
Changes in v2
- Rebased ontop of v6.1-rc4
Boris
- Moved exported functions (microcode_sanity_check(),
find_matching_signature ) from microcode/intel.c to cpu/intel.c
(patch4,6)
- Removed microcode metadata specific code changes to
microcode_sanity_check() (patch6)
- Moved find_meta_data() from common to IFS driver (Patch 8)
Sohil
- Dropped portions of Patch2 from v1 and folded remaining to Patch12
- Rewording multiple commits
- Avoid meta prefix in fields of metadata_header (patch8)
- Defining MICROCODE_HEADER_VER* into common location (patch6, 9)
- Elaborating error messages w.r.t processor flags (patch9)
- Changed sysfs_emit() parameter (patch 11)
v1 patches available from
Link: https://lore.kernel.org/lkml/[email protected]/
Initial implementation of IFS driver assumed a single IFS test image
file with a fixed name.
Subsequently, it became evident that supporting more than one
test image file is needed to provide more comprehensive
test coverage. (Test coverage in this scenario refers to testing
more transistors in the core to identify faults).
This series makes the driver aware of multiple scan test image files,
modifies IFS test image file headers to make it fully compatible
with microcode headers and adds a few other bug fixes and changes.
Patch organization:
Patches 1, and 2: bug fixes
Patch 3: Removes image loading during init path
Patch 4: Move memory allocation from load to init path
Patch 5, 6, 7, 8: exports a couple of microcode/intel.c functions
for use by driver
Rest of them are IFS driver changes
Patch 9,10 and 12: microcode/IFS metadata section related
Patches 11: IFS header format changes to make it fully compatible
to intel microcode header format
Patches 13, 14 and 15: native support for multiple scan test image files
Patch 16: reverts the broken flag
Ashok Raj (1):
platform/x86/intel/ifs: Add metadata support
Jithu Joseph (15):
platform/x86/intel/ifs: Remove unused selection
platform/x86/intel/ifs: Return a more appropriate Error code
platform/x86/intel/ifs: Remove image loading during init
platform/x86/intel/ifs: Remove memory allocation from load path
x86/microcode/intel: Reuse find_matching_signature()
x86/microcode/intel: Use appropriate type in microcode_sanity_check()
x86/microcode/intel: Reuse microcode_sanity_check()
x86/microcode/intel: Add hdr_type to intel_microcode_sanity_check()
x86/microcode/intel: Use a reserved field for metasize
platform/x86/intel/ifs: Use generic microcode headers and functions
platform/x86/intel/ifs: Add metadata validation
platform/x86/intel/ifs: Remove reload sysfs entry
platform/x86/intel/ifs: Add current_batch sysfs entry
Documentation/ABI: Update IFS ABI doc
Revert "platform/x86/intel/ifs: Mark as BROKEN"
arch/x86/include/asm/cpu.h | 2 +
arch/x86/include/asm/microcode_intel.h | 5 +-
drivers/platform/x86/intel/ifs/ifs.h | 27 ++-
arch/x86/kernel/cpu/intel.c | 141 +++++++++++
arch/x86/kernel/cpu/microcode/intel.c | 146 +-----------
drivers/platform/x86/intel/ifs/core.c | 14 +-
drivers/platform/x86/intel/ifs/load.c | 218 ++++++++++--------
drivers/platform/x86/intel/ifs/runtest.c | 10 +-
drivers/platform/x86/intel/ifs/sysfs.c | 41 ++--
.../ABI/testing/sysfs-platform-intel-ifs | 30 +--
drivers/platform/x86/intel/ifs/Kconfig | 4 -
11 files changed, 356 insertions(+), 282 deletions(-)
base-commit: 094226ad94f471a9f19e8f8e7140a09c2625abaa
--
2.25.1
Issues with user interface [1] to load scan test images
has been addressed, so the following can be reverted.
commit c483e7ea10fa ("platform/x86/intel/ifs: Mark as BROKEN")
Link: https://lore.kernel.org/lkml/[email protected]/ [1]
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Sohil Mehta <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/Kconfig | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig
index 89152d46deee..3eded966757e 100644
--- a/drivers/platform/x86/intel/ifs/Kconfig
+++ b/drivers/platform/x86/intel/ifs/Kconfig
@@ -1,9 +1,6 @@
config INTEL_IFS
tristate "Intel In Field Scan"
depends on X86 && CPU_SUP_INTEL && 64BIT && SMP
- # Discussion on the list has shown that the sysfs API needs a bit
- # more work, mark this as broken for now
- depends on BROKEN
help
Enable support for the In Field Scan capability in select
CPUs. The capability allows for running low level tests via
--
2.25.1
IFS test images and microcode blobs use the same header format.
Microcode blobs use header type of 1, whereas IFS test images
will use header type of 2.
In preparation for IFS reusing intel_microcode_sanity_check(),
add header type as a parameter for sanity check.
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Ashok Raj <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
arch/x86/include/asm/cpu.h | 2 +-
arch/x86/include/asm/microcode_intel.h | 1 +
arch/x86/kernel/cpu/intel.c | 19 ++++++++++++++++---
arch/x86/kernel/cpu/microcode/intel.c | 4 ++--
4 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index 9e3ac95acf2d..78796b98a544 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -96,6 +96,6 @@ static inline bool intel_cpu_signatures_match(unsigned int s1, unsigned int p1,
extern u64 x86_read_arch_cap_msr(void);
int intel_find_matching_signature(void *mc, unsigned int csig, int cpf);
-int intel_microcode_sanity_check(void *mc, bool print_err);
+int intel_microcode_sanity_check(void *mc, bool print_err, int hdr_type);
#endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 4c92cea7e4b5..2a999bf91ef0 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -41,6 +41,7 @@ struct extended_sigtable {
#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
#define EXT_HEADER_SIZE (sizeof(struct extended_sigtable))
#define EXT_SIGNATURE_SIZE (sizeof(struct extended_signature))
+#define MC_HEADER_TYPE_MICROCODE 1
#define get_totalsize(mc) \
(((struct microcode_intel *)mc)->hdr.datasize ? \
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 01e73ec1d585..ff305e3784d1 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -245,7 +245,19 @@ int intel_find_matching_signature(void *mc, unsigned int csig, int cpf)
}
EXPORT_SYMBOL_GPL(intel_find_matching_signature);
-int intel_microcode_sanity_check(void *mc, bool print_err)
+/**
+ * intel_microcode_sanity_check() - Sanity check microcode file.
+ * @mc: Pointer to the microcode file contents.
+ * @print_err: Display failure reason if true, silent if false.
+ * @hdr_type: Type of file, i.e. normal microcode file or In Field Scan file.
+ * Validate if the microcode header type matches with the type specified here.
+ *
+ * Validate certain header fields and verify if computed checksum matches
+ * with the one specified in the header.
+ *
+ * Return: 0 if the file passes all the checks, -ERR for invalid files
+ */
+int intel_microcode_sanity_check(void *mc, bool print_err, int hdr_type)
{
unsigned long total_size, data_size, ext_table_size;
struct microcode_header_intel *mc_header = mc;
@@ -262,9 +274,10 @@ int intel_microcode_sanity_check(void *mc, bool print_err)
return -EINVAL;
}
- if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
+ if (mc_header->ldrver != 1 || mc_header->hdrver != hdr_type) {
if (print_err)
- pr_err("Error: invalid/unknown microcode update format.\n");
+ pr_err("Error: invalid/unknown microcode update format. Header type %d\n",
+ mc_header->hdrver);
return -EINVAL;
}
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index af7134073e65..bf8026304f6f 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -155,7 +155,7 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
mc_size = get_totalsize(mc_header);
if (!mc_size ||
mc_size > size ||
- intel_microcode_sanity_check(data, false) < 0)
+ intel_microcode_sanity_check(data, false, MC_HEADER_TYPE_MICROCODE) < 0)
break;
size -= mc_size;
@@ -694,7 +694,7 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
memcpy(mc, &mc_header, sizeof(mc_header));
data = mc + sizeof(mc_header);
if (!copy_from_iter_full(data, data_size, iter) ||
- intel_microcode_sanity_check(mc, true) < 0) {
+ intel_microcode_sanity_check(mc, true, MC_HEADER_TYPE_MICROCODE) < 0) {
break;
}
--
2.25.1
Initial implementation assumed a single IFS test image file with a
fixed name ff-mm-ss.scan. (where ff, mm, ss refers to family, model
and stepping of the core)
Subsequently, it became evident that supporting more than one
test image file is needed to provide more comprehensive
test coverage. (Test coverage in this scenario refers to testing
more transistors in the core to identify faults)
The other alternative of increasing the size of a single scan test image
file would not work as the upper bound is limited by the size of memory
area reserved by BIOS for loading IFS test image.
Introduce "current_batch" file which accepts a number. Writing a
number to the current_batch file would load the test image file by name
ff-mm-ss-<xy>.scan, where <xy> is the number written to the
"current_batch" file in hex. Range check of the input is done to verify
it not greater than 0xff.
For e.g if the scan test image comprises of 6 files, they would be named
as show below:
06-8f-06-01.scan
06-8f-06-02.scan
06-8f-06-03.scan
06-8f-06-04.scan
06-8f-06-05.scan
06-8f-06-06.scan
And writing 3 to current_batch would result in loading 06-8f-06-03.scan
in the above e.g. The file can also be read to know the currently loaded
file.
And testing a system looks like:
for each scan file
do
load the IFS test image file (write to the batch file)
for each core
do
test the core with this set of tests
done
done
Qualify few error messages with the test image file suffix to
provide better context.
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Sohil Mehta <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/ifs.h | 23 ++++++++++----
drivers/platform/x86/intel/ifs/core.c | 1 +
drivers/platform/x86/intel/ifs/load.c | 18 +++++++----
drivers/platform/x86/intel/ifs/runtest.c | 10 ++++---
drivers/platform/x86/intel/ifs/sysfs.c | 38 ++++++++++++++++++++++++
5 files changed, 74 insertions(+), 16 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index e3e8210ebd57..6ebedff4d6b8 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -33,13 +33,23 @@
* The driver loads the tests into memory reserved BIOS local to each CPU
* 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::
+ * feedback on the success/failure of these steps.
*
- * # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload
+ * The test files are kept in a fixed location: /lib/firmware/intel/ifs_0/
+ * For e.g if there are 3 test files, they would be named in the following
+ * fashion:
+ * ff-mm-ss-01.scan
+ * ff-mm-ss-02.scan
+ * ff-mm-ss-03.scan
+ * (where ff refers to family, mm indicates model and ss indicates stepping)
*
- * 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
+ * A different testfile can be loaded by writing the numerical portion
+ * (e.g 1, 2 or 3 in the above scenario) into the curent_batch file.
+ * To load ff-mm-ss-02.scan, the following command can be used::
+ *
+ * # echo 2 > /sys/devices/virtual/misc/intel_ifs_0/current_batch
+ *
+ * The above file can also be read to know the currently loaded image.
*
* Running tests
* -------------
@@ -207,6 +217,7 @@ struct ifs_data {
int status;
u64 scan_details;
u32 cur_batch;
+ int test_num;
};
struct ifs_work {
@@ -227,7 +238,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);
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 4b39f2359180..c74cd8138ee6 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -23,6 +23,7 @@ MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
static struct ifs_device ifs_device = {
.data = {
.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
+ .test_num = 0,
},
.misc = {
.name = "intel_ifs_0",
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 3f1de9d4cf4b..74a50e99cacd 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -253,17 +253,18 @@ static int image_sanity_check(struct device *dev, const struct microcode_header_
/*
* 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 /lib/firmware/intel/ifs_x/ and named as family-model-stepping-02x.{testname}.
*/
-void ifs_load_firmware(struct device *dev)
+int ifs_load_firmware(struct device *dev)
{
struct ifs_data *ifsd = ifs_get_data(dev);
const struct firmware *fw;
- char scan_path[32];
- int ret;
+ char scan_path[64];
+ int ret = -EINVAL;
- 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);
+ snprintf(scan_path, sizeof(scan_path), "intel/ifs_%d/%02x-%02x-%02x-%02x.scan",
+ ifsd->test_num, boot_cpu_data.x86, boot_cpu_data.x86_model,
+ boot_cpu_data.x86_stepping, ifsd->cur_batch);
ret = request_firmware_direct(&fw, scan_path, dev);
if (ret) {
@@ -279,8 +280,13 @@ void ifs_load_firmware(struct device *dev)
ifs_hash_ptr = (u64)(ifs_header_ptr + 1);
ret = scan_chunks_sanity_check(dev);
+ if (ret)
+ dev_err(dev, "Load failure for batch: %02x\n", ifsd->cur_batch);
+
release:
release_firmware(fw);
done:
ifsd->loaded = (ret == 0);
+
+ return ret;
}
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index b2ca2bb4501f..0bfd8fcdd7e8 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -78,14 +78,16 @@ static void message_not_tested(struct device *dev, int cpu, union ifs_status sta
static void message_fail(struct device *dev, int cpu, union ifs_status status)
{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+
/*
* control_error is set when the microcode runs into a problem
* loading the image from the reserved BIOS memory, or it has
* been corrupted. Reloading the image may fix this issue.
*/
if (status.control_error) {
- dev_err(dev, "CPU(s) %*pbl: could not execute from loaded scan image\n",
- cpumask_pr_args(cpu_smt_mask(cpu)));
+ dev_err(dev, "CPU(s) %*pbl: could not execute from loaded scan image. Batch: %02x version: 0x%x\n",
+ cpumask_pr_args(cpu_smt_mask(cpu)), ifsd->cur_batch, ifsd->loaded_version);
}
/*
@@ -96,8 +98,8 @@ static void message_fail(struct device *dev, int cpu, union ifs_status status)
* the core being tested.
*/
if (status.signature_error) {
- dev_err(dev, "CPU(s) %*pbl: test signature incorrect.\n",
- cpumask_pr_args(cpu_smt_mask(cpu)));
+ dev_err(dev, "CPU(s) %*pbl: test signature incorrect. Batch: %02x version: 0x%x\n",
+ cpumask_pr_args(cpu_smt_mask(cpu)), ifsd->cur_batch, ifsd->loaded_version);
}
}
diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
index e077910c5d28..ee636a76b083 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -87,6 +87,43 @@ static ssize_t run_test_store(struct device *dev,
static DEVICE_ATTR_WO(run_test);
+static ssize_t current_batch_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+ unsigned int cur_batch;
+ int rc;
+
+ rc = kstrtouint(buf, 0, &cur_batch);
+ if (rc < 0 || cur_batch > 0xff)
+ return -EINVAL;
+
+ if (down_interruptible(&ifs_sem))
+ return -EINTR;
+
+ ifsd->cur_batch = cur_batch;
+
+ rc = ifs_load_firmware(dev);
+
+ up(&ifs_sem);
+
+ return (rc == 0) ? count : rc;
+}
+
+static ssize_t current_batch_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+
+ if (!ifsd->loaded)
+ return sysfs_emit(buf, "none\n");
+ else
+ return sysfs_emit(buf, "0x%02x\n", ifsd->cur_batch);
+}
+
+static DEVICE_ATTR_RW(current_batch);
+
/*
* Display currently loaded IFS image version.
*/
@@ -108,6 +145,7 @@ static struct attribute *plat_ifs_attrs[] = {
&dev_attr_details.attr,
&dev_attr_status.attr,
&dev_attr_run_test.attr,
+ &dev_attr_current_batch.attr,
&dev_attr_image_version.attr,
NULL
};
--
2.25.1
The following commit has been merged into the x86/microcode branch of tip:
Commit-ID: e0788c3281a72386e75b53a010de4bfbac7e80db
Gitweb: https://git.kernel.org/tip/e0788c3281a72386e75b53a010de4bfbac7e80db
Author: Jithu Joseph <[email protected]>
AuthorDate: Wed, 16 Nov 2022 19:59:27 -08:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Fri, 18 Nov 2022 22:08:19 +01:00
x86/microcode/intel: Add hdr_type to intel_microcode_sanity_check()
IFS test images and microcode blobs use the same header format.
Microcode blobs use header type of 1, whereas IFS test images
will use header type of 2.
In preparation for IFS reusing intel_microcode_sanity_check(),
add header type as a parameter for sanity check.
[ bp: Touchups. ]
Signed-off-by: Jithu Joseph <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Ashok Raj <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/include/asm/cpu.h | 2 +-
arch/x86/include/asm/microcode_intel.h | 1 +
arch/x86/kernel/cpu/intel.c | 21 ++++++++++++++++++---
arch/x86/kernel/cpu/microcode/intel.c | 4 ++--
4 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index 9e3ac95..78796b9 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -96,6 +96,6 @@ static inline bool intel_cpu_signatures_match(unsigned int s1, unsigned int p1,
extern u64 x86_read_arch_cap_msr(void);
int intel_find_matching_signature(void *mc, unsigned int csig, int cpf);
-int intel_microcode_sanity_check(void *mc, bool print_err);
+int intel_microcode_sanity_check(void *mc, bool print_err, int hdr_type);
#endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 4c92cea..2a999bf 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -41,6 +41,7 @@ struct extended_sigtable {
#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
#define EXT_HEADER_SIZE (sizeof(struct extended_sigtable))
#define EXT_SIGNATURE_SIZE (sizeof(struct extended_signature))
+#define MC_HEADER_TYPE_MICROCODE 1
#define get_totalsize(mc) \
(((struct microcode_intel *)mc)->hdr.datasize ? \
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index bef06a1..b6997eb 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -244,7 +244,21 @@ int intel_find_matching_signature(void *mc, unsigned int csig, int cpf)
}
EXPORT_SYMBOL_GPL(intel_find_matching_signature);
-int intel_microcode_sanity_check(void *mc, bool print_err)
+/**
+ * intel_microcode_sanity_check() - Sanity check microcode file.
+ * @mc: Pointer to the microcode file contents.
+ * @print_err: Display failure reason if true, silent if false.
+ * @hdr_type: Type of file, i.e. normal microcode file or In Field Scan file.
+ * Validate if the microcode header type matches with the type
+ * specified here.
+ *
+ * Validate certain header fields and verify if computed checksum matches
+ * with the one specified in the header.
+ *
+ * Return: 0 if the file passes all the checks, -EINVAL if any of the checks
+ * fail.
+ */
+int intel_microcode_sanity_check(void *mc, bool print_err, int hdr_type)
{
unsigned long total_size, data_size, ext_table_size;
struct microcode_header_intel *mc_header = mc;
@@ -261,9 +275,10 @@ int intel_microcode_sanity_check(void *mc, bool print_err)
return -EINVAL;
}
- if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
+ if (mc_header->ldrver != 1 || mc_header->hdrver != hdr_type) {
if (print_err)
- pr_err("Error: invalid/unknown microcode update format.\n");
+ pr_err("Error: invalid/unknown microcode update format. Header type %d\n",
+ mc_header->hdrver);
return -EINVAL;
}
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index fb6ff71..c4a00fb 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -155,7 +155,7 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
mc_size = get_totalsize(mc_header);
if (!mc_size ||
mc_size > size ||
- intel_microcode_sanity_check(data, false) < 0)
+ intel_microcode_sanity_check(data, false, MC_HEADER_TYPE_MICROCODE) < 0)
break;
size -= mc_size;
@@ -694,7 +694,7 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
memcpy(mc, &mc_header, sizeof(mc_header));
data = mc + sizeof(mc_header);
if (!copy_from_iter_full(data, data_size, iter) ||
- intel_microcode_sanity_check(mc, true) < 0) {
+ intel_microcode_sanity_check(mc, true, MC_HEADER_TYPE_MICROCODE) < 0) {
break;
}
The following commit has been merged into the x86/microcode branch of tip:
Commit-ID: 1a63b58082869273bfbab1b945007193f7bd3a78
Gitweb: https://git.kernel.org/tip/1a63b58082869273bfbab1b945007193f7bd3a78
Author: Jithu Joseph <[email protected]>
AuthorDate: Wed, 16 Nov 2022 19:59:35 -08:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Sat, 19 Nov 2022 11:31:20 +01:00
Revert "platform/x86/intel/ifs: Mark as BROKEN"
Issues with user interface [1] to load scan test images have been
addressed so remove the dependency on BROKEN.
Signed-off-by: Jithu Joseph <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Sohil Mehta <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/ [1]
Link: https://lore.kernel.org/r/[email protected]
---
drivers/platform/x86/intel/ifs/Kconfig | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig
index 89152d4..3eded96 100644
--- a/drivers/platform/x86/intel/ifs/Kconfig
+++ b/drivers/platform/x86/intel/ifs/Kconfig
@@ -1,9 +1,6 @@
config INTEL_IFS
tristate "Intel In Field Scan"
depends on X86 && CPU_SUP_INTEL && 64BIT && SMP
- # Discussion on the list has shown that the sysfs API needs a bit
- # more work, mark this as broken for now
- depends on BROKEN
help
Enable support for the In Field Scan capability in select
CPUs. The capability allows for running low level tests via
The following commit has been merged into the x86/microcode branch of tip:
Commit-ID: 4fb858f3dcd25cf568e35ff53ce8fa8a660fc372
Gitweb: https://git.kernel.org/tip/4fb858f3dcd25cf568e35ff53ce8fa8a660fc372
Author: Jithu Joseph <[email protected]>
AuthorDate: Wed, 16 Nov 2022 19:59:33 -08:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Sat, 19 Nov 2022 11:29:00 +01:00
platform/x86/intel/ifs: Add current_batch sysfs entry
Initial implementation assumed a single IFS test image file with a
fixed name ff-mm-ss.scan. (where ff, mm, ss refers to family, model and
stepping of the core).
Subsequently, it became evident that supporting more than one test
image file is needed to provide more comprehensive test coverage. (Test
coverage in this scenario refers to testing more transistors in the core
to identify faults).
The other alternative of increasing the size of a single scan test image
file would not work as the upper bound is limited by the size of memory
area reserved by BIOS for loading IFS test image.
Introduce "current_batch" file which accepts a number. Writing a
number to the current_batch file would load the test image file by
name ff-mm-ss-<xy>.scan, where <xy> is the number written to the
"current_batch" file in hex. Range check of the input is done to verify
it not greater than 0xff.
For e.g if the scan test image comprises of 6 files, they would be named:
06-8f-06-01.scan
06-8f-06-02.scan
06-8f-06-03.scan
06-8f-06-04.scan
06-8f-06-05.scan
06-8f-06-06.scan
And writing 3 to current_batch would result in loading 06-8f-06-03.scan
above. The file can also be read to know the currently loaded file.
And testing a system looks like:
for each scan file
do
load the IFS test image file (write to the batch file)
for each core
do
test the core with this set of tests
done
done
Qualify few error messages with the test image file suffix to provide
better context.
[ bp: Massage commit message. Add link to the discussion. ]
Signed-off-by: Jithu Joseph <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Sohil Mehta <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/platform/x86/intel/ifs/core.c | 1 +-
drivers/platform/x86/intel/ifs/ifs.h | 23 ++++++++++----
drivers/platform/x86/intel/ifs/load.c | 18 +++++++----
drivers/platform/x86/intel/ifs/runtest.c | 10 +++---
drivers/platform/x86/intel/ifs/sysfs.c | 38 +++++++++++++++++++++++-
5 files changed, 74 insertions(+), 16 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 943eb2a..206a617 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -23,6 +23,7 @@ MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
static struct ifs_device ifs_device = {
.data = {
.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
+ .test_num = 0,
},
.misc = {
.name = "intel_ifs_0",
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 74c051c..da1474e 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -33,13 +33,23 @@
* The driver loads the tests into memory reserved BIOS local to each CPU
* 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::
+ * feedback on the success/failure of these steps.
*
- * # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload
+ * The test files are kept in a fixed location: /lib/firmware/intel/ifs_0/
+ * For e.g if there are 3 test files, they would be named in the following
+ * fashion:
+ * ff-mm-ss-01.scan
+ * ff-mm-ss-02.scan
+ * ff-mm-ss-03.scan
+ * (where ff refers to family, mm indicates model and ss indicates stepping)
*
- * 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
+ * A different test file can be loaded by writing the numerical portion
+ * (e.g 1, 2 or 3 in the above scenario) into the curent_batch file.
+ * To load ff-mm-ss-02.scan, the following command can be used::
+ *
+ * # echo 2 > /sys/devices/virtual/misc/intel_ifs_0/current_batch
+ *
+ * The above file can also be read to know the currently loaded image.
*
* Running tests
* -------------
@@ -209,6 +219,7 @@ struct ifs_data {
int status;
u64 scan_details;
u32 cur_batch;
+ int test_num;
};
struct ifs_work {
@@ -229,7 +240,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);
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/load.c b/drivers/platform/x86/intel/ifs/load.c
index edc7baa..c5c24e6 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -253,17 +253,18 @@ static int image_sanity_check(struct device *dev, const struct microcode_header_
/*
* 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 /lib/firmware/intel/ifs_x/ and named as family-model-stepping-02x.{testname}.
*/
-void ifs_load_firmware(struct device *dev)
+int ifs_load_firmware(struct device *dev)
{
struct ifs_data *ifsd = ifs_get_data(dev);
const struct firmware *fw;
- char scan_path[32];
- int ret;
+ char scan_path[64];
+ int ret = -EINVAL;
- 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);
+ snprintf(scan_path, sizeof(scan_path), "intel/ifs_%d/%02x-%02x-%02x-%02x.scan",
+ ifsd->test_num, boot_cpu_data.x86, boot_cpu_data.x86_model,
+ boot_cpu_data.x86_stepping, ifsd->cur_batch);
ret = request_firmware_direct(&fw, scan_path, dev);
if (ret) {
@@ -279,8 +280,13 @@ void ifs_load_firmware(struct device *dev)
ifs_hash_ptr = (u64)(ifs_header_ptr + 1);
ret = scan_chunks_sanity_check(dev);
+ if (ret)
+ dev_err(dev, "Load failure for batch: %02x\n", ifsd->cur_batch);
+
release:
release_firmware(fw);
done:
ifsd->loaded = (ret == 0);
+
+ return ret;
}
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index b2ca2bb..0bfd8fc 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -78,14 +78,16 @@ static void message_not_tested(struct device *dev, int cpu, union ifs_status sta
static void message_fail(struct device *dev, int cpu, union ifs_status status)
{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+
/*
* control_error is set when the microcode runs into a problem
* loading the image from the reserved BIOS memory, or it has
* been corrupted. Reloading the image may fix this issue.
*/
if (status.control_error) {
- dev_err(dev, "CPU(s) %*pbl: could not execute from loaded scan image\n",
- cpumask_pr_args(cpu_smt_mask(cpu)));
+ dev_err(dev, "CPU(s) %*pbl: could not execute from loaded scan image. Batch: %02x version: 0x%x\n",
+ cpumask_pr_args(cpu_smt_mask(cpu)), ifsd->cur_batch, ifsd->loaded_version);
}
/*
@@ -96,8 +98,8 @@ static void message_fail(struct device *dev, int cpu, union ifs_status status)
* the core being tested.
*/
if (status.signature_error) {
- dev_err(dev, "CPU(s) %*pbl: test signature incorrect.\n",
- cpumask_pr_args(cpu_smt_mask(cpu)));
+ dev_err(dev, "CPU(s) %*pbl: test signature incorrect. Batch: %02x version: 0x%x\n",
+ cpumask_pr_args(cpu_smt_mask(cpu)), ifsd->cur_batch, ifsd->loaded_version);
}
}
diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
index e077910..ee636a7 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -87,6 +87,43 @@ static ssize_t run_test_store(struct device *dev,
static DEVICE_ATTR_WO(run_test);
+static ssize_t current_batch_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+ unsigned int cur_batch;
+ int rc;
+
+ rc = kstrtouint(buf, 0, &cur_batch);
+ if (rc < 0 || cur_batch > 0xff)
+ return -EINVAL;
+
+ if (down_interruptible(&ifs_sem))
+ return -EINTR;
+
+ ifsd->cur_batch = cur_batch;
+
+ rc = ifs_load_firmware(dev);
+
+ up(&ifs_sem);
+
+ return (rc == 0) ? count : rc;
+}
+
+static ssize_t current_batch_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ifs_data *ifsd = ifs_get_data(dev);
+
+ if (!ifsd->loaded)
+ return sysfs_emit(buf, "none\n");
+ else
+ return sysfs_emit(buf, "0x%02x\n", ifsd->cur_batch);
+}
+
+static DEVICE_ATTR_RW(current_batch);
+
/*
* Display currently loaded IFS image version.
*/
@@ -108,6 +145,7 @@ static struct attribute *plat_ifs_attrs[] = {
&dev_attr_details.attr,
&dev_attr_status.attr,
&dev_attr_run_test.attr,
+ &dev_attr_current_batch.attr,
&dev_attr_image_version.attr,
NULL
};