| Message ID | 20220506014035.1173578-9-tony.luck@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Introduce In Field Scan driver | expand |
On Thu, May 05 2022 at 18:40, Tony Luck wrote: > +/* > + * Note all code and data in this file is protected by > + * ifs_sem. On HT systems all threads on a core will > + * execute together, but only the first thread on the > + * core will update results of the test. > + */ > +struct workqueue_struct *ifs_wq; Seems to be unused. > +static bool oscan_enabled = true; What changes this? > +static void message_not_tested(struct device *dev, int cpu, union ifs_status status) > +{ > + if (status.error_code < ARRAY_SIZE(scan_test_status)) Please add curly brackets as these are not one-line statements. > + dev_info(dev, "CPU(s) %*pbl: SCAN operation did not start. %s\n", > + cpumask_pr_args(topology_sibling_cpumask(cpu)), > + scan_test_status[status.error_code]); > +/* > + * Execute the scan. Called "simultaneously" on all threads of a core > + * at high priority using the stop_cpus mechanism. > + */ > +static int doscan(void *data) > +{ > + int cpu = smp_processor_id(); > + u64 *msrs = data; > + int first; > + > + /* Only the first logical CPU on a core reports result */ > + first = cpumask_first(topology_sibling_cpumask(cpu)); Shouldn't that be cpu_smt_mask()? > + /* > + * This WRMSR will wait for other HT threads to also write > + * to this MSR (at most for activate.delay cycles). Then it > + * starts scan of each requested chunk. The core scan happens > + * during the "execution" of the WRMSR. This instruction can > + * take up to 200 milliseconds before it retires. 200ms per test chunk? > + */ > + wrmsrl(MSR_ACTIVATE_SCAN, msrs[0]); > + > + while (activate.start <= activate.stop) { > + if (time_after(jiffies, timeout)) { > + status.error_code = IFS_SW_TIMEOUT; > + break; > + } > + > + msrvals[0] = activate.data; > + stop_core_cpuslocked(cpu, doscan, msrvals); > + > + status.data = msrvals[1]; > + > + /* Some cases can be retried, give up for others */ > + if (!can_restart(status)) > + break; > + > + if (status.chunk_num == activate.start) { > + /* Check for forward progress */ > + if (retries-- == 0) { > + if (status.error_code == IFS_NO_ERROR) > + status.error_code = IFS_SW_PARTIAL_COMPLETION; > + break; > + } > + } else { > + retries = MAX_IFS_RETRIES; > + activate.start = status.chunk_num; > + } > + } Looks way better now. > +} > +/* > + * Initiate per core test. It wakes up work queue threads on the target cpu and > + * its sibling cpu. Once all sibling threads wake up, the scan test gets executed and > + * wait for all sibling threads to finish the scan test. > + */ > +int do_core_test(int cpu, struct device *dev) > +{ > + int ret = 0; > + > + if (!scan_enabled) > + return -ENXIO; > + > + /* Prevent CPUs from being taken offline during the scan test */ > + cpus_read_lock(); > + > + if (!cpu_online(cpu)) { > + dev_info(dev, "cannot test on the offline cpu %d\n", cpu); > + ret = -EINVAL; > + goto out; > + } Coming back to my points from the previous round: 1) How is that supposed to work on a system which has HT enabled in BIOS, but disabled on the kernel command line or via /sys/..../smt/control or when a HT sibling is offlined temporarily? I assume it cannot work, but I can't see anything which handles those cases. 2) That documentation for the admin/user got eaten by the gremlins in the intertubes again. Thanks, tglx
On Fri, May 06, 2022 at 03:30:30PM +0200, Thomas Gleixner wrote: > On Thu, May 05 2022 at 18:40, Tony Luck wrote: > > +/* > > + * Note all code and data in this file is protected by > > + * ifs_sem. On HT systems all threads on a core will > > + * execute together, but only the first thread on the > > + * core will update results of the test. > > + */ > > +struct workqueue_struct *ifs_wq; > > Seems to be unused. Missed deleting the definition after dropping all the users. Deleted now. > > +static bool oscan_enabled = true; > > What changes this? Code that changed this has been deleted (was to deal with a "can't happen" case where the kernel threads didn't set completion). Variable (and the remaing place that checked it) now deleted. > > +static void message_not_tested(struct device *dev, int cpu, union ifs_status status) > > +{ > > + if (status.error_code < ARRAY_SIZE(scan_test_status)) > > Please add curly brackets as these are not one-line statements. Done. > > + dev_info(dev, "CPU(s) %*pbl: SCAN operation did not start. %s\n", > > + cpumask_pr_args(topology_sibling_cpumask(cpu)), > > + scan_test_status[status.error_code]); > > +/* > > + * Execute the scan. Called "simultaneously" on all threads of a core > > + * at high priority using the stop_cpus mechanism. > > + */ > > +static int doscan(void *data) > > +{ > > + int cpu = smp_processor_id(); > > + u64 *msrs = data; > > + int first; > > + > > + /* Only the first logical CPU on a core reports result */ > > + first = cpumask_first(topology_sibling_cpumask(cpu)); > > Shouldn't that be cpu_smt_mask()? I guess so. It seems part of a maze of CONFIG options and #defines. The code worked because (except on power) cpu_smt_mask() is just an inline funtion that calls topology_sibling_cpumask(). I've changed this (and the other places that use topology_sibling_cpumask() to cpu_smt_mask(). Will probably save me from a randconfig build error some time in the future. > > + /* > > + * This WRMSR will wait for other HT threads to also write > > + * to this MSR (at most for activate.delay cycles). Then it > > + * starts scan of each requested chunk. The core scan happens > > + * during the "execution" of the WRMSR. This instruction can > > + * take up to 200 milliseconds before it retires. > > 200ms per test chunk? Updated comment to say that 200 ms is the time for all chunks. Note that the loop that calls here tries to do all (remaining) chunks on each iteration. Doing them 1 at a time would reduce the time each spends in stomp_machine(), but not as much as you'd like. Each WRMSR(ACTIVATE_SCAN)) has to save/restore the whole state of the core (similar to a C6 entry+exit). > > + */ > > + wrmsrl(MSR_ACTIVATE_SCAN, msrs[0]); > > + > > > + while (activate.start <= activate.stop) { > > + if (time_after(jiffies, timeout)) { > > + status.error_code = IFS_SW_TIMEOUT; > > + break; > > + } > > + > > + msrvals[0] = activate.data; > > + stop_core_cpuslocked(cpu, doscan, msrvals); > > + > > + status.data = msrvals[1]; > > + > > + /* Some cases can be retried, give up for others */ > > + if (!can_restart(status)) > > + break; > > + > > + if (status.chunk_num == activate.start) { > > + /* Check for forward progress */ > > + if (retries-- == 0) { > > + if (status.error_code == IFS_NO_ERROR) > > + status.error_code = IFS_SW_PARTIAL_COMPLETION; > > + break; > > + } > > + } else { > > + retries = MAX_IFS_RETRIES; > > + activate.start = status.chunk_num; > > + } > > + } > > Looks way better now. Thanks to you! > > +} > > +/* > > + * Initiate per core test. It wakes up work queue threads on the target cpu and > > + * its sibling cpu. Once all sibling threads wake up, the scan test gets executed and > > + * wait for all sibling threads to finish the scan test. > > + */ > > +int do_core_test(int cpu, struct device *dev) > > +{ > > + int ret = 0; > > + > > + if (!scan_enabled) > > + return -ENXIO; > > + > > + /* Prevent CPUs from being taken offline during the scan test */ > > + cpus_read_lock(); > > + > > + if (!cpu_online(cpu)) { > > + dev_info(dev, "cannot test on the offline cpu %d\n", cpu); > > + ret = -EINVAL; > > + goto out; > > + } > > Coming back to my points from the previous round: > > 1) How is that supposed to work on a system which has HT enabled in BIOS, > but disabled on the kernel command line or via /sys/..../smt/control or > when a HT sibling is offlined temporarily? > > I assume it cannot work, but I can't see anything which handles those > cases. Correct. If HT is disabled in BIOS, then there is no other thread, so core tests just use a single thread. If a logical CPU is "offline" due to Linux actions, then core test will fail. In an earlier version we did attempt to detect this before trying to run the test. But we didn't find a simple way to determine that a core has one thread online, and another offline. Rather than a bunch of code to detect an operator error it seemed better to let it run & fail. Here is what the user will see: # echo 45 > run_test # cat status untested # cat details 0x100008000 Console will see this message: misc intel_ifs_0: CPU(s) 45: SCAN operation did not start. Other thread could not join. If the user debugs using the trace point I included in the code they will see: sh-411499 [067] ..... 61260.698969: ifs_status: cpu: 45, start: 00, stop: 80, status: 100008000 sh-411499 [067] ..... 61260.699968: ifs_status: cpu: 45, start: 00, stop: 80, status: 100008000 sh-411499 [067] ..... 61260.700076: ifs_status: cpu: 45, start: 00, stop: 80, status: 100008000 sh-411499 [067] ..... 61260.700187: ifs_status: cpu: 45, start: 00, stop: 80, status: 100008000 sh-411499 [067] ..... 61260.700334: ifs_status: cpu: 45, start: 00, stop: 80, status: 100008000 sh-411499 [067] ..... 61260.700437: ifs_status: cpu: 45, start: 00, stop: 80, status: 100008000 Hmmm ... looks like I have an off-by one error in the retry check. /* Max retries on the same chunk */ #define MAX_IFS_RETRIES 5 But it tried SIX times before giving up. Will fix. > 2) That documentation for the admin/user got eaten by the gremlins in > the intertubes again. GregKH wasn't a fan of this itty bitty driver cluttering up Documentation/x86. He said: I don't know which is better, it's just that creating a whole new documentation file for a single tiny driver feels very odd as it will get out of date and is totally removed from the driver itself. I'd prefer that drivers be self-contained, including the documentation, as it is much more obvious what is happening with that. Spreading stuff around the tree only causes stuff to get out of sync easier. So the documentation patch was dropped after v3. Last version here: https://lore.kernel.org/r/20220419163859.2228874-3-tony.luck@intel.com That doc would need pathnames updated to match the move from a platform device to a virtual misc device. But otherwise seems still accurate. Does that cover what you want from documentation for this driver (wherever it gets located in the tree)? Are you looking for more? -Tony
On Fri, May 06 2022 at 11:49, Luck, Tony wrote: > On Fri, May 06, 2022 at 03:30:30PM +0200, Thomas Gleixner wrote: >> 1) How is that supposed to work on a system which has HT enabled in BIOS, >> but disabled on the kernel command line or via /sys/..../smt/control or >> when a HT sibling is offlined temporarily? >> >> I assume it cannot work, but I can't see anything which handles those >> cases. > > Correct. If HT is disabled in BIOS, then there is no other thread, so > core tests just use a single thread. > > If a logical CPU is "offline" due to Linux actions, then core test will > fail. In an earlier version we did attempt to detect this before trying > to run the test. But we didn't find a simple way to determine that a > core has one thread online, and another offline. Rather than a bunch of > code to detect an operator error it seemed better to let it run & > fail. Fair enough. > GregKH wasn't a fan of this itty bitty driver cluttering up > Documentation/x86. He said: > > I don't know which is better, it's just that creating a whole new > documentation file for a single tiny driver feels very odd as it will > get out of date and is totally removed from the driver itself. > > I'd prefer that drivers be self-contained, including the documentation, > as it is much more obvious what is happening with that. Spreading stuff > around the tree only causes stuff to get out of sync easier. Well, I agree to some extent, but the documentation which I want to see is documentation for admins. I'm not sure whether we want them to search the code. Those are consumers of Documentation/ AFAICT. > So the documentation patch was dropped after v3. Last version here: > > https://lore.kernel.org/r/20220419163859.2228874-3-tony.luck@intel.com > > That doc would need pathnames updated to match the move from a platform > device to a virtual misc device. But otherwise seems still accurate. > > Does that cover what you want from documentation for this driver > (wherever it gets located in the tree)? Are you looking for more? It's pretty detailed on the inner workings, but lacks a big fat warning for the admin vs. the impact, i.e. that it makes the core go out for lunch for a while, which has consequences on workloads and interrupts directed at that core. Plus some explanation vs. the HT (SMT=off, soft offline) case above. Similar to what we have e.g. for buslocks. Thanks, tglx
diff --git a/drivers/platform/x86/intel/ifs/Makefile b/drivers/platform/x86/intel/ifs/Makefile index 98b6fde15689..cedcb103f860 100644 --- a/drivers/platform/x86/intel/ifs/Makefile +++ b/drivers/platform/x86/intel/ifs/Makefile @@ -1,3 +1,3 @@ obj-$(CONFIG_INTEL_IFS) += intel_ifs.o -intel_ifs-objs := core.o load.o +intel_ifs-objs := core.o load.o runtest.o diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index 1a606c999b12..7435a5582df3 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -11,6 +11,11 @@ #define MSR_SCAN_HASHES_STATUS 0x000002c3 #define MSR_AUTHENTICATE_AND_COPY_CHUNK 0x000002c4 #define MSR_CHUNKS_AUTHENTICATION_STATUS 0x000002c5 +#define MSR_ACTIVATE_SCAN 0x000002c6 +#define MSR_SCAN_STATUS 0x000002c7 +#define SCAN_NOT_TESTED 0 +#define SCAN_TEST_PASS 1 +#define SCAN_TEST_FAIL 2 /* MSR_SCAN_HASHES_STATUS bit fields */ union ifs_scan_hashes_status { @@ -38,6 +43,40 @@ union ifs_chunks_auth_status { }; }; +/* MSR_ACTIVATE_SCAN bit fields */ +union ifs_scan { + u64 data; + struct { + u32 start :8; + u32 stop :8; + u32 rsvd :16; + u32 delay :31; + u32 sigmce :1; + }; +}; + +/* MSR_SCAN_STATUS bit fields */ +union ifs_status { + u64 data; + struct { + u32 chunk_num :8; + u32 chunk_stop_index :8; + u32 rsvd1 :16; + u32 error_code :8; + u32 rsvd2 :22; + u32 control_error :1; + u32 signature_error :1; + }; +}; + +/* + * Driver populated error-codes + * 0xFD: Test timed out before completing all the chunks. + * 0xFE: not all scan chunks were executed. Maximum forward progress retries exceeded. + */ +#define IFS_SW_TIMEOUT 0xFD +#define IFS_SW_PARTIAL_COMPLETION 0xFE + /** * struct ifs_data - attributes related to intel IFS driver * @integrity_cap_bit - MSR_INTEGRITY_CAPS bit enumerating this test @@ -45,6 +84,8 @@ union ifs_chunks_auth_status { * @loaded: If a valid test binary has been loaded into the memory * @loading_error: Error occurred on another CPU while loading image * @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 */ struct ifs_data { int integrity_cap_bit; @@ -52,6 +93,8 @@ struct ifs_data { bool loaded; bool loading_error; int valid_chunks; + int status; + u64 scan_details; }; struct ifs_work { @@ -73,5 +116,6 @@ static inline struct ifs_data *ifs_get_data(struct device *dev) } void ifs_load_firmware(struct device *dev); +int do_core_test(int cpu, struct device *dev); #endif diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c new file mode 100644 index 000000000000..fd3f5f3f31e5 --- /dev/null +++ b/drivers/platform/x86/intel/ifs/runtest.c @@ -0,0 +1,250 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright(c) 2022 Intel Corporation. */ + +#include <linux/cpu.h> +#include <linux/delay.h> +#include <linux/fs.h> +#include <linux/nmi.h> +#include <linux/slab.h> +#include <linux/stop_machine.h> + +#include "ifs.h" + +/* + * Note all code and data in this file is protected by + * ifs_sem. On HT systems all threads on a core will + * execute together, but only the first thread on the + * core will update results of the test. + */ +struct workqueue_struct *ifs_wq; +static bool scan_enabled = true; + +/* Max retries on the same chunk */ +#define MAX_IFS_RETRIES 5 + +/* + * Number of TSC cycles that a logical CPU will wait for the other + * logical CPU on the core in the WRMSR(ACTIVATE_SCAN). + */ +#define IFS_THREAD_WAIT 100000 + +enum ifs_status_err_code { + IFS_NO_ERROR = 0, + IFS_OTHER_THREAD_COULD_NOT_JOIN = 1, + IFS_INTERRUPTED_BEFORE_RENDEZVOUS = 2, + IFS_POWER_MGMT_INADEQUATE_FOR_SCAN = 3, + IFS_INVALID_CHUNK_RANGE = 4, + IFS_MISMATCH_ARGUMENTS_BETWEEN_THREADS = 5, + IFS_CORE_NOT_CAPABLE_CURRENTLY = 6, + IFS_UNASSIGNED_ERROR_CODE = 7, + IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT = 8, + IFS_INTERRUPTED_DURING_EXECUTION = 9, +}; + +static const char * const scan_test_status[] = { + [IFS_NO_ERROR] = "SCAN no error", + [IFS_OTHER_THREAD_COULD_NOT_JOIN] = "Other thread could not join.", + [IFS_INTERRUPTED_BEFORE_RENDEZVOUS] = "Interrupt occurred prior to SCAN coordination.", + [IFS_POWER_MGMT_INADEQUATE_FOR_SCAN] = + "Core Abort SCAN Response due to power management condition.", + [IFS_INVALID_CHUNK_RANGE] = "Non valid chunks in the range", + [IFS_MISMATCH_ARGUMENTS_BETWEEN_THREADS] = "Mismatch in arguments between threads T0/T1.", + [IFS_CORE_NOT_CAPABLE_CURRENTLY] = "Core not capable of performing SCAN currently", + [IFS_UNASSIGNED_ERROR_CODE] = "Unassigned error code 0x7", + [IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT] = + "Exceeded number of Logical Processors (LP) allowed to run Scan-At-Field concurrently", + [IFS_INTERRUPTED_DURING_EXECUTION] = "Interrupt occurred prior to SCAN start", +}; + +static void message_not_tested(struct device *dev, int cpu, union ifs_status status) +{ + if (status.error_code < ARRAY_SIZE(scan_test_status)) + dev_info(dev, "CPU(s) %*pbl: SCAN operation did not start. %s\n", + cpumask_pr_args(topology_sibling_cpumask(cpu)), + scan_test_status[status.error_code]); + else if (status.error_code == IFS_SW_TIMEOUT) + dev_info(dev, "CPU(s) %*pbl: software timeout during scan\n", + cpumask_pr_args(topology_sibling_cpumask(cpu))); + else if (status.error_code == IFS_SW_PARTIAL_COMPLETION) + dev_info(dev, "CPU(s) %*pbl: %s\n", + cpumask_pr_args(topology_sibling_cpumask(cpu)), + "Not all scan chunks were executed. Maximum forward progress retries exceeded"); + else + dev_info(dev, "CPU(s) %*pbl: SCAN unknown status %llx\n", + cpumask_pr_args(topology_sibling_cpumask(cpu)), status.data); +} + +static void message_fail(struct device *dev, int cpu, union ifs_status status) +{ + /* + * 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(topology_sibling_cpumask(cpu))); + } + + /* + * signature_error is set when the output from the scan chains does not + * match the expected signature. This might be a transient problem (e.g. + * due to a bit flip from an alpha particle or neutron). If the problem + * repeats on a subsequent test, then it indicates an actual problem in + * the core being tested. + */ + if (status.signature_error) { + dev_err(dev, "CPU(s) %*pbl: test signature incorrect.\n", + cpumask_pr_args(topology_sibling_cpumask(cpu))); + } +} + +static bool can_restart(union ifs_status status) +{ + enum ifs_status_err_code err_code = status.error_code; + + /* Signature for chunk is bad, or scan test failed */ + if (status.signature_error || status.control_error) + return false; + + switch (err_code) { + case IFS_NO_ERROR: + case IFS_OTHER_THREAD_COULD_NOT_JOIN: + case IFS_INTERRUPTED_BEFORE_RENDEZVOUS: + case IFS_POWER_MGMT_INADEQUATE_FOR_SCAN: + case IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT: + case IFS_INTERRUPTED_DURING_EXECUTION: + return true; + case IFS_INVALID_CHUNK_RANGE: + case IFS_MISMATCH_ARGUMENTS_BETWEEN_THREADS: + case IFS_CORE_NOT_CAPABLE_CURRENTLY: + case IFS_UNASSIGNED_ERROR_CODE: + break; + } + return false; +} + +/* + * Execute the scan. Called "simultaneously" on all threads of a core + * at high priority using the stop_cpus mechanism. + */ +static int doscan(void *data) +{ + int cpu = smp_processor_id(); + u64 *msrs = data; + int first; + + /* Only the first logical CPU on a core reports result */ + first = cpumask_first(topology_sibling_cpumask(cpu)); + + /* + * This WRMSR will wait for other HT threads to also write + * to this MSR (at most for activate.delay cycles). Then it + * starts scan of each requested chunk. The core scan happens + * during the "execution" of the WRMSR. This instruction can + * take up to 200 milliseconds before it retires. + */ + wrmsrl(MSR_ACTIVATE_SCAN, msrs[0]); + + if (cpu == first) { + /* Pass back the result of the scan */ + rdmsrl(MSR_SCAN_STATUS, msrs[1]); + } + + return 0; +} + +/* + * Use stop_core_cpuslocked() to synchronize writing to MSR_ACTIVATE_SCAN + * on all threads of the core to be tested. Loop if necessary to complete + * run of all chunks. Include some defensive tests to make sure forward + * progress is made, and that the whole test completes in a reasonable time. + */ +static void ifs_test_core(int cpu, struct device *dev) +{ + union ifs_scan activate; + union ifs_status status; + unsigned long timeout; + struct ifs_data *ifsd; + u64 msrvals[2]; + int retries; + + ifsd = ifs_get_data(dev); + + activate.rsvd = 0; + activate.delay = IFS_THREAD_WAIT; + activate.sigmce = 0; + activate.start = 0; + activate.stop = ifsd->valid_chunks - 1; + + timeout = jiffies + HZ / 2; + retries = MAX_IFS_RETRIES; + + while (activate.start <= activate.stop) { + if (time_after(jiffies, timeout)) { + status.error_code = IFS_SW_TIMEOUT; + break; + } + + msrvals[0] = activate.data; + stop_core_cpuslocked(cpu, doscan, msrvals); + + status.data = msrvals[1]; + + /* Some cases can be retried, give up for others */ + if (!can_restart(status)) + break; + + if (status.chunk_num == activate.start) { + /* Check for forward progress */ + if (retries-- == 0) { + if (status.error_code == IFS_NO_ERROR) + status.error_code = IFS_SW_PARTIAL_COMPLETION; + break; + } + } else { + retries = MAX_IFS_RETRIES; + activate.start = status.chunk_num; + } + } + + /* Update status for this core */ + ifsd->scan_details = status.data; + + if (status.control_error || status.signature_error) { + ifsd->status = SCAN_TEST_FAIL; + message_fail(dev, cpu, status); + } else if (status.error_code) { + ifsd->status = SCAN_NOT_TESTED; + message_not_tested(dev, cpu, status); + } else { + ifsd->status = SCAN_TEST_PASS; + } +} + +/* + * Initiate per core test. It wakes up work queue threads on the target cpu and + * its sibling cpu. Once all sibling threads wake up, the scan test gets executed and + * wait for all sibling threads to finish the scan test. + */ +int do_core_test(int cpu, struct device *dev) +{ + int ret = 0; + + if (!scan_enabled) + return -ENXIO; + + /* Prevent CPUs from being taken offline during the scan test */ + cpus_read_lock(); + + if (!cpu_online(cpu)) { + dev_info(dev, "cannot test on the offline cpu %d\n", cpu); + ret = -EINVAL; + goto out; + } + + ifs_test_core(cpu, dev); +out: + cpus_read_unlock(); + return ret; +}