| Message ID | 20230627074132.1016795-1-ardb@kernel.org |
|---|---|
| State | Accepted |
| Commit | 2e28a798c3092ea42b968fa16ac835969d124898 |
| Headers | show |
| Series | efi/libstub: Disable PCI DMA before grabbing the EFI memory map | expand |
On Tue, Jun 27, 2023 at 09:41:32AM +0200, Ard Biesheuvel wrote: > However, the stub will invoke DisconnectController() on all endpoints > downstream of the PCI bridges it disables, and this may affect the > layout of the EFI memory map, making it likely that ExitBootServices() > will fail the first time around, and that the EFI memory map needs to be > reloaded. Isn't it always likely that ExitBootServices() will fail the first time around, but disable_early_pci_dma makes it more likely it'll have changed by enough that we need a bigger map? Other than that potential quibble over the changelog, Acked-by: Matthew Garrett <mjg59@srcf.ucam.org>
On Tue, 27 Jun 2023 at 10:00, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > On Tue, Jun 27, 2023 at 09:41:32AM +0200, Ard Biesheuvel wrote: > > > However, the stub will invoke DisconnectController() on all endpoints > > downstream of the PCI bridges it disables, and this may affect the > > layout of the EFI memory map, making it likely that ExitBootServices() > > will fail the first time around, and that the EFI memory map needs to be > > reloaded. > > Isn't it always likely that ExitBootServices() will fail the first time > around, but disable_early_pci_dma makes it more likely it'll have > changed by enough that we need a bigger map? Not quite. It should only fail the first time if the memory map changed since the last call to GetMemoryMap(), and normally, this will only happen if some kind of asynchronous event was triggered after GetMemoryMap() but before ExitBootServices(). (This is why calling ExitBootServices() at most twice should always suffice: the first call disables the timer interrupt, so the second time around, no events will fire in the mean time) In this case, we explicitly invoke boot services between GetMemoryMap() and ExitBootServices(), making the first failure substantially more likely. > Other than that potential > quibble over the changelog, > > Acked-by: Matthew Garrett <mjg59@srcf.ucam.org> Thanks
On Tue, Jun 27, 2023 at 10:14:16AM +0200, Ard Biesheuvel wrote: > Not quite. It should only fail the first time if the memory map > changed since the last call to GetMemoryMap(), and normally, this will > only happen if some kind of asynchronous event was triggered after > GetMemoryMap() but before ExitBootServices(). (This is why calling > ExitBootServices() at most twice should always suffice: the first call > disables the timer interrupt, so the second time around, no events > will fire in the mean time) Can't driver shutdown code also end up altering it? I've certainly had extremely deterministic requirements to call it twice, which doesn't line up terribly well with it just being down to async events.
On Tue, 27 Jun 2023 at 10:17, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > On Tue, Jun 27, 2023 at 10:14:16AM +0200, Ard Biesheuvel wrote: > > > Not quite. It should only fail the first time if the memory map > > changed since the last call to GetMemoryMap(), and normally, this will > > only happen if some kind of asynchronous event was triggered after > > GetMemoryMap() but before ExitBootServices(). (This is why calling > > ExitBootServices() at most twice should always suffice: the first call > > disables the timer interrupt, so the second time around, no events > > will fire in the mean time) > > Can't driver shutdown code also end up altering it? Yes, but doing so violates the UEFI spec: EVT_SIGNAL_EXIT_BOOT_SERVICES is documented as not permitting the use of memory allocation services, either directly or indirectly (via the use of other external code that might use them) > I've certainly had > extremely deterministic requirements to call it twice, which doesn't > line up terribly well with it just being down to async events. Yeah, I guess you can still get the Windows sticker if you violate this requirement :-)
On Tue, Jun 27, 2023 at 10:32:36AM +0200, Ard Biesheuvel wrote: > On Tue, 27 Jun 2023 at 10:17, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > > > On Tue, Jun 27, 2023 at 10:14:16AM +0200, Ard Biesheuvel wrote: > > > > > Not quite. It should only fail the first time if the memory map > > > changed since the last call to GetMemoryMap(), and normally, this will > > > only happen if some kind of asynchronous event was triggered after > > > GetMemoryMap() but before ExitBootServices(). (This is why calling > > > ExitBootServices() at most twice should always suffice: the first call > > > disables the timer interrupt, so the second time around, no events > > > will fire in the mean time) > > > > Can't driver shutdown code also end up altering it? > > Yes, but doing so violates the UEFI spec: > EVT_SIGNAL_EXIT_BOOT_SERVICES is documented as not permitting the use > of memory allocation services, either directly or indirectly (via the > use of other external code that might use them) Maybe people have become better at observing that restriction! Anyway, feel free to ignore my nit in that case.
On Tue, 27 Jun 2023 at 10:37, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > On Tue, Jun 27, 2023 at 10:32:36AM +0200, Ard Biesheuvel wrote: > > On Tue, 27 Jun 2023 at 10:17, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > > > > > On Tue, Jun 27, 2023 at 10:14:16AM +0200, Ard Biesheuvel wrote: > > > > > > > Not quite. It should only fail the first time if the memory map > > > > changed since the last call to GetMemoryMap(), and normally, this will > > > > only happen if some kind of asynchronous event was triggered after > > > > GetMemoryMap() but before ExitBootServices(). (This is why calling > > > > ExitBootServices() at most twice should always suffice: the first call > > > > disables the timer interrupt, so the second time around, no events > > > > will fire in the mean time) > > > > > > Can't driver shutdown code also end up altering it? > > > > Yes, but doing so violates the UEFI spec: > > EVT_SIGNAL_EXIT_BOOT_SERVICES is documented as not permitting the use > > of memory allocation services, either directly or indirectly (via the > > use of other external code that might use them) > > Maybe people have become better at observing that restriction! Anyway, > feel free to ignore my nit in that case. I haven't dealt with actual production x86 hardware built to boot Windows via EFI (or CSM) as much as you have, so my world view tends to be a bit naive when it comes to actual EFI compliance :-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index 51779279fbff21b5..bfa30625f5d03167 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -380,6 +380,9 @@ efi_status_t efi_exit_boot_services(void *handle, void *priv, struct efi_boot_memmap *map; efi_status_t status; + if (efi_disable_pci_dma) + efi_pci_disable_bridge_busmaster(); + status = efi_get_memory_map(&map, true); if (status != EFI_SUCCESS) return status; @@ -390,9 +393,6 @@ efi_status_t efi_exit_boot_services(void *handle, void *priv, return status; } - if (efi_disable_pci_dma) - efi_pci_disable_bridge_busmaster(); - status = efi_bs_call(exit_boot_services, handle, map->map_key); if (status == EFI_INVALID_PARAMETER) {
Currently, the EFI stub will disable PCI DMA as the very last thing it does before calling ExitBootServices(), to avoid interfering with the firmware's normal operation as much as possible. However, the stub will invoke DisconnectController() on all endpoints downstream of the PCI bridges it disables, and this may affect the layout of the EFI memory map, making it likely that ExitBootServices() will fail the first time around, and that the EFI memory map needs to be reloaded. This, in turn, increases the likelihood that the slack space we allocated is insufficient (and we can no longer allocate memory via boot services after having called ExitBootServices() once), causing the second call to GetMemoryMap (and therefore the boot) to fail. This makes the PCI DMA disable feature a bit more fragile than it already is, so let's make it more robust, by allocating the space for the EFI memory map after disabling PCI DMA. Cc: Matthew Garrett <mjg59@srcf.ucam.org> Cc: Daniel Kiper <dkiper@net-space.pl> Reported-by: Glenn Washburn <development@efficientek.com> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- drivers/firmware/efi/libstub/efi-stub-helper.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)