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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.herbert@xxxxxxxxxx>
> ---
>  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?

> --- 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.

> +    switch( config->tpm_version )

Nit: Style (missing blank).

> --- 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.

Jan