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 8BA0421B03843 for ; Thu, 25 May 2017 10:58:49 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C88347AE8B; Thu, 25 May 2017 17:58:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C88347AE8B Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com C88347AE8B Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-147.phx2.redhat.com [10.3.116.147]) by smtp.corp.redhat.com (Postfix) with ESMTP id C323E5C540; Thu, 25 May 2017 17:58:46 +0000 (UTC) From: Laszlo Ersek To: Brijesh Singh , jordan.l.justen@intel.com Cc: edk2-devel@lists.01.org, Thomas.Lendacky@amd.com, leo.duran@amd.com, Jiewen Yao , "Ni, Ruiyu" References: <1495466592-21641-1-git-send-email-brijesh.singh@amd.com> <1495466592-21641-8-git-send-email-brijesh.singh@amd.com> Message-ID: Date: Thu, 25 May 2017 19:58:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 25 May 2017 17:58:49 +0000 (UTC) Subject: Re: [PATCH v5 07/14] OvmfPkg:IoMmuDxe: Add IoMmuDxe driver 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: Thu, 25 May 2017 17:58:49 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/24/17 17:09, Laszlo Ersek wrote: > (1) So, I don't think that splitting this driver off of AmdSevDxe is a > significant improvement, given that we still need to add AmdSevDxe to > the APRIORI DXE file, in order to clear the C bit on NonExistent and > MMIO ranges. > > If Jordan thinks it is an improvement nonetheless, I don't mind the > split, of course. > > On 05/22/17 17:23, Brijesh Singh wrote: >> The IOMMU protocol driver provides capabilities to set a DMA access >> attribute and methods to allocate, free, map and unmap the DMA memory >> for the PCI Bus devices. >> >> Due to security reasons all DMA operations inside the SEV guest must >> be performed on shared (i.e unencrypted) pages. The IOMMU protocol >> driver for the SEV guest uses a bounce buffer to map guest DMA buffer >> to shared pages inorder to provide the support for DMA operations inside >> SEV guest. >> >> The patch adds a new synthetic/placeholder protocol >> 'gIoMmuDetectedProtocolGuid" to allow other dependent modules to depend >> on IoMmuDxe driver being run. > > (2) If we add the protocol GUID to the OVMF DEC file, that should be a > separate patch, in my opinion. The commit message should explain, in a > stand-alone manner, what the protocol stands for. > > (I see Jordan's suggestion for the proto name in > , > namely "gOvmfIoMmuDetectionProtocolGuid", but I think that Brijesh's > suggestion is closer to the protocol names we already have under > [Protocols] in OvmfPkg.dec.) > >> IoMmuDxe driver looks for SEV capabilities, if present then it installs >> the real IOMMU protocol otherwise it installs placeholder protocol. >> Currently, PciRoot Bridge and QemuFWCfgLib need to know the existance >> of IOMMU protocol. So the modules needing the IOMMU support should add >> both gEdkiiIoMmuProtocolGuid and gIoMmuDetectedProtocolGuid in there depex. > > (3) This description (which again belongs to the separate patch that > introduces the protocol) should be formulated without mentioning SEV or > QemuFwCfgLib. Something like: > > Platforms that optionally provide an IOMMU protocol should do so by > including a DXE driver (usually called IoMmuDxe) that produces either > the IOMMU protocol -- if the underlying capabilities are available --, > or gIoMmuDetectedProtocolGuid, to signal that the IOMMU capability > detection completed with negative result (i.e., no IOMMU will be > available in the system). > > In turn, DXE drivers (and library instances) that are supposed to use > the IOMMU protocol if it is available should add the following to > their DEPEX: > > gEdkiiIoMmuProtocolGuid OR gIoMmuDetectedProtocolGuid > > This ensures these client modules will only be dispatched after IOMMU > detection completes (with positive or negative result). > >> Please note that since PciRoot Bridge driver does not run until the BDS >> phase, and IoMmuDxe driver would have been dispatched by then hence we >> do not need to add depex in PciRoot Bridge driver inf file. > > (4) This statement is incorrect. > > PciHostBridgeDxe is definitely dispatched before BDS is entered, it is a > plain DXE driver. It uses platform knowledge (abstracted into > PciHostBridgeLib --> PciHostBridgeGetRootBridges()) to produce root > bridge IO protocol instances, in its entry point (--> > InitializePciHostBridge()). > > PciHostBridgeDxe indeed does not have a depex on the IOMMU protocol. It > registers a protocol notify instead. > > The reason why PciHostBridgeDxe gets away with this *usually* is that > the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL member functions that it exposes are > *usually* only called from within BDS, after: > > - platform BDS connects the root bridge protocol instances to the PCI > Bus driver (which is a UEFI driver), > > - the PCI Bus driver produces PciIo protocol instances on top of the > root bridge IO instances, > > - then various PCI device drivers start massaging the devices via PciIo > protocol instances, ultimately boiling down to PciHostBridgeDxe()'s > Map() function and friends. > > However, the following driver dispatch order is also possible, entirely > within DXE: > > (a) a platform DXE driver is dispatched and registers a protocol notify > for root bridge IO, > > (b) PciHostBridgeDxe is dispatched and produces a number of root bridge > IO protocol instances, > > (c) the platform DXE driver gets called back and it uses the root bridge > IO member functions (such as Map etc), > > (d) The IOMMU DXE driver is dispatched and installs the IOMMU protocol, > > (e) the PciHostBridgeDxe driver is called back, and its Map() etc > functions will rely on the IOMMU *only* from this point forward. > > This suggests that: > > - "gIoMmuDetectedProtocolGuid" should actually be called > "gEdkiiIoMmuAbsentProtocolGuid", and should be upstreamed to MdeModulePkg, > > - all DXE drivers (no exceptions) that *conditionally* depend on > gEdkiiIoMmuProtocolGuid (with a protocol notify or otherwise) should use > the following DEPEX instead: > > gEdkiiIoMmuProtocolGuid OR gEdkiiIoMmuAbsentProtocolGuid > > My point is basically that, when PciHostBridgeDxe installs the > EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instances, they are not (necessarily) > ready for use. I've been thinking about this. If you add a patch to the series where you change the DEPEX of PciHostBridgeDxe to gEdkiiIoMmuProtocolGuid OR gEdkiiIoMmuAbsentProtocolGuid then the maintainer (Ray or Jiewen I think) will likely reject that patch, because afterwards, all platforms that include PciHostBridgeDxe would suddenly have to produce gEdkiiIoMmuAbsentProtocolGuid, minimally. We've faced a similar situation before. The solution was a header-less library instance, linked into the affected MdeModulePkg driver via NULL class resolution, through the platform DSC file. The lib instance did nothing at all (it only had an empty constructor function), but its INF file spelled out the necessary DEPEX. By linking the lib instance into the driver, the driver's behavior didn't change, but it became dependent on the DEPEX. See the following commits: 05db0948cc60 EmbeddedPkg: introduce EDKII Platform Has ACPI GUID 786f476323a6 EmbeddedPkg: introduce PlatformHasAcpiLib (See also later commit a391e5925dc3, "MdeModulePkg: move PlatformHasAcpiGuid from EmbeddedPkg", 2017-04-05.) We recognized that imparting a depex on a driver "from the outside" is a somewhat general need, so we filed . The point of this BZ is to have just one such depex-giving library, and to parametrize that library with the actual GUID at build time (with a fixed-at-build PCD). Alas, BZ#443 doesn't really apply here, because the depex we want to impart on PciHostBridgeDxe here is a disjunction (=OR) of two protocol GUIDs, not just a single protocol GUID. So... Not sure if Jordan will agree, but I think ultimately my suggestion is this: * call the new placeholder protocol GUID "gIoMmuAbsentProtocolGuid", * add it to OvmfPkg.dec, but split the GUID introduction off to a separate patch (see commit message suggested above), * add a new library instance like described above, under OvmfPkg/Library, with the OR depex, following the pattern of PlatformHasAcpiLib. I guess this library instance could be called PlatformHasIoMmuLib. * what remains of this patch, for the second part, should be correct then (install either the real IOMMU proto or the placeholder) * "[PATCH v5 12/14] OvmfPkg/QemuFwCfgLib: Implement SEV internal function for Dxe phase" will also be correct, you'll just have to update the placeholder protocol's name in the DEPEX (plus clean up the cosmetic remarks) * finally, identify all DXE_DRIVER, DXE_SMM_DRIVER and DXE_RUNTIME_DRIVER modules pulled into the OVMF build(s) -- see the DSCs -- that make use of the IOMMU protocol in some way. Then, hook the new "OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf" into all those modules in the DSCs, via NULL class resolution. See commit 3a2c1548fe2d as an example ("ArmVirtPkg: enable AcpiTableDxe and EFI_ACPI_TABLE_PROTOCOL dynamically", 2017-03-17). These steps together will ensure: - no modification to PciHostBridgeDxe, or the other platforms that consume it - all IOMMU-capable drivers in OVMF will be sufficiently delayed until either of the protocols is installed. Jordan, are you OK with this idea? Thanks! Laszlo >> Cc: Jordan Justen >> Cc: Laszlo Ersek >> Cc: Leo Duran >> Cc: Jiewen Yao >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Suggested-by: Jiewen Yao >> Signed-off-by: Brijesh Singh >> Reviewed-by: Jiewen Yao >> --- >> OvmfPkg/OvmfPkg.dec | 1 + >> OvmfPkg/OvmfPkgIa32.dsc | 1 + >> OvmfPkg/OvmfPkgIa32X64.dsc | 1 + >> OvmfPkg/OvmfPkgX64.dsc | 1 + >> OvmfPkg/OvmfPkgIa32.fdf | 1 + >> OvmfPkg/OvmfPkgIa32X64.fdf | 1 + >> OvmfPkg/OvmfPkgX64.fdf | 1 + >> OvmfPkg/IoMmuDxe/IoMmuDxe.inf | 49 +++ >> OvmfPkg/IoMmuDxe/AmdSevIoMmu.h | 43 ++ >> OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 459 ++++++++++++++++++++ >> OvmfPkg/IoMmuDxe/IoMmuDxe.c | 53 +++ >> 11 files changed, 611 insertions(+)