Re: [PATCH v2 2/2] acpi: Add TPM2 interface definition.

From: Jan Beulich <jbeulich@suse.com>
To: Jennifer Herbert <jennifer.herbert@citrix.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 2/2] acpi: Add TPM2 interface definition.
Date: Tue, 20 Dec 2022 15:44:26 +0100	[thread overview]
Message-ID: <87a0153a-aa90-6358-54eb-e78d005bcb18@suse.com> (raw)
In-Reply-To: <20221215170934.123889-3-jennifer.herbert@citrix.com>

On 15.12.2022 18:09, Jennifer Herbert wrote:
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -1009,6 +1009,13 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
>          config->table_flags |= ACPI_HAS_TPM;
>          config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
>          break;
> +    case 2:

Nit: Blank line please between non-fall-through case blocks.

> +        config->table_flags |= ACPI_HAS_TPM;
> +        config->crb_hdr = (uint16_t *)ACPI_CRB_HDR_ADDRESS;
> +
> +        mem_hole_populate_ram(TPM_LOG_AREA_ADDRESS >> PAGE_SHIFT, TPM_LOG_SIZE >> PAGE_SHIFT);

Nit: Overlong line.

> +        memset((void *)(TPM_LOG_AREA_ADDRESS), 0, TPM_LOG_SIZE);

Nit: Excess pair of parentheses.

> --- a/tools/libacpi/Makefile
> +++ b/tools/libacpi/Makefile
> @@ -25,7 +25,7 @@ C_SRC-$(CONFIG_X86) = dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c dsdt_pvh
>  C_SRC-$(CONFIG_ARM_64) = dsdt_anycpu_arm.c
>  DSDT_FILES ?= $(C_SRC-y)
>  C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, $(DSDT_FILES))
> -H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h ssdt_laptop_slate.h)
> +H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h ssdt_tpm2.h ssdt_laptop_slate.h)

This line could (the latest) now also do with splitting up.

> --- a/tools/libacpi/acpi2_0.h
> +++ b/tools/libacpi/acpi2_0.h
> @@ -121,6 +121,30 @@ struct acpi_20_tcpa {
>  };
>  #define ACPI_2_0_TCPA_LAML_SIZE (64*1024)
>  
> +/*
> + * TPM2
> + */
> +struct acpi_20_tpm2 {
> +    struct acpi_header header;
> +    uint16_t platform_class;
> +    uint16_t reserved;
> +    uint64_t control_area_address;
> +    uint32_t start_method;
> +    uint8_t start_method_params[12];
> +    uint32_t log_area_minimum_length;
> +    uint64_t log_area_start_address;
> +};
> +#define TPM2_ACPI_CLASS_CLIENT      0
> +#define TPM2_START_METHOD_CRB       7
> +
> +#define TPM_CRB_ADDR_BASE           0xFED40000
> +#define TPM_CRB_ADDR_CTRL           (TPM_CRB_ADDR_BASE + 0x40)

What is the relation between these two and ACPI_CRB_HDR_ADDRESS
(0xFED40034)? Independent of the answer it would be nice to have a
BUILD_BUG_ON()-like check somewhere tying the two together (and I have
a vague recollection that I might have asked for such in a comment on
v1 already).

And since afaics the space at that address also isn't filled
anywhere in hvmloader, the description could also do with saying what
entity is doing that (qemu?) and hence with whom this needs to remain
in sync.

> @@ -449,6 +451,39 @@ static int construct_secondary_tables(struct acpi_ctxt *ctxt,
>                               tcpa->header.length);
>              }
>              break;
> +
> +        case 2:
> +            if (!config->crb_hdr ||

See the respective comment on the earlier patch.

Jan