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

["Wang, Jian J" <jian.j.wang@intel.com>]

Add Cc to Ruiyu, who has plan to consolidate page table manipulation method.
He may want to share more information.

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