Re: [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common buffers at ExitBootServices()

[Laszlo Ersek <lersek@redhat.com>]

On 09/04/17 12:36, Zeng, Star wrote:
> Curious about one question when I am reviewing this patch series.
> 
> Does/Should the IOMMU component disable the address translation at ExitBootServices?

It is a very good and interesting question.

My answer is, I don't know. The IOMMU protocol abstraction is specific
to edk2, and its behavior is not documented in the PI or UEFI specs (as
far as I know).

For one example, the VT-d IOMMU implementation in

  IntelSiliconPkg/IntelVTdDxe

zaps all translations at ExitBootServices():

VOID
EFIAPI
OnExitBootServices (
  IN EFI_EVENT                               Event,
  IN VOID                                    *Context
  )
{
  DEBUG ((DEBUG_INFO, "Vtd OnExitBootServices\n"));
  DumpVtdRegsAll ();
  DisableDmar ();
  DumpVtdRegsAll ();
}

Now, in case someone suggested that the same approach should be followed
by all IOMMU implementations, I'd have three counter-arguments:

(1) My series states the problem (and seeks to solve it) on the PciIo
protocol level, which is standardized. The IOMMU protocol is edk2-only.
I feel that this kind of "unmap on ExitBootServices" should be done by
all UEFI drivers that map DMA common buffers, regardless of
platform-specific PciIo implementations.

(2) ExitBootServices() notify functions are called in unspecified order
(there's no way to express dependencies between them). This means that,
following the above pattern, an IO address translation could be torn
down before a reliant PCI device is de-configured in its own UEFI_DRIVER
ExitBootServices() handler. The result can be a series of IOMMU faults.
I'm not sure if that's bad in practice, but it does look like a layering
violation. (We shouldn't tear down resources from the bottom up.)

(3) The DisableDmar() function is relatively simple. It uses VT-d
registers that exist independently of any PCI devices, and of any "live"
mappings.

Some IOMMU protocol implementations might be different -- for them the
live configuration might only be known by constantly tracking the full
set of Map() and Unmap() operations. That's a hurdle for the implementation.

Furthermore, what is supposed to happen if both the IOMMU driver and the
individual PCI driver destroy the mapping at ExitBootServices()? If the
PCI driver's notification function runs first, then the IOMMU driver can
mark the mapping as "unmapped", and ignore it in its own notification
function. If the IOMMU driver's notification function runs first, then
it can again only mark the mapping as "unmapped", so that when the PCI
driver tries to unmap it as well, things don't blow up. This seems to
imply that an unmapped mapping data structure can never be recycled,
because we never know what action might follow. This sounds very
confusing to me.


Now, there are two arguments in favor of the IOMMU ExitBootServices()
callback as well:

- Security. It doesn't matter if some driver forgets to de-configure a
translation, the IOMMU driver won't. Layering violations be damned.

- In ExitBootServices() notification functions, the UEFI memory map must
not be altered (memory cannot be released). This means that Bus Master
Write and Bus Master Read operations, for pending transfers, cannot be
unmapped (only CommonBuffer operations can be), because unmapping
Write/Read might free memory (bounce buffers) *implicitly*. For
CommonBuffer operations, separate PciIo.FreeBuffer() calls are necessary
for releasing memory, thus the notification functions can simply *not*
call FreeBuffer(), to stay safe. But this is not possible for pending
Read/Write operations.


My current thinking is that (a) PCI drivers should unmap pending Common
Buffer operations at ExitBootServices; (b) PCI drivers should never
return from their protocol member functions with pending Write/Read
operations, only CommonBuffer operations (put differently, if the
operation is asynchronous on the higher protocol level, then make the
underlying BMDMA operation CommonBuffer); (c) IOMMU protocol
implementations should not be required to clean up translations, only
allowed to, if they can do it without interfering with (a).

Basically, the question is, who *owns* the translations. Based on the
PciIo interfaces, and on the requirement that PCI drivers Map() and
Unmap() buffers, my opinion is that the PCI drivers own the translations.

If you think that we should take this question to the USWG, I'm OK with
that.

Thanks,
Laszlo



> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Monday, September 4, 2017 3:55 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common buffers at ExitBootServices()
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: pci_host_controllers_unmap
> 
> This series updates four PCI Host Controller drivers so that they don't leave CommonBuffer mappings hanging when control is transfered to the OS.
> 
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (4):
>   MdeModulePkg/UhciDxe: unmap BM common buffers when exiting boot
>     services
>   MdeModulePkg/EhciDxe: unmap BM common buffers when exiting boot
>     services
>   MdeModulePkg/XhciDxe: unmap BM common buffers when exiting boot
>     services
>   MdeModulePkg/AtaAtapiPassThru: unmap common buffers at
>     ExitBootServices()
> 
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h         | 18 ++++++
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |  5 ++  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 67 +++++++++++++++++++-
>  MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c                      | 25 +++++++-
>  MdeModulePkg/Bus/Pci/UhciDxe/Uhci.c                      | 25 +++++++-
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c                      | 31 +++++++++
>  6 files changed, 168 insertions(+), 3 deletions(-)
> 
> --
> 2.14.1.3.gb7cf6e02401b
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>