From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-in6.apple.com (mail-out6.apple.com [17.151.62.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 CAEFB21AE30CE for ; Thu, 1 Jun 2017 08:36:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; d=apple.com; s=mailout2048s; c=relaxed/simple; q=dns/txt; i=@apple.com; t=1496331471; h=From:Sender:Reply-To:Subject:Date:Message-id:To:Cc:MIME-version:Content-type: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-reply-to:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=HY9xk1NfdShfLlT+cKpKjcF5vE+hHbxlhdBjn2+kYQs=; b=FaZTzqq7/G7zjkjwW9yE6S/cFKYBqEEYByIIWTVJXS32UFsVx/bNrnS/ojnWi1xb oefIAHP2s0BEUiQ60jX9GPfyfoMSW/xGP+aBwpMOXLWLpGrXuOyJjJ5ozRc577MT VQ4sbvSZaDRLPG27S8mPo+aVzJRy7x4imh0tHTDeNzI4aGAadvnr4FkKcgq8c7OY 6iNFUsws4oK9AoZWFuuf5xxKRFd+r12+qrVyAstndjGaIm5zmJ1SOqlYcD8kJba3 TOOo6Tzpamsvfg8X0VIJtVHY8eMlqgcKl/O/j/qNgHVJ9CevW+KtkbVbPATSar+q vDOeMuwAiYhsJFXSKZ6HzQ==; Received: from relay6.apple.com (relay6.apple.com [17.128.113.90]) (using TLS with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mail-in6.apple.com (Apple Secure Mail Relay) with SMTP id A2.8D.24649.FC430395; Thu, 1 Jun 2017 08:37:51 -0700 (PDT) X-AuditID: 11973e15-f22c49a000006049-81-593034cf3b49 Received: from nwk-mmpp-sz09.apple.com (nwk-mmpp-sz09.apple.com [17.128.115.80]) by relay6.apple.com (Apple SCV relay) with SMTP id 49.5B.09762.EC430395; Thu, 1 Jun 2017 08:37:50 -0700 (PDT) MIME-version: 1.0 Received: from da0601a-dhcp124.apple.com (da0601a-dhcp124.apple.com [17.226.15.124]) by nwk-mmpp-sz09.apple.com (Oracle Communications Messaging Server 8.0.1.2.20170210 64bit (built Feb 10 2017)) with ESMTPSA id <0OQV00KNAKR2C200@nwk-mmpp-sz09.apple.com>; Thu, 01 Jun 2017 08:37:50 -0700 (PDT) Sender: afish@apple.com From: Andrew Fish Message-id: <9C0D940E-6A36-497B-9840-5012B6D365D6@apple.com> Date: Thu, 01 Jun 2017 08:37:48 -0700 In-reply-to: Cc: Laszlo Ersek , Thomas.Lendacky@amd.com, Jordan Justen , edk2-devel-01 , Liming Gao , leo.duran@amd.com, Jiewen Yao , Jeff Fan To: Brijesh Singh References: <1495809845-32472-1-git-send-email-brijesh.singh@amd.com> <149583274037.25973.13062338567511386932@jljusten-skl> <6ecd0138-454e-6a6e-d034-beaf63466120@redhat.com> <149609029319.5770.13917390389219314003@jljusten-skl> <14301d64-9fa3-8231-42c1-52c2dcd9f96f@amd.com> <149630284935.10663.16670660897918560882@jljusten-skl> <181773F8-7C21-4CBD-A552-AEC02B57CEA0@apple.com> X-Mailer: Apple Mail (2.3273) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFmpmkeLIzCtJLcpLzFFi42IRbCiM0j1vYhBpsPkSi8XMTX2MFnsOHWW2 OLl+CaPFuo8eFjuu9bNYdD8/yW6x7NgOFosV9zawWxyZso/VgdOj9dJfNo/Fe14yeXTP/sfi 8X7fVbYAligum5TUnMyy1CJ9uwSujCmrzrIWtH9irPj0/DlLA+OWC4xdjJwcEgImEg//bWXt YuTiEBJYzSQx5dx6oAQHWOLx2xSI+EFGiV0bd7CBNPAKCEr8mHyPBcRmFgiTaNhxkAmiaC2T xNfXx8ASwgLiEu/ObGIGsdkElCVWzP/ADtFsI9H/6gxUjb/EmZ9XwGpYBFQlrs9uZwKxOQWs JX6cmAo2lFlgIpPEri/7wBIiApoSD8/9gDr1DbPEj5d/oX6Qlbg1+xIzSEJCoJtdYtfD6UwT GIVmITl3FpJzIWwtie+PWoHiHEC2vMTB87IQYU2JZ/c+sUPY2hJP3l1gXcDItopRKDcxM0c3 M89ML7GgICdVLzk/dxMjKNam24nuYDyzyuoQowAHoxIP7wNZg0gh1sSy4srcQ4zSHCxK4rw3 5IFCAumJJanZqakFqUXxRaU5qcWHGJk4OKUaGFddCH0pWhf//UTiXMO322zm/G2KP9dkKM7+ XnpBYfjs2pURFmH6kbF176VnO90z1OeoCF6hmfkzSUMkVf/ky2UyfqJV6SxdxTw1MaULSizv 9L9VMFn0YktS/Ov1a6cWu6/hPXTuxM7VuhdVnip53bFe5sipsz9Ezn+FfqF3OV/8Ccv5t3ZX K7EUZyQaajEXFScCAK8o4lmWAgAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrLIsWRmVeSWpSXmKPExsUi2FAcoHvOxCDS4PB/M4uZm/oYLfYcOsps cXL9EkaLdR89LHZc62ex6H5+kt1i2bEdLBYr7m1gtzgyZR+rA6dH66W/bB6L97xk8uie/Y/F 4/2+q2wBLFFcNimpOZllqUX6dglcGVNWnWUtaP/EWPHp+XOWBsYtFxi7GDk4JARMJB6/Teli 5OIQEjjIKLFr4w62LkZODl4BQYkfk++xgNjMAmESDTsOMkEUrWWS+Pr6GFhCWEBc4t2ZTcwg NpuAssSK+R/YIZptJPpfnYGq8Zc48/MKWA2LgKrE9dntTCA2p4C1xI8TU8GGMgtMZJLY9WUf WEJEQFPi4bkfrBDb3jBL/Hj5lxEkISEgK3Fr9iXmCYz8s5BcOAvJhRC2lsT3R61AcQ4gW17i 4HlZiLCmxLN7n9ghbG2JJ+8usC5gZFvFKFCUmpNYaaaXWFCQk6qXnJ+7iREcG4VROxgbllsd YhTgYFTi4bVQMIgUYk0sK67MPcQowcGsJMJ7RBMoxJuSWFmVWpQfX1Sak1p8iHEiI9CbE5ml RJPzgZGbVxJvaGJiYGJsbGZsbG5iTkthJXFeDk79SCGB9MSS1OzU1ILUIpijmDg4pRoYw1Xc lq+WmmnD92Fnqcj55ntrzezPsy26s/n5WpYZH0umP1TnV2BwPrBCf0bbjY8TRbtq2P5P+Knh uiJ2vuGOwFXBrUE7bXlaUn4t1PuyPiE9g+2J5bpyk9tLkp4Wxv9p3V/G92f55I4FHpflz81j i9q/8xVDz5t3SeuYPpjr/l39zfaD/toXZUosxRmJhlrMRcWJADM4MBYAAwAA X-Content-Filtered-By: Mailman/MimeDel 2.1.22 Subject: Re: [PATCH v6 00/17] x86: Secure Encrypted Virtualization (AMD) 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, 01 Jun 2017 15:36:52 -0000 Content-Type: text/plain; CHARSET=US-ASCII Content-Transfer-Encoding: 7BIT > On Jun 1, 2017, at 8:01 AM, Brijesh Singh wrote: > > Hi Andrew, > > The goal is to clear the "C" bit in PTE for all the MMIO areas in the GCD memory > space map. I think Leo looked at SetMemoryAttributes() based on Mike's feedback, > but I believe SetMemoryAttribute may get called on any range without specifying > types (we are interested in MMIO ranges, which are specified in ADD/REMOVE operations). > Brijesh, Sorry pre coffee... I gest I'm trying to ask more generic questions. 1) Should the DXE Core queue GCD_SET_ATTRIBUTES_MEMORY_OPERATIONs so you can update things from PEI, or not require a priori dispatch? 2) Is there an issue in the GCD implementation or architecture. For example I recently ran into an issue use EfiGcdMemoryTypeMemoryMappedIo as it was causing massive amounts of virtual mapping requests to the OS. It looks like by default EfiGcdMemoryTypeMemoryMappedIo sets the EFI_MEMORY_RUNTIME Capability. I'm not exactly sure that is what the PI Spec requires? It makes sense to me that from EFI MemoryMappedIo things require the EFI_MEMORY_RUNTIME, but for GCD seems like you might have massive ranges of MMIO that need to get mapped, and those regions get owned by the OS at runtime? Anyway not trying to derail your solution, or block progress. I just want to make sure we have the conversation as to why this ended up being so hard to make sure we don't have an implementation or spec issue around things GCD. PI Spec on EfiGcdMemoryTypeMemoryMappedIo The GetMemoryMap() implementation must include into the UEFI memory map all GCD map entries of types EfiGcdMemoryTypeReserved and EfiPersistentMemory, and all GCD map entries of type EfiGcdMemoryTypeMemoryMappedIo that have EFI_MEMORY_RUNTIME attribute set. vs. The GetMemoryMap() implementation must include all GCD map entries of types EfiGcdMemoryTypeReserved and EfiGcdMemoryTypeMemoryMappedIo into the UEFI memory map. Thanks, Andrew Fish > -Brijesh > > On 06/01/2017 08:48 AM, Andrew Fish wrote: >> Laszlo, >> The current design is DXE IPL and gEfiCpuArchProtocolGuid abstract the CPU specifics from the DXE Core. >> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Gcd/Gcd.c#L866 >> if (Operation == GCD_SET_ATTRIBUTES_MEMORY_OPERATION) { >> // >> // Call CPU Arch Protocol to attempt to set attributes on the range >> // >> CpuArchAttributes = ConverToCpuArchAttributes (Attributes); >> if (CpuArchAttributes != INVALID_CPU_ARCH_ATTRIBUTES) { >> if (gCpu == NULL) { >> Status = EFI_NOT_AVAILABLE_YET; >> } else { >> Status = gCpu->SetMemoryAttributes ( >> gCpu, >> BaseAddress, >> Length, >> CpuArchAttributes >> ); >> } >> if (EFI_ERROR (Status)) { >> CoreFreePool (TopEntry); >> CoreFreePool (BottomEntry); >> goto Done; >> } >> } >> } >> Maybe the issue is there is an attempt to change attributes too early and they currently get sent to the bit bucket? I guess they could get queued up and replayed after gEfiCpuArchProtocolGuid is preset? >> Thanks, >> Andrew Fish >>> On Jun 1, 2017, at 2:10 AM, Laszlo Ersek >> wrote: >>> >>> On 06/01/17 09:40, Jordan Justen wrote: >>>> On 2017-05-29 14:59:46, Brijesh Singh wrote: >>>>> >>>>> >>>>> On 5/29/17 3:38 PM, Jordan Justen wrote: >>>>>> On 2017-05-29 04:16:15, Laszlo Ersek wrote: >>>>>>> (looks like I was the one to comment as second reviewer after all :) ) >>>>>>> >>>>>>> On 05/26/17 23:05, Jordan Justen wrote: >>>>>>>> On 2017-05-26 07:43:48, Brijesh Singh wrote: >>>>>>>>> Changes since v4: >>>>>>>>> - decouple IoMmu protocol implementation from AmdSevDxe into a seperate >>>>>>>>> IoMmuDxe driver. And introduce a placeholder protocol to provide the >>>>>>>>> dependency support for the dependent modules. >>>>>>>> I think you split IoMmuDxe out from AmdSevDxe based on my feedback >>>>>>>> regarding APRIORI, but I don't think this helped. >>>>>>>> >>>>>>>> Ideally I would like to see one driver named IoMmuDxe that is *not* in >>>>>>>> APRIORI. >>>>>>> There are two separate goals here: >>>>>>> >>>>>>> (1) Make sure that any driver that adds MMIO ranges will automatically >>>>>>> add those ranges with the C bit cleared in the PTEs, without actually >>>>>>> knowing about SEV. >>>>>> Ok, this sounds reasonable. >>>>>> >>>>>> The APRIORI method looks like a hack. Why is this not being handled at >>>>>> the time the page tables are being built, in DxeIpl? Couldn't we >>>>>> define a platform Page Tables library to allow a platform to somehow >>>>>> modify the page tables as they are built? Or, maybe just after? This >>>>>> would also make sure it happens before DXE runs. >>>>> >>>>> Before introducing AmdSevDxe driver, we did proposed patches to clear >>>>> the C-bit during the page table creation time. In the first patch [1], >>>>> Leo tried to teach gcd.c to clear the C-bit from MMIO. IIRC, the main >>>>> concern was -- typically Dxecore does not do any CPU specific thing >>>>> hence we should try to find some alternative approach. >>>> >>>> DxeCore doesn't build the page tables. DxeIpl builds them. I agree >>>> that DxeCore is not the right place to handle this. In >>>> https://lists.01.org/pipermail/edk2-devel/2017-March/008987.html >>>> Jiewen suggested that DxeIpl could be updated during page table >>>> creation time. >>>> >>>> In https://lists.01.org/pipermail/edk2-devel/2017-April/009883.html >>>> Leo said that DxeIpl won't work because new I/O ranges might be added. >>>> I don't understand this, because isn't DxeIpl and an early APRIORI >>>> entry are roughly equivalent in the boot sequence? >>> >>> I think you are right. I believe a patch for this exact idea hasn't been >>> posted yet. Jiewen's message that you linked above contains the expression >>> >>> always clear SEV mask for MMIO *and all rest* >>> >>> (emphasis mine), which I think we may have missed *in combination with* >>> the DxeIpl. >>> >>> So the idea would be to iterate over all the HOBs in the DxeIpl PEIM. >>> Keep the C bit set for system memory regions. Clear the C bit for MMIO >>> regions that are known from the HOB list. Also clear the C bit >>> everywhere else in the address space (known from the CPU HOB) where no >>> coverage is provided by any memory resource descriptor HOB. >>> >>> This is going to be harder than the current approach, because: >>> >>> - The current approach can work off of the GCD memory space map, which >>> provides explicit NonExistent entries, covering the entire address space >>> (according to the CPU HOB). >>> >>> - However, the DxeIpl method would take place before entering DXE, so no >>> GCD memory space map would be available -- the "NonExistent" entries >>> would have to be synthesized manually from the address space size (known >>> from the CPU HOB) and the lack of coverage by memory resource descriptor >>> HOBs. >>> >>> Basically, in order to move the current GCD memory space map traversal >>> from early DXE to late PEI, the memory space map building logic of the >>> DXE Core would have to be duplicated in the DxeIpl PEIM. If I understand >>> correctly. (The DxeIpl PEIM may already contain very similar code, for >>> the page table building, which might not be difficult to extend like >>> this -- I haven't looked.) >>> >>> Is this what you have in mind? >>> >>> Thanks >>> Laszlo >>> >>>> -Jordan >>>> >>>>> In second patch >>>>> [2], Leo tried to introduce a new notify protocol to get MMIO add/remove >>>>> events. During discussion Jiewen suggested to look into adding a new >>>>> platform driver into APRIORI to avoid the need for any modifications >>>>> inside the Gcdcore - this seems workable solution which did not require >>>>> adding any CPU specific code inside the Gcd. >>>>> >>>>> [1] https://lists.01.org/pipermail/edk2-devel/2017-March/008974.html >>>>> [2] https://lists.01.org/pipermail/edk2-devel/2017-April/009852.html >>>>> >>> >>> _______________________________________________ >>> 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