[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:24:45 UTC 2018
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
> ---
> 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