* [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu
@ 2020-01-07 9:47 Ard Biesheuvel
2020-01-07 9:47 ` [PATCH 1/4] OvmfPkg/Tcg2ConfigPei: introduce a signalling PPI to depex on Ard Biesheuvel
` (4 more replies)
0 siblings, 5 replies; 23+ messages in thread
From: Ard Biesheuvel @ 2020-01-07 9:47 UTC (permalink / raw)
To: devel; +Cc: lersek, Ard Biesheuvel
Wire up the various existing pieces so that we can implemented measured
boot on ArmVirtQemu based on the TPM support in QEMU, just like it has
been implemented for x86 in OvmfPkg.
The main difference is that on ARM, we first need to discover the TPM base
address from the device tree provided by QEMU, as well as the PSCI method
used to perform a cold reset, so this is added to the existing implementation
of PlatformPeiLib.
The associated QEMU changes are under development in Linaro, and will be
sent out for review to the appropriate mailing list shortly.
Ard Biesheuvel (4):
OvmfPkg/Tcg2ConfigPei: introduce a signalling PPI to depex on
ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT
ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI
ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot
ArmVirtPkg/ArmVirtPkg.dec | 5 +
OvmfPkg/OvmfPkg.dec | 4 +
ArmVirtPkg/ArmVirtQemu.dsc | 71 +++++++
ArmVirtPkg/ArmVirtQemu.fdf | 5 +
ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf | 15 +-
OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 6 +-
ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c | 205 +++++++++++++++++++-
ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 10 +
8 files changed, 308 insertions(+), 13 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/4] OvmfPkg/Tcg2ConfigPei: introduce a signalling PPI to depex on
2020-01-07 9:47 [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu Ard Biesheuvel
@ 2020-01-07 9:47 ` Ard Biesheuvel
2020-01-07 11:58 ` Laszlo Ersek
2020-01-07 9:47 ` [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT Ard Biesheuvel
` (3 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-01-07 9:47 UTC (permalink / raw)
To: devel; +Cc: lersek, Ard Biesheuvel
On ARM systems, the TPM does not live at a fixed address, and so we
need the platform to discover it first. So introduce a PPI that signals
that the TPM address has been discovered and recorded in the appropriate
PCD, and make Tcg2ConfigPei depex on it when built for ARM or AARCH64.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
OvmfPkg/OvmfPkg.dec | 4 ++++
OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 6 +++++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index d5fee805ef4a..10a2b714c1b4 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -101,6 +101,10 @@ [Protocols]
gEfiLegacyInterruptProtocolGuid = {0x31ce593d, 0x108a, 0x485d, {0xad, 0xb2, 0x78, 0xf2, 0x1f, 0x29, 0x66, 0xbe}}
gEfiVgaMiniPortProtocolGuid = {0xc7735a2f, 0x88f5, 0x4882, {0xae, 0x63, 0xfa, 0xac, 0x8c, 0x8b, 0x86, 0xb3}}
+[Ppis]
+ # PPI whose presence in the PPI database signals that the TPM base address has been discovered and recorded
+ gOvmfTpmDiscoveredPpiGuid = {0xb9a61ad0, 0x2802, 0x41f3, {0xb5, 0x13, 0x96, 0x51, 0xce, 0x6b, 0xd5, 0x75}}
+
[PcdsFixedAtBuild]
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|0x0|UINT32|0
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize|0x0|UINT32|1
diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
index e34cd6210611..55684ba045b3 100644
--- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -25,6 +25,7 @@ [Sources]
[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
+ OvmfPkg/OvmfPkg.dec
SecurityPkg/SecurityPkg.dec
[LibraryClasses]
@@ -43,5 +44,8 @@ [Ppis]
[Pcd]
gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES
-[Depex]
+[Depex.IA32, Depex.X64]
TRUE
+
+[Depex.ARM, Depex.AARCH64]
+ gOvmfTpmDiscoveredPpiGuid
--
2.20.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT
2020-01-07 9:47 [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu Ard Biesheuvel
2020-01-07 9:47 ` [PATCH 1/4] OvmfPkg/Tcg2ConfigPei: introduce a signalling PPI to depex on Ard Biesheuvel
@ 2020-01-07 9:47 ` Ard Biesheuvel
2020-01-07 15:42 ` Laszlo Ersek
2020-01-07 9:47 ` [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI Ard Biesheuvel
` (2 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-01-07 9:47 UTC (permalink / raw)
To: devel; +Cc: lersek, Ard Biesheuvel
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(-)
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
+
+[Ppis]
+ gOvmfTpmDiscoveredPpiGuid ## SOMETIMES_PRODUCES
[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));
*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
+ if (RangesLen != 0) {
+ // assume a single translated range with 2 cells for the parent base
+ 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
+ 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;
}
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI
2020-01-07 9:47 [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu Ard Biesheuvel
2020-01-07 9:47 ` [PATCH 1/4] OvmfPkg/Tcg2ConfigPei: introduce a signalling PPI to depex on Ard Biesheuvel
2020-01-07 9:47 ` [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT Ard Biesheuvel
@ 2020-01-07 9:47 ` Ard Biesheuvel
2020-01-07 16:50 ` Laszlo Ersek
2020-01-07 9:48 ` [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot Ard Biesheuvel
2020-01-07 11:55 ` [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu Laszlo Ersek
4 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-01-07 9:47 UTC (permalink / raw)
To: devel; +Cc: lersek, Ard Biesheuvel
Extend the existing DT traversal routine in PlatformPeiLib with
discovery of the PSCI method, and expose an implementation of the
Reset2 PPI based on the method found.
This satisfies a dependency of Tcg2Pei, which needs to reset the
platform in some cases. Since there are no other uses for system
reset in PEI on ArmVirtQemu, simply expose the PPI directly rather
than using the generic ResetSystemPei and the associated plumbing.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf | 3 +
ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c | 123 ++++++++++++++++++++
2 files changed, 126 insertions(+)
diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
index c41ee22c9767..72ed2413a768 100644
--- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
+++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
@@ -28,6 +28,8 @@ [Packages]
SecurityPkg/SecurityPkg.dec
[LibraryClasses]
+ ArmSmcLib
+ ArmHvcLib
DebugLib
HobLib
FdtLib
@@ -44,6 +46,7 @@ [Pcd]
gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_PRODUCES
[Ppis]
+ gEfiPeiReset2PpiGuid ## SOMETIMES_PRODUCES
gOvmfTpmDiscoveredPpiGuid ## SOMETIMES_PRODUCES
[Guids]
diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
index 249e45c04624..7af351eda003 100644
--- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
+++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
@@ -9,6 +9,8 @@
#include <PiPei.h>
+#include <Library/ArmHvcLib.h>
+#include <Library/ArmSmcLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/DebugLib.h>
#include <Library/HobLib.h>
@@ -16,15 +18,113 @@
#include <Library/PeiServicesLib.h>
#include <libfdt.h>
+#include <IndustryStandard/ArmStdSmc.h>
+
#include <Guid/EarlyPL011BaseAddress.h>
#include <Guid/FdtHob.h>
+#include <Ppi/Reset2.h>
+
STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpm2DiscoveredPpi = {
EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
&gOvmfTpmDiscoveredPpiGuid,
NULL
};
+/**
+ The ResetSystem function resets the entire platform.
+
+ @param[in] ResetType The type of reset to perform.
+ @param[in] ResetStatus The status code for the reset.
+ @param[in] DataSize The size, in bytes, of ResetData.
+ @param[in] ResetData For a ResetType of EfiResetCold, EfiResetWarm, or
+ EfiResetShutdown the data buffer starts with a
+ Null-terminated string, optionally followed by
+ additional binary data. The string is a description
+ that the caller may use to further indicate the
+ reason for the system reset.
+**/
+STATIC
+VOID
+EFIAPI
+ResetSystemHvc (
+ IN EFI_RESET_TYPE ResetType,
+ IN EFI_STATUS ResetStatus,
+ IN UINTN DataSize,
+ IN VOID *ResetData OPTIONAL
+ )
+{
+ ARM_HVC_ARGS ArmHvcArgs;
+
+ switch (ResetType) {
+ case EfiResetWarm:
+ case EfiResetCold:
+ case EfiResetPlatformSpecific:
+ // Send a PSCI 0.2 SYSTEM_RESET command
+ ArmHvcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET;
+ break;
+
+ case EfiResetShutdown:
+ // Send a PSCI 0.2 SYSTEM_OFF command
+ ArmHvcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_OFF;
+ break;
+
+ default:
+ ASSERT (FALSE);
+ return;
+ }
+ ArmCallHvc (&ArmHvcArgs);
+}
+
+STATIC
+VOID
+EFIAPI
+ResetSystemSmc (
+ IN EFI_RESET_TYPE ResetType,
+ IN EFI_STATUS ResetStatus,
+ IN UINTN DataSize,
+ IN VOID *ResetData OPTIONAL
+ )
+{
+ ARM_SMC_ARGS ArmSmcArgs;
+
+ switch (ResetType) {
+ case EfiResetWarm:
+ case EfiResetCold:
+ case EfiResetPlatformSpecific:
+ // Send a PSCI 0.2 SYSTEM_RESET command
+ ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET;
+ break;
+
+ case EfiResetShutdown:
+ // Send a PSCI 0.2 SYSTEM_OFF command
+ ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_OFF;
+ break;
+
+ default:
+ ASSERT (FALSE);
+ return;
+ }
+ ArmCallSmc (&ArmSmcArgs);
+}
+
+STATIC CONST EFI_PEI_RESET2_PPI mPpiReset[] = {
+ { ResetSystemHvc },
+ { ResetSystemSmc },
+};
+
+STATIC CONST EFI_PEI_PPI_DESCRIPTOR mPlatformHvcResetPpi = {
+ EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
+ &gEfiPeiReset2PpiGuid,
+ (EFI_PEI_RESET2_PPI *)&mPpiReset[0]
+};
+
+STATIC CONST EFI_PEI_PPI_DESCRIPTOR mPlatformSmcResetPpi = {
+ EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
+ &gEfiPeiReset2PpiGuid,
+ (EFI_PEI_RESET2_PPI *)&mPpiReset[1]
+};
+
EFI_STATUS
EFIAPI
PlatformPeim (
@@ -47,6 +147,7 @@ PlatformPeim (
INT32 StatusLen;
CONST UINT64 *RegProp;
CONST UINT32 *RangesProp;
+ CONST VOID *MethodProp;
UINT64 UartBase;
UINT64 TpmBase;
EFI_STATUS Status;
@@ -155,6 +256,28 @@ PlatformPeim (
Status = PeiServicesInstallPpi (&mTpm2DiscoveredPpi);
ASSERT_EFI_ERROR (Status);
break;
+ } else if (AsciiStrCmp (CompItem, "arm,psci-0.2") == 0) {
+ MethodProp = fdt_getprop (Base, Node, "method", &Len);
+ if (MethodProp == NULL) {
+ DEBUG ((DEBUG_ERROR, "%a: Missing PSCI method property\n",
+ __FUNCTION__));
+ break;
+ }
+
+ if (AsciiStrnCmp (MethodProp, "hvc", 3) == 0) {
+ Status = PeiServicesInstallPpi (&mPlatformHvcResetPpi);
+ ASSERT_EFI_ERROR (Status);
+ } else if (AsciiStrnCmp (MethodProp, "smc", 3) == 0) {
+ Status = PeiServicesInstallPpi (&mPlatformSmcResetPpi);
+ ASSERT_EFI_ERROR (Status);
+ } else {
+ DEBUG ((DEBUG_ERROR, "%a: Unknown PSCI method \"%a\"\n", __FUNCTION__,
+ MethodProp));
+ break;
+ }
+ DEBUG ((DEBUG_INFO, "%a: Detected PSCI method \"%a\"\n", __FUNCTION__,
+ MethodProp));
+ break;
}
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot
2020-01-07 9:47 [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu Ard Biesheuvel
` (2 preceding siblings ...)
2020-01-07 9:47 ` [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI Ard Biesheuvel
@ 2020-01-07 9:48 ` Ard Biesheuvel
2020-01-07 17:37 ` Laszlo Ersek
2020-01-07 11:55 ` [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu Laszlo Ersek
4 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-01-07 9:48 UTC (permalink / raw)
To: devel; +Cc: lersek, Ard Biesheuvel
Duplicate the TPM2_ENABLE and TPM2_CONFIG_ENABLE build time flags that
already exist in OvmfPkg, and wire them up in the .DSC and .FDF so
that setting those flags produces a ArmVirtQemu build that implements
measured boot using a TPM provided by QEMU and described in the device
tree.
Note that the TPM2 driver stack relies on a PEI phase being implemented,
so there is no point in enabling this for ArmVirtQemuKernel or ArmVirtXen.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmVirtPkg/ArmVirtQemu.dsc | 71 ++++++++++++++++++++
ArmVirtPkg/ArmVirtQemu.fdf | 5 ++
ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 10 +++
3 files changed, 86 insertions(+)
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 7ae6702ac1f0..0a37f613ae23 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -29,6 +29,8 @@ [Defines]
#
DEFINE TTY_TERMINAL = FALSE
DEFINE SECURE_BOOT_ENABLE = FALSE
+ DEFINE TPM2_ENABLE = FALSE
+ DEFINE TPM2_CONFIG_ENABLE = FALSE
#
# Network definition
@@ -74,12 +76,32 @@ [LibraryClasses.common]
PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
+!if $(TPM2_ENABLE) == TRUE
+ Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
+ Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
+ Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
+ TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
+!else
+ Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
+ TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
+!endif
+
[LibraryClasses.common.PEIM]
ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
+!if $(TPM2_ENABLE) == TRUE
+ BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+ ResetSystemLib|MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
+ Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
+!endif
+
[LibraryClasses.common.DXE_DRIVER]
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
+!if $(TPM2_ENABLE) == TRUE
+ Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
+!endif
+
[LibraryClasses.common.UEFI_DRIVER]
UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
@@ -177,6 +199,8 @@ [PcdsFixedAtBuild.common]
gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|3
+ gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
+
[PcdsFixedAtBuild.AARCH64]
# Clearing BIT0 in this PCD prevents installing a 32-bit SMBIOS entry point,
# if the entry point version is >= 3.0. AARCH64 OSes cannot assume the
@@ -237,9 +261,26 @@ [PcdsDynamicDefault.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev|0x0
gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
+!if $(TPM2_ENABLE) == TRUE
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress|0x0
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpm2SelfTestPolicy|1
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpm2ScrtmPolicy|1
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmInitializationPolicy|1
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmScrtmPolicy|1
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|3
+ gEfiSecurityPkgTokenSpaceGuid.PcdTcg2HashAlgorithmBitmap|3
+!endif
+
[PcdsDynamicHii]
gArmVirtTokenSpaceGuid.PcdForceNoAcpi|L"ForceNoAcpi"|gArmVirtVariableGuid|0x0|FALSE|NV,BS
+!if $(TPM2_ENABLE) == TRUE
+ gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
+!endif
+
################################################################################
#
# Components Section - list of all EDK II Modules needed by this Platform
@@ -295,6 +336,9 @@ [Components.common]
MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
<LibraryClasses>
NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
+!if $(TPM2_ENABLE) == TRUE
+ NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
+!endif
}
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
@@ -430,6 +474,33 @@ [Components.common]
MdeModulePkg/Bus/Usb/UsbKbDxe/UsbKbDxe.inf
MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf
+!if $(TPM2_ENABLE) == TRUE
+ OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+ SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
+ <LibraryClasses>
+ HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
+ NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
+ NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
+ NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
+ NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
+ NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
+ }
+!if $(TPM2_CONFIG_ENABLE) == TRUE
+ SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
+!endif
+ SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
+ <LibraryClasses>
+ Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
+ NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
+ HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
+ NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
+ NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
+ NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
+ NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
+ NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
+ }
+!endif
+
#
# ACPI Support
#
diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
index 2c8936a1ae15..d866e62c529b 100644
--- a/ArmVirtPkg/ArmVirtQemu.fdf
+++ b/ArmVirtPkg/ArmVirtQemu.fdf
@@ -113,6 +113,11 @@ [FV.FVMAIN_COMPACT]
INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+!if $(TPM2_ENABLE) == TRUE
+ INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+ INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
+!endif
+
FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
SECTION FV_IMAGE = FVMAIN
diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
index 31f615a9d0f9..d481e4b2b8fb 100644
--- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
+++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
@@ -182,3 +182,13 @@ [FV.FvMain]
# Ramdisk support
#
INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
+
+ #
+ # TPM2 support
+ #
+!if $(TPM2_ENABLE) == TRUE
+ INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
+!if $(TPM2_CONFIG_ENABLE) == TRUE
+ INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
+!endif
+!endif
--
2.20.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu
2020-01-07 9:47 [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu Ard Biesheuvel
` (3 preceding siblings ...)
2020-01-07 9:48 ` [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot Ard Biesheuvel
@ 2020-01-07 11:55 ` Laszlo Ersek
2020-01-07 12:04 ` Ard Biesheuvel
4 siblings, 1 reply; 23+ messages in thread
From: Laszlo Ersek @ 2020-01-07 11:55 UTC (permalink / raw)
To: Ard Biesheuvel, devel
On 01/07/20 10:47, Ard Biesheuvel wrote:
> Wire up the various existing pieces so that we can implemented measured
> boot on ArmVirtQemu based on the TPM support in QEMU, just like it has
> been implemented for x86 in OvmfPkg.
>
> The main difference is that on ARM, we first need to discover the TPM base
> address from the device tree provided by QEMU, as well as the PSCI method
> used to perform a cold reset, so this is added to the existing implementation
> of PlatformPeiLib.
>
> The associated QEMU changes are under development in Linaro, and will be
> sent out for review to the appropriate mailing list shortly.
We usually merge firmware patches after merging the QEMU patches. Will
that work for you in this case?
Thanks!
Laszlo
> Ard Biesheuvel (4):
> OvmfPkg/Tcg2ConfigPei: introduce a signalling PPI to depex on
> ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT
> ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI
> ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot
>
> ArmVirtPkg/ArmVirtPkg.dec | 5 +
> OvmfPkg/OvmfPkg.dec | 4 +
> ArmVirtPkg/ArmVirtQemu.dsc | 71 +++++++
> ArmVirtPkg/ArmVirtQemu.fdf | 5 +
> ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf | 15 +-
> OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 6 +-
> ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c | 205 +++++++++++++++++++-
> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 10 +
> 8 files changed, 308 insertions(+), 13 deletions(-)
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/4] OvmfPkg/Tcg2ConfigPei: introduce a signalling PPI to depex on
2020-01-07 9:47 ` [PATCH 1/4] OvmfPkg/Tcg2ConfigPei: introduce a signalling PPI to depex on Ard Biesheuvel
@ 2020-01-07 11:58 ` Laszlo Ersek
0 siblings, 0 replies; 23+ messages in thread
From: Laszlo Ersek @ 2020-01-07 11:58 UTC (permalink / raw)
To: Ard Biesheuvel, devel
On 01/07/20 10:47, Ard Biesheuvel wrote:
> On ARM systems, the TPM does not live at a fixed address, and so we
> need the platform to discover it first. So introduce a PPI that signals
> that the TPM address has been discovered and recorded in the appropriate
> PCD, and make Tcg2ConfigPei depex on it when built for ARM or AARCH64.
I got briefly confused about this model, but after reviewing the commit
message of 6cf1880fb5b6 ("OvmfPkg: add customized Tcg2ConfigPei clone",
2018-03-09), I agree the above is a good addition / good fit.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> OvmfPkg/OvmfPkg.dec | 4 ++++
> OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 6 +++++-
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index d5fee805ef4a..10a2b714c1b4 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -101,6 +101,10 @@ [Protocols]
> gEfiLegacyInterruptProtocolGuid = {0x31ce593d, 0x108a, 0x485d, {0xad, 0xb2, 0x78, 0xf2, 0x1f, 0x29, 0x66, 0xbe}}
> gEfiVgaMiniPortProtocolGuid = {0xc7735a2f, 0x88f5, 0x4882, {0xae, 0x63, 0xfa, 0xac, 0x8c, 0x8b, 0x86, 0xb3}}
>
> +[Ppis]
> + # PPI whose presence in the PPI database signals that the TPM base address has been discovered and recorded
> + gOvmfTpmDiscoveredPpiGuid = {0xb9a61ad0, 0x2802, 0x41f3, {0xb5, 0x13, 0x96, 0x51, 0xce, 0x6b, 0xd5, 0x75}}
> +
(1) Please rewrap the comment to 80 characters. (The DEC file is
consistent in that, so I'd like to stick with it.)
(2) If you agree, I'd suggest moving the [Ppis] section above the
[Protocols] section, but still below the [Guids] section -- somehow, for
me, that seems to match the "level of abstraction" in PPIs. But, again,
this is optional.
With the above addressed/considered:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks,
Laszlo
> [PcdsFixedAtBuild]
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|0x0|UINT32|0
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize|0x0|UINT32|1
> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> index e34cd6210611..55684ba045b3 100644
> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> @@ -25,6 +25,7 @@ [Sources]
> [Packages]
> MdePkg/MdePkg.dec
> MdeModulePkg/MdeModulePkg.dec
> + OvmfPkg/OvmfPkg.dec
> SecurityPkg/SecurityPkg.dec
>
> [LibraryClasses]
> @@ -43,5 +44,8 @@ [Ppis]
> [Pcd]
> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES
>
> -[Depex]
> +[Depex.IA32, Depex.X64]
> TRUE
> +
> +[Depex.ARM, Depex.AARCH64]
> + gOvmfTpmDiscoveredPpiGuid
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu
2020-01-07 11:55 ` [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu Laszlo Ersek
@ 2020-01-07 12:04 ` Ard Biesheuvel
0 siblings, 0 replies; 23+ messages in thread
From: Ard Biesheuvel @ 2020-01-07 12:04 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-groups-io
On Tue, 7 Jan 2020 at 12:55, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 01/07/20 10:47, Ard Biesheuvel wrote:
> > Wire up the various existing pieces so that we can implemented measured
> > boot on ArmVirtQemu based on the TPM support in QEMU, just like it has
> > been implemented for x86 in OvmfPkg.
> >
> > The main difference is that on ARM, we first need to discover the TPM base
> > address from the device tree provided by QEMU, as well as the PSCI method
> > used to perform a cold reset, so this is added to the existing implementation
> > of PlatformPeiLib.
> >
> > The associated QEMU changes are under development in Linaro, and will be
> > sent out for review to the appropriate mailing list shortly.
>
> We usually merge firmware patches after merging the QEMU patches. Will
> that work for you in this case?
>
Absolutely. I am not in any kind of rush, but I had the patches ready
so I sent them out.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT
2020-01-07 9:47 ` [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT Ard Biesheuvel
@ 2020-01-07 15:42 ` Laszlo Ersek
2020-01-08 14:41 ` Ard Biesheuvel
0 siblings, 1 reply; 23+ messages in thread
From: Laszlo Ersek @ 2020-01-07 15:42 UTC (permalink / raw)
To: Ard Biesheuvel, devel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI
2020-01-07 9:47 ` [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI Ard Biesheuvel
@ 2020-01-07 16:50 ` Laszlo Ersek
2020-01-07 16:55 ` [edk2-devel] " Ard Biesheuvel
0 siblings, 1 reply; 23+ messages in thread
From: Laszlo Ersek @ 2020-01-07 16:50 UTC (permalink / raw)
To: Ard Biesheuvel, devel
On 01/07/20 10:47, Ard Biesheuvel wrote:
> Extend the existing DT traversal routine in PlatformPeiLib with
> discovery of the PSCI method, and expose an implementation of the
> Reset2 PPI based on the method found.
>
> This satisfies a dependency of Tcg2Pei, which needs to reset the
> platform in some cases. Since there are no other uses for system
> reset in PEI on ArmVirtQemu, simply expose the PPI directly rather
> than using the generic ResetSystemPei and the associated plumbing.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf | 3 +
> ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c | 123 ++++++++++++++++++++
> 2 files changed, 126 insertions(+)
Tcg2Pei uses ResetCold() from ResetSystemLib.
ArmVirtPkg's existent ResetSystemLib instance
(ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf) is only
suitable for DXE_DRIVER and DXE_RUNTIME_DRIVER instances. It uses our
FDT Client protocol for looking up (I think) more or less the same
things that you parse here.
As a PEI phase replacement, this patch produces gEfiPeiReset2PpiGuid,
and then in patch#4, we resolve ResetSystemLib, for PEIMs, to the
MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
instance, which depends on the PPI installed here.
I'm not too happy about installing the gEfiPeiReset2PpiGuid from
PlatformPeiLib.
As replacement, it's not ResetSystemPei what I'd recommend (which
depends on a PEI-phase ResetSystemLib instance anyway, which we don't
have, in the first place).
(1) Instead, I'd recommend implementing a PEI-phase ResetSystemLib for
ArmVirtQemu. (Under ArmVirtPkg/Library/ArmVirtPsciResetSystemLib -- new
INF file.)
Would that be a large burden? I think we'd need a helper function in
that lib instance, for returning HVC versus SMC (from the FDT), and then
we'd have to call the proper interface for the actual reset. Not fast,
but resets don't need to be fast I think.
BTW I think the following modules are never meant to be used together:
MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf
because they seem to depend mutually on each other's abstract interface
(PPI vs. lib class). So I think a given platform should include *at most
one* of these, on top of the "other" facility that it already exposes.
In ArmVirtQemu's case, I'd suggest implementing the lib class for PEI,
and then we can include "ResetSystemPei" whenever the need arises.
Thanks,
Laszlo
>
> diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
> index c41ee22c9767..72ed2413a768 100644
> --- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
> +++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
> @@ -28,6 +28,8 @@ [Packages]
> SecurityPkg/SecurityPkg.dec
>
> [LibraryClasses]
> + ArmSmcLib
> + ArmHvcLib
> DebugLib
> HobLib
> FdtLib
> @@ -44,6 +46,7 @@ [Pcd]
> gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_PRODUCES
>
> [Ppis]
> + gEfiPeiReset2PpiGuid ## SOMETIMES_PRODUCES
> gOvmfTpmDiscoveredPpiGuid ## SOMETIMES_PRODUCES
>
> [Guids]
> diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> index 249e45c04624..7af351eda003 100644
> --- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> +++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> @@ -9,6 +9,8 @@
>
> #include <PiPei.h>
>
> +#include <Library/ArmHvcLib.h>
> +#include <Library/ArmSmcLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/DebugLib.h>
> #include <Library/HobLib.h>
> @@ -16,15 +18,113 @@
> #include <Library/PeiServicesLib.h>
> #include <libfdt.h>
>
> +#include <IndustryStandard/ArmStdSmc.h>
> +
> #include <Guid/EarlyPL011BaseAddress.h>
> #include <Guid/FdtHob.h>
>
> +#include <Ppi/Reset2.h>
> +
> STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpm2DiscoveredPpi = {
> EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> &gOvmfTpmDiscoveredPpiGuid,
> NULL
> };
>
> +/**
> + The ResetSystem function resets the entire platform.
> +
> + @param[in] ResetType The type of reset to perform.
> + @param[in] ResetStatus The status code for the reset.
> + @param[in] DataSize The size, in bytes, of ResetData.
> + @param[in] ResetData For a ResetType of EfiResetCold, EfiResetWarm, or
> + EfiResetShutdown the data buffer starts with a
> + Null-terminated string, optionally followed by
> + additional binary data. The string is a description
> + that the caller may use to further indicate the
> + reason for the system reset.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +ResetSystemHvc (
> + IN EFI_RESET_TYPE ResetType,
> + IN EFI_STATUS ResetStatus,
> + IN UINTN DataSize,
> + IN VOID *ResetData OPTIONAL
> + )
> +{
> + ARM_HVC_ARGS ArmHvcArgs;
> +
> + switch (ResetType) {
> + case EfiResetWarm:
> + case EfiResetCold:
> + case EfiResetPlatformSpecific:
> + // Send a PSCI 0.2 SYSTEM_RESET command
> + ArmHvcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET;
> + break;
> +
> + case EfiResetShutdown:
> + // Send a PSCI 0.2 SYSTEM_OFF command
> + ArmHvcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_OFF;
> + break;
> +
> + default:
> + ASSERT (FALSE);
> + return;
> + }
> + ArmCallHvc (&ArmHvcArgs);
> +}
> +
> +STATIC
> +VOID
> +EFIAPI
> +ResetSystemSmc (
> + IN EFI_RESET_TYPE ResetType,
> + IN EFI_STATUS ResetStatus,
> + IN UINTN DataSize,
> + IN VOID *ResetData OPTIONAL
> + )
> +{
> + ARM_SMC_ARGS ArmSmcArgs;
> +
> + switch (ResetType) {
> + case EfiResetWarm:
> + case EfiResetCold:
> + case EfiResetPlatformSpecific:
> + // Send a PSCI 0.2 SYSTEM_RESET command
> + ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET;
> + break;
> +
> + case EfiResetShutdown:
> + // Send a PSCI 0.2 SYSTEM_OFF command
> + ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_OFF;
> + break;
> +
> + default:
> + ASSERT (FALSE);
> + return;
> + }
> + ArmCallSmc (&ArmSmcArgs);
> +}
> +
> +STATIC CONST EFI_PEI_RESET2_PPI mPpiReset[] = {
> + { ResetSystemHvc },
> + { ResetSystemSmc },
> +};
> +
> +STATIC CONST EFI_PEI_PPI_DESCRIPTOR mPlatformHvcResetPpi = {
> + EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> + &gEfiPeiReset2PpiGuid,
> + (EFI_PEI_RESET2_PPI *)&mPpiReset[0]
> +};
> +
> +STATIC CONST EFI_PEI_PPI_DESCRIPTOR mPlatformSmcResetPpi = {
> + EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> + &gEfiPeiReset2PpiGuid,
> + (EFI_PEI_RESET2_PPI *)&mPpiReset[1]
> +};
> +
> EFI_STATUS
> EFIAPI
> PlatformPeim (
> @@ -47,6 +147,7 @@ PlatformPeim (
> INT32 StatusLen;
> CONST UINT64 *RegProp;
> CONST UINT32 *RangesProp;
> + CONST VOID *MethodProp;
> UINT64 UartBase;
> UINT64 TpmBase;
> EFI_STATUS Status;
> @@ -155,6 +256,28 @@ PlatformPeim (
> Status = PeiServicesInstallPpi (&mTpm2DiscoveredPpi);
> ASSERT_EFI_ERROR (Status);
> break;
> + } else if (AsciiStrCmp (CompItem, "arm,psci-0.2") == 0) {
> + MethodProp = fdt_getprop (Base, Node, "method", &Len);
> + if (MethodProp == NULL) {
> + DEBUG ((DEBUG_ERROR, "%a: Missing PSCI method property\n",
> + __FUNCTION__));
> + break;
> + }
> +
> + if (AsciiStrnCmp (MethodProp, "hvc", 3) == 0) {
> + Status = PeiServicesInstallPpi (&mPlatformHvcResetPpi);
> + ASSERT_EFI_ERROR (Status);
> + } else if (AsciiStrnCmp (MethodProp, "smc", 3) == 0) {
> + Status = PeiServicesInstallPpi (&mPlatformSmcResetPpi);
> + ASSERT_EFI_ERROR (Status);
> + } else {
> + DEBUG ((DEBUG_ERROR, "%a: Unknown PSCI method \"%a\"\n", __FUNCTION__,
> + MethodProp));
> + break;
> + }
> + DEBUG ((DEBUG_INFO, "%a: Detected PSCI method \"%a\"\n", __FUNCTION__,
> + MethodProp));
> + break;
> }
> }
> }
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [edk2-devel] [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI
2020-01-07 16:50 ` Laszlo Ersek
@ 2020-01-07 16:55 ` Ard Biesheuvel
2020-01-07 18:47 ` Laszlo Ersek
0 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-01-07 16:55 UTC (permalink / raw)
To: edk2-devel-groups-io, Laszlo Ersek
On Tue, 7 Jan 2020 at 17:50, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 01/07/20 10:47, Ard Biesheuvel wrote:
> > Extend the existing DT traversal routine in PlatformPeiLib with
> > discovery of the PSCI method, and expose an implementation of the
> > Reset2 PPI based on the method found.
> >
> > This satisfies a dependency of Tcg2Pei, which needs to reset the
> > platform in some cases. Since there are no other uses for system
> > reset in PEI on ArmVirtQemu, simply expose the PPI directly rather
> > than using the generic ResetSystemPei and the associated plumbing.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf | 3 +
> > ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c | 123 ++++++++++++++++++++
> > 2 files changed, 126 insertions(+)
>
> Tcg2Pei uses ResetCold() from ResetSystemLib.
>
> ArmVirtPkg's existent ResetSystemLib instance
> (ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf) is only
> suitable for DXE_DRIVER and DXE_RUNTIME_DRIVER instances. It uses our
> FDT Client protocol for looking up (I think) more or less the same
> things that you parse here.
>
> As a PEI phase replacement, this patch produces gEfiPeiReset2PpiGuid,
> and then in patch#4, we resolve ResetSystemLib, for PEIMs, to the
>
> MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
>
> instance, which depends on the PPI installed here.
>
> I'm not too happy about installing the gEfiPeiReset2PpiGuid from
> PlatformPeiLib.
>
> As replacement, it's not ResetSystemPei what I'd recommend (which
> depends on a PEI-phase ResetSystemLib instance anyway, which we don't
> have, in the first place).
>
> (1) Instead, I'd recommend implementing a PEI-phase ResetSystemLib for
> ArmVirtQemu. (Under ArmVirtPkg/Library/ArmVirtPsciResetSystemLib -- new
> INF file.)
>
> Would that be a large burden? I think we'd need a helper function in
> that lib instance, for returning HVC versus SMC (from the FDT), and then
> we'd have to call the proper interface for the actual reset. Not fast,
> but resets don't need to be fast I think.
>
That is what I started out with. The problem is that I am not 100%
convinced that it is safe to dereference the initial FDT base address
at arbitrary times during PEI, and so it would be better to consume
the FDT from the GUIDed HOB. That, however, creates another ordering
issue, and so we should install a PPI that signals the availability of
the FDT GUIDed HOB.
At this point, I decided it would be better to just produce the PPI in
the PlatformPeiLib, but I agree it is not the cleanest approach.
> BTW I think the following modules are never meant to be used together:
>
> MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
> MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf
>
> because they seem to depend mutually on each other's abstract interface
> (PPI vs. lib class). So I think a given platform should include *at most
> one* of these, on top of the "other" facility that it already exposes.
> In ArmVirtQemu's case, I'd suggest implementing the lib class for PEI,
> and then we can include "ResetSystemPei" whenever the need arises.
>
The idea is that other PEIMs use the library, which is backed by the
PEIM, so that any hooks and notifications that occur at reset time can
be dispatched correctly. If every PEIM that needs to reset the system
calls into a library directly, this no longer works.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot
2020-01-07 9:48 ` [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot Ard Biesheuvel
@ 2020-01-07 17:37 ` Laszlo Ersek
2020-01-08 14:13 ` Ard Biesheuvel
0 siblings, 1 reply; 23+ messages in thread
From: Laszlo Ersek @ 2020-01-07 17:37 UTC (permalink / raw)
To: Ard Biesheuvel, devel
On 01/07/20 10:48, Ard Biesheuvel wrote:
> Duplicate the TPM2_ENABLE and TPM2_CONFIG_ENABLE build time flags that
> already exist in OvmfPkg, and wire them up in the .DSC and .FDF so
> that setting those flags produces a ArmVirtQemu build that implements
> measured boot using a TPM provided by QEMU and described in the device
> tree.
>
> Note that the TPM2 driver stack relies on a PEI phase being implemented,
> so there is no point in enabling this for ArmVirtQemuKernel or ArmVirtXen.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/ArmVirtQemu.dsc | 71 ++++++++++++++++++++
> ArmVirtPkg/ArmVirtQemu.fdf | 5 ++
> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 10 +++
> 3 files changed, 86 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 7ae6702ac1f0..0a37f613ae23 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -29,6 +29,8 @@ [Defines]
> #
> DEFINE TTY_TERMINAL = FALSE
> DEFINE SECURE_BOOT_ENABLE = FALSE
> + DEFINE TPM2_ENABLE = FALSE
> + DEFINE TPM2_CONFIG_ENABLE = FALSE
>
> #
> # Network definition
> @@ -74,12 +76,32 @@ [LibraryClasses.common]
> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>
> +!if $(TPM2_ENABLE) == TRUE
> + Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> + Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> + Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> + TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> +!else
> + Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> + TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> +!endif
> +
> [LibraryClasses.common.PEIM]
> ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
>
> +!if $(TPM2_ENABLE) == TRUE
> + BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> + ResetSystemLib|MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
(1) So, according to my suggestion / question under patch #3, this would
be resolved to an ArmVirtPkg specific instance.
> + Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> +!endif
> +
> [LibraryClasses.common.DXE_DRIVER]
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>
> +!if $(TPM2_ENABLE) == TRUE
> + Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> +!endif
> +
> [LibraryClasses.common.UEFI_DRIVER]
> UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
>
> @@ -177,6 +199,8 @@ [PcdsFixedAtBuild.common]
>
> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|3
>
> + gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
> +
(2) This should be a [PcdsFeatureFlag] in the DSC file too (per patch#2
request).
> [PcdsFixedAtBuild.AARCH64]
> # Clearing BIT0 in this PCD prevents installing a 32-bit SMBIOS entry point,
> # if the entry point version is >= 3.0. AARCH64 OSes cannot assume the
> @@ -237,9 +261,26 @@ [PcdsDynamicDefault.common]
> gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev|0x0
> gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
>
> +!if $(TPM2_ENABLE) == TRUE
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
OK
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress|0x0
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2SelfTestPolicy|1
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2ScrtmPolicy|1
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmInitializationPolicy|1
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmScrtmPolicy|1
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|3
> + gEfiSecurityPkgTokenSpaceGuid.PcdTcg2HashAlgorithmBitmap|3
(3) Why is it necessary to provide dynamic defaults for these?
Are we missing something important in OVMF, or are these specific
defaults that we like for ArmVirtPkg? In the latter case, I think we
should add them with a separate patch, as the commit message here refers
to OvmfPkg.
> +!endif
> +
> [PcdsDynamicHii]
> gArmVirtTokenSpaceGuid.PcdForceNoAcpi|L"ForceNoAcpi"|gArmVirtVariableGuid|0x0|FALSE|NV,BS
>
> +!if $(TPM2_ENABLE) == TRUE
> + gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> +!endif
(4) Same as (3) -- I don't know what these do and why we need them here,
unlike in OvmfPkg. If they really belong here (in this patch), can you
explain in the commit message?
> +
> ################################################################################
> #
> # Components Section - list of all EDK II Modules needed by this Platform
> @@ -295,6 +336,9 @@ [Components.common]
> MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
> <LibraryClasses>
> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> +!if $(TPM2_ENABLE) == TRUE
> + NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> +!endif
> }
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
> OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
> @@ -430,6 +474,33 @@ [Components.common]
> MdeModulePkg/Bus/Usb/UsbKbDxe/UsbKbDxe.inf
> MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf
>
> +!if $(TPM2_ENABLE) == TRUE
> + OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> + SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> + <LibraryClasses>
> + HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> + NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> + NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> + NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> + NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> + NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> + }
Hrmpf, this is a bit messy. Not this patch, but the original OvmfPkg DSC
code.
The general idea is to keep the "PEI phase modules" and "DXE phase
modules" nicely separated. Unfortunately, that's already broken in the
OvmfPkg DSC files, as follows:
---------
#
# PEI Phase modules
#
...
!if $(TPM2_ENABLE) == TRUE
OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
<LibraryClasses>
HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
}
!if $(TPM2_CONFIG_ENABLE) == TRUE
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
!endif
!endif
#
# DXE Phase modules
#
---------
In other words, in OvmfPkg the Tcg2ConfigDxe driver is added under "PEI
Phase modules", and not under "DXE Phase modules".
Conversely, in the present patch for ArmVirtQemu, "Tcg2ConfigPei.inf"
and "Tcg2Pei.inf" would be listed near "USB Support" (DXE phase).
So... I hope I won't annoy you too much by asking that we should please
fix up both :)
(5) Please disentangle the OvmfPkg DSC files: please move
"Tcg2ConfigDxe.inf" near "Tcg2Dxe.inf". In a separate patch. :)
(6a) In ArmVirtPkg (in this patch), please move "Tcg2ConfigPei.inf" and
"Tcg2Pei.inf" under
#
# PEI Phase modules
#
(6b) furthermore, for the DXE modules, please add a new header "TPM2
Support":
---------------
#
# USB Support
#
...
#
# TPM2 Support
#
!if $(TPM2_ENABLE) == TRUE
SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
...
}
!if $(TPM2_CONFIG_ENABLE) == TRUE
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
!endif
!endif
#
# ACPI Support
#
...
---------------
> +!if $(TPM2_CONFIG_ENABLE) == TRUE
> + SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> +!endif
> + SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> + <LibraryClasses>
> + Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> + NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> + HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> + NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> + NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> + NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> + NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> + NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> + }
> +!endif
> +
> #
> # ACPI Support
> #
> diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
> index 2c8936a1ae15..d866e62c529b 100644
> --- a/ArmVirtPkg/ArmVirtQemu.fdf
> +++ b/ArmVirtPkg/ArmVirtQemu.fdf
> @@ -113,6 +113,11 @@ [FV.FVMAIN_COMPACT]
> INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>
> +!if $(TPM2_ENABLE) == TRUE
> + INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> + INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> +!endif
> +
> FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
> SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
> SECTION FV_IMAGE = FVMAIN
Looks good.
> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> index 31f615a9d0f9..d481e4b2b8fb 100644
> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> @@ -182,3 +182,13 @@ [FV.FvMain]
> # Ramdisk support
> #
> INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> +
> + #
> + # TPM2 support
> + #
> +!if $(TPM2_ENABLE) == TRUE
> + INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> +!if $(TPM2_CONFIG_ENABLE) == TRUE
> + INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> +!endif
> +!endif
>
I think this also belongs in "ArmVirtQemu.fdf":
"ArmVirtQemuFvMain.fdf.inc" is for sharing between ArmVirtQemu and
ArmVirtQemuKernel, and the commit message correctly states that
ArmVirtQemuKernel is not a suitable platform for TPM2. ArmVirtQemu-only
stuff should go into "ArmVirtQemu.fdf".
Now... I realize what I'm requesting would look quite awkward, because
"Tcg2Dxe.inf" and "Tcg2ConfigDxe.inf" would have to be listed in
"ArmVirtQemu.fdf" right after
!include ArmVirtQemuFvMain.fdf.inc
and that indeed looks super ugly.
We could move the [FV.FvMain] section header from
"ArmVirtQemuFvMain.fdf.inc" to both "ArmVirtQemu.fdf" and
"ArmVirtQemuKernel.fdf":
----
[FV.FvMain]
!include ArmVirtQemuFvMain.fdf.inc
----
and then listing "Tcg2Dxe.inf" and "Tcg2ConfigDxe.inf" in just
"ArmVirtQemu.fdf", after the include directive, would not look *that*
ugly. (The section header would be visible nearby.)
But that's a lot of churn little benefit. So ultimately I'm OK with this
too, just please
(7) mention in the commit message:
"ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc is being modified -- despite
ArmVirtQemuKernel being unaffected -- for keeping the contexts of the
referring !include directives simple."
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [edk2-devel] [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI
2020-01-07 16:55 ` [edk2-devel] " Ard Biesheuvel
@ 2020-01-07 18:47 ` Laszlo Ersek
2020-01-08 9:59 ` Ard Biesheuvel
0 siblings, 1 reply; 23+ messages in thread
From: Laszlo Ersek @ 2020-01-07 18:47 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel-groups-io
On 01/07/20 17:55, Ard Biesheuvel wrote:
> On Tue, 7 Jan 2020 at 17:50, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 01/07/20 10:47, Ard Biesheuvel wrote:
>>> Extend the existing DT traversal routine in PlatformPeiLib with
>>> discovery of the PSCI method, and expose an implementation of the
>>> Reset2 PPI based on the method found.
>>>
>>> This satisfies a dependency of Tcg2Pei, which needs to reset the
>>> platform in some cases. Since there are no other uses for system
>>> reset in PEI on ArmVirtQemu, simply expose the PPI directly rather
>>> than using the generic ResetSystemPei and the associated plumbing.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>> ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf | 3 +
>>> ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c | 123 ++++++++++++++++++++
>>> 2 files changed, 126 insertions(+)
>>
>> Tcg2Pei uses ResetCold() from ResetSystemLib.
>>
>> ArmVirtPkg's existent ResetSystemLib instance
>> (ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf) is only
>> suitable for DXE_DRIVER and DXE_RUNTIME_DRIVER instances. It uses our
>> FDT Client protocol for looking up (I think) more or less the same
>> things that you parse here.
>>
>> As a PEI phase replacement, this patch produces gEfiPeiReset2PpiGuid,
>> and then in patch#4, we resolve ResetSystemLib, for PEIMs, to the
>>
>> MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
>>
>> instance, which depends on the PPI installed here.
>>
>> I'm not too happy about installing the gEfiPeiReset2PpiGuid from
>> PlatformPeiLib.
>>
>> As replacement, it's not ResetSystemPei what I'd recommend (which
>> depends on a PEI-phase ResetSystemLib instance anyway, which we don't
>> have, in the first place).
>>
>> (1) Instead, I'd recommend implementing a PEI-phase ResetSystemLib for
>> ArmVirtQemu. (Under ArmVirtPkg/Library/ArmVirtPsciResetSystemLib -- new
>> INF file.)
>>
>> Would that be a large burden? I think we'd need a helper function in
>> that lib instance, for returning HVC versus SMC (from the FDT), and then
>> we'd have to call the proper interface for the actual reset. Not fast,
>> but resets don't need to be fast I think.
>>
>
> That is what I started out with. The problem is that I am not 100%
> convinced that it is safe to dereference the initial FDT base address
> at arbitrary times during PEI,
Great point; this is one of those things that I had to think about for
many minutes before posting my email.
I think it's safe. For two reasons:
(i) all of the PEIMs, and the PEI_CORE, in ArmVirtQemu, use the
ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
instance for writing to the serial port. This library instance re-parses
the initial DTB at every DEBUG call, in effect, across all the PEIMs.
(See SerialPortWrite() --> SerialPortGetBaseAddress() -->
PcdGet64(PcdDeviceTreeInitialBaseAddress)).
In other words, we're already doing this, at the moment.
(ii) In
ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c
we have:
//
// We need to make sure that the machine we are running on has at least
// 128 MB of memory configured, and is currently executing this binary from
// NOR flash. This prevents a device tree image in DRAM from getting
// clobbered when our caller installs permanent PEI RAM, before we have a
// chance of marking its location as reserved or copy it to a freshly
// allocated block in the permanent PEI RAM in the platform PEIM.
//
ASSERT (NewSize >= SIZE_128MB);
ASSERT (
(((UINT64)PcdGet64 (PcdFdBaseAddress) +
(UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));
To elaborate on this: initially we use the temporary SEC/PEI heap+stack;
later on we use the permanent PEI RAM.
(ii.1) The temp SEC/PEI heap+stack is set up in
ArmPlatformPkg/PrePeiCore/MainUniCore.c
and it is based on PcdCPUCoresStackBase. The value of
PcdCPUCoresStackBase is fixed, in ArmVirtQemu.dsc:
gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000
whereas the initial DTB is at the base of DRAM:
gArmTokenSpaceGuid.PcdSystemMemoryBase|0x40000000
gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress|0x40000000
so if the initial DTB fits in 0x7C000 bytes (496 KiB), we're good, as
far as the temporary SEC/PEI heap+stack is concerned.
(ii.2) The permanent PEI RAM is 64MB in size:
# Size of the region used by UEFI in permanent memory (Reserved 64MB)
gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x04000000
Because of the *two* ASSERT()s in "QemuVirtMemInfoPeiLibConstructor.c"
that I quoted above, we know that the lowest DRAM node is at least 128MB
in size, and also that it does not overlap with the flash device.
Consequently, the the InitializeMemory() function in
ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
will take the following branch:
------
} else {
// Check the Firmware does not overlapped with the system memory
ASSERT ((FdBase < SystemMemoryBase) || (FdBase >= SystemMemoryTop));
ASSERT ((FdTop <= SystemMemoryBase) || (FdTop > SystemMemoryTop));
UefiMemoryBase = SystemMemoryTop - FixedPcdGet32 (PcdSystemMemoryUefiRegionSize);
}
Status = PeiServicesInstallPeiMemory (UefiMemoryBase, FixedPcdGet32 (PcdSystemMemoryUefiRegionSize));
------
and therefore
UefiMemoryBase == (PcdSystemMemoryBase + PcdSystemMemorySize) - PcdSystemMemoryUefiRegionSize
== 1GiB + PcdSystemMemorySize - 64MiB
Given that PcdSystemMemorySize >= 128 MiB, we get
UefiMemoryBase >= 1GiB + 64 MiB
Which means that the permanent PEI RAM too is safely distinct from the
initial DTB (which is at the base of DRAM: at 1 GiB).
In summary: it's safe, and it's so by design. We did this intentionally.
Originally in commit a36d531f5d56 ("ArmPlatformPkg/ArmVirtualizationPkg:
add ArmVirtualizationPlatformLib library", 2014-09-18):
commit a36d531f5d565e6cb5496ea53824e36487a227dd
Author: Michael Casadevall <michael.casadevall@linaro.org>
Date: Thu Sep 18 18:05:03 2014 +0000
ArmPlatformPkg/ArmVirtualizationPkg: add ArmVirtualizationPlatformLib library
This is an implementation of ArmPlatformLib that discovers the size of system
DRAM from a device tree blob located at the address passed in
gArmTokenSpaceGuid.PcdDeviceTreeBaseAddress, which should equal the value in
gArmTokenSpaceGuid.PcdSystemMemoryBase.
As the device tree blob is passed in system DRAM, this library can only be used
if sufficient DRAM is available (>= 128 MB) and if not using shadowed NOR. The
reason for this is that it makes it easier to guarantee that such a device tree
blob at base of DRAM will not be clobbered before we get a chance to preserve it.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Casadevall <michael.casadevall@linaro.org>
Acked-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-By: Olivier Martin <olivier.martin@arm.com>
And then we moved it to its present spot in the series that contains
commit 048651260b66 ("ArmVirtPkg: create QemuVirtMemInfoLib version for
ArmVirtQemu", 2017-11-23).
> and so it would be better to consume
> the FDT from the GUIDed HOB. That, however, creates another ordering
> issue, and so we should install a PPI that signals the availability of
> the FDT GUIDed HOB.
>
> At this point, I decided it would be better to just produce the PPI in
> the PlatformPeiLib, but I agree it is not the cleanest approach.
>
>> BTW I think the following modules are never meant to be used together:
>>
>> MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
>> MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf
>>
>> because they seem to depend mutually on each other's abstract interface
>> (PPI vs. lib class). So I think a given platform should include *at most
>> one* of these, on top of the "other" facility that it already exposes.
>> In ArmVirtQemu's case, I'd suggest implementing the lib class for PEI,
>> and then we can include "ResetSystemPei" whenever the need arises.
>>
>
> The idea is that other PEIMs use the library, which is backed by the
> PEIM, so that any hooks and notifications that occur at reset time can
> be dispatched correctly. If every PEIM that needs to reset the system
> calls into a library directly, this no longer works.
>
Good point -- now I realize my exclusivity argument above is wrong. I
now recall why.
The following is a quite common pattern in edk2:
- there is a lib class offering some service, at the abstract level
- there is a PEIM or DXE driver that exposes the same service as a PPI
or protocol, respectively
- this PEIM or DXE driver internally calls the lib API
- there are two lib instances: one instance does the real thing, and the
other instance calls out to the PPI or protocol
- all PEIMs / DXE drivers *except* the one PEIM or DXE driver mentioned
above get the "shim" lib instance
- the one PEIM / DXE driver that exposes the PPI / protocol gets the
"real deal" lib instance, via module-level lib class resolution
override in the DSC file.
We can do the same here, I think:
- resolve ResetSystemLib, generally for PEIMs, to
"MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf",
- include "MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf" in
the firmware binary,
- resolve ResetSystemLib, specifically for "ResetSystemPei.inf", to our
new (FDT-parsing) lib instance.
How does it sound to you? Yes, it's more fluff, but it's clean, and
native to edk2.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [edk2-devel] [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI
2020-01-07 18:47 ` Laszlo Ersek
@ 2020-01-08 9:59 ` Ard Biesheuvel
0 siblings, 0 replies; 23+ messages in thread
From: Ard Biesheuvel @ 2020-01-08 9:59 UTC (permalink / raw)
To: edk2-devel-groups-io, Laszlo Ersek
On Tue, 7 Jan 2020 at 19:47, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 01/07/20 17:55, Ard Biesheuvel wrote:
> > On Tue, 7 Jan 2020 at 17:50, Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >> On 01/07/20 10:47, Ard Biesheuvel wrote:
> >>> Extend the existing DT traversal routine in PlatformPeiLib with
> >>> discovery of the PSCI method, and expose an implementation of the
> >>> Reset2 PPI based on the method found.
> >>>
> >>> This satisfies a dependency of Tcg2Pei, which needs to reset the
> >>> platform in some cases. Since there are no other uses for system
> >>> reset in PEI on ArmVirtQemu, simply expose the PPI directly rather
> >>> than using the generic ResetSystemPei and the associated plumbing.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> ---
> >>> ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf | 3 +
> >>> ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c | 123 ++++++++++++++++++++
> >>> 2 files changed, 126 insertions(+)
> >>
> >> Tcg2Pei uses ResetCold() from ResetSystemLib.
> >>
> >> ArmVirtPkg's existent ResetSystemLib instance
> >> (ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf) is only
> >> suitable for DXE_DRIVER and DXE_RUNTIME_DRIVER instances. It uses our
> >> FDT Client protocol for looking up (I think) more or less the same
> >> things that you parse here.
> >>
> >> As a PEI phase replacement, this patch produces gEfiPeiReset2PpiGuid,
> >> and then in patch#4, we resolve ResetSystemLib, for PEIMs, to the
> >>
> >> MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
> >>
> >> instance, which depends on the PPI installed here.
> >>
> >> I'm not too happy about installing the gEfiPeiReset2PpiGuid from
> >> PlatformPeiLib.
> >>
> >> As replacement, it's not ResetSystemPei what I'd recommend (which
> >> depends on a PEI-phase ResetSystemLib instance anyway, which we don't
> >> have, in the first place).
> >>
> >> (1) Instead, I'd recommend implementing a PEI-phase ResetSystemLib for
> >> ArmVirtQemu. (Under ArmVirtPkg/Library/ArmVirtPsciResetSystemLib -- new
> >> INF file.)
> >>
> >> Would that be a large burden? I think we'd need a helper function in
> >> that lib instance, for returning HVC versus SMC (from the FDT), and then
> >> we'd have to call the proper interface for the actual reset. Not fast,
> >> but resets don't need to be fast I think.
> >>
> >
> > That is what I started out with. The problem is that I am not 100%
> > convinced that it is safe to dereference the initial FDT base address
> > at arbitrary times during PEI,
>
> Great point; this is one of those things that I had to think about for
> many minutes before posting my email.
>
> I think it's safe. For two reasons:
>
> (i) all of the PEIMs, and the PEI_CORE, in ArmVirtQemu, use the
>
> ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
>
> instance for writing to the serial port. This library instance re-parses
> the initial DTB at every DEBUG call, in effect, across all the PEIMs.
>
> (See SerialPortWrite() --> SerialPortGetBaseAddress() -->
> PcdGet64(PcdDeviceTreeInitialBaseAddress)).
>
> In other words, we're already doing this, at the moment.
>
> (ii) In
>
> ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c
>
> we have:
>
> //
> // We need to make sure that the machine we are running on has at least
> // 128 MB of memory configured, and is currently executing this binary from
> // NOR flash. This prevents a device tree image in DRAM from getting
> // clobbered when our caller installs permanent PEI RAM, before we have a
> // chance of marking its location as reserved or copy it to a freshly
> // allocated block in the permanent PEI RAM in the platform PEIM.
> //
> ASSERT (NewSize >= SIZE_128MB);
> ASSERT (
> (((UINT64)PcdGet64 (PcdFdBaseAddress) +
> (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
> ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));
>
>
> To elaborate on this: initially we use the temporary SEC/PEI heap+stack;
> later on we use the permanent PEI RAM.
>
> (ii.1) The temp SEC/PEI heap+stack is set up in
>
> ArmPlatformPkg/PrePeiCore/MainUniCore.c
>
> and it is based on PcdCPUCoresStackBase. The value of
> PcdCPUCoresStackBase is fixed, in ArmVirtQemu.dsc:
>
> gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000
>
> whereas the initial DTB is at the base of DRAM:
>
> gArmTokenSpaceGuid.PcdSystemMemoryBase|0x40000000
> gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress|0x40000000
>
> so if the initial DTB fits in 0x7C000 bytes (496 KiB), we're good, as
> far as the temporary SEC/PEI heap+stack is concerned.
>
> (ii.2) The permanent PEI RAM is 64MB in size:
>
> # Size of the region used by UEFI in permanent memory (Reserved 64MB)
> gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x04000000
>
> Because of the *two* ASSERT()s in "QemuVirtMemInfoPeiLibConstructor.c"
> that I quoted above, we know that the lowest DRAM node is at least 128MB
> in size, and also that it does not overlap with the flash device.
> Consequently, the the InitializeMemory() function in
>
> ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
>
> will take the following branch:
>
> ------
> } else {
> // Check the Firmware does not overlapped with the system memory
> ASSERT ((FdBase < SystemMemoryBase) || (FdBase >= SystemMemoryTop));
> ASSERT ((FdTop <= SystemMemoryBase) || (FdTop > SystemMemoryTop));
>
> UefiMemoryBase = SystemMemoryTop - FixedPcdGet32 (PcdSystemMemoryUefiRegionSize);
> }
>
> Status = PeiServicesInstallPeiMemory (UefiMemoryBase, FixedPcdGet32 (PcdSystemMemoryUefiRegionSize));
> ------
>
> and therefore
>
> UefiMemoryBase == (PcdSystemMemoryBase + PcdSystemMemorySize) - PcdSystemMemoryUefiRegionSize
> == 1GiB + PcdSystemMemorySize - 64MiB
>
> Given that PcdSystemMemorySize >= 128 MiB, we get
>
> UefiMemoryBase >= 1GiB + 64 MiB
>
> Which means that the permanent PEI RAM too is safely distinct from the
> initial DTB (which is at the base of DRAM: at 1 GiB).
>
> In summary: it's safe, and it's so by design. We did this intentionally.
> Originally in commit a36d531f5d56 ("ArmPlatformPkg/ArmVirtualizationPkg:
> add ArmVirtualizationPlatformLib library", 2014-09-18):
>
> commit a36d531f5d565e6cb5496ea53824e36487a227dd
> Author: Michael Casadevall <michael.casadevall@linaro.org>
> Date: Thu Sep 18 18:05:03 2014 +0000
>
> ArmPlatformPkg/ArmVirtualizationPkg: add ArmVirtualizationPlatformLib library
>
> This is an implementation of ArmPlatformLib that discovers the size of system
> DRAM from a device tree blob located at the address passed in
> gArmTokenSpaceGuid.PcdDeviceTreeBaseAddress, which should equal the value in
> gArmTokenSpaceGuid.PcdSystemMemoryBase.
>
> As the device tree blob is passed in system DRAM, this library can only be used
> if sufficient DRAM is available (>= 128 MB) and if not using shadowed NOR. The
> reason for this is that it makes it easier to guarantee that such a device tree
> blob at base of DRAM will not be clobbered before we get a chance to preserve it.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Casadevall <michael.casadevall@linaro.org>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-By: Olivier Martin <olivier.martin@arm.com>
>
> And then we moved it to its present spot in the series that contains
> commit 048651260b66 ("ArmVirtPkg: create QemuVirtMemInfoLib version for
> ArmVirtQemu", 2017-11-23).
>
> > and so it would be better to consume
> > the FDT from the GUIDed HOB. That, however, creates another ordering
> > issue, and so we should install a PPI that signals the availability of
> > the FDT GUIDed HOB.
> >
> > At this point, I decided it would be better to just produce the PPI in
> > the PlatformPeiLib, but I agree it is not the cleanest approach.
> >
> >> BTW I think the following modules are never meant to be used together:
> >>
> >> MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
> >> MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf
> >>
> >> because they seem to depend mutually on each other's abstract interface
> >> (PPI vs. lib class). So I think a given platform should include *at most
> >> one* of these, on top of the "other" facility that it already exposes.
> >> In ArmVirtQemu's case, I'd suggest implementing the lib class for PEI,
> >> and then we can include "ResetSystemPei" whenever the need arises.
> >>
> >
> > The idea is that other PEIMs use the library, which is backed by the
> > PEIM, so that any hooks and notifications that occur at reset time can
> > be dispatched correctly. If every PEIM that needs to reset the system
> > calls into a library directly, this no longer works.
> >
>
> Good point -- now I realize my exclusivity argument above is wrong. I
> now recall why.
>
> The following is a quite common pattern in edk2:
>
> - there is a lib class offering some service, at the abstract level
> - there is a PEIM or DXE driver that exposes the same service as a PPI
> or protocol, respectively
> - this PEIM or DXE driver internally calls the lib API
> - there are two lib instances: one instance does the real thing, and the
> other instance calls out to the PPI or protocol
> - all PEIMs / DXE drivers *except* the one PEIM or DXE driver mentioned
> above get the "shim" lib instance
> - the one PEIM / DXE driver that exposes the PPI / protocol gets the
> "real deal" lib instance, via module-level lib class resolution
> override in the DSC file.
>
> We can do the same here, I think:
>
> - resolve ResetSystemLib, generally for PEIMs, to
> "MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf",
> - include "MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf" in
> the firmware binary,
> - resolve ResetSystemLib, specifically for "ResetSystemPei.inf", to our
> new (FDT-parsing) lib instance.
>
> How does it sound to you? Yes, it's more fluff, but it's clean, and
> native to edk2.
>
Yeah, I'm fine with all of that. Thanks for reminding me why it is
safe to refer to the DT at the base of memory throughout the PEI
phase.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot
2020-01-07 17:37 ` Laszlo Ersek
@ 2020-01-08 14:13 ` Ard Biesheuvel
2020-01-08 14:45 ` Laszlo Ersek
0 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-01-08 14:13 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-groups-io
On Tue, 7 Jan 2020 at 18:37, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 01/07/20 10:48, Ard Biesheuvel wrote:
> > Duplicate the TPM2_ENABLE and TPM2_CONFIG_ENABLE build time flags that
> > already exist in OvmfPkg, and wire them up in the .DSC and .FDF so
> > that setting those flags produces a ArmVirtQemu build that implements
> > measured boot using a TPM provided by QEMU and described in the device
> > tree.
> >
> > Note that the TPM2 driver stack relies on a PEI phase being implemented,
> > so there is no point in enabling this for ArmVirtQemuKernel or ArmVirtXen.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > ArmVirtPkg/ArmVirtQemu.dsc | 71 ++++++++++++++++++++
> > ArmVirtPkg/ArmVirtQemu.fdf | 5 ++
> > ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 10 +++
> > 3 files changed, 86 insertions(+)
> >
> > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> > index 7ae6702ac1f0..0a37f613ae23 100644
> > --- a/ArmVirtPkg/ArmVirtQemu.dsc
> > +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> > @@ -29,6 +29,8 @@ [Defines]
> > #
> > DEFINE TTY_TERMINAL = FALSE
> > DEFINE SECURE_BOOT_ENABLE = FALSE
> > + DEFINE TPM2_ENABLE = FALSE
> > + DEFINE TPM2_CONFIG_ENABLE = FALSE
> >
> > #
> > # Network definition
> > @@ -74,12 +76,32 @@ [LibraryClasses.common]
> > PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> > PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> >
> > +!if $(TPM2_ENABLE) == TRUE
> > + Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> > + Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> > + Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> > + TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> > +!else
> > + Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> > + TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> > +!endif
> > +
> > [LibraryClasses.common.PEIM]
> > ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> >
> > +!if $(TPM2_ENABLE) == TRUE
> > + BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > + ResetSystemLib|MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
>
> (1) So, according to my suggestion / question under patch #3, this would
> be resolved to an ArmVirtPkg specific instance.
>
> > + Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> > +!endif
> > +
> > [LibraryClasses.common.DXE_DRIVER]
> > ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> >
> > +!if $(TPM2_ENABLE) == TRUE
> > + Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> > +!endif
> > +
> > [LibraryClasses.common.UEFI_DRIVER]
> > UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> >
> > @@ -177,6 +199,8 @@ [PcdsFixedAtBuild.common]
> >
> > gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|3
> >
> > + gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
> > +
>
> (2) This should be a [PcdsFeatureFlag] in the DSC file too (per patch#2
> request).
>
> > [PcdsFixedAtBuild.AARCH64]
> > # Clearing BIT0 in this PCD prevents installing a 32-bit SMBIOS entry point,
> > # if the entry point version is >= 3.0. AARCH64 OSes cannot assume the
> > @@ -237,9 +261,26 @@ [PcdsDynamicDefault.common]
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev|0x0
> > gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
> >
> > +!if $(TPM2_ENABLE) == TRUE
> > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
>
> OK
>
> > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress|0x0
> > + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1
> > + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2SelfTestPolicy|1
> > + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2ScrtmPolicy|1
> > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmInitializationPolicy|1
> > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmScrtmPolicy|1
> > + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|3
> > + gEfiSecurityPkgTokenSpaceGuid.PcdTcg2HashAlgorithmBitmap|3
>
> (3) Why is it necessary to provide dynamic defaults for these?
>
> Are we missing something important in OVMF, or are these specific
> defaults that we like for ArmVirtPkg? In the latter case, I think we
> should add them with a separate patch, as the commit message here refers
> to OvmfPkg.
>
The policy ones can be dropped, but I see warnings like these
WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC)
WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD)
FinalEventsTable->NumberOfEvents - 0x3
Size - 0x15A
SupportedEventLogs - 0x00000003
LogFormat - 0x00000001
LogFormat - 0x00000002
WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC)
WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD)
if I leave PcdTpm2HashMask at its default value
> > +!endif
> > +
> > [PcdsDynamicHii]
> > gArmVirtTokenSpaceGuid.PcdForceNoAcpi|L"ForceNoAcpi"|gArmVirtVariableGuid|0x0|FALSE|NV,BS
> >
> > +!if $(TPM2_ENABLE) == TRUE
> > + gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> > + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> > +!endif
>
> (4) Same as (3) -- I don't know what these do and why we need them here,
> unlike in OvmfPkg. If they really belong here (in this patch), can you
> explain in the commit message?
>
These are related to the TPM2 ACPI table that describes the physical
presence interface to the OS, but I'm not even sure we can support
this on ARM today, given that it relies on SMIs so I can drop these
for now, I think.
> > +
> > ################################################################################
> > #
> > # Components Section - list of all EDK II Modules needed by this Platform
> > @@ -295,6 +336,9 @@ [Components.common]
> > MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
> > <LibraryClasses>
> > NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> > +!if $(TPM2_ENABLE) == TRUE
> > + NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> > +!endif
> > }
> > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
> > OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
> > @@ -430,6 +474,33 @@ [Components.common]
> > MdeModulePkg/Bus/Usb/UsbKbDxe/UsbKbDxe.inf
> > MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf
> >
> > +!if $(TPM2_ENABLE) == TRUE
> > + OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > + SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> > + <LibraryClasses>
> > + HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> > + NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > + NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > + NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > + NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > + NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > + }
>
> Hrmpf, this is a bit messy. Not this patch, but the original OvmfPkg DSC
> code.
>
> The general idea is to keep the "PEI phase modules" and "DXE phase
> modules" nicely separated. Unfortunately, that's already broken in the
> OvmfPkg DSC files, as follows:
>
> ---------
> #
> # PEI Phase modules
> #
> ...
> !if $(TPM2_ENABLE) == TRUE
> OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> <LibraryClasses>
> HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> }
> !if $(TPM2_CONFIG_ENABLE) == TRUE
> SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> !endif
> !endif
>
> #
> # DXE Phase modules
> #
> ---------
>
> In other words, in OvmfPkg the Tcg2ConfigDxe driver is added under "PEI
> Phase modules", and not under "DXE Phase modules".
>
> Conversely, in the present patch for ArmVirtQemu, "Tcg2ConfigPei.inf"
> and "Tcg2Pei.inf" would be listed near "USB Support" (DXE phase).
>
> So... I hope I won't annoy you too much by asking that we should please
> fix up both :)
>
> (5) Please disentangle the OvmfPkg DSC files: please move
> "Tcg2ConfigDxe.inf" near "Tcg2Dxe.inf". In a separate patch. :)
>
NP
> (6a) In ArmVirtPkg (in this patch), please move "Tcg2ConfigPei.inf" and
> "Tcg2Pei.inf" under
>
> #
> # PEI Phase modules
> #
>
> (6b) furthermore, for the DXE modules, please add a new header "TPM2
> Support":
>
> ---------------
> #
> # USB Support
> #
> ...
> #
> # TPM2 Support
> #
> !if $(TPM2_ENABLE) == TRUE
> SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> ...
> }
> !if $(TPM2_CONFIG_ENABLE) == TRUE
> SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> !endif
> !endif
>
> #
> # ACPI Support
> #
> ...
> ---------------
>
>
> > +!if $(TPM2_CONFIG_ENABLE) == TRUE
> > + SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > +!endif
> > + SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> > + <LibraryClasses>
> > + Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> > + NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> > + HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> > + NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > + NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > + NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > + NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > + NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > + }
> > +!endif
> > +
> > #
> > # ACPI Support
> > #
> > diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
> > index 2c8936a1ae15..d866e62c529b 100644
> > --- a/ArmVirtPkg/ArmVirtQemu.fdf
> > +++ b/ArmVirtPkg/ArmVirtQemu.fdf
> > @@ -113,6 +113,11 @@ [FV.FVMAIN_COMPACT]
> > INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> > INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >
> > +!if $(TPM2_ENABLE) == TRUE
> > + INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > + INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> > +!endif
> > +
> > FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
> > SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
> > SECTION FV_IMAGE = FVMAIN
>
> Looks good.
>
> > diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> > index 31f615a9d0f9..d481e4b2b8fb 100644
> > --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> > +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> > @@ -182,3 +182,13 @@ [FV.FvMain]
> > # Ramdisk support
> > #
> > INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> > +
> > + #
> > + # TPM2 support
> > + #
> > +!if $(TPM2_ENABLE) == TRUE
> > + INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> > +!if $(TPM2_CONFIG_ENABLE) == TRUE
> > + INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > +!endif
> > +!endif
> >
>
> I think this also belongs in "ArmVirtQemu.fdf":
>
> "ArmVirtQemuFvMain.fdf.inc" is for sharing between ArmVirtQemu and
> ArmVirtQemuKernel, and the commit message correctly states that
> ArmVirtQemuKernel is not a suitable platform for TPM2. ArmVirtQemu-only
> stuff should go into "ArmVirtQemu.fdf".
>
> Now... I realize what I'm requesting would look quite awkward, because
> "Tcg2Dxe.inf" and "Tcg2ConfigDxe.inf" would have to be listed in
> "ArmVirtQemu.fdf" right after
>
> !include ArmVirtQemuFvMain.fdf.inc
>
> and that indeed looks super ugly.
>
> We could move the [FV.FvMain] section header from
> "ArmVirtQemuFvMain.fdf.inc" to both "ArmVirtQemu.fdf" and
> "ArmVirtQemuKernel.fdf":
>
> ----
> [FV.FvMain]
> !include ArmVirtQemuFvMain.fdf.inc
> ----
>
> and then listing "Tcg2Dxe.inf" and "Tcg2ConfigDxe.inf" in just
> "ArmVirtQemu.fdf", after the include directive, would not look *that*
> ugly. (The section header would be visible nearby.)
>
> But that's a lot of churn little benefit. So ultimately I'm OK with this
> too, just please
>
> (7) mention in the commit message:
>
> "ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc is being modified -- despite
> ArmVirtQemuKernel being unaffected -- for keeping the contexts of the
> referring !include directives simple."
>
> Thanks!
> Laszlo
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT
2020-01-07 15:42 ` Laszlo Ersek
@ 2020-01-08 14:41 ` Ard Biesheuvel
2020-01-09 13:04 ` Laszlo Ersek
0 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-01-08 14:41 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-groups-io
On Tue, 7 Jan 2020 at 16:42, Laszlo Ersek <lersek@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.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.)
>
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
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot
2020-01-08 14:13 ` Ard Biesheuvel
@ 2020-01-08 14:45 ` Laszlo Ersek
2020-01-09 0:51 ` Yao, Jiewen
0 siblings, 1 reply; 23+ messages in thread
From: Laszlo Ersek @ 2020-01-08 14:45 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-groups-io, Marc-André Lureau, Jiewen Yao
(CC Marc-André and Jiewen)
On 01/08/20 15:13, Ard Biesheuvel wrote:
> On Tue, 7 Jan 2020 at 18:37, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 01/07/20 10:48, Ard Biesheuvel wrote:
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress|0x0
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2SelfTestPolicy|1
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2ScrtmPolicy|1
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmInitializationPolicy|1
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmScrtmPolicy|1
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|3
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTcg2HashAlgorithmBitmap|3
>>
>> (3) Why is it necessary to provide dynamic defaults for these?
>>
>> Are we missing something important in OVMF, or are these specific
>> defaults that we like for ArmVirtPkg? In the latter case, I think we
>> should add them with a separate patch, as the commit message here refers
>> to OvmfPkg.
>>
>
> The policy ones can be dropped, but I see warnings like these
>
> WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC)
> WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD)
> FinalEventsTable->NumberOfEvents - 0x3
> Size - 0x15A
> SupportedEventLogs - 0x00000003
> LogFormat - 0x00000001
> LogFormat - 0x00000002
> WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC)
> WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD)
>
>
> if I leave PcdTpm2HashMask at its default value
Hmmm. My TPM2 "knowledge" is insufficient to judge and/or explain these
warnings.
Jiewen, Marc-André, can you help with this perhaps?
>>> +!if $(TPM2_ENABLE) == TRUE
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
>>> +!endif
>>
>> (4) Same as (3) -- I don't know what these do and why we need them here,
>> unlike in OvmfPkg. If they really belong here (in this patch), can you
>> explain in the commit message?
>>
>
> These are related to the TPM2 ACPI table that describes the physical
> presence interface to the OS, but I'm not even sure we can support
> this on ARM today, given that it relies on SMIs so I can drop these
> for now, I think.
"PcdTcgPhysicalPresenceInterfaceVer" is used by
SmmTcg2PhysicalPresenceLib, Tcg2ConfigDxe, and Tcg2Smm. None of those
are inclued in either OvmfPkg or ArmVirtPkg, so I think
"PcdTcgPhysicalPresenceInterfaceVer" should be dropped.
... Small correction: Tcg2ConfigDxe is included for TPM2_CONFIG_ENABLE.
For me, TPM2_CONFIG_ENABLE is uncharted (and most likely: broken)
territory. We added it in commit 3103389043bd because Stefan Berger
really wanted it -- I insisted it be sequestered with a dedicated build
flag (for "containing the damage"), and that's how we ended up with
TPM2_CONFIG_ENABLE.
Therefore, if we add PcdTcgPhysicalPresenceInterfaceVer *only* when
TPM2_CONFIG_ENABLE is TRUE, I'm fine. (I basically don't care about
TPM2_CONFIG_ENABLE==TRUE -- I wanted the dedicated flag so I can
*afford* not caring about those modules.)
Regarding "PcdTpm2AcpiTableRev": it is *consumed* by Tcg2Dxe too, so we
might want to set it, if we're not pleased with the default. But, as far
as I understand, we still only need it to be under [PcdsDynamicHii] if
we want to configure it through HII (usually: the display browser),
which is again not the case unless we have TPM2_CONFIG_ENABLE.
So in the end, I'd like to see both PCDs either removed, or made
dependent on TPM2_CONFIG_ENABLE == TRUE.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot
2020-01-08 14:45 ` Laszlo Ersek
@ 2020-01-09 0:51 ` Yao, Jiewen
2020-01-09 13:07 ` Laszlo Ersek
0 siblings, 1 reply; 23+ messages in thread
From: Yao, Jiewen @ 2020-01-09 0:51 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel; +Cc: edk2-devel-groups-io, Marc-André Lureau
Hi
Comment for the warning:
> > WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC)
> > WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD)
The reason is that: The DSC added all HASH algorithm to the TCG2 driver. (SHA1/SHA256/SHA384/SHA512/SM3).
But the current TPM hardware device does not support SHA384 (0xC) and SHA512 (0xD).
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
<LibraryClasses>
HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
}
It is warning because the Firmware Image *may* want to support another TPM2 which has such capability.
It just means the *current* TPM2 does not support this hash.
The platform owner may decide to clean up the warning by remove the SHA384/SHA512 null lib instance
support for current TPM2, or leave them as is for another TPM2.
BTW: Is there any document on how to enable TPM2 on QEMU ?
I would like to have a try. :-)
Thank you
Yao Jiewen
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, January 8, 2020 10:45 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Marc-André Lureau
> <marcandre.lureau@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for
> TPM2 measured boot
>
> (CC Marc-André and Jiewen)
>
> On 01/08/20 15:13, Ard Biesheuvel wrote:
> > On Tue, 7 Jan 2020 at 18:37, Laszlo Ersek <lersek@redhat.com> wrote:
> >> On 01/07/20 10:48, Ard Biesheuvel wrote:
>
> >>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress|0x0
> >>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1
> >>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2SelfTestPolicy|1
> >>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2ScrtmPolicy|1
> >>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmInitializationPolicy|1
> >>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmScrtmPolicy|1
> >>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|3
> >>> + gEfiSecurityPkgTokenSpaceGuid.PcdTcg2HashAlgorithmBitmap|3
> >>
> >> (3) Why is it necessary to provide dynamic defaults for these?
> >>
> >> Are we missing something important in OVMF, or are these specific
> >> defaults that we like for ArmVirtPkg? In the latter case, I think we
> >> should add them with a separate patch, as the commit message here refers
> >> to OvmfPkg.
> >>
> >
> > The policy ones can be dropped, but I see warnings like these
> >
> > WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC)
> > WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD)
> > FinalEventsTable->NumberOfEvents - 0x3
> > Size - 0x15A
> > SupportedEventLogs - 0x00000003
> > LogFormat - 0x00000001
> > LogFormat - 0x00000002
> > WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC)
> > WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD)
> >
> >
> > if I leave PcdTpm2HashMask at its default value
>
> Hmmm. My TPM2 "knowledge" is insufficient to judge and/or explain these
> warnings.
>
> Jiewen, Marc-André, can you help with this perhaps?
>
> >>> +!if $(TPM2_ENABLE) == TRUE
> >>> +
> gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_V
> ERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> >>> +
> gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTc
> g2ConfigFormSetGuid|0x8|3|NV,BS
> >>> +!endif
> >>
> >> (4) Same as (3) -- I don't know what these do and why we need them here,
> >> unlike in OvmfPkg. If they really belong here (in this patch), can you
> >> explain in the commit message?
> >>
> >
> > These are related to the TPM2 ACPI table that describes the physical
> > presence interface to the OS, but I'm not even sure we can support
> > this on ARM today, given that it relies on SMIs so I can drop these
> > for now, I think.
>
> "PcdTcgPhysicalPresenceInterfaceVer" is used by
> SmmTcg2PhysicalPresenceLib, Tcg2ConfigDxe, and Tcg2Smm. None of those
> are inclued in either OvmfPkg or ArmVirtPkg, so I think
> "PcdTcgPhysicalPresenceInterfaceVer" should be dropped.
>
> ... Small correction: Tcg2ConfigDxe is included for TPM2_CONFIG_ENABLE.
> For me, TPM2_CONFIG_ENABLE is uncharted (and most likely: broken)
> territory. We added it in commit 3103389043bd because Stefan Berger
> really wanted it -- I insisted it be sequestered with a dedicated build
> flag (for "containing the damage"), and that's how we ended up with
> TPM2_CONFIG_ENABLE.
>
> Therefore, if we add PcdTcgPhysicalPresenceInterfaceVer *only* when
> TPM2_CONFIG_ENABLE is TRUE, I'm fine. (I basically don't care about
> TPM2_CONFIG_ENABLE==TRUE -- I wanted the dedicated flag so I can
> *afford* not caring about those modules.)
>
>
> Regarding "PcdTpm2AcpiTableRev": it is *consumed* by Tcg2Dxe too, so we
> might want to set it, if we're not pleased with the default. But, as far
> as I understand, we still only need it to be under [PcdsDynamicHii] if
> we want to configure it through HII (usually: the display browser),
> which is again not the case unless we have TPM2_CONFIG_ENABLE.
>
> So in the end, I'd like to see both PCDs either removed, or made
> dependent on TPM2_CONFIG_ENABLE == TRUE.
>
> Thanks!
> Laszlo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT
2020-01-08 14:41 ` Ard Biesheuvel
@ 2020-01-09 13:04 ` Laszlo Ersek
0 siblings, 0 replies; 23+ messages in thread
From: Laszlo Ersek @ 2020-01-09 13:04 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-groups-io
On 01/08/20 15:41, Ard Biesheuvel wrote:
> On Tue, 7 Jan 2020 at 16:42, Laszlo Ersek <lersek@redhat.com> 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot
2020-01-09 0:51 ` Yao, Jiewen
@ 2020-01-09 13:07 ` Laszlo Ersek
2020-01-10 0:32 ` Yao, Jiewen
0 siblings, 1 reply; 23+ messages in thread
From: Laszlo Ersek @ 2020-01-09 13:07 UTC (permalink / raw)
To: Yao, Jiewen, Ard Biesheuvel; +Cc: edk2-devel-groups-io, Marc-André Lureau
On 01/09/20 01:51, Yao, Jiewen wrote:
> Hi
> Comment for the warning:
>>> WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC)
>>> WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD)
>
> The reason is that: The DSC added all HASH algorithm to the TCG2 driver. (SHA1/SHA256/SHA384/SHA512/SM3).
> But the current TPM hardware device does not support SHA384 (0xC) and SHA512 (0xD).
>
> SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> <LibraryClasses>
> HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> }
>
>
> It is warning because the Firmware Image *may* want to support another TPM2 which has such capability.
> It just means the *current* TPM2 does not support this hash.
> The platform owner may decide to clean up the warning by remove the SHA384/SHA512 null lib instance
> support for current TPM2, or leave them as is for another TPM2.
Thank you for the explanation!
> BTW: Is there any document on how to enable TPM2 on QEMU ?
> I would like to have a try. :-)
Please ask Marc-André (already CC'd) about vTPM usage with QEMU;
unfortunately, I don't know.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot
2020-01-09 13:07 ` Laszlo Ersek
@ 2020-01-10 0:32 ` Yao, Jiewen
2020-01-13 1:55 ` [edk2-devel] " Gary Lin
0 siblings, 1 reply; 23+ messages in thread
From: Yao, Jiewen @ 2020-01-10 0:32 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel; +Cc: edk2-devel-groups-io, Marc-André Lureau
Hi Marc-André
Would you please share some information on how to use vTPM with QEMU?
I saw https://github.com/stefanberger/qemu-tpm
But I am not sure if that has been integrated to official QEMU release?
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 9, 2020 9:07 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Marc-André Lureau
> <marcandre.lureau@redhat.com>
> Subject: Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for
> TPM2 measured boot
>
> On 01/09/20 01:51, Yao, Jiewen wrote:
> > Hi
> > Comment for the warning:
> >>> WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC)
> >>> WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD)
> >
> > The reason is that: The DSC added all HASH algorithm to the TCG2 driver.
> (SHA1/SHA256/SHA384/SHA512/SM3).
> > But the current TPM hardware device does not support SHA384 (0xC) and
> SHA512 (0xD).
> >
> > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> > <LibraryClasses>
> >
> HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRout
> erPei.inf
> > NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> >
> NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> >
> NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> >
> NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > }
> >
> >
> > It is warning because the Firmware Image *may* want to support another
> TPM2 which has such capability.
> > It just means the *current* TPM2 does not support this hash.
> > The platform owner may decide to clean up the warning by remove the
> SHA384/SHA512 null lib instance
> > support for current TPM2, or leave them as is for another TPM2.
>
> Thank you for the explanation!
>
> > BTW: Is there any document on how to enable TPM2 on QEMU ?
> > I would like to have a try. :-)
>
> Please ask Marc-André (already CC'd) about vTPM usage with QEMU;
> unfortunately, I don't know.
>
> Thanks!
> Laszlo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot
2020-01-10 0:32 ` Yao, Jiewen
@ 2020-01-13 1:55 ` Gary Lin
2020-01-13 15:56 ` Laszlo Ersek
0 siblings, 1 reply; 23+ messages in thread
From: Gary Lin @ 2020-01-13 1:55 UTC (permalink / raw)
To: devel@edk2.groups.io, jiewen.yao@intel.com
Cc: Laszlo Ersek, Ard Biesheuvel, Marc-André Lureau
On Fri, Jan 10, 2020 at 12:32:02AM +0000, Yao, Jiewen wrote:
> Hi Marc-André
> Would you please share some information on how to use vTPM with QEMU?
>
> I saw https://github.com/stefanberger/qemu-tpm
>
> But I am not sure if that has been integrated to official QEMU release?
>
Actually the TPM document can be found in the qemu package:
https://github.com/qemu/qemu/blob/master/docs/specs/tpm.txt
I also maintained a wiki page for openSUSE:
https://en.opensuse.org/Software_TPM_Emulator_For_QEMU
Hope this helps.
Cheers,
Gary Lin
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Thursday, January 9, 2020 9:07 PM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>
> > Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Marc-André Lureau
> > <marcandre.lureau@redhat.com>
> > Subject: Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for
> > TPM2 measured boot
> >
> > On 01/09/20 01:51, Yao, Jiewen wrote:
> > > Hi
> > > Comment for the warning:
> > >>> WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC)
> > >>> WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD)
> > >
> > > The reason is that: The DSC added all HASH algorithm to the TCG2 driver.
> > (SHA1/SHA256/SHA384/SHA512/SM3).
> > > But the current TPM hardware device does not support SHA384 (0xC) and
> > SHA512 (0xD).
> > >
> > > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> > > <LibraryClasses>
> > >
> > HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRout
> > erPei.inf
> > > NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > >
> > NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > >
> > NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > >
> > NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > > NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > > }
> > >
> > >
> > > It is warning because the Firmware Image *may* want to support another
> > TPM2 which has such capability.
> > > It just means the *current* TPM2 does not support this hash.
> > > The platform owner may decide to clean up the warning by remove the
> > SHA384/SHA512 null lib instance
> > > support for current TPM2, or leave them as is for another TPM2.
> >
> > Thank you for the explanation!
> >
> > > BTW: Is there any document on how to enable TPM2 on QEMU ?
> > > I would like to have a try. :-)
> >
> > Please ask Marc-André (already CC'd) about vTPM usage with QEMU;
> > unfortunately, I don't know.
> >
> > Thanks!
> > Laszlo
>
>
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot
2020-01-13 1:55 ` [edk2-devel] " Gary Lin
@ 2020-01-13 15:56 ` Laszlo Ersek
0 siblings, 0 replies; 23+ messages in thread
From: Laszlo Ersek @ 2020-01-13 15:56 UTC (permalink / raw)
To: Gary Lin, edk2-devel-groups-io, jiewen.yao@intel.com
Cc: Ard Biesheuvel, Marc-André Lureau
On 01/13/20 02:55, Gary Lin wrote:
> On Fri, Jan 10, 2020 at 12:32:02AM +0000, Yao, Jiewen wrote:
>> Hi Marc-André
>> Would you please share some information on how to use vTPM with QEMU?
>>
>> I saw https://github.com/stefanberger/qemu-tpm
>>
>> But I am not sure if that has been integrated to official QEMU release?
>>
> Actually the TPM document can be found in the qemu package:
> https://github.com/qemu/qemu/blob/master/docs/specs/tpm.txt
Ugh, I've completely forgotten that this file has a part dedicated to
starting up the swtpm program!
The text file itself is referenced near the top of
"OvmfPkg/Include/IndustryStandard/QemuTpm.h". Now that I looked again at
the text file, I only expected to see the device interface, and was
surprised by the "swtpm" instructions :)
> I also maintained a wiki page for openSUSE:
> https://en.opensuse.org/Software_TPM_Emulator_For_QEMU
Very nice.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2020-01-13 15:56 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-07 9:47 [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu Ard Biesheuvel
2020-01-07 9:47 ` [PATCH 1/4] OvmfPkg/Tcg2ConfigPei: introduce a signalling PPI to depex on Ard Biesheuvel
2020-01-07 11:58 ` Laszlo Ersek
2020-01-07 9:47 ` [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT Ard Biesheuvel
2020-01-07 15:42 ` Laszlo Ersek
2020-01-08 14:41 ` Ard Biesheuvel
2020-01-09 13:04 ` Laszlo Ersek
2020-01-07 9:47 ` [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI Ard Biesheuvel
2020-01-07 16:50 ` Laszlo Ersek
2020-01-07 16:55 ` [edk2-devel] " Ard Biesheuvel
2020-01-07 18:47 ` Laszlo Ersek
2020-01-08 9:59 ` Ard Biesheuvel
2020-01-07 9:48 ` [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot Ard Biesheuvel
2020-01-07 17:37 ` Laszlo Ersek
2020-01-08 14:13 ` Ard Biesheuvel
2020-01-08 14:45 ` Laszlo Ersek
2020-01-09 0:51 ` Yao, Jiewen
2020-01-09 13:07 ` Laszlo Ersek
2020-01-10 0:32 ` Yao, Jiewen
2020-01-13 1:55 ` [edk2-devel] " Gary Lin
2020-01-13 15:56 ` Laszlo Ersek
2020-01-07 11:55 ` [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu Laszlo Ersek
2020-01-07 12:04 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox