* [PATCH v4 1/6] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe
2022-09-28 15:33 [PATCH v4 0/6] Add safe unaccepted memory behavior Dionna Glaze
@ 2022-09-28 15:33 ` Dionna Glaze
2022-09-28 16:29 ` [edk2-devel] " Ard Biesheuvel
2022-09-28 21:02 ` Lendacky, Thomas
2022-09-28 15:33 ` [PATCH v4 2/6] MdeModulePkg: Add PcdEnableUnacceptedMemory Dionna Glaze
` (4 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Dionna Glaze @ 2022-09-28 15:33 UTC (permalink / raw)
To: devel
Cc: Dionna Glaze, Gerd Hoffmann, James Bottomley, Jiewen Yao,
Tom Lendacky, Sophia Wolf
From: Sophia Wolf <phiawolf@google.com>
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@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sophia Wolf <phiawolf@google.com>
---
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 34 ++++++++++++++++++++
OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 3 ++
OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c | 24 +++++++++++---
3 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index 662d3c4ccb..09aa15165d 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,29 @@ STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION mSnpBootDxeTable = {
FixedPcdGet32 (PcdOvmfCpuidSize),
};
+STATIC EFI_HANDLE mAmdSevDxeHandle = NULL;
+
+STATIC
+EFI_STATUS
+EFIAPI
+AmdSevMemoryAccept (
+ IN EFI_MEMORY_ACCEPT_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS StartAddress,
+ IN UINTN Size
+)
+{
+ MemEncryptSevSnpPreValidateSystemRam (
+ StartAddress,
+ EFI_SIZE_TO_PAGES (Size)
+ );
+
+ return EFI_SUCCESS;
+}
+
+STATIC EFI_MEMORY_ACCEPT_PROTOCOL mMemoryAcceptProtocol = {
+ AmdSevMemoryAccept
+};
+
EFI_STATUS
EFIAPI
AmdSevDxeEntryPoint (
@@ -147,6 +171,16 @@ AmdSevDxeEntryPoint (
}
}
+ Status = gBS->InstallProtocolInterface (
+ &mAmdSevDxeHandle,
+ &gEfiMemoryAcceptProtocolGuid,
+ EFI_NATIVE_INTERFACE,
+ &mMemoryAcceptProtocol
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Install EfiMemoryAcceptProtocol failed.\n"));
+ }
+
//
// 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.
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.37.3.998.g577e59143f-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v4 1/6] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe
2022-09-28 15:33 ` [PATCH v4 1/6] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe Dionna Glaze
@ 2022-09-28 16:29 ` Ard Biesheuvel
2022-09-28 21:02 ` Lendacky, Thomas
1 sibling, 0 replies; 19+ messages in thread
From: Ard Biesheuvel @ 2022-09-28 16:29 UTC (permalink / raw)
To: devel, dionnaglaze
Cc: Gerd Hoffmann, James Bottomley, Jiewen Yao, Tom Lendacky,
Sophia Wolf
On Wed, 28 Sept 2022 at 17:33, Dionna Glaze via groups.io
<dionnaglaze=google.com@groups.io> wrote:
>
> From: Sophia Wolf <phiawolf@google.com>
>
> 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@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>
> Signed-off-by: Sophia Wolf <phiawolf@google.com>
A signoff is an assertion by the contributor that the contribution is
compatible with the license. It is *not* a statement of authorship.
This means that you can keep the from: line, and credit the author in
another way in the commit log, but the signed-off-by needs to mention
the contributor of the patch not the author.
> ---
> OvmfPkg/AmdSevDxe/AmdSevDxe.c | 34 ++++++++++++++++++++
> OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 3 ++
> OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c | 24 +++++++++++---
> 3 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> index 662d3c4ccb..09aa15165d 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,29 @@ STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION mSnpBootDxeTable = {
> FixedPcdGet32 (PcdOvmfCpuidSize),
> };
>
> +STATIC EFI_HANDLE mAmdSevDxeHandle = NULL;
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +AmdSevMemoryAccept (
> + IN EFI_MEMORY_ACCEPT_PROTOCOL *This,
> + IN EFI_PHYSICAL_ADDRESS StartAddress,
> + IN UINTN Size
> +)
> +{
> + MemEncryptSevSnpPreValidateSystemRam (
> + StartAddress,
> + EFI_SIZE_TO_PAGES (Size)
> + );
> +
> + return EFI_SUCCESS;
> +}
> +
> +STATIC EFI_MEMORY_ACCEPT_PROTOCOL mMemoryAcceptProtocol = {
> + AmdSevMemoryAccept
> +};
> +
> EFI_STATUS
> EFIAPI
> AmdSevDxeEntryPoint (
> @@ -147,6 +171,16 @@ AmdSevDxeEntryPoint (
> }
> }
>
> + Status = gBS->InstallProtocolInterface (
> + &mAmdSevDxeHandle,
> + &gEfiMemoryAcceptProtocolGuid,
> + EFI_NATIVE_INTERFACE,
> + &mMemoryAcceptProtocol
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Install EfiMemoryAcceptProtocol failed.\n"));
> + }
> +
> //
> // 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.
> 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.37.3.998.g577e59143f-goog
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 1/6] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe
2022-09-28 15:33 ` [PATCH v4 1/6] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe Dionna Glaze
2022-09-28 16:29 ` [edk2-devel] " Ard Biesheuvel
@ 2022-09-28 21:02 ` Lendacky, Thomas
2022-09-30 17:48 ` Dionna Glaze
1 sibling, 1 reply; 19+ messages in thread
From: Lendacky, Thomas @ 2022-09-28 21:02 UTC (permalink / raw)
To: Dionna Glaze, devel
Cc: Gerd Hoffmann, James Bottomley, Jiewen Yao, Sophia Wolf
On 9/28/22 10:33, Dionna Glaze wrote:
> From: Sophia Wolf <phiawolf@google.com>
>
> 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@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>
> Signed-off-by: Sophia Wolf <phiawolf@google.com>
> ---
> OvmfPkg/AmdSevDxe/AmdSevDxe.c | 34 ++++++++++++++++++++
> OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 3 ++
> OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c | 24 +++++++++++---
> 3 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> index 662d3c4ccb..09aa15165d 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,29 @@ STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION mSnpBootDxeTable = {
> FixedPcdGet32 (PcdOvmfCpuidSize),
> };
>
> +STATIC EFI_HANDLE mAmdSevDxeHandle = NULL;
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +AmdSevMemoryAccept (
> + IN EFI_MEMORY_ACCEPT_PROTOCOL *This,
> + IN EFI_PHYSICAL_ADDRESS StartAddress,
> + IN UINTN Size
> +)
> +{
> + MemEncryptSevSnpPreValidateSystemRam (
> + StartAddress,
> + EFI_SIZE_TO_PAGES (Size)
Sorry, I forgot to ask this earlier in the series, but is StartAddress
guaranteed to be page-aligned and Size a multiple of 4KB? Should there be
any asserts for those just in case?
Also, can Size be 0? In which case MemEncryptSevSnpPreValidateSystemRam()
shouldn't be called?
> + );
> +
> + return EFI_SUCCESS;
> +}
> +
> +STATIC EFI_MEMORY_ACCEPT_PROTOCOL mMemoryAcceptProtocol = {
> + AmdSevMemoryAccept
> +};
> +
> EFI_STATUS
> EFIAPI
> AmdSevDxeEntryPoint (
> @@ -147,6 +171,16 @@ AmdSevDxeEntryPoint (
> }
> }
>
> + Status = gBS->InstallProtocolInterface (
> + &mAmdSevDxeHandle,
> + &gEfiMemoryAcceptProtocolGuid,
> + EFI_NATIVE_INTERFACE,
> + &mMemoryAcceptProtocol
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Install EfiMemoryAcceptProtocol failed.\n"));
> + }
Should this only be installed for an SNP guest, e.g. put within the
"if (MemEncryptSevSnpIsEnabled ()) {" check?
Maybe use ASSERT_EFI_ERROR (Status)?
Thanks,
Tom
> +
> //
> // 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.
> 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);
> }
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 1/6] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe
2022-09-28 21:02 ` Lendacky, Thomas
@ 2022-09-30 17:48 ` Dionna Glaze
0 siblings, 0 replies; 19+ messages in thread
From: Dionna Glaze @ 2022-09-30 17:48 UTC (permalink / raw)
To: Tom Lendacky
Cc: devel, Gerd Hoffmann, James Bottomley, Jiewen Yao, Sophia Wolf
> > +{
> > + MemEncryptSevSnpPreValidateSystemRam (
> > + StartAddress,
> > + EFI_SIZE_TO_PAGES (Size)
>
> Sorry, I forgot to ask this earlier in the series, but is StartAddress
> guaranteed to be page-aligned and Size a multiple of 4KB? Should there be
> any asserts for those just in case?
>
> Also, can Size be 0? In which case MemEncryptSevSnpPreValidateSystemRam()
> shouldn't be called?
>
It shouldn't happen, but I'll return EFI_INVALID_PARAMETER on those conditions.
> > + Status = gBS->InstallProtocolInterface (
> > + &mAmdSevDxeHandle,
> > + &gEfiMemoryAcceptProtocolGuid,
> > + EFI_NATIVE_INTERFACE,
> > + &mMemoryAcceptProtocol
> > + );
>
> Should this only be installed for an SNP guest, e.g. put within the
> "if (MemEncryptSevSnpIsEnabled ()) {" check?
>
> Maybe use ASSERT_EFI_ERROR (Status)?
>
Will do, thanks for your review.
--
-Dionna Glaze, PhD (she/her)
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 2/6] MdeModulePkg: Add PcdEnableUnacceptedMemory
2022-09-28 15:33 [PATCH v4 0/6] Add safe unaccepted memory behavior Dionna Glaze
2022-09-28 15:33 ` [PATCH v4 1/6] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe Dionna Glaze
@ 2022-09-28 15:33 ` Dionna Glaze
2022-09-28 16:33 ` Ard Biesheuvel
2022-09-28 15:33 ` [PATCH v4 3/6] OvmfPkg: set PcdEnableUnacceptedMemory to FALSE Dionna Glaze
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Dionna Glaze @ 2022-09-28 15:33 UTC (permalink / raw)
To: devel
Cc: Dionna Glaze, Gerd Hoffmann, James Bottomley, Jiewen Yao,
Tom Lendacky, Ard Biesheuvel
This Pcd is used to toggle whether ExitBootServices should not accept
all unaccepted memory. It's the loaded image's responsibility to enable
support so that it doesn't get memory types it doesn't understand in its
memory map.
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
MdeModulePkg/MdeModulePkg.dec | 6 ++++++
MdeModulePkg/MdeModulePkg.uni | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 58e6ab0048..dd07b3725a 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -2102,6 +2102,12 @@
# @Prompt The shared bit mask when Intel Tdx is enabled.
gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0|UINT64|0x10000025
+ ## Indicates if the memory map may include unaccepted memory after ExitBootServices().<BR><BR>
+ # TRUE - The memory map may include unaccepted memory after ExitBootServices().<BR>
+ # FALSE - The memory map may not include unaccepted memory after ExitBootServices().<BR>
+ # @Prompt Support unaccepted memory type.
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE|BOOLEAN|0x10000026
+
[PcdsPatchableInModule]
## Specify memory size with page number for PEI code when
# Loading Module at Fixed Address feature is enabled.
diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
index 33ce9f6198..fde57da123 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -1338,3 +1338,9 @@
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdPcieResizableBarSupport_HELP #language en-US "Indicates if the PCIe Resizable BAR Capability Supported.<BR><BR>\n"
"TRUE - PCIe Resizable BAR Capability is supported.<BR>\n"
"FALSE - PCIe Resizable BAR Capability is not supported.<BR>"
+
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdEnableUnacceptedMemory_PROMPT #language en-US "Support unaccepted memory type"
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdEnableUnacceptedMemory_HELP #language en-US "Indicates if the memory map may include unaccepted memory "
+ "after ExitBootServices().<BR><BR>\n"
+ "TRUE - The memory map may include unaccepted memory after ExitBootServices().<BR>\n"
+ "FALSE - The memory map may not include unaccepted memory after ExitBootServices().<BR>\n"
--
2.37.3.998.g577e59143f-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 2/6] MdeModulePkg: Add PcdEnableUnacceptedMemory
2022-09-28 15:33 ` [PATCH v4 2/6] MdeModulePkg: Add PcdEnableUnacceptedMemory Dionna Glaze
@ 2022-09-28 16:33 ` Ard Biesheuvel
2022-09-29 18:38 ` Dionna Glaze
0 siblings, 1 reply; 19+ messages in thread
From: Ard Biesheuvel @ 2022-09-28 16:33 UTC (permalink / raw)
To: Dionna Glaze
Cc: devel, Gerd Hoffmann, James Bottomley, Jiewen Yao, Tom Lendacky
On Wed, 28 Sept 2022 at 17:33, Dionna Glaze <dionnaglaze@google.com> wrote:
>
> This Pcd is used to toggle whether ExitBootServices should not accept
> all unaccepted memory. It's the loaded image's responsibility to enable
> support so that it doesn't get memory types it doesn't understand in its
> memory map.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
The name PcdEnableUnacceptedMemory is a bit confusing imho. Could we
rename this to PcdAcceptAllUnacceptedMemory or something like that?
> ---
> MdeModulePkg/MdeModulePkg.dec | 6 ++++++
> MdeModulePkg/MdeModulePkg.uni | 6 ++++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 58e6ab0048..dd07b3725a 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -2102,6 +2102,12 @@
> # @Prompt The shared bit mask when Intel Tdx is enabled.
> gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0|UINT64|0x10000025
>
> + ## Indicates if the memory map may include unaccepted memory after ExitBootServices().<BR><BR>
> + # TRUE - The memory map may include unaccepted memory after ExitBootServices().<BR>
> + # FALSE - The memory map may not include unaccepted memory after ExitBootServices().<BR>
> + # @Prompt Support unaccepted memory type.
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE|BOOLEAN|0x10000026
> +
> [PcdsPatchableInModule]
> ## Specify memory size with page number for PEI code when
> # Loading Module at Fixed Address feature is enabled.
> diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
> index 33ce9f6198..fde57da123 100644
> --- a/MdeModulePkg/MdeModulePkg.uni
> +++ b/MdeModulePkg/MdeModulePkg.uni
> @@ -1338,3 +1338,9 @@
> #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdPcieResizableBarSupport_HELP #language en-US "Indicates if the PCIe Resizable BAR Capability Supported.<BR><BR>\n"
> "TRUE - PCIe Resizable BAR Capability is supported.<BR>\n"
> "FALSE - PCIe Resizable BAR Capability is not supported.<BR>"
> +
> +#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdEnableUnacceptedMemory_PROMPT #language en-US "Support unaccepted memory type"
> +#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdEnableUnacceptedMemory_HELP #language en-US "Indicates if the memory map may include unaccepted memory "
> + "after ExitBootServices().<BR><BR>\n"
> + "TRUE - The memory map may include unaccepted memory after ExitBootServices().<BR>\n"
> + "FALSE - The memory map may not include unaccepted memory after ExitBootServices().<BR>\n"
> --
> 2.37.3.998.g577e59143f-goog
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/6] MdeModulePkg: Add PcdEnableUnacceptedMemory
2022-09-28 16:33 ` Ard Biesheuvel
@ 2022-09-29 18:38 ` Dionna Glaze
0 siblings, 0 replies; 19+ messages in thread
From: Dionna Glaze @ 2022-09-29 18:38 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: devel, Gerd Hoffmann, James Bottomley, Jiewen Yao, Tom Lendacky
> The name PcdEnableUnacceptedMemory is a bit confusing imho. Could we
> rename this to PcdAcceptAllUnacceptedMemory or something like that?
>
Will do. Is the protocol name EnableUnacceptedMemory still acceptable
now that it's setting an AcceptAllUnacceptedMemory value to FALSE?
--
-Dionna Glaze, PhD (she/her)
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 3/6] OvmfPkg: set PcdEnableUnacceptedMemory to FALSE
2022-09-28 15:33 [PATCH v4 0/6] Add safe unaccepted memory behavior Dionna Glaze
2022-09-28 15:33 ` [PATCH v4 1/6] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe Dionna Glaze
2022-09-28 15:33 ` [PATCH v4 2/6] MdeModulePkg: Add PcdEnableUnacceptedMemory Dionna Glaze
@ 2022-09-28 15:33 ` Dionna Glaze
2022-09-28 16:37 ` Ard Biesheuvel
2022-09-28 15:33 ` [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed Dionna Glaze
` (2 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Dionna Glaze @ 2022-09-28 15:33 UTC (permalink / raw)
To: devel
Cc: Dionna Glaze, Gerd Hoffmann, James Bottomley, Jiewen Yao,
Tom Lendacky, Ard Biesheuvel
The default value of PcdEnableUnacceptedMemory should be FALSE in order
for default safe behavior. If the next started image does not yet
understand UEFI v2.9's new memory type, then it's stuck with most of its
memory inaccessible.
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
OvmfPkg/Bhyve/BhyveX64.dsc | 2 ++
OvmfPkg/CloudHv/CloudHvX64.dsc | 2 ++
OvmfPkg/IntelTdx/IntelTdxX64.dsc | 2 ++
OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
OvmfPkg/OvmfPkgX64.dsc | 2 ++
OvmfPkg/OvmfXen.dsc | 2 ++
7 files changed, 13 insertions(+)
diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 90e8a213ef..23086748c5 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -526,6 +526,7 @@
# Set ConfidentialComputing defaults
gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr|0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
!include OvmfPkg/OvmfTpmPcds.dsc.inc
diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index 475b88b21a..004be8b019 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -559,6 +559,8 @@
# Set Tdx shared bit mask
gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
+
gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
# MdeModulePkg resolution sets up the system display resolution
diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index 10b16104ac..41f43a2631 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -618,6 +618,8 @@
# Set Tdx shared bit mask
gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
+
# Set SEV-ES defaults
gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
index c0c1a15b09..55b6a2a845 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
@@ -514,6 +514,8 @@
# Set Tdx shared bit mask
gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
+
# Set SEV-ES defaults
gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index af566b953f..aebe1c3192 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -655,6 +655,8 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
+
# Set SEV-ES defaults
gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index f39d9cd117..6e4418388e 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -679,6 +679,8 @@
# Set Tdx shared bit mask
gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
+
# Set SEV-ES defaults
gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 58a7c97cdd..0f57e22a2b 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -505,6 +505,8 @@
# Set Tdx shared bit mask
gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
+
gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
################################################################################
--
2.37.3.998.g577e59143f-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 3/6] OvmfPkg: set PcdEnableUnacceptedMemory to FALSE
2022-09-28 15:33 ` [PATCH v4 3/6] OvmfPkg: set PcdEnableUnacceptedMemory to FALSE Dionna Glaze
@ 2022-09-28 16:37 ` Ard Biesheuvel
2022-09-29 18:50 ` Dionna Glaze
0 siblings, 1 reply; 19+ messages in thread
From: Ard Biesheuvel @ 2022-09-28 16:37 UTC (permalink / raw)
To: Dionna Glaze
Cc: devel, Gerd Hoffmann, James Bottomley, Jiewen Yao, Tom Lendacky
On Wed, 28 Sept 2022 at 17:33, Dionna Glaze <dionnaglaze@google.com> wrote:
>
> The default value of PcdEnableUnacceptedMemory should be FALSE in order
> for default safe behavior. If the next started image does not yet
> understand UEFI v2.9's new memory type, then it's stuck with most of its
> memory inaccessible.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
Generally, we tend to rely on the DEC default for new PCDs if we're
not deviating from it.
If there is no specific reason to deviate from this here, I think we
can drop this patch.
Or is this also needed to declare them as the right type? In that
case, I think you can drop the hunks that touch non-CC platforms.
> ---
> OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
> OvmfPkg/Bhyve/BhyveX64.dsc | 2 ++
> OvmfPkg/CloudHv/CloudHvX64.dsc | 2 ++
> OvmfPkg/IntelTdx/IntelTdxX64.dsc | 2 ++
> OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
> OvmfPkg/OvmfPkgX64.dsc | 2 ++
> OvmfPkg/OvmfXen.dsc | 2 ++
> 7 files changed, 13 insertions(+)
>
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index 90e8a213ef..23086748c5 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -526,6 +526,7 @@
>
> # Set ConfidentialComputing defaults
> gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
>
> !include OvmfPkg/OvmfTpmPcds.dsc.inc
>
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index 475b88b21a..004be8b019 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -559,6 +559,8 @@
> # Set Tdx shared bit mask
> gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
> +
> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
>
> # MdeModulePkg resolution sets up the system display resolution
> diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
> index 10b16104ac..41f43a2631 100644
> --- a/OvmfPkg/CloudHv/CloudHvX64.dsc
> +++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
> @@ -618,6 +618,8 @@
> # Set Tdx shared bit mask
> gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
> +
> # Set SEV-ES defaults
> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
> diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> index c0c1a15b09..55b6a2a845 100644
> --- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> +++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> @@ -514,6 +514,8 @@
> # Set Tdx shared bit mask
> gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
> +
> # Set SEV-ES defaults
> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index af566b953f..aebe1c3192 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -655,6 +655,8 @@
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
> gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
> +
> # Set SEV-ES defaults
> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index f39d9cd117..6e4418388e 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -679,6 +679,8 @@
> # Set Tdx shared bit mask
> gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
> +
> # Set SEV-ES defaults
> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 58a7c97cdd..0f57e22a2b 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -505,6 +505,8 @@
> # Set Tdx shared bit mask
> gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory|FALSE
> +
> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
>
> ################################################################################
> --
> 2.37.3.998.g577e59143f-goog
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 3/6] OvmfPkg: set PcdEnableUnacceptedMemory to FALSE
2022-09-28 16:37 ` Ard Biesheuvel
@ 2022-09-29 18:50 ` Dionna Glaze
0 siblings, 0 replies; 19+ messages in thread
From: Dionna Glaze @ 2022-09-29 18:50 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: devel, Gerd Hoffmann, James Bottomley, Jiewen Yao, Tom Lendacky
> Generally, we tend to rely on the DEC default for new PCDs if we're
> not deviating from it.
> If there is no specific reason to deviate from this here, I think we
> can drop this patch.
>
> Or is this also needed to declare them as the right type? In that
> case, I think you can drop the hunks that touch non-CC platforms.
>
I probably did something wrong. Without this patch, the protocol patch
with PcdSetBoolS fails to build.
INFO - In file included from
/usr/local/google/home/dionnaglaze/gitrepos/edk2/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/AutoGen.h:17,
INFO - from <command-line>:
INFO - /usr/local/google/home/dionnaglaze/gitrepos/edk2/MdeModulePkg/Core/Dxe/Mem/Page.c:
In function ‘CoreEnableUnacceptedMemory’:
INFO - /usr/local/google/home/dionnaglaze/gitrepos/edk2/MdePkg/Include/Library/PcdLib.h:549:40:
error: implicit declaration of function
‘_PCD_SET_MODE_BOOL_S_PcdAcceptAllUnacceptedMemory’
[-Werror=implicit-function-declaration]
INFO - 549 | #define PcdSetBoolS(TokenName, Value)
_PCD_SET_MODE_BOOL_S_##TokenName ((Value))
INFO - | ^~~~~~~~~~~~~~~~~~~~~
INFO - /usr/local/google/home/dionnaglaze/gitrepos/edk2/MdeModulePkg/Core/Dxe/Mem/Page.c:2232:10:
note: in expansion of macro ‘PcdSetBoolS’
INFO - 2232 | return PcdSetBoolS(PcdAcceptAllUnacceptedMemory, FALSE);
INFO - | ^~~~~~~~~~~
INFO - cc1: all warnings being treated as errors
INFO - make: *** [GNUmakefile:452:
/usr/local/google/home/dionnaglaze/gitrepos/edk2/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Core/Dxe/DxeMain/OUTPUT/Mem/Page.obj]
Error 1
INFO -
What's the right way to do this?
--
-Dionna Glaze, PhD (she/her)
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed
2022-09-28 15:33 [PATCH v4 0/6] Add safe unaccepted memory behavior Dionna Glaze
` (2 preceding siblings ...)
2022-09-28 15:33 ` [PATCH v4 3/6] OvmfPkg: set PcdEnableUnacceptedMemory to FALSE Dionna Glaze
@ 2022-09-28 15:33 ` Dionna Glaze
2022-09-28 16:50 ` Ard Biesheuvel
2022-09-29 0:11 ` [edk2-devel] " Ni, Ray
2022-09-28 15:33 ` [PATCH v4 5/6] MdeModulePkg: add EnableUnacceptedMemoryProtocol Dionna Glaze
2022-09-28 15:33 ` [PATCH v4 6/6] OvmfPkg/PlatformPei: SEV-SNP make >=4GB unaccepted Dionna Glaze
5 siblings, 2 replies; 19+ messages in thread
From: Dionna Glaze @ 2022-09-28 15:33 UTC (permalink / raw)
To: devel
Cc: Dionna Glaze, Gerd Hoffmann, James Bottomley, Jiewen Yao,
Tom Lendacky, Ard Biesheuvel
With the addition of the EfiUnacceptedMemory memory type, it is possible
the EFI-enlightened guests do not themselves support the new memory
type. This commit uses the new PcdEnableUnacceptedMemory to enable
unaccepted memory support before ExitBootServices is called by not
accepting all unaccepted memory at EBS.
The expected usage is to set the new Pcd with a protocol that is usable
by bootloaders and directly-booted OSes when they can determine that the
OS does indeed support unaccepted memory.
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
MdeModulePkg/Core/Dxe/DxeMain.h | 10 +++
MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 14 +++-
MdeModulePkg/Core/Dxe/Mem/Page.c | 87 ++++++++++++++++++++
4 files changed, 112 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 815a6b4bd8..ac943c87a3 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -2698,6 +2698,16 @@ CoreInitializeMemoryProtection (
VOID
);
+/**
+ Accept and convert unaccepted memory to conventional memory if unaccepted
+ memory is not enabled and there is an implementation of MemoryAcceptProtocol
+ installed.
+ **/
+EFI_STATUS
+CoreResolveUnacceptedMemory (
+ VOID
+ );
+
/**
Install MemoryAttributesTable on memory allocation.
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index e4bca89577..deb8bb2ba8 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
+ gEfiMemoryAcceptProtocolGuid ## SOMETIMES_CONSUMES
# Arch Protocols
gEfiBdsArchProtocolGuid ## CONSUMES
@@ -186,6 +187,7 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory ## CONSUMES
# [Hob]
# RESOURCE_DESCRIPTOR ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
index 5733f0c8ec..8d1de32fe7 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
+++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
@@ -768,13 +768,25 @@ CoreExitBootServices (
//
gTimer->SetTimerPeriod (gTimer, 0);
+ //
+ // Accept all memory if unaccepted memory isn't enabled.
+ //
+ Status = CoreResolveUnacceptedMemory();
+ if (EFI_ERROR (Status)) {
+ //
+ // Notify other drivers that ExitBootServices failed
+ //
+ CoreNotifySignalList (&gEventExitBootServicesFailedGuid);
+ return Status;
+ }
+
//
// Terminate memory services if the MapKey matches
//
Status = CoreTerminateMemoryMap (MapKey);
if (EFI_ERROR (Status)) {
//
- // Notify other drivers that ExitBootServices fail
+ // Notify other drivers that ExitBootServices failed
//
CoreNotifySignalList (&gEventExitBootServicesFailedGuid);
return Status;
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index ffe79dcca9..cbebe62a28 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include "DxeMain.h"
#include "Imem.h"
#include "HeapGuard.h"
+#include <Library/PcdLib.h>
+#include <Protocol/MemoryAccept.h>
//
// Entry for tracking the memory regions for each memory type to coalesce similar memory types
@@ -2118,6 +2120,91 @@ CoreFreePoolPages (
CoreConvertPages (Memory, NumberOfPages, EfiConventionalMemory);
}
+EFI_EVENT gExitBootServiceEvent = NULL;
+
+STATIC
+EFI_STATUS
+AcceptAllUnacceptedMemory (
+ IN EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory
+ )
+{
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap;
+ UINTN NumEntries;
+ UINTN Index;
+ EFI_STATUS Status;
+
+ /*
+ * Get a copy of the memory space map to iterate over while
+ * changing the map.
+ */
+ Status = CoreGetMemorySpaceMap (&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
+ );
+ if (EFI_ERROR(Status)) {
+ goto done;
+ }
+
+ Status = CoreRemoveMemorySpace(Desc->BaseAddress, Desc->Length);
+ if (EFI_ERROR(Status)) {
+ goto done;
+ }
+
+ Status = CoreAddMemorySpace (
+ EfiGcdMemoryTypeSystemMemory,
+ Desc->BaseAddress,
+ Desc->Length,
+ EFI_MEMORY_CPU_CRYPTO | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_RP
+ );
+ if (EFI_ERROR(Status)) {
+ goto done;
+ }
+ }
+
+done:
+ FreePool (AllDescMap);
+ return Status;
+}
+
+EFI_STATUS
+CoreResolveUnacceptedMemory (
+ VOID
+ )
+{
+ EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory;
+ EFI_STATUS Status;
+
+ // No need to accept anything. Unaccepted memory is enabled.
+ if (PcdGetBool(PcdEnableUnacceptedMemory)) {
+ return EFI_SUCCESS;
+ }
+
+ Status = gBS->LocateProtocol (&gEfiMemoryAcceptProtocolGuid, NULL,
+ (VOID **)&AcceptMemory);
+ if (Status == EFI_NOT_FOUND) {
+ return EFI_SUCCESS;
+ }
+ if (Status != EFI_SUCCESS) {
+ DEBUG ((DEBUG_ERROR, "Error locating MemoryAcceptProtocol: %d\n", Status));
+ return Status;
+ }
+
+ return AcceptAllUnacceptedMemory(AcceptMemory);
+}
+
/**
Make sure the memory map is following all the construction rules,
it is the last time to check memory map error before exit boot services.
--
2.37.3.998.g577e59143f-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed
2022-09-28 15:33 ` [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed Dionna Glaze
@ 2022-09-28 16:50 ` Ard Biesheuvel
2022-09-29 0:11 ` [edk2-devel] " Ni, Ray
1 sibling, 0 replies; 19+ messages in thread
From: Ard Biesheuvel @ 2022-09-28 16:50 UTC (permalink / raw)
To: Dionna Glaze
Cc: devel, Gerd Hoffmann, James Bottomley, Jiewen Yao, Tom Lendacky
On Wed, 28 Sept 2022 at 17:33, Dionna Glaze <dionnaglaze@google.com> wrote:
>
> With the addition of the EfiUnacceptedMemory memory type, it is possible
> the EFI-enlightened guests do not themselves support the new memory
> type. This commit uses the new PcdEnableUnacceptedMemory to enable
> unaccepted memory support before ExitBootServices is called by not
> accepting all unaccepted memory at EBS.
>
> The expected usage is to set the new Pcd with a protocol that is usable
> by bootloaders and directly-booted OSes when they can determine that the
> OS does indeed support unaccepted memory.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
> MdeModulePkg/Core/Dxe/DxeMain.h | 10 +++
> MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
> MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 14 +++-
> MdeModulePkg/Core/Dxe/Mem/Page.c | 87 ++++++++++++++++++++
> 4 files changed, 112 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 815a6b4bd8..ac943c87a3 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> @@ -2698,6 +2698,16 @@ CoreInitializeMemoryProtection (
> VOID
> );
>
> +/**
> + Accept and convert unaccepted memory to conventional memory if unaccepted
> + memory is not enabled and there is an implementation of MemoryAcceptProtocol
> + installed.
> + **/
> +EFI_STATUS
> +CoreResolveUnacceptedMemory (
> + VOID
> + );
> +
> /**
> Install MemoryAttributesTable on memory allocation.
>
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index e4bca89577..deb8bb2ba8 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
> + gEfiMemoryAcceptProtocolGuid ## SOMETIMES_CONSUMES
>
> # Arch Protocols
> gEfiBdsArchProtocolGuid ## CONSUMES
> @@ -186,6 +187,7 @@
> gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory ## CONSUMES
>
> # [Hob]
> # RESOURCE_DESCRIPTOR ## CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> index 5733f0c8ec..8d1de32fe7 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> @@ -768,13 +768,25 @@ CoreExitBootServices (
> //
> gTimer->SetTimerPeriod (gTimer, 0);
>
> + //
> + // Accept all memory if unaccepted memory isn't enabled.
> + //
> + Status = CoreResolveUnacceptedMemory();
> + if (EFI_ERROR (Status)) {
> + //
> + // Notify other drivers that ExitBootServices failed
> + //
> + CoreNotifySignalList (&gEventExitBootServicesFailedGuid);
> + return Status;
> + }
> +
> //
> // Terminate memory services if the MapKey matches
> //
> Status = CoreTerminateMemoryMap (MapKey);
> if (EFI_ERROR (Status)) {
> //
> - // Notify other drivers that ExitBootServices fail
> + // Notify other drivers that ExitBootServices failed
> //
> CoreNotifySignalList (&gEventExitBootServicesFailedGuid);
> return Status;
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index ffe79dcca9..cbebe62a28 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include "DxeMain.h"
> #include "Imem.h"
> #include "HeapGuard.h"
> +#include <Library/PcdLib.h>
> +#include <Protocol/MemoryAccept.h>
>
> //
> // Entry for tracking the memory regions for each memory type to coalesce similar memory types
> @@ -2118,6 +2120,91 @@ CoreFreePoolPages (
> CoreConvertPages (Memory, NumberOfPages, EfiConventionalMemory);
> }
>
> +EFI_EVENT gExitBootServiceEvent = NULL;
> +
> +STATIC
> +EFI_STATUS
> +AcceptAllUnacceptedMemory (
> + IN EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory
> + )
> +{
> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap;
> + UINTN NumEntries;
> + UINTN Index;
> + EFI_STATUS Status;
> +
> + /*
> + * Get a copy of the memory space map to iterate over while
> + * changing the map.
> + */
> + Status = CoreGetMemorySpaceMap (&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
> + );
> + if (EFI_ERROR(Status)) {
> + goto done;
> + }
> +
> + Status = CoreRemoveMemorySpace(Desc->BaseAddress, Desc->Length);
> + if (EFI_ERROR(Status)) {
> + goto done;
> + }
> +
> + Status = CoreAddMemorySpace (
> + EfiGcdMemoryTypeSystemMemory,
> + Desc->BaseAddress,
> + Desc->Length,
> + EFI_MEMORY_CPU_CRYPTO | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_RP
> + );
> + if (EFI_ERROR(Status)) {
> + goto done;
> + }
> + }
> +
> +done:
> + FreePool (AllDescMap);
> + return Status;
> +}
> +
I am not following the logic here 100%. As far as I can tell, if
accepting all memory succeeded without errors, ExitBootServices()
returns with EFI_SUCCESS, even though it has modified the memory map.
This means the actual memory map is out of sync with the last
GetMemoryMap() call performed by the OS loader before it called
ExitBootServices(), and so it will still contain unaccepted memory,
right?
The approach I suggested before was to accept all memory and then
forcible fail the ExitBootServices() call [which is documented in the
spec as an expected occurrence, as events dispatched off the timer
interrupt may race and allocate or free pages between GetMemoryMap and
ExitBootServices). Doing so would force the caller to call
GetMemoryMap() again, which now no longer contains any unaccepted
memory, and call ExitBootServices() a second time.
This means that, afaict, the call to CoreResolveUnacceptedMemory () is
in the right spot, i.e., after the point where the timer interrupt is
disabled (so we don't risk failing in ExitBootServices() twice). I
also wonder whether we need to deal specifically with the fact that,
if CoreResolveUnacceptedMemory() accepts any memory, it will be called
again the second time around as well, but perhaps we can just rely on
the fact that no unaccepted regions should remain in the GCD memory
map. But a comment to that effect would be helpful.
> +EFI_STATUS
> +CoreResolveUnacceptedMemory (
> + VOID
> + )
> +{
> + EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory;
> + EFI_STATUS Status;
> +
> + // No need to accept anything. Unaccepted memory is enabled.
> + if (PcdGetBool(PcdEnableUnacceptedMemory)) {
> + return EFI_SUCCESS;
> + }
> +
> + Status = gBS->LocateProtocol (&gEfiMemoryAcceptProtocolGuid, NULL,
> + (VOID **)&AcceptMemory);
> + if (Status == EFI_NOT_FOUND) {
> + return EFI_SUCCESS;
> + }
> + if (Status != EFI_SUCCESS) {
> + DEBUG ((DEBUG_ERROR, "Error locating MemoryAcceptProtocol: %d\n", Status));
> + return Status;
> + }
> +
> + return AcceptAllUnacceptedMemory(AcceptMemory);
> +}
> +
> /**
> Make sure the memory map is following all the construction rules,
> it is the last time to check memory map error before exit boot services.
> --
> 2.37.3.998.g577e59143f-goog
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed
2022-09-28 15:33 ` [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed Dionna Glaze
2022-09-28 16:50 ` Ard Biesheuvel
@ 2022-09-29 0:11 ` Ni, Ray
2022-09-29 18:14 ` Dionna Glaze
1 sibling, 1 reply; 19+ messages in thread
From: Ni, Ray @ 2022-09-29 0:11 UTC (permalink / raw)
To: devel@edk2.groups.io, dionnaglaze@google.com
Cc: Gerd Hoffmann, James Bottomley, Yao, Jiewen, Tom Lendacky,
Ard Biesheuvel
Can you explain a bit more why this PCD is needed?
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dionna Glaze via groups.io
> Sent: Wednesday, September 28, 2022 11:33 PM
> To: devel@edk2.groups.io
> Cc: Dionna Glaze <dionnaglaze@google.com>; Gerd Hoffmann <kraxel@redhat.com>; James Bottomley
> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Ard
> Biesheuvel <ardb@kernel.org>
> Subject: [edk2-devel] [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed
>
> With the addition of the EfiUnacceptedMemory memory type, it is possible
> the EFI-enlightened guests do not themselves support the new memory
> type. This commit uses the new PcdEnableUnacceptedMemory to enable
> unaccepted memory support before ExitBootServices is called by not
> accepting all unaccepted memory at EBS.
>
> The expected usage is to set the new Pcd with a protocol that is usable
> by bootloaders and directly-booted OSes when they can determine that the
> OS does indeed support unaccepted memory.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
> MdeModulePkg/Core/Dxe/DxeMain.h | 10 +++
> MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
> MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 14 +++-
> MdeModulePkg/Core/Dxe/Mem/Page.c | 87 ++++++++++++++++++++
> 4 files changed, 112 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 815a6b4bd8..ac943c87a3 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> @@ -2698,6 +2698,16 @@ CoreInitializeMemoryProtection (
> VOID
> );
>
> +/**
> + Accept and convert unaccepted memory to conventional memory if unaccepted
> + memory is not enabled and there is an implementation of MemoryAcceptProtocol
> + installed.
> + **/
> +EFI_STATUS
> +CoreResolveUnacceptedMemory (
> + VOID
> + );
> +
> /**
> Install MemoryAttributesTable on memory allocation.
>
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index e4bca89577..deb8bb2ba8 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
> + gEfiMemoryAcceptProtocolGuid ## SOMETIMES_CONSUMES
>
> # Arch Protocols
> gEfiBdsArchProtocolGuid ## CONSUMES
> @@ -186,6 +187,7 @@
> gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory ## CONSUMES
>
> # [Hob]
> # RESOURCE_DESCRIPTOR ## CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> index 5733f0c8ec..8d1de32fe7 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> @@ -768,13 +768,25 @@ CoreExitBootServices (
> //
> gTimer->SetTimerPeriod (gTimer, 0);
>
> + //
> + // Accept all memory if unaccepted memory isn't enabled.
> + //
> + Status = CoreResolveUnacceptedMemory();
> + if (EFI_ERROR (Status)) {
> + //
> + // Notify other drivers that ExitBootServices failed
> + //
> + CoreNotifySignalList (&gEventExitBootServicesFailedGuid);
> + return Status;
> + }
> +
> //
> // Terminate memory services if the MapKey matches
> //
> Status = CoreTerminateMemoryMap (MapKey);
> if (EFI_ERROR (Status)) {
> //
> - // Notify other drivers that ExitBootServices fail
> + // Notify other drivers that ExitBootServices failed
> //
> CoreNotifySignalList (&gEventExitBootServicesFailedGuid);
> return Status;
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index ffe79dcca9..cbebe62a28 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include "DxeMain.h"
> #include "Imem.h"
> #include "HeapGuard.h"
> +#include <Library/PcdLib.h>
> +#include <Protocol/MemoryAccept.h>
>
> //
> // Entry for tracking the memory regions for each memory type to coalesce similar memory types
> @@ -2118,6 +2120,91 @@ CoreFreePoolPages (
> CoreConvertPages (Memory, NumberOfPages, EfiConventionalMemory);
> }
>
> +EFI_EVENT gExitBootServiceEvent = NULL;
> +
> +STATIC
> +EFI_STATUS
> +AcceptAllUnacceptedMemory (
> + IN EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory
> + )
> +{
> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap;
> + UINTN NumEntries;
> + UINTN Index;
> + EFI_STATUS Status;
> +
> + /*
> + * Get a copy of the memory space map to iterate over while
> + * changing the map.
> + */
> + Status = CoreGetMemorySpaceMap (&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
> + );
> + if (EFI_ERROR(Status)) {
> + goto done;
> + }
> +
> + Status = CoreRemoveMemorySpace(Desc->BaseAddress, Desc->Length);
> + if (EFI_ERROR(Status)) {
> + goto done;
> + }
> +
> + Status = CoreAddMemorySpace (
> + EfiGcdMemoryTypeSystemMemory,
> + Desc->BaseAddress,
> + Desc->Length,
> + EFI_MEMORY_CPU_CRYPTO | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_RP
> + );
> + if (EFI_ERROR(Status)) {
> + goto done;
> + }
> + }
> +
> +done:
> + FreePool (AllDescMap);
> + return Status;
> +}
> +
> +EFI_STATUS
> +CoreResolveUnacceptedMemory (
> + VOID
> + )
> +{
> + EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory;
> + EFI_STATUS Status;
> +
> + // No need to accept anything. Unaccepted memory is enabled.
> + if (PcdGetBool(PcdEnableUnacceptedMemory)) {
> + return EFI_SUCCESS;
> + }
> +
> + Status = gBS->LocateProtocol (&gEfiMemoryAcceptProtocolGuid, NULL,
> + (VOID **)&AcceptMemory);
> + if (Status == EFI_NOT_FOUND) {
> + return EFI_SUCCESS;
> + }
> + if (Status != EFI_SUCCESS) {
> + DEBUG ((DEBUG_ERROR, "Error locating MemoryAcceptProtocol: %d\n", Status));
> + return Status;
> + }
> +
> + return AcceptAllUnacceptedMemory(AcceptMemory);
> +}
> +
> /**
> Make sure the memory map is following all the construction rules,
> it is the last time to check memory map error before exit boot services.
> --
> 2.37.3.998.g577e59143f-goog
>
>
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed
2022-09-29 0:11 ` [edk2-devel] " Ni, Ray
@ 2022-09-29 18:14 ` Dionna Glaze
2022-09-30 8:06 ` Ard Biesheuvel
0 siblings, 1 reply; 19+ messages in thread
From: Dionna Glaze @ 2022-09-29 18:14 UTC (permalink / raw)
To: Ni, Ray
Cc: devel@edk2.groups.io, Gerd Hoffmann, James Bottomley, Yao, Jiewen,
Tom Lendacky, Ard Biesheuvel
>
> Can you explain a bit more why this PCD is needed?
>
I'll expand the comment further, but essentially we need a bit in the
firmware to persist from a call into a protocol to the eventual call
to ExitBootServices. If we needed offline persistence, then we'd need
to standardize a new bit in the OsIndications variable. We don't so we
make due with a Pcd.
>
> I am not following the logic here 100%. As far as I can tell, if
> accepting all memory succeeded without errors, ExitBootServices()
> returns with EFI_SUCCESS, even though it has modified the memory map.
> This means the actual memory map is out of sync with the last
> GetMemoryMap() call performed by the OS loader before it called
> ExitBootServices(), and so it will still contain unaccepted memory,
> right?
Ah yes you're right, I missed that part. I'll change it to return
EFI_WARN_STALE_DATA if any memory region gets accepted, and force a
failure in DxeMain if the status is an error or that warning.
--
-Dionna Glaze, PhD (she/her)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [edk2-devel] [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed
2022-09-29 18:14 ` Dionna Glaze
@ 2022-09-30 8:06 ` Ard Biesheuvel
0 siblings, 0 replies; 19+ messages in thread
From: Ard Biesheuvel @ 2022-09-30 8:06 UTC (permalink / raw)
To: Dionna Amalie Glaze, Ni, Ray
Cc: devel@edk2.groups.io, Gerd Hoffmann, James Bottomley, Yao, Jiewen,
Tom Lendacky
On Thu, 29 Sept 2022 at 20:14, Dionna Amalie Glaze
<dionnaglaze@google.com> wrote:
>
> >
> > Can you explain a bit more why this PCD is needed?
> >
>
> I'll expand the comment further, but essentially we need a bit in the
> firmware to persist from a call into a protocol to the eventual call
> to ExitBootServices. If we needed offline persistence, then we'd need
> to standardize a new bit in the OsIndications variable. We don't so we
> make due with a Pcd.
>
As I mentioned elsewhere, I'd prefer to avoid a dynamic PCD here.
For Ray's benefit, I'll copy/paste my proposal here that I made in one
of the other thread:
"""
Given that dynamic PCDs are optional but protocols are not, can we
just drop the PCD and (in view of some other comments below) do the
following:
- Define a protocol in MdeModulePkg that ExitBootServices() will
invoke after disarming the timer but before finalizing the memory map.
It doesn't have to be specific to memory acceptance at all, the only
thing that matters is that it is invoked at the right time (and
perhaps only on the first call to EBS()). E.g.,
gEdkiiExitBootServicesCallbackProtocol with a single method called
TerminateMemoryMapPreHook(). This protocol is fundamentally internal
to EDK2 and does not require inclusion in PI or UEFI
- Define a protocol in OvmfPkg that will be called by the OS loader to
indicate that it is capable of taking charge of the unaccepted memory,
and so it does not need the firmware to do so on its behalf, by
accepting all of it during ExitBootServices().
- Implement a single DXE driver in OvmfPkg (or add the functionality
to an existing one) that provides an implementation of the former
protocol, and implements the latter protocol by uninstalling the
former again. This means the 'accept all memory' routine can be moved
out of DXE core as well, and into your driver.
"""
>
> >
> > I am not following the logic here 100%. As far as I can tell, if
> > accepting all memory succeeded without errors, ExitBootServices()
> > returns with EFI_SUCCESS, even though it has modified the memory map.
> > This means the actual memory map is out of sync with the last
> > GetMemoryMap() call performed by the OS loader before it called
> > ExitBootServices(), and so it will still contain unaccepted memory,
> > right?
>
> Ah yes you're right, I missed that part. I'll change it to return
> EFI_WARN_STALE_DATA if any memory region gets accepted, and force a
> failure in DxeMain if the status is an error or that warning.
>
The only documented error code for ExitBootServices() is
EFI_INVALID_PARAMETER, which indicates that the map key has changes,
so this is the one you should return in this case. EFI_WARN_STALE_DATA
is not an error code (it has the MSB cleared) so macros like
EFI_ERROR() will not identify it as an error.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 5/6] MdeModulePkg: add EnableUnacceptedMemoryProtocol
2022-09-28 15:33 [PATCH v4 0/6] Add safe unaccepted memory behavior Dionna Glaze
` (3 preceding siblings ...)
2022-09-28 15:33 ` [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed Dionna Glaze
@ 2022-09-28 15:33 ` Dionna Glaze
2022-09-28 17:27 ` Ard Biesheuvel
2022-09-28 15:33 ` [PATCH v4 6/6] OvmfPkg/PlatformPei: SEV-SNP make >=4GB unaccepted Dionna Glaze
5 siblings, 1 reply; 19+ messages in thread
From: Dionna Glaze @ 2022-09-28 15:33 UTC (permalink / raw)
To: devel
Cc: Dionna Glaze, Gerd Hoffmann, James Bottomley, Jiewen Yao,
Tom Lendacky, Ard Biesheuvel
Add a simple protocol that enables the use of the unaccepted memory
type. Must be called before ExitBootServices to be effective.
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
MdeModulePkg/Core/Dxe/DxeMain.h | 22 ++++++++++++
MdeModulePkg/Core/Dxe/DxeMain.inf | 3 +-
MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 5 +++
MdeModulePkg/Core/Dxe/Mem/Page.c | 35 ++++++++++++++++++++
MdeModulePkg/MdeModulePkg.dec | 3 ++
5 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index ac943c87a3..5f0114b04f 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -2708,6 +2708,28 @@ CoreResolveUnacceptedMemory (
VOID
);
+
+typedef struct _ENABLE_UNACCEPTED_MEMORY_PROTOCOL
+ ENABLE_UNACCEPTED_MEMORY_PROTOCOL;
+
+typedef EFI_STATUS (EFIAPI *ENABLE_UNACCEPTED_MEMORY)(
+ IN ENABLE_UNACCEPTED_MEMORY_PROTOCOL *
+ );
+
+struct _ENABLE_UNACCEPTED_MEMORY_PROTOCOL {
+ ENABLE_UNACCEPTED_MEMORY Enable;
+};
+
+extern EFI_GUID gEnableUnacceptedMemoryProtocolGuid;
+
+/**
+ Implement the protocol for enabling unaccepted memory.
+ **/
+VOID
+InstallEnableUnacceptedMemoryProtocol (
+ VOID
+ );
+
/**
Install MemoryAttributesTable on memory allocation.
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index deb8bb2ba8..39dcac98bb 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -122,6 +122,7 @@
gEfiMemoryAttributesTableGuid ## SOMETIMES_PRODUCES ## SystemTable
gEfiEndOfDxeEventGroupGuid ## SOMETIMES_CONSUMES ## Event
gEfiHobMemoryAllocStackGuid ## SOMETIMES_CONSUMES ## SystemTable
+ gEnableUnacceptedMemoryProtocolGuid ## PRODUCES ## GUID # Install protocol
[Ppis]
gEfiVectorHandoffInfoPpiGuid ## UNDEFINED # HOB
@@ -187,7 +188,7 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth ## CONSUMES
- gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory ## CONSUMES ## SOMETIMES_PRODUCES
# [Hob]
# RESOURCE_DESCRIPTOR ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
index 8d1de32fe7..bc1a8ab6b2 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
+++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
@@ -354,6 +354,11 @@ DxeMain (
Status = CoreInstallConfigurationTable (&gEfiMemoryTypeInformationGuid, &gMemoryTypeInformation);
ASSERT_EFI_ERROR (Status);
+ //
+ // Install unaccepted memory configuration protocol
+ //
+ InstallEnableUnacceptedMemoryProtocol();
+
//
// If Loading modules At fixed address feature is enabled, install Load moduels at fixed address
// Configuration Table so that user could easily to retrieve the top address to load Dxe and PEI
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index cbebe62a28..10e152d80d 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -96,6 +96,14 @@ EFI_MEMORY_TYPE_INFORMATION gMemoryTypeInformation[EfiMaxMemoryType + 1] = {
//
GLOBAL_REMOVE_IF_UNREFERENCED BOOLEAN gLoadFixedAddressCodeMemoryReady = FALSE;
+EFI_STATUS EFIAPI CoreEnableUnacceptedMemory(IN ENABLE_UNACCEPTED_MEMORY_PROTOCOL *);
+
+struct {
+ ENABLE_UNACCEPTED_MEMORY enable;
+} mEnableUnacceptedMemoryProtocol = {
+ CoreEnableUnacceptedMemory,
+};
+
/**
Enter critical section by gaining lock on gMemoryLock.
@@ -2205,6 +2213,33 @@ CoreResolveUnacceptedMemory (
return AcceptAllUnacceptedMemory(AcceptMemory);
}
+EFI_STATUS
+EFIAPI
+CoreEnableUnacceptedMemory (
+ IN ENABLE_UNACCEPTED_MEMORY_PROTOCOL *This
+ )
+{
+ return PcdSetBoolS(PcdEnableUnacceptedMemory, TRUE);
+}
+
+VOID
+InstallEnableUnacceptedMemoryProtocol (
+ VOID
+ )
+{
+ EFI_HANDLE Handle;
+ EFI_STATUS Status;
+
+ Handle = NULL;
+ Status = CoreInstallMultipleProtocolInterfaces (
+ &Handle,
+ &gEnableUnacceptedMemoryProtocolGuid,
+ &mEnableUnacceptedMemoryProtocol,
+ NULL
+ );
+ ASSERT_EFI_ERROR (Status);
+}
+
/**
Make sure the memory map is following all the construction rules,
it is the last time to check memory map error before exit boot services.
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index dd07b3725a..ce72c06a93 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -244,6 +244,9 @@
gEdkiiPerformanceMeasurementProtocolGuid = { 0xc85d06be, 0x5f75, 0x48ce, { 0xa8, 0x0f, 0x12, 0x36, 0xba, 0x3b, 0x87, 0xb1 } }
gEdkiiSmmPerformanceMeasurementProtocolGuid = { 0xd56b6d73, 0x1a7b, 0x4015, { 0x9b, 0xb4, 0x7b, 0x07, 0x17, 0x29, 0xed, 0x24 } }
+ ## Bootloader protocol Guid for enabling unaccepted memory support.
+ gEnableUnacceptedMemoryProtocolGuid = { 0xc5a010fe, 0x38a7, 0x4531, { 0x8a, 0x4a, 0x05, 0x00, 0xd2, 0xfd, 0x16, 0x49 } }
+
## Guid is defined for CRC32 encapsulation scheme.
# Include/Guid/Crc32GuidedSectionExtraction.h
gEfiCrc32GuidedSectionExtractionGuid = { 0xFC1BCDB0, 0x7D31, 0x49aa, {0x93, 0x6A, 0xA4, 0x60, 0x0D, 0x9D, 0xD0, 0x83 } }
--
2.37.3.998.g577e59143f-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 5/6] MdeModulePkg: add EnableUnacceptedMemoryProtocol
2022-09-28 15:33 ` [PATCH v4 5/6] MdeModulePkg: add EnableUnacceptedMemoryProtocol Dionna Glaze
@ 2022-09-28 17:27 ` Ard Biesheuvel
0 siblings, 0 replies; 19+ messages in thread
From: Ard Biesheuvel @ 2022-09-28 17:27 UTC (permalink / raw)
To: Dionna Glaze, Liming Gao (Byosoft address), Andrew Fish,
Michael Kinney
Cc: devel, Gerd Hoffmann, James Bottomley, Jiewen Yao, Tom Lendacky
(cc some other maintainers)
On Wed, 28 Sept 2022 at 17:33, Dionna Glaze <dionnaglaze@google.com> wrote:
>
> Add a simple protocol that enables the use of the unaccepted memory
> type. Must be called before ExitBootServices to be effective.
Calling protocols is generally not permitted after ExitBootServices()
anyway, so this statement is somewhat redundant.
I'd clarify here is that it is the expectation that the OS loader not
the firmware invokes this protocol (if it exists) to inform the
firmware that it should not accept all memory on behalf of the OS.
It also means we have to align this very carefully, and perhaps
promote this to a formal EFI protocol.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
AIUI, the only thing the protocol does is change the value of the PCD, right?
Given that dynamic PCDs are optional but protocols are not, can we
just drop the PCD and (in view of some other comments below) do the
following:
- Define a protocol in MdeModulePkg that ExitBootServices() will
invoke after disarming the timer but before finalizing the memory map.
It doesn't have to be specific to memory acceptance at all, the only
thing that matters is that it is invoked at the right time (and
perhaps only on the first call to EBS()). E.g.,
gEdkiiExitBootServicesCallbackProtocol with a single method called
TerminateMemoryMapPreHook(). This protocol is fundamentally internal
to EDK2 and does not require inclusion in PI or UEFI
- Define a protocol in OvmfPkg that will be called by the OS loader to
indicate that it is capable of taking charge of the unaccepted memory,
and so it does not need the firmware to do so on its behalf, by
accepting all of it during ExitBootServices(). [Basically, this patch
but moved out of MdeModulePkg]
- Implement a single DXE driver in OvmfPkg (or add the functionality
to an existing one) that provides an implementation of the former
protocol, and implements the latter protocol by uninstalling the
former again. This means the 'accept all memory' routine can be moved
out of DXE core as well, and into your driver.
Apologies for not bringing this up before arriving at v4 of your
series, but i think these change will be a lot more palatable to the
MdeModulePkg maintainers if the DXE core changes are minimal, and not
specifically tied to memory acceptance.
(some more comments below)
> ---
> MdeModulePkg/Core/Dxe/DxeMain.h | 22 ++++++++++++
> MdeModulePkg/Core/Dxe/DxeMain.inf | 3 +-
> MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 5 +++
> MdeModulePkg/Core/Dxe/Mem/Page.c | 35 ++++++++++++++++++++
> MdeModulePkg/MdeModulePkg.dec | 3 ++
> 5 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> index ac943c87a3..5f0114b04f 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> @@ -2708,6 +2708,28 @@ CoreResolveUnacceptedMemory (
> VOID
> );
>
> +
> +typedef struct _ENABLE_UNACCEPTED_MEMORY_PROTOCOL
> + ENABLE_UNACCEPTED_MEMORY_PROTOCOL;
> +
> +typedef EFI_STATUS (EFIAPI *ENABLE_UNACCEPTED_MEMORY)(
> + IN ENABLE_UNACCEPTED_MEMORY_PROTOCOL *
> + );
> +
> +struct _ENABLE_UNACCEPTED_MEMORY_PROTOCOL {
> + ENABLE_UNACCEPTED_MEMORY Enable;
> +};
> +
Protocol definitions should live in a dedicated .h file under Include/Protocol/
> +extern EFI_GUID gEnableUnacceptedMemoryProtocolGuid;
> +
We tend to use the Edkii prefix for EDK2 protocols that have no basis
in the PI or UEFI specifications. So in this case
gEdkiiEnableUnacceptedMemoryProtocolGuid
unless we decide that this should be in the EFI spec, in which case
we'll need to do a code-first ECR. I'd be happy to collaborate on
that. OTOH, I have no problems whatsoever with adopting a protocol on
the Linux side that is not in the EFI spec.
Same comment as before applies, though: could we find a better name for this?
> +/**
> + Implement the protocol for enabling unaccepted memory.
> + **/
> +VOID
> +InstallEnableUnacceptedMemoryProtocol (
> + VOID
> + );
> +
> /**
> Install MemoryAttributesTable on memory allocation.
>
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index deb8bb2ba8..39dcac98bb 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -122,6 +122,7 @@
> gEfiMemoryAttributesTableGuid ## SOMETIMES_PRODUCES ## SystemTable
> gEfiEndOfDxeEventGroupGuid ## SOMETIMES_CONSUMES ## Event
> gEfiHobMemoryAllocStackGuid ## SOMETIMES_CONSUMES ## SystemTable
> + gEnableUnacceptedMemoryProtocolGuid ## PRODUCES ## GUID # Install protocol
>
This should be under [Protocols] not [Guids]
> [Ppis]
> gEfiVectorHandoffInfoPpiGuid ## UNDEFINED # HOB
> @@ -187,7 +188,7 @@
> gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth ## CONSUMES
> - gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory ## CONSUMES ## SOMETIMES_PRODUCES
>
> # [Hob]
> # RESOURCE_DESCRIPTOR ## CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> index 8d1de32fe7..bc1a8ab6b2 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> @@ -354,6 +354,11 @@ DxeMain (
> Status = CoreInstallConfigurationTable (&gEfiMemoryTypeInformationGuid, &gMemoryTypeInformation);
> ASSERT_EFI_ERROR (Status);
>
> + //
> + // Install unaccepted memory configuration protocol
> + //
> + InstallEnableUnacceptedMemoryProtocol();
> +
> //
> // If Loading modules At fixed address feature is enabled, install Load moduels at fixed address
> // Configuration Table so that user could easily to retrieve the top address to load Dxe and PEI
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index cbebe62a28..10e152d80d 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -96,6 +96,14 @@ EFI_MEMORY_TYPE_INFORMATION gMemoryTypeInformation[EfiMaxMemoryType + 1] = {
> //
> GLOBAL_REMOVE_IF_UNREFERENCED BOOLEAN gLoadFixedAddressCodeMemoryReady = FALSE;
>
> +EFI_STATUS EFIAPI CoreEnableUnacceptedMemory(IN ENABLE_UNACCEPTED_MEMORY_PROTOCOL *);
> +
> +struct {
> + ENABLE_UNACCEPTED_MEMORY enable;
> +} mEnableUnacceptedMemoryProtocol = {
> + CoreEnableUnacceptedMemory,
> +};
> +
> /**
> Enter critical section by gaining lock on gMemoryLock.
>
> @@ -2205,6 +2213,33 @@ CoreResolveUnacceptedMemory (
> return AcceptAllUnacceptedMemory(AcceptMemory);
> }
>
> +EFI_STATUS
> +EFIAPI
> +CoreEnableUnacceptedMemory (
> + IN ENABLE_UNACCEPTED_MEMORY_PROTOCOL *This
> + )
> +{
> + return PcdSetBoolS(PcdEnableUnacceptedMemory, TRUE);
> +}
> +
> +VOID
> +InstallEnableUnacceptedMemoryProtocol (
> + VOID
> + )
> +{
> + EFI_HANDLE Handle;
> + EFI_STATUS Status;
> +
> + Handle = NULL;
> + Status = CoreInstallMultipleProtocolInterfaces (
> + &Handle,
> + &gEnableUnacceptedMemoryProtocolGuid,
> + &mEnableUnacceptedMemoryProtocol,
> + NULL
> + );
> + ASSERT_EFI_ERROR (Status);
> +}
> +
> /**
> Make sure the memory map is following all the construction rules,
> it is the last time to check memory map error before exit boot services.
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index dd07b3725a..ce72c06a93 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -244,6 +244,9 @@
> gEdkiiPerformanceMeasurementProtocolGuid = { 0xc85d06be, 0x5f75, 0x48ce, { 0xa8, 0x0f, 0x12, 0x36, 0xba, 0x3b, 0x87, 0xb1 } }
> gEdkiiSmmPerformanceMeasurementProtocolGuid = { 0xd56b6d73, 0x1a7b, 0x4015, { 0x9b, 0xb4, 0x7b, 0x07, 0x17, 0x29, 0xed, 0x24 } }
>
> + ## Bootloader protocol Guid for enabling unaccepted memory support.
> + gEnableUnacceptedMemoryProtocolGuid = { 0xc5a010fe, 0x38a7, 0x4531, { 0x8a, 0x4a, 0x05, 0x00, 0xd2, 0xfd, 0x16, 0x49 } }
> +
Should be under [Protocols] as well
> ## Guid is defined for CRC32 encapsulation scheme.
> # Include/Guid/Crc32GuidedSectionExtraction.h
> gEfiCrc32GuidedSectionExtractionGuid = { 0xFC1BCDB0, 0x7D31, 0x49aa, {0x93, 0x6A, 0xA4, 0x60, 0x0D, 0x9D, 0xD0, 0x83 } }
> --
> 2.37.3.998.g577e59143f-goog
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 6/6] OvmfPkg/PlatformPei: SEV-SNP make >=4GB unaccepted
2022-09-28 15:33 [PATCH v4 0/6] Add safe unaccepted memory behavior Dionna Glaze
` (4 preceding siblings ...)
2022-09-28 15:33 ` [PATCH v4 5/6] MdeModulePkg: add EnableUnacceptedMemoryProtocol Dionna Glaze
@ 2022-09-28 15:33 ` Dionna Glaze
5 siblings, 0 replies; 19+ messages in thread
From: Dionna Glaze @ 2022-09-28 15:33 UTC (permalink / raw)
To: devel
Cc: Dionna Glaze, Ard Biescheuvel, Min M. Xu, Gerd Hoffmann,
James Bottomley, Tom Lendacky, Jiewen Yao, Erdem Aktas
Instead of eagerly accepting all memory in PEI, only accept memory under
the 4GB address. This allows a loaded image to use the
ENABLE_UNACCEPTED_MEMORY_PROTOCOL to indicate that it can interpret the
memory type accordingly.
This classification is safe since ExitBootServices will accept and
reclassify the memory as conventional if the enable protocol is not
used.
Cc: Ard Biescheuvel <ardb@kernel.org>
Cc: "Min M. Xu" <min.m.xu@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
OvmfPkg/PlatformPei/AmdSev.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index 385562b44c..2a52d6f491 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -16,6 +16,7 @@
#include <Library/MemEncryptSevLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/PcdLib.h>
+#include <Pi/PrePiHob.h>
#include <PiPei.h>
#include <Register/Amd/Msr.h>
#include <Register/Intel/SmramSaveStateMap.h>
@@ -63,6 +64,10 @@ AmdSevSnpInitialize (
for (Hob.Raw = GetHobList (); !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB (Hob)) {
if ((Hob.Raw != NULL) && (GET_HOB_TYPE (Hob) == EFI_HOB_TYPE_RESOURCE_DESCRIPTOR)) {
ResourceHob = Hob.ResourceDescriptor;
+ if (ResourceHob->PhysicalStart >= SIZE_4GB) {
+ ResourceHob->ResourceType = EFI_RESOURCE_MEMORY_UNACCEPTED;
+ continue;
+ }
if (ResourceHob->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY) {
MemEncryptSevSnpPreValidateSystemRam (
--
2.37.3.998.g577e59143f-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread