Messages in this thread | | | Date | Tue, 1 Nov 2022 01:51:10 -0700 | Subject | Re: [PATCH 08/14] x86/microcode/intel: Meta-data support in microcode file | From | Sohil Mehta <> |
| |
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 µcode_intel_ops; > } > +
Sohil
| |