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

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

Hi Laszlo,


On 01/04/2018 01:07 PM, Laszlo Ersek wrote:
> 
> meta comment: please also CC Ard on OvmfPkg patches; he too co-maintains
> OvmfPkg.

I will keep that in mind and  include Ard on all OvmfPkg patches.

> 
> The following line is missing, from above your S-o-b:
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> 

Ah, if v2 needed then I will fix that in commit.

>> ---
>>   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.


I am also against this duplication and I did silently hinted out this 
issue while adding the memory encryption PCD and BaseMemEncryptSevLib 
support. I must admit that I am new to EDKII world and don't have much 
history to strongly say that the page table creation or split code 
duplication was actually for a good reason or it was just an easy way to 
get the things done. As you say there is a good possibility that things 
will need to change more as Jian does more security related fixes in 
core packages.


> 
> 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. 


It is totally understandable, I do have a cron job which pulls OVMF 
every week and runs SEV test but I was on paternity leave in Dec hence 
was not able to flag this issue sooner. I think after SEV patches gets 
accepted in Qemu/KVM then we should be able to expand your smoke test to 
cover some SEV specific test.


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.

It will be awesome. We will add some SEV specific test when this becomes 
online.


> 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.
> 

Agreed.

> Do we really need two definitions of the macro PAGING_L4_ADDRESS_SHIFT?
> 

Since the macro is defined in MdeModulePkg's local header 
(MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h) and I was not able to 
include the header outside the MdeModulePkg and have to redefine in 
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h


> 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.
> 

Totally agree, may be the right thing to do is to create a Library 
(either in MdeModulePkg or UefiCpuPkg). The library should provide the 
routines to create and manipulate the page table all in one place. Any 
packages wanting to use the services can simply call the function.


> 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).
> 

Can we please do #a.

I don't know how much core package maintainer want to cleanup the code 
duplication, I don't mind opening a bugzilla talking about this page 
table creation/manipulation code duplication.

Thanks