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

[Brijesh Singh <brijesh.singh@amd.com>]

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>; Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>
> Cc: Thomas.Lendacky@amd.com; Gao, Liming <liming.gao@intel.com>; leo.duran@amd.com; Yao, Jiewen <jiewen.yao@intel.com>; Fan, Jeff <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
>> https://lists.01.org/mailman/listinfo/edk2-devel