Re: [PATCH v4 5/6] MdeModulePkg: add EnableUnacceptedMemoryProtocol

["Ard Biesheuvel" <ardb@kernel.org>]

(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
>