Re: [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid

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

On 12 April 2018 at 11:05, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/12/18 08:28, Ard Biesheuvel wrote:
>> On 12 April 2018 at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
>>> PlatformHasAcpiDtDxe consumes the DynamicHii PCD called
>>> "gArmVirtTokenSpaceGuid.PcdForceNoAcpi". The PcdGetBool() library call
>>> terminates in gRT->GetVariable(), in the MdeModulePkg/Universal/PCD/Dxe
>>> driver. Put "gEfiVariableArchProtocolGuid" on PlatformHasAcpiDtDxe's DEPEX
>>> so that we not attempt the call before the PCD driver can successfully
>>> read non-volatile variables.
>>>
>>
>> OK, so does this mean that UefiRuntimeServicesTableLib deliberately
>> does not depex on gEfiVariableArchProtocolGuid,
>
> Yes, I do think so; the runtime services are expected to become
> available gradually. For example, "MdePkg/Include/Protocol/Variable.h"
> has a great long comment on this.
>
>> and it will return
>> EFI_NOT_AVAILABLE_YET when called too early? If that is the case, I
>> would assume it is the responsibility of the PcdLib implementation to
>> deal with that, not the PcdLib clients.
>
> Yes, this idea occurred to me as well, but I think it is not correct.
> The PcdLib instance for the DXE phase is responsible for talking to the
> PCD protocol, and indeed it has a depex on that protocol
> (MdePkg/Library/DxePcdLib/DxePcdLib.inf). However, if a module uses
> dynamic PCDs, but none of those dynamic PCDs are HII (i.e., backed by
> non-volatile UEFI variables), then IMO neither the PcdLib instance, nor
> the PCD DXE driver, should depend on the variable read service
> (gEfiVariableArchProtocolGuid).
>
> (Well the PCD DXE driver is shared / used by all of the DXE modules, so
> it couldn't depend on gEfiVariableArchProtocolGuid in any case at all.)
>
> Ultimately, in the optimal case, the PcdLib instance for DXE should
> "know" if any PCDs used by the main driver module are DynamicHII, and in
> that case, the PcdLib instance should create a dependency on the
> variable read (and possibly write) services. I think edk2 simply doesn't
> support this use case at this moment (it is ultimately a DSC decision
> what type we give to a PCD, choosing from the possibilities listed in
> the DEC), so we have to take care of it ourselves. I do see that this
> limits the reusability of individual drivers across different DSC files
> -- if another DSC makes the same PCD fixed, for example, then the
> gEfiVariableArchProtocolGuid dependency becomes useless / needlessly
> restrictive in the driver.
>
> We could factor out just the gEfiVariableArchProtocolGuid depex into
> another NULL class lib, and hook that into PlatformHasAcpiDtDxe in the
> DSC file, because in the DSC we do know that the PCD is DynamicHII. If
> you feel strongly about it, it's doable, but I certainly think it's
> overkill. PlatformHasAcpiDtDxe got "Platform" in its name; it's pretty
> unlikely to be reused in any other DSC. (Even ArmVirtXen has a different
> implementation, which does not use the PCD.) I feel the proposed patch
> should be fine -- what's your take?
>

I missed the bit where gRT->GetVariable() is only called for HII style
dynamic PCDs, so I agree that it makes sense to deal with things as
you are doing here.