[08/14] x86/microcode/intel: Meta-data support in microcode file

  From: Ashok Raj <ashok.raj@intel.com>

Intel has made extensions to the microcode file format so that it
can also be used for In Field Scan. One of the existing reserved fields
has been allocated to indicate the size of the region in the file allocated
for metadata structures.

    Microcode Format
   +----------------------+  Base
   |Header Version        |
   +----------------------+
   |Update revision       |
   +----------------------+
   |Date DDMMYYYY         |
   +----------------------+
   |Sig                   |
   +----------------------+
   |Checksum              |
   +----------------------+
   |Loader Version        |
   +----------------------+
   |Processor Flags       |
   +----------------------+
   |Data Size             |
   +----------------------+
   |Total Size            |
   +----------------------+
   |Meta Size             |
   +----------------------+
   |Reserved              |
   +----------------------+
   |Reserved              |
   +----------------------+  Base+48
   |                      |
   |                      |
   |                      |
   |                      |
   |    Microcode         |
   |                      |
   |     Data             |
   |                      |
   |                      |
   +----------------------+  Base+48+data_size-
   |                      |     meta_size
   |   Meta Data          |
   |                      |
   +----------------------+  Base+48+data_size
   |   Extended Signature |
   |        Table         |
   |                      |
   |                      |
   |                      |
   |                      |
   |                      |
   +----------------------+  Base+total_size

In subsequent patches IFS test image file (which reuse microcode header
format) will make use of metadata section. Though IFS is the first
consumer of this metadata within microcode file, it is expected that
this will be used for supporting upcoming microcode update
related enhancements.

Also add an accessor function which will return a pointer to the
start of a specific meta_type being queried.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
---
 arch/x86/include/asm/microcode_intel.h | 19 +++++++++-
 arch/x86/kernel/cpu/microcode/intel.c  | 48 ++++++++++++++++++++++++--
 2 files changed, 64 insertions(+), 3 deletions(-)
  

How about?

x86/microcode/intel: Add metadata support

> +struct metadata_header {
> +	unsigned int		meta_type;
> +	unsigned int		meta_blk_size;
> +};
> +
> +struct metadata_intel {
> +	struct metadata_header	meta_hdr;
> +	unsigned int		meta_bits[];
> +};
> +

Can we avoid the meta_ prefixes in the struct variables since the struct 
name already includes meta?

>   #define DEFAULT_UCODE_DATASIZE	(2000)
>   #define MC_HEADER_SIZE		(sizeof(struct microcode_header_intel))
>   #define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
> @@ -76,6 +89,7 @@ extern int __init save_microcode_in_initrd_intel(void);
>   void reload_ucode_intel(void);
>   int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf);
>   int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver);
> +struct metadata_header *microcode_intel_find_meta_data(void *ucode, unsigned int meta_type);

Is there a difference between "ucode" and "mc"? They seem to be used 
interchangeably all over.

At least to keep it consistent across the exported functions, should the 
  parameter be named "mc"?

>   int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
>   {
> -	unsigned long total_size, data_size, ext_table_size;
> +	unsigned long total_size, data_size, ext_table_size, total_meta;
>   	struct microcode_header_intel *mc_header = mc;
>   	struct extended_sigtable *ext_header = NULL;
>   	u32 sum, orig_sum, ext_sigcount = 0, i;
>   	struct extended_signature *ext_sig;
> +	struct metadata_header *meta_header;
> +	unsigned long meta_size = 0;
>   
>   	total_size = get_totalsize(mc_header);
>   	data_size = get_datasize(mc_header);
> +	total_meta = mc_header->metasize;
>   
>   	if (data_size + MC_HEADER_SIZE > total_size) {
>   		if (print_err)
> @@ -245,7 +248,7 @@ int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
>   	}
>   
>   	if (!ext_table_size)
> -		return 0;
> +		goto check_meta;
>   

The code flow in this function seems a bit confusing. Can we avoid the 
goto and make this a bit cleaner?

There is already a check for ext_table_size above. Can the extended 
signature checking be merged with that?


>   	/*
>   	 * Check extended signature checksum: 0 => valid.
> @@ -262,6 +265,22 @@ int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
>   			return -EINVAL;
>   		}
>   	}
> +
> +check_meta:
> +	if (!total_meta)
> +		return 0;
> +
> +	meta_header =  (mc + MC_HEADER_SIZE + data_size) - total_meta;
> +	while (meta_header->meta_type != META_TYPE_END) {
> +		meta_size += meta_header->meta_blk_size;
> +		if (!meta_header->meta_blk_size || meta_size > total_meta) {
> +			if (print_err) {
> +				pr_err("Bad value for metadata size, aborting.\n");
> +				return -EINVAL;
> +			}

This seems to be returning an error only when print_err is enabled. 
Otherwise, it treats as a success.

> +		}
> +		meta_header = (void *)meta_header + meta_header->meta_blk_size;
> +	}
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(microcode_intel_sanity_check);
> @@ -967,3 +986,28 @@ struct microcode_ops * __init init_intel_microcode(void)
>   
>   	return &microcode_intel_ops;
>   }
> +

Sohil
  

On 11/1/2022 1:51 AM, Sohil Mehta wrote:
> How about?
> 
> x86/microcode/intel: Add metadata support

Will reword as you suggest above

> 
>> +struct metadata_header {
>> +    unsigned int        meta_type;
>> +    unsigned int        meta_blk_size;
>> +};
>> +
>> +struct metadata_intel {
>> +    struct metadata_header    meta_hdr;
>> +    unsigned int        meta_bits[];
>> +};
>> +
> 
> Can we avoid the meta_ prefixes in the struct variables since the struct name already includes meta?

Will do

> 
>>   #define DEFAULT_UCODE_DATASIZE    (2000)
>>   #define MC_HEADER_SIZE        (sizeof(struct microcode_header_intel))
>>   #define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
>> @@ -76,6 +89,7 @@ extern int __init save_microcode_in_initrd_intel(void);
>>   void reload_ucode_intel(void);
>>   int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf);
>>   int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver);
>> +struct metadata_header *microcode_intel_find_meta_data(void *ucode, unsigned int meta_type);
> 
> Is there a difference between "ucode" and "mc"? They seem to be used interchangeably all over.
> 
> At least to keep it consistent across the exported functions, should the  parameter be named "mc"?

Will change the parameter to mc

> 
>>   int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
>>   {
>> -    unsigned long total_size, data_size, ext_table_size;
>> +    unsigned long total_size, data_size, ext_table_size, total_meta;
>>       struct microcode_header_intel *mc_header = mc;
>>       struct extended_sigtable *ext_header = NULL;
>>       u32 sum, orig_sum, ext_sigcount = 0, i;
>>       struct extended_signature *ext_sig;
>> +    struct metadata_header *meta_header;
>> +    unsigned long meta_size = 0;
>>         total_size = get_totalsize(mc_header);
>>       data_size = get_datasize(mc_header);
>> +    total_meta = mc_header->metasize;
>>         if (data_size + MC_HEADER_SIZE > total_size) {
>>           if (print_err)
>> @@ -245,7 +248,7 @@ int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
>>       }
>>         if (!ext_table_size)
>> -        return 0;
>> +        goto check_meta;
>>   
> 
> The code flow in this function seems a bit confusing. Can we avoid the goto and make this a bit cleaner?
> 
> There is already a check for ext_table_size above. Can the extended signature checking be merged with that?

Will modify the flow as below

-       if (!ext_table_size)
-               goto check_meta;
-
+       if (ext_table_size) {
        /*
         * Check extended signature checksum: 0 => valid.
         */
         for( ...) {

                        return -EINVAL;
                }
        }
+       }

>> +
>> +check_meta:
>> +    if (!total_meta)
>> +        return 0;
>> +
>> +    meta_header =  (mc + MC_HEADER_SIZE + data_size) - total_meta;
>> +    while (meta_header->meta_type != META_TYPE_END) {
>> +        meta_size += meta_header->meta_blk_size;
>> +        if (!meta_header->meta_blk_size || meta_size > total_meta) {
>> +            if (print_err) {
>> +                pr_err("Bad value for metadata size, aborting.\n");
>> +                return -EINVAL;
>> +            }
> 
> This seems to be returning an error only when print_err is enabled. Otherwise, it treats as a success.
> 

Thanks for pointing this, will remove the {} following the "if (print_err)"

Jithu
  

On Fri, Oct 21, 2022 at 01:34:07PM -0700, Jithu Joseph wrote:
> From: Ashok Raj <ashok.raj@intel.com>
> 
> Intel has made extensions to the microcode file format so that it
> can also be used for In Field Scan.

And this basically confirms what I just said in my previous mail: you
want to do IFS-specific changes already.

So please copy all code into your driver and reuse as you like so that
there are no cross-breakages.