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.herb...@citrix.com>
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
