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

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

On Tue, 7 Jan 2020 at 16:42, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 01/07/20 10:47, Ard Biesheuvel wrote:
> > Introduce a boolean PCD that tells us whether TPM support is enabled
> > in the build, and if it is, record the TPM base address in the existing
> > routine that traverses the device tree in the platform PEIM.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmVirtPkg/ArmVirtPkg.dec                            |  5 ++
> >  ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf | 12 ++-
> >  ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c   | 82 +++++++++++++++++---
> >  3 files changed, 87 insertions(+), 12 deletions(-)
>
> (1) I recommend replacing "boolean PCD" in the commit message with
> "feature PCD", and updating the rest of the patch accordingly. (New
> [PcdsFeatureFlag] section in the DEC file, [FeaturePcd] section in the
> INF file, matching API calls.)
>

OK

> >
> > 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

>
> > +[Ppis]
> > +  gOvmfTpmDiscoveredPpiGuid                               ## SOMETIMES_PRODUCES
>
> Per commit 6cf1880fb5b6 ("OvmfPkg: add customized Tcg2ConfigPei clone",
> 2018-03-09), we try to make sure that gPeiTpmInitializationDonePpiGuid
> is always installed.
>
> Normally, Tcg2ConfigPei installs gEfiTpmDeviceSelectedGuid, which
> dispatches Tcg2Pei. In turn, Tcg2Pei installs
> gPeiTpmInitializationDonePpiGuid.
>
> However, if Tcg2ConfigPei finds no TPM2 device (at the known base
> address), then  gEfiTpmDeviceSelectedGuid is not installed, and so
> Tcg2Pei is not dispatched. Which would prevent the installation of
> gPeiTpmInitializationDonePpiGuid. To make sure the latter PPI gets
> installed nonetheless, in this scenario Tcg2ConfigPei installs
> gPeiTpmInitializationDonePpiGuid (sort of "on behalf of" Tcg2Pei).
>
> With another layer of depex prepended in this patch series, which may
> even prevent the dispatching of Tcg2ConfigPei, I think we might have to
> install gPeiTpmInitializationDonePpiGuid in this module -- to remain
> consistent with the goal "always install
> gPeiTpmInitializationDonePpiGuid in case TPM2 support is included in the
> binary".
>
> The above is an entirely formal (not semantic) argument on my part.
>
> For the semantic argument, I think we should look at the following hunk
> in commit 6cf1880fb5b6:
>
> +      DEBUG ((DEBUG_INFO, "%a: no TPM2 detected\n", __FUNCTION__));
> +      // If no TPM2 was detected, we still need to install
> +      // TpmInitializationDonePpi. Namely, Tcg2Pei will exit early upon
> +      // seeing the default (all-bits-zero) contents of
> +      // PcdTpmInstanceGuid, thus we have to install the PPI in its place,
> +      // in order to unblock any dependent PEIMs.
> +      Status = PeiServicesInstallPpi (&mTpmInitializationDonePpiList);
> +      ASSERT_EFI_ERROR (Status);
>
> Note that there is no actual consumer of, or dependent module on,
> gPeiTpmInitializationDonePpiGuid, in edk2. Therefore even the "semantic"
> argument boils down to "be a good citizen". Nonetheless, it matches the
> PPIs documentation in SecurityPkg.dec:
>
>   ## The PPI GUID for that TPM initialization is done. TPM initialization may be success or fail.
>   # Include/Ppi/TpmInitialized.h
>   gPeiTpmInitializationDonePpiGuid = { 0xa030d115, 0x54dd, 0x447b, { 0x90, 0x64, 0xf2, 0x6, 0x88, 0x3d, 0x7c, 0xcc }}
>
> (3) Do you agree with installing gPeiTpmInitializationDonePpiGuid in
> this lib instance in case we do *not* install gOvmfTpmDiscoveredPpiGuid,
> but PcdTpm2SupportEnabled is TRUE?
>

Yes, that makes sense.

>
> >
> >  [Guids]
> >    gEarlyPL011BaseAddressGuid
> > diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> > index 0a1469550db0..249e45c04624 100644
> > --- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> > +++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >  *
> >  *  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
> > -*  Copyright (c) 2014, Linaro Limited. All rights reserved.
> > +*  Copyright (c) 2014-2020, Linaro Limited. All rights reserved.
> >  *
> >  *  SPDX-License-Identifier: BSD-2-Clause-Patent
> >  *
> > @@ -13,11 +13,18 @@
> >  #include <Library/DebugLib.h>
> >  #include <Library/HobLib.h>
> >  #include <Library/PcdLib.h>
> > +#include <Library/PeiServicesLib.h>
> >  #include <libfdt.h>
> >
> >  #include <Guid/EarlyPL011BaseAddress.h>
> >  #include <Guid/FdtHob.h>
> >
> > +STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpm2DiscoveredPpi = {
> > +  EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> > +  &gOvmfTpmDiscoveredPpiGuid,
> > +  NULL
> > +};
> > +
> >  EFI_STATUS
> >  EFIAPI
> >  PlatformPeim (
> > @@ -31,13 +38,18 @@ PlatformPeim (
> >    UINT64             *FdtHobData;
> >    UINT64             *UartHobData;
> >    INT32              Node, Prev;
> > +  INT32              Parent, Depth;
> >    CONST CHAR8        *Compatible;
> >    CONST CHAR8        *CompItem;
> >    CONST CHAR8        *NodeStatus;
> >    INT32              Len;
> > +  INT32              RangesLen;
> >    INT32              StatusLen;
> >    CONST UINT64       *RegProp;
> > +  CONST UINT32       *RangesProp;
> >    UINT64             UartBase;
> > +  UINT64             TpmBase;
> > +  EFI_STATUS         Status;
> >
> >
> >    Base = (VOID*)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
> > @@ -58,18 +70,16 @@ PlatformPeim (
> >    ASSERT (UartHobData != NULL);
> >    *UartHobData = 0;
> >
> > -  //
> > -  // Look for a UART node
> > -  //
> > -  for (Prev = 0;; Prev = Node) {
> > -    Node = fdt_next_node (Base, Prev, NULL);
> > +  for (Prev = Depth = 0;; Prev = Node) {
> > +    Node = fdt_next_node (Base, Prev, &Depth);
> >      if (Node < 0) {
> >        break;
> >      }
> >
> > -    //
> > -    // Check for UART node
> > -    //
> > +    if (Depth == 1) {
> > +      Parent = Node;
> > +    }
> > +
> >      Compatible = fdt_getprop (Base, Node, "compatible", &Len);
> >
> >      //
> > @@ -89,10 +99,62 @@ PlatformPeim (
> >
> >          UartBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
> >
> > -        DEBUG ((EFI_D_INFO, "%a: PL011 UART @ 0x%lx\n", __FUNCTION__, UartBase));
> > +        DEBUG ((DEBUG_INFO, "%a: PL011 UART @ 0x%lx\n", __FUNCTION__, UartBase));
>
> (4) If we're not touching this line otherwise, then please drop this
> change (or isolate it to another patch).
>

OK


> >
> >          *UartHobData = UartBase;
> >          break;
> > +      } else if (FixedPcdGetBool (PcdTpm2SupportEnabled) &&
> > +                 AsciiStrCmp (CompItem, "tcg,tpm-tis-mmio") == 0) {
> > +
> > +        RegProp = fdt_getprop (Base, Node, "reg", &Len);
> > +        ASSERT (Len == 8 || Len == 16);
> > +        if (Len == 8) {
> > +          TpmBase = fdt32_to_cpu (RegProp[0]);
> > +        } else if (Len == 16) {
> > +          TpmBase = fdt64_to_cpu (ReadUnaligned64 ((UINT64 *)RegProp));
> > +        }
> > +
> > +        if (Depth > 1) {
> > +          //
> > +          // QEMU/mach-virt may put the TPM on the platform bus, in which case
> > +          // we have to take its 'ranges' property into account to translate the
> > +          // MMIO address. This consists of a <child base, parent base, size>
> > +          // tuple, where the child base and the size use the same number of
> > +          // cells as the 'reg' property above, and the parent base uses 2 cells
> > +          //
> > +          RangesProp = fdt_getprop (Base, Parent, "ranges", &RangesLen);
> > +          ASSERT (RangesProp != NULL);
> > +
> > +          // a plain 'ranges' attribute without a value implies a 1:1 mapping
>
> (5) please use the edk2 coding style for comments (empty "//" lines
> before and after)
>
>
> > +          if (RangesLen != 0) {
> > +            // assume a single translated range with 2 cells for the parent base
>
> (6) same as (5)
>
> > +            if (RangesLen != Len + 2 * sizeof (UINT32)) {
> > +              DEBUG ((DEBUG_WARN,
> > +                "%a: 'ranges' property has unexpected size %d\n",
> > +                __FUNCTION__, RangesLen));
> > +              break;
> > +            }
> > +
> > +            if (Len == 8) {
> > +              TpmBase -= fdt32_to_cpu (RangesProp[0]);
> > +            } else {
> > +              TpmBase -= fdt64_to_cpu (ReadUnaligned64 ((UINT64 *)RangesProp));
> > +            }
> > +
> > +            // advance RangesProp to the parent bus address
>
> (7) same as (5)
>
> > +            RangesProp = (UINT32 *)((UINT8 *)RangesProp + Len / 2);
> > +            TpmBase += fdt64_to_cpu (ReadUnaligned64 ((UINT64 *)RangesProp));
> > +          }
> > +        }
> > +
> > +        DEBUG ((DEBUG_INFO, "%a: TPM @ 0x%lx\n", __FUNCTION__, TpmBase));
> > +
> > +        Status = PcdSet64S (PcdTpmBaseAddress, TpmBase);
> > +        ASSERT_RETURN_ERROR (Status);
> > +
> > +        Status = PeiServicesInstallPpi (&mTpm2DiscoveredPpi);
> > +        ASSERT_EFI_ERROR (Status);
> > +        break;
> >        }
> >      }
> >    }
> >
>
> So I was quite unhappy about adding this bunch of FDT parsing to this
> lib instance. Because, the original reason for this library is (from
> commit 433b31ddeeeb): "it allows us to preserve the device tree blob if
> it was passed to us in system DRAM."
>
> However:
>
> (a) I can also see commit 337d450e2014 ("ArmVirtualizationPkg: move
> early UART discovery to PlatformPeim", 2015-02-28), where we said "this
> is a more suitable place anyway", for parsing the FDT for UART details.
>
> (b) In ArmVirtQemu's PEI phase (unlike in the DXE phase), we have no
> centralized FDT parsing facility. That means we parse the FDT whenever
> and wherever we need something from it. Examples:
>
> - "Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c" -->
> causes basically all SEC, PEI_CORE, and PEIM modules to re-fetch the
> UART details from the FDT. (The HOB that is produced in the above-quoted
> context is only consumed in DXE_CORE and later modules)
>
> - "Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c" -->
> causes PcdSystemMemorySize to be set internally to
> "ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf", parsed from the FDT's
> lowest memory node.
>
> (8) Can you please summarize this briefly in the commit message? (I.e.
> that it's OK to parse the FDT in the PEI phase wherever we need it,
> whatever we need it for, because we have no centralized parsing service
> or data collection facility for that, in PEI).
>
>
> Peeking ahead, I can see that in the next patch, we dump yet more
> functionality in this lib instance; I feel that *there* I'm going to
> recommend against doing that. But I'll need to look at that patch in
> more depth first.
>
> Thanks!
> Laszlo
>