Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7ceeaf97-fbc8-54ac-2041-75f2ca8bc7e2@intel.com>
Date:   Tue, 1 Nov 2022 01:51:10 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH 08/14] x86/microcode/intel: Meta-data support in microcode
 file
Content-Language: en-US
To:     Jithu Joseph <jithu.joseph@intel.com>, <hdegoede@redhat.com>,
        <markgross@kernel.org>
CC:     <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
        <dave.hansen@linux.intel.com>, <x86@kernel.org>, <hpa@zytor.com>,
        <gregkh@linuxfoundation.org>, <ashok.raj@intel.com>,
        <tony.luck@intel.com>, <linux-kernel@vger.kernel.org>,
        <platform-driver-x86@vger.kernel.org>, <patches@lists.linux.dev>,
        <ravi.v.shankar@intel.com>, <thiago.macieira@intel.com>,
        <athenas.jimenez.gonzalez@intel.com>
References: <20221021203413.1220137-1-jithu.joseph@intel.com>
 <20221021203413.1220137-9-jithu.joseph@intel.com>
From:   Sohil Mehta <sohil.mehta@intel.com>
In-Reply-To: <20221021203413.1220137-9-jithu.joseph@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dEN6RFBMeDF6cW1PeFlGc0pLVlZUdVBSejRpRU1ydkJvMHN4MEM5bG8yVGFy?=
 =?utf-8?B?L2FYcjVjSUowRU9CMVF5cnFDNEoraHVkdHRUQW81T1oyWVNvbFVjYzRrQlkr?=
 =?utf-8?B?Q0lsb1Ura0JIVllldEZqdk16M1Rha3VhZDJPWkI0OEN3OTE5YkFJYk8rRCtV?=
 =?utf-8?B?UjZvbDNNWk5kZ1FweXA4Qzd0TEhCODJXaThVMDdEd1BGaGVYWGtHTkd0SFZW?=
 =?utf-8?B?cEl2VVNMSEtzZ0JjR3JBTGg0WVRlN0MrVTUrVFBkbStWUysraW12U2FYYzlr?=
 =?utf-8?B?K0tIT2JTOHFKRFpZa2pxS0llWWRGUmpvaUJRQit0NThQTUNSRFpzUmFCcTVt?=
 =?utf-8?B?Vm05cE44MWZPYkxKT1BQN0NNWnM3T0xJVjRJSzdhOFd0MU1BWC8vTlBZOVUy?=
 =?utf-8?B?cHpKVkFGcDlPQkVhTk1oUko5a05VK0pSU0sxdUpwelFSN3czQVFsdU5vSlgr?=
 =?utf-8?B?dDNNVGhibHpYUUpxdHRHWlc1ZVQ0S091S2lWZDZlTUpYMFJNYi82aVFKeURo?=
 =?utf-8?B?S2w5SWdRN3c4Wmw3WUMzM3E0dy9qRzMxdmJMd0tSUUtGTnlvcDJwN3J0RU80?=
 =?utf-8?B?eDZWSnBlZExzNzdVVkRhQi9Vb0pWSzNXSkdJT1pFRThtdk0veERyNHJxaHlq?=
 =?utf-8?B?cElSZ3ZYdTA4QmJoSHE4OFgwTUxFMU9Ba2c2dHYxVEUrOWRhelZXbWdNZ01H?=
 =?utf-8?B?dDk4NmtTZDBkczZJT2g0aUZlbGRzSS95bzdadnc0c3lYYWo0TFhLZCs3RG11?=
 =?utf-8?B?ZWxLbG9VSm0yYnZ4Mzc2d2I2TnVjaGMzdG5OQ0xiK1IwajhRcmVtK1hOTjho?=
 =?utf-8?B?OFNiYlpTZm8zRzhNMG9EWGFuTVpTcXgvU003RmhXRDVTam9pTk9ZcmZtVThy?=
 =?utf-8?B?TERreW1WL3JXODdId1cvNHM4bVIvVmVaSXJlVWhzSWlhVzZ0YU12L0p6RitU?=
 =?utf-8?B?NkdJeEJka0h2UzdYTWVKRy9TNG5GOFo1Vk5zSzFQOEM1QkFzQmFJRm5BZnZm?=
 =?utf-8?B?SzF3MDJGNkdmQWpWYVRLQ2lhUGhIQW50T0ZYM3lxZTJFdVgydXQrcmQyV3BR?=
 =?utf-8?B?RjBPdnQ0blNCbnJUZGFqdFdsZUxjYXJsUGhRWFZ4dHpKR2ZOVis2WHp4TUtY?=
 =?utf-8?B?MU5pS0pudEgvM1BYeDdQNlQ0OWJHWC9Ccm1EOXd2RDJ1Vk9UVmo3K0M2Sm8v?=
 =?utf-8?B?d3graThoOGJlYW80V2RRWFZCU1dIaUNUcWdrKy8xdUJieWs1L25XUDNCT3pK?=
 =?utf-8?B?L0FudFFveG1WSmEzak5wa0F6dldkU2ZPak1PVDRHbzRqcStIVG1nMlpVU2xw?=
 =?utf-8?B?NFBNeDlGZzAxTTdEeXJFb2lyR0llZVU3L0dURmE2N3VKdFhmL0g4SG5MUGMv?=
 =?utf-8?B?aStZdTUrMlZTODRONUlaKzlYc3F1dWtoZmNySHk1blBVc0hobHkrZFlzV3lO?=
 =?utf-8?B?cHZERWpVYlF1MFpqYytwQjFodWh6dzhYYkMwcmVmNHMrY2hXd1dMMU14SkxJ?=
 =?utf-8?B?VVdnalYrYnlIbmE0cmhqODJibFJ2dHk0S1VjdFc2TDVGQVNiWk5sMTcrM0JX?=
 =?utf-8?B?YkdrT1hsZ1ZLdVlid05GYzkxMUkwMG00TDRMQUcvZEVFU3NMQjhYTVhWdEdV?=
 =?utf-8?B?Vnk2Nnh2YjMzWlAydm04NVlscXpZTGsvbHR4cTl3M0JtRWIyeWJsU2VpZ0Z0?=
 =?utf-8?B?V214ak5GdlFpM1l5U1NKMHRiRElWK0RzeGVKTExoUExaYVNpYks0STJETmlB?=
 =?utf-8?B?RjRUaUtNZVoyMXorQ2s2cGsxb0JvbEhsNytPS2ZJUWIvUDVrK2FpZVIreVVl?=
 =?utf-8?B?TWJUVE1GNkJKU0MvMk9hc2ZCejZGSkZrUnR1MDNJSGFaWk90OCtrdktVd2w4?=
 =?utf-8?B?a2tIbjhka2pCNGl5QVhpM3NCNExvVTZiUDhLWXVLVk5jZW4wUkNjN0FZM29n?=
 =?utf-8?B?K0doYlp5cWJXTlo2ZjV6NDU1aDhGcnBhRDJibW80eUgzVHJQQStqL2E1WTNi?=
 =?utf-8?B?RW9sQlFCUFhwOWxhNk0zRnBUU052eVp4NnFNRThMUXZpYWlaWXNPbFhnYnBo?=
 =?utf-8?B?MFFTc0xZZXJOTmV3NGJPdzloZWZiMVdtcHMxUUtXYmlFZklXeUZjMVBYK0NN?=
 =?utf-8?B?Z1Z0VTJRanRtaThrc3FJTXJzakFXNzVPbkVKQTZzODFBRFVoelA1VXN3b3V5?=
 =?utf-8?B?T2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ff44eafd-73e3-434b-9fdb-08dabbe63a6e
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3320.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Nov 2022 08:51:12.4640
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ewWT7AnxB0LDW7LPzBWEmp0nMGcsFUn2O6ovAxjKyirKPSzdq9LtDtmDCPFlDtaY7oE9irMlinyEXZN6lHwkWg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR11MB5694
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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