From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35633) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YtHaP-00054X-Dr for qemu-devel@nongnu.org; Fri, 15 May 2015 11:31:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YtHaK-0000HB-40 for qemu-devel@nongnu.org; Fri, 15 May 2015 11:31:41 -0400 Received: from e18.ny.us.ibm.com ([129.33.205.208]:40200) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YtHaJ-0000Go-VF for qemu-devel@nongnu.org; Fri, 15 May 2015 11:31:36 -0400 Received: from /spool/local by e18.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 15 May 2015 11:31:34 -0400 Received: from b01cxnp23033.gho.pok.ibm.com (b01cxnp23033.gho.pok.ibm.com [9.57.198.28]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 507476E803C for ; Fri, 15 May 2015 11:23:20 -0400 (EDT) Received: from d01av01.pok.ibm.com (d01av01.pok.ibm.com [9.56.224.215]) by b01cxnp23033.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t4FFVVfo54001746 for ; Fri, 15 May 2015 15:31:32 GMT Received: from d01av01.pok.ibm.com (localhost [127.0.0.1]) by d01av01.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t4FFVVsx007448 for ; Fri, 15 May 2015 11:31:31 -0400 Message-ID: <55561152.7060800@linux.vnet.ibm.com> Date: Fri, 15 May 2015 11:31:30 -0400 From: Stefan Berger MIME-Version: 1.0 References: <1431100366-700144-1-git-send-email-stefanb@linux.vnet.ibm.com> <1431100366-700144-4-git-send-email-stefanb@linux.vnet.ibm.com> <20150515164401.409f58dd@nial.brq.redhat.com> In-Reply-To: <20150515164401.409f58dd@nial.brq.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 3/3] TPM2 ACPI table support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: stefanb@us.ibm.com, mst@redhat.com, qemu-devel@nongnu.org, George Wilson , Dimitrios Pendarakis , quan.xu@intel.com On 05/15/2015 10:44 AM, Igor Mammedov wrote: > On Fri, 8 May 2015 11:52:46 -0400 > Stefan Berger 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 >> >> --- >> >> 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 . >> + */ >> +#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 */