Re: [PATCH] OvmfPkg/BaseMemEncryptSevLib: Enable protection for newly added page table

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 1/5/2018 9:10 AM, Wang, Jian J wrote:
> Add Cc to Ruiyu, who has plan to consolidate page table manipulation method.
> He may want to share more information.

Yes I do have a plan to create a page table library to abstract all page
table manipulations.
I will share more details in a separate mail, but maybe in middle or end
of this quarter.

> 
> Regards,
> Jian
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, January 05, 2018 3:07 AM
>> To: Brijesh Singh <brijesh.singh@amd.com>; edk2-devel@lists.01.org
>> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan
>> L <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [PATCH] OvmfPkg/BaseMemEncryptSevLib: Enable protection for
>> newly added page table
>>
>> Hi Brijesh,
>>
>> meta comment: please also CC Ard on OvmfPkg patches; he too co-maintains
>> OvmfPkg.
>>
>> More below:
>>
>> On 01/04/18 18:06, Brijesh Singh wrote:
>>> Commit 2ac1730bf2a5 (MdeModulePkg/DxeIpl: Mark page table as read-only)
>>> sets the memory pages used for page table as read-only after paging is
>>> setup and sets CR0.WP to protect CPU modifying the read-only pages.
>>> The commit causes #PF when MemEncryptSevClearPageEncMask() or
>>> MemEncryptSevSetPageEncMask() tries to change the page-table attributes.
>>>
>>> This patch takes the similar approach as Commit 147fd35c3e38
>>> (UefiCpuPkg/CpuDxe: Enable protection for newly added page table).
>>> When page table protection is enabled, we disable it temporarily before
>>> changing the page table attributes.
>>>
>>> This patch makes use of the same approach as Commit 2ac1730bf2a5
>>> (MdeModulePkg/DxeIpl: Mark page table as read-only)) for allocating
>>> page table memory from reserved memory pool, which helps to reduce a
>>
>> "reserved memory pool" is a misleading term, it invokes thoughts about
>> EfiReservedMemoryType, which is also allocatable.
>>
>>> potential "split" operation.
>>>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>
>> The following line is missing, from above your S-o-b:
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>>
>>> ---
>>>   OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h |  28 ++
>>>   OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c | 378
>> +++++++++++++++++++-
>>>   2 files changed, 399 insertions(+), 7 deletions(-)
>>
>> I find it awful that we have to duplicate all this code in OvmfPkg.
>>
>> The page protection (+ other security) features have been constantly
>> refined since Jian started to work on them. There's no guarantee that
>> this is the last synchronization we have to do in OvmfPkg due to another
>> feature (or bugfix) under UefiCpuPkg.
>>
>> You can see in the messages of both commits 2ac1730bf2a5 and
>> 147fd35c3e38 that I participated in the regression-testing of those commits.
>>
>> You can also see on the commit messages that I simply ran out of steam
>> while trying to keep up with rapid iterations of those patches -- I
>> regression-tested versions that I thought would be final, but even after
>> that, further updates were made, and I couldn't test the final versions.
>>
>> Even in those regression tests that I managed to do, I didn't test the
>> patches in a SEV guest. The reason is that the test matrix has now grown
>> to an unmanageable size, such that sometimes it doesn't even occur to me
>> that this or that virt environment could be affected by a UefiCpuPkg or
>> Mde*Pkg patch. I realized the risk, which is why I asked for, and got,
>> Reviewer role under UefiCpuPkg -- purely so I could *test* (not
>> necessarily review!) UefiCpuPkg patches first-hand, *before* they are
>> committed. But, I'm at (and beyond) capacity, even in recognizing what
>> affects what.
>>
>> There are only two fixes for this (they are independent, and both should
>> be done):
>>
>> (1) An automated regression test environment. We discussed this earlier
>> with Jian (because his work had both introduced, and elsewhere exposed,
>> a large number of bugs). After that, I also talked to virt-QE at Red
>> Hat. Hopefully virt-QE @RH will find some resources in February 2018 to
>> write OVMF test cases for the avocado-vt project. I'm not sure.
>> Currently, *zero* OVMF test cases exist in any automated test
>> environment that I'm aware of; and I have spent a frightening amount of
>> time on manual regression testing.
>>
>> (2) Code sharing / reuse between top-level Pkgs in edk2, as diligently
>> as possible. I very much dislike that we have page table management
>> scattered between MdeModulePkg/Core/DxeIplPeim, UefiCpuPkg/CpuDxe, and
>> OvmfPkg. If there is a well-defined security feature, namely "protect
>> page tables by making them read-only", then the core feature had better
>> provide the toolkit for *all* relevant modules to reuse.
>>
>> Do we really need two definitions of the macro PAGING_L4_ADDRESS_SHIFT?
>>
>> Do we really need *three* definitions of the macro IA32_PG_RW?
>>
>> - MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
>> - OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
>> - UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>
>> This is just sloppy work, mindless code duplication.
>>
>> I'm not willing to review ~400 lines of page table manipulation in
>> detail that admittedly duplicates UefiCpuPkg code, from commit
>> 147fd35c3e38. Sorry, I don't scale to that level.
>>
>>
>> Here's what we can do:
>>
>> (a) I can ACK this patch, and push it, without looking at more than just
>> the commit message and the diffstat.
>>
>> (b) Or else, you and Jian could collaborate on factoring out the shared
>> bits to new Include/IndustryStandard headers, Include/Library class
>> headers, and Library instances. (UefiCpuPkg can consume Mde*Pkg, and
>> OvmfPkg can consume all of those.) After that, we might not need any
>> such OvmfPkg patches in the first place, or if we did, I could
>> reasonably find the time to look at them.
>>
>> I totally don't request that library interfaces and industry standard
>> macros be added to public headers *upfront*, before *multiple* consumers
>> appear. I don't believe in "design by committee".
>>
>> However, I do subscribe to interface extraction when organic project
>> growth results in multiple uses of the same pattern.
>>
>> (c) Obviously, other OvmfPkg maintainers are welcome to review the patch
>> for you!
>>
>>
>> NB, it is not lost on me that previous edk2 practice has actively
>> discouraged feature normalization, on occasion. The rejection of the
>> IoFifoLib class was a prime example, and I disagree with that decision
>> -- including the resultant duplication of the new IoFifo* functions to
>> all IoLib instances -- to this day.
>>
>> Another utility function we sorely miss is scanning PCI config space for
>> a given capability that has known size constraints. So we have
>> open-coded such scanning at least thrice.
>>
>> This dire situation is not helped by the fact that upstreaming features
>> to core packages, such as MdeModulePkg and UefiCpuPkg, is *incredibly*
>> hard. *Way* harder than it should be. So I don't "blame" you for
>> starting with the easier way (I have followed that path myself in the
>> past, several times, out of desperation), but as a reviewer /
>> co-maintainer, I simply cannot keep up with this level of code
>> duplication, and the consequent (predictable) churn in the future.
>>
>>
>> Please pick (a), (b) or (c).
>>
>> Thanks
>> Laszlo


-- 
Thanks,
Ray