Re: [PATCH v2 4/7] platform/x86/intel/ifs: Implement Array BIST test

[Joseph, Jithu]

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