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: <dcdc52a6-c1f1-cbe3-81d4-cc06332f61c2@intel.com>
Date:   Tue, 1 Nov 2022 12:06:35 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Subject: Re: [PATCH 07/14] x86/microcode/intel: Expose
 microcode_sanity_check()
Content-Language: en-US
To:     Sohil Mehta <sohil.mehta@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-8-jithu.joseph@intel.com>
 <c225e969-a88d-f3b4-de44-ef71f44dd463@intel.com>
From:   "Joseph, Jithu" <jithu.joseph@intel.com>
In-Reply-To: <c225e969-a88d-f3b4-de44-ef71f44dd463@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TzFGZUxDS04zT0hhWDByczk5WjUwV3p1V3J2Y2VTZTlETWZRN1Z0R2VpZk93?=
 =?utf-8?B?OTFJeVJqeStpQTRXS1VNcXQxQVFVeC81MXE4RGlWWXM4QktXRUM2QTBMZ1A3?=
 =?utf-8?B?c1BCZnBlRWZZTHVRT1RQak1xSktBUlllK29ScGtOcyt5dnVQYVRDTmZyK2pC?=
 =?utf-8?B?c0hEQ1RtMEZEd3R2WTF5SEs0M05MSkhzRlJHTXd5VVJjVjI2RGFWb09hNVpq?=
 =?utf-8?B?OGNoc3VMekxScTNhK2RRZFJMU2laOEVHc2QzVGlRTzVLUnRPdUloN0tFam1t?=
 =?utf-8?B?d3llSG5xc0xSYWd3cHgvWHpvZWlnYzlKdjFURVFUZlNtdzgydTE5a3dCZVph?=
 =?utf-8?B?eU9rM2thSGtVUnRuTXUwVFM2cTBOazF4VWFNUTdoTHBHWFhwSE40dTFoU2Y2?=
 =?utf-8?B?MGswb09FWFBLSlVaVWFHT1BBZk5ZUU1lUzMxaGdrNk9iWW1HbWNDZ0ZCV21z?=
 =?utf-8?B?cStYWEJrT05XQlg3ckFPbzhkanlsTFB5eTZLMVBWaVNvbWxDQ29hT3FoZkE0?=
 =?utf-8?B?dTV6NWtLMCthYmw2aHRyb0w3OWpZZVRtMUhqbklNVXE3d0gyY0dxeUNjeGpE?=
 =?utf-8?B?RUlyRW9GdFl6aG5MbXU3czRYd2RIVi9yUHVyaFNyeGRmcSs2STBwc2IybUNL?=
 =?utf-8?B?TWtLRVpnTDlTYXhGaGFaUzBPTVh1YnhXNlphV0xBOHF2cWJ2cmxQK2FWWHNt?=
 =?utf-8?B?eHpHZXBkdDRweGxBdFRodXpaLzZUMTNqOGU2V09uVjNJalZqdHpJc2ZzK2dK?=
 =?utf-8?B?Sy9MMnRMQVJ1em9SMzZiM2tYRldySFR6YXNOTDllblU3M1lkVFJvYTVDNDBM?=
 =?utf-8?B?SmZHNUh1cm9DVUZCTUtSVXBwbkdjeVdrb0hDMmNKbDlHT2NEWlEwS1Fuc1hE?=
 =?utf-8?B?dlg1cXYxc2xodjJBRjFFdmZxZDh2QWpMZU1VMS9RL3QzZDVleVhMdW1oMndM?=
 =?utf-8?B?bDE4dldMRnkzeWZsOGhSUTVHRFJyZWZIZzk4bXpEODdSSVptK3FldUErd2hY?=
 =?utf-8?B?UnlyZlI2OUtJQytndXR3UjlDS2JYMHlVY05UYkJaQnVjUkhtcUFSRjBZK0tB?=
 =?utf-8?B?TXFFSjhMTXpFZnpIWDlQSEhxMEowMjdJZUpnMlp5WlRhWWNRMFNQY2xHMWpE?=
 =?utf-8?B?ZzRuWFFJMjI0TVVob2UvSW81MEdyMG5TUWVmNXJaa3VxY2I4MXJaNTRMZ245?=
 =?utf-8?B?dmZXUVBBcGJmMTI1VXZvUHY5czJWZ2diUUo1cVNSTjF2a09pNWdZSE5rMzFC?=
 =?utf-8?B?Y3JuZytUNjAvWWdyVGxDY2RrOXhuY3Z4NFljV3BESTd0YUMvZDlkK0pCdWtX?=
 =?utf-8?B?S0tyNm82TUFVMnVNeCtMTkVVSnY0ZWFTWm1EVllIZ2dVSFgxd0hYVjVRTzhC?=
 =?utf-8?B?N1FWSk9OSmVqV1RXVDBJTWZDZUNmVDI0M3Z4eEUrakRNTjBkaWxMT0pkem53?=
 =?utf-8?B?Ny9yYUV6emVMWGNzeU4zZ1lEK0RPN0craTVTT3Z6UXJOUThLYi9mSUMvUzll?=
 =?utf-8?B?NUlrZHhNNnlIektWbm5PMmNlbW54MzhsdjdLREpyb1ZCZWdqSkFzT1BXUXlL?=
 =?utf-8?B?VlYyYVhiRVl2WmMvL21aZVRVSXAwakcyaDcxakF1ZGZNNXdaSGlQa0pFdVJz?=
 =?utf-8?B?NDdrcTIwVUNGWkpodzA3Y2FXUFJGSzZEbkxTSTZPZzVEQmZXVG1Kc3FzZk8v?=
 =?utf-8?B?UzJqQjhRMktnSUVhYXptK3ZadUJFZG52QmRxK2JZRUNTV1E3dmtYTkFaMFE4?=
 =?utf-8?B?dVFSRjloRGxIUzIyU3lyZnMxU3JBd1F5QmhoYUs2SWlKeE9SNStRUGc5RU5T?=
 =?utf-8?B?cDRSU3o4Y0dyL2g0N1NxbkYvU0RYZVdITkNOcDZTNkQ2MWFmUXlZSlZnZ1I4?=
 =?utf-8?B?eHRsMXpoS2hPcFBtUmJJejVUNkMyUWVPRm5TSTg5ajFDWWRzVlc0MlEvYU5E?=
 =?utf-8?B?SFpvSnovOWpSbjBhR2M1bGF4Rk1qaWZsdHc4VHJRdzVUOEF0ckwrZmpXZ0RI?=
 =?utf-8?B?V0FWSlpxQjN1LzNSMk1yeSs1dHRWb1MwNkMzWWhHaWRSN2crbUJvNEFEcFp2?=
 =?utf-8?B?a2ptc29OYTRFTERyYnlLTVpCWXZjM3QydE1GeWlQakN2dGEvemlEdDZhWGFu?=
 =?utf-8?B?TXh3bWdnT3JBdUhGdG5NQXJhc0JkbG9HejNpYlQrV1czYVlwbzh4TnYvREVy?=
 =?utf-8?B?bkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 96cd14c0-124f-44d7-649e-08dabc3c3516
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3771.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Nov 2022 19:06:40.2941
 (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: Ff5WCfDtXqnrE+eK7rQAhCf/QKkH8UvC6y96eVmU/DADzwucXpgyVAvS9F8mCvydTX8uMbBlz2ajaMMv9a8cUA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB6314
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


On 11/1/2022 12:28 AM, Sohil Mehta wrote:
> On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> 
>> Refactor header version as a parameter and expose this function.
> 
> Isn't the header version part of the microcode data itself?
> 

Header forms the first 48 bytes in a microcode file followed by contents

> Microcode Format
>    +----------------------+  Base
>    |Header Version        |
>    +----------------------+
>    |Update revision       |
>    +----------------------+
> 
> If so, why the need to pass it as a parameter to sanity_check()?
> 

It is for sanity checking if the contents of the input file matches with the usage scenario.
All microcode files have 1 in the header and IFS test images will use 2. 

Existing usages check if the microcode file has Header version 1. This (along with other sanity checks) is done by the below code 
microcode_intel_sanity_check(data, false, MICROCODE_HEADER_VER)

and IFS usages check whether the file has Header version 2 (along with other sanity checks shared with microcode files)
microcode_intel_sanity_check(data, true, IFS_HEADER_VER))

In other words the sanity check will flag somebody mistakenly passing in a microcode file as IFS test image (and vice-versa)


>>
>> No functional change intended.
> 
> Maybe skip this statement. Apart from adding a parameter to an newly exported function, there is a change in an error print as well.
> 
>> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
>> index 5473b094baee..bc3f33a25d7a 100644
>> --- a/arch/x86/kernel/cpu/microcode/intel.c
>> +++ b/arch/x86/kernel/cpu/microcode/intel.c
>> @@ -37,6 +37,8 @@
>>   #include <asm/setup.h>
>>   #include <asm/msr.h>
>>   +#define MICROCODE_HEADER_VER 1
>> +
> 
> Should this define be in a central location, like microcode_intel.h?
> 

It can be moved there

> You would soon be adding a define for IFS_HEADER_VER. Having them defined together would make it easier to follow.
> 
> Sohil


Jithu