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

[Laszlo Ersek <lersek@redhat.com>]

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?

Thanks!
Laszlo

> 
> 
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Cc: Steve Capper <steve.capper@linaro.org>
>> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>> index 5351e741aa41..8346f7f24b32 100644
>> --- a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>> +++ b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>> @@ -44,7 +44,7 @@ [Guids]
>>    gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL
>>
>>  [Pcd]
>>    gArmVirtTokenSpaceGuid.PcdForceNoAcpi
>>
>>  [Depex]
>> -  TRUE
>> +  gEfiVariableArchProtocolGuid
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
>>