summaryrefslogtreecommitdiffstats
path: root/src/interface/efi
diff options
context:
space:
mode:
authorMichael Brown2018-02-20 11:56:31 +0100
committerMichael Brown2018-02-20 11:56:31 +0100
commitc89a446cf09f30a121ae21d91f4a1aa071044084 (patch)
treed9b8a253fc68ac4e97221ad5dfae8707c5bab873 /src/interface/efi
parent[xhci] Consume event TRB before reporting completion to USB core (diff)
downloadipxe-c89a446cf09f30a121ae21d91f4a1aa071044084.tar.gz
ipxe-c89a446cf09f30a121ae21d91f4a1aa071044084.tar.xz
ipxe-c89a446cf09f30a121ae21d91f4a1aa071044084.zip
[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>
Diffstat (limited to 'src/interface/efi')
-rw-r--r--src/interface/efi/efi_snp.c12
-rw-r--r--src/interface/efi/efi_timer.c41
-rw-r--r--src/interface/efi/efi_usb.c48
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 ) {