From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web09.7108.1578411725893027144 for ; Tue, 07 Jan 2020 07:42:06 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=N4wEUpUr; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578411725; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=fD7CmAjJdJ1/htBFQZkkWVSIENdbWcBPBPh8Gi7Coo8=; b=N4wEUpUrqL93EG0eXnN3tJqQRtUwSF3TpDm4WRXPL4wHOhAYzmzk2HqoxEdhLtuOpe6YLH iJ2bhAiymf7dZJX+b7HZaTEftlv66YRmUOtq/RjW7+DgMc1OuhI3+AFBUve0klarSCJQ7+ ssa+vAVtj0H55I8HsVKgiNlvbHsbPYs= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-436-_NEg2sP-NACkbfjEWL3bsw-1; Tue, 07 Jan 2020 10:42:03 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B9B87800D4C; Tue, 7 Jan 2020 15:42:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-126.ams2.redhat.com [10.36.117.126]) by smtp.corp.redhat.com (Postfix) with ESMTP id BA824272C4; Tue, 7 Jan 2020 15:42:01 +0000 (UTC) Subject: Re: [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT To: Ard Biesheuvel , devel@edk2.groups.io References: <20200107094800.4488-1-ard.biesheuvel@linaro.org> <20200107094800.4488-3-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <211c92c6-b4fe-1667-fd26-ae282e303ae8@redhat.com> Date: Tue, 7 Jan 2020 16:42:00 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200107094800.4488-3-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: _NEg2sP-NACkbfjEWL3bsw-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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.) > > 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 > #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). > > *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