On 02/05/2023 12:54, Jan Beulich wrote:
On 25.04.2023 19:47, Jennifer Herbert wrote:
This patch makes the TPM version, for which the ACPI libary 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_version xenstore key.
Signed-off-by: Jennifer Herbert <jennifer.herb...@citrix.com>
---
docs/misc/xenstore-paths.pandoc | 9 +++++
tools/firmware/hvmloader/util.c | 19 ++++++---
tools/libacpi/build.c | 69 +++++++++++++++++++--------------
tools/libacpi/libacpi.h | 3 +-
4 files changed, 64 insertions(+), 36 deletions(-)
Please can you get used to providing a brief rev log somewhere here?
Yes, ok.
--- 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 |
- ACPI_HAS_WAET | ACPI_HAS_PMTIMER |
- ACPI_HAS_BUTTONS | ACPI_HAS_VGA |
- ACPI_HAS_8042 | ACPI_HAS_CMOS_RTC);
+ config->table_flags |= (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", "1");
+ config->tpm_version = strtoll(s, NULL, 0);
Due to field width, someone specifying 257 will also get a 1.2 TPM,
if I'm not mistaken.
Seems likely. And i few other wacky values would give you 1.2 as well
I'd think. There could also be trailing junk on the version number.
I was a bit phased by the lack of any real error cases in
hvmloader_acpi_build_tables. It seemed the approch was if you put in
junk, you'll get something, but possibly not what your expecting.
Do I take it you'd prefer it to only accept a strict '1' for 1.2 and any
other value would result in no TPM being probed? Or is it only the
overflow cases your concerned about?
+ switch( config->tpm_version )
Nit: Style (missing blank).
yup
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -409,38 +409,47 @@ 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 its 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 0: /* Assume legacy code wanted tpm 1.2 */
Along the lines of what Jason said: Unless this is known to be needed for
anything, I'd prefer if it was omitted.
I'm not awair of anything, but your comment 2 lines down from version 2
made me think you knew of some. So if your happy with me removing this
line, I am!
Jan
