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
