| Message ID | 20230425174733.795961-3-jennifer.herbert@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | acpi: Make TPM version configurable. | expand |
On Tue, Apr 25, 2023 at 1:48 PM Jennifer Herbert <jennifer.herbert@citrix.com> wrote: > > This patch introduces an optional TPM 2 interface definition to the ACPI table, > which is to be used as part of a vTPM 2 implementation. > > Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com> > --- ... > diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c > index f39a8e584f..51272530fe 100644 > --- a/tools/firmware/hvmloader/util.c > +++ b/tools/firmware/hvmloader/util.c > @@ -1009,6 +1009,15 @@ void hvmloader_acpi_build_tables(struct acpi_config *config, > config->table_flags |= ACPI_HAS_TPM; > config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS; > break; > + > + case 2: > + config->table_flags |= ACPI_HAS_TPM; > + config->crb_id = (uint16_t *)TPM_CRB_INTF_ID; > + > + mem_hole_populate_ram(TPM_LOG_AREA_ADDRESS >> PAGE_SHIFT, > + TPM_LOG_SIZE >> PAGE_SHIFT); > + memset((void *)TPM_LOG_AREA_ADDRESS, 0, TPM_LOG_SIZE); TPM_LOG_AREA_ADDRESS is reserved in the e820 table since it is the high memory range after the ACPI data, correct? Reviewed-by: Jason Andryuk <jandryuk@gmail.com> Thanks, Jason
On 26/04/2023 21:29, Jason Andryuk wrote: > On Tue, Apr 25, 2023 at 1:48 PM Jennifer Herbert > <jennifer.herbert@citrix.com> wrote: >> This patch introduces an optional TPM 2 interface definition to the ACPI table, >> which is to be used as part of a vTPM 2 implementation. >> >> Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com> >> --- > ... >> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c >> index f39a8e584f..51272530fe 100644 >> --- a/tools/firmware/hvmloader/util.c >> +++ b/tools/firmware/hvmloader/util.c >> @@ -1009,6 +1009,15 @@ void hvmloader_acpi_build_tables(struct acpi_config *config, >> config->table_flags |= ACPI_HAS_TPM; >> config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS; >> break; >> + >> + case 2: >> + config->table_flags |= ACPI_HAS_TPM; >> + config->crb_id = (uint16_t *)TPM_CRB_INTF_ID; >> + >> + mem_hole_populate_ram(TPM_LOG_AREA_ADDRESS >> PAGE_SHIFT, >> + TPM_LOG_SIZE >> PAGE_SHIFT); >> + memset((void *)TPM_LOG_AREA_ADDRESS, 0, TPM_LOG_SIZE); > TPM_LOG_AREA_ADDRESS is reserved in the e820 table since it is the > high memory range after the ACPI data, correct? This is my understanding yes. We made sure to put it well clear of the qemu impremnted TPM, just incase it later decided to support more localitie levels, but still well within the RESERVED area, in the e820. -jenny > Reviewed-by: Jason Andryuk <jandryuk@gmail.com> > > Thanks, > Jason >
On 25.04.2023 19:47, Jennifer Herbert wrote: > --- a/tools/libacpi/acpi2_0.h > +++ b/tools/libacpi/acpi2_0.h > @@ -121,6 +121,36 @@ struct acpi_20_tcpa { > }; > #define ACPI_2_0_TCPA_LAML_SIZE (64*1024) > > +/* > + * TPM2 > + */ Nit: While I'm willing to accept the comment style violation here as (apparently) intentional, ... > +struct acpi_20_tpm2 { > + struct acpi_header header; > + uint16_t platform_class; > + uint16_t reserved; > + uint64_t control_area_address; > + uint32_t start_method; > + uint8_t start_method_params[12]; > + uint32_t log_area_minimum_length; > + uint64_t log_area_start_address; > +}; > +#define TPM2_ACPI_CLASS_CLIENT 0 > +#define TPM2_START_METHOD_CRB 7 > + > +/* TPM register I/O Mapped region, location of which defined in the > + * TCG PC Client Platform TPM Profile Specification for TPM 2.0. > + * See table 9 - Only Locality 0 is used here. This is emulated by QEMU. > + * Definition of Register space is found in table 12. > + */ ... this comment wants adjusting to hypervisor style (/* on its own line), as that looks to be the aimed-at style in this file. > @@ -352,6 +353,7 @@ static int construct_secondary_tables(struct acpi_ctxt *ctxt, > struct acpi_20_tcpa *tcpa; > unsigned char *ssdt; > void *lasa; > + struct acpi_20_tpm2 *tpm2; Could I talk you into moving this up by two lines, such that it'll be adjacent to "tcpa"? > @@ -450,6 +452,43 @@ static int construct_secondary_tables(struct acpi_ctxt *ctxt, > tcpa->header.length); > } > break; > + > + case 2: > + /* Check VID stored in bits 37:32 (3rd 16 bit word) of CRB > + * identifier register. See table 16 of TCG PC client platform > + * TPM profile specification for TPM 2.0. > + */ Nit: This comment again wants a style adjustment. > --- /dev/null > +++ b/tools/libacpi/ssdt_tpm2.asl > @@ -0,0 +1,36 @@ > +/* > + * ssdt_tpm2.asl > + * > + * Copyright (c) 2018-2022, Citrix Systems, Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU Lesser General Public License as published > + * by the Free Software Foundation; version 2.1 only. with the special > + * exception on linking described in file LICENSE. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU Lesser General Public License for more details. > + */ While the full conversion to SPDX was done in the hypervisor only so far, I think new tool stack source files would better use the much shorter SPDX equivalent, too. Then on top of Jason's R-b, Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On 02/05/2023 14:41, Jan Beulich wrote: > On 25.04.2023 19:47, Jennifer Herbert wrote: >> --- a/tools/libacpi/acpi2_0.h >> +++ b/tools/libacpi/acpi2_0.h >> @@ -121,6 +121,36 @@ struct acpi_20_tcpa { >> }; >> #define ACPI_2_0_TCPA_LAML_SIZE (64*1024) >> >> +/* >> + * TPM2 >> + */ > Nit: While I'm willing to accept the comment style violation here as > (apparently) intentional, ... Well, I was trying to keep the file consistant. As far as I can tell, this styling is used thoughout the file - unless I'm misunderstanding your 'Nit'. (You object to a multi-line coment used for a single line? ) But I'm codes style blind, so just say how you want it. >> +struct acpi_20_tpm2 { >> + struct acpi_header header; >> + uint16_t platform_class; >> + uint16_t reserved; >> + uint64_t control_area_address; >> + uint32_t start_method; >> + uint8_t start_method_params[12]; >> + uint32_t log_area_minimum_length; >> + uint64_t log_area_start_address; >> +}; >> +#define TPM2_ACPI_CLASS_CLIENT 0 >> +#define TPM2_START_METHOD_CRB 7 >> + >> +/* TPM register I/O Mapped region, location of which defined in the >> + * TCG PC Client Platform TPM Profile Specification for TPM 2.0. >> + * See table 9 - Only Locality 0 is used here. This is emulated by QEMU. >> + * Definition of Register space is found in table 12. >> + */ > ... this comment wants adjusting to hypervisor style (/* on its own line), > as that looks to be the aimed-at style in this file. Will do. >> @@ -352,6 +353,7 @@ static int construct_secondary_tables(struct acpi_ctxt *ctxt, >> struct acpi_20_tcpa *tcpa; >> unsigned char *ssdt; >> void *lasa; >> + struct acpi_20_tpm2 *tpm2; > Could I talk you into moving this up by two lines, such that it'll be > adjacent to "tcpa"? No problem. >> @@ -450,6 +452,43 @@ static int construct_secondary_tables(struct acpi_ctxt *ctxt, >> tcpa->header.length); >> } >> break; >> + >> + case 2: >> + /* Check VID stored in bits 37:32 (3rd 16 bit word) of CRB >> + * identifier register. See table 16 of TCG PC client platform >> + * TPM profile specification for TPM 2.0. >> + */ > Nit: This comment again wants a style adjustment. ok >> --- /dev/null >> +++ b/tools/libacpi/ssdt_tpm2.asl >> @@ -0,0 +1,36 @@ >> +/* >> + * ssdt_tpm2.asl >> + * >> + * Copyright (c) 2018-2022, Citrix Systems, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU Lesser General Public License as published >> + * by the Free Software Foundation; version 2.1 only. with the special >> + * exception on linking described in file LICENSE. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU Lesser General Public License for more details. >> + */ > While the full conversion to SPDX was done in the hypervisor only so far, > I think new tool stack source files would better use the much shorter > SPDX equivalent, too. OK, this is where I get a bit confused. I belive I copied the licence from ssdt_tpm.asl, for consistancy. So I think i need to use a 'LGPL-2.1-only' but then it says its using exceptions on linking as discribed in LICENSE, but um, which LICENSE file? So i'm not sure what exception I should be adding. Do you know? > Then on top of Jason's R-b, > Acked-by: Jan Beulich <jbeulich@suse.com> > > Jan Thanks, -jenny
On 03.05.2023 01:29, Jennifer Herbert wrote: > On 02/05/2023 14:41, Jan Beulich wrote: >> On 25.04.2023 19:47, Jennifer Herbert wrote: >>> --- a/tools/libacpi/acpi2_0.h >>> +++ b/tools/libacpi/acpi2_0.h >>> @@ -121,6 +121,36 @@ struct acpi_20_tcpa { >>> }; >>> #define ACPI_2_0_TCPA_LAML_SIZE (64*1024) >>> >>> +/* >>> + * TPM2 >>> + */ >> Nit: While I'm willing to accept the comment style violation here as >> (apparently) intentional, ... > > Well, I was trying to keep the file consistant. As far as I can tell, > this styling is used thoughout the file - unless I'm misunderstanding > your 'Nit'. (You object to a multi-line coment used for a single line? ) > But I'm codes style blind, so just say how you want it. Right - strictly speaking those multi-line comments all ought to be single- line ones, but aiui they're multi-line intentionally so they stand out. Hence - as you say, for consistency - it's okay for this one to follow suit. >>> --- /dev/null >>> +++ b/tools/libacpi/ssdt_tpm2.asl >>> @@ -0,0 +1,36 @@ >>> +/* >>> + * ssdt_tpm2.asl >>> + * >>> + * Copyright (c) 2018-2022, Citrix Systems, Inc. >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU Lesser General Public License as published >>> + * by the Free Software Foundation; version 2.1 only. with the special >>> + * exception on linking described in file LICENSE. >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> + * GNU Lesser General Public License for more details. >>> + */ >> While the full conversion to SPDX was done in the hypervisor only so far, >> I think new tool stack source files would better use the much shorter >> SPDX equivalent, too. > > OK, this is where I get a bit confused. I belive I copied the licence > from ssdt_tpm.asl, for consistancy. > > So I think i need to use a 'LGPL-2.1-only' but then it says its using > exceptions on linking as discribed in LICENSE, but um, which LICENSE > file? So i'm not sure what exception I should be adding. Do you know? First of all I think commit 68823df358e8 ("acpi: Re-license ACPI builder files from GPLv2 to LGPLv2.1") was wrong using LICENSE; I'm pretty sure COPYING was meant instead. And indeed the difference between libacpi's COPYING and LICENCES/LGPL-2.1 look to be formatting plus an extra section at the bottom of the latter; I haven't found any "special exception on linking" anywhere. IOW I think using LGPL-2.1 here is what is wanted (unlike e.g. GPL-2.0-only there's no LGPL-2.1-only afaics), the more that you're contributing a new file (and of course provided you're okay to put the new file under that license). Jan
diff --git a/docs/misc/xenstore-paths.pandoc b/docs/misc/xenstore-paths.pandoc index e67e164855..bffb8ea544 100644 --- a/docs/misc/xenstore-paths.pandoc +++ b/docs/misc/xenstore-paths.pandoc @@ -274,7 +274,8 @@ circumstances where the generation ID needs to be changed. The TPM version to be probed for. -A value of 1 indicates to probe for TPM 1.2. +A value of 1 indicates to probe for TPM 1.2, whereas a value of 2 +indicates that a TPM 2.0 using CRB should be probed. A value of 0 or an invalid value will result in no TPM being probed. If unset, a default of 1 is assumed. diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c index f39a8e584f..51272530fe 100644 --- a/tools/firmware/hvmloader/util.c +++ b/tools/firmware/hvmloader/util.c @@ -1009,6 +1009,15 @@ void hvmloader_acpi_build_tables(struct acpi_config *config, config->table_flags |= ACPI_HAS_TPM; config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS; break; + + case 2: + config->table_flags |= ACPI_HAS_TPM; + config->crb_id = (uint16_t *)TPM_CRB_INTF_ID; + + mem_hole_populate_ram(TPM_LOG_AREA_ADDRESS >> PAGE_SHIFT, + TPM_LOG_SIZE >> PAGE_SHIFT); + memset((void *)TPM_LOG_AREA_ADDRESS, 0, TPM_LOG_SIZE); + break; } config->numa.nr_vmemranges = nr_vmemranges; diff --git a/tools/libacpi/Makefile b/tools/libacpi/Makefile index 60860eaa00..23278f6a61 100644 --- a/tools/libacpi/Makefile +++ b/tools/libacpi/Makefile @@ -25,7 +25,8 @@ C_SRC-$(CONFIG_X86) = dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c dsdt_pvh C_SRC-$(CONFIG_ARM_64) = dsdt_anycpu_arm.c DSDT_FILES ?= $(C_SRC-y) C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, $(DSDT_FILES)) -H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h ssdt_laptop_slate.h) +H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h) +H_SRC += $(addprefix $(ACPI_BUILD_DIR)/, ssdt_tpm.h ssdt_tpm2.h ssdt_laptop_slate.h) MKDSDT_CFLAGS-$(CONFIG_ARM_64) = -DCONFIG_ARM_64 MKDSDT_CFLAGS-$(CONFIG_X86) = -DCONFIG_X86 diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h index 2619ba32db..19a43d4b2e 100644 --- a/tools/libacpi/acpi2_0.h +++ b/tools/libacpi/acpi2_0.h @@ -121,6 +121,36 @@ struct acpi_20_tcpa { }; #define ACPI_2_0_TCPA_LAML_SIZE (64*1024) +/* + * TPM2 + */ +struct acpi_20_tpm2 { + struct acpi_header header; + uint16_t platform_class; + uint16_t reserved; + uint64_t control_area_address; + uint32_t start_method; + uint8_t start_method_params[12]; + uint32_t log_area_minimum_length; + uint64_t log_area_start_address; +}; +#define TPM2_ACPI_CLASS_CLIENT 0 +#define TPM2_START_METHOD_CRB 7 + +/* TPM register I/O Mapped region, location of which defined in the + * TCG PC Client Platform TPM Profile Specification for TPM 2.0. + * See table 9 - Only Locality 0 is used here. This is emulated by QEMU. + * Definition of Register space is found in table 12. + */ +#define TPM_REGISTER_BASE 0xFED40000 +#define TPM_CRB_CTRL_REQ (TPM_REGISTER_BASE + 0x40) +#define TPM_CRB_INTF_ID (TPM_REGISTER_BASE + 0x30) + +#define TPM_LOG_AREA_ADDRESS 0xFED50000 + +#define TPM_LOG_AREA_MINIMUM_SIZE (64 << 10) +#define TPM_LOG_SIZE (64 << 10) + /* * Fixed ACPI Description Table Structure (FADT) in ACPI 1.0. */ @@ -431,6 +461,7 @@ struct acpi_20_slit { #define ACPI_2_0_RSDT_SIGNATURE ASCII32('R','S','D','T') #define ACPI_2_0_XSDT_SIGNATURE ASCII32('X','S','D','T') #define ACPI_2_0_TCPA_SIGNATURE ASCII32('T','C','P','A') +#define ACPI_2_0_TPM2_SIGNATURE ASCII32('T','P','M','2') #define ACPI_2_0_HPET_SIGNATURE ASCII32('H','P','E','T') #define ACPI_2_0_WAET_SIGNATURE ASCII32('W','A','E','T') #define ACPI_2_0_SRAT_SIGNATURE ASCII32('S','R','A','T') @@ -444,6 +475,7 @@ struct acpi_20_slit { #define ACPI_2_0_RSDT_REVISION 0x01 #define ACPI_2_0_XSDT_REVISION 0x01 #define ACPI_2_0_TCPA_REVISION 0x02 +#define ACPI_2_0_TPM2_REVISION 0x04 #define ACPI_2_0_HPET_REVISION 0x01 #define ACPI_2_0_WAET_REVISION 0x01 #define ACPI_1_0_FADT_REVISION 0x01 diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c index 716cb49624..359a4dbba4 100644 --- a/tools/libacpi/build.c +++ b/tools/libacpi/build.c @@ -19,6 +19,7 @@ #include "ssdt_s3.h" #include "ssdt_s4.h" #include "ssdt_tpm.h" +#include "ssdt_tpm2.h" #include "ssdt_pm.h" #include "ssdt_laptop_slate.h" #include <xen/hvm/hvm_info_table.h> @@ -352,6 +353,7 @@ static int construct_secondary_tables(struct acpi_ctxt *ctxt, struct acpi_20_tcpa *tcpa; unsigned char *ssdt; void *lasa; + struct acpi_20_tpm2 *tpm2; /* MADT. */ if ( (config->hvminfo->nr_vcpus > 1) || config->hvminfo->apic_mode ) @@ -450,6 +452,43 @@ static int construct_secondary_tables(struct acpi_ctxt *ctxt, tcpa->header.length); } break; + + case 2: + /* Check VID stored in bits 37:32 (3rd 16 bit word) of CRB + * identifier register. See table 16 of TCG PC client platform + * TPM profile specification for TPM 2.0. + */ + if ( config->crb_id[2] == 0 || config->crb_id[2] == 0xffff ) + break; + + ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_tpm2), 16); + if (!ssdt) return -1; + memcpy(ssdt, ssdt_tpm2, sizeof(ssdt_tpm2)); + table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt); + + tpm2 = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_tpm2), 16); + if (!tpm2) return -1; + memset(tpm2, 0, sizeof(*tpm2)); + table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, tpm2); + + tpm2->header.signature = ACPI_2_0_TPM2_SIGNATURE; + tpm2->header.length = sizeof(*tpm2); + tpm2->header.revision = ACPI_2_0_TPM2_REVISION; + fixed_strcpy(tpm2->header.oem_id, ACPI_OEM_ID); + fixed_strcpy(tpm2->header.oem_table_id, ACPI_OEM_TABLE_ID); + tpm2->header.oem_revision = ACPI_OEM_REVISION; + tpm2->header.creator_id = ACPI_CREATOR_ID; + tpm2->header.creator_revision = ACPI_CREATOR_REVISION; + tpm2->platform_class = TPM2_ACPI_CLASS_CLIENT; + tpm2->control_area_address = TPM_CRB_CTRL_REQ; + tpm2->start_method = TPM2_START_METHOD_CRB; + tpm2->log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE; + tpm2->log_area_start_address = TPM_LOG_AREA_ADDRESS; + + set_checksum(tpm2, + offsetof(struct acpi_header, checksum), + tpm2->header.length); + break; } } diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h index f69452401f..0d19f9fc4d 100644 --- a/tools/libacpi/libacpi.h +++ b/tools/libacpi/libacpi.h @@ -80,6 +80,7 @@ struct acpi_config { const struct hvm_info_table *hvminfo; const uint16_t *tis_hdr; + const uint16_t *crb_id; /* * Address where acpi_info should be placed. diff --git a/tools/libacpi/ssdt_tpm2.asl b/tools/libacpi/ssdt_tpm2.asl new file mode 100644 index 0000000000..1801c338df --- /dev/null +++ b/tools/libacpi/ssdt_tpm2.asl @@ -0,0 +1,36 @@ +/* + * ssdt_tpm2.asl + * + * Copyright (c) 2018-2022, Citrix Systems, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published + * by the Free Software Foundation; version 2.1 only. with the special + * exception on linking described in file LICENSE. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + */ + +/* SSDT for TPM CRB Interface for Xen with Qemu device model. */ + +DefinitionBlock ("SSDT_TPM2.aml", "SSDT", 2, "Xen", "HVM", 0) +{ + Device (TPM) + { + Name (_HID, "MSFT0101" /* TPM 2.0 Security Device */) // _HID: Hardware ID + Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings + { + Memory32Fixed (ReadWrite, + 0xFED40000, // Address Base + 0x00001000, // Address Length + ) + }) + Method (_STA, 0, NotSerialized) // _STA: Status + { + Return (0x0F) + } + } +}
This patch introduces an optional TPM 2 interface definition to the ACPI table, which is to be used as part of a vTPM 2 implementation. Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com> --- docs/misc/xenstore-paths.pandoc | 3 ++- tools/firmware/hvmloader/util.c | 9 ++++++++ tools/libacpi/Makefile | 3 ++- tools/libacpi/acpi2_0.h | 32 +++++++++++++++++++++++++++ tools/libacpi/build.c | 39 +++++++++++++++++++++++++++++++++ tools/libacpi/libacpi.h | 1 + tools/libacpi/ssdt_tpm2.asl | 36 ++++++++++++++++++++++++++++++ 7 files changed, 121 insertions(+), 2 deletions(-) create mode 100644 tools/libacpi/ssdt_tpm2.asl