On Tue, 7 Jan 2020 at 16:42, Laszlo Ersek <ler...@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.biesheu...@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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53023): https://edk2.groups.io/g/devel/message/53023
Mute This Topic: https://groups.io/mt/69499021/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
