[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 <mcb30@ipxe.org>

diff --git a/src/interface/efi/efi_snp.c b/src/interface/efi/efi_snp.c
index 263a25ac696c248fe86199b62e95b2065aa00cc7..88d999bf0d445f2852e01896dcfca9344acc35c2 100644 (file)
--- 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 0ffe2a1a005bfdecb9a18166e36c92f6de1cdbb8..8fe307ceb1fdd8f2cab279257a4caff039dd8812 100644 (file)
--- 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 db8c3d348ca5101d71ec790751668e702b3ac26e..f9c91d09ba431820897d0c4cc331abe9e652d55a 100644 (file)
--- 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 ) {