Re: [PATCH 5/7] OvmfPkg/PlatformPei: don't allocate reserved mem varstore if SMM_REQUIRE

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

On 2017-05-05 14:02:56, Laszlo Ersek wrote:
> For the emulated variable store, PlatformPei allocates reserved memory (as
> early as possible, so that the address remains the same during reboot),
> and PcdEmuVariableNvStoreReserved carries the address to
> EmuVariableFvbRuntimeDxe.
> 
> However, EmuVariableFvbRuntimeDxe is excluded from the SMM_REQUIRE build,
> and then noone consumes PcdEmuVariableNvStoreReserved. Don't waste
> reserved memory whenever that's the case.
> 
> (Even a dynamic default for PcdEmuVariableNvStoreReserved would be
> unnecessary; but that way the PcdSet64S() call in the
> ReserveEmuVariableNvStore() function doesn't compile.)
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc        | 3 +++
>  OvmfPkg/OvmfPkgIa32X64.dsc     | 3 +++
>  OvmfPkg/OvmfPkgX64.dsc         | 3 +++
>  OvmfPkg/PlatformPei/Platform.c | 4 +++-
>  4 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 64427716c53c..b46eef6cabc3 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -495,7 +495,10 @@ [PcdsFixedAtBuild]
>  ################################################################################
>  
>  [PcdsDynamicDefault]
> +  # only set when
> +  #   ($(SMM_REQUIRE) == FALSE)
>    gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
> +
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 887964cd27c2..08f471fbc542 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -501,7 +501,10 @@ [PcdsFixedAtBuild.X64]
>  ################################################################################
>  
>  [PcdsDynamicDefault]
> +  # only set when
> +  #   ($(SMM_REQUIRE) == FALSE)

I don't think we should bother adding these comments into the .dsc.

Ultimately, I would prefer to always allocate this, even when SMM is
set to be required. It'd be nice if we could always fallback to
EmuFvb, but I understand that this might not be possible given how
difficult it is to determine if QEMU actually has SMM enabled. Anyway,
I think this patch makes sense until we can potentially fix that.
(Which may or may not be worth fixing.)

Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

>    gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
> +
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index dc5fea3577d4..24053e5ff82d 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -500,7 +500,10 @@ [PcdsFixedAtBuild]
>  ################################################################################
>  
>  [PcdsDynamicDefault]
> +  # only set when
> +  #   ($(SMM_REQUIRE) == FALSE)
>    gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
> +
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 5e983a8dcea9..1b4dc00b0180 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -672,7 +672,9 @@ InitializePlatform (
>    mHostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
>  
>    if (mBootMode != BOOT_ON_S3_RESUME) {
> -    ReserveEmuVariableNvStore ();
> +    if (!FeaturePcdGet (PcdSmmSmramRequire)) {
> +      ReserveEmuVariableNvStore ();
> +    }
>      PeiFvInitialization ();
>      MemMapInitialization ();
>      NoexecDxeInitialization ();
> -- 
> 2.9.3
> 
>