Re: [PATCH v2 1/2] acpi: Make TPM version configurable.

[Jan Beulich <jbeulich@xxxxxxxx>]

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