Re: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 8 September 2017 at 10:48, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/08/17 10:53, Ard Biesheuvel wrote:
>> (cc'ing the trinity)
>>
>> On 7 September 2017 at 23:41, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org>
>
> 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