On Tue, Mar 10, 2020 at 23:27:39 +0100, Laszlo Ersek wrote:
> * In the Intel whitepaper:
>
> --v--
> A Tour Beyond BIOS -- Secure SMM Communication
>
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Security-White-Papers
> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Secure_SMM_Communication.pdf
> --^--
>
> bullet#3 in section "Assumption and Recommendation", and bullet#4 in "Call
> for action", recommend enabling the (adaptive) Memory Type Information
> feature.
>
> * In the Intel whitepaper:
>
> --v--
> A Tour Beyond BIOS -- Memory Map and Practices in UEFI BIOS
>
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-white-papers
> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf
> --^--
>
> figure#6 describes the Memory Type Information feature in detail; namely
> as a feedback loop between the Platform PEIM, the DXE IPL PEIM, the DXE
> Core, and BDS.
>
> Implement the missing PlatformPei functionality in OvmfPkg, for fulfilling
> the Secure SMM Communication recommendation.
>
> In the longer term, OVMF should install the WSMT ACPI table, and this
> patch contributes to that.
>
> Notes:
>
> - the step in figure#6 where the UEFI variable is copied into the HOB is
> covered by the DXE IPL PEIM, in the DxeLoadCore() function,
>
> - "PcdResetOnMemoryTypeInformationChange" must be reverted to the DEC
> default TRUE value, because both whitepapers indicate that BDS needs to
> reset the system if the Memory Type Information changes.
>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Philippe Mathieu-Daudé <phi...@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=386
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
> OvmfPkg/OvmfPkgIa32.dsc | 2 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 2 +
> OvmfPkg/OvmfPkgX64.dsc | 2 +
> OvmfPkg/PlatformPei/PlatformPei.inf | 2 +
> OvmfPkg/PlatformPei/Platform.h | 5 +
> OvmfPkg/PlatformPei/MemTypeInfo.c | 147 ++++++++++++++++++++
> OvmfPkg/PlatformPei/Platform.c | 23 +--
> 7 files changed, 161 insertions(+), 22 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 4c9ff419c462..02ca17db8b2a 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -448,7 +448,9 @@ [PcdsFeatureFlag]
>
> [PcdsFixedAtBuild]
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> +!if $(SMM_REQUIRE) == FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> +!endif
> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 56681e9e4580..d08cf558c6aa 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -452,7 +452,9 @@ [PcdsFeatureFlag]
>
> [PcdsFixedAtBuild]
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> +!if $(SMM_REQUIRE) == FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> +!endif
> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 0e74b6f4d26c..b2dccc40a865 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -452,7 +452,9 @@ [PcdsFeatureFlag]
>
> [PcdsFixedAtBuild]
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> +!if $(SMM_REQUIRE) == FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> +!endif
> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
> b/OvmfPkg/PlatformPei/PlatformPei.inf
> index c51a6176aa2e..8531c63995c1 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -30,6 +30,7 @@ [Sources]
> FeatureControl.c
> Fv.c
> MemDetect.c
> + MemTypeInfo.c
> Platform.c
> Platform.h
> Xen.c
> @@ -112,6 +113,7 @@ [FeaturePcd]
> [Ppis]
> gEfiPeiMasterBootModePpiGuid
> gEfiPeiMpServicesPpiGuid
> + gEfiPeiReadOnlyVariable2PpiGuid
>
> [Depex]
> TRUE
> diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
> index 43f20f067f22..0484ec9e6b4c 100644
> --- a/OvmfPkg/PlatformPei/Platform.h
> +++ b/OvmfPkg/PlatformPei/Platform.h
> @@ -82,6 +82,11 @@ PeiFvInitialization (
> VOID
> );
>
> +VOID
> +MemTypeInfoInitialization (
> + VOID
> + );
> +
> VOID
> InstallFeatureControlCallback (
> VOID
> diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c
> b/OvmfPkg/PlatformPei/MemTypeInfo.c
> new file mode 100644
> index 000000000000..c709236a457a
> --- /dev/null
> +++ b/OvmfPkg/PlatformPei/MemTypeInfo.c
> @@ -0,0 +1,147 @@
> +/** @file
> + Produce a default memory type information HOB unless we can determine, from
> + the existence of the "MemoryTypeInformation" variable, that the DXE IPL
> PEIM
> + will produce the HOB.
> +
> + Copyright (C) 2017-2020, Red Hat, Inc.
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Guid/MemoryTypeInformation.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PeiServicesLib.h>
> +#include <Ppi/ReadOnlyVariable2.h>
> +#include <Uefi/UefiMultiPhase.h>
> +
> +#include "Platform.h"
> +
> +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = {
> + { EfiACPIMemoryNVS, 0x004 },
> + { EfiACPIReclaimMemory, 0x008 },
> + { EfiReservedMemoryType, 0x004 },
> + { EfiRuntimeServicesData, 0x024 },
> + { EfiRuntimeServicesCode, 0x030 },
> + { EfiBootServicesCode, 0x180 },
> + { EfiBootServicesData, 0xF00 },
> + { EfiMaxMemoryType, 0x000 }
> +};
Could you add a comment as to where these page counts come from?
Oh, right, they're just moved across from OvmfPkg/PlatformPei/Platform.c.
It *would* be nice if that could be cleaned up at the same time...
/
Leif
> +
> +STATIC
> +VOID
> +BuildMemTypeInfoHob (
> + VOID
> + )
> +{
> + BuildGuidDataHob (
> + &gEfiMemoryTypeInformationGuid,
> + mDefaultMemoryTypeInformation,
> + sizeof mDefaultMemoryTypeInformation
> + );
> + DEBUG ((DEBUG_INFO, "%a: default memory type information HOB built\n",
> + __FUNCTION__));
> +}
> +
> +/**
> + Notification function called when EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes
> + available.
> +
> + @param[in] PeiServices Indirect reference to the PEI Services Table.
> + @param[in] NotifyDescriptor Address of the notification descriptor data
> + structure.
> + @param[in] Ppi Address of the PPI that was installed.
> +
> + @return Status of the notification. The status code returned from this
> + function is ignored.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +OnReadOnlyVariable2Available (
> + IN EFI_PEI_SERVICES **PeiServices,
> + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor,
> + IN VOID *Ppi
> + )
> +{
> + EFI_PEI_READ_ONLY_VARIABLE2_PPI *ReadOnlyVariable2;
> + UINTN DataSize;
> + EFI_STATUS Status;
> +
> + DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__));
> +
> + //
> + // Check if the "MemoryTypeInformation" variable exists, in the
> + // gEfiMemoryTypeInformationGuid namespace.
> + //
> + ReadOnlyVariable2 = Ppi;
> + DataSize = 0;
> + Status = ReadOnlyVariable2->GetVariable (
> + ReadOnlyVariable2,
> + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
> + &gEfiMemoryTypeInformationGuid,
> + NULL,
> + &DataSize,
> + NULL
> + );
> + switch (Status) {
> + case EFI_BUFFER_TOO_SMALL:
> + //
> + // The variable exists; the DXE IPL PEIM will build the HOB from it.
> + //
> + break;
> + case EFI_NOT_FOUND:
> + //
> + // The variable does not exist; install the default memory type
> information
> + // HOB.
> + //
> + BuildMemTypeInfoHob ();
> + break;
> + default:
> + DEBUG ((DEBUG_ERROR, "%a: unexpected: GetVariable(): %r\n", __FUNCTION__,
> + Status));
> + ASSERT (FALSE);
> + CpuDeadLoop ();
> + break;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> +//
> +// Notification object for registering the callback, for when
> +// EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes available.
> +//
> +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mReadOnlyVariable2Notify = {
> + (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH |
> + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), // Flags
> + &gEfiPeiReadOnlyVariable2PpiGuid, // Guid
> + OnReadOnlyVariable2Available // Notify
> +};
> +
> +VOID
> +MemTypeInfoInitialization (
> + VOID
> + )
> +{
> + EFI_STATUS Status;
> +
> + if (!FeaturePcdGet (PcdSmmSmramRequire)) {
> + //
> + // EFI_PEI_READ_ONLY_VARIABLE2_PPI will never be available; install
> + // the default memory type information HOB right away.
> + //
> + BuildMemTypeInfoHob ();
> + return;
> + }
> +
> + Status = PeiServicesNotifyPpi (&mReadOnlyVariable2Notify);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "%a: failed to set up R/O Variable 2 callback:
> %r\n",
> + __FUNCTION__, Status));
> + ASSERT (FALSE);
> + CpuDeadLoop ();
> + }
> +}
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 473af102783a..587ca68fc210 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -28,7 +28,6 @@
> #include <Library/QemuFwCfgLib.h>
> #include <Library/QemuFwCfgS3Lib.h>
> #include <Library/ResourcePublicationLib.h>
> -#include <Guid/MemoryTypeInformation.h>
> #include <Ppi/MasterBootMode.h>
> #include <IndustryStandard/I440FxPiix4.h>
> #include <IndustryStandard/Pci22.h>
> @@ -39,18 +38,6 @@
> #include "Platform.h"
> #include "Cmos.h"
>
> -EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = {
> - { EfiACPIMemoryNVS, 0x004 },
> - { EfiACPIReclaimMemory, 0x008 },
> - { EfiReservedMemoryType, 0x004 },
> - { EfiRuntimeServicesData, 0x024 },
> - { EfiRuntimeServicesCode, 0x030 },
> - { EfiBootServicesCode, 0x180 },
> - { EfiBootServicesData, 0xF00 },
> - { EfiMaxMemoryType, 0x000 }
> -};
> -
> -
> EFI_PEI_PPI_DESCRIPTOR mPpiBootMode[] = {
> {
> EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> @@ -162,15 +149,6 @@ MemMapInitialization (
> PciIoBase = 0xC000;
> PciIoSize = 0x4000;
>
> - //
> - // Create Memory Type Information HOB
> - //
> - BuildGuidDataHob (
> - &gEfiMemoryTypeInformationGuid,
> - mDefaultMemoryTypeInformation,
> - sizeof(mDefaultMemoryTypeInformation)
> - );
> -
> //
> // Video memory + Legacy BIOS region
> //
> @@ -811,6 +789,7 @@ InitializePlatform (
> ReserveEmuVariableNvStore ();
> }
> PeiFvInitialization ();
> + MemTypeInfoInitialization ();
> MemMapInitialization ();
> NoexecDxeInitialization ();
> }
> --
> 2.19.1.3.g30247aa5d201
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55760): https://edk2.groups.io/g/devel/message/55760
Mute This Topic: https://groups.io/mt/71867512/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
