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

["Yao, Jiewen" <jiewen.yao@intel.com>]

HI Laszlo
Thank you very much to raise this DMA topic.

I have a chat with Star. We have couples of question, from high level to low level.
Hope you can clarify:


1)       About disable DMA

As you mentioned, in UEFI_Driver_Writer_Guide_V1.0.1_120308.pdf, 7.7 Adding the Exit Boot Services feature
=========================
Examples from the EDK II that use this feature are the PCI device drivers for USB Host
Controllers. Some USB Host Controllers are PCI Bus Masters that continuously access a
memory buffer to poll for operation requests. Access to this memory buffer by a USB
Host Controller may be required to boot an operation system, but this activity must be
terminated when the OS calls ExitBootServices(). The typical action in the Exit Boot
Services Event for these types of drivers is to disable the PCI bus master and place the
USB Host Controller into a halted state
=========================

I believe the fundamental requirement is to "disable DMA".
As such, the simplest action is to clear PCI Bus Master Enable bit in the command register.

Do you think clearing BME is enough?
Is there any special reason that we must unmap the DMA buffer?


2)       About common buffer
If there is a special requirement that we must unmap the DMA buffer, why we only unmap the common buffer?

I understand your statement that unmapping a read buffer or write buffer may cause free memory action. But that is implementation choice.

If we have strong requirement to unmap the DMA buffer, we should achieve that.


3)       About interaction with IOMMU
I am not worried about IOMMU interaction.
I believe an proper IOMMU implementation should not block any action taken by a PCI device driver.
Current IntelVTdDxe driver disables VTd engine in ExitBootService, which means the DMA access is no longer managed by VTd engine. It is NOT related to disable DMA buffer.


4)       About the driver you modified
I believe we have some other PCI device driver (NVMe/SDMMC/UFS), besides the ones you modified in this patch (ATA/UCHI/EHCI/XHCI).
If we need take this approach, I assume they also need update, right?

Thank you
Yao Jiewen


From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Tuesday, September 5, 2017 5:20 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common buffers at ExitBootServices()

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<mailto:edk2-devel@lists.01.org>>
> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto: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<mailto:brijesh.singh@amd.com>>
> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Cc: Star Zeng <star.zeng@intel.com<mailto: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<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel