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 3C05F21967BC1 for ; Tue, 6 Jun 2017 11:28:44 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BEB7BC04B31B; Tue, 6 Jun 2017 18:29:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com BEB7BC04B31B 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 BEB7BC04B31B Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-130.phx2.redhat.com [10.3.116.130]) by smtp.corp.redhat.com (Postfix) with ESMTP id 58A375DC1D; Tue, 6 Jun 2017 18:29:48 +0000 (UTC) To: "Yao, Jiewen" , Brijesh Singh , "Zeng, Star" , "Justen, Jordan L" , "edk2-devel@lists.01.org" , "Dong, Eric" Cc: "Thomas.Lendacky@amd.com" , "Gao, Liming" , "leo.duran@amd.com" , "Fan, Jeff" 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> <661e46af-5e1c-733a-d027-1ae2e3052a28@amd.com> <149671154262.11907.18297341281786344033@jljusten-skl> <0C09AFA07DD0434D9E2A0C6AEB0483103B8E3C1D@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A964542@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Tue, 6 Jun 2017 20:29:47 +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: <74D8A39837DF1E4DA445A8C0B3885C503A964542@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 06 Jun 2017 18:29:51 +0000 (UTC) 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: Tue, 06 Jun 2017 18:28:44 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 06/06/17 16:54, Yao, Jiewen wrote: > Hi > It takes me some time to read all email below. I believe all of us have a clean understanding on what problem we have now and the possible solutions to clear C bit are below > > 1) In DxeIpl, when it builds page table. > > 2) In DxeCore > > a) By use CpuArch > > b) By use page table lib > > c) By use a GCD update callback > > d) By use PlatformHook lib > > 3) In a standalone AmdSev driver. > > Here is my thought: > 2.a) is not possible, per Leo’s investigation. > 2.b) is not a good design, because we do not introduce any Cpu Specific thing to DxeCore so far. > 2.c) and 2.d) are same. I do not suggest we add a private interface to the core just to support one specific feature. > > 1) is one possible solution, I suggested before. But if Leo/Laszlo think it is too hard to implement, I am OK. I think that implementing the logic in the DXE IPL PEIM will be more difficult than doing the same somewhere in the DXE phase. But, I wouldn't oppose the idea. The implementation difficulty is not for me to overcome: it affects Brijesh and Leo (as the implementors) and the DXE IPL PEIM maintainers (in review). NB, Brijesh mentioned up-thread that wherever (= in whichever firmware phase) they massaged the C-bit, they'd like to use BaseMemEncryptSevLib for it, because that would help with live migrating VMs. I can't say off-hand whether the current library instance is appropriate for use in the DXE IPL PEIM -- for example: repeated memory allocation and freeing isn't very good for PEIMs, since no freeing occurs in PEI --, so that's something for Brijesh & Leo to evaluate. Anyhow, if the DXE IPL PEIM approach can be made work, technically, then I'm 100% fine with it personally. Thanks Laszlo > > If 1) cannot be chosen, I still think 3) is the best idea. > It makes the code very clean by introducing a standalone driver to resolve the problem. > Zero impact on existing platform. > If this feature is not needed, just remove the driver. > > I do not see any issue on using a priori, because: A) “a priori” is clearly defined in PI spec, B) “a priori” has already been widely used in current platform in EDKII open source, as well as close source platform. > > Thank you > Yao Jiewen > > > > From: Brijesh Singh [mailto:brijesh.singh@amd.com] > Sent: Tuesday, June 6, 2017 11:51 AM > To: Zeng, Star ; Justen, Jordan L ; Laszlo Ersek ; edk2-devel@lists.01.org; Dong, Eric ; Yao, Jiewen > Cc: Thomas.Lendacky@amd.com; Gao, Liming ; leo.duran@amd.com; Fan, Jeff > Subject: Re: [edk2] [PATCH v6 00/17] x86: Secure Encrypted Virtualization (AMD) > > Hi Jordan, > > > On 6/5/17 9:08 PM, Zeng, Star wrote: >> I was not tracking this thread. >> Jiewen will help give comments about the potential change in MdeModulePkg. >> >> Thanks, >> Star >> -----Original Message----- >> From: Justen, Jordan L >> Sent: Tuesday, June 6, 2017 9:12 AM >> To: Brijesh Singh >; Laszlo Ersek >; edk2-devel@lists.01.org; Zeng, Star >; Dong, Eric > >> Cc: Thomas.Lendacky@amd.com; Gao, Liming >; leo.duran@amd.com; Yao, Jiewen >; Fan, Jeff > >> Subject: Re: [edk2] [PATCH v6 00/17] x86: Secure Encrypted Virtualization (AMD) >> >> On 2017-06-05 14:56:04, Brijesh Singh wrote: >>> On 06/01/2017 04:10 AM, Laszlo Ersek wrote: >>>> On 06/01/17 09:40, Jordan Justen wrote: >>>>> 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? >>>> >>> Do you have any further thought on this? >> Regarding Laszlo's feedback, I'm not convinced that it would be excessively difficult to accomplish this in DxeIpl. (I'm not saying that I couldn't be convinced. :) >> >> As far as I can see, this is an architecturally defined AMD feature. >> (Is this true, or is BaseMemcryptSevLib actually OVMF specific?) > > Yes, SEV is AMD-V architecture extension and its applicable to > virtualization platform only (we can says BaseMemEncryptSevLib is OVMF > specific). >> You've asserted that it should work (SEV would not be detected) with any Intel processor as well. Therefore, I don't see a good reason that we shouldn't be able to support it in modules that already have >> IA32/X64 specific code. (I'm recalling >> 881813d7a93d9009c873515b043c41c4554779e4.) >> >> Since DxeIpl builds the IA32/X64 page tables, and you need to modify the page tables for this feature (correct?), I think we should try to support the feature there if it is feasible. I can understand the argument that this doesn't apply to all non-VM platforms, so I think we could add a PCD which disables this support by default. >> >> I don't know that the owners of MdeModulePkg and UefiCpuPkg will agree with me though. > > I am flexible to implement APRIORI or Platform hooks Lib. But one thing > I want to highlight is: I'll prefer clearing C-bit through > BaseMemEncryptSevLib functions. One of the main reason for doing so - In > future when we add migration support for the SEV guest then we will be > required to notify the unencrypted page range to hypevisor ( through > hypercall). During migration phase, Hypervisor will use this information > to make decision on whether to invoke the SEV firmware to encrypt the > memory region for transport purposes. If clearing C-bit logic is > contained inside BaseMemEncryptSevLib then it will make life much easier. > >>> In meantime, I have been looking into MdeModule/Core/Dxe/DxeMain to >>> see if I can invoke a platform dependent library to clear C-bit before >>> DxeMain finishes its execution. As Laszlo pointed, current approach is >>> using GCD memory space map to get MMIO and NonExistent entries. I have >>> pushed two patches in my development branch to show what I have been doing: >>> >>> 1) add a new null DxeGcdCorePlatformHookLib >>> >>> https://github.com/codomania/edk2/commit/171f816376b3b0677cbfb90271a94 >>> a920d7ad72d >>> >>> The library provides a function "DxeGcdCorePlatformHookReady" which >>> can be called by DxeMain just after it initializes the GcdServices >>> (which will guarantee that Gcd memory space map is available). >> Regarding hooking into DxeCore, I don't think it is the best approach, but it is better than APRIORI. I wonder if the MdeModulePkg owners could jump in with an opinion. (Hopefully besides just pushing the problem away via APRIORI.) > > Jiewen, any comments ? > >> -Jordan >> >>> 2) override DxeGcdCorePlatformHookLib inside the Ovmf to clear the C-bit when >>> SEV is detected. >>> >>> https://github.com/codomania/edk2/commit/914ce904ca1b7647c966562596ba5 >>> 3c95949f659 >>> >>> I've tested the approach and it seems to work. Is this something >>> aligned with your thinking? >>> >>> >>>> 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