From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.8235.1578575091071485015 for ; Thu, 09 Jan 2020 05:04:51 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=K9gYbOV8; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578575090; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wALCUaCz7EEMqUDj1WuY9yYz2UNM3tYAy6KzSgpQ9Dg=; b=K9gYbOV8pKJKk+ec8g0ihNyd8qyu6qnhdn66UxT4+BR+MhXBGDvrJ6alVFr9cZWgFstc5d IN8K7W+ehoEcoKlxaKCESHc0T/QwQg9J4h/dqzfyIKniqy6MoUtSGKxq/Y6Ibx60iRmhx/ mVTroqBfXbpbfekCl9O7/tduiVrr2oE= 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-404-vU15CHEuNfW6v86iTqwBjA-1; Thu, 09 Jan 2020 08:04:46 -0500 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 81715800D54; Thu, 9 Jan 2020 13:04:45 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.145]) by smtp.corp.redhat.com (Postfix) with ESMTP id BE5165C541; Thu, 9 Jan 2020 13:04:44 +0000 (UTC) Subject: Re: [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT To: Ard Biesheuvel Cc: edk2-devel-groups-io References: <20200107094800.4488-1-ard.biesheuvel@linaro.org> <20200107094800.4488-3-ard.biesheuvel@linaro.org> <211c92c6-b4fe-1667-fd26-ae282e303ae8@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 9 Jan 2020 14:04:43 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-MC-Unique: vU15CHEuNfW6v86iTqwBjA-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 01/08/20 15:41, Ard Biesheuvel wrote: > On Tue, 7 Jan 2020 at 16:42, Laszlo Ersek wrote: >> On 01/07/20 10:47, Ard Biesheuvel wrote: >>> >>> 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 My apologies, I may have been unclear -- I meant listing those lib classes unconditionally, i.e. regardless of "gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled". The idea is, if a lib class header is included by any source file in the module, then the module's INF file too should list the lib class. For example, PcdLib is already pulled in, indirectly (I checked it in the build report file -- even the gEfiPeiPcdPpiGuid DEPEX is already there), we're just not listing it. We have a direct dependency on it (even without your present patch -- again, originating from the lib class header which is already #included), so we should list it explicitly in [LibraryClasses]. That's my whole point, nothing more. Sorry about overcomplicating my previous email. Thanks! Laszlo