[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>

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
@@ -65,50 +65,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)
  *
  * @v event		EFI event
@@ -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 ) {