MdeModulePkg/DxeCore: Call BeforeExitBootServices event group only once

[ardbiesheuvel]

According to UEFI spec 2.10 errata A section 7.4.6

"All events from the EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES and
EFI_EVENT_GROUP_EXIT_BOOT_SERVICES event notification groups as well
as events of type EVT_SIGNAL_EXIT_BOOT_SERVICES must be signaled
before ExitBootServices() returns EFI_SUCCESS. The events are only
signaled once even if ExitBootServices() is called multiple times."

So keep track of whether ExitBootServices() has been called, and signal
the event group EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES only the first
time around.

EFI_EVENT_GROUP_EXIT_BOOT_SERVICES will only be signalled if
ExitBootServices() is going to run to [successful] completion, after
which calling it a second time is not possible anyway. So for this case,
no special handling is needed.

[ardbiesheuvel]

cc @deeglaze

[deeglaze]

LGTM, thanks Ard. Happy thanksgiving.

[os-d]

A related topic I have been discussing and impemented in other environments is disallowing alloc/free during BeforeEBS and EBS handlers. For anything non-runtime memory, that memory is about to be forfeit anyway to the OS, for runtime types it is suspicious that it is being allocated or freed during EBS. Any alloc/free will change the memory map of course and fail EBS, but depending on what happens, it can be a persistent failure. We have a recent case where a platform had frees during an EBS handler and Linux couldn’t boot afterwards because grub only allocates 8 extra memory map entries for possible changes across EBS (Windows allocates double the memory map entry count). The memory map changed by more than 8 entries and thus Linux could never boot. So we’ve taken the approach that allocs should be explicitly rejected during EBS and frees should silently fail (i.e. just return success) unless they are a runtime type, then an assert and explicit failure seems worthwhile to amend those drivers. Anyway, this is just a thought related to this PR, I don’t think this PR needs to change as a result, but it could come later as a stronger statement towards not changing the memory map during EBS, because even with this we can still fail to boot an OS based on memory map changes during EBS.

[deeglaze]

A related topic I have been discussing and impemented in other environments is disallowing alloc/free during BeforeEBS and EBS handlers.

This suggestion would break the use case that led me to add this event group signaling support for in the first place: accepting unaccepted memory if the OS doesn’t use a certain protocol to indicate that it supports the new kind of memory map entry.

[os-d]

A related topic I have been discussing and impemented in other environments is disallowing alloc/free during BeforeEBS and EBS handlers.

This suggestion would break the use case that led me to add this event group signaling support for in the first place: accepting unaccepted memory if the OS doesn’t use a certain protocol to indicate that it supports the new kind of memory map entry.

Is there a public example of that? In this case, os the unaccepted memory allocated or just changed type?

[ardbiesheuvel]

A related topic I have been discussing and impemented in other environments is disallowing alloc/free during BeforeEBS and EBS handlers.

This suggestion would break the use case that led me to add this event group signaling support for in the first place: accepting unaccepted memory if the OS doesn’t use a certain protocol to indicate that it supports the new kind of memory map entry.

Indeed. Alloc/free are already disallowed during EFI_EVENT_GROUP_EXIT_BOOT_SERVICES, and I guess we could police that by failing any calls to the memory services when gMemoryMapTerminated is TRUE. But the point of EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES is that it provides the last opportunity to use all firmware services, including memory services, and with this patch applied, there should be no issue using those, as long as the calling code has enough spare entries to accommodate the updates without having to reallocate its memory map storage.

[deeglaze]

https://github.com/tianocore/edk2/blob/master/OvmfPkg/AmdSevDxe/AmdSevDxe.c

I don’t think it’s possible to simply change type without removing and adding, which can allocate memory IIRC

[os-d]

A related topic I have been discussing and impemented in other environments is disallowing alloc/free during BeforeEBS and EBS handlers.

This suggestion would break the use case that led me to add this event group signaling support for in the first place: accepting unaccepted memory if the OS doesn’t use a certain protocol to indicate that it supports the new kind of memory map entry.

Indeed. Alloc/free are already disallowed during EFI_EVENT_GROUP_EXIT_BOOT_SERVICES, and I guess we could police that by failing any calls to the memory services when gMemoryMapTerminated is TRUE. But the point of EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES is that it provides the last opportunity to use all firmware services, including memory services, and with this patch applied, there should be no issue using those, as long as the calling code has enough spare entries to accommodate the updates without having to reallocate its memory map storage.

Well, that last part being the problem :). Grub only allocating 8 extra entries can be tight. Grub should probably be updated to allocate more, but then you also run into the problem of distros carrying 6+ years old shim/grub and not updating.

I do agree this patchmakes the prospect a lot better. I think this reduces the number of failures for Windows to almost none.

[ardbiesheuvel]

A related topic I have been discussing and impemented in other environments is disallowing alloc/free during BeforeEBS and EBS handlers.

This suggestion would break the use case that led me to add this event group signaling support for in the first place: accepting unaccepted memory if the OS doesn’t use a certain protocol to indicate that it supports the new kind of memory map entry.

Indeed. Alloc/free are already disallowed during EFI_EVENT_GROUP_EXIT_BOOT_SERVICES, and I guess we could police that by failing any calls to the memory services when gMemoryMapTerminated is TRUE. But the point of EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES is that it provides the last opportunity to use all firmware services, including memory services, and with this patch applied, there should be no issue using those, as long as the calling code has enough spare entries to accommodate the updates without having to reallocate its memory map storage.

Well, that last part being the problem :). Grub only allocating 8 extra entries can be tight. Grub should probably be updated to allocate more, but then you also run into the problem of distros carrying 6+ years old shim/grub and not updating.

GRUB has no business calling ExitBootServices() on a modern system - that should be left up to the EFI stub in Linux (or whatever OS and loader is in use). If Linux is problematic in the same way, we should fix it.

I do agree this patch makes the prospect a lot better. I think this reduces the number of failures for Windows to almost none.

To be honest, I wasn't aware that EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES is that widely used. But I ended up looking into this because Lenovo Snapdragon laptops (which are built to boot Windows but being repurposed for Linux) are experiencing persistent failures of ExitBootServices(), and it seems like these callbacks may be the culprit (although I fail to understand why only Linux would be affected by this)

[os-d]

A related topic I have been discussing and impemented in other environments is disallowing alloc/free during BeforeEBS and EBS handlers.

This suggestion would break the use case that led me to add this event group signaling support for in the first place: accepting unaccepted memory if the OS doesn’t use a certain protocol to indicate that it supports the new kind of memory map entry.

Indeed. Alloc/free are already disallowed during EFI_EVENT_GROUP_EXIT_BOOT_SERVICES, and I guess we could police that by failing any calls to the memory services when gMemoryMapTerminated is TRUE. But the point of EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES is that it provides the last opportunity to use all firmware services, including memory services, and with this patch applied, there should be no issue using those, as long as the calling code has enough spare entries to accommodate the updates without having to reallocate its memory map storage.

Well, that last part being the problem :). Grub only allocating 8 extra entries can be tight. Grub should probably be updated to allocate more, but then you also run into the problem of distros carrying 6+ years old shim/grub and not updating.

GRUB has no business calling ExitBootServices() on a modern system - that should be left up to the EFI stub in Linux (or whatever OS and loader is in use). If Linux is problematic in the same way, we should fix it.

I do agree this patch makes the prospect a lot better. I think this reduces the number of failures for Windows to almost none.

To be honest, I wasn't aware that EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES is that widely used. But I ended up looking into this because Lenovo Snapdragon laptops (which are built to boot Windows but being repurposed for Linux) are experiencing persistent failures of ExitBootServices(), and it seems like these callbacks may be the culprit (although I fail to understand why only Linux would be affected by this)

This may be my misremembering the bug report, so it may be the Linux EFI stub and not grub that only allocates 8 extra entries. This is for a modern Linux distro. That may factor into what you are seeing. I cannot update Linux or look at the source code, due to some restrictions for other projects I am working on. Like I said, we are seeing persistent failures for ExitBootServices for Linux and not Windows, because of Linux only allocating 8 extra entries and failing if the memory map changes more than that. I would encourage you to follow up on the Linux side and confirm what our Linux team reported to me. Definitely it is also an issue that drivers don’t close their BeforeEBS callbacks and so get called more than once, your current patch obviously fixes that.

[ardbiesheuvel]

This may be my misremembering the bug report, so it may be the Linux EFI stub and not grub that only allocates 8 extra entries. This is for a modern Linux distro. That may factor into what you are seeing. I cannot update Linux or look at the source code, due to some restrictions for other projects I am working on. Like I said, we are seeing persistent failures for ExitBootServices for Linux and not Windows, because of Linux only allocating 8 extra entries and failing if the memory map changes more than that. I would encourage you to follow up on the Linux side and confirm what our Linux team reported to me. Definitely it is also an issue that drivers don’t close their BeforeEBS callbacks and so get called more than once, your current patch obviously fixes that.

Indeed - it's the Linux EFI stub that allocates spare entries. But re-reading the spec, it seems that it is permitted to free the memory map and re-allocate it using boot services, rather than bend over backwards to avoid updating the memory map.

[ardbiesheuvel]

@lgao4 any thoughts?

[ardbiesheuvel]

@lgao4 @mdkinney if there are no objections to this PR, I intend to merge it tomorrow.

[mdkinney]

Is there any info from UEFI Spec on the required behavior here?

[ardbiesheuvel]

Is there any info from UEFI Spec on the required behavior here?

Yes. As Oliver pointed out,

UEFI spec 2.10 errata A section 7.4.6
“All events from the EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES and EFI_EVENT_GROUP_EXIT_BOOT_SERVICES event notification groups as well as events of type EVT_SIGNAL_EXIT_BOOT_SERVICES must be signaled before ExitBootServices() returns EFI_SUCCESS. The events are only signaled once even if ExitBootServices() is called multiple times.“

[mdkinney]

I did not see that specific spec reference in the PR or the commit message. That should be the main reason for this change.

I recommend updating the PR description and commit message with that specific spec reference and that the reason for the change is to follow the spec.

[ardbiesheuvel]

I did not see that specific spec reference in the PR or the commit message. That should be the main reason for this change.

I recommend updating the PR description and commit message with that specific spec reference and that the reason for the change is to follow the spec.

Done