Browse Source

[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>
tags/v1.20.1
Michael Brown 6 years ago
parent
commit
c89a446cf0
3 changed files with 49 additions and 52 deletions
  1. 12
    0
      src/interface/efi/efi_snp.c
  2. 35
    6
      src/interface/efi/efi_timer.c
  3. 2
    46
      src/interface/efi/efi_usb.c

+ 12
- 0
src/interface/efi/efi_snp.c View File

45
 /** Network devices are currently claimed for use by iPXE */
45
 /** Network devices are currently claimed for use by iPXE */
46
 static int efi_snp_claimed;
46
 static int efi_snp_claimed;
47
 
47
 
48
+/** TPL prior to network devices being claimed */
49
+static EFI_TPL efi_snp_old_tpl;
50
+
48
 /* Downgrade user experience if configured to do so
51
 /* Downgrade user experience if configured to do so
49
  *
52
  *
50
  * The default UEFI user experience for network boot is somewhat
53
  * The default UEFI user experience for network boot is somewhat
1895
  * @v delta		Claim count change
1898
  * @v delta		Claim count change
1896
  */
1899
  */
1897
 void efi_snp_add_claim ( int delta ) {
1900
 void efi_snp_add_claim ( int delta ) {
1901
+	EFI_BOOT_SERVICES *bs = efi_systab->BootServices;
1898
 	struct efi_snp_device *snpdev;
1902
 	struct efi_snp_device *snpdev;
1899
 
1903
 
1904
+	/* Raise TPL if we are about to claim devices */
1905
+	if ( ! efi_snp_claimed )
1906
+		efi_snp_old_tpl = bs->RaiseTPL ( TPL_CALLBACK );
1907
+
1900
 	/* Claim SNP devices */
1908
 	/* Claim SNP devices */
1901
 	efi_snp_claimed += delta;
1909
 	efi_snp_claimed += delta;
1902
 	assert ( efi_snp_claimed >= 0 );
1910
 	assert ( efi_snp_claimed >= 0 );
1904
 	/* Update SNP mode state for each interface */
1912
 	/* Update SNP mode state for each interface */
1905
 	list_for_each_entry ( snpdev, &efi_snp_devices, list )
1913
 	list_for_each_entry ( snpdev, &efi_snp_devices, list )
1906
 		efi_snp_set_state ( snpdev );
1914
 		efi_snp_set_state ( snpdev );
1915
+
1916
+	/* Restore TPL if we have released devices */
1917
+	if ( ! efi_snp_claimed )
1918
+		bs->RestoreTPL ( efi_snp_old_tpl );
1907
 }
1919
 }

+ 35
- 6
src/interface/efi/efi_timer.c View File

76
  * @ret ticks		Current time, in ticks
76
  * @ret ticks		Current time, in ticks
77
  */
77
  */
78
 static unsigned long efi_currticks ( void ) {
78
 static unsigned long efi_currticks ( void ) {
79
+	EFI_BOOT_SERVICES *bs = efi_systab->BootServices;
79
 
80
 
80
-	/* EFI provides no clean way for device drivers to shut down
81
-	 * in preparation for handover to a booted operating system.
82
-	 * The platform firmware simply doesn't bother to call the
83
-	 * drivers' Stop() methods.  Instead, drivers must register an
81
+	/* UEFI manages to ingeniously combine the worst aspects of
82
+	 * both polling and interrupt-driven designs.  There is no way
83
+	 * to support proper interrupt-driven operation, since there
84
+	 * is no way to hook in an interrupt service routine.  A
85
+	 * mockery of interrupts is provided by UEFI timers, which
86
+	 * trigger at a preset rate and can fire at any time.
87
+	 *
88
+	 * We therefore have all of the downsides of a polling design
89
+	 * (inefficiency and inability to sleep until something
90
+	 * interesting happens) combined with all of the downsides of
91
+	 * an interrupt-driven design (the complexity of code that
92
+	 * could be preempted at any time).
93
+	 *
94
+	 * The UEFI specification expects us to litter the entire
95
+	 * codebase with calls to RaiseTPL() as needed for sections of
96
+	 * code that are not reentrant.  Since this doesn't actually
97
+	 * gain us any substantive benefits (since even with such
98
+	 * calls we would still be suffering from the limitations of a
99
+	 * polling design), we instead choose to run at TPL_CALLBACK
100
+	 * almost all of the time, dropping to TPL_APPLICATION to
101
+	 * allow timer ticks to occur.
102
+	 *
103
+	 *
104
+	 * For added excitement, UEFI provides no clean way for device
105
+	 * drivers to shut down in preparation for handover to a
106
+	 * booted operating system.  The platform firmware simply
107
+	 * doesn't bother to call the drivers' Stop() methods.
108
+	 * Instead, all non-trivial drivers must register an
84
 	 * EVT_SIGNAL_EXIT_BOOT_SERVICES event to be signalled when
109
 	 * EVT_SIGNAL_EXIT_BOOT_SERVICES event to be signalled when
85
 	 * ExitBootServices() is called, and clean up without any
110
 	 * ExitBootServices() is called, and clean up without any
86
 	 * reference to the EFI driver model.
111
 	 * reference to the EFI driver model.
97
 	 * the API lazily assumes that the host system continues to
122
 	 * the API lazily assumes that the host system continues to
98
 	 * travel through time in the usual direction.  Work around
123
 	 * travel through time in the usual direction.  Work around
99
 	 * EFI's violation of this assumption by falling back to a
124
 	 * EFI's violation of this assumption by falling back to a
100
-	 * simple free-running monotonic counter.
125
+	 * simple free-running monotonic counter during shutdown.
101
 	 */
126
 	 */
102
-	if ( efi_shutdown_in_progress )
127
+	if ( efi_shutdown_in_progress ) {
103
 		efi_jiffies++;
128
 		efi_jiffies++;
129
+	} else {
130
+		bs->RestoreTPL ( TPL_APPLICATION );
131
+		bs->RaiseTPL ( TPL_CALLBACK );
132
+	}
104
 
133
 
105
 	return ( efi_jiffies * ( TICKS_PER_SEC / EFI_JIFFIES_PER_SEC ) );
134
 	return ( efi_jiffies * ( TICKS_PER_SEC / EFI_JIFFIES_PER_SEC ) );
106
 }
135
 }

+ 2
- 46
src/interface/efi/efi_usb.c View File

64
  ******************************************************************************
64
  ******************************************************************************
65
  */
65
  */
66
 
66
 
67
-/**
68
- * Poll USB bus
69
- *
70
- * @v usbdev		EFI USB device
71
- */
72
-static void efi_usb_poll ( struct efi_usb_device *usbdev ) {
73
-	EFI_BOOT_SERVICES *bs = efi_systab->BootServices;
74
-	struct usb_bus *bus = usbdev->usb->port->hub->bus;
75
-	EFI_TPL tpl;
76
-
77
-	/* UEFI manages to ingeniously combine the worst aspects of
78
-	 * both polling and interrupt-driven designs.  There is no way
79
-	 * to support proper interrupt-driven operation, since there
80
-	 * is no way to hook in an interrupt service routine.  A
81
-	 * mockery of interrupts is provided by UEFI timers, which
82
-	 * trigger at a preset rate and can fire at any time.
83
-	 *
84
-	 * We therefore have all of the downsides of a polling design
85
-	 * (inefficiency and inability to sleep until something
86
-	 * interesting happens) combined with all of the downsides of
87
-	 * an interrupt-driven design (the complexity of code that
88
-	 * could be preempted at any time).
89
-	 *
90
-	 * The UEFI specification expects us to litter the entire
91
-	 * codebase with calls to RaiseTPL() as needed for sections of
92
-	 * code that are not reentrant.  Since this doesn't actually
93
-	 * gain us any substantive benefits (since even with such
94
-	 * calls we would still be suffering from the limitations of a
95
-	 * polling design), we instead choose to wrap only calls to
96
-	 * usb_poll().  This should be sufficient for most practical
97
-	 * purposes.
98
-	 *
99
-	 * A "proper" solution would involve rearchitecting the whole
100
-	 * codebase to support interrupt-driven operation.
101
-	 */
102
-	tpl = bs->RaiseTPL ( TPL_NOTIFY );
103
-
104
-	/* Poll bus */
105
-	usb_poll ( bus );
106
-
107
-	/* Restore task priority level */
108
-	bs->RestoreTPL ( tpl );
109
-}
110
-
111
 /**
67
 /**
112
  * Poll USB bus (from endpoint event timer)
68
  * Poll USB bus (from endpoint event timer)
113
  *
69
  *
216
 
172
 
217
 	/* Create event */
173
 	/* Create event */
218
 	if ( ( efirc = bs->CreateEvent ( ( EVT_TIMER | EVT_NOTIFY_SIGNAL ),
174
 	if ( ( efirc = bs->CreateEvent ( ( EVT_TIMER | EVT_NOTIFY_SIGNAL ),
219
-					 TPL_NOTIFY, efi_usb_timer, usbep,
175
+					 TPL_CALLBACK, efi_usb_timer, usbep,
220
 					 &usbep->event ) ) != 0 ) {
176
 					 &usbep->event ) ) != 0 ) {
221
 		rc = -EEFI ( efirc );
177
 		rc = -EEFI ( efirc );
222
 		DBGC ( usbdev, "USBDEV %s %s could not create event: %s\n",
178
 		DBGC ( usbdev, "USBDEV %s %s could not create event: %s\n",
363
 	for ( i = 0 ; ( ( timeout == 0 ) || ( i < timeout ) ) ; i++ ) {
319
 	for ( i = 0 ; ( ( timeout == 0 ) || ( i < timeout ) ) ; i++ ) {
364
 
320
 
365
 		/* Poll bus */
321
 		/* Poll bus */
366
-		efi_usb_poll ( usbdev );
322
+		usb_poll ( usbdev->usb->port->hub->bus );
367
 
323
 
368
 		/* Check for completion */
324
 		/* Check for completion */
369
 		if ( usbep->rc != -EINPROGRESS ) {
325
 		if ( usbep->rc != -EINPROGRESS ) {

Loading…
Cancel
Save