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

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

(+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:

> VOID
> ShadowMicrocodeUpdatePatch (
>   IN OUT CPU_MP_DATA             *CpuMpData
>   )
> {
>   EFI_STATUS     Status;
>
>   if (FeaturePcdGet (PcdFirmwareInterfaceTableSupported)) {
>     Status = ShadowMicrocodePatchByFit (CpuMpData);
>   } else {
>     Status = EFI_UNSUPPORTED;
>   }
>
>   if (EFI_ERROR (Status)) {
>     ShadowMicrocodePatchByPcd (CpuMpData);
>   }
> }

Alternatively, leave ShadowMicrocodeUpdatePatch() as it is currently
proposed, but start ShadowMicrocodePatchByFit() as follows:

>   if (!FeaturePcdGet (PcdFirmwareInterfaceTableSupported)) {
>     return EFI_UNSUPPORTED;
>   }


In closing: it seems short-sighted that the FIT specification placed a
"naked" pointer at a fixed offset in flash, rather than a three-field
structure consisting of:
- a GUID,
- preceded by a structure size,
- preceded by the FIT pointer.

Because, using a GUID-ed approach, the chance to *incorrectly* deduce
"yes FIT" would be 1 in (2^128) -- all 128-bit values except one magic
value would indicate "no FIT". That's good.

Whereas, with the spec's current "naked pointer" approach, the chance to
*correctly* deduce "yes FIT" is 3 in (2^64) -- all 64-bit values except
three magic values indicate "yes FIT". Not good.

Thanks
Laszlo