diff mbox series

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

Message ID 20230913183348.1349409-4-jithu.joseph@intel.com (mailing list archive)
State Changes Requested, archived
Headers show
Series IFS support for GNR and SRF | expand

Commit Message

Joseph, Jithu Sept. 13, 2023, 6:33 p.m. UTC
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(-)

Comments

Ilpo Järvinen Sept. 15, 2023, 4:46 p.m. UTC | #1
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();
>  
>
Joseph, Jithu Sept. 15, 2023, 5:20 p.m. UTC | #2
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
Ilpo Järvinen Sept. 18, 2023, 8:49 a.m. UTC | #3
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).
Tony Luck Sept. 18, 2023, 3:25 p.m. UTC | #4
> 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
Ilpo Järvinen Sept. 18, 2023, 3:46 p.m. UTC | #5
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/
Tony Luck Sept. 18, 2023, 4:09 p.m. UTC | #6
> 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
Ilpo Järvinen Sept. 18, 2023, 4:29 p.m. UTC | #7
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.
Joseph, Jithu Sept. 18, 2023, 4:51 p.m. UTC | #8
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
Dave Hansen Sept. 18, 2023, 4:58 p.m. UTC | #9
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;
	};

?
Joseph, Jithu Sept. 18, 2023, 5:45 p.m. UTC | #10
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
diff mbox series

Patch

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();