Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <274d2502-4304-b8bd-ca42-5d53c8c778d9@intel.com>
Date:   Tue, 19 Sep 2023 09:22:12 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH 04/10] platform/x86/intel/ifs: Scan test for new
 generations
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>
CC:     Hans de Goede <hdegoede@redhat.com>, <markgross@kernel.org>,
        <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
        <dave.hansen@linux.intel.com>, <x86@kernel.org>, <hpa@zytor.com>,
        <rostedt@goodmis.org>, <ashok.raj@intel.com>,
        <tony.luck@intel.com>, LKML <linux-kernel@vger.kernel.org>,
        <platform-driver-x86@vger.kernel.org>, <patches@lists.linux.dev>,
        <ravi.v.shankar@intel.com>, <pengfei.xu@intel.com>
References: <20230913183348.1349409-1-jithu.joseph@intel.com>
 <20230913183348.1349409-5-jithu.joseph@intel.com>
 <74882e91-f41e-7672-3835-d74f1364bbdd@linux.intel.com>
 <5a66a02d-3504-ab5f-5373-789c8789557f@intel.com>
 <d94e516f-6331-2f20-468-bb8c6cf899c4@linux.intel.com>
From:   "Joseph, Jithu" <jithu.joseph@intel.com>
In-Reply-To: <d94e516f-6331-2f20-468-bb8c6cf899c4@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MlVzRnRjQ2ljUnM5SjdISE9qSGNFOEpUK2NaOE9HM3Y4ajVoaDBBZEsybnVj?=
 =?utf-8?B?SDJhejFjUEtTRWNWZHpDNml0WHhjZlZQdXMzTDZabzZXL3VnTnNGWlVVeVFn?=
 =?utf-8?B?Q21NdFBSaGtuQVErWHI2NXBxa3Z2WnBWSHVyKysyeEIrZWNmZllyVVNYNHA1?=
 =?utf-8?B?MFRTMUZmSnpJbDhnblBoc3pNcWVJTTBsa0x2bld1THBxbktWamVSRTQ4ajBM?=
 =?utf-8?B?Ni94YXM3Y3A1SmdVMGFDTnlJZ0szRjNLWFQxcDhtdHBDTXdWa2NIU0Nienoz?=
 =?utf-8?B?OHdyVVJuQUxVVE1ZRFBOc3dLU3NzWnhMZmYreTlzU3NWZEhxcW50d3NtazAy?=
 =?utf-8?B?L0phd1pQdW41aDNJaGU3L3BkTm9MakcxbldTZjZ2eHF4VS9hVlUrSHJEdkZP?=
 =?utf-8?B?dUVHb2NnM1dydVJVTENMdldXTUNHbjlPVmNLTFNxZTJqcHJWd0xTMkVDRjAy?=
 =?utf-8?B?b0ZXb3c0aUlCMW1aVGh0RkY3TGpHRi9OdTZJQWVEUHYyTDE5SDBDbW1oWWNC?=
 =?utf-8?B?OFpTRFBJb3dnZkVOSG9raVUwamlyRjR0cVlNUTU2ZFJPYW1yaWdzTExhQS9a?=
 =?utf-8?B?L0ZtMGtpMTRBa2RHTnBqUnRJQWo4MDBndG80U0JSUVNIK1FPL2dkVTRLNE9i?=
 =?utf-8?B?K0hSUlMrSHdGY0xBRExmUHhLZUg4SEVXSSt2MmpwcDAyVThCclBKaWF5cmtp?=
 =?utf-8?B?TUxXeGFUZGJkTWxJT1dGOTRTd3RqYk1kT3RoczNwektjaG1wMTJteC93N09T?=
 =?utf-8?B?RGRrNTc0bmRyaHpBVU9ZSCtBQXNWdUMwZGNYQ294OXBjeWgzc24yci8yaWZy?=
 =?utf-8?B?UHFKUTJzQzlBT2wxb2dQZERuTzFDSkpBT21LQVdsR3QvY1RTMmtOR0U5SlFu?=
 =?utf-8?B?QzRWVWpvNDQ5N1c4N1Rsa3VPSjJDMWJKbzhFeVBxRFBpZFlMdlpQV21yeVRI?=
 =?utf-8?B?ODVrc3gvajBiMGUwN09lN0ltc1RTb1hPaTdSVGx2Rlo2NllSc2pJdk55N0Uz?=
 =?utf-8?B?RnpqMHpjOTh6cGlpNzZvZ2lnOWlVTDJ6QldOcDFYMW0vRklQNTlMUmYwVXdX?=
 =?utf-8?B?N1pFUFVmSGN2T3c2WU9Edlh6MlcxMGUvUDRudWZYdDJYbndLQjI1TEVsNDFH?=
 =?utf-8?B?V2tFRE1GK1BRSDVOU1pTUU9Tbmk1aXBzL1lWbFMyemhtNlFCbjZpaUtpcFds?=
 =?utf-8?B?M1RPblgyRE51Z3BpM3VFSXVpZzF5QmdHalFONmJ5enFSRDcwb3dGN2VBZWJB?=
 =?utf-8?B?VHRDS0dQeTIyVkVFNm1kNysrSFJzYjZzVy9uUk8rWjBqK1g5VHV3Q3Vhd2E4?=
 =?utf-8?B?WnpBMnNtSUFuRy95WUQ3V21jZzBGMTFlS2p1WWNrTkNLYW1VbGliekhJQms0?=
 =?utf-8?B?T0RtNTJJMXdheHZ5Wm5CZXVoMGxvVllNLzBrNGZIVGVnTU9qV2huZ0E4WndI?=
 =?utf-8?B?bHovRXZ0RlB1U0RxYUcxVllXbHJMc2ZLSUFKRS9DaUh1bTFtaFBTd1BiYjY5?=
 =?utf-8?B?RHpMWnpQR2JodGVINUxCSFpUY2cyTzJSUG1mbjk5M2dLVUt5RnN5UURNczJS?=
 =?utf-8?B?YUhIelo5N0pEc09STWVzZWpDTWdDcThWWlZkM3NPNVNGSWpFSGZRMUlMWjl2?=
 =?utf-8?B?NVIwWVhzVGJ4YytNcTlSUDV4cDRRaXJjNDNYci9jOFhHMFpxd0cxTDlvMFlL?=
 =?utf-8?B?ZHlwTnZERnZRSVdmaml6dk5MKzBpM0R0Qmw1L0RDRTM1VllxejRLZFZPQzZT?=
 =?utf-8?B?b1VEUmtEREZmcUxHc0hqR0ZCQUNIKzdFU00weWFvQWdrei9OdHJUbkZmWkEx?=
 =?utf-8?B?dWJuRWwwN0NoTmJWTkJzVkcvamIrbjVaNDZxbStVbUxSOTNseGZuUDZqRW1m?=
 =?utf-8?B?U3oyUWxCemY4N1VsUGRUMWs5dzAySTg2eGdNSk1DazJLazdXRThJdVFBUUNo?=
 =?utf-8?B?bEhhbEdMY2E2Wm81OVVVNU1lV2MvR3Rpa2pldFhHd2E0ZG82YXNMQjZhc2Ry?=
 =?utf-8?B?Y1hjUnFrQ1puWlhQTDR6dEhwYmRraVBXOURBK2VlbkVIT0lhb3lqR2RjV3Np?=
 =?utf-8?B?cThaQVZmRzFoNzNLRStjamlnOGlpclJXWFdGSTJxM1M0TWdyWTk4SkhjYmd2?=
 =?utf-8?B?WTRqQUp3d0QrZFBUUWk4cGovL1lwTWFGazdoay9KWFpVYnhuZ25uYm9OUnBo?=
 =?utf-8?B?Qnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a5fc75a2-aaa9-4009-8ba7-08dbb92c9636
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3768.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Sep 2023 16:22:15.4389
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: KuzUZEx5dOUUqNuo2WkL7SxzBiy71KwLySCUy0s6HYxT9fJFiS2kw6xDwY8pdz7g8RMMwp9Tv4TogmiGC22rZA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6616
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.3 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Tue, 19 Sep 2023 09:22:37 -0700 (PDT)


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@intel.com>
>>>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>>>> Tested-by: Pengfei Xu <pengfei.xu@intel.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