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:[email protected]] > Sent: Tuesday, June 6, 2017 11:51 AM > To: Zeng, Star <[email protected]>; Justen, Jordan L > <[email protected]>; Laszlo Ersek <[email protected]>; > [email protected]; Dong, Eric <[email protected]>; Yao, Jiewen > <[email protected]> > Cc: [email protected]; Gao, Liming <[email protected]>; > [email protected]; Fan, Jeff <[email protected]> > 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 <[email protected]<mailto:[email protected]>>; >> Laszlo Ersek <[email protected]<mailto:[email protected]>>; >> [email protected]<mailto:[email protected]>; Zeng, Star >> <[email protected]<mailto:[email protected]>>; Dong, Eric >> <[email protected]<mailto:[email protected]>> >> Cc: [email protected]<mailto:[email protected]>; Gao, Liming >> <[email protected]<mailto:[email protected]>>; >> [email protected]<mailto:[email protected]>; Yao, Jiewen >> <[email protected]<mailto:[email protected]>>; Fan, Jeff >> <[email protected]<mailto:[email protected]>> >> 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 >>> [email protected]<mailto:[email protected]> >>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
