On 8 September 2017 at 10:48, Laszlo Ersek <[email protected]> wrote: > On 09/08/17 10:53, Ard Biesheuvel wrote: >> (cc'ing the trinity) >> >> On 7 September 2017 at 23:41, Laszlo Ersek <[email protected]> wrote: >>> Repo: https://github.com/lersek/edk2.git >>> Branch: iommu_exit_boot >>> >>> This series is the result of the discussion under >>> >>> [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common >>> buffers at ExitBootServices() >>> https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html >>> >>> At ExitBootServices(), PCI and VirtIo drivers should only care about >>> aborting pending DMA on the devices. Cleaning up PciIo mappings (which >>> ultimately boil down to IOMMU mappings) for those aborted DMA operations >>> should be the job of the IOMMU driver. >>> >>> Patches 01 through 03 clean up the AtaAtapiPassThru driver in >>> MdeModulePkg a little bit, because at present, (a) it unmaps the buffers >>> and disables BMDMA in the wrong order in its DriverBindingStop() >>> function, (b) it doesn't abort pending DMA at ExitBootServices(). >>> >>> This subset can be treated separately from the rest of the series, but I >>> thought they belonged loosely together (given that AtaAtapiPassThru is >>> used on QEMU's Q35 machine type). >>> >>> Patches 04 through 07 remove VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer() >>> calls from the VirtIo drivers' ExitBootServices() handlers. >>> >>> (The conversion of VirtioNetDxe to device addresses is still in progress >>> -- Brijesh, when you submit v2 of that, under this approach, there is no >>> need to change VirtioNetExitBoot() relative to current upstream, and you >>> can use VirtioOperationBusMasterRead to map outgoing packets.) >>> >>> Patches 08 through 10 make OvmfPkg/IoMmuDxe track all mappings, and >>> unmap all mappings (Read, Write, CommonBuffer) that are in effect when >>> ExitBootServices() is called. It is ensured that PCI and VirtIo drivers >>> abort pending DMA first, and IoMmuDxe clean up the mappings last. >>> >> >> The patches look fine to me >> >> Reviewed-by: Ard Biesheuvel <[email protected]> > > Thank you! > >> Given that we are now depending on events signalled in an event >> handler to be queued after all currently pending events, > > marking this (*) > >> I think we >> need to explicitly agree that this behavior that needs to be >> preserved, and documented somewhere, given that the UEFI spec does not >> offer this guarantee. > > The condition that you describe under (*) *is* guaranteed in the UEFI spec. > > The *only* bit that is edk2 specific in the last patch is that we invoke > gBS->SignalEvent() from the notification function of an event that > *specifically* has type EVT_SIGNAL_EXIT_BOOT_SERVICES. > > If the event, whose notification function we were calling > gBS->SignalEvent() from, was a plain EVT_NOTIFY_SIGNAL type event, then > *nothing* in the last patch would be edk2-specific. > > * It is guaranteed by the UEFI spec that signaling an event group queues > the notification functions for all not-yet-signaled events in that > group, before the first notification function is invoked (regardless of > the signaled events' TPLs). From "CreateEventEx()": "All events are > guaranteed to be signaled before the first notification action is taken." > > * The UEFI spec guarantees that, within the same TPL, if an event is > signaled that is not pending yet, the notify function will be queued > after notify functions already queued on the same TPL. See "CreateEvent()": > > Events exist in one of two states, “waiting” or “signaled.” When an > event is created, firmware puts it in the “waiting” state. When the > event is signaled, firmware changes its state to “signaled” and, if > EVT_NOTIFY_SIGNAL is specified, places a call to its notification > function in a FIFO queue. There is a queue for each of the “basic” > task priority levels defined in Section 7.1 (TPL_CALLBACK, and > TPL_NOTIFY). The functions in these queues are invoked in FIFO > order, starting with the highest priority level queue and proceeding > to the lowest priority queue that is unmasked by the current TPL. If > the current TPL is equal to or greater than the queued notification, > it will wait until the TPL is lowered via > EFI_BOOT_SERVICES.RestoreTPL(). > > * gBS->SignalEvent() is valid to call at TPL_CALLBACK and TPL_NOTIFY > levels (see "Table 23. TPL Restrictions"); in fact it is one of the few > services that can even be called at TPL_HIGH_LEVEL (which is reserved > for the platform firmware). > > The upshot is: > > (1) assume you have Event1, Event2, Event3, Event4 in event group > EventGroupFoobarGuid > > (2) assume all events are EVT_NOTIFY_SIGNAL type > > (3) assume Event1 and Event2 have TPL_NOTIFY, Event3 and Event4 are > TPL_CALLBACK > > (4) Assume Event5 is also of type EVT_NOTIFY_SIGNAL, not part of any > event group, and has TPL_CALLBACK task prio level > > (5) Assume an agent running at TPL_APPLICATION creates an event > temporarily, with type 0 (see the code example in CreateEventEx), and > signals EventGroupFoobarGuid through it. > > (6) All of Event1, Event2, Event3 and Event4 move into the signaled > state at once. NotifyFunction1 and NotifyFunction2 are queued in > unspecified order in the TPL_NOTIFY queue, and Event3 and Event4 are > queued in unspecified order in the TPL_CALLBACK queue. > > (7) Because our current TPL is TPL_APPLICATION, and signaled events of > type EVT_NOTIFY_SIGNAL with higher TPLs exist, the SignalEvent() service > will start dispatching them immediately. > > (8) Either NotifyFunction1 or NotifyFunction2 is called first, running > at TPL_NOTIFY. > > (9) Assume that NotifyFunction2 signals Event5. (it doesn't matter if > NotifyFunction2 is called before or after NotifyFunction1). > > NotifyFunction5 is not dispatched immediately (on the stack of > SignalEvent()) because the current TPL, TPL_NOTIFY, is higher than or > equal to, the TPL of Event5, which is TPL_CALLBACK. So NotifyFunction5 > is queued instead. > > Because NotifyFunction3 and NotifyFunction4 are already in the > TPL_CALLBACK queue (in unspecified relative order), from step (6), > NotifyFunction5 will be queued after both of them, in FIFO order. > > (10) If NotifyFunction1 has not been invoked yet, it is invoked now. It > runs at TPL_NOTIFY. The TPL_NOTIFY queue becomes empty. > > (11) NotifyFunction3 and NotifyFunction4 are invoked in unspecified > order. They both run at TPL_CALLBACK. > > (12) NotifyFunction5 is invoked. It runs at TPL_CALLBACK. The > TPL_CALLBACK queue becomes empty. > > (13) SignalEvent() returns to the agent mentioned in (5). > > > The one thing to remember, from the client programmer's POV, is that > *any* event of type EVT_NOTIFY_SIGNAL can only be dispatched from within > two service calls: > > - SignalEvent(), if the caller is running at a TPL strictly below the > TPL of the event being signaled (directly or via event group GUID), and > > - RestoreTPL(), if the restored TPL falls strictly below the TPL of the > event, and the event has already been signaled. > > IOW, SignalEvent() "calls or queues", and RestoreTPL "de-queues and > calls, or does nothing". > > > So, again, the only question that the UEFI spec does not clarify is > whether we can call gBS->SignalEvent() specifically from an EBS > notification function. > > The intent is likely just that, because: > > EVT_SIGNAL_EXIT_BOOT_SERVICES > [...] This event is of type EVT_NOTIFY_SIGNAL [...] > > and, again, if we call SignalEvent from the NotifyFunction of a plain > EVT_NOTIFY_SIGNAL event, then it's all covered by the spec already. > > My apologies about the wall of text. I probably over-explained it five-fold. >
OK _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
