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]>; Laszlo Ersek <[email protected]>; > [email protected]; Zeng, Star <[email protected]>; Dong, Eric > <[email protected]> > Cc: [email protected]; Gao, Liming <[email protected]>; > [email protected]; Yao, Jiewen <[email protected]>; Fan, Jeff > <[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] >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
