Message-ID: <dd501d6a-e7e9-be82-24fb-33b18fb6192b@intel.com>
Date:   Wed, 15 Feb 2023 12:22:43 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.2
Subject: Re: [PATCH v2 4/7] platform/x86/intel/ifs: Implement Array BIST test
Content-Language: en-US
To:     Dave Hansen <dave.hansen@intel.com>, <hdegoede@redhat.com>,
        <markgross@kernel.org>
CC:     <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
        <dave.hansen@linux.intel.com>, <x86@kernel.org>, <hpa@zytor.com>,
        <gregkh@linuxfoundation.org>, <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>, <thiago.macieira@intel.com>,
        <athenas.jimenez.gonzalez@intel.com>, <sohil.mehta@intel.com>
References: <20230131234302.3997223-1-jithu.joseph@intel.com>
 <20230214234426.344960-1-jithu.joseph@intel.com>
 <20230214234426.344960-5-jithu.joseph@intel.com>
 <a24c65f8-978d-8968-7874-6b83e14b01ba@intel.com>
 <d9d18954-8434-4c85-88b8-8e1d99cd6a4b@intel.com>
From:   "Joseph, Jithu" <jithu.joseph@intel.com>
In-Reply-To: <d9d18954-8434-4c85-88b8-8e1d99cd6a4b@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SElERGhiS2lyWkQrK1FYaE1yRUkvMzlPbEo0UmhzNTZFNXlVTHlCeU1INTlm?=
 =?utf-8?B?VGZNT2tJWmJoZTZDSnVpcWpWalFKTHlPcW84K0lvdDNmMDJWR1hPWno0VHNT?=
 =?utf-8?B?eWdnakMxRENSR29lWjhZZ1B1dVYveUVBYUNEa1hRT3Q1Y3J0RHRJdnB3VGtm?=
 =?utf-8?B?NzJmL0p2R002S0hXQ0VqVmt1dzBVRVlTazBDTFArZGN0WksxKzlqMytScnRR?=
 =?utf-8?B?S0pYUkQ3QmZjQ3NpUHdUVkNTcUtZUmdMYytlNGs2ZnRKaWExU3JDMy9aWkxN?=
 =?utf-8?B?Nm9OQTROWFZtQUpucmExbTU2NEgzeHE4QUVuWnJML2hPaVo1R0haMWd1SEJK?=
 =?utf-8?B?K0g1NisvQndQSmV2RUt0Q3dPLzZ1dDZ3emFuMGFRSXcrR1lqbDJ6UDNZcHlk?=
 =?utf-8?B?U09aMElHdC83ZHpqek1xWG5WRnZua1VsVDc1L1ZGK3ZRMnN5c0hhOFd2bWlw?=
 =?utf-8?B?Y0h0c0s0MDlmbDJvc2FzcDRENDlxdGRIM1l4N1dOZk1mQnZ5YUsxRmxSYXVi?=
 =?utf-8?B?M3FtRkNKS3lKYXBmVW10VFA5bkpRRFl1S3RjZDdzWThsUjEya0RlWVRkb0Js?=
 =?utf-8?B?T2FUREhwV0pMcEY5R2tYeE4zek9TbHh2QXlDVkhRT1JBVUFLWmhieU9CeHRP?=
 =?utf-8?B?VGpBRmhuNi9kZnhvS095c1grYlRmbFZ3cHZJWU9ZVjNpS3V0L1VILzR0djZ2?=
 =?utf-8?B?Y3hPQWJNaEYvaktSYTdIemdRb1pQNlJsMXFaN0QyWnk1MG5GK2ZibUNYWWtw?=
 =?utf-8?B?SkNnODdtTGxJRm1XY0t2MkVuZFJ3cjIvd1hkbHJieWdBUGd1SWY0cThNTWdj?=
 =?utf-8?B?N2Nma2pWWHRsVXRTZlU0R2RnN0lSTmY2OG92cVhRREg1Q2d6bkNtU013NllG?=
 =?utf-8?B?WTJsRVlRWnpJMmRTOTBRQktCWEhaSE90aWo4VXRiOVFFMXhTVUFMRy9rc2NG?=
 =?utf-8?B?Qk9pNldsc21Ba0xPS0hVRXYrSldxMnpHcTlrelJUNndXSFNlb0JEVkFNcG5X?=
 =?utf-8?B?OVlEMlFDNi9aNmlyN0wwZTBVeGtkQ3pRN2Z6VXVRWkxCKzA4VS9MOHJacUIz?=
 =?utf-8?B?NnY3TzJ1MEJwYjlXOXFrTWhGM0lLa1hOdnF4cWlOZ2ZoTFpJNThHYzEwYXJa?=
 =?utf-8?B?WmRNd3ZjN3ZWbE94c2R3QXdBL0ZFMUxPNHZ0S0RJYkNrTGRqQzMxV3QrNGp2?=
 =?utf-8?B?cTBVTnl4NjN0Zk45ZStxSEI4NkFqRVU1V0RydFNsUnFRTExpMFdyYWtSNXhF?=
 =?utf-8?B?SEloWitkaVg1Z012MVpmUDRhQTNadDY3c0NOaDNkaXVGWlgwUzJjSjZxODJq?=
 =?utf-8?B?VlRGL0hrbjZlZGlBdFFud040T0ZpVFN2ckVsUW9BZlRmQmJtdFVTM01HM2Ni?=
 =?utf-8?B?ak4yeEVySHhrV24rajBlRVFCbE1lS3Q5YlJZaXBNaWNDMnNlOFgyRU9Rd0dZ?=
 =?utf-8?B?OWorLzB1Y01pTW5xZXRGZkNUeDFxOWZ0SE1XK0U5Sm9rSkFmMXRTV2lpYWJh?=
 =?utf-8?B?aWRXY2pKcjEzTUFvS1VHcUJ1MkwvL1VNaG93RzFlTGw0KzB2V1IxamRaM1lJ?=
 =?utf-8?B?UzNEbGo2K2htWE00SDJXV29EMDV1Mkx4Q29xNlFkSmUwRHhud3JuWVlIaUk0?=
 =?utf-8?B?bWF6QjhjNFlGZTFDb29nSUcyWmxsS3J5Wjl6RDNpbEg1RGpJYjNsWEhQdWF5?=
 =?utf-8?B?TlFnUWdMdnNBQkJEckNZc0V2d1VJUXIvY1N2NXdHWmtQOVFXZUNDTlpYWkJn?=
 =?utf-8?B?anlmOWFxbUcxRHpKeTVmYVA3eGpURk1EaWVqL3JSYU9Hajd6OTQwL2hwZ2hy?=
 =?utf-8?B?WEhaaURZTGdhLzFjNnlEaEFlNm4xZ0R3WUV6OU5wN2pVZk4zSmx4aVlTSitZ?=
 =?utf-8?B?K1QrclJVMFUrQng0bzN5aEs5MkJVRGxYS2JzQ2dWY2NNdnZzaWdjcURSdzhH?=
 =?utf-8?B?UHVSd0xEcytXdk9zMTJRL1FMUDJYNkF2akN3c2pRdWhNR2RRMmRJSExmS0k1?=
 =?utf-8?B?SkpVb3M4Uk5vWHV0bGVnOHpmOXNtd28vZWI1SUw1Y1lleFFsTXU3NldBdHJ1?=
 =?utf-8?B?TmU4NEtqQi80RW40UDBFWUg1ZDhZdit5K0kycVY3dDgrUjljWGxLT1l4cmxy?=
 =?utf-8?B?amYxaGR5eTJCQUNrbkNiZk5xa2JzcG40bHlRV3A1T2JzNGt1Rm85YVo1T1Fa?=
 =?utf-8?B?TGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b020ad31-7f40-4011-5ca0-08db0f926670
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3768.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Feb 2023 20:22:46.4151
 (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: 7GDU2I9F14/frZ6WS0yt1xUFAz5nbEjvHwKvIS8Y6iYjaRERyx1wXiq9/WQ7NVVqbfJCykJwYZtjqpdb2Mec+w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5205
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 2/15/2023 9:11 AM, Dave Hansen wrote:
> On 2/15/23 08:58, Dave Hansen wrote:
>> On 2/14/23 15:44, Jithu Joseph wrote:
>> I'd probably do something like the attached patch.  It gets rid of
>> 'data' and uses sane types for the bitfield.  It does away with separate
>> variables and munging into/out of the msr[] array and just passes a
>> single command struct to the work function.  It doesn't have any
>> uninitialized structure/bitfield fields.
> 
> Real patch attached now.

I did try out Dave's suggested patch. With few additional type castings, I could get it to work.
We can go with this. I will incorporate this while posting v3. 

(Will await a few day for additional comments before posting v3)

Jithu

For context, the functions under discussion incorporating Dave's changes would look as below:
static int do_array_test(void *data)
{
	struct ifs_array *command = data;
	int cpu = smp_processor_id();

	int first;

	/*
	 * Only one logical CPU on a core needs to trigger the Array test via MSR write.
	 */
	first = cpumask_first(cpu_smt_mask(cpu));

	if (cpu == first) {
		wrmsrl(MSR_ARRAY_BIST, *((u64 *)command));
		/* Pass back the result of the test */
		rdmsrl(MSR_ARRAY_BIST, *((u64 *)command));
	}

	/* Tests complete faster if the sibling is spinning here */
	wait_for_sibling_cpu(&array_cpus_out, NSEC_PER_SEC);

	return 0;
}

static void ifs_array_test_core(int cpu, struct device *dev)
{
	struct ifs_array command = {};
	bool timed_out = false;
	struct ifs_data *ifsd;
	unsigned long timeout;

	ifsd = ifs_get_data(dev);

	command.array_bitmask = ~0U;
	timeout = jiffies + HZ / 2;

	do {
		struct ifs_array before = command;

		if (time_after(jiffies, timeout)) {
			timed_out = true;
			break;
		}
		atomic_set(&array_cpus_out, 0);
		stop_core_cpuslocked(cpu, do_array_test, &command);

		trace_ifs_array(cpu, *((u64 *)&before), *((u64 *)&command));
		if (command.ctrl_result)
			break;
	} while (command.array_bitmask);

	ifsd->scan_details = *((u64 *)&command);

	if (command.ctrl_result)
		ifsd->status = SCAN_TEST_FAIL;
	else if (timed_out || command.array_bitmask)
		ifsd->status = SCAN_NOT_TESTED;
	else
		ifsd->status = SCAN_TEST_PASS;
}