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

[Laszlo Ersek <lersek@redhat.com>]

On 05/15/17 20:09, Jordan Justen wrote:
> 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>

Series committed as 11a6cc5bda81..639c7dd86d1d.

Thank you,
Laszlo

> 
>>    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
>>
>>