Re: [edk2-devel] [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed

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

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.