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

["Gao, Liming" <liming.gao@intel.com>]

Laszlo:
  We also have the similar idea that auto appends Variable read or Variable write protocol. Build tool can know which PCD is configured as DynamicHii. But, it doesn't know whether this PCD is consumed or produced in the driver entry point. If PCD is used in driver entry point, Build tool needs to append Variable arch protocol depex. Otherwise, BaseTool doesn't need do it. Further, I think BaseTools can use PCD usage described in Pcd section. CONSUMES means PCD read in the driver entry point, PRODUCES means PCD write in the driver entry point. Seemly, this solution can work well. But, it depends on the correct PCD usage in module INF. If PCD usage is not described or wrongly described, it may cause system boot failure. Consider its impact, we have not added this support. 

[Pcd]
  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate        ## CONSUMES
  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits         ## SOMETIMES_CONSUMES
  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity           ## PRODUCES
  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits         ## SOMETIMES_PRODUCES

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Thursday, April 12, 2018 5:06 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>
> Subject: Re: [edk2] [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid
> 
> 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
> >>
> >>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel