Re: [for-4.17 PATCH] acpi: Add TPM2 interface definition and make the TPM version configurable.

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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