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 10:44:14 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     "Joseph, Jithu" <jithu.joseph@...el.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 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):

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.

-- 
 i.