On 04/06/16 14:40, Star Zeng wrote:
> Also need to declare PcdAcpiS3Enable as DynamicDefault in *.dsc.
>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.z...@intel.com>
> ---
> OvmfPkg/OvmfPkgIa32.dsc | 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> OvmfPkg/OvmfPkgX64.dsc | 1 +
> OvmfPkg/PlatformPei/Platform.c | 4 +++-
> OvmfPkg/PlatformPei/PlatformPei.inf | 3 ++-
> 5 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 9e5b47738bd3..8136b80769b4 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -462,6 +462,7 @@ [PcdsDynamicDefault]
> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size|0x0
>
> gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable|TRUE
(1) Although this is not a "hard" requirement, and also not an
all-too-well defined one, I like to keep PCD settings "clustered" by
token space. (Functional relations are a stronger clustering guideline,
but this new PCD is not functionally related to any other PCD we have in
here.)
Thus, can you please locate
gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution
just a bit higher up (it is not visible in the context), and insert
PcdAcpiS3Enable right under it?
For the other DSC files as well. Thanks.
(2) Please set the default value to FALSE.
More comments below:
>
> # Set video resolution for text setup.
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution|640
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 6e4da4ff55c4..92f98fe23fcc 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -470,6 +470,7 @@ [PcdsDynamicDefault]
> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
>
> gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable|TRUE
>
> # Set video resolution for text setup.
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution|640
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 3d6d43ef73ab..9e60b498f97d 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -469,6 +469,7 @@ [PcdsDynamicDefault]
> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
>
> gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable|TRUE
>
> # Set video resolution for text setup.
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution|640
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 509343e0bee5..5774c61ad2a7 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -1,7 +1,7 @@
> /**@file
> Platform PEI driver
>
> - Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> Copyright (c) 2011, Andrei Warkentin <andr...@motorola.com>
>
> This program and the accompanying materials
> @@ -585,6 +585,8 @@ InitializePlatform (
> if (QemuFwCfgS3Enabled ()) {
> DEBUG ((EFI_D_INFO, "S3 support was detected on QEMU\n"));
> mS3Supported = TRUE;
> + } else {
> + PcdSetBoolS (PcdAcpiS3Enable, FALSE);
> }
(3) So, this else branch should be eliminated, and the opposite setting
(-> TRUE) should occur on the existent branch.
(4) Please either use PcdSetBool(), or check the return status of
PcdSetBoolS(). My vote would be PcdSetBool(), but if you prefer
PcdSetBoolS(), please add an ASSERT_EFI_ERROR().
>
> S3Verification ();
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
> b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 5b643a358f1f..de574684ebf2 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -2,7 +2,7 @@
> # Platform PEI driver
> #
> # This module provides platform specific function to detect boot mode.
> -# Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> #
> # This program and the accompanying materials
> # are licensed and made available under the terms and conditions of the BSD
> License
> @@ -95,6 +95,7 @@ [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack
> gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable
> gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
> + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
(5) Similarly to (1), please move this right under
gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable
Thanks!
Laszlo
>
> [FixedPcd]
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
