Re: [PATCH v6 00/17] x86: Secure Encrypted Virtualization (AMD)

["Duran, Leo" <leo.duran@amd.com>]

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:lersek@redhat.com]
> Sent: Tuesday, June 06, 2017 1:30 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Singh, Brijesh
> <brijesh.singh@amd.com>; Zeng, Star <star.zeng@intel.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Dong, Eric
> <eric.dong@intel.com>
> Cc: Lendacky, Thomas <Thomas.Lendacky@amd.com>; Gao, Liming
> <liming.gao@intel.com>; Duran, Leo <leo.duran@amd.com>; Fan, Jeff
> <jeff.fan@intel.com>
> 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:brijesh.singh@amd.com]
> > Sent: Tuesday, June 6, 2017 11:51 AM
> > To: Zeng, Star <star.zeng@intel.com>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>
> > Cc: Thomas.Lendacky@amd.com; Gao, Liming <liming.gao@intel.com>;
> > leo.duran@amd.com; Fan, Jeff <jeff.fan@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.singh@amd.com<mailto:brijesh.singh@amd.com>>; Laszlo Ersek
> >> <lersek@redhat.com<mailto:lersek@redhat.com>>;
> >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Zeng, Star
> >> <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Dong, Eric
> >> <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> >> Cc: Thomas.Lendacky@amd.com<mailto:Thomas.Lendacky@amd.com>;
> Gao,
> >> Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>;
> >> leo.duran@amd.com<mailto:leo.duran@amd.com>; Yao, Jiewen
> >> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Fan, Jeff
> >> <jeff.fan@intel.com<mailto:jeff.fan@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/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
> >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> >>> https://lists.01.org/mailman/listinfo/edk2-devel