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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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.

Thank you
Yao Jiewen

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