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

[Sohil Mehta <sohil.mehta@intel.com>]

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