* [PATCH 1/6] OvmfPkg/ResetSystemLib: wrap long lines
2020-04-17 15:37 [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Laszlo Ersek
@ 2020-04-17 15:37 ` Laszlo Ersek
2020-04-17 16:01 ` Philippe Mathieu-Daudé
2020-04-17 15:37 ` [PATCH 2/6] OvmfPkg/ResetSystemLib: clean up library dependencies Laszlo Ersek
` (7 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2020-04-17 15:37 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé,
Rebecca Cran
Wrap the source code and the INF file at 79 characters.
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf | 3 ++-
OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c | 20 +++++++++++---------
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
index 2f24dac87faf..063dc49f2453 100644
--- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
+++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
@@ -15,7 +15,8 @@ [Defines]
LIBRARY_CLASS = ResetSystemLib
#
-# The following information is for reference only and not required by the build tools.
+# The following information is for reference only and not required by the build
+# tools.
#
# VALID_ARCHITECTURES = IA32 X64
#
diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
index 2f2e1293a3ef..b3abda80e75a 100644
--- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
+++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
@@ -103,10 +103,11 @@ ResetShutdown (
/**
This function causes a systemwide reset. The exact type of the reset is
- defined by the EFI_GUID that follows the Null-terminated Unicode string passed
- into ResetData. If the platform does not recognize the EFI_GUID in ResetData
- the platform must pick a supported reset type to perform.The platform may
- optionally log the parameters from any non-normal reset that occurs.
+ defined by the EFI_GUID that follows the Null-terminated Unicode string
+ passed into ResetData. If the platform does not recognize the EFI_GUID in
+ ResetData the platform must pick a supported reset type to perform.The
+ platform may optionally log the parameters from any non-normal reset that
+ occurs.
@param[in] DataSize The size, in bytes, of ResetData.
@param[in] ResetData The data buffer starts with a Null-terminated string,
@@ -128,11 +129,12 @@ ResetPlatformSpecific (
@param[in] ResetType The type of reset to perform.
@param[in] ResetStatus The status code for the reset.
@param[in] DataSize The size, in bytes, of ResetData.
- @param[in] ResetData For a ResetType of EfiResetCold, EfiResetWarm, or EfiResetShutdown
- the data buffer starts with a Null-terminated string, optionally
- followed by additional binary data. The string is a description
- that the caller may use to further indicate the reason for the
- system reset.
+ @param[in] ResetData For a ResetType of EfiResetCold, EfiResetWarm, or
+ EfiResetShutdown the data buffer starts with a
+ Null-terminated string, optionally followed by
+ additional binary data. The string is a description
+ that the caller may use to further indicate the
+ reason for the system reset.
**/
VOID
EFIAPI
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 1/6] OvmfPkg/ResetSystemLib: wrap long lines
2020-04-17 15:37 ` [PATCH 1/6] OvmfPkg/ResetSystemLib: wrap long lines Laszlo Ersek
@ 2020-04-17 16:01 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-17 16:01 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Ard Biesheuvel, Jordan Justen, Rebecca Cran
On 4/17/20 5:37 PM, Laszlo Ersek wrote:
> Wrap the source code and the INF file at 79 characters.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf | 3 ++-
> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c | 20 +++++++++++---------
> 2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> index 2f24dac87faf..063dc49f2453 100644
> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> +++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> @@ -15,7 +15,8 @@ [Defines]
> LIBRARY_CLASS = ResetSystemLib
>
> #
> -# The following information is for reference only and not required by the build tools.
> +# The following information is for reference only and not required by the build
> +# tools.
> #
> # VALID_ARCHITECTURES = IA32 X64
> #
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> index 2f2e1293a3ef..b3abda80e75a 100644
> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> +++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> @@ -103,10 +103,11 @@ ResetShutdown (
>
> /**
> This function causes a systemwide reset. The exact type of the reset is
> - defined by the EFI_GUID that follows the Null-terminated Unicode string passed
> - into ResetData. If the platform does not recognize the EFI_GUID in ResetData
> - the platform must pick a supported reset type to perform.The platform may
> - optionally log the parameters from any non-normal reset that occurs.
> + defined by the EFI_GUID that follows the Null-terminated Unicode string
> + passed into ResetData. If the platform does not recognize the EFI_GUID in
> + ResetData the platform must pick a supported reset type to perform.The
> + platform may optionally log the parameters from any non-normal reset that
> + occurs.
>
> @param[in] DataSize The size, in bytes, of ResetData.
> @param[in] ResetData The data buffer starts with a Null-terminated string,
> @@ -128,11 +129,12 @@ ResetPlatformSpecific (
> @param[in] ResetType The type of reset to perform.
> @param[in] ResetStatus The status code for the reset.
> @param[in] DataSize The size, in bytes, of ResetData.
> - @param[in] ResetData For a ResetType of EfiResetCold, EfiResetWarm, or EfiResetShutdown
> - the data buffer starts with a Null-terminated string, optionally
> - followed by additional binary data. The string is a description
> - that the caller may use to further indicate the reason for the
> - system reset.
> + @param[in] ResetData For a ResetType of EfiResetCold, EfiResetWarm, or
> + EfiResetShutdown the data buffer starts with a
> + Null-terminated string, optionally followed by
> + additional binary data. The string is a description
> + that the caller may use to further indicate the
> + reason for the system reset.
> **/
> VOID
> EFIAPI
>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/6] OvmfPkg/ResetSystemLib: clean up library dependencies
2020-04-17 15:37 [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Laszlo Ersek
2020-04-17 15:37 ` [PATCH 1/6] OvmfPkg/ResetSystemLib: wrap long lines Laszlo Ersek
@ 2020-04-17 15:37 ` Laszlo Ersek
2020-04-17 16:02 ` Philippe Mathieu-Daudé
2020-04-17 15:37 ` [PATCH 3/6] OvmfPkg/ResetSystemLib: improve coding style in ResetSystem() Laszlo Ersek
` (6 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2020-04-17 15:37 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé,
Rebecca Cran
Annotate the #include directives with the interfaces that this lib
instance needs from the included lib class headers. This will help us keep
the #include set minimal, when we move code around later.
While at it, synchronize the [LibraryClasses] section with the #include
directives -- list BaseLib.
Also #include the ResetSystemLib class header, which declares the
interfaces that this lib instance implements.
This forces us to spell out the "MdeModulePkg.dec" dependency too, under
[Packages].
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf | 2 ++
OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c | 15 ++++++++-------
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
index 063dc49f2453..af20f516c035 100644
--- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
+++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
@@ -25,10 +25,12 @@ [Sources]
ResetSystemLib.c
[Packages]
+ MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ BaseLib
DebugLib
IoLib
PciLib
diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
index b3abda80e75a..23816183a953 100644
--- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
+++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
@@ -6,14 +6,15 @@
**/
-#include <Base.h>
+#include <Base.h> // BIT1
-#include <Library/BaseLib.h>
-#include <Library/DebugLib.h>
-#include <Library/IoLib.h>
-#include <Library/PciLib.h>
-#include <Library/TimerLib.h>
-#include <OvmfPlatforms.h>
+#include <Library/BaseLib.h> // CpuDeadLoop()
+#include <Library/DebugLib.h> // ASSERT()
+#include <Library/IoLib.h> // IoWrite8()
+#include <Library/PciLib.h> // PciRead16()
+#include <Library/ResetSystemLib.h> // ResetCold()
+#include <Library/TimerLib.h> // MicroSecondDelay()
+#include <OvmfPlatforms.h> // OVMF_HOSTBRIDGE_DID
VOID
AcpiPmControl (
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/6] OvmfPkg/ResetSystemLib: clean up library dependencies
2020-04-17 15:37 ` [PATCH 2/6] OvmfPkg/ResetSystemLib: clean up library dependencies Laszlo Ersek
@ 2020-04-17 16:02 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-17 16:02 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Ard Biesheuvel, Jordan Justen, Rebecca Cran
On 4/17/20 5:37 PM, Laszlo Ersek wrote:
> Annotate the #include directives with the interfaces that this lib
> instance needs from the included lib class headers. This will help us keep
> the #include set minimal, when we move code around later.
>
> While at it, synchronize the [LibraryClasses] section with the #include
> directives -- list BaseLib.
>
> Also #include the ResetSystemLib class header, which declares the
> interfaces that this lib instance implements.
>
> This forces us to spell out the "MdeModulePkg.dec" dependency too, under
> [Packages].
>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf | 2 ++
> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c | 15 ++++++++-------
> 2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> index 063dc49f2453..af20f516c035 100644
> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> +++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> @@ -25,10 +25,12 @@ [Sources]
> ResetSystemLib.c
>
> [Packages]
> + MdeModulePkg/MdeModulePkg.dec
> MdePkg/MdePkg.dec
> OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> + BaseLib
> DebugLib
> IoLib
> PciLib
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> index b3abda80e75a..23816183a953 100644
> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> +++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> @@ -6,14 +6,15 @@
>
> **/
>
> -#include <Base.h>
> +#include <Base.h> // BIT1
>
> -#include <Library/BaseLib.h>
> -#include <Library/DebugLib.h>
> -#include <Library/IoLib.h>
> -#include <Library/PciLib.h>
> -#include <Library/TimerLib.h>
> -#include <OvmfPlatforms.h>
> +#include <Library/BaseLib.h> // CpuDeadLoop()
> +#include <Library/DebugLib.h> // ASSERT()
> +#include <Library/IoLib.h> // IoWrite8()
> +#include <Library/PciLib.h> // PciRead16()
> +#include <Library/ResetSystemLib.h> // ResetCold()
> +#include <Library/TimerLib.h> // MicroSecondDelay()
> +#include <OvmfPlatforms.h> // OVMF_HOSTBRIDGE_DID
>
> VOID
> AcpiPmControl (
>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/6] OvmfPkg/ResetSystemLib: improve coding style in ResetSystem()
2020-04-17 15:37 [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Laszlo Ersek
2020-04-17 15:37 ` [PATCH 1/6] OvmfPkg/ResetSystemLib: wrap long lines Laszlo Ersek
2020-04-17 15:37 ` [PATCH 2/6] OvmfPkg/ResetSystemLib: clean up library dependencies Laszlo Ersek
@ 2020-04-17 15:37 ` Laszlo Ersek
2020-04-17 16:02 ` Philippe Mathieu-Daudé
2020-04-17 15:37 ` [PATCH 4/6] OvmfPkg/ResetSystemLib: factor out ResetShutdown() Laszlo Ersek
` (5 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2020-04-17 15:37 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé,
Rebecca Cran
The cases under ResetSystem() currently mix "break"s with "return"s for no
good reason. Use "break" consistently.
(The inconsistency was introduced in commit 84c0b80de716,
"OvmfPkg/ResetSystemLib: Add new API ResetSystem", 2019-04-28.)
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
index 23816183a953..0fc142325ece 100644
--- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
+++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
@@ -157,13 +157,13 @@ ResetSystem (
case EfiResetShutdown:
ResetShutdown ();
- return;
+ break;
case EfiResetPlatformSpecific:
ResetPlatformSpecific (DataSize, ResetData);
- return;
+ break;
default:
- return;
+ break;
}
}
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 3/6] OvmfPkg/ResetSystemLib: improve coding style in ResetSystem()
2020-04-17 15:37 ` [PATCH 3/6] OvmfPkg/ResetSystemLib: improve coding style in ResetSystem() Laszlo Ersek
@ 2020-04-17 16:02 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-17 16:02 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Ard Biesheuvel, Jordan Justen, Rebecca Cran
On 4/17/20 5:37 PM, Laszlo Ersek wrote:
> The cases under ResetSystem() currently mix "break"s with "return"s for no
> good reason. Use "break" consistently.
>
> (The inconsistency was introduced in commit 84c0b80de716,
> "OvmfPkg/ResetSystemLib: Add new API ResetSystem", 2019-04-28.)
>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> index 23816183a953..0fc142325ece 100644
> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> +++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> @@ -157,13 +157,13 @@ ResetSystem (
>
> case EfiResetShutdown:
> ResetShutdown ();
> - return;
> + break;
>
> case EfiResetPlatformSpecific:
> ResetPlatformSpecific (DataSize, ResetData);
> - return;
> + break;
>
> default:
> - return;
> + break;
> }
> }
>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/6] OvmfPkg/ResetSystemLib: factor out ResetShutdown()
2020-04-17 15:37 [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Laszlo Ersek
` (2 preceding siblings ...)
2020-04-17 15:37 ` [PATCH 3/6] OvmfPkg/ResetSystemLib: improve coding style in ResetSystem() Laszlo Ersek
@ 2020-04-17 15:37 ` Laszlo Ersek
2020-04-17 16:05 ` Philippe Mathieu-Daudé
2020-04-17 15:37 ` [PATCH 5/6] OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib Laszlo Ersek
` (4 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2020-04-17 15:37 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé,
Rebecca Cran
Move the ResetShutdown() definition to its own file. This will help us
introduce:
- a new library instance that is not broken in runtime modules (the
current library instance is broken in runtime modules),
- another new library instance for bhyve support.
While at it, squash AcpiPmControl() into ResetShutdown(), open-coding
SuspendType=0. This is justified because we've had no other callers for
AcpiPmControl() since commit 2d9950a2bff8 ("OvmfPkg: remove
EnterS3WithImmediateWake () from ResetSystemLib", 2020-01-10).
Tested with the "reset -s" UEFI shell command, on both i440fx and q35.
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf | 1 +
OvmfPkg/Library/ResetSystemLib/ResetShutdown.c | 51 ++++++++++++++++++++
OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c | 49 -------------------
3 files changed, 52 insertions(+), 49 deletions(-)
diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
index af20f516c035..9362f884f124 100644
--- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
+++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
@@ -22,6 +22,7 @@ [Defines]
#
[Sources]
+ ResetShutdown.c
ResetSystemLib.c
[Packages]
diff --git a/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c b/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c
new file mode 100644
index 000000000000..971d94fa5766
--- /dev/null
+++ b/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c
@@ -0,0 +1,51 @@
+/** @file
+ Reset System Library Shutdown API implementation for OVMF.
+
+ Copyright (C) 2020, Red Hat, Inc.
+ Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Base.h> // BIT13
+
+#include <Library/BaseLib.h> // CpuDeadLoop()
+#include <Library/DebugLib.h> // ASSERT()
+#include <Library/IoLib.h> // IoOr16()
+#include <Library/PciLib.h> // PciRead16()
+#include <Library/ResetSystemLib.h> // ResetShutdown()
+#include <OvmfPlatforms.h> // OVMF_HOSTBRIDGE_DID
+
+/**
+ Calling this function causes the system to enter a power state equivalent
+ to the ACPI G2/S5 or G3 states.
+
+ System shutdown should not return, if it returns, it means the system does
+ not support shut down reset.
+**/
+VOID
+EFIAPI
+ResetShutdown (
+ VOID
+ )
+{
+ UINT16 AcpiPmBaseAddress;
+ UINT16 HostBridgeDevId;
+
+ AcpiPmBaseAddress = 0;
+ HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
+ switch (HostBridgeDevId) {
+ case INTEL_82441_DEVICE_ID:
+ AcpiPmBaseAddress = PIIX4_PMBA_VALUE;
+ break;
+ case INTEL_Q35_MCH_DEVICE_ID:
+ AcpiPmBaseAddress = ICH9_PMBASE_VALUE;
+ break;
+ default:
+ ASSERT (FALSE);
+ CpuDeadLoop ();
+ }
+
+ IoBitFieldWrite16 (AcpiPmBaseAddress + 4, 10, 13, 0);
+ IoOr16 (AcpiPmBaseAddress + 4, BIT13);
+ CpuDeadLoop ();
+}
diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
index 0fc142325ece..fe51f53d1df2 100644
--- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
+++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
@@ -9,41 +9,9 @@
#include <Base.h> // BIT1
#include <Library/BaseLib.h> // CpuDeadLoop()
-#include <Library/DebugLib.h> // ASSERT()
#include <Library/IoLib.h> // IoWrite8()
-#include <Library/PciLib.h> // PciRead16()
#include <Library/ResetSystemLib.h> // ResetCold()
#include <Library/TimerLib.h> // MicroSecondDelay()
-#include <OvmfPlatforms.h> // OVMF_HOSTBRIDGE_DID
-
-VOID
-AcpiPmControl (
- UINTN SuspendType
- )
-{
- UINT16 AcpiPmBaseAddress;
- UINT16 HostBridgeDevId;
-
- ASSERT (SuspendType < 6);
-
- AcpiPmBaseAddress = 0;
- HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
- switch (HostBridgeDevId) {
- case INTEL_82441_DEVICE_ID:
- AcpiPmBaseAddress = PIIX4_PMBA_VALUE;
- break;
- case INTEL_Q35_MCH_DEVICE_ID:
- AcpiPmBaseAddress = ICH9_PMBASE_VALUE;
- break;
- default:
- ASSERT (FALSE);
- CpuDeadLoop ();
- }
-
- IoBitFieldWrite16 (AcpiPmBaseAddress + 4, 10, 13, (UINT16) SuspendType);
- IoOr16 (AcpiPmBaseAddress + 4, BIT13);
- CpuDeadLoop ();
-}
/**
Calling this function causes a system-wide reset. This sets
@@ -84,23 +52,6 @@ ResetWarm (
CpuDeadLoop ();
}
-/**
- Calling this function causes the system to enter a power state equivalent
- to the ACPI G2/S5 or G3 states.
-
- System shutdown should not return, if it returns, it means the system does
- not support shut down reset.
-**/
-VOID
-EFIAPI
-ResetShutdown (
- VOID
- )
-{
- AcpiPmControl (0);
- ASSERT (FALSE);
-}
-
/**
This function causes a systemwide reset. The exact type of the reset is
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 4/6] OvmfPkg/ResetSystemLib: factor out ResetShutdown()
2020-04-17 15:37 ` [PATCH 4/6] OvmfPkg/ResetSystemLib: factor out ResetShutdown() Laszlo Ersek
@ 2020-04-17 16:05 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-17 16:05 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Ard Biesheuvel, Jordan Justen, Rebecca Cran
On 4/17/20 5:37 PM, Laszlo Ersek wrote:
> Move the ResetShutdown() definition to its own file. This will help us
> introduce:
>
> - a new library instance that is not broken in runtime modules (the
> current library instance is broken in runtime modules),
>
> - another new library instance for bhyve support.
>
> While at it, squash AcpiPmControl() into ResetShutdown(), open-coding
> SuspendType=0. This is justified because we've had no other callers for
> AcpiPmControl() since commit 2d9950a2bff8 ("OvmfPkg: remove
> EnterS3WithImmediateWake () from ResetSystemLib", 2020-01-10).
>
> Tested with the "reset -s" UEFI shell command, on both i440fx and q35.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf | 1 +
> OvmfPkg/Library/ResetSystemLib/ResetShutdown.c | 51 ++++++++++++++++++++
> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c | 49 -------------------
> 3 files changed, 52 insertions(+), 49 deletions(-)
>
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> index af20f516c035..9362f884f124 100644
> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> +++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> @@ -22,6 +22,7 @@ [Defines]
> #
>
> [Sources]
> + ResetShutdown.c
> ResetSystemLib.c
>
> [Packages]
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c b/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c
> new file mode 100644
> index 000000000000..971d94fa5766
> --- /dev/null
> +++ b/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c
> @@ -0,0 +1,51 @@
> +/** @file
> + Reset System Library Shutdown API implementation for OVMF.
> +
> + Copyright (C) 2020, Red Hat, Inc.
> + Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Base.h> // BIT13
> +
> +#include <Library/BaseLib.h> // CpuDeadLoop()
> +#include <Library/DebugLib.h> // ASSERT()
> +#include <Library/IoLib.h> // IoOr16()
> +#include <Library/PciLib.h> // PciRead16()
> +#include <Library/ResetSystemLib.h> // ResetShutdown()
> +#include <OvmfPlatforms.h> // OVMF_HOSTBRIDGE_DID
> +
> +/**
> + Calling this function causes the system to enter a power state equivalent
> + to the ACPI G2/S5 or G3 states.
> +
> + System shutdown should not return, if it returns, it means the system does
> + not support shut down reset.
> +**/
> +VOID
> +EFIAPI
> +ResetShutdown (
> + VOID
> + )
> +{
> + UINT16 AcpiPmBaseAddress;
> + UINT16 HostBridgeDevId;
> +
> + AcpiPmBaseAddress = 0;
> + HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
> + switch (HostBridgeDevId) {
> + case INTEL_82441_DEVICE_ID:
> + AcpiPmBaseAddress = PIIX4_PMBA_VALUE;
> + break;
> + case INTEL_Q35_MCH_DEVICE_ID:
> + AcpiPmBaseAddress = ICH9_PMBASE_VALUE;
> + break;
> + default:
> + ASSERT (FALSE);
> + CpuDeadLoop ();
> + }
> +
> + IoBitFieldWrite16 (AcpiPmBaseAddress + 4, 10, 13, 0);
> + IoOr16 (AcpiPmBaseAddress + 4, BIT13);
> + CpuDeadLoop ();
> +}
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> index 0fc142325ece..fe51f53d1df2 100644
> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> +++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> @@ -9,41 +9,9 @@
> #include <Base.h> // BIT1
>
> #include <Library/BaseLib.h> // CpuDeadLoop()
> -#include <Library/DebugLib.h> // ASSERT()
> #include <Library/IoLib.h> // IoWrite8()
> -#include <Library/PciLib.h> // PciRead16()
> #include <Library/ResetSystemLib.h> // ResetCold()
> #include <Library/TimerLib.h> // MicroSecondDelay()
> -#include <OvmfPlatforms.h> // OVMF_HOSTBRIDGE_DID
> -
> -VOID
> -AcpiPmControl (
> - UINTN SuspendType
> - )
> -{
> - UINT16 AcpiPmBaseAddress;
> - UINT16 HostBridgeDevId;
> -
> - ASSERT (SuspendType < 6);
> -
> - AcpiPmBaseAddress = 0;
> - HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
> - switch (HostBridgeDevId) {
> - case INTEL_82441_DEVICE_ID:
> - AcpiPmBaseAddress = PIIX4_PMBA_VALUE;
> - break;
> - case INTEL_Q35_MCH_DEVICE_ID:
> - AcpiPmBaseAddress = ICH9_PMBASE_VALUE;
> - break;
> - default:
> - ASSERT (FALSE);
> - CpuDeadLoop ();
> - }
> -
> - IoBitFieldWrite16 (AcpiPmBaseAddress + 4, 10, 13, (UINT16) SuspendType);
> - IoOr16 (AcpiPmBaseAddress + 4, BIT13);
> - CpuDeadLoop ();
> -}
>
> /**
> Calling this function causes a system-wide reset. This sets
> @@ -84,23 +52,6 @@ ResetWarm (
> CpuDeadLoop ();
> }
>
> -/**
> - Calling this function causes the system to enter a power state equivalent
> - to the ACPI G2/S5 or G3 states.
> -
> - System shutdown should not return, if it returns, it means the system does
> - not support shut down reset.
> -**/
> -VOID
> -EFIAPI
> -ResetShutdown (
> - VOID
> - )
> -{
> - AcpiPmControl (0);
> - ASSERT (FALSE);
> -}
> -
>
> /**
> This function causes a systemwide reset. The exact type of the reset is
>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/6] OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib
2020-04-17 15:37 [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Laszlo Ersek
` (3 preceding siblings ...)
2020-04-17 15:37 ` [PATCH 4/6] OvmfPkg/ResetSystemLib: factor out ResetShutdown() Laszlo Ersek
@ 2020-04-17 15:37 ` Laszlo Ersek
2020-04-21 17:27 ` [edk2-devel] " Laszlo Ersek
2020-04-17 15:37 ` [PATCH 6/6] OvmfPkg/ResetSystemLib: introduce the DxeResetSystemLib instance Laszlo Ersek
` (3 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2020-04-17 15:37 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Anthony Perard, Ard Biesheuvel, Jordan Justen, Julien Grall,
Philippe Mathieu-Daudé, Rebecca Cran
In preparation for introducing DxeResetSystemLib, rename the current
(only) ResetSystemLib instance to BaseResetSystemLib.
In the DSC files, keep the ResetSystemLib resolution in the same
[LibraryClasses] section, but move it near the TimerLib resolution, as the
differences between the ResetSystemLib instances will mostly follow those
seen under OvmfPkg/Library/AcpiTimerLib.
(While OvmfXen does not use "OvmfPkg/Library/AcpiTimerLib", perform the
same movement there too, for keeping future DSC diffing simple.)
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien@xen.org>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/OvmfPkgIa32.dsc | 2 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
OvmfPkg/OvmfPkgX64.dsc | 2 +-
OvmfPkg/OvmfXen.dsc | 2 +-
OvmfPkg/Library/ResetSystemLib/{ResetSystemLib.inf => BaseResetSystemLib.inf} | 6 +++---
OvmfPkg/Library/ResetSystemLib/{ResetShutdown.c => BaseResetShutdown.c} | 2 +-
6 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index cbc5f0e583bc..cd0ed34e0e5a 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -114,6 +114,7 @@ [SkuIds]
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
@@ -176,7 +177,6 @@ [LibraryClasses]
DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
!endif
- ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 6d69cc6cb56f..3c377c6e858e 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -118,6 +118,7 @@ [SkuIds]
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
@@ -180,7 +181,6 @@ [LibraryClasses]
DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
!endif
- ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 5ad4f461ce52..701a7ccea987 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -118,6 +118,7 @@ [SkuIds]
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
@@ -180,7 +181,6 @@ [LibraryClasses]
DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
!endif
- ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 47ee8db8b884..86b24d1716b9 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -110,6 +110,7 @@ [SkuIds]
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
@@ -169,7 +170,6 @@ [LibraryClasses]
DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
!endif
- ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
similarity index 80%
rename from OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
rename to OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
index 9362f884f124..0772780b2dc2 100644
--- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
+++ b/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
@@ -1,5 +1,5 @@
## @file
-# Library instance for ResetSystem library class for OVMF
+# Base library instance for ResetSystem library class for OVMF
#
# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -8,7 +8,7 @@
[Defines]
INF_VERSION = 0x00010005
- BASE_NAME = ResetSystemLib
+ BASE_NAME = BaseResetSystemLib
FILE_GUID = 66564872-21d4-4d2a-a68b-1e844f980820
MODULE_TYPE = BASE
VERSION_STRING = 1.0
@@ -22,7 +22,7 @@ [Defines]
#
[Sources]
- ResetShutdown.c
+ BaseResetShutdown.c
ResetSystemLib.c
[Packages]
diff --git a/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c b/OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
similarity index 91%
rename from OvmfPkg/Library/ResetSystemLib/ResetShutdown.c
rename to OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
index 971d94fa5766..21c80e43230c 100644
--- a/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c
+++ b/OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
@@ -1,5 +1,5 @@
/** @file
- Reset System Library Shutdown API implementation for OVMF.
+ Base Reset System Library Shutdown API implementation for OVMF.
Copyright (C) 2020, Red Hat, Inc.
Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [edk2-devel] [PATCH 5/6] OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib
2020-04-17 15:37 ` [PATCH 5/6] OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib Laszlo Ersek
@ 2020-04-21 17:27 ` Laszlo Ersek
2020-04-21 18:08 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2020-04-21 17:27 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: edk2-devel-groups-io, Anthony Perard, Ard Biesheuvel,
Jordan Justen, Julien Grall, Rebecca Cran
Phil,
On 04/17/20 17:37, Laszlo Ersek wrote:
> In preparation for introducing DxeResetSystemLib, rename the current
> (only) ResetSystemLib instance to BaseResetSystemLib.
>
> In the DSC files, keep the ResetSystemLib resolution in the same
> [LibraryClasses] section, but move it near the TimerLib resolution, as the
> differences between the ResetSystemLib instances will mostly follow those
> seen under OvmfPkg/Library/AcpiTimerLib.
>
> (While OvmfXen does not use "OvmfPkg/Library/AcpiTimerLib", perform the
> same movement there too, for keeping future DSC diffing simple.)
>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/OvmfPkgIa32.dsc | 2 +-
> OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
> OvmfPkg/OvmfPkgX64.dsc | 2 +-
> OvmfPkg/OvmfXen.dsc | 2 +-
> OvmfPkg/Library/ResetSystemLib/{ResetSystemLib.inf => BaseResetSystemLib.inf} | 6 +++---
> OvmfPkg/Library/ResetSystemLib/{ResetShutdown.c => BaseResetShutdown.c} | 2 +-
> 6 files changed, 8 insertions(+), 8 deletions(-)
Do you have comments on this patch?
If not, I think I can go ahead and merge the series (moving part of the
last patch's commit message over to the bugzilla ticket).
Thanks!
Laszlo
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index cbc5f0e583bc..cd0ed34e0e5a 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -114,6 +114,7 @@ [SkuIds]
> [LibraryClasses]
> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
> BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
> BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> @@ -176,7 +177,6 @@ [LibraryClasses]
> DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
> !endif
>
> - ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
> DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
>
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 6d69cc6cb56f..3c377c6e858e 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -118,6 +118,7 @@ [SkuIds]
> [LibraryClasses]
> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
> BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
> BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> @@ -180,7 +181,6 @@ [LibraryClasses]
> DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
> !endif
>
> - ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
> DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 5ad4f461ce52..701a7ccea987 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -118,6 +118,7 @@ [SkuIds]
> [LibraryClasses]
> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
> BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
> BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> @@ -180,7 +181,6 @@ [LibraryClasses]
> DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
> !endif
>
> - ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
> DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
>
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 47ee8db8b884..86b24d1716b9 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -110,6 +110,7 @@ [SkuIds]
> [LibraryClasses]
> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
> BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
> BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> @@ -169,7 +170,6 @@ [LibraryClasses]
> DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
> !endif
>
> - ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
> DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
>
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> similarity index 80%
> rename from OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> rename to OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> index 9362f884f124..0772780b2dc2 100644
> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> +++ b/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> @@ -1,5 +1,5 @@
> ## @file
> -# Library instance for ResetSystem library class for OVMF
> +# Base library instance for ResetSystem library class for OVMF
> #
> # Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -8,7 +8,7 @@
>
> [Defines]
> INF_VERSION = 0x00010005
> - BASE_NAME = ResetSystemLib
> + BASE_NAME = BaseResetSystemLib
> FILE_GUID = 66564872-21d4-4d2a-a68b-1e844f980820
> MODULE_TYPE = BASE
> VERSION_STRING = 1.0
> @@ -22,7 +22,7 @@ [Defines]
> #
>
> [Sources]
> - ResetShutdown.c
> + BaseResetShutdown.c
> ResetSystemLib.c
>
> [Packages]
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c b/OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
> similarity index 91%
> rename from OvmfPkg/Library/ResetSystemLib/ResetShutdown.c
> rename to OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
> index 971d94fa5766..21c80e43230c 100644
> --- a/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c
> +++ b/OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
> @@ -1,5 +1,5 @@
> /** @file
> - Reset System Library Shutdown API implementation for OVMF.
> + Base Reset System Library Shutdown API implementation for OVMF.
>
> Copyright (C) 2020, Red Hat, Inc.
> Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [edk2-devel] [PATCH 5/6] OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib
2020-04-21 17:27 ` [edk2-devel] " Laszlo Ersek
@ 2020-04-21 18:08 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-21 18:08 UTC (permalink / raw)
To: devel, lersek
Cc: Anthony Perard, Ard Biesheuvel, Jordan Justen, Julien Grall,
Rebecca Cran
On 4/21/20 7:27 PM, Laszlo Ersek wrote:
> Phil,
>
> On 04/17/20 17:37, Laszlo Ersek wrote:
>> In preparation for introducing DxeResetSystemLib, rename the current
>> (only) ResetSystemLib instance to BaseResetSystemLib.
>>
>> In the DSC files, keep the ResetSystemLib resolution in the same
>> [LibraryClasses] section, but move it near the TimerLib resolution, as the
>> differences between the ResetSystemLib instances will mostly follow those
>> seen under OvmfPkg/Library/AcpiTimerLib.
>>
>> (While OvmfXen does not use "OvmfPkg/Library/AcpiTimerLib", perform the
>> same movement there too, for keeping future DSC diffing simple.)
>>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Rebecca Cran <rebecca@bsdio.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> OvmfPkg/OvmfPkgIa32.dsc | 2 +-
>> OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
>> OvmfPkg/OvmfPkgX64.dsc | 2 +-
>> OvmfPkg/OvmfXen.dsc | 2 +-
>> OvmfPkg/Library/ResetSystemLib/{ResetSystemLib.inf => BaseResetSystemLib.inf} | 6 +++---
>> OvmfPkg/Library/ResetSystemLib/{ResetShutdown.c => BaseResetShutdown.c} | 2 +-
>> 6 files changed, 8 insertions(+), 8 deletions(-)
>
> Do you have comments on this patch?
>
> If not, I think I can go ahead and merge the series (moving part of the
> last patch's commit message over to the bugzilla ticket).
No comment!
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> Thanks!
> Laszlo
>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index cbc5f0e583bc..cd0ed34e0e5a 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -114,6 +114,7 @@ [SkuIds]
>> [LibraryClasses]
>> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>> TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
>> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
>> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
>> BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
>> BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
>> @@ -176,7 +177,6 @@ [LibraryClasses]
>> DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
>> !endif
>>
>> - ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
>> LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
>> DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 6d69cc6cb56f..3c377c6e858e 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -118,6 +118,7 @@ [SkuIds]
>> [LibraryClasses]
>> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>> TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
>> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
>> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
>> BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
>> BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
>> @@ -180,7 +181,6 @@ [LibraryClasses]
>> DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
>> !endif
>>
>> - ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
>> LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
>> DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
>>
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 5ad4f461ce52..701a7ccea987 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -118,6 +118,7 @@ [SkuIds]
>> [LibraryClasses]
>> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>> TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
>> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
>> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
>> BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
>> BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
>> @@ -180,7 +181,6 @@ [LibraryClasses]
>> DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
>> !endif
>>
>> - ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
>> LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
>> DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
>>
>> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
>> index 47ee8db8b884..86b24d1716b9 100644
>> --- a/OvmfPkg/OvmfXen.dsc
>> +++ b/OvmfPkg/OvmfXen.dsc
>> @@ -110,6 +110,7 @@ [SkuIds]
>> [LibraryClasses]
>> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>> TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
>> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
>> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
>> BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
>> BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
>> @@ -169,7 +170,6 @@ [LibraryClasses]
>> DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
>> !endif
>>
>> - ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
>> LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
>> DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
>>
>> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
>> similarity index 80%
>> rename from OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
>> rename to OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
>> index 9362f884f124..0772780b2dc2 100644
>> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
>> +++ b/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
>> @@ -1,5 +1,5 @@
>> ## @file
>> -# Library instance for ResetSystem library class for OVMF
>> +# Base library instance for ResetSystem library class for OVMF
>> #
>> # Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>> # SPDX-License-Identifier: BSD-2-Clause-Patent
>> @@ -8,7 +8,7 @@
>>
>> [Defines]
>> INF_VERSION = 0x00010005
>> - BASE_NAME = ResetSystemLib
>> + BASE_NAME = BaseResetSystemLib
>> FILE_GUID = 66564872-21d4-4d2a-a68b-1e844f980820
>> MODULE_TYPE = BASE
>> VERSION_STRING = 1.0
>> @@ -22,7 +22,7 @@ [Defines]
>> #
>>
>> [Sources]
>> - ResetShutdown.c
>> + BaseResetShutdown.c
>> ResetSystemLib.c
>>
>> [Packages]
>> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c b/OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
>> similarity index 91%
>> rename from OvmfPkg/Library/ResetSystemLib/ResetShutdown.c
>> rename to OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
>> index 971d94fa5766..21c80e43230c 100644
>> --- a/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c
>> +++ b/OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
>> @@ -1,5 +1,5 @@
>> /** @file
>> - Reset System Library Shutdown API implementation for OVMF.
>> + Base Reset System Library Shutdown API implementation for OVMF.
>>
>> Copyright (C) 2020, Red Hat, Inc.
>> Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 6/6] OvmfPkg/ResetSystemLib: introduce the DxeResetSystemLib instance
2020-04-17 15:37 [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Laszlo Ersek
` (4 preceding siblings ...)
2020-04-17 15:37 ` [PATCH 5/6] OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib Laszlo Ersek
@ 2020-04-17 15:37 ` Laszlo Ersek
2020-04-17 16:23 ` Philippe Mathieu-Daudé
2020-04-17 15:59 ` [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Ard Biesheuvel
` (2 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2020-04-17 15:37 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Anthony Perard, Ard Biesheuvel, Jordan Justen, Julien Grall,
Philippe Mathieu-Daudé, Rebecca Cran
The BaseResetSystemLib instance is not suitable for OS runtime, because
its ResetShutdown() implementation calls PciRead16 (OVMF_HOSTBRIDGE_DID).
On q35, this boils down to a memory-mapped config space access -- but we
never ask the OS to map MMCONFIG for runtime.
There are at least three alternatives to approach this:
(1) Investigate "MdePkg/Library/DxeRuntimePciExpressLib", which offers
some kind of runtime mapping for MMCONFIG.
(2) Consume PciCf8Lib directly, rather than PciLib, in ResetSystemLib.
Then we'll read OVMF_HOSTBRIDGE_DID from the config space with IO port
accesses on q35 too, not just on i440fx. IO ports don't depend on page
tables.
(3) In the lib constructor, cache "mAcpiPmBaseAddress" based on
"PcdOvmfHostBridgePciDevId" (which is set by PlatformPei). Then the
host bridge type will be known at runtime without PCI config space
accesses.
This patch follows approach (3), in order to mirror AcpiTimerLib.
Notes:
* This patch is best viewed with "git show --find-copies-harder -C43".
* PCDs are not usable in the DXE_CORE, as the PCD PPI is gone, and the PCD
protocol is not available yet. (The DXE_CORE does consume ResetSystemLib
in practice, when OVMF is built with -D SOURCE_DEBUG_ENABLE.)
* The bug is not easy to trigger in common setups, because e.g. the Linux
guest cannot easily be convinced to use the EFI runtime service for
poweroff. One way to trigger the bug is to (a) patch OVMF as follows:
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> index fe51f53d1df2..1edc4349ad20 100644
> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> +++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> @@ -28,6 +28,7 @@ ResetCold (
> VOID
> )
> {
> + ResetShutdown ();
> IoWrite8 (0xCF9, BIT2 | BIT1); // 1st choice: PIIX3 RCR, RCPU|SRST
> MicroSecondDelay (50);
(b) boot a Linux guest with "reboot=efi" on q35, and (c) reboot the
guest with the "reboot" command. Then the guest kernel will log:
> reboot: Restarting system
> reboot: machine restart
> ------------[ cut here ]------------
> [Firmware Bug]: Page fault caused by firmware at PA: 0xb0000002
> WARNING: CPU: 0 PID: 1362 at arch/x86/platform/efi/quirks.c:738
> efi_recover_from_page_fault+0x2a/0xc8
> Modules linked in: ip_set nfnetlink sunrpc vfat fat intel_rapl_msr
> intel_rapl_common kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul
> bochs_drm drm_vram_helper ttm ghash_clmulni_intel drm_kms_helper
> iTCO_wdt iTCO_vendor
> CPU: 0 PID: 1362 Comm: reboot Not tainted 5.3.6-200.fc30.x86_64 #1
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0
> 02/06/2015
> RIP: 0010:efi_recover_from_page_fault+0x2a/0xc8
> Code: 0f 1f 44 00 00 8b 15 35 c2 c1 01 85 d2 74 09 48 81 ff ff 0f 00 00
> 77 01 c3 53 48 89 fe 48 c7 c7 58 ba 12 aa 50 e8 74 f0 00 00 <0f> 0b 83
> 3d 0d c2 c1 01 0a 0f 84 8f 00 00 00 48 8b 05 70 c8 4a 01
> RSP: 0018:ffffaf3a402539d8 EFLAGS: 00010286
> RAX: 0000000000000000 RBX: ffff8a3c33711f40 RCX: 00000000000003b2
> RDX: 0000000000000001 RSI: 0000000000000096 RDI: 0000000000000246
> RBP: ffffaf3a40253a88 R08: 0000000000000000 R09: 00000000000003b2
> R10: 0000000000000001 R11: ffffffffa9ee3800 R12: 00000000b0000002
> R13: 0000000000000000 R14: 000000000000000b R15: 0000000000000001
> FS: 00007fbca9cf8940(0000) GS:ffff8a3c3ae00000(0000)
> knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: fffffffeff8a67a1 CR3: 0000000072868006 CR4: 00000000001606f0
> Call Trace:
> no_context+0x15b/0x380
> ? do_user_addr_fault+0x12e/0x440
> do_page_fault+0x31/0x110
> async_page_fault+0x3e/0x50
> RIP: 0010:0xfffffffeff8a67a1
> Code: Bad RIP value.
> RSP: 0018:ffffaf3a40253b30 EFLAGS: 00010046
> RAX: 00000000b0000002 RBX: 0000000000000000 RCX: 00000000b0000002
> RDX: 00000000b0000000 RSI: 0000000000000000 RDI: fffffffeff8a80f0
> RBP: ffffaf3a40253b60 R08: fffffffeff8aadb8 R09: 0000000000000000
> R10: ffffffffaa5762c0 R11: ffffffffa9ee3800 R12: 0000000000000000
> R13: 0000000000000000 R14: 0000000000000046 R15: 0000000000000000
> ? efi_call+0x58/0x90
> ? virt_efi_reset_system+0x8d/0x100
> ? efi_reboot+0x85/0xb8
> ? native_machine_emergency_restart+0x9f/0x250
> ? native_apic_msr_read+0x16/0x20
> ? disconnect_bsp_APIC+0x8c/0xd0
> ? __do_sys_reboot+0x1d2/0x210
> ? __fput+0x168/0x250
> ? do_writev+0x6b/0x110
> ? do_syscall_64+0x5f/0x1a0
> ? entry_SYSCALL_64_after_hwframe+0x44/0xa9
> ---[ end trace d5bee708166d198b ]---
> efi: efi_reset_system() buggy! Reboot through BIOS
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien@xen.org>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/OvmfPkgIa32.dsc | 6 +++
OvmfPkg/OvmfPkgIa32X64.dsc | 6 +++
OvmfPkg/OvmfPkgX64.dsc | 6 +++
OvmfPkg/OvmfXen.dsc | 4 ++
OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf | 2 +-
OvmfPkg/Library/ResetSystemLib/{BaseResetSystemLib.inf => DxeResetSystemLib.inf} | 21 +++++----
OvmfPkg/Library/ResetSystemLib/{BaseResetShutdown.c => DxeResetShutdown.c} | 49 ++++++++++++--------
7 files changed, 66 insertions(+), 28 deletions(-)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index cd0ed34e0e5a..d5e90c001370 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -314,6 +314,7 @@ [LibraryClasses.common.DXE_CORE]
[LibraryClasses.common.DXE_RUNTIME_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
@@ -331,6 +332,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
[LibraryClasses.common.UEFI_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
@@ -346,6 +348,7 @@ [LibraryClasses.common.UEFI_DRIVER]
[LibraryClasses.common.DXE_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
@@ -383,6 +386,7 @@ [LibraryClasses.common.DXE_DRIVER]
[LibraryClasses.common.UEFI_APPLICATION]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
@@ -396,6 +400,7 @@ [LibraryClasses.common.UEFI_APPLICATION]
[LibraryClasses.common.DXE_SMM_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
@@ -417,6 +422,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
[LibraryClasses.common.SMM_CORE]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
SmmCorePlatformHookLib|MdeModulePkg/Library/SmmCorePlatformHookLibNull/SmmCorePlatformHookLibNull.inf
MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 3c377c6e858e..066f49aeaee0 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -318,6 +318,7 @@ [LibraryClasses.common.DXE_CORE]
[LibraryClasses.common.DXE_RUNTIME_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
@@ -335,6 +336,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
[LibraryClasses.common.UEFI_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
@@ -350,6 +352,7 @@ [LibraryClasses.common.UEFI_DRIVER]
[LibraryClasses.common.DXE_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
@@ -387,6 +390,7 @@ [LibraryClasses.common.DXE_DRIVER]
[LibraryClasses.common.UEFI_APPLICATION]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
@@ -400,6 +404,7 @@ [LibraryClasses.common.UEFI_APPLICATION]
[LibraryClasses.common.DXE_SMM_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
@@ -421,6 +426,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
[LibraryClasses.common.SMM_CORE]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
SmmCorePlatformHookLib|MdeModulePkg/Library/SmmCorePlatformHookLibNull/SmmCorePlatformHookLibNull.inf
MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 701a7ccea987..ac510522a9ff 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -318,6 +318,7 @@ [LibraryClasses.common.DXE_CORE]
[LibraryClasses.common.DXE_RUNTIME_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
@@ -335,6 +336,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
[LibraryClasses.common.UEFI_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
@@ -350,6 +352,7 @@ [LibraryClasses.common.UEFI_DRIVER]
[LibraryClasses.common.DXE_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
@@ -387,6 +390,7 @@ [LibraryClasses.common.DXE_DRIVER]
[LibraryClasses.common.UEFI_APPLICATION]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
@@ -400,6 +404,7 @@ [LibraryClasses.common.UEFI_APPLICATION]
[LibraryClasses.common.DXE_SMM_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
@@ -421,6 +426,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
[LibraryClasses.common.SMM_CORE]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
SmmCorePlatformHookLib|MdeModulePkg/Library/SmmCorePlatformHookLibNull/SmmCorePlatformHookLibNull.inf
MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 86b24d1716b9..f6214bd3465a 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -288,6 +288,7 @@ [LibraryClasses.common.DXE_CORE]
[LibraryClasses.common.DXE_RUNTIME_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
@@ -304,6 +305,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
[LibraryClasses.common.UEFI_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
@@ -318,6 +320,7 @@ [LibraryClasses.common.UEFI_DRIVER]
[LibraryClasses.common.DXE_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
@@ -341,6 +344,7 @@ [LibraryClasses.common.DXE_DRIVER]
[LibraryClasses.common.UEFI_APPLICATION]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
+ ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
diff --git a/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
index 0772780b2dc2..35d317f1e0b3 100644
--- a/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
+++ b/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
@@ -12,7 +12,7 @@ [Defines]
FILE_GUID = 66564872-21d4-4d2a-a68b-1e844f980820
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = ResetSystemLib
+ LIBRARY_CLASS = ResetSystemLib|SEC PEI_CORE PEIM DXE_CORE
#
# The following information is for reference only and not required by the build
diff --git a/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
similarity index 43%
copy from OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
copy to OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
index 0772780b2dc2..a9b4ce90000a 100644
--- a/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
+++ b/OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
@@ -1,18 +1,20 @@
## @file
-# Base library instance for ResetSystem library class for OVMF
+# DXE library instance for ResetSystem library class for OVMF
#
+# Copyright (C) 2020, Red Hat, Inc.
# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
[Defines]
- INF_VERSION = 0x00010005
- BASE_NAME = BaseResetSystemLib
- FILE_GUID = 66564872-21d4-4d2a-a68b-1e844f980820
- MODULE_TYPE = BASE
+ INF_VERSION = 1.29
+ BASE_NAME = DxeResetSystemLib
+ FILE_GUID = bc7835ea-4094-41fe-b770-bad9e6c479b2
+ MODULE_TYPE = DXE_DRIVER
VERSION_STRING = 1.0
- LIBRARY_CLASS = ResetSystemLib
+ LIBRARY_CLASS = ResetSystemLib|DXE_DRIVER DXE_RUNTIME_DRIVER SMM_CORE DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION
+ CONSTRUCTOR = DxeResetInit
#
# The following information is for reference only and not required by the build
@@ -22,7 +24,7 @@ [Defines]
#
[Sources]
- BaseResetShutdown.c
+ DxeResetShutdown.c
ResetSystemLib.c
[Packages]
@@ -34,5 +36,8 @@ [LibraryClasses]
BaseLib
DebugLib
IoLib
- PciLib
+ PcdLib
TimerLib
+
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId ## CONSUMES
diff --git a/OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c b/OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c
similarity index 57%
copy from OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
copy to OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c
index 21c80e43230c..5a75c32df361 100644
--- a/OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
+++ b/OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c
@@ -1,5 +1,5 @@
/** @file
- Base Reset System Library Shutdown API implementation for OVMF.
+ DXE Reset System Library Shutdown API implementation for OVMF.
Copyright (C) 2020, Red Hat, Inc.
Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
@@ -11,41 +11,52 @@
#include <Library/BaseLib.h> // CpuDeadLoop()
#include <Library/DebugLib.h> // ASSERT()
#include <Library/IoLib.h> // IoOr16()
-#include <Library/PciLib.h> // PciRead16()
+#include <Library/PcdLib.h> // PcdGet16()
#include <Library/ResetSystemLib.h> // ResetShutdown()
-#include <OvmfPlatforms.h> // OVMF_HOSTBRIDGE_DID
+#include <OvmfPlatforms.h> // PIIX4_PMBA_VALUE
-/**
- Calling this function causes the system to enter a power state equivalent
- to the ACPI G2/S5 or G3 states.
+STATIC UINT16 mAcpiPmBaseAddress;
- System shutdown should not return, if it returns, it means the system does
- not support shut down reset.
-**/
-VOID
+EFI_STATUS
EFIAPI
-ResetShutdown (
- VOID
+DxeResetInit (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
)
{
- UINT16 AcpiPmBaseAddress;
UINT16 HostBridgeDevId;
- AcpiPmBaseAddress = 0;
- HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
+ HostBridgeDevId = PcdGet16 (PcdOvmfHostBridgePciDevId);
switch (HostBridgeDevId) {
case INTEL_82441_DEVICE_ID:
- AcpiPmBaseAddress = PIIX4_PMBA_VALUE;
+ mAcpiPmBaseAddress = PIIX4_PMBA_VALUE;
break;
case INTEL_Q35_MCH_DEVICE_ID:
- AcpiPmBaseAddress = ICH9_PMBASE_VALUE;
+ mAcpiPmBaseAddress = ICH9_PMBASE_VALUE;
break;
default:
ASSERT (FALSE);
CpuDeadLoop ();
+ return EFI_UNSUPPORTED;
}
- IoBitFieldWrite16 (AcpiPmBaseAddress + 4, 10, 13, 0);
- IoOr16 (AcpiPmBaseAddress + 4, BIT13);
+ return EFI_SUCCESS;
+}
+
+/**
+ Calling this function causes the system to enter a power state equivalent
+ to the ACPI G2/S5 or G3 states.
+
+ System shutdown should not return, if it returns, it means the system does
+ not support shut down reset.
+**/
+VOID
+EFIAPI
+ResetShutdown (
+ VOID
+ )
+{
+ IoBitFieldWrite16 (mAcpiPmBaseAddress + 4, 10, 13, 0);
+ IoOr16 (mAcpiPmBaseAddress + 4, BIT13);
CpuDeadLoop ();
}
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 6/6] OvmfPkg/ResetSystemLib: introduce the DxeResetSystemLib instance
2020-04-17 15:37 ` [PATCH 6/6] OvmfPkg/ResetSystemLib: introduce the DxeResetSystemLib instance Laszlo Ersek
@ 2020-04-17 16:23 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-17 16:23 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Anthony Perard, Ard Biesheuvel, Jordan Justen, Julien Grall,
Rebecca Cran
On 4/17/20 5:37 PM, Laszlo Ersek wrote:
> The BaseResetSystemLib instance is not suitable for OS runtime, because
> its ResetShutdown() implementation calls PciRead16 (OVMF_HOSTBRIDGE_DID).
> On q35, this boils down to a memory-mapped config space access -- but we
> never ask the OS to map MMCONFIG for runtime.
>
> There are at least three alternatives to approach this:
>
> (1) Investigate "MdePkg/Library/DxeRuntimePciExpressLib", which offers
> some kind of runtime mapping for MMCONFIG.
>
> (2) Consume PciCf8Lib directly, rather than PciLib, in ResetSystemLib.
> Then we'll read OVMF_HOSTBRIDGE_DID from the config space with IO port
> accesses on q35 too, not just on i440fx. IO ports don't depend on page
> tables.
>
> (3) In the lib constructor, cache "mAcpiPmBaseAddress" based on
> "PcdOvmfHostBridgePciDevId" (which is set by PlatformPei). Then the
> host bridge type will be known at runtime without PCI config space
> accesses.
>
> This patch follows approach (3), in order to mirror AcpiTimerLib.
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>
> Notes:
>
> * This patch is best viewed with "git show --find-copies-harder -C43".
>
> * PCDs are not usable in the DXE_CORE, as the PCD PPI is gone, and the PCD
> protocol is not available yet. (The DXE_CORE does consume ResetSystemLib
> in practice, when OVMF is built with -D SOURCE_DEBUG_ENABLE.)
>
> * The bug is not easy to trigger in common setups, because e.g. the Linux
> guest cannot easily be convinced to use the EFI runtime service for
> poweroff. One way to trigger the bug is to (a) patch OVMF as follows:
>
>> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
>> index fe51f53d1df2..1edc4349ad20 100644
>> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
>> +++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
>> @@ -28,6 +28,7 @@ ResetCold (
>> VOID
>> )
>> {
>> + ResetShutdown ();
>> IoWrite8 (0xCF9, BIT2 | BIT1); // 1st choice: PIIX3 RCR, RCPU|SRST
>> MicroSecondDelay (50);
>
> (b) boot a Linux guest with "reboot=efi" on q35, and (c) reboot the
> guest with the "reboot" command. Then the guest kernel will log:
>
>> reboot: Restarting system
>> reboot: machine restart
>> ------------[ cut here ]------------
>> [Firmware Bug]: Page fault caused by firmware at PA: 0xb0000002
>> WARNING: CPU: 0 PID: 1362 at arch/x86/platform/efi/quirks.c:738
>> efi_recover_from_page_fault+0x2a/0xc8
>> Modules linked in: ip_set nfnetlink sunrpc vfat fat intel_rapl_msr
>> intel_rapl_common kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul
>> bochs_drm drm_vram_helper ttm ghash_clmulni_intel drm_kms_helper
>> iTCO_wdt iTCO_vendor
>> CPU: 0 PID: 1362 Comm: reboot Not tainted 5.3.6-200.fc30.x86_64 #1
>> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0
>> 02/06/2015
>> RIP: 0010:efi_recover_from_page_fault+0x2a/0xc8
>> Code: 0f 1f 44 00 00 8b 15 35 c2 c1 01 85 d2 74 09 48 81 ff ff 0f 00 00
>> 77 01 c3 53 48 89 fe 48 c7 c7 58 ba 12 aa 50 e8 74 f0 00 00 <0f> 0b 83
>> 3d 0d c2 c1 01 0a 0f 84 8f 00 00 00 48 8b 05 70 c8 4a 01
>> RSP: 0018:ffffaf3a402539d8 EFLAGS: 00010286
>> RAX: 0000000000000000 RBX: ffff8a3c33711f40 RCX: 00000000000003b2
>> RDX: 0000000000000001 RSI: 0000000000000096 RDI: 0000000000000246
>> RBP: ffffaf3a40253a88 R08: 0000000000000000 R09: 00000000000003b2
>> R10: 0000000000000001 R11: ffffffffa9ee3800 R12: 00000000b0000002
>> R13: 0000000000000000 R14: 000000000000000b R15: 0000000000000001
>> FS: 00007fbca9cf8940(0000) GS:ffff8a3c3ae00000(0000)
>> knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: fffffffeff8a67a1 CR3: 0000000072868006 CR4: 00000000001606f0
>> Call Trace:
>> no_context+0x15b/0x380
>> ? do_user_addr_fault+0x12e/0x440
>> do_page_fault+0x31/0x110
>> async_page_fault+0x3e/0x50
>> RIP: 0010:0xfffffffeff8a67a1
>> Code: Bad RIP value.
>> RSP: 0018:ffffaf3a40253b30 EFLAGS: 00010046
>> RAX: 00000000b0000002 RBX: 0000000000000000 RCX: 00000000b0000002
>> RDX: 00000000b0000000 RSI: 0000000000000000 RDI: fffffffeff8a80f0
>> RBP: ffffaf3a40253b60 R08: fffffffeff8aadb8 R09: 0000000000000000
>> R10: ffffffffaa5762c0 R11: ffffffffa9ee3800 R12: 0000000000000000
>> R13: 0000000000000000 R14: 0000000000000046 R15: 0000000000000000
>> ? efi_call+0x58/0x90
>> ? virt_efi_reset_system+0x8d/0x100
>> ? efi_reboot+0x85/0xb8
>> ? native_machine_emergency_restart+0x9f/0x250
>> ? native_apic_msr_read+0x16/0x20
>> ? disconnect_bsp_APIC+0x8c/0xd0
>> ? __do_sys_reboot+0x1d2/0x210
>> ? __fput+0x168/0x250
>> ? do_writev+0x6b/0x110
>> ? do_syscall_64+0x5f/0x1a0
>> ? entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> ---[ end trace d5bee708166d198b ]---
>> efi: efi_reset_system() buggy! Reboot through BIOS
>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/OvmfPkgIa32.dsc | 6 +++
> OvmfPkg/OvmfPkgIa32X64.dsc | 6 +++
> OvmfPkg/OvmfPkgX64.dsc | 6 +++
> OvmfPkg/OvmfXen.dsc | 4 ++
> OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf | 2 +-
> OvmfPkg/Library/ResetSystemLib/{BaseResetSystemLib.inf => DxeResetSystemLib.inf} | 21 +++++----
> OvmfPkg/Library/ResetSystemLib/{BaseResetShutdown.c => DxeResetShutdown.c} | 49 ++++++++++++--------
> 7 files changed, 66 insertions(+), 28 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index cd0ed34e0e5a..d5e90c001370 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -314,6 +314,7 @@ [LibraryClasses.common.DXE_CORE]
> [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -331,6 +332,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> [LibraryClasses.common.UEFI_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -346,6 +348,7 @@ [LibraryClasses.common.UEFI_DRIVER]
> [LibraryClasses.common.DXE_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> @@ -383,6 +386,7 @@ [LibraryClasses.common.DXE_DRIVER]
> [LibraryClasses.common.UEFI_APPLICATION]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> @@ -396,6 +400,7 @@ [LibraryClasses.common.UEFI_APPLICATION]
> [LibraryClasses.common.DXE_SMM_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> @@ -417,6 +422,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
> [LibraryClasses.common.SMM_CORE]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> SmmCorePlatformHookLib|MdeModulePkg/Library/SmmCorePlatformHookLibNull/SmmCorePlatformHookLibNull.inf
> MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 3c377c6e858e..066f49aeaee0 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -318,6 +318,7 @@ [LibraryClasses.common.DXE_CORE]
> [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -335,6 +336,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> [LibraryClasses.common.UEFI_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -350,6 +352,7 @@ [LibraryClasses.common.UEFI_DRIVER]
> [LibraryClasses.common.DXE_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> @@ -387,6 +390,7 @@ [LibraryClasses.common.DXE_DRIVER]
> [LibraryClasses.common.UEFI_APPLICATION]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> @@ -400,6 +404,7 @@ [LibraryClasses.common.UEFI_APPLICATION]
> [LibraryClasses.common.DXE_SMM_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> @@ -421,6 +426,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
> [LibraryClasses.common.SMM_CORE]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> SmmCorePlatformHookLib|MdeModulePkg/Library/SmmCorePlatformHookLibNull/SmmCorePlatformHookLibNull.inf
> MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 701a7ccea987..ac510522a9ff 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -318,6 +318,7 @@ [LibraryClasses.common.DXE_CORE]
> [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -335,6 +336,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> [LibraryClasses.common.UEFI_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -350,6 +352,7 @@ [LibraryClasses.common.UEFI_DRIVER]
> [LibraryClasses.common.DXE_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> @@ -387,6 +390,7 @@ [LibraryClasses.common.DXE_DRIVER]
> [LibraryClasses.common.UEFI_APPLICATION]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> @@ -400,6 +404,7 @@ [LibraryClasses.common.UEFI_APPLICATION]
> [LibraryClasses.common.DXE_SMM_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> @@ -421,6 +426,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
> [LibraryClasses.common.SMM_CORE]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> SmmCorePlatformHookLib|MdeModulePkg/Library/SmmCorePlatformHookLibNull/SmmCorePlatformHookLibNull.inf
> MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 86b24d1716b9..f6214bd3465a 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -288,6 +288,7 @@ [LibraryClasses.common.DXE_CORE]
>
> [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -304,6 +305,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>
> [LibraryClasses.common.UEFI_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -318,6 +320,7 @@ [LibraryClasses.common.UEFI_DRIVER]
>
> [LibraryClasses.common.DXE_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> @@ -341,6 +344,7 @@ [LibraryClasses.common.DXE_DRIVER]
>
> [LibraryClasses.common.UEFI_APPLICATION]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> + ResetSystemLib|OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> diff --git a/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> index 0772780b2dc2..35d317f1e0b3 100644
> --- a/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> +++ b/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> @@ -12,7 +12,7 @@ [Defines]
> FILE_GUID = 66564872-21d4-4d2a-a68b-1e844f980820
> MODULE_TYPE = BASE
> VERSION_STRING = 1.0
> - LIBRARY_CLASS = ResetSystemLib
> + LIBRARY_CLASS = ResetSystemLib|SEC PEI_CORE PEIM DXE_CORE
>
> #
> # The following information is for reference only and not required by the build
> diff --git a/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> similarity index 43%
> copy from OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> copy to OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> index 0772780b2dc2..a9b4ce90000a 100644
> --- a/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> +++ b/OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> @@ -1,18 +1,20 @@
> ## @file
> -# Base library instance for ResetSystem library class for OVMF
> +# DXE library instance for ResetSystem library class for OVMF
> #
> +# Copyright (C) 2020, Red Hat, Inc.
> # Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
> #
> ##
>
> [Defines]
> - INF_VERSION = 0x00010005
> - BASE_NAME = BaseResetSystemLib
> - FILE_GUID = 66564872-21d4-4d2a-a68b-1e844f980820
> - MODULE_TYPE = BASE
> + INF_VERSION = 1.29
> + BASE_NAME = DxeResetSystemLib
> + FILE_GUID = bc7835ea-4094-41fe-b770-bad9e6c479b2
> + MODULE_TYPE = DXE_DRIVER
> VERSION_STRING = 1.0
> - LIBRARY_CLASS = ResetSystemLib
> + LIBRARY_CLASS = ResetSystemLib|DXE_DRIVER DXE_RUNTIME_DRIVER SMM_CORE DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION
> + CONSTRUCTOR = DxeResetInit
>
> #
> # The following information is for reference only and not required by the build
> @@ -22,7 +24,7 @@ [Defines]
> #
>
> [Sources]
> - BaseResetShutdown.c
> + DxeResetShutdown.c
> ResetSystemLib.c
>
> [Packages]
> @@ -34,5 +36,8 @@ [LibraryClasses]
> BaseLib
> DebugLib
> IoLib
> - PciLib
> + PcdLib
> TimerLib
> +
> +[Pcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId ## CONSUMES
> diff --git a/OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c b/OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c
> similarity index 57%
> copy from OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
> copy to OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c
> index 21c80e43230c..5a75c32df361 100644
> --- a/OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
> +++ b/OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c
> @@ -1,5 +1,5 @@
> /** @file
> - Base Reset System Library Shutdown API implementation for OVMF.
> + DXE Reset System Library Shutdown API implementation for OVMF.
>
> Copyright (C) 2020, Red Hat, Inc.
> Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> @@ -11,41 +11,52 @@
> #include <Library/BaseLib.h> // CpuDeadLoop()
> #include <Library/DebugLib.h> // ASSERT()
> #include <Library/IoLib.h> // IoOr16()
> -#include <Library/PciLib.h> // PciRead16()
> +#include <Library/PcdLib.h> // PcdGet16()
> #include <Library/ResetSystemLib.h> // ResetShutdown()
> -#include <OvmfPlatforms.h> // OVMF_HOSTBRIDGE_DID
> +#include <OvmfPlatforms.h> // PIIX4_PMBA_VALUE
>
> -/**
> - Calling this function causes the system to enter a power state equivalent
> - to the ACPI G2/S5 or G3 states.
> +STATIC UINT16 mAcpiPmBaseAddress;
>
> - System shutdown should not return, if it returns, it means the system does
> - not support shut down reset.
> -**/
> -VOID
> +EFI_STATUS
> EFIAPI
> -ResetShutdown (
> - VOID
> +DxeResetInit (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
> - UINT16 AcpiPmBaseAddress;
> UINT16 HostBridgeDevId;
>
> - AcpiPmBaseAddress = 0;
> - HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
> + HostBridgeDevId = PcdGet16 (PcdOvmfHostBridgePciDevId);
> switch (HostBridgeDevId) {
> case INTEL_82441_DEVICE_ID:
> - AcpiPmBaseAddress = PIIX4_PMBA_VALUE;
> + mAcpiPmBaseAddress = PIIX4_PMBA_VALUE;
> break;
> case INTEL_Q35_MCH_DEVICE_ID:
> - AcpiPmBaseAddress = ICH9_PMBASE_VALUE;
> + mAcpiPmBaseAddress = ICH9_PMBASE_VALUE;
> break;
> default:
> ASSERT (FALSE);
> CpuDeadLoop ();
> + return EFI_UNSUPPORTED;
> }
>
> - IoBitFieldWrite16 (AcpiPmBaseAddress + 4, 10, 13, 0);
> - IoOr16 (AcpiPmBaseAddress + 4, BIT13);
> + return EFI_SUCCESS;
> +}
> +
> +/**
> + Calling this function causes the system to enter a power state equivalent
> + to the ACPI G2/S5 or G3 states.
> +
> + System shutdown should not return, if it returns, it means the system does
> + not support shut down reset.
> +**/
> +VOID
> +EFIAPI
> +ResetShutdown (
> + VOID
> + )
> +{
> + IoBitFieldWrite16 (mAcpiPmBaseAddress + 4, 10, 13, 0);
> + IoOr16 (mAcpiPmBaseAddress + 4, BIT13);
> CpuDeadLoop ();
> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix
2020-04-17 15:37 [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Laszlo Ersek
` (5 preceding siblings ...)
2020-04-17 15:37 ` [PATCH 6/6] OvmfPkg/ResetSystemLib: introduce the DxeResetSystemLib instance Laszlo Ersek
@ 2020-04-17 15:59 ` Ard Biesheuvel
2020-04-17 16:19 ` Philippe Mathieu-Daudé
2020-04-20 9:46 ` Laszlo Ersek
2020-04-21 17:49 ` Rebecca Cran
2020-04-22 20:35 ` [edk2-devel] " Laszlo Ersek
8 siblings, 2 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2020-04-17 15:59 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Anthony Perard, Jordan Justen, Julien Grall,
Philippe Mathieu-Daudé, Rebecca Cran
On 4/17/20 5:37 PM, Laszlo Ersek wrote:
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
> Repo: https://pagure.io/lersek/edk2.git
> Branch: rsl_cleanup
>
> Rebecca's
>
> [PATCH 02/13] OvmfPkg: support powering off bhyve guests
>
> at
>
> https://edk2.groups.io/g/devel/message/57450
> http://mid.mail-archive.com/e4e9b29189b83076e1d1a0b9c989938f5226cab6.1586991816.git.rebecca@bsdio.com
>
> made me realize OvmfPkg/ResetSystemLib should be refreshed. This will
> let us add a simple bhyve-specific instance (later), and also allows us
> to fix a long time dormant bug (now).
>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (6):
> OvmfPkg/ResetSystemLib: wrap long lines
> OvmfPkg/ResetSystemLib: clean up library dependencies
> OvmfPkg/ResetSystemLib: improve coding style in ResetSystem()
> OvmfPkg/ResetSystemLib: factor out ResetShutdown()
> OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib
> OvmfPkg/ResetSystemLib: introduce the DxeResetSystemLib instance
>
For the series,
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
One nit: putting a diff block inside the commit log [6/6] doesn't help
legibility a lot, and the issue of not being able to access memory that
is not mapped for runtime is so basic that it doesn't require that level
of detail to describe a reproducer and the Linux kernel log output when
the issue is triggered.
> OvmfPkg/OvmfPkgIa32.dsc | 8 +-
> OvmfPkg/OvmfPkgIa32X64.dsc | 8 +-
> OvmfPkg/OvmfPkgX64.dsc | 8 +-
> OvmfPkg/OvmfXen.dsc | 6 +-
> OvmfPkg/Library/ResetSystemLib/{ResetSystemLib.inf => BaseResetSystemLib.inf} | 12 ++-
> OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf | 43 ++++++++++
> OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c | 51 ++++++++++++
> OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c | 62 +++++++++++++++
> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c | 84 +++++---------------
> 9 files changed, 209 insertions(+), 73 deletions(-)
> rename OvmfPkg/Library/ResetSystemLib/{ResetSystemLib.inf => BaseResetSystemLib.inf} (65%)
> create mode 100644 OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> create mode 100644 OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
> create mode 100644 OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix
2020-04-17 15:59 ` [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Ard Biesheuvel
@ 2020-04-17 16:19 ` Philippe Mathieu-Daudé
2020-04-20 9:48 ` Laszlo Ersek
2020-04-20 9:46 ` Laszlo Ersek
1 sibling, 1 reply; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-17 16:19 UTC (permalink / raw)
To: Ard Biesheuvel, Laszlo Ersek, edk2-devel-groups-io
Cc: Anthony Perard, Jordan Justen, Julien Grall, Rebecca Cran
On 4/17/20 5:59 PM, Ard Biesheuvel wrote:
> On 4/17/20 5:37 PM, Laszlo Ersek wrote:
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
>> Repo: https://pagure.io/lersek/edk2.git
>> Branch: rsl_cleanup
>>
>> Rebecca's
>>
>> [PATCH 02/13] OvmfPkg: support powering off bhyve guests
>>
>> at
>>
>> https://edk2.groups.io/g/devel/message/57450
>>
>> http://mid.mail-archive.com/e4e9b29189b83076e1d1a0b9c989938f5226cab6.1586991816.git.rebecca@bsdio.com
>>
>>
>> made me realize OvmfPkg/ResetSystemLib should be refreshed. This will
>> let us add a simple bhyve-specific instance (later), and also allows us
>> to fix a long time dormant bug (now).
>>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Rebecca Cran <rebecca@bsdio.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (6):
>> OvmfPkg/ResetSystemLib: wrap long lines
>> OvmfPkg/ResetSystemLib: clean up library dependencies
>> OvmfPkg/ResetSystemLib: improve coding style in ResetSystem()
>> OvmfPkg/ResetSystemLib: factor out ResetShutdown()
>> OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib
>> OvmfPkg/ResetSystemLib: introduce the DxeResetSystemLib instance
>>
>
> For the series,
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>
> One nit: putting a diff block inside the commit log [6/6] doesn't help
> legibility a lot, and the issue of not being able to access memory that
> is not mapped for runtime is so basic that it doesn't require that level
> of detail to describe a reproducer and the Linux kernel log output when
> the issue is triggered.
Personally I find the kernel log relevant, it will help to understand th
e patch when looking at it in >5years from now.
>
>>
>> OvmfPkg/OvmfPkgIa32.dsc
>> | 8 +-
>>
>> OvmfPkg/OvmfPkgIa32X64.dsc
>> | 8 +-
>>
>> OvmfPkg/OvmfPkgX64.dsc
>> | 8 +-
>>
>> OvmfPkg/OvmfXen.dsc
>> | 6 +-
>> OvmfPkg/Library/ResetSystemLib/{ResetSystemLib.inf =>
>> BaseResetSystemLib.inf} | 12 ++-
>>
>> OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
>> | 43 ++++++++++
>>
>> OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
>> | 51 ++++++++++++
>>
>> OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c
>> | 62 +++++++++++++++
>>
>> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
>> | 84 +++++---------------
>> 9 files changed, 209 insertions(+), 73 deletions(-)
>> rename OvmfPkg/Library/ResetSystemLib/{ResetSystemLib.inf =>
>> BaseResetSystemLib.inf} (65%)
>> create mode 100644 OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
>> create mode 100644 OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
>> create mode 100644 OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix
2020-04-17 16:19 ` Philippe Mathieu-Daudé
@ 2020-04-20 9:48 ` Laszlo Ersek
2020-04-20 10:17 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2020-04-20 9:48 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Ard Biesheuvel
Cc: edk2-devel-groups-io, Anthony Perard, Jordan Justen, Julien Grall,
Rebecca Cran
On 04/17/20 18:19, Philippe Mathieu-Daudé wrote:
> On 4/17/20 5:59 PM, Ard Biesheuvel wrote:
>> On 4/17/20 5:37 PM, Laszlo Ersek wrote:
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
>>> Repo: https://pagure.io/lersek/edk2.git
>>> Branch: rsl_cleanup
>>>
>>> Rebecca's
>>>
>>> [PATCH 02/13] OvmfPkg: support powering off bhyve guests
>>>
>>> at
>>>
>>> https://edk2.groups.io/g/devel/message/57450
>>>
>>> http://mid.mail-archive.com/e4e9b29189b83076e1d1a0b9c989938f5226cab6.1586991816.git.rebecca@bsdio.com
>>>
>>>
>>> made me realize OvmfPkg/ResetSystemLib should be refreshed. This will
>>> let us add a simple bhyve-specific instance (later), and also allows us
>>> to fix a long time dormant bug (now).
>>>
>>> Cc: Anthony Perard <anthony.perard@citrix.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Julien Grall <julien@xen.org>
>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Cc: Rebecca Cran <rebecca@bsdio.com>
>>>
>>> Thanks,
>>> Laszlo
>>>
>>> Laszlo Ersek (6):
>>> OvmfPkg/ResetSystemLib: wrap long lines
>>> OvmfPkg/ResetSystemLib: clean up library dependencies
>>> OvmfPkg/ResetSystemLib: improve coding style in ResetSystem()
>>> OvmfPkg/ResetSystemLib: factor out ResetShutdown()
>>> OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib
>>> OvmfPkg/ResetSystemLib: introduce the DxeResetSystemLib instance
>>>
>>
>> For the series,
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>
>> One nit: putting a diff block inside the commit log [6/6] doesn't help
>> legibility a lot, and the issue of not being able to access memory
>> that is not mapped for runtime is so basic that it doesn't require
>> that level of detail to describe a reproducer and the Linux kernel log
>> output when the issue is triggered.
>
> Personally I find the kernel log relevant, it will help to understand th
> e patch when looking at it in >5years from now.
Is it acceptable to both of you (Phil and Ard) if I remove the third
note from the 6/6 commit message, but paste it into a new comment on
<https://bugzilla.tianocore.org/show_bug.cgi?id=2675>?
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix
2020-04-20 9:48 ` Laszlo Ersek
@ 2020-04-20 10:17 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-20 10:17 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel
Cc: edk2-devel-groups-io, Anthony Perard, Jordan Justen, Julien Grall,
Rebecca Cran
On 4/20/20 11:48 AM, Laszlo Ersek wrote:
> On 04/17/20 18:19, Philippe Mathieu-Daudé wrote:
>> On 4/17/20 5:59 PM, Ard Biesheuvel wrote:
>>> On 4/17/20 5:37 PM, Laszlo Ersek wrote:
>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
>>>> Repo: https://pagure.io/lersek/edk2.git
>>>> Branch: rsl_cleanup
>>>>
>>>> Rebecca's
>>>>
>>>> [PATCH 02/13] OvmfPkg: support powering off bhyve guests
>>>>
>>>> at
>>>>
>>>> https://edk2.groups.io/g/devel/message/57450
>>>>
>>>> http://mid.mail-archive.com/e4e9b29189b83076e1d1a0b9c989938f5226cab6.1586991816.git.rebecca@bsdio.com
>>>>
>>>>
>>>> made me realize OvmfPkg/ResetSystemLib should be refreshed. This will
>>>> let us add a simple bhyve-specific instance (later), and also allows us
>>>> to fix a long time dormant bug (now).
>>>>
>>>> Cc: Anthony Perard <anthony.perard@citrix.com>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>> Cc: Julien Grall <julien@xen.org>
>>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> Cc: Rebecca Cran <rebecca@bsdio.com>
>>>>
>>>> Thanks,
>>>> Laszlo
>>>>
>>>> Laszlo Ersek (6):
>>>> OvmfPkg/ResetSystemLib: wrap long lines
>>>> OvmfPkg/ResetSystemLib: clean up library dependencies
>>>> OvmfPkg/ResetSystemLib: improve coding style in ResetSystem()
>>>> OvmfPkg/ResetSystemLib: factor out ResetShutdown()
>>>> OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib
>>>> OvmfPkg/ResetSystemLib: introduce the DxeResetSystemLib instance
>>>>
>>>
>>> For the series,
>>>
>>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>>
>>> One nit: putting a diff block inside the commit log [6/6] doesn't help
>>> legibility a lot, and the issue of not being able to access memory
>>> that is not mapped for runtime is so basic that it doesn't require
>>> that level of detail to describe a reproducer and the Linux kernel log
>>> output when the issue is triggered.
>>
>> Personally I find the kernel log relevant, it will help to understand th
>> e patch when looking at it in >5years from now.
>
> Is it acceptable to both of you (Phil and Ard) if I remove the third
> note from the 6/6 commit message, but paste it into a new comment on
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2675>?
Certainly.
Personal note: I usually try to find answers to my questions on the
mailing list archives, usually they are already answered (even often
with many details!). I noticed with other (smaller) projects that moved
to GitLab that 'googling' similarly is not very practical, eventually
after learning something that looks like a mix of SQL and JSON I could
find some information in failed merge request comments... Nothing as
easy as grepping my local MAILBOX (which, by the way, works offline
too). Having that in mind (that mailing list archives are a thing from
the previous century and are going to disappear) I prefer to keep all
the information in the commit message. There is always a trade off.
I'm not sure the Tianocore Bugzilla will make sense once EDK2 move to a
git forge, it seems duplicating efforts.
My 2 cents.
> Thanks!
> Laszlo
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix
2020-04-17 15:59 ` [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Ard Biesheuvel
2020-04-17 16:19 ` Philippe Mathieu-Daudé
@ 2020-04-20 9:46 ` Laszlo Ersek
1 sibling, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2020-04-20 9:46 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel-groups-io
Cc: Anthony Perard, Jordan Justen, Julien Grall,
Philippe Mathieu-Daudé, Rebecca Cran
On 04/17/20 17:59, Ard Biesheuvel wrote:
> On 4/17/20 5:37 PM, Laszlo Ersek wrote:
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
>> Repo: https://pagure.io/lersek/edk2.git
>> Branch: rsl_cleanup
>>
>> Rebecca's
>>
>> [PATCH 02/13] OvmfPkg: support powering off bhyve guests
>>
>> at
>>
>> https://edk2.groups.io/g/devel/message/57450
>>
>> http://mid.mail-archive.com/e4e9b29189b83076e1d1a0b9c989938f5226cab6.1586991816.git.rebecca@bsdio.com
>>
>>
>> made me realize OvmfPkg/ResetSystemLib should be refreshed. This will
>> let us add a simple bhyve-specific instance (later), and also allows us
>> to fix a long time dormant bug (now).
>>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Rebecca Cran <rebecca@bsdio.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (6):
>> OvmfPkg/ResetSystemLib: wrap long lines
>> OvmfPkg/ResetSystemLib: clean up library dependencies
>> OvmfPkg/ResetSystemLib: improve coding style in ResetSystem()
>> OvmfPkg/ResetSystemLib: factor out ResetShutdown()
>> OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib
>> OvmfPkg/ResetSystemLib: introduce the DxeResetSystemLib instance
>>
>
> For the series,
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>
> One nit: putting a diff block inside the commit log [6/6] doesn't help
> legibility a lot, and the issue of not being able to access memory that
> is not mapped for runtime is so basic that it doesn't require that level
> of detail to describe a reproducer and the Linux kernel log output when
> the issue is triggered.
Thanks! I will remove the third note altogether (the one that starts
with "The bug is not easy to trigger in common setups...").
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix
2020-04-17 15:37 [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Laszlo Ersek
` (6 preceding siblings ...)
2020-04-17 15:59 ` [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Ard Biesheuvel
@ 2020-04-21 17:49 ` Rebecca Cran
2020-04-22 20:35 ` [edk2-devel] " Laszlo Ersek
8 siblings, 0 replies; 21+ messages in thread
From: Rebecca Cran @ 2020-04-21 17:49 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Anthony Perard, Ard Biesheuvel, Jordan Justen, Julien Grall,
Philippe Mathieu-Daudé
On 4/17/20 9:37 AM, Laszlo Ersek wrote:
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
> Repo: https://pagure.io/lersek/edk2.git
> Branch: rsl_cleanup
>
> Rebecca's
>
> [PATCH 02/13] OvmfPkg: support powering off bhyve guests
>
> at
>
> https://edk2.groups.io/g/devel/message/57450
> http://mid.mail-archive.com/e4e9b29189b83076e1d1a0b9c989938f5226cab6.1586991816.git.rebecca@bsdio.com
>
> made me realize OvmfPkg/ResetSystemLib should be refreshed. This will
> let us add a simple bhyve-specific instance (later), and also allows us
> to fix a long time dormant bug (now).
>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
For the series,
Reviewed-by: Rebecca Cran <rebecca@bsdio.com>
--
Rebecca Cran
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [edk2-devel] [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix
2020-04-17 15:37 [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix Laszlo Ersek
` (7 preceding siblings ...)
2020-04-21 17:49 ` Rebecca Cran
@ 2020-04-22 20:35 ` Laszlo Ersek
8 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2020-04-22 20:35 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Anthony Perard, Ard Biesheuvel, Jordan Justen, Julien Grall,
Philippe Mathieu-Daudé, Rebecca Cran
On 04/17/20 17:37, Laszlo Ersek wrote:
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
> Repo: https://pagure.io/lersek/edk2.git
> Branch: rsl_cleanup
>
> Rebecca's
>
> [PATCH 02/13] OvmfPkg: support powering off bhyve guests
>
> at
>
> https://edk2.groups.io/g/devel/message/57450
> http://mid.mail-archive.com/e4e9b29189b83076e1d1a0b9c989938f5226cab6.1586991816.git.rebecca@bsdio.com
>
> made me realize OvmfPkg/ResetSystemLib should be refreshed. This will
> let us add a simple bhyve-specific instance (later), and also allows us
> to fix a long time dormant bug (now).
>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (6):
> OvmfPkg/ResetSystemLib: wrap long lines
> OvmfPkg/ResetSystemLib: clean up library dependencies
> OvmfPkg/ResetSystemLib: improve coding style in ResetSystem()
> OvmfPkg/ResetSystemLib: factor out ResetShutdown()
> OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib
> OvmfPkg/ResetSystemLib: introduce the DxeResetSystemLib instance
>
> OvmfPkg/OvmfPkgIa32.dsc | 8 +-
> OvmfPkg/OvmfPkgIa32X64.dsc | 8 +-
> OvmfPkg/OvmfPkgX64.dsc | 8 +-
> OvmfPkg/OvmfXen.dsc | 6 +-
> OvmfPkg/Library/ResetSystemLib/{ResetSystemLib.inf => BaseResetSystemLib.inf} | 12 ++-
> OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf | 43 ++++++++++
> OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c | 51 ++++++++++++
> OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c | 62 +++++++++++++++
> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c | 84 +++++---------------
> 9 files changed, 209 insertions(+), 73 deletions(-)
> rename OvmfPkg/Library/ResetSystemLib/{ResetSystemLib.inf => BaseResetSystemLib.inf} (65%)
> create mode 100644 OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
> create mode 100644 OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
> create mode 100644 OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c
>
Merged as commit range c6a60cf4b990..93f6df5f3b25, via
<https://github.com/tianocore/edk2/pull/538>.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread