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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- 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.