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

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 05/15/2015 10:44 AM, Igor Mammedov wrote:
> On Fri,  8 May 2015 11:52:46 -0400
> Stefan Berger <stefanb@linux.vnet.ibm.com> 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 <stefanb@linux.vnet.ibm.com>
>>
>> ---
>>
>> 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?


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

    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 */