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
