Re: [Patch 0/2] Shadow microcode patch according to FIT microcode table.

["Laszlo Ersek" <lersek@redhat.com>]

On 01/08/20 11:58, Fu, Siyuan wrote:
> Hi, Laszlo
> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: 2020年1月8日 17:43
>> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
>> <ray.ni@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>
>> Subject: Re: [Patch 0/2] Shadow microcode patch according to FIT microcode
>> table.
>>
>> (+Tom)
>>
>> On 01/08/20 05:25, Siyuan Fu wrote:
>>> The existing MpInitLib will shadow the microcode update patches from
>>> flash to memory and this is done by searching microcode region
>>> specified by PCD PcdCpuMicrocodePatchAddress and
>>> PcdCpuMicrocodePatchRegionSize.
>>> This brings a limition to platform FW that all the microcode patches
>>> must be placed in one continuous flash space.
>>>
>>> This patch set shadows microcode update according to FIT microcode
>>> entries if it's present, otherwise it will fallback to original logic
>>> (by PCD).
>>>
>>> Patch 1/2: Add FIT header file to MdePkg.
>>> Patch 2/2: Update microcode loader to shadow microcode according to FIT.
>>>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>
>>> Siyuan Fu (2):
>>>   MdePkg: Add header file for Firmware Interface Table specification.
>>>   UefiCpuPkg: Shadow microcode patch according to FIT microcode entry.
>>>
>>>  .../IndustryStandard/FirmwareInterfaceTable.h |  76 ++++++
>>>  UefiCpuPkg/Library/MpInitLib/Microcode.c      | 255 +++++++++++++-----
>>>  UefiCpuPkg/Library/MpInitLib/MpLib.c          |   4 +-
>>>  UefiCpuPkg/Library/MpInitLib/MpLib.h          |   7 +-
>>>  4 files changed, 276 insertions(+), 66 deletions(-)
>>>  create mode 100644
>> MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
>>>
>>
>> I have now checked
>>
>>
>> https://www.intel.com/content/dam/www/public/us/en/documents/guides
>> /fit-bios-specification.pdf
>>
>> that is referenced in
>>
>>   https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449#c0
>>
>> I think it is wrong for MpInitLib to deduce anything from the flash
>> contents at fixed physical address 0xFFFFFFC0 *unless* the platform
>> advertizes up-front adherence to the FIT specification.
>>
>> The proposed logic in patch#2 goes like this:
>>
>> (1) read the UINT64 at physical address 0xFFFFFFC0
>>
>> (2) If the value read is zero, all-nibles-F, or all-nibbles-E, then
>>     assume "no FIT"
>>
>> (3) otherwise, dereference the value read, as a
>>     pointer-to-FIRMWARE_INTERFACE_TABLE_ENTRY
>>
>> (4) if the assumed FIRMWARE_INTERFACE_TABLE_ENTRY doesn't look right,
>>     assume "no FIT"
>>
>> (5) if "no FIT", then fall back to previous PCDs (which describe if
>>     there is a microcode update in the flash, and if so, where)
>>
>> This approach is wrong. If a platform has never heard of the FIT
>> specification, it may have any value at all at address 0xFFFFFFC0. The
>> value there may easily differ from the three values that step (2)
>> equates to "no FIT".
>>
>> Therefore the whole procedure above needs to be gated with a new
>> FeaturePCD (default value FALSE).
>>
>>
>> This is not a theoretical risk. Please see the following patches:
>>
>> * [edk2-devel] [RFC PATCH v3 36/43] UefiCpuPkg: Allow AP booting under
>> SEV-ES
>>   https://edk2.groups.io/g/devel/message/50976
>>   http://mid.mail-
>> archive.com/ddc10e014822c9a87a7dc9a44ee854751ffcf442.1574280425.git.
>> thomas.lendacky@amd.com
>>
>> * [edk2-devel] [RFC PATCH v3 37/43] OvmfPkg: Reserve a page in memory for
>> the SEV-ES AP reset vector
>>   https://edk2.groups.io/g/devel/message/50977
>>   http://mid.mail-
>> archive.com/706f17aa0ea3ed3f57c1e933e93164a94ba1a0cf.1574280425.git.
>> thomas.lendacky@amd.com
>>
>> Those patches describe a structure whose *last* field, and EFI_GUID, is
>> placed ath 0xffffffd0. This structure is extensible, and new fields are
>> supposed to be *prepended* to it. The currently defined first field
>> starts at 0xffffffca. Whereas the (exclusive) end of the FIT pointer is
>> at 0xffffffc8. This means there is a 2 (two) bytes gap between them. If
>> more than two bytes are prepended to the extensible structure in the
>> future, that may easily lead step (2) above to incorrectly determine
>> "yes FIT".
>>
>> So please introduce a new FeaturePCD in MdePkg (or even a dynamic
>> boolean PCD, if you think that's more flexible), and add logic similar
>> to:
> 
> I think a Feature PCD should be enough for this since whether using
> the FIT based booting flow is decided by platform owner and not expected
> to be changed once decided.
> 
> And considering that the FIT table is very critical to processor init flow, 
> I propose to add a DEBUG ASSERT if the Feature PCD is true, but couldn't
> find a valid FIT table structure. This could reminder platform developer 
> that the FIT address may have been incorrectly override by someone else,
> like the extensible structure as you mentioned above.
> 
> So the checking in ShadowMicrocodePatchByFit() should be as follows:
>  
>    if (!FeaturePcdGet (PcdFirmwareInterfaceTableSupported)) {
>      return EFI_UNSUPPORTED;
>    }
>   
>   if (FIRMWARE_INTERFACE_TABLE_ENTRY doesn't look right) {
>      ASSERT(FALSE);
>   }
> 
> What's your opinion on this?

Works fine for me.

Thanks!
Laszlo