From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 87BC621E3EA63 for ; Mon, 4 Sep 2017 14:16:58 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4C2F1750FC; Mon, 4 Sep 2017 21:19:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4C2F1750FC Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-2.rdu2.redhat.com [10.10.120.2]) by smtp.corp.redhat.com (Postfix) with ESMTP id 34FE2121E36; Mon, 4 Sep 2017 21:19:44 +0000 (UTC) To: "Zeng, Star" , edk2-devel-01 Cc: "Dong, Eric" References: <20170903195449.30261-1-lersek@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B93A125@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <4b24b1eb-362f-3b46-97e2-bdfda53f40c9@redhat.com> Date: Mon, 4 Sep 2017 23:19:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B93A125@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Mon, 04 Sep 2017 21:19:46 +0000 (UTC) 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: Mon, 04 Sep 2017 21:16:58 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Dong, Eric ; Zeng, Star > 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 > 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 ++ 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 >