Re: [Qemu-devel] [PATCH v3 3/3] TPM2 ACPI table support
From:
Stefan Berger
Subject:
Re: [Qemu-devel] [PATCH v3 3/3] TPM2 ACPI table support
Date:
Fri, 15 May 2015 15:13:14 -0400
Igor Mammedov <address@hidden> wrote on 05/15/2015
12:57:40 PM:
> From: Igor Mammedov <address@hidden> > To: Stefan Berger <address@hidden> > Cc: address@hidden, address@hidden, address@hidden,
Stefan
> Berger/Watson/address@hidden, Dimitrios Pendarakis/Watson/address@hidden,
> George Wilson/Austin/address@hidden > Date: 05/15/2015 01:07 PM > Subject: Re: [PATCH v3 3/3] TPM2 ACPI table support >
> 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).
Phew, I looked at the code. It becomes quite a hurdle
to contribute something in this area. Being able to write and learn ASL is one thing, building
something similar to the ASL in C is yet another thing.
In the TPM case we have 2 SSDTs, one for TPM 1.2 and
one for TPM 2. They are very similar and could be merged into one asl code with #if around
the parts that are different, which are related to the IRQ and the check for ranges
of opcodes supported. Couldn't we use these first and then, if absolutely necessary,
convert it?
> 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.
IASL is from an rpm, right ? Why not use it for the
simpler cases?
>
> 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.
Stefan
>
> >
> >
> > >
> > >> +
> > >> + 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 */
> >
>