[edk2-devel] [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for missing Memory Attributes Table (MAT) update

Liming Gao posted 1 patch 4 years, 7 months ago
Failed in applying to current master (apply log)
MdeModulePkg/Core/Dxe/Mem/Page.c | 41 ++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)
[edk2-devel] [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for missing Memory Attributes Table (MAT) update
Posted by Liming Gao 4 years, 7 months ago
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 (S4).

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

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.
3. Update comments style to follow edk2 style.

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>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
In v2, add more description for this issue. 

 MdeModulePkg/Core/Dxe/Mem/Page.c | 41 ++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index bd9e116aa5..1f0e3d94b9 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,33 @@ CoreInternalAllocatePages (
         (End > MaxAddress)) {
       return EFI_NOT_FOUND;
     }
+
+    //
+    // 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45825): https://edk2.groups.io/g/devel/message/45825
Mute This Topic: https://groups.io/mt/32899946/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for missing Memory Attributes Table (MAT) update
Posted by Laszlo Ersek 4 years, 7 months ago
On 08/16/19 17:39, 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.
> 
> 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 (S4).
> 
> 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 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.
> 
> 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.
> 3. Update comments style to follow edk2 style.
> 
> 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>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> ---
> In v2, add more description for this issue. 
> 
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 41 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index bd9e116aa5..1f0e3d94b9 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,33 @@ CoreInternalAllocatePages (
>          (End > MaxAddress)) {
>        return EFI_NOT_FOUND;
>      }
> +
> +    //
> +    // 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) {
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>

Please wait for maintainer approval too.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45852): https://edk2.groups.io/g/devel/message/45852
Mute This Topic: https://groups.io/mt/32899946/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for missing Memory Attributes Table (MAT) update
Posted by Dandan Bi 4 years, 7 months ago
Jian and Hao, could you also help take a look at this patch?
	
This patch is ok to me.
Reviewed-by: Dandan Bi <dandan.bi@intel.com>


Thanks,
Dandan

> -----Original Message-----
> From: Gao, Liming
> Sent: Friday, August 16, 2019 11:40 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>; Laszlo Ersek <lersek@redhat.com>
> Subject: [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for
> missing Memory Attributes Table (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 (S4).
> 
> 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 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.
> 
> This patch is cherry pick from Project Mu:
> https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af9401
> 6ebd391ea6f340920735a
> 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.
> 3. Update comments style to follow edk2 style.
> 
> 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>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> ---
> In v2, add more description for this issue.
> 
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 41
> ++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index bd9e116aa5..1f0e3d94b9 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,33 @@ CoreInternalAllocatePages (
>          (End > MaxAddress)) {
>        return EFI_NOT_FOUND;
>      }
> +
> +    //
> +    // 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46021): https://edk2.groups.io/g/devel/message/46021
Mute This Topic: https://groups.io/mt/32899946/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for missing Memory Attributes Table (MAT) update
Posted by Wu, Hao A 4 years, 7 months ago
> -----Original Message-----
> From: Gao, Liming
> Sent: Friday, August 16, 2019 11:40 PM
> To: devel@edk2.groups.io
> Cc: Mike Turner; Wang, Jian J; Wu, Hao A; Bi, Dandan; Laszlo Ersek
> Subject: [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for
> missing Memory Attributes Table (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 (S4).
> 
> 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 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.
> 
> This patch is cherry pick from Project Mu:
> https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af9401
> 6ebd391ea6f340920735a
> 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.
> 3. Update comments style to follow edk2 style.
> 
> 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>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> ---
> In v2, add more description for this issue.
> 
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 41
> ++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index bd9e116aa5..1f0e3d94b9 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,33 @@ CoreInternalAllocatePages (
>          (End > MaxAddress)) {
>        return EFI_NOT_FOUND;
>      }
> +
> +    //
> +    // 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;
> +        }
> +      }
> +    }


Acked-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


>    }
> 
>    if (Type == AllocateMaxAddress) {
> --
> 2.13.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46022): https://edk2.groups.io/g/devel/message/46022
Mute This Topic: https://groups.io/mt/32899946/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for missing Memory Attributes Table (MAT) update
Posted by Liming Gao 4 years, 7 months ago
Push @ ada905ab5c0e7ea7017e71d52219aaec1abd8dcb

> -----Original Message-----
> From: Wu, Hao A
> Sent: Monday, August 19, 2019 1:42 PM
> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io
> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: RE: [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for missing Memory Attributes Table (MAT) update
> 
> > -----Original Message-----
> > From: Gao, Liming
> > Sent: Friday, August 16, 2019 11:40 PM
> > To: devel@edk2.groups.io
> > Cc: Mike Turner; Wang, Jian J; Wu, Hao A; Bi, Dandan; Laszlo Ersek
> > Subject: [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for
> > missing Memory Attributes Table (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 (S4).
> >
> > 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 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.
> >
> > This patch is cherry pick from Project Mu:
> > https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af9401
> > 6ebd391ea6f340920735a
> > 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.
> > 3. Update comments style to follow edk2 style.
> >
> > 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>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Liming Gao <liming.gao@intel.com>
> > ---
> > In v2, add more description for this issue.
> >
> >  MdeModulePkg/Core/Dxe/Mem/Page.c | 41
> > ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > index bd9e116aa5..1f0e3d94b9 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,33 @@ CoreInternalAllocatePages (
> >          (End > MaxAddress)) {
> >        return EFI_NOT_FOUND;
> >      }
> > +
> > +    //
> > +    // 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;
> > +        }
> > +      }
> > +    }
> 
> 
> Acked-by: Hao A Wu <hao.a.wu@intel.com>
> 
> Best Regards,
> Hao Wu
> 
> 
> >    }
> >
> >    if (Type == AllocateMaxAddress) {
> > --
> > 2.13.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46116): https://edk2.groups.io/g/devel/message/46116
Mute This Topic: https://groups.io/mt/32899946/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-