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

[Laszlo Ersek <lersek@redhat.com>]

On 12/01/17 12:03, Laszlo Ersek wrote:
> 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,

correction -- ReserveEmuVariableNvStore() is totally irrelevant here;
what I meant here were the PcdSet calls for these "target" PCDs that
wouldn't compile in QemuFlashFvbServicesRuntimeDxe. (See patch 6.)

Thanks,
Laszlo

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