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

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

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

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


> +[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?


>
>  [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).

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