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

Sohil Mehta
2022-11-01T08:51:24Z

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