[edk2-devel] [Patch 0/2] Shadow microcode patch according to FIT microcode table.

Siyuan, Fu posted 2 patches 1 week ago
Failed in applying to current master (apply log)
.../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

[edk2-devel] [Patch 0/2] Shadow microcode patch according to FIT microcode table.

Posted by Siyuan, Fu 1 week ago
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

-- 
2.19.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52998): https://edk2.groups.io/g/devel/message/52998
Mute This Topic: https://groups.io/mt/69521538/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Liming Gao 1 week ago
Siyuan:
  This is new feature, please add it into https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning

  And, FIT header file is moved from edk2-platform to edk2. After this change, you also need to remove the one in edk2-platform. Right?

Thanks
Liming
-----Original Message-----
From: Fu, Siyuan <siyuan.fu@intel.com> 
Sent: 2020年1月8日 12:25
To: 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>; lersek@redhat.com
Subject: [Patch 0/2] Shadow microcode patch according to FIT microcode table.

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

--
2.19.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53038): https://edk2.groups.io/g/devel/message/53038
Mute This Topic: https://groups.io/mt/69521538/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Siyuan, Fu 1 week ago
Hi, Liming

Yes I will make another patch to remove the edk2-platforms header file once this patch is committed.

Best Regards
Siyuan 

> -----Original Message-----
> From: Gao, Liming <liming.gao@intel.com>
> Sent: 2020年1月9日 10:04
> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; lersek@redhat.com
> Subject: RE: [Patch 0/2] Shadow microcode patch according to FIT microcode
> table.
> 
> Siyuan:
>   This is new feature, please add it into
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-
> Planning
> 
>   And, FIT header file is moved from edk2-platform to edk2. After this change,
> you also need to remove the one in edk2-platform. Right?
> 
> Thanks
> Liming
> -----Original Message-----
> From: Fu, Siyuan <siyuan.fu@intel.com>
> Sent: 2020年1月8日 12:25
> To: 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>; lersek@redhat.com
> Subject: [Patch 0/2] Shadow microcode patch according to FIT microcode
> table.
> 
> 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
> 
> --
> 2.19.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53039): https://edk2.groups.io/g/devel/message/53039
Mute This Topic: https://groups.io/mt/69521538/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Laszlo Ersek 1 week ago
(+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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53007): https://edk2.groups.io/g/devel/message/53007
Mute This Topic: https://groups.io/mt/69521538/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Siyuan, Fu 1 week ago
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?

Best Regards,
Siyuan

> 
> > 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53011): https://edk2.groups.io/g/devel/message/53011
Mute This Topic: https://groups.io/mt/69521538/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Laszlo Ersek 1 week ago
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53013): https://edk2.groups.io/g/devel/message/53013
Mute This Topic: https://groups.io/mt/69521538/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Laszlo Ersek 1 week ago
On 01/08/20 10:42, Laszlo Ersek wrote:

> 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.

Sorry I messed up the first half of the last paragraph. I meant:

With the spec's current "naked pointer" approach, the chance to
incorrectly deduce "yes FIT" is ((2^64)-3) in (2^64). So an incorrect
decision is almost certain.

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53010): https://edk2.groups.io/g/devel/message/53010
Mute This Topic: https://groups.io/mt/69521538/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-