| Message ID | 20230131234302.3997223-5-jithu.joseph@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Array BIST test support to IFS | expand |
On Tue, Jan 31, 2023 at 03:43:01PM -0800, Jithu Joseph wrote: > Array BIST test (for a particlular core) is triggered by writing > to MSR_ARRAY_BIST from one sibling of the core. > > This will initiate a test for all supported arrays on that > CPU. Array BIST test may be aborted before completing all the > arrays in the event of an interrupt or other reasons. > In this case, kernel will restart the test from that point > onwards. Array test will also be aborted when the test fails, > in which case the test is stopped immediately without further > retry. > > Signed-off-by: Jithu Joseph <jithu.joseph@intel.com> > Reviewed-by: Tony Luck <tony.luck@intel.com> > --- > drivers/platform/x86/intel/ifs/ifs.h | 12 ++++ > drivers/platform/x86/intel/ifs/runtest.c | 92 ++++++++++++++++++++++++ > 2 files changed, 104 insertions(+) > > diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h > index 07423bc4e368..b1a997e39216 100644 > --- a/drivers/platform/x86/intel/ifs/ifs.h > +++ b/drivers/platform/x86/intel/ifs/ifs.h > @@ -127,6 +127,7 @@ > #include <linux/device.h> > #include <linux/miscdevice.h> > > +#define MSR_ARRAY_BIST 0x00000105 > #define MSR_COPY_SCAN_HASHES 0x000002c2 > #define MSR_SCAN_HASHES_STATUS 0x000002c3 > #define MSR_AUTHENTICATE_AND_COPY_CHUNK 0x000002c4 > @@ -194,6 +195,17 @@ union ifs_status { > }; > }; > > +/* MSR_ARRAY_BIST bit fields */ > +union ifs_array { > + u64 data; > + struct { > + u32 array_bitmask :32; > + u32 array_bank :16; > + u32 rsvd :15; > + u32 ctrl_result :1; This isn't going to work well over time, just mask the bits you want off properly, don't rely on the compiler to lay them out like this. Note, we have bitmask and bitfield operations, please use them. thanks, greg k-h
> > +/* MSR_ARRAY_BIST bit fields */ > > +union ifs_array { > > + u64 data; > > + struct { > > + u32 array_bitmask :32; > > + u32 array_bank :16; > > + u32 rsvd :15; > > + u32 ctrl_result :1; > > This isn't going to work well over time, just mask the bits you want off > properly, don't rely on the compiler to lay them out like this. What is this "time" issue? This driver is X86_64 specific (and it seems incredibly unlikely that some other architecture will copy this h/w interface so closely that they want to re-use this driver. There's an x86_64 ABI that says how bitfields in C are allocated. So should not break moving to other C compilers. Is there going to be a "re-write all drivers in Rust" edict coming soon? > Note, we have bitmask and bitfield operations, please use them. We do, but code written using them is not as easy to read (unless you wrap in even more macros, which has its own maintainability issues). There are already thousands of bitfields in Linux kernel source: $ git grep ':[1-9][0-9]*;' -- include/ | wc -l 2251 Has there been a change in attitude at the kernel maintainers summit? -Tony
On Wed, Feb 01, 2023 at 05:22:18PM +0000, Luck, Tony wrote: > > > +/* MSR_ARRAY_BIST bit fields */ > > > +union ifs_array { > > > + u64 data; > > > + struct { > > > + u32 array_bitmask :32; > > > + u32 array_bank :16; > > > + u32 rsvd :15; > > > + u32 ctrl_result :1; > > > > This isn't going to work well over time, just mask the bits you want off > > properly, don't rely on the compiler to lay them out like this. > > What is this "time" issue? This driver is X86_64 specific (and it seems > incredibly unlikely that some other architecture will copy this h/w > interface so closely that they want to re-use this driver. There's an x86_64 > ABI that says how bitfields in C are allocated. So should not break moving > to other C compilers. Ok, but generally this is considered a "bad idea" that you should not do. It's been this way for many many years, just because this file only runs on one system now, doesn't mean you shouldn't use the proper apis. Also, odds are, using the proper apis will get you faster/smaller code than using a bitfield like this as compilers were notorious for doing odd things here in the past. > Is there going to be a "re-write all drivers in Rust" edict coming soon? Don't be silly, it's been this way for drivers for decades. > > > Note, we have bitmask and bitfield operations, please use them. > > We do, but code written using them is not as easy to read (unless > you wrap in even more macros, which has its own maintainability > issues). It shouldn't be that hard, lots of people use them today. Try and see! thanks, greg k-h
On Wed, Feb 01, 2023 at 07:19:15PM +0100, Greg KH wrote: > It shouldn't be that hard, lots of people use them today. > > Try and see! Extract from the first of our in-field-scan tests: while (activate.start <= activate.stop) { ... trigger scan ... if (status.chunk_num == activate.start) { ... check for too many retries on same chunk ... } else { activate.start = status.chunk_num; } } using <linux/bitfield.h> becomes: while (FIELD_GET(GENMASK(7, 0), activate) <= FIELD_GET(GENMASK(15, 8), activate) { if (FIELD_GET(GENMASK(7, 0), status) == FIELD_GET(GENMASK(7, 0), activate) { ... } else { activate &= ~GENMASK(7, 0); activate |= FIELD_PREP(GENMASK(7, 0), FIELD_GET(GENMASK(7, 0), status)); } } While I can make that more legible with some helper #defines for the fields, it becomes more difficult to write, and no easier to read (since I then have to chase down what the macros are doing). If this were in some performance critical path, I might worry about whether the generated code was good enough. But this code path isn't remotely critical to anyone. The test takes up to 200 usec, so saving a handful of cycles in the surrounding code will be in the noise. -Tony
On 1/31/23 15:43, Jithu Joseph wrote: > +union ifs_array { > + u64 data; > + struct { > + u32 array_bitmask :32; > + u32 array_bank :16; > + u32 rsvd :15; > + u32 ctrl_result :1; > + }; > +}; Why bother with a bitfield? Just do: union ifs_array { u64 data; struct { u32 array_bitmask; u16 array_bank; u16 flags; }; }; Then you only need to mask 'ctrl_result' out of flags. You don't need any crazy macros.
> Why bother with a bitfield? Just do: How much "bother" is a bitfield? > union ifs_array { > u64 data; > struct { > u32 array_bitmask; > u16 array_bank; > u16 flags; > }; >}; > > Then you only need to mask 'ctrl_result' out of flags. You don't need > any crazy macros. "only need" to special case this one field ... but that's extra code for humans to read (and humans aren't good at that) rather than the computer (compiler) which is really good at doing this. Using bitfields is also following the existing style of this driver. -Tony
On 1/31/23 15:43, Jithu Joseph wrote: > +static void ifs_array_test_core(int cpu, struct device *dev) > +{ > + union ifs_array activate, status; > + bool timed_out = false; > + struct ifs_data *ifsd; > + unsigned long timeout; > + u64 msrvals[2]; > + > + ifsd = ifs_get_data(dev); > + > + activate.data = 0; > + activate.array_bitmask = ~0U; > + activate.ctrl_result = 0; I think this whole 'ifs_array' as a union thing is bogus. It's actually obfuscating and *COMPLICATING* the code more than anything. Look what you have: union ifs_array activate; // declare it on the stack, unzeroed activate.data = 0; // zero the structure; activate.array_bitmask = ~0U; // set one field activate.ctrl_result = 0; // set the field to zero again??? Can we make it less obfuscated? How about: struct ifs_array activate = {}; // zero it ... activate.array_bitmask = ~0U; // set the only nonzero field Voila! Less code, less obfuscation, less duplicated effort. Or, worst case: struct ifs_array activate; ... memset(&activate, 0, sizeof(activate)); activate.array_bitmask = ~0U; That's sane and everyone knows what it does and doesn't have to know what unions are involved or how they are used. It's correct code no matter *WHAT* craziness lies within 'activate'.
On 2/1/23 11:43, Luck, Tony wrote: >> Why bother with a bitfield? Just do: > How much "bother" is a bitfield? > >> union ifs_array { >> u64 data; >> struct { >> u32 array_bitmask; >> u16 array_bank; >> u16 flags; >> }; >> }; >> >> Then you only need to mask 'ctrl_result' out of flags. You don't need >> any crazy macros. > "only need" to special case this one field ... but that's extra > code for humans to read (and humans aren't good at that) > rather than the computer (compiler) which is really good at > doing this. I don't follow. If you have: struct foo { u16 rsvd :15; u16 ctrl_result :1; }; versus: struct bar { u16 flags; }; and you do: if (foo->ctrl_result) something(); versus: if (bar->flags & CTRL_RESULT_MASK) something(); I think both of those are quite readable. I'd argue that humans will be less _surprised_ by 'bar'. I also like to write portable code even if it's going to be x86 only. It helps people who are used to reading portable generic code read and find bugs in x86 only code.
On 2/1/2023 11:45 AM, Dave Hansen wrote: > On 1/31/23 15:43, Jithu Joseph wrote: >> +static void ifs_array_test_core(int cpu, struct device *dev) >> +{ >> + union ifs_array activate, status; >> + bool timed_out = false; >> + struct ifs_data *ifsd; >> + unsigned long timeout; >> + u64 msrvals[2]; >> + >> + ifsd = ifs_get_data(dev); >> + >> + activate.data = 0; >> + activate.array_bitmask = ~0U; >> + activate.ctrl_result = 0; > > I think this whole 'ifs_array' as a union thing is bogus. It's actually > obfuscating and *COMPLICATING* the code more than anything. Look what > you have: > > union ifs_array activate; // declare it on the stack, unzeroed > > activate.data = 0; // zero the structure; > activate.array_bitmask = ~0U; // set one field > activate.ctrl_result = 0; // set the field to zero again??? > > Can we make it less obfuscated? How about: > > struct ifs_array activate = {}; // zero it > ... > activate.array_bitmask = ~0U; // set the only nonzero field > > Voila! Less code, less obfuscation, less duplicated effort. Or, worst Agreed, will modify it as you suggest above to remove the duplicate zero assignments > case: > > struct ifs_array activate; > ... > memset(&activate, 0, sizeof(activate)); > activate.array_bitmask = ~0U; > > That's sane and everyone knows what it does and doesn't have to know > what unions are involved or how they are used. It's correct code no > matter *WHAT* craziness lies within 'activate'.
On 2/1/23 11:56, Joseph, Jithu wrote: >> Voila! Less code, less obfuscation, less duplicated effort. Or, worst > Agreed, will modify it as you suggest above to remove the duplicate zero assignments ... and the union ... and the _unnecessary_ bitfields. You can make an argument that ->ctrl_result should be a bitfield. The other structure members, not so much. Make them standalone unsigned integers. But, when it's down to a single bit in an otherwise completely unpopulated byte-sized field, your arguments for using a bitfield kinda dry up. But, heck, if that's the hill you want to die on, who am I to stop you?
> But, when it's down to a single bit in an otherwise completely > unpopulated byte-sized field, your arguments for using a bitfield kinda > dry up. But, heck, if that's the hill you want to die on, who am I to > stop you? It helps for consistency of style with all the other definitions here where some other registers don't have such trivial mappings of fields to C base types. I know that using bitfields in arch independent code is fraught with problems. But these definitions are for the bits in Intel MSRs (model specific registers). This seems to be an open & shut case that this code is never going to be used on some other architecture. -Tony
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index 07423bc4e368..b1a997e39216 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -127,6 +127,7 @@ #include <linux/device.h> #include <linux/miscdevice.h> +#define MSR_ARRAY_BIST 0x00000105 #define MSR_COPY_SCAN_HASHES 0x000002c2 #define MSR_SCAN_HASHES_STATUS 0x000002c3 #define MSR_AUTHENTICATE_AND_COPY_CHUNK 0x000002c4 @@ -194,6 +195,17 @@ union ifs_status { }; }; +/* MSR_ARRAY_BIST bit fields */ +union ifs_array { + u64 data; + struct { + u32 array_bitmask :32; + u32 array_bank :16; + u32 rsvd :15; + u32 ctrl_result :1; + }; +}; + /* * Driver populated error-codes * 0xFD: Test timed out before completing all the chunks. diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c index 65e08af70994..ec0ceb6b5890 100644 --- a/drivers/platform/x86/intel/ifs/runtest.c +++ b/drivers/platform/x86/intel/ifs/runtest.c @@ -229,6 +229,96 @@ static void ifs_test_core(int cpu, struct device *dev) } } +#define SPINUNIT 100 /* 100 nsec */ +static atomic_t array_cpus_out; + +/* + * Simplified cpu sibling rendezvous loop based on microcode loader __wait_for_cpus() + */ +static void wait_for_sibling_cpu(atomic_t *t, long long timeout) +{ + int cpu = smp_processor_id(); + const struct cpumask *smt_mask = cpu_smt_mask(cpu); + int all_cpus = cpumask_weight(smt_mask); + + atomic_inc(t); + while (atomic_read(t) < all_cpus) { + if (timeout < SPINUNIT) + return; + ndelay(SPINUNIT); + timeout -= SPINUNIT; + touch_nmi_watchdog(); + } +} + +static int do_array_test(void *data) +{ + int cpu = smp_processor_id(); + u64 *msrs = data; + int first; + + /* + * Only one logical CPU on a core needs to trigger the Array test via MSR write. + */ + first = cpumask_first(cpu_smt_mask(cpu)); + + if (cpu == first) { + wrmsrl(MSR_ARRAY_BIST, msrs[0]); + /* Pass back the result of the test */ + rdmsrl(MSR_ARRAY_BIST, msrs[1]); + } + + /* Tests complete faster if the sibling is spinning here */ + wait_for_sibling_cpu(&array_cpus_out, NSEC_PER_SEC); + + return 0; +} + +static void ifs_array_test_core(int cpu, struct device *dev) +{ + union ifs_array activate, status; + bool timed_out = false; + struct ifs_data *ifsd; + unsigned long timeout; + u64 msrvals[2]; + + ifsd = ifs_get_data(dev); + + activate.data = 0; + activate.array_bitmask = ~0U; + activate.ctrl_result = 0; + timeout = jiffies + HZ / 2; + + do { + if (time_after(jiffies, timeout)) { + timed_out = true; + break; + } + + msrvals[0] = activate.data; + + atomic_set(&array_cpus_out, 0); + stop_core_cpuslocked(cpu, do_array_test, msrvals); + status.data = msrvals[1]; + + if (status.ctrl_result) + break; + + activate.array_bitmask = status.array_bitmask; + activate.array_bank = status.array_bank; + + } while (status.array_bitmask); + + ifsd->scan_details = status.data; + + if (status.ctrl_result) + ifsd->status = SCAN_TEST_FAIL; + else if (timed_out || status.array_bitmask) + ifsd->status = SCAN_NOT_TESTED; + 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 @@ -253,6 +343,8 @@ int do_core_test(int cpu, struct device *dev) ifs_test_core(cpu, dev); break; case IFS_ARRAY: + ifs_array_test_core(cpu, dev); + break; default: return -EINVAL; }