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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.05.2023 19:51, Jennifer Herbert wrote:
> This patch makes the TPM version, for which the ACPI library 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_verion xenstore key.
> 
> Signed-off-by: Jennifer Herbert <jennifer.herbert@xxxxxxxxxx>
> Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
albeit with two minor further request (which I'd be happy to make while
committing):

> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -920,6 +920,8 @@ void hvmloader_acpi_build_tables(struct acpi_config 
> *config,
>  {
>      const char *s;
>      struct acpi_ctxt ctxt;
> +    long long tpm_version = 0;

I don't see the need for an initializer here.

> @@ -967,8 +969,6 @@ void hvmloader_acpi_build_tables(struct acpi_config 
> *config,
>      s = xenstore_read("platform/generation-id", "0:0");
>      if ( s )
>      {
> -        char *end;
> -
>          config->vm_gid[0] = strtoll(s, &end, 0);
>          config->vm_gid[1] = 0;
>          if ( end && end[0] == ':' )

While there is a similarly odd pattern here, ...

> @@ -994,13 +994,27 @@ 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;
> +    config->tpm_version = 0;
> +    s = xenstore_read("platform/tpm_version", "1");
> +    tpm_version = strtoll(s, &end, 0);
> +
> +    if ( end && end[0] == '\0' )

... I don't think it should be further propagated. There's no need for
the left hand part of the condition.

Jan