[PATCH 0/3] SEV-SNP accepted memory and BeforeExitBootServices

[PATCH 0/3] SEV-SNP accepted memory and BeforeExitBootServices

[Dionna Glaze]

This is the first half of the patch series

[PATCH v8 0/7] Add safe unaccepted memory behavior

These patches add SEV-SNP support for the MemoryAccept protocol, and
implement an already standardized mechanism for performing any actions
just before terminating the memory map.

We implement a standardized event group from UEFI v2.9,
EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES, since it provides exactly
the right invocation point for eagerly accepting memory if eager
acceptance has not been disabled (i.e., unaccepted memory is enabled).

The use of the BeforeExitBootServices addition will come in the second
half of this series.

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>
Cc: Andrew Fish <afish@apple.com>
Cc: "Michael D. Kinney" <michael.d.kinney@intel.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>

Dionna Glaze (3):
  OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe
  MdePkg: Add EFI_EVENT_BEFORE_EXIT_BOOT_SERVICES_GUID
  MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

 MdeModulePkg/Core/Dxe/DxeMain.inf                                  |  1 +
 MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c                            |  6 +++
 MdePkg/Include/Guid/EventGroup.h                                   |  5 ++
 MdePkg/MdePkg.dec                                                  |  5 +-
 OvmfPkg/AmdSevDxe/AmdSevDxe.c                                      | 55 ++++++++++++++++++--
 OvmfPkg/AmdSevDxe/AmdSevDxe.inf                                    |  3 ++
 OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c | 24 +++++++--
 7 files changed, 90 insertions(+), 9 deletions(-)

-- 
2.38.1.431.g37b22c650d-goog

[PATCH 1/3] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe

[Dionna Glaze]

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>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 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..f7600c3c81 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 EDKII_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 EDKII_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,
+                    &gEdkiiMemoryAcceptProtocolGuid,
+                    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..cd1b686c53 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
@@ -47,6 +47,9 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
 
+[Protocols]
+  gEdkiiMemoryAcceptProtocolGuid
+
 [Guids]
   gConfidentialComputingSevSnpBlobGuid
 
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c
index d3a95e4913..cbcdd46f52 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.1.431.g37b22c650d-goog

[PATCH 2/3] MdePkg: Add EFI_EVENT_BEFORE_EXIT_BOOT_SERVICES_GUID

[Dionna Glaze]

Event group as defined in UEFI standard v2.9.

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>

Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 MdePkg/Include/Guid/EventGroup.h | 5 +++++
 MdePkg/MdePkg.dec                | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Guid/EventGroup.h b/MdePkg/Include/Guid/EventGroup.h
index 063d1f7157..64bfd4bab9 100644
--- a/MdePkg/Include/Guid/EventGroup.h
+++ b/MdePkg/Include/Guid/EventGroup.h
@@ -14,6 +14,11 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 extern EFI_GUID  gEfiEventExitBootServicesGuid;
 
+#define EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES \
+  { 0x8be0e274, 0x3970, 0x4b44, { 0x80, 0xc5, 0x1a, 0xb9, 0x50, 0x2f, 0x3b, 0xfc } }
+
+extern EFI_GUID  gEfiEventBeforeExitBootServicesGuid;
+
 #define EFI_EVENT_GROUP_VIRTUAL_ADDRESS_CHANGE \
   { 0x13fa7698, 0xc831, 0x49c7, { 0x87, 0xea, 0x8f, 0x43, 0xfc, 0xc2, 0x51, 0x96 } }
 
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 6b6bfbec29..359a85ea10 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -408,7 +408,10 @@
   gEfiEventMemoryMapChangeGuid   = { 0x78BEE926, 0x692F, 0x48FD, { 0x9E, 0xDB, 0x01, 0x42, 0x2E, 0xF0, 0xD7, 0xAB }}
 
   ## Include/Guid/EventGroup.h
-  gEfiEventVirtualAddressChangeGuid = { 0x13FA7698, 0xC831, 0x49C7, { 0x87, 0xEA, 0x8F, 0x43, 0xFC, 0xC2, 0x51, 0x96 }}
+  gEfiEventVirtualAddressChangeGuid   = { 0x13FA7698, 0xC831, 0x49C7, { 0x87, 0xEA, 0x8F, 0x43, 0xFC, 0xC2, 0x51, 0x96 }}
+
+  ## Include/Guid/EventGroup.h
+  gEfiEventBeforeExitBootServicesGuid = { 0x8BE0E274, 0x3970, 0x4B44, { 0x80, 0xC5, 0x1A, 0xB9, 0x50, 0x2F, 0x3B, 0xFC }}
 
   ## Include/Guid/EventGroup.h
   gEfiEventExitBootServicesGuid  = { 0x27ABF055, 0xB1B8, 0x4C26, { 0x80, 0x48, 0x74, 0x8F, 0x37, 0xBA, 0xA2, 0xDF }}
-- 
2.38.1.431.g37b22c650d-goog

[PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

[Dionna Glaze]

Location of notification is has been specified in UEFI v2.9.

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>
Cc: "Min M. Xu" <min.m.xu@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: "Michael D. Kinney" <michael.d.kinney@intel.com>
Cc: Ray Ni <ray.ni@intel.com>

Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 MdeModulePkg/Core/Dxe/DxeMain.inf       | 1 +
 MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index e4bca89577..35d5bf0dee 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -100,6 +100,7 @@
   gEfiEventVirtualAddressChangeGuid             ## CONSUMES             ## Event
   ## CONSUMES   ## Event
   ## PRODUCES   ## Event
+  gEfiEventBeforeExitBootServicesGuid
   gEfiEventExitBootServicesGuid
   gEfiHobMemoryAllocModuleGuid                  ## SOMETIMES_CONSUMES   ## HOB
   gEfiFirmwareFileSystem2Guid                   ## CONSUMES             ## GUID # Used to compare with FV's file system guid and get the FV's file system format
diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
index 5733f0c8ec..4683016ed7 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
+++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
@@ -763,6 +763,12 @@ CoreExitBootServices (
 {
   EFI_STATUS  Status;
 
+  //
+  // Notify other drivers of their last chance to use boot services
+  // before the memory map is terminated.
+  //
+  CoreNotifySignalList (&gEfiEventBeforeExitBootServicesGuid);
+
   //
   // Disable Timer
   //
-- 
2.38.1.431.g37b22c650d-goog

Re: [PATCH 1/3] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe

[Yao, Jiewen]

Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: Dionna Glaze <dionnaglaze@google.com>
> Sent: Wednesday, November 9, 2022 12:46 AM
> 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>
> Subject: [PATCH 1/3] OvmfPkg: Realize EfiMemoryAcceptProtocol in
> AmdSevDxe
> 
> 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>
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  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..f7600c3c81 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 EDKII_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 EDKII_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,
> +                    &gEdkiiMemoryAcceptProtocolGuid,
> +                    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..cd1b686c53 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> @@ -47,6 +47,9 @@
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
> 
> +[Protocols]
> +  gEdkiiMemoryAcceptProtocolGuid
> +
>  [Guids]
>    gConfidentialComputingSevSnpBlobGuid
> 
> diff --git
> a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidat
> e.c
> b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValida
> te.c
> index d3a95e4913..cbcdd46f52 100644
> ---
> a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidat
> e.c
> +++
> b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValida
> te.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.1.431.g37b22c650d-goog

Re: [edk2-devel] [PATCH 2/3] MdePkg: Add EFI_EVENT_BEFORE_EXIT_BOOT_SERVICES_GUID

[Michael D Kinney]

Hi Dionna,

For the feature you are working on, it looks like you only require the before exit boot services event.

However, the UEFI 2.9 Spec added the before and after events at the same time.

I recommend these patches be updated to add both events and signal both events.

Thanks,

Mike 


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dionna Glaze via groups.io
> Sent: Tuesday, November 8, 2022 8:46 AM
> To: devel@edk2.groups.io
> Cc: Dionna Glaze <dionnaglaze@google.com>; Ard Biescheuvel <ardb@kernel.org>; Xu, Min M <min.m.xu@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; James Bottomley <jejb@linux.ibm.com>; Tom Lendacky <Thomas.Lendacky@amd.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Aktas, Erdem <erdemaktas@google.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [edk2-devel] [PATCH 2/3] MdePkg: Add EFI_EVENT_BEFORE_EXIT_BOOT_SERVICES_GUID
> 
> Event group as defined in UEFI standard v2.9.
> 
> 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>
> 
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  MdePkg/Include/Guid/EventGroup.h | 5 +++++
>  MdePkg/MdePkg.dec                | 5 ++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Include/Guid/EventGroup.h b/MdePkg/Include/Guid/EventGroup.h
> index 063d1f7157..64bfd4bab9 100644
> --- a/MdePkg/Include/Guid/EventGroup.h
> +++ b/MdePkg/Include/Guid/EventGroup.h
> @@ -14,6 +14,11 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  extern EFI_GUID  gEfiEventExitBootServicesGuid;
> 
> +#define EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES \
> +  { 0x8be0e274, 0x3970, 0x4b44, { 0x80, 0xc5, 0x1a, 0xb9, 0x50, 0x2f, 0x3b, 0xfc } }
> +
> +extern EFI_GUID  gEfiEventBeforeExitBootServicesGuid;
> +
>  #define EFI_EVENT_GROUP_VIRTUAL_ADDRESS_CHANGE \
>    { 0x13fa7698, 0xc831, 0x49c7, { 0x87, 0xea, 0x8f, 0x43, 0xfc, 0xc2, 0x51, 0x96 } }
> 
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index 6b6bfbec29..359a85ea10 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -408,7 +408,10 @@
>    gEfiEventMemoryMapChangeGuid   = { 0x78BEE926, 0x692F, 0x48FD, { 0x9E, 0xDB, 0x01, 0x42, 0x2E, 0xF0, 0xD7, 0xAB }}
> 
>    ## Include/Guid/EventGroup.h
> -  gEfiEventVirtualAddressChangeGuid = { 0x13FA7698, 0xC831, 0x49C7, { 0x87, 0xEA, 0x8F, 0x43, 0xFC, 0xC2, 0x51, 0x96 }}
> +  gEfiEventVirtualAddressChangeGuid   = { 0x13FA7698, 0xC831, 0x49C7, { 0x87, 0xEA, 0x8F, 0x43, 0xFC, 0xC2, 0x51, 0x96 }}
> +
> +  ## Include/Guid/EventGroup.h
> +  gEfiEventBeforeExitBootServicesGuid = { 0x8BE0E274, 0x3970, 0x4B44, { 0x80, 0xC5, 0x1A, 0xB9, 0x50, 0x2F, 0x3B, 0xFC }}
> 
>    ## Include/Guid/EventGroup.h
>    gEfiEventExitBootServicesGuid  = { 0x27ABF055, 0xB1B8, 0x4C26, { 0x80, 0x48, 0x74, 0x8F, 0x37, 0xBA, 0xA2, 0xDF }}
> --
> 2.38.1.431.g37b22c650d-goog
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 2/3] MdePkg: Add EFI_EVENT_BEFORE_EXIT_BOOT_SERVICES_GUID

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dionna Glaze via groups.io
> Sent: Tuesday, November 8, 2022 8:46 AM
> To: devel@edk2.groups.io
> Cc: Dionna Glaze <dionnaglaze@google.com>; Ard Biescheuvel <ardb@kernel.org>; Xu, Min M <min.m.xu@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; James Bottomley <jejb@linux.ibm.com>; Tom Lendacky <Thomas.Lendacky@amd.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Aktas, Erdem <erdemaktas@google.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [edk2-devel] [PATCH 2/3] MdePkg: Add EFI_EVENT_BEFORE_EXIT_BOOT_SERVICES_GUID
> 
> Event group as defined in UEFI standard v2.9.
> 
> 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>
> 
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  MdePkg/Include/Guid/EventGroup.h | 5 +++++
>  MdePkg/MdePkg.dec                | 5 ++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Include/Guid/EventGroup.h b/MdePkg/Include/Guid/EventGroup.h
> index 063d1f7157..64bfd4bab9 100644
> --- a/MdePkg/Include/Guid/EventGroup.h
> +++ b/MdePkg/Include/Guid/EventGroup.h
> @@ -14,6 +14,11 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  extern EFI_GUID  gEfiEventExitBootServicesGuid;
> 
> +#define EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES \
> +  { 0x8be0e274, 0x3970, 0x4b44, { 0x80, 0xc5, 0x1a, 0xb9, 0x50, 0x2f, 0x3b, 0xfc } }
> +
> +extern EFI_GUID  gEfiEventBeforeExitBootServicesGuid;
> +
>  #define EFI_EVENT_GROUP_VIRTUAL_ADDRESS_CHANGE \
>    { 0x13fa7698, 0xc831, 0x49c7, { 0x87, 0xea, 0x8f, 0x43, 0xfc, 0xc2, 0x51, 0x96 } }
> 
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index 6b6bfbec29..359a85ea10 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -408,7 +408,10 @@
>    gEfiEventMemoryMapChangeGuid   = { 0x78BEE926, 0x692F, 0x48FD, { 0x9E, 0xDB, 0x01, 0x42, 0x2E, 0xF0, 0xD7, 0xAB }}
> 
>    ## Include/Guid/EventGroup.h
> -  gEfiEventVirtualAddressChangeGuid = { 0x13FA7698, 0xC831, 0x49C7, { 0x87, 0xEA, 0x8F, 0x43, 0xFC, 0xC2, 0x51, 0x96 }}
> +  gEfiEventVirtualAddressChangeGuid   = { 0x13FA7698, 0xC831, 0x49C7, { 0x87, 0xEA, 0x8F, 0x43, 0xFC, 0xC2, 0x51, 0x96 }}
> +
> +  ## Include/Guid/EventGroup.h
> +  gEfiEventBeforeExitBootServicesGuid = { 0x8BE0E274, 0x3970, 0x4B44, { 0x80, 0xC5, 0x1A, 0xB9, 0x50, 0x2F, 0x3B, 0xFC }}
> 
>    ## Include/Guid/EventGroup.h
>    gEfiEventExitBootServicesGuid  = { 0x27ABF055, 0xB1B8, 0x4C26, { 0x80, 0x48, 0x74, 0x8F, 0x37, 0xBA, 0xA2, 0xDF }}
> --
> 2.38.1.431.g37b22c650d-goog
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dionna Glaze via groups.io
> Sent: Tuesday, November 8, 2022 8:46 AM
> 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>; Xu, Min M
> <min.m.xu@intel.com>; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices
> 
> Location of notification is has been specified in UEFI v2.9.
> 
> 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>
> Cc: "Min M. Xu" <min.m.xu@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: "Michael D. Kinney" <michael.d.kinney@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> 
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.inf       | 1 +
>  MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index e4bca89577..35d5bf0dee 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -100,6 +100,7 @@
>    gEfiEventVirtualAddressChangeGuid             ## CONSUMES             ## Event
>    ## CONSUMES   ## Event
>    ## PRODUCES   ## Event
> +  gEfiEventBeforeExitBootServicesGuid
>    gEfiEventExitBootServicesGuid
>    gEfiHobMemoryAllocModuleGuid                  ## SOMETIMES_CONSUMES   ## HOB
>    gEfiFirmwareFileSystem2Guid                   ## CONSUMES             ## GUID # Used to compare with FV's file system guid
> and get the FV's file system format
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> index 5733f0c8ec..4683016ed7 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> @@ -763,6 +763,12 @@ CoreExitBootServices (
>  {
>    EFI_STATUS  Status;
> 
> +  //
> +  // Notify other drivers of their last chance to use boot services
> +  // before the memory map is terminated.
> +  //
> +  CoreNotifySignalList (&gEfiEventBeforeExitBootServicesGuid);
> +
>    //
>    // Disable Timer
>    //
> --
> 2.38.1.431.g37b22c650d-goog
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

[Dionna Glaze]

> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

Great, thanks Michael. With all three patches reviewed/acked, does
that mean they get merged, or what else has to be done? I'm new to
this.

--
-Dionna Glaze, PhD (she/her)

Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

[Michael D Kinney]

Hi Dionna,

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

I think you are at step 13.  If you have not done so already, please update the
commit messages with all the Reviewed-by and Acked-by tags and make sure your
branch is rebased against the latest edk2/master and update the PR with that
content and verify that all EDK II CI checks pass.

At that point, it is the responsibility of the EDK II Maintainers to review
the final state of the PR and set the "push" label if everything looks correct.

At this moment, we are in Soft Freeze and will be in Hard Freeze for the next
2 weeks.  If this is considered critical for the stable tag release, then 
please send a request to Liming with justification for stable tag.  Otherwise,
it will be merged after the stable tag release.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning

Thanks,

Mike



> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dionna Glaze via groups.io
> Sent: Thursday, November 10, 2022 8:44 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; 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>; Xu, Min M
> <min.m.xu@intel.com>; Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices
> 
> > Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> 
> Great, thanks Michael. With all three patches reviewed/acked, does
> that mean they get merged, or what else has to be done? I'm new to
> this.
> 
> --
> -Dionna Glaze, PhD (she/her)
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

[Dionna Glaze]

On Thu, Nov 10, 2022 at 8:55 AM Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Hi Dionna,
>
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
>
> I think you are at step 13.  If you have not done so already, please update the
> commit messages with all the Reviewed-by and Acked-by tags and make sure your
> branch is rebased against the latest edk2/master and update the PR with that
> content and verify that all EDK II CI checks pass.
>
> At that point, it is the responsibility of the EDK II Maintainers to review
> the final state of the PR and set the "push" label if everything looks correct.
>
> At this moment, we are in Soft Freeze and will be in Hard Freeze for the next
> 2 weeks.  If this is considered critical for the stable tag release, then
> please send a request to Liming with justification for stable tag.  Otherwise,
> it will be merged after the stable tag release.
>
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
>
> Thanks,
>
> Mike
>

Hi Mike, my PR was closed without comment
https://github.com/tianocore/edk2/pull/3623 so I rebased and created a
new PR after the holidays and hard freeze. I hope this isn't
considered bad practice https://github.com/tianocore/edk2/pull/3885

-- 
-Dionna Glaze, PhD (she/her)

Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

[Laszlo Ersek]

On 1/12/23 00:08, Dionna Glaze via groups.io wrote:
> On Thu, Nov 10, 2022 at 8:55 AM Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
>>
>> Hi Dionna,
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
>>
>> I think you are at step 13.  If you have not done so already, please update the
>> commit messages with all the Reviewed-by and Acked-by tags and make sure your
>> branch is rebased against the latest edk2/master and update the PR with that
>> content and verify that all EDK II CI checks pass.
>>
>> At that point, it is the responsibility of the EDK II Maintainers to review
>> the final state of the PR and set the "push" label if everything looks correct.
>>
>> At this moment, we are in Soft Freeze and will be in Hard Freeze for the next
>> 2 weeks.  If this is considered critical for the stable tag release, then
>> please send a request to Liming with justification for stable tag.  Otherwise,
>> it will be merged after the stable tag release.
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
>>
>> Thanks,
>>
>> Mike
>>
> 
> Hi Mike, my PR was closed without comment
> https://github.com/tianocore/edk2/pull/3623 so I rebased and created a
> new PR after the holidays and hard freeze. I hope this isn't
> considered bad practice https://github.com/tianocore/edk2/pull/3885
> 

I closed the PR -- similarly to how I manually closed 400+ stale PRs
then -- because it was not a maintainer-submitted PR with the "push"
label set. In other words, it was just a personal build, for triggering
a CI run.

And, in fact regardlessly of whether it was a "push"-labeled maintainer
PR or just a personal PR, the PR was almost two months old. Clearly
stale and abandoned -- per edk2 contribution workflow, anyway.

Please excuse me for not explaining this in a comment on the PR, but (as
I said above) I closed 400+ stale PRs manually within 10-15 minutes on
the github.com WebUI. The necessary muscle memory training didn't allow
for individual comments.

(I actually tried scripting the mass-closure at first, with the "gh"
utility, but something in the github.com data model was broken, and the
server wouldn't allow me to close those PRs with the utility, even
though I had authenticated the utility under my account, with a complete
set of scopes -- see my report at
<https://github.com/cli/cli/discussions/6814>.)

Regarding your new PR: it's again good for a personal CI run only. If it
completes fine, please post the patches to the mailing list, using
git-send-email.

(From browsing recent list traffic, it seems that your colleague Moritz
Fischer <moritzf@google.com> has posted patches like that to the list;
please consider consulting with Moritz regarding the git setup (SMTP
server etc) withing Google. Cc'ing Moritz now.)

When sending the patches like that, please CC the maintainers and
reviewers that are supposed to review them. Once they are happy with the
series, one of them will submit a PR with them, and set the "push" label
on the PR. When the CI run succeeds, the mergify bot will merge *that*
PR automatically.

Laszlo

Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

[Ard Biesheuvel]

On Thu, 12 Jan 2023 at 13:24, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 1/12/23 00:08, Dionna Glaze via groups.io wrote:
> > On Thu, Nov 10, 2022 at 8:55 AM Kinney, Michael D
> > <michael.d.kinney@intel.com> wrote:
> >>
> >> Hi Dionna,
> >>
> >> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
> >>
> >> I think you are at step 13.  If you have not done so already, please update the
> >> commit messages with all the Reviewed-by and Acked-by tags and make sure your
> >> branch is rebased against the latest edk2/master and update the PR with that
> >> content and verify that all EDK II CI checks pass.
> >>
> >> At that point, it is the responsibility of the EDK II Maintainers to review
> >> the final state of the PR and set the "push" label if everything looks correct.
> >>
> >> At this moment, we are in Soft Freeze and will be in Hard Freeze for the next
> >> 2 weeks.  If this is considered critical for the stable tag release, then
> >> please send a request to Liming with justification for stable tag.  Otherwise,
> >> it will be merged after the stable tag release.
> >>
> >> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
> >>
> >> Thanks,
> >>
> >> Mike
> >>
> >
> > Hi Mike, my PR was closed without comment
> > https://github.com/tianocore/edk2/pull/3623 so I rebased and created a
> > new PR after the holidays and hard freeze. I hope this isn't
> > considered bad practice https://github.com/tianocore/edk2/pull/3885
> >
>
> I closed the PR -- similarly to how I manually closed 400+ stale PRs
> then -- because it was not a maintainer-submitted PR with the "push"
> label set. In other words, it was just a personal build, for triggering
> a CI run.
>
> And, in fact regardlessly of whether it was a "push"-labeled maintainer
> PR or just a personal PR, the PR was almost two months old. Clearly
> stale and abandoned -- per edk2 contribution workflow, anyway.
>
> Please excuse me for not explaining this in a comment on the PR, but (as
> I said above) I closed 400+ stale PRs manually within 10-15 minutes on
> the github.com WebUI. The necessary muscle memory training didn't allow
> for individual comments.
>
> (I actually tried scripting the mass-closure at first, with the "gh"
> utility, but something in the github.com data model was broken, and the
> server wouldn't allow me to close those PRs with the utility, even
> though I had authenticated the utility under my account, with a complete
> set of scopes -- see my report at
> <https://github.com/cli/cli/discussions/6814>.)
>
> Regarding your new PR: it's again good for a personal CI run only. If it
> completes fine, please post the patches to the mailing list, using
> git-send-email.
>
> (From browsing recent list traffic, it seems that your colleague Moritz
> Fischer <moritzf@google.com> has posted patches like that to the list;
> please consider consulting with Moritz regarding the git setup (SMTP
> server etc) withing Google. Cc'ing Moritz now.)
>
> When sending the patches like that, please CC the maintainers and
> reviewers that are supposed to review them. Once they are happy with the
> series, one of them will submit a PR with them, and set the "push" label
> on the PR. When the CI run succeeds, the mergify bot will merge *that*
> PR automatically.
>

I think we were already way past this with Dionna's work - numerous
iterations have been posted and discussed, and the merge of the final
version was only deferred because of the soft freeze.

Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

[Laszlo Ersek]

On 1/12/23 14:38, Ard Biesheuvel wrote:
> On Thu, 12 Jan 2023 at 13:24, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 1/12/23 00:08, Dionna Glaze via groups.io wrote:
>>> On Thu, Nov 10, 2022 at 8:55 AM Kinney, Michael D
>>> <michael.d.kinney@intel.com> wrote:
>>>>
>>>> Hi Dionna,
>>>>
>>>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
>>>>
>>>> I think you are at step 13.  If you have not done so already,
>>>> please update the commit messages with all the Reviewed-by and
>>>> Acked-by tags and make sure your branch is rebased against the
>>>> latest edk2/master and update the PR with that content and verify
>>>> that all EDK II CI checks pass.
>>>>
>>>> At that point, it is the responsibility of the EDK II Maintainers
>>>> to review the final state of the PR and set the "push" label if
>>>> everything looks correct.

I didn't realize this had been adopted -- "upgrading" a contributor
submitted PR. (More below.)

>>>> At this moment, we are in Soft Freeze and will be in Hard Freeze
>>>> for the next 2 weeks.  If this is considered critical for the
>>>> stable tag release, then please send a request to Liming with
>>>> justification for stable tag.  Otherwise, it will be merged after
>>>> the stable tag release.
>>>>
>>>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
>>>>
>>>> Thanks,
>>>>
>>>> Mike
>>>>
>>>
>>> Hi Mike, my PR was closed without comment
>>> https://github.com/tianocore/edk2/pull/3623 so I rebased and created
>>> a new PR after the holidays and hard freeze. I hope this isn't
>>> considered bad practice https://github.com/tianocore/edk2/pull/3885
>>>
>>
>> I closed the PR -- similarly to how I manually closed 400+ stale PRs
>> then -- because it was not a maintainer-submitted PR with the "push"
>> label set. In other words, it was just a personal build, for
>> triggering a CI run.
>>
>> And, in fact regardlessly of whether it was a "push"-labeled
>> maintainer PR or just a personal PR, the PR was almost two months
>> old. Clearly stale and abandoned -- per edk2 contribution workflow,
>> anyway.
>>
>> Please excuse me for not explaining this in a comment on the PR, but
>> (as I said above) I closed 400+ stale PRs manually within 10-15
>> minutes on the github.com WebUI. The necessary muscle memory training
>> didn't allow for individual comments.
>>
>> (I actually tried scripting the mass-closure at first, with the "gh"
>> utility, but something in the github.com data model was broken, and
>> the server wouldn't allow me to close those PRs with the utility,
>> even though I had authenticated the utility under my account, with a
>> complete set of scopes -- see my report at
>> <https://github.com/cli/cli/discussions/6814>.)
>>
>> Regarding your new PR: it's again good for a personal CI run only. If
>> it completes fine, please post the patches to the mailing list, using
>> git-send-email.
>>
>> (From browsing recent list traffic, it seems that your colleague
>> Moritz Fischer <moritzf@google.com> has posted patches like that to
>> the list; please consider consulting with Moritz regarding the git
>> setup (SMTP server etc) withing Google. Cc'ing Moritz now.)
>>
>> When sending the patches like that, please CC the maintainers and
>> reviewers that are supposed to review them. Once they are happy with
>> the series, one of them will submit a PR with them, and set the
>> "push" label on the PR. When the CI run succeeds, the mergify bot
>> will merge *that* PR automatically.
>>
>
> I think we were already way past this with Dionna's work - numerous
> iterations have been posted and discussed, and the merge of the final
> version was only deferred because of the soft freeze.
>

That may very well be, but the specific PR I closed (3623) was not
queued by a maintainer, nor did it have the "push" label. So I'd expect
that now a maintainer has to submit a new PR, with the patches most
recently approved on the list.

The official documentation in the wiki at
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process#the-maintainer-process-for-the-edk-ii-project>
still says the maintainer has to use "git-am" for picking up the
patches, and to submit a new (separate) PR, with the "push" label set.
So I wouldn't expect the "push" label to be set, as an additional
maintainer action, on a contributor-submitted (pre-existent) PR.

Sorry for obsessing about this, but given the 400+ stale open PRs, it
was literally impossible to tell apart this one (3623) from the rest.
And in retrospect, I do think I was right to close 3623 as well. IIRC, I
left the PRs younger than 2 weeks alone.

Now, regarding *where* the most recent version of the series lives (i.e.
what needs to be picked up from the list by the maintainer, for
merging), that beats me. We're conducting the present discussion under
the following thread:

  [edk2-devel] [PATCH 0/3] SEV-SNP accepted memory and BeforeExitBootServices
  http://mid.mail-archive.com/20221108164616.3251967-1-dionnaglaze@google.com
  https://edk2.groups.io/g/devel/message/96088
  https://listman.redhat.com/archives/edk2-devel-archive/2022-November/055386.html

so I'm tempted to think that this is the version that needs to be
merged.

However, if I check the November 2022 archive, I see v2 as well:

  [edk2-devel] [PATCH v2 0/4] SEV-SNP accepted memory and BeforeExitBootServices
  https://listman.redhat.com/archives/edk2-devel-archive/2022-November/055402.html
  https://edk2.groups.io/g/devel/message/96104

Then again, I find, in the v2 thread:

  https://listman.redhat.com/archives/edk2-devel-archive/2022-December/056363.html
  https://edk2.groups.io/g/devel/message/97065

where Dionna writes, "we decided to go ahead and consider v1 of this
series as final".

So, in effect, this patch series had reached v8 previously (in
*October*), as a part of a larger series
(<https://listman.redhat.com/archives/edk2-devel-archive/2022-October/054823.html>),
then got re-started (split-out) as a smaller series, then reached v2
like that, then v2 got abandoned in favor of v1; all without a feature
tracking BZ linking the *evolution* of the patch series across the
various postings.

And then we're surprised stuff falls through the cracks, and I remain
the OCD person that alone finds it problematic that the PR tracker and
the BZ tracker are in total shambles ¯\_(ツ)_/¯

Anyway, Dionna pinging in this particular thread (= v1 of the split-out
series) is consistent with the comment quoted from v2 qualifying v1 the
final version. As a courtesy, while my temporarily renewed subscription
to this list lasts a bit longer, I'm going to grab the v1 series in full
from the November (gzipped mbox) archive at
<https://listman.redhat.com/archives/edk2-devel-archive/>, and merge it.

... I've picked up the patches, picked up the pending feedback tags too,
and compared the result against Dionna's latest PR
<https://github.com/tianocore/edk2/pull/3885>.

Beyond the Message-Id tags that git-am picked up on my side, I've found
another difference: patch#1 was originally authored by Sophia Wolf
<phiawolf@google.com>, but that fact has been mangled in the first patch
of <https://github.com/tianocore/edk2/pull/3885> -- commit cd4c186f1099.
Namely, in commit cd4c186f1099, the original authorship is recorded as
another line in the commit message, when it should be reflected by the
Author meta-datum in git. So I'm going to stick with what I have, from
the list archive and git-am (git-am correctly translates the From: line
back to the Author meta-datum), for the new PR:

  https://github.com/tianocore/edk2/pull/3889

Laszlo

Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

[Dionna Glaze]

Thanks for your explanation and fastidiousness, Laszlo. Much appreciated.

On Thu, Jan 12, 2023 at 7:32 AM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 1/12/23 14:38, Ard Biesheuvel wrote:
> > On Thu, 12 Jan 2023 at 13:24, Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >> On 1/12/23 00:08, Dionna Glaze via groups.io wrote:
> >>> On Thu, Nov 10, 2022 at 8:55 AM Kinney, Michael D
> >>> <michael.d.kinney@intel.com> wrote:
> >>>>
> >>>> Hi Dionna,
> >>>>
> >>>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
> >>>>
> >>>> I think you are at step 13.  If you have not done so already,
> >>>> please update the commit messages with all the Reviewed-by and
> >>>> Acked-by tags and make sure your branch is rebased against the
> >>>> latest edk2/master and update the PR with that content and verify
> >>>> that all EDK II CI checks pass.
> >>>>
> >>>> At that point, it is the responsibility of the EDK II Maintainers
> >>>> to review the final state of the PR and set the "push" label if
> >>>> everything looks correct.
>
> I didn't realize this had been adopted -- "upgrading" a contributor
> submitted PR. (More below.)
>
> >>>> At this moment, we are in Soft Freeze and will be in Hard Freeze
> >>>> for the next 2 weeks.  If this is considered critical for the
> >>>> stable tag release, then please send a request to Liming with
> >>>> justification for stable tag.  Otherwise, it will be merged after
> >>>> the stable tag release.
> >>>>
> >>>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
> >>>>
> >>>> Thanks,
> >>>>
> >>>> Mike
> >>>>
> >>>
> >>> Hi Mike, my PR was closed without comment
> >>> https://github.com/tianocore/edk2/pull/3623 so I rebased and created
> >>> a new PR after the holidays and hard freeze. I hope this isn't
> >>> considered bad practice https://github.com/tianocore/edk2/pull/3885
> >>>
> >>
> >> I closed the PR -- similarly to how I manually closed 400+ stale PRs
> >> then -- because it was not a maintainer-submitted PR with the "push"
> >> label set. In other words, it was just a personal build, for
> >> triggering a CI run.
> >>
> >> And, in fact regardlessly of whether it was a "push"-labeled
> >> maintainer PR or just a personal PR, the PR was almost two months
> >> old. Clearly stale and abandoned -- per edk2 contribution workflow,
> >> anyway.
> >>
> >> Please excuse me for not explaining this in a comment on the PR, but
> >> (as I said above) I closed 400+ stale PRs manually within 10-15
> >> minutes on the github.com WebUI. The necessary muscle memory training
> >> didn't allow for individual comments.
> >>
> >> (I actually tried scripting the mass-closure at first, with the "gh"
> >> utility, but something in the github.com data model was broken, and
> >> the server wouldn't allow me to close those PRs with the utility,
> >> even though I had authenticated the utility under my account, with a
> >> complete set of scopes -- see my report at
> >> <https://github.com/cli/cli/discussions/6814>.)
> >>
> >> Regarding your new PR: it's again good for a personal CI run only. If
> >> it completes fine, please post the patches to the mailing list, using
> >> git-send-email.
> >>
> >> (From browsing recent list traffic, it seems that your colleague
> >> Moritz Fischer <moritzf@google.com> has posted patches like that to
> >> the list; please consider consulting with Moritz regarding the git
> >> setup (SMTP server etc) withing Google. Cc'ing Moritz now.)
> >>
> >> When sending the patches like that, please CC the maintainers and
> >> reviewers that are supposed to review them. Once they are happy with
> >> the series, one of them will submit a PR with them, and set the
> >> "push" label on the PR. When the CI run succeeds, the mergify bot
> >> will merge *that* PR automatically.
> >>
> >
> > I think we were already way past this with Dionna's work - numerous
> > iterations have been posted and discussed, and the merge of the final
> > version was only deferred because of the soft freeze.
> >
>
> That may very well be, but the specific PR I closed (3623) was not
> queued by a maintainer, nor did it have the "push" label. So I'd expect
> that now a maintainer has to submit a new PR, with the patches most
> recently approved on the list.
>
> The official documentation in the wiki at
> <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process#the-maintainer-process-for-the-edk-ii-project>
> still says the maintainer has to use "git-am" for picking up the
> patches, and to submit a new (separate) PR, with the "push" label set.
> So I wouldn't expect the "push" label to be set, as an additional
> maintainer action, on a contributor-submitted (pre-existent) PR.
>
> Sorry for obsessing about this, but given the 400+ stale open PRs, it
> was literally impossible to tell apart this one (3623) from the rest.
> And in retrospect, I do think I was right to close 3623 as well. IIRC, I
> left the PRs younger than 2 weeks alone.
>
> Now, regarding *where* the most recent version of the series lives (i.e.
> what needs to be picked up from the list by the maintainer, for
> merging), that beats me. We're conducting the present discussion under
> the following thread:
>
>   [edk2-devel] [PATCH 0/3] SEV-SNP accepted memory and BeforeExitBootServices
>   http://mid.mail-archive.com/20221108164616.3251967-1-dionnaglaze@google.com
>   https://edk2.groups.io/g/devel/message/96088
>   https://listman.redhat.com/archives/edk2-devel-archive/2022-November/055386.html
>
> so I'm tempted to think that this is the version that needs to be
> merged.
>
> However, if I check the November 2022 archive, I see v2 as well:
>
>   [edk2-devel] [PATCH v2 0/4] SEV-SNP accepted memory and BeforeExitBootServices
>   https://listman.redhat.com/archives/edk2-devel-archive/2022-November/055402.html
>   https://edk2.groups.io/g/devel/message/96104
>
> Then again, I find, in the v2 thread:
>
>   https://listman.redhat.com/archives/edk2-devel-archive/2022-December/056363.html
>   https://edk2.groups.io/g/devel/message/97065
>
> where Dionna writes, "we decided to go ahead and consider v1 of this
> series as final".
>
> So, in effect, this patch series had reached v8 previously (in
> *October*), as a part of a larger series
> (<https://listman.redhat.com/archives/edk2-devel-archive/2022-October/054823.html>),
> then got re-started (split-out) as a smaller series, then reached v2
> like that, then v2 got abandoned in favor of v1; all without a feature
> tracking BZ linking the *evolution* of the patch series across the
> various postings.
>
> And then we're surprised stuff falls through the cracks, and I remain
> the OCD person that alone finds it problematic that the PR tracker and
> the BZ tracker are in total shambles ¯\_(ツ)_/¯
>
> Anyway, Dionna pinging in this particular thread (= v1 of the split-out
> series) is consistent with the comment quoted from v2 qualifying v1 the
> final version. As a courtesy, while my temporarily renewed subscription
> to this list lasts a bit longer, I'm going to grab the v1 series in full
> from the November (gzipped mbox) archive at
> <https://listman.redhat.com/archives/edk2-devel-archive/>, and merge it.
>
> ... I've picked up the patches, picked up the pending feedback tags too,
> and compared the result against Dionna's latest PR
> <https://github.com/tianocore/edk2/pull/3885>.
>
> Beyond the Message-Id tags that git-am picked up on my side, I've found
> another difference: patch#1 was originally authored by Sophia Wolf
> <phiawolf@google.com>, but that fact has been mangled in the first patch
> of <https://github.com/tianocore/edk2/pull/3885> -- commit cd4c186f1099.
> Namely, in commit cd4c186f1099, the original authorship is recorded as
> another line in the commit message, when it should be reflected by the
> Author meta-datum in git. So I'm going to stick with what I have, from
> the list archive and git-am (git-am correctly translates the From: line
> back to the Author meta-datum), for the new PR:
>
>   https://github.com/tianocore/edk2/pull/3889
>
> Laszlo
>


-- 
-Dionna Glaze, PhD (she/her)

Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

[Laszlo Ersek]

On 1/12/23 16:32, Laszlo Ersek wrote:

> ... I've picked up the patches, picked up the pending feedback tags too,
> and compared the result against Dionna's latest PR
> <https://github.com/tianocore/edk2/pull/3885>.
> 
> Beyond the Message-Id tags that git-am picked up on my side, I've found
> another difference: patch#1 was originally authored by Sophia Wolf
> <phiawolf@google.com>, but that fact has been mangled in the first patch
> of <https://github.com/tianocore/edk2/pull/3885> -- commit cd4c186f1099.
> Namely, in commit cd4c186f1099, the original authorship is recorded as
> another line in the commit message, when it should be reflected by the
> Author meta-datum in git. So I'm going to stick with what I have, from
> the list archive and git-am (git-am correctly translates the From: line
> back to the Author meta-datum), for the new PR:
> 
>   https://github.com/tianocore/edk2/pull/3889

Commit range e5ec3ba409b5..9d70d8f20d0f.

Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

[Ard Biesheuvel]

On Thu, 12 Jan 2023 at 18:03, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 1/12/23 16:32, Laszlo Ersek wrote:
>
> > ... I've picked up the patches, picked up the pending feedback tags too,
> > and compared the result against Dionna's latest PR
> > <https://github.com/tianocore/edk2/pull/3885>.
> >
> > Beyond the Message-Id tags that git-am picked up on my side, I've found
> > another difference: patch#1 was originally authored by Sophia Wolf
> > <phiawolf@google.com>, but that fact has been mangled in the first patch
> > of <https://github.com/tianocore/edk2/pull/3885> -- commit cd4c186f1099.
> > Namely, in commit cd4c186f1099, the original authorship is recorded as
> > another line in the commit message, when it should be reflected by the
> > Author meta-datum in git. So I'm going to stick with what I have, from
> > the list archive and git-am (git-am correctly translates the From: line
> > back to the Author meta-datum), for the new PR:
> >
> >   https://github.com/tianocore/edk2/pull/3889
>
> Commit range e5ec3ba409b5..9d70d8f20d0f.
>

Thanks Laszlo