[PATCH 0/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT

[PATCH 0/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT

[Laszlo Ersek]

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

[PATCH 1/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT

[Laszlo Ersek]

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

Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT

[Dhiru Kholia]

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

Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT

[Jordan Justen]

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
> 

Re: [PATCH 0/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT

[Laszlo Ersek]

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