Re: [Qemu-devel] [PATCH v3 3/3] TPM2 ACPI table support

[Igor Mammedov]

On Fri, 15 May 2015 11:31:30 -0400
Stefan Berger <address@hidden> wrote:

> On 05/15/2015 10:44 AM, Igor Mammedov wrote:
> > On Fri,  8 May 2015 11:52:46 -0400
> > Stefan Berger <address@hidden> wrote:
> >
> >> Add a TPM2 ACPI table if a TPM 2 is used in the backend.
> >> Also add an SSDT for the TPM 2.
> >>
> >> Rename tpm_find() to tpm_get_version() and have this function
> >> return the version of the TPM found, TPMVersion_Unspec if
> >> no TPM is found. Use the version number to build version
> >> specific ACPI tables.
> >>
> >> Signed-off-by: Stefan Berger <address@hidden>
> >>
> >> ---
> >>
> >> v2->v3:
> >>     - followed Igor's suggestion of a default case with an assert()
> >>     - added an SSDT for TPM2; it will be a bit different than TPM1.2's
> >>       SSDT once we add PPI to it
> >> ---
> >>   hw/i386/Makefile.objs |  2 +-
> >>   hw/i386/acpi-build.c  | 38 ++++++++++++++++++++++++++++++++++----
> >>   hw/i386/acpi-defs.h   | 18 ++++++++++++++++++
> >>   hw/i386/ssdt-tpm2.dsl | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >>   hw/tpm/tpm_tis.c      | 11 +++++++++++
> >>   include/hw/acpi/tpm.h |  5 +++++
> >>   include/sysemu/tpm.h  | 11 +++++++++--
> >>   7 files changed, 122 insertions(+), 7 deletions(-)
> >>   create mode 100644 hw/i386/ssdt-tpm2.dsl
> >>
> >> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> >> index e058a39..0be5d97 100644
> >> --- a/hw/i386/Makefile.objs
> >> +++ b/hw/i386/Makefile.objs
> >> @@ -9,7 +9,7 @@ obj-y += kvmvapic.o
> >>   obj-y += acpi-build.o
> >>   hw/i386/acpi-build.o: hw/i386/acpi-build.c \
> >>    hw/i386/acpi-dsdt.hex hw/i386/q35-acpi-dsdt.hex \
> >> -  hw/i386/ssdt-tpm.hex
> >> +  hw/i386/ssdt-tpm.hex hw/i386/ssdt-tpm2.hex
> >>   
> >>   iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \
> >>       ; then echo "$(2)"; else echo "$(3)"; fi ;)
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index e761005..e648ab5 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -42,6 +42,7 @@
> >>   #include "hw/acpi/memory_hotplug.h"
> >>   #include "sysemu/tpm.h"
> >>   #include "hw/acpi/tpm.h"
> >> +#include "sysemu/tpm_backend.h"
> >>   
> >>   /* Supported chipsets: */
> >>   #include "hw/acpi/piix4.h"
> >> @@ -111,7 +112,7 @@ typedef struct AcpiPmInfo {
> >>   
> >>   typedef struct AcpiMiscInfo {
> >>       bool has_hpet;
> >> -    bool has_tpm;
> >> +    TPMVersion tpm_version;
> >>       const unsigned char *dsdt_code;
> >>       unsigned dsdt_size;
> >>       uint16_t pvpanic_port;
> >> @@ -239,7 +240,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> >>   static void acpi_get_misc_info(AcpiMiscInfo *info)
> >>   {
> >>       info->has_hpet = hpet_find();
> >> -    info->has_tpm = tpm_find();
> >> +    info->tpm_version = tpm_get_version();
> >>       info->pvpanic_port = pvpanic_port();
> >>       info->applesmc_io_base = applesmc_port();
> >>   }
> >> @@ -468,6 +469,7 @@ build_madt(GArray *table_data, GArray *linker, 
> >> AcpiCpuInfo *cpu,
> >>   }
> >>   
> >>   #include "hw/i386/ssdt-tpm.hex"
> >> +#include "hw/i386/ssdt-tpm2.hex"
> >>   
> >>   /* Assign BSEL property to all buses.  In the future, this can be changed
> >>    * to only assign to buses that support hotplug.
> >> @@ -1065,6 +1067,25 @@ build_tpm_ssdt(GArray *table_data, GArray *linker)
> >>       memcpy(tpm_ptr, ssdt_tpm_aml, sizeof(ssdt_tpm_aml));
> >>   }
> >>   
> >> +static void
> >> +build_tpm2(GArray *table_data, GArray *linker)
> >> +{
> >> +    Acpi20TPM2 *tpm2_ptr;
> >> +    void *tpm_ptr;
> >> +
> >> +    tpm_ptr = acpi_data_push(table_data, sizeof(ssdt_tpm2_aml));
> >> +    memcpy(tpm_ptr, ssdt_tpm2_aml, sizeof(ssdt_tpm2_aml));
> > ^^^ does almost the same as build_tpm_ssdt(), replacing
> > AML template with dynamic AML generation would help to consolidate
> > v1 and v2 versions of  build_tpm_ssdt().
> 
> So the SSDT's for the TPM v1.2 and TPM 2 differ, but only slightly so.
> What does building the AML template via Opcodes etc. help in this case? 
> Doesn't this
> make the code more or less unreadable? I don't understand why we 
> shoudln't use
> the AML compiler on it?
Generating AML code directly from C allows us to drop maintaining
binary blobs for AML templates in QEMU source tree simplifying
code in most cases (but sometimes it's a bit verbose).
So far we've got rid of SSDT templates creating it completely
from C and when I'm get my hands in remaining DSDT eventually
it would be possible to drop dependency on IASL.

AML API is not operating in terms of opcodes but mostly
in terms that resemble ASL.
For example look at:

 8ac6f7a pc: acpi-build: drop template patching and create Device(SMC) 
dynamically

which I think is the closest to what is needed to convert TPM device.
Also since you are building a dedicated SSDT table for TPM
you might need to look at

 011bb749 pc: acpi-build: use aml_scope() for \_SB scope

to see how API is initialized, but instead of generating a
separate SSDT I'd suggest to move Device(TPM) generation into
common SSDT (build_ssdt()).

Ask me if you have any questions regarding usage of API,
I should be able to point to relevant commits.

> 
> 
> >
> >> +
> >> +    tpm2_ptr = acpi_data_push(table_data, sizeof *tpm2_ptr);
> >> +
> >> +    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> >> +    tpm2_ptr->control_area_address = cpu_to_le64(0);
> >> +    tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
> >> +
> >> +    build_header(linker, table_data,
> >> +                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4);
> >> +}
> >> +
> >>   typedef enum {
> >>       MEM_AFFINITY_NOFLAGS      = 0,
> >>       MEM_AFFINITY_ENABLED      = (1 << 0),
> >> @@ -1428,12 +1449,21 @@ void acpi_build(PcGuestInfo *guest_info, 
> >> AcpiBuildTables *tables)
> >>           acpi_add_table(table_offsets, tables_blob);
> >>           build_hpet(tables_blob, tables->linker);
> >>       }
> >> -    if (misc.has_tpm) {
> >> +    if (misc.tpm_version != TPM_VERSION_UNSPEC) {
> >>           acpi_add_table(table_offsets, tables_blob);
> >>           build_tpm_tcpa(tables_blob, tables->linker, tables->tcpalog);
> >>   
> >>           acpi_add_table(table_offsets, tables_blob);
> >> -        build_tpm_ssdt(tables_blob, tables->linker);
> >> +        switch (misc.tpm_version) {
> >> +        case TPM_VERSION_1_2:
> >> +            build_tpm_ssdt(tables_blob, tables->linker);
> >> +            break;
> >> +        case TPM_VERSION_2_0:
> >> +            build_tpm2(tables_blob, tables->linker);
> >> +            break;
> >> +        default:
> >> +            assert(false);
> >> +        }
> >>       }
> >>       if (guest_info->numa_nodes) {
> >>           acpi_add_table(table_offsets, tables_blob);
> >> diff --git a/hw/i386/acpi-defs.h b/hw/i386/acpi-defs.h
> >> index c4468f8..79ee9b1 100644
> >> --- a/hw/i386/acpi-defs.h
> >> +++ b/hw/i386/acpi-defs.h
> >> @@ -316,6 +316,9 @@ typedef struct AcpiTableMcfg AcpiTableMcfg;
> >>   
> >>   /*
> >>    * TCPA Description Table
> >> + *
> >> + * Following Level 00, Rev 00.37 of specs:
> >> + * http://www.trustedcomputinggroup.org/resources/tcg_acpi_specification
> >>    */
> >>   struct Acpi20Tcpa {
> >>       ACPI_TABLE_HEADER_DEF                    /* ACPI common table header 
> >> */
> >> @@ -325,6 +328,21 @@ struct Acpi20Tcpa {
> >>   } QEMU_PACKED;
> >>   typedef struct Acpi20Tcpa Acpi20Tcpa;
> >>   
> >> +/*
> >> + * TPM2
> >> + *
> >> + * Following Level 00, Rev 00.37 of specs:
> >> + * http://www.trustedcomputinggroup.org/resources/tcg_acpi_specification
> >> + */
> >> +struct Acpi20TPM2 {
> >> +    ACPI_TABLE_HEADER_DEF
> >> +    uint16_t platform_class;
> >> +    uint16_t reserved;
> >> +    uint64_t control_area_address;
> >> +    uint32_t start_method;
> >> +} QEMU_PACKED;
> >> +typedef struct Acpi20TPM2 Acpi20TPM2;
> >> +
> >>   /* DMAR - DMA Remapping table r2.2 */
> >>   struct AcpiTableDmar {
> >>       ACPI_TABLE_HEADER_DEF
> >> diff --git a/hw/i386/ssdt-tpm2.dsl b/hw/i386/ssdt-tpm2.dsl
> >> new file mode 100644
> >> index 0000000..96936ed
> >> --- /dev/null
> >> +++ b/hw/i386/ssdt-tpm2.dsl
> >> @@ -0,0 +1,44 @@
> >> +/*
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; either version 2 of the License, or
> >> + * (at your option) any later version.
> >> +
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> +
> >> + * You should have received a copy of the GNU General Public License along
> >> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +#include "hw/acpi/tpm.h"
> >> +
> >> +ACPI_EXTRACT_ALL_CODE ssdt_tpm2_aml
> >> +
> >> +DefinitionBlock (
> >> +    "ssdt-tpm2.aml",    // Output Filename
> >> +    "SSDT",             // Signature
> >> +    0x01,               // SSDT Compliance Revision
> >> +    "BXPC",             // OEMID
> >> +    "BXSSDT",           // TABLE ID
> >> +    0x1                 // OEM Revision
> >> +    )
> >> +{
> >> +    Scope(\_SB) {
> >> +        /* TPM with emulated TPM TIS interface */
> >> +        Device (TPM) {
> >> +            Name (_HID, EisaID ("PNP0C31"))
> >> +            Name (_CRS, ResourceTemplate ()
> >> +            {
> >> +                Memory32Fixed (ReadWrite, TPM_TIS_ADDR_BASE, 
> >> TPM_TIS_ADDR_SIZE)
> >> +
> >> +                /* TPM 2 is recent enough to support interrupts */
> >> +                IRQNoFlags () {TPM_TIS_IRQ}
> >> +            })
> >> +            Method (_STA, 0, NotSerialized) {
> >> +                Return (0x0F)
> >> +            }
> >> +        }
> >> +    }
> >> +}
> > Pls, don't add another ASL template.
> 
> Pls tell me why it is better to compile by hand rather than use a higher 
> level language here?
It addition to above reasons creating it dynamically
it would be possible to make TPM_TIS_ADDR_BASE not
hardcoded throughout QEMU and SeaBIOS but configurable
as tpm device property.

> 
>     Regards,
>         Stefan
> 
> > It would be better to rewrite above using AML API in C
> > and while at it convert hw/i386/ssdt-tpm.dsl to AML API as well.
> > hw/i386/ssdt-tpm.dsl is pretty much the same as ssdt-tpm2.aml
> > except of IRQNoFlags() so they could share common code
> > and for v2 you'll add extra IRQ resource.
> >
> > Also I'd suggest to put TPM device under \_SB.PCI0 scope
> > so that its _CRS wouldn't conflict with PCI0._CRS but rather
> > be a consumer of it.
> >
> >
> >
> >> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> >> index 69fbfae..daf2ac9 100644
> >> --- a/hw/tpm/tpm_tis.c
> >> +++ b/hw/tpm/tpm_tis.c
> >> @@ -32,6 +32,7 @@
> >>   #include "tpm_tis.h"
> >>   #include "qemu-common.h"
> >>   #include "qemu/main-loop.h"
> >> +#include "sysemu/tpm_backend.h"
> >>   
> >>   #define DEBUG_TIS 0
> >>   
> >> @@ -962,6 +963,16 @@ static int tpm_tis_do_startup_tpm(TPMState *s)
> >>   }
> >>   
> >>   /*
> >> + * Get the TPMVersion of the backend device being used
> >> + */
> >> +TPMVersion tpm_tis_get_tpm_version(Object *obj)
> >> +{
> >> +    TPMState *s = TPM(obj);
> >> +
> >> +    return tpm_backend_get_tpm_version(s->be_driver);
> >> +}
> >> +
> >> +/*
> >>    * This function is called when the machine starts, resets or due to
> >>    * S3 resume.
> >>    */
> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> index 792fcbf..6d516c6 100644
> >> --- a/include/hw/acpi/tpm.h
> >> +++ b/include/hw/acpi/tpm.h
> >> @@ -26,4 +26,9 @@
> >>   #define TPM_TCPA_ACPI_CLASS_CLIENT  0
> >>   #define TPM_TCPA_ACPI_CLASS_SERVER  1
> >>   
> >> +#define TPM2_ACPI_CLASS_CLIENT      0
> >> +#define TPM2_ACPI_CLASS_SERVER      1
> >> +
> >> +#define TPM2_START_METHOD_MMIO      6
> >> +
> >>   #endif /* HW_ACPI_TPM_H */
> >> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> >> index 848df41..c143890 100644
> >> --- a/include/sysemu/tpm.h
> >> +++ b/include/sysemu/tpm.h
> >> @@ -26,11 +26,18 @@ typedef enum  TPMVersion {
> >>       TPM_VERSION_2_0 = 2,
> >>   } TPMVersion;
> >>   
> >> +TPMVersion tpm_tis_get_tpm_version(Object *obj);
> >> +
> >>   #define TYPE_TPM_TIS                "tpm-tis"
> >>   
> >> -static inline bool tpm_find(void)
> >> +static inline TPMVersion tpm_get_version(void)
> >>   {
> >> -    return object_resolve_path_type("", TYPE_TPM_TIS, NULL);
> >> +    Object *obj = object_resolve_path_type("", TYPE_TPM_TIS, NULL);
> >> +
> >> +    if (obj) {
> >> +        return tpm_tis_get_tpm_version(obj);
> >> +    }
> >> +    return TPM_VERSION_UNSPEC;
> >>   }
> >>   
> >>   #endif /* QEMU_TPM_H */
>