On 12 April 2018 at 11:05, Laszlo Ersek <ler...@redhat.com> wrote: > On 04/12/18 08:28, Ard Biesheuvel wrote: >> On 12 April 2018 at 02:55, Laszlo Ersek <ler...@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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
