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 4DA0821A00A1B for ; Thu, 18 May 2017 01:50:30 -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 7D439C04B923; Thu, 18 May 2017 08:50:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7D439C04B923 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 7D439C04B923 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-86.phx2.redhat.com [10.3.116.86]) by smtp.corp.redhat.com (Postfix) with ESMTP id B01595C884; Thu, 18 May 2017 08:50:27 +0000 (UTC) To: Brijesh Singh , Jordan Justen , edk2-devel@lists.01.org Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com, Jiewen Yao References: <1494454162-9940-1-git-send-email-brijesh.singh@amd.com> <1494454162-9940-7-git-send-email-brijesh.singh@amd.com> <1ebdde6a-bad7-ed9a-b2af-7334477c8ae3@redhat.com> <149452462682.9607.8151737434916832513@jljusten-skl.jf.intel.com> <149487045771.31444.19976106484440238@jljusten-skl> <89da82c5-1e23-190b-0725-76f4da595987@amd.com> <149495739728.2794.9440373089056382638@jljusten-skl> <70e53c60-60a0-9cf8-9dc9-49d84718cc85@amd.com> From: Laszlo Ersek Message-ID: <6c3b33c1-1dde-4c54-671f-ac1e749c0e71@redhat.com> Date: Thu, 18 May 2017 10:50:26 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <70e53c60-60a0-9cf8-9dc9-49d84718cc85@amd.com> 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.31]); Thu, 18 May 2017 08:50:29 +0000 (UTC) Subject: Re: [RFC v4 06/13] OvmfPkg:AmdSevDxe: add AmdSevDxe 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, 18 May 2017 08:50:30 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/16/17 22:25, Brijesh Singh wrote: > > > On 05/16/2017 12:56 PM, Jordan Justen wrote: >> On 2017-05-16 05:04:58, Brijesh Singh wrote: >>> Hi Jordan, >>> >>> On 5/15/17 12:47 PM, Jordan Justen wrote: >>>> On 2017-05-11 11:01:57, Brijesh Singh wrote: >>>>> >>>>> We basically need some kind of guarantee that this driver is run >>>>> before any other >>>>> drivers or libs access MMIO register/buffers. In additional to >>>>> clearing encryption >>>>> bit from MMIO spaces, the driver also installs IOMMU protocol. So >>>>> far, IOMMU protocol >>>>> is directly consumed by PciHostBridgeDxe driver and QemuFwCfgDxeLib. >>>> What about adding a NULL protocol named >>>> gOvmfIoMmuDetectionProtocolGuid? (Better name suggestions welcomed. :) >>>> Then we can use this protocol in a depex where needed. >>> It should be doable, If I find better name then I will use that :) >>>> Maybe we should consider naming the driver IoMmuDxe instead? >>>> >>>> I think the generic PciRoot bridge driver shouldn't need this in the >>>> depex because it will not start until the BDS phase, and the IoMmuDxe >>>> driver would have been dispatched by then. >>> Are you suggesting that we introduce a new IoMmuDxe driver and install >>> IOMMU protocol unconditionally ? >> >> No. I'm suggesting we have a new protocol that only exists to allow >> dependency expressions to know that we've attempted to detect an IoMmu >> implementation. >> > > Okay got it thanks. > >> The driver would "install" the protocol with a NULL pointer regardless >> of whether the IoMmu protocol was installed. Maybe >> gOvmfIoMmuDetectionAttemptedProtocolGuid would be a better name? >> >> The DXE fw-cfg library should then list this under depex. I think the >> PCI Host bridge driver doesn't require the depex for the reason I >> mentioned abobe. >> >> The gEdkiiIoMmuProtocolGuid protocol would only be installed be >> installed when detected like your patches currently do. >> >> This method should allow the driver runtime order dependency to be >> explicitly indicated. >> >> Regarding the 'IoMmuDxe' name, I was suggesting that AmdSevDxe be >> renamed to IoMmuDxe. Since we would be installing the 'we tried to >> detect iommu' protocol, it probably makes sense to put all the iommu >> implementation support into a single driver and only install the >> 'detection attempted' protocol it after trying to detect all supported >> iommu implementations. >> >> There could be an issue with this. FvbServicesRuntimeDxe.inf is in the >> apriori currently if SMM_REQUIRE is set, so if it needs the iommu >> treatment, then this wouldn't work. This driver does use MM I/O just >> below 4GB. I don't think your current patches would change how this >> driver runs since it doesn't use the PCI host bridge protocol, so I >> guess it is ok? >> > > We do need to ensure that AmdSevDxe runs before FvbServicesRuntimeDxe.inf. > As you rightly pointed, FvbServicesRuntimeDxe uses MM I/O below 4GB hence > we need to clear encryption attribute from MMIO space before QemuFlash > detection logic is invoked. In my patch set, I have listed AmdSevDxe.inf > before > FvbServicesRuntimeDxe.inf to ensure that we clear the MMIO before QemuFlash > detection logic from FvbServicesRuntimeDxe.inf is invoked. > > Here is fdt file snippet after my patches. > > APRIORI DXE { > INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf > INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf > INF OvmfPkg/AmdSevDxe/AmdSevDxe.inf > !if $(SMM_REQUIRE) == FALSE > INF OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf > !endif > } > > >> (It would be nice to get FvbServicesRuntimeDxe.inf out of the apriori >> too, but that is a separate issue.) >> > > Yes, if we can move that out from Apriori then maybe also need to add some > kind of depex to ensure that it gets called after we clear memory > encryption > bit from MMIO regions. Adding a couple of points down here. (1) As Brijesh points out, AmdSevDxe does two things. The second thing (the IOMMU protocol implementation) could be detached, yes, and as Jordan suggests, we could introduce a synthetic / placeholder protocol as well, for dependent modules to depend on. DEPEXes can use "OR" operators (see "Table 21. Dependency Expression Opcode Summary" in Vol2 of PI 1.5), so a library instance could impart client modules with an alternative dependency on either the real IOMMU protocol thing or the placeholder thing. AmdSevDxe would then look for the SEV capability, and install the appropriate (IOMMU or placeholder) protocol. However, the other thing that AmdSevDxe provides doesn't look possible to move out of the APRIORI DXE file. Clearing the C bit on all nonexistent GCD ranges, and on the MMIO GCD ranges (which at that point all come from PEI HOBs) enables *all* DXE drivers to go about their MMIO range additions and allocations without knowing about SEV. I think keeping generic MMIO-using DXE drivers blissfully unaware of SEV is an important design goal. (2) QemuFlashFvbServicesRuntimeDxe is in the APRIORI DXE file when SMM_REQUIRE is *clear*, not when it is set. When SMM_REQUIRE is set, then pflash is a hard requirement (*), and the build includes only QemuFlashFvbServicesRuntimeDxe -- namely, the SMM build thereof --; the build doesn't include EmuVariableFvbRuntimeDxe. Therefore only one FVB provider exists, so there's no need to enforce any dispatch order between "competing" FVB providers. With SMM_REQUIRE being *clear*, there are two competing FVB providers, and QemuFlashFvbServicesRuntimeDxe must get priority over EmuVariableFvbRuntimeDxe. This is why we add QemuFlashFvbServicesRuntimeDxe to the APRIORI DXE file in that case. Please see commit 46df0216b0ed ("OvmfPkg: pull in SMM-based variable driver stack", 2015-11-30). (*) Dynamically degrading flash access from pflash to emulated would be a security bug. This is why SMM_REQUIRE is called SMM_REQUIRE and not SMM_ENABLE, and why hanging the SMM_REQUIRE build when pflash is missing is the right thing. Thanks, Laszlo