From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D75132095BB8E for ; Mon, 4 Sep 2017 19:15:52 -0700 (PDT) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Sep 2017 19:18:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,477,1498546800"; d="scan'208,217";a="897082250" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by FMSMGA003.fm.intel.com with ESMTP; 04 Sep 2017 19:18:40 -0700 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 4 Sep 2017 19:18:40 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx122.amr.corp.intel.com (10.18.125.37) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 4 Sep 2017 19:18:40 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.39]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.168]) with mapi id 14.03.0319.002; Tue, 5 Sep 2017 10:18:38 +0800 From: "Yao, Jiewen" To: Laszlo Ersek , "Zeng, Star" , edk2-devel-01 CC: "Dong, Eric" Thread-Topic: [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common buffers at ExitBootServices() Thread-Index: AQHTJO6HlEgpQf73uk+WGvJRCtH+06KkA0GAgACz0wCAAMTM8A== Date: Tue, 5 Sep 2017 02:18:37 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A9A79BD@shsmsx102.ccr.corp.intel.com> References: <20170903195449.30261-1-lersek@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B93A125@shsmsx102.ccr.corp.intel.com> <4b24b1eb-362f-3b46-97e2-bdfda53f40c9@redhat.com> In-Reply-To: <4b24b1eb-362f-3b46-97e2-bdfda53f40c9@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.22 Subject: Re: [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common buffers at ExitBootServices() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Sep 2017 02:15:53 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 lo= w 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Examples from the EDK II that use this feature are the PCI device drivers f= or USB Host Controllers. Some USB Host Controllers are PCI Bus Masters that continuousl= y 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 activ= ity 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D I believe the fundamental requirement is to "disable DMA". As such, the simplest action is to clear PCI Bus Master Enable bit in the c= ommand 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 ma= y cause free memory action. But that is implementation choice. If we have strong requirement to unmap the DMA buffer, we should achieve th= at. 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 me= ans 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 th= e 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 Lasz= lo Ersek Sent: Tuesday, September 5, 2017 5:20 AM To: Zeng, Star ; edk2-devel-01 Cc: Dong, Eric Subject: Re: [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap co= mmon 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 ExitBo= otServices? 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 La= szlo Ersek > Sent: Monday, September 4, 2017 3:55 AM > To: edk2-devel-01 > > Cc: Dong, Eric >; Zeng, S= tar > > Subject: [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap comm= on 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 l= eave CommonBuffer mappings hanging when control is transfered to the OS. > > Cc: Brijesh Singh > > Cc: Eric Dong > > Cc: Star Zeng > > > 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 ++ MdeMod= ulePkg/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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel