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(-)
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, ....)
--
2.31.1On 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
© 2016 - 2024 Red Hat, Inc.