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

[Laszlo Ersek <lersek@redhat.com>]

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.

Thanks
Laszlo