Skip to content

Commit

Permalink
[efi] Run at TPL_CALLBACK to protect against UEFI timers
Browse files Browse the repository at this point in the history 
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 <mcb30@ipxe.org>
  • Loading branch information
@mcb30
mcb30 committed Feb 20, 2018
1 parent 8dbb73a commit c89a446
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 52 deletions.
12 changes: 12 additions & 0 deletions src/interface/efi/efi_snp.c
View file
Expand Up @@ -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
Expand Down Expand Up @@ -1895,13 +1898,22 @@ 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 );

/* 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 );
}
41 changes: 35 additions & 6 deletions src/interface/efi/efi_timer.c
View file
Expand Up @@ -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.
Expand All @@ -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 ) );
}
Expand Down
48 changes: 2 additions & 46 deletions src/interface/efi/efi_usb.c
View file
Expand Up @@ -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)
*
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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 ) {
Expand Down

0 comments on commit c89a446

Please sign in to comment.