[PATCH 0/3] Fix bugs in core ResetSystem software stack

[PATCH 0/3] Fix bugs in core ResetSystem software stack

[Ruiyu Ni]

Ruiyu Ni (3):
  MdeModulePkg/DxeResetSystemLib: Avoid depending on UefiRuntimeLib
  MdeModulePkg: Make sure ResetSystemRuntimeDxe uses ResetSystemLibNull
  MdePkg/UefiRuntimeLib: Do not allow to be linked by DXE driver

 MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c   | 10 +++++-----
 MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf |  3 +--
 MdeModulePkg/MdeModulePkg.dsc                                |  5 ++++-
 MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf             |  4 ++--
 4 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.16.1.windows.1

[PATCH 1/3] MdeModulePkg/DxeResetSystemLib: Avoid depending on UefiRuntimeLib

[Ruiyu Ni]

Current DxeResetSystemLib depends on UefiRuntimeLib because it calls
EfiResetSystem() API exposed by UefiRuntimeLib.

Due to the commit XXX which reverts UefiRuntimeLib to only support
DXE_RUNTIME_DRIVER, removing UefiRuntimeLib dependency makes the
DxeResetSystemLib can be used by DXE drivers.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---
 MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c   | 10 +++++-----
 MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf |  3 +--
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c b/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c
index ea452e3231..76bca223ae 100644
--- a/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c
+++ b/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c
@@ -14,7 +14,7 @@
 
 #include <PiDxe.h>
 #include <Library/ResetSystemLib.h>
-#include <Library/UefiRuntimeLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>
 
 /**
   This function causes a system-wide reset (cold reset), in which
@@ -30,7 +30,7 @@ ResetCold (
   VOID
   )
 {
-  EfiResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
+  gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
 }
 
 /**
@@ -45,7 +45,7 @@ ResetWarm (
   VOID
   )
 {
-  EfiResetSystem (EfiResetWarm, EFI_SUCCESS, 0, NULL);
+  gRT->ResetSystem (EfiResetWarm, EFI_SUCCESS, 0, NULL);
 }
 
 /**
@@ -60,7 +60,7 @@ ResetShutdown (
   VOID
   )
 {
-  EfiResetSystem (EfiResetShutdown, EFI_SUCCESS, 0, NULL);
+  gRT->ResetSystem (EfiResetShutdown, EFI_SUCCESS, 0, NULL);
 }
 
 /**
@@ -94,5 +94,5 @@ ResetPlatformSpecific (
   IN VOID    *ResetData
   )
 {
-  EfiResetSystem (EfiResetPlatformSpecific, EFI_SUCCESS, DataSize, ResetData);
+  gRT->ResetSystem (EfiResetPlatformSpecific, EFI_SUCCESS, DataSize, ResetData);
 }
diff --git a/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf b/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf
index 6eb2766b93..0d97d5899b 100644
--- a/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf
+++ b/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf
@@ -35,5 +35,4 @@ [Packages]
   MdeModulePkg/MdeModulePkg.dec
 
 [LibraryClasses]
-  UefiRuntimeLib
-
+  UefiRuntimeServicesTableLib
-- 
2.16.1.windows.1

[PATCH 2/3] MdeModulePkg: Make sure ResetSystemRuntimeDxe uses ResetSystemLibNull

[Ruiyu Ni]

Because the DxeResetSystemLib calls gRT->ResetSystem(), make sure
the gRT->ResetSystem() implementation doesn't call into
DxeResetSystemLib to avoid chicken-egg issue.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---
 MdeModulePkg/MdeModulePkg.dsc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index ec24a50c7d..734c5e5ab7 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -363,7 +363,10 @@ [Components]
     <LibraryClasses>
       ResetSystemLib|MdeModulePkg/Library/BaseResetSystemLibNull/BaseResetSystemLibNull.inf
   }
-  MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
+  MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf {
+    <LibraryClasses>
+      ResetSystemLib|MdeModulePkg/Library/BaseResetSystemLibNull/BaseResetSystemLibNull.inf
+  }
   MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
   MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.inf
 
-- 
2.16.1.windows.1

[PATCH 3/3] MdePkg/UefiRuntimeLib: Do not allow to be linked by DXE driver

[Ruiyu Ni]

When UefiRuntimeLib links to a DXE driver, its constructor
still registers a Virtual Address Change event. The event callback
will get called when RT.SetVirtualAddressMap() is called from OS.
But when the driver is a DXE driver, the memory occupied by the
callback function might be zeroed or used by OS since the BS type
memory is free memory when entering to RT phase.

The patch reverts commit 97511979b4fdd84cf7cd51e43c22dc03e79bd4f3
"MdePkg/UefiRuntimeLib: Support more module types."
It makes sure that DXE driver cannot link to this library.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf b/MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
index d053da545a..8f46495fc5 100644
--- a/MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
+++ b/MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
@@ -5,7 +5,7 @@
 #  EVT_SIGNAL_EXIT_BOOT_SERVICES event, to provide runtime services.
 # This instance also supports SAL drivers for better performance.
 #
-# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
 #
 #  This program and the accompanying materials
 #  are licensed and made available under the terms and conditions of the BSD License
@@ -24,7 +24,7 @@ [Defines]
   FILE_GUID                      = b1ee6c28-54aa-4d17-b705-3e28ccb27b2e
   MODULE_TYPE                    = DXE_RUNTIME_DRIVER
   VERSION_STRING                 = 1.0
-  LIBRARY_CLASS                  = UefiRuntimeLib|DXE_RUNTIME_DRIVER DXE_SAL_DRIVER DXE_CORE DXE_DRIVER DXE_SMM_DRIVER 
+  LIBRARY_CLASS                  = UefiRuntimeLib|DXE_RUNTIME_DRIVER DXE_SAL_DRIVER
 
   CONSTRUCTOR                    = RuntimeDriverLibConstruct
   DESTRUCTOR                     = RuntimeDriverLibDeconstruct
-- 
2.16.1.windows.1

Re: [PATCH 3/3] MdePkg/UefiRuntimeLib: Do not allow to be linked by DXE driver

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Friday, June 1, 2018 3:22 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 3/3] MdePkg/UefiRuntimeLib: Do not allow to be linked by DXE driver
> 
> When UefiRuntimeLib links to a DXE driver, its constructor
> still registers a Virtual Address Change event. The event callback
> will get called when RT.SetVirtualAddressMap() is called from OS.
> But when the driver is a DXE driver, the memory occupied by the
> callback function might be zeroed or used by OS since the BS type
> memory is free memory when entering to RT phase.
> 
> The patch reverts commit 97511979b4fdd84cf7cd51e43c22dc03e79bd4f3
> "MdePkg/UefiRuntimeLib: Support more module types."
> It makes sure that DXE driver cannot link to this library.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
>  MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf b/MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
> index d053da545a..8f46495fc5 100644
> --- a/MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
> +++ b/MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
> @@ -5,7 +5,7 @@
>  #  EVT_SIGNAL_EXIT_BOOT_SERVICES event, to provide runtime services.
>  # This instance also supports SAL drivers for better performance.
>  #
> -# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
> @@ -24,7 +24,7 @@ [Defines]
>    FILE_GUID                      = b1ee6c28-54aa-4d17-b705-3e28ccb27b2e
>    MODULE_TYPE                    = DXE_RUNTIME_DRIVER
>    VERSION_STRING                 = 1.0
> -  LIBRARY_CLASS                  = UefiRuntimeLib|DXE_RUNTIME_DRIVER DXE_SAL_DRIVER DXE_CORE DXE_DRIVER
> DXE_SMM_DRIVER
> +  LIBRARY_CLASS                  = UefiRuntimeLib|DXE_RUNTIME_DRIVER DXE_SAL_DRIVER
> 
>    CONSTRUCTOR                    = RuntimeDriverLibConstruct
>    DESTRUCTOR                     = RuntimeDriverLibDeconstruct
> --
> 2.16.1.windows.1

Re: [PATCH 1/3] MdeModulePkg/DxeResetSystemLib: Avoid depending on UefiRuntimeLib

[Laszlo Ersek]

Hi Ray,

On 06/01/18 09:22, Ruiyu Ni wrote:
> Current DxeResetSystemLib depends on UefiRuntimeLib because it calls
> EfiResetSystem() API exposed by UefiRuntimeLib.
> 
> Due to the commit XXX which reverts UefiRuntimeLib to only support

I really just cursorily skimmed the commit messages here, but I think
you forgot to update the XXX placeholder above.

Thanks!
Laszlo

Re: [PATCH 1/3] MdeModulePkg/DxeResetSystemLib: Avoid depending on UefiRuntimeLib

[Zeng, Star]

Could interfaces in DxeResetSystemLib be used by runtime code?
If yes, gRT needs to be converted for runtime environment as we discussed about it at https://lists.01.org/pipermail/edk2-devel/2018-February/021260.html before.



Thanks,
Star

-----Original Message-----
From: Ni, Ruiyu 
Sent: Friday, June 1, 2018 3:22 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: [PATCH 1/3] MdeModulePkg/DxeResetSystemLib: Avoid depending on UefiRuntimeLib

Current DxeResetSystemLib depends on UefiRuntimeLib because it calls
EfiResetSystem() API exposed by UefiRuntimeLib.

Due to the commit XXX which reverts UefiRuntimeLib to only support DXE_RUNTIME_DRIVER, removing UefiRuntimeLib dependency makes the DxeResetSystemLib can be used by DXE drivers.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---
 MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c   | 10 +++++-----
 MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf |  3 +--
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c b/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c
index ea452e3231..76bca223ae 100644
--- a/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c
+++ b/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c
@@ -14,7 +14,7 @@
 
 #include <PiDxe.h>
 #include <Library/ResetSystemLib.h>
-#include <Library/UefiRuntimeLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>
 
 /**
   This function causes a system-wide reset (cold reset), in which @@ -30,7 +30,7 @@ ResetCold (
   VOID
   )
 {
-  EfiResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
+  gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
 }
 
 /**
@@ -45,7 +45,7 @@ ResetWarm (
   VOID
   )
 {
-  EfiResetSystem (EfiResetWarm, EFI_SUCCESS, 0, NULL);
+  gRT->ResetSystem (EfiResetWarm, EFI_SUCCESS, 0, NULL);
 }
 
 /**
@@ -60,7 +60,7 @@ ResetShutdown (
   VOID
   )
 {
-  EfiResetSystem (EfiResetShutdown, EFI_SUCCESS, 0, NULL);
+  gRT->ResetSystem (EfiResetShutdown, EFI_SUCCESS, 0, NULL);
 }
 
 /**
@@ -94,5 +94,5 @@ ResetPlatformSpecific (
   IN VOID    *ResetData
   )
 {
-  EfiResetSystem (EfiResetPlatformSpecific, EFI_SUCCESS, DataSize, ResetData);
+  gRT->ResetSystem (EfiResetPlatformSpecific, EFI_SUCCESS, DataSize, 
+ ResetData);
 }
diff --git a/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf b/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf
index 6eb2766b93..0d97d5899b 100644
--- a/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf
+++ b/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf
@@ -35,5 +35,4 @@ [Packages]
   MdeModulePkg/MdeModulePkg.dec
 
 [LibraryClasses]
-  UefiRuntimeLib
-
+  UefiRuntimeServicesTableLib
--
2.16.1.windows.1

Re: [PATCH 1/3] MdeModulePkg/DxeResetSystemLib: Avoid depending on UefiRuntimeLib

[Ni, Ruiyu]

Haha, you are correct! I forgot to update the commit messages!
My bad!!
I will post a new version to address all concerns, including Star's.

Thanks/Ray

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Saturday, June 2, 2018 3:06 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 1/3] MdeModulePkg/DxeResetSystemLib: Avoid
> depending on UefiRuntimeLib
> 
> Hi Ray,
> 
> On 06/01/18 09:22, Ruiyu Ni wrote:
> > Current DxeResetSystemLib depends on UefiRuntimeLib because it calls
> > EfiResetSystem() API exposed by UefiRuntimeLib.
> >
> > Due to the commit XXX which reverts UefiRuntimeLib to only support
> 
> I really just cursorily skimmed the commit messages here, but I think you
> forgot to update the XXX placeholder above.
> 
> Thanks!
> Laszlo

Re: [PATCH 1/3] MdeModulePkg/DxeResetSystemLib: Avoid depending on UefiRuntimeLib

[Ni, Ruiyu]

No.
DxeResetSystemLib cannot be used by runtime driver.
1. the gRT is not converted by itself.
2. Even if we change the library implementation to convert gRT in VirtualAddressChange callback,
when DXE driver links to this library, the problem similar to UefiRuntimeLib still occurs.

So I will make a V2 patch to remove runtime driver support.

Thanks/Ray

> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, June 4, 2018 10:37 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH 1/3] MdeModulePkg/DxeResetSystemLib: Avoid
> depending on UefiRuntimeLib
> 
> Could interfaces in DxeResetSystemLib be used by runtime code?
> If yes, gRT needs to be converted for runtime environment as we discussed
> about it at https://lists.01.org/pipermail/edk2-devel/2018-
> February/021260.html before.
> 
> 
> 
> Thanks,
> Star
> 
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Friday, June 1, 2018 3:22 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [PATCH 1/3] MdeModulePkg/DxeResetSystemLib: Avoid depending
> on UefiRuntimeLib
> 
> Current DxeResetSystemLib depends on UefiRuntimeLib because it calls
> EfiResetSystem() API exposed by UefiRuntimeLib.
> 
> Due to the commit XXX which reverts UefiRuntimeLib to only support
> DXE_RUNTIME_DRIVER, removing UefiRuntimeLib dependency makes the
> DxeResetSystemLib can be used by DXE drivers.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
>  MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c   | 10
> +++++-----
>  MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf |  3 +--
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c
> b/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c
> index ea452e3231..76bca223ae 100644
> --- a/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c
> +++ b/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.c
> @@ -14,7 +14,7 @@
> 
>  #include <PiDxe.h>
>  #include <Library/ResetSystemLib.h>
> -#include <Library/UefiRuntimeLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
> 
>  /**
>    This function causes a system-wide reset (cold reset), in which @@ -30,7
> +30,7 @@ ResetCold (
>    VOID
>    )
>  {
> -  EfiResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
> +  gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
>  }
> 
>  /**
> @@ -45,7 +45,7 @@ ResetWarm (
>    VOID
>    )
>  {
> -  EfiResetSystem (EfiResetWarm, EFI_SUCCESS, 0, NULL);
> +  gRT->ResetSystem (EfiResetWarm, EFI_SUCCESS, 0, NULL);
>  }
> 
>  /**
> @@ -60,7 +60,7 @@ ResetShutdown (
>    VOID
>    )
>  {
> -  EfiResetSystem (EfiResetShutdown, EFI_SUCCESS, 0, NULL);
> +  gRT->ResetSystem (EfiResetShutdown, EFI_SUCCESS, 0, NULL);
>  }
> 
>  /**
> @@ -94,5 +94,5 @@ ResetPlatformSpecific (
>    IN VOID    *ResetData
>    )
>  {
> -  EfiResetSystem (EfiResetPlatformSpecific, EFI_SUCCESS, DataSize,
> ResetData);
> +  gRT->ResetSystem (EfiResetPlatformSpecific, EFI_SUCCESS, DataSize,
> + ResetData);
>  }
> diff --git
> a/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf
> b/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf
> index 6eb2766b93..0d97d5899b 100644
> --- a/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf
> +++ b/MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf
> @@ -35,5 +35,4 @@ [Packages]
>    MdeModulePkg/MdeModulePkg.dec
> 
>  [LibraryClasses]
> -  UefiRuntimeLib
> -
> +  UefiRuntimeServicesTableLib
> --
> 2.16.1.windows.1