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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 1 December 2017 at 11:41, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.)
>

Yes, something like that. I strongly prefer a NULL library class
resolution over A PRIORI, but having neither is even better.