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

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

(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>

Given that we are now depending on events signalled in an event
handler to be queued after all currently pending events, 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.