On 06/06/17 17:43, Yao, Jiewen wrote: > Hi Andrew > Yes, I agree. If we could figure out a cleaner way to resolve the problem, we > should use the cleaner way. > > > > If we really really do not want to use a priori for AmdSec, we can > let AmdSec to publish a special protocol, and let the driver depend > that protocol, if this driver need add MMIO region. Because AmdSec is > inside of OvmfPkg, this special protocol can be in OvmfPkg. > > That is just another option. We considered this approach before, and it is something that -- for a change :) -- *I* happen to dislike.
My issue with this is that it doesn't scale. Assume that in a few months we'd like to pull another driver from MdeModulePkg into the OVMF binary, and that QEMU implemented emulation for that device. In virtual machines without SEV, the driver could just work (we might have to set a few PCDs for it, but that would be all). For making the driver work in SEV guests however, we'd have to clone the driver to OvmfPkg, and add code to the driver to explicitly use the special protocol. Not good. :( Your suggestion is actually a variant of the IOMMU protocol. The IOMMU protocol is different however, because PCI physical address space and CPU physical address space are *already* different, and for this mapping to work, all drivers that use PCI DMA must *already* use the IOMMU protocol, even if they run on physical hardware, and even if they live under MdeModulePkg. Therefore if we provide a SEV-specific IOMMU driver in OvmfPkg, those PCI DMA drivers in MdeModulePkg will automatically start using it; no change needed. However, any drivers under MdeModulePkg (current and future) that use MMIO directly, without going through PCI DMA, would have to be cloned and modified in OvmfPkg. Thanks Laszlo > > Thank you > Yao Jiewen > > From: af...@apple.com [mailto:af...@apple.com] > Sent: Tuesday, June 6, 2017 11:25 PM > To: Yao, Jiewen <jiewen....@intel.com> > Cc: Brijesh Singh <brijesh.si...@amd.com>; Zeng, Star <star.z...@intel.com>; > Justen, Jordan L <jordan.l.jus...@intel.com>; Laszlo Ersek > <ler...@redhat.com>; edk2-devel@lists.01.org; Dong, Eric > <eric.d...@intel.com>; thomas.lenda...@amd.com; leo.du...@amd.com; Fan, Jeff > <jeff....@intel.com>; Gao, Liming <liming....@intel.com> > Subject: Re: [edk2] [PATCH v6 00/17] x86: Secure Encrypted Virtualization > (AMD) > > >> On Jun 6, 2017, at 7:54 AM, Yao, Jiewen >> <jiewen....@intel.com<mailto:jiewen....@intel.com>> 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. >> >> 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. >> > > Jiewen, > > I agree that "a priori" is part of the architecture so it is OK to use it, > but "a priori" was never really intended as a way to add basic features. it > was more for debugging and work arounds. It seems like a feature like this > should not require a work around.... > > 'So I think it is OK to accept this patch to get the feature enabled, but we > need to look at the GCD implementation and PI architecture to figure out why > there is not a cleaner way to add this feature. Maybe we need to change the > implementation, and/or the PI Spec? > > Thanks, > > Andrew Fish > >> Thank you >> Yao Jiewen >> >> >> >> From: Brijesh Singh [mailto:brijesh.si...@amd.com] >> Sent: Tuesday, June 6, 2017 11:51 AM >> To: Zeng, Star <star.z...@intel.com<mailto:star.z...@intel.com>>; Justen, >> Jordan L <jordan.l.jus...@intel.com<mailto:jordan.l.jus...@intel.com>>; >> Laszlo Ersek <ler...@redhat.com<mailto:ler...@redhat.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Dong, Eric >> <eric.d...@intel.com<mailto:eric.d...@intel.com>>; Yao, Jiewen >> <jiewen....@intel.com<mailto:jiewen....@intel.com>> >> Cc: thomas.lenda...@amd.com<mailto:thomas.lenda...@amd.com>; Gao, Liming >> <liming....@intel.com<mailto:liming....@intel.com>>; >> leo.du...@amd.com<mailto:leo.du...@amd.com>; Fan, Jeff >> <jeff....@intel.com<mailto:jeff....@intel.com>> >> 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 >>> <brijesh.si...@amd.com<mailto:brijesh.si...@amd.com<mailto:brijesh.si...@amd.com%3cmailto:brijesh.si...@amd.com>>>; >>> Laszlo Ersek >>> <ler...@redhat.com<mailto:ler...@redhat.com<mailto:ler...@redhat.com%3cmailto:ler...@redhat.com>>>; >>> >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>; >>> Zeng, Star >>> <star.z...@intel.com<mailto:star.z...@intel.com<mailto:star.z...@intel.com%3cmailto:star.z...@intel.com>>>; >>> Dong, Eric >>> <eric.d...@intel.com<mailto:eric.d...@intel.com<mailto:eric.d...@intel.com%3cmailto:eric.d...@intel.com>>> >>> Cc: >>> thomas.lenda...@amd.com<mailto:thomas.lenda...@amd.com<mailto:thomas.lenda...@amd.com%3cmailto:thomas.lenda...@amd.com>>; >>> Gao, Liming >>> <liming....@intel.com<mailto:liming....@intel.com<mailto:liming....@intel.com%3cmailto:liming....@intel.com>>>; >>> >>> leo.du...@amd.com<mailto:leo.du...@amd.com<mailto:leo.du...@amd.com%3cmailto:leo.du...@amd.com>>; >>> Yao, Jiewen >>> <jiewen....@intel.com<mailto:jiewen....@intel.com<mailto:jiewen....@intel.com%3cmailto:jiewen....@intel.com>>>; >>> Fan, Jeff >>> <jeff....@intel.com<mailto:jeff....@intel.com<mailto:jeff....@intel.com%3cmailto:jeff....@intel.com>>> >>> 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<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>> >>>> https://lists.01.org/mailman/listinfo/edk2-devel >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org<mailto: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
