linux-kernel - Re: [PATCH 4/5] platform/x86/intel/ifs: Implement Array BIST test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 1 Feb 2023 11:56:51 -0800
From:   "Joseph, Jithu" <jithu.joseph@...el.com>
To:     Dave Hansen <dave.hansen@...el.com>, <hdegoede@...hat.com>,
        <markgross@...nel.org>
CC:     <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
        <dave.hansen@...ux.intel.com>, <x86@...nel.org>, <hpa@...or.com>,
        <gregkh@...uxfoundation.org>, <rostedt@...dmis.org>,
        <ashok.raj@...el.com>, <tony.luck@...el.com>,
        <linux-kernel@...r.kernel.org>,
        <platform-driver-x86@...r.kernel.org>, <patches@...ts.linux.dev>,
        <ravi.v.shankar@...el.com>, <thiago.macieira@...el.com>,
        <athenas.jimenez.gonzalez@...el.com>, <sohil.mehta@...el.com>
Subject: Re: [PATCH 4/5] platform/x86/intel/ifs: Implement Array BIST test



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'.