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

[Jennifer Herbert <jennifer.herbert@xxxxxxxxxx>]

On 02/05/2023 12:54, Jan Beulich wrote:
> On 25.04.2023 19:47, Jennifer Herbert wrote:
> > This patch makes the TPM version, for which the ACPI libary 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_version xenstore key.
> >
> > Signed-off-by: Jennifer Herbert <jennifer.herbert@xxxxxxxxxx>
> > ---
> >   docs/misc/xenstore-paths.pandoc |  9 +++++
> >   tools/firmware/hvmloader/util.c | 19 ++++++---
> >   tools/libacpi/build.c           | 69 +++++++++++++++++++--------------
> >   tools/libacpi/libacpi.h         |  3 +-
> >   4 files changed, 64 insertions(+), 36 deletions(-)
> Please can you get used to providing a brief rev log somewhere here?
Yes, ok.

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

> > +    switch( config->tpm_version )
> Nit: Style (missing blank).
yup
> > --- 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!

> Jan