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 <mc...@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 ) { >> > > _______________________________________________ ipxe-devel mailing list ipxe-devel@lists.ipxe.org https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel