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
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>
---
 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h |  28 ++
 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c | 378 +++++++++++++++++++-
 2 files changed, 399 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
index 70cd2187a326..e7b5634b45c1 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
@@ -128,6 +128,20 @@ typedef union {
 
 #define IA32_PG_P                   BIT0
 #define IA32_PG_RW                  BIT1
+#define IA32_PG_PS                  BIT7
+
+#define PAGING_PAE_INDEX_MASK       0x1FF
+
+#define PAGING_4K_ADDRESS_MASK_64   0x000FFFFFFFFFF000ull
+#define PAGING_2M_ADDRESS_MASK_64   0x000FFFFFFFE00000ull
+#define PAGING_1G_ADDRESS_MASK_64   0x000FFFFFC0000000ull
+
+#define PAGING_L1_ADDRESS_SHIFT     12
+#define PAGING_L2_ADDRESS_SHIFT     21
+#define PAGING_L3_ADDRESS_SHIFT     30
+#define PAGING_L4_ADDRESS_SHIFT     39
+
+#define PAGING_PML4E_NUMBER         4
 
 #define PAGETABLE_ENTRY_MASK        ((1UL << 9) - 1)
 #define PML4_OFFSET(x)              ( (x >> 39) & PAGETABLE_ENTRY_MASK)
@@ -136,6 +150,20 @@ typedef union {
 #define PTE_OFFSET(x)               ( (x >> 12) & PAGETABLE_ENTRY_MASK)
 #define PAGING_1G_ADDRESS_MASK_64   0x000FFFFFC0000000ull
 
+#define PAGE_TABLE_POOL_ALIGNMENT   BASE_2MB
+#define PAGE_TABLE_POOL_UNIT_SIZE   SIZE_2MB
+#define PAGE_TABLE_POOL_UNIT_PAGES  EFI_SIZE_TO_PAGES (PAGE_TABLE_POOL_UNIT_SIZE)
+#define PAGE_TABLE_POOL_ALIGN_MASK  \
+  (~(EFI_PHYSICAL_ADDRESS)(PAGE_TABLE_POOL_ALIGNMENT - 1))
+
+typedef struct {
+  VOID            *NextPool;
+  UINTN           Offset;
+  UINTN           FreePages;
+} PAGE_TABLE_POOL;
+
+
+
 /**
   This function clears memory encryption bit for the memory region specified by PhysicalAddress
   and length from the current page table context.
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
index e1e705c34626..4185874c99b8 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
@@ -25,6 +25,7 @@ Code is derived from MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
 
 STATIC BOOLEAN mAddressEncMaskChecked = FALSE;
 STATIC UINT64  mAddressEncMask;
+STATIC PAGE_TABLE_POOL   *mPageTablePool = NULL;
 
 typedef enum {
    SetCBit,
@@ -63,6 +64,123 @@ GetMemEncryptionAddressMask (
 }
 
 /**
+  Initialize a buffer pool for page table use only.
+
+  To reduce the potential split operation on page table, the pages reserved for
+  page table should be allocated in the times of PAGE_TABLE_POOL_UNIT_PAGES and
+  at the boundary of PAGE_TABLE_POOL_ALIGNMENT. So the page pool is always
+  initialized with number of pages greater than or equal to the given PoolPages.
+
+  Once the pages in the pool are used up, this method should be called again to
+  reserve at least another PAGE_TABLE_POOL_UNIT_PAGES. Usually this won't happen
+  often in practice.
+
+  @param[in] PoolPages      The least page number of the pool to be created.
+
+  @retval TRUE    The pool is initialized successfully.
+  @retval FALSE   The memory is out of resource.
+**/
+STATIC
+BOOLEAN
+InitializePageTablePool (
+  IN  UINTN                           PoolPages
+  )
+{
+  VOID                      *Buffer;
+
+  //
+  // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
+  // header.
+  //
+  PoolPages += 1;   // Add one page for header.
+  PoolPages = ((PoolPages - 1) / PAGE_TABLE_POOL_UNIT_PAGES + 1) *
+              PAGE_TABLE_POOL_UNIT_PAGES;
+  Buffer = AllocateAlignedPages (PoolPages, PAGE_TABLE_POOL_ALIGNMENT);
+  if (Buffer == NULL) {
+    DEBUG ((DEBUG_ERROR, "ERROR: Out of aligned pages\r\n"));
+    return FALSE;
+  }
+
+  //
+  // Link all pools into a list for easier track later.
+  //
+  if (mPageTablePool == NULL) {
+    mPageTablePool = Buffer;
+    mPageTablePool->NextPool = mPageTablePool;
+  } else {
+    ((PAGE_TABLE_POOL *)Buffer)->NextPool = mPageTablePool->NextPool;
+    mPageTablePool->NextPool = Buffer;
+    mPageTablePool = Buffer;
+  }
+
+  //
+  // Reserve one page for pool header.
+  //
+  mPageTablePool->FreePages  = PoolPages - 1;
+  mPageTablePool->Offset = EFI_PAGES_TO_SIZE (1);
+
+  return TRUE;
+}
+
+/**
+  This API provides a way to allocate memory for page table.
+
+  This API can be called more than once to allocate memory for page tables.
+
+  Allocates the number of 4KB pages and returns a pointer to the allocated
+  buffer. The buffer returned is aligned on a 4KB boundary.
+
+  If Pages is 0, then NULL is returned.
+  If there is not enough memory remaining to satisfy the request, then NULL is
+  returned.
+
+  @param  Pages                 The number of 4 KB pages to allocate.
+
+  @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+STATIC
+VOID *
+EFIAPI
+AllocatePageTableMemory (
+  IN UINTN           Pages
+  )
+{
+  VOID                            *Buffer;
+
+  if (Pages == 0) {
+    return NULL;
+  }
+
+  //
+  // Renew the pool if necessary.
+  //
+  if (mPageTablePool == NULL ||
+      Pages > mPageTablePool->FreePages) {
+    if (!InitializePageTablePool (Pages)) {
+      return NULL;
+    }
+  }
+
+  Buffer = (UINT8 *)mPageTablePool + mPageTablePool->Offset;
+
+  mPageTablePool->Offset     += EFI_PAGES_TO_SIZE (Pages);
+  mPageTablePool->FreePages  -= Pages;
+
+  DEBUG ((
+    DEBUG_VERBOSE,
+    "%a:%a: Buffer=0x%Lx Pages=%ld\n",
+    gEfiCallerBaseName,
+    __FUNCTION__,
+    Buffer,
+    Pages
+    ));
+
+  return Buffer;
+}
+
+
+/**
   Split 2M page to 4K.
 
   @param[in]      PhysicalAddress       Start physical address the 2M page covered.
@@ -85,7 +203,7 @@ Split2MPageTo4K (
   PAGE_TABLE_4K_ENTRY               *PageTableEntry, *PageTableEntry1;
   UINT64                            AddressEncMask;
 
-  PageTableEntry = AllocatePages(1);
+  PageTableEntry = AllocatePageTableMemory(1);
 
   PageTableEntry1 = PageTableEntry;
 
@@ -117,6 +235,179 @@ Split2MPageTo4K (
 }
 
 /**
+  Set one page of page table pool memory to be read-only.
+
+  @param[in] PageTableBase    Base address of page table (CR3).
+  @param[in] Address          Start address of a page to be set as read-only.
+  @param[in] Level4Paging     Level 4 paging flag.
+
+**/
+STATIC
+VOID
+SetPageTablePoolReadOnly (
+  IN  UINTN                             PageTableBase,
+  IN  EFI_PHYSICAL_ADDRESS              Address,
+  IN  BOOLEAN                           Level4Paging
+  )
+{
+  UINTN                 Index;
+  UINTN                 EntryIndex;
+  UINT64                AddressEncMask;
+  EFI_PHYSICAL_ADDRESS  PhysicalAddress;
+  UINT64                *PageTable;
+  UINT64                *NewPageTable;
+  UINT64                PageAttr;
+  UINT64                LevelSize[5];
+  UINT64                LevelMask[5];
+  UINTN                 LevelShift[5];
+  UINTN                 Level;
+  UINT64                PoolUnitSize;
+
+  ASSERT (PageTableBase != 0);
+
+  //
+  // Since the page table is always from page table pool, which is always
+  // located at the boundary of PcdPageTablePoolAlignment, we just need to
+  // set the whole pool unit to be read-only.
+  //
+  Address = Address & PAGE_TABLE_POOL_ALIGN_MASK;
+
+  LevelShift[1] = PAGING_L1_ADDRESS_SHIFT;
+  LevelShift[2] = PAGING_L2_ADDRESS_SHIFT;
+  LevelShift[3] = PAGING_L3_ADDRESS_SHIFT;
+  LevelShift[4] = PAGING_L4_ADDRESS_SHIFT;
+
+  LevelMask[1] = PAGING_4K_ADDRESS_MASK_64;
+  LevelMask[2] = PAGING_2M_ADDRESS_MASK_64;
+  LevelMask[3] = PAGING_1G_ADDRESS_MASK_64;
+  LevelMask[4] = PAGING_1G_ADDRESS_MASK_64;
+
+  LevelSize[1] = SIZE_4KB;
+  LevelSize[2] = SIZE_2MB;
+  LevelSize[3] = SIZE_1GB;
+  LevelSize[4] = SIZE_512GB;
+
+  AddressEncMask  = GetMemEncryptionAddressMask() &
+                    PAGING_1G_ADDRESS_MASK_64;
+  PageTable       = (UINT64 *)(UINTN)PageTableBase;
+  PoolUnitSize    = PAGE_TABLE_POOL_UNIT_SIZE;
+
+  for (Level = (Level4Paging) ? 4 : 3; Level > 0; --Level) {
+    Index = ((UINTN)RShiftU64 (Address, LevelShift[Level]));
+    Index &= PAGING_PAE_INDEX_MASK;
+
+    PageAttr = PageTable[Index];
+    if ((PageAttr & IA32_PG_PS) == 0) {
+      //
+      // Go to next level of table.
+      //
+      PageTable = (UINT64 *)(UINTN)(PageAttr & ~AddressEncMask &
+                                    PAGING_4K_ADDRESS_MASK_64);
+      continue;
+    }
+
+    if (PoolUnitSize >= LevelSize[Level]) {
+      //
+      // Clear R/W bit if current page granularity is not larger than pool unit
+      // size.
+      //
+      if ((PageAttr & IA32_PG_RW) != 0) {
+        while (PoolUnitSize > 0) {
+          //
+          // PAGE_TABLE_POOL_UNIT_SIZE and PAGE_TABLE_POOL_ALIGNMENT are fit in
+          // one page (2MB). Then we don't need to update attributes for pages
+          // crossing page directory. ASSERT below is for that purpose.
+          //
+          ASSERT (Index < EFI_PAGE_SIZE/sizeof (UINT64));
+
+          PageTable[Index] &= ~(UINT64)IA32_PG_RW;
+          PoolUnitSize    -= LevelSize[Level];
+
+          ++Index;
+        }
+      }
+
+      break;
+
+    } else {
+      //
+      // The smaller granularity of page must be needed.
+      //
+      ASSERT (Level > 1);
+
+      NewPageTable = AllocatePageTableMemory (1);
+      ASSERT (NewPageTable != NULL);
+
+      PhysicalAddress = PageAttr & LevelMask[Level];
+      for (EntryIndex = 0;
+            EntryIndex < EFI_PAGE_SIZE/sizeof (UINT64);
+            ++EntryIndex) {
+        NewPageTable[EntryIndex] = PhysicalAddress  | AddressEncMask |
+                                   IA32_PG_P | IA32_PG_RW;
+        if (Level > 2) {
+          NewPageTable[EntryIndex] |= IA32_PG_PS;
+        }
+        PhysicalAddress += LevelSize[Level - 1];
+      }
+
+      PageTable[Index] = (UINT64)(UINTN)NewPageTable | AddressEncMask |
+                                        IA32_PG_P | IA32_PG_RW;
+      PageTable = NewPageTable;
+    }
+  }
+}
+
+/**
+  Prevent the memory pages used for page table from been overwritten.
+
+  @param[in] PageTableBase    Base address of page table (CR3).
+  @param[in] Level4Paging     Level 4 paging flag.
+
+**/
+STATIC
+VOID
+EnablePageTableProtection (
+  IN  UINTN     PageTableBase,
+  IN  BOOLEAN   Level4Paging
+  )
+{
+  PAGE_TABLE_POOL         *HeadPool;
+  PAGE_TABLE_POOL         *Pool;
+  UINT64                  PoolSize;
+  EFI_PHYSICAL_ADDRESS    Address;
+
+  if (mPageTablePool == NULL) {
+    return;
+  }
+
+  //
+  // SetPageTablePoolReadOnly might update mPageTablePool. It's safer to
+  // remember original one in advance.
+  //
+  HeadPool = mPageTablePool;
+  Pool = HeadPool;
+  do {
+    Address  = (EFI_PHYSICAL_ADDRESS)(UINTN)Pool;
+    PoolSize = Pool->Offset + EFI_PAGES_TO_SIZE (Pool->FreePages);
+
+    //
+    // The size of one pool must be multiple of PAGE_TABLE_POOL_UNIT_SIZE, which
+    // is one of page size of the processor (2MB by default). Let's apply the
+    // protection to them one by one.
+    //
+    while (PoolSize > 0) {
+      SetPageTablePoolReadOnly(PageTableBase, Address, Level4Paging);
+      Address   += PAGE_TABLE_POOL_UNIT_SIZE;
+      PoolSize  -= PAGE_TABLE_POOL_UNIT_SIZE;
+    }
+
+    Pool = Pool->NextPool;
+  } while (Pool != HeadPool);
+
+}
+
+
+/**
   Split 1G page to 2M.
 
   @param[in]      PhysicalAddress       Start physical address the 1G page covered.
@@ -139,7 +430,7 @@ Split1GPageTo2M (
   PAGE_TABLE_ENTRY                  *PageDirectoryEntry;
   UINT64                            AddressEncMask;
 
-  PageDirectoryEntry = AllocatePages(1);
+  PageDirectoryEntry = AllocatePageTableMemory(1);
 
   AddressEncMask = GetMemEncryptionAddressMask ();
   ASSERT (PageDirectoryEntry != NULL);
@@ -195,6 +486,47 @@ SetOrClearCBit(
 }
 
 /**
+ Check the WP status in CR0 register. This bit is used to lock or unlock write
+ access to pages marked as read-only.
+
+  @retval TRUE    Write protection is enabled.
+  @retval FALSE   Write protection is disabled.
+**/
+STATIC
+BOOLEAN
+IsReadOnlyPageWriteProtected (
+  VOID
+  )
+{
+  return ((AsmReadCr0 () & BIT16) != 0);
+}
+
+
+/**
+ Disable Write Protect on pages marked as read-only.
+**/
+STATIC
+VOID
+DisableReadOnlyPageWriteProtect (
+  VOID
+  )
+{
+  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
+}
+
+/**
+ Enable Write Protect on pages marked as read-only.
+**/
+VOID
+EnableReadOnlyPageWriteProtect (
+  VOID
+  )
+{
+  AsmWriteCr0 (AsmReadCr0() | BIT16);
+}
+
+
+/**
   This function either sets or clears memory encryption bit for the memory region
   specified by PhysicalAddress and length from the current page table context.
 
@@ -238,6 +570,8 @@ SetMemoryEncDec (
   PAGE_TABLE_4K_ENTRY            *PageTableEntry;
   UINT64                         PgTableMask;
   UINT64                         AddressEncMask;
+  BOOLEAN                        IsWpEnabled;
+  RETURN_STATUS                  Status;
 
   DEBUG ((
     DEBUG_VERBOSE,
@@ -274,6 +608,16 @@ SetMemoryEncDec (
     WriteBackInvalidateDataCacheRange((VOID*) (UINTN)PhysicalAddress, Length);
   }
 
+  //
+  // Make sure that the page table is changeable.
+  //
+  IsWpEnabled = IsReadOnlyPageWriteProtected ();
+  if (IsWpEnabled) {
+    DisableReadOnlyPageWriteProtect ();
+  }
+
+  Status = EFI_SUCCESS;
+
   while (Length)
   {
     //
@@ -293,7 +637,8 @@ SetMemoryEncDec (
         __FUNCTION__,
         PhysicalAddress
         ));
-      return RETURN_NO_MAPPING;
+      Status = RETURN_NO_MAPPING;
+      goto Done;
     }
 
     PageDirectory1GEntry = (VOID*) ((PageMapLevel4Entry->Bits.PageTableBaseAddress<<12) & ~PgTableMask);
@@ -306,7 +651,8 @@ SetMemoryEncDec (
         __FUNCTION__,
         PhysicalAddress
         ));
-      return RETURN_NO_MAPPING;
+      Status = RETURN_NO_MAPPING;
+      goto Done;
     }
 
     //
@@ -357,7 +703,8 @@ SetMemoryEncDec (
           __FUNCTION__,
           PhysicalAddress
           ));
-        return RETURN_NO_MAPPING;
+        Status = RETURN_NO_MAPPING;
+        goto Done;
       }
       //
       // If the MustBe1 bit is not a 1, it's not a 2MB entry
@@ -397,7 +744,8 @@ SetMemoryEncDec (
             __FUNCTION__,
             PhysicalAddress
             ));
-          return RETURN_NO_MAPPING;
+          Status = RETURN_NO_MAPPING;
+          goto Done;
         }
         SetOrClearCBit (&PageTableEntry->Uint64, Mode);
         PhysicalAddress += EFI_PAGE_SIZE;
@@ -407,11 +755,27 @@ SetMemoryEncDec (
   }
 
   //
+  // Protect the page table by marking the memory used for page table to be
+  // read-only.
+  //
+  if (IsWpEnabled) {
+    EnablePageTableProtection ((UINTN)PageMapLevel4Entry, TRUE);
+  }
+
+  //
   // Flush TLB
   //
   CpuFlushTlb();
 
-  return RETURN_SUCCESS;
+Done:
+  //
+  // Restore page table write protection, if any.
+  //
+  if (IsWpEnabled) {
+    EnableReadOnlyPageWriteProtect ();
+  }
+
+  return Status;
 }
 
 /**
-- 
2.9.5

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi Brijesh,

(Adding Ray, based on Jian's and Ray's feedback in another branch of
this thread.)

This time I prefer to write a shorter email:

(1) First of all, congrats on your family :)

(2) Please file a new TianoCore BZ:
- for UefiCpuPkg (for the time being),
- with the title "create page table library to abstract all page
  table manipulations",
- please assign it to Ray,
- please CC Jian and myself on it,
- please add the edk2-devel archive URL of this email thread to the BZ
- I wonder if the earlier-filed TianoCore BZ
  <https://bugzilla.tianocore.org/show_bug.cgi?id=623> should be
  associated with this new BZ somehow; e.g. in the "See Also" field.

(3) Please resubmit the patch with an updated commit message:
- the "Contributed-under" line is something that I specifically cannot /
  must not add to the commit message myself; it must come from you;
- please reference the TianoCore BZ from point (2) in the commit
  message, as expected future cleanup / refactoring;
- please do the other commit message modifications that I requested.

(4) Because Ray intends to work on the library later, I agree we
shouldn't allow SEV support to remain broken until then. So, I will ACK
and push your (upcoming) v2 patch.

(5) Adding SEV testing steps to the TianoCore Wiki, once the QEMU/KVM
patches are upstream, would be very helpful -- a great idea. I'd like
you to do that, because that could be the basis for SEV test scenarios
in avocado-vt.

The contribution process to the TianoCore Wiki hasn't been fleshed out
yet, generally speaking, to my knowledge. As far as I recall, I asked
Jordan about it last February, and he made really good points. I'll
follow up on that (off-list for now).
Thanks!
Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi Laszlo,

On 01/05/2018 05:38 AM, Laszlo Ersek wrote:
> Hi Brijesh,
> 
> (Adding Ray, based on Jian's and Ray's feedback in another branch of
> this thread.)
> 
> This time I prefer to write a shorter email:
> 
> (1) First of all, congrats on your family :)

Thank you :)

> 
> (2) Please file a new TianoCore BZ:
> - for UefiCpuPkg (for the time being),
> - with the title "create page table library to abstract all page
>    table manipulations",
> - please assign it to Ray,
> - please CC Jian and myself on it,
> - please add the edk2-devel archive URL of this email thread to the BZ
> - I wonder if the earlier-filed TianoCore BZ
>    <https://bugzilla.tianocore.org/show_bug.cgi?id=623> should be
>    associated with this new BZ somehow; e.g. in the "See Also" field.
> 

I will create a new BZ and resubmit the patch soon. thanks
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel