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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 19 Sep 2023 09:22:12 -0700
From:   "Joseph, Jithu" <jithu.joseph@...el.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC:     Hans de Goede <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>, LKML <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/19/2023 12:44 AM, Ilpo Järvinen wrote:
> On Fri, 15 Sep 2023, Joseph, Jithu wrote:
>> 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
> 
> I would certainly prefer the generation dependent fields to marked as 
> such. However, it does not say you couldn't have the other fields remain 
> w/o gen.
> 
> How about this definition (it comes with the added benefit that you 
> cannot accidently use start/stop without specifying gen which guards 
> against one type of bugs):
> 

Yes this looks better. I will adopt this in v2.

> union ifs_scan {
>         u64     data;
>         struct {
> 		union {
> 			struct {
> 	        	        u8      start;
>         	        	u8      stop;
> 	        	        u16     rsvd;
> 			} gen0;
> 		        struct {
>                 		u16     start;
> 		                u16     stop;
> 			} gen2;
> 		};
>                 u32     delay   :31;
>                 u32     sigmce  :1;
>         };
> };
> 
> Note that I used start and stop in gen0 without the bitfield that
> seems unnecessary.
> 

Thanks
Jithu