[v3,4/8] platform/x86/intel/ifs: Introduce Array Scan test to IFS

  Array BIST is a new type of core test introduced under the Intel Infield
Scan (IFS) suite of tests.

Emerald Rapids (EMR) is the first CPU to support Array BIST.
Array BIST performs tests on some portions of the core logic such as
caches and register files. These are different portions of the silicon
compared to the parts tested by the first test type
i.e Scan at Field (SAF).

Make changes in the device driver init flow to register this new test
type with the device driver framework. Each test will have its own
sysfs directory (intel_ifs_0 , intel_ifs_1) under misc hierarchy to
accommodate for the differences in test type and how they are initiated.

Upcoming patches will add actual support.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 drivers/platform/x86/intel/ifs/ifs.h  |  3 +
 drivers/platform/x86/intel/ifs/core.c | 85 +++++++++++++++++++--------
 2 files changed, 62 insertions(+), 26 deletions(-)
  

Hi,

On 3/1/23 02:59, Jithu Joseph wrote:
> Array BIST is a new type of core test introduced under the Intel Infield
> Scan (IFS) suite of tests.
> 
> Emerald Rapids (EMR) is the first CPU to support Array BIST.
> Array BIST performs tests on some portions of the core logic such as
> caches and register files. These are different portions of the silicon
> compared to the parts tested by the first test type
> i.e Scan at Field (SAF).
> 
> Make changes in the device driver init flow to register this new test
> type with the device driver framework. Each test will have its own
> sysfs directory (intel_ifs_0 , intel_ifs_1) under misc hierarchy to
> accommodate for the differences in test type and how they are initiated.
> 
> Upcoming patches will add actual support.
> 
> Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> ---
>  drivers/platform/x86/intel/ifs/ifs.h  |  3 +
>  drivers/platform/x86/intel/ifs/core.c | 85 +++++++++++++++++++--------
>  2 files changed, 62 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index ab168ddf28f1..b8b956e29653 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -137,6 +137,9 @@
>  #define SCAN_TEST_PASS				1
>  #define SCAN_TEST_FAIL				2
>  
> +#define IFS_TYPE_SAF			0
> +#define IFS_TYPE_ARRAY_BIST		1
> +
>  /* MSR_SCAN_HASHES_STATUS bit fields */
>  union ifs_scan_hashes_status {
>  	u64	data;
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 62c44dbae757..2237aaba7078 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -16,6 +16,7 @@
>  
>  static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
>  	X86_MATCH(SAPPHIRERAPIDS_X),
> +	X86_MATCH(EMERALDRAPIDS_X),
>  	{}
>  };
>  MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);

Note you can add driver_data to a match table like this. What you should
do here is use the driver data to point to the const ifs_hw_caps discussed
before, so what you get here is:

#define X86_MATCH(model, data)                          \
        X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6,    \
                INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, (unsigned long)(data))

static const struct ifs_hw_caps saphire_rapids_caps = {
	.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
	.test_num = 0,
};

static const struct ifs_hw_caps emerald_rapids_caps = {
	.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
	.test_num = 0,
};

static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
	X86_MATCH(SAPPHIRERAPIDS_X, &saphire_rapids_caps),
	X86_MATCH(EMERALDRAPIDS_X, &emerald_rapids_caps),
	{}
};
MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);

and then drop all the code related to having an array of ifs_device structs
(of which only 1 will ever get used) and instead at the beginning of
ifs_init(void), after:

        m = x86_match_cpu(ifs_cpu_ids);
        if (!m)
                return -ENODEV;

add:

	ifs_device.hwcaps = (const struct ifs_hw_caps *)m->driver_data;

And then you can pretty much drop all the rest of this patch and we
end up with much nicer code for differentiating between the models :)

Regards,

Hans






> @@ -24,23 +25,51 @@ ATTRIBUTE_GROUPS(plat_ifs);
>  
>  bool *ifs_pkg_auth;
>  
> -static struct ifs_device ifs_device = {
> -	.ro_data = {
> -		.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
> -		.test_num = 0,
> +static struct ifs_device ifs_devices[] = {
> +	[IFS_TYPE_SAF] = {
> +		.ro_data = {
> +			.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
> +			.test_num = IFS_TYPE_SAF,
> +		},
> +		.misc = {
> +			.name = "intel_ifs_0",
> +			.minor = MISC_DYNAMIC_MINOR,
> +			.groups = plat_ifs_groups,
> +		},
>  	},
> -	.misc = {
> -		.name = "intel_ifs_0",
> -		.minor = MISC_DYNAMIC_MINOR,
> -		.groups = plat_ifs_groups,
> +	[IFS_TYPE_ARRAY_BIST] = {
> +		.ro_data = {
> +			.integrity_cap_bit = MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT,
> +			.test_num = IFS_TYPE_ARRAY_BIST,
> +		},
> +		.misc = {
> +			.name = "intel_ifs_1",
> +			.minor = MISC_DYNAMIC_MINOR,
> +		},
>  	},
>  };
>  
> +#define IFS_NUMTESTS ARRAY_SIZE(ifs_devices)
> +
> +static void ifs_cleanup(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < IFS_NUMTESTS; i++) {
> +		if (ifs_devices[i].misc.this_device) {
> +			misc_deregister(&ifs_devices[i].misc);
> +			kfree(ifs_devices[i].rw_data);
> +		}
> +	}
> +	kfree(ifs_pkg_auth);
> +}
> +
>  static int __init ifs_init(void)
>  {
>  	const struct x86_cpu_id *m;
>  	struct ifs_data *ifsd;
>  	u64 msrval;
> +	int i, ret;
>  
>  	m = x86_match_cpu(ifs_cpu_ids);
>  	if (!m)
> @@ -55,35 +84,39 @@ static int __init ifs_init(void)
>  	if (rdmsrl_safe(MSR_INTEGRITY_CAPS, &msrval))
>  		return -ENODEV;
>  
> -	if (!(msrval & BIT(ifs_device.ro_data.integrity_cap_bit)))
> -		return -ENODEV;
> -
>  	ifs_pkg_auth = kmalloc_array(topology_max_packages(), sizeof(bool), GFP_KERNEL);
>  	if (!ifs_pkg_auth)
>  		return -ENOMEM;
>  
> -	ifsd = kzalloc(sizeof(*ifsd), GFP_KERNEL);
> -	if (!ifsd)
> -		return -ENOMEM;
> -
> -	ifsd->ro_info = &ifs_device.ro_data;
> -	ifs_device.rw_data = ifsd;
> -
> -	if (misc_register(&ifs_device.misc)) {
> -		kfree(ifsd);
> -		kfree(ifs_pkg_auth);
> -		return -ENODEV;
> +	for (i = 0; i < IFS_NUMTESTS; i++) {
> +		ifsd = NULL;
> +		if (!(msrval & BIT(ifs_devices[i].ro_data.integrity_cap_bit)))
> +			continue;
> +
> +		ifsd = kzalloc(sizeof(*ifsd), GFP_KERNEL);
> +		if (!ifsd) {
> +			ret = -ENOMEM;
> +			goto err_exit;
> +		}
> +		ifsd->ro_info = &ifs_devices[i].ro_data;
> +		ifs_devices[i].rw_data = ifsd;
> +
> +		if (misc_register(&ifs_devices[i].misc)) {
> +			ret = -ENODEV;
> +			kfree(ifsd);
> +			goto err_exit;
> +		}
>  	}
> -
>  	return 0;
>  
> +err_exit:
> +	ifs_cleanup();
> +	return ret;
>  }
>  
>  static void __exit ifs_exit(void)
>  {
> -	misc_deregister(&ifs_device.misc);
> -	kfree(ifs_device.rw_data);
> -	kfree(ifs_pkg_auth);
> +	ifs_cleanup();
>  }
>  
>  module_init(ifs_init);
  

Hi,

On 3/13/23 17:10, Hans de Goede wrote:
> Hi,
> 
> On 3/1/23 02:59, Jithu Joseph wrote:
>> Array BIST is a new type of core test introduced under the Intel Infield
>> Scan (IFS) suite of tests.
>>
>> Emerald Rapids (EMR) is the first CPU to support Array BIST.
>> Array BIST performs tests on some portions of the core logic such as
>> caches and register files. These are different portions of the silicon
>> compared to the parts tested by the first test type
>> i.e Scan at Field (SAF).
>>
>> Make changes in the device driver init flow to register this new test
>> type with the device driver framework. Each test will have its own
>> sysfs directory (intel_ifs_0 , intel_ifs_1) under misc hierarchy to
>> accommodate for the differences in test type and how they are initiated.
>>
>> Upcoming patches will add actual support.
>>
>> Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> ---
>>  drivers/platform/x86/intel/ifs/ifs.h  |  3 +
>>  drivers/platform/x86/intel/ifs/core.c | 85 +++++++++++++++++++--------
>>  2 files changed, 62 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
>> index ab168ddf28f1..b8b956e29653 100644
>> --- a/drivers/platform/x86/intel/ifs/ifs.h
>> +++ b/drivers/platform/x86/intel/ifs/ifs.h
>> @@ -137,6 +137,9 @@
>>  #define SCAN_TEST_PASS				1
>>  #define SCAN_TEST_FAIL				2
>>  
>> +#define IFS_TYPE_SAF			0
>> +#define IFS_TYPE_ARRAY_BIST		1
>> +
>>  /* MSR_SCAN_HASHES_STATUS bit fields */
>>  union ifs_scan_hashes_status {
>>  	u64	data;
>> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
>> index 62c44dbae757..2237aaba7078 100644
>> --- a/drivers/platform/x86/intel/ifs/core.c
>> +++ b/drivers/platform/x86/intel/ifs/core.c
>> @@ -16,6 +16,7 @@
>>  
>>  static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
>>  	X86_MATCH(SAPPHIRERAPIDS_X),
>> +	X86_MATCH(EMERALDRAPIDS_X),
>>  	{}
>>  };
>>  MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
> 
> Note you can add driver_data to a match table like this. What you should
> do here is use the driver data to point to the const ifs_hw_caps discussed
> before, so what you get here is:
> 
> #define X86_MATCH(model, data)                          \
>         X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6,    \
>                 INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, (unsigned long)(data))
> 
> static const struct ifs_hw_caps saphire_rapids_caps = {
> 	.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
> 	.test_num = 0,
> };
> 
> static const struct ifs_hw_caps emerald_rapids_caps = {
> 	.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
> 	.test_num = 0,
> };
> 
> static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
> 	X86_MATCH(SAPPHIRERAPIDS_X, &saphire_rapids_caps),
> 	X86_MATCH(EMERALDRAPIDS_X, &emerald_rapids_caps),
> 	{}
> };
> MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
> 
> and then drop all the code related to having an array of ifs_device structs
> (of which only 1 will ever get used) and instead at the beginning of
> ifs_init(void), after:
> 
>         m = x86_match_cpu(ifs_cpu_ids);
>         if (!m)
>                 return -ENODEV;
> 
> add:
> 
> 	ifs_device.hwcaps = (const struct ifs_hw_caps *)m->driver_data;
> 
> And then you can pretty much drop all the rest of this patch and we
> end up with much nicer code for differentiating between the models :)

Upon reading the rest of the series, I think the above might be based
on me misunderstanding this bit.

If I understand things correctly then what is new with emerald_rapids
is support for a second set/type of tests called " Array Scan test"
and the old test method / test-type is also still supported.

And you have chosen to register 2 misc-devices , one per supported
test type ?

Have I understood that correcty?

May I ask why use 1 misc device per test-type. Why not just add
a single new sysfs_attr to the existing misc device to trigger
the new test-type ?

Regards,

Hans


>> @@ -24,23 +25,51 @@ ATTRIBUTE_GROUPS(plat_ifs);
>>  
>>  bool *ifs_pkg_auth;
>>  
>> -static struct ifs_device ifs_device = {
>> -	.ro_data = {
>> -		.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
>> -		.test_num = 0,
>> +static struct ifs_device ifs_devices[] = {
>> +	[IFS_TYPE_SAF] = {
>> +		.ro_data = {
>> +			.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
>> +			.test_num = IFS_TYPE_SAF,
>> +		},
>> +		.misc = {
>> +			.name = "intel_ifs_0",
>> +			.minor = MISC_DYNAMIC_MINOR,
>> +			.groups = plat_ifs_groups,
>> +		},
>>  	},
>> -	.misc = {
>> -		.name = "intel_ifs_0",
>> -		.minor = MISC_DYNAMIC_MINOR,
>> -		.groups = plat_ifs_groups,
>> +	[IFS_TYPE_ARRAY_BIST] = {
>> +		.ro_data = {
>> +			.integrity_cap_bit = MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT,
>> +			.test_num = IFS_TYPE_ARRAY_BIST,
>> +		},
>> +		.misc = {
>> +			.name = "intel_ifs_1",
>> +			.minor = MISC_DYNAMIC_MINOR,
>> +		},
>>  	},
>>  };
>>  
>> +#define IFS_NUMTESTS ARRAY_SIZE(ifs_devices)
>> +
>> +static void ifs_cleanup(void)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < IFS_NUMTESTS; i++) {
>> +		if (ifs_devices[i].misc.this_device) {
>> +			misc_deregister(&ifs_devices[i].misc);
>> +			kfree(ifs_devices[i].rw_data);
>> +		}
>> +	}
>> +	kfree(ifs_pkg_auth);
>> +}
>> +
>>  static int __init ifs_init(void)
>>  {
>>  	const struct x86_cpu_id *m;
>>  	struct ifs_data *ifsd;
>>  	u64 msrval;
>> +	int i, ret;
>>  
>>  	m = x86_match_cpu(ifs_cpu_ids);
>>  	if (!m)
>> @@ -55,35 +84,39 @@ static int __init ifs_init(void)
>>  	if (rdmsrl_safe(MSR_INTEGRITY_CAPS, &msrval))
>>  		return -ENODEV;
>>  
>> -	if (!(msrval & BIT(ifs_device.ro_data.integrity_cap_bit)))
>> -		return -ENODEV;
>> -
>>  	ifs_pkg_auth = kmalloc_array(topology_max_packages(), sizeof(bool), GFP_KERNEL);
>>  	if (!ifs_pkg_auth)
>>  		return -ENOMEM;
>>  
>> -	ifsd = kzalloc(sizeof(*ifsd), GFP_KERNEL);
>> -	if (!ifsd)
>> -		return -ENOMEM;
>> -
>> -	ifsd->ro_info = &ifs_device.ro_data;
>> -	ifs_device.rw_data = ifsd;
>> -
>> -	if (misc_register(&ifs_device.misc)) {
>> -		kfree(ifsd);
>> -		kfree(ifs_pkg_auth);
>> -		return -ENODEV;
>> +	for (i = 0; i < IFS_NUMTESTS; i++) {
>> +		ifsd = NULL;
>> +		if (!(msrval & BIT(ifs_devices[i].ro_data.integrity_cap_bit)))
>> +			continue;
>> +
>> +		ifsd = kzalloc(sizeof(*ifsd), GFP_KERNEL);
>> +		if (!ifsd) {
>> +			ret = -ENOMEM;
>> +			goto err_exit;
>> +		}
>> +		ifsd->ro_info = &ifs_devices[i].ro_data;
>> +		ifs_devices[i].rw_data = ifsd;
>> +
>> +		if (misc_register(&ifs_devices[i].misc)) {
>> +			ret = -ENODEV;
>> +			kfree(ifsd);
>> +			goto err_exit;
>> +		}
>>  	}
>> -
>>  	return 0;
>>  
>> +err_exit:
>> +	ifs_cleanup();
>> +	return ret;
>>  }
>>  
>>  static void __exit ifs_exit(void)
>>  {
>> -	misc_deregister(&ifs_device.misc);
>> -	kfree(ifs_device.rw_data);
>> -	kfree(ifs_pkg_auth);
>> +	ifs_cleanup();
>>  }
>>  
>>  module_init(ifs_init);
  

> Upon reading the rest of the series, I think the above might be based
> on me misunderstanding this bit.
>
> If I understand things correctly then what is new with emerald_rapids
> is support for a second set/type of tests called " Array Scan test"
> and the old test method / test-type is also still supported.

Yes. Emerald Rapids supports the new array scan test *in addition to*
the existing scan test.  Future CPUs will support both of these tests
and may add additional tests. Further in the future if some new tests
cover the same functionality as older tests, some of the older tests
may be dropped.

So the INTEGRITY_CAPABILITIES MSR is a bitmap enumerating
which tests are supported on each model.

> And you have chosen to register 2 misc-devices , one per supported
> test type ?
>
> Have I understood that correctly?

Yes.

> May I ask why use 1 misc device per test-type. Why not just add
> a single new sysfs_attr to the existing misc device to trigger
> the new test-type ?

That's an interesting idea. So we'd have:

# echo $cpu > run_test
# cat status
...
# cat details
...

for our first type of test (introduced with Sapphire Rapids). Then
in the same directory add a new file to run the array test (on Emerald
Rapids andother future CPUs that support it)

# echo $cpu > run_array_test
# cat status
...
# cat details
...

But I see a problem with the "current_batch" file (that will show up if a future
test also needs to load some test data ... spoiler ... it will).

We can't do:

# echo 2 > current_batch
  ... what should this load, don't know until the user types one of

# echo $cpu > run_test

or

# echo $cpu > run_${name}_test

Perhaps also have per test type names to load the test images?

# echo 2 > current_${name}_batch
# echo $cpu > run_${name}_test


I'm not sure this is better than splitting the tests into different directories.

-Tony
  

On 3/13/2023 10:21 AM, Luck, Tony wrote:

> 
> 
> I'm not sure this is better than splitting the tests into different directories.
> 

To provide a bit more context to Tony's response - 
There are similarities between tests (one of the test inputs is cpu number , which is 
common among different tests , so are the test outputs described via status / details attributes)
and some differences too (distinct to each test are the test patterns, some tests needs to
load test patterns appropriate for that test type and some does not)

Current approach of keeping each test's attributes in its own directory (intel_ifs_n)
allows the driver to account for the differences naturally, for e.g load test file
suited for a specific test. (I.e if the load is issued from intel_ifs_<n> , the driver
will load test patterns from /lib/firmware/intel/ifs/ifs_<n>/ff-mm-ss-xx.<type_n>).
If load is not applicable for a test type , test directory doesn’t show load and image_version attributes). 

As Tony mentioned, similar effect might be achieved using distinct load / run (and image_version)
files for each test type, but the current approach of organizing files per test feels a little
bit more intuitive. 

Grouping attributes per test was the original design intent , when the first test was introduced,
as indicated by the existing ABI doc (Documentation/ABI/testing/sysfs-platform-intel-ifs),
wherein attributes are described under /sys/devices/virtual/misc/intel_ifs_<N>/ …

Hans, Shall I revise the series incorporating the rest of your comments ?

Jithu
  

Hi,

On 3/15/23 20:29, Joseph, Jithu wrote:
> 
> 
> On 3/13/2023 10:21 AM, Luck, Tony wrote:
> 
>>
>>
>> I'm not sure this is better than splitting the tests into different directories.
>>
> 
> To provide a bit more context to Tony's response - 
> There are similarities between tests (one of the test inputs is cpu number , which is 
> common among different tests , so are the test outputs described via status / details attributes)
> and some differences too (distinct to each test are the test patterns, some tests needs to
> load test patterns appropriate for that test type and some does not)
> 
> Current approach of keeping each test's attributes in its own directory (intel_ifs_n)
> allows the driver to account for the differences naturally, for e.g load test file
> suited for a specific test. (I.e if the load is issued from intel_ifs_<n> , the driver
> will load test patterns from /lib/firmware/intel/ifs/ifs_<n>/ff-mm-ss-xx.<type_n>).
> If load is not applicable for a test type , test directory doesn’t show load and image_version attributes). 
> 
> As Tony mentioned, similar effect might be achieved using distinct load / run (and image_version)
> files for each test type, but the current approach of organizing files per test feels a little
> bit more intuitive. 
> 
> Grouping attributes per test was the original design intent , when the first test was introduced,
> as indicated by the existing ABI doc (Documentation/ABI/testing/sysfs-platform-intel-ifs),
> wherein attributes are described under /sys/devices/virtual/misc/intel_ifs_<N>/ …

Ok I see, lets go with 1 intel_ifs device per test-type then.

If I understood things correctly esp. also with the /lib/firmware path then the <N> in intel_ifs_<N> basically specifies the test-type, correct ?

If I have that correct please add this to the ABI documentation in the form of a list with

intel_ifs_<N> <-> test-type

mappings. And also add documentation to each attribute for which test-types the attribute is valid (this can be "all" for e.g. status, to avoid churn when adding more test types).

> Hans, Shall I revise the series incorporating the rest of your comments ?

Yes please.

Regards,

Hans
  

On 3/16/2023 2:50 AM, Hans de Goede wrote:

> Ok I see, lets go with 1 intel_ifs device per test-type then.
> 
> If I understood things correctly esp. also with the /lib/firmware path then the <N> in intel_ifs_<N> basically specifies the test-type, correct ?
> 

Correct

> If I have that correct please add this to the ABI documentation in the form of a list with
> 
> intel_ifs_<N> <-> test-type
> 
> mappings. And also add documentation to each attribute for which test-types the attribute is valid (this can be "all" for e.g. status, to avoid churn when adding more test types).

Will do. Thanks for the review comments

Jithu