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

[Jordan Justen <jordan.l.justen@intel.com>]

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
>