[ipxe-devel] [PATCH 1/1] [efi] Run at TPL_CALLBACK to protect against UEFI timers
Heinrich Schuchardt
xypron.glpk at gmx.de
Sat Mar 24 16:43:48 UTC 2018
On 03/24/2018 05:24 PM, Heinrich Schuchardt wrote:
> On 03/24/2018 05:16 PM, Heinrich Schuchardt wrote:
>> 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 at ipxe.org>
>
> Unfortunately with this patch you broke booting from iSCSI with U-Boot.
>
> Up to now U-Boot was using a timer event running at level TPL_CALLBACK
> to poll the network card. Of cause this timer can be changed to run at
> TPL_NOTIFY.
>
> But, please, do not raise the TPL level for the iPXE application any
> further.
>
> Best regards
>
> Heinrich Schuchardt
The patch clearly contradicts the UEFI spec:
"Good coding practice dictates that all code should execute at its
lowest possible TPL level, and the use of TPL levels above
TPL_APPLICATION must be minimized. Executing at TPL levels above
TPL_APPLICATION for extended periods of time may also result in
unpredictable behavior."
I suggest that iPXE sticks to the UEFI spec.
Best regards
Heinrich
>
>> ---
>> src/interface/efi/efi_snp.c | 12 +++++++++++
>> src/interface/efi/efi_timer.c | 41 ++++++++++++++++++++++++++++++------
>> src/interface/efi/efi_usb.c | 48 ++-----------------------------------------
>> 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
>> @@ -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 ) {
>>
>
>
More information about the ipxe-devel
mailing list