Re: [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT

["Laszlo Ersek" <lersek@redhat.com>]

On 01/08/20 15:41, Ard Biesheuvel wrote:
> On Tue, 7 Jan 2020 at 16:42, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 01/07/20 10:47, Ard Biesheuvel wrote:

>>>
>>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>>> index a019cc269d10..ed5114887489 100644
>>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>>> @@ -58,6 +58,11 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>>    #
>>>    gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
>>>
>>> +  #
>>> +  # Boolean PCD that defines whether TPM2 support is enabled
>>> +  #
>>> +  gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|FALSE|BOOLEAN|0x00000004
>>> +
>>>  [PcdsDynamic]
>>>    #
>>>    # Whether to force disable ACPI, regardless of the fw_cfg settings
>>> diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
>>> index 46db117ac28e..c41ee22c9767 100644
>>> --- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
>>> +++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
>>> @@ -21,22 +21,30 @@ [Sources]
>>>  [Packages]
>>>    ArmPkg/ArmPkg.dec
>>>    ArmVirtPkg/ArmVirtPkg.dec
>>> -  MdePkg/MdePkg.dec
>>> -  MdeModulePkg/MdeModulePkg.dec
>>>    EmbeddedPkg/EmbeddedPkg.dec
>>> +  MdeModulePkg/MdeModulePkg.dec
>>> +  MdePkg/MdePkg.dec
>>> +  OvmfPkg/OvmfPkg.dec
>>> +  SecurityPkg/SecurityPkg.dec
>>>
>>>  [LibraryClasses]
>>>    DebugLib
>>>    HobLib
>>>    FdtLib
>>> +  PeiServicesLib
>>>
>>>  [FixedPcd]
>>>    gArmTokenSpaceGuid.PcdFvSize
>>>    gArmVirtTokenSpaceGuid.PcdDeviceTreeAllocationPadding
>>> +  gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled
>>>
>>>  [Pcd]
>>>    gArmTokenSpaceGuid.PcdFvBaseAddress
>>>    gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
>>> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress         ## SOMETIMES_PRODUCES
>>> +
>>
>> This is a dynamic PCD -- we're going to set it below --, and this lib
>> instance does not use dynamic PCDs at the moment (I checked the build
>> report file, and all PCDs in
>> "ArmPlatformPkg/PlatformPei/PlatformPeim.inf", when built for
>> ArmVirtQemu, are FIXED, or FLAG (i.e. Feature PCD)).
>>
>> This gave me pause for a good while. In particular, in commit
>> cc667df08ae8 ("ArmVirtualizationPkg: use a HOB to store device tree
>> blob", 2015-02-28), we replaced a PcdSet64() in this lib instance with a
>> HOB production:
>>
>>     Instead of using a dynamic PCD, store the device tree address in a HOB
>>     so that we can also run under a configuration that does not support
>>     dynamic PCDs.
>>
>> So, this change seemed to conflict with that, at first look.
>>
>> Then I noticed the new PcdSet64() is protected with the new feature flag
>> (PcdTpm2SupportEnabled), which we only set to TRUE in "ArmVirtQemu.dsc".
>>
>> So things seem safe for ArmVirtQemuKernel and ArmVirtXen (the dynamic
>> PCD setting will never be reached). But can we guarantee the PCD PPI
>> exists by the time we reach the PcdSet64() in ArmVirtQemu?
>>
>> Apparently: yes. This lib instance depends on PcdLib, and in the
>> ArmVirtQemu build, "ArmPlatformPkg/PlatformPei/PlatformPeim.inf"
>> receives a PcdLib resolution to
>> "MdePkg/Library/PeiPcdLib/PeiPcdLib.inf". From which the module inherits
>> a DEPEX on "gEfiPeiPcdPpiGuid". Therefore, the PcdSet64() call is safe
>> here -- but it's hard to see why.
>>
>> (2) Can you please add a separate patch for this lib instance, for
>> spelling out PcdLib under [LibraryClasses] in the INF file? Please
>> mention in the commit message that we inherit a dependency on
>> gEfiPeiPcdPpiGuid.
>>
> 
> I tried adding this to [LibraryClasses] but apparently, this syntax is
> not supported yet :-(
> 
> 
>   PcdLib          |gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled
>   PeiServicesLib  |gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled

My apologies, I may have been unclear -- I meant listing those lib
classes unconditionally, i.e. regardless of
"gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled". The idea is, if a lib
class header is included by any source file in the module, then the
module's INF file too should list the lib class.

For example, PcdLib is already pulled in, indirectly (I checked it in
the build report file -- even the gEfiPeiPcdPpiGuid DEPEX is already
there), we're just not listing it. We have a direct dependency on it
(even without your present patch -- again, originating from the lib
class header which is already #included), so we should list it
explicitly in [LibraryClasses].

That's my whole point, nothing more. Sorry about overcomplicating my
previous email.

Thanks!
Laszlo