* 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