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

[Laszlo Ersek <lersek@redhat.com>]

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/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>
>>> https://lists.01.org/mailman/listinfo/edk2-devel