From c89a446cf09f30a121ae21d91f4a1aa071044084 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Tue, 20 Feb 2018 10:56:31 +0000 Subject: [efi] Run at TPL_CALLBACK to protect against UEFI timers As noted in the comments, UEFI manages to combines the all of the worst aspects of both a polling design (inefficiency and inability to sleep until something interesting happens) and of an interrupt-driven design (the complexity of code that could be preempted at any time, thanks to UEFI timers). This causes problems in particular for UEFI USB keyboards: the keyboard driver calls UsbAsyncInterruptTransfer() to set up a periodic timer which is used to poll the USB bus. This poll may interrupt a critical section within iPXE, typically resulting in list corruption and either a hang or reboot. Work around this problem by mirroring the BIOS design, in which we run with interrupts disabled almost all of the time. Signed-off-by: Michael Brown --- src/interface/efi/efi_snp.c | 12 +++++++++++ src/interface/efi/efi_timer.c | 41 ++++++++++++++++++++++++++++++------ src/interface/efi/efi_usb.c | 48 ++----------------------------------------- 3 files changed, 49 insertions(+), 52 deletions(-) diff --git a/src/interface/efi/efi_snp.c b/src/interface/efi/efi_snp.c index 263a25ac..88d999bf 100644 --- a/src/interface/efi/efi_snp.c +++ b/src/interface/efi/efi_snp.c @@ -45,6 +45,9 @@ static LIST_HEAD ( efi_snp_devices ); /** Network devices are currently claimed for use by iPXE */ static int efi_snp_claimed; +/** TPL prior to network devices being claimed */ +static EFI_TPL efi_snp_old_tpl; + /* Downgrade user experience if configured to do so * * The default UEFI user experience for network boot is somewhat @@ -1895,8 +1898,13 @@ struct efi_snp_device * last_opened_snpdev ( void ) { * @v delta Claim count change */ void efi_snp_add_claim ( int delta ) { + EFI_BOOT_SERVICES *bs = efi_systab->BootServices; struct efi_snp_device *snpdev; + /* Raise TPL if we are about to claim devices */ + if ( ! efi_snp_claimed ) + efi_snp_old_tpl = bs->RaiseTPL ( TPL_CALLBACK ); + /* Claim SNP devices */ efi_snp_claimed += delta; assert ( efi_snp_claimed >= 0 ); @@ -1904,4 +1912,8 @@ void efi_snp_add_claim ( int delta ) { /* Update SNP mode state for each interface */ list_for_each_entry ( snpdev, &efi_snp_devices, list ) efi_snp_set_state ( snpdev ); + + /* Restore TPL if we have released devices */ + if ( ! efi_snp_claimed ) + bs->RestoreTPL ( efi_snp_old_tpl ); } diff --git a/src/interface/efi/efi_timer.c b/src/interface/efi/efi_timer.c index 0ffe2a1a..8fe307ce 100644 --- a/src/interface/efi/efi_timer.c +++ b/src/interface/efi/efi_timer.c @@ -76,11 +76,36 @@ static void efi_udelay ( unsigned long usecs ) { * @ret ticks Current time, in ticks */ static unsigned long efi_currticks ( void ) { + EFI_BOOT_SERVICES *bs = efi_systab->BootServices; - /* EFI provides no clean way for device drivers to shut down - * in preparation for handover to a booted operating system. - * The platform firmware simply doesn't bother to call the - * drivers' Stop() methods. Instead, drivers must register an + /* UEFI manages to ingeniously combine the worst aspects of + * both polling and interrupt-driven designs. There is no way + * to support proper interrupt-driven operation, since there + * is no way to hook in an interrupt service routine. A + * mockery of interrupts is provided by UEFI timers, which + * trigger at a preset rate and can fire at any time. + * + * We therefore have all of the downsides of a polling design + * (inefficiency and inability to sleep until something + * interesting happens) combined with all of the downsides of + * an interrupt-driven design (the complexity of code that + * could be preempted at any time). + * + * The UEFI specification expects us to litter the entire + * codebase with calls to RaiseTPL() as needed for sections of + * code that are not reentrant. Since this doesn't actually + * gain us any substantive benefits (since even with such + * calls we would still be suffering from the limitations of a + * polling design), we instead choose to run at TPL_CALLBACK + * almost all of the time, dropping to TPL_APPLICATION to + * allow timer ticks to occur. + * + * + * For added excitement, UEFI provides no clean way for device + * drivers to shut down in preparation for handover to a + * booted operating system. The platform firmware simply + * doesn't bother to call the drivers' Stop() methods. + * Instead, all non-trivial drivers must register an * EVT_SIGNAL_EXIT_BOOT_SERVICES event to be signalled when * ExitBootServices() is called, and clean up without any * reference to the EFI driver model. @@ -97,10 +122,14 @@ static unsigned long efi_currticks ( void ) { * the API lazily assumes that the host system continues to * travel through time in the usual direction. Work around * EFI's violation of this assumption by falling back to a - * simple free-running monotonic counter. + * simple free-running monotonic counter during shutdown. */ - if ( efi_shutdown_in_progress ) + if ( efi_shutdown_in_progress ) { efi_jiffies++; + } else { + bs->RestoreTPL ( TPL_APPLICATION ); + bs->RaiseTPL ( TPL_CALLBACK ); + } return ( efi_jiffies * ( TICKS_PER_SEC / EFI_JIFFIES_PER_SEC ) ); } diff --git a/src/interface/efi/efi_usb.c b/src/interface/efi/efi_usb.c index db8c3d34..f9c91d09 100644 --- a/src/interface/efi/efi_usb.c +++ b/src/interface/efi/efi_usb.c @@ -64,50 +64,6 @@ static const char * efi_usb_direction_name ( EFI_USB_DATA_DIRECTION direction ){ ****************************************************************************** */ -/** - * Poll USB bus - * - * @v usbdev EFI USB device - */ -static void efi_usb_poll ( struct efi_usb_device *usbdev ) { - EFI_BOOT_SERVICES *bs = efi_systab->BootServices; - struct usb_bus *bus = usbdev->usb->port->hub->bus; - EFI_TPL tpl; - - /* UEFI manages to ingeniously combine the worst aspects of - * both polling and interrupt-driven designs. There is no way - * to support proper interrupt-driven operation, since there - * is no way to hook in an interrupt service routine. A - * mockery of interrupts is provided by UEFI timers, which - * trigger at a preset rate and can fire at any time. - * - * We therefore have all of the downsides of a polling design - * (inefficiency and inability to sleep until something - * interesting happens) combined with all of the downsides of - * an interrupt-driven design (the complexity of code that - * could be preempted at any time). - * - * The UEFI specification expects us to litter the entire - * codebase with calls to RaiseTPL() as needed for sections of - * code that are not reentrant. Since this doesn't actually - * gain us any substantive benefits (since even with such - * calls we would still be suffering from the limitations of a - * polling design), we instead choose to wrap only calls to - * usb_poll(). This should be sufficient for most practical - * purposes. - * - * A "proper" solution would involve rearchitecting the whole - * codebase to support interrupt-driven operation. - */ - tpl = bs->RaiseTPL ( TPL_NOTIFY ); - - /* Poll bus */ - usb_poll ( bus ); - - /* Restore task priority level */ - bs->RestoreTPL ( tpl ); -} - /** * Poll USB bus (from endpoint event timer) * @@ -216,7 +172,7 @@ static int efi_usb_open ( struct efi_usb_interface *usbintf, /* Create event */ if ( ( efirc = bs->CreateEvent ( ( EVT_TIMER | EVT_NOTIFY_SIGNAL ), - TPL_NOTIFY, efi_usb_timer, usbep, + TPL_CALLBACK, efi_usb_timer, usbep, &usbep->event ) ) != 0 ) { rc = -EEFI ( efirc ); DBGC ( usbdev, "USBDEV %s %s could not create event: %s\n", @@ -363,7 +319,7 @@ static int efi_usb_sync_transfer ( struct efi_usb_interface *usbintf, for ( i = 0 ; ( ( timeout == 0 ) || ( i < timeout ) ) ; i++ ) { /* Poll bus */ - efi_usb_poll ( usbdev ); + usb_poll ( usbdev->usb->port->hub->bus ); /* Check for completion */ if ( usbep->rc != -EINPROGRESS ) { -- cgit v1.2.3-55-g7522