Re: [edk2] [PATCH v5 03/19] OvmfPkg QemuFwCfgLib: determine if S3 support is explicitly enabled

Subject: Re: [edk2] [PATCH v5 03/19] OvmfPkg QemuFwCfgLib: determine if S3 support is explicitly enabled

From: Laszlo Ersek <lersek@redhat.com>

To: Jordan Justen <jordan.l.justen@intel.com>

Date: 2014-02-11 23:44:58

On 02/08/14 00:17, Jordan Justen wrote:
> From: Laszlo Ersek 
> 
> Such a packaged query function will come in handy in the following
> patches.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> Reviewed-by: Jordan Justen 
> [jordan.l.justen@intel.com: check for enabled rather than disabled]
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jordan Justen 
> ---
>  OvmfPkg/Include/Library/QemuFwCfgLib.h      | 14 ++++++++++++++
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 28 ++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/OvmfPkg/Include/Library/QemuFwCfgLib.h b/OvmfPkg/Include/Library/QemuFwCfgLib.h
> index 2519fc2..8d3b835 100644
> --- a/OvmfPkg/Include/Library/QemuFwCfgLib.h
> +++ b/OvmfPkg/Include/Library/QemuFwCfgLib.h
> @@ -214,5 +214,19 @@ InternalQemuFwCfgIsAvailable (
>    VOID
>    );
>  
> +
> +/**
> +  Determine if S3 support is explicitly enabled.
> +
> +  @retval  TRUE   if S3 support is explicitly enabled.
> +           FALSE  otherwise. This includes unavailability of the firmware
> +                  configuration interface.
> +**/
> +BOOLEAN
> +EFIAPI
> +QemuFwCfgS3Enabled (
> +  VOID
> +  );
> +
>  #endif
>  
> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> index 3c5963f..543c22c 100644
> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> @@ -294,3 +294,31 @@ QemuFwCfgFindFile (
>  
>    return RETURN_NOT_FOUND;
>  }
> +
> +
> +/**
> +  Determine if S3 support is explicitly enabled.
> +
> +  @retval  TRUE   if S3 support is explicitly enabled.
> +           FALSE  otherwise. This includes unavailability of the firmware
> +                  configuration interface.
> +**/
> +BOOLEAN
> +EFIAPI
> +QemuFwCfgS3Enabled (
> +  VOID
> +  )
> +{
> +  RETURN_STATUS        Status;
> +  FIRMWARE_CONFIG_ITEM FwCfgItem;
> +  UINTN                FwCfgSize;
> +  UINT8                SystemStates[6];
> +
> +  Status = QemuFwCfgFindFile ("etc/system-states", &FwCfgItem, &FwCfgSize);
> +  if (Status != RETURN_SUCCESS || FwCfgSize != sizeof SystemStates) {
> +    return FALSE;
> +  }
> +  QemuFwCfgSelectItem (FwCfgItem);
> +  QemuFwCfgReadBytes (sizeof SystemStates, SystemStates);
> +  return (SystemStates[3] & BIT7) != 0 ? TRUE : FALSE;
> +}
> 

So, if the "etc/system-states" fw_cfg file is not available, we'll now
opt for disabling S3 resume (including the ACPI NVS memory
reservations). I guess that's OK for maximum safety, although it'll
prevent us from providing S3 resume on qemu versions that support S3
resume but lack the fw_cfg file.

... disable_s3 was added in qemu commit 459ae5ea, first released in
v1.2.0. That counts as pretty compatible I believe.

Then... the expression

  condition ? TRUE : FALSE

especially where it is

  (integer-expression != 0) ? TRUE : FALSE

kind of makes me want to wash out my eyes. All of the following would
have been nicer:

  (BOOLEAN) (SystemStates[3] & BIT7)

  (SystemStates[3] & BIT7) != 0

  !!(SystemStates[3] & BIT7)

But it doesn't matter.

Reviewed-by: Laszlo Ersek 

Thanks,
Laszlo

------------------------------------------------------------------------------
Android apps run on BlackBerry 10
Introducing the new BlackBerry 10.2.1 Runtime for Android apps.
Now with support for Jelly Bean, Bluetooth, Mapview and more.
Get your Android app in front of a whole new audience.  Start now.
http://pubads.g.doubleclick.net/gampad/clk?id=124407151&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel