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:ler...@redhat.com] > Sent: Friday, January 05, 2018 3:07 AM > To: Brijesh Singh <brijesh.si...@amd.com>; edk2-devel@lists.01.org > Cc: Tom Lendacky <thomas.lenda...@amd.com>; Wang, Jian J > <jian.j.w...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Justen, Jordan > L <jordan.l.jus...@intel.com>; Ard Biesheuvel <ard.biesheu...@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.w...@intel.com> > > Cc: Jiewen Yao <jiewen....@intel.com> > > Cc: Jordan Justen <jordan.l.jus...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Signed-off-by: Brijesh Singh <brijesh.si...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
