Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <5a66a02d-3504-ab5f-5373-789c8789557f@intel.com>
Date:   Fri, 15 Sep 2023 13:10:28 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.0
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:     <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>, <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>
From:   "Joseph, Jithu" <jithu.joseph@intel.com>
In-Reply-To: <74882e91-f41e-7672-3835-d74f1364bbdd@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?THVWREdTd0R2Z0UyRUs2blg1cG83bXRYZTVaRmNuNE8xMFlIeStpVFIyc2NO?=
 =?utf-8?B?ckJxWE9RRXBsZ21yYU8va3FvcnlMUjR1NjJGcGx2U0dtSDVhNklJcytkdFdT?=
 =?utf-8?B?dU9aNHVXdWZHVVRqY1A1YzduK0JPZWJmbTFBZ3RWR20vNnlHRmJJck1MS0p1?=
 =?utf-8?B?YUtoY0dNcUtHbmc4MC91dUNoZlNEMkZIeWlnYWNnTTREdzdYditzKzlPTUdt?=
 =?utf-8?B?UmN5L1RIeTlka2ZTblFSYmlHSnVSaE80QXBzaWw5OFlpdVlVcjNlclVTaWg3?=
 =?utf-8?B?RWhBNkZaaFJQU1NYN2YwQjFSY3M3clI0YUFROFJ1d0N1cmM5N2luSzBKZjR3?=
 =?utf-8?B?TjNHL0M2MkRyTjlsbUFLbDhJN3IrQStXNHd4ZVdGWWluR3RUVW5VU1R0R2hZ?=
 =?utf-8?B?NmhWR0d5bXI2Z2lKTjNHazk0aXhEZ2xPSmFQbkI1Sk00L3BZbWUwYWhHRGo0?=
 =?utf-8?B?cHhSOXoxK3hnUnpIelJhS0Nwb3Y2Q1ByaUpXVTI1alpRRVNLS1pGdG81c2Vl?=
 =?utf-8?B?VDFmUlpLdytudzR1VndGREFPdUJMV3JVc3l3QVg3UGFjS3JMZG91d0U5b3pR?=
 =?utf-8?B?K0JYOHlXNFdkbU5xSXdNWU1iZitwSEc2ZUdLMjJ3UVlPVXFDSmk4OXkwaDBD?=
 =?utf-8?B?Y0Y2c3hFT3J1ZnpRTmx2QzlBcTAwR0tSY25JRmJXYWd5T3dWU2dCY2VENk5Q?=
 =?utf-8?B?eWdQbWZUYVQzZGJtM0tvZjBleENOQktRSWxBek1teUM3ZE5QbENTOTR0MVRG?=
 =?utf-8?B?WlJpRHB3a0ZCUWRxSm1YSThKeWpDakxsLzY5bUluN21HTG5Eb094MllMMkwy?=
 =?utf-8?B?MmNQYWt3MlVYTUxPN0RVUWMzMjVmcUh4OEhHRWJyZkpCSytjN1dtZ0ZVUklC?=
 =?utf-8?B?a09Lb3AwS3QxYXQxNGRsUUxDbkxvM1NpQStOaXU2QXg1SUJQcXdzOG9IUmoy?=
 =?utf-8?B?SmxNNFpQQ3FLTXlPa1hCM1pKMDlUOFNPM0IvSzR2L0FZK21VMDIrZTU0UWdN?=
 =?utf-8?B?QjZuZDJMVnI1cjhBV2hXREFzcCtDLzhwdmJJemJNNHpMbkxQZnpPdTQrY0Rw?=
 =?utf-8?B?aVEyODh4eXFNOGtNTUlSMnBHYXR0bi81TjU5azk3a09ES1NoQ3g4ZDJHa0Jy?=
 =?utf-8?B?UmV6Y2RKZWh2S0tzQjhIQXp3MWh3UUFFb2xLQTZOTXZucng3ZlNjeVJFeHU5?=
 =?utf-8?B?WTNtS1luMzdUMFROMGpLNWM0OVIrRkFYVDYwY1pEUnNWR0RyL0pKK2xCc1pi?=
 =?utf-8?B?cjN2MGE5VkUvV1U4SVd3Zy9IMFBRM3EzSjBxbmtTM2E4d2Y4Z2RXSStEL2NE?=
 =?utf-8?B?dzIyUjZqeWJGY2RwU09oL3BCTHRlSmk4MTNCTzVkQmlvUnhrbmJESXdwRDlR?=
 =?utf-8?B?OGZ3YUthWUNpV0FZTW1CVGhYWWdTTUJUbmd1Zi9PM3J5UHZzM3BJeWwyRDhv?=
 =?utf-8?B?VGw4T1o4VHFjbHQ0MXZndFErSHJkV0xvMFlFRzBXSnNOa2MxbGJiN2pMUGgz?=
 =?utf-8?B?Q1JKb2piQVh1M2VxV2ZhUmhoS3Vqdjh1SFBrTytLT1JVeUxkdWNNQ0thZ3l0?=
 =?utf-8?B?YmJtbG9MWVpZZWVDQzVraW1tMHVXM2Yvc1hSWm4xZnRsZDF1dkNsV0N6NXJY?=
 =?utf-8?B?YzZTVFFhSmtYUFc2N21NZGJWUUo0aGVqdlA3cUZBM0lEZ0JXSDRVVm5jbWJK?=
 =?utf-8?B?QVVweHRxUUVOM2tkMWM3S3lEQnU2SDhhYlljVlJOb0NmT1VtWjFhK1R4M1lV?=
 =?utf-8?B?VElPbGpFSUJjelVNeTNhODdkYXJRZmdkLy96NHRMQStKcUZPQTl1ODhUVTNN?=
 =?utf-8?B?Mmo2WmdZZk92ZmQ0dk9hdlVBZ1BlOEkyc2pjc2ZlSWdVdWprTmpnd0xXeGwr?=
 =?utf-8?B?UUgxcm4rYmlZb1pLZTZRUkxMSUk4STE4QUNkdkh1VHhuUVNyd2pyOEFZakds?=
 =?utf-8?B?SWNOL1BHNCt0ZFJnMEFXaE91U1kydmpObFdZNEZWR0R1eFFXU2RNU2p4NUYz?=
 =?utf-8?B?NVY0RzN0MWZzdjVnR1NkdFFqeEVnNGxMTzY2TE1seHdodTFjdmhYM1A2ejZ2?=
 =?utf-8?B?VW4rbGMwUjRKRElLeFViVzJudEpyWnpxWXJvQ1JYa0haamxES0RTcWVycWpv?=
 =?utf-8?B?ZndZSFQvOGtFZE5OdzZsYkpCQjZ6MUo3amJ4dFRjK1l3cVhKWVhFRnREekVj?=
 =?utf-8?B?Y2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4a451d27-a52d-4992-78c6-08dbb627d0b4
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3768.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Sep 2023 20:10:32.5911
 (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: RjbNZk0M7ByevNp14MsSVqlRB3RP7N0w40pbz7nKcG3/eF7kFKamYgqXsVbZskdYAcktV4+d08nJ2C7OQNhXJA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7090
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 morse.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 (morse.vger.email [0.0.0.0]); Fri, 15 Sep 2023 13:11:38 -0700 (PDT)


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

>>  };
>>  
>>  /* 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