Re: [edk2-devel] [PATCH v5 10/28] OvmfPkg: Add Memory Protection Library Definitions to Platforms

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

On 10/9/23 02:07, Taylor Beebe wrote:
> Add library classes for SetMemoryProtectionsLib and
> GetMemoryProtectionsLib to OvmfPkg platfomrs.
> 
> Signed-off-by: Taylor Beebe <taylor.d.beebe@gmail.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Peter Grehan <grehan@freebsd.org>
> Cc: Corvin Köhne <corvink@freebsd.org>
> Cc: Jianyong Wu <jianyong.wu@arm.com>
> Cc: Anatol Belski <anbelski@linux.microsoft.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien@xen.org>
> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc                          |  1 +
>  OvmfPkg/Bhyve/BhyveX64.dsc                            |  1 +
>  OvmfPkg/CloudHv/CloudHvX64.dsc                        |  1 +
>  OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc | 15 +++++++++++++++
>  OvmfPkg/IntelTdx/IntelTdxX64.dsc                      |  2 ++
>  OvmfPkg/Microvm/MicrovmX64.dsc                        |  2 ++
>  OvmfPkg/OvmfPkgIa32.dsc                               |  1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                            |  1 +
>  OvmfPkg/OvmfPkgX64.dsc                                |  1 +
>  OvmfPkg/OvmfXen.dsc                                   |  2 ++
>  OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc                   |  2 ++
>  11 files changed, 29 insertions(+)
> 
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index 302c90e7c2b4..b67b50b833b9 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -203,6 +203,7 @@ [LibraryClasses]
>    S3BootScriptLib|MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
>  
>  !include OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc
> +!include OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
>  
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index 6693342c5f6e..5af5831196f6 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -229,6 +229,7 @@ [LibraryClasses]
>    XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
>  
>  !include OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc
> +!include OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
>  
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
> index 35942e02df93..c550ebcd659e 100644
> --- a/OvmfPkg/CloudHv/CloudHvX64.dsc
> +++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
> @@ -239,6 +239,7 @@ [LibraryClasses]
>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>  
>  !include OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc
> +!include OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
>  
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> diff --git a/OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc b/OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
> new file mode 100644
> index 000000000000..049fdef3f0c1
> --- /dev/null
> +++ b/OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
> @@ -0,0 +1,15 @@
> +##
> +#    SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +
> +#
> +# Memory Protection Libraries
> +#
> +[LibraryClasses.common]
> +  SetMemoryProtectionsLib|MdeModulePkg/Library/SetMemoryProtectionsLib/SetMemoryProtectionsLib.inf
> +
> +[LibraryClasses.common.SMM_CORE, LibraryClasses.common.DXE_SMM_DRIVER, LibraryClasses.common.MM_CORE_STANDALONE, LibraryClasses.common.MM_STANDALONE]
> +  GetMemoryProtectionsLib|MdeModulePkg/Library/GetMemoryProtectionsLib/MmGetMemoryProtectionsLib.inf
> +
> +[LibraryClasses.common.DXE_CORE, LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.UEFI_APPLICATION, LibraryClasses.common.UEFI_DRIVER]
> +  GetMemoryProtectionsLib|MdeModulePkg/Library/GetMemoryProtectionsLib/DxeGetMemoryProtectionsLib.inf
> diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> index 182ec3705dd3..a3370f45940c 100644
> --- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> +++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> @@ -211,6 +211,8 @@ [LibraryClasses]
>    Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
>    TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
>  
> +!include OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
> +
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>    CcExitLib|OvmfPkg/Library/CcExitLib/CcExitLib.inf
> diff --git a/OvmfPkg/Microvm/MicrovmX64.dsc b/OvmfPkg/Microvm/MicrovmX64.dsc
> index 0f26f2a9a97d..da5a4862bfdc 100644
> --- a/OvmfPkg/Microvm/MicrovmX64.dsc
> +++ b/OvmfPkg/Microvm/MicrovmX64.dsc
> @@ -238,6 +238,8 @@ [LibraryClasses]
>    Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
>    TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>  
> +!include OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
> +
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>    CcExitLib|OvmfPkg/Library/CcExitLib/CcExitLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index fcd3a3fda5f2..d4d14b69ef1d 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -246,6 +246,7 @@ [LibraryClasses]
>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>  
>  !include OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc
> +!include OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
>  
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index d0ae0b996d66..674010323df1 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -251,6 +251,7 @@ [LibraryClasses]
>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>  
>  !include OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc
> +!include OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
>  
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index a6811eee557e..08b70d76d292 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -267,6 +267,7 @@ [LibraryClasses]
>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>  
>  !include OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc
> +!include OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
>  
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index ccd3a873c714..d80e20a151d7 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -231,6 +231,8 @@ [LibraryClasses]
>    DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
>  !endif
>  
> +!include OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
> +
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>    CcExitLib|UefiCpuPkg/Library/CcExitLibNull/CcExitLibNull.inf
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
> index 34b2037824f1..dd25a34796e5 100644
> --- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
> +++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
> @@ -112,6 +112,8 @@ [LibraryClasses.common]
>    TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf
>  !endif
>  
> +!include OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
> +
>  [LibraryClasses.common.DXE_DRIVER]
>    AcpiPlatformLib|OvmfPkg/Library/AcpiPlatformLib/DxeAcpiPlatformLib.inf
>    ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf

I'm unconvinced that a new DSC include file buys us much in this case.
But, I'm not dead-set against it; if other reviewers like this approach,
then I'm fine.

However, the new file
"OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc" includes neither
a top-level @file comment (bad, but not critical), nor a Copyright
notice (which *is* critical).

In fact, none of the dsc.inc files under OvmfPkg/Include/Dsc have
copyright notices, and some of the fdf.inc files under
/OvmfPkg/Include/Fdf lack them too. That's a serious bug IMO that goes
back to the introduction of those include files.

At least don't perpetuate that mistake here, please.

The EccCheck CI plugin actually enforces copyright notices in metafiles.
We have at least two reasons why ECC has not been catching these
copyright notice problems:

- EccCheck is disabled for OvmfPkg (not surprisingly: ECC also makes a
lot of invalid / inane complaints)

- I *think* ECC does not recognize "*.dsc.inc", "*.fdf.inc", etc as
metafiles.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109441): https://edk2.groups.io/g/devel/message/109441
Mute This Topic: https://groups.io/mt/101843352/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-