Re: [PATCH v2 4/8] OvmfPkg: introduce FlashNvStorageAddressLib

[Laszlo Ersek <lersek@redhat.com>]

On 12/01/17 09:55, Ard Biesheuvel wrote:
> On 30 November 2017 at 16:30, Laszlo Ersek <lersek@redhat.com> wrote:
>> An OVMF binary built with
>>
>>   -D SMM_REQUIRE
>>
>> or with
>>
>>   -D MEM_VARSTORE_EMU_ENABLE=FALSE
>>
>> requires flash to be present at the expected (build-time determined)
>> location; falling back to the emulated varstore is not possible.
>>
>> In such builds, we can replace the settings of the
>> - varstore,
>> - FTW working block,
>> - and FTW spare area
>> address PCDs in QemuFlashFvbServicesRuntimeDxe with identical settings in
>> a new plug-in (NULL class) library, to be linked into variable-related
>> PEIMs.
>>
>> This will enable such builds to access variables during the PEI phase,
>> without dynamic flash detection and without ordering constraints against
>> other PEIMs.
>>
> 
> Why can't we set these at build time in this case?

Hmm, let me think... Oh yes, I think I remember.

The PCDs that are used as *source* here (i.e., passed as arguments to
PcdGet32()) *are* set at build time. However, they take some calculation
at build time too, and they are set in the "OvmfPkg/OvmfPkg.fdf.inc"
file, with explicit SET statments.

This is because you can build OVMF for a 1MB, 2MB, or 4MB unified flash
image size, and the "source PCDs" in question depend on them.
Furthermore, these "source PCDs" are fixed-at-build.

I think I couldn't find a way to set the defaults for the "target",
dynamic, PCDs, from the build-time-calculated "source PCDs". The usual
dynamic-default setting in the DSC does not work (because the default
value is not a numeric constant), and, as far as I remember, when I
tried the SET command on the dynamic ("target") PCDs as well, it simply
didn't work.

The SET statements *could* work if the target PCDs could be made
(conditionally) fixed-at-build as well, but then we run into the same
build problem of ReserveEmuVariableNvStore() not compiling.

In short:

- the "target" PCDs have to remain dynamic, or else
ReserveEmuVariableNvStore() won't compile,

- as long as the "target" PCDs are dynamic, I cannot set their defaults
at build time *from* the "source" fixed-at-build PCDs, which are
calculated at each build separately, with the SET statement.

Thanks
Laszlo


> 
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien.grall@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf | 48 ++++++++++++++++++
>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   | 53 ++++++++++++++++++++
>>  2 files changed, 101 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf b/OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
>> new file mode 100644
>> index 000000000000..f79194f80de9
>> --- /dev/null
>> +++ b/OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
>> @@ -0,0 +1,48 @@
>> +## @file
>> +#
>> +# A hook-in library for variable-related PEIMs, in order to set
>> +# - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64,
>> +# - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase,
>> +# - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase,
>> +# from their gUefiOvmfPkgTokenSpaceGuid counterparts, just before those PEIMs
>> +# consume them.
>> +#
>> +# Copyright (C) 2017, Red Hat, Inc.
>> +#
>> +# This program and the accompanying materials are licensed and made available
>> +# under the terms and conditions of the BSD License which accompanies this
>> +# distribution. The full text of the license may be found at
>> +# http://opensource.org/licenses/bsd-license.php
>> +#
>> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
>> +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 1.25
>> +  BASE_NAME                      = FlashNvStorageAddressLib
>> +  FILE_GUID                      = 5FF5A9F9-D01E-49EC-9A17-1682FC85122F
>> +  MODULE_TYPE                    = BASE
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = FlashNvStorageAddressLib|PEIM
>> +  CONSTRUCTOR                    = SetFlashNvStorageAddresses
>> +
>> +[Sources]
>> +  FlashNvStorageAddressLib.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>> +  OvmfPkg/OvmfPkg.dec
>> +
>> +[LibraryClasses]
>> +  PcdLib
>> +
>> +[Pcd]
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
>> diff --git a/OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c b/OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
>> new file mode 100644
>> index 000000000000..dc1280cc23f1
>> --- /dev/null
>> +++ b/OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
>> @@ -0,0 +1,53 @@
>> +/** @file
>> +
>> +  A hook-in library for variable-related PEIMs, in order to set
>> +  - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64,
>> +  - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase,
>> +  - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase,
>> +  from their gUefiOvmfPkgTokenSpaceGuid counterparts, just before those PEIMs
>> +  consume them.
>> +
>> +  Copyright (C) 2017, Red Hat, Inc.
>> +
>> +  This program and the accompanying materials are licensed and made available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution. The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
>> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include <Library/PcdLib.h>
>> +
>> +RETURN_STATUS
>> +EFIAPI
>> +SetFlashNvStorageAddresses (
>> +  VOID
>> +  )
>> +{
>> +  RETURN_STATUS PcdStatus;
>> +
>> +  PcdStatus = PcdSet64S (
>> +                PcdFlashNvStorageVariableBase64,
>> +                PcdGet32 (PcdOvmfFlashNvStorageVariableBase)
>> +                );
>> +  if (RETURN_ERROR (PcdStatus)) {
>> +    return PcdStatus;
>> +  }
>> +
>> +  PcdStatus = PcdSet32S (
>> +                PcdFlashNvStorageFtwWorkingBase,
>> +                PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase)
>> +                );
>> +  if (RETURN_ERROR (PcdStatus)) {
>> +    return PcdStatus;
>> +  }
>> +
>> +  PcdStatus = PcdSet32S (
>> +                PcdFlashNvStorageFtwSpareBase,
>> +                PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase)
>> +                );
>> +  return PcdStatus;
>> +}
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
>>