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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.05.2023 00:40, Jennifer Herbert wrote:
> On 02/05/2023 12:54, Jan Beulich wrote:
>> On 25.04.2023 19:47, Jennifer Herbert wrote:
>>> --- 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.
> 
> Seems likely.   And i few other wacky values would give you 1.2 as well 
> I'd think.   There could also be trailing junk on the version number.
> 
> I was a bit phased by the lack of any real error cases in 
> hvmloader_acpi_build_tables.  It seemed the approch was if you put in 
> junk, you'll get something, but possibly not what your expecting.
> 
> Do I take it you'd prefer it to only accept a strict '1' for 1.2 and any 
> other value would result in no TPM being probed?  Or is it only the 
> overflow cases your concerned about?

Iirc xs convention is that numeric values can be spelled out in arbitrary
ways. So limiting to strictly "1" may be too restrictive. Anything that
evaluates to 1 without overflow nor trailing junk ought to be taken to
mean 1, I think.

>>> --- 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.
> 
> I'm not awair of anything, but your comment 2 lines down  from version 2 
> made me think you knew of some.  So if your happy with me removing this 
> line, I am!

I'm afraid I can't make the connection to that comment (assuming I fished
out the right one). In any event, especially with Jason's observation that
ACPI_HAS_TPM won't be set alongside ->tpm_version being zero, I see no
use for keeping the "case 0:".

Jan