[PATCH 00/10] IFS support for GNR and SRF

[PATCH 00/10] IFS support for GNR and SRF

[Jithu Joseph]

This series adds IFS support for newer CPUs like Granite Rapids(GNR)
and Sierra Forest(SRF).

There are changes in the IFS image loading and test flow to support
these new CPUs.

Note to reviewers:
 - patch 01/10 adds a bit definition to arch/x86/.../msr-index.h,
  hence x86 maintainers are cc-d.
 - patch 05/10 modifies an existing tracepoint, cc Steven Rostedt
 - Rest are localized to IFS driver

Jithu Joseph (10):
  platform/x86/intel/ifs: Store IFS generation number
  platform/x86/intel/ifs: Refactor image loading code
  platform/x86/intel/ifs: Image loading for new generations
  platform/x86/intel/ifs: Scan test for new generations
  trace: platform/x86/intel/ifs: Modify scan trace
  platform/x86/intel/ifs: Validate image size
  platform/x86/intel/ifs: Metadata validation for start_chunk
  platform/x86/intel/ifs: Add new CPU support
  platform/x86/intel/ifs: Add new error code
  platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

 arch/x86/include/asm/msr-index.h         |   2 +
 drivers/platform/x86/intel/ifs/ifs.h     |  47 +++++++
 include/trace/events/intel_ifs.h         |  16 +--
 drivers/platform/x86/intel/ifs/core.c    |  14 +-
 drivers/platform/x86/intel/ifs/load.c    | 159 +++++++++++++++++++++--
 drivers/platform/x86/intel/ifs/runtest.c |  68 +++++++++-
 6 files changed, 273 insertions(+), 33 deletions(-)


base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
-- 
2.25.1

[PATCH 01/10] platform/x86/intel/ifs: Store IFS generation number

[Jithu Joseph]

IFS generation number is reported via MSR_INTEGRITY_CAPS.
As IFS support gets added to newer CPUs, some differences
are expected during IFS image loading and test flows.

Define MSR bitmasks to extract and store the generation in
driver data, so that driver can modify its MSR interaction
appropriately.

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>
---
 arch/x86/include/asm/msr-index.h      | 2 ++
 drivers/platform/x86/intel/ifs/ifs.h  | 2 ++
 drivers/platform/x86/intel/ifs/core.c | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 1d111350197f..a71a86a01488 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -222,6 +222,8 @@
 #define MSR_INTEGRITY_CAPS_ARRAY_BIST          BIT(MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT)
 #define MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT	4
 #define MSR_INTEGRITY_CAPS_PERIODIC_BIST	BIT(MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT)
+#define MSR_INTEGRITY_CAPS_SAF_GEN_REV_SHIFT	9
+#define MSR_INTEGRITY_CAPS_SAF_GEN_REV_MASK	(0x3ull << MSR_INTEGRITY_CAPS_SAF_GEN_REV_SHIFT)
 
 #define MSR_LBR_NHM_FROM		0x00000680
 #define MSR_LBR_NHM_TO			0x000006c0
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 93191855890f..d666aeed20fc 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -229,6 +229,7 @@ struct ifs_test_caps {
  * @status: it holds simple status pass/fail/untested
  * @scan_details: opaque scan status code from h/w
  * @cur_batch: number indicating the currently loaded test file
+ * @generation: IFS test generation enumerated by hardware
  */
 struct ifs_data {
 	int	loaded_version;
@@ -238,6 +239,7 @@ struct ifs_data {
 	int	status;
 	u64	scan_details;
 	u32	cur_batch;
+	u32	generation;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 306f886b52d2..88d84aad9334 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -94,6 +94,8 @@ static int __init ifs_init(void)
 	for (i = 0; i < IFS_NUMTESTS; i++) {
 		if (!(msrval & BIT(ifs_devices[i].test_caps->integrity_cap_bit)))
 			continue;
+		ifs_devices[i].rw_data.generation = (msrval & MSR_INTEGRITY_CAPS_SAF_GEN_REV_MASK)
+							>> MSR_INTEGRITY_CAPS_SAF_GEN_REV_SHIFT;
 		ret = misc_register(&ifs_devices[i].misc);
 		if (ret)
 			goto err_exit;
-- 
2.25.1

Re: [PATCH 01/10] platform/x86/intel/ifs: Store IFS generation number

[Ilpo Järvinen]

On Wed, 13 Sep 2023, Jithu Joseph wrote:

> IFS generation number is reported via MSR_INTEGRITY_CAPS.

Please use more characters per line, the limit is 72 characters.

> As IFS support gets added to newer CPUs, some differences
> are expected during IFS image loading and test flows.
> 
> Define MSR bitmasks to extract and store the generation in
> driver data, so that driver can modify its MSR interaction
> appropriately.
> 
> 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>
> ---
>  arch/x86/include/asm/msr-index.h      | 2 ++
>  drivers/platform/x86/intel/ifs/ifs.h  | 2 ++
>  drivers/platform/x86/intel/ifs/core.c | 2 ++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 1d111350197f..a71a86a01488 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -222,6 +222,8 @@
>  #define MSR_INTEGRITY_CAPS_ARRAY_BIST          BIT(MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT)
>  #define MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT	4
>  #define MSR_INTEGRITY_CAPS_PERIODIC_BIST	BIT(MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT)
> +#define MSR_INTEGRITY_CAPS_SAF_GEN_REV_SHIFT	9
> +#define MSR_INTEGRITY_CAPS_SAF_GEN_REV_MASK	(0x3ull << MSR_INTEGRITY_CAPS_SAF_GEN_REV_SHIFT)

GENMASK_ULL(), don't add _SHIFT at all as FIELD_GET/PREP() will handle 
it for you.

>  #define MSR_LBR_NHM_FROM		0x00000680
>  #define MSR_LBR_NHM_TO			0x000006c0
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 93191855890f..d666aeed20fc 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -229,6 +229,7 @@ struct ifs_test_caps {
>   * @status: it holds simple status pass/fail/untested
>   * @scan_details: opaque scan status code from h/w
>   * @cur_batch: number indicating the currently loaded test file
> + * @generation: IFS test generation enumerated by hardware
>   */
>  struct ifs_data {
>  	int	loaded_version;
> @@ -238,6 +239,7 @@ struct ifs_data {
>  	int	status;
>  	u64	scan_details;
>  	u32	cur_batch;
> +	u32	generation;
>  };
>  
>  struct ifs_work {
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 306f886b52d2..88d84aad9334 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -94,6 +94,8 @@ static int __init ifs_init(void)
>  	for (i = 0; i < IFS_NUMTESTS; i++) {
>  		if (!(msrval & BIT(ifs_devices[i].test_caps->integrity_cap_bit)))
>  			continue;
> +		ifs_devices[i].rw_data.generation = (msrval & MSR_INTEGRITY_CAPS_SAF_GEN_REV_MASK)
> +							>> MSR_INTEGRITY_CAPS_SAF_GEN_REV_SHIFT;

FIELD_GET(), don't forget to make sure use have the include for it.

>  		ret = misc_register(&ifs_devices[i].misc);
>  		if (ret)
>  			goto err_exit;
> 

-- 
 i.

Re: [PATCH 01/10] platform/x86/intel/ifs: Store IFS generation number

[Joseph, Jithu]

Appreciate the review

On 9/15/2023 9:22 AM, Ilpo Järvinen wrote:
> On Wed, 13 Sep 2023, Jithu Joseph wrote:
> 
>> IFS generation number is reported via MSR_INTEGRITY_CAPS.
> 
> Please use more characters per line, the limit is 72 characters.
> 

Noted
...


>> +#define MSR_INTEGRITY_CAPS_SAF_GEN_REV_SHIFT	9
>> +#define MSR_INTEGRITY_CAPS_SAF_GEN_REV_MASK	(0x3ull << MSR_INTEGRITY_CAPS_SAF_GEN_REV_SHIFT)
> 
> GENMASK_ULL(), don't add _SHIFT at all as FIELD_GET/PREP() will handle 
> it for you.

Thanks, Will change
...

>>  struct ifs_work {
>> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
>> index 306f886b52d2..88d84aad9334 100644
>> --- a/drivers/platform/x86/intel/ifs/core.c
>> +++ b/drivers/platform/x86/intel/ifs/core.c
>> @@ -94,6 +94,8 @@ static int __init ifs_init(void)
>>  	for (i = 0; i < IFS_NUMTESTS; i++) {
>>  		if (!(msrval & BIT(ifs_devices[i].test_caps->integrity_cap_bit)))
>>  			continue;
>> +		ifs_devices[i].rw_data.generation = (msrval & MSR_INTEGRITY_CAPS_SAF_GEN_REV_MASK)
>> +							>> MSR_INTEGRITY_CAPS_SAF_GEN_REV_SHIFT;
> 
> FIELD_GET(), don't forget to make sure use have the include for it.

Will change


Jithu

[PATCH 02/10] platform/x86/intel/ifs: Refactor image loading code

[Jithu Joseph]

IFS image loading flow is slightly different for newer IFS
generations.

In preparation for adding support for newer IFS generations,
refactor portions of existing image loading code for reuse.

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/load.c | 31 ++++++++++++++++-----------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index cefd0d886cfd..851c97cc6a6b 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -80,6 +80,23 @@ static struct metadata_header *find_meta_data(void *ucode, unsigned int meta_typ
 	return NULL;
 }
 
+static void hashcopy_err_message(struct device *dev, u32 err_code)
+{
+	if (err_code >= ARRAY_SIZE(scan_hash_status))
+		dev_err(dev, "invalid error code 0x%x for hash copy\n", err_code);
+	else
+		dev_err(dev, "Hash copy error : %s\n", scan_hash_status[err_code]);
+}
+
+static void auth_err_message(struct device *dev, u32 err_code)
+{
+	if (err_code >= ARRAY_SIZE(scan_authentication_status))
+		dev_err(dev, "invalid error code 0x%x for authentication\n", err_code);
+	else
+		dev_err(dev, "Chunk authentication error : %s\n",
+			scan_authentication_status[err_code]);
+}
+
 /*
  * To copy scan hashes and authenticate test chunks, the initiating cpu must point
  * to the EDX:EAX to the test image in linear address.
@@ -109,11 +126,7 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
 
 	if (!hashes_status.valid) {
 		ifsd->loading_error = true;
-		if (err_code >= ARRAY_SIZE(scan_hash_status)) {
-			dev_err(dev, "invalid error code 0x%x for hash copy\n", err_code);
-			goto done;
-		}
-		dev_err(dev, "Hash copy error : %s", scan_hash_status[err_code]);
+		hashcopy_err_message(dev, err_code);
 		goto done;
 	}
 
@@ -133,13 +146,7 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
 
 		if (err_code) {
 			ifsd->loading_error = true;
-			if (err_code >= ARRAY_SIZE(scan_authentication_status)) {
-				dev_err(dev,
-					"invalid error code 0x%x for authentication\n", err_code);
-				goto done;
-			}
-			dev_err(dev, "Chunk authentication error %s\n",
-				scan_authentication_status[err_code]);
+			auth_err_message(dev, err_code);
 			goto done;
 		}
 	}
-- 
2.25.1

[PATCH 03/10] platform/x86/intel/ifs: Image loading for new generations

[Jithu Joseph]

Scan image loading flow for newer IFS generations (1 and 2) are slightly
different from that of current generation (0). In newer schemes,
loading need not be done once for each socket as was done in gen0.

Also the width of CHUNK related bitfields in SCAN_HASHES_STATUS MSR has
increased from 8 -> 16 bits. Similarly there are width differences
for CHUNK_AUTHENTICATION_STATUS too.

Further the parameter to AUTHENTICATE_AND_COPY_CHUNK is passed
differently in newer generations.

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  |  27 ++++++
 drivers/platform/x86/intel/ifs/load.c | 113 +++++++++++++++++++++++++-
 2 files changed, 138 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index d666aeed20fc..886dc74de57d 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -137,6 +137,8 @@
 #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
 #define MSR_ACTIVATE_SCAN			0x000002c6
 #define MSR_SCAN_STATUS				0x000002c7
+#define MSR_SAF_CTRL				0x000004f0
+
 #define SCAN_NOT_TESTED				0
 #define SCAN_TEST_PASS				1
 #define SCAN_TEST_FAIL				2
@@ -158,6 +160,19 @@ union ifs_scan_hashes_status {
 	};
 };
 
+union ifs_scan_hashes_status_gen2 {
+	u64	data;
+	struct {
+		u16	chunk_size;
+		u16	num_chunks;
+		u8	error_code;
+		u32	chunks_in_stride :9;
+		u32	rsvd		:2;
+		u32	max_core_limit	:12;
+		u32	valid		:1;
+	};
+};
+
 /* MSR_CHUNKS_AUTH_STATUS bit fields */
 union ifs_chunks_auth_status {
 	u64	data;
@@ -170,6 +185,16 @@ union ifs_chunks_auth_status {
 	};
 };
 
+union ifs_chunks_auth_status_gen2 {
+	u64	data;
+	struct {
+		u16	valid_chunks;
+		u16	total_chunks;
+		u8	error_code;
+		u32	rsvd		:24;
+	};
+};
+
 /* MSR_ACTIVATE_SCAN bit fields */
 union ifs_scan {
 	u64	data;
@@ -230,6 +255,7 @@ struct ifs_test_caps {
  * @scan_details: opaque scan status code from h/w
  * @cur_batch: number indicating the currently loaded test file
  * @generation: IFS test generation enumerated by hardware
+ * @chunk_size: size of a test chunk
  */
 struct ifs_data {
 	int	loaded_version;
@@ -240,6 +266,7 @@ struct ifs_data {
 	u64	scan_details;
 	u32	cur_batch;
 	u32	generation;
+	u32	chunk_size;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 851c97cc6a6b..e8fb03dd8bcf 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -26,6 +26,11 @@ union meta_data {
 
 #define IFS_HEADER_SIZE	(sizeof(struct microcode_header_intel))
 #define META_TYPE_IFS	1
+#define INVALIDATE_STRIDE (0x1UL)
+#define IFS_GEN_STRIDE_AWARE	2
+#define AUTH_INTERRUPTED_ERROR	5
+#define IFS_AUTH_RETRY_CT	10
+
 static  struct microcode_header_intel *ifs_header_ptr;	/* pointer to the ifs image header */
 static u64 ifs_hash_ptr;			/* Address of ifs metadata (hash) */
 static u64 ifs_test_image_ptr;			/* 256B aligned address of test pattern */
@@ -44,7 +49,10 @@ static const char * const scan_hash_status[] = {
 static const char * const scan_authentication_status[] = {
 	[0] = "No error reported",
 	[1] = "Attempt to authenticate a chunk which is already marked as authentic",
-	[2] = "Chunk authentication error. The hash of chunk did not match expected value"
+	[2] = "Chunk authentication error. The hash of chunk did not match expected value",
+	[3] = "Reserved",
+	[4] = "Chunk outside the current stride",
+	[5] = "Authentication flow interrupted"
 };
 
 #define MC_HEADER_META_TYPE_END		(0)
@@ -154,6 +162,104 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
 	complete(&ifs_done);
 }
 
+static int get_num_chunks(int gen, union ifs_scan_hashes_status_gen2 status)
+{
+	return  gen >= IFS_GEN_STRIDE_AWARE ? status.chunks_in_stride : status.num_chunks;
+}
+
+static bool need_copy_scan_hashes(struct ifs_data *ifsd)
+{
+	if (!ifsd->loaded || ifsd->generation < IFS_GEN_STRIDE_AWARE ||
+	    ifsd->loaded_version != ifs_header_ptr->rev) {
+		return true;
+	}
+	return false;
+}
+
+static int copy_hashes_authenticate_chunks_gen2(struct device *dev)
+{
+	union ifs_scan_hashes_status_gen2 hashes_status;
+	union ifs_chunks_auth_status_gen2 chunk_status;
+	u32 err_code, valid_chunks, total_chunks;
+	int i, num_chunks, chunk_size;
+	union meta_data *ifs_meta;
+	int starting_chunk_nr;
+	struct ifs_data *ifsd;
+	u64 linear_addr, base;
+	u64 chunk_table[2];
+	int retry_count;
+
+	ifsd = ifs_get_data(dev);
+
+	if (need_copy_scan_hashes(ifsd)) {
+		wrmsrl(MSR_COPY_SCAN_HASHES, ifs_hash_ptr);
+		rdmsrl(MSR_SCAN_HASHES_STATUS, hashes_status.data);
+
+		/* enumerate the scan image information */
+		chunk_size = hashes_status.chunk_size * 1024;
+		err_code = hashes_status.error_code;
+
+		num_chunks = get_num_chunks(ifsd->generation, hashes_status);
+
+		if (!hashes_status.valid) {
+			hashcopy_err_message(dev, err_code);
+			return -EIO;
+		}
+		ifsd->loaded_version = ifs_header_ptr->rev;
+		ifsd->chunk_size = chunk_size;
+	} else {
+		num_chunks = ifsd->valid_chunks;
+		chunk_size = ifsd->chunk_size;
+	}
+
+	if (ifsd->generation >= IFS_GEN_STRIDE_AWARE) {
+		wrmsrl(MSR_SAF_CTRL, INVALIDATE_STRIDE);
+		rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
+		if (chunk_status.valid_chunks != 0) {
+			dev_err(dev, "Couldn't invalidate installed stride - %d\n",
+				chunk_status.valid_chunks);
+			return -EIO;
+		}
+	}
+
+	base = ifs_test_image_ptr;
+	ifs_meta = (union meta_data *)find_meta_data(ifs_header_ptr, META_TYPE_IFS);
+	starting_chunk_nr = ifs_meta->starting_chunk;
+
+	/* scan data authentication and copy chunks to secured memory */
+	for (i = 0; i < num_chunks; i++) {
+		retry_count = IFS_AUTH_RETRY_CT;
+		linear_addr = base + i * chunk_size;
+
+		chunk_table[0] = starting_chunk_nr + i;
+		chunk_table[1] = linear_addr;
+auth_retry:
+		wrmsrl(MSR_AUTHENTICATE_AND_COPY_CHUNK, (u64)chunk_table);
+		rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
+		err_code = chunk_status.error_code;
+		if (err_code == AUTH_INTERRUPTED_ERROR && --retry_count)
+			goto auth_retry;
+		if (err_code) {
+			ifsd->loading_error = true;
+			auth_err_message(dev, err_code);
+			return -EIO;
+		}
+	}
+
+	valid_chunks = chunk_status.valid_chunks;
+	total_chunks = chunk_status.total_chunks;
+
+	if (valid_chunks != total_chunks) {
+		ifsd->loading_error = true;
+		dev_err(dev, "Couldn't authenticate all the chunks.Authenticated %d total %d.\n",
+			valid_chunks, total_chunks);
+		return -EIO;
+	}
+	ifsd->valid_chunks = valid_chunks;
+
+	return 0;
+}
+
 static int validate_ifs_metadata(struct device *dev)
 {
 	struct ifs_data *ifsd = ifs_get_data(dev);
@@ -206,7 +312,9 @@ static int scan_chunks_sanity_check(struct device *dev)
 		return ret;
 
 	ifsd->loading_error = false;
-	ifsd->loaded_version = ifs_header_ptr->rev;
+
+	if (ifsd->generation > 0)
+		return copy_hashes_authenticate_chunks_gen2(dev);
 
 	/* copy the scan hash and authenticate per package */
 	cpus_read_lock();
@@ -226,6 +334,7 @@ static int scan_chunks_sanity_check(struct device *dev)
 		ifs_pkg_auth[curr_pkg] = 1;
 	}
 	ret = 0;
+	ifsd->loaded_version = ifs_header_ptr->rev;
 out:
 	cpus_read_unlock();
 
-- 
2.25.1

Re: [PATCH 03/10] platform/x86/intel/ifs: Image loading for new generations

[Ilpo Järvinen]

On Wed, 13 Sep 2023, Jithu Joseph wrote:

> Scan image loading flow for newer IFS generations (1 and 2) are slightly
> different from that of current generation (0). In newer schemes,
> loading need not be done once for each socket as was done in gen0.
> 
> Also the width of CHUNK related bitfields in SCAN_HASHES_STATUS MSR has
> increased from 8 -> 16 bits. Similarly there are width differences
> for CHUNK_AUTHENTICATION_STATUS too.
> 
> Further the parameter to AUTHENTICATE_AND_COPY_CHUNK is passed
> differently in newer generations.
> 
> 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  |  27 ++++++
>  drivers/platform/x86/intel/ifs/load.c | 113 +++++++++++++++++++++++++-
>  2 files changed, 138 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index d666aeed20fc..886dc74de57d 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -137,6 +137,8 @@
>  #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
>  #define MSR_ACTIVATE_SCAN			0x000002c6
>  #define MSR_SCAN_STATUS				0x000002c7
> +#define MSR_SAF_CTRL				0x000004f0
> +
>  #define SCAN_NOT_TESTED				0
>  #define SCAN_TEST_PASS				1
>  #define SCAN_TEST_FAIL				2
> @@ -158,6 +160,19 @@ union ifs_scan_hashes_status {
>  	};
>  };
>  
> +union ifs_scan_hashes_status_gen2 {
> +	u64	data;
> +	struct {
> +		u16	chunk_size;
> +		u16	num_chunks;
> +		u8	error_code;
> +		u32	chunks_in_stride :9;
> +		u32	rsvd		:2;
> +		u32	max_core_limit	:12;
> +		u32	valid		:1;

This doesn't look it would be guaranteed to provide the alignment you seem 
to want for the fields.

> +	};
> +};
> +
>  /* MSR_CHUNKS_AUTH_STATUS bit fields */
>  union ifs_chunks_auth_status {
>  	u64	data;
> @@ -170,6 +185,16 @@ union ifs_chunks_auth_status {
>  	};
>  };
>  
> +union ifs_chunks_auth_status_gen2 {
> +	u64	data;
> +	struct {
> +		u16	valid_chunks;
> +		u16	total_chunks;
> +		u8	error_code;
> +		u32	rsvd		:24;

Ditto.

> +	};
> +};
> +
>  /* MSR_ACTIVATE_SCAN bit fields */
>  union ifs_scan {
>  	u64	data;
> @@ -230,6 +255,7 @@ struct ifs_test_caps {
>   * @scan_details: opaque scan status code from h/w
>   * @cur_batch: number indicating the currently loaded test file
>   * @generation: IFS test generation enumerated by hardware
> + * @chunk_size: size of a test chunk
>   */
>  struct ifs_data {
>  	int	loaded_version;
> @@ -240,6 +266,7 @@ struct ifs_data {
>  	u64	scan_details;
>  	u32	cur_batch;
>  	u32	generation;
> +	u32	chunk_size;
>  };
>  
>  struct ifs_work {
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index 851c97cc6a6b..e8fb03dd8bcf 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -26,6 +26,11 @@ union meta_data {
>  
>  #define IFS_HEADER_SIZE	(sizeof(struct microcode_header_intel))
>  #define META_TYPE_IFS	1
> +#define INVALIDATE_STRIDE (0x1UL)

Unnecessary parenthesis. 

Align.

> +#define IFS_GEN_STRIDE_AWARE	2
> +#define AUTH_INTERRUPTED_ERROR	5
> +#define IFS_AUTH_RETRY_CT	10
> +
>  static  struct microcode_header_intel *ifs_header_ptr;	/* pointer to the ifs image header */
>  static u64 ifs_hash_ptr;			/* Address of ifs metadata (hash) */
>  static u64 ifs_test_image_ptr;			/* 256B aligned address of test pattern */
> @@ -44,7 +49,10 @@ static const char * const scan_hash_status[] = {
>  static const char * const scan_authentication_status[] = {
>  	[0] = "No error reported",
>  	[1] = "Attempt to authenticate a chunk which is already marked as authentic",
> -	[2] = "Chunk authentication error. The hash of chunk did not match expected value"
> +	[2] = "Chunk authentication error. The hash of chunk did not match expected value",
> +	[3] = "Reserved",
> +	[4] = "Chunk outside the current stride",
> +	[5] = "Authentication flow interrupted"

Add the trailing comma to avoid the need to touch the line later if more
entries are added.

>  };
>  
>  #define MC_HEADER_META_TYPE_END		(0)
> @@ -154,6 +162,104 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
>  	complete(&ifs_done);
>  }
>  
> +static int get_num_chunks(int gen, union ifs_scan_hashes_status_gen2 status)
> +{
> +	return  gen >= IFS_GEN_STRIDE_AWARE ? status.chunks_in_stride : status.num_chunks;

Remove extra space.

> +}
> +
> +static bool need_copy_scan_hashes(struct ifs_data *ifsd)
> +{
> +	if (!ifsd->loaded || ifsd->generation < IFS_GEN_STRIDE_AWARE ||
> +	    ifsd->loaded_version != ifs_header_ptr->rev) {
> +		return true;
> +	}
> +	return false;

IMO, this would be easier to read:

	return !ifsd->loaded ||
	       ifsd->generation < IFS_GEN_STRIDE_AWARE ||
	       ifsd->loaded_version != ifs_header_ptr->rev;

> +}
> +
> +static int copy_hashes_authenticate_chunks_gen2(struct device *dev)
> +{
> +	union ifs_scan_hashes_status_gen2 hashes_status;
> +	union ifs_chunks_auth_status_gen2 chunk_status;
> +	u32 err_code, valid_chunks, total_chunks;
> +	int i, num_chunks, chunk_size;
> +	union meta_data *ifs_meta;
> +	int starting_chunk_nr;
> +	struct ifs_data *ifsd;
> +	u64 linear_addr, base;
> +	u64 chunk_table[2];
> +	int retry_count;
> +
> +	ifsd = ifs_get_data(dev);
> +
> +	if (need_copy_scan_hashes(ifsd)) {
> +		wrmsrl(MSR_COPY_SCAN_HASHES, ifs_hash_ptr);
> +		rdmsrl(MSR_SCAN_HASHES_STATUS, hashes_status.data);
> +
> +		/* enumerate the scan image information */
> +		chunk_size = hashes_status.chunk_size * 1024;

SZ_1K ?

> +		err_code = hashes_status.error_code;
> +
> +		num_chunks = get_num_chunks(ifsd->generation, hashes_status);
> +
> +		if (!hashes_status.valid) {
> +			hashcopy_err_message(dev, err_code);
> +			return -EIO;
> +		}
> +		ifsd->loaded_version = ifs_header_ptr->rev;
> +		ifsd->chunk_size = chunk_size;
> +	} else {
> +		num_chunks = ifsd->valid_chunks;
> +		chunk_size = ifsd->chunk_size;
> +	}
> +
> +	if (ifsd->generation >= IFS_GEN_STRIDE_AWARE) {
> +		wrmsrl(MSR_SAF_CTRL, INVALIDATE_STRIDE);
> +		rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
> +		if (chunk_status.valid_chunks != 0) {
> +			dev_err(dev, "Couldn't invalidate installed stride - %d\n",
> +				chunk_status.valid_chunks);
> +			return -EIO;
> +		}
> +	}
> +
> +	base = ifs_test_image_ptr;
> +	ifs_meta = (union meta_data *)find_meta_data(ifs_header_ptr, META_TYPE_IFS);
> +	starting_chunk_nr = ifs_meta->starting_chunk;
> +
> +	/* scan data authentication and copy chunks to secured memory */
> +	for (i = 0; i < num_chunks; i++) {
> +		retry_count = IFS_AUTH_RETRY_CT;
> +		linear_addr = base + i * chunk_size;
> +
> +		chunk_table[0] = starting_chunk_nr + i;
> +		chunk_table[1] = linear_addr;
> +auth_retry:
> +		wrmsrl(MSR_AUTHENTICATE_AND_COPY_CHUNK, (u64)chunk_table);
> +		rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
> +		err_code = chunk_status.error_code;
> +		if (err_code == AUTH_INTERRUPTED_ERROR && --retry_count)
> +			goto auth_retry;

do {

} while ();

> +		if (err_code) {
> +			ifsd->loading_error = true;
> +			auth_err_message(dev, err_code);
> +			return -EIO;
> +		}
> +	}
> +
> +	valid_chunks = chunk_status.valid_chunks;
> +	total_chunks = chunk_status.total_chunks;
> +
> +	if (valid_chunks != total_chunks) {
> +		ifsd->loading_error = true;
> +		dev_err(dev, "Couldn't authenticate all the chunks.Authenticated %d total %d.\n",

Missing whitespace.

> +			valid_chunks, total_chunks);
> +		return -EIO;
> +	}
> +	ifsd->valid_chunks = valid_chunks;
> +
> +	return 0;
> +}
> +
>  static int validate_ifs_metadata(struct device *dev)
>  {
>  	struct ifs_data *ifsd = ifs_get_data(dev);
> @@ -206,7 +312,9 @@ static int scan_chunks_sanity_check(struct device *dev)
>  		return ret;
>  
>  	ifsd->loading_error = false;
> -	ifsd->loaded_version = ifs_header_ptr->rev;
> +
> +	if (ifsd->generation > 0)
> +		return copy_hashes_authenticate_chunks_gen2(dev);
>  
>  	/* copy the scan hash and authenticate per package */
>  	cpus_read_lock();
> @@ -226,6 +334,7 @@ static int scan_chunks_sanity_check(struct device *dev)
>  		ifs_pkg_auth[curr_pkg] = 1;
>  	}
>  	ret = 0;
> +	ifsd->loaded_version = ifs_header_ptr->rev;
>  out:
>  	cpus_read_unlock();
>  
> 

-- 
 i.

Re: [PATCH 03/10] platform/x86/intel/ifs: Image loading for new generations

[Joseph, Jithu]

On 9/15/2023 9:46 AM, Ilpo Järvinen wrote:
> On Wed, 13 Sep 2023, Jithu Joseph wrote:
> 
>> Scan image loading flow for newer IFS generations (1 and 2) are slightly
>> different from that of current generation (0). In newer schemes,
>> loading need not be done once for each socket as was done in gen0.
>>
>> Also the width of CHUNK related bitfields in SCAN_HASHES_STATUS MSR has
>> increased from 8 -> 16 bits. Similarly there are width differences
>> for CHUNK_AUTHENTICATION_STATUS too.
>>
>> Further the parameter to AUTHENTICATE_AND_COPY_CHUNK is passed
>> differently in newer generations.
>>
>> 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  |  27 ++++++
>>  drivers/platform/x86/intel/ifs/load.c | 113 +++++++++++++++++++++++++-
>>  2 files changed, 138 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
>> index d666aeed20fc..886dc74de57d 100644
>> --- a/drivers/platform/x86/intel/ifs/ifs.h
>> +++ b/drivers/platform/x86/intel/ifs/ifs.h
>> @@ -137,6 +137,8 @@
>>  #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
>>  #define MSR_ACTIVATE_SCAN			0x000002c6
>>  #define MSR_SCAN_STATUS				0x000002c7
>> +#define MSR_SAF_CTRL				0x000004f0
>> +
>>  #define SCAN_NOT_TESTED				0
>>  #define SCAN_TEST_PASS				1
>>  #define SCAN_TEST_FAIL				2
>> @@ -158,6 +160,19 @@ union ifs_scan_hashes_status {
>>  	};
>>  };
>>  
>> +union ifs_scan_hashes_status_gen2 {
>> +	u64	data;
>> +	struct {
>> +		u16	chunk_size;
>> +		u16	num_chunks;
>> +		u8	error_code;
>> +		u32	chunks_in_stride :9;
>> +		u32	rsvd		:2;
>> +		u32	max_core_limit	:12;
>> +		u32	valid		:1;
> 
> This doesn't look it would be guaranteed to provide the alignment you seem 
> to want for the fields.

To Quote Tony from an earlier response to a similar query[1]

"This driver is X86_64 specific (and it seems
incredibly unlikely that some other architecture will copy this h/w
interface so closely that they want to re-use this driver. There's an x86_64
ABI that says how bitfields in C are allocated."



[1] https://lore.kernel.org/lkml/SJ1PR11MB6083EBD2D2826E0A247AF242FCD19@SJ1PR11MB6083.namprd11.prod.outlook.com/

Agree to the rest of your comments ... will revise them as per your suggestion.

Jithu

Re: [PATCH 03/10] platform/x86/intel/ifs: Image loading for new generations

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3214 bytes --]

On Fri, 15 Sep 2023, Joseph, Jithu wrote:
> On 9/15/2023 9:46 AM, Ilpo Järvinen wrote:
> > On Wed, 13 Sep 2023, Jithu Joseph wrote:
> > 
> >> Scan image loading flow for newer IFS generations (1 and 2) are slightly
> >> different from that of current generation (0). In newer schemes,
> >> loading need not be done once for each socket as was done in gen0.
> >>
> >> Also the width of CHUNK related bitfields in SCAN_HASHES_STATUS MSR has
> >> increased from 8 -> 16 bits. Similarly there are width differences
> >> for CHUNK_AUTHENTICATION_STATUS too.
> >>
> >> Further the parameter to AUTHENTICATE_AND_COPY_CHUNK is passed
> >> differently in newer generations.
> >>
> >> 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  |  27 ++++++
> >>  drivers/platform/x86/intel/ifs/load.c | 113 +++++++++++++++++++++++++-
> >>  2 files changed, 138 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> >> index d666aeed20fc..886dc74de57d 100644
> >> --- a/drivers/platform/x86/intel/ifs/ifs.h
> >> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> >> @@ -137,6 +137,8 @@
> >>  #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
> >>  #define MSR_ACTIVATE_SCAN			0x000002c6
> >>  #define MSR_SCAN_STATUS				0x000002c7
> >> +#define MSR_SAF_CTRL				0x000004f0
> >> +
> >>  #define SCAN_NOT_TESTED				0
> >>  #define SCAN_TEST_PASS				1
> >>  #define SCAN_TEST_FAIL				2
> >> @@ -158,6 +160,19 @@ union ifs_scan_hashes_status {
> >>  	};
> >>  };
> >>  
> >> +union ifs_scan_hashes_status_gen2 {
> >> +	u64	data;
> >> +	struct {
> >> +		u16	chunk_size;
> >> +		u16	num_chunks;
> >> +		u8	error_code;
> >> +		u32	chunks_in_stride :9;
> >> +		u32	rsvd		:2;
> >> +		u32	max_core_limit	:12;
> >> +		u32	valid		:1;
> > 
> > This doesn't look it would be guaranteed to provide the alignment you seem 
> > to want for the fields.
> 
> To Quote Tony from an earlier response to a similar query[1]
> 
> "This driver is X86_64 specific (and it seems
> incredibly unlikely that some other architecture will copy this h/w
> interface so closely that they want to re-use this driver. There's an x86_64
> ABI that says how bitfields in C are allocated."
> 
> 
> 
> [1] https://lore.kernel.org/lkml/SJ1PR11MB6083EBD2D2826E0A247AF242FCD19@SJ1PR11MB6083.namprd11.prod.outlook.com/

Hi,

I was actually not that worried about this from portability perspective
but from placing u32 bitfield after u8 which according to some info I read 
about this topic way back would not get the alignment you're after. As I 
could not find anything concrete which "says" (does somebody have some 
reference for something which actually documents this?) something about 
x86_64 I ended up using pahole and checked that gcc did not leave hole 
there so it seems to be fine after all.

I think Tony's "proof" is pretty invalid. He doesn't differentiate
HW interface related bitfields from those which are not HW interface 
related (to the extent that in fact most of those bitfields likely are not 
HW interface related).

-- 
 i.

RE: [PATCH 03/10] platform/x86/intel/ifs: Image loading for new generations

[Luck, Tony]

> I think Tony's "proof" is pretty invalid. He doesn't differentiate
> HW interface related bitfields from those which are not HW interface 
> related (to the extent that in fact most of those bitfields likely are not 
> HW interface related).

When I made that comment it was about a patch series that used
bitfields to decode the subfields in Intel model specific MSRs.  I
think that's true of use in this series too.

I think most of these are for MSR decode. The one mentioned in
this thread: "union ifs_scan_hashes_status_gen2 {" definitely is.

Are there any that are not for MSRs? I'd also claim "Intel
specific" if there are some decoding parts of the Intel scan
file format.

-Tony

RE: [PATCH 03/10] platform/x86/intel/ifs: Image loading for new generations

[Ilpo Järvinen]

On Mon, 18 Sep 2023, Luck, Tony wrote:

> > I think Tony's "proof" is pretty invalid. He doesn't differentiate
> > HW interface related bitfields from those which are not HW interface 
> > related (to the extent that in fact most of those bitfields likely are not 
> > HW interface related).
> 
> When I made that comment it was about a patch series that used
> bitfields to decode the subfields in Intel model specific MSRs.  I
> think that's true of use in this series too.

But your grep in [1] was not limited to such cases nor to HW interface 
related ones in general.

What I meant with your proof being invalid is that the argument against
bitfields have been related to using them with HW interfaces, not just 
generic use of the bitfields (even if there have been some performance 
issues in that area as well). Simply grepping through include/ directly is 
not going to tell anything if the bitfield in question is related to HW 
interfaces or not.

> I think most of these are for MSR decode. The one mentioned in
> this thread: "union ifs_scan_hashes_status_gen2 {" definitely is.
> 
> Are there any that are not for MSRs? I'd also claim "Intel
> specific" if there are some decoding parts of the Intel scan
> file format.

First of all, I already checked myself that the alignment is not 
incorrect so I don't find it as problematic as I thought it was (I did 
not even flag all bitfield addition in the patches, just the cases were u8 
was followed by u32 bitfield which I thought is not going to work because 
of something I read about this topic some time ago claimed if the type 
changes the bitfield does not carry over).

Since you replied, would you happen to have a pointer something that tells 
(in writing) how the bitfields in C are allocated in case of x86_64? I 
spent a bit of time trying to find something but came up nothing.


[1] https://lore.kernel.org/lkml/SJ1PR11MB6083EBD2D2826E0A247AF242FCD19@SJ1PR11MB6083.namprd11.prod.outlook.com/

-- 
 i.

RE: [PATCH 03/10] platform/x86/intel/ifs: Image loading for new generations

[Luck, Tony]

> Since you replied, would you happen to have a pointer something that tells 
> (in writing) how the bitfields in C are allocated in case of x86_64? I 
> spent a bit of time trying to find something but came up nothing.

Search engines don't seem to be as good as they used to be (or I'm not as
good at finding the right query). 

There's a bit on page 14 of:

https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf

that says bit fields are allocated right to left (which is a good
start). But I thought there was a doc somewhere that gave
more detail about alignment of bitfields.

-Tony

RE: [PATCH 03/10] platform/x86/intel/ifs: Image loading for new generations

[Ilpo Järvinen]

On Mon, 18 Sep 2023, Luck, Tony wrote:

> > Since you replied, would you happen to have a pointer something that tells 
> > (in writing) how the bitfields in C are allocated in case of x86_64? I 
> > spent a bit of time trying to find something but came up nothing.
> 
> Search engines don't seem to be as good as they used to be (or I'm not as
> good at finding the right query). 
> 
> There's a bit on page 14 of:
> 
> https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf
> 
> that says bit fields are allocated right to left (which is a good
> start). But I thought there was a doc somewhere that gave
> more detail about alignment of bitfields.

Thanks, appreciated.

In this case it is not just about the bitfield itself nor the bit 
allocation order but sharing the storage unit with another member, and to 
further complicate things, members have different alignment requirement 
too (32-bit aligned u8 followed by u32 bitfield).

The document states: "Bit-fields obey the same size and alignment rules as 
other structure and union members." which seems to contradict my test 
that found that the u32 bitfield won't be 32-bit aligned but gets combined 
with the 32-bit aligned u8. Perhaps it's because the total number of bits 
still fits to 32 bits so the bitfield doesn't cross the 32-bit boundary 
even when combined with the preceeding u8.


-- 
 i.

Re: [PATCH 03/10] platform/x86/intel/ifs: Image loading for new generations

[Joseph, Jithu]

On 9/18/2023 9:29 AM, Ilpo Järvinen wrote:

> 
> In this case it is not just about the bitfield itself nor the bit 
> allocation order but sharing the storage unit with another member, and to 
> further complicate things, members have different alignment requirement 
> too (32-bit aligned u8 followed by u32 bitfield).
> 

I too verified that the size of the whole structure matches that of MSR  64 bits (8 bytes). 

Initially when IFS scan was added the  all MSR structure members were  bit-fields, later there was a suggestion to
use basic C types if applicable during subsequent Array BIST patch series. I followed this approach with the  current patch series . 

I will change the current series to use all bit-field MSR structures in v2, given mixing basic types and bitfields is a a source of confusion

Jithu

Re: [PATCH 03/10] platform/x86/intel/ifs: Image loading for new generations

[Dave Hansen]

On 9/18/23 09:51, Joseph, Jithu wrote:
> 
> On 9/18/2023 9:29 AM, Ilpo Järvinen wrote:
> 
>> In this case it is not just about the bitfield itself nor the bit 
>> allocation order but sharing the storage unit with another member, and to 
>> further complicate things, members have different alignment requirement 
>> too (32-bit aligned u8 followed by u32 bitfield).
>>
> I too verified that the size of the whole structure matches that of MSR  64 bits (8 bytes). 
> 
> Initially when IFS scan was added the  all MSR structure members were  bit-fields, later there was a suggestion to
> use basic C types if applicable during subsequent Array BIST patch series. I followed this approach with the  current patch series . 
> 
> I will change the current series to use all bit-field MSR structures in v2, given mixing basic types and bitfields is a a source of confusion

That's the wrong direction. :)

What is more obviously correct.  This:

	struct {
		u16	valid_chunks;
		u16	total_chunks;
		u8	error_code;
		u8	rsvd1;
		u8	rsvd2;
		u8	rsvd3;
	};

or this:

	struct {
		u16	valid_chunks;
		u16	total_chunks;
		u8	error_code;
		u32	error_code	:8;
		u32	rsvd		:24;
	};

?

Re: [PATCH 03/10] platform/x86/intel/ifs: Image loading for new generations

[Joseph, Jithu]

On 9/18/2023 9:58 AM, Dave Hansen wrote:
> On 9/18/23 09:51, Joseph, Jithu wrote:
>>
>> On 9/18/2023 9:29 AM, Ilpo Järvinen wrote:
>>
>>> In this case it is not just about the bitfield itself nor the bit 
>>> allocation order but sharing the storage unit with another member, and to 
>>> further complicate things, members have different alignment requirement 
>>> too (32-bit aligned u8 followed by u32 bitfield).
>>>
>> I too verified that the size of the whole structure matches that of MSR  64 bits (8 bytes). 
>>
>> Initially when IFS scan was added the  all MSR structure members were  bit-fields, later there was a suggestion to
>> use basic C types if applicable during subsequent Array BIST patch series. I followed this approach with the  current patch series . 
>>
>> I will change the current series to use all bit-field MSR structures in v2, given mixing basic types and bitfields is a a source of confusion
> 
> That's the wrong direction. :)
> 
> What is more obviously correct.  This:
> 
> 	struct {
> 		u16	valid_chunks;
> 		u16	total_chunks;
> 		u8	error_code;
> 		u8	rsvd1;
> 		u8	rsvd2;
> 		u8	rsvd3;
> 	};
> 
> or this:
> 
> 	struct {
> 		u16	valid_chunks;
> 		u16	total_chunks;
> 		u32	error_code	:8;
> 		u32	rsvd		:24;
> 	};

I will go with the second pattern above, given that pattern can be followed for other MSR structures too, where fields doesn't split as evenly

Jithu

[PATCH 04/10] platform/x86/intel/ifs: Scan test for new generations

[Jithu Joseph]

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;
 };
 
 /* 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;
+	} gen2;
 };
 
 /* MSR_ARRAY_BIST bit fields */
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 1061eb7ec399..4bbab6be2fa2 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -171,6 +171,8 @@ static void ifs_test_core(int cpu, struct device *dev)
 	union ifs_status status;
 	unsigned long timeout;
 	struct ifs_data *ifsd;
+	int to_start, to_stop;
+	int status_chunk;
 	u64 msrvals[2];
 	int retries;
 
@@ -179,13 +181,21 @@ static void ifs_test_core(int cpu, struct device *dev)
 	activate.rsvd = 0;
 	activate.delay = IFS_THREAD_WAIT;
 	activate.sigmce = 0;
-	activate.start = 0;
-	activate.stop = ifsd->valid_chunks - 1;
+	to_start = 0;
+	to_stop = ifsd->valid_chunks - 1;
+
+	if (ifsd->generation) {
+		activate.gen2.start = to_start;
+		activate.gen2.stop = to_stop;
+	} else {
+		activate.start = to_start;
+		activate.stop = to_stop;
+	}
 
 	timeout = jiffies + HZ / 2;
 	retries = MAX_IFS_RETRIES;
 
-	while (activate.start <= activate.stop) {
+	while (to_start <= to_stop) {
 		if (time_after(jiffies, timeout)) {
 			status.error_code = IFS_SW_TIMEOUT;
 			break;
@@ -202,7 +212,8 @@ static void ifs_test_core(int cpu, struct device *dev)
 		if (!can_restart(status))
 			break;
 
-		if (status.chunk_num == activate.start) {
+		status_chunk = ifsd->generation ? status.gen2.chunk_num : status.chunk_num;
+		if (status_chunk == to_start) {
 			/* Check for forward progress */
 			if (--retries == 0) {
 				if (status.error_code == IFS_NO_ERROR)
@@ -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);
+			to_start = status_chunk;
 		}
 	}
 
-- 
2.25.1

Re: [PATCH 04/10] platform/x86/intel/ifs: Scan test for new generations

[Ilpo Järvinen]

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.

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

> +	} gen2;
>  };
>  
>  /* MSR_ARRAY_BIST bit fields */
> diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
> index 1061eb7ec399..4bbab6be2fa2 100644
> --- a/drivers/platform/x86/intel/ifs/runtest.c
> +++ b/drivers/platform/x86/intel/ifs/runtest.c
> @@ -171,6 +171,8 @@ static void ifs_test_core(int cpu, struct device *dev)
>  	union ifs_status status;
>  	unsigned long timeout;
>  	struct ifs_data *ifsd;
> +	int to_start, to_stop;
> +	int status_chunk;
>  	u64 msrvals[2];
>  	int retries;
>  
> @@ -179,13 +181,21 @@ static void ifs_test_core(int cpu, struct device *dev)
>  	activate.rsvd = 0;
>  	activate.delay = IFS_THREAD_WAIT;
>  	activate.sigmce = 0;
> -	activate.start = 0;
> -	activate.stop = ifsd->valid_chunks - 1;
> +	to_start = 0;
> +	to_stop = ifsd->valid_chunks - 1;
> +
> +	if (ifsd->generation) {
> +		activate.gen2.start = to_start;
> +		activate.gen2.stop = to_stop;
> +	} else {
> +		activate.start = to_start;
> +		activate.stop = to_stop;
> +	}
>  
>  	timeout = jiffies + HZ / 2;
>  	retries = MAX_IFS_RETRIES;
>  
> -	while (activate.start <= activate.stop) {
> +	while (to_start <= to_stop) {
>  		if (time_after(jiffies, timeout)) {
>  			status.error_code = IFS_SW_TIMEOUT;
>  			break;
> @@ -202,7 +212,8 @@ static void ifs_test_core(int cpu, struct device *dev)
>  		if (!can_restart(status))
>  			break;
>  
> -		if (status.chunk_num == activate.start) {
> +		status_chunk = ifsd->generation ? status.gen2.chunk_num : status.chunk_num;
> +		if (status_chunk == to_start) {
>  			/* Check for forward progress */
>  			if (--retries == 0) {
>  				if (status.error_code == IFS_NO_ERROR)
> @@ -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.

> +			to_start = status_chunk;
>  		}
>  	}
>  
> 

-- 
 i.

Re: [PATCH 04/10] platform/x86/intel/ifs: Scan test for new generations

[Joseph, Jithu]

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

Re: [PATCH 04/10] platform/x86/intel/ifs: Scan test for new generations

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2474 bytes --]

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):

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.

-- 
 i.

Re: [PATCH 04/10] platform/x86/intel/ifs: Scan test for new generations

[Joseph, Jithu]

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

[PATCH 05/10] trace: platform/x86/intel/ifs: Modify scan trace

[Jithu Joseph]

Account for the 8/16 bit MSR bitfield width differences between
gen0 and newer generations in the scan trace path.

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>
---
 include/trace/events/intel_ifs.h         | 16 ++++++++--------
 drivers/platform/x86/intel/ifs/runtest.c |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/trace/events/intel_ifs.h b/include/trace/events/intel_ifs.h
index d7353024016c..af0af3f1d9b7 100644
--- a/include/trace/events/intel_ifs.h
+++ b/include/trace/events/intel_ifs.h
@@ -10,25 +10,25 @@
 
 TRACE_EVENT(ifs_status,
 
-	TP_PROTO(int cpu, union ifs_scan activate, union ifs_status status),
+	TP_PROTO(int cpu, int start, int stop, u64 status),
 
-	TP_ARGS(cpu, activate, status),
+	TP_ARGS(cpu, start, stop, status),
 
 	TP_STRUCT__entry(
 		__field(	u64,	status	)
 		__field(	int,	cpu	)
-		__field(	u8,	start	)
-		__field(	u8,	stop	)
+		__field(	u16,	start	)
+		__field(	u16,	stop	)
 	),
 
 	TP_fast_assign(
 		__entry->cpu	= cpu;
-		__entry->start	= activate.start;
-		__entry->stop	= activate.stop;
-		__entry->status	= status.data;
+		__entry->start	= start;
+		__entry->stop	= stop;
+		__entry->status	= status;
 	),
 
-	TP_printk("cpu: %d, start: %.2x, stop: %.2x, status: %llx",
+	TP_printk("cpu: %d, start: %.4x, stop: %.4x, status: %.16llx",
 		__entry->cpu,
 		__entry->start,
 		__entry->stop,
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 4bbab6be2fa2..ba3f9ad88a82 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -206,7 +206,7 @@ static void ifs_test_core(int cpu, struct device *dev)
 
 		status.data = msrvals[1];
 
-		trace_ifs_status(cpu, activate, status);
+		trace_ifs_status(cpu, to_start, to_stop, status.data);
 
 		/* Some cases can be retried, give up for others */
 		if (!can_restart(status))
-- 
2.25.1

[PATCH 06/10] platform/x86/intel/ifs: Validate image size

[Jithu Joseph]

Perform additional validation prior to loading IFS image.

Error out if the size of the file being loaded doesn't
match the size specified in the header.

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/load.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index e8fb03dd8bcf..778a3b89a24d 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -376,6 +376,7 @@ int ifs_load_firmware(struct device *dev)
 {
 	const struct ifs_test_caps *test = ifs_get_test_caps(dev);
 	struct ifs_data *ifsd = ifs_get_data(dev);
+	unsigned int expected_size;
 	const struct firmware *fw;
 	char scan_path[64];
 	int ret = -EINVAL;
@@ -390,6 +391,13 @@ int ifs_load_firmware(struct device *dev)
 		goto done;
 	}
 
+	expected_size = ((struct microcode_header_intel *)fw->data)->totalsize;
+	if (fw->size != expected_size) {
+		dev_err(dev, "File size mismatch (expected %d, actual %ld). Corrupted IFS image.\n",
+			expected_size, fw->size);
+		return -EBADFD;
+	}
+
 	ret = image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
 	if (ret)
 		goto release;
-- 
2.25.1

Re: [PATCH 06/10] platform/x86/intel/ifs: Validate image size

[Ilpo Järvinen]

On Wed, 13 Sep 2023, Jithu Joseph wrote:

> Perform additional validation prior to loading IFS image.
> 
> Error out if the size of the file being loaded doesn't
> match the size specified in the header.

Please fix these short lines in all your patches.
 
> 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/load.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index e8fb03dd8bcf..778a3b89a24d 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -376,6 +376,7 @@ int ifs_load_firmware(struct device *dev)
>  {
>  	const struct ifs_test_caps *test = ifs_get_test_caps(dev);
>  	struct ifs_data *ifsd = ifs_get_data(dev);
> +	unsigned int expected_size;
>  	const struct firmware *fw;
>  	char scan_path[64];
>  	int ret = -EINVAL;
> @@ -390,6 +391,13 @@ int ifs_load_firmware(struct device *dev)
>  		goto done;
>  	}
>  
> +	expected_size = ((struct microcode_header_intel *)fw->data)->totalsize;
> +	if (fw->size != expected_size) {
> +		dev_err(dev, "File size mismatch (expected %d, actual %ld). Corrupted IFS image.\n",
> +			expected_size, fw->size);
> +		return -EBADFD;
> +	}
> +
>  	ret = image_sanity_check(dev, (struct microcode_header_intel *)fw->data);

It looks than a bit odd to add the check here and not into a function 
called image_sanity_check()?!?

>  	if (ret)
>  		goto release;
> 

-- 
 i.

Re: [PATCH 06/10] platform/x86/intel/ifs: Validate image size

[Joseph, Jithu]

On 9/15/2023 9:57 AM, Ilpo Järvinen wrote:
> On Wed, 13 Sep 2023, Jithu Joseph wrote:
> 
>> Perform additional validation prior to loading IFS image.
>>
>> Error out if the size of the file being loaded doesn't
>> match the size specified in the header.
> 
> Please fix these short lines in all your patches.

Will do

>  
>> 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/load.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
>> index e8fb03dd8bcf..778a3b89a24d 100644
>> --- a/drivers/platform/x86/intel/ifs/load.c
>> +++ b/drivers/platform/x86/intel/ifs/load.c
>> @@ -376,6 +376,7 @@ int ifs_load_firmware(struct device *dev)
>>  {
>>  	const struct ifs_test_caps *test = ifs_get_test_caps(dev);
>>  	struct ifs_data *ifsd = ifs_get_data(dev);
>> +	unsigned int expected_size;
>>  	const struct firmware *fw;
>>  	char scan_path[64];
>>  	int ret = -EINVAL;
>> @@ -390,6 +391,13 @@ int ifs_load_firmware(struct device *dev)
>>  		goto done;
>>  	}
>>  
>> +	expected_size = ((struct microcode_header_intel *)fw->data)->totalsize;
>> +	if (fw->size != expected_size) {
>> +		dev_err(dev, "File size mismatch (expected %d, actual %ld). Corrupted IFS image.\n",
>> +			expected_size, fw->size);
>> +		return -EBADFD;
>> +	}
>> +
>>  	ret = image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
> 
> It looks than a bit odd to add the check here and not into a function 
> called image_sanity_check()?!?

image_sanity_check() validates the contents of the image, whereas the new check
in some sense validates request_firmware_direct() results. Hence it was placed
outside of content validation / closer to request_firmware_direct()


Jithu

[PATCH 07/10] platform/x86/intel/ifs: Metadata validation for start_chunk

[Jithu Joseph]

Add an additional check to validate IFS image metadata field
prior to loading the test image.

If start_chunk is not a multiple of chunks_per_stride error out.

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/load.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 778a3b89a24d..88630366a23c 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -292,6 +292,13 @@ static int validate_ifs_metadata(struct device *dev)
 		return ret;
 	}
 
+	if (ifs_meta->chunks_per_stride &&
+	    (ifs_meta->starting_chunk % ifs_meta->chunks_per_stride)) {
+		dev_warn(dev, "Starting chunk num %d not a multiple of chunks_per_stride %d\n",
+			 ifs_meta->starting_chunk, ifs_meta->chunks_per_stride);
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH 07/10] platform/x86/intel/ifs: Metadata validation for start_chunk

[Ilpo Järvinen]

On Wed, 13 Sep 2023, Jithu Joseph wrote:

> Add an additional check to validate IFS image metadata field
> prior to loading the test image.
> 
> If start_chunk is not a multiple of chunks_per_stride error out.
> 
> 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/load.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index 778a3b89a24d..88630366a23c 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -292,6 +292,13 @@ static int validate_ifs_metadata(struct device *dev)
>  		return ret;
>  	}
>  
> +	if (ifs_meta->chunks_per_stride &&
> +	    (ifs_meta->starting_chunk % ifs_meta->chunks_per_stride)) {

While this is not incorrect, it would make the code easier to understand 
if you add != 0.

> +		dev_warn(dev, "Starting chunk num %d not a multiple of chunks_per_stride %d\n",
> +			 ifs_meta->starting_chunk, ifs_meta->chunks_per_stride);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> 

-- 
 i.

Re: [PATCH 07/10] platform/x86/intel/ifs: Metadata validation for start_chunk

[Joseph, Jithu]

On 9/15/2023 9:59 AM, Ilpo Järvinen wrote:
> On Wed, 13 Sep 2023, Jithu Joseph wrote:
> 
>> Add an additional check to validate IFS image metadata field
>> prior to loading the test image.
>>
>> If start_chunk is not a multiple of chunks_per_stride error out.
>>
>> 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/load.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
>> index 778a3b89a24d..88630366a23c 100644
>> --- a/drivers/platform/x86/intel/ifs/load.c
>> +++ b/drivers/platform/x86/intel/ifs/load.c
>> @@ -292,6 +292,13 @@ static int validate_ifs_metadata(struct device *dev)
>>  		return ret;
>>  	}
>>  
>> +	if (ifs_meta->chunks_per_stride &&
>> +	    (ifs_meta->starting_chunk % ifs_meta->chunks_per_stride)) {
> 
> While this is not incorrect, it would make the code easier to understand 
> if you add != 0.

Agree ... Will change

Jithu

[PATCH 08/10] platform/x86/intel/ifs: Add new CPU support

[Jithu Joseph]

Add Granite Rapids and Sierra Forest cpuids to x86 match
table so that IFS driver can be loaded for those.

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/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 88d84aad9334..0938bfbd9086 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -17,6 +17,9 @@
 static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
 	X86_MATCH(SAPPHIRERAPIDS_X),
 	X86_MATCH(EMERALDRAPIDS_X),
+	X86_MATCH(GRANITERAPIDS_X),
+	X86_MATCH(GRANITERAPIDS_D),
+	X86_MATCH(ATOM_CRESTMONT_X),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
-- 
2.25.1

[PATCH 09/10] platform/x86/intel/ifs: Add new error code

[Jithu Joseph]

Make driver aware of a newly added error code so that it can
provide a more appropriate error message.

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/runtest.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index ba3f9ad88a82..997d2f07aa0c 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -40,6 +40,8 @@ enum ifs_status_err_code {
 	IFS_UNASSIGNED_ERROR_CODE		= 7,
 	IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT	= 8,
 	IFS_INTERRUPTED_DURING_EXECUTION	= 9,
+	IFS_UNASSIGNED_ERROR_CODE_0xA		= 0xA,
+	IFS_CORRUPTED_CHUNK		= 0xB,
 };
 
 static const char * const scan_test_status[] = {
@@ -55,6 +57,8 @@ static const char * const scan_test_status[] = {
 	[IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT] =
 	"Exceeded number of Logical Processors (LP) allowed to run Scan-At-Field concurrently",
 	[IFS_INTERRUPTED_DURING_EXECUTION] = "Interrupt occurred prior to SCAN start",
+	[IFS_UNASSIGNED_ERROR_CODE_0xA] = "Unassigned error code 0xA",
+	[IFS_CORRUPTED_CHUNK] = "Scan operation aborted due to corrupted image. Try reloading",
 };
 
 static void message_not_tested(struct device *dev, int cpu, union ifs_status status)
@@ -123,6 +127,8 @@ static bool can_restart(union ifs_status status)
 	case IFS_MISMATCH_ARGUMENTS_BETWEEN_THREADS:
 	case IFS_CORE_NOT_CAPABLE_CURRENTLY:
 	case IFS_UNASSIGNED_ERROR_CODE:
+	case IFS_UNASSIGNED_ERROR_CODE_0xA:
+	case IFS_CORRUPTED_CHUNK:
 		break;
 	}
 	return false;
-- 
2.25.1

[PATCH 10/10] platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

[Jithu Joseph]

Array BIST MSR addresses, bit definition and semantics
are different for Sierra Forest. Branch into a separate
Array BIST flow on Sierra Forest when user invokes Array Test.

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     |  4 +++
 drivers/platform/x86/intel/ifs/core.c    | 15 +++++-----
 drivers/platform/x86/intel/ifs/runtest.c | 37 +++++++++++++++++++++++-
 3 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 3265a6d8a6f3..2f20588a71f1 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -137,6 +137,8 @@
 #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
 #define MSR_ACTIVATE_SCAN			0x000002c6
 #define MSR_SCAN_STATUS				0x000002c7
+#define MSR_ARRAY_TRIGGER			0x000002d6
+#define MSR_ARRAY_STATUS			0x000002d7
 #define MSR_SAF_CTRL				0x000004f0
 
 #define SCAN_NOT_TESTED				0
@@ -270,6 +272,7 @@ struct ifs_test_caps {
  * @cur_batch: number indicating the currently loaded test file
  * @generation: IFS test generation enumerated by hardware
  * @chunk_size: size of a test chunk
+ * @array_gen: test generation of array test
  */
 struct ifs_data {
 	int	loaded_version;
@@ -281,6 +284,7 @@ struct ifs_data {
 	u32	cur_batch;
 	u32	generation;
 	u32	chunk_size;
+	u32	array_gen;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 0938bfbd9086..e8b570930c16 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -10,16 +10,16 @@
 
 #include "ifs.h"
 
-#define X86_MATCH(model)				\
+#define X86_MATCH(model, array_gen)				\
 	X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6,	\
-		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL)
+		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, array_gen)
 
 static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
-	X86_MATCH(SAPPHIRERAPIDS_X),
-	X86_MATCH(EMERALDRAPIDS_X),
-	X86_MATCH(GRANITERAPIDS_X),
-	X86_MATCH(GRANITERAPIDS_D),
-	X86_MATCH(ATOM_CRESTMONT_X),
+	X86_MATCH(SAPPHIRERAPIDS_X, 0),
+	X86_MATCH(EMERALDRAPIDS_X, 0),
+	X86_MATCH(GRANITERAPIDS_X, 0),
+	X86_MATCH(GRANITERAPIDS_D, 0),
+	X86_MATCH(ATOM_CRESTMONT_X, 1),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
@@ -99,6 +99,7 @@ static int __init ifs_init(void)
 			continue;
 		ifs_devices[i].rw_data.generation = (msrval & MSR_INTEGRITY_CAPS_SAF_GEN_REV_MASK)
 							>> MSR_INTEGRITY_CAPS_SAF_GEN_REV_SHIFT;
+		ifs_devices[i].rw_data.array_gen = (u32)m->driver_data;
 		ret = misc_register(&ifs_devices[i].misc);
 		if (ret)
 			goto err_exit;
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 997d2f07aa0c..9cfd5c015cb2 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -327,6 +327,38 @@ static void ifs_array_test_core(int cpu, struct device *dev)
 		ifsd->status = SCAN_TEST_PASS;
 }
 
+#define ARRAY_GEN1_TEST_ALL_ARRAYS	(0x0ULL)
+#define ARRAY_GEN1_STATUS_FAIL		(0x1ULL)
+
+static int do_array_test_gen1(void *status)
+{
+	int cpu = smp_processor_id();
+	int first;
+
+	first = cpumask_first(cpu_smt_mask(cpu));
+
+	if (cpu == first) {
+		wrmsrl(MSR_ARRAY_TRIGGER, ARRAY_GEN1_TEST_ALL_ARRAYS);
+		rdmsrl(MSR_ARRAY_STATUS, *((u64 *)status));
+	}
+
+	return 0;
+}
+
+static void ifs_array_test_gen1(int cpu, struct device *dev)
+{
+	struct ifs_data *ifsd = ifs_get_data(dev);
+	u64	status = 0;
+
+	stop_core_cpuslocked(cpu, do_array_test_gen1, &status);
+	ifsd->scan_details = status;
+
+	if (status & ARRAY_GEN1_STATUS_FAIL)
+		ifsd->status = SCAN_TEST_FAIL;
+	else
+		ifsd->status = SCAN_TEST_PASS;
+}
+
 /*
  * Initiate per core test. It wakes up work queue threads on the target cpu and
  * its sibling cpu. Once all sibling threads wake up, the scan test gets executed and
@@ -354,7 +386,10 @@ int do_core_test(int cpu, struct device *dev)
 		ifs_test_core(cpu, dev);
 		break;
 	case IFS_TYPE_ARRAY_BIST:
-		ifs_array_test_core(cpu, dev);
+		if (ifsd->array_gen == 0)
+			ifs_array_test_core(cpu, dev);
+		else
+			ifs_array_test_gen1(cpu, dev);
 		break;
 	default:
 		return -EINVAL;
-- 
2.25.1

Re: [PATCH 10/10] platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

[Ilpo Järvinen]

On Wed, 13 Sep 2023, Jithu Joseph wrote:

> Array BIST MSR addresses, bit definition and semantics
> are different for Sierra Forest. Branch into a separate
> Array BIST flow on Sierra Forest when user invokes Array Test.
> 
> 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     |  4 +++
>  drivers/platform/x86/intel/ifs/core.c    | 15 +++++-----
>  drivers/platform/x86/intel/ifs/runtest.c | 37 +++++++++++++++++++++++-
>  3 files changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 3265a6d8a6f3..2f20588a71f1 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -137,6 +137,8 @@
>  #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
>  #define MSR_ACTIVATE_SCAN			0x000002c6
>  #define MSR_SCAN_STATUS				0x000002c7
> +#define MSR_ARRAY_TRIGGER			0x000002d6
> +#define MSR_ARRAY_STATUS			0x000002d7
>  #define MSR_SAF_CTRL				0x000004f0
>  
>  #define SCAN_NOT_TESTED				0
> @@ -270,6 +272,7 @@ struct ifs_test_caps {
>   * @cur_batch: number indicating the currently loaded test file
>   * @generation: IFS test generation enumerated by hardware
>   * @chunk_size: size of a test chunk
> + * @array_gen: test generation of array test
>   */
>  struct ifs_data {
>  	int	loaded_version;
> @@ -281,6 +284,7 @@ struct ifs_data {
>  	u32	cur_batch;
>  	u32	generation;
>  	u32	chunk_size;
> +	u32	array_gen;
>  };
>  
>  struct ifs_work {
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 0938bfbd9086..e8b570930c16 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -10,16 +10,16 @@
>  
>  #include "ifs.h"
>  
> -#define X86_MATCH(model)				\
> +#define X86_MATCH(model, array_gen)				\
>  	X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6,	\
> -		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL)
> +		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, array_gen)
>  
>  static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
> -	X86_MATCH(SAPPHIRERAPIDS_X),
> -	X86_MATCH(EMERALDRAPIDS_X),
> -	X86_MATCH(GRANITERAPIDS_X),
> -	X86_MATCH(GRANITERAPIDS_D),
> -	X86_MATCH(ATOM_CRESTMONT_X),
> +	X86_MATCH(SAPPHIRERAPIDS_X, 0),
> +	X86_MATCH(EMERALDRAPIDS_X, 0),
> +	X86_MATCH(GRANITERAPIDS_X, 0),
> +	X86_MATCH(GRANITERAPIDS_D, 0),
> +	X86_MATCH(ATOM_CRESTMONT_X, 1),
>  	{}
>  };
>  MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
> @@ -99,6 +99,7 @@ static int __init ifs_init(void)
>  			continue;
>  		ifs_devices[i].rw_data.generation = (msrval & MSR_INTEGRITY_CAPS_SAF_GEN_REV_MASK)
>  							>> MSR_INTEGRITY_CAPS_SAF_GEN_REV_SHIFT;
> +		ifs_devices[i].rw_data.array_gen = (u32)m->driver_data;
>  		ret = misc_register(&ifs_devices[i].misc);
>  		if (ret)
>  			goto err_exit;
> diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
> index 997d2f07aa0c..9cfd5c015cb2 100644
> --- a/drivers/platform/x86/intel/ifs/runtest.c
> +++ b/drivers/platform/x86/intel/ifs/runtest.c
> @@ -327,6 +327,38 @@ static void ifs_array_test_core(int cpu, struct device *dev)
>  		ifsd->status = SCAN_TEST_PASS;
>  }
>  
> +#define ARRAY_GEN1_TEST_ALL_ARRAYS	(0x0ULL)
> +#define ARRAY_GEN1_STATUS_FAIL		(0x1ULL)

Unnecessary parenthesis

> +static int do_array_test_gen1(void *status)
> +{
> +	int cpu = smp_processor_id();
> +	int first;
> +
> +	first = cpumask_first(cpu_smt_mask(cpu));
> +
> +	if (cpu == first) {
> +		wrmsrl(MSR_ARRAY_TRIGGER, ARRAY_GEN1_TEST_ALL_ARRAYS);
> +		rdmsrl(MSR_ARRAY_STATUS, *((u64 *)status));
> +	}
> +
> +	return 0;
> +}
> +
> +static void ifs_array_test_gen1(int cpu, struct device *dev)
> +{
> +	struct ifs_data *ifsd = ifs_get_data(dev);
> +	u64	status = 0;

Just use space instead of tab.

> +
> +	stop_core_cpuslocked(cpu, do_array_test_gen1, &status);
> +	ifsd->scan_details = status;
> +
> +	if (status & ARRAY_GEN1_STATUS_FAIL)
> +		ifsd->status = SCAN_TEST_FAIL;
> +	else
> +		ifsd->status = SCAN_TEST_PASS;
> +}
> +
>  /*
>   * Initiate per core test. It wakes up work queue threads on the target cpu and
>   * its sibling cpu. Once all sibling threads wake up, the scan test gets executed and
> @@ -354,7 +386,10 @@ int do_core_test(int cpu, struct device *dev)
>  		ifs_test_core(cpu, dev);
>  		break;
>  	case IFS_TYPE_ARRAY_BIST:
> -		ifs_array_test_core(cpu, dev);
> +		if (ifsd->array_gen == 0)
> +			ifs_array_test_core(cpu, dev);
> +		else
> +			ifs_array_test_gen1(cpu, dev);
>  		break;
>  	default:
>  		return -EINVAL;
> 

-- 
 i.

Re: [PATCH 10/10] platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

[Joseph, Jithu]

On 9/15/2023 10:04 AM, Ilpo Järvinen wrote:
> On Wed, 13 Sep 2023, Jithu Joseph wrote:
> 

...

>> diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
>> index 997d2f07aa0c..9cfd5c015cb2 100644
>> --- a/drivers/platform/x86/intel/ifs/runtest.c
>> +++ b/drivers/platform/x86/intel/ifs/runtest.c
>> @@ -327,6 +327,38 @@ static void ifs_array_test_core(int cpu, struct device *dev)
>>  		ifsd->status = SCAN_TEST_PASS;
>>  }
>>  
>> +#define ARRAY_GEN1_TEST_ALL_ARRAYS	(0x0ULL)
>> +#define ARRAY_GEN1_STATUS_FAIL		(0x1ULL)
> 
> Unnecessary parenthesis
> 

noted

...

>> +
>> +static void ifs_array_test_gen1(int cpu, struct device *dev)
>> +{
>> +	struct ifs_data *ifsd = ifs_get_data(dev);
>> +	u64	status = 0;
> 
> Just use space instead of tab.
> 

Will do



Jithu

Re: [PATCH 00/10] IFS support for GNR and SRF

[Hans de Goede]

Hi Jithu,

On 9/13/23 20:33, Jithu Joseph wrote:
> This series adds IFS support for newer CPUs like Granite Rapids(GNR)
> and Sierra Forest(SRF).
> 
> There are changes in the IFS image loading and test flow to support
> these new CPUs.
> 
> Note to reviewers:
>  - patch 01/10 adds a bit definition to arch/x86/.../msr-index.h,
>   hence x86 maintainers are cc-d.
>  - patch 05/10 modifies an existing tracepoint, cc Steven Rostedt
>  - Rest are localized to IFS driver
> 
> Jithu Joseph (10):
>   platform/x86/intel/ifs: Store IFS generation number
>   platform/x86/intel/ifs: Refactor image loading code
>   platform/x86/intel/ifs: Image loading for new generations
>   platform/x86/intel/ifs: Scan test for new generations
>   trace: platform/x86/intel/ifs: Modify scan trace
>   platform/x86/intel/ifs: Validate image size
>   platform/x86/intel/ifs: Metadata validation for start_chunk
>   platform/x86/intel/ifs: Add new CPU support
>   platform/x86/intel/ifs: Add new error code
>   platform/x86/intel/ifs: ARRAY BIST for Sierra Forest
> 
>  arch/x86/include/asm/msr-index.h         |   2 +
>  drivers/platform/x86/intel/ifs/ifs.h     |  47 +++++++
>  include/trace/events/intel_ifs.h         |  16 +--
>  drivers/platform/x86/intel/ifs/core.c    |  14 +-
>  drivers/platform/x86/intel/ifs/load.c    | 159 +++++++++++++++++++++--
>  drivers/platform/x86/intel/ifs/runtest.c |  68 +++++++++-
>  6 files changed, 273 insertions(+), 33 deletions(-)
> 
> 
> base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d

Thank you for the patch series, please submit a new version addressing
the various review-remarks.

Regards,

Hans

Re: [PATCH 00/10] IFS support for GNR and SRF

[Joseph, Jithu]

On 9/18/2023 5:32 AM, Hans de Goede wrote:
> Hi Jithu,
> 
> On 9/13/23 20:33, Jithu Joseph wrote:
>> This series adds IFS support for newer CPUs like Granite Rapids(GNR)
>> and Sierra Forest(SRF).
>>
>> There are changes in the IFS image loading and test flow to support
>> these new CPUs.
>>
>> Note to reviewers:
>>  - patch 01/10 adds a bit definition to arch/x86/.../msr-index.h,
>>   hence x86 maintainers are cc-d.
>>  - patch 05/10 modifies an existing tracepoint, cc Steven Rostedt
>>  - Rest are localized to IFS driver
>>
>> Jithu Joseph (10):
>>   platform/x86/intel/ifs: Store IFS generation number
>>   platform/x86/intel/ifs: Refactor image loading code
>>   platform/x86/intel/ifs: Image loading for new generations
>>   platform/x86/intel/ifs: Scan test for new generations
>>   trace: platform/x86/intel/ifs: Modify scan trace
>>   platform/x86/intel/ifs: Validate image size
>>   platform/x86/intel/ifs: Metadata validation for start_chunk
>>   platform/x86/intel/ifs: Add new CPU support
>>   platform/x86/intel/ifs: Add new error code
>>   platform/x86/intel/ifs: ARRAY BIST for Sierra Forest
>>
>>  arch/x86/include/asm/msr-index.h         |   2 +
>>  drivers/platform/x86/intel/ifs/ifs.h     |  47 +++++++
>>  include/trace/events/intel_ifs.h         |  16 +--
>>  drivers/platform/x86/intel/ifs/core.c    |  14 +-
>>  drivers/platform/x86/intel/ifs/load.c    | 159 +++++++++++++++++++++--
>>  drivers/platform/x86/intel/ifs/runtest.c |  68 +++++++++-
>>  6 files changed, 273 insertions(+), 33 deletions(-)
>>
>>
>> base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
> 
> Thank you for the patch series, please submit a new version addressing
> the various review-remarks.
> 

Will do ... Thanks

Jithu

[PATCH v2 0/9] IFS support for GNR and SRF

[Jithu Joseph]

Changes in v2
   Ilpo Jarvinen
    - Use GENMASK_ULL() / FIELD_GET() for bitops (patch 01)
    - Avoid mixing u8 type and bitfields in certain MSR structure
       scenarios (patch03 also suggested by Dave Hansen)
    - Expand bitfield structures to use consistent genx naming (patch 04)
    - Replace goto with do / while (patch 03)
    - general formatting (multiple patches)
        - remove un-necessary parenthesis
        - reformat commit message to use whole allowed 72 columns
        - alignment changes
    Other change
     - fold v1 04/10 and 05/10 into v2 patch 04/09 to satisfy build
        constraints due to consistent genx naming
v1 submission:
Link: https://lore.kernel.org/lkml/20230913183348.1349409-1-jithu.joseph@intel.com/

This series adds In Field Scan(IFS) support for newer CPUs like Granite
Rapids(GNR) and Sierra Forest(SRF).

There are changes in the IFS image loading and test flow to support
these new CPUs.

Note to reviewers:
 - patch 01/09 adds a bit mask to arch/x86/.../msr-index.h,
  hence x86 maintainers are cc-d.
 - patch 04/09 modifies an existing tracepoint, cc Steven Rostedt
 - Rest are localized to IFS driver

Jithu Joseph (9):
  platform/x86/intel/ifs: Store IFS generation number
  platform/x86/intel/ifs: Refactor image loading code
  platform/x86/intel/ifs: Gen2 scan image loading
  platform/x86/intel/ifs: Gen2 Scan test support
  platform/x86/intel/ifs: Validate image size
  platform/x86/intel/ifs: Metadata validation for start_chunk
  platform/x86/intel/ifs: Add new CPU support
  platform/x86/intel/ifs: Add new error code
  platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

 arch/x86/include/asm/msr-index.h         |   1 +
 drivers/platform/x86/intel/ifs/ifs.h     |  61 ++++++++-
 include/trace/events/intel_ifs.h         |  16 +--
 drivers/platform/x86/intel/ifs/core.c    |  15 ++-
 drivers/platform/x86/intel/ifs/load.c    | 158 +++++++++++++++++++++--
 drivers/platform/x86/intel/ifs/runtest.c |  69 ++++++++--
 6 files changed, 280 insertions(+), 40 deletions(-)


base-commit: ce9ecca0238b140b88f43859b211c9fdfd8e5b70
-- 
2.25.1

[PATCH v2 1/9] platform/x86/intel/ifs: Store IFS generation number

[Jithu Joseph]

IFS generation number is reported via MSR_INTEGRITY_CAPS.  As IFS
support gets added to newer CPUs, some differences are expected during
IFS image loading and test flows.

Define MSR bitmasks to extract and store the generation in driver data,
so that driver can modify its MSR interaction appropriately.

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>
---
 arch/x86/include/asm/msr-index.h      | 1 +
 drivers/platform/x86/intel/ifs/ifs.h  | 2 ++
 drivers/platform/x86/intel/ifs/core.c | 3 +++
 3 files changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 1d111350197f..838e5a013a07 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -222,6 +222,7 @@
 #define MSR_INTEGRITY_CAPS_ARRAY_BIST          BIT(MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT)
 #define MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT	4
 #define MSR_INTEGRITY_CAPS_PERIODIC_BIST	BIT(MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT)
+#define MSR_INTEGRITY_CAPS_SAF_GEN_MASK	GENMASK_ULL(10, 9)
 
 #define MSR_LBR_NHM_FROM		0x00000680
 #define MSR_LBR_NHM_TO			0x000006c0
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 93191855890f..d666aeed20fc 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -229,6 +229,7 @@ struct ifs_test_caps {
  * @status: it holds simple status pass/fail/untested
  * @scan_details: opaque scan status code from h/w
  * @cur_batch: number indicating the currently loaded test file
+ * @generation: IFS test generation enumerated by hardware
  */
 struct ifs_data {
 	int	loaded_version;
@@ -238,6 +239,7 @@ struct ifs_data {
 	int	status;
 	u64	scan_details;
 	u32	cur_batch;
+	u32	generation;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 306f886b52d2..4ff2aa4b484b 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2022 Intel Corporation. */
 
+#include <linux/bitfield.h>
 #include <linux/module.h>
 #include <linux/kdev_t.h>
 #include <linux/semaphore.h>
@@ -94,6 +95,8 @@ static int __init ifs_init(void)
 	for (i = 0; i < IFS_NUMTESTS; i++) {
 		if (!(msrval & BIT(ifs_devices[i].test_caps->integrity_cap_bit)))
 			continue;
+		ifs_devices[i].rw_data.generation = FIELD_GET(MSR_INTEGRITY_CAPS_SAF_GEN_MASK,
+							      msrval);
 		ret = misc_register(&ifs_devices[i].misc);
 		if (ret)
 			goto err_exit;
-- 
2.25.1

Re: [PATCH v2 1/9] platform/x86/intel/ifs: Store IFS generation number

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2912 bytes --]

On Fri, 22 Sep 2023, Jithu Joseph wrote:

> IFS generation number is reported via MSR_INTEGRITY_CAPS.  As IFS
> support gets added to newer CPUs, some differences are expected during
> IFS image loading and test flows.
> 
> Define MSR bitmasks to extract and store the generation in driver data,
> so that driver can modify its MSR interaction appropriately.
> 
> 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>
> ---
>  arch/x86/include/asm/msr-index.h      | 1 +
>  drivers/platform/x86/intel/ifs/ifs.h  | 2 ++
>  drivers/platform/x86/intel/ifs/core.c | 3 +++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 1d111350197f..838e5a013a07 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -222,6 +222,7 @@
>  #define MSR_INTEGRITY_CAPS_ARRAY_BIST          BIT(MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT)
>  #define MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT	4
>  #define MSR_INTEGRITY_CAPS_PERIODIC_BIST	BIT(MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT)
> +#define MSR_INTEGRITY_CAPS_SAF_GEN_MASK	GENMASK_ULL(10, 9)
>  
>  #define MSR_LBR_NHM_FROM		0x00000680
>  #define MSR_LBR_NHM_TO			0x000006c0
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 93191855890f..d666aeed20fc 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -229,6 +229,7 @@ struct ifs_test_caps {
>   * @status: it holds simple status pass/fail/untested
>   * @scan_details: opaque scan status code from h/w
>   * @cur_batch: number indicating the currently loaded test file
> + * @generation: IFS test generation enumerated by hardware
>   */
>  struct ifs_data {
>  	int	loaded_version;
> @@ -238,6 +239,7 @@ struct ifs_data {
>  	int	status;
>  	u64	scan_details;
>  	u32	cur_batch;
> +	u32	generation;
>  };
>  
>  struct ifs_work {
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 306f886b52d2..4ff2aa4b484b 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2022 Intel Corporation. */
>  
> +#include <linux/bitfield.h>
>  #include <linux/module.h>
>  #include <linux/kdev_t.h>
>  #include <linux/semaphore.h>
> @@ -94,6 +95,8 @@ static int __init ifs_init(void)
>  	for (i = 0; i < IFS_NUMTESTS; i++) {
>  		if (!(msrval & BIT(ifs_devices[i].test_caps->integrity_cap_bit)))
>  			continue;
> +		ifs_devices[i].rw_data.generation = FIELD_GET(MSR_INTEGRITY_CAPS_SAF_GEN_MASK,
> +							      msrval);
>  		ret = misc_register(&ifs_devices[i].misc);
>  		if (ret)
>  			goto err_exit;
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v2 2/9] platform/x86/intel/ifs: Refactor image loading code

[Jithu Joseph]

IFS image loading flow is slightly different for newer IFS generations.

In preparation for adding support for newer IFS generations, refactor
portions of existing image loading code for reuse.

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/load.c | 31 ++++++++++++++++-----------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index cefd0d886cfd..851c97cc6a6b 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -80,6 +80,23 @@ static struct metadata_header *find_meta_data(void *ucode, unsigned int meta_typ
 	return NULL;
 }
 
+static void hashcopy_err_message(struct device *dev, u32 err_code)
+{
+	if (err_code >= ARRAY_SIZE(scan_hash_status))
+		dev_err(dev, "invalid error code 0x%x for hash copy\n", err_code);
+	else
+		dev_err(dev, "Hash copy error : %s\n", scan_hash_status[err_code]);
+}
+
+static void auth_err_message(struct device *dev, u32 err_code)
+{
+	if (err_code >= ARRAY_SIZE(scan_authentication_status))
+		dev_err(dev, "invalid error code 0x%x for authentication\n", err_code);
+	else
+		dev_err(dev, "Chunk authentication error : %s\n",
+			scan_authentication_status[err_code]);
+}
+
 /*
  * To copy scan hashes and authenticate test chunks, the initiating cpu must point
  * to the EDX:EAX to the test image in linear address.
@@ -109,11 +126,7 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
 
 	if (!hashes_status.valid) {
 		ifsd->loading_error = true;
-		if (err_code >= ARRAY_SIZE(scan_hash_status)) {
-			dev_err(dev, "invalid error code 0x%x for hash copy\n", err_code);
-			goto done;
-		}
-		dev_err(dev, "Hash copy error : %s", scan_hash_status[err_code]);
+		hashcopy_err_message(dev, err_code);
 		goto done;
 	}
 
@@ -133,13 +146,7 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
 
 		if (err_code) {
 			ifsd->loading_error = true;
-			if (err_code >= ARRAY_SIZE(scan_authentication_status)) {
-				dev_err(dev,
-					"invalid error code 0x%x for authentication\n", err_code);
-				goto done;
-			}
-			dev_err(dev, "Chunk authentication error %s\n",
-				scan_authentication_status[err_code]);
+			auth_err_message(dev, err_code);
 			goto done;
 		}
 	}
-- 
2.25.1

Re: [PATCH v2 2/9] platform/x86/intel/ifs: Refactor image loading code

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2672 bytes --]

On Fri, 22 Sep 2023, Jithu Joseph wrote:

> IFS image loading flow is slightly different for newer IFS generations.
> 
> In preparation for adding support for newer IFS generations, refactor
> portions of existing image loading code for reuse.
> 
> 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/load.c | 31 ++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index cefd0d886cfd..851c97cc6a6b 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -80,6 +80,23 @@ static struct metadata_header *find_meta_data(void *ucode, unsigned int meta_typ
>  	return NULL;
>  }
>  
> +static void hashcopy_err_message(struct device *dev, u32 err_code)
> +{
> +	if (err_code >= ARRAY_SIZE(scan_hash_status))
> +		dev_err(dev, "invalid error code 0x%x for hash copy\n", err_code);
> +	else
> +		dev_err(dev, "Hash copy error : %s\n", scan_hash_status[err_code]);
> +}
> +
> +static void auth_err_message(struct device *dev, u32 err_code)
> +{
> +	if (err_code >= ARRAY_SIZE(scan_authentication_status))
> +		dev_err(dev, "invalid error code 0x%x for authentication\n", err_code);
> +	else
> +		dev_err(dev, "Chunk authentication error : %s\n",
> +			scan_authentication_status[err_code]);
> +}
> +
>  /*
>   * To copy scan hashes and authenticate test chunks, the initiating cpu must point
>   * to the EDX:EAX to the test image in linear address.
> @@ -109,11 +126,7 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
>  
>  	if (!hashes_status.valid) {
>  		ifsd->loading_error = true;
> -		if (err_code >= ARRAY_SIZE(scan_hash_status)) {
> -			dev_err(dev, "invalid error code 0x%x for hash copy\n", err_code);
> -			goto done;
> -		}
> -		dev_err(dev, "Hash copy error : %s", scan_hash_status[err_code]);
> +		hashcopy_err_message(dev, err_code);
>  		goto done;
>  	}
>  
> @@ -133,13 +146,7 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
>  
>  		if (err_code) {
>  			ifsd->loading_error = true;
> -			if (err_code >= ARRAY_SIZE(scan_authentication_status)) {
> -				dev_err(dev,
> -					"invalid error code 0x%x for authentication\n", err_code);
> -				goto done;
> -			}
> -			dev_err(dev, "Chunk authentication error %s\n",
> -				scan_authentication_status[err_code]);
> +			auth_err_message(dev, err_code);
>  			goto done;
>  		}
>  	}
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v2 3/9] platform/x86/intel/ifs: Gen2 scan image loading

[Jithu Joseph]

Scan image loading flow for newer IFS generations are slightly different
from that of current generation. In newer schemes, loading need not be
done once for each socket as was done in gen0.

Also the width of NUM_CHUNKS bitfield in SCAN_HASHES_STATUS MSR has
increased from 8 -> 16 bits. Similarly there are width differences for
CHUNK_AUTHENTICATION_STATUS too.

Further the parameter to AUTHENTICATE_AND_COPY_CHUNK is passed
differently in newer generations.

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  |  27 +++++++
 drivers/platform/x86/intel/ifs/load.c | 112 +++++++++++++++++++++++++-
 2 files changed, 137 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index d666aeed20fc..43281d456a09 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -137,6 +137,8 @@
 #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
 #define MSR_ACTIVATE_SCAN			0x000002c6
 #define MSR_SCAN_STATUS				0x000002c7
+#define MSR_SAF_CTRL				0x000004f0
+
 #define SCAN_NOT_TESTED				0
 #define SCAN_TEST_PASS				1
 #define SCAN_TEST_FAIL				2
@@ -158,6 +160,19 @@ union ifs_scan_hashes_status {
 	};
 };
 
+union ifs_scan_hashes_status_gen2 {
+	u64	data;
+	struct {
+		u16	chunk_size;
+		u16	num_chunks;
+		u32	error_code	:8;
+		u32	chunks_in_stride :9;
+		u32	rsvd		:2;
+		u32	max_core_limit	:12;
+		u32	valid		:1;
+	};
+};
+
 /* MSR_CHUNKS_AUTH_STATUS bit fields */
 union ifs_chunks_auth_status {
 	u64	data;
@@ -170,6 +185,16 @@ union ifs_chunks_auth_status {
 	};
 };
 
+union ifs_chunks_auth_status_gen2 {
+	u64	data;
+	struct {
+		u16	valid_chunks;
+		u16	total_chunks;
+		u32	error_code	:8;
+		u32	rsvd2		:24;
+	};
+};
+
 /* MSR_ACTIVATE_SCAN bit fields */
 union ifs_scan {
 	u64	data;
@@ -230,6 +255,7 @@ struct ifs_test_caps {
  * @scan_details: opaque scan status code from h/w
  * @cur_batch: number indicating the currently loaded test file
  * @generation: IFS test generation enumerated by hardware
+ * @chunk_size: size of a test chunk
  */
 struct ifs_data {
 	int	loaded_version;
@@ -240,6 +266,7 @@ struct ifs_data {
 	u64	scan_details;
 	u32	cur_batch;
 	u32	generation;
+	u32	chunk_size;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 851c97cc6a6b..6b827247945b 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -2,6 +2,7 @@
 /* Copyright(c) 2022 Intel Corporation. */
 
 #include <linux/firmware.h>
+#include <linux/sizes.h>
 #include <asm/cpu.h>
 #include <asm/microcode.h>
 
@@ -26,6 +27,11 @@ union meta_data {
 
 #define IFS_HEADER_SIZE	(sizeof(struct microcode_header_intel))
 #define META_TYPE_IFS	1
+#define INVALIDATE_STRIDE	0x1UL
+#define IFS_GEN_STRIDE_AWARE	2
+#define AUTH_INTERRUPTED_ERROR	5
+#define IFS_AUTH_RETRY_CT	10
+
 static  struct microcode_header_intel *ifs_header_ptr;	/* pointer to the ifs image header */
 static u64 ifs_hash_ptr;			/* Address of ifs metadata (hash) */
 static u64 ifs_test_image_ptr;			/* 256B aligned address of test pattern */
@@ -44,7 +50,10 @@ static const char * const scan_hash_status[] = {
 static const char * const scan_authentication_status[] = {
 	[0] = "No error reported",
 	[1] = "Attempt to authenticate a chunk which is already marked as authentic",
-	[2] = "Chunk authentication error. The hash of chunk did not match expected value"
+	[2] = "Chunk authentication error. The hash of chunk did not match expected value",
+	[3] = "Reserved",
+	[4] = "Chunk outside the current stride",
+	[5] = "Authentication flow interrupted",
 };
 
 #define MC_HEADER_META_TYPE_END		(0)
@@ -154,6 +163,102 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
 	complete(&ifs_done);
 }
 
+static int get_num_chunks(int gen, union ifs_scan_hashes_status_gen2 status)
+{
+	return gen >= IFS_GEN_STRIDE_AWARE ? status.chunks_in_stride : status.num_chunks;
+}
+
+static bool need_copy_scan_hashes(struct ifs_data *ifsd)
+{
+	return !ifsd->loaded ||
+		ifsd->generation < IFS_GEN_STRIDE_AWARE ||
+		ifsd->loaded_version != ifs_header_ptr->rev;
+}
+
+static int copy_hashes_authenticate_chunks_gen2(struct device *dev)
+{
+	union ifs_scan_hashes_status_gen2 hashes_status;
+	union ifs_chunks_auth_status_gen2 chunk_status;
+	u32 err_code, valid_chunks, total_chunks;
+	int i, num_chunks, chunk_size;
+	union meta_data *ifs_meta;
+	int starting_chunk_nr;
+	struct ifs_data *ifsd;
+	u64 linear_addr, base;
+	u64 chunk_table[2];
+	int retry_count;
+
+	ifsd = ifs_get_data(dev);
+
+	if (need_copy_scan_hashes(ifsd)) {
+		wrmsrl(MSR_COPY_SCAN_HASHES, ifs_hash_ptr);
+		rdmsrl(MSR_SCAN_HASHES_STATUS, hashes_status.data);
+
+		/* enumerate the scan image information */
+		chunk_size = hashes_status.chunk_size * SZ_1K;
+		err_code = hashes_status.error_code;
+
+		num_chunks = get_num_chunks(ifsd->generation, hashes_status);
+
+		if (!hashes_status.valid) {
+			hashcopy_err_message(dev, err_code);
+			return -EIO;
+		}
+		ifsd->loaded_version = ifs_header_ptr->rev;
+		ifsd->chunk_size = chunk_size;
+	} else {
+		num_chunks = ifsd->valid_chunks;
+		chunk_size = ifsd->chunk_size;
+	}
+
+	if (ifsd->generation >= IFS_GEN_STRIDE_AWARE) {
+		wrmsrl(MSR_SAF_CTRL, INVALIDATE_STRIDE);
+		rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
+		if (chunk_status.valid_chunks != 0) {
+			dev_err(dev, "Couldn't invalidate installed stride - %d\n",
+				chunk_status.valid_chunks);
+			return -EIO;
+		}
+	}
+
+	base = ifs_test_image_ptr;
+	ifs_meta = (union meta_data *)find_meta_data(ifs_header_ptr, META_TYPE_IFS);
+	starting_chunk_nr = ifs_meta->starting_chunk;
+
+	/* scan data authentication and copy chunks to secured memory */
+	for (i = 0; i < num_chunks; i++) {
+		retry_count = IFS_AUTH_RETRY_CT;
+		linear_addr = base + i * chunk_size;
+
+		chunk_table[0] = starting_chunk_nr + i;
+		chunk_table[1] = linear_addr;
+		do {
+			wrmsrl(MSR_AUTHENTICATE_AND_COPY_CHUNK, (u64)chunk_table);
+			rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
+			err_code = chunk_status.error_code;
+		} while (err_code == AUTH_INTERRUPTED_ERROR && --retry_count);
+
+		if (err_code) {
+			ifsd->loading_error = true;
+			auth_err_message(dev, err_code);
+			return -EIO;
+		}
+	}
+
+	valid_chunks = chunk_status.valid_chunks;
+	total_chunks = chunk_status.total_chunks;
+
+	if (valid_chunks != total_chunks) {
+		ifsd->loading_error = true;
+		dev_err(dev, "Couldn't authenticate all the chunks. Authenticated %d total %d.\n",
+			valid_chunks, total_chunks);
+		return -EIO;
+	}
+	ifsd->valid_chunks = valid_chunks;
+
+	return 0;
+}
+
 static int validate_ifs_metadata(struct device *dev)
 {
 	struct ifs_data *ifsd = ifs_get_data(dev);
@@ -206,7 +311,9 @@ static int scan_chunks_sanity_check(struct device *dev)
 		return ret;
 
 	ifsd->loading_error = false;
-	ifsd->loaded_version = ifs_header_ptr->rev;
+
+	if (ifsd->generation > 0)
+		return copy_hashes_authenticate_chunks_gen2(dev);
 
 	/* copy the scan hash and authenticate per package */
 	cpus_read_lock();
@@ -226,6 +333,7 @@ static int scan_chunks_sanity_check(struct device *dev)
 		ifs_pkg_auth[curr_pkg] = 1;
 	}
 	ret = 0;
+	ifsd->loaded_version = ifs_header_ptr->rev;
 out:
 	cpus_read_unlock();
 
-- 
2.25.1

Re: [PATCH v2 3/9] platform/x86/intel/ifs: Gen2 scan image loading

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 8135 bytes --]

On Fri, 22 Sep 2023, Jithu Joseph wrote:

> Scan image loading flow for newer IFS generations are slightly different
> from that of current generation. In newer schemes, loading need not be
> done once for each socket as was done in gen0.
> 
> Also the width of NUM_CHUNKS bitfield in SCAN_HASHES_STATUS MSR has
> increased from 8 -> 16 bits. Similarly there are width differences for
> CHUNK_AUTHENTICATION_STATUS too.
> 
> Further the parameter to AUTHENTICATE_AND_COPY_CHUNK is passed
> differently in newer generations.
> 
> 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  |  27 +++++++
>  drivers/platform/x86/intel/ifs/load.c | 112 +++++++++++++++++++++++++-
>  2 files changed, 137 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index d666aeed20fc..43281d456a09 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -137,6 +137,8 @@
>  #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
>  #define MSR_ACTIVATE_SCAN			0x000002c6
>  #define MSR_SCAN_STATUS				0x000002c7
> +#define MSR_SAF_CTRL				0x000004f0
> +
>  #define SCAN_NOT_TESTED				0
>  #define SCAN_TEST_PASS				1
>  #define SCAN_TEST_FAIL				2
> @@ -158,6 +160,19 @@ union ifs_scan_hashes_status {
>  	};
>  };
>  
> +union ifs_scan_hashes_status_gen2 {
> +	u64	data;
> +	struct {
> +		u16	chunk_size;
> +		u16	num_chunks;
> +		u32	error_code	:8;
> +		u32	chunks_in_stride :9;

If you need to respin, it would be nice to align these but don't do 
another version of the series just because of that.

> +		u32	rsvd		:2;
> +		u32	max_core_limit	:12;
> +		u32	valid		:1;
> +	};
> +};
> +
>  /* MSR_CHUNKS_AUTH_STATUS bit fields */
>  union ifs_chunks_auth_status {
>  	u64	data;
> @@ -170,6 +185,16 @@ union ifs_chunks_auth_status {
>  	};
>  };
>  
> +union ifs_chunks_auth_status_gen2 {
> +	u64	data;
> +	struct {
> +		u16	valid_chunks;
> +		u16	total_chunks;
> +		u32	error_code	:8;
> +		u32	rsvd2		:24;
> +	};
> +};
> +
>  /* MSR_ACTIVATE_SCAN bit fields */
>  union ifs_scan {
>  	u64	data;
> @@ -230,6 +255,7 @@ struct ifs_test_caps {
>   * @scan_details: opaque scan status code from h/w
>   * @cur_batch: number indicating the currently loaded test file
>   * @generation: IFS test generation enumerated by hardware
> + * @chunk_size: size of a test chunk
>   */
>  struct ifs_data {
>  	int	loaded_version;
> @@ -240,6 +266,7 @@ struct ifs_data {
>  	u64	scan_details;
>  	u32	cur_batch;
>  	u32	generation;
> +	u32	chunk_size;
>  };
>  
>  struct ifs_work {
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index 851c97cc6a6b..6b827247945b 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -2,6 +2,7 @@
>  /* Copyright(c) 2022 Intel Corporation. */
>  
>  #include <linux/firmware.h>
> +#include <linux/sizes.h>
>  #include <asm/cpu.h>
>  #include <asm/microcode.h>
>  
> @@ -26,6 +27,11 @@ union meta_data {
>  
>  #define IFS_HEADER_SIZE	(sizeof(struct microcode_header_intel))
>  #define META_TYPE_IFS	1
> +#define INVALIDATE_STRIDE	0x1UL
> +#define IFS_GEN_STRIDE_AWARE	2
> +#define AUTH_INTERRUPTED_ERROR	5
> +#define IFS_AUTH_RETRY_CT	10
> +
>  static  struct microcode_header_intel *ifs_header_ptr;	/* pointer to the ifs image header */
>  static u64 ifs_hash_ptr;			/* Address of ifs metadata (hash) */
>  static u64 ifs_test_image_ptr;			/* 256B aligned address of test pattern */
> @@ -44,7 +50,10 @@ static const char * const scan_hash_status[] = {
>  static const char * const scan_authentication_status[] = {
>  	[0] = "No error reported",
>  	[1] = "Attempt to authenticate a chunk which is already marked as authentic",
> -	[2] = "Chunk authentication error. The hash of chunk did not match expected value"
> +	[2] = "Chunk authentication error. The hash of chunk did not match expected value",
> +	[3] = "Reserved",
> +	[4] = "Chunk outside the current stride",
> +	[5] = "Authentication flow interrupted",
>  };
>  
>  #define MC_HEADER_META_TYPE_END		(0)
> @@ -154,6 +163,102 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
>  	complete(&ifs_done);
>  }
>  
> +static int get_num_chunks(int gen, union ifs_scan_hashes_status_gen2 status)
> +{
> +	return gen >= IFS_GEN_STRIDE_AWARE ? status.chunks_in_stride : status.num_chunks;
> +}
> +
> +static bool need_copy_scan_hashes(struct ifs_data *ifsd)
> +{
> +	return !ifsd->loaded ||
> +		ifsd->generation < IFS_GEN_STRIDE_AWARE ||
> +		ifsd->loaded_version != ifs_header_ptr->rev;
> +}
> +
> +static int copy_hashes_authenticate_chunks_gen2(struct device *dev)
> +{
> +	union ifs_scan_hashes_status_gen2 hashes_status;
> +	union ifs_chunks_auth_status_gen2 chunk_status;
> +	u32 err_code, valid_chunks, total_chunks;
> +	int i, num_chunks, chunk_size;
> +	union meta_data *ifs_meta;
> +	int starting_chunk_nr;
> +	struct ifs_data *ifsd;
> +	u64 linear_addr, base;
> +	u64 chunk_table[2];
> +	int retry_count;
> +
> +	ifsd = ifs_get_data(dev);
> +
> +	if (need_copy_scan_hashes(ifsd)) {
> +		wrmsrl(MSR_COPY_SCAN_HASHES, ifs_hash_ptr);
> +		rdmsrl(MSR_SCAN_HASHES_STATUS, hashes_status.data);
> +
> +		/* enumerate the scan image information */
> +		chunk_size = hashes_status.chunk_size * SZ_1K;
> +		err_code = hashes_status.error_code;
> +
> +		num_chunks = get_num_chunks(ifsd->generation, hashes_status);
> +
> +		if (!hashes_status.valid) {
> +			hashcopy_err_message(dev, err_code);
> +			return -EIO;
> +		}
> +		ifsd->loaded_version = ifs_header_ptr->rev;
> +		ifsd->chunk_size = chunk_size;
> +	} else {
> +		num_chunks = ifsd->valid_chunks;
> +		chunk_size = ifsd->chunk_size;
> +	}
> +
> +	if (ifsd->generation >= IFS_GEN_STRIDE_AWARE) {
> +		wrmsrl(MSR_SAF_CTRL, INVALIDATE_STRIDE);
> +		rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
> +		if (chunk_status.valid_chunks != 0) {
> +			dev_err(dev, "Couldn't invalidate installed stride - %d\n",
> +				chunk_status.valid_chunks);
> +			return -EIO;
> +		}
> +	}
> +
> +	base = ifs_test_image_ptr;
> +	ifs_meta = (union meta_data *)find_meta_data(ifs_header_ptr, META_TYPE_IFS);
> +	starting_chunk_nr = ifs_meta->starting_chunk;
> +
> +	/* scan data authentication and copy chunks to secured memory */
> +	for (i = 0; i < num_chunks; i++) {
> +		retry_count = IFS_AUTH_RETRY_CT;
> +		linear_addr = base + i * chunk_size;
> +
> +		chunk_table[0] = starting_chunk_nr + i;
> +		chunk_table[1] = linear_addr;
> +		do {
> +			wrmsrl(MSR_AUTHENTICATE_AND_COPY_CHUNK, (u64)chunk_table);
> +			rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
> +			err_code = chunk_status.error_code;
> +		} while (err_code == AUTH_INTERRUPTED_ERROR && --retry_count);
> +
> +		if (err_code) {
> +			ifsd->loading_error = true;
> +			auth_err_message(dev, err_code);
> +			return -EIO;
> +		}
> +	}
> +
> +	valid_chunks = chunk_status.valid_chunks;
> +	total_chunks = chunk_status.total_chunks;
> +
> +	if (valid_chunks != total_chunks) {
> +		ifsd->loading_error = true;
> +		dev_err(dev, "Couldn't authenticate all the chunks. Authenticated %d total %d.\n",
> +			valid_chunks, total_chunks);
> +		return -EIO;
> +	}
> +	ifsd->valid_chunks = valid_chunks;
> +
> +	return 0;
> +}
> +
>  static int validate_ifs_metadata(struct device *dev)
>  {
>  	struct ifs_data *ifsd = ifs_get_data(dev);
> @@ -206,7 +311,9 @@ static int scan_chunks_sanity_check(struct device *dev)
>  		return ret;
>  
>  	ifsd->loading_error = false;
> -	ifsd->loaded_version = ifs_header_ptr->rev;
> +
> +	if (ifsd->generation > 0)
> +		return copy_hashes_authenticate_chunks_gen2(dev);
>  
>  	/* copy the scan hash and authenticate per package */
>  	cpus_read_lock();
> @@ -226,6 +333,7 @@ static int scan_chunks_sanity_check(struct device *dev)
>  		ifs_pkg_auth[curr_pkg] = 1;
>  	}
>  	ret = 0;
> +	ifsd->loaded_version = ifs_header_ptr->rev;
>  out:
>  	cpus_read_unlock();
>  
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v2 4/9] platform/x86/intel/ifs: Gen2 Scan test support

[Jithu Joseph]

Width of chunk related bitfields is ACTIVATE_SCAN and SCAN_STATUS MSRs
are different in newer IFS generation compared to gen0.

Make changes to scan test flow such that MSRs are populated
appropriately based on the generation supported by hardware.

Account for the 8/16 bit MSR bitfield width differences between gen0 and
newer generations for the scan test trace event too.

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     | 28 +++++++++++++++++++-----
 include/trace/events/intel_ifs.h         | 16 +++++++-------
 drivers/platform/x86/intel/ifs/runtest.c | 26 ++++++++++++++++------
 3 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 43281d456a09..cd213b89d278 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -199,9 +199,17 @@ union ifs_chunks_auth_status_gen2 {
 union ifs_scan {
 	u64	data;
 	struct {
-		u32	start	:8;
-		u32	stop	:8;
-		u32	rsvd	:16;
+		union {
+			struct {
+				u8	start;
+				u8	stop;
+				u16	rsvd;
+			} gen0;
+			struct {
+				u16	start;
+				u16	stop;
+			} gen2;
+		};
 		u32	delay	:31;
 		u32	sigmce	:1;
 	};
@@ -211,9 +219,17 @@ union ifs_scan {
 union ifs_status {
 	u64	data;
 	struct {
-		u32	chunk_num		:8;
-		u32	chunk_stop_index	:8;
-		u32	rsvd1			:16;
+		union {
+			struct {
+				u8	chunk_num;
+				u8	chunk_stop_index;
+				u16	rsvd1;
+			} gen0;
+			struct {
+				u16	chunk_num;
+				u16	chunk_stop_index;
+			} gen2;
+		};
 		u32	error_code		:8;
 		u32	rsvd2			:22;
 		u32	control_error		:1;
diff --git a/include/trace/events/intel_ifs.h b/include/trace/events/intel_ifs.h
index d7353024016c..af0af3f1d9b7 100644
--- a/include/trace/events/intel_ifs.h
+++ b/include/trace/events/intel_ifs.h
@@ -10,25 +10,25 @@
 
 TRACE_EVENT(ifs_status,
 
-	TP_PROTO(int cpu, union ifs_scan activate, union ifs_status status),
+	TP_PROTO(int cpu, int start, int stop, u64 status),
 
-	TP_ARGS(cpu, activate, status),
+	TP_ARGS(cpu, start, stop, status),
 
 	TP_STRUCT__entry(
 		__field(	u64,	status	)
 		__field(	int,	cpu	)
-		__field(	u8,	start	)
-		__field(	u8,	stop	)
+		__field(	u16,	start	)
+		__field(	u16,	stop	)
 	),
 
 	TP_fast_assign(
 		__entry->cpu	= cpu;
-		__entry->start	= activate.start;
-		__entry->stop	= activate.stop;
-		__entry->status	= status.data;
+		__entry->start	= start;
+		__entry->stop	= stop;
+		__entry->status	= status;
 	),
 
-	TP_printk("cpu: %d, start: %.2x, stop: %.2x, status: %llx",
+	TP_printk("cpu: %d, start: %.4x, stop: %.4x, status: %.16llx",
 		__entry->cpu,
 		__entry->start,
 		__entry->stop,
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 1061eb7ec399..94d486e5d263 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -171,21 +171,30 @@ static void ifs_test_core(int cpu, struct device *dev)
 	union ifs_status status;
 	unsigned long timeout;
 	struct ifs_data *ifsd;
+	int to_start, to_stop;
+	int status_chunk;
 	u64 msrvals[2];
 	int retries;
 
 	ifsd = ifs_get_data(dev);
 
-	activate.rsvd = 0;
 	activate.delay = IFS_THREAD_WAIT;
 	activate.sigmce = 0;
-	activate.start = 0;
-	activate.stop = ifsd->valid_chunks - 1;
+	to_start = 0;
+	to_stop = ifsd->valid_chunks - 1;
+
+	if (ifsd->generation) {
+		activate.gen2.start = to_start;
+		activate.gen2.stop = to_stop;
+	} else {
+		activate.gen0.start = to_start;
+		activate.gen0.stop = to_stop;
+	}
 
 	timeout = jiffies + HZ / 2;
 	retries = MAX_IFS_RETRIES;
 
-	while (activate.start <= activate.stop) {
+	while (to_start <= to_stop) {
 		if (time_after(jiffies, timeout)) {
 			status.error_code = IFS_SW_TIMEOUT;
 			break;
@@ -196,13 +205,14 @@ static void ifs_test_core(int cpu, struct device *dev)
 
 		status.data = msrvals[1];
 
-		trace_ifs_status(cpu, activate, status);
+		trace_ifs_status(cpu, to_start, to_stop, status.data);
 
 		/* Some cases can be retried, give up for others */
 		if (!can_restart(status))
 			break;
 
-		if (status.chunk_num == activate.start) {
+		status_chunk = ifsd->generation ? status.gen2.chunk_num : status.gen0.chunk_num;
+		if (status_chunk == to_start) {
 			/* Check for forward progress */
 			if (--retries == 0) {
 				if (status.error_code == IFS_NO_ERROR)
@@ -211,7 +221,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.gen0.start = status_chunk);
+			to_start = status_chunk;
 		}
 	}
 
-- 
2.25.1

Re: [PATCH v2 4/9] platform/x86/intel/ifs: Gen2 Scan test support

[Ilpo Järvinen]

On Fri, 22 Sep 2023, Jithu Joseph wrote:

> Width of chunk related bitfields is ACTIVATE_SCAN and SCAN_STATUS MSRs
> are different in newer IFS generation compared to gen0.
> 
> Make changes to scan test flow such that MSRs are populated
> appropriately based on the generation supported by hardware.
> 
> Account for the 8/16 bit MSR bitfield width differences between gen0 and
> newer generations for the scan test trace event too.
> 
> 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     | 28 +++++++++++++++++++-----
>  include/trace/events/intel_ifs.h         | 16 +++++++-------
>  drivers/platform/x86/intel/ifs/runtest.c | 26 ++++++++++++++++------
>  3 files changed, 49 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 43281d456a09..cd213b89d278 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -199,9 +199,17 @@ union ifs_chunks_auth_status_gen2 {
>  union ifs_scan {
>  	u64	data;
>  	struct {
> -		u32	start	:8;
> -		u32	stop	:8;
> -		u32	rsvd	:16;
> +		union {
> +			struct {
> +				u8	start;
> +				u8	stop;
> +				u16	rsvd;
> +			} gen0;
> +			struct {
> +				u16	start;
> +				u16	stop;
> +			} gen2;
> +		};
>  		u32	delay	:31;
>  		u32	sigmce	:1;
>  	};
> @@ -211,9 +219,17 @@ union ifs_scan {
>  union ifs_status {
>  	u64	data;
>  	struct {
> -		u32	chunk_num		:8;
> -		u32	chunk_stop_index	:8;
> -		u32	rsvd1			:16;
> +		union {
> +			struct {
> +				u8	chunk_num;
> +				u8	chunk_stop_index;
> +				u16	rsvd1;
> +			} gen0;
> +			struct {
> +				u16	chunk_num;
> +				u16	chunk_stop_index;
> +			} gen2;
> +		};
>  		u32	error_code		:8;
>  		u32	rsvd2			:22;
>  		u32	control_error		:1;
> diff --git a/include/trace/events/intel_ifs.h b/include/trace/events/intel_ifs.h
> index d7353024016c..af0af3f1d9b7 100644
> --- a/include/trace/events/intel_ifs.h
> +++ b/include/trace/events/intel_ifs.h
> @@ -10,25 +10,25 @@
>  
>  TRACE_EVENT(ifs_status,
>  
> -	TP_PROTO(int cpu, union ifs_scan activate, union ifs_status status),
> +	TP_PROTO(int cpu, int start, int stop, u64 status),
>  
> -	TP_ARGS(cpu, activate, status),
> +	TP_ARGS(cpu, start, stop, status),
>  
>  	TP_STRUCT__entry(
>  		__field(	u64,	status	)
>  		__field(	int,	cpu	)
> -		__field(	u8,	start	)
> -		__field(	u8,	stop	)
> +		__field(	u16,	start	)
> +		__field(	u16,	stop	)
>  	),
>  
>  	TP_fast_assign(
>  		__entry->cpu	= cpu;
> -		__entry->start	= activate.start;
> -		__entry->stop	= activate.stop;
> -		__entry->status	= status.data;
> +		__entry->start	= start;
> +		__entry->stop	= stop;
> +		__entry->status	= status;
>  	),
>  
> -	TP_printk("cpu: %d, start: %.2x, stop: %.2x, status: %llx",
> +	TP_printk("cpu: %d, start: %.4x, stop: %.4x, status: %.16llx",
>  		__entry->cpu,
>  		__entry->start,
>  		__entry->stop,
> diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
> index 1061eb7ec399..94d486e5d263 100644
> --- a/drivers/platform/x86/intel/ifs/runtest.c
> +++ b/drivers/platform/x86/intel/ifs/runtest.c
> @@ -171,21 +171,30 @@ static void ifs_test_core(int cpu, struct device *dev)
>  	union ifs_status status;
>  	unsigned long timeout;
>  	struct ifs_data *ifsd;
> +	int to_start, to_stop;
> +	int status_chunk;
>  	u64 msrvals[2];
>  	int retries;
>  
>  	ifsd = ifs_get_data(dev);
>  
> -	activate.rsvd = 0;
>  	activate.delay = IFS_THREAD_WAIT;
>  	activate.sigmce = 0;
> -	activate.start = 0;
> -	activate.stop = ifsd->valid_chunks - 1;
> +	to_start = 0;
> +	to_stop = ifsd->valid_chunks - 1;
> +
> +	if (ifsd->generation) {
> +		activate.gen2.start = to_start;
> +		activate.gen2.stop = to_stop;
> +	} else {
> +		activate.gen0.start = to_start;
> +		activate.gen0.stop = to_stop;
> +	}

Is it okay to not do activate.gen0.rsvd = 0 anymore? If you know it is, it 
would be nice to record that fact into the changelog so that it can be 
found in the history.

>  
>  	timeout = jiffies + HZ / 2;
>  	retries = MAX_IFS_RETRIES;
>  
> -	while (activate.start <= activate.stop) {
> +	while (to_start <= to_stop) {
>  		if (time_after(jiffies, timeout)) {
>  			status.error_code = IFS_SW_TIMEOUT;
>  			break;
> @@ -196,13 +205,14 @@ static void ifs_test_core(int cpu, struct device *dev)
>  
>  		status.data = msrvals[1];
>  
> -		trace_ifs_status(cpu, activate, status);
> +		trace_ifs_status(cpu, to_start, to_stop, status.data);
>  
>  		/* Some cases can be retried, give up for others */
>  		if (!can_restart(status))
>  			break;
>  
> -		if (status.chunk_num == activate.start) {
> +		status_chunk = ifsd->generation ? status.gen2.chunk_num : status.gen0.chunk_num;
> +		if (status_chunk == to_start) {
>  			/* Check for forward progress */
>  			if (--retries == 0) {
>  				if (status.error_code == IFS_NO_ERROR)
> @@ -211,7 +221,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.gen0.start = status_chunk);

The alignment of the second line is still not correct but now I notice how 
the left-hand side is hidden within those expressions. Just do a normal if 
instead so that it is simpler to understand, please.

> +			to_start = status_chunk;
>  		}
>  	}
>  
> 

-- 
 i.

Re: [PATCH v2 4/9] platform/x86/intel/ifs: Gen2 Scan test support

[Joseph, Jithu]

On 9/25/2023 8:39 AM, Ilpo Järvinen wrote:
> On Fri, 22 Sep 2023, Jithu Joseph wrote:
> 

...

>>  
>> -	activate.rsvd = 0;
>>  	activate.delay = IFS_THREAD_WAIT;
>>  	activate.sigmce = 0;
>> -	activate.start = 0;
>> -	activate.stop = ifsd->valid_chunks - 1;
>> +	to_start = 0;
>> +	to_stop = ifsd->valid_chunks - 1;
>> +
>> +	if (ifsd->generation) {
>> +		activate.gen2.start = to_start;
>> +		activate.gen2.stop = to_stop;
>> +	} else {
>> +		activate.gen0.start = to_start;
>> +		activate.gen0.stop = to_stop;
>> +	}
> 
> Is it okay to not do activate.gen0.rsvd = 0 anymore? If you know it is, it 
> would be nice to record that fact into the changelog so that it can be 
> found in the history.

I did test on a gen0 to check if there is a problem due to this (and it seemed fine).
I will make a note in changelog as you suggest

> 
>>  
>>  	timeout = jiffies + HZ / 2;
>>  	retries = MAX_IFS_RETRIES;
>>  
>> -	while (activate.start <= activate.stop) {
>> +	while (to_start <= to_stop) {
>>  		if (time_after(jiffies, timeout)) {
>>  			status.error_code = IFS_SW_TIMEOUT;
>>  			break;
>> @@ -196,13 +205,14 @@ static void ifs_test_core(int cpu, struct device *dev)
>>  
>>  		status.data = msrvals[1];
>>  
>> -		trace_ifs_status(cpu, activate, status);
>> +		trace_ifs_status(cpu, to_start, to_stop, status.data);
>>  
>>  		/* Some cases can be retried, give up for others */
>>  		if (!can_restart(status))
>>  			break;
>>  
>> -		if (status.chunk_num == activate.start) {
>> +		status_chunk = ifsd->generation ? status.gen2.chunk_num : status.gen0.chunk_num;
>> +		if (status_chunk == to_start) {
>>  			/* Check for forward progress */
>>  			if (--retries == 0) {
>>  				if (status.error_code == IFS_NO_ERROR)
>> @@ -211,7 +221,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.gen0.start = status_chunk);
> 
> The alignment of the second line is still not correct but now I notice how 
> the left-hand side is hidden within those expressions. Just do a normal if 
> instead so that it is simpler to understand, please.

In v1 the second line was kept 1 space to the right of previous line.  In v2  I kept
them at the same indent, since your original comment was Misaliged.

I will change these two lines to
if (ifsd->generation)
	activate.gen2.start = status_chunk;
else
	activate.gen0.start = status_chunk


Jithu

Re: [PATCH v2 4/9] platform/x86/intel/ifs: Gen2 Scan test support

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]

On Mon, 25 Sep 2023, Joseph, Jithu wrote:
> On 9/25/2023 8:39 AM, Ilpo Järvinen wrote:
> > On Fri, 22 Sep 2023, Jithu Joseph wrote:
> > 
> 
> ...
> 
> >>  
> >> -	activate.rsvd = 0;
> >>  	activate.delay = IFS_THREAD_WAIT;
> >>  	activate.sigmce = 0;
> >> -	activate.start = 0;
> >> -	activate.stop = ifsd->valid_chunks - 1;
> >> +	to_start = 0;
> >> +	to_stop = ifsd->valid_chunks - 1;
> >> +
> >> +	if (ifsd->generation) {
> >> +		activate.gen2.start = to_start;
> >> +		activate.gen2.stop = to_stop;
> >> +	} else {
> >> +		activate.gen0.start = to_start;
> >> +		activate.gen0.stop = to_stop;
> >> +	}
> > 
> > Is it okay to not do activate.gen0.rsvd = 0 anymore? If you know it is, it 
> > would be nice to record that fact into the changelog so that it can be 
> > found in the history.
> 
> I did test on a gen0 to check if there is a problem due to this (and it seemed fine).
> I will make a note in changelog as you suggest

Actually, I realized activate is a variable in stack and those bits are
uninitilized without that assignment so don't remove it.

-- 
 i.

Re: [PATCH v2 4/9] platform/x86/intel/ifs: Gen2 Scan test support

[Joseph, Jithu]

On 9/26/2023 3:20 AM, Ilpo Järvinen wrote:
> On Mon, 25 Sep 2023, Joseph, Jithu wrote:
>> On 9/25/2023 8:39 AM, Ilpo Järvinen wrote:
>>> On Fri, 22 Sep 2023, Jithu Joseph wrote:
>>>
>>
>> ...
>>
>>>>  
>>>> -	activate.rsvd = 0;
>>>>  	activate.delay = IFS_THREAD_WAIT;
>>>>  	activate.sigmce = 0;
>>>> -	activate.start = 0;
>>>> -	activate.stop = ifsd->valid_chunks - 1;
>>>> +	to_start = 0;
>>>> +	to_stop = ifsd->valid_chunks - 1;
>>>> +
>>>> +	if (ifsd->generation) {
>>>> +		activate.gen2.start = to_start;
>>>> +		activate.gen2.stop = to_stop;
>>>> +	} else {
>>>> +		activate.gen0.start = to_start;
>>>> +		activate.gen0.stop = to_stop;
>>>> +	}
>>>
>>> Is it okay to not do activate.gen0.rsvd = 0 anymore? If you know it is, it 
>>> would be nice to record that fact into the changelog so that it can be 
>>> found in the history.
>>
>> I did test on a gen0 to check if there is a problem due to this (and it seemed fine).
>> I will make a note in changelog as you suggest
> 
> Actually, I realized activate is a variable in stack and those bits are
> uninitilized without that assignment so don't remove it.
> 

Ok, I will post a v3 with all the suggested changes in a few days ... Thanks for the review

Jithu

[PATCH v2 5/9] platform/x86/intel/ifs: Validate image size

[Jithu Joseph]

Perform additional validation prior to loading IFS image.

Error out if the size of the file being loaded doesn't match the size
specified in the header.

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/load.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 6b827247945b..b09106034fac 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -375,6 +375,7 @@ int ifs_load_firmware(struct device *dev)
 {
 	const struct ifs_test_caps *test = ifs_get_test_caps(dev);
 	struct ifs_data *ifsd = ifs_get_data(dev);
+	unsigned int expected_size;
 	const struct firmware *fw;
 	char scan_path[64];
 	int ret = -EINVAL;
@@ -389,6 +390,13 @@ int ifs_load_firmware(struct device *dev)
 		goto done;
 	}
 
+	expected_size = ((struct microcode_header_intel *)fw->data)->totalsize;
+	if (fw->size != expected_size) {
+		dev_err(dev, "File size mismatch (expected %d, actual %ld). Corrupted IFS image.\n",
+			expected_size, fw->size);
+		return -EBADFD;
+	}
+
 	ret = image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
 	if (ret)
 		goto release;
-- 
2.25.1

Re: [PATCH v2 5/9] platform/x86/intel/ifs: Validate image size

[Ilpo Järvinen]

On Fri, 22 Sep 2023, Jithu Joseph wrote:

> Perform additional validation prior to loading IFS image.
> 
> Error out if the size of the file being loaded doesn't match the size
> specified in the header.
> 
> 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/load.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index 6b827247945b..b09106034fac 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -375,6 +375,7 @@ int ifs_load_firmware(struct device *dev)
>  {
>  	const struct ifs_test_caps *test = ifs_get_test_caps(dev);
>  	struct ifs_data *ifsd = ifs_get_data(dev);
> +	unsigned int expected_size;
>  	const struct firmware *fw;
>  	char scan_path[64];
>  	int ret = -EINVAL;
> @@ -389,6 +390,13 @@ int ifs_load_firmware(struct device *dev)
>  		goto done;
>  	}
>  
> +	expected_size = ((struct microcode_header_intel *)fw->data)->totalsize;
> +	if (fw->size != expected_size) {
> +		dev_err(dev, "File size mismatch (expected %d, actual %ld). Corrupted IFS image.\n",
> +			expected_size, fw->size);
> +		return -EBADFD;

I don't think this error code is best suited for what occurred. I guess 
returning just -EINVAL would be fine.

-- 
 i.

> +	}
> +
>  	ret = image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
>  	if (ret)
>  		goto release;
> 

Re: [PATCH v2 5/9] platform/x86/intel/ifs: Validate image size

[Joseph, Jithu]

On 9/25/2023 8:43 AM, Ilpo Järvinen wrote:
> On Fri, 22 Sep 2023, Jithu Joseph wrote:
> 
>> Perform additional validation prior to loading IFS image.
>>
>> Error out if the size of the file being loaded doesn't match the size
>> specified in the header.
>>
>> 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/load.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
>> index 6b827247945b..b09106034fac 100644
>> --- a/drivers/platform/x86/intel/ifs/load.c
>> +++ b/drivers/platform/x86/intel/ifs/load.c
>> @@ -375,6 +375,7 @@ int ifs_load_firmware(struct device *dev)
>>  {
>>  	const struct ifs_test_caps *test = ifs_get_test_caps(dev);
>>  	struct ifs_data *ifsd = ifs_get_data(dev);
>> +	unsigned int expected_size;
>>  	const struct firmware *fw;
>>  	char scan_path[64];
>>  	int ret = -EINVAL;
>> @@ -389,6 +390,13 @@ int ifs_load_firmware(struct device *dev)
>>  		goto done;
>>  	}
>>  
>> +	expected_size = ((struct microcode_header_intel *)fw->data)->totalsize;
>> +	if (fw->size != expected_size) {
>> +		dev_err(dev, "File size mismatch (expected %d, actual %ld). Corrupted IFS image.\n",
>> +			expected_size, fw->size);
>> +		return -EBADFD;
> 
> I don't think this error code is best suited for what occurred. I guess 
> returning just -EINVAL would be fine.

Will change

Jithu

[PATCH v2 6/9] platform/x86/intel/ifs: Metadata validation for start_chunk

[Jithu Joseph]

Add an additional check to validate IFS image metadata field prior to
loading the test image.

If start_chunk is not a multiple of chunks_per_stride error out.

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/load.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index b09106034fac..c92d313b921f 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -291,6 +291,13 @@ static int validate_ifs_metadata(struct device *dev)
 		return ret;
 	}
 
+	if (ifs_meta->chunks_per_stride != 0 &&
+	    (ifs_meta->starting_chunk % ifs_meta->chunks_per_stride)) {
+		dev_warn(dev, "Starting chunk num %d not a multiple of chunks_per_stride %d\n",
+			 ifs_meta->starting_chunk, ifs_meta->chunks_per_stride);
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH v2 6/9] platform/x86/intel/ifs: Metadata validation for start_chunk

[Ilpo Järvinen]

On Fri, 22 Sep 2023, Jithu Joseph wrote:

> Add an additional check to validate IFS image metadata field prior to
> loading the test image.
> 
> If start_chunk is not a multiple of chunks_per_stride error out.
> 
> 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/load.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index b09106034fac..c92d313b921f 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -291,6 +291,13 @@ static int validate_ifs_metadata(struct device *dev)
>  		return ret;
>  	}
>  
> +	if (ifs_meta->chunks_per_stride != 0 &&
> +	    (ifs_meta->starting_chunk % ifs_meta->chunks_per_stride)) {

I meant that != 0 should be on the second line.

-- 
 i.

> +		dev_warn(dev, "Starting chunk num %d not a multiple of chunks_per_stride %d\n",
> +			 ifs_meta->starting_chunk, ifs_meta->chunks_per_stride);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> 

Re: [PATCH v2 6/9] platform/x86/intel/ifs: Metadata validation for start_chunk

[Joseph, Jithu]

On 9/25/2023 8:45 AM, Ilpo Järvinen wrote:
> On Fri, 22 Sep 2023, Jithu Joseph wrote:
> 
>> Add an additional check to validate IFS image metadata field prior to
>> loading the test image.
>>
>> If start_chunk is not a multiple of chunks_per_stride error out.
>>
>> 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/load.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
>> index b09106034fac..c92d313b921f 100644
>> --- a/drivers/platform/x86/intel/ifs/load.c
>> +++ b/drivers/platform/x86/intel/ifs/load.c
>> @@ -291,6 +291,13 @@ static int validate_ifs_metadata(struct device *dev)
>>  		return ret;
>>  	}
>>  
>> +	if (ifs_meta->chunks_per_stride != 0 &&
>> +	    (ifs_meta->starting_chunk % ifs_meta->chunks_per_stride)) {
> 
> I meant that != 0 should be on the second line.
>

Ah I see ... Will change

Jithu

[PATCH v2 7/9] platform/x86/intel/ifs: Add new CPU support

[Jithu Joseph]

Add Granite Rapids(GNR) and Sierra Forest(SRF) cpuids to x86 match table
so that IFS driver can be loaded for those.

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/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 4ff2aa4b484b..0c8927916373 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -18,6 +18,9 @@
 static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
 	X86_MATCH(SAPPHIRERAPIDS_X),
 	X86_MATCH(EMERALDRAPIDS_X),
+	X86_MATCH(GRANITERAPIDS_X),
+	X86_MATCH(GRANITERAPIDS_D),
+	X86_MATCH(ATOM_CRESTMONT_X),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
-- 
2.25.1

Re: [PATCH v2 7/9] platform/x86/intel/ifs: Add new CPU support

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

On Fri, 22 Sep 2023, Jithu Joseph wrote:

> Add Granite Rapids(GNR) and Sierra Forest(SRF) cpuids to x86 match table
> so that IFS driver can be loaded for those.
> 
> 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/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 4ff2aa4b484b..0c8927916373 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -18,6 +18,9 @@
>  static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
>  	X86_MATCH(SAPPHIRERAPIDS_X),
>  	X86_MATCH(EMERALDRAPIDS_X),
> +	X86_MATCH(GRANITERAPIDS_X),
> +	X86_MATCH(GRANITERAPIDS_D),
> +	X86_MATCH(ATOM_CRESTMONT_X),
>  	{}
>  };
>  MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v2 8/9] platform/x86/intel/ifs: Add new error code

[Jithu Joseph]

Make driver aware of a newly added error code so that it can provide a
more appropriate error message.

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/runtest.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 94d486e5d263..a29f7b8fa48e 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -40,6 +40,8 @@ enum ifs_status_err_code {
 	IFS_UNASSIGNED_ERROR_CODE		= 7,
 	IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT	= 8,
 	IFS_INTERRUPTED_DURING_EXECUTION	= 9,
+	IFS_UNASSIGNED_ERROR_CODE_0xA		= 0xA,
+	IFS_CORRUPTED_CHUNK		= 0xB,
 };
 
 static const char * const scan_test_status[] = {
@@ -55,6 +57,8 @@ static const char * const scan_test_status[] = {
 	[IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT] =
 	"Exceeded number of Logical Processors (LP) allowed to run Scan-At-Field concurrently",
 	[IFS_INTERRUPTED_DURING_EXECUTION] = "Interrupt occurred prior to SCAN start",
+	[IFS_UNASSIGNED_ERROR_CODE_0xA] = "Unassigned error code 0xA",
+	[IFS_CORRUPTED_CHUNK] = "Scan operation aborted due to corrupted image. Try reloading",
 };
 
 static void message_not_tested(struct device *dev, int cpu, union ifs_status status)
@@ -123,6 +127,8 @@ static bool can_restart(union ifs_status status)
 	case IFS_MISMATCH_ARGUMENTS_BETWEEN_THREADS:
 	case IFS_CORE_NOT_CAPABLE_CURRENTLY:
 	case IFS_UNASSIGNED_ERROR_CODE:
+	case IFS_UNASSIGNED_ERROR_CODE_0xA:
+	case IFS_CORRUPTED_CHUNK:
 		break;
 	}
 	return false;
-- 
2.25.1

Re: [PATCH v2 8/9] platform/x86/intel/ifs: Add new error code

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]

On Fri, 22 Sep 2023, Jithu Joseph wrote:

> Make driver aware of a newly added error code so that it can provide a
> more appropriate error message.
> 
> 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/runtest.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
> index 94d486e5d263..a29f7b8fa48e 100644
> --- a/drivers/platform/x86/intel/ifs/runtest.c
> +++ b/drivers/platform/x86/intel/ifs/runtest.c
> @@ -40,6 +40,8 @@ enum ifs_status_err_code {
>  	IFS_UNASSIGNED_ERROR_CODE		= 7,
>  	IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT	= 8,
>  	IFS_INTERRUPTED_DURING_EXECUTION	= 9,
> +	IFS_UNASSIGNED_ERROR_CODE_0xA		= 0xA,
> +	IFS_CORRUPTED_CHUNK		= 0xB,
>  };
>  
>  static const char * const scan_test_status[] = {
> @@ -55,6 +57,8 @@ static const char * const scan_test_status[] = {
>  	[IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT] =
>  	"Exceeded number of Logical Processors (LP) allowed to run Scan-At-Field concurrently",
>  	[IFS_INTERRUPTED_DURING_EXECUTION] = "Interrupt occurred prior to SCAN start",
> +	[IFS_UNASSIGNED_ERROR_CODE_0xA] = "Unassigned error code 0xA",
> +	[IFS_CORRUPTED_CHUNK] = "Scan operation aborted due to corrupted image. Try reloading",
>  };
>  
>  static void message_not_tested(struct device *dev, int cpu, union ifs_status status)
> @@ -123,6 +127,8 @@ static bool can_restart(union ifs_status status)
>  	case IFS_MISMATCH_ARGUMENTS_BETWEEN_THREADS:
>  	case IFS_CORE_NOT_CAPABLE_CURRENTLY:
>  	case IFS_UNASSIGNED_ERROR_CODE:
> +	case IFS_UNASSIGNED_ERROR_CODE_0xA:
> +	case IFS_CORRUPTED_CHUNK:
>  		break;
>  	}
>  	return false;
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v2 9/9] platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

[Jithu Joseph]

Array BIST MSR addresses, bit definition and semantics are different for
Sierra Forest. Branch into a separate Array BIST flow on Sierra Forest
when user invokes Array Test.

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     |  4 +++
 drivers/platform/x86/intel/ifs/core.c    | 15 +++++-----
 drivers/platform/x86/intel/ifs/runtest.c | 37 +++++++++++++++++++++++-
 3 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index cd213b89d278..73268f5dcff1 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -137,6 +137,8 @@
 #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
 #define MSR_ACTIVATE_SCAN			0x000002c6
 #define MSR_SCAN_STATUS				0x000002c7
+#define MSR_ARRAY_TRIGGER			0x000002d6
+#define MSR_ARRAY_STATUS			0x000002d7
 #define MSR_SAF_CTRL				0x000004f0
 
 #define SCAN_NOT_TESTED				0
@@ -272,6 +274,7 @@ struct ifs_test_caps {
  * @cur_batch: number indicating the currently loaded test file
  * @generation: IFS test generation enumerated by hardware
  * @chunk_size: size of a test chunk
+ * @array_gen: test generation of array test
  */
 struct ifs_data {
 	int	loaded_version;
@@ -283,6 +286,7 @@ struct ifs_data {
 	u32	cur_batch;
 	u32	generation;
 	u32	chunk_size;
+	u32	array_gen;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 0c8927916373..934eaf348f9d 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -11,16 +11,16 @@
 
 #include "ifs.h"
 
-#define X86_MATCH(model)				\
+#define X86_MATCH(model, array_gen)				\
 	X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6,	\
-		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL)
+		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, array_gen)
 
 static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
-	X86_MATCH(SAPPHIRERAPIDS_X),
-	X86_MATCH(EMERALDRAPIDS_X),
-	X86_MATCH(GRANITERAPIDS_X),
-	X86_MATCH(GRANITERAPIDS_D),
-	X86_MATCH(ATOM_CRESTMONT_X),
+	X86_MATCH(SAPPHIRERAPIDS_X, 0),
+	X86_MATCH(EMERALDRAPIDS_X, 0),
+	X86_MATCH(GRANITERAPIDS_X, 0),
+	X86_MATCH(GRANITERAPIDS_D, 0),
+	X86_MATCH(ATOM_CRESTMONT_X, 1),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
@@ -100,6 +100,7 @@ static int __init ifs_init(void)
 			continue;
 		ifs_devices[i].rw_data.generation = FIELD_GET(MSR_INTEGRITY_CAPS_SAF_GEN_MASK,
 							      msrval);
+		ifs_devices[i].rw_data.array_gen = (u32)m->driver_data;
 		ret = misc_register(&ifs_devices[i].misc);
 		if (ret)
 			goto err_exit;
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index a29f7b8fa48e..bf3310bedcdc 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -326,6 +326,38 @@ static void ifs_array_test_core(int cpu, struct device *dev)
 		ifsd->status = SCAN_TEST_PASS;
 }
 
+#define ARRAY_GEN1_TEST_ALL_ARRAYS	0x0ULL
+#define ARRAY_GEN1_STATUS_FAIL		0x1ULL
+
+static int do_array_test_gen1(void *status)
+{
+	int cpu = smp_processor_id();
+	int first;
+
+	first = cpumask_first(cpu_smt_mask(cpu));
+
+	if (cpu == first) {
+		wrmsrl(MSR_ARRAY_TRIGGER, ARRAY_GEN1_TEST_ALL_ARRAYS);
+		rdmsrl(MSR_ARRAY_STATUS, *((u64 *)status));
+	}
+
+	return 0;
+}
+
+static void ifs_array_test_gen1(int cpu, struct device *dev)
+{
+	struct ifs_data *ifsd = ifs_get_data(dev);
+	u64 status = 0;
+
+	stop_core_cpuslocked(cpu, do_array_test_gen1, &status);
+	ifsd->scan_details = status;
+
+	if (status & ARRAY_GEN1_STATUS_FAIL)
+		ifsd->status = SCAN_TEST_FAIL;
+	else
+		ifsd->status = SCAN_TEST_PASS;
+}
+
 /*
  * Initiate per core test. It wakes up work queue threads on the target cpu and
  * its sibling cpu. Once all sibling threads wake up, the scan test gets executed and
@@ -353,7 +385,10 @@ int do_core_test(int cpu, struct device *dev)
 		ifs_test_core(cpu, dev);
 		break;
 	case IFS_TYPE_ARRAY_BIST:
-		ifs_array_test_core(cpu, dev);
+		if (ifsd->array_gen == 0)
+			ifs_array_test_core(cpu, dev);
+		else
+			ifs_array_test_gen1(cpu, dev);
 		break;
 	default:
 		return -EINVAL;
-- 
2.25.1

[PATCH v3 0/9] IFS support for GNR and SRF

[Jithu Joseph]

Changes in v3
   Ilpo Jarvinen
    - Added Reviewed-by tags wherever provided
    - In function validate_ifs_metadata() (patch 6)
        - Add != 0 to next line for clarity
    - In function ifs_load_firmware() (patch 5)
        - return -EINVAL instead of -BADFD
    - In function ifs_test_core() (patch 4)
        - initialize activate.gen0.rsvd = 0
        - use if instead of conditional operator
    - alignment change in ifs_scan_hashes_status_gen2 (patch 3)

v2 submission
Link: https://lore.kernel.org/lkml/20230922232606.1928026-1-jithu.joseph@intel.com/

Changes in v2
   Ilpo Jarvinen
    - Use GENMASK_ULL() / FIELD_GET() for bitops (patch 01)
    - Avoid mixing u8 type and bitfields in certain MSR structure
       scenarios (patch03 also suggested by Dave Hansen)
    - Expand bitfield structures to use consistent genx naming (patch 04)
    - Replace goto with do / while (patch 03)
    - general formatting (multiple patches)
        - remove un-necessary parenthesis
        - reformat commit message to use whole allowed 72 columns
        - alignment changes
    Other change
     - fold v1 04/10 and 05/10 into v2 patch 04/09 to satisfy build
        constraints due to consistent genx naming

v1 submission:
Link: https://lore.kernel.org/lkml/20230913183348.1349409-1-jithu.joseph@intel.com/

This series adds In Field Scan(IFS) support for newer CPUs like Granite
Rapids(GNR) and Sierra Forest(SRF).

There are changes in the IFS image loading and test flow to support
these new CPUs.

Note to reviewers:
 - patch 1/9 adds a bit mask to arch/x86/.../msr-index.h,
  hence x86 maintainers are cc-d.
 - patch 4/9 modifies an existing tracepoint, cc Steven Rostedt
 - Rest are localized to IFS driver

Jithu Joseph (9):
  platform/x86/intel/ifs: Store IFS generation number
  platform/x86/intel/ifs: Refactor image loading code
  platform/x86/intel/ifs: Gen2 scan image loading
  platform/x86/intel/ifs: Gen2 Scan test support
  platform/x86/intel/ifs: Validate image size
  platform/x86/intel/ifs: Metadata validation for start_chunk
  platform/x86/intel/ifs: Add new CPU support
  platform/x86/intel/ifs: Add new error code
  platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

 arch/x86/include/asm/msr-index.h         |   1 +
 drivers/platform/x86/intel/ifs/ifs.h     |  61 ++++++++-
 include/trace/events/intel_ifs.h         |  16 +--
 drivers/platform/x86/intel/ifs/core.c    |  15 ++-
 drivers/platform/x86/intel/ifs/load.c    | 158 +++++++++++++++++++++--
 drivers/platform/x86/intel/ifs/runtest.c |  72 +++++++++--
 6 files changed, 283 insertions(+), 40 deletions(-)


base-commit: 6465e260f48790807eef06b583b38ca9789b6072
-- 
2.25.1

[PATCH v3 1/9] platform/x86/intel/ifs: Store IFS generation number

[Jithu Joseph]

IFS generation number is reported via MSR_INTEGRITY_CAPS.  As IFS
support gets added to newer CPUs, some differences are expected during
IFS image loading and test flows.

Define MSR bitmasks to extract and store the generation in driver data,
so that driver can modify its MSR interaction appropriately.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
 arch/x86/include/asm/msr-index.h      | 1 +
 drivers/platform/x86/intel/ifs/ifs.h  | 2 ++
 drivers/platform/x86/intel/ifs/core.c | 3 +++
 3 files changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 1d111350197f..838e5a013a07 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -222,6 +222,7 @@
 #define MSR_INTEGRITY_CAPS_ARRAY_BIST          BIT(MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT)
 #define MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT	4
 #define MSR_INTEGRITY_CAPS_PERIODIC_BIST	BIT(MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT)
+#define MSR_INTEGRITY_CAPS_SAF_GEN_MASK	GENMASK_ULL(10, 9)
 
 #define MSR_LBR_NHM_FROM		0x00000680
 #define MSR_LBR_NHM_TO			0x000006c0
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 93191855890f..d666aeed20fc 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -229,6 +229,7 @@ struct ifs_test_caps {
  * @status: it holds simple status pass/fail/untested
  * @scan_details: opaque scan status code from h/w
  * @cur_batch: number indicating the currently loaded test file
+ * @generation: IFS test generation enumerated by hardware
  */
 struct ifs_data {
 	int	loaded_version;
@@ -238,6 +239,7 @@ struct ifs_data {
 	int	status;
 	u64	scan_details;
 	u32	cur_batch;
+	u32	generation;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 306f886b52d2..4ff2aa4b484b 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2022 Intel Corporation. */
 
+#include <linux/bitfield.h>
 #include <linux/module.h>
 #include <linux/kdev_t.h>
 #include <linux/semaphore.h>
@@ -94,6 +95,8 @@ static int __init ifs_init(void)
 	for (i = 0; i < IFS_NUMTESTS; i++) {
 		if (!(msrval & BIT(ifs_devices[i].test_caps->integrity_cap_bit)))
 			continue;
+		ifs_devices[i].rw_data.generation = FIELD_GET(MSR_INTEGRITY_CAPS_SAF_GEN_MASK,
+							      msrval);
 		ret = misc_register(&ifs_devices[i].misc);
 		if (ret)
 			goto err_exit;
-- 
2.25.1

[PATCH v3 2/9] platform/x86/intel/ifs: Refactor image loading code

[Jithu Joseph]

IFS image loading flow is slightly different for newer IFS generations.

In preparation for adding support for newer IFS generations, refactor
portions of existing image loading code for reuse.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
 drivers/platform/x86/intel/ifs/load.c | 31 ++++++++++++++++-----------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index cefd0d886cfd..851c97cc6a6b 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -80,6 +80,23 @@ static struct metadata_header *find_meta_data(void *ucode, unsigned int meta_typ
 	return NULL;
 }
 
+static void hashcopy_err_message(struct device *dev, u32 err_code)
+{
+	if (err_code >= ARRAY_SIZE(scan_hash_status))
+		dev_err(dev, "invalid error code 0x%x for hash copy\n", err_code);
+	else
+		dev_err(dev, "Hash copy error : %s\n", scan_hash_status[err_code]);
+}
+
+static void auth_err_message(struct device *dev, u32 err_code)
+{
+	if (err_code >= ARRAY_SIZE(scan_authentication_status))
+		dev_err(dev, "invalid error code 0x%x for authentication\n", err_code);
+	else
+		dev_err(dev, "Chunk authentication error : %s\n",
+			scan_authentication_status[err_code]);
+}
+
 /*
  * To copy scan hashes and authenticate test chunks, the initiating cpu must point
  * to the EDX:EAX to the test image in linear address.
@@ -109,11 +126,7 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
 
 	if (!hashes_status.valid) {
 		ifsd->loading_error = true;
-		if (err_code >= ARRAY_SIZE(scan_hash_status)) {
-			dev_err(dev, "invalid error code 0x%x for hash copy\n", err_code);
-			goto done;
-		}
-		dev_err(dev, "Hash copy error : %s", scan_hash_status[err_code]);
+		hashcopy_err_message(dev, err_code);
 		goto done;
 	}
 
@@ -133,13 +146,7 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
 
 		if (err_code) {
 			ifsd->loading_error = true;
-			if (err_code >= ARRAY_SIZE(scan_authentication_status)) {
-				dev_err(dev,
-					"invalid error code 0x%x for authentication\n", err_code);
-				goto done;
-			}
-			dev_err(dev, "Chunk authentication error %s\n",
-				scan_authentication_status[err_code]);
+			auth_err_message(dev, err_code);
 			goto done;
 		}
 	}
-- 
2.25.1

[PATCH v3 3/9] platform/x86/intel/ifs: Gen2 scan image loading

[Jithu Joseph]

Scan image loading flow for newer IFS generations are slightly different
from that of current generation. In newer schemes, loading need not be
done once for each socket as was done in gen0.

Also the width of NUM_CHUNKS bitfield in SCAN_HASHES_STATUS MSR has
increased from 8 -> 16 bits. Similarly there are width differences for
CHUNK_AUTHENTICATION_STATUS too.

Further the parameter to AUTHENTICATE_AND_COPY_CHUNK is passed
differently in newer generations.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
 drivers/platform/x86/intel/ifs/ifs.h  |  27 +++++++
 drivers/platform/x86/intel/ifs/load.c | 112 +++++++++++++++++++++++++-
 2 files changed, 137 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index d666aeed20fc..4824316b3acd 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -137,6 +137,8 @@
 #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
 #define MSR_ACTIVATE_SCAN			0x000002c6
 #define MSR_SCAN_STATUS				0x000002c7
+#define MSR_SAF_CTRL				0x000004f0
+
 #define SCAN_NOT_TESTED				0
 #define SCAN_TEST_PASS				1
 #define SCAN_TEST_FAIL				2
@@ -158,6 +160,19 @@ union ifs_scan_hashes_status {
 	};
 };
 
+union ifs_scan_hashes_status_gen2 {
+	u64	data;
+	struct {
+		u16	chunk_size;
+		u16	num_chunks;
+		u32	error_code		:8;
+		u32	chunks_in_stride	:9;
+		u32	rsvd			:2;
+		u32	max_core_limit		:12;
+		u32	valid			:1;
+	};
+};
+
 /* MSR_CHUNKS_AUTH_STATUS bit fields */
 union ifs_chunks_auth_status {
 	u64	data;
@@ -170,6 +185,16 @@ union ifs_chunks_auth_status {
 	};
 };
 
+union ifs_chunks_auth_status_gen2 {
+	u64	data;
+	struct {
+		u16	valid_chunks;
+		u16	total_chunks;
+		u32	error_code	:8;
+		u32	rsvd2		:24;
+	};
+};
+
 /* MSR_ACTIVATE_SCAN bit fields */
 union ifs_scan {
 	u64	data;
@@ -230,6 +255,7 @@ struct ifs_test_caps {
  * @scan_details: opaque scan status code from h/w
  * @cur_batch: number indicating the currently loaded test file
  * @generation: IFS test generation enumerated by hardware
+ * @chunk_size: size of a test chunk
  */
 struct ifs_data {
 	int	loaded_version;
@@ -240,6 +266,7 @@ struct ifs_data {
 	u64	scan_details;
 	u32	cur_batch;
 	u32	generation;
+	u32	chunk_size;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 851c97cc6a6b..6b827247945b 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -2,6 +2,7 @@
 /* Copyright(c) 2022 Intel Corporation. */
 
 #include <linux/firmware.h>
+#include <linux/sizes.h>
 #include <asm/cpu.h>
 #include <asm/microcode.h>
 
@@ -26,6 +27,11 @@ union meta_data {
 
 #define IFS_HEADER_SIZE	(sizeof(struct microcode_header_intel))
 #define META_TYPE_IFS	1
+#define INVALIDATE_STRIDE	0x1UL
+#define IFS_GEN_STRIDE_AWARE	2
+#define AUTH_INTERRUPTED_ERROR	5
+#define IFS_AUTH_RETRY_CT	10
+
 static  struct microcode_header_intel *ifs_header_ptr;	/* pointer to the ifs image header */
 static u64 ifs_hash_ptr;			/* Address of ifs metadata (hash) */
 static u64 ifs_test_image_ptr;			/* 256B aligned address of test pattern */
@@ -44,7 +50,10 @@ static const char * const scan_hash_status[] = {
 static const char * const scan_authentication_status[] = {
 	[0] = "No error reported",
 	[1] = "Attempt to authenticate a chunk which is already marked as authentic",
-	[2] = "Chunk authentication error. The hash of chunk did not match expected value"
+	[2] = "Chunk authentication error. The hash of chunk did not match expected value",
+	[3] = "Reserved",
+	[4] = "Chunk outside the current stride",
+	[5] = "Authentication flow interrupted",
 };
 
 #define MC_HEADER_META_TYPE_END		(0)
@@ -154,6 +163,102 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
 	complete(&ifs_done);
 }
 
+static int get_num_chunks(int gen, union ifs_scan_hashes_status_gen2 status)
+{
+	return gen >= IFS_GEN_STRIDE_AWARE ? status.chunks_in_stride : status.num_chunks;
+}
+
+static bool need_copy_scan_hashes(struct ifs_data *ifsd)
+{
+	return !ifsd->loaded ||
+		ifsd->generation < IFS_GEN_STRIDE_AWARE ||
+		ifsd->loaded_version != ifs_header_ptr->rev;
+}
+
+static int copy_hashes_authenticate_chunks_gen2(struct device *dev)
+{
+	union ifs_scan_hashes_status_gen2 hashes_status;
+	union ifs_chunks_auth_status_gen2 chunk_status;
+	u32 err_code, valid_chunks, total_chunks;
+	int i, num_chunks, chunk_size;
+	union meta_data *ifs_meta;
+	int starting_chunk_nr;
+	struct ifs_data *ifsd;
+	u64 linear_addr, base;
+	u64 chunk_table[2];
+	int retry_count;
+
+	ifsd = ifs_get_data(dev);
+
+	if (need_copy_scan_hashes(ifsd)) {
+		wrmsrl(MSR_COPY_SCAN_HASHES, ifs_hash_ptr);
+		rdmsrl(MSR_SCAN_HASHES_STATUS, hashes_status.data);
+
+		/* enumerate the scan image information */
+		chunk_size = hashes_status.chunk_size * SZ_1K;
+		err_code = hashes_status.error_code;
+
+		num_chunks = get_num_chunks(ifsd->generation, hashes_status);
+
+		if (!hashes_status.valid) {
+			hashcopy_err_message(dev, err_code);
+			return -EIO;
+		}
+		ifsd->loaded_version = ifs_header_ptr->rev;
+		ifsd->chunk_size = chunk_size;
+	} else {
+		num_chunks = ifsd->valid_chunks;
+		chunk_size = ifsd->chunk_size;
+	}
+
+	if (ifsd->generation >= IFS_GEN_STRIDE_AWARE) {
+		wrmsrl(MSR_SAF_CTRL, INVALIDATE_STRIDE);
+		rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
+		if (chunk_status.valid_chunks != 0) {
+			dev_err(dev, "Couldn't invalidate installed stride - %d\n",
+				chunk_status.valid_chunks);
+			return -EIO;
+		}
+	}
+
+	base = ifs_test_image_ptr;
+	ifs_meta = (union meta_data *)find_meta_data(ifs_header_ptr, META_TYPE_IFS);
+	starting_chunk_nr = ifs_meta->starting_chunk;
+
+	/* scan data authentication and copy chunks to secured memory */
+	for (i = 0; i < num_chunks; i++) {
+		retry_count = IFS_AUTH_RETRY_CT;
+		linear_addr = base + i * chunk_size;
+
+		chunk_table[0] = starting_chunk_nr + i;
+		chunk_table[1] = linear_addr;
+		do {
+			wrmsrl(MSR_AUTHENTICATE_AND_COPY_CHUNK, (u64)chunk_table);
+			rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
+			err_code = chunk_status.error_code;
+		} while (err_code == AUTH_INTERRUPTED_ERROR && --retry_count);
+
+		if (err_code) {
+			ifsd->loading_error = true;
+			auth_err_message(dev, err_code);
+			return -EIO;
+		}
+	}
+
+	valid_chunks = chunk_status.valid_chunks;
+	total_chunks = chunk_status.total_chunks;
+
+	if (valid_chunks != total_chunks) {
+		ifsd->loading_error = true;
+		dev_err(dev, "Couldn't authenticate all the chunks. Authenticated %d total %d.\n",
+			valid_chunks, total_chunks);
+		return -EIO;
+	}
+	ifsd->valid_chunks = valid_chunks;
+
+	return 0;
+}
+
 static int validate_ifs_metadata(struct device *dev)
 {
 	struct ifs_data *ifsd = ifs_get_data(dev);
@@ -206,7 +311,9 @@ static int scan_chunks_sanity_check(struct device *dev)
 		return ret;
 
 	ifsd->loading_error = false;
-	ifsd->loaded_version = ifs_header_ptr->rev;
+
+	if (ifsd->generation > 0)
+		return copy_hashes_authenticate_chunks_gen2(dev);
 
 	/* copy the scan hash and authenticate per package */
 	cpus_read_lock();
@@ -226,6 +333,7 @@ static int scan_chunks_sanity_check(struct device *dev)
 		ifs_pkg_auth[curr_pkg] = 1;
 	}
 	ret = 0;
+	ifsd->loaded_version = ifs_header_ptr->rev;
 out:
 	cpus_read_unlock();
 
-- 
2.25.1

[PATCH v3 4/9] platform/x86/intel/ifs: Gen2 Scan test support

[Jithu Joseph]

Width of chunk related bitfields is ACTIVATE_SCAN and SCAN_STATUS MSRs
are different in newer IFS generation compared to gen0.

Make changes to scan test flow such that MSRs are populated
appropriately based on the generation supported by hardware.

Account for the 8/16 bit MSR bitfield width differences between gen0 and
newer generations for the scan test trace event too.

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     | 28 ++++++++++++++++++-----
 include/trace/events/intel_ifs.h         | 16 ++++++-------
 drivers/platform/x86/intel/ifs/runtest.c | 29 ++++++++++++++++++------
 3 files changed, 52 insertions(+), 21 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 4824316b3acd..f0dd849b3400 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -199,9 +199,17 @@ union ifs_chunks_auth_status_gen2 {
 union ifs_scan {
 	u64	data;
 	struct {
-		u32	start	:8;
-		u32	stop	:8;
-		u32	rsvd	:16;
+		union {
+			struct {
+				u8	start;
+				u8	stop;
+				u16	rsvd;
+			} gen0;
+			struct {
+				u16	start;
+				u16	stop;
+			} gen2;
+		};
 		u32	delay	:31;
 		u32	sigmce	:1;
 	};
@@ -211,9 +219,17 @@ union ifs_scan {
 union ifs_status {
 	u64	data;
 	struct {
-		u32	chunk_num		:8;
-		u32	chunk_stop_index	:8;
-		u32	rsvd1			:16;
+		union {
+			struct {
+				u8	chunk_num;
+				u8	chunk_stop_index;
+				u16	rsvd1;
+			} gen0;
+			struct {
+				u16	chunk_num;
+				u16	chunk_stop_index;
+			} gen2;
+		};
 		u32	error_code		:8;
 		u32	rsvd2			:22;
 		u32	control_error		:1;
diff --git a/include/trace/events/intel_ifs.h b/include/trace/events/intel_ifs.h
index d7353024016c..af0af3f1d9b7 100644
--- a/include/trace/events/intel_ifs.h
+++ b/include/trace/events/intel_ifs.h
@@ -10,25 +10,25 @@
 
 TRACE_EVENT(ifs_status,
 
-	TP_PROTO(int cpu, union ifs_scan activate, union ifs_status status),
+	TP_PROTO(int cpu, int start, int stop, u64 status),
 
-	TP_ARGS(cpu, activate, status),
+	TP_ARGS(cpu, start, stop, status),
 
 	TP_STRUCT__entry(
 		__field(	u64,	status	)
 		__field(	int,	cpu	)
-		__field(	u8,	start	)
-		__field(	u8,	stop	)
+		__field(	u16,	start	)
+		__field(	u16,	stop	)
 	),
 
 	TP_fast_assign(
 		__entry->cpu	= cpu;
-		__entry->start	= activate.start;
-		__entry->stop	= activate.stop;
-		__entry->status	= status.data;
+		__entry->start	= start;
+		__entry->stop	= stop;
+		__entry->status	= status;
 	),
 
-	TP_printk("cpu: %d, start: %.2x, stop: %.2x, status: %llx",
+	TP_printk("cpu: %d, start: %.4x, stop: %.4x, status: %.16llx",
 		__entry->cpu,
 		__entry->start,
 		__entry->stop,
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 1061eb7ec399..925c30c79011 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -171,21 +171,31 @@ static void ifs_test_core(int cpu, struct device *dev)
 	union ifs_status status;
 	unsigned long timeout;
 	struct ifs_data *ifsd;
+	int to_start, to_stop;
+	int status_chunk;
 	u64 msrvals[2];
 	int retries;
 
 	ifsd = ifs_get_data(dev);
 
-	activate.rsvd = 0;
+	activate.gen0.rsvd = 0;
 	activate.delay = IFS_THREAD_WAIT;
 	activate.sigmce = 0;
-	activate.start = 0;
-	activate.stop = ifsd->valid_chunks - 1;
+	to_start = 0;
+	to_stop = ifsd->valid_chunks - 1;
+
+	if (ifsd->generation) {
+		activate.gen2.start = to_start;
+		activate.gen2.stop = to_stop;
+	} else {
+		activate.gen0.start = to_start;
+		activate.gen0.stop = to_stop;
+	}
 
 	timeout = jiffies + HZ / 2;
 	retries = MAX_IFS_RETRIES;
 
-	while (activate.start <= activate.stop) {
+	while (to_start <= to_stop) {
 		if (time_after(jiffies, timeout)) {
 			status.error_code = IFS_SW_TIMEOUT;
 			break;
@@ -196,13 +206,14 @@ static void ifs_test_core(int cpu, struct device *dev)
 
 		status.data = msrvals[1];
 
-		trace_ifs_status(cpu, activate, status);
+		trace_ifs_status(cpu, to_start, to_stop, status.data);
 
 		/* Some cases can be retried, give up for others */
 		if (!can_restart(status))
 			break;
 
-		if (status.chunk_num == activate.start) {
+		status_chunk = ifsd->generation ? status.gen2.chunk_num : status.gen0.chunk_num;
+		if (status_chunk == to_start) {
 			/* Check for forward progress */
 			if (--retries == 0) {
 				if (status.error_code == IFS_NO_ERROR)
@@ -211,7 +222,11 @@ static void ifs_test_core(int cpu, struct device *dev)
 			}
 		} else {
 			retries = MAX_IFS_RETRIES;
-			activate.start = status.chunk_num;
+			if (ifsd->generation)
+				activate.gen2.start = status_chunk;
+			else
+				activate.gen0.start = status_chunk;
+			to_start = status_chunk;
 		}
 	}
 
-- 
2.25.1

Re: [PATCH v3 4/9] platform/x86/intel/ifs: Gen2 Scan test support

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

On Fri, 29 Sep 2023, Jithu Joseph wrote:

> Width of chunk related bitfields is ACTIVATE_SCAN and SCAN_STATUS MSRs
> are different in newer IFS generation compared to gen0.
> 
> Make changes to scan test flow such that MSRs are populated
> appropriately based on the generation supported by hardware.
> 
> Account for the 8/16 bit MSR bitfield width differences between gen0 and
> newer generations for the scan test trace event too.
> 
> 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>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

[PATCH v3 5/9] platform/x86/intel/ifs: Validate image size

[Jithu Joseph]

Perform additional validation prior to loading IFS image.

Error out if the size of the file being loaded doesn't match the size
specified in the header.

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/load.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 6b827247945b..da54fd060878 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -375,6 +375,7 @@ int ifs_load_firmware(struct device *dev)
 {
 	const struct ifs_test_caps *test = ifs_get_test_caps(dev);
 	struct ifs_data *ifsd = ifs_get_data(dev);
+	unsigned int expected_size;
 	const struct firmware *fw;
 	char scan_path[64];
 	int ret = -EINVAL;
@@ -389,6 +390,13 @@ int ifs_load_firmware(struct device *dev)
 		goto done;
 	}
 
+	expected_size = ((struct microcode_header_intel *)fw->data)->totalsize;
+	if (fw->size != expected_size) {
+		dev_err(dev, "File size mismatch (expected %d, actual %ld). Corrupted IFS image.\n",
+			expected_size, fw->size);
+		return -EINVAL;
+	}
+
 	ret = image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
 	if (ret)
 		goto release;
-- 
2.25.1

Re: [PATCH v3 5/9] platform/x86/intel/ifs: Validate image size

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1509 bytes --]

On Fri, 29 Sep 2023, Jithu Joseph wrote:

> Perform additional validation prior to loading IFS image.
> 
> Error out if the size of the file being loaded doesn't match the size
> specified in the header.
> 
> 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/load.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index 6b827247945b..da54fd060878 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -375,6 +375,7 @@ int ifs_load_firmware(struct device *dev)
>  {
>  	const struct ifs_test_caps *test = ifs_get_test_caps(dev);
>  	struct ifs_data *ifsd = ifs_get_data(dev);
> +	unsigned int expected_size;
>  	const struct firmware *fw;
>  	char scan_path[64];
>  	int ret = -EINVAL;
> @@ -389,6 +390,13 @@ int ifs_load_firmware(struct device *dev)
>  		goto done;
>  	}
>  
> +	expected_size = ((struct microcode_header_intel *)fw->data)->totalsize;
> +	if (fw->size != expected_size) {
> +		dev_err(dev, "File size mismatch (expected %d, actual %ld). Corrupted IFS image.\n",
> +			expected_size, fw->size);
> +		return -EINVAL;
> +	}
> +
>  	ret = image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
>  	if (ret)
>  		goto release;
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v3 5/9] platform/x86/intel/ifs: Validate image size

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

On Mon, 2 Oct 2023, Ilpo Järvinen wrote:

> On Fri, 29 Sep 2023, Jithu Joseph wrote:
> 
> > Perform additional validation prior to loading IFS image.
> > 
> > Error out if the size of the file being loaded doesn't match the size
> > specified in the header.
> > 
> > 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/load.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> > index 6b827247945b..da54fd060878 100644
> > --- a/drivers/platform/x86/intel/ifs/load.c
> > +++ b/drivers/platform/x86/intel/ifs/load.c
> > @@ -375,6 +375,7 @@ int ifs_load_firmware(struct device *dev)
> >  {
> >  	const struct ifs_test_caps *test = ifs_get_test_caps(dev);
> >  	struct ifs_data *ifsd = ifs_get_data(dev);
> > +	unsigned int expected_size;
> >  	const struct firmware *fw;
> >  	char scan_path[64];
> >  	int ret = -EINVAL;
> > @@ -389,6 +390,13 @@ int ifs_load_firmware(struct device *dev)
> >  		goto done;
> >  	}
> >  
> > +	expected_size = ((struct microcode_header_intel *)fw->data)->totalsize;
> > +	if (fw->size != expected_size) {
> > +		dev_err(dev, "File size mismatch (expected %d, actual %ld). Corrupted IFS image.\n",
> > +			expected_size, fw->size);
> > +		return -EINVAL;
> > +	}
> > +
> >  	ret = image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
> >  	if (ret)
> >  		goto release;
> > 
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Just after sending the rev-by, I realized this also has %d vs unsigned int
problem, and %ld should be using %zu as fw->size of size_t.

Feel free to add my rev-by after those two problems have been fixed.

-- 
 i.

Re: [PATCH v3 5/9] platform/x86/intel/ifs: Validate image size

[Joseph, Jithu]

On 10/2/2023 4:50 AM, Ilpo Järvinen wrote:
> On Mon, 2 Oct 2023, Ilpo Järvinen wrote:

>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Just after sending the rev-by, I realized this also has %d vs unsigned int
> problem, and %ld should be using %zu as fw->size of size_t.
> 
> Feel free to add my rev-by after those two problems have been fixed.
> 

Thanks for the review. I will make this change

Jithu

[PATCH v3 5/9] platform/x86/intel/ifs: Validate image size

[Jithu Joseph]

Perform additional validation prior to loading IFS image.

Error out if the size of the file being loaded doesn't match the size
specified in the header.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
  - changed the dev_err printk format specifiers (Ilpo Järvinen)

 drivers/platform/x86/intel/ifs/load.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 6b827247945b..582f1801aaaa 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -375,6 +375,7 @@ int ifs_load_firmware(struct device *dev)
 {
 	const struct ifs_test_caps *test = ifs_get_test_caps(dev);
 	struct ifs_data *ifsd = ifs_get_data(dev);
+	unsigned int expected_size;
 	const struct firmware *fw;
 	char scan_path[64];
 	int ret = -EINVAL;
@@ -389,6 +390,13 @@ int ifs_load_firmware(struct device *dev)
 		goto done;
 	}
 
+	expected_size = ((struct microcode_header_intel *)fw->data)->totalsize;
+	if (fw->size != expected_size) {
+		dev_err(dev, "File size mismatch (expected %u, actual %zu). Corrupted IFS image.\n",
+			expected_size, fw->size);
+		return -EINVAL;
+	}
+
 	ret = image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
 	if (ret)
 		goto release;
-- 
2.25.1

[PATCH v3 6/9] platform/x86/intel/ifs: Metadata validation for start_chunk

[Jithu Joseph]

Add an additional check to validate IFS image metadata field prior to
loading the test image.

If start_chunk is not a multiple of chunks_per_stride error out.

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/load.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index da54fd060878..6f8abb4729e9 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -291,6 +291,13 @@ static int validate_ifs_metadata(struct device *dev)
 		return ret;
 	}
 
+	if (ifs_meta->chunks_per_stride &&
+	    (ifs_meta->starting_chunk % ifs_meta->chunks_per_stride != 0)) {
+		dev_warn(dev, "Starting chunk num %d not a multiple of chunks_per_stride %d\n",
+			 ifs_meta->starting_chunk, ifs_meta->chunks_per_stride);
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH v3 6/9] platform/x86/intel/ifs: Metadata validation for start_chunk

[Ilpo Järvinen]

On Fri, 29 Sep 2023, Jithu Joseph wrote:

> Add an additional check to validate IFS image metadata field prior to
> loading the test image.
> 
> If start_chunk is not a multiple of chunks_per_stride error out.
> 
> 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/load.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index da54fd060878..6f8abb4729e9 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -291,6 +291,13 @@ static int validate_ifs_metadata(struct device *dev)
>  		return ret;
>  	}
>  
> +	if (ifs_meta->chunks_per_stride &&
> +	    (ifs_meta->starting_chunk % ifs_meta->chunks_per_stride != 0)) {
> +		dev_warn(dev, "Starting chunk num %d not a multiple of chunks_per_stride %d\n",
> +			 ifs_meta->starting_chunk, ifs_meta->chunks_per_stride);

Please use %u as both are u32s.

-- 
 i.

> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> 

Re: [PATCH v3 6/9] platform/x86/intel/ifs: Metadata validation for start_chunk

[Joseph, Jithu]

On 10/2/2023 4:47 AM, Ilpo Järvinen wrote:
> On Fri, 29 Sep 2023, Jithu Joseph wrote:
> 
>> Add an additional check to validate IFS image metadata field prior to
>> loading the test image.
>>
>> If start_chunk is not a multiple of chunks_per_stride error out.
>>
>> 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/load.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
>> index da54fd060878..6f8abb4729e9 100644
>> --- a/drivers/platform/x86/intel/ifs/load.c
>> +++ b/drivers/platform/x86/intel/ifs/load.c
>> @@ -291,6 +291,13 @@ static int validate_ifs_metadata(struct device *dev)
>>  		return ret;
>>  	}
>>  
>> +	if (ifs_meta->chunks_per_stride &&
>> +	    (ifs_meta->starting_chunk % ifs_meta->chunks_per_stride != 0)) {
>> +		dev_warn(dev, "Starting chunk num %d not a multiple of chunks_per_stride %d\n",
>> +			 ifs_meta->starting_chunk, ifs_meta->chunks_per_stride);
> 
> Please use %u as both are u32s.
> 

Thanks for the review... I will make this change


Jithu

[PATCH v3 6/9] platform/x86/intel/ifs: Metadata validation for start_chunk

[Jithu Joseph]

Add an additional check to validate IFS image metadata field prior to
loading the test image.

If start_chunk is not a multiple of chunks_per_stride error out.

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>
---
 - changed the dev_warn printk format specifiers (Ilpo Järvinen)

 drivers/platform/x86/intel/ifs/load.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 582f1801aaaa..959b1878cae6 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -291,6 +291,13 @@ static int validate_ifs_metadata(struct device *dev)
 		return ret;
 	}
 
+	if (ifs_meta->chunks_per_stride &&
+	    (ifs_meta->starting_chunk % ifs_meta->chunks_per_stride != 0)) {
+		dev_warn(dev, "Starting chunk num %u not a multiple of chunks_per_stride %u\n",
+			 ifs_meta->starting_chunk, ifs_meta->chunks_per_stride);
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH v3 7/9] platform/x86/intel/ifs: Add new CPU support

[Jithu Joseph]

Add Granite Rapids(GNR) and Sierra Forest(SRF) cpuids to x86 match table
so that IFS driver can be loaded for those.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
 drivers/platform/x86/intel/ifs/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 4ff2aa4b484b..0c8927916373 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -18,6 +18,9 @@
 static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
 	X86_MATCH(SAPPHIRERAPIDS_X),
 	X86_MATCH(EMERALDRAPIDS_X),
+	X86_MATCH(GRANITERAPIDS_X),
+	X86_MATCH(GRANITERAPIDS_D),
+	X86_MATCH(ATOM_CRESTMONT_X),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
-- 
2.25.1

[PATCH v3 8/9] platform/x86/intel/ifs: Add new error code

[Jithu Joseph]

Make driver aware of a newly added error code so that it can provide a
more appropriate error message.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
 drivers/platform/x86/intel/ifs/runtest.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 925c30c79011..4fe544d79946 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -40,6 +40,8 @@ enum ifs_status_err_code {
 	IFS_UNASSIGNED_ERROR_CODE		= 7,
 	IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT	= 8,
 	IFS_INTERRUPTED_DURING_EXECUTION	= 9,
+	IFS_UNASSIGNED_ERROR_CODE_0xA		= 0xA,
+	IFS_CORRUPTED_CHUNK		= 0xB,
 };
 
 static const char * const scan_test_status[] = {
@@ -55,6 +57,8 @@ static const char * const scan_test_status[] = {
 	[IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT] =
 	"Exceeded number of Logical Processors (LP) allowed to run Scan-At-Field concurrently",
 	[IFS_INTERRUPTED_DURING_EXECUTION] = "Interrupt occurred prior to SCAN start",
+	[IFS_UNASSIGNED_ERROR_CODE_0xA] = "Unassigned error code 0xA",
+	[IFS_CORRUPTED_CHUNK] = "Scan operation aborted due to corrupted image. Try reloading",
 };
 
 static void message_not_tested(struct device *dev, int cpu, union ifs_status status)
@@ -123,6 +127,8 @@ static bool can_restart(union ifs_status status)
 	case IFS_MISMATCH_ARGUMENTS_BETWEEN_THREADS:
 	case IFS_CORE_NOT_CAPABLE_CURRENTLY:
 	case IFS_UNASSIGNED_ERROR_CODE:
+	case IFS_UNASSIGNED_ERROR_CODE_0xA:
+	case IFS_CORRUPTED_CHUNK:
 		break;
 	}
 	return false;
-- 
2.25.1

[PATCH v3 9/9] platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

[Jithu Joseph]

Array BIST MSR addresses, bit definition and semantics are different for
Sierra Forest. Branch into a separate Array BIST flow on Sierra Forest
when user invokes Array Test.

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     |  4 +++
 drivers/platform/x86/intel/ifs/core.c    | 15 +++++-----
 drivers/platform/x86/intel/ifs/runtest.c | 37 +++++++++++++++++++++++-
 3 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index f0dd849b3400..b183aba3ffdf 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -137,6 +137,8 @@
 #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
 #define MSR_ACTIVATE_SCAN			0x000002c6
 #define MSR_SCAN_STATUS				0x000002c7
+#define MSR_ARRAY_TRIGGER			0x000002d6
+#define MSR_ARRAY_STATUS			0x000002d7
 #define MSR_SAF_CTRL				0x000004f0
 
 #define SCAN_NOT_TESTED				0
@@ -272,6 +274,7 @@ struct ifs_test_caps {
  * @cur_batch: number indicating the currently loaded test file
  * @generation: IFS test generation enumerated by hardware
  * @chunk_size: size of a test chunk
+ * @array_gen: test generation of array test
  */
 struct ifs_data {
 	int	loaded_version;
@@ -283,6 +286,7 @@ struct ifs_data {
 	u32	cur_batch;
 	u32	generation;
 	u32	chunk_size;
+	u32	array_gen;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 0c8927916373..934eaf348f9d 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -11,16 +11,16 @@
 
 #include "ifs.h"
 
-#define X86_MATCH(model)				\
+#define X86_MATCH(model, array_gen)				\
 	X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6,	\
-		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL)
+		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, array_gen)
 
 static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
-	X86_MATCH(SAPPHIRERAPIDS_X),
-	X86_MATCH(EMERALDRAPIDS_X),
-	X86_MATCH(GRANITERAPIDS_X),
-	X86_MATCH(GRANITERAPIDS_D),
-	X86_MATCH(ATOM_CRESTMONT_X),
+	X86_MATCH(SAPPHIRERAPIDS_X, 0),
+	X86_MATCH(EMERALDRAPIDS_X, 0),
+	X86_MATCH(GRANITERAPIDS_X, 0),
+	X86_MATCH(GRANITERAPIDS_D, 0),
+	X86_MATCH(ATOM_CRESTMONT_X, 1),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
@@ -100,6 +100,7 @@ static int __init ifs_init(void)
 			continue;
 		ifs_devices[i].rw_data.generation = FIELD_GET(MSR_INTEGRITY_CAPS_SAF_GEN_MASK,
 							      msrval);
+		ifs_devices[i].rw_data.array_gen = (u32)m->driver_data;
 		ret = misc_register(&ifs_devices[i].misc);
 		if (ret)
 			goto err_exit;
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 4fe544d79946..a54cd97920c4 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -329,6 +329,38 @@ static void ifs_array_test_core(int cpu, struct device *dev)
 		ifsd->status = SCAN_TEST_PASS;
 }
 
+#define ARRAY_GEN1_TEST_ALL_ARRAYS	0x0ULL
+#define ARRAY_GEN1_STATUS_FAIL		0x1ULL
+
+static int do_array_test_gen1(void *status)
+{
+	int cpu = smp_processor_id();
+	int first;
+
+	first = cpumask_first(cpu_smt_mask(cpu));
+
+	if (cpu == first) {
+		wrmsrl(MSR_ARRAY_TRIGGER, ARRAY_GEN1_TEST_ALL_ARRAYS);
+		rdmsrl(MSR_ARRAY_STATUS, *((u64 *)status));
+	}
+
+	return 0;
+}
+
+static void ifs_array_test_gen1(int cpu, struct device *dev)
+{
+	struct ifs_data *ifsd = ifs_get_data(dev);
+	u64 status = 0;
+
+	stop_core_cpuslocked(cpu, do_array_test_gen1, &status);
+	ifsd->scan_details = status;
+
+	if (status & ARRAY_GEN1_STATUS_FAIL)
+		ifsd->status = SCAN_TEST_FAIL;
+	else
+		ifsd->status = SCAN_TEST_PASS;
+}
+
 /*
  * Initiate per core test. It wakes up work queue threads on the target cpu and
  * its sibling cpu. Once all sibling threads wake up, the scan test gets executed and
@@ -356,7 +388,10 @@ int do_core_test(int cpu, struct device *dev)
 		ifs_test_core(cpu, dev);
 		break;
 	case IFS_TYPE_ARRAY_BIST:
-		ifs_array_test_core(cpu, dev);
+		if (ifsd->array_gen == 0)
+			ifs_array_test_core(cpu, dev);
+		else
+			ifs_array_test_gen1(cpu, dev);
 		break;
 	default:
 		return -EINVAL;
-- 
2.25.1

Re: [PATCH v3 9/9] platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

[Ilpo Järvinen]

On Fri, 29 Sep 2023, Jithu Joseph wrote:

> Array BIST MSR addresses, bit definition and semantics are different for
> Sierra Forest. Branch into a separate Array BIST flow on Sierra Forest
> when user invokes Array Test.
> 
> 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     |  4 +++
>  drivers/platform/x86/intel/ifs/core.c    | 15 +++++-----
>  drivers/platform/x86/intel/ifs/runtest.c | 37 +++++++++++++++++++++++-
>  3 files changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index f0dd849b3400..b183aba3ffdf 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -137,6 +137,8 @@
>  #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
>  #define MSR_ACTIVATE_SCAN			0x000002c6
>  #define MSR_SCAN_STATUS				0x000002c7
> +#define MSR_ARRAY_TRIGGER			0x000002d6
> +#define MSR_ARRAY_STATUS			0x000002d7
>  #define MSR_SAF_CTRL				0x000004f0
>  
>  #define SCAN_NOT_TESTED				0
> @@ -272,6 +274,7 @@ struct ifs_test_caps {
>   * @cur_batch: number indicating the currently loaded test file
>   * @generation: IFS test generation enumerated by hardware
>   * @chunk_size: size of a test chunk
> + * @array_gen: test generation of array test
>   */
>  struct ifs_data {
>  	int	loaded_version;
> @@ -283,6 +286,7 @@ struct ifs_data {
>  	u32	cur_batch;
>  	u32	generation;
>  	u32	chunk_size;
> +	u32	array_gen;
>  };
>  
>  struct ifs_work {
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 0c8927916373..934eaf348f9d 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -11,16 +11,16 @@
>  
>  #include "ifs.h"
>  
> -#define X86_MATCH(model)				\
> +#define X86_MATCH(model, array_gen)				\
>  	X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6,	\
> -		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL)
> +		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, array_gen)
>  
>  static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
> -	X86_MATCH(SAPPHIRERAPIDS_X),
> -	X86_MATCH(EMERALDRAPIDS_X),
> -	X86_MATCH(GRANITERAPIDS_X),
> -	X86_MATCH(GRANITERAPIDS_D),
> -	X86_MATCH(ATOM_CRESTMONT_X),
> +	X86_MATCH(SAPPHIRERAPIDS_X, 0),
> +	X86_MATCH(EMERALDRAPIDS_X, 0),
> +	X86_MATCH(GRANITERAPIDS_X, 0),
> +	X86_MATCH(GRANITERAPIDS_D, 0),
> +	X86_MATCH(ATOM_CRESTMONT_X, 1),

Just a suggestion that would IMO make these easier to understand, you 
could name these array generations with defines so that one does not need 
to look what's defined in X86_MATCH() to understand the purpose of the 
second parameter. But it's up to you.

-- 
 i.

>  	{}
>  };
>  MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
> @@ -100,6 +100,7 @@ static int __init ifs_init(void)
>  			continue;
>  		ifs_devices[i].rw_data.generation = FIELD_GET(MSR_INTEGRITY_CAPS_SAF_GEN_MASK,
>  							      msrval);
> +		ifs_devices[i].rw_data.array_gen = (u32)m->driver_data;
>  		ret = misc_register(&ifs_devices[i].misc);
>  		if (ret)
>  			goto err_exit;
> diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
> index 4fe544d79946..a54cd97920c4 100644
> --- a/drivers/platform/x86/intel/ifs/runtest.c
> +++ b/drivers/platform/x86/intel/ifs/runtest.c
> @@ -329,6 +329,38 @@ static void ifs_array_test_core(int cpu, struct device *dev)
>  		ifsd->status = SCAN_TEST_PASS;
>  }
>  
> +#define ARRAY_GEN1_TEST_ALL_ARRAYS	0x0ULL
> +#define ARRAY_GEN1_STATUS_FAIL		0x1ULL
> +
> +static int do_array_test_gen1(void *status)
> +{
> +	int cpu = smp_processor_id();
> +	int first;
> +
> +	first = cpumask_first(cpu_smt_mask(cpu));
> +
> +	if (cpu == first) {
> +		wrmsrl(MSR_ARRAY_TRIGGER, ARRAY_GEN1_TEST_ALL_ARRAYS);
> +		rdmsrl(MSR_ARRAY_STATUS, *((u64 *)status));
> +	}
> +
> +	return 0;
> +}
> +
> +static void ifs_array_test_gen1(int cpu, struct device *dev)
> +{
> +	struct ifs_data *ifsd = ifs_get_data(dev);
> +	u64 status = 0;
> +
> +	stop_core_cpuslocked(cpu, do_array_test_gen1, &status);
> +	ifsd->scan_details = status;
> +
> +	if (status & ARRAY_GEN1_STATUS_FAIL)
> +		ifsd->status = SCAN_TEST_FAIL;
> +	else
> +		ifsd->status = SCAN_TEST_PASS;
> +}
> +
>  /*
>   * Initiate per core test. It wakes up work queue threads on the target cpu and
>   * its sibling cpu. Once all sibling threads wake up, the scan test gets executed and
> @@ -356,7 +388,10 @@ int do_core_test(int cpu, struct device *dev)
>  		ifs_test_core(cpu, dev);
>  		break;
>  	case IFS_TYPE_ARRAY_BIST:
> -		ifs_array_test_core(cpu, dev);
> +		if (ifsd->array_gen == 0)
> +			ifs_array_test_core(cpu, dev);
> +		else
> +			ifs_array_test_gen1(cpu, dev);
>  		break;
>  	default:
>  		return -EINVAL;
> 

Re: [PATCH v3 9/9] platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

[Joseph, Jithu]

On 10/2/2023 4:59 AM, Ilpo Järvinen wrote:

> 
> Just a suggestion that would IMO make these easier to understand, you 
> could name these array generations with defines so that one does not need 
> to look what's defined in X86_MATCH() to understand the purpose of the 
> second parameter. But it's up to you.
> 

I can make this change ... Thanks for the review

Jithu

[PATCH v3 9/9] platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

[Jithu Joseph]

Array BIST MSR addresses, bit definition and semantics are different for
Sierra Forest. Branch into a separate Array BIST flow on Sierra Forest
when user invokes Array Test.

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>
---
 - Add #define for Array generation (Ilpo Järvinen)

 drivers/platform/x86/intel/ifs/ifs.h     |  7 +++++
 drivers/platform/x86/intel/ifs/core.c    | 15 +++++-----
 drivers/platform/x86/intel/ifs/runtest.c | 37 +++++++++++++++++++++++-
 3 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index f0dd849b3400..2dd5e3406dac 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -137,6 +137,8 @@
 #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
 #define MSR_ACTIVATE_SCAN			0x000002c6
 #define MSR_SCAN_STATUS				0x000002c7
+#define MSR_ARRAY_TRIGGER			0x000002d6
+#define MSR_ARRAY_STATUS			0x000002d7
 #define MSR_SAF_CTRL				0x000004f0
 
 #define SCAN_NOT_TESTED				0
@@ -146,6 +148,9 @@
 #define IFS_TYPE_SAF			0
 #define IFS_TYPE_ARRAY_BIST		1
 
+#define ARRAY_GEN_0			0
+#define ARRAY_GEN_1			1
+
 /* MSR_SCAN_HASHES_STATUS bit fields */
 union ifs_scan_hashes_status {
 	u64	data;
@@ -272,6 +277,7 @@ struct ifs_test_caps {
  * @cur_batch: number indicating the currently loaded test file
  * @generation: IFS test generation enumerated by hardware
  * @chunk_size: size of a test chunk
+ * @array_gen: test generation of array test
  */
 struct ifs_data {
 	int	loaded_version;
@@ -283,6 +289,7 @@ struct ifs_data {
 	u32	cur_batch;
 	u32	generation;
 	u32	chunk_size;
+	u32	array_gen;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 0c8927916373..0a872b874af8 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -11,16 +11,16 @@
 
 #include "ifs.h"
 
-#define X86_MATCH(model)				\
+#define X86_MATCH(model, array_gen)				\
 	X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6,	\
-		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL)
+		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, array_gen)
 
 static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
-	X86_MATCH(SAPPHIRERAPIDS_X),
-	X86_MATCH(EMERALDRAPIDS_X),
-	X86_MATCH(GRANITERAPIDS_X),
-	X86_MATCH(GRANITERAPIDS_D),
-	X86_MATCH(ATOM_CRESTMONT_X),
+	X86_MATCH(SAPPHIRERAPIDS_X, ARRAY_GEN_0),
+	X86_MATCH(EMERALDRAPIDS_X, ARRAY_GEN_0),
+	X86_MATCH(GRANITERAPIDS_X, ARRAY_GEN_0),
+	X86_MATCH(GRANITERAPIDS_D, ARRAY_GEN_0),
+	X86_MATCH(ATOM_CRESTMONT_X, ARRAY_GEN_1),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
@@ -100,6 +100,7 @@ static int __init ifs_init(void)
 			continue;
 		ifs_devices[i].rw_data.generation = FIELD_GET(MSR_INTEGRITY_CAPS_SAF_GEN_MASK,
 							      msrval);
+		ifs_devices[i].rw_data.array_gen = (u32)m->driver_data;
 		ret = misc_register(&ifs_devices[i].misc);
 		if (ret)
 			goto err_exit;
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 4fe544d79946..9ac75420c15e 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -329,6 +329,38 @@ static void ifs_array_test_core(int cpu, struct device *dev)
 		ifsd->status = SCAN_TEST_PASS;
 }
 
+#define ARRAY_GEN1_TEST_ALL_ARRAYS	0x0ULL
+#define ARRAY_GEN1_STATUS_FAIL		0x1ULL
+
+static int do_array_test_gen1(void *status)
+{
+	int cpu = smp_processor_id();
+	int first;
+
+	first = cpumask_first(cpu_smt_mask(cpu));
+
+	if (cpu == first) {
+		wrmsrl(MSR_ARRAY_TRIGGER, ARRAY_GEN1_TEST_ALL_ARRAYS);
+		rdmsrl(MSR_ARRAY_STATUS, *((u64 *)status));
+	}
+
+	return 0;
+}
+
+static void ifs_array_test_gen1(int cpu, struct device *dev)
+{
+	struct ifs_data *ifsd = ifs_get_data(dev);
+	u64 status = 0;
+
+	stop_core_cpuslocked(cpu, do_array_test_gen1, &status);
+	ifsd->scan_details = status;
+
+	if (status & ARRAY_GEN1_STATUS_FAIL)
+		ifsd->status = SCAN_TEST_FAIL;
+	else
+		ifsd->status = SCAN_TEST_PASS;
+}
+
 /*
  * Initiate per core test. It wakes up work queue threads on the target cpu and
  * its sibling cpu. Once all sibling threads wake up, the scan test gets executed and
@@ -356,7 +388,10 @@ int do_core_test(int cpu, struct device *dev)
 		ifs_test_core(cpu, dev);
 		break;
 	case IFS_TYPE_ARRAY_BIST:
-		ifs_array_test_core(cpu, dev);
+		if (ifsd->array_gen == ARRAY_GEN_0)
+			ifs_array_test_core(cpu, dev);
+		else
+			ifs_array_test_gen1(cpu, dev);
 		break;
 	default:
 		return -EINVAL;
-- 
2.25.1

Re: [PATCH v3 0/9] IFS support for GNR and SRF

[Joseph, Jithu]

On 9/29/2023 1:24 PM, Jithu Joseph wrote:
> Changes in v3
>    Ilpo Jarvinen
>     - Added Reviewed-by tags wherever provided
>     - In function validate_ifs_metadata() (patch 6)
>         - Add != 0 to next line for clarity
>     - In function ifs_load_firmware() (patch 5)
>         - return -EINVAL instead of -BADFD
>     - In function ifs_test_core() (patch 4)
>         - initialize activate.gen0.rsvd = 0
>         - use if instead of conditional operator
>     - alignment change in ifs_scan_hashes_status_gen2 (patch 3)
> 

Since the suggested changes from v3 are minimal, I am sending just the 3 modified patches
in this v3 thread itself.

Hans let me know if you want me to send a v4.

Jithu

Re: [PATCH v3 0/9] IFS support for GNR and SRF

[Ilpo Järvinen]

On Wed, 4 Oct 2023, Joseph, Jithu wrote:

> 
> 
> On 9/29/2023 1:24 PM, Jithu Joseph wrote:
> > Changes in v3
> >    Ilpo Jarvinen
> >     - Added Reviewed-by tags wherever provided
> >     - In function validate_ifs_metadata() (patch 6)
> >         - Add != 0 to next line for clarity
> >     - In function ifs_load_firmware() (patch 5)
> >         - return -EINVAL instead of -BADFD
> >     - In function ifs_test_core() (patch 4)
> >         - initialize activate.gen0.rsvd = 0
> >         - use if instead of conditional operator
> >     - alignment change in ifs_scan_hashes_status_gen2 (patch 3)
> > 
> 
> Since the suggested changes from v3 are minimal, I am sending just the 3 modified patches
> in this v3 thread itself.
> 
> Hans let me know if you want me to send a v4.

I'm taking care of 6.7 cycle (or begun from -rc3 onwards to be precise), 
not Hans.

Please send a full v4 please because I want to give lkp an opportunity to 
test the whole series (even if I think the patches look okay).


-- 
 i.

Re: [PATCH v3 0/9] IFS support for GNR and SRF

[Joseph, Jithu]

On 10/5/2023 3:51 AM, Ilpo Järvinen wrote:
> On Wed, 4 Oct 2023, Joseph, Jithu wrote:
> 
>>
>>
>> On 9/29/2023 1:24 PM, Jithu Joseph wrote:
>>> Changes in v3
>>>    Ilpo Jarvinen
>>>     - Added Reviewed-by tags wherever provided
>>>     - In function validate_ifs_metadata() (patch 6)
>>>         - Add != 0 to next line for clarity
>>>     - In function ifs_load_firmware() (patch 5)
>>>         - return -EINVAL instead of -BADFD
>>>     - In function ifs_test_core() (patch 4)
>>>         - initialize activate.gen0.rsvd = 0
>>>         - use if instead of conditional operator
>>>     - alignment change in ifs_scan_hashes_status_gen2 (patch 3)
>>>
>>
>> Since the suggested changes from v3 are minimal, I am sending just the 3 modified patches
>> in this v3 thread itself.
>>
>> Hans let me know if you want me to send a v4.
> 
> I'm taking care of 6.7 cycle (or begun from -rc3 onwards to be precise), 
> not Hans.

Thanks for doing this

> 
> Please send a full v4 please because I want to give lkp an opportunity to 
> test the whole series (even if I think the patches look okay).
> 

Sent the v4 series

Jithu
 

[PATCH v4 0/9] IFS support for GNR and SRF

[Jithu Joseph]

Changes in v4
   Ilpo Järvinen
    - Changed the dev_err/warn printk format specifiers to more
       appropriate ones (patch 5, 6)
    - Add defines for array generation (patch 9)

v3 submission
Link: https://lore.kernel.org/lkml/20230929202436.2850388-1-jithu.joseph@intel.com/

Changes in v3
   Ilpo Järvinen
    - Added Reviewed-by tags wherever provided
    - In function validate_ifs_metadata() (patch 6)
        - Add != 0 to next line for clarity
    - In function ifs_load_firmware() (patch 5)
        - return -EINVAL instead of -BADFD
    - In function ifs_test_core() (patch 4)
        - initialize activate.gen0.rsvd = 0
        - use if instead of conditional operator
    - alignment change in ifs_scan_hashes_status_gen2 (patch 3)

v2 submission
Link: https://lore.kernel.org/lkml/20230922232606.1928026-1-jithu.joseph@intel.com/

Changes in v2
   Ilpo Järvinen
    - Use GENMASK_ULL() / FIELD_GET() for bitops (patch 01)
    - Avoid mixing u8 type and bitfields in certain MSR structure
       scenarios (patch03 also suggested by Dave Hansen)
    - Expand bitfield structures to use consistent genx naming (patch 04)
    - Replace goto with do / while (patch 03)
    - general formatting (multiple patches)
        - remove un-necessary parenthesis
        - reformat commit message to use whole allowed 72 columns
        - alignment changes
    Other change
     - fold v1 04/10 and 05/10 into v2 patch 04/09 to satisfy build
        constraints due to consistent genx naming

v1 submission:
Link: https://lore.kernel.org/lkml/20230913183348.1349409-1-jithu.joseph@intel.com/

This series adds In Field Scan(IFS) support for newer CPUs like Granite
Rapids(GNR) and Sierra Forest(SRF).

There are changes in the IFS image loading and test flow to support
these new CPUs.

Note to reviewers:
 - patch 1/9 adds a bit mask to arch/x86/.../msr-index.h,
  hence x86 maintainers are cc-d.
 - patch 4/9 modifies an existing tracepoint, cc Steven Rostedt
 - Rest are localized to IFS driver

Jithu Joseph (9):
  platform/x86/intel/ifs: Store IFS generation number
  platform/x86/intel/ifs: Refactor image loading code
  platform/x86/intel/ifs: Gen2 scan image loading
  platform/x86/intel/ifs: Gen2 Scan test support
  platform/x86/intel/ifs: Validate image size
  platform/x86/intel/ifs: Metadata validation for start_chunk
  platform/x86/intel/ifs: Add new CPU support
  platform/x86/intel/ifs: Add new error code
  platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

 arch/x86/include/asm/msr-index.h         |   1 +
 drivers/platform/x86/intel/ifs/ifs.h     |  64 ++++++++-
 include/trace/events/intel_ifs.h         |  16 +--
 drivers/platform/x86/intel/ifs/core.c    |  15 ++-
 drivers/platform/x86/intel/ifs/load.c    | 158 +++++++++++++++++++++--
 drivers/platform/x86/intel/ifs/runtest.c |  72 +++++++++--
 6 files changed, 286 insertions(+), 40 deletions(-)


base-commit: 8a749fd1a8720d4619c91c8b6e7528c0a355c0aa
-- 
2.25.1

[PATCH v4 1/9] platform/x86/intel/ifs: Store IFS generation number

[Jithu Joseph]

IFS generation number is reported via MSR_INTEGRITY_CAPS.  As IFS
support gets added to newer CPUs, some differences are expected during
IFS image loading and test flows.

Define MSR bitmasks to extract and store the generation in driver data,
so that driver can modify its MSR interaction appropriately.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
 arch/x86/include/asm/msr-index.h      | 1 +
 drivers/platform/x86/intel/ifs/ifs.h  | 2 ++
 drivers/platform/x86/intel/ifs/core.c | 3 +++
 3 files changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 1d111350197f..838e5a013a07 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -222,6 +222,7 @@
 #define MSR_INTEGRITY_CAPS_ARRAY_BIST          BIT(MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT)
 #define MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT	4
 #define MSR_INTEGRITY_CAPS_PERIODIC_BIST	BIT(MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT)
+#define MSR_INTEGRITY_CAPS_SAF_GEN_MASK	GENMASK_ULL(10, 9)
 
 #define MSR_LBR_NHM_FROM		0x00000680
 #define MSR_LBR_NHM_TO			0x000006c0
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 93191855890f..d666aeed20fc 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -229,6 +229,7 @@ struct ifs_test_caps {
  * @status: it holds simple status pass/fail/untested
  * @scan_details: opaque scan status code from h/w
  * @cur_batch: number indicating the currently loaded test file
+ * @generation: IFS test generation enumerated by hardware
  */
 struct ifs_data {
 	int	loaded_version;
@@ -238,6 +239,7 @@ struct ifs_data {
 	int	status;
 	u64	scan_details;
 	u32	cur_batch;
+	u32	generation;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 306f886b52d2..4ff2aa4b484b 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2022 Intel Corporation. */
 
+#include <linux/bitfield.h>
 #include <linux/module.h>
 #include <linux/kdev_t.h>
 #include <linux/semaphore.h>
@@ -94,6 +95,8 @@ static int __init ifs_init(void)
 	for (i = 0; i < IFS_NUMTESTS; i++) {
 		if (!(msrval & BIT(ifs_devices[i].test_caps->integrity_cap_bit)))
 			continue;
+		ifs_devices[i].rw_data.generation = FIELD_GET(MSR_INTEGRITY_CAPS_SAF_GEN_MASK,
+							      msrval);
 		ret = misc_register(&ifs_devices[i].misc);
 		if (ret)
 			goto err_exit;
-- 
2.25.1

[PATCH v4 2/9] platform/x86/intel/ifs: Refactor image loading code

[Jithu Joseph]

IFS image loading flow is slightly different for newer IFS generations.

In preparation for adding support for newer IFS generations, refactor
portions of existing image loading code for reuse.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
 drivers/platform/x86/intel/ifs/load.c | 31 ++++++++++++++++-----------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index cefd0d886cfd..851c97cc6a6b 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -80,6 +80,23 @@ static struct metadata_header *find_meta_data(void *ucode, unsigned int meta_typ
 	return NULL;
 }
 
+static void hashcopy_err_message(struct device *dev, u32 err_code)
+{
+	if (err_code >= ARRAY_SIZE(scan_hash_status))
+		dev_err(dev, "invalid error code 0x%x for hash copy\n", err_code);
+	else
+		dev_err(dev, "Hash copy error : %s\n", scan_hash_status[err_code]);
+}
+
+static void auth_err_message(struct device *dev, u32 err_code)
+{
+	if (err_code >= ARRAY_SIZE(scan_authentication_status))
+		dev_err(dev, "invalid error code 0x%x for authentication\n", err_code);
+	else
+		dev_err(dev, "Chunk authentication error : %s\n",
+			scan_authentication_status[err_code]);
+}
+
 /*
  * To copy scan hashes and authenticate test chunks, the initiating cpu must point
  * to the EDX:EAX to the test image in linear address.
@@ -109,11 +126,7 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
 
 	if (!hashes_status.valid) {
 		ifsd->loading_error = true;
-		if (err_code >= ARRAY_SIZE(scan_hash_status)) {
-			dev_err(dev, "invalid error code 0x%x for hash copy\n", err_code);
-			goto done;
-		}
-		dev_err(dev, "Hash copy error : %s", scan_hash_status[err_code]);
+		hashcopy_err_message(dev, err_code);
 		goto done;
 	}
 
@@ -133,13 +146,7 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
 
 		if (err_code) {
 			ifsd->loading_error = true;
-			if (err_code >= ARRAY_SIZE(scan_authentication_status)) {
-				dev_err(dev,
-					"invalid error code 0x%x for authentication\n", err_code);
-				goto done;
-			}
-			dev_err(dev, "Chunk authentication error %s\n",
-				scan_authentication_status[err_code]);
+			auth_err_message(dev, err_code);
 			goto done;
 		}
 	}
-- 
2.25.1

[PATCH v4 3/9] platform/x86/intel/ifs: Gen2 scan image loading

[Jithu Joseph]

Scan image loading flow for newer IFS generations are slightly different
from that of current generation. In newer schemes, loading need not be
done once for each socket as was done in gen0.

Also the width of NUM_CHUNKS bitfield in SCAN_HASHES_STATUS MSR has
increased from 8 -> 16 bits. Similarly there are width differences for
CHUNK_AUTHENTICATION_STATUS too.

Further the parameter to AUTHENTICATE_AND_COPY_CHUNK is passed
differently in newer generations.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
 drivers/platform/x86/intel/ifs/ifs.h  |  27 +++++++
 drivers/platform/x86/intel/ifs/load.c | 112 +++++++++++++++++++++++++-
 2 files changed, 137 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index d666aeed20fc..4824316b3acd 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -137,6 +137,8 @@
 #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
 #define MSR_ACTIVATE_SCAN			0x000002c6
 #define MSR_SCAN_STATUS				0x000002c7
+#define MSR_SAF_CTRL				0x000004f0
+
 #define SCAN_NOT_TESTED				0
 #define SCAN_TEST_PASS				1
 #define SCAN_TEST_FAIL				2
@@ -158,6 +160,19 @@ union ifs_scan_hashes_status {
 	};
 };
 
+union ifs_scan_hashes_status_gen2 {
+	u64	data;
+	struct {
+		u16	chunk_size;
+		u16	num_chunks;
+		u32	error_code		:8;
+		u32	chunks_in_stride	:9;
+		u32	rsvd			:2;
+		u32	max_core_limit		:12;
+		u32	valid			:1;
+	};
+};
+
 /* MSR_CHUNKS_AUTH_STATUS bit fields */
 union ifs_chunks_auth_status {
 	u64	data;
@@ -170,6 +185,16 @@ union ifs_chunks_auth_status {
 	};
 };
 
+union ifs_chunks_auth_status_gen2 {
+	u64	data;
+	struct {
+		u16	valid_chunks;
+		u16	total_chunks;
+		u32	error_code	:8;
+		u32	rsvd2		:24;
+	};
+};
+
 /* MSR_ACTIVATE_SCAN bit fields */
 union ifs_scan {
 	u64	data;
@@ -230,6 +255,7 @@ struct ifs_test_caps {
  * @scan_details: opaque scan status code from h/w
  * @cur_batch: number indicating the currently loaded test file
  * @generation: IFS test generation enumerated by hardware
+ * @chunk_size: size of a test chunk
  */
 struct ifs_data {
 	int	loaded_version;
@@ -240,6 +266,7 @@ struct ifs_data {
 	u64	scan_details;
 	u32	cur_batch;
 	u32	generation;
+	u32	chunk_size;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 851c97cc6a6b..6b827247945b 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -2,6 +2,7 @@
 /* Copyright(c) 2022 Intel Corporation. */
 
 #include <linux/firmware.h>
+#include <linux/sizes.h>
 #include <asm/cpu.h>
 #include <asm/microcode.h>
 
@@ -26,6 +27,11 @@ union meta_data {
 
 #define IFS_HEADER_SIZE	(sizeof(struct microcode_header_intel))
 #define META_TYPE_IFS	1
+#define INVALIDATE_STRIDE	0x1UL
+#define IFS_GEN_STRIDE_AWARE	2
+#define AUTH_INTERRUPTED_ERROR	5
+#define IFS_AUTH_RETRY_CT	10
+
 static  struct microcode_header_intel *ifs_header_ptr;	/* pointer to the ifs image header */
 static u64 ifs_hash_ptr;			/* Address of ifs metadata (hash) */
 static u64 ifs_test_image_ptr;			/* 256B aligned address of test pattern */
@@ -44,7 +50,10 @@ static const char * const scan_hash_status[] = {
 static const char * const scan_authentication_status[] = {
 	[0] = "No error reported",
 	[1] = "Attempt to authenticate a chunk which is already marked as authentic",
-	[2] = "Chunk authentication error. The hash of chunk did not match expected value"
+	[2] = "Chunk authentication error. The hash of chunk did not match expected value",
+	[3] = "Reserved",
+	[4] = "Chunk outside the current stride",
+	[5] = "Authentication flow interrupted",
 };
 
 #define MC_HEADER_META_TYPE_END		(0)
@@ -154,6 +163,102 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
 	complete(&ifs_done);
 }
 
+static int get_num_chunks(int gen, union ifs_scan_hashes_status_gen2 status)
+{
+	return gen >= IFS_GEN_STRIDE_AWARE ? status.chunks_in_stride : status.num_chunks;
+}
+
+static bool need_copy_scan_hashes(struct ifs_data *ifsd)
+{
+	return !ifsd->loaded ||
+		ifsd->generation < IFS_GEN_STRIDE_AWARE ||
+		ifsd->loaded_version != ifs_header_ptr->rev;
+}
+
+static int copy_hashes_authenticate_chunks_gen2(struct device *dev)
+{
+	union ifs_scan_hashes_status_gen2 hashes_status;
+	union ifs_chunks_auth_status_gen2 chunk_status;
+	u32 err_code, valid_chunks, total_chunks;
+	int i, num_chunks, chunk_size;
+	union meta_data *ifs_meta;
+	int starting_chunk_nr;
+	struct ifs_data *ifsd;
+	u64 linear_addr, base;
+	u64 chunk_table[2];
+	int retry_count;
+
+	ifsd = ifs_get_data(dev);
+
+	if (need_copy_scan_hashes(ifsd)) {
+		wrmsrl(MSR_COPY_SCAN_HASHES, ifs_hash_ptr);
+		rdmsrl(MSR_SCAN_HASHES_STATUS, hashes_status.data);
+
+		/* enumerate the scan image information */
+		chunk_size = hashes_status.chunk_size * SZ_1K;
+		err_code = hashes_status.error_code;
+
+		num_chunks = get_num_chunks(ifsd->generation, hashes_status);
+
+		if (!hashes_status.valid) {
+			hashcopy_err_message(dev, err_code);
+			return -EIO;
+		}
+		ifsd->loaded_version = ifs_header_ptr->rev;
+		ifsd->chunk_size = chunk_size;
+	} else {
+		num_chunks = ifsd->valid_chunks;
+		chunk_size = ifsd->chunk_size;
+	}
+
+	if (ifsd->generation >= IFS_GEN_STRIDE_AWARE) {
+		wrmsrl(MSR_SAF_CTRL, INVALIDATE_STRIDE);
+		rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
+		if (chunk_status.valid_chunks != 0) {
+			dev_err(dev, "Couldn't invalidate installed stride - %d\n",
+				chunk_status.valid_chunks);
+			return -EIO;
+		}
+	}
+
+	base = ifs_test_image_ptr;
+	ifs_meta = (union meta_data *)find_meta_data(ifs_header_ptr, META_TYPE_IFS);
+	starting_chunk_nr = ifs_meta->starting_chunk;
+
+	/* scan data authentication and copy chunks to secured memory */
+	for (i = 0; i < num_chunks; i++) {
+		retry_count = IFS_AUTH_RETRY_CT;
+		linear_addr = base + i * chunk_size;
+
+		chunk_table[0] = starting_chunk_nr + i;
+		chunk_table[1] = linear_addr;
+		do {
+			wrmsrl(MSR_AUTHENTICATE_AND_COPY_CHUNK, (u64)chunk_table);
+			rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
+			err_code = chunk_status.error_code;
+		} while (err_code == AUTH_INTERRUPTED_ERROR && --retry_count);
+
+		if (err_code) {
+			ifsd->loading_error = true;
+			auth_err_message(dev, err_code);
+			return -EIO;
+		}
+	}
+
+	valid_chunks = chunk_status.valid_chunks;
+	total_chunks = chunk_status.total_chunks;
+
+	if (valid_chunks != total_chunks) {
+		ifsd->loading_error = true;
+		dev_err(dev, "Couldn't authenticate all the chunks. Authenticated %d total %d.\n",
+			valid_chunks, total_chunks);
+		return -EIO;
+	}
+	ifsd->valid_chunks = valid_chunks;
+
+	return 0;
+}
+
 static int validate_ifs_metadata(struct device *dev)
 {
 	struct ifs_data *ifsd = ifs_get_data(dev);
@@ -206,7 +311,9 @@ static int scan_chunks_sanity_check(struct device *dev)
 		return ret;
 
 	ifsd->loading_error = false;
-	ifsd->loaded_version = ifs_header_ptr->rev;
+
+	if (ifsd->generation > 0)
+		return copy_hashes_authenticate_chunks_gen2(dev);
 
 	/* copy the scan hash and authenticate per package */
 	cpus_read_lock();
@@ -226,6 +333,7 @@ static int scan_chunks_sanity_check(struct device *dev)
 		ifs_pkg_auth[curr_pkg] = 1;
 	}
 	ret = 0;
+	ifsd->loaded_version = ifs_header_ptr->rev;
 out:
 	cpus_read_unlock();
 
-- 
2.25.1

[PATCH v4 4/9] platform/x86/intel/ifs: Gen2 Scan test support

[Jithu Joseph]

Width of chunk related bitfields is ACTIVATE_SCAN and SCAN_STATUS MSRs
are different in newer IFS generation compared to gen0.

Make changes to scan test flow such that MSRs are populated
appropriately based on the generation supported by hardware.

Account for the 8/16 bit MSR bitfield width differences between gen0 and
newer generations for the scan test trace event too.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
 drivers/platform/x86/intel/ifs/ifs.h     | 28 ++++++++++++++++++-----
 include/trace/events/intel_ifs.h         | 16 ++++++-------
 drivers/platform/x86/intel/ifs/runtest.c | 29 ++++++++++++++++++------
 3 files changed, 52 insertions(+), 21 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 4824316b3acd..f0dd849b3400 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -199,9 +199,17 @@ union ifs_chunks_auth_status_gen2 {
 union ifs_scan {
 	u64	data;
 	struct {
-		u32	start	:8;
-		u32	stop	:8;
-		u32	rsvd	:16;
+		union {
+			struct {
+				u8	start;
+				u8	stop;
+				u16	rsvd;
+			} gen0;
+			struct {
+				u16	start;
+				u16	stop;
+			} gen2;
+		};
 		u32	delay	:31;
 		u32	sigmce	:1;
 	};
@@ -211,9 +219,17 @@ union ifs_scan {
 union ifs_status {
 	u64	data;
 	struct {
-		u32	chunk_num		:8;
-		u32	chunk_stop_index	:8;
-		u32	rsvd1			:16;
+		union {
+			struct {
+				u8	chunk_num;
+				u8	chunk_stop_index;
+				u16	rsvd1;
+			} gen0;
+			struct {
+				u16	chunk_num;
+				u16	chunk_stop_index;
+			} gen2;
+		};
 		u32	error_code		:8;
 		u32	rsvd2			:22;
 		u32	control_error		:1;
diff --git a/include/trace/events/intel_ifs.h b/include/trace/events/intel_ifs.h
index d7353024016c..af0af3f1d9b7 100644
--- a/include/trace/events/intel_ifs.h
+++ b/include/trace/events/intel_ifs.h
@@ -10,25 +10,25 @@
 
 TRACE_EVENT(ifs_status,
 
-	TP_PROTO(int cpu, union ifs_scan activate, union ifs_status status),
+	TP_PROTO(int cpu, int start, int stop, u64 status),
 
-	TP_ARGS(cpu, activate, status),
+	TP_ARGS(cpu, start, stop, status),
 
 	TP_STRUCT__entry(
 		__field(	u64,	status	)
 		__field(	int,	cpu	)
-		__field(	u8,	start	)
-		__field(	u8,	stop	)
+		__field(	u16,	start	)
+		__field(	u16,	stop	)
 	),
 
 	TP_fast_assign(
 		__entry->cpu	= cpu;
-		__entry->start	= activate.start;
-		__entry->stop	= activate.stop;
-		__entry->status	= status.data;
+		__entry->start	= start;
+		__entry->stop	= stop;
+		__entry->status	= status;
 	),
 
-	TP_printk("cpu: %d, start: %.2x, stop: %.2x, status: %llx",
+	TP_printk("cpu: %d, start: %.4x, stop: %.4x, status: %.16llx",
 		__entry->cpu,
 		__entry->start,
 		__entry->stop,
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 1061eb7ec399..925c30c79011 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -171,21 +171,31 @@ static void ifs_test_core(int cpu, struct device *dev)
 	union ifs_status status;
 	unsigned long timeout;
 	struct ifs_data *ifsd;
+	int to_start, to_stop;
+	int status_chunk;
 	u64 msrvals[2];
 	int retries;
 
 	ifsd = ifs_get_data(dev);
 
-	activate.rsvd = 0;
+	activate.gen0.rsvd = 0;
 	activate.delay = IFS_THREAD_WAIT;
 	activate.sigmce = 0;
-	activate.start = 0;
-	activate.stop = ifsd->valid_chunks - 1;
+	to_start = 0;
+	to_stop = ifsd->valid_chunks - 1;
+
+	if (ifsd->generation) {
+		activate.gen2.start = to_start;
+		activate.gen2.stop = to_stop;
+	} else {
+		activate.gen0.start = to_start;
+		activate.gen0.stop = to_stop;
+	}
 
 	timeout = jiffies + HZ / 2;
 	retries = MAX_IFS_RETRIES;
 
-	while (activate.start <= activate.stop) {
+	while (to_start <= to_stop) {
 		if (time_after(jiffies, timeout)) {
 			status.error_code = IFS_SW_TIMEOUT;
 			break;
@@ -196,13 +206,14 @@ static void ifs_test_core(int cpu, struct device *dev)
 
 		status.data = msrvals[1];
 
-		trace_ifs_status(cpu, activate, status);
+		trace_ifs_status(cpu, to_start, to_stop, status.data);
 
 		/* Some cases can be retried, give up for others */
 		if (!can_restart(status))
 			break;
 
-		if (status.chunk_num == activate.start) {
+		status_chunk = ifsd->generation ? status.gen2.chunk_num : status.gen0.chunk_num;
+		if (status_chunk == to_start) {
 			/* Check for forward progress */
 			if (--retries == 0) {
 				if (status.error_code == IFS_NO_ERROR)
@@ -211,7 +222,11 @@ static void ifs_test_core(int cpu, struct device *dev)
 			}
 		} else {
 			retries = MAX_IFS_RETRIES;
-			activate.start = status.chunk_num;
+			if (ifsd->generation)
+				activate.gen2.start = status_chunk;
+			else
+				activate.gen0.start = status_chunk;
+			to_start = status_chunk;
 		}
 	}
 
-- 
2.25.1

[PATCH v4 5/9] platform/x86/intel/ifs: Validate image size

[Jithu Joseph]

Perform additional validation prior to loading IFS image.

Error out if the size of the file being loaded doesn't match the size
specified in the header.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
 drivers/platform/x86/intel/ifs/load.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 6b827247945b..582f1801aaaa 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -375,6 +375,7 @@ int ifs_load_firmware(struct device *dev)
 {
 	const struct ifs_test_caps *test = ifs_get_test_caps(dev);
 	struct ifs_data *ifsd = ifs_get_data(dev);
+	unsigned int expected_size;
 	const struct firmware *fw;
 	char scan_path[64];
 	int ret = -EINVAL;
@@ -389,6 +390,13 @@ int ifs_load_firmware(struct device *dev)
 		goto done;
 	}
 
+	expected_size = ((struct microcode_header_intel *)fw->data)->totalsize;
+	if (fw->size != expected_size) {
+		dev_err(dev, "File size mismatch (expected %u, actual %zu). Corrupted IFS image.\n",
+			expected_size, fw->size);
+		return -EINVAL;
+	}
+
 	ret = image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
 	if (ret)
 		goto release;
-- 
2.25.1

[PATCH v4 6/9] platform/x86/intel/ifs: Metadata validation for start_chunk

[Jithu Joseph]

Add an additional check to validate IFS image metadata field prior to
loading the test image.

If start_chunk is not a multiple of chunks_per_stride error out.

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/load.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index 582f1801aaaa..959b1878cae6 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -291,6 +291,13 @@ static int validate_ifs_metadata(struct device *dev)
 		return ret;
 	}
 
+	if (ifs_meta->chunks_per_stride &&
+	    (ifs_meta->starting_chunk % ifs_meta->chunks_per_stride != 0)) {
+		dev_warn(dev, "Starting chunk num %u not a multiple of chunks_per_stride %u\n",
+			 ifs_meta->starting_chunk, ifs_meta->chunks_per_stride);
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH v4 7/9] platform/x86/intel/ifs: Add new CPU support

[Jithu Joseph]

Add Granite Rapids(GNR) and Sierra Forest(SRF) cpuids to x86 match table
so that IFS driver can be loaded for those.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
 drivers/platform/x86/intel/ifs/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 4ff2aa4b484b..0c8927916373 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -18,6 +18,9 @@
 static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
 	X86_MATCH(SAPPHIRERAPIDS_X),
 	X86_MATCH(EMERALDRAPIDS_X),
+	X86_MATCH(GRANITERAPIDS_X),
+	X86_MATCH(GRANITERAPIDS_D),
+	X86_MATCH(ATOM_CRESTMONT_X),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
-- 
2.25.1

[PATCH v4 8/9] platform/x86/intel/ifs: Add new error code

[Jithu Joseph]

Make driver aware of a newly added error code so that it can provide a
more appropriate error message.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
 drivers/platform/x86/intel/ifs/runtest.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 925c30c79011..4fe544d79946 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -40,6 +40,8 @@ enum ifs_status_err_code {
 	IFS_UNASSIGNED_ERROR_CODE		= 7,
 	IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT	= 8,
 	IFS_INTERRUPTED_DURING_EXECUTION	= 9,
+	IFS_UNASSIGNED_ERROR_CODE_0xA		= 0xA,
+	IFS_CORRUPTED_CHUNK		= 0xB,
 };
 
 static const char * const scan_test_status[] = {
@@ -55,6 +57,8 @@ static const char * const scan_test_status[] = {
 	[IFS_EXCEED_NUMBER_OF_THREADS_CONCURRENT] =
 	"Exceeded number of Logical Processors (LP) allowed to run Scan-At-Field concurrently",
 	[IFS_INTERRUPTED_DURING_EXECUTION] = "Interrupt occurred prior to SCAN start",
+	[IFS_UNASSIGNED_ERROR_CODE_0xA] = "Unassigned error code 0xA",
+	[IFS_CORRUPTED_CHUNK] = "Scan operation aborted due to corrupted image. Try reloading",
 };
 
 static void message_not_tested(struct device *dev, int cpu, union ifs_status status)
@@ -123,6 +127,8 @@ static bool can_restart(union ifs_status status)
 	case IFS_MISMATCH_ARGUMENTS_BETWEEN_THREADS:
 	case IFS_CORE_NOT_CAPABLE_CURRENTLY:
 	case IFS_UNASSIGNED_ERROR_CODE:
+	case IFS_UNASSIGNED_ERROR_CODE_0xA:
+	case IFS_CORRUPTED_CHUNK:
 		break;
 	}
 	return false;
-- 
2.25.1

[PATCH v4 9/9] platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

[Jithu Joseph]

Array BIST MSR addresses, bit definition and semantics are different for
Sierra Forest. Branch into a separate Array BIST flow on Sierra Forest
when user invokes Array Test.

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     |  7 +++++
 drivers/platform/x86/intel/ifs/core.c    | 15 +++++-----
 drivers/platform/x86/intel/ifs/runtest.c | 37 +++++++++++++++++++++++-
 3 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index f0dd849b3400..2dd5e3406dac 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -137,6 +137,8 @@
 #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
 #define MSR_ACTIVATE_SCAN			0x000002c6
 #define MSR_SCAN_STATUS				0x000002c7
+#define MSR_ARRAY_TRIGGER			0x000002d6
+#define MSR_ARRAY_STATUS			0x000002d7
 #define MSR_SAF_CTRL				0x000004f0
 
 #define SCAN_NOT_TESTED				0
@@ -146,6 +148,9 @@
 #define IFS_TYPE_SAF			0
 #define IFS_TYPE_ARRAY_BIST		1
 
+#define ARRAY_GEN_0			0
+#define ARRAY_GEN_1			1
+
 /* MSR_SCAN_HASHES_STATUS bit fields */
 union ifs_scan_hashes_status {
 	u64	data;
@@ -272,6 +277,7 @@ struct ifs_test_caps {
  * @cur_batch: number indicating the currently loaded test file
  * @generation: IFS test generation enumerated by hardware
  * @chunk_size: size of a test chunk
+ * @array_gen: test generation of array test
  */
 struct ifs_data {
 	int	loaded_version;
@@ -283,6 +289,7 @@ struct ifs_data {
 	u32	cur_batch;
 	u32	generation;
 	u32	chunk_size;
+	u32	array_gen;
 };
 
 struct ifs_work {
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 0c8927916373..0a872b874af8 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -11,16 +11,16 @@
 
 #include "ifs.h"
 
-#define X86_MATCH(model)				\
+#define X86_MATCH(model, array_gen)				\
 	X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6,	\
-		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL)
+		INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, array_gen)
 
 static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
-	X86_MATCH(SAPPHIRERAPIDS_X),
-	X86_MATCH(EMERALDRAPIDS_X),
-	X86_MATCH(GRANITERAPIDS_X),
-	X86_MATCH(GRANITERAPIDS_D),
-	X86_MATCH(ATOM_CRESTMONT_X),
+	X86_MATCH(SAPPHIRERAPIDS_X, ARRAY_GEN_0),
+	X86_MATCH(EMERALDRAPIDS_X, ARRAY_GEN_0),
+	X86_MATCH(GRANITERAPIDS_X, ARRAY_GEN_0),
+	X86_MATCH(GRANITERAPIDS_D, ARRAY_GEN_0),
+	X86_MATCH(ATOM_CRESTMONT_X, ARRAY_GEN_1),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
@@ -100,6 +100,7 @@ static int __init ifs_init(void)
 			continue;
 		ifs_devices[i].rw_data.generation = FIELD_GET(MSR_INTEGRITY_CAPS_SAF_GEN_MASK,
 							      msrval);
+		ifs_devices[i].rw_data.array_gen = (u32)m->driver_data;
 		ret = misc_register(&ifs_devices[i].misc);
 		if (ret)
 			goto err_exit;
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 4fe544d79946..9ac75420c15e 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -329,6 +329,38 @@ static void ifs_array_test_core(int cpu, struct device *dev)
 		ifsd->status = SCAN_TEST_PASS;
 }
 
+#define ARRAY_GEN1_TEST_ALL_ARRAYS	0x0ULL
+#define ARRAY_GEN1_STATUS_FAIL		0x1ULL
+
+static int do_array_test_gen1(void *status)
+{
+	int cpu = smp_processor_id();
+	int first;
+
+	first = cpumask_first(cpu_smt_mask(cpu));
+
+	if (cpu == first) {
+		wrmsrl(MSR_ARRAY_TRIGGER, ARRAY_GEN1_TEST_ALL_ARRAYS);
+		rdmsrl(MSR_ARRAY_STATUS, *((u64 *)status));
+	}
+
+	return 0;
+}
+
+static void ifs_array_test_gen1(int cpu, struct device *dev)
+{
+	struct ifs_data *ifsd = ifs_get_data(dev);
+	u64 status = 0;
+
+	stop_core_cpuslocked(cpu, do_array_test_gen1, &status);
+	ifsd->scan_details = status;
+
+	if (status & ARRAY_GEN1_STATUS_FAIL)
+		ifsd->status = SCAN_TEST_FAIL;
+	else
+		ifsd->status = SCAN_TEST_PASS;
+}
+
 /*
  * Initiate per core test. It wakes up work queue threads on the target cpu and
  * its sibling cpu. Once all sibling threads wake up, the scan test gets executed and
@@ -356,7 +388,10 @@ int do_core_test(int cpu, struct device *dev)
 		ifs_test_core(cpu, dev);
 		break;
 	case IFS_TYPE_ARRAY_BIST:
-		ifs_array_test_core(cpu, dev);
+		if (ifsd->array_gen == ARRAY_GEN_0)
+			ifs_array_test_core(cpu, dev);
+		else
+			ifs_array_test_gen1(cpu, dev);
 		break;
 	default:
 		return -EINVAL;
-- 
2.25.1

Re: [PATCH v4 9/9] platform/x86/intel/ifs: ARRAY BIST for Sierra Forest

[Ilpo Järvinen]

On Thu, 5 Oct 2023, Jithu Joseph wrote:

> Array BIST MSR addresses, bit definition and semantics are different for
> Sierra Forest. Branch into a separate Array BIST flow on Sierra Forest
> when user invokes Array Test.
> 
> 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     |  7 +++++
>  drivers/platform/x86/intel/ifs/core.c    | 15 +++++-----
>  drivers/platform/x86/intel/ifs/runtest.c | 37 +++++++++++++++++++++++-
>  3 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index f0dd849b3400..2dd5e3406dac 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -137,6 +137,8 @@
>  #define MSR_CHUNKS_AUTHENTICATION_STATUS	0x000002c5
>  #define MSR_ACTIVATE_SCAN			0x000002c6
>  #define MSR_SCAN_STATUS				0x000002c7
> +#define MSR_ARRAY_TRIGGER			0x000002d6
> +#define MSR_ARRAY_STATUS			0x000002d7
>  #define MSR_SAF_CTRL				0x000004f0
>  
>  #define SCAN_NOT_TESTED				0
> @@ -146,6 +148,9 @@
>  #define IFS_TYPE_SAF			0
>  #define IFS_TYPE_ARRAY_BIST		1
>  
> +#define ARRAY_GEN_0			0
> +#define ARRAY_GEN_1			1

Thank you for your contribution. I've applied this series to my local 
review-ilpo branch.

I ended up renaming these defines ARRAY_GEN_* -> ARRAY_GEN* to make
them more consistent.

-- 
 i.