[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [for-4.17 PATCH] acpi: Add TPM2 interface definition and make the TPM version configurable.
On 30/08/2022 21:27, Jennifer Herbert 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. > To enable the new interface - I have made the TPM interface version > configurable in the acpi_config, with the default being the existing > 1.2.(TCPA) > I have also added to hvmloader an option to utilise this new config, which can > be triggered by setting the platform/tpm_verion xenstore key. > > Signed-off-by: Jennifer Herbert <jennifer.herbert@xxxxxxxxxx> We're past the 4.17 feature submission deadline. CC'ing Henry. Henry: This is a fairly simple change and a critical building block for getting Windows 11 support on Xen. Given that feature freeze was slipped several weeks for other reasons, this should be considered for inclusion too. Jenny: This needs splitting up into at least 2 patches. Patch 1 should be the rename of ACPI_HAS_{TCPA => TPM} and introduction of tpm_version (inc suitable rearranging). Patch 2 should be the introduction of TPM2. > diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c > index 581b35e5cf..e3af32581b 100644 > --- a/tools/firmware/hvmloader/util.c > +++ b/tools/firmware/hvmloader/util.c > @@ -994,13 +994,24 @@ 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; > + if ( !strncmp(xenstore_read("platform/tpm_version", "0"), "2", 1) ) { Brace on new line. Also, this is a new key, so needs an entry in docs/misc/xenstore-paths.pandoc > + > + config->tpm_version = 2; > + config->crb_hdr = (uint16_t *)ACPI_CRB_HDR_ADDRESS; > + config->tis_hdr = NULL; > + } > + else > + { > + config->tpm_version = 1; > + config->crb_hdr = NULL; > + config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS; > + } This logic makes any value, including "0" mean "use TPM 1", which isn't terribly good. Furthermore, ACPI_HAS_TPM doesn't mean "has a TPM", it means "probe for a TPM". So what this actually wants to be is something more like this: s = xenstore_read("platform/tpm-version"); config->tpm_version = stroll(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; } You don't need to set the NULL values because config is suitably zeroed to begin with, and patch 2 will just add case 2: config->table_flags |= ACPI_HAS_TPM; config->crb_hdr = (uint16_t *)ACPI_CRB_HDR_ADDRESS; break; > diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h > index 2619ba32db..5754daa985 100644 > --- a/tools/libacpi/acpi2_0.h > +++ b/tools/libacpi/acpi2_0.h > @@ -121,6 +121,28 @@ struct acpi_20_tcpa { > }; > #define ACPI_2_0_TCPA_LAML_SIZE (64*1024) > > +/* > + * TPM2 > + */ > +struct Acpi20TPM2 { acpi_20_tpm2, consistent with everything else here. > diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c > index fe2db66a62..478cbec5dd 100644 > --- a/tools/libacpi/build.c > +++ b/tools/libacpi/build.c > @@ -409,38 +412,86 @@ 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) > { Style, here and lower down. The end result wants to look something like: if ( config->table_flags & ACPI_HAS_TPM ) { switch ( config->tpm_version ) { case 1: if ( !config->tis_hdr || config->tis_hdr[0] == 0 || config->tis_hdr[0] == 0xffff ) break; ssdt = ... break; } } In particular, I don't think the printf()'s are particularly useful for bad internal input into a probe function. > - 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 ) > + if (config-> tpm_version == 2) > + { > + if ( (config->crb_hdr) && > + (config->crb_hdr[0] != 0 && config->crb_hdr[0] != 0xffff)) > + { > + 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 Acpi20TPM2), > 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_ADDR_CTRL; > + tpm2->start_method = TPM2_START_METHOD_CRB; > + tpm2->log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE; > + > + log = ctxt->mem_ops.alloc(ctxt, TPM_LOG_SIZE, 4096); > + if (!log) return -1; This is buggy. APCI table memory is covered by an E820_ACPI range (specifically, is eligible to be used as general RAM after boot), while the TPM log should be in an E820_RESERVED region. To start with, it's probably fine to hardcode something in the 2M window at 0xfed40000 to be fixed location for the log. ~Andrew
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |