* [PATCH 0/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT
@ 2017-08-06 12:16 Laszlo Ersek
2017-08-06 12:16 ` [PATCH 1/1] " Laszlo Ersek
2017-08-08 13:16 ` [PATCH 0/1] " Laszlo Ersek
0 siblings, 2 replies; 5+ messages in thread
From: Laszlo Ersek @ 2017-08-06 12:16 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Dhiru Kholia, Jordan Justen
The commit message says it all; in the blurb I'm only mentioning the
fetch location and the fact that I regression-tested the bugfix with
VMGENID. Dhiru should please report back with a Tested-by if the patch
solves the symptom he sees.
Repo: https://github.com/lersek/edk2.git
Branch: acpi_s3ctx_empty
Cc: Dhiru Kholia <dhiru.kholia@gmail.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Thanks
Laszlo
Laszlo Ersek (1):
OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty
S3_CONTEXT
OvmfPkg/AcpiPlatformDxe/BootScript.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT
2017-08-06 12:16 [PATCH 0/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT Laszlo Ersek
@ 2017-08-06 12:16 ` Laszlo Ersek
2017-08-06 13:57 ` Dhiru Kholia
2017-08-07 19:48 ` Jordan Justen
2017-08-08 13:16 ` [PATCH 0/1] " Laszlo Ersek
1 sibling, 2 replies; 5+ messages in thread
From: Laszlo Ersek @ 2017-08-06 12:16 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Dhiru Kholia, Jordan Justen
In commit 805762252733 ("OvmfPkg/AcpiPlatformDxe: save fw_cfg boot script
with QemuFwCfgS3Lib", 2017-02-23), we replaced the explicit S3 boot script
manipulation in TransferS3ContextToBootScript() with a call to
QemuFwCfgS3CallWhenBootScriptReady(). (Passing AppendFwCfgBootScript() as
callback.)
QemuFwCfgS3CallWhenBootScriptReady() checks for fw_cfg DMA up-front, and
bails with RETURN_NOT_FOUND if fw_cfg DMA is missing.
(This is justified as the goal of QemuFwCfgS3Lib is to "enable[] driver
modules [...] to produce fw_cfg DMA operations that are to be replayed at
S3 resume time".)
In turn, if QemuFwCfgS3CallWhenBootScriptReady() fails, then
OvmfPkg/AcpiPlatformDxe rolls back any earlier linker/loader script
processing, and falls back to the built-in ACPI tables.
(This is also justified because failure to save WRITE_POINTER commands for
replaying at S3 resume implies failure to process the linker/loader script
comprehensively.)
Calling QemuFwCfgS3CallWhenBootScriptReady() from
TransferS3ContextToBootScript() *unconditionally* is wrong however. For
the case when the linker/loader script contains no WRITE_POINTER commands,
the call perpetuated an earlier side effect, and introduced another one:
(1) On machine types that provide fw_cfg DMA (i.e., 2.5+),
QemuFwCfgS3CallWhenBootScriptReady() would succeed, and allocate
workspace for the boot script opcodes in reserved memory. However, no
opcodes would actually be produced in the AppendFwCfgBootScript()
callback, due to lack of any WRITE_POINTER commands.
This waste of reserved memory had been introduced in earlier commit
df73df138d9d ("OvmfPkg/AcpiPlatformDxe: replay
QEMU_LOADER_WRITE_POINTER commands at S3", 2017-02-09).
(2) On machine types that lack fw_cfg DMA (i.e., 2.4 and earlier),
TransferS3ContextToBootScript() would now fail the linker/loader
script for no reason.
(Note that QEMU itself prevents adding devices that depend on
WRITE_POINTER if the machine type lacks fw_cfg DMA:
$ qemu-system-x86_64 -M pc-q35-2.4 -device vmgenid
qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write
support in fw_cfg, which this machine type does not provide)
Short-circuit an empty S3_CONTEXT in TransferS3ContextToBootScript() by
dropping S3_CONTEXT on the floor. This is compatible with the current
contract of the function as it constitutes a transfer of ownership.
Regression (2) was found and reported by Dhiru Kholia as an OSX guest boot
failure on the "pc-q35-2.4" machine type:
http://mid.mail-archive.com/CANO7a6x6EaWNZ8y=MvLU=w_LjRLXserO3NmsgHvaYE0aUCCWzg@mail.gmail.com
Dhiru bisected the issue to commit 805762252733.
Cc: Dhiru Kholia <dhiru.kholia@gmail.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Fixes: df73df138d9d53f7f7570f4fe97a6cde941a2656
Fixes: 805762252733bb67bc5157f0137c64e010724c77
Reported-by: Dhiru Kholia <dhiru.kholia@gmail.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/AcpiPlatformDxe/BootScript.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/AcpiPlatformDxe/BootScript.c b/OvmfPkg/AcpiPlatformDxe/BootScript.c
index a7d2f9e38f57..1a188bfd615c 100644
--- a/OvmfPkg/AcpiPlatformDxe/BootScript.c
+++ b/OvmfPkg/AcpiPlatformDxe/BootScript.c
@@ -249,7 +249,10 @@ FatalError:
because the ownership of S3Context has been transfered.
@retval EFI_SUCCESS The translation of S3Context to ACPI S3 Boot Script
- opcodes has been successfully executed or queued.
+ opcodes has been successfully executed or queued. (This
+ includes the case when S3Context was empty on input and
+ no ACPI S3 Boot Script opcodes have been necessary to
+ produce.)
@return Error codes from underlying functions.
**/
@@ -260,6 +263,11 @@ TransferS3ContextToBootScript (
{
RETURN_STATUS Status;
+ if (S3Context->Used == 0) {
+ ReleaseS3Context (S3Context);
+ return EFI_SUCCESS;
+ }
+
Status = QemuFwCfgS3CallWhenBootScriptReady (AppendFwCfgBootScript,
S3Context, sizeof (SCRATCH_BUFFER));
return (EFI_STATUS)Status;
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT
2017-08-06 12:16 ` [PATCH 1/1] " Laszlo Ersek
@ 2017-08-06 13:57 ` Dhiru Kholia
2017-08-07 19:48 ` Jordan Justen
1 sibling, 0 replies; 5+ messages in thread
From: Dhiru Kholia @ 2017-08-06 13:57 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On Sun, Aug 06, 2017 at 02:16:22PM +0200, Laszlo Ersek wrote:
> In commit 805762252733 ("OvmfPkg/AcpiPlatformDxe: save fw_cfg boot script
> with QemuFwCfgS3Lib", 2017-02-23), we replaced the explicit S3 boot script
> manipulation in TransferS3ContextToBootScript() with a call to
> QemuFwCfgS3CallWhenBootScriptReady(). (Passing AppendFwCfgBootScript() as
> callback.)
>
> QemuFwCfgS3CallWhenBootScriptReady() checks for fw_cfg DMA up-front, and
> bails with RETURN_NOT_FOUND if fw_cfg DMA is missing.
>
> (This is justified as the goal of QemuFwCfgS3Lib is to "enable[] driver
> modules [...] to produce fw_cfg DMA operations that are to be replayed at
> S3 resume time".)
>
> In turn, if QemuFwCfgS3CallWhenBootScriptReady() fails, then
> OvmfPkg/AcpiPlatformDxe rolls back any earlier linker/loader script
> processing, and falls back to the built-in ACPI tables.
>
> (This is also justified because failure to save WRITE_POINTER commands for
> replaying at S3 resume implies failure to process the linker/loader script
> comprehensively.)
>
> Calling QemuFwCfgS3CallWhenBootScriptReady() from
> TransferS3ContextToBootScript() *unconditionally* is wrong however. For
> the case when the linker/loader script contains no WRITE_POINTER commands,
> the call perpetuated an earlier side effect, and introduced another one:
>
> (1) On machine types that provide fw_cfg DMA (i.e., 2.5+),
> QemuFwCfgS3CallWhenBootScriptReady() would succeed, and allocate
> workspace for the boot script opcodes in reserved memory. However, no
> opcodes would actually be produced in the AppendFwCfgBootScript()
> callback, due to lack of any WRITE_POINTER commands.
>
> This waste of reserved memory had been introduced in earlier commit
> df73df138d9d ("OvmfPkg/AcpiPlatformDxe: replay
> QEMU_LOADER_WRITE_POINTER commands at S3", 2017-02-09).
>
> (2) On machine types that lack fw_cfg DMA (i.e., 2.4 and earlier),
> TransferS3ContextToBootScript() would now fail the linker/loader
> script for no reason.
>
> (Note that QEMU itself prevents adding devices that depend on
> WRITE_POINTER if the machine type lacks fw_cfg DMA:
>
> $ qemu-system-x86_64 -M pc-q35-2.4 -device vmgenid
>
> qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write
> support in fw_cfg, which this machine type does not provide)
>
> Short-circuit an empty S3_CONTEXT in TransferS3ContextToBootScript() by
> dropping S3_CONTEXT on the floor. This is compatible with the current
> contract of the function as it constitutes a transfer of ownership.
>
> Regression (2) was found and reported by Dhiru Kholia as an OSX guest boot
> failure on the "pc-q35-2.4" machine type:
>
> http://mid.mail-archive.com/CANO7a6x6EaWNZ8y=MvLU=w_LjRLXserO3NmsgHvaYE0aUCCWzg@mail.gmail.com
>
> Dhiru bisected the issue to commit 805762252733.
>
> Cc: Dhiru Kholia <dhiru.kholia@gmail.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Fixes: df73df138d9d53f7f7570f4fe97a6cde941a2656
> Fixes: 805762252733bb67bc5157f0137c64e010724c77
> Reported-by: Dhiru Kholia <dhiru.kholia@gmail.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/AcpiPlatformDxe/BootScript.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/BootScript.c b/OvmfPkg/AcpiPlatformDxe/BootScript.c
> index a7d2f9e38f57..1a188bfd615c 100644
> --- a/OvmfPkg/AcpiPlatformDxe/BootScript.c
> +++ b/OvmfPkg/AcpiPlatformDxe/BootScript.c
> @@ -249,7 +249,10 @@ FatalError:
> because the ownership of S3Context has been transfered.
>
> @retval EFI_SUCCESS The translation of S3Context to ACPI S3 Boot Script
> - opcodes has been successfully executed or queued.
> + opcodes has been successfully executed or queued. (This
> + includes the case when S3Context was empty on input and
> + no ACPI S3 Boot Script opcodes have been necessary to
> + produce.)
>
> @return Error codes from underlying functions.
> **/
> @@ -260,6 +263,11 @@ TransferS3ContextToBootScript (
> {
> RETURN_STATUS Status;
>
> + if (S3Context->Used == 0) {
> + ReleaseS3Context (S3Context);
> + return EFI_SUCCESS;
> + }
> +
> Status = QemuFwCfgS3CallWhenBootScriptReady (AppendFwCfgBootScript,
> S3Context, sizeof (SCRATCH_BUFFER));
> return (EFI_STATUS)Status;
Tested-by: Dhiru Kholia <dhiru.kholia@gmail.com>
I can now successfully boot macOS 10.12.5 + Clover combination with
"pc-q35-2.4" machine type with this patch applied on top of commit
1fceaddb12.
Thanks,
Dhiru
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT
2017-08-06 12:16 ` [PATCH 1/1] " Laszlo Ersek
2017-08-06 13:57 ` Dhiru Kholia
@ 2017-08-07 19:48 ` Jordan Justen
1 sibling, 0 replies; 5+ messages in thread
From: Jordan Justen @ 2017-08-07 19:48 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
On 2017-08-06 05:16:22, Laszlo Ersek wrote:
> In commit 805762252733 ("OvmfPkg/AcpiPlatformDxe: save fw_cfg boot script
> with QemuFwCfgS3Lib", 2017-02-23), we replaced the explicit S3 boot script
> manipulation in TransferS3ContextToBootScript() with a call to
> QemuFwCfgS3CallWhenBootScriptReady(). (Passing AppendFwCfgBootScript() as
> callback.)
>
> QemuFwCfgS3CallWhenBootScriptReady() checks for fw_cfg DMA up-front, and
> bails with RETURN_NOT_FOUND if fw_cfg DMA is missing.
>
> (This is justified as the goal of QemuFwCfgS3Lib is to "enable[] driver
> modules [...] to produce fw_cfg DMA operations that are to be replayed at
> S3 resume time".)
>
> In turn, if QemuFwCfgS3CallWhenBootScriptReady() fails, then
> OvmfPkg/AcpiPlatformDxe rolls back any earlier linker/loader script
> processing, and falls back to the built-in ACPI tables.
>
> (This is also justified because failure to save WRITE_POINTER commands for
> replaying at S3 resume implies failure to process the linker/loader script
> comprehensively.)
>
> Calling QemuFwCfgS3CallWhenBootScriptReady() from
> TransferS3ContextToBootScript() *unconditionally* is wrong however. For
> the case when the linker/loader script contains no WRITE_POINTER commands,
> the call perpetuated an earlier side effect, and introduced another one:
>
> (1) On machine types that provide fw_cfg DMA (i.e., 2.5+),
> QemuFwCfgS3CallWhenBootScriptReady() would succeed, and allocate
> workspace for the boot script opcodes in reserved memory. However, no
> opcodes would actually be produced in the AppendFwCfgBootScript()
> callback, due to lack of any WRITE_POINTER commands.
>
> This waste of reserved memory had been introduced in earlier commit
> df73df138d9d ("OvmfPkg/AcpiPlatformDxe: replay
> QEMU_LOADER_WRITE_POINTER commands at S3", 2017-02-09).
>
> (2) On machine types that lack fw_cfg DMA (i.e., 2.4 and earlier),
> TransferS3ContextToBootScript() would now fail the linker/loader
> script for no reason.
>
> (Note that QEMU itself prevents adding devices that depend on
> WRITE_POINTER if the machine type lacks fw_cfg DMA:
>
> $ qemu-system-x86_64 -M pc-q35-2.4 -device vmgenid
>
> qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write
> support in fw_cfg, which this machine type does not provide)
>
> Short-circuit an empty S3_CONTEXT in TransferS3ContextToBootScript() by
> dropping S3_CONTEXT on the floor. This is compatible with the current
> contract of the function as it constitutes a transfer of ownership.
>
> Regression (2) was found and reported by Dhiru Kholia as an OSX guest boot
> failure on the "pc-q35-2.4" machine type:
>
> http://mid.mail-archive.com/CANO7a6x6EaWNZ8y=MvLU=w_LjRLXserO3NmsgHvaYE0aUCCWzg@mail.gmail.com
>
> Dhiru bisected the issue to commit 805762252733.
>
> Cc: Dhiru Kholia <dhiru.kholia@gmail.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Fixes: df73df138d9d53f7f7570f4fe97a6cde941a2656
> Fixes: 805762252733bb67bc5157f0137c64e010724c77
> Reported-by: Dhiru Kholia <dhiru.kholia@gmail.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/AcpiPlatformDxe/BootScript.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/BootScript.c b/OvmfPkg/AcpiPlatformDxe/BootScript.c
> index a7d2f9e38f57..1a188bfd615c 100644
> --- a/OvmfPkg/AcpiPlatformDxe/BootScript.c
> +++ b/OvmfPkg/AcpiPlatformDxe/BootScript.c
> @@ -249,7 +249,10 @@ FatalError:
> because the ownership of S3Context has been transfered.
>
> @retval EFI_SUCCESS The translation of S3Context to ACPI S3 Boot Script
> - opcodes has been successfully executed or queued.
> + opcodes has been successfully executed or queued. (This
> + includes the case when S3Context was empty on input and
> + no ACPI S3 Boot Script opcodes have been necessary to
> + produce.)
>
> @return Error codes from underlying functions.
> **/
> @@ -260,6 +263,11 @@ TransferS3ContextToBootScript (
> {
> RETURN_STATUS Status;
>
> + if (S3Context->Used == 0) {
> + ReleaseS3Context (S3Context);
> + return EFI_SUCCESS;
> + }
> +
> Status = QemuFwCfgS3CallWhenBootScriptReady (AppendFwCfgBootScript,
> S3Context, sizeof (SCRATCH_BUFFER));
> return (EFI_STATUS)Status;
> --
> 2.13.1.3.g8be5a757fa67
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 0/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT
2017-08-06 12:16 [PATCH 0/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT Laszlo Ersek
2017-08-06 12:16 ` [PATCH 1/1] " Laszlo Ersek
@ 2017-08-08 13:16 ` Laszlo Ersek
1 sibling, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2017-08-08 13:16 UTC (permalink / raw)
To: Jordan Justen, Dhiru Kholia; +Cc: edk2-devel-01
On 08/06/17 14:16, Laszlo Ersek wrote:
> The commit message says it all; in the blurb I'm only mentioning the
> fetch location and the fact that I regression-tested the bugfix with
> VMGENID. Dhiru should please report back with a Tested-by if the patch
> solves the symptom he sees.
>
> Repo: https://github.com/lersek/edk2.git
> Branch: acpi_s3ctx_empty
>
> Cc: Dhiru Kholia <dhiru.kholia@gmail.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (1):
> OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty
> S3_CONTEXT
>
> OvmfPkg/AcpiPlatformDxe/BootScript.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
Thank you guys for the testing and the review; I've pushed the patch as
commit 9e2a8e928995.
Cheers
Laszlo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-08-08 13:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-06 12:16 [PATCH 0/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT Laszlo Ersek
2017-08-06 12:16 ` [PATCH 1/1] " Laszlo Ersek
2017-08-06 13:57 ` Dhiru Kholia
2017-08-07 19:48 ` Jordan Justen
2017-08-08 13:16 ` [PATCH 0/1] " Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox