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

From: Jan Beulich <jbeulich@suse.com>
To: Jennifer Herbert <jennifer.herbert@citrix.com>
Cc: xen-devel@lists.xenproject.org, andrew.cooper3@citrix.com,
	wl@xen.org, roger.pau@citrix.com
Subject: Re: [PATCH 1/2] acpi: Make TPM version configurable.
Date: Mon, 19 Sep 2022 12:19:26 +0200	[thread overview]
Message-ID: <039ff47a-064a-bfaf-31a0-68d4e055cfdc@suse.com> (raw)
In-Reply-To: <20220915204029.1227112-1-jennifer.herbert@citrix.com>

On 15.09.2022 22:40, Jennifer Herbert wrote:

First of all - please follow patch submission guidelines and
send patches To: the list only, with maintainers Cc:-ed.

> --- 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 |
>                              ACPI_HAS_WAET | ACPI_HAS_PMTIMER |
>                              ACPI_HAS_BUTTONS | ACPI_HAS_VGA |
>                              ACPI_HAS_8042 | ACPI_HAS_CMOS_RTC);

Did you not mean to drop ACPI_HAS_TPM here when ...

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

... you now OR it in here? Or else what use is this statement?

As to the use of strtoll() - I realize we have nothing else in
hvmloader, but I'm still weary of the overflow potential. Just
a remark, not really something to act upon.

> @@ -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;
> -
>      /*
>       * Address where acpi_info should be placed.
>       * This must match the OperationRegion(BIOS, SystemMemory, ....)

Please don't remove the blank line here.

Jan