[edk2-devel] [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode

Ni, Ray posted 4 patches 2 years, 12 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/20210402055807.858-1-ray.ni@intel.com
OvmfPkg/AmdSev/AmdSevX64.dsc                  |   1 +
OvmfPkg/Bhyve/BhyveX64.dsc                    |   1 +
OvmfPkg/OvmfPkgIa32.dsc                       |   1 +
OvmfPkg/OvmfPkgIa32X64.dsc                    |   1 +
OvmfPkg/OvmfPkgX64.dsc                        |   1 +
OvmfPkg/OvmfXen.dsc                           |   1 +
UefiCpuPkg/Include/Library/MicrocodeLib.h     | 120 +++++
.../Library/MicrocodeLib/MicrocodeLib.c       | 322 ++++++++++++
.../Library/MicrocodeLib/MicrocodeLib.inf     |  32 ++
UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   1 +
UefiCpuPkg/Library/MpInitLib/Microcode.c      | 484 ++++--------------
UefiCpuPkg/Library/MpInitLib/MpLib.h          |   1 +
UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   1 +
UefiCpuPkg/UefiCpuPkg.dec                     |   5 +-
UefiCpuPkg/UefiCpuPkg.dsc                     |   1 +
UefiPayloadPkg/UefiPayloadPkg.dsc             |   1 +
16 files changed, 582 insertions(+), 392 deletions(-)
create mode 100644 UefiCpuPkg/Include/Library/MicrocodeLib.h
create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
[edk2-devel] [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode
Posted by Ni, Ray 2 years, 12 months ago
The patch set creates a new MicrocodeLib for loading microcode.
Then updates all platforms to include this lib in DSC.
Then updates the MpInitLib to consume this lib.

Edk2-platforms change will be sent out in a separate patch set.


Ray Ni (4):
  UefiCpuPkg: Add MicrocodeLib for loading microcode
  OvmfPkg: Add MicrocodeLib in DSC files.
  UefiPayloadPkg/UefiPayloadPkg.dsc: Consume MicrocodeLib
  UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code

 OvmfPkg/AmdSev/AmdSevX64.dsc                  |   1 +
 OvmfPkg/Bhyve/BhyveX64.dsc                    |   1 +
 OvmfPkg/OvmfPkgIa32.dsc                       |   1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                    |   1 +
 OvmfPkg/OvmfPkgX64.dsc                        |   1 +
 OvmfPkg/OvmfXen.dsc                           |   1 +
 UefiCpuPkg/Include/Library/MicrocodeLib.h     | 120 +++++
 .../Library/MicrocodeLib/MicrocodeLib.c       | 322 ++++++++++++
 .../Library/MicrocodeLib/MicrocodeLib.inf     |  32 ++
 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   1 +
 UefiCpuPkg/Library/MpInitLib/Microcode.c      | 484 ++++--------------
 UefiCpuPkg/Library/MpInitLib/MpLib.h          |   1 +
 UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   1 +
 UefiCpuPkg/UefiCpuPkg.dec                     |   5 +-
 UefiCpuPkg/UefiCpuPkg.dsc                     |   1 +
 UefiPayloadPkg/UefiPayloadPkg.dsc             |   1 +
 16 files changed, 582 insertions(+), 392 deletions(-)
 create mode 100644 UefiCpuPkg/Include/Library/MicrocodeLib.h
 create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
 create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf

-- 
2.27.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73637): https://edk2.groups.io/g/devel/message/73637
Mute This Topic: https://groups.io/mt/81796730/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode
Posted by Laszlo Ersek 2 years, 11 months ago
Hi Ray,

On 04/02/21 07:58, Ni, Ray wrote:
> The patch set creates a new MicrocodeLib for loading microcode.
> Then updates all platforms to include this lib in DSC.
> Then updates the MpInitLib to consume this lib.
> 
> Edk2-platforms change will be sent out in a separate patch set.
> 
> 
> Ray Ni (4):
>   UefiCpuPkg: Add MicrocodeLib for loading microcode
>   OvmfPkg: Add MicrocodeLib in DSC files.
>   UefiPayloadPkg/UefiPayloadPkg.dsc: Consume MicrocodeLib
>   UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code
> 
>  OvmfPkg/AmdSev/AmdSevX64.dsc                  |   1 +
>  OvmfPkg/Bhyve/BhyveX64.dsc                    |   1 +
>  OvmfPkg/OvmfPkgIa32.dsc                       |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |   1 +
>  OvmfPkg/OvmfPkgX64.dsc                        |   1 +
>  OvmfPkg/OvmfXen.dsc                           |   1 +
>  UefiCpuPkg/Include/Library/MicrocodeLib.h     | 120 +++++
>  .../Library/MicrocodeLib/MicrocodeLib.c       | 322 ++++++++++++
>  .../Library/MicrocodeLib/MicrocodeLib.inf     |  32 ++
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   1 +
>  UefiCpuPkg/Library/MpInitLib/Microcode.c      | 484 ++++--------------
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          |   1 +
>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   1 +
>  UefiCpuPkg/UefiCpuPkg.dec                     |   5 +-
>  UefiCpuPkg/UefiCpuPkg.dsc                     |   1 +
>  UefiPayloadPkg/UefiPayloadPkg.dsc             |   1 +
>  16 files changed, 582 insertions(+), 392 deletions(-)
>  create mode 100644 UefiCpuPkg/Include/Library/MicrocodeLib.h
>  create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
>  create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
> 

(1) I think we should use a new TianoCore feature request BZ for this
feature, and the commit messages should link it. (I understand the
library only factors out existent logic, but still.)

(2) As I understand it, a platform can provide microcode in three ways:
- via the "microcode patch" GUIDed HOB (PEI and DXE phases both),
- via the "shadow microcode" PPI (PEI phase only),
- via the PcdCpuMicrocodePatch* PCDs (PEI and DXE phases both).

If a platform uses none of these methods (for example, OVMF does not),
then I think it would benefit from a Null instance of the new
MicrocodeLib class.

Would you consider introducing a Null instance too, and using that one
in the OVMF DSC files?

Thanks,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73720): https://edk2.groups.io/g/devel/message/73720
Mute This Topic: https://groups.io/mt/81796730/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode
Posted by Ni, Ray 2 years, 11 months ago
On Tue, Apr  6, 2021 at 08:03 PM, Laszlo Ersek wrote:

>
> (1) I think we should use a new TianoCore feature request BZ for this
> feature, and the commit messages should link it. (I understand the
> library only factors out existent logic, but still.)

sure. https://bugzilla.tianocore.org/show_bug.cgi?id=3303

> 
> (2) As I understand it, a platform can provide microcode in three ways:
> - via the "microcode patch" GUIDed HOB (PEI and DXE phases both),
> - via the "shadow microcode" PPI (PEI phase only),
> - via the PcdCpuMicrocodePatch* PCDs (PEI and DXE phases both).
> 
> If a platform uses none of these methods (for example, OVMF does not),
> then I think it would benefit from a Null instance of the new
> MicrocodeLib class.
> 
> Would you consider introducing a Null instance too, and using that one
> in the OVMF DSC files?
>

No. I don't think it's necessary for a NULL instance.
Because:
1. MicrocodePatch GUIDed HOB is only produced when "shadow microcode" PPI or "PcdCpuMicrocodePatch *" exists.
    I will further simplify today's MpInitLib to skip loading microcode when this HOB exists (because DXE re-load is unnecessary).
    This is captured by https://bugzilla.tianocore.org/show_bug.cgi?id=3155.

2. Today's logic only calls the MicrocodeLib API when "shadow microcode" PPI or "PcdCpuMicrocodePatch *" exists.
    Even NULL instance is provided, it's not called.

3. MicrocodeLib calls ASSERT() when the supplied microcode binary is NULL.
    If the logic in MpInitLib is changed by accident to allow the call to MicrocodeLib even in OVMF, the assertion can catch this.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73753): https://edk2.groups.io/g/devel/message/73753
Mute This Topic: https://groups.io/mt/81796730/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode
Posted by Laszlo Ersek 2 years, 11 months ago
On 04/07/21 04:43, Ni, Ray wrote:
> On Tue, Apr  6, 2021 at 08:03 PM, Laszlo Ersek wrote:
> 
>>
>> (1) I think we should use a new TianoCore feature request BZ for this
>> feature, and the commit messages should link it. (I understand the
>> library only factors out existent logic, but still.)
> 
> sure. https://bugzilla.tianocore.org/show_bug.cgi?id=3303
> 
>>
>> (2) As I understand it, a platform can provide microcode in three ways:
>> - via the "microcode patch" GUIDed HOB (PEI and DXE phases both),
>> - via the "shadow microcode" PPI (PEI phase only),
>> - via the PcdCpuMicrocodePatch* PCDs (PEI and DXE phases both).
>>
>> If a platform uses none of these methods (for example, OVMF does not),
>> then I think it would benefit from a Null instance of the new
>> MicrocodeLib class.
>>
>> Would you consider introducing a Null instance too, and using that one
>> in the OVMF DSC files?
>>
> 
> No. I don't think it's necessary for a NULL instance.
> Because:
> 1. MicrocodePatch GUIDed HOB is only produced when "shadow microcode" PPI or "PcdCpuMicrocodePatch *" exists.
>     I will further simplify today's MpInitLib to skip loading microcode when this HOB exists (because DXE re-load is unnecessary).
>     This is captured by https://bugzilla.tianocore.org/show_bug.cgi?id=3155.
> 
> 2. Today's logic only calls the MicrocodeLib API when "shadow microcode" PPI or "PcdCpuMicrocodePatch *" exists.
>     Even NULL instance is provided, it's not called.
> 
> 3. MicrocodeLib calls ASSERT() when the supplied microcode binary is NULL.
>     If the logic in MpInitLib is changed by accident to allow the call to MicrocodeLib even in OVMF, the assertion can catch this.

I was thinking that a Null instance would be useful for eliminating dead
code from the binary (because: the PPI check is dynamic, unlike a
compile-time PCD check, so it can not be optimized out). But it's not
critical.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73775): https://edk2.groups.io/g/devel/message/73775
Mute This Topic: https://groups.io/mt/81796730/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-