|
[PATCH v6 4/7] OvmfPkg: Introduce CocoDxe driver
Dionna Glaze
This driver is meant as a join point for all Confidential Compute
technologies to put shared behavior that doesn't belong anywhere else. The first behavior added here is to accept all unaccepted memory at ExitBootServices if the protocol is not disabled. This allows safe upgrades for OS loaders to affirm their support for the unaccepted memory type. Cc: Gerd Hoffmann <kraxel@...> Cc: James Bottomley <jejb@...> Cc: Jiewen Yao <jiewen.yao@...> Cc: Tom Lendacky <thomas.lendacky@...> Cc: Ard Biesheuvel <ardb@...> Cc: "Min M. Xu" <min.m.xu@...> Cc: Andrew Fish <afish@...> Cc: "Michael D. Kinney" <michael.d.kinney@...> Signed-off-by: Dionna Glaze <dionnaglaze@...> --- OvmfPkg/AmdSev/AmdSevX64.dsc | 1 + OvmfPkg/AmdSev/AmdSevX64.fdf | 1 + OvmfPkg/CocoDxe/CocoDxe.c | 144 ++++++++++++++++++++ OvmfPkg/CocoDxe/CocoDxe.inf | 43 ++++++ OvmfPkg/IntelTdx/IntelTdxX64.dsc | 1 + OvmfPkg/IntelTdx/IntelTdxX64.fdf | 1 + OvmfPkg/OvmfPkgIa32X64.dsc | 1 + OvmfPkg/OvmfPkgIa32X64.fdf | 1 + OvmfPkg/OvmfPkgX64.dsc | 1 + OvmfPkg/OvmfPkgX64.fdf | 1 + 10 files changed, 195 insertions(+) diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc index 90e8a213ef..ad6b73ca4a 100644 --- a/OvmfPkg/AmdSev/AmdSevX64.dsc +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc @@ -747,6 +747,7 @@ <LibraryClasses> PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf } + OvmfPkg/CocoDxe/CocoDxe.inf OvmfPkg/IoMmuDxe/IoMmuDxe.inf # diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf index 4658e1d30e..3717ec9094 100644 --- a/OvmfPkg/AmdSev/AmdSevX64.fdf +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf @@ -302,6 +302,7 @@ INF OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf INF OvmfPkg/VirtioGpuDxe/VirtioGpu.inf INF OvmfPkg/PlatformDxe/Platform.inf INF OvmfPkg/AmdSevDxe/AmdSevDxe.inf +INF OvmfPkg/CocoDxe/CocoDxe.inf INF OvmfPkg/IoMmuDxe/IoMmuDxe.inf diff --git a/OvmfPkg/CocoDxe/CocoDxe.c b/OvmfPkg/CocoDxe/CocoDxe.c new file mode 100644 index 0000000000..9e9a405af1 --- /dev/null +++ b/OvmfPkg/CocoDxe/CocoDxe.c @@ -0,0 +1,144 @@ +/** @file + + Confidential Compute Dxe driver. This driver installs protocols that are + generic over confidential compute techonology. + + Copyright (c) 2022, Google LLC. All rights reserved.<BR> + + SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +#include <Library/BaseLib.h> +#include <Library/BaseMemoryLib.h> +#include <Library/DebugLib.h> +#include <Library/DxeServicesTableLib.h> +#include <Library/UefiBootServicesTableLib.h> +#include <Library/MemEncryptSevLib.h> +#include <Library/MemEncryptTdxLib.h> +#include <Protocol/ExitBootServicesCallback.h> +#include <Protocol/MemoryAccept.h> + +STATIC EFI_HANDLE mCocoDxeHandle = NULL; + +STATIC +EFI_STATUS +AcceptAllUnacceptedMemory ( + IN EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory + ) +{ + EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap; + UINTN NumEntries; + UINTN Index; + EFI_STATUS Status; + BOOLEAN AcceptedAny; + + DEBUG ((DEBUG_INFO, "Accepting all memory\n")); + AcceptedAny = FALSE; + Status = EFI_SUCCESS; + /* + * Get a copy of the memory space map to iterate over while + * changing the map. + */ + Status = gDS->GetMemorySpaceMap (&NumEntries, &AllDescMap); + if (EFI_ERROR (Status)) { + return Status; + } + for (Index = 0; Index < NumEntries; Index++) { + CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Desc; + + Desc = &AllDescMap[Index]; + if (Desc->GcdMemoryType != EfiGcdMemoryTypeUnaccepted) { + continue; + } + + Status = AcceptMemory->AcceptMemory ( + AcceptMemory, + Desc->BaseAddress, + Desc->Length + ); + ASSERT_EFI_ERROR (Status); + + Status = gDS->RemoveMemorySpace(Desc->BaseAddress, Desc->Length); + ASSERT_EFI_ERROR (Status); + + Status = gDS->AddMemorySpace ( + EfiGcdMemoryTypeSystemMemory, + Desc->BaseAddress, + Desc->Length, + EFI_MEMORY_CPU_CRYPTO | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_RP + ); + ASSERT_EFI_ERROR (Status); + + AcceptedAny = TRUE; + } + + // If any memory is accepted, cause ExitBootServices to fail with + // EFI_INVALID_PARAMETER in order to force the caller to refresh + // their view of the MemoryMap. + if (AcceptedAny) { + Status = EFI_INVALID_PARAMETER; + } + +done: + gBS->FreePool (AllDescMap); + return Status; +} + +EFI_STATUS +EFIAPI +ResolveUnacceptedMemory ( + IN EDKII_EXIT_BOOT_SERVICES_CALLBACK_PROTOCOL *This + ) +{ + EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory; + EFI_STATUS Status; + + if (This->Disabled) { + return EFI_SUCCESS; + } + + Status = gBS->LocateProtocol (&gEfiMemoryAcceptProtocolGuid, NULL, + (VOID **)&AcceptMemory); + if (Status == EFI_NOT_FOUND) { + return EFI_SUCCESS; + } + ASSERT_EFI_ERROR (Status); + + return AcceptAllUnacceptedMemory(AcceptMemory); +} + +STATIC EDKII_EXIT_BOOT_SERVICES_CALLBACK_PROTOCOL mExitBootServicesCallbackProcotol = { + ResolveUnacceptedMemory, + FALSE, +}; + +EFI_STATUS +EFIAPI +CocoDxeEntryPoint ( + IN EFI_HANDLE ImageHandle, + IN EFI_SYSTEM_TABLE *SystemTable + ) +{ + EFI_STATUS Status; + + // + // Do nothing when confidential compute technologies that require memory + // acceptance are not enabled. + // + if (!MemEncryptSevSnpIsEnabled () && + !MemEncryptTdxIsEnabled ()) { + return EFI_UNSUPPORTED; + } + + Status = gBS->InstallProtocolInterface (&mCocoDxeHandle, + &gEdkiiExitBootServicesCallbackProtocolGuid, + EFI_NATIVE_INTERFACE, + &mExitBootServicesCallbackProcotol + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Install EdkiiExitBootServicesCallbackProtocol failed.\n")); + } + + return EFI_SUCCESS; +} diff --git a/OvmfPkg/CocoDxe/CocoDxe.inf b/OvmfPkg/CocoDxe/CocoDxe.inf new file mode 100644 index 0000000000..3ff2a6fade --- /dev/null +++ b/OvmfPkg/CocoDxe/CocoDxe.inf @@ -0,0 +1,43 @@ +#/** @file +# +# Driver installs shared protocols needed for confidential compute +# technologies. +# +# Copyright (c) 2022, Google LLC. All rights reserved.<BR> +# +# SPDX-License-Identifier: BSD-2-Clause-Patent +# +#**/ + +[Defines] + INF_VERSION = 1.25 + BASE_NAME = CocoDxe + FILE_GUID = 08162f1e-5147-4d3e-b5a9-fa48c9808419 + MODULE_TYPE = DXE_DRIVER + VERSION_STRING = 1.0 + ENTRY_POINT = CocoDxeEntryPoint + +[Sources] + CocoDxe.c + +[Packages] + MdeModulePkg/MdeModulePkg.dec + MdePkg/MdePkg.dec + OvmfPkg/OvmfPkg.dec + +[LibraryClasses] + BaseLib + BaseMemoryLib + DebugLib + DxeServicesTableLib + MemEncryptSevLib + MemEncryptTdxLib + MemoryAllocationLib + UefiDriverEntryPoint + +[Depex] + TRUE + +[Protocols] + gEdkiiExitBootServicesCallbackProtocolGuid + gEfiMemoryAcceptProtocolGuid diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc index c0c1a15b09..8136d50eb2 100644 --- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc +++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc @@ -753,6 +753,7 @@ OvmfPkg/IoMmuDxe/IoMmuDxe.inf OvmfPkg/TdxDxe/TdxDxe.inf + OvmfPkg/CocoDxe/CocoDxe.inf # # Variable driver stack (non-SMM) diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.fdf b/OvmfPkg/IntelTdx/IntelTdxX64.fdf index 6923eb8831..e612608c0c 100644 --- a/OvmfPkg/IntelTdx/IntelTdxX64.fdf +++ b/OvmfPkg/IntelTdx/IntelTdxX64.fdf @@ -269,6 +269,7 @@ INF ShellPkg/Application/Shell/Shell.inf INF MdeModulePkg/Logo/LogoDxe.inf INF OvmfPkg/TdxDxe/TdxDxe.inf +INF OvmfPkg/CocoDxe/CocoDxe.inf # # Usb Support diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index af566b953f..2cfb3fbc6b 100644 --- a/OvmfPkg/OvmfPkgIa32X64.dsc +++ b/OvmfPkg/OvmfPkgIa32X64.dsc @@ -965,6 +965,7 @@ <LibraryClasses> PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf } + OvmfPkg/CocoDxe/CocoDxe.inf OvmfPkg/IoMmuDxe/IoMmuDxe.inf !if $(SMM_REQUIRE) == TRUE diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf index 80de4fa2c0..2ab7f3b95b 100644 --- a/OvmfPkg/OvmfPkgIa32X64.fdf +++ b/OvmfPkg/OvmfPkgIa32X64.fdf @@ -343,6 +343,7 @@ INF OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf INF OvmfPkg/VirtioGpuDxe/VirtioGpu.inf INF OvmfPkg/PlatformDxe/Platform.inf INF OvmfPkg/AmdSevDxe/AmdSevDxe.inf +INF OvmfPkg/CocoDxe/CocoDxe.inf INF OvmfPkg/IoMmuDxe/IoMmuDxe.inf !if $(SMM_REQUIRE) == TRUE diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index f39d9cd117..3ead476b61 100644 --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -1036,6 +1036,7 @@ OvmfPkg/IoMmuDxe/IoMmuDxe.inf OvmfPkg/TdxDxe/TdxDxe.inf + OvmfPkg/CocoDxe/CocoDxe.inf !if $(SMM_REQUIRE) == TRUE OvmfPkg/SmmAccess/SmmAccess2Dxe.inf diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf index c0f5a1ef3c..5dd452f42b 100644 --- a/OvmfPkg/OvmfPkgX64.fdf +++ b/OvmfPkg/OvmfPkgX64.fdf @@ -370,6 +370,7 @@ INF OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf INF OvmfPkg/VirtioGpuDxe/VirtioGpu.inf INF OvmfPkg/PlatformDxe/Platform.inf INF OvmfPkg/AmdSevDxe/AmdSevDxe.inf +INF OvmfPkg/CocoDxe/CocoDxe.inf INF OvmfPkg/IoMmuDxe/IoMmuDxe.inf !if $(SMM_REQUIRE) == TRUE -- 2.38.0.rc1.362.ged0d419d3c-goog
|
|
|
|
[PATCH v6 3/7] MdeModulePkg: Invoke all ExitBootServicesCallback instances at ExitBootServices
Dionna Glaze
The protocol's intent is to allow drivers to install callbacks that can
modify the memory map at ExitBootServices time, so that any changes will lead to the EFI_INVALID_PARAMETER error. This error is specified to require the EBS caller to call GetMemoryMap again if it already had. Cc: Gerd Hoffmann <kraxel@...> Cc: James Bottomley <jejb@...> Cc: Jiewen Yao <jiewen.yao@...> Cc: Tom Lendacky <thomas.lendacky@...> Cc: Ard Biesheuvel <ardb@...> Cc: "Min M. Xu" <min.m.xu@...> Cc: Andrew Fish <afish@...> Cc: "Michael D. Kinney" <michael.d.kinney@...> Cc: Ray Ni <ray.ni@...> Signed-off-by: Dionna Glaze <dionnaglaze@...> --- MdeModulePkg/Core/Dxe/DxeMain.h | 1 + MdeModulePkg/Core/Dxe/DxeMain.inf | 1 + MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 41 ++++++++++++++++++++ 3 files changed, 43 insertions(+) diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h index 815a6b4bd8..185b9dc3d6 100644 --- a/MdeModulePkg/Core/Dxe/DxeMain.h +++ b/MdeModulePkg/Core/Dxe/DxeMain.h @@ -45,6 +45,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include <Protocol/HiiPackageList.h> #include <Protocol/SmmBase2.h> #include <Protocol/PeCoffImageEmulator.h> +#include <Protocol/ExitBootServicesCallback.h> #include <Guid/MemoryTypeInformation.h> #include <Guid/FirmwareFileSystem2.h> #include <Guid/FirmwareFileSystem3.h> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf index e4bca89577..bdd9cf8222 100644 --- a/MdeModulePkg/Core/Dxe/DxeMain.inf +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf @@ -153,6 +153,7 @@ gEfiHiiPackageListProtocolGuid ## SOMETIMES_PRODUCES gEfiSmmBase2ProtocolGuid ## SOMETIMES_CONSUMES gEdkiiPeCoffImageEmulatorProtocolGuid ## SOMETIMES_CONSUMES + gEdkiiExitBootServicesCallbackProtocolGuid ## CONSUMES # Arch Protocols gEfiBdsArchProtocolGuid ## CONSUMES diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c index 5733f0c8ec..3270f9858d 100644 --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c @@ -744,6 +744,34 @@ CalculateEfiHdrCrc ( Hdr->CRC32 = Crc; } +/** + Invokes TerminateMemoryMapPrehook from the first located instance of + EdkiiExitBootServicesProtocol. +**/ +STATIC +EFI_STATUS +InvokeTerminateMemoryMapPrehooks ( + VOID + ) +{ + EFI_STATUS Status; + EDKII_EXIT_BOOT_SERVICES_CALLBACK_PROTOCOL *Callback; + + Status = gBS->LocateHandle ( + &gEdkiiExitBootServicesCallbackProtocolGuid, + NULL, + (VOID**)Callback + ); + if (Status == EFI_NOT_FOUND) { + return EFI_SUCCESS; + } + if (EFI_ERROR (Status)) { + return Status; + } + + return Callback->TerminateMemoryMapPrehook(Callback); +} + /** Terminates all boot services. @@ -768,6 +796,19 @@ CoreExitBootServices ( // gTimer->SetTimerPeriod (gTimer, 0); + // + // Invoke all protocols installed for ExitBootServices prior to + // CoreTerminateMemoryMap. + // + Status = InvokeTerminateMemoryMapPrehooks(); + if (EFI_ERROR (Status)) { + // + // Notify other drivers that ExitBootServices failed + // + CoreNotifySignalList (&gEventExitBootServicesFailedGuid); + return Status; + } + // // Terminate memory services if the MapKey matches // -- 2.38.0.rc1.362.ged0d419d3c-goog
|
|
|
|
[PATCH v6 2/7] MdeModulePkg: Introduce ExitBootServicesCallbackProtocol
Dionna Glaze
This introduces a callback after the time that the timer is disabled and before
the MemoryMap is finalized. This callback is useful to make final changes to the memory map due to protocols initiated (or not initiated) by the OS loader. Cc: Gerd Hoffmann <kraxel@...> Cc: "Min M. Xu" <min.m.xu@...> Cc: James Bottomley <jejb@...> Cc: Jiewen Yao <jiewen.yao@...> Cc: Tom Lendacky <thomas.lendacky@...> Cc: Ard Biesheuvel <ardb@...> Cc: Andrew Fish <afish@...> Cc: "Michael D. Kinney" <michael.d.kinney@...> Signed-off-by: Dionna Glaze <dionnaglaze@...> --- MdeModulePkg/Include/Protocol/ExitBootServicesCallback.h | 40 ++++++++++++++++++++ MdeModulePkg/MdeModulePkg.dec | 3 ++ 2 files changed, 43 insertions(+) diff --git a/MdeModulePkg/Include/Protocol/ExitBootServicesCallback.h b/MdeModulePkg/Include/Protocol/ExitBootServicesCallback.h new file mode 100644 index 0000000000..5c9f973b79 --- /dev/null +++ b/MdeModulePkg/Include/Protocol/ExitBootServicesCallback.h @@ -0,0 +1,40 @@ +/** @file + The file provides the protocol that allows callbacks in ExitBootServices + immediately before TerminateMemoryMap. + + Copyright (c) 2022, Google LLC. All rights reserved.<BR> + SPDX-License-Identifier: BSD-2-Clause-Patent +**/ +#ifndef EXIT_BOOT_SERVICES_CALLBACK_H_ +#define EXIT_BOOT_SERVICES_CALLBACK_H_ + +/* This protocol is internal to EDK2 only */ + +#define EDKII_EXIT_BOOT_SERVICES_CALLBACK_PROTOCOL_GUID {0xf5684799, 0x9a33, 0x40f7, {0xa1, 0x5c, 0x10, 0x8e, 0x0e, 0x6b, 0x45, 0x25}} + +typedef struct _EDKII_EXIT_BOOT_SERVICES_CALLBACK_PROTOCOL + EDKII_EXIT_BOOT_SERVICES_CALLBACK_PROTOCOL; + +/** + @param This A pointer to a EDKII_EXIT_BOOT_SERVICES_CALLBACK_PROTOCOL. + + The returned status may only be EFI_SUCCESS or EFI_INVALID_PARAMETER. +**/ +typedef +EFI_STATUS +(EFIAPI *EDKII_TERMINATE_MEMORY_MAP_PREHOOK)( + IN EDKII_EXIT_BOOT_SERVICES_CALLBACK_PROTOCOL *This + ); + +/// +/// The EDKII_EXIT_BOOT_SERVICES_CALLBACK_PROTOCOL allows callbacks in +/// ExitBootServices immediately before TerminateMemoryMap. +/// +struct _EDKII_EXIT_BOOT_SERVICES_CALLBACK_PROTOCOL { + EDKII_TERMINATE_MEMORY_MAP_PREHOOK TerminateMemoryMapPrehook; + BOOLEAN Disabled; +}; + +extern EFI_GUID gEdkiiExitBootServicesCallbackProtocolGuid; + +#endif diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 58e6ab0048..add4e304ca 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -677,6 +677,9 @@ ## Include/Protocol/VariablePolicy.h gEdkiiVariablePolicyProtocolGuid = { 0x81D1675C, 0x86F6, 0x48DF, { 0xBD, 0x95, 0x9A, 0x6E, 0x4F, 0x09, 0x25, 0xC3 } } + ## Include/Protocol/ExitBootServicesCallback.h + gEdkiiExitBootServicesCallbackProtocolGuid = { 0xf5684799, 0x9a33, 0x40f7, {0xa1, 0x5c, 0x10, 0x8e, 0x0e, 0x6b, 0x45, 0x25 }} + [PcdsFeatureFlag] ## Indicates if the platform can support update capsule across a system reset.<BR><BR> # TRUE - Supports update capsule across a system reset.<BR> -- 2.38.0.rc1.362.ged0d419d3c-goog
|
|
|
|
[PATCH v6 1/7] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe
Dionna Glaze
From: Sophia Wolf <phiawolf@...>
When a guest OS does not support unaccepted memory, the unaccepted memory must be accepted before returning a memory map to the caller. EfiMemoryAcceptProtocol is defined in MdePkg and is implemented / Installed in AmdSevDxe for AMD SEV-SNP memory acceptance. Cc: Gerd Hoffmann <kraxel@...> Cc: James Bottomley <jejb@...> Cc: Jiewen Yao <jiewen.yao@...> Cc: Tom Lendacky <thomas.lendacky@...> Signed-off-by: Dionna Glaze <dionnaglaze@...> --- OvmfPkg/AmdSevDxe/AmdSevDxe.c | 55 ++++++++++++++++++-- OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 3 ++ OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c | 24 +++++++-- 3 files changed, 74 insertions(+), 8 deletions(-) diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c index 662d3c4ccb..13fb58cc02 100644 --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c @@ -20,6 +20,7 @@ #include <Library/UefiBootServicesTableLib.h> #include <Guid/ConfidentialComputingSevSnpBlob.h> #include <Library/PcdLib.h> +#include <Protocol/MemoryAccept.h> STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION mSnpBootDxeTable = { SIGNATURE_32 ('A', 'M', 'D', 'E'), @@ -31,6 +32,40 @@ STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION mSnpBootDxeTable = { FixedPcdGet32 (PcdOvmfCpuidSize), }; +STATIC EFI_HANDLE mAmdSevDxeHandle = NULL; + +#define IS_ALIGNED(x, y) ((((x) & (y - 1)) == 0)) + +STATIC +EFI_STATUS +EFIAPI +AmdSevMemoryAccept ( + IN EFI_MEMORY_ACCEPT_PROTOCOL *This, + IN EFI_PHYSICAL_ADDRESS StartAddress, + IN UINTN Size +) +{ + // + // The StartAddress must be page-aligned, and the Size must be a positive + // multiple of SIZE_4KB. Use an assert instead of returning an erros since + // this is an EDK2-internal protocol. + // + ASSERT (IS_ALIGNED (StartAddress, SIZE_4KB)); + ASSERT (IS_ALIGNED (Size, SIZE_4KB)); + ASSERT (Size != 0); + + MemEncryptSevSnpPreValidateSystemRam ( + StartAddress, + EFI_SIZE_TO_PAGES (Size) + ); + + return EFI_SUCCESS; +} + +STATIC EFI_MEMORY_ACCEPT_PROTOCOL mMemoryAcceptProtocol = { + AmdSevMemoryAccept +}; + EFI_STATUS EFIAPI AmdSevDxeEntryPoint ( @@ -147,11 +182,23 @@ AmdSevDxeEntryPoint ( } } - // - // If its SEV-SNP active guest then install the CONFIDENTIAL_COMPUTING_SEV_SNP_BLOB. - // It contains the location for both the Secrets and CPUID page. - // if (MemEncryptSevSnpIsEnabled ()) { + // + // Memory acceptance began being required in SEV-SNP, so install the + // memory accept protocol implementation for a SEV-SNP active guest. + // + Status = gBS->InstallProtocolInterface ( + &mAmdSevDxeHandle, + &gEfiMemoryAcceptProtocolGuid, + EFI_NATIVE_INTERFACE, + &mMemoryAcceptProtocol + ); + ASSERT_EFI_ERROR (Status); + + // + // If its SEV-SNP active guest then install the CONFIDENTIAL_COMPUTING_SEV_SNP_BLOB. + // It contains the location for both the Secrets and CPUID page. + // return gBS->InstallConfigurationTable ( &gConfidentialComputingSevSnpBlobGuid, &mSnpBootDxeTable diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf index 9acf860cf2..5ddddabc32 100644 --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf @@ -47,6 +47,9 @@ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize +[Protocols] + gEfiMemoryAcceptProtocolGuid + [Guids] gConfidentialComputingSevSnpBlobGuid diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c index d3a95e4913..ee3710f7b3 100644 --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c @@ -14,6 +14,7 @@ #include <Library/MemEncryptSevLib.h> #include "SnpPageStateChange.h" +#include "VirtualMemory.h" /** Pre-validate the system RAM when SEV-SNP is enabled in the guest VM. @@ -29,12 +30,27 @@ MemEncryptSevSnpPreValidateSystemRam ( IN UINTN NumPages ) { + EFI_STATUS Status; + if (!MemEncryptSevSnpIsEnabled ()) { return; } - // - // All the pre-validation must be completed in the PEI phase. - // - ASSERT (FALSE); + // DXE pre-validation may happen with the memory accept protocol. + // The protocol should only be called outside the prevalidated ranges + // that the PEI stage code explicitly skips. Specifically, only memory + // ranges that are classified as unaccepted. + if (BaseAddress >= SIZE_4GB) { + Status = InternalMemEncryptSevCreateIdentityMap1G ( + 0, + BaseAddress, + EFI_PAGES_TO_SIZE (NumPages) + ); + if (EFI_ERROR (Status)) { + ASSERT (FALSE); + CpuDeadLoop (); + } + } + + InternalSetPageState (BaseAddress, NumPages, SevSnpPagePrivate, TRUE); } -- 2.38.0.rc1.362.ged0d419d3c-goog
|
|
|
|
[PATCH v6 0/7] Add safe unaccepted memory behavior
Dionna Glaze
These seven patches build on the lazy-accept patch series
"Introduce Lazy-accept for Tdx guest" by adding SEV-SNP support for the MemoryAccept protocol, and importantly making eager memory acceptance the default behavior. We add a new protocol, ExitBootServicesCallbackProtocol, with a single interface: TerminateMemoryMapPrehook(). We invoke all prehooks in CoreExitBootServices after disabling the timer and before TerminateMemoryMap. This gives hooks the chance to change the memory map and cause ExitBootServices to fail with EFI_INVALID_PARAMETER. The failure is specified to require the caller to update their view of the MemoryMap and call ExitBootServices again. To make use of this new protocol, we add a new driver that is meant to carry behavior that is needed for all confidential compute technologies, not just specific platforms, CocoDxe. In CocoDxe we implement the default safe behavior to accept all unaccepted memory and invalidate the MemoryMap on ExitBootServices. To allow the OS loader to prevent the eager acceptance, add another protocol, up for standardization, AcceptAllUnacceptedMemoryProtocol. This protocol has one interface, Disable(). The OS loader can inform the UEFI that it supports the unaccepted memory type and accepts the responsibility to accept it. All images that support unaccepted memory must now locate and call this new BZ3987_ACCEPT_ALL_UNACCEPTED_MEMORY_PROTOCOL and call the Disable function. Changes since v5: - Generic callback protocol moved to MdeModulePkg - Removed use of EFI_WARN_STALE_DATA and added comment that the callback should only return EFI_SUCCESS or EFI_INVALID_PARAMETER. - Removed errant log statement and fixed formatting. Changes since v4: - Commit message wording - Replaced direct change to DxeMain with a more generic callback protocol. - Implemented the direct change as an instance of the callback protocol from a new CocoDxe driver. - Replaced "enable" protocol with a "disable" protocol, since the name was confusing. The AcceptAllUnacceptedMemory protocol directly names the behavior that is disabling. Changes since v3: - "DxeMain accepts all memory" patch split into 3 to make each patch affect only one package at a time. Changes since v2: - Removed the redundant memory accept interface and added the accept behavior to the DXE implementation of MemEncryptSevSnpPreValidateSystemRam. - Fixed missing #include in >=4GB patch. Changes since v1: - Added a patch to classify SEV-SNP memory above 4GB unaccepted. - Fixed style problems in EfiMemoryAcceptProtocol implementation. Cc: Ard Biescheuvel <ardb@...> Cc: "Min M. Xu" <min.m.xu@...> Cc: Gerd Hoffmann <kraxel@...> Cc: James Bottomley <jejb@...> Cc: Tom Lendacky <Thomas.Lendacky@...> Cc: Jiewen Yao <jiewen.yao@...> Cc: Erdem Aktas <erdemaktas@...> Cc: Andrew Fish <afish@...> Cc: "Michael D. Kinney" <michael.d.kinney@...> Dionna Glaze (7): OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe MdeModulePkg: Introduce ExitBootServicesCallbackProtocol MdeModulePkg: Invoke all ExitBootServicesCallback instances at ExitBootServices OvmfPkg: Introduce CocoDxe driver MdePkg: Introduce the AcceptAllUnacceptedMemory protocol OvmfPkg: Implement AcceptAllUnacceptedMemory in CocoDxe OvmfPkg/PlatformPei: SEV-SNP make >=4GB unaccepted MdeModulePkg/Core/Dxe/DxeMain.h | 1 + MdeModulePkg/Core/Dxe/DxeMain.inf | 1 + MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 41 +++++ MdeModulePkg/Include/Protocol/ExitBootServicesCallback.h | 40 +++++ MdeModulePkg/MdeModulePkg.dec | 3 + MdePkg/Include/Protocol/Bz3987AcceptAllUnacceptedMemory.h | 40 +++++ MdePkg/MdePkg.dec | 3 + OvmfPkg/AmdSev/AmdSevX64.dsc | 1 + OvmfPkg/AmdSev/AmdSevX64.fdf | 1 + OvmfPkg/AmdSevDxe/AmdSevDxe.c | 55 ++++++- OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 3 + OvmfPkg/CocoDxe/CocoDxe.c | 169 ++++++++++++++++++++ OvmfPkg/CocoDxe/CocoDxe.inf | 44 +++++ OvmfPkg/IntelTdx/IntelTdxX64.dsc | 1 + OvmfPkg/IntelTdx/IntelTdxX64.fdf | 1 + OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c | 24 ++- OvmfPkg/OvmfPkgIa32X64.dsc | 1 + OvmfPkg/OvmfPkgIa32X64.fdf | 1 + OvmfPkg/OvmfPkgX64.dsc | 1 + OvmfPkg/OvmfPkgX64.fdf | 1 + OvmfPkg/PlatformPei/AmdSev.c | 5 + 21 files changed, 429 insertions(+), 8 deletions(-) create mode 100644 MdeModulePkg/Include/Protocol/ExitBootServicesCallback.h create mode 100644 MdePkg/Include/Protocol/Bz3987AcceptAllUnacceptedMemory.h create mode 100644 OvmfPkg/CocoDxe/CocoDxe.c create mode 100644 OvmfPkg/CocoDxe/CocoDxe.inf -- 2.38.0.rc1.362.ged0d419d3c-goog
|
|
|
|
Re: [PATCH v2 3/6] IntelFsp2Pkg: Add CI YAML file
Michael Kubacki
+Other IntelFsp2Pkg & IntelFsp2WrapperPkg maintainers to To line
toggle quoted messageShow quoted text
Please review the remaining patches in this patch series: https://edk2.groups.io/g/devel/message/93859 Thanks, Michael
On 10/3/2022 10:44 AM, Michael Kubacki wrote:
Another reminder.
|
|
|
|
Re: Can NULL pointer be a valid event?
Michael D Kinney
Hi Ayush,
toggle quoted messageShow quoted text
Quick answer is that the UEFI Spec may not explicitly disallow NULLL, but in practice, it will never return NULL. =================================== EFI_EVENT is same as VOID*. typedef VOID *EFI_EVENT CreateEvent() returns a pointer to an Event, so it is really a double pointer. CreateEvent() returns EFI_INVALID_PARAMETER if Event (pointer to EFI_EVENT structure) is NULL. But CreateEvent/Ex() do not explicitly state that the pointer to the EFI_EVENT structure returned cannot be address 0. Internally to the EDK II, EFI_EVENT is a structure so it must be a valid pointer. Though I would point out that even this is an implementation choice. An implementation could treat the pointer to the EFI_EVENT as a handle number and could internally convert a handle number to a structure pointer to further hide details of the event structure and prevent the reuse of the same pointer value for different events across allocates/frees. The EDK II implementation choice to use pointers instead of handles is for the smallest/fastest implementation. It is possible to have a pointer to a structure at address 0. However, the EDK II implementations of the UEFI services do not allow the use of memory at 0 for normal memory allocations. I am aware of one use case of memory at 0 for an x86 IDT structure for 16-bit code. So it is not possible for the EDK II implementation of an UEFI service that returns pointers to structures to return a pointer value of 0. In fact, there are guard page features in EDK II that check if there is any access to the first page of memory in the address range 0x0..0xFFF. So the real restriction EDK II imposes is to never allocate a data structure in the first page of memory (0x0..0xFFF). Given it would be possible to implement many UEFI services using handle numbers instead of pointers. I would recommend those implementations do not use a handle value of 0. And instead start at a handle value of at least 1. Best regards, Mike
-----Original Message-----
|
|
|
|
Re: [PATCH v5 2/7] MdePkg: Introduce ExitBootServicesCallbackProtocol
Dionna Glaze
Oh, MemoryAcceptProtocol is also not specified AFAICT, so I thought this is where it would go. I can move it. -- -Dionna Glaze, PhD (she/her)
|
|
|
|
Re: Can NULL pointer be a valid event?
Dionna Glaze
CreateEvent returns an EFI_STATUS. It expects the OUT parameter, a
toggle quoted messageShow quoted text
pointer to an EFI_EVENT, to be non-NULL. A null pointer results in EFI_INVALID_PARAMETER. If the CreateEvent is successful, then `event` points to the newly created event. It's the caller's responsibility to pass a pointer to valid writable memory.
On Mon, Oct 3, 2022 at 8:08 AM Ayush Singh <ayushdevel1325@...> wrote:
--
-Dionna Glaze, PhD (she/her)
|
|
|
|
Re: The principles of EDK2 module reconstruction for archs
Michael D Kinney
Hi Abner,
toggle quoted messageShow quoted text
responses below. Mike
-----Original Message-----Agreed Yes However, there is one scenario. Assume there were two arch folders IA32_X64 and RISCV64. Then ARM shares the code with IA32_X64,I would let the maintainer make the decision. For your example, another approach may be to move the IA32_X64 content up a level so it is common and is used by IA32, X64, ARM. And leave RISCV64 folder for an arch that has special requirements. I think having some flexibility in the guidelines is very beneficial. The main goal is for consistency when a specific guideline is followed.
|
|
|
|
Can NULL pointer be a valid event?
Hello everyone, I wanted to ask if a NULL pointer can be returned as a valid event from `EFI_BOOT_SERVICES.CreateEvent()` or `EFI_BOOT_SERVICES.CreateEventEx()`? Or does the specification state that a valid event pointer has to be non-NULL? Yours Sincerely, Ayush Singh
|
|
|
|
Re: [PATCH v5 1/7] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe
Lendacky, Thomas
On 9/30/22 18:06, Dionna Glaze wrote:
From: Sophia Wolf <phiawolf@...>Just some formatting suggestions and one area of cleanup from previous version of the patch below. Assuming you take care of those: Reviewed-by: Tom Lendacky <thomas.lendacky@...> ---Create a generic alignment check macro? #define IS_ALIGNED(x, y) (((x) & ((y) - 1)) == 0) Maybe keep the ASSERTs separate so they better identify which condition caused the assert, e.g.: ASSERT (IS_ALIGNED (StartAddress, SIZE_4KB)); ASSERT (IS_ALIGNED (Size, SIZE_4KB)); ASSERT (Size != 0); ? Not sure if those are worth it or not, though. +Looks like this shouldn't be here. +Need to indent these two more spaces to align with the "s" in Install. Thanks, Tom + ASSERT_EFI_ERROR (Status);
|
|
|
|
Re: [PATCH v1 1/2] SignedCapsulePkg: Add package CI YAML file
Michael Kubacki
Another reminder.
toggle quoted messageShow quoted text
On 9/22/2022 9:06 PM, Michael Kubacki wrote:
Review reminder
|
|
|
|
Re: [PATCH v2 3/6] IntelFsp2Pkg: Add CI YAML file
Michael Kubacki
Another reminder.
toggle quoted messageShow quoted text
On 9/22/2022 9:07 PM, Michael Kubacki wrote:
Review reminder
|
|
|
|
Re: 回复: [edk2-devel] [RFC] Adoption of CodeQL in edk2
Michael Kubacki
I believe you are asking, when CodeQL CI is enabled in the edk2 project, how soon after will all of the issues be fixed so CI passes?
toggle quoted messageShow quoted text
The process that will be used to enable CodeQL in CI will follow what is described in the "Enable One Query at a Time" section in the RFC (https://github.com/tianocore/edk2/discussions/3258). As proposed in that section, there should not be a time when a new CodeQL CLI query is enabled that does not pass. Queries will be enabled one at a time. Each time a new query is enabled, the query enable and the corresponding changes will be staged on a branch that get merged to edk2 master in a single PR. CodeQL CI will run in that PR and it must pass for the PR to be completed.
On 9/30/2022 2:33 AM, gaoliming wrote:
Michael:
|
|
|
|
Re: [RFC] Adoption of CodeQL in edk2
Michael Kubacki
Code changes do not need to be made for anything except actual fixes and improvements. The process to dismiss alerts is mentioned in the RFC post (https://github.com/tianocore/edk2/discussions/3258) under "Dismissing CodeQL Alerts" which links to to this page with more info:
toggle quoted messageShow quoted text
https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/managing-code-scanning-alerts-for-your-repository#dismissing--alerts.
On 9/29/2022 10:53 PM, Ni, Ray wrote:
Multiplication result converted to larger type · Code scanning alert #66 · tianocore/edk2 (github.com) <https://github.com/tianocore/edk2/security/code-scanning/66>
|
|
|
|
Re: [PATCH 1/2] MdePkg: Support for MPAM ACPI Table
Sami Mujawar
Hi Swatisri,
Rohit pointed me to this patch series, which I believe is v2. I happened to have missed this as the subject line was not clear. However, I believe my feedback (or some parts) for the v1 series would still apply. Regards, Sami Mujawar
|
|
|
|
Re: [PATCH v5 3/7] MdeModulePkg: Invoke all ExitBootServicesCallback instances at ExitBootServices
Ard Biesheuvel
(cc Ray)
On Sat, 1 Oct 2022 at 01:06, Dionna Glaze <dionnaglaze@...> wrote: This include belongs in DxeMain.h #include "DxeMain.h"We should decide whether or not we want to support a multitude of these callback protocols. Some protocols in EFI are simply considered singleton protocols, and I think we might want to stick with that approach for the time being, as it makes the code much simpler, especially because most users of this code are not confidential compute. +{EFI_WARN_STALE_DATA is not an error, so this routine might return a non-error, which we fail to check for below. Also, if multiple protocols exist, shouldn't we invoke all of them, and collate the return codes in some way? (Another reason to stick with a singleton protocol) + goto done;This condition does not hold for EFI_WARN_STALE_DATA, nor is EFI_WARN_STALE_DATA documented as a valid return value for ExitBootServices(). + //
|
|
|
|
Re: [PATCH v5 2/7] MdePkg: Introduce ExitBootServicesCallbackProtocol
Ard Biesheuvel
On Mon, 3 Oct 2022 at 03:16, Dionna Amalie Glaze <dionnaglaze@...> wrote:
It is not in the spec, which is why we called itIt is not. This is Ard's suggested solution gEdkiiExitBootServicesCallbackProtocolGuid What Ray is getting to (I think) is that that means that it should be defined in MdeModulePkg not MdePkg.
|
|
|
|
Re: [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table
Sami Mujawar
Hi Swatisri,
Apologies for the delay in feedback. There is a new version of "ACPI for Memory System Resource Partitioning and Monitoring 2.0, Platform Design Document" published on 2022/Sep/30 at https://developer.arm.com/documentation/den0065/latest. The version 2.0 of ACPI MPAM table has a few errata fixes and some new additions. Therefore, the version 1.0 of the ACPI MPAM table stands deprecated and must not be used. I would therefore suggest that this patch is updated to reflect ACPI MPAM table version 2.0. I also have some feedback below that would still apply. Regards, Sami Mujawar On 16/08/2022 09:18 pm, Name wrote:
From: Swatisri Kantamsetti <swatisrik@...> Added MPAM table header, MSC and Resource Node info structures Signed-off-by: Swatisri Kantamsetti <swatisrik@...> --- MdePkg/Include/IndustryStandard/Acpi64.h | 5 ++ MdePkg/Include/IndustryStandard/Mpam.h | 69 ++++++++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 MdePkg/Include/IndustryStandard/Mpam.h diff --git a/MdePkg/Include/IndustryStandard/Acpi64.h b/MdePkg/Include/IndustryStandard/Acpi64.h index fe5ebfac2b..e54f631186 100644 --- a/MdePkg/Include/IndustryStandard/Acpi64.h +++ b/MdePkg/Include/IndustryStandard/Acpi64.h @@ -2952,6 +2952,11 @@ typedef struct { /// #define EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SIGNATURE SIGNATURE_32('P', 'P', 'T', 'T') +/// +/// "MPAM" Memory System Resource Partitioning And Monitoring Table +/// +#define EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_STRUCTURE_SIGNATURE SIGNATURE_32('M', 'P', 'A', 'M') + /// /// "PSDT" Persistent System Description Table /// diff --git a/MdePkg/Include/IndustryStandard/Mpam.h b/MdePkg/Include/IndustryStandard/Mpam.h new file mode 100644 index 0000000000..e0f75f0114 --- /dev/null +++ b/MdePkg/Include/IndustryStandard/Mpam.h @@ -0,0 +1,69 @@ +/** @file + ACPI Memory System Resource Partitioning And Monitoring (MPAM) + as specified in ARM spec DEN0065 + + Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved. + Copyright (c) 2022, ARM Limited. All rights reserved. + SPDX-License-Identifier: BSD-2-Clause-Patent [SAMI] Please add a '@par Specification Reference:' section as shown in https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/53_include_files#5.3.7-include-files-shall-not-generate-code-or-define-data-variables. e.g. @par Specification Reference: - [1] ACPI
for Memory System Resource Partitioning and Monitoring 2.0,
Platform Design Document (https://developer.arm.com/documentation/den0065/latest) [/SAMI] [SAMI] I think the leading underscore should not be present, see https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions/43_identifiers#4.3.5.4-the-names-of-guard-macros-shall-end-with-an-underscore-character.+**/ + +#ifndef _MPAM_H_ +#define _MPAM_H_
[SAMI] I do not see the NumNodes, NodeOffset and the Reserved filed
in the specification. Can you check, please?
+} EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER; [SAMI] I think the infix '_6_4_' is not going to work, as the
ACPI MPAM table specification is not going to be in sync with the
ACPI specification revision. So, this can be dropped. Also, it may be better to add a @glossary section in the file
header describing the MPAM acronym and abbreviate the structure
names to EFI_ACPI_MPAM_TABLE_HEADER. The macro describing the
table revision can also follow a similar approach. [/SAMI] [SAMI] I believe this should be (a defined by the 'ACPI for Memory System Resource Partitioning and Monitoring, Platform Design Document'). Or simply refer it as '(as defined by [1])'.+ +/// +/// MPAM Revision (as defined in ACPI 6.4 spec.) [SAMI] Please move this definition before the definition of EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER. The ACPI MPAM 1.0 table revision was 0. But for ACPI MPAM table 2.0 the revision is 1, so there is no need to change.+/// +#define EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_REVISION 0x01
[SAMI] ACPI MPAM 2.0 introduces splits the Reserved field to
introduce 'Interface type'.
[SAMI] Would it be possible to define a Locator structure with
the first field as the LocatorType followed by a union of Locators
type specific structures? + UINT32 NumFuncDep; +} EFI_ACPI_6_4_MPAM_RESOURCE_NODE; + [SAMI] Do you have plans to add definitions for the other structures defined by the specifications, or the intention is to add them as required? Also, if possible, it would be good to add an ACPI MPAM table parser to Acpiview. [/SAMI] +#pragma pack() + +#endif
|
|
|