Lazlo, et al, Calling back from DxeIpl after page tables are created seems like a major hack: - Duplicates logic already in GCD - Would need to pass the page-table root pointer
So, as stated earlier, I'm favor of: using APRIORI in the interim, and reviewing GCD to support a more correct framework. Leo. > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Tuesday, June 06, 2017 1:30 PM > To: Yao, Jiewen <[email protected]>; Singh, Brijesh > <[email protected]>; Zeng, Star <[email protected]>; Justen, > Jordan L <[email protected]>; [email protected]; Dong, Eric > <[email protected]> > Cc: Lendacky, Thomas <[email protected]>; Gao, Liming > <[email protected]>; Duran, Leo <[email protected]>; Fan, Jeff > <[email protected]> > Subject: Re: [edk2] [PATCH v6 00/17] x86: Secure Encrypted Virtualization > (AMD) > > 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/171f816376b3b0677cbfb90271 > a > >>> 94 > >>> 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/914ce904ca1b7647c966562596 > b > >>> a5 > >>> 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
