| Message ID | 20221215170934.123889-2-jennifer.herbert@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi: Make TPM version configurable. | expand |
On 15.12.2022 18:09, Jennifer Herbert wrote: > --- a/docs/misc/xenstore-paths.pandoc > +++ b/docs/misc/xenstore-paths.pandoc > @@ -269,6 +269,14 @@ at the guest physical address in HVM_PARAM_VM_GENERATION_ID_ADDR. > See Microsoft's "Virtual Machine Generation ID" specification for the > circumstances where the generation ID needs to be changed. > > + > +#### ~/platform/tpm_version = INTEGER [HVM,INTERNAL] > + > +The TPM version to be probed for. > + > +A value of 1 indicates to probe for TPM 1.2. If unset, or an > +invalid version, then no TPM is probed. And who's writing this new key? Aren't you regressing existing guests by defaulting to "no TPM" in the absence of this key? > --- a/tools/firmware/hvmloader/util.c > +++ b/tools/firmware/hvmloader/util.c > @@ -994,13 +994,22 @@ void hvmloader_acpi_build_tables(struct acpi_config *config, > if ( !strncmp(xenstore_read("platform/acpi_laptop_slate", "0"), "1", 1) ) > config->table_flags |= ACPI_HAS_SSDT_LAPTOP_SLATE; > > - config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC | > + config->table_flags |= (ACPI_HAS_TPM | ACPI_HAS_IOAPIC | I'm puzzled by ACPI_HAS_TPM being present both here and ... > ACPI_HAS_WAET | ACPI_HAS_PMTIMER | > ACPI_HAS_BUTTONS | ACPI_HAS_VGA | > ACPI_HAS_8042 | ACPI_HAS_CMOS_RTC); > config->acpi_revision = 4; > > - config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS; > + s = xenstore_read("platform/tpm_version", "0"); > + config->tpm_version = strtoll(s, NULL, 0); > + > + switch( config->tpm_version ) > + { > + case 1: > + config->table_flags |= ACPI_HAS_TPM; .. here. Also (nit): Missing blank after "switch". > --- a/tools/libacpi/build.c > +++ b/tools/libacpi/build.c > @@ -409,38 +409,46 @@ static int construct_secondary_tables(struct acpi_ctxt *ctxt, > memcpy(ssdt, ssdt_laptop_slate, sizeof(ssdt_laptop_slate)); > table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt); > } > - > - /* TPM TCPA and SSDT. */ > - if ( (config->table_flags & ACPI_HAS_TCPA) && > - (config->tis_hdr[0] != 0 && config->tis_hdr[0] != 0xffff) && > - (config->tis_hdr[1] != 0 && config->tis_hdr[1] != 0xffff) ) > + /* TPM and SSDT. */ May I ask that since you're altering the comment, you make it "TPM and its SSDT"? > + if (config->table_flags & ACPI_HAS_TPM) Nit: The file is predominantly using Xen style, so please adhere to that (also again further down). > { > - ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_tpm), 16); > - if (!ssdt) return -1; > - memcpy(ssdt, ssdt_tpm, sizeof(ssdt_tpm)); > - table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt); > - > - tcpa = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_tcpa), 16); > - if (!tcpa) return -1; > - memset(tcpa, 0, sizeof(*tcpa)); > - table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, tcpa); > - > - tcpa->header.signature = ACPI_2_0_TCPA_SIGNATURE; > - tcpa->header.length = sizeof(*tcpa); > - tcpa->header.revision = ACPI_2_0_TCPA_REVISION; > - fixed_strcpy(tcpa->header.oem_id, ACPI_OEM_ID); > - fixed_strcpy(tcpa->header.oem_table_id, ACPI_OEM_TABLE_ID); > - tcpa->header.oem_revision = ACPI_OEM_REVISION; > - tcpa->header.creator_id = ACPI_CREATOR_ID; > - tcpa->header.creator_revision = ACPI_CREATOR_REVISION; > - if ( (lasa = ctxt->mem_ops.alloc(ctxt, ACPI_2_0_TCPA_LAML_SIZE, 16)) != NULL ) > + switch (config->tpm_version) > { > - tcpa->lasa = ctxt->mem_ops.v2p(ctxt, lasa); > - tcpa->laml = ACPI_2_0_TCPA_LAML_SIZE; > - memset(lasa, 0, tcpa->laml); > - set_checksum(tcpa, > - offsetof(struct acpi_header, checksum), > - tcpa->header.length); > + case 1: > + if (!config->tis_hdr || There was no such check in the original code, and I think it would better remain the case that the field is set if and only if ACPI_HAS_TPM is set and (now) version is 1. (Aiui this check actually covers for the double setting of ACPI_HAS_TPM commented on above.) > + config->tis_hdr[0] == 0 || config->tis_hdr[0] == 0xffff || > + config->tis_hdr[1] == 0 || config->tis_hdr[1] == 0xffff) > + break; > + > + ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_tpm), 16); > + if (!ssdt) return -1; > + memcpy(ssdt, ssdt_tpm, sizeof(ssdt_tpm)); > + table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt); > + > + tcpa = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_tcpa), 16); > + if (!tcpa) return -1; > + memset(tcpa, 0, sizeof(*tcpa)); > + table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, tcpa); > + > + tcpa->header.signature = ACPI_2_0_TCPA_SIGNATURE; > + tcpa->header.length = sizeof(*tcpa); > + tcpa->header.revision = ACPI_2_0_TCPA_REVISION; > + fixed_strcpy(tcpa->header.oem_id, ACPI_OEM_ID); > + fixed_strcpy(tcpa->header.oem_table_id, ACPI_OEM_TABLE_ID); > + tcpa->header.oem_revision = ACPI_OEM_REVISION; > + tcpa->header.creator_id = ACPI_CREATOR_ID; > + tcpa->header.creator_revision = ACPI_CREATOR_REVISION; > + > + if ( (lasa = ctxt->mem_ops.alloc(ctxt, ACPI_2_0_TCPA_LAML_SIZE, 16)) != NULL ) Nit: Please wrap this line (it was too long before, but it's getting worse with the re-indentation). > @@ -78,8 +78,8 @@ struct acpi_config { > struct acpi_numa numa; > const struct hvm_info_table *hvminfo; > > + uint8_t tpm_version; > const uint16_t *tis_hdr; Hmm, sticking a uint8_t between two pointers is kind of inefficient. How about putting it next to acpi_revision and, if so desired to keep related fields together, moving up the tis_hdr field? > - > /* > * Address where acpi_info should be placed. Please don't remove blank lines like the one here, the more when they're unrelated. Jan
diff --git a/docs/misc/xenstore-paths.pandoc b/docs/misc/xenstore-paths.pandoc index 5cd5c8a3b9..7270b46721 100644 --- a/docs/misc/xenstore-paths.pandoc +++ b/docs/misc/xenstore-paths.pandoc @@ -269,6 +269,14 @@ at the guest physical address in HVM_PARAM_VM_GENERATION_ID_ADDR. See Microsoft's "Virtual Machine Generation ID" specification for the circumstances where the generation ID needs to be changed. + +#### ~/platform/tpm_version = INTEGER [HVM,INTERNAL] + +The TPM version to be probed for. + +A value of 1 indicates to probe for TPM 1.2. If unset, or an +invalid version, then no TPM is probed. + ### Frontend device paths Paravirtual device frontends are generally specified by their own diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c index 581b35e5cf..87bc2d677f 100644 --- a/tools/firmware/hvmloader/util.c +++ b/tools/firmware/hvmloader/util.c @@ -994,13 +994,22 @@ void hvmloader_acpi_build_tables(struct acpi_config *config, if ( !strncmp(xenstore_read("platform/acpi_laptop_slate", "0"), "1", 1) ) config->table_flags |= ACPI_HAS_SSDT_LAPTOP_SLATE; - config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC | + config->table_flags |= (ACPI_HAS_TPM | ACPI_HAS_IOAPIC | ACPI_HAS_WAET | ACPI_HAS_PMTIMER | ACPI_HAS_BUTTONS | ACPI_HAS_VGA | ACPI_HAS_8042 | ACPI_HAS_CMOS_RTC); config->acpi_revision = 4; - config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS; + s = xenstore_read("platform/tpm_version", "0"); + config->tpm_version = strtoll(s, NULL, 0); + + switch( config->tpm_version ) + { + case 1: + config->table_flags |= ACPI_HAS_TPM; + config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS; + break; + } config->numa.nr_vmemranges = nr_vmemranges; config->numa.nr_vnodes = nr_vnodes; diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c index fe2db66a62..d313ccd8cf 100644 --- a/tools/libacpi/build.c +++ b/tools/libacpi/build.c @@ -409,38 +409,46 @@ static int construct_secondary_tables(struct acpi_ctxt *ctxt, memcpy(ssdt, ssdt_laptop_slate, sizeof(ssdt_laptop_slate)); table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt); } - - /* TPM TCPA and SSDT. */ - if ( (config->table_flags & ACPI_HAS_TCPA) && - (config->tis_hdr[0] != 0 && config->tis_hdr[0] != 0xffff) && - (config->tis_hdr[1] != 0 && config->tis_hdr[1] != 0xffff) ) + /* TPM and SSDT. */ + if (config->table_flags & ACPI_HAS_TPM) { - ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_tpm), 16); - if (!ssdt) return -1; - memcpy(ssdt, ssdt_tpm, sizeof(ssdt_tpm)); - table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt); - - tcpa = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_tcpa), 16); - if (!tcpa) return -1; - memset(tcpa, 0, sizeof(*tcpa)); - table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, tcpa); - - tcpa->header.signature = ACPI_2_0_TCPA_SIGNATURE; - tcpa->header.length = sizeof(*tcpa); - tcpa->header.revision = ACPI_2_0_TCPA_REVISION; - fixed_strcpy(tcpa->header.oem_id, ACPI_OEM_ID); - fixed_strcpy(tcpa->header.oem_table_id, ACPI_OEM_TABLE_ID); - tcpa->header.oem_revision = ACPI_OEM_REVISION; - tcpa->header.creator_id = ACPI_CREATOR_ID; - tcpa->header.creator_revision = ACPI_CREATOR_REVISION; - if ( (lasa = ctxt->mem_ops.alloc(ctxt, ACPI_2_0_TCPA_LAML_SIZE, 16)) != NULL ) + switch (config->tpm_version) { - tcpa->lasa = ctxt->mem_ops.v2p(ctxt, lasa); - tcpa->laml = ACPI_2_0_TCPA_LAML_SIZE; - memset(lasa, 0, tcpa->laml); - set_checksum(tcpa, - offsetof(struct acpi_header, checksum), - tcpa->header.length); + case 1: + if (!config->tis_hdr || + config->tis_hdr[0] == 0 || config->tis_hdr[0] == 0xffff || + config->tis_hdr[1] == 0 || config->tis_hdr[1] == 0xffff) + break; + + ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_tpm), 16); + if (!ssdt) return -1; + memcpy(ssdt, ssdt_tpm, sizeof(ssdt_tpm)); + table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt); + + tcpa = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_tcpa), 16); + if (!tcpa) return -1; + memset(tcpa, 0, sizeof(*tcpa)); + table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, tcpa); + + tcpa->header.signature = ACPI_2_0_TCPA_SIGNATURE; + tcpa->header.length = sizeof(*tcpa); + tcpa->header.revision = ACPI_2_0_TCPA_REVISION; + fixed_strcpy(tcpa->header.oem_id, ACPI_OEM_ID); + fixed_strcpy(tcpa->header.oem_table_id, ACPI_OEM_TABLE_ID); + tcpa->header.oem_revision = ACPI_OEM_REVISION; + tcpa->header.creator_id = ACPI_CREATOR_ID; + tcpa->header.creator_revision = ACPI_CREATOR_REVISION; + + if ( (lasa = ctxt->mem_ops.alloc(ctxt, ACPI_2_0_TCPA_LAML_SIZE, 16)) != NULL ) + { + tcpa->lasa = ctxt->mem_ops.v2p(ctxt, lasa); + tcpa->laml = ACPI_2_0_TCPA_LAML_SIZE; + memset(lasa, 0, tcpa->laml); + set_checksum(tcpa, + offsetof(struct acpi_header, checksum), + tcpa->header.length); + } + break; } } diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h index a2efd23b0b..9143616130 100644 --- a/tools/libacpi/libacpi.h +++ b/tools/libacpi/libacpi.h @@ -27,7 +27,7 @@ #define ACPI_HAS_SSDT_PM (1<<4) #define ACPI_HAS_SSDT_S3 (1<<5) #define ACPI_HAS_SSDT_S4 (1<<6) -#define ACPI_HAS_TCPA (1<<7) +#define ACPI_HAS_TPM (1<<7) #define ACPI_HAS_IOAPIC (1<<8) #define ACPI_HAS_WAET (1<<9) #define ACPI_HAS_PMTIMER (1<<10) @@ -78,8 +78,8 @@ struct acpi_config { struct acpi_numa numa; const struct hvm_info_table *hvminfo; + uint8_t tpm_version; const uint16_t *tis_hdr; - /* * Address where acpi_info should be placed. * This must match the OperationRegion(BIOS, SystemMemory, ....)
This patch makes the TPM version, for which the ACPI library probes, configurable. If acpi_config.tpm_verison is set to 1, it indicates that 1.2 (TCPA) should be probed. I have also added to hvmloader an option to allow setting this new config, which can be triggered by setting the platform/tpm_verion xenstore key. Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com> --- docs/misc/xenstore-paths.pandoc | 8 ++++ tools/firmware/hvmloader/util.c | 13 ++++++- tools/libacpi/build.c | 68 ++++++++++++++++++--------------- tools/libacpi/libacpi.h | 4 +- 4 files changed, 59 insertions(+), 34 deletions(-)