Re: [PATCH v2 3/8] OvmfPkg: conditionally disable reserved memory varstore emulation at build

[Laszlo Ersek <lersek@redhat.com>]

On 12/01/17 11:53, Ard Biesheuvel wrote:
> On 1 December 2017 at 10:52, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 12/01/17 09:51, Ard Biesheuvel wrote:
>>> On 30 November 2017 at 16:30, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> (All of the below is only relevant for SMM_REQUIRE=FALSE.)
>>>>
>>>> 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.
>>>>
>>>> In addition, QemuFlashFvbServicesRuntimeDxe is always launched before
>>>> EmuVariableFvbRuntimeDxe, so that if flash variables are available,
>>>> QemuFlashFvbServicesRuntimeDxe can set PcdFlashNvStorageVariableBase64
>>>> first, and EmuVariableFvbRuntimeDxe can exit early. This ordering is
>>>> currently enforced by adding QemuFlashFvbServicesRuntimeDxe to the APRIORI
>>>> DXE file.
>>>>
>>>> All of this is unnecessary when MEM_VARSTORE_EMU_ENABLE is set to FALSE.
>>>> In such a build,
>>>>
>>>> - (almost) remove the dynamic default for PcdEmuVariableNvStoreReserved
>>>>   (we can't really do this because the PcdSet64() in
>>>>   ReserveEmuVariableNvStore() wouldn't compile),
>>>>
>>>
>>> If that is the only concern, and the value is irrelevant, you could
>>> make it a patchable PCD instead
>>
>> This sounds interesting; I've never used patchable PCDs. Can you please
>> elaborate?
>>
>> Do you mean that for (SMM_REQUIRE==TRUE ||
>> MEM_VARSTORE_EMU_ENABLE==FALSE), the DSC file should list
>> PcdEmuVariableNvStoreReserved somewhere else?
>>
> 
> Yes.
> 
>> Does PcdSet work on patchable PCDs?
>>
> 
> Yes. It's a bit icky because the value does not actually propagate to
> other modules, but that doesn't matter in this case.

Hm, this sounds like a recipe. We have two sets of affected PCDs:

- PcdEmuVariableNvStoreReserved --> in the "flash required" builds, make
it patchable, don't bother setting a good value at build time, and the
PcdSet calls will compile (but won't ever be reached due to code logic)

- PcdFlashNvStorageVariableBase64, PcdFlashNvStorageFtwWorkingBase,
PcdFlashNvStorageFtwSpareBase: in the "flash required" builds, make them
patchable, *try* to set the right default values at build time, with SET
commands, and the PcdSet calls will compile (but won't ever be reached
due to code logic)

That could let us drop FlashNvStorageAddressLib. (On patch #6, only the
commit message would change.)

Thanks,
Laszlo