Re: [Qemu-devel] [PATCH] Add ACPI tables for TPM

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jul 22, 2014 at 04:13:01PM -0400, Stefan Berger wrote:
> On 07/22/2014 03:05 PM, Michael S. Tsirkin wrote:
> >On Tue, Jul 22, 2014 at 03:55:38PM -0400, Stefan Berger wrote:
> >>From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>
> >>Add an SSDT ACPI table for the TPM device.
> >>Add a TCPA table for BIOS logging area when a TPM is being used.
> >>
> >>The latter follows this spec here:
> >>
> >>http://www.trustedcomputinggroup.org/files/static_page_files/DCD4188E-1A4B-B294-D050A155FB6F7385/TCG_ACPIGeneralSpecification_PublicReview.pdf
> >>
> >>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >Some comments below.
> >Also pls remember to repost when 2.1 is out.
> 
> Will do.
> See answers below.
> 
> >
> >>---
> >>  hw/i386/Makefile.objs |  3 ++-
> >>  hw/i386/acpi-build.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
> >>  hw/i386/acpi-defs.h   | 11 +++++++++++
> >>  hw/i386/ssdt-tpm.dsl  | 28 ++++++++++++++++++++++++++++
> >>  include/sysemu/tpm.h  |  5 +++++
> >>  5 files changed, 89 insertions(+), 1 deletion(-)
> >>  create mode 100644 hw/i386/ssdt-tpm.dsl
> >>
> >>diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> >>index 48014ab..3688cf8 100644
> >>--- a/hw/i386/Makefile.objs
> >>+++ b/hw/i386/Makefile.objs
> >>@@ -10,7 +10,8 @@ obj-y += bios-linker-loader.o
> >>  hw/i386/acpi-build.o: hw/i386/acpi-build.c hw/i386/acpi-dsdt.hex \
> >>  	hw/i386/ssdt-proc.hex hw/i386/ssdt-pcihp.hex hw/i386/ssdt-misc.hex \
> >>  	hw/i386/acpi-dsdt.hex hw/i386/q35-acpi-dsdt.hex \
> >>-	hw/i386/q35-acpi-dsdt.hex hw/i386/ssdt-mem.hex
> >>+	hw/i386/q35-acpi-dsdt.hex hw/i386/ssdt-mem.hex \
> >>+	hw/i386/ssdt-tpm.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 ebc5f03..e5c2cdb 100644
> >>--- a/hw/i386/acpi-build.c
> >>+++ b/hw/i386/acpi-build.c
> >>@@ -38,6 +38,7 @@
> >>  #include "hw/loader.h"
> >>  #include "hw/isa/isa.h"
> >>  #include "hw/acpi/memory_hotplug.h"
> >>+#include "sysemu/tpm.h"
> >>  /* Supported chipsets: */
> >>  #include "hw/acpi/piix4.h"
> >>@@ -75,6 +76,7 @@ typedef struct AcpiPmInfo {
> >>  typedef struct AcpiMiscInfo {
> >>      bool has_hpet;
> >>+    bool has_tpm;
> >>      DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> >>      const unsigned char *dsdt_code;
> >>      unsigned dsdt_size;
> >>@@ -193,6 +195,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->pvpanic_port = pvpanic_port();
> >>  }
> >>@@ -681,6 +684,7 @@ static inline char acpi_get_hex(uint32_t val)
> >>  #include "hw/i386/ssdt-misc.hex"
> >>  #include "hw/i386/ssdt-pcihp.hex"
> >>+#include "hw/i386/ssdt-tpm.hex"
> >>  static void
> >>  build_append_notify_method(GArray *device, const char *name,
> >>@@ -1167,6 +1171,38 @@ build_hpet(GArray *table_data, GArray *linker)
> >>                   (void *)hpet, "HPET", sizeof(*hpet), 1);
> >>  }
> >>+static void
> >>+build_tpm_tcpa(GArray *table_data, GArray *linker)
> >>+{
> >>+    Acpi20Tcpa *tcpa;
> >>+    uint32_t laml = 128 * 1024;
> >what are these numbers for?
> 
> Follow above referenced specs, laml stands for log area minimum length, in
> bytes. So this is the number of bytes of a memory area to log into.

I see. Might be more obvious as a define e.g. TPM_LAML or TPM_LOG_AREA_MIN_LENGTH.

> >
> >>+    uint64_t lasa;

and what does lasa stand for? why not just open-code?

> >>+
> >>+    lasa = table_data->len + sizeof(*tcpa);
> >>+
> >>+    tcpa = acpi_data_push(table_data, sizeof(*tcpa) + laml);
> >>+
> >>+    tcpa->laml = cpu_to_le32(laml);
> >>+    tcpa->lasa = cpu_to_le64((uint64_t)lasa);

cast not needed.

> >>+
> >>+    /* LASA address to be filled by Guest linker */
> >>+    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> >>+                                   ACPI_BUILD_TABLE_FILE,
> >>+                                   table_data, &tcpa->lasa,
> >>+                                   sizeof(tcpa->lasa));
> >>+    build_header(linker, table_data,
> >>+                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> >>+}
> >>+
> >>+static void
> >>+build_tpm_ssdt(GArray *table_data, GArray *linker)
> >>+{
> >>+    void *tpm_ptr;
> >>+
> >>+    tpm_ptr = acpi_data_push(table_data, sizeof(ssdt_tpm_aml));
> >>+    memcpy(tpm_ptr, ssdt_tpm_aml, sizeof(ssdt_tpm_aml));
> >>+}
> >>+
> >>  typedef enum {
> >>      MEM_AFFINITY_NOFLAGS      = 0,
> >>      MEM_AFFINITY_ENABLED      = (1 << 0),
> >>@@ -1489,6 +1525,13 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
> >>          acpi_add_table(table_offsets, tables->table_data);
> >>          build_hpet(tables->table_data, tables->linker);
> >>      }
> >>+    if (misc.has_tpm) {
> >>+        acpi_add_table(table_offsets, tables->table_data);
> >>+        build_tpm_ssdt(tables->table_data, tables->linker);
> >>+
> >>+        acpi_add_table(table_offsets, tables->table_data);
> >>+        build_tpm_tcpa(tables->table_data, tables->linker);
> >>+    }
> >>      if (guest_info->numa_nodes) {
> >>          acpi_add_table(table_offsets, tables->table_data);
> >>          build_srat(tables->table_data, tables->linker, &cpu, guest_info);
> >>diff --git a/hw/i386/acpi-defs.h b/hw/i386/acpi-defs.h
> >>index e93babb..8526215 100644
> >>--- a/hw/i386/acpi-defs.h
> >>+++ b/hw/i386/acpi-defs.h
> >>@@ -314,4 +314,15 @@ struct AcpiTableMcfg {
> >>  } QEMU_PACKED;
> >>  typedef struct AcpiTableMcfg AcpiTableMcfg;
> >>+/*
> >>+ * TCPA Description Table
> >>+ */
> >>+struct Acpi20Tcpa {
> >>+    ACPI_TABLE_HEADER_DEF                    /* ACPI common table header */
> >>+    uint16_t platform_class;
> >>+    uint32_t laml;
> >>+    uint64_t lasa;
> >>+} QEMU_PACKED;
> >>+typedef struct Acpi20Tcpa Acpi20Tcpa;
> >>+
> >>  #endif
> >>diff --git a/hw/i386/ssdt-tpm.dsl b/hw/i386/ssdt-tpm.dsl
> >>new file mode 100644
> >>index 0000000..fd53cfb
> >>--- /dev/null
> >>+++ b/hw/i386/ssdt-tpm.dsl
> >>@@ -0,0 +1,28 @@
> >>+#include "hw/acpi/pc-hotplug.h"
> >>+
> >>+ACPI_EXTRACT_ALL_CODE ssdt_tpm_aml
> >>+
> >>+DefinitionBlock (
> >>+    "ssdt-tpm.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, 0xFED40000, 0x00005000)
> >what are these magic numbers?
> 
> TOM TIS MMIO area start and length.

Isn't there a define for this? We pre-process dsl so you
can just use an existing define.


> >you can use defines for them.
> >
> >>+                //IRQNoFlags () {5}
> >What's this?
> 
> If we run the TPM with IRQ, older Linux kernels may not work; so polling
> mode is unfortunately better.

So replace this comment with an explanation.


> >
> >>+            })
> >>+            Method (_STA, 0, NotSerialized) {
> >>+                Return (0x0F)
> >>+            }
> >>+        }
> >>+    }
> >>+}
> >>diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> >>index 13febdd..7cf2fc3 100644
> >>--- a/include/sysemu/tpm.h
> >>+++ b/include/sysemu/tpm.h
> >>@@ -20,4 +20,9 @@ int tpm_config_parse(QemuOptsList *opts_list, const char *optarg);
> >>  int tpm_init(void);
> >>  void tpm_cleanup(void);
> >>+static inline bool tpm_find(void)
> >>+{
> >>+    return (object_resolve_path_type("", "tpm-tis", NULL) != NULL);
> >outer () not needed.
> 
> Ok.
> 
> >
> >>+}
> >>+
> >>  #endif /* QEMU_TPM_H */
> >>-- 
> >>1.9.3