[Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Liming Gao]

From: Mike Turner <miketur@microsoft.com>

The Fpdt driver (FirmwarePerformanceDxe) saves a memory address across
reboots, and then does an AllocatePage for that memory address.
If, on this boot, that memory comes from a Runtime memory bucket,
the MAT table is not updated. This causes Windows to boot into Recovery.

This patch blocks the memory manager from changing the page
from a special bucket to a different memory type.  Once the buckets are
allocated, we freeze the memory ranges for the OS, and fragmenting
the special buckets will cause errors resuming from hibernate.

This patch is cherry pick from Project Mu:
https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af94016ebd391ea6f340920735a
With the minor change,
1. Update commit message format to keep the message in 80 characters one line.
2. Remove // MU_CHANGE comments in source code.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
 MdeModulePkg/Core/Dxe/Mem/Page.c | 43 ++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index bd9e116aa5..637518889d 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1265,12 +1265,13 @@ CoreInternalAllocatePages (
   IN BOOLEAN                NeedGuard
   )
 {
-  EFI_STATUS      Status;
-  UINT64          Start;
-  UINT64          NumberOfBytes;
-  UINT64          End;
-  UINT64          MaxAddress;
-  UINTN           Alignment;
+  EFI_STATUS       Status;
+  UINT64           Start;
+  UINT64           NumberOfBytes;
+  UINT64           End;
+  UINT64           MaxAddress;
+  UINTN            Alignment;
+  EFI_MEMORY_TYPE  CheckType;
 
   if ((UINT32)Type >= MaxAllocateType) {
     return EFI_INVALID_PARAMETER;
@@ -1321,6 +1322,7 @@ CoreInternalAllocatePages (
   // if (Start + NumberOfBytes) rolls over 0 or
   // if Start is above MAX_ALLOC_ADDRESS or
   // if End is above MAX_ALLOC_ADDRESS,
+  // if Start..End overlaps any tracked MemoryTypeStatistics range
   // return EFI_NOT_FOUND.
   //
   if (Type == AllocateAddress) {
@@ -1336,6 +1338,35 @@ CoreInternalAllocatePages (
         (End > MaxAddress)) {
       return EFI_NOT_FOUND;
     }
+
+    // Problem summary
+
+    /*
+    A driver is allowed to call AllocatePages using an AllocateAddress type.  This type of
+    AllocatePage request the exact physical address if it is not used.  The existing code
+    will allow this request even in 'special' pages.  The problem with this is that the
+    reason to have 'special' pages for OS hibernate/resume is defeated as memory is
+    fragmented.
+    */
+
+    for (CheckType = (EFI_MEMORY_TYPE) 0; CheckType < EfiMaxMemoryType; CheckType++) {
+      if (MemoryType != CheckType &&
+          mMemoryTypeStatistics[CheckType].Special &&
+          mMemoryTypeStatistics[CheckType].NumberOfPages > 0) {
+        if (Start >= mMemoryTypeStatistics[CheckType].BaseAddress &&
+            Start <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
+          return EFI_NOT_FOUND;
+        }
+        if (End >= mMemoryTypeStatistics[CheckType].BaseAddress &&
+            End <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
+          return EFI_NOT_FOUND;
+        }
+        if (Start < mMemoryTypeStatistics[CheckType].BaseAddress &&
+            End   > mMemoryTypeStatistics[CheckType].MaximumAddress) {
+          return EFI_NOT_FOUND;
+        }
+      }
+    }
   }
 
   if (Type == AllocateMaxAddress) {
-- 
2.13.0.windows.1

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Liming Gao]

Hi, all

This is one bug fix. I plan to catch it for 201808 stable tag. 

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1978

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Liming Gao
> Sent: Saturday, August 10, 2019 10:10 PM
> To: devel@edk2.groups.io
> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
> 
> From: Mike Turner <miketur@microsoft.com>
> 
> The Fpdt driver (FirmwarePerformanceDxe) saves a memory address across
> reboots, and then does an AllocatePage for that memory address.
> If, on this boot, that memory comes from a Runtime memory bucket,
> the MAT table is not updated. This causes Windows to boot into Recovery.
> 
> This patch blocks the memory manager from changing the page
> from a special bucket to a different memory type.  Once the buckets are
> allocated, we freeze the memory ranges for the OS, and fragmenting
> the special buckets will cause errors resuming from hibernate.
> 
> This patch is cherry pick from Project Mu:
> https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af94016ebd391ea6f340920735a
> With the minor change,
> 1. Update commit message format to keep the message in 80 characters one line.
> 2. Remove // MU_CHANGE comments in source code.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 43 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index bd9e116aa5..637518889d 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1265,12 +1265,13 @@ CoreInternalAllocatePages (
>    IN BOOLEAN                NeedGuard
>    )
>  {
> -  EFI_STATUS      Status;
> -  UINT64          Start;
> -  UINT64          NumberOfBytes;
> -  UINT64          End;
> -  UINT64          MaxAddress;
> -  UINTN           Alignment;
> +  EFI_STATUS       Status;
> +  UINT64           Start;
> +  UINT64           NumberOfBytes;
> +  UINT64           End;
> +  UINT64           MaxAddress;
> +  UINTN            Alignment;
> +  EFI_MEMORY_TYPE  CheckType;
> 
>    if ((UINT32)Type >= MaxAllocateType) {
>      return EFI_INVALID_PARAMETER;
> @@ -1321,6 +1322,7 @@ CoreInternalAllocatePages (
>    // if (Start + NumberOfBytes) rolls over 0 or
>    // if Start is above MAX_ALLOC_ADDRESS or
>    // if End is above MAX_ALLOC_ADDRESS,
> +  // if Start..End overlaps any tracked MemoryTypeStatistics range
>    // return EFI_NOT_FOUND.
>    //
>    if (Type == AllocateAddress) {
> @@ -1336,6 +1338,35 @@ CoreInternalAllocatePages (
>          (End > MaxAddress)) {
>        return EFI_NOT_FOUND;
>      }
> +
> +    // Problem summary
> +
> +    /*
> +    A driver is allowed to call AllocatePages using an AllocateAddress type.  This type of
> +    AllocatePage request the exact physical address if it is not used.  The existing code
> +    will allow this request even in 'special' pages.  The problem with this is that the
> +    reason to have 'special' pages for OS hibernate/resume is defeated as memory is
> +    fragmented.
> +    */
> +
> +    for (CheckType = (EFI_MEMORY_TYPE) 0; CheckType < EfiMaxMemoryType; CheckType++) {
> +      if (MemoryType != CheckType &&
> +          mMemoryTypeStatistics[CheckType].Special &&
> +          mMemoryTypeStatistics[CheckType].NumberOfPages > 0) {
> +        if (Start >= mMemoryTypeStatistics[CheckType].BaseAddress &&
> +            Start <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
> +          return EFI_NOT_FOUND;
> +        }
> +        if (End >= mMemoryTypeStatistics[CheckType].BaseAddress &&
> +            End <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
> +          return EFI_NOT_FOUND;
> +        }
> +        if (Start < mMemoryTypeStatistics[CheckType].BaseAddress &&
> +            End   > mMemoryTypeStatistics[CheckType].MaximumAddress) {
> +          return EFI_NOT_FOUND;
> +        }
> +      }
> +    }
>    }
> 
>    if (Type == AllocateMaxAddress) {
> --
> 2.13.0.windows.1
> 
> 
> 

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Wang, Jian J]

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>


> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Liming Gao
> Sent: Saturday, August 10, 2019 10:10 PM
> To: devel@edk2.groups.io
> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT
> update
> 
> From: Mike Turner <miketur@microsoft.com>
> 
> The Fpdt driver (FirmwarePerformanceDxe) saves a memory address across
> reboots, and then does an AllocatePage for that memory address.
> If, on this boot, that memory comes from a Runtime memory bucket,
> the MAT table is not updated. This causes Windows to boot into Recovery.
> 
> This patch blocks the memory manager from changing the page
> from a special bucket to a different memory type.  Once the buckets are
> allocated, we freeze the memory ranges for the OS, and fragmenting
> the special buckets will cause errors resuming from hibernate.
> 
> This patch is cherry pick from Project Mu:
> https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af94
> 016ebd391ea6f340920735a
> With the minor change,
> 1. Update commit message format to keep the message in 80 characters
> one line.
> 2. Remove // MU_CHANGE comments in source code.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 43
> ++++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index bd9e116aa5..637518889d 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1265,12 +1265,13 @@ CoreInternalAllocatePages (
>    IN BOOLEAN                NeedGuard
>    )
>  {
> -  EFI_STATUS      Status;
> -  UINT64          Start;
> -  UINT64          NumberOfBytes;
> -  UINT64          End;
> -  UINT64          MaxAddress;
> -  UINTN           Alignment;
> +  EFI_STATUS       Status;
> +  UINT64           Start;
> +  UINT64           NumberOfBytes;
> +  UINT64           End;
> +  UINT64           MaxAddress;
> +  UINTN            Alignment;
> +  EFI_MEMORY_TYPE  CheckType;
> 
>    if ((UINT32)Type >= MaxAllocateType) {
>      return EFI_INVALID_PARAMETER;
> @@ -1321,6 +1322,7 @@ CoreInternalAllocatePages (
>    // if (Start + NumberOfBytes) rolls over 0 or
>    // if Start is above MAX_ALLOC_ADDRESS or
>    // if End is above MAX_ALLOC_ADDRESS,
> +  // if Start..End overlaps any tracked MemoryTypeStatistics range
>    // return EFI_NOT_FOUND.
>    //
>    if (Type == AllocateAddress) {
> @@ -1336,6 +1338,35 @@ CoreInternalAllocatePages (
>          (End > MaxAddress)) {
>        return EFI_NOT_FOUND;
>      }
> +
> +    // Problem summary
> +
> +    /*
> +    A driver is allowed to call AllocatePages using an AllocateAddress type.
> This type of
> +    AllocatePage request the exact physical address if it is not used.  The
> existing code
> +    will allow this request even in 'special' pages.  The problem with this is
> that the
> +    reason to have 'special' pages for OS hibernate/resume is defeated as
> memory is
> +    fragmented.
> +    */
> +
> +    for (CheckType = (EFI_MEMORY_TYPE) 0; CheckType <
> EfiMaxMemoryType; CheckType++) {
> +      if (MemoryType != CheckType &&
> +          mMemoryTypeStatistics[CheckType].Special &&
> +          mMemoryTypeStatistics[CheckType].NumberOfPages > 0) {
> +        if (Start >= mMemoryTypeStatistics[CheckType].BaseAddress &&
> +            Start <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
> +          return EFI_NOT_FOUND;
> +        }
> +        if (End >= mMemoryTypeStatistics[CheckType].BaseAddress &&
> +            End <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
> +          return EFI_NOT_FOUND;
> +        }
> +        if (Start < mMemoryTypeStatistics[CheckType].BaseAddress &&
> +            End   > mMemoryTypeStatistics[CheckType].MaximumAddress) {
> +          return EFI_NOT_FOUND;
> +        }
> +      }
> +    }
>    }
> 
>    if (Type == AllocateMaxAddress) {
> --
> 2.13.0.windows.1
> 
> 
> 

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Laszlo Ersek]

On 08/10/19 16:10, Liming Gao wrote:
> From: Mike Turner <miketur@microsoft.com>
> 
> The Fpdt driver (FirmwarePerformanceDxe) saves a memory address across
> reboots, and then does an AllocatePage for that memory address.
> If, on this boot, that memory comes from a Runtime memory bucket,
> the MAT table is not updated. This causes Windows to boot into Recovery.

(1) What is "MAT"?

> This patch blocks the memory manager from changing the page
> from a special bucket to a different memory type.  Once the buckets are
> allocated, we freeze the memory ranges for the OS, and fragmenting
> the special buckets will cause errors resuming from hibernate.

(2) My understanding is that CoreConvertPages() will only hand out the
requested pages if those pages are currently free. I suggest clarifying
the commit message that the intent is to prevent the allocation of
otherwise *free* pages, if the allocation would fragment special buckets.

(3) I don't understand the conjunction "and". I would understand if the
statement were:

    Once the buckets are allocated, we freeze the memory ranges for the
    OS, *because* fragmenting the special buckets *would* cause errors
    resuming from hibernate.

Is this the intent?

> 
> This patch is cherry pick from Project Mu:
> https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af94016ebd391ea6f340920735a
> With the minor change,
> 1. Update commit message format to keep the message in 80 characters one line.
> 2. Remove // MU_CHANGE comments in source code.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 43 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index bd9e116aa5..637518889d 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1265,12 +1265,13 @@ CoreInternalAllocatePages (
>    IN BOOLEAN                NeedGuard
>    )
>  {
> -  EFI_STATUS      Status;
> -  UINT64          Start;
> -  UINT64          NumberOfBytes;
> -  UINT64          End;
> -  UINT64          MaxAddress;
> -  UINTN           Alignment;
> +  EFI_STATUS       Status;
> +  UINT64           Start;
> +  UINT64           NumberOfBytes;
> +  UINT64           End;
> +  UINT64           MaxAddress;
> +  UINTN            Alignment;
> +  EFI_MEMORY_TYPE  CheckType;
>  
>    if ((UINT32)Type >= MaxAllocateType) {
>      return EFI_INVALID_PARAMETER;
> @@ -1321,6 +1322,7 @@ CoreInternalAllocatePages (
>    // if (Start + NumberOfBytes) rolls over 0 or
>    // if Start is above MAX_ALLOC_ADDRESS or
>    // if End is above MAX_ALLOC_ADDRESS,
> +  // if Start..End overlaps any tracked MemoryTypeStatistics range
>    // return EFI_NOT_FOUND.
>    //
>    if (Type == AllocateAddress) {
> @@ -1336,6 +1338,35 @@ CoreInternalAllocatePages (
>          (End > MaxAddress)) {
>        return EFI_NOT_FOUND;
>      }
> +
> +    // Problem summary
> +
> +    /*
> +    A driver is allowed to call AllocatePages using an AllocateAddress type.  This type of
> +    AllocatePage request the exact physical address if it is not used.  The existing code
> +    will allow this request even in 'special' pages.  The problem with this is that the
> +    reason to have 'special' pages for OS hibernate/resume is defeated as memory is
> +    fragmented.
> +    */

(4) This comment style is not native to edk2.

I think the "problem summary" line should be removed, and the actual
problem statement should use the following comment style:

  //
  // blah
  //


> +
> +    for (CheckType = (EFI_MEMORY_TYPE) 0; CheckType < EfiMaxMemoryType; CheckType++) {
> +      if (MemoryType != CheckType &&
> +          mMemoryTypeStatistics[CheckType].Special &&
> +          mMemoryTypeStatistics[CheckType].NumberOfPages > 0) {
> +        if (Start >= mMemoryTypeStatistics[CheckType].BaseAddress &&
> +            Start <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
> +          return EFI_NOT_FOUND;
> +        }
> +        if (End >= mMemoryTypeStatistics[CheckType].BaseAddress &&
> +            End <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
> +          return EFI_NOT_FOUND;
> +        }
> +        if (Start < mMemoryTypeStatistics[CheckType].BaseAddress &&
> +            End   > mMemoryTypeStatistics[CheckType].MaximumAddress) {
> +          return EFI_NOT_FOUND;
> +        }
> +      }
> +    }
>    }

(5) Checking for overlap (i.e., whether the intersection is non-empty)
can be done more simply (i.e., with fewer comparisons in the worst case,
and with less code):

  if (MAX (Start, mMemoryTypeStatistics[CheckType].BaseAddress) <=
      MIN (End, mMemoryTypeStatistics[CheckType].MaximumAddress)) {
    return EFI_NOT_FOUND;
  }

but the proposed intersection check is technically right already, IMO,
so there's no strong need to update it.

(Somewhat unusually for this kind of comparison, all four boundaries are
inclusive here.)

(6) Both the commit message and the code comment state that this problem
is specific to S4. Therefore, we can distinguish three cases:

(6a) Platform doesn't support (or doesn't enable) S4 at all.

(6b) Platform supports & enables S4, and this is a normal boot.

(6c) Platform supports & enables S4, and this is actually an S4 resume.

The code being proposed applies to all three cases. Is that the intent?
Shouldn't the new check be made conditional on (6c) -- from the boot
mode HOB --, or at least on (6b)||(6c) -- i.e. the check should be
disabled if S4 is absent entirely?

Thanks,
Laszlo


>  
>    if (Type == AllocateMaxAddress) {
> 

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Michael D Kinney]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> On Behalf Of Laszlo Ersek
> Sent: Monday, August 12, 2019 9:24 AM
> To: devel@edk2.groups.io; Gao, Liming
> <liming.gao@intel.com>
> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>
> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore:
> Fix for missing MAT update
> 
> On 08/10/19 16:10, Liming Gao wrote:
> > From: Mike Turner <miketur@microsoft.com>
> >
> > The Fpdt driver (FirmwarePerformanceDxe) saves a memory
> address across
> > reboots, and then does an AllocatePage for that memory
> address.
> > If, on this boot, that memory comes from a Runtime
> memory bucket, the
> > MAT table is not updated. This causes Windows to boot
> into Recovery.
> 
> (1) What is "MAT"?

Memory Attributes Table (EFI_MEMORY_ATTRIBUTES_TABLE)

> 
> > This patch blocks the memory manager from changing the
> page from a
> > special bucket to a different memory type.  Once the
> buckets are
> > allocated, we freeze the memory ranges for the OS, and
> fragmenting the
> > special buckets will cause errors resuming from
> hibernate.
> 
> (2) My understanding is that CoreConvertPages() will only
> hand out the requested pages if those pages are currently
> free. I suggest clarifying the commit message that the
> intent is to prevent the allocation of otherwise *free*
> pages, if the allocation would fragment special buckets.
> 
> (3) I don't understand the conjunction "and". I would
> understand if the statement were:
> 
>     Once the buckets are allocated, we freeze the memory
> ranges for the
>     OS, *because* fragmenting the special buckets *would*
> cause errors
>     resuming from hibernate.
> 
> Is this the intent?
> 
> >
> > This patch is cherry pick from Project Mu:
> >
> https://github.com/microsoft/mu_basecore/commit/a9be767d9
> be96af94016eb
> > d391ea6f340920735a
> > With the minor change,
> > 1. Update commit message format to keep the message in
> 80 characters one line.
> > 2. Remove // MU_CHANGE comments in source code.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Signed-off-by: Liming Gao <liming.gao@intel.com>
> > ---
> >  MdeModulePkg/Core/Dxe/Mem/Page.c | 43
> > ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > index bd9e116aa5..637518889d 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > @@ -1265,12 +1265,13 @@ CoreInternalAllocatePages (
> >    IN BOOLEAN                NeedGuard
> >    )
> >  {
> > -  EFI_STATUS      Status;
> > -  UINT64          Start;
> > -  UINT64          NumberOfBytes;
> > -  UINT64          End;
> > -  UINT64          MaxAddress;
> > -  UINTN           Alignment;
> > +  EFI_STATUS       Status;
> > +  UINT64           Start;
> > +  UINT64           NumberOfBytes;
> > +  UINT64           End;
> > +  UINT64           MaxAddress;
> > +  UINTN            Alignment;
> > +  EFI_MEMORY_TYPE  CheckType;
> >
> >    if ((UINT32)Type >= MaxAllocateType) {
> >      return EFI_INVALID_PARAMETER;
> > @@ -1321,6 +1322,7 @@ CoreInternalAllocatePages (
> >    // if (Start + NumberOfBytes) rolls over 0 or
> >    // if Start is above MAX_ALLOC_ADDRESS or
> >    // if End is above MAX_ALLOC_ADDRESS,
> > +  // if Start..End overlaps any tracked
> MemoryTypeStatistics range
> >    // return EFI_NOT_FOUND.
> >    //
> >    if (Type == AllocateAddress) {
> > @@ -1336,6 +1338,35 @@ CoreInternalAllocatePages (
> >          (End > MaxAddress)) {
> >        return EFI_NOT_FOUND;
> >      }
> > +
> > +    // Problem summary
> > +
> > +    /*
> > +    A driver is allowed to call AllocatePages using an
> AllocateAddress type.  This type of
> > +    AllocatePage request the exact physical address if
> it is not used.  The existing code
> > +    will allow this request even in 'special' pages.
> The problem with this is that the
> > +    reason to have 'special' pages for OS
> hibernate/resume is defeated as memory is
> > +    fragmented.
> > +    */
> 
> (4) This comment style is not native to edk2.
> 
> I think the "problem summary" line should be removed, and
> the actual problem statement should use the following
> comment style:
> 
>   //
>   // blah
>   //
> 
> 
> > +
> > +    for (CheckType = (EFI_MEMORY_TYPE) 0; CheckType <
> EfiMaxMemoryType; CheckType++) {
> > +      if (MemoryType != CheckType &&
> > +          mMemoryTypeStatistics[CheckType].Special &&
> > +
> mMemoryTypeStatistics[CheckType].NumberOfPages > 0) {
> > +        if (Start >=
> mMemoryTypeStatistics[CheckType].BaseAddress &&
> > +            Start <=
> mMemoryTypeStatistics[CheckType].MaximumAddress) {
> > +          return EFI_NOT_FOUND;
> > +        }
> > +        if (End >=
> mMemoryTypeStatistics[CheckType].BaseAddress &&
> > +            End <=
> mMemoryTypeStatistics[CheckType].MaximumAddress) {
> > +          return EFI_NOT_FOUND;
> > +        }
> > +        if (Start <
> mMemoryTypeStatistics[CheckType].BaseAddress &&
> > +            End   >
> mMemoryTypeStatistics[CheckType].MaximumAddress) {
> > +          return EFI_NOT_FOUND;
> > +        }
> > +      }
> > +    }
> >    }
> 
> (5) Checking for overlap (i.e., whether the intersection
> is non-empty) can be done more simply (i.e., with fewer
> comparisons in the worst case, and with less code):
> 
>   if (MAX (Start,
> mMemoryTypeStatistics[CheckType].BaseAddress) <=
>       MIN (End,
> mMemoryTypeStatistics[CheckType].MaximumAddress)) {
>     return EFI_NOT_FOUND;
>   }
> 
> but the proposed intersection check is technically right
> already, IMO, so there's no strong need to update it.
> 
> (Somewhat unusually for this kind of comparison, all four
> boundaries are inclusive here.)
> 
> (6) Both the commit message and the code comment state
> that this problem is specific to S4. Therefore, we can
> distinguish three cases:
> 
> (6a) Platform doesn't support (or doesn't enable) S4 at
> all.
> 
> (6b) Platform supports & enables S4, and this is a normal
> boot.
> 
> (6c) Platform supports & enables S4, and this is actually
> an S4 resume.
> 
> The code being proposed applies to all three cases. Is
> that the intent?
> Shouldn't the new check be made conditional on (6c) --
> from the boot mode HOB --, or at least on (6b)||(6c) --
> i.e. the check should be disabled if S4 is absent
> entirely?

Hi Laszlo,

I think this check should be added for all cases. Without
this change, memory allocations using type AllocateAddress
Is allowed to allocate in the unused portion of a bin.  This
means the memory allocations are not consist with the memory
map returned by GetMemoryMap() that shows the entire bin as
allocated.  The only exception that is allowed is if an
AllocateAddress request is made to the unused portion of a
bin where the request and the bin have the same MemoryType.

The references to S4 here are the use case that fails.  This
failure is root caused to an inconsistent behavior of the 
core memory services themselves when type AllocateAddress is
used.  

The only time these types of check can be disabled is if the
Memory Type Information feature is disabled.

Thanks,

Mike

> 
> Thanks,
> Laszlo
> 
> 
> >
> >    if (Type == AllocateMaxAddress) {
> >
> 
> 
> 

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Laszlo Ersek]

On 08/13/19 01:22, Kinney, Michael D wrote:
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
>> On Behalf Of Laszlo Ersek
>> Sent: Monday, August 12, 2019 9:24 AM
>> To: devel@edk2.groups.io; Gao, Liming
>> <liming.gao@intel.com>
>> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
>> Bi, Dandan <dandan.bi@intel.com>
>> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore:
>> Fix for missing MAT update
>>
>> On 08/10/19 16:10, Liming Gao wrote:
>>> From: Mike Turner <miketur@microsoft.com>
>>>
>>> The Fpdt driver (FirmwarePerformanceDxe) saves a memory
>> address across
>>> reboots, and then does an AllocatePage for that memory
>> address.
>>> If, on this boot, that memory comes from a Runtime
>> memory bucket, the
>>> MAT table is not updated. This causes Windows to boot
>> into Recovery.
>>
>> (1) What is "MAT"?
> 
> Memory Attributes Table (EFI_MEMORY_ATTRIBUTES_TABLE)
> 
>>
>>> This patch blocks the memory manager from changing the
>> page from a
>>> special bucket to a different memory type.  Once the
>> buckets are
>>> allocated, we freeze the memory ranges for the OS, and
>> fragmenting the
>>> special buckets will cause errors resuming from
>> hibernate.
>>
>> (2) My understanding is that CoreConvertPages() will only
>> hand out the requested pages if those pages are currently
>> free. I suggest clarifying the commit message that the
>> intent is to prevent the allocation of otherwise *free*
>> pages, if the allocation would fragment special buckets.
>>
>> (3) I don't understand the conjunction "and". I would
>> understand if the statement were:
>>
>>     Once the buckets are allocated, we freeze the memory
>> ranges for the
>>     OS, *because* fragmenting the special buckets *would*
>> cause errors
>>     resuming from hibernate.
>>
>> Is this the intent?
>>
>>>
>>> This patch is cherry pick from Project Mu:
>>>
>> https://github.com/microsoft/mu_basecore/commit/a9be767d9
>> be96af94016eb
>>> d391ea6f340920735a
>>> With the minor change,
>>> 1. Update commit message format to keep the message in
>> 80 characters one line.
>>> 2. Remove // MU_CHANGE comments in source code.
>>>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Hao A Wu <hao.a.wu@intel.com>
>>> Cc: Dandan Bi <dandan.bi@intel.com>
>>> Signed-off-by: Liming Gao <liming.gao@intel.com>
>>> ---
>>>  MdeModulePkg/Core/Dxe/Mem/Page.c | 43
>>> ++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 37 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
>>> b/MdeModulePkg/Core/Dxe/Mem/Page.c
>>> index bd9e116aa5..637518889d 100644
>>> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
>>> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
>>> @@ -1265,12 +1265,13 @@ CoreInternalAllocatePages (
>>>    IN BOOLEAN                NeedGuard
>>>    )
>>>  {
>>> -  EFI_STATUS      Status;
>>> -  UINT64          Start;
>>> -  UINT64          NumberOfBytes;
>>> -  UINT64          End;
>>> -  UINT64          MaxAddress;
>>> -  UINTN           Alignment;
>>> +  EFI_STATUS       Status;
>>> +  UINT64           Start;
>>> +  UINT64           NumberOfBytes;
>>> +  UINT64           End;
>>> +  UINT64           MaxAddress;
>>> +  UINTN            Alignment;
>>> +  EFI_MEMORY_TYPE  CheckType;
>>>
>>>    if ((UINT32)Type >= MaxAllocateType) {
>>>      return EFI_INVALID_PARAMETER;
>>> @@ -1321,6 +1322,7 @@ CoreInternalAllocatePages (
>>>    // if (Start + NumberOfBytes) rolls over 0 or
>>>    // if Start is above MAX_ALLOC_ADDRESS or
>>>    // if End is above MAX_ALLOC_ADDRESS,
>>> +  // if Start..End overlaps any tracked
>> MemoryTypeStatistics range
>>>    // return EFI_NOT_FOUND.
>>>    //
>>>    if (Type == AllocateAddress) {
>>> @@ -1336,6 +1338,35 @@ CoreInternalAllocatePages (
>>>          (End > MaxAddress)) {
>>>        return EFI_NOT_FOUND;
>>>      }
>>> +
>>> +    // Problem summary
>>> +
>>> +    /*
>>> +    A driver is allowed to call AllocatePages using an
>> AllocateAddress type.  This type of
>>> +    AllocatePage request the exact physical address if
>> it is not used.  The existing code
>>> +    will allow this request even in 'special' pages.
>> The problem with this is that the
>>> +    reason to have 'special' pages for OS
>> hibernate/resume is defeated as memory is
>>> +    fragmented.
>>> +    */
>>
>> (4) This comment style is not native to edk2.
>>
>> I think the "problem summary" line should be removed, and
>> the actual problem statement should use the following
>> comment style:
>>
>>   //
>>   // blah
>>   //
>>
>>
>>> +
>>> +    for (CheckType = (EFI_MEMORY_TYPE) 0; CheckType <
>> EfiMaxMemoryType; CheckType++) {
>>> +      if (MemoryType != CheckType &&
>>> +          mMemoryTypeStatistics[CheckType].Special &&
>>> +
>> mMemoryTypeStatistics[CheckType].NumberOfPages > 0) {
>>> +        if (Start >=
>> mMemoryTypeStatistics[CheckType].BaseAddress &&
>>> +            Start <=
>> mMemoryTypeStatistics[CheckType].MaximumAddress) {
>>> +          return EFI_NOT_FOUND;
>>> +        }
>>> +        if (End >=
>> mMemoryTypeStatistics[CheckType].BaseAddress &&
>>> +            End <=
>> mMemoryTypeStatistics[CheckType].MaximumAddress) {
>>> +          return EFI_NOT_FOUND;
>>> +        }
>>> +        if (Start <
>> mMemoryTypeStatistics[CheckType].BaseAddress &&
>>> +            End   >
>> mMemoryTypeStatistics[CheckType].MaximumAddress) {
>>> +          return EFI_NOT_FOUND;
>>> +        }
>>> +      }
>>> +    }
>>>    }
>>
>> (5) Checking for overlap (i.e., whether the intersection
>> is non-empty) can be done more simply (i.e., with fewer
>> comparisons in the worst case, and with less code):
>>
>>   if (MAX (Start,
>> mMemoryTypeStatistics[CheckType].BaseAddress) <=
>>       MIN (End,
>> mMemoryTypeStatistics[CheckType].MaximumAddress)) {
>>     return EFI_NOT_FOUND;
>>   }
>>
>> but the proposed intersection check is technically right
>> already, IMO, so there's no strong need to update it.
>>
>> (Somewhat unusually for this kind of comparison, all four
>> boundaries are inclusive here.)
>>
>> (6) Both the commit message and the code comment state
>> that this problem is specific to S4. Therefore, we can
>> distinguish three cases:
>>
>> (6a) Platform doesn't support (or doesn't enable) S4 at
>> all.
>>
>> (6b) Platform supports & enables S4, and this is a normal
>> boot.
>>
>> (6c) Platform supports & enables S4, and this is actually
>> an S4 resume.
>>
>> The code being proposed applies to all three cases. Is
>> that the intent?
>> Shouldn't the new check be made conditional on (6c) --
>> from the boot mode HOB --, or at least on (6b)||(6c) --
>> i.e. the check should be disabled if S4 is absent
>> entirely?
> 
> Hi Laszlo,
> 
> I think this check should be added for all cases. Without
> this change, memory allocations using type AllocateAddress
> Is allowed to allocate in the unused portion of a bin.  This
> means the memory allocations are not consist with the memory
> map returned by GetMemoryMap() that shows the entire bin as
> allocated.  The only exception that is allowed is if an
> AllocateAddress request is made to the unused portion of a
> bin where the request and the bin have the same MemoryType.

Thanks for the explanation. It helps! I understand now.

> The references to S4 here are the use case that fails.  This
> failure is root caused to an inconsistent behavior of the 
> core memory services themselves when type AllocateAddress is
> used.  

Can the commit message be framed accordingly, please?

The main issue is apparently with the UEFI memory map -- the UEFI memory
map reflects the pre-allocated bins, but the actual allocations at fixed
addresses may go out of sync with that. Everything else, such as:

- EFI_MEMORY_ATTRIBUTES_TABLE (page protections) being out of sync,

- S4 failing

are just symptoms / consequences.

> The only time these types of check can be disabled is if the
> Memory Type Information feature is disabled.

The gEfiMemoryTypeInformationGuid HOB is supposed to be built -- if it
is built at all -- no later than in the DXE IPL PEIM (if VariablePei is
included in the platform, and the underlying UEFI variable exists). Is
that correct?

Becase if it is correct, then I think the check could be based (in the
DXE core) on the presence of this HOB.

Thank you!
Laszlo

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Liming Gao]

Laszlo:

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, August 13, 2019 5:48 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
> 
> On 08/13/19 01:22, Kinney, Michael D wrote:
> >
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> >> On Behalf Of Laszlo Ersek
> >> Sent: Monday, August 12, 2019 9:24 AM
> >> To: devel@edk2.groups.io; Gao, Liming
> >> <liming.gao@intel.com>
> >> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
> >> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> >> Bi, Dandan <dandan.bi@intel.com>
> >> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore:
> >> Fix for missing MAT update
> >>
> >> On 08/10/19 16:10, Liming Gao wrote:
> >>> From: Mike Turner <miketur@microsoft.com>
> >>>
> >>> The Fpdt driver (FirmwarePerformanceDxe) saves a memory
> >> address across
> >>> reboots, and then does an AllocatePage for that memory
> >> address.
> >>> If, on this boot, that memory comes from a Runtime
> >> memory bucket, the
> >>> MAT table is not updated. This causes Windows to boot
> >> into Recovery.
> >>
> >> (1) What is "MAT"?
> >
> > Memory Attributes Table (EFI_MEMORY_ATTRIBUTES_TABLE)
> >
> >>
> >>> This patch blocks the memory manager from changing the
> >> page from a
> >>> special bucket to a different memory type.  Once the
> >> buckets are
> >>> allocated, we freeze the memory ranges for the OS, and
> >> fragmenting the
> >>> special buckets will cause errors resuming from
> >> hibernate.
> >>
> >> (2) My understanding is that CoreConvertPages() will only
> >> hand out the requested pages if those pages are currently
> >> free. I suggest clarifying the commit message that the
> >> intent is to prevent the allocation of otherwise *free*
> >> pages, if the allocation would fragment special buckets.
> >>
> >> (3) I don't understand the conjunction "and". I would
> >> understand if the statement were:
> >>
> >>     Once the buckets are allocated, we freeze the memory
> >> ranges for the
> >>     OS, *because* fragmenting the special buckets *would*
> >> cause errors
> >>     resuming from hibernate.
> >>
> >> Is this the intent?
> >>
> >>>
> >>> This patch is cherry pick from Project Mu:
> >>>
> >> https://github.com/microsoft/mu_basecore/commit/a9be767d9
> >> be96af94016eb
> >>> d391ea6f340920735a
> >>> With the minor change,
> >>> 1. Update commit message format to keep the message in
> >> 80 characters one line.
> >>> 2. Remove // MU_CHANGE comments in source code.
> >>>
> >>> Cc: Jian J Wang <jian.j.wang@intel.com>
> >>> Cc: Hao A Wu <hao.a.wu@intel.com>
> >>> Cc: Dandan Bi <dandan.bi@intel.com>
> >>> Signed-off-by: Liming Gao <liming.gao@intel.com>
> >>> ---
> >>>  MdeModulePkg/Core/Dxe/Mem/Page.c | 43
> >>> ++++++++++++++++++++++++++++++++++------
> >>>  1 file changed, 37 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> >>> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> >>> index bd9e116aa5..637518889d 100644
> >>> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> >>> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> >>> @@ -1265,12 +1265,13 @@ CoreInternalAllocatePages (
> >>>    IN BOOLEAN                NeedGuard
> >>>    )
> >>>  {
> >>> -  EFI_STATUS      Status;
> >>> -  UINT64          Start;
> >>> -  UINT64          NumberOfBytes;
> >>> -  UINT64          End;
> >>> -  UINT64          MaxAddress;
> >>> -  UINTN           Alignment;
> >>> +  EFI_STATUS       Status;
> >>> +  UINT64           Start;
> >>> +  UINT64           NumberOfBytes;
> >>> +  UINT64           End;
> >>> +  UINT64           MaxAddress;
> >>> +  UINTN            Alignment;
> >>> +  EFI_MEMORY_TYPE  CheckType;
> >>>
> >>>    if ((UINT32)Type >= MaxAllocateType) {
> >>>      return EFI_INVALID_PARAMETER;
> >>> @@ -1321,6 +1322,7 @@ CoreInternalAllocatePages (
> >>>    // if (Start + NumberOfBytes) rolls over 0 or
> >>>    // if Start is above MAX_ALLOC_ADDRESS or
> >>>    // if End is above MAX_ALLOC_ADDRESS,
> >>> +  // if Start..End overlaps any tracked
> >> MemoryTypeStatistics range
> >>>    // return EFI_NOT_FOUND.
> >>>    //
> >>>    if (Type == AllocateAddress) {
> >>> @@ -1336,6 +1338,35 @@ CoreInternalAllocatePages (
> >>>          (End > MaxAddress)) {
> >>>        return EFI_NOT_FOUND;
> >>>      }
> >>> +
> >>> +    // Problem summary
> >>> +
> >>> +    /*
> >>> +    A driver is allowed to call AllocatePages using an
> >> AllocateAddress type.  This type of
> >>> +    AllocatePage request the exact physical address if
> >> it is not used.  The existing code
> >>> +    will allow this request even in 'special' pages.
> >> The problem with this is that the
> >>> +    reason to have 'special' pages for OS
> >> hibernate/resume is defeated as memory is
> >>> +    fragmented.
> >>> +    */
> >>
> >> (4) This comment style is not native to edk2.
> >>
> >> I think the "problem summary" line should be removed, and
> >> the actual problem statement should use the following
> >> comment style:
> >>
> >>   //
> >>   // blah
> >>   //

I cherry pick this patch from Mu project with the minimal change. 
I can update the comment style. 

> >>
> >>
> >>> +
> >>> +    for (CheckType = (EFI_MEMORY_TYPE) 0; CheckType <
> >> EfiMaxMemoryType; CheckType++) {
> >>> +      if (MemoryType != CheckType &&
> >>> +          mMemoryTypeStatistics[CheckType].Special &&
> >>> +
> >> mMemoryTypeStatistics[CheckType].NumberOfPages > 0) {
> >>> +        if (Start >=
> >> mMemoryTypeStatistics[CheckType].BaseAddress &&
> >>> +            Start <=
> >> mMemoryTypeStatistics[CheckType].MaximumAddress) {
> >>> +          return EFI_NOT_FOUND;
> >>> +        }
> >>> +        if (End >=
> >> mMemoryTypeStatistics[CheckType].BaseAddress &&
> >>> +            End <=
> >> mMemoryTypeStatistics[CheckType].MaximumAddress) {
> >>> +          return EFI_NOT_FOUND;
> >>> +        }
> >>> +        if (Start <
> >> mMemoryTypeStatistics[CheckType].BaseAddress &&
> >>> +            End   >
> >> mMemoryTypeStatistics[CheckType].MaximumAddress) {
> >>> +          return EFI_NOT_FOUND;
> >>> +        }
> >>> +      }
> >>> +    }
> >>>    }
> >>
> >> (5) Checking for overlap (i.e., whether the intersection
> >> is non-empty) can be done more simply (i.e., with fewer
> >> comparisons in the worst case, and with less code):
> >>
> >>   if (MAX (Start,
> >> mMemoryTypeStatistics[CheckType].BaseAddress) <=
> >>       MIN (End,
> >> mMemoryTypeStatistics[CheckType].MaximumAddress)) {
> >>     return EFI_NOT_FOUND;
> >>   }
> >>
> >> but the proposed intersection check is technically right
> >> already, IMO, so there's no strong need to update it.
> >>
> >> (Somewhat unusually for this kind of comparison, all four
> >> boundaries are inclusive here.)
> >>
> >> (6) Both the commit message and the code comment state
> >> that this problem is specific to S4. Therefore, we can
> >> distinguish three cases:
> >>
> >> (6a) Platform doesn't support (or doesn't enable) S4 at
> >> all.
> >>
> >> (6b) Platform supports & enables S4, and this is a normal
> >> boot.
> >>
> >> (6c) Platform supports & enables S4, and this is actually
> >> an S4 resume.
> >>
> >> The code being proposed applies to all three cases. Is
> >> that the intent?
> >> Shouldn't the new check be made conditional on (6c) --
> >> from the boot mode HOB --, or at least on (6b)||(6c) --
> >> i.e. the check should be disabled if S4 is absent
> >> entirely?
> >
> > Hi Laszlo,
> >
> > I think this check should be added for all cases. Without
> > this change, memory allocations using type AllocateAddress
> > Is allowed to allocate in the unused portion of a bin.  This
> > means the memory allocations are not consist with the memory
> > map returned by GetMemoryMap() that shows the entire bin as
> > allocated.  The only exception that is allowed is if an
> > AllocateAddress request is made to the unused portion of a
> > bin where the request and the bin have the same MemoryType.
> 
> Thanks for the explanation. It helps! I understand now.
> 
> > The references to S4 here are the use case that fails.  This
> > failure is root caused to an inconsistent behavior of the
> > core memory services themselves when type AllocateAddress is
> > used.
> 
> Can the commit message be framed accordingly, please?
> 
> The main issue is apparently with the UEFI memory map -- the UEFI memory
> map reflects the pre-allocated bins, but the actual allocations at fixed
> addresses may go out of sync with that. Everything else, such as:
> 
> - EFI_MEMORY_ATTRIBUTES_TABLE (page protections) being out of sync,
> 
> - S4 failing
> 
> are just symptoms / consequences.
> 
> > The only time these types of check can be disabled is if the
> > Memory Type Information feature is disabled.
> 
> The gEfiMemoryTypeInformationGuid HOB is supposed to be built -- if it
> is built at all -- no later than in the DXE IPL PEIM (if VariablePei is
> included in the platform, and the underlying UEFI variable exists). Is
> that correct?
> 
gEfiMemoryTypeInformationGuid HOB is installed by platform PEI. 
If the platform PEI doesn't install this HOB, it means this feature is disabled. 

Thanks
Liming
> Becase if it is correct, then I think the check could be based (in the
> DXE core) on the presence of this HOB.
> 
> Thank you!
> Laszlo

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Laszlo Ersek]

On 08/14/19 16:00, Gao, Liming wrote:
> Laszlo:

> 
> I cherry pick this patch from Mu project with the minimal change. 
> I can update the comment style. 

Yes, please. Thanks!

>> The gEfiMemoryTypeInformationGuid HOB is supposed to be built -- if it
>> is built at all -- no later than in the DXE IPL PEIM (if VariablePei is
>> included in the platform, and the underlying UEFI variable exists). Is
>> that correct?
>>
> gEfiMemoryTypeInformationGuid HOB is installed by platform PEI. 

Yes, that's what I meant by "no later than in the DXE IPL PEIM".

> If the platform PEI doesn't install this HOB, it means this feature is disabled. 

PlatformPei is supposed to build the HOB in the first place, yes.

However, if it doesn't, then there still may be a feedback loop between
the DXE IPL PEIM and BDS. The former builds the HOB from the UEFI
variable, and the latter updates the variable (as I understand) for
future boots.

Thanks
Laszlo

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Liming Gao]

Laszlo:

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Wednesday, August 14, 2019 11:13 PM
> To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
> 
> On 08/14/19 16:00, Gao, Liming wrote:
> > Laszlo:
> 
> >
> > I cherry pick this patch from Mu project with the minimal change.
> > I can update the comment style.
> 
> Yes, please. Thanks!
> 
> >> The gEfiMemoryTypeInformationGuid HOB is supposed to be built -- if it
> >> is built at all -- no later than in the DXE IPL PEIM (if VariablePei is
> >> included in the platform, and the underlying UEFI variable exists). Is
> >> that correct?
> >>
> > gEfiMemoryTypeInformationGuid HOB is installed by platform PEI.
> 
> Yes, that's what I meant by "no later than in the DXE IPL PEIM".
> 
> > If the platform PEI doesn't install this HOB, it means this feature is disabled.
> 
> PlatformPei is supposed to build the HOB in the first place, yes.
> 
> However, if it doesn't, then there still may be a feedback loop between
> the DXE IPL PEIM and BDS. The former builds the HOB from the UEFI
> variable, and the latter updates the variable (as I understand) for
> future boots.
> 
UEFI variable is created by BDS only when HOB can be found. 
If Platform PEIM doesn't build HOB, DxeIpl will not build HOB, 
then BDS will not create UEFI variable. If so, there is no HOB in 
every boot. 

Thanks
Liming
> Thanks
> Laszlo
> 
> 

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Laszlo Ersek]

On 08/14/19 17:55, Gao, Liming wrote:

> If Platform PEIM doesn't build HOB, DxeIpl will not build HOB, 

My reading of the code is the opposite. If the platform PEIM does not build the HOB, then the DXE IPL PEIM will attempt to build the HOB, from the UEFI variable.

At commit caa7d3a896f6, in file "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c", function DxeLoadCore(), we have:

   363    if (GetFirstGuidHob ((CONST EFI_GUID *)&gEfiMemoryTypeInformationGuid) == NULL) {
   364      //
   365      // Don't build GuidHob if GuidHob has been installed.
   366      //
   367      Status = PeiServicesLocatePpi (
   368                 &gEfiPeiReadOnlyVariable2PpiGuid,
   369                 0,
   370                 NULL,
   371                 (VOID **)&Variable
   372                 );
   373      if (!EFI_ERROR (Status)) {
   374        DataSize = sizeof (MemoryData);
   375        Status = Variable->GetVariable (
   376                             Variable,
   377                             EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
   378                             &gEfiMemoryTypeInformationGuid,
   379                             NULL,
   380                             &DataSize,
   381                             &MemoryData
   382                             );
   383        if (!EFI_ERROR (Status) && ValidateMemoryTypeInfoVariable(MemoryData, DataSize)) {
   384          //
   385          // Build the GUID'd HOB for DXE
   386          //
   387          BuildGuidDataHob (
   388            &gEfiMemoryTypeInformationGuid,
   389            MemoryData,
   390            DataSize
   391            );
   392        }
   393      }
   394    }

Thanks
Laszlo

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Liming Gao]

Laszlo:

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Friday, August 16, 2019 11:18 PM
> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
> 
> On 08/14/19 17:55, Gao, Liming wrote:
> 
> > If Platform PEIM doesn't build HOB, DxeIpl will not build HOB,
> 
> My reading of the code is the opposite. If the platform PEIM does not build the HOB, then the DXE IPL PEIM will attempt to build the HOB,
> from the UEFI variable.
> 
> At commit caa7d3a896f6, in file "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c", function DxeLoadCore(), we have:
> 
>    363    if (GetFirstGuidHob ((CONST EFI_GUID *)&gEfiMemoryTypeInformationGuid) == NULL) {
>    364      //
>    365      // Don't build GuidHob if GuidHob has been installed.
>    366      //
>    367      Status = PeiServicesLocatePpi (
>    368                 &gEfiPeiReadOnlyVariable2PpiGuid,
>    369                 0,
>    370                 NULL,
>    371                 (VOID **)&Variable
>    372                 );
>    373      if (!EFI_ERROR (Status)) {
>    374        DataSize = sizeof (MemoryData);
>    375        Status = Variable->GetVariable (
>    376                             Variable,
>    377                             EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
>    378                             &gEfiMemoryTypeInformationGuid,
>    379                             NULL,
>    380                             &DataSize,
>    381                             &MemoryData
>    382                             );
>    383        if (!EFI_ERROR (Status) && ValidateMemoryTypeInfoVariable(MemoryData, DataSize)) {

Only when this variable exists, Hob will be built. But, if no PEIM builds Hob, BDS will not set the variable. 
So, there is still no HOB.

Thanks
Liming
>    384          //
>    385          // Build the GUID'd HOB for DXE
>    386          //
>    387          BuildGuidDataHob (
>    388            &gEfiMemoryTypeInformationGuid,
>    389            MemoryData,
>    390            DataSize
>    391            );
>    392        }
>    393      }
>    394    }
> 
> Thanks
> Laszlo
> 
> 

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Laszlo Ersek]

On 08/16/19 17:24, Gao, Liming wrote:
> Laszlo:
>
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Friday, August 16, 2019 11:18 PM
>> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney,
>> Michael D <michael.d.kinney@intel.com>
>> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
>> <dandan.bi@intel.com>
>> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for
>> missing MAT update
>>
>> On 08/14/19 17:55, Gao, Liming wrote:
>>
>>> If Platform PEIM doesn't build HOB, DxeIpl will not build HOB,
>>
>> My reading of the code is the opposite. If the platform PEIM does not
>> build the HOB, then the DXE IPL PEIM will attempt to build the HOB,
>> from the UEFI variable.
>>
>> At commit caa7d3a896f6, in file
>> "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c", function DxeLoadCore(), we
>> have:
>>
>>    363    if (GetFirstGuidHob ((CONST EFI_GUID *)&gEfiMemoryTypeInformationGuid) == NULL) {
>>    364      //
>>    365      // Don't build GuidHob if GuidHob has been installed.
>>    366      //
>>    367      Status = PeiServicesLocatePpi (
>>    368                 &gEfiPeiReadOnlyVariable2PpiGuid,
>>    369                 0,
>>    370                 NULL,
>>    371                 (VOID **)&Variable
>>    372                 );
>>    373      if (!EFI_ERROR (Status)) {
>>    374        DataSize = sizeof (MemoryData);
>>    375        Status = Variable->GetVariable (
>>    376                             Variable,
>>    377                             EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
>>    378                             &gEfiMemoryTypeInformationGuid,
>>    379                             NULL,
>>    380                             &DataSize,
>>    381                             &MemoryData
>>    382                             );
>>    383        if (!EFI_ERROR (Status) && ValidateMemoryTypeInfoVariable(MemoryData, DataSize)) {
>
> Only when this variable exists, Hob will be built. But, if no PEIM
> builds Hob, BDS will not set the variable.
> So, there is still no HOB.

So how is a platform supposed to enable this feature?

If PlatformPei never builds the HOB, the variable will never be created,
so the DXE IPL PEIM will also not build the HOB, ever.

If PlatformPei builds the HOB with static data, then BDS will set
(update) the variable, yes, but the DXE IPL PEIM will ignore the
variable, because PlatformPei already built the HOB.

So... Is PlatformPei supposed to use the Variable PPI, check if the
variable exists, and create the static HOB only if the variable is
absent?

... Ugh, wait. I've actually implemented this for OVMF almost 2 years
ago! And the answer to my question is "yes":

  https://bugzilla.tianocore.org/show_bug.cgi?id=386
  https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html

See the OnReadOnlyVariable2Available() function:

+  //
+  // Check if the "MemoryTypeInformation" variable exists, in the
+  // gEfiMemoryTypeInformationGuid namespace.
+  //
+  ReadOnlyVariable2 = Ppi;
+  DataSize = 0;
+  Status = ReadOnlyVariable2->GetVariable (
+                                ReadOnlyVariable2,
+                                EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
+                                &gEfiMemoryTypeInformationGuid,
+                                NULL,
+                                &DataSize,
+                                NULL
+                                );
+  if (Status == EFI_BUFFER_TOO_SMALL) {
+    //
+    // The variable exists; the DXE IPL PEIM will build the HOB from it.
+    //
+    return EFI_SUCCESS;
+  }
+  //
+  // Install the default memory type information HOB.
+  //
+  BuildMemTypeInfoHob ();

Apologies for forgetting about this; you are right.

Too bad my work for TianoCore#386 was rejected. :(

Thanks
Laszlo

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Liming Gao]

Laszlo:

>-----Original Message-----
>From: Laszlo Ersek [mailto:lersek@redhat.com]
>Sent: Saturday, August 17, 2019 2:54 AM
>To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney,
>Michael D <michael.d.kinney@intel.com>
>Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
><jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
><dandan.bi@intel.com>
>Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing
>MAT update
>
>On 08/16/19 17:24, Gao, Liming wrote:
>> Laszlo:
>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>>> Laszlo Ersek
>>> Sent: Friday, August 16, 2019 11:18 PM
>>> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney,
>>> Michael D <michael.d.kinney@intel.com>
>>> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
>>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
>>> <dandan.bi@intel.com>
>>> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for
>>> missing MAT update
>>>
>>> On 08/14/19 17:55, Gao, Liming wrote:
>>>
>>>> If Platform PEIM doesn't build HOB, DxeIpl will not build HOB,
>>>
>>> My reading of the code is the opposite. If the platform PEIM does not
>>> build the HOB, then the DXE IPL PEIM will attempt to build the HOB,
>>> from the UEFI variable.
>>>
>>> At commit caa7d3a896f6, in file
>>> "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c", function DxeLoadCore(),
>we
>>> have:
>>>
>>>    363    if (GetFirstGuidHob ((CONST EFI_GUID
>*)&gEfiMemoryTypeInformationGuid) == NULL) {
>>>    364      //
>>>    365      // Don't build GuidHob if GuidHob has been installed.
>>>    366      //
>>>    367      Status = PeiServicesLocatePpi (
>>>    368                 &gEfiPeiReadOnlyVariable2PpiGuid,
>>>    369                 0,
>>>    370                 NULL,
>>>    371                 (VOID **)&Variable
>>>    372                 );
>>>    373      if (!EFI_ERROR (Status)) {
>>>    374        DataSize = sizeof (MemoryData);
>>>    375        Status = Variable->GetVariable (
>>>    376                             Variable,
>>>    377                             EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
>>>    378                             &gEfiMemoryTypeInformationGuid,
>>>    379                             NULL,
>>>    380                             &DataSize,
>>>    381                             &MemoryData
>>>    382                             );
>>>    383        if (!EFI_ERROR (Status) &&
>ValidateMemoryTypeInfoVariable(MemoryData, DataSize)) {
>>
>> Only when this variable exists, Hob will be built. But, if no PEIM
>> builds Hob, BDS will not set the variable.
>> So, there is still no HOB.
>
>So how is a platform supposed to enable this feature?
>
>If PlatformPei never builds the HOB, the variable will never be created,
>so the DXE IPL PEIM will also not build the HOB, ever.
>
>If PlatformPei builds the HOB with static data, then BDS will set
>(update) the variable, yes, but the DXE IPL PEIM will ignore the
>variable, because PlatformPei already built the HOB.
>
>So... Is PlatformPei supposed to use the Variable PPI, check if the
>variable exists, and create the static HOB only if the variable is
>absent?
>
>... Ugh, wait. I've actually implemented this for OVMF almost 2 years
>ago! And the answer to my question is "yes":
>
>  https://bugzilla.tianocore.org/show_bug.cgi?id=386
>  https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html
>
>See the OnReadOnlyVariable2Available() function:
>
>+  //
>+  // Check if the "MemoryTypeInformation" variable exists, in the
>+  // gEfiMemoryTypeInformationGuid namespace.
>+  //
>+  ReadOnlyVariable2 = Ppi;
>+  DataSize = 0;
>+  Status = ReadOnlyVariable2->GetVariable (
>+                                ReadOnlyVariable2,
>+                                EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
>+                                &gEfiMemoryTypeInformationGuid,
>+                                NULL,
>+                                &DataSize,
>+                                NULL
>+                                );
>+  if (Status == EFI_BUFFER_TOO_SMALL) {
>+    //
>+    // The variable exists; the DXE IPL PEIM will build the HOB from it.
>+    //
>+    return EFI_SUCCESS;
>+  }
>+  //
>+  // Install the default memory type information HOB.
>+  //
>+  BuildMemTypeInfoHob ();
>
>Apologies for forgetting about this; you are right.
>
>Too bad my work for TianoCore#386 was rejected. :(
>

So, this is one value to add PEI variable support in OVMF. 
Do you consider to approve this feature request? 

Thanks
Liming

>Thanks
>Laszlo

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Laszlo Ersek]

Hi Liming,

On 08/19/19 02:40, Gao, Liming wrote:

>> ... Ugh, wait. I've actually implemented this for OVMF almost 2 years
>> ago! And the answer to my question is "yes":
>>
>>  https://bugzilla.tianocore.org/show_bug.cgi?id=386
>>  https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html
>>
>> See the OnReadOnlyVariable2Available() function:
>>
>> +  //
>> +  // Check if the "MemoryTypeInformation" variable exists, in the
>> +  // gEfiMemoryTypeInformationGuid namespace.
>> +  //
>> +  ReadOnlyVariable2 = Ppi;
>> +  DataSize = 0;
>> +  Status = ReadOnlyVariable2->GetVariable (
>> +                                ReadOnlyVariable2,
>> +                                EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
>> +                                &gEfiMemoryTypeInformationGuid,
>> +                                NULL,
>> +                                &DataSize,
>> +                                NULL
>> +                                );
>> +  if (Status == EFI_BUFFER_TOO_SMALL) {
>> +    //
>> +    // The variable exists; the DXE IPL PEIM will build the HOB from it.
>> +    //
>> +    return EFI_SUCCESS;
>> +  }
>> +  //
>> +  // Install the default memory type information HOB.
>> +  //
>> +  BuildMemTypeInfoHob ();
>>
>> Apologies for forgetting about this; you are right.
>>
>> Too bad my work for TianoCore#386 was rejected. :(
>>
> 
> So, this is one value to add PEI variable support in OVMF. 
> Do you consider to approve this feature request? 

Sorry, I don't understand your question.

Are you asking me if, in my opinion, we should fix TianoCore#386 (= add
PEI variable support to OVMF)?

If that is your question, then my answer is -- trivially -- "yes". If
you read through TianoCore#386, you will see that I submitted patches
for solving the BZ, twice (comment 6, comment 9).

Jordan rejected my patches both times, because he thought that the same
feature should be implemented in OVMF for Xen as well. I disagreed (and
still disagree) -- my patches depend on a build-time flag that produces
an OVMF binary that is only compatible with QEMU, and not Xen. The
reason for that is that the feature (PEI variable support) cannot be
implemented *sensibly* on Xen. (Xen offers no spec-conformant variable
storage, and in the PEI phase, the variable store would always appear
empty.) Later, Jordan tried to add dynamic pflash detection to PEI, but
it didn't work in all cases, because OVMF's PEI phase doesn't run in
SMM, while QEMU restricts pflash access to SMM. (In other words, pflash
protection in QEMU is less flexible than SMRAM protection -- SMRAM is
unlocked in PEI, but pflash is always locked.) Please see the threads
linked in the BZ for the discussion.

With TianoCore#1689, Anthony has started separating OVMF into different
firmware platforms for Xen and QEMU. In a year or so, "OVMF for QEMU"
will likely have nothing Xen-related in it (because "OVMF for Xen" will
exist separately). Then we can reopen TianoCore#386 just for "OVMF for
QEMU", and solve this feature.

In summary, I have had patches for TianoCore#386 ready for 2+ years,
they've been blocked because they only target QEMU (with a build-time
flag), and I expect to upstream them finally as soon as OvmfPkg offers
dedicated firmware platforms for Xen and QEMU separately.

Thanks
Laszlo

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Liming Gao]

Laszlo:

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Wednesday, August 21, 2019 4:46 PM
> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
> 
> Hi Liming,
> 
> On 08/19/19 02:40, Gao, Liming wrote:
> 
> >> ... Ugh, wait. I've actually implemented this for OVMF almost 2 years
> >> ago! And the answer to my question is "yes":
> >>
> >>  https://bugzilla.tianocore.org/show_bug.cgi?id=386
> >>  https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html
> >>
> >> See the OnReadOnlyVariable2Available() function:
> >>
> >> +  //
> >> +  // Check if the "MemoryTypeInformation" variable exists, in the
> >> +  // gEfiMemoryTypeInformationGuid namespace.
> >> +  //
> >> +  ReadOnlyVariable2 = Ppi;
> >> +  DataSize = 0;
> >> +  Status = ReadOnlyVariable2->GetVariable (
> >> +                                ReadOnlyVariable2,
> >> +                                EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
> >> +                                &gEfiMemoryTypeInformationGuid,
> >> +                                NULL,
> >> +                                &DataSize,
> >> +                                NULL
> >> +                                );
> >> +  if (Status == EFI_BUFFER_TOO_SMALL) {
> >> +    //
> >> +    // The variable exists; the DXE IPL PEIM will build the HOB from it.
> >> +    //
> >> +    return EFI_SUCCESS;
> >> +  }
> >> +  //
> >> +  // Install the default memory type information HOB.
> >> +  //
> >> +  BuildMemTypeInfoHob ();
> >>
> >> Apologies for forgetting about this; you are right.
> >>
> >> Too bad my work for TianoCore#386 was rejected. :(
> >>
> >
> > So, this is one value to add PEI variable support in OVMF.
> > Do you consider to approve this feature request?
> 
> Sorry, I don't understand your question.
> 
> Are you asking me if, in my opinion, we should fix TianoCore#386 (= add
> PEI variable support to OVMF)?

Yes. Fix TianoCore#386 is my suggestion.

> 
> If that is your question, then my answer is -- trivially -- "yes". If
> you read through TianoCore#386, you will see that I submitted patches
> for solving the BZ, twice (comment 6, comment 9).

I see your patch link in BZ. 

> 
> Jordan rejected my patches both times, because he thought that the same
> feature should be implemented in OVMF for Xen as well. I disagreed (and
> still disagree) -- my patches depend on a build-time flag that produces
> an OVMF binary that is only compatible with QEMU, and not Xen. The
> reason for that is that the feature (PEI variable support) cannot be
> implemented *sensibly* on Xen. (Xen offers no spec-conformant variable
> storage, and in the PEI phase, the variable store would always appear
> empty.) Later, Jordan tried to add dynamic pflash detection to PEI, but
> it didn't work in all cases, because OVMF's PEI phase doesn't run in
> SMM, while QEMU restricts pflash access to SMM. (In other words, pflash
> protection in QEMU is less flexible than SMRAM protection -- SMRAM is
> unlocked in PEI, but pflash is always locked.) Please see the threads
> linked in the BZ for the discussion.

Thanks for you detail information. I miss them before. 

> 
> With TianoCore#1689, Anthony has started separating OVMF into different
> firmware platforms for Xen and QEMU. In a year or so, "OVMF for QEMU"
> will likely have nothing Xen-related in it (because "OVMF for Xen" will
> exist separately). Then we can reopen TianoCore#386 just for "OVMF for
> QEMU", and solve this feature.

TianoCore#1689 is in edk2 stable tag 201908 feature planning. 
Dose it still catch 201808 stable tag?

> 
> In summary, I have had patches for TianoCore#386 ready for 2+ years,
> they've been blocked because they only target QEMU (with a build-time
> flag), and I expect to upstream them finally as soon as OvmfPkg offers
> dedicated firmware platforms for Xen and QEMU separately.

How about add BZ386 for 201911 stable tag?

Thanks
Liming
> 
> Thanks
> Laszlo
> 
> 

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Laszlo Ersek]

(+Anthony, +Jordan)

On 08/21/19 16:14, Gao, Liming wrote:
> Laszlo:
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
>> Sent: Wednesday, August 21, 2019 4:46 PM
>> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
>> <dandan.bi@intel.com>
>> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
>>
>> Hi Liming,
>>
>> On 08/19/19 02:40, Gao, Liming wrote:
>>
>>>> ... Ugh, wait. I've actually implemented this for OVMF almost 2 years
>>>> ago! And the answer to my question is "yes":
>>>>
>>>>  https://bugzilla.tianocore.org/show_bug.cgi?id=386
>>>>  https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html
>>>>
>>>> See the OnReadOnlyVariable2Available() function:
>>>>
>>>> +  //
>>>> +  // Check if the "MemoryTypeInformation" variable exists, in the
>>>> +  // gEfiMemoryTypeInformationGuid namespace.
>>>> +  //
>>>> +  ReadOnlyVariable2 = Ppi;
>>>> +  DataSize = 0;
>>>> +  Status = ReadOnlyVariable2->GetVariable (
>>>> +                                ReadOnlyVariable2,
>>>> +                                EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
>>>> +                                &gEfiMemoryTypeInformationGuid,
>>>> +                                NULL,
>>>> +                                &DataSize,
>>>> +                                NULL
>>>> +                                );
>>>> +  if (Status == EFI_BUFFER_TOO_SMALL) {
>>>> +    //
>>>> +    // The variable exists; the DXE IPL PEIM will build the HOB from it.
>>>> +    //
>>>> +    return EFI_SUCCESS;
>>>> +  }
>>>> +  //
>>>> +  // Install the default memory type information HOB.
>>>> +  //
>>>> +  BuildMemTypeInfoHob ();
>>>>
>>>> Apologies for forgetting about this; you are right.
>>>>
>>>> Too bad my work for TianoCore#386 was rejected. :(
>>>>
>>>
>>> So, this is one value to add PEI variable support in OVMF.
>>> Do you consider to approve this feature request?
>>
>> Sorry, I don't understand your question.
>>
>> Are you asking me if, in my opinion, we should fix TianoCore#386 (= add
>> PEI variable support to OVMF)?
> 
> Yes. Fix TianoCore#386 is my suggestion.
> 
>>
>> If that is your question, then my answer is -- trivially -- "yes". If
>> you read through TianoCore#386, you will see that I submitted patches
>> for solving the BZ, twice (comment 6, comment 9).
> 
> I see your patch link in BZ. 
> 
>>
>> Jordan rejected my patches both times, because he thought that the same
>> feature should be implemented in OVMF for Xen as well. I disagreed (and
>> still disagree) -- my patches depend on a build-time flag that produces
>> an OVMF binary that is only compatible with QEMU, and not Xen. The
>> reason for that is that the feature (PEI variable support) cannot be
>> implemented *sensibly* on Xen. (Xen offers no spec-conformant variable
>> storage, and in the PEI phase, the variable store would always appear
>> empty.) Later, Jordan tried to add dynamic pflash detection to PEI, but
>> it didn't work in all cases, because OVMF's PEI phase doesn't run in
>> SMM, while QEMU restricts pflash access to SMM. (In other words, pflash
>> protection in QEMU is less flexible than SMRAM protection -- SMRAM is
>> unlocked in PEI, but pflash is always locked.) Please see the threads
>> linked in the BZ for the discussion.
> 
> Thanks for you detail information. I miss them before. 
> 
>>
>> With TianoCore#1689, Anthony has started separating OVMF into different
>> firmware platforms for Xen and QEMU. In a year or so, "OVMF for QEMU"
>> will likely have nothing Xen-related in it (because "OVMF for Xen" will
>> exist separately). Then we can reopen TianoCore#386 just for "OVMF for
>> QEMU", and solve this feature.
> 
> TianoCore#1689 is in edk2 stable tag 201908 feature planning. 
> Dose it still catch 201808 stable tag?

Yes, I pushed Anthony's v5 series yesterday, and closed TianoCore#1689.

TianoCore#1689 is also listed on the feature planning page, as "New
OvmfXen platform with Xen PVH support":

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning

> 
>>
>> In summary, I have had patches for TianoCore#386 ready for 2+ years,
>> they've been blocked because they only target QEMU (with a build-time
>> flag), and I expect to upstream them finally as soon as OvmfPkg offers
>> dedicated firmware platforms for Xen and QEMU separately.
> 
> How about add BZ386 for 201911 stable tag?

That would make me very happy, but I don't think we can do it so quickly
(in three months). Here's why: TianoCore#1689 has introduced the new,
dedicated OVMF Xen platform, but it has not removed the Xen parts from
the "traditional" OVMF platform. The separation between "OVMF for Xen"
and "OVMF for QEMU" is not complete yet.

This is the current status:

- OvmfXen:
  - runs in Xen HVM guests
  - runs in Xen PVH guests

- traditional OVMF
  - runs in Xen HVM guests
  - runs in QEMU/KVM guests

The desired (and agreed upon) end-status is the following:

- OvmfXen:
  - runs in Xen HVM guests
  - runs in Xen PVH guests

- traditional OVMF
  - runs in QEMU/KVM guests

(This will match the platform separation that we have under ArmVirtPkg too.)

Anthony plans to remove the Xen HVM code from traditional OVMF in a year
or so, if I understand correctly. That's when "traditional OVMF" will
become QEMU/KVM-only, completing the separation. And that's when I
expect we can fix TianoCore#386 for QEMU/KVM *only*, without Jordan
NACKing the patch set.

Basically, "PcdMemVarstoreEmuEnable" would be constant TRUE in OvmfXen
(inherited from the OVMF DEC file), and it would be exposed to the
platform builder via "-D MEM_VARSTORE_EMU_ENABLE=FALSE" in the "OVMF for
QEMU/KVM" platform. FaultTolerantWritePei and VariablePei would be
included in the "OVMF for QEMU/KVM" DSC files only, and so on.

Thanks
Laszlo

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Liming Gao]

Laszlo:

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, August 22, 2019 7:56 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Mike Turner <miketur@microsoft.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Anthony Perard
> <anthony.perard@citrix.com>; Justen, Jordan L <jordan.l.justen@intel.com>
> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
> 
> (+Anthony, +Jordan)
> 
> On 08/21/19 16:14, Gao, Liming wrote:
> > Laszlo:
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> >> Sent: Wednesday, August 21, 2019 4:46 PM
> >> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> >> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> >> <dandan.bi@intel.com>
> >> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
> >>
> >> Hi Liming,
> >>
> >> On 08/19/19 02:40, Gao, Liming wrote:
> >>
> >>>> ... Ugh, wait. I've actually implemented this for OVMF almost 2 years
> >>>> ago! And the answer to my question is "yes":
> >>>>
> >>>>  https://bugzilla.tianocore.org/show_bug.cgi?id=386
> >>>>  https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html
> >>>>
> >>>> See the OnReadOnlyVariable2Available() function:
> >>>>
> >>>> +  //
> >>>> +  // Check if the "MemoryTypeInformation" variable exists, in the
> >>>> +  // gEfiMemoryTypeInformationGuid namespace.
> >>>> +  //
> >>>> +  ReadOnlyVariable2 = Ppi;
> >>>> +  DataSize = 0;
> >>>> +  Status = ReadOnlyVariable2->GetVariable (
> >>>> +                                ReadOnlyVariable2,
> >>>> +                                EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
> >>>> +                                &gEfiMemoryTypeInformationGuid,
> >>>> +                                NULL,
> >>>> +                                &DataSize,
> >>>> +                                NULL
> >>>> +                                );
> >>>> +  if (Status == EFI_BUFFER_TOO_SMALL) {
> >>>> +    //
> >>>> +    // The variable exists; the DXE IPL PEIM will build the HOB from it.
> >>>> +    //
> >>>> +    return EFI_SUCCESS;
> >>>> +  }
> >>>> +  //
> >>>> +  // Install the default memory type information HOB.
> >>>> +  //
> >>>> +  BuildMemTypeInfoHob ();
> >>>>
> >>>> Apologies for forgetting about this; you are right.
> >>>>
> >>>> Too bad my work for TianoCore#386 was rejected. :(
> >>>>
> >>>
> >>> So, this is one value to add PEI variable support in OVMF.
> >>> Do you consider to approve this feature request?
> >>
> >> Sorry, I don't understand your question.
> >>
> >> Are you asking me if, in my opinion, we should fix TianoCore#386 (= add
> >> PEI variable support to OVMF)?
> >
> > Yes. Fix TianoCore#386 is my suggestion.
> >
> >>
> >> If that is your question, then my answer is -- trivially -- "yes". If
> >> you read through TianoCore#386, you will see that I submitted patches
> >> for solving the BZ, twice (comment 6, comment 9).
> >
> > I see your patch link in BZ.
> >
> >>
> >> Jordan rejected my patches both times, because he thought that the same
> >> feature should be implemented in OVMF for Xen as well. I disagreed (and
> >> still disagree) -- my patches depend on a build-time flag that produces
> >> an OVMF binary that is only compatible with QEMU, and not Xen. The
> >> reason for that is that the feature (PEI variable support) cannot be
> >> implemented *sensibly* on Xen. (Xen offers no spec-conformant variable
> >> storage, and in the PEI phase, the variable store would always appear
> >> empty.) Later, Jordan tried to add dynamic pflash detection to PEI, but
> >> it didn't work in all cases, because OVMF's PEI phase doesn't run in
> >> SMM, while QEMU restricts pflash access to SMM. (In other words, pflash
> >> protection in QEMU is less flexible than SMRAM protection -- SMRAM is
> >> unlocked in PEI, but pflash is always locked.) Please see the threads
> >> linked in the BZ for the discussion.
> >
> > Thanks for you detail information. I miss them before.
> >
> >>
> >> With TianoCore#1689, Anthony has started separating OVMF into different
> >> firmware platforms for Xen and QEMU. In a year or so, "OVMF for QEMU"
> >> will likely have nothing Xen-related in it (because "OVMF for Xen" will
> >> exist separately). Then we can reopen TianoCore#386 just for "OVMF for
> >> QEMU", and solve this feature.
> >
> > TianoCore#1689 is in edk2 stable tag 201908 feature planning.
> > Dose it still catch 201808 stable tag?
> 
> Yes, I pushed Anthony's v5 series yesterday, and closed TianoCore#1689.
> 
> TianoCore#1689 is also listed on the feature planning page, as "New
> OvmfXen platform with Xen PVH support":
> 
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
> 
Thanks. I have seen this feature is done. 

> >
> >>
> >> In summary, I have had patches for TianoCore#386 ready for 2+ years,
> >> they've been blocked because they only target QEMU (with a build-time
> >> flag), and I expect to upstream them finally as soon as OvmfPkg offers
> >> dedicated firmware platforms for Xen and QEMU separately.
> >
> > How about add BZ386 for 201911 stable tag?
> 
> That would make me very happy, but I don't think we can do it so quickly
> (in three months). Here's why: TianoCore#1689 has introduced the new,
> dedicated OVMF Xen platform, but it has not removed the Xen parts from
> the "traditional" OVMF platform. The separation between "OVMF for Xen"
> and "OVMF for QEMU" is not complete yet.
> 
> This is the current status:
> 
> - OvmfXen:
>   - runs in Xen HVM guests
>   - runs in Xen PVH guests
> 
> - traditional OVMF
>   - runs in Xen HVM guests
>   - runs in QEMU/KVM guests
> 
> The desired (and agreed upon) end-status is the following:
> 
> - OvmfXen:
>   - runs in Xen HVM guests
>   - runs in Xen PVH guests
> 
> - traditional OVMF
>   - runs in QEMU/KVM guests
> 
> (This will match the platform separation that we have under ArmVirtPkg too.)
> 
> Anthony plans to remove the Xen HVM code from traditional OVMF in a year
> or so, if I understand correctly. That's when "traditional OVMF" will
> become QEMU/KVM-only, completing the separation. And that's when I
> expect we can fix TianoCore#386 for QEMU/KVM *only*, without Jordan
> NACKing the patch set.
> 
Ok. Is there BZ for this change? The contributor can propose his work planning. 
Then, I will update edk2 feature planning wiki. 

> Basically, "PcdMemVarstoreEmuEnable" would be constant TRUE in OvmfXen
> (inherited from the OVMF DEC file), and it would be exposed to the
> platform builder via "-D MEM_VARSTORE_EMU_ENABLE=FALSE" in the "OVMF for
> QEMU/KVM" platform. FaultTolerantWritePei and VariablePei would be
> included in the "OVMF for QEMU/KVM" DSC files only, and so on.
> 
I think this design meets with the request BZ386. 

Thanks
Liming

> Thanks
> Laszlo

Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

[Laszlo Ersek]

On 08/22/19 16:52, Gao, Liming wrote:
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Thursday, August 22, 2019 7:56 PM
>> To: Gao, Liming <liming.gao@intel.com>
>> Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Mike Turner <miketur@microsoft.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Anthony Perard
>> <anthony.perard@citrix.com>; Justen, Jordan L <jordan.l.justen@intel.com>
>> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

>> This is the current status:
>>
>> - OvmfXen:
>>   - runs in Xen HVM guests
>>   - runs in Xen PVH guests
>>
>> - traditional OVMF
>>   - runs in Xen HVM guests
>>   - runs in QEMU/KVM guests
>>
>> The desired (and agreed upon) end-status is the following:
>>
>> - OvmfXen:
>>   - runs in Xen HVM guests
>>   - runs in Xen PVH guests
>>
>> - traditional OVMF
>>   - runs in QEMU/KVM guests
>>
>> (This will match the platform separation that we have under ArmVirtPkg too.)
>>
>> Anthony plans to remove the Xen HVM code from traditional OVMF in a year
>> or so, if I understand correctly. That's when "traditional OVMF" will
>> become QEMU/KVM-only, completing the separation. And that's when I
>> expect we can fix TianoCore#386 for QEMU/KVM *only*, without Jordan
>> NACKing the patch set.
>>
> Ok. Is there BZ for this change? The contributor can propose his work planning. 
> Then, I will update edk2 feature planning wiki.

I've now reported:

https://bugzilla.tianocore.org/show_bug.cgi?id=2122

and assigned it to Anthony. (Anthony, I hope that's OK with you.)

>> Basically, "PcdMemVarstoreEmuEnable" would be constant TRUE in OvmfXen
>> (inherited from the OVMF DEC file), and it would be exposed to the
>> platform builder via "-D MEM_VARSTORE_EMU_ENABLE=FALSE" in the "OVMF for
>> QEMU/KVM" platform. FaultTolerantWritePei and VariablePei would be
>> included in the "OVMF for QEMU/KVM" DSC files only, and so on.
>>
> I think this design meets with the request BZ386. 

I've also reopened

https://bugzilla.tianocore.org/show_bug.cgi?id=386

now (moving it back to CONFIRMED state, from RESOLVED|WONTFIX), and I've
made it dependent on TianoCore#2122.

Thanks,
Laszlo