diff mbox series

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

Message ID 20221021203413.1220137-9-jithu.joseph@intel.com (mailing list archive)
State Changes Requested, archived
Headers show
Series IFS multi test image support and misc changes | expand

Commit Message

Joseph, Jithu Oct. 21, 2022, 8:34 p.m. UTC 
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(-)

Comments

Sohil Mehta Nov. 1, 2022, 8:51 a.m. UTC | #1 
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
Joseph, Jithu Nov. 1, 2022, 6:05 p.m. UTC | #2 
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
Borislav Petkov Nov. 3, 2022, 11:35 a.m. UTC | #3 
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.
diff mbox series

Patch

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 27eba991c6b6..dcbc377f67d1 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -14,7 +14,8 @@  struct microcode_header_intel {
 	unsigned int            pf;
 	unsigned int            datasize;
 	unsigned int            totalsize;
-	unsigned int            reserved[3];
+	unsigned int            metasize;
+	unsigned int            reserved[2];
 };
 
 struct microcode_intel {
@@ -36,6 +37,18 @@  struct extended_sigtable {
 	struct extended_signature sigs[];
 };
 
+#define META_TYPE_END		(0)
+
+struct metadata_header {
+	unsigned int		meta_type;
+	unsigned int		meta_blk_size;
+};
+
+struct metadata_intel {
+	struct metadata_header	meta_hdr;
+	unsigned int		meta_bits[];
+};
+
 #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);
 #else
 static inline __init void load_ucode_intel_bsp(void) {}
 static inline void load_ucode_intel_ap(void) {}
@@ -86,6 +100,9 @@  static inline int microcode_intel_find_matching_signature(void *mc, unsigned int
 	{ return 0; }
 static inline int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
 	{ return -EINVAL; }
+static inline struct metadata_header *microcode_intel_find_meta_data(void *ucode,
+								     unsigned int meta_type)
+	{ return NULL; }
 #endif
 
 #endif /* _ASM_X86_MICROCODE_INTEL_H */
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index bc3f33a25d7a..179ca345bc06 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -168,14 +168,17 @@  static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
 
 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;
 
 	/*
 	 * 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;
+			}
+		}
+		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;
 }
+
+struct metadata_header *microcode_intel_find_meta_data(void *ucode, unsigned int meta_type)
+{
+	struct metadata_header *meta_header;
+	unsigned long data_size, total_meta;
+	unsigned long meta_size = 0;
+
+	data_size = get_datasize(ucode);
+	total_meta = ((struct microcode_intel *)ucode)->hdr.metasize;
+
+	if (!total_meta)
+		return NULL;
+
+	meta_header = (ucode + MC_HEADER_SIZE + data_size) - total_meta;
+
+	while ((meta_header->meta_type != META_TYPE_END) && meta_size < total_meta) {
+		meta_size += meta_header->meta_blk_size;
+		if (meta_header->meta_type == meta_type)
+			return meta_header;
+
+		meta_header = (void *)meta_header + meta_header->meta_blk_size;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(microcode_intel_find_meta_data);