Re: [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline


Yao, Jiewen
 

Hi James
You are right that initially EDKII did recommend to put a lib under Library dir.
But with more and more feature, people start realizing that it is NOT efficient way to identify a *feature*.
We changed the direction later - if we can define a feature dir, we can lib to feature dir.


I used " find . -name *Lib*.inf -print", because I cannot assume the Lib is under Library dir for feature.

I can find lots of stuff. Most of them are under Library, below is NOT in *Pkg/Library/
=============
./edk2/ArmPkg/Drivers/ArmGic/ArmGicLib.inf
./edk2/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
./edk2/ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
./edk2/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/TestBaseCryptLibHost.inf
./edk2/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/TestBaseCryptLibShell.inf
./edk2/FmpDevicePkg/FmpDxe/FmpDxeLib.inf
./edk2/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/FmpDependencyLibUnitTestsHost.inf
./edk2/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/FmpDependencyLibUnitTestsUefi.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseLib/BaseLibUnitTestsHost.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseLib/BaseLibUnitTestsUefi.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibDxe.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibHost.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibPei.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibSmm.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibUefiShell.inf
./edk2/OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf
./edk2/OvmfPkg/Csm/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf
./edk2/OvmfPkg/Csm/LegacyBootManagerLib/LegacyBootManagerLib.inf
============

If I search on edk2-platform (https://github.com/tianocore/edk2-platforms), I can find more:
============
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BaseFuncLib/BaseFuncLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BaseGpioCheckConflictLib/BaseGpioCheckConflictLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BaseGpioCheckConflictLibNull/BaseGpioCheckConflictLibNull.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BasePlatformHookLib/BasePlatformHookLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardAcpiLib/SmmBoardAcpiEnableLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardAcpiLib/SmmMultiBoardAcpiSupportLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardInitLib/PeiBoardInitPostMemLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardInitLib/PeiBoardInitPreMemLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardInitLib/PeiMultiBoardInitPostMemLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardInitLib/PeiMultiBoardInitPreMemLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/DxePolicyBoardConfigLib/DxePolicyBoardConfigLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/PeiPolicyBoardConfigLib/PeiPolicyBoardConfigLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/Features/Tbt/Library/DxeTbtPolicyLib/DxeTbtPolicyLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/Features/Tbt/Library/PeiDxeSmmTbtCommonLib/TbtCommonLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/Features/Tbt/Library/PeiTbtPolicyLib/PeiTbtPolicyLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/Features/Tbt/Library/Private/PeiDTbtInitLib/PeiDTbtInitLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/FspWrapper/Library/PeiFspPolicyInitLib/PeiFspPolicyInitLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/FspWrapper/Library/PeiSiliconPolicyUpdateLibFsp/PeiSiliconPolicyUpdateLibFsp.inf
./Platform/Intel/CometlakeOpenBoardPkg/Policy/Library/DxePolicyUpdateLib/DxePolicyUpdateLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/Policy/Library/PeiPolicyInitLib/PeiPolicyInitLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/Policy/Library/PeiPolicyUpdateLib/PeiPolicyUpdateLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/Features/Tbt/Library/DxeTbtPolicyLib/DxeTbtPolicyLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/Features/Tbt/Library/PeiDxeSmmTbtCommonLib/TbtCommonLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/Features/Tbt/Library/PeiTbtPolicyLib/PeiTbtPolicyLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/Features/Tbt/Library/Private/PeiDTbtInitLib/PeiDTbtInitLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/FspWrapper/Library/PeiSiliconPolicyNotifyLib/PeiPreMemSiliconPolicyNotifyLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/FspWrapper/Library/PeiSiliconPolicyNotifyLib/PeiPreMemSiliconPolicyNotifyLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/FspWrapper/Library/PeiSiliconPolicyUpdateLibFsp/PeiSiliconPolicyUpdateLibFsp.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BasePlatformHookLib/BasePlatformHookLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardAcpiLib/DxeBoardAcpiTableLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardAcpiLib/DxeMultiBoardAcpiSupportLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardAcpiLib/SmmBoardAcpiEnableLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardAcpiLib/SmmMultiBoardAcpiSupportLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardInitLib/PeiBoardInitPostMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardInitLib/PeiBoardInitPreMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardInitLib/PeiMultiBoardInitPostMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardInitLib/PeiMultiBoardInitPreMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Policy/Library/DxeSiliconPolicyUpdateLib/DxeSiliconPolicyUpdateLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/FspWrapper/Library/PeiSiliconPolicyUpdateLibFsp/PeiSiliconPolicyUpdateLibFsp.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BasePlatformHookLib/BasePlatformHookLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardAcpiLib/DxeBoardAcpiTableLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardAcpiLib/DxeMultiBoardAcpiSupportLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardAcpiLib/SmmBoardAcpiEnableLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardAcpiLib/SmmMultiBoardAcpiSupportLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardInitLib/PeiBoardInitPostMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardInitLib/PeiBoardInitPreMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardInitLib/PeiMultiBoardInitPostMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardInitLib/PeiMultiBoardInitPreMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Policy/Library/DxeSiliconPolicyUpdateLib/DxeSiliconPolicyUpdateLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Policy/Library/PeiSiliconPolicyUpdateLib/PeiSiliconPolicyUpdateLib.inf
./Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/BoardAcpiEnableLibNull.inf
./Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/BoardAcpiTableLibNull.inf
./Platform/Intel/MinPlatformPkg/Acpi/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf
./Platform/Intel/MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/DxeMultiBoardAcpiSupportLib.inf
./Platform/Intel/MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/SmmMultiBoardAcpiSupportLib.inf
./Platform/Intel/MinPlatformPkg/Bds/Library/BoardBootManagerLibNull/BoardBootManagerLibNull.inf
./Platform/Intel/MinPlatformPkg/Bds/Library/DxePlatformBootManagerLib/DxePlatformBootManagerLib.inf
./Platform/Intel/MinPlatformPkg/Flash/Library/SpiFlashCommonLibNull/SpiFlashCommonLibNull.inf
./Platform/Intel/MinPlatformPkg/FspWrapper/Library/DxeFspWrapperPlatformLib/DxeFspWrapperPlatformLib.inf
./Platform/Intel/MinPlatformPkg/FspWrapper/Library/PeiFspWrapperHobProcessLib/PeiFspWrapperHobProcessLib.inf
./Platform/Intel/MinPlatformPkg/FspWrapper/Library/PeiFspWrapperPlatformLib/PeiFspWrapperPlatformLib.inf
./Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecFspWrapperPlatformSecLib.inf
./Platform/Intel/MinPlatformPkg/Pci/Library/PciHostBridgeLibSimple/PciHostBridgeLibSimple.inf
./Platform/Intel/MinPlatformPkg/Pci/Library/PciSegmentInfoLibSimple/PciSegmentInfoLibSimple.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/BoardInitLibNull/BoardInitLibNull.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/MultiBoardInitSupportLib/DxeMultiBoardInitSupportLib.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/MultiBoardInitSupportLib/PeiMultiBoardInitSupportLib.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/PeiReportFvLib/PeiReportFvLib.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/ReportCpuHobLib/ReportCpuHobLib.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/SecBoardInitLibNull/SecBoardInitLibNull.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/SiliconPolicyInitLibNull/SiliconPolicyInitLibNull.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/SiliconPolicyUpdateLibNull/SiliconPolicyUpdateLibNull.inf
./Platform/Intel/MinPlatformPkg/Tcg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SecTestPointCheckLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLibNull/TestPointCheckLibNull.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/DxeTestPointLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/PeiTestPointLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/SmmTestPointLib.inf
......
============

-----Original Message-----
From: James Bottomley <jejb@linux.ibm.com>
Sent: Monday, July 26, 2021 10:50 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
dovmurik@linux.ibm.com
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum
<tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>; Hubertus Franke
<frankeh@us.ibm.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
Jordan L <jordan.l.justen@intel.com>; Ashish Kalra <ashish.kalra@amd.com>;
Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas
<erdemaktas@google.com>; Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
<thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami
Mujawar <sami.mujawar@arm.com>
Subject: Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
kernel/initrd/cmdline

On Mon, 2021-07-26 at 00:55 +0000, Yao, Jiewen wrote:
Hi James
"However, this ran into problems when it was decided AmdSev shouldn't
have it's own Library."

I am not clear on the history. Would you please clarify why AmdSev
should not have its own library?
The history predates me. It was already done for the Bhyve package
which also has a modified PlatformBootManagerLib when I came along with
this. However, only having Library in the top level package seems to
be a common edk2 pattern if you run a find.

It looks not reasonable to me. AmdSev is just a feature. A feature
may have its own library. We have enough examples.
We do? Running

find . -name Library -print

only turns up

./FmpDevicePkg/Test/UnitTest/Library

As not following the top level package only pattern.

Also, the instance name "Grub" is very confusing. I compared
PlatformBootManagerLib and PlatformBootManagerLibGrub. This is just a
customized PlatformBootManagerLib.
It's called Grub because it places Grub in the Fv for combined pre-
attestation. Either SEV or TDX could use this (Although TDX looks
likely not to want to).

For example, XEN feature removing and PIIX4 difference has nothing to
do with Grub...
=================
PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x60), 0x0b); // A
PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x61), 0x0b); // B
PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x62), 0x0a); // C
PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x63), 0x0a); // D
=================
It's part of the boot path stripping to make sure there's a hard
failure if Grub fails to execute. There's a Bugzilla requiring more of
this because a grub only booting platform library needs fewer
extraneous things which could constitute an attack surface for the
injected secret.

It is a big misleading. Can we move the PlatformBootManagerLibGrub To
AmdSev now?
I think you probably want to ask around older edk2 package maintainers
and see if there's any reason for this pattern, which seems to be
strongly enforced. If no-one can remember, then likely it can be
broken.

James

Join devel@edk2.groups.io to automatically receive all group messages.