linux-kernel - Re: [PATCH 04/10] platform/x86/intel/ifs: Scan test for new generations

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 15 Sep 2023 13:10:28 -0700
From:   "Joseph, Jithu" <jithu.joseph@...el.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC:     <hdegoede@...hat.com>, <markgross@...nel.org>,
        <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
        <dave.hansen@...ux.intel.com>, <x86@...nel.org>, <hpa@...or.com>,
        <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>, <pengfei.xu@...el.com>
Subject: Re: [PATCH 04/10] platform/x86/intel/ifs: Scan test for new
 generations



On 9/15/2023 9:51 AM, Ilpo Järvinen wrote:
> On Wed, 13 Sep 2023, Jithu Joseph wrote:
> 
>> Make changes to scan test flow such that MSRs are populated
>> appropriately based on the generation supported by hardware.
>>
>> Width of chunk related bitfields is ACTIVATE_SCAN and SCAN_STATUS MSRs
>> are different in newer IFS generation compared to gen0.
>>
>> Signed-off-by: Jithu Joseph <jithu.joseph@...el.com>
>> Reviewed-by: Tony Luck <tony.luck@...el.com>
>> Tested-by: Pengfei Xu <pengfei.xu@...el.com>
>> ---
>>  drivers/platform/x86/intel/ifs/ifs.h     | 14 ++++++++++++++
>>  drivers/platform/x86/intel/ifs/runtest.c | 23 ++++++++++++++++++-----
>>  2 files changed, 32 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
>> index 886dc74de57d..3265a6d8a6f3 100644
>> --- a/drivers/platform/x86/intel/ifs/ifs.h
>> +++ b/drivers/platform/x86/intel/ifs/ifs.h
>> @@ -205,6 +205,12 @@ union ifs_scan {
>>  		u32	delay	:31;
>>  		u32	sigmce	:1;
>>  	};
>> +	struct {
>> +		u16	start;
>> +		u16	stop;
>> +		u32	delay	:31;
>> +		u32	sigmce	:1;
>> +	} gen2;
> 
> I don't like the way old struct is left without genx naming. It makes the 
> code below more confusing as is.
> 

Given that less than half the fields (2/4 in ifs_scan and 2/5 in ifs_status ) are changing across
generations(and rest are common) , I felt the code would be more readable if the common fields are
accessed without generation as is done now. 

That said I don’t mind changing if you feel strongly about this

>>  };
>>  
>>  /* MSR_SCAN_STATUS bit fields */
>> @@ -219,6 +225,14 @@ union ifs_status {
>>  		u32	control_error		:1;
>>  		u32	signature_error		:1;
>>  	};
>> +	struct {
>> +		u16	chunk_num;
>> +		u16	chunk_stop_index;
>> +		u8	error_code;
>> +		u32	rsvd1			:22;
>> +		u32	control_error		:1;
>> +		u32	signature_error		:1;
> 
> Again, I don't think the alignment will be correct in this case.
> 

I hope this is clarified in the reply in patch03/10

...


>> @@ -211,7 +222,9 @@ static void ifs_test_core(int cpu, struct device *dev)
>>  			}
>>  		} else {
>>  			retries = MAX_IFS_RETRIES;
>> -			activate.start = status.chunk_num;
>> +			ifsd->generation ? (activate.gen2.start = status_chunk) :
>> +			 (activate.start = status_chunk);
> 
> Misaligned.

Will align it to start

Jithu