From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by mx.groups.io with SMTP id smtpd.web09.8660.1578494486057877509 for ; Wed, 08 Jan 2020 06:41:26 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=cco/bq0y; spf=pass (domain: linaro.org, ip: 209.85.221.66, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f66.google.com with SMTP id q10so3600650wrm.11 for ; Wed, 08 Jan 2020 06:41:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ilfnNXnFszFqh9mQEcoPgMLBV5y7Nv408Gk6CYiHUUE=; b=cco/bq0yfjIZtAQzZ6PWnP234pZhfHsxpyr3uRi/K2EKYLbAFe1IqMN0fheYt/4E8e YzzWkVo9YtrJaGn/UVN7GUmf9eUcdOX4jVTkgaTR51BShIgrNoC7BVOMCwQH9fYKC9Zm uNEOluy/QFa8P3cFiQPgbPUPMkkm/y8Hct+Q/SO6dgPiFTWeRuAgKEqe71Og+ilQNm+Y Kgmkszys0PeBaC+kXBiB6UOLDDXjCiye4YH+3V2Xa+pjQ4TQg8uwxPmCfgvBqN0f5XNg gquOxKFLt7JI8mNzl+WMS017DMDd5CaE+V6hL6FSdMQvzLP6EKfTeJ457MN8BHK0leSF XmMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ilfnNXnFszFqh9mQEcoPgMLBV5y7Nv408Gk6CYiHUUE=; b=N6luQ64r3wery+ElEqnqmZAK97dqilmi4sP1L333whVXONSkYeacCy4YC3r6DdMe5L yZLYzDEituzPtpBbpZw1W+DvFG2EMwbmZCed6duJTWwCpGy07Ze+0HLFTi9l1sOiJibF hQr3EMKKhAt6ai1Eo6b9YO7XctUBECsVKk5DfYDO8//MG5qgMFRM+xdDa7SYerXcHj5d jlpzm97iPMnt8c1bY79przdssgvpSfGeXsJtuWcnNmr69blJGPVKUmTqDoTlheby+KYU 3euqDiah0M2SefngveElf3RzVKyEp6Me8W8eKzH3+1KEVi+xfOz3ZHVPM15fK4wQbIAA QA8Q== X-Gm-Message-State: APjAAAW7IEonYRB75I+yxX6hEfIgnDMWm4IzIfxLyMyYQ28TsTu2W+QU rRxR6TFNszMEWJSIGhx0FOiXZNX56adTyrkvJjFcCPE2+7c= X-Google-Smtp-Source: APXvYqwzE+QHNQFu169ROgUSSw+1t20AlDd4txc8KLFcTsd08iXOSHcUaFpwhPTJFZdxagTJyJcz6SLXPCcaE4Ysh+Q= X-Received: by 2002:a5d:43c7:: with SMTP id v7mr4727313wrr.32.1578494484264; Wed, 08 Jan 2020 06:41:24 -0800 (PST) MIME-Version: 1.0 References: <20200107094800.4488-1-ard.biesheuvel@linaro.org> <20200107094800.4488-3-ard.biesheuvel@linaro.org> <211c92c6-b4fe-1667-fd26-ae282e303ae8@redhat.com> In-Reply-To: <211c92c6-b4fe-1667-fd26-ae282e303ae8@redhat.com> From: "Ard Biesheuvel" Date: Wed, 8 Jan 2020 15:41:13 +0100 Message-ID: Subject: Re: [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT To: Laszlo Ersek Cc: edk2-devel-groups-io Content-Type: text/plain; charset="UTF-8" On Tue, 7 Jan 2020 at 16:42, Laszlo Ersek 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 > > --- > > 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 > > #include > > #include > > +#include > > #include > > > > #include > > #include > > > > +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 > > + // 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 >