[Qemu-devel] [PATCH v2] Add ACPI tables for TPM

[Qemu-devel] [PATCH v2] Add ACPI tables for TPM

[Stefan Berger]

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>
---
 hw/i386/Makefile.objs |  3 ++-
 hw/i386/acpi-build.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-defs.h   | 11 +++++++++++
 hw/i386/ssdt-tpm.dsl  | 43 +++++++++++++++++++++++++++++++++++++++++++
 hw/tpm/tpm_tis.h      |  5 +----
 include/hw/acpi/tpm.h | 29 +++++++++++++++++++++++++++++
 include/sysemu/tpm.h  |  5 +++++
 7 files changed, 137 insertions(+), 5 deletions(-)
 create mode 100644 hw/i386/ssdt-tpm.dsl
 create mode 100644 include/hw/acpi/tpm.h

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..d767e37 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -38,6 +38,8 @@
 #include "hw/loader.h"
 #include "hw/isa/isa.h"
 #include "hw/acpi/memory_hotplug.h"
+#include "sysemu/tpm.h"
+#include "hw/acpi/tpm.h"
 
 /* Supported chipsets: */
 #include "hw/acpi/piix4.h"
@@ -75,6 +77,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 +196,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 +685,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 +1172,40 @@ 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 log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
+    uint64_t log_area_start_address;
+    size_t len = log_area_minimum_length + sizeof(*tcpa);
+
+    log_area_start_address = table_data->len + sizeof(*tcpa);
+
+    tcpa = acpi_data_push(table_data, len);
+
+    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
+    tcpa->log_area_minimum_length = cpu_to_le32(log_area_minimum_length);
+    tcpa->log_area_start_address = cpu_to_le64(log_area_start_address);
+
+    /* 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->log_area_start_address,
+                                   sizeof(tcpa->log_area_start_address));
+    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 +1528,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..1bc974a 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 log_area_minimum_length;
+    uint64_t log_area_start_address;
+} 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..75d9691
--- /dev/null
+++ b/hw/i386/ssdt-tpm.dsl
@@ -0,0 +1,43 @@
+/*
+ * 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_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, TPM_TIS_ADDR_BASE, TPM_TIS_ADDR_SIZE)
+                // older Linux tpm_tis drivers do not work with IRQ
+                //IRQNoFlags () {TPM_TIS_IRQ}
+            })
+            Method (_STA, 0, NotSerialized) {
+                Return (0x0F)
+            }
+        }
+    }
+}
diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h
index 916152a..6cdac24 100644
--- a/hw/tpm/tpm_tis.h
+++ b/hw/tpm/tpm_tis.h
@@ -18,18 +18,15 @@
 #define TPM_TPM_TIS_H
 
 #include "hw/isa/isa.h"
+#include "hw/acpi/tpm.h"
 #include "qemu-common.h"
 
-#define TPM_TIS_ADDR_BASE           0xFED40000
-
 #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
 #define TPM_TIS_LOCALITY_SHIFT      12
 #define TPM_TIS_NO_LOCALITY         0xff
 
 #define TPM_TIS_IS_VALID_LOCTY(x)   ((x) < TPM_TIS_NUM_LOCALITIES)
 
-#define TPM_TIS_IRQ                 5
-
 #define TPM_TIS_BUFFER_MAX          4096
 
 #define TYPE_TPM_TIS                "tpm-tis"
diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
new file mode 100644
index 0000000..792fcbf
--- /dev/null
+++ b/include/hw/acpi/tpm.h
@@ -0,0 +1,29 @@
+/*
+ * tpm.h - TPM ACPI definitions
+ *
+ * Copyright (C) 2014 IBM Corporation
+ *
+ * Authors:
+ *  Stefan Berger <stefanb@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * Implementation of the TIS interface according to specs found at
+ * http://www.trustedcomputinggroup.org
+ *
+ */
+#ifndef HW_ACPI_TPM_H
+#define HW_ACPI_TPM_H
+
+#define TPM_TIS_ADDR_BASE           0xFED40000
+#define TPM_TIS_ADDR_SIZE           0x5000
+
+#define TPM_TIS_IRQ                 5
+
+#define TPM_LOG_AREA_MINIMUM_SIZE   (64 * 1024)
+
+#define TPM_TCPA_ACPI_CLASS_CLIENT  0
+#define TPM_TCPA_ACPI_CLASS_SERVER  1
+
+#endif /* HW_ACPI_TPM_H */
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 13febdd..7030109 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;
+}
+
 #endif /* QEMU_TPM_H */
-- 
1.9.3

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

[Michael S. Tsirkin]

On Tue, Jul 29, 2014 at 06:52:19AM -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>

OK just to confirm, so far we only support passthrough tpm, this means
we don't need to worry about generating compatible ACPI for old machine
types, since passthrough disables migration.  Correct?

> ---
>  hw/i386/Makefile.objs |  3 ++-
>  hw/i386/acpi-build.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-defs.h   | 11 +++++++++++
>  hw/i386/ssdt-tpm.dsl  | 43 +++++++++++++++++++++++++++++++++++++++++++
>  hw/tpm/tpm_tis.h      |  5 +----
>  include/hw/acpi/tpm.h | 29 +++++++++++++++++++++++++++++
>  include/sysemu/tpm.h  |  5 +++++
>  7 files changed, 137 insertions(+), 5 deletions(-)
>  create mode 100644 hw/i386/ssdt-tpm.dsl
>  create mode 100644 include/hw/acpi/tpm.h
> 
> 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..d767e37 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -38,6 +38,8 @@
>  #include "hw/loader.h"
>  #include "hw/isa/isa.h"
>  #include "hw/acpi/memory_hotplug.h"
> +#include "sysemu/tpm.h"
> +#include "hw/acpi/tpm.h"
>  
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -75,6 +77,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 +196,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 +685,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 +1172,40 @@ 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 log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> +    uint64_t log_area_start_address;
> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> +
> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> +
> +    tcpa = acpi_data_push(table_data, len);
> +
> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> +    tcpa->log_area_minimum_length = cpu_to_le32(log_area_minimum_length);
> +    tcpa->log_area_start_address = cpu_to_le64(log_area_start_address);
> +
> +    /* 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->log_area_start_address,
> +                                   sizeof(tcpa->log_area_start_address));
> +    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 +1528,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..1bc974a 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 log_area_minimum_length;
> +    uint64_t log_area_start_address;
> +} 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..75d9691
> --- /dev/null
> +++ b/hw/i386/ssdt-tpm.dsl
> @@ -0,0 +1,43 @@
> +/*
> + * 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_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, TPM_TIS_ADDR_BASE, TPM_TIS_ADDR_SIZE)
> +                // older Linux tpm_tis drivers do not work with IRQ
> +                //IRQNoFlags () {TPM_TIS_IRQ}
> +            })
> +            Method (_STA, 0, NotSerialized) {
> +                Return (0x0F)
> +            }
> +        }
> +    }
> +}
> diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h
> index 916152a..6cdac24 100644
> --- a/hw/tpm/tpm_tis.h
> +++ b/hw/tpm/tpm_tis.h
> @@ -18,18 +18,15 @@
>  #define TPM_TPM_TIS_H
>  
>  #include "hw/isa/isa.h"
> +#include "hw/acpi/tpm.h"
>  #include "qemu-common.h"
>  
> -#define TPM_TIS_ADDR_BASE           0xFED40000
> -
>  #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
>  #define TPM_TIS_LOCALITY_SHIFT      12
>  #define TPM_TIS_NO_LOCALITY         0xff
>  
>  #define TPM_TIS_IS_VALID_LOCTY(x)   ((x) < TPM_TIS_NUM_LOCALITIES)
>  
> -#define TPM_TIS_IRQ                 5
> -
>  #define TPM_TIS_BUFFER_MAX          4096
>  
>  #define TYPE_TPM_TIS                "tpm-tis"
> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> new file mode 100644
> index 0000000..792fcbf
> --- /dev/null
> +++ b/include/hw/acpi/tpm.h
> @@ -0,0 +1,29 @@
> +/*
> + * tpm.h - TPM ACPI definitions
> + *
> + * Copyright (C) 2014 IBM Corporation
> + *
> + * Authors:
> + *  Stefan Berger <stefanb@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + * Implementation of the TIS interface according to specs found at
> + * http://www.trustedcomputinggroup.org
> + *
> + */
> +#ifndef HW_ACPI_TPM_H
> +#define HW_ACPI_TPM_H
> +
> +#define TPM_TIS_ADDR_BASE           0xFED40000
> +#define TPM_TIS_ADDR_SIZE           0x5000
> +
> +#define TPM_TIS_IRQ                 5
> +
> +#define TPM_LOG_AREA_MINIMUM_SIZE   (64 * 1024)
> +
> +#define TPM_TCPA_ACPI_CLASS_CLIENT  0
> +#define TPM_TCPA_ACPI_CLASS_SERVER  1
> +
> +#endif /* HW_ACPI_TPM_H */
> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> index 13febdd..7030109 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;
> +}
> +
>  #endif /* QEMU_TPM_H */
> -- 
> 1.9.3

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

[Stefan Berger]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

"Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 07:17:27 AM:

> From: "Michael S. Tsirkin" <mst@redhat.com>
> To: Stefan Berger/Watson/IBM@IBMUS
> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
> Date: 07/30/2014 07:18 AM
> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> 
> On Tue, Jul 29, 2014 at 06:52:19AM -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>
> 
> OK just to confirm, so far we only support passthrough tpm, this means
> we don't need to worry about generating compatible ACPI for old machine
> types, since passthrough disables migration.  Correct?

Passthrough disables migration, this is correct. 

Regards, Stefan

[-- Attachment #2: Type: text/html, Size: 1725 bytes --]

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

[Michael S. Tsirkin]

On Tue, Jul 29, 2014 at 06:52:19AM -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>
> ---
>  hw/i386/Makefile.objs |  3 ++-
>  hw/i386/acpi-build.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-defs.h   | 11 +++++++++++
>  hw/i386/ssdt-tpm.dsl  | 43 +++++++++++++++++++++++++++++++++++++++++++
>  hw/tpm/tpm_tis.h      |  5 +----
>  include/hw/acpi/tpm.h | 29 +++++++++++++++++++++++++++++
>  include/sysemu/tpm.h  |  5 +++++
>  7 files changed, 137 insertions(+), 5 deletions(-)
>  create mode 100644 hw/i386/ssdt-tpm.dsl
>  create mode 100644 include/hw/acpi/tpm.h
> 
> 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..d767e37 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -38,6 +38,8 @@
>  #include "hw/loader.h"
>  #include "hw/isa/isa.h"
>  #include "hw/acpi/memory_hotplug.h"
> +#include "sysemu/tpm.h"
> +#include "hw/acpi/tpm.h"
>  
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -75,6 +77,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 +196,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 +685,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 +1172,40 @@ 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 log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> +    uint64_t log_area_start_address;
> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> +
> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> +
> +    tcpa = acpi_data_push(table_data, len);
> +
> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> +    tcpa->log_area_minimum_length = cpu_to_le32(log_area_minimum_length);
> +    tcpa->log_area_start_address = cpu_to_le64(log_area_start_address);
> +
> +    /* LASA address to be filled by Guest linker */

Hmm, you are simply allocating log area as part of the ACPI table.  It
works because bios happens to allocate tables from high memory.
But I think this is a problem in practice because
bios is allowed to allocate acpi memory differently.
On the other hand log presumably needs to reside in
physical memory somewhere.

If you need bios to allocate this memory, then we will
need a new allocation type for this, add it to linker
in bios and qemu.

Alternatively, find some other way to get hold of
physical memory.
Is there a way to disable the log completely?
As defined in your patch, I doubt there's anything there, ever ..



> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> +                                   ACPI_BUILD_TABLE_FILE,
> +                                   table_data, &tcpa->log_area_start_address,
> +                                   sizeof(tcpa->log_area_start_address));
> +    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 +1528,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..1bc974a 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 log_area_minimum_length;
> +    uint64_t log_area_start_address;
> +} 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..75d9691
> --- /dev/null
> +++ b/hw/i386/ssdt-tpm.dsl
> @@ -0,0 +1,43 @@
> +/*
> + * 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_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, TPM_TIS_ADDR_BASE, TPM_TIS_ADDR_SIZE)
> +                // older Linux tpm_tis drivers do not work with IRQ
> +                //IRQNoFlags () {TPM_TIS_IRQ}
> +            })
> +            Method (_STA, 0, NotSerialized) {
> +                Return (0x0F)
> +            }
> +        }
> +    }
> +}
> diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h
> index 916152a..6cdac24 100644
> --- a/hw/tpm/tpm_tis.h
> +++ b/hw/tpm/tpm_tis.h
> @@ -18,18 +18,15 @@
>  #define TPM_TPM_TIS_H
>  
>  #include "hw/isa/isa.h"
> +#include "hw/acpi/tpm.h"
>  #include "qemu-common.h"
>  
> -#define TPM_TIS_ADDR_BASE           0xFED40000
> -
>  #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
>  #define TPM_TIS_LOCALITY_SHIFT      12
>  #define TPM_TIS_NO_LOCALITY         0xff
>  
>  #define TPM_TIS_IS_VALID_LOCTY(x)   ((x) < TPM_TIS_NUM_LOCALITIES)
>  
> -#define TPM_TIS_IRQ                 5
> -
>  #define TPM_TIS_BUFFER_MAX          4096
>  
>  #define TYPE_TPM_TIS                "tpm-tis"
> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> new file mode 100644
> index 0000000..792fcbf
> --- /dev/null
> +++ b/include/hw/acpi/tpm.h
> @@ -0,0 +1,29 @@
> +/*
> + * tpm.h - TPM ACPI definitions
> + *
> + * Copyright (C) 2014 IBM Corporation
> + *
> + * Authors:
> + *  Stefan Berger <stefanb@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + * Implementation of the TIS interface according to specs found at
> + * http://www.trustedcomputinggroup.org
> + *
> + */
> +#ifndef HW_ACPI_TPM_H
> +#define HW_ACPI_TPM_H
> +
> +#define TPM_TIS_ADDR_BASE           0xFED40000
> +#define TPM_TIS_ADDR_SIZE           0x5000
> +
> +#define TPM_TIS_IRQ                 5
> +
> +#define TPM_LOG_AREA_MINIMUM_SIZE   (64 * 1024)
> +
> +#define TPM_TCPA_ACPI_CLASS_CLIENT  0
> +#define TPM_TCPA_ACPI_CLASS_SERVER  1
> +
> +#endif /* HW_ACPI_TPM_H */
> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> index 13febdd..7030109 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;
> +}
> +
>  #endif /* QEMU_TPM_H */
> -- 
> 1.9.3

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

[Laszlo Ersek]

On 07/30/14 15:20, Michael S. Tsirkin wrote:
> On Tue, Jul 29, 2014 at 06:52:19AM -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

(Thanks for CC'ing me, Michael.)

I skimmed this spec.

>> +static void
>> +build_tpm_tcpa(GArray *table_data, GArray *linker)
>> +{
>> +    Acpi20Tcpa *tcpa;
>> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
>> +    uint64_t log_area_start_address;
>> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
>> +
>> +    log_area_start_address = table_data->len + sizeof(*tcpa);
>> +
>> +    tcpa = acpi_data_push(table_data, len);
>> +
>> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
>> +    tcpa->log_area_minimum_length = cpu_to_le32(log_area_minimum_length);
>> +    tcpa->log_area_start_address = cpu_to_le64(log_area_start_address);
>> +
>> +    /* LASA address to be filled by Guest linker */
> 
> Hmm, you are simply allocating log area as part of the ACPI table.  It
> works because bios happens to allocate tables from high memory.
> But I think this is a problem in practice because
> bios is allowed to allocate acpi memory differently.
> On the other hand log presumably needs to reside in
> physical memory somewhere.
> 
> If you need bios to allocate this memory, then we will
> need a new allocation type for this, add it to linker
> in bios and qemu.
> 
> Alternatively, find some other way to get hold of
> physical memory.
> Is there a way to disable the log completely?
> As defined in your patch, I doubt there's anything there, ever ..
> 
> 
> 
>> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> +                                   ACPI_BUILD_TABLE_FILE,
>> +                                   table_data, &tcpa->log_area_start_address,
>> +                                   sizeof(tcpa->log_area_start_address));
>> +    build_header(linker, table_data,
>> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
>> +}

So here's my understanding. The spec referenced above describes three
ACPI tables: two (client vs. server) for TPM 1.2, and a third one
(usable by both client & server platforms) for TPM 2.0.

The code above prepares a TPM 1.2 table. (Signature: "TCPA".)

This table has a field called LASA (Log Area Start Address) which points
to somewhere in (guest-)physical memory. The patch adds a "dummy range"
to the end of the TCPA table itself, and asks the linker to set LASA to
the beginning of that range.

This won't work in OVMF, and not just because of the reason that Michael
mentions (ie. because the firmware, in particular SeaBIOS, might
allocate the TCPA table in an area that is unsuitable as LASA target).

Rather, in OVMF this won't work because OVMF doesn't implement the
linking part of the linker. The *generic* edk2 protocol
(EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF uses
(as a client) to install ACPI tables in guest-phys memory requires
tables to be passed in one-by-one.

The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
well-known tables specially. It has knowledge of their internal
pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
updates pointers automatically. (For example when you install the FACS,
the protocol links it automatically into FACP.)

The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to know
anything about the TCPA table, let alone the unstructured (?) TCG event
log that is pointed-to by TCPA.LASA.

(I grepped for the TCPA signature,
EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)

This means that if you pass down a TCPA table, OVMF will install it
right now, but TCPA.LASA will be bogus.

If I wanted to implement the complete linker as Michael envisioned it,
then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
ACPI table installation from zero, trying to mimic the SeaBIOS client
code, but in a way that matches the UEFI environment. I'm not ready to
do that. Definitely not without an "official" human-language
specification of the linker-loader interface.

I skimmed the patch but I'm not sure what exactly the TPM emulation in
qemu depends on. Is it a command line option? Is it default for some
machine types?

Alternatively, I could recognize the TCPA signature in OVMF when parsing
the ACPI blobs for table headers, and filter it out.

Thanks
Laszlo

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

[Michael S. Tsirkin]

On Wed, Jul 30, 2014 at 04:36:38PM +0200, Laszlo Ersek wrote:
> On 07/30/14 15:20, Michael S. Tsirkin wrote:
> > On Tue, Jul 29, 2014 at 06:52:19AM -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
> 
> (Thanks for CC'ing me, Michael.)
> 
> I skimmed this spec.
> 
> >> +static void
> >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> >> +{
> >> +    Acpi20Tcpa *tcpa;
> >> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> >> +    uint64_t log_area_start_address;
> >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> >> +
> >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> >> +
> >> +    tcpa = acpi_data_push(table_data, len);
> >> +
> >> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> >> +    tcpa->log_area_minimum_length = cpu_to_le32(log_area_minimum_length);
> >> +    tcpa->log_area_start_address = cpu_to_le64(log_area_start_address);
> >> +
> >> +    /* LASA address to be filled by Guest linker */
> > 
> > Hmm, you are simply allocating log area as part of the ACPI table.  It
> > works because bios happens to allocate tables from high memory.
> > But I think this is a problem in practice because
> > bios is allowed to allocate acpi memory differently.
> > On the other hand log presumably needs to reside in
> > physical memory somewhere.
> > 
> > If you need bios to allocate this memory, then we will
> > need a new allocation type for this, add it to linker
> > in bios and qemu.
> > 
> > Alternatively, find some other way to get hold of
> > physical memory.
> > Is there a way to disable the log completely?
> > As defined in your patch, I doubt there's anything there, ever ..
> > 
> > 
> > 
> >> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> >> +                                   ACPI_BUILD_TABLE_FILE,
> >> +                                   table_data, &tcpa->log_area_start_address,
> >> +                                   sizeof(tcpa->log_area_start_address));
> >> +    build_header(linker, table_data,
> >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> >> +}
> 
> So here's my understanding. The spec referenced above describes three
> ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> (usable by both client & server platforms) for TPM 2.0.
> 
> The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> 
> This table has a field called LASA (Log Area Start Address) which points
> to somewhere in (guest-)physical memory. The patch adds a "dummy range"
> to the end of the TCPA table itself, and asks the linker to set LASA to
> the beginning of that range.
> 
> This won't work in OVMF, and not just because of the reason that Michael
> mentions (ie. because the firmware, in particular SeaBIOS, might
> allocate the TCPA table in an area that is unsuitable as LASA target).
> 
> Rather, in OVMF this won't work because OVMF doesn't implement the
> linking part of the linker. The *generic* edk2 protocol
> (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF uses
> (as a client) to install ACPI tables in guest-phys memory requires
> tables to be passed in one-by-one.
> 
> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> well-known tables specially. It has knowledge of their internal
> pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
> updates pointers automatically. (For example when you install the FACS,
> the protocol links it automatically into FACP.)
> 
> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to know
> anything about the TCPA table, let alone the unstructured (?) TCG event
> log that is pointed-to by TCPA.LASA.
> 
> (I grepped for the TCPA signature,
> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> 
> This means that if you pass down a TCPA table, OVMF will install it
> right now, but TCPA.LASA will be bogus.
> 
> If I wanted to implement the complete linker as Michael envisioned it,
> then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
> ACPI table installation from zero, trying to mimic the SeaBIOS client
> code, but in a way that matches the UEFI environment. I'm not ready to
> do that. Definitely not without an "official" human-language
> specification of the linker-loader interface.
>
> I skimmed the patch but I'm not sure what exactly the TPM emulation in
> qemu depends on. Is it a command line option? Is it default for some
> machine types?
> 
> Alternatively, I could recognize the TCPA signature in OVMF when parsing
> the ACPI blobs for table headers, and filter it out.
> 
> Thanks
> Laszlo

Assuming we want guest to allocate it, I think the way
would be to add a new zone for the linker, e.g. PHYSICAL.
You would then parse only these entries and ignore
others, this way you can still use EFI_ACPI_TABLE_PROTOCOL.

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

[Laszlo Ersek]

On 07/30/14 16:46, Michael S. Tsirkin wrote:
> On Wed, Jul 30, 2014 at 04:36:38PM +0200, Laszlo Ersek wrote:
>> On 07/30/14 15:20, Michael S. Tsirkin wrote:
>>> On Tue, Jul 29, 2014 at 06:52:19AM -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
>>
>> (Thanks for CC'ing me, Michael.)
>>
>> I skimmed this spec.
>>
>>>> +static void
>>>> +build_tpm_tcpa(GArray *table_data, GArray *linker)
>>>> +{
>>>> +    Acpi20Tcpa *tcpa;
>>>> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
>>>> +    uint64_t log_area_start_address;
>>>> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
>>>> +
>>>> +    log_area_start_address = table_data->len + sizeof(*tcpa);
>>>> +
>>>> +    tcpa = acpi_data_push(table_data, len);
>>>> +
>>>> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
>>>> +    tcpa->log_area_minimum_length = cpu_to_le32(log_area_minimum_length);
>>>> +    tcpa->log_area_start_address = cpu_to_le64(log_area_start_address);
>>>> +
>>>> +    /* LASA address to be filled by Guest linker */
>>>
>>> Hmm, you are simply allocating log area as part of the ACPI table.  It
>>> works because bios happens to allocate tables from high memory.
>>> But I think this is a problem in practice because
>>> bios is allowed to allocate acpi memory differently.
>>> On the other hand log presumably needs to reside in
>>> physical memory somewhere.
>>>
>>> If you need bios to allocate this memory, then we will
>>> need a new allocation type for this, add it to linker
>>> in bios and qemu.
>>>
>>> Alternatively, find some other way to get hold of
>>> physical memory.
>>> Is there a way to disable the log completely?
>>> As defined in your patch, I doubt there's anything there, ever ..
>>>
>>>
>>>
>>>> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>>> +                                   ACPI_BUILD_TABLE_FILE,
>>>> +                                   table_data, &tcpa->log_area_start_address,
>>>> +                                   sizeof(tcpa->log_area_start_address));
>>>> +    build_header(linker, table_data,
>>>> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
>>>> +}
>>
>> So here's my understanding. The spec referenced above describes three
>> ACPI tables: two (client vs. server) for TPM 1.2, and a third one
>> (usable by both client & server platforms) for TPM 2.0.
>>
>> The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
>>
>> This table has a field called LASA (Log Area Start Address) which points
>> to somewhere in (guest-)physical memory. The patch adds a "dummy range"
>> to the end of the TCPA table itself, and asks the linker to set LASA to
>> the beginning of that range.
>>
>> This won't work in OVMF, and not just because of the reason that Michael
>> mentions (ie. because the firmware, in particular SeaBIOS, might
>> allocate the TCPA table in an area that is unsuitable as LASA target).
>>
>> Rather, in OVMF this won't work because OVMF doesn't implement the
>> linking part of the linker. The *generic* edk2 protocol
>> (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF uses
>> (as a client) to install ACPI tables in guest-phys memory requires
>> tables to be passed in one-by-one.
>>
>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
>> well-known tables specially. It has knowledge of their internal
>> pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
>> updates pointers automatically. (For example when you install the FACS,
>> the protocol links it automatically into FACP.)
>>
>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to know
>> anything about the TCPA table, let alone the unstructured (?) TCG event
>> log that is pointed-to by TCPA.LASA.
>>
>> (I grepped for the TCPA signature,
>> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
>>
>> This means that if you pass down a TCPA table, OVMF will install it
>> right now, but TCPA.LASA will be bogus.
>>
>> If I wanted to implement the complete linker as Michael envisioned it,
>> then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
>> ACPI table installation from zero, trying to mimic the SeaBIOS client
>> code, but in a way that matches the UEFI environment. I'm not ready to
>> do that. Definitely not without an "official" human-language
>> specification of the linker-loader interface.
>>
>> I skimmed the patch but I'm not sure what exactly the TPM emulation in
>> qemu depends on. Is it a command line option? Is it default for some
>> machine types?
>>
>> Alternatively, I could recognize the TCPA signature in OVMF when parsing
>> the ACPI blobs for table headers, and filter it out.
>>
>> Thanks
>> Laszlo
> 
> Assuming we want guest to allocate it, I think the way
> would be to add a new zone for the linker, e.g. PHYSICAL.
> You would then parse only these entries and ignore
> others, this way you can still use EFI_ACPI_TABLE_PROTOCOL.

If I understand correctly, this wouldn't save me from having to update
TCPA.LASA manually, to the separately allocated TCG event log area
(which I reckon you meant by PHYSICAL).

Once I did that, I could also look at TCPA.LAML
(log_area_minimum_length) in the table, and just allocate the correctly
sized area knowingly.

If you meant that TCPA itself would be PHYSICAL, and it would include
(as proposed by the patch) the event log, tacked to its end, then that
won't work again. Because EFI_ACPI_TABLE_PROTOCOL installs deep copies
of tables, as EfiACPIReclaimMemory (for most tables), and as
EfiACPIMemoryNVS (for "FACS" and "UEFI"). Allocating TCPA as
EfiACPIReclaimMemory may not be correct:

Table 26. Memory Type Usage after ExitBootServices():

EfiACPIReclaimMemory: This memory is to be preserved by the loader and
                      OS until ACPI is enabled. Once ACPI is enabled,
                      the memory in this range is available for general
                      use.

I don't know if this misery will ever end unless I give up on
EFI_ACPI_TABLE_PROTOCOL.

Thanks
Laszlo

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

[Michael S. Tsirkin]

On Wed, Jul 30, 2014 at 05:15:37PM +0200, Laszlo Ersek wrote:
> On 07/30/14 16:46, Michael S. Tsirkin wrote:
> > On Wed, Jul 30, 2014 at 04:36:38PM +0200, Laszlo Ersek wrote:
> >> On 07/30/14 15:20, Michael S. Tsirkin wrote:
> >>> On Tue, Jul 29, 2014 at 06:52:19AM -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
> >>
> >> (Thanks for CC'ing me, Michael.)
> >>
> >> I skimmed this spec.
> >>
> >>>> +static void
> >>>> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> >>>> +{
> >>>> +    Acpi20Tcpa *tcpa;
> >>>> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> >>>> +    uint64_t log_area_start_address;
> >>>> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> >>>> +
> >>>> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> >>>> +
> >>>> +    tcpa = acpi_data_push(table_data, len);
> >>>> +
> >>>> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> >>>> +    tcpa->log_area_minimum_length = cpu_to_le32(log_area_minimum_length);
> >>>> +    tcpa->log_area_start_address = cpu_to_le64(log_area_start_address);
> >>>> +
> >>>> +    /* LASA address to be filled by Guest linker */
> >>>
> >>> Hmm, you are simply allocating log area as part of the ACPI table.  It
> >>> works because bios happens to allocate tables from high memory.
> >>> But I think this is a problem in practice because
> >>> bios is allowed to allocate acpi memory differently.
> >>> On the other hand log presumably needs to reside in
> >>> physical memory somewhere.
> >>>
> >>> If you need bios to allocate this memory, then we will
> >>> need a new allocation type for this, add it to linker
> >>> in bios and qemu.
> >>>
> >>> Alternatively, find some other way to get hold of
> >>> physical memory.
> >>> Is there a way to disable the log completely?
> >>> As defined in your patch, I doubt there's anything there, ever ..
> >>>
> >>>
> >>>
> >>>> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> >>>> +                                   ACPI_BUILD_TABLE_FILE,
> >>>> +                                   table_data, &tcpa->log_area_start_address,
> >>>> +                                   sizeof(tcpa->log_area_start_address));
> >>>> +    build_header(linker, table_data,
> >>>> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> >>>> +}
> >>
> >> So here's my understanding. The spec referenced above describes three
> >> ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> >> (usable by both client & server platforms) for TPM 2.0.
> >>
> >> The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> >>
> >> This table has a field called LASA (Log Area Start Address) which points
> >> to somewhere in (guest-)physical memory. The patch adds a "dummy range"
> >> to the end of the TCPA table itself, and asks the linker to set LASA to
> >> the beginning of that range.
> >>
> >> This won't work in OVMF, and not just because of the reason that Michael
> >> mentions (ie. because the firmware, in particular SeaBIOS, might
> >> allocate the TCPA table in an area that is unsuitable as LASA target).
> >>
> >> Rather, in OVMF this won't work because OVMF doesn't implement the
> >> linking part of the linker. The *generic* edk2 protocol
> >> (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF uses
> >> (as a client) to install ACPI tables in guest-phys memory requires
> >> tables to be passed in one-by-one.
> >>
> >> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> >> well-known tables specially. It has knowledge of their internal
> >> pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
> >> updates pointers automatically. (For example when you install the FACS,
> >> the protocol links it automatically into FACP.)
> >>
> >> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to know
> >> anything about the TCPA table, let alone the unstructured (?) TCG event
> >> log that is pointed-to by TCPA.LASA.
> >>
> >> (I grepped for the TCPA signature,
> >> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> >>
> >> This means that if you pass down a TCPA table, OVMF will install it
> >> right now, but TCPA.LASA will be bogus.
> >>
> >> If I wanted to implement the complete linker as Michael envisioned it,
> >> then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
> >> ACPI table installation from zero, trying to mimic the SeaBIOS client
> >> code, but in a way that matches the UEFI environment. I'm not ready to
> >> do that. Definitely not without an "official" human-language
> >> specification of the linker-loader interface.
> >>
> >> I skimmed the patch but I'm not sure what exactly the TPM emulation in
> >> qemu depends on. Is it a command line option? Is it default for some
> >> machine types?
> >>
> >> Alternatively, I could recognize the TCPA signature in OVMF when parsing
> >> the ACPI blobs for table headers, and filter it out.
> >>
> >> Thanks
> >> Laszlo
> > 
> > Assuming we want guest to allocate it, I think the way
> > would be to add a new zone for the linker, e.g. PHYSICAL.
> > You would then parse only these entries and ignore
> > others, this way you can still use EFI_ACPI_TABLE_PROTOCOL.
> 
> If I understand correctly, this wouldn't save me from having to update
> TCPA.LASA manually, to the separately allocated TCG event log area
> (which I reckon you meant by PHYSICAL).

Not sure what you mean by manually - you would have to interpret
the pointer command for that.

> Once I did that, I could also look at TCPA.LAML
> (log_area_minimum_length) in the table, and just allocate the correctly
> sized area knowingly.
> 
> If you meant that TCPA itself would be PHYSICAL, and it would include
> (as proposed by the patch) the event log, tacked to its end, then that
> won't work again. Because EFI_ACPI_TABLE_PROTOCOL installs deep copies
> of tables, as EfiACPIReclaimMemory (for most tables), and as
> EfiACPIMemoryNVS (for "FACS" and "UEFI"). Allocating TCPA as
> EfiACPIReclaimMemory may not be correct:
> 
> Table 26. Memory Type Usage after ExitBootServices():
> 
> EfiACPIReclaimMemory: This memory is to be preserved by the loader and
>                       OS until ACPI is enabled. Once ACPI is enabled,
>                       the memory in this range is available for general
>                       use.
> 
> I don't know if this misery will ever end unless I give up on
> EFI_ACPI_TABLE_PROTOCOL.
> 
> Thanks
> Laszlo

You can though you don't have to if you don't want to.

What you have to do is implement the linker protocol fully though.

This means you need to:

1. execute alloc instructions, building a data structure mapping fwcfg
   file names to memory.

   Zone field tells you what this file is.
	at the moment it can be 0x1 for ACPI or 0x2 for RSDP
	(RSDP you skip right?)
	if we ever add more stuff it will be something else,
	for example, with this patch log will have to use
	a separate file from a different, new zone

2. execute pointer instructions, these give you pointers to
   start of each table in the files you allocated

to make things easier, pointers never refer to files
allocated afterwards, so you can do it in a single pass
if you wish.

you can free up the datastructure afterwards.



-- 
MST
-- 
MST

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

[Laszlo Ersek]

On 07/30/14 17:37, Michael S. Tsirkin wrote:

> 1. execute alloc instructions, building a data structure mapping fwcfg
>    file names to memory.

Yes, edk2 currently lacks a good (== sub-linear) dictionary data type.
This week I started porting a red-black tree library that I had
originally written in 1999 or 2000 or so. In OVMF coding I've faced a
few occasions when I would have wanted a dictionary, and one of them is
the above.

Laszlo

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

[Michael S. Tsirkin]

On Wed, Jul 30, 2014 at 06:02:21PM +0200, Laszlo Ersek wrote:
> On 07/30/14 17:37, Michael S. Tsirkin wrote:
> 
> > 1. execute alloc instructions, building a data structure mapping fwcfg
> >    file names to memory.
> 
> Yes, edk2 currently lacks a good (== sub-linear) dictionary data type.
> This week I started porting a red-black tree library that I had
> originally written in 1999 or 2000 or so. In OVMF coding I've faced a
> few occasions when I would have wanted a dictionary, and one of them is
> the above.
> 
> Laszlo

number of tables is small though, seabios just uses a linked list
and a linear search, to get N^2 complexity where N is number
of tables.

it's up to you.

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

[Laszlo Ersek]

On 07/30/14 18:07, Michael S. Tsirkin wrote:
> On Wed, Jul 30, 2014 at 06:02:21PM +0200, Laszlo Ersek wrote:
>> On 07/30/14 17:37, Michael S. Tsirkin wrote:
>>
>>> 1. execute alloc instructions, building a data structure mapping fwcfg
>>>    file names to memory.
>>
>> Yes, edk2 currently lacks a good (== sub-linear) dictionary data type.
>> This week I started porting a red-black tree library that I had
>> originally written in 1999 or 2000 or so. In OVMF coding I've faced a
>> few occasions when I would have wanted a dictionary, and one of them is
>> the above.
>>
>> Laszlo
> 
> number of tables is small though, seabios just uses a linked list
> and a linear search, to get N^2 complexity where N is number
> of tables.
> 
> it's up to you.

Correct, I did consider that, but I probed edk2-devel for opinions about
an associative data structure first, and feedback was positive, so I
started porting. I'll even admit that for small N, the O(N^2) of lists
might beat the rbtree's O(NlogN) in practice, due to the greater
constants in the "smart" data structure, but just the API should be that
much more convenient that I'm willing to accept that.

Laszlo

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

[Igor Mammedov]

On Wed, 30 Jul 2014 16:36:38 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 07/30/14 15:20, Michael S. Tsirkin wrote:
> > On Tue, Jul 29, 2014 at 06:52:19AM -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
> 
> (Thanks for CC'ing me, Michael.)
> 
> I skimmed this spec.
> 
> >> +static void
> >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> >> +{
> >> +    Acpi20Tcpa *tcpa;
> >> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> >> +    uint64_t log_area_start_address;
> >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> >> +
> >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> >> +
> >> +    tcpa = acpi_data_push(table_data, len);
> >> +
> >> +    tcpa->platform_class =
> >> cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> >> +    tcpa->log_area_minimum_length =
> >> cpu_to_le32(log_area_minimum_length);
> >> +    tcpa->log_area_start_address =
> >> cpu_to_le64(log_area_start_address); +
> >> +    /* LASA address to be filled by Guest linker */
> > 
> > Hmm, you are simply allocating log area as part of the ACPI table.
> > It works because bios happens to allocate tables from high memory.
> > But I think this is a problem in practice because
> > bios is allowed to allocate acpi memory differently.
> > On the other hand log presumably needs to reside in
> > physical memory somewhere.
> > 
> > If you need bios to allocate this memory, then we will
> > need a new allocation type for this, add it to linker
> > in bios and qemu.
> > 
> > Alternatively, find some other way to get hold of
> > physical memory.
> > Is there a way to disable the log completely?
> > As defined in your patch, I doubt there's anything there, ever ..
> > 
> > 
> > 
> >> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> >> +                                   ACPI_BUILD_TABLE_FILE,
> >> +                                   table_data,
> >> &tcpa->log_area_start_address,
> >> +
> >> sizeof(tcpa->log_area_start_address));
> >> +    build_header(linker, table_data,
> >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> >> +}
> 
> So here's my understanding. The spec referenced above describes three
> ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> (usable by both client & server platforms) for TPM 2.0.
> 
> The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> 
> This table has a field called LASA (Log Area Start Address) which
> points to somewhere in (guest-)physical memory. The patch adds a
> "dummy range" to the end of the TCPA table itself, and asks the
> linker to set LASA to the beginning of that range.
> 
> This won't work in OVMF, and not just because of the reason that
> Michael mentions (ie. because the firmware, in particular SeaBIOS,
> might allocate the TCPA table in an area that is unsuitable as LASA
> target).
> 
> Rather, in OVMF this won't work because OVMF doesn't implement the
> linking part of the linker. The *generic* edk2 protocol
> (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF
> uses (as a client) to install ACPI tables in guest-phys memory
> requires tables to be passed in one-by-one.
> 
> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> well-known tables specially. It has knowledge of their internal
> pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
> updates pointers automatically. (For example when you install the
> FACS, the protocol links it automatically into FACP.)
> 
> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to
> know anything about the TCPA table, let alone the unstructured (?)
> TCG event log that is pointed-to by TCPA.LASA.
> 
> (I grepped for the TCPA signature,
> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> 
> This means that if you pass down a TCPA table, OVMF will install it
> right now, but TCPA.LASA will be bogus.
Can we allocate memory for log in QEMU and just pass its address in
ACPI tables, marking arrea as reserved in e820? No linker would be
involved and arrea would be easily accesssible to qemu/mgmt tools.
  
> 
> If I wanted to implement the complete linker as Michael envisioned it,
> then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
> ACPI table installation from zero, trying to mimic the SeaBIOS client
> code, but in a way that matches the UEFI environment. I'm not ready to
> do that. Definitely not without an "official" human-language
> specification of the linker-loader interface.
> 
> I skimmed the patch but I'm not sure what exactly the TPM emulation in
> qemu depends on. Is it a command line option? Is it default for some
> machine types?
> 
> Alternatively, I could recognize the TCPA signature in OVMF when
> parsing the ACPI blobs for table headers, and filter it out.
> 
> Thanks
> Laszlo
> 
> 

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

[Laszlo Ersek]

On 07/30/14 17:03, Igor Mammedov wrote:
> On Wed, 30 Jul 2014 16:36:38 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 07/30/14 15:20, Michael S. Tsirkin wrote:
>>> On Tue, Jul 29, 2014 at 06:52:19AM -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
>>
>> (Thanks for CC'ing me, Michael.)
>>
>> I skimmed this spec.
>>
>>>> +static void
>>>> +build_tpm_tcpa(GArray *table_data, GArray *linker)
>>>> +{
>>>> +    Acpi20Tcpa *tcpa;
>>>> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
>>>> +    uint64_t log_area_start_address;
>>>> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
>>>> +
>>>> +    log_area_start_address = table_data->len + sizeof(*tcpa);
>>>> +
>>>> +    tcpa = acpi_data_push(table_data, len);
>>>> +
>>>> +    tcpa->platform_class =
>>>> cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
>>>> +    tcpa->log_area_minimum_length =
>>>> cpu_to_le32(log_area_minimum_length);
>>>> +    tcpa->log_area_start_address =
>>>> cpu_to_le64(log_area_start_address); +
>>>> +    /* LASA address to be filled by Guest linker */
>>>
>>> Hmm, you are simply allocating log area as part of the ACPI table.
>>> It works because bios happens to allocate tables from high memory.
>>> But I think this is a problem in practice because
>>> bios is allowed to allocate acpi memory differently.
>>> On the other hand log presumably needs to reside in
>>> physical memory somewhere.
>>>
>>> If you need bios to allocate this memory, then we will
>>> need a new allocation type for this, add it to linker
>>> in bios and qemu.
>>>
>>> Alternatively, find some other way to get hold of
>>> physical memory.
>>> Is there a way to disable the log completely?
>>> As defined in your patch, I doubt there's anything there, ever ..
>>>
>>>
>>>
>>>> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>>> +                                   ACPI_BUILD_TABLE_FILE,
>>>> +                                   table_data,
>>>> &tcpa->log_area_start_address,
>>>> +
>>>> sizeof(tcpa->log_area_start_address));
>>>> +    build_header(linker, table_data,
>>>> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
>>>> +}
>>
>> So here's my understanding. The spec referenced above describes three
>> ACPI tables: two (client vs. server) for TPM 1.2, and a third one
>> (usable by both client & server platforms) for TPM 2.0.
>>
>> The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
>>
>> This table has a field called LASA (Log Area Start Address) which
>> points to somewhere in (guest-)physical memory. The patch adds a
>> "dummy range" to the end of the TCPA table itself, and asks the
>> linker to set LASA to the beginning of that range.
>>
>> This won't work in OVMF, and not just because of the reason that
>> Michael mentions (ie. because the firmware, in particular SeaBIOS,
>> might allocate the TCPA table in an area that is unsuitable as LASA
>> target).
>>
>> Rather, in OVMF this won't work because OVMF doesn't implement the
>> linking part of the linker. The *generic* edk2 protocol
>> (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF
>> uses (as a client) to install ACPI tables in guest-phys memory
>> requires tables to be passed in one-by-one.
>>
>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
>> well-known tables specially. It has knowledge of their internal
>> pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
>> updates pointers automatically. (For example when you install the
>> FACS, the protocol links it automatically into FACP.)
>>
>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to
>> know anything about the TCPA table, let alone the unstructured (?)
>> TCG event log that is pointed-to by TCPA.LASA.
>>
>> (I grepped for the TCPA signature,
>> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
>>
>> This means that if you pass down a TCPA table, OVMF will install it
>> right now, but TCPA.LASA will be bogus.

> Can we allocate memory for log in QEMU and just pass its address in
> ACPI tables, marking arrea as reserved in e820? No linker would be
> involved and arrea would be easily accesssible to qemu/mgmt tools.

UEFI considers e820 legacy, and replaces it with the "memory space map"
(which describes hardware properties of memory ranges, approximately)
and the "memory map" (which describes what range is used how).

But, in any case, the platform code of OVMF does not "consume" these
memory maps, it *produces* them. Keying off from fw_cfg, CMOS, and so on.

(For example, the /etc/pci-info fw_cfg file would have been one input
point for this logic, to influence the _CRS, but that need has been
lifted by two chages, the first being the PCI hole allocation change in
qemu (for OVMF's ACPI generator), the second being exposing the ACPI
tables whole-sale to OVMF (which prevents OVMF's ACPI generator from
running).)

(If qemu currently exposes some e820 map in an fw_cfg file (I don't
remember), then OVMF certainly doesn't look at it. Even if it did, it
would have to translate it entry by entry to a memory space map (by
producing memory resource descriptor hand-off blocks in PlatformPei),
and that translation itself would be fraught with incompatibilities.)

IOW the issue is the same as it always has been: exposing information to
OVMF via easily parseable fw_cfg files (for hot-patching builtin
templates / generators), or via something else that it doesn't have to
parse *at all*, just pass through to the OS blindly.

Thanks
Laszlo

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

[Stefan Berger]

[-- Attachment #1: Type: text/plain, Size: 5789 bytes --]

Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:

> From: Laszlo Ersek <lersek@redhat.com>
> To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan 
Berger/Watson/IBM@IBMUS
> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
> Date: 07/30/2014 10:36 AM
> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> 
> On 07/30/14 15:20, Michael S. Tsirkin wrote:
> > On Tue, Jul 29, 2014 at 06:52:19AM -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
> 
> (Thanks for CC'ing me, Michael.)
> 
> I skimmed this spec.
> 
> >> +static void
> >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> >> +{
> >> +    Acpi20Tcpa *tcpa;
> >> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> >> +    uint64_t log_area_start_address;
> >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> >> +
> >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> >> +
> >> +    tcpa = acpi_data_push(table_data, len);
> >> +
> >> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> >> +    tcpa->log_area_minimum_length = 
cpu_to_le32(log_area_minimum_length);
> >> +    tcpa->log_area_start_address = 
cpu_to_le64(log_area_start_address);
> >> +
> >> +    /* LASA address to be filled by Guest linker */
> > 
> > Hmm, you are simply allocating log area as part of the ACPI table.  It
> > works because bios happens to allocate tables from high memory.
> > But I think this is a problem in practice because
> > bios is allowed to allocate acpi memory differently.
> > On the other hand log presumably needs to reside in
> > physical memory somewhere.
> > 
> > If you need bios to allocate this memory, then we will
> > need a new allocation type for this, add it to linker
> > in bios and qemu.
> > 
> > Alternatively, find some other way to get hold of
> > physical memory.
> > Is there a way to disable the log completely?
> > As defined in your patch, I doubt there's anything there, ever ..
> > 
> > 
> > 
> >> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> >> +                                   ACPI_BUILD_TABLE_FILE,
> >> +                                   table_data, 
> &tcpa->log_area_start_address,
> >> + sizeof(tcpa->log_area_start_address));
> >> +    build_header(linker, table_data,
> >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> >> +}
> 
> So here's my understanding. The spec referenced above describes three
> ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> (usable by both client & server platforms) for TPM 2.0.
> 
> The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> 
> This table has a field called LASA (Log Area Start Address) which points
> to somewhere in (guest-)physical memory. The patch adds a "dummy range"
> to the end of the TCPA table itself, and asks the linker to set LASA to
> the beginning of that range.
> 
> This won't work in OVMF, and not just because of the reason that Michael
> mentions (ie. because the firmware, in particular SeaBIOS, might
> allocate the TCPA table in an area that is unsuitable as LASA target).
> 
> Rather, in OVMF this won't work because OVMF doesn't implement the
> linking part of the linker. The *generic* edk2 protocol
> (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF uses
> (as a client) to install ACPI tables in guest-phys memory requires
> tables to be passed in one-by-one.
> 
> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> well-known tables specially. It has knowledge of their internal
> pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
> updates pointers automatically. (For example when you install the FACS,
> the protocol links it automatically into FACP.)
> 
> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to know
> anything about the TCPA table, let alone the unstructured (?) TCG event
> log that is pointed-to by TCPA.LASA.
> 
> (I grepped for the TCPA signature,
> 
EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> 
> This means that if you pass down a TCPA table, OVMF will install it
> right now, but TCPA.LASA will be bogus.
> 
> If I wanted to implement the complete linker as Michael envisioned it,
> then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
> ACPI table installation from zero, trying to mimic the SeaBIOS client
> code, but in a way that matches the UEFI environment. I'm not ready to
> do that. Definitely not without an "official" human-language
> specification of the linker-loader interface.
> 
> I skimmed the patch but I'm not sure what exactly the TPM emulation in
> qemu depends on. Is it a command line option? Is it default for some
> machine types?
> 
> Alternatively, I could recognize the TCPA signature in OVMF when parsing
> the ACPI blobs for table headers, and filter it out.

This is the code for what I would call 'pointer relocation'. The TCPA 
table is not the only place where this is used, but why is it an issue 
there while not with the following?

    fadt->firmware_ctrl = cpu_to_le32(facs);
    /* FACS address to be filled by Guest linker */
    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE, 
                                   ACPI_BUILD_TABLE_FILE,
                                   table_data, &fadt->firmware_ctrl,
                                   sizeof fadt->firmware_ctrl);

Regards,
    Stefan

[-- Attachment #2: Type: text/html, Size: 8373 bytes --]

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

[Michael S. Tsirkin]

On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
> Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
> 
> > From: Laszlo Ersek <lersek@redhat.com>
> > To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan Berger/Watson/IBM@IBMUS
> > Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
> > Date: 07/30/2014 10:36 AM
> > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >
> > On 07/30/14 15:20, Michael S. Tsirkin wrote:
> > > On Tue, Jul 29, 2014 at 06:52:19AM -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
> >
> > (Thanks for CC'ing me, Michael.)
> >
> > I skimmed this spec.
> >
> > >> +static void
> > >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> > >> +{
> > >> +    Acpi20Tcpa *tcpa;
> > >> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> > >> +    uint64_t log_area_start_address;
> > >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> > >> +
> > >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> > >> +
> > >> +    tcpa = acpi_data_push(table_data, len);
> > >> +
> > >> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> > >> +    tcpa->log_area_minimum_length = cpu_to_le32(log_area_minimum_length);
> > >> +    tcpa->log_area_start_address = cpu_to_le64(log_area_start_address);
> > >> +
> > >> +    /* LASA address to be filled by Guest linker */
> > >
> > > Hmm, you are simply allocating log area as part of the ACPI table.  It
> > > works because bios happens to allocate tables from high memory.
> > > But I think this is a problem in practice because
> > > bios is allowed to allocate acpi memory differently.
> > > On the other hand log presumably needs to reside in
> > > physical memory somewhere.
> > >
> > > If you need bios to allocate this memory, then we will
> > > need a new allocation type for this, add it to linker
> > > in bios and qemu.
> > >
> > > Alternatively, find some other way to get hold of
> > > physical memory.
> > > Is there a way to disable the log completely?
> > > As defined in your patch, I doubt there's anything there, ever ..
> > >
> > >
> > >
> > >> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> > >> +                                   ACPI_BUILD_TABLE_FILE,
> > >> +                                   table_data,
> > &tcpa->log_area_start_address,
> > >> +                                   sizeof(tcpa->log_area_start_address));
> > >> +    build_header(linker, table_data,
> > >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> > >> +}
> >
> > So here's my understanding. The spec referenced above describes three
> > ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> > (usable by both client & server platforms) for TPM 2.0.
> >
> > The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> >
> > This table has a field called LASA (Log Area Start Address) which points
> > to somewhere in (guest-)physical memory. The patch adds a "dummy range"
> > to the end of the TCPA table itself, and asks the linker to set LASA to
> > the beginning of that range.
> >
> > This won't work in OVMF, and not just because of the reason that Michael
> > mentions (ie. because the firmware, in particular SeaBIOS, might
> > allocate the TCPA table in an area that is unsuitable as LASA target).
> >
> > Rather, in OVMF this won't work because OVMF doesn't implement the
> > linking part of the linker. The *generic* edk2 protocol
> > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF uses
> > (as a client) to install ACPI tables in guest-phys memory requires
> > tables to be passed in one-by-one.
> >
> > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> > well-known tables specially. It has knowledge of their internal
> > pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
> > updates pointers automatically. (For example when you install the FACS,
> > the protocol links it automatically into FACP.)
> >
> > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to know
> > anything about the TCPA table, let alone the unstructured (?) TCG event
> > log that is pointed-to by TCPA.LASA.
> >
> > (I grepped for the TCPA signature,
> >
> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> >
> > This means that if you pass down a TCPA table, OVMF will install it
> > right now, but TCPA.LASA will be bogus.
> >
> > If I wanted to implement the complete linker as Michael envisioned it,
> > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
> > ACPI table installation from zero, trying to mimic the SeaBIOS client
> > code, but in a way that matches the UEFI environment. I'm not ready to
> > do that. Definitely not without an "official" human-language
> > specification of the linker-loader interface.
> >
> > I skimmed the patch but I'm not sure what exactly the TPM emulation in
> > qemu depends on. Is it a command line option? Is it default for some
> > machine types?
> >
> > Alternatively, I could recognize the TCPA signature in OVMF when parsing
> > the ACPI blobs for table headers, and filter it out.
> 
> This is the code for what I would call 'pointer relocation'. The TCPA table is
> not the only place where this is used, but why is it an issue there while not
> with the following?
> 
>     fadt->firmware_ctrl = cpu_to_le32(facs);
>     /* FACS address to be filled by Guest linker */
>     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
>                                    ACPI_BUILD_TABLE_FILE,
>                                    table_data, &fadt->firmware_ctrl,
>                                    sizeof fadt->firmware_ctrl);
> 
> Regards,
>     Stefan


Becase FACS is an ACPI table. So BIOS allocates it
from E820_RESERVED at the moment but it does not have to,
it could mark it with E820_ACPI.
Guest can then interpret the tables and then release the
memory if it wishes.

If you want to do it for TCPA you must tell bios that
this is not ACPI memory.


-- 
MST

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

[Stefan Berger]

[-- Attachment #1: Type: text/plain, Size: 7240 bytes --]

"Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:20:41 AM:

> From: "Michael S. Tsirkin" <mst@redhat.com>
> To: Stefan Berger/Watson/IBM@IBMUS
> Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan 
> Berger <stefanb@linux.vnet.ibm.com>
> Date: 07/30/2014 11:20 AM
> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> 
> On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
> > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
> > 
> > > From: Laszlo Ersek <lersek@redhat.com>
> > > To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan 
Berger/Watson/IBM@IBMUS
> > > Cc: qemu-devel@nongnu.org, Stefan Berger 
<stefanb@linux.vnet.ibm.com>
> > > Date: 07/30/2014 10:36 AM
> > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> > >
> > > On 07/30/14 15:20, Michael S. Tsirkin wrote:
> > > > On Tue, Jul 29, 2014 at 06:52:19AM -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
> > >
> > > (Thanks for CC'ing me, Michael.)
> > >
> > > I skimmed this spec.
> > >
> > > >> +static void
> > > >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> > > >> +{
> > > >> +    Acpi20Tcpa *tcpa;
> > > >> +    uint32_t log_area_minimum_length = 
TPM_LOG_AREA_MINIMUM_SIZE;
> > > >> +    uint64_t log_area_start_address;
> > > >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> > > >> +
> > > >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> > > >> +
> > > >> +    tcpa = acpi_data_push(table_data, len);
> > > >> +
> > > >> +    tcpa->platform_class = 
cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> > > >> +    tcpa->log_area_minimum_length = cpu_to_le32
> (log_area_minimum_length);
> > > >> +    tcpa->log_area_start_address = cpu_to_le64
> (log_area_start_address);
> > > >> +
> > > >> +    /* LASA address to be filled by Guest linker */
> > > >
> > > > Hmm, you are simply allocating log area as part of the ACPI table. 
 It
> > > > works because bios happens to allocate tables from high memory.
> > > > But I think this is a problem in practice because
> > > > bios is allowed to allocate acpi memory differently.
> > > > On the other hand log presumably needs to reside in
> > > > physical memory somewhere.
> > > >
> > > > If you need bios to allocate this memory, then we will
> > > > need a new allocation type for this, add it to linker
> > > > in bios and qemu.
> > > >
> > > > Alternatively, find some other way to get hold of
> > > > physical memory.
> > > > Is there a way to disable the log completely?
> > > > As defined in your patch, I doubt there's anything there, ever ..
> > > >
> > > >
> > > >
> > > >> +    bios_linker_loader_add_pointer(linker, 
ACPI_BUILD_TABLE_FILE,
> > > >> +                                   ACPI_BUILD_TABLE_FILE,
> > > >> +                                   table_data,
> > > &tcpa->log_area_start_address,
> > > >> +                                   sizeof
> (tcpa->log_area_start_address));
> > > >> +    build_header(linker, table_data,
> > > >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> > > >> +}
> > >
> > > So here's my understanding. The spec referenced above describes 
three
> > > ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> > > (usable by both client & server platforms) for TPM 2.0.
> > >
> > > The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> > >
> > > This table has a field called LASA (Log Area Start Address) which 
points
> > > to somewhere in (guest-)physical memory. The patch adds a "dummy 
range"
> > > to the end of the TCPA table itself, and asks the linker to set LASA 
to
> > > the beginning of that range.
> > >
> > > This won't work in OVMF, and not just because of the reason that 
Michael
> > > mentions (ie. because the firmware, in particular SeaBIOS, might
> > > allocate the TCPA table in an area that is unsuitable as LASA 
target).
> > >
> > > Rather, in OVMF this won't work because OVMF doesn't implement the
> > > linking part of the linker. The *generic* edk2 protocol
> > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF 
uses
> > > (as a client) to install ACPI tables in guest-phys memory requires
> > > tables to be passed in one-by-one.
> > >
> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> > > well-known tables specially. It has knowledge of their internal
> > > pointers, and when you install an ACPI table, 
EFI_ACPI_TABLE_PROTOCOL
> > > updates pointers automatically. (For example when you install the 
FACS,
> > > the protocol links it automatically into FACP.)
> > >
> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to 
know
> > > anything about the TCPA table, let alone the unstructured (?) TCG 
event
> > > log that is pointed-to by TCPA.LASA.
> > >
> > > (I grepped for the TCPA signature,
> > >
> > 
> 
EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> > >
> > > This means that if you pass down a TCPA table, OVMF will install it
> > > right now, but TCPA.LASA will be bogus.
> > >
> > > If I wanted to implement the complete linker as Michael envisioned 
it,
> > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
> > > ACPI table installation from zero, trying to mimic the SeaBIOS 
client
> > > code, but in a way that matches the UEFI environment. I'm not ready 
to
> > > do that. Definitely not without an "official" human-language
> > > specification of the linker-loader interface.
> > >
> > > I skimmed the patch but I'm not sure what exactly the TPM emulation 
in
> > > qemu depends on. Is it a command line option? Is it default for some
> > > machine types?
> > >
> > > Alternatively, I could recognize the TCPA signature in OVMF when 
parsing
> > > the ACPI blobs for table headers, and filter it out.
> > 
> > This is the code for what I would call 'pointer relocation'. The 
> TCPA table is
> > not the only place where this is used, but why is it an issue 
> there while not
> > with the following?
> > 
> >     fadt->firmware_ctrl = cpu_to_le32(facs);
> >     /* FACS address to be filled by Guest linker */
> >     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE, 
> >                                    ACPI_BUILD_TABLE_FILE,
> >                                    table_data, &fadt->firmware_ctrl,
> >                                    sizeof fadt->firmware_ctrl);
> > 
> > Regards,
> >     Stefan
> 
> 
> Becase FACS is an ACPI table. So BIOS allocates it
> from E820_RESERVED at the moment but it does not have to,
> it could mark it with E820_ACPI.
> Guest can then interpret the tables and then release the
> memory if it wishes.
> 
> If you want to do it for TCPA you must tell bios that
> this is not ACPI memory.

I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT would need a 
similar tag to keep the S3 resume vector around?

Stefan

[-- Attachment #2: Type: text/html, Size: 10631 bytes --]

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

[Laszlo Ersek]

On 07/30/14 17:29, Stefan Berger wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:20:41 AM:
> 
>> From: "Michael S. Tsirkin" <mst@redhat.com>
>> To: Stefan Berger/Watson/IBM@IBMUS
>> Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan
>> Berger <stefanb@linux.vnet.ibm.com>
>> Date: 07/30/2014 11:20 AM
>> Subject: Re: [PATCH v2] Add ACPI tables for TPM
>>
>> On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
>> > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
>> >
>> > > From: Laszlo Ersek <lersek@redhat.com>
>> > > To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan
> Berger/Watson/IBM@IBMUS
>> > > Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
>> > > Date: 07/30/2014 10:36 AM
>> > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
>> > >
>> > > On 07/30/14 15:20, Michael S. Tsirkin wrote:
>> > > > On Tue, Jul 29, 2014 at 06:52:19AM -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
>> > >
>> > > (Thanks for CC'ing me, Michael.)
>> > >
>> > > I skimmed this spec.
>> > >
>> > > >> +static void
>> > > >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
>> > > >> +{
>> > > >> +    Acpi20Tcpa *tcpa;
>> > > >> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
>> > > >> +    uint64_t log_area_start_address;
>> > > >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
>> > > >> +
>> > > >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
>> > > >> +
>> > > >> +    tcpa = acpi_data_push(table_data, len);
>> > > >> +
>> > > >> +    tcpa->platform_class =
> cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
>> > > >> +    tcpa->log_area_minimum_length = cpu_to_le32
>> (log_area_minimum_length);
>> > > >> +    tcpa->log_area_start_address = cpu_to_le64
>> (log_area_start_address);
>> > > >> +
>> > > >> +    /* LASA address to be filled by Guest linker */
>> > > >
>> > > > Hmm, you are simply allocating log area as part of the ACPI
> table.  It
>> > > > works because bios happens to allocate tables from high memory.
>> > > > But I think this is a problem in practice because
>> > > > bios is allowed to allocate acpi memory differently.
>> > > > On the other hand log presumably needs to reside in
>> > > > physical memory somewhere.
>> > > >
>> > > > If you need bios to allocate this memory, then we will
>> > > > need a new allocation type for this, add it to linker
>> > > > in bios and qemu.
>> > > >
>> > > > Alternatively, find some other way to get hold of
>> > > > physical memory.
>> > > > Is there a way to disable the log completely?
>> > > > As defined in your patch, I doubt there's anything there, ever ..
>> > > >
>> > > >
>> > > >
>> > > >> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> > > >> +                                   ACPI_BUILD_TABLE_FILE,
>> > > >> +                                   table_data,
>> > > &tcpa->log_area_start_address,
>> > > >> +                                   sizeof
>> (tcpa->log_area_start_address));
>> > > >> +    build_header(linker, table_data,
>> > > >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
>> > > >> +}
>> > >
>> > > So here's my understanding. The spec referenced above describes three
>> > > ACPI tables: two (client vs. server) for TPM 1.2, and a third one
>> > > (usable by both client & server platforms) for TPM 2.0.
>> > >
>> > > The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
>> > >
>> > > This table has a field called LASA (Log Area Start Address) which
> points
>> > > to somewhere in (guest-)physical memory. The patch adds a "dummy
> range"
>> > > to the end of the TCPA table itself, and asks the linker to set
> LASA to
>> > > the beginning of that range.
>> > >
>> > > This won't work in OVMF, and not just because of the reason that
> Michael
>> > > mentions (ie. because the firmware, in particular SeaBIOS, might
>> > > allocate the TCPA table in an area that is unsuitable as LASA target).
>> > >
>> > > Rather, in OVMF this won't work because OVMF doesn't implement the
>> > > linking part of the linker. The *generic* edk2 protocol
>> > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that
> OVMF uses
>> > > (as a client) to install ACPI tables in guest-phys memory requires
>> > > tables to be passed in one-by-one.
>> > >
>> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
>> > > well-known tables specially. It has knowledge of their internal
>> > > pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
>> > > updates pointers automatically. (For example when you install the
> FACS,
>> > > the protocol links it automatically into FACP.)
>> > >
>> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to
> know
>> > > anything about the TCPA table, let alone the unstructured (?) TCG
> event
>> > > log that is pointed-to by TCPA.LASA.
>> > >
>> > > (I grepped for the TCPA signature,
>> > >
>> >
>>
> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
>> > >
>> > > This means that if you pass down a TCPA table, OVMF will install it
>> > > right now, but TCPA.LASA will be bogus.
>> > >
>> > > If I wanted to implement the complete linker as Michael envisioned it,
>> > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
>> > > ACPI table installation from zero, trying to mimic the SeaBIOS client
>> > > code, but in a way that matches the UEFI environment. I'm not ready to
>> > > do that. Definitely not without an "official" human-language
>> > > specification of the linker-loader interface.
>> > >
>> > > I skimmed the patch but I'm not sure what exactly the TPM emulation in
>> > > qemu depends on. Is it a command line option? Is it default for some
>> > > machine types?
>> > >
>> > > Alternatively, I could recognize the TCPA signature in OVMF when
> parsing
>> > > the ACPI blobs for table headers, and filter it out.
>> >
>> > This is the code for what I would call 'pointer relocation'. The
>> TCPA table is
>> > not the only place where this is used, but why is it an issue
>> there while not
>> > with the following?
>> >
>> >     fadt->firmware_ctrl = cpu_to_le32(facs);
>> >     /* FACS address to be filled by Guest linker */
>> >     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
>> >                                    ACPI_BUILD_TABLE_FILE,
>> >                                    table_data, &fadt->firmware_ctrl,
>> >                                    sizeof fadt->firmware_ctrl);
>> >
>> > Regards,
>> >     Stefan
>>
>>
>> Becase FACS is an ACPI table. So BIOS allocates it
>> from E820_RESERVED at the moment but it does not have to,
>> it could mark it with E820_ACPI.
>> Guest can then interpret the tables and then release the
>> memory if it wishes.
>>
>> If you want to do it for TCPA you must tell bios that
>> this is not ACPI memory.
> 
> I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT would need
> a similar tag to keep the S3 resume vector around?

Not in OVMF, because edk2's EFI_ACPI_TABLE_PROTOCOL special cases FACS
(containing the S3 resume vector), allocating it in EfiACPIMemoryNVS memory.

Table 26. Memory Type Usage after ExitBootServices()
EfiACPIMemoryNVS: This memory is to be preserved by the loader and OS
                  in the working and ACPI S1–S3 states.

Thanks
Laszlo

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

[Stefan Berger]

[-- Attachment #1: Type: text/plain, Size: 8850 bytes --]

Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 11:41:10 AM:

> From: Laszlo Ersek <lersek@redhat.com>
> To: Stefan Berger/Watson/IBM@IBMUS, "Michael S. Tsirkin" 
<mst@redhat.com>
> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
> Date: 07/30/2014 11:41 AM
> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> 
> On 07/30/14 17:29, Stefan Berger wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:20:41 AM:
> > 
> >> From: "Michael S. Tsirkin" <mst@redhat.com>
> >> To: Stefan Berger/Watson/IBM@IBMUS
> >> Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan
> >> Berger <stefanb@linux.vnet.ibm.com>
> >> Date: 07/30/2014 11:20 AM
> >> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >>
> >> On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
> >> > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
> >> >
> >> > > From: Laszlo Ersek <lersek@redhat.com>
> >> > > To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan
> > Berger/Watson/IBM@IBMUS
> >> > > Cc: qemu-devel@nongnu.org, Stefan Berger 
<stefanb@linux.vnet.ibm.com>
> >> > > Date: 07/30/2014 10:36 AM
> >> > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >> > >
> >> > > On 07/30/14 15:20, Michael S. Tsirkin wrote:
> >> > > > On Tue, Jul 29, 2014 at 06:52:19AM -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
> >> > >
> >> > > (Thanks for CC'ing me, Michael.)
> >> > >
> >> > > I skimmed this spec.
> >> > >
> >> > > >> +static void
> >> > > >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> >> > > >> +{
> >> > > >> +    Acpi20Tcpa *tcpa;
> >> > > >> +    uint32_t log_area_minimum_length = 
TPM_LOG_AREA_MINIMUM_SIZE;
> >> > > >> +    uint64_t log_area_start_address;
> >> > > >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> >> > > >> +
> >> > > >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> >> > > >> +
> >> > > >> +    tcpa = acpi_data_push(table_data, len);
> >> > > >> +
> >> > > >> +    tcpa->platform_class =
> > cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> >> > > >> +    tcpa->log_area_minimum_length = cpu_to_le32
> >> (log_area_minimum_length);
> >> > > >> +    tcpa->log_area_start_address = cpu_to_le64
> >> (log_area_start_address);
> >> > > >> +
> >> > > >> +    /* LASA address to be filled by Guest linker */
> >> > > >
> >> > > > Hmm, you are simply allocating log area as part of the ACPI
> > table.  It
> >> > > > works because bios happens to allocate tables from high memory.
> >> > > > But I think this is a problem in practice because
> >> > > > bios is allowed to allocate acpi memory differently.
> >> > > > On the other hand log presumably needs to reside in
> >> > > > physical memory somewhere.
> >> > > >
> >> > > > If you need bios to allocate this memory, then we will
> >> > > > need a new allocation type for this, add it to linker
> >> > > > in bios and qemu.
> >> > > >
> >> > > > Alternatively, find some other way to get hold of
> >> > > > physical memory.
> >> > > > Is there a way to disable the log completely?
> >> > > > As defined in your patch, I doubt there's anything there, ever 
..
> >> > > >
> >> > > >
> >> > > >
> >> > > >> +    bios_linker_loader_add_pointer(linker, 
ACPI_BUILD_TABLE_FILE,
> >> > > >> +                                   ACPI_BUILD_TABLE_FILE,
> >> > > >> +                                   table_data,
> >> > > &tcpa->log_area_start_address,
> >> > > >> +                                   sizeof
> >> (tcpa->log_area_start_address));
> >> > > >> +    build_header(linker, table_data,
> >> > > >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> >> > > >> +}
> >> > >
> >> > > So here's my understanding. The spec referenced above describes 
three
> >> > > ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> >> > > (usable by both client & server platforms) for TPM 2.0.
> >> > >
> >> > > The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> >> > >
> >> > > This table has a field called LASA (Log Area Start Address) which
> > points
> >> > > to somewhere in (guest-)physical memory. The patch adds a "dummy
> > range"
> >> > > to the end of the TCPA table itself, and asks the linker to set
> > LASA to
> >> > > the beginning of that range.
> >> > >
> >> > > This won't work in OVMF, and not just because of the reason that
> > Michael
> >> > > mentions (ie. because the firmware, in particular SeaBIOS, might
> >> > > allocate the TCPA table in an area that is unsuitable as LASA 
target).
> >> > >
> >> > > Rather, in OVMF this won't work because OVMF doesn't implement 
the
> >> > > linking part of the linker. The *generic* edk2 protocol
> >> > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that
> > OVMF uses
> >> > > (as a client) to install ACPI tables in guest-phys memory 
requires
> >> > > tables to be passed in one-by-one.
> >> > >
> >> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> >> > > well-known tables specially. It has knowledge of their internal
> >> > > pointers, and when you install an ACPI table, 
EFI_ACPI_TABLE_PROTOCOL
> >> > > updates pointers automatically. (For example when you install the
> > FACS,
> >> > > the protocol links it automatically into FACP.)
> >> > >
> >> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem 
to
> > know
> >> > > anything about the TCPA table, let alone the unstructured (?) TCG
> > event
> >> > > log that is pointed-to by TCPA.LASA.
> >> > >
> >> > > (I grepped for the TCPA signature,
> >> > >
> >> >
> >>
> > 
> 
EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> >> > >
> >> > > This means that if you pass down a TCPA table, OVMF will install 
it
> >> > > right now, but TCPA.LASA will be bogus.
> >> > >
> >> > > If I wanted to implement the complete linker as Michael 
envisioned it,
> >> > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and 
implement
> >> > > ACPI table installation from zero, trying to mimic the SeaBIOS 
client
> >> > > code, but in a way that matches the UEFI environment. I'm not 
ready to
> >> > > do that. Definitely not without an "official" human-language
> >> > > specification of the linker-loader interface.
> >> > >
> >> > > I skimmed the patch but I'm not sure what exactly the TPM 
emulation in
> >> > > qemu depends on. Is it a command line option? Is it default for 
some
> >> > > machine types?
> >> > >
> >> > > Alternatively, I could recognize the TCPA signature in OVMF when
> > parsing
> >> > > the ACPI blobs for table headers, and filter it out.
> >> >
> >> > This is the code for what I would call 'pointer relocation'. The
> >> TCPA table is
> >> > not the only place where this is used, but why is it an issue
> >> there while not
> >> > with the following?
> >> >
> >> >     fadt->firmware_ctrl = cpu_to_le32(facs);
> >> >     /* FACS address to be filled by Guest linker */
> >> >     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE, 
> >> >                                    ACPI_BUILD_TABLE_FILE,
> >> >                                    table_data, 
&fadt->firmware_ctrl,
> >> >                                    sizeof fadt->firmware_ctrl);
> >> >
> >> > Regards,
> >> >     Stefan
> >>
> >>
> >> Becase FACS is an ACPI table. So BIOS allocates it
> >> from E820_RESERVED at the moment but it does not have to,
> >> it could mark it with E820_ACPI.
> >> Guest can then interpret the tables and then release the
> >> memory if it wishes.
> >>
> >> If you want to do it for TCPA you must tell bios that
> >> this is not ACPI memory.
> > 
> > I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT would 
need
> > a similar tag to keep the S3 resume vector around?
> 
> Not in OVMF, because edk2's EFI_ACPI_TABLE_PROTOCOL special cases FACS
> (containing the S3 resume vector), allocating it in EfiACPIMemoryNVS 
memory.
> 
> Table 26. Memory Type Usage after ExitBootServices()
> EfiACPIMemoryNVS: This memory is to be preserved by the loader and OS
>                   in the working and ACPI S1–S3 states.
> 

So what is a solution then for OVMF? Add another special case for TCPA? Is 
this counter to the specs ? Skip TCPA?

   Stefan


[-- Attachment #2: Type: text/html, Size: 13629 bytes --]

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

[Laszlo Ersek]

On 07/30/14 17:44, Stefan Berger wrote:
> Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 11:41:10 AM:
> 
>> From: Laszlo Ersek <lersek@redhat.com>
>> To: Stefan Berger/Watson/IBM@IBMUS, "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Date: 07/30/2014 11:41 AM
>> Subject: Re: [PATCH v2] Add ACPI tables for TPM
>>
>> On 07/30/14 17:29, Stefan Berger wrote:
>> > "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:20:41 AM:
>> >
>> >> From: "Michael S. Tsirkin" <mst@redhat.com>
>> >> To: Stefan Berger/Watson/IBM@IBMUS
>> >> Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan
>> >> Berger <stefanb@linux.vnet.ibm.com>
>> >> Date: 07/30/2014 11:20 AM
>> >> Subject: Re: [PATCH v2] Add ACPI tables for TPM
>> >>
>> >> On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
>> >> > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
>> >> >
>> >> > > From: Laszlo Ersek <lersek@redhat.com>
>> >> > > To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan
>> > Berger/Watson/IBM@IBMUS
>> >> > > Cc: qemu-devel@nongnu.org, Stefan Berger
> <stefanb@linux.vnet.ibm.com>
>> >> > > Date: 07/30/2014 10:36 AM
>> >> > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
>> >> > >
>> >> > > On 07/30/14 15:20, Michael S. Tsirkin wrote:
>> >> > > > On Tue, Jul 29, 2014 at 06:52:19AM -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
>> >> > >
>> >> > > (Thanks for CC'ing me, Michael.)
>> >> > >
>> >> > > I skimmed this spec.
>> >> > >
>> >> > > >> +static void
>> >> > > >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
>> >> > > >> +{
>> >> > > >> +    Acpi20Tcpa *tcpa;
>> >> > > >> +    uint32_t log_area_minimum_length =
> TPM_LOG_AREA_MINIMUM_SIZE;
>> >> > > >> +    uint64_t log_area_start_address;
>> >> > > >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
>> >> > > >> +
>> >> > > >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
>> >> > > >> +
>> >> > > >> +    tcpa = acpi_data_push(table_data, len);
>> >> > > >> +
>> >> > > >> +    tcpa->platform_class =
>> > cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
>> >> > > >> +    tcpa->log_area_minimum_length = cpu_to_le32
>> >> (log_area_minimum_length);
>> >> > > >> +    tcpa->log_area_start_address = cpu_to_le64
>> >> (log_area_start_address);
>> >> > > >> +
>> >> > > >> +    /* LASA address to be filled by Guest linker */
>> >> > > >
>> >> > > > Hmm, you are simply allocating log area as part of the ACPI
>> > table.  It
>> >> > > > works because bios happens to allocate tables from high memory.
>> >> > > > But I think this is a problem in practice because
>> >> > > > bios is allowed to allocate acpi memory differently.
>> >> > > > On the other hand log presumably needs to reside in
>> >> > > > physical memory somewhere.
>> >> > > >
>> >> > > > If you need bios to allocate this memory, then we will
>> >> > > > need a new allocation type for this, add it to linker
>> >> > > > in bios and qemu.
>> >> > > >
>> >> > > > Alternatively, find some other way to get hold of
>> >> > > > physical memory.
>> >> > > > Is there a way to disable the log completely?
>> >> > > > As defined in your patch, I doubt there's anything there, ever ..
>> >> > > >
>> >> > > >
>> >> > > >
>> >> > > >> +    bios_linker_loader_add_pointer(linker,
> ACPI_BUILD_TABLE_FILE,
>> >> > > >> +                                   ACPI_BUILD_TABLE_FILE,
>> >> > > >> +                                   table_data,
>> >> > > &tcpa->log_area_start_address,
>> >> > > >> +                                   sizeof
>> >> (tcpa->log_area_start_address));
>> >> > > >> +    build_header(linker, table_data,
>> >> > > >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
>> >> > > >> +}
>> >> > >
>> >> > > So here's my understanding. The spec referenced above describes
> three
>> >> > > ACPI tables: two (client vs. server) for TPM 1.2, and a third one
>> >> > > (usable by both client & server platforms) for TPM 2.0.
>> >> > >
>> >> > > The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
>> >> > >
>> >> > > This table has a field called LASA (Log Area Start Address) which
>> > points
>> >> > > to somewhere in (guest-)physical memory. The patch adds a "dummy
>> > range"
>> >> > > to the end of the TCPA table itself, and asks the linker to set
>> > LASA to
>> >> > > the beginning of that range.
>> >> > >
>> >> > > This won't work in OVMF, and not just because of the reason that
>> > Michael
>> >> > > mentions (ie. because the firmware, in particular SeaBIOS, might
>> >> > > allocate the TCPA table in an area that is unsuitable as LASA
> target).
>> >> > >
>> >> > > Rather, in OVMF this won't work because OVMF doesn't implement the
>> >> > > linking part of the linker. The *generic* edk2 protocol
>> >> > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that
>> > OVMF uses
>> >> > > (as a client) to install ACPI tables in guest-phys memory requires
>> >> > > tables to be passed in one-by-one.
>> >> > >
>> >> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
>> >> > > well-known tables specially. It has knowledge of their internal
>> >> > > pointers, and when you install an ACPI table,
> EFI_ACPI_TABLE_PROTOCOL
>> >> > > updates pointers automatically. (For example when you install the
>> > FACS,
>> >> > > the protocol links it automatically into FACP.)
>> >> > >
>> >> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to
>> > know
>> >> > > anything about the TCPA table, let alone the unstructured (?) TCG
>> > event
>> >> > > log that is pointed-to by TCPA.LASA.
>> >> > >
>> >> > > (I grepped for the TCPA signature,
>> >> > >
>> >> >
>> >>
>> >
>>
> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
>> >> > >
>> >> > > This means that if you pass down a TCPA table, OVMF will install it
>> >> > > right now, but TCPA.LASA will be bogus.
>> >> > >
>> >> > > If I wanted to implement the complete linker as Michael
> envisioned it,
>> >> > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and
> implement
>> >> > > ACPI table installation from zero, trying to mimic the SeaBIOS
> client
>> >> > > code, but in a way that matches the UEFI environment. I'm not
> ready to
>> >> > > do that. Definitely not without an "official" human-language
>> >> > > specification of the linker-loader interface.
>> >> > >
>> >> > > I skimmed the patch but I'm not sure what exactly the TPM
> emulation in
>> >> > > qemu depends on. Is it a command line option? Is it default for
> some
>> >> > > machine types?
>> >> > >
>> >> > > Alternatively, I could recognize the TCPA signature in OVMF when
>> > parsing
>> >> > > the ACPI blobs for table headers, and filter it out.
>> >> >
>> >> > This is the code for what I would call 'pointer relocation'. The
>> >> TCPA table is
>> >> > not the only place where this is used, but why is it an issue
>> >> there while not
>> >> > with the following?
>> >> >
>> >> >     fadt->firmware_ctrl = cpu_to_le32(facs);
>> >> >     /* FACS address to be filled by Guest linker */
>> >> >     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
>> >> >                                    ACPI_BUILD_TABLE_FILE,
>> >> >                                    table_data, &fadt->firmware_ctrl,
>> >> >                                    sizeof fadt->firmware_ctrl);
>> >> >
>> >> > Regards,
>> >> >     Stefan
>> >>
>> >>
>> >> Becase FACS is an ACPI table. So BIOS allocates it
>> >> from E820_RESERVED at the moment but it does not have to,
>> >> it could mark it with E820_ACPI.
>> >> Guest can then interpret the tables and then release the
>> >> memory if it wishes.
>> >>
>> >> If you want to do it for TCPA you must tell bios that
>> >> this is not ACPI memory.
>> >
>> > I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT would need
>> > a similar tag to keep the S3 resume vector around?
>>
>> Not in OVMF, because edk2's EFI_ACPI_TABLE_PROTOCOL special cases FACS
>> (containing the S3 resume vector), allocating it in EfiACPIMemoryNVS
> memory.
>>
>> Table 26. Memory Type Usage after ExitBootServices()
>> EfiACPIMemoryNVS: This memory is to be preserved by the loader and OS
>>                   in the working and ACPI S1–S3 states.
>>
> 
> So what is a solution then for OVMF? Add another special case for TCPA?
> Is this counter to the specs ? Skip TCPA?

In the short term, probably skip TCPA, or advise users in documentation
not to enable the TPM device when running OVMF.

In the long term, I'm trying to convince myself to implement the full
linker/loader interface in OVMF. I'll probably have to stop relying on
EFI_ACPI_TABLE_PROTOCOL too. As I said, I don't think I'll start that
without a formal interface specification under qemu's docs/ directory,
one that is complete and binds the QEMU code too.

Thanks
Laszlo

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

[Stefan Berger]

[-- Attachment #1: Type: text/plain, Size: 10183 bytes --]

Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 11:58:52 AM:

> From: Laszlo Ersek <lersek@redhat.com>
> To: Stefan Berger/Watson/IBM@IBMUS
> Cc: "Michael S. Tsirkin" <mst@redhat.com>, qemu-devel@nongnu.org, 
> Stefan Berger <stefanb@linux.vnet.ibm.com>
> Date: 07/30/2014 11:59 AM
> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> 
> On 07/30/14 17:44, Stefan Berger wrote:
> > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 11:41:10 AM:
> > 
> >> From: Laszlo Ersek <lersek@redhat.com>
> >> To: Stefan Berger/Watson/IBM@IBMUS, "Michael S. Tsirkin" 
<mst@redhat.com>
> >> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Date: 07/30/2014 11:41 AM
> >> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >>
> >> On 07/30/14 17:29, Stefan Berger wrote:
> >> > "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:20:41 
AM:
> >> >
> >> >> From: "Michael S. Tsirkin" <mst@redhat.com>
> >> >> To: Stefan Berger/Watson/IBM@IBMUS
> >> >> Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, 
Stefan
> >> >> Berger <stefanb@linux.vnet.ibm.com>
> >> >> Date: 07/30/2014 11:20 AM
> >> >> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >> >>
> >> >> On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
> >> >> > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 
AM:
> >> >> >
> >> >> > > From: Laszlo Ersek <lersek@redhat.com>
> >> >> > > To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan
> >> > Berger/Watson/IBM@IBMUS
> >> >> > > Cc: qemu-devel@nongnu.org, Stefan Berger
> > <stefanb@linux.vnet.ibm.com>
> >> >> > > Date: 07/30/2014 10:36 AM
> >> >> > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >> >> > >
> >> >> > > On 07/30/14 15:20, Michael S. Tsirkin wrote:
> >> >> > > > On Tue, Jul 29, 2014 at 06:52:19AM -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
> >> >> > >
> >> >> > > (Thanks for CC'ing me, Michael.)
> >> >> > >
> >> >> > > I skimmed this spec.
> >> >> > >
> >> >> > > >> +static void
> >> >> > > >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> >> >> > > >> +{
> >> >> > > >> +    Acpi20Tcpa *tcpa;
> >> >> > > >> +    uint32_t log_area_minimum_length =
> > TPM_LOG_AREA_MINIMUM_SIZE;
> >> >> > > >> +    uint64_t log_area_start_address;
> >> >> > > >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> >> >> > > >> +
> >> >> > > >> +    log_area_start_address = table_data->len + 
sizeof(*tcpa);
> >> >> > > >> +
> >> >> > > >> +    tcpa = acpi_data_push(table_data, len);
> >> >> > > >> +
> >> >> > > >> +    tcpa->platform_class =
> >> > cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> >> >> > > >> +    tcpa->log_area_minimum_length = cpu_to_le32
> >> >> (log_area_minimum_length);
> >> >> > > >> +    tcpa->log_area_start_address = cpu_to_le64
> >> >> (log_area_start_address);
> >> >> > > >> +
> >> >> > > >> +    /* LASA address to be filled by Guest linker */
> >> >> > > >
> >> >> > > > Hmm, you are simply allocating log area as part of the ACPI
> >> > table.  It
> >> >> > > > works because bios happens to allocate tables from high 
memory.
> >> >> > > > But I think this is a problem in practice because
> >> >> > > > bios is allowed to allocate acpi memory differently.
> >> >> > > > On the other hand log presumably needs to reside in
> >> >> > > > physical memory somewhere.
> >> >> > > >
> >> >> > > > If you need bios to allocate this memory, then we will
> >> >> > > > need a new allocation type for this, add it to linker
> >> >> > > > in bios and qemu.
> >> >> > > >
> >> >> > > > Alternatively, find some other way to get hold of
> >> >> > > > physical memory.
> >> >> > > > Is there a way to disable the log completely?
> >> >> > > > As defined in your patch, I doubt there's anything there, 
ever ..
> >> >> > > >
> >> >> > > >
> >> >> > > >
> >> >> > > >> +    bios_linker_loader_add_pointer(linker,
> > ACPI_BUILD_TABLE_FILE,
> >> >> > > >> +                                   ACPI_BUILD_TABLE_FILE,
> >> >> > > >> +                                   table_data,
> >> >> > > &tcpa->log_area_start_address,
> >> >> > > >> +                                   sizeof
> >> >> (tcpa->log_area_start_address));
> >> >> > > >> +    build_header(linker, table_data,
> >> >> > > >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> >> >> > > >> +}
> >> >> > >
> >> >> > > So here's my understanding. The spec referenced above 
describes
> > three
> >> >> > > ACPI tables: two (client vs. server) for TPM 1.2, and a third 
one
> >> >> > > (usable by both client & server platforms) for TPM 2.0.
> >> >> > >
> >> >> > > The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> >> >> > >
> >> >> > > This table has a field called LASA (Log Area Start Address) 
which
> >> > points
> >> >> > > to somewhere in (guest-)physical memory. The patch adds a 
"dummy
> >> > range"
> >> >> > > to the end of the TCPA table itself, and asks the linker to 
set
> >> > LASA to
> >> >> > > the beginning of that range.
> >> >> > >
> >> >> > > This won't work in OVMF, and not just because of the reason 
that
> >> > Michael
> >> >> > > mentions (ie. because the firmware, in particular SeaBIOS, 
might
> >> >> > > allocate the TCPA table in an area that is unsuitable as LASA
> > target).
> >> >> > >
> >> >> > > Rather, in OVMF this won't work because OVMF doesn't implement 
the
> >> >> > > linking part of the linker. The *generic* edk2 protocol
> >> >> > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that
> >> > OVMF uses
> >> >> > > (as a client) to install ACPI tables in guest-phys memory 
requires
> >> >> > > tables to be passed in one-by-one.
> >> >> > >
> >> >> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles 
*some*
> >> >> > > well-known tables specially. It has knowledge of their 
internal
> >> >> > > pointers, and when you install an ACPI table,
> > EFI_ACPI_TABLE_PROTOCOL
> >> >> > > updates pointers automatically. (For example when you install 
the
> >> > FACS,
> >> >> > > the protocol links it automatically into FACP.)
> >> >> > >
> >> >> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't 
seem to
> >> > know
> >> >> > > anything about the TCPA table, let alone the unstructured (?) 
TCG
> >> > event
> >> >> > > log that is pointed-to by TCPA.LASA.
> >> >> > >
> >> >> > > (I grepped for the TCPA signature,
> >> >> > >
> >> >> >
> >> >>
> >> >
> >>
> > 
> 
EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> >> >> > >
> >> >> > > This means that if you pass down a TCPA table, OVMF will 
install it
> >> >> > > right now, but TCPA.LASA will be bogus.
> >> >> > >
> >> >> > > If I wanted to implement the complete linker as Michael
> > envisioned it,
> >> >> > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and
> > implement
> >> >> > > ACPI table installation from zero, trying to mimic the SeaBIOS
> > client
> >> >> > > code, but in a way that matches the UEFI environment. I'm not
> > ready to
> >> >> > > do that. Definitely not without an "official" human-language
> >> >> > > specification of the linker-loader interface.
> >> >> > >
> >> >> > > I skimmed the patch but I'm not sure what exactly the TPM
> > emulation in
> >> >> > > qemu depends on. Is it a command line option? Is it default 
for
> > some
> >> >> > > machine types?
> >> >> > >
> >> >> > > Alternatively, I could recognize the TCPA signature in OVMF 
when
> >> > parsing
> >> >> > > the ACPI blobs for table headers, and filter it out.
> >> >> >
> >> >> > This is the code for what I would call 'pointer relocation'. The
> >> >> TCPA table is
> >> >> > not the only place where this is used, but why is it an issue
> >> >> there while not
> >> >> > with the following?
> >> >> >
> >> >> >     fadt->firmware_ctrl = cpu_to_le32(facs);
> >> >> >     /* FACS address to be filled by Guest linker */
> >> >> >     bios_linker_loader_add_pointer(linker, 
ACPI_BUILD_TABLE_FILE, 
> >> >> >                                    ACPI_BUILD_TABLE_FILE,
> >> >> >                                    table_data, 
&fadt->firmware_ctrl,
> >> >> >                                    sizeof fadt->firmware_ctrl);
> >> >> >
> >> >> > Regards,
> >> >> >     Stefan
> >> >>
> >> >>
> >> >> Becase FACS is an ACPI table. So BIOS allocates it
> >> >> from E820_RESERVED at the moment but it does not have to,
> >> >> it could mark it with E820_ACPI.
> >> >> Guest can then interpret the tables and then release the
> >> >> memory if it wishes.
> >> >>
> >> >> If you want to do it for TCPA you must tell bios that
> >> >> this is not ACPI memory.
> >> >
> >> > I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT would 
need
> >> > a similar tag to keep the S3 resume vector around?
> >>
> >> Not in OVMF, because edk2's EFI_ACPI_TABLE_PROTOCOL special cases 
FACS
> >> (containing the S3 resume vector), allocating it in EfiACPIMemoryNVS
> > memory.
> >>
> >> Table 26. Memory Type Usage after ExitBootServices()
> >> EfiACPIMemoryNVS: This memory is to be preserved by the loader and OS
> >>                   in the working and ACPI S1–S3 states.
> >>
> > 
> > So what is a solution then for OVMF? Add another special case for 
TCPA?
> > Is this counter to the specs ? Skip TCPA?
> 
> In the short term, probably skip TCPA, or advise users in documentation
> not to enable the TPM device when running OVMF.

I guess we can do that. You can skip the TCPA for now; once the UEFI has 
TPM support, this table would then  be needed.

    Stefan


[-- Attachment #2: Type: text/html, Size: 16429 bytes --]

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

[Michael S. Tsirkin]

On Wed, Jul 30, 2014 at 12:03:46PM -0400, Stefan Berger wrote:
> Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 11:58:52 AM:
> 
> > From: Laszlo Ersek <lersek@redhat.com>
> > To: Stefan Berger/Watson/IBM@IBMUS
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>, qemu-devel@nongnu.org,
> > Stefan Berger <stefanb@linux.vnet.ibm.com>
> > Date: 07/30/2014 11:59 AM
> > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >
> > On 07/30/14 17:44, Stefan Berger wrote:
> > > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 11:41:10 AM:
> > >
> > >> From: Laszlo Ersek <lersek@redhat.com>
> > >> To: Stefan Berger/Watson/IBM@IBMUS, "Michael S. Tsirkin" <mst@redhat.com>
> > >> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
> > >> Date: 07/30/2014 11:41 AM
> > >> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> > >>
> > >> On 07/30/14 17:29, Stefan Berger wrote:
> > >> > "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:20:41 AM:
> > >> >
> > >> >> From: "Michael S. Tsirkin" <mst@redhat.com>
> > >> >> To: Stefan Berger/Watson/IBM@IBMUS
> > >> >> Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan
> > >> >> Berger <stefanb@linux.vnet.ibm.com>
> > >> >> Date: 07/30/2014 11:20 AM
> > >> >> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> > >> >>
> > >> >> On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
> > >> >> > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
> > >> >> >
> > >> >> > > From: Laszlo Ersek <lersek@redhat.com>
> > >> >> > > To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan
> > >> > Berger/Watson/IBM@IBMUS
> > >> >> > > Cc: qemu-devel@nongnu.org, Stefan Berger
> > > <stefanb@linux.vnet.ibm.com>
> > >> >> > > Date: 07/30/2014 10:36 AM
> > >> >> > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> > >> >> > >
> > >> >> > > On 07/30/14 15:20, Michael S. Tsirkin wrote:
> > >> >> > > > On Tue, Jul 29, 2014 at 06:52:19AM -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
> > >> >> > >
> > >> >> > > (Thanks for CC'ing me, Michael.)
> > >> >> > >
> > >> >> > > I skimmed this spec.
> > >> >> > >
> > >> >> > > >> +static void
> > >> >> > > >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> > >> >> > > >> +{
> > >> >> > > >> +    Acpi20Tcpa *tcpa;
> > >> >> > > >> +    uint32_t log_area_minimum_length =
> > > TPM_LOG_AREA_MINIMUM_SIZE;
> > >> >> > > >> +    uint64_t log_area_start_address;
> > >> >> > > >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> > >> >> > > >> +
> > >> >> > > >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> > >> >> > > >> +
> > >> >> > > >> +    tcpa = acpi_data_push(table_data, len);
> > >> >> > > >> +
> > >> >> > > >> +    tcpa->platform_class =
> > >> > cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> > >> >> > > >> +    tcpa->log_area_minimum_length = cpu_to_le32
> > >> >> (log_area_minimum_length);
> > >> >> > > >> +    tcpa->log_area_start_address = cpu_to_le64
> > >> >> (log_area_start_address);
> > >> >> > > >> +
> > >> >> > > >> +    /* LASA address to be filled by Guest linker */
> > >> >> > > >
> > >> >> > > > Hmm, you are simply allocating log area as part of the ACPI
> > >> > table.  It
> > >> >> > > > works because bios happens to allocate tables from high memory.
> > >> >> > > > But I think this is a problem in practice because
> > >> >> > > > bios is allowed to allocate acpi memory differently.
> > >> >> > > > On the other hand log presumably needs to reside in
> > >> >> > > > physical memory somewhere.
> > >> >> > > >
> > >> >> > > > If you need bios to allocate this memory, then we will
> > >> >> > > > need a new allocation type for this, add it to linker
> > >> >> > > > in bios and qemu.
> > >> >> > > >
> > >> >> > > > Alternatively, find some other way to get hold of
> > >> >> > > > physical memory.
> > >> >> > > > Is there a way to disable the log completely?
> > >> >> > > > As defined in your patch, I doubt there's anything there, ever ..
> > >> >> > > >
> > >> >> > > >
> > >> >> > > >
> > >> >> > > >> +    bios_linker_loader_add_pointer(linker,
> > > ACPI_BUILD_TABLE_FILE,
> > >> >> > > >> +                                   ACPI_BUILD_TABLE_FILE,
> > >> >> > > >> +                                   table_data,
> > >> >> > > &tcpa->log_area_start_address,
> > >> >> > > >> +                                   sizeof
> > >> >> (tcpa->log_area_start_address));
> > >> >> > > >> +    build_header(linker, table_data,
> > >> >> > > >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> > >> >> > > >> +}
> > >> >> > >
> > >> >> > > So here's my understanding. The spec referenced above describes
> > > three
> > >> >> > > ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> > >> >> > > (usable by both client & server platforms) for TPM 2.0.
> > >> >> > >
> > >> >> > > The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> > >> >> > >
> > >> >> > > This table has a field called LASA (Log Area Start Address) which
> > >> > points
> > >> >> > > to somewhere in (guest-)physical memory. The patch adds a "dummy
> > >> > range"
> > >> >> > > to the end of the TCPA table itself, and asks the linker to set
> > >> > LASA to
> > >> >> > > the beginning of that range.
> > >> >> > >
> > >> >> > > This won't work in OVMF, and not just because of the reason that
> > >> > Michael
> > >> >> > > mentions (ie. because the firmware, in particular SeaBIOS, might
> > >> >> > > allocate the TCPA table in an area that is unsuitable as LASA
> > > target).
> > >> >> > >
> > >> >> > > Rather, in OVMF this won't work because OVMF doesn't implement the
> > >> >> > > linking part of the linker. The *generic* edk2 protocol
> > >> >> > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that
> > >> > OVMF uses
> > >> >> > > (as a client) to install ACPI tables in guest-phys memory requires
> > >> >> > > tables to be passed in one-by-one.
> > >> >> > >
> > >> >> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> > >> >> > > well-known tables specially. It has knowledge of their internal
> > >> >> > > pointers, and when you install an ACPI table,
> > > EFI_ACPI_TABLE_PROTOCOL
> > >> >> > > updates pointers automatically. (For example when you install the
> > >> > FACS,
> > >> >> > > the protocol links it automatically into FACP.)
> > >> >> > >
> > >> >> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to
> > >> > know
> > >> >> > > anything about the TCPA table, let alone the unstructured (?) TCG
> > >> > event
> > >> >> > > log that is pointed-to by TCPA.LASA.
> > >> >> > >
> > >> >> > > (I grepped for the TCPA signature,
> > >> >> > >
> > >> >> >
> > >> >>
> > >> >
> > >>
> > >
> >
> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> > >> >> > >
> > >> >> > > This means that if you pass down a TCPA table, OVMF will install it
> > >> >> > > right now, but TCPA.LASA will be bogus.
> > >> >> > >
> > >> >> > > If I wanted to implement the complete linker as Michael
> > > envisioned it,
> > >> >> > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and
> > > implement
> > >> >> > > ACPI table installation from zero, trying to mimic the SeaBIOS
> > > client
> > >> >> > > code, but in a way that matches the UEFI environment. I'm not
> > > ready to
> > >> >> > > do that. Definitely not without an "official" human-language
> > >> >> > > specification of the linker-loader interface.
> > >> >> > >
> > >> >> > > I skimmed the patch but I'm not sure what exactly the TPM
> > > emulation in
> > >> >> > > qemu depends on. Is it a command line option? Is it default for
> > > some
> > >> >> > > machine types?
> > >> >> > >
> > >> >> > > Alternatively, I could recognize the TCPA signature in OVMF when
> > >> > parsing
> > >> >> > > the ACPI blobs for table headers, and filter it out.
> > >> >> >
> > >> >> > This is the code for what I would call 'pointer relocation'. The
> > >> >> TCPA table is
> > >> >> > not the only place where this is used, but why is it an issue
> > >> >> there while not
> > >> >> > with the following?
> > >> >> >
> > >> >> >     fadt->firmware_ctrl = cpu_to_le32(facs);
> > >> >> >     /* FACS address to be filled by Guest linker */
> > >> >> >     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
> > >> >> >                                    ACPI_BUILD_TABLE_FILE,
> > >> >> >                                    table_data, &fadt->firmware_ctrl,
> > >> >> >                                    sizeof fadt->firmware_ctrl);
> > >> >> >
> > >> >> > Regards,
> > >> >> >     Stefan
> > >> >>
> > >> >>
> > >> >> Becase FACS is an ACPI table. So BIOS allocates it
> > >> >> from E820_RESERVED at the moment but it does not have to,
> > >> >> it could mark it with E820_ACPI.
> > >> >> Guest can then interpret the tables and then release the
> > >> >> memory if it wishes.
> > >> >>
> > >> >> If you want to do it for TCPA you must tell bios that
> > >> >> this is not ACPI memory.
> > >> >
> > >> > I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT would need
> > >> > a similar tag to keep the S3 resume vector around?
> > >>
> > >> Not in OVMF, because edk2's EFI_ACPI_TABLE_PROTOCOL special cases FACS
> > >> (containing the S3 resume vector), allocating it in EfiACPIMemoryNVS
> > > memory.
> > >>
> > >> Table 26. Memory Type Usage after ExitBootServices()
> > >> EfiACPIMemoryNVS: This memory is to be preserved by the loader and OS
> > >>                   in the working and ACPI S1–S3 states.
> > >>
> > >
> > > So what is a solution then for OVMF? Add another special case for TCPA?
> > > Is this counter to the specs ? Skip TCPA?
> >
> > In the short term, probably skip TCPA, or advise users in documentation
> > not to enable the TPM device when running OVMF.

Hmm but doesn't OVMF rely on all tables being packed without holes
in memory?
I remember it did, and if it still does, this breaks unless TCPA is
the last ...

> 
> I guess we can do that. You can skip the TCPA for now; once the UEFI has TPM
> support, this table would then  be needed.
> 
>     Stefan

If it doesn't work anyway, we can just tell people not to enable TPM
with OVMF. No need for hack in OVMF to skip it.


-- 
MST

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

[Laszlo Ersek]

On 07/30/14 18:10, Michael S. Tsirkin wrote:
> On Wed, Jul 30, 2014 at 12:03:46PM -0400, Stefan Berger wrote:
>> Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 11:58:52 AM:

>>> In the short term, probably skip TCPA, or advise users in documentation
>>> not to enable the TPM device when running OVMF.
> 
> Hmm but doesn't OVMF rely on all tables being packed without holes
> in memory?
> I remember it did, and if it still does, this breaks unless TCPA is
> the last ...

Ah, do you mean that the table size field in the ACPI table header of
TCPA would *not* include the TCG event log that is tacked-on? Yes, that
would certainly trip up the parser. It advances by looking at the table
size fields in the headers.

>> I guess we can do that. You can skip the TCPA for now; once the UEFI has TPM
>> support, this table would then  be needed.

> If it doesn't work anyway, we can just tell people not to enable TPM
> with OVMF. No need for hack in OVMF to skip it.

Agree 100%.

Thanks
Laszlo

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

[Stefan Berger]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 12:18:02 PM:

> From: Laszlo Ersek <lersek@redhat.com>
> To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan 
Berger/Watson/IBM@IBMUS
> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
> Date: 07/30/2014 12:18 PM
> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> 
> On 07/30/14 18:10, Michael S. Tsirkin wrote:
> > On Wed, Jul 30, 2014 at 12:03:46PM -0400, Stefan Berger wrote:
> >> Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 11:58:52 AM:
> 
> >>> In the short term, probably skip TCPA, or advise users in 
documentation
> >>> not to enable the TPM device when running OVMF.
> > 
> > Hmm but doesn't OVMF rely on all tables being packed without holes
> > in memory?
> > I remember it did, and if it still does, this breaks unless TCPA is
> > the last ...
> 
> Ah, do you mean that the table size field in the ACPI table header of
> TCPA would *not* include the TCG event log that is tacked-on? Yes, that
> would certainly trip up the parser. It advances by looking at the table
> size fields in the headers.

Is this what finally requires another command for the linker/loader to
allocate a chunk of memory where the size of the to-be-allocated memory is 
found
at the location of the pointer ? 

At least SeaBIOS does not seem to need it. Whether TCPA is the last table 
or not, doesn't seem to matter.

Stefan

[-- Attachment #2: Type: text/html, Size: 1987 bytes --]

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

[Laszlo Ersek]

On 07/30/14 18:35, Stefan Berger wrote:
> Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 12:18:02 PM:
> 
>> From: Laszlo Ersek <lersek@redhat.com>
>> To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan Berger/Watson/IBM@IBMUS
>> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Date: 07/30/2014 12:18 PM
>> Subject: Re: [PATCH v2] Add ACPI tables for TPM
>>
>> On 07/30/14 18:10, Michael S. Tsirkin wrote:
>> > On Wed, Jul 30, 2014 at 12:03:46PM -0400, Stefan Berger wrote:
>> >> Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 11:58:52 AM:
>>
>> >>> In the short term, probably skip TCPA, or advise users in
> documentation
>> >>> not to enable the TPM device when running OVMF.
>> >
>> > Hmm but doesn't OVMF rely on all tables being packed without holes
>> > in memory?
>> > I remember it did, and if it still does, this breaks unless TCPA is
>> > the last ...
>>
>> Ah, do you mean that the table size field in the ACPI table header of
>> TCPA would *not* include the TCG event log that is tacked-on? Yes, that
>> would certainly trip up the parser. It advances by looking at the table
>> size fields in the headers.
> 
> Is this what finally requires another command for the linker/loader to
> allocate a chunk of memory where the size of the to-be-allocated memory
> is found
> at the location of the pointer ?

No, the above is a limitation of the current OVMF parser.

Thanks
Laszlo

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

[Michael S. Tsirkin]

On Wed, Jul 30, 2014 at 11:29:36AM -0400, Stefan Berger wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:20:41 AM:
> 
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > To: Stefan Berger/Watson/IBM@IBMUS
> > Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan
> > Berger <stefanb@linux.vnet.ibm.com>
> > Date: 07/30/2014 11:20 AM
> > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >
> > On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
> > > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
> > >
> > > > From: Laszlo Ersek <lersek@redhat.com>
> > > > To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan Berger/Watson/IBM@IBMUS
> > > > Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > Date: 07/30/2014 10:36 AM
> > > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> > > >
> > > > On 07/30/14 15:20, Michael S. Tsirkin wrote:
> > > > > On Tue, Jul 29, 2014 at 06:52:19AM -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
> > > >
> > > > (Thanks for CC'ing me, Michael.)
> > > >
> > > > I skimmed this spec.
> > > >
> > > > >> +static void
> > > > >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> > > > >> +{
> > > > >> +    Acpi20Tcpa *tcpa;
> > > > >> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> > > > >> +    uint64_t log_area_start_address;
> > > > >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> > > > >> +
> > > > >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> > > > >> +
> > > > >> +    tcpa = acpi_data_push(table_data, len);
> > > > >> +
> > > > >> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> > > > >> +    tcpa->log_area_minimum_length = cpu_to_le32
> > (log_area_minimum_length);
> > > > >> +    tcpa->log_area_start_address = cpu_to_le64
> > (log_area_start_address);
> > > > >> +
> > > > >> +    /* LASA address to be filled by Guest linker */
> > > > >
> > > > > Hmm, you are simply allocating log area as part of the ACPI table.  It
> > > > > works because bios happens to allocate tables from high memory.
> > > > > But I think this is a problem in practice because
> > > > > bios is allowed to allocate acpi memory differently.
> > > > > On the other hand log presumably needs to reside in
> > > > > physical memory somewhere.
> > > > >
> > > > > If you need bios to allocate this memory, then we will
> > > > > need a new allocation type for this, add it to linker
> > > > > in bios and qemu.
> > > > >
> > > > > Alternatively, find some other way to get hold of
> > > > > physical memory.
> > > > > Is there a way to disable the log completely?
> > > > > As defined in your patch, I doubt there's anything there, ever ..
> > > > >
> > > > >
> > > > >
> > > > >> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> > > > >> +                                   ACPI_BUILD_TABLE_FILE,
> > > > >> +                                   table_data,
> > > > &tcpa->log_area_start_address,
> > > > >> +                                   sizeof
> > (tcpa->log_area_start_address));
> > > > >> +    build_header(linker, table_data,
> > > > >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> > > > >> +}
> > > >
> > > > So here's my understanding. The spec referenced above describes three
> > > > ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> > > > (usable by both client & server platforms) for TPM 2.0.
> > > >
> > > > The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> > > >
> > > > This table has a field called LASA (Log Area Start Address) which points
> > > > to somewhere in (guest-)physical memory. The patch adds a "dummy range"
> > > > to the end of the TCPA table itself, and asks the linker to set LASA to
> > > > the beginning of that range.
> > > >
> > > > This won't work in OVMF, and not just because of the reason that Michael
> > > > mentions (ie. because the firmware, in particular SeaBIOS, might
> > > > allocate the TCPA table in an area that is unsuitable as LASA target).
> > > >
> > > > Rather, in OVMF this won't work because OVMF doesn't implement the
> > > > linking part of the linker. The *generic* edk2 protocol
> > > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF uses
> > > > (as a client) to install ACPI tables in guest-phys memory requires
> > > > tables to be passed in one-by-one.
> > > >
> > > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> > > > well-known tables specially. It has knowledge of their internal
> > > > pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
> > > > updates pointers automatically. (For example when you install the FACS,
> > > > the protocol links it automatically into FACP.)
> > > >
> > > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to know
> > > > anything about the TCPA table, let alone the unstructured (?) TCG event
> > > > log that is pointed-to by TCPA.LASA.
> > > >
> > > > (I grepped for the TCPA signature,
> > > >
> > >
> >
> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> > > >
> > > > This means that if you pass down a TCPA table, OVMF will install it
> > > > right now, but TCPA.LASA will be bogus.
> > > >
> > > > If I wanted to implement the complete linker as Michael envisioned it,
> > > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
> > > > ACPI table installation from zero, trying to mimic the SeaBIOS client
> > > > code, but in a way that matches the UEFI environment. I'm not ready to
> > > > do that. Definitely not without an "official" human-language
> > > > specification of the linker-loader interface.
> > > >
> > > > I skimmed the patch but I'm not sure what exactly the TPM emulation in
> > > > qemu depends on. Is it a command line option? Is it default for some
> > > > machine types?
> > > >
> > > > Alternatively, I could recognize the TCPA signature in OVMF when parsing
> > > > the ACPI blobs for table headers, and filter it out.
> > >
> > > This is the code for what I would call 'pointer relocation'. The
> > TCPA table is
> > > not the only place where this is used, but why is it an issue
> > there while not
> > > with the following?
> > >
> > >     fadt->firmware_ctrl = cpu_to_le32(facs);
> > >     /* FACS address to be filled by Guest linker */
> > >     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
> > >                                    ACPI_BUILD_TABLE_FILE,
> > >                                    table_data, &fadt->firmware_ctrl,
> > >                                    sizeof fadt->firmware_ctrl);
> > >
> > > Regards,
> > >     Stefan
> >
> >
> > Becase FACS is an ACPI table. So BIOS allocates it
> > from E820_RESERVED at the moment but it does not have to,
> > it could mark it with E820_ACPI.
> > Guest can then interpret the tables and then release the
> > memory if it wishes.
> >
> > If you want to do it for TCPA you must tell bios that
> > this is not ACPI memory.
> 
> I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT would need a
> similar tag to keep the S3 resume vector around?
> 
> Stefan

Interesting, good point.
Yes ACPI spec says
	The BIOS aligns the FACS on a 64-byte boundary anywhere within the
	system’s memory address space. The memory where the FACS structure
	resides must not be reported as system AddressRangeMemory in the system
	address map. For example, the E820 address map reporting interface would
	report the region as AddressRangeReserved. For more information about
	system address map reporting interfaces, see Section 15, “System Address
	Map Interfaces.”

I don't see where would the requirement for other tables come from.
Can you clarify please?

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

[Stefan Berger]

[-- Attachment #1: Type: text/plain, Size: 9849 bytes --]

"Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:50:36 AM:

> From: "Michael S. Tsirkin" <mst@redhat.com>
> To: Stefan Berger/Watson/IBM@IBMUS
> Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan 
> Berger <stefanb@linux.vnet.ibm.com>
> Date: 07/30/2014 11:50 AM
> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> 
> On Wed, Jul 30, 2014 at 11:29:36AM -0400, Stefan Berger wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:20:41 AM:
> > 
> > > From: "Michael S. Tsirkin" <mst@redhat.com>
> > > To: Stefan Berger/Watson/IBM@IBMUS
> > > Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan
> > > Berger <stefanb@linux.vnet.ibm.com>
> > > Date: 07/30/2014 11:20 AM
> > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> > >
> > > On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
> > > > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
> > > >
> > > > > From: Laszlo Ersek <lersek@redhat.com>
> > > > > To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan Berger/
> Watson/IBM@IBMUS
> > > > > Cc: qemu-devel@nongnu.org, Stefan Berger 
<stefanb@linux.vnet.ibm.com>
> > > > > Date: 07/30/2014 10:36 AM
> > > > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> > > > >
> > > > > On 07/30/14 15:20, Michael S. Tsirkin wrote:
> > > > > > On Tue, Jul 29, 2014 at 06:52:19AM -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
> > > > >
> > > > > (Thanks for CC'ing me, Michael.)
> > > > >
> > > > > I skimmed this spec.
> > > > >
> > > > > >> +static void
> > > > > >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> > > > > >> +{
> > > > > >> +    Acpi20Tcpa *tcpa;
> > > > > >> +    uint32_t log_area_minimum_length = 
TPM_LOG_AREA_MINIMUM_SIZE;
> > > > > >> +    uint64_t log_area_start_address;
> > > > > >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> > > > > >> +
> > > > > >> +    log_area_start_address = table_data->len + 
sizeof(*tcpa);
> > > > > >> +
> > > > > >> +    tcpa = acpi_data_push(table_data, len);
> > > > > >> +
> > > > > >> +    tcpa->platform_class = cpu_to_le16
> (TPM_TCPA_ACPI_CLASS_CLIENT);
> > > > > >> +    tcpa->log_area_minimum_length = cpu_to_le32
> > > (log_area_minimum_length);
> > > > > >> +    tcpa->log_area_start_address = cpu_to_le64
> > > (log_area_start_address);
> > > > > >> +
> > > > > >> +    /* LASA address to be filled by Guest linker */
> > > > > >
> > > > > > Hmm, you are simply allocating log area as part of the 
> ACPI table.  It
> > > > > > works because bios happens to allocate tables from high 
memory.
> > > > > > But I think this is a problem in practice because
> > > > > > bios is allowed to allocate acpi memory differently.
> > > > > > On the other hand log presumably needs to reside in
> > > > > > physical memory somewhere.
> > > > > >
> > > > > > If you need bios to allocate this memory, then we will
> > > > > > need a new allocation type for this, add it to linker
> > > > > > in bios and qemu.
> > > > > >
> > > > > > Alternatively, find some other way to get hold of
> > > > > > physical memory.
> > > > > > Is there a way to disable the log completely?
> > > > > > As defined in your patch, I doubt there's anything there, ever 
..
> > > > > >
> > > > > >
> > > > > >
> > > > > >> +    bios_linker_loader_add_pointer(linker, 
ACPI_BUILD_TABLE_FILE,
> > > > > >> +                                   ACPI_BUILD_TABLE_FILE,
> > > > > >> +                                   table_data,
> > > > > &tcpa->log_area_start_address,
> > > > > >> +                                   sizeof
> > > (tcpa->log_area_start_address));
> > > > > >> +    build_header(linker, table_data,
> > > > > >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> > > > > >> +}
> > > > >
> > > > > So here's my understanding. The spec referenced above describes 
three
> > > > > ACPI tables: two (client vs. server) for TPM 1.2, and a third 
one
> > > > > (usable by both client & server platforms) for TPM 2.0.
> > > > >
> > > > > The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> > > > >
> > > > > This table has a field called LASA (Log Area Start Address) 
> which points
> > > > > to somewhere in (guest-)physical memory. The patch adds a 
> "dummy range"
> > > > > to the end of the TCPA table itself, and asks the linker to 
> set LASA to
> > > > > the beginning of that range.
> > > > >
> > > > > This won't work in OVMF, and not just because of the reason 
> that Michael
> > > > > mentions (ie. because the firmware, in particular SeaBIOS, might
> > > > > allocate the TCPA table in an area that is unsuitable as LASA 
target).
> > > > >
> > > > > Rather, in OVMF this won't work because OVMF doesn't implement 
the
> > > > > linking part of the linker. The *generic* edk2 protocol
> > > > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) 
> that OVMF uses
> > > > > (as a client) to install ACPI tables in guest-phys memory 
requires
> > > > > tables to be passed in one-by-one.
> > > > >
> > > > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles 
*some*
> > > > > well-known tables specially. It has knowledge of their internal
> > > > > pointers, and when you install an ACPI table, 
EFI_ACPI_TABLE_PROTOCOL
> > > > > updates pointers automatically. (For example when you 
> install the FACS,
> > > > > the protocol links it automatically into FACP.)
> > > > >
> > > > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't 
> seem to know
> > > > > anything about the TCPA table, let alone the unstructured 
> (?) TCG event
> > > > > log that is pointed-to by TCPA.LASA.
> > > > >
> > > > > (I grepped for the TCPA signature,
> > > > >
> > > >
> > >
> > 
> 
EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> > > > >
> > > > > This means that if you pass down a TCPA table, OVMF will install 
it
> > > > > right now, but TCPA.LASA will be bogus.
> > > > >
> > > > > If I wanted to implement the complete linker as Michael 
envisioned it,
> > > > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and 
implement
> > > > > ACPI table installation from zero, trying to mimic the SeaBIOS 
client
> > > > > code, but in a way that matches the UEFI environment. I'm not 
ready to
> > > > > do that. Definitely not without an "official" human-language
> > > > > specification of the linker-loader interface.
> > > > >
> > > > > I skimmed the patch but I'm not sure what exactly the TPM 
emulation in
> > > > > qemu depends on. Is it a command line option? Is it default for 
some
> > > > > machine types?
> > > > >
> > > > > Alternatively, I could recognize the TCPA signature in OVMF 
> when parsing
> > > > > the ACPI blobs for table headers, and filter it out.
> > > >
> > > > This is the code for what I would call 'pointer relocation'. The
> > > TCPA table is
> > > > not the only place where this is used, but why is it an issue
> > > there while not
> > > > with the following?
> > > >
> > > >     fadt->firmware_ctrl = cpu_to_le32(facs);
> > > >     /* FACS address to be filled by Guest linker */
> > > >     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE, 
> > > >                                    ACPI_BUILD_TABLE_FILE,
> > > >                                    table_data, 
&fadt->firmware_ctrl,
> > > >                                    sizeof fadt->firmware_ctrl);
> > > >
> > > > Regards,
> > > >     Stefan
> > >
> > >
> > > Becase FACS is an ACPI table. So BIOS allocates it
> > > from E820_RESERVED at the moment but it does not have to,
> > > it could mark it with E820_ACPI.
> > > Guest can then interpret the tables and then release the
> > > memory if it wishes.
> > >
> > > If you want to do it for TCPA you must tell bios that
> > > this is not ACPI memory.
> > 
> > I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT would 
need a
> > similar tag to keep the S3 resume vector around?
> > 
> > Stefan
> 
> Interesting, good point.
> Yes ACPI spec says
>    The BIOS aligns the FACS on a 64-byte boundary anywhere within the
>    system’s memory address space. The memory where the FACS structure
>    resides must not be reported as system AddressRangeMemory in the 
system
>    address map. For example, the E820 address map reporting interface 
would
>    report the region as AddressRangeReserved. For more information about
>    system address map reporting interfaces, see Section 15, “System 
Address
>    Map Interfaces.”
> 
> I don't see where would the requirement for other tables come from.
> Can you clarify please?
> 

Looking at SeaBIOS:

resume.c::s3_resume() calls find_resume_vector()

fw/biostables::find_resume_vector() calls find_fadt()

fw/biostables::find_fadt() accesses the RSDP then the RSDT then traverses 
its table of pointers to other ACPI tables and checks all their signatures 
until the FACP_SIGNATURE is found.

fw/biostables::find_resume_vector() then accesses the FACS and takes the 
firmware_waking_vector from it.

Had any of the tables been deallocated, S3 resume wouldn't work anymore. 
Besides that the signature checking wouldn't be all that great if the 
memory was now used for something else. 
   Stefan


[-- Attachment #2: Type: text/html, Size: 14869 bytes --]

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

[Michael S. Tsirkin]

On Wed, Jul 30, 2014 at 11:59:43AM -0400, Stefan Berger wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:50:36 AM:
> 
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > To: Stefan Berger/Watson/IBM@IBMUS
> > Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan
> > Berger <stefanb@linux.vnet.ibm.com>
> > Date: 07/30/2014 11:50 AM
> > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >
> > On Wed, Jul 30, 2014 at 11:29:36AM -0400, Stefan Berger wrote:
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:20:41 AM:
> > >
> > > > From: "Michael S. Tsirkin" <mst@redhat.com>
> > > > To: Stefan Berger/Watson/IBM@IBMUS
> > > > Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan
> > > > Berger <stefanb@linux.vnet.ibm.com>
> > > > Date: 07/30/2014 11:20 AM
> > > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> > > >
> > > > On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
> > > > > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
> > > > >
> > > > > > From: Laszlo Ersek <lersek@redhat.com>
> > > > > > To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan Berger/
> > Watson/IBM@IBMUS
> > > > > > Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > > > Date: 07/30/2014 10:36 AM
> > > > > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> > > > > >
> > > > > > On 07/30/14 15:20, Michael S. Tsirkin wrote:
> > > > > > > On Tue, Jul 29, 2014 at 06:52:19AM -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
> > > > > >
> > > > > > (Thanks for CC'ing me, Michael.)
> > > > > >
> > > > > > I skimmed this spec.
> > > > > >
> > > > > > >> +static void
> > > > > > >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> > > > > > >> +{
> > > > > > >> +    Acpi20Tcpa *tcpa;
> > > > > > >> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> > > > > > >> +    uint64_t log_area_start_address;
> > > > > > >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> > > > > > >> +
> > > > > > >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> > > > > > >> +
> > > > > > >> +    tcpa = acpi_data_push(table_data, len);
> > > > > > >> +
> > > > > > >> +    tcpa->platform_class = cpu_to_le16
> > (TPM_TCPA_ACPI_CLASS_CLIENT);
> > > > > > >> +    tcpa->log_area_minimum_length = cpu_to_le32
> > > > (log_area_minimum_length);
> > > > > > >> +    tcpa->log_area_start_address = cpu_to_le64
> > > > (log_area_start_address);
> > > > > > >> +
> > > > > > >> +    /* LASA address to be filled by Guest linker */
> > > > > > >
> > > > > > > Hmm, you are simply allocating log area as part of the
> > ACPI table.  It
> > > > > > > works because bios happens to allocate tables from high memory.
> > > > > > > But I think this is a problem in practice because
> > > > > > > bios is allowed to allocate acpi memory differently.
> > > > > > > On the other hand log presumably needs to reside in
> > > > > > > physical memory somewhere.
> > > > > > >
> > > > > > > If you need bios to allocate this memory, then we will
> > > > > > > need a new allocation type for this, add it to linker
> > > > > > > in bios and qemu.
> > > > > > >
> > > > > > > Alternatively, find some other way to get hold of
> > > > > > > physical memory.
> > > > > > > Is there a way to disable the log completely?
> > > > > > > As defined in your patch, I doubt there's anything there, ever ..
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> > > > > > >> +                                   ACPI_BUILD_TABLE_FILE,
> > > > > > >> +                                   table_data,
> > > > > > &tcpa->log_area_start_address,
> > > > > > >> +                                   sizeof
> > > > (tcpa->log_area_start_address));
> > > > > > >> +    build_header(linker, table_data,
> > > > > > >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> > > > > > >> +}
> > > > > >
> > > > > > So here's my understanding. The spec referenced above describes three
> > > > > > ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> > > > > > (usable by both client & server platforms) for TPM 2.0.
> > > > > >
> > > > > > The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> > > > > >
> > > > > > This table has a field called LASA (Log Area Start Address)
> > which points
> > > > > > to somewhere in (guest-)physical memory. The patch adds a
> > "dummy range"
> > > > > > to the end of the TCPA table itself, and asks the linker to
> > set LASA to
> > > > > > the beginning of that range.
> > > > > >
> > > > > > This won't work in OVMF, and not just because of the reason
> > that Michael
> > > > > > mentions (ie. because the firmware, in particular SeaBIOS, might
> > > > > > allocate the TCPA table in an area that is unsuitable as LASA
> target).
> > > > > >
> > > > > > Rather, in OVMF this won't work because OVMF doesn't implement the
> > > > > > linking part of the linker. The *generic* edk2 protocol
> > > > > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF)
> > that OVMF uses
> > > > > > (as a client) to install ACPI tables in guest-phys memory requires
> > > > > > tables to be passed in one-by-one.
> > > > > >
> > > > > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> > > > > > well-known tables specially. It has knowledge of their internal
> > > > > > pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
> > > > > > updates pointers automatically. (For example when you
> > install the FACS,
> > > > > > the protocol links it automatically into FACP.)
> > > > > >
> > > > > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't
> > seem to know
> > > > > > anything about the TCPA table, let alone the unstructured
> > (?) TCG event
> > > > > > log that is pointed-to by TCPA.LASA.
> > > > > >
> > > > > > (I grepped for the TCPA signature,
> > > > > >
> > > > >
> > > >
> > >
> >
> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> > > > > >
> > > > > > This means that if you pass down a TCPA table, OVMF will install it
> > > > > > right now, but TCPA.LASA will be bogus.
> > > > > >
> > > > > > If I wanted to implement the complete linker as Michael envisioned
> it,
> > > > > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
> > > > > > ACPI table installation from zero, trying to mimic the SeaBIOS client
> > > > > > code, but in a way that matches the UEFI environment. I'm not ready
> to
> > > > > > do that. Definitely not without an "official" human-language
> > > > > > specification of the linker-loader interface.
> > > > > >
> > > > > > I skimmed the patch but I'm not sure what exactly the TPM emulation
> in
> > > > > > qemu depends on. Is it a command line option? Is it default for some
> > > > > > machine types?
> > > > > >
> > > > > > Alternatively, I could recognize the TCPA signature in OVMF
> > when parsing
> > > > > > the ACPI blobs for table headers, and filter it out.
> > > > >
> > > > > This is the code for what I would call 'pointer relocation'. The
> > > > TCPA table is
> > > > > not the only place where this is used, but why is it an issue
> > > > there while not
> > > > > with the following?
> > > > >
> > > > >     fadt->firmware_ctrl = cpu_to_le32(facs);
> > > > >     /* FACS address to be filled by Guest linker */
> > > > >     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
> > > > >                                    ACPI_BUILD_TABLE_FILE,
> > > > >                                    table_data, &fadt->firmware_ctrl,
> > > > >                                    sizeof fadt->firmware_ctrl);
> > > > >
> > > > > Regards,
> > > > >     Stefan
> > > >
> > > >
> > > > Becase FACS is an ACPI table. So BIOS allocates it
> > > > from E820_RESERVED at the moment but it does not have to,
> > > > it could mark it with E820_ACPI.
> > > > Guest can then interpret the tables and then release the
> > > > memory if it wishes.
> > > >
> > > > If you want to do it for TCPA you must tell bios that
> > > > this is not ACPI memory.
> > >
> > > I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT would need a
> > > similar tag to keep the S3 resume vector around?
> > >
> > > Stefan
> >
> > Interesting, good point.
> > Yes ACPI spec says
> >    The BIOS aligns the FACS on a 64-byte boundary anywhere within the
> >    system’s memory address space. The memory where the FACS structure
> >    resides must not be reported as system AddressRangeMemory in the system
> >    address map. For example, the E820 address map reporting interface would
> >    report the region as AddressRangeReserved. For more information about
> >    system address map reporting interfaces, see Section 15, “System Address
> >    Map Interfaces.”
> >
> > I don't see where would the requirement for other tables come from.
> > Can you clarify please?
> >
> 
> Looking at SeaBIOS:
> 
> resume.c::s3_resume() calls find_resume_vector()
> 
> fw/biostables::find_resume_vector() calls find_fadt()
> 
> fw/biostables::find_fadt() accesses the RSDP then the RSDT then traverses its
> table of pointers to other ACPI tables and checks all their signatures until
> the FACP_SIGNATURE is found.
> 
> fw/biostables::find_resume_vector() then accesses the FACS and takes the
> firmware_waking_vector from it.
> 
> Had any of the tables been deallocated, S3 resume wouldn't work anymore.
> Besides that the signature checking wouldn't be all that great if the memory
> was now used for something else.
>    Stefan

Well this works because it reserves all tables.
If seabios wanted to stop doing this,
I guess it would have to stop looking for these things,
instead store FACS address somewhere else in reserved memory.

But nothing says it can't right?

-- 
MST

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

[Laszlo Ersek]

On 07/30/14 18:05, Michael S. Tsirkin wrote:
> On Wed, Jul 30, 2014 at 11:59:43AM -0400, Stefan Berger wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:50:36 AM:
>>
>>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>> To: Stefan Berger/Watson/IBM@IBMUS
>>> Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan
>>> Berger <stefanb@linux.vnet.ibm.com>
>>> Date: 07/30/2014 11:50 AM
>>> Subject: Re: [PATCH v2] Add ACPI tables for TPM
>>>
>>> On Wed, Jul 30, 2014 at 11:29:36AM -0400, Stefan Berger wrote:
>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:20:41 AM:
>>>>
>>>>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>>>> To: Stefan Berger/Watson/IBM@IBMUS
>>>>> Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan
>>>>> Berger <stefanb@linux.vnet.ibm.com>
>>>>> Date: 07/30/2014 11:20 AM
>>>>> Subject: Re: [PATCH v2] Add ACPI tables for TPM
>>>>>
>>>>> On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
>>>>>> Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
>>>>>>
>>>>>>> From: Laszlo Ersek <lersek@redhat.com>
>>>>>>> To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan Berger/
>>> Watson/IBM@IBMUS
>>>>>>> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>>>>> Date: 07/30/2014 10:36 AM
>>>>>>> Subject: Re: [PATCH v2] Add ACPI tables for TPM
>>>>>>>
>>>>>>> On 07/30/14 15:20, Michael S. Tsirkin wrote:
>>>>>>>> On Tue, Jul 29, 2014 at 06:52:19AM -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
>>>>>>>
>>>>>>> (Thanks for CC'ing me, Michael.)
>>>>>>>
>>>>>>> I skimmed this spec.
>>>>>>>
>>>>>>>>> +static void
>>>>>>>>> +build_tpm_tcpa(GArray *table_data, GArray *linker)
>>>>>>>>> +{
>>>>>>>>> +    Acpi20Tcpa *tcpa;
>>>>>>>>> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
>>>>>>>>> +    uint64_t log_area_start_address;
>>>>>>>>> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
>>>>>>>>> +
>>>>>>>>> +    log_area_start_address = table_data->len + sizeof(*tcpa);
>>>>>>>>> +
>>>>>>>>> +    tcpa = acpi_data_push(table_data, len);
>>>>>>>>> +
>>>>>>>>> +    tcpa->platform_class = cpu_to_le16
>>> (TPM_TCPA_ACPI_CLASS_CLIENT);
>>>>>>>>> +    tcpa->log_area_minimum_length = cpu_to_le32
>>>>> (log_area_minimum_length);
>>>>>>>>> +    tcpa->log_area_start_address = cpu_to_le64
>>>>> (log_area_start_address);
>>>>>>>>> +
>>>>>>>>> +    /* LASA address to be filled by Guest linker */
>>>>>>>>
>>>>>>>> Hmm, you are simply allocating log area as part of the
>>> ACPI table.  It
>>>>>>>> works because bios happens to allocate tables from high memory.
>>>>>>>> But I think this is a problem in practice because
>>>>>>>> bios is allowed to allocate acpi memory differently.
>>>>>>>> On the other hand log presumably needs to reside in
>>>>>>>> physical memory somewhere.
>>>>>>>>
>>>>>>>> If you need bios to allocate this memory, then we will
>>>>>>>> need a new allocation type for this, add it to linker
>>>>>>>> in bios and qemu.
>>>>>>>>
>>>>>>>> Alternatively, find some other way to get hold of
>>>>>>>> physical memory.
>>>>>>>> Is there a way to disable the log completely?
>>>>>>>> As defined in your patch, I doubt there's anything there, ever ..
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>>>>>>>> +                                   ACPI_BUILD_TABLE_FILE,
>>>>>>>>> +                                   table_data,
>>>>>>> &tcpa->log_area_start_address,
>>>>>>>>> +                                   sizeof
>>>>> (tcpa->log_area_start_address));
>>>>>>>>> +    build_header(linker, table_data,
>>>>>>>>> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
>>>>>>>>> +}
>>>>>>>
>>>>>>> So here's my understanding. The spec referenced above describes three
>>>>>>> ACPI tables: two (client vs. server) for TPM 1.2, and a third one
>>>>>>> (usable by both client & server platforms) for TPM 2.0.
>>>>>>>
>>>>>>> The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
>>>>>>>
>>>>>>> This table has a field called LASA (Log Area Start Address)
>>> which points
>>>>>>> to somewhere in (guest-)physical memory. The patch adds a
>>> "dummy range"
>>>>>>> to the end of the TCPA table itself, and asks the linker to
>>> set LASA to
>>>>>>> the beginning of that range.
>>>>>>>
>>>>>>> This won't work in OVMF, and not just because of the reason
>>> that Michael
>>>>>>> mentions (ie. because the firmware, in particular SeaBIOS, might
>>>>>>> allocate the TCPA table in an area that is unsuitable as LASA
>> target).
>>>>>>>
>>>>>>> Rather, in OVMF this won't work because OVMF doesn't implement the
>>>>>>> linking part of the linker. The *generic* edk2 protocol
>>>>>>> (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF)
>>> that OVMF uses
>>>>>>> (as a client) to install ACPI tables in guest-phys memory requires
>>>>>>> tables to be passed in one-by-one.
>>>>>>>
>>>>>>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
>>>>>>> well-known tables specially. It has knowledge of their internal
>>>>>>> pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
>>>>>>> updates pointers automatically. (For example when you
>>> install the FACS,
>>>>>>> the protocol links it automatically into FACP.)
>>>>>>>
>>>>>>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't
>>> seem to know
>>>>>>> anything about the TCPA table, let alone the unstructured
>>> (?) TCG event
>>>>>>> log that is pointed-to by TCPA.LASA.
>>>>>>>
>>>>>>> (I grepped for the TCPA signature,
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
>>>>>>>
>>>>>>> This means that if you pass down a TCPA table, OVMF will install it
>>>>>>> right now, but TCPA.LASA will be bogus.
>>>>>>>
>>>>>>> If I wanted to implement the complete linker as Michael envisioned
>> it,
>>>>>>> then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
>>>>>>> ACPI table installation from zero, trying to mimic the SeaBIOS client
>>>>>>> code, but in a way that matches the UEFI environment. I'm not ready
>> to
>>>>>>> do that. Definitely not without an "official" human-language
>>>>>>> specification of the linker-loader interface.
>>>>>>>
>>>>>>> I skimmed the patch but I'm not sure what exactly the TPM emulation
>> in
>>>>>>> qemu depends on. Is it a command line option? Is it default for some
>>>>>>> machine types?
>>>>>>>
>>>>>>> Alternatively, I could recognize the TCPA signature in OVMF
>>> when parsing
>>>>>>> the ACPI blobs for table headers, and filter it out.
>>>>>>
>>>>>> This is the code for what I would call 'pointer relocation'. The
>>>>> TCPA table is
>>>>>> not the only place where this is used, but why is it an issue
>>>>> there while not
>>>>>> with the following?
>>>>>>
>>>>>>     fadt->firmware_ctrl = cpu_to_le32(facs);
>>>>>>     /* FACS address to be filled by Guest linker */
>>>>>>     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
>>>>>>                                    ACPI_BUILD_TABLE_FILE,
>>>>>>                                    table_data, &fadt->firmware_ctrl,
>>>>>>                                    sizeof fadt->firmware_ctrl);
>>>>>>
>>>>>> Regards,
>>>>>>     Stefan
>>>>>
>>>>>
>>>>> Becase FACS is an ACPI table. So BIOS allocates it
>>>>> from E820_RESERVED at the moment but it does not have to,
>>>>> it could mark it with E820_ACPI.
>>>>> Guest can then interpret the tables and then release the
>>>>> memory if it wishes.
>>>>>
>>>>> If you want to do it for TCPA you must tell bios that
>>>>> this is not ACPI memory.
>>>>
>>>> I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT would need a
>>>> similar tag to keep the S3 resume vector around?
>>>>
>>>> Stefan
>>>
>>> Interesting, good point.
>>> Yes ACPI spec says
>>>    The BIOS aligns the FACS on a 64-byte boundary anywhere within the
>>>    system’s memory address space. The memory where the FACS structure
>>>    resides must not be reported as system AddressRangeMemory in the system
>>>    address map. For example, the E820 address map reporting interface would
>>>    report the region as AddressRangeReserved. For more information about
>>>    system address map reporting interfaces, see Section 15, “System Address
>>>    Map Interfaces.”
>>>
>>> I don't see where would the requirement for other tables come from.
>>> Can you clarify please?
>>>
>>
>> Looking at SeaBIOS:
>>
>> resume.c::s3_resume() calls find_resume_vector()
>>
>> fw/biostables::find_resume_vector() calls find_fadt()
>>
>> fw/biostables::find_fadt() accesses the RSDP then the RSDT then traverses its
>> table of pointers to other ACPI tables and checks all their signatures until
>> the FACP_SIGNATURE is found.
>>
>> fw/biostables::find_resume_vector() then accesses the FACS and takes the
>> firmware_waking_vector from it.
>>
>> Had any of the tables been deallocated, S3 resume wouldn't work anymore.
>> Besides that the signature checking wouldn't be all that great if the memory
>> was now used for something else.
>>    Stefan
> 
> Well this works because it reserves all tables.
> If seabios wanted to stop doing this,
> I guess it would have to stop looking for these things,
> instead store FACS address somewhere else in reserved memory.
> 
> But nothing says it can't right?

As far as I recall without looking, this is exactly what edk2 does. The
OS is allowed to reclaim memory (EfiACPIReclaimMemory) that backs the
"other" tables at some point.

In this regard the OS only needs to preserve EfiACPIMemoryNVS. Whatever
the firmware needs for S3 resume, it allocates as EfiACPIMemoryNVS (or
EfiReservedMemoryType, yes), including the FACS, and some extra stuff
that leads to it (LockBox in particular).

As far as I understand, if OVMF had a full linker/loader client right
now, it couldn't distinguish what to allocate as EfiACPIReclaimMemory,
and what else to allocate as EfiACPIMemoryNVS. So I would just opt for
safety and alloc everything as EfiACPIMemoryNVS. (I gather from your
above words that this is what SeaBIOS does too, in essence.)

Thanks
Laszlo

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

[Stefan Berger]

[-- Attachment #1: Type: text/plain, Size: 11709 bytes --]

"Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 12:05:56 PM:

> From: "Michael S. Tsirkin" <mst@redhat.com>
> To: Stefan Berger/Watson/IBM@IBMUS
> Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan 
> Berger <stefanb@linux.vnet.ibm.com>
> Date: 07/30/2014 12:05 PM
> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> 
> On Wed, Jul 30, 2014 at 11:59:43AM -0400, Stefan Berger wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:50:36 AM:
> > 
> > > From: "Michael S. Tsirkin" <mst@redhat.com>
> > > To: Stefan Berger/Watson/IBM@IBMUS
> > > Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, Stefan
> > > Berger <stefanb@linux.vnet.ibm.com>
> > > Date: 07/30/2014 11:50 AM
> > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> > >
> > > On Wed, Jul 30, 2014 at 11:29:36AM -0400, Stefan Berger wrote:
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:20:41 
AM:
> > > >
> > > > > From: "Michael S. Tsirkin" <mst@redhat.com>
> > > > > To: Stefan Berger/Watson/IBM@IBMUS
> > > > > Cc: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org, 
Stefan
> > > > > Berger <stefanb@linux.vnet.ibm.com>
> > > > > Date: 07/30/2014 11:20 AM
> > > > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> > > > >
> > > > > On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
> > > > > > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 
AM:
> > > > > >
> > > > > > > From: Laszlo Ersek <lersek@redhat.com>
> > > > > > > To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan Berger/
> > > Watson/IBM@IBMUS
> > > > > > > Cc: qemu-devel@nongnu.org, Stefan Berger 
> <stefanb@linux.vnet.ibm.com>
> > > > > > > Date: 07/30/2014 10:36 AM
> > > > > > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> > > > > > >
> > > > > > > On 07/30/14 15:20, Michael S. Tsirkin wrote:
> > > > > > > > On Tue, Jul 29, 2014 at 06:52:19AM -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
> > > > > > >
> > > > > > > (Thanks for CC'ing me, Michael.)
> > > > > > >
> > > > > > > I skimmed this spec.
> > > > > > >
> > > > > > > >> +static void
> > > > > > > >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> > > > > > > >> +{
> > > > > > > >> +    Acpi20Tcpa *tcpa;
> > > > > > > >> +    uint32_t log_area_minimum_length = 
> TPM_LOG_AREA_MINIMUM_SIZE;
> > > > > > > >> +    uint64_t log_area_start_address;
> > > > > > > >> +    size_t len = log_area_minimum_length + 
sizeof(*tcpa);
> > > > > > > >> +
> > > > > > > >> +    log_area_start_address = table_data->len + 
sizeof(*tcpa);
> > > > > > > >> +
> > > > > > > >> +    tcpa = acpi_data_push(table_data, len);
> > > > > > > >> +
> > > > > > > >> +    tcpa->platform_class = cpu_to_le16
> > > (TPM_TCPA_ACPI_CLASS_CLIENT);
> > > > > > > >> +    tcpa->log_area_minimum_length = cpu_to_le32
> > > > > (log_area_minimum_length);
> > > > > > > >> +    tcpa->log_area_start_address = cpu_to_le64
> > > > > (log_area_start_address);
> > > > > > > >> +
> > > > > > > >> +    /* LASA address to be filled by Guest linker */
> > > > > > > >
> > > > > > > > Hmm, you are simply allocating log area as part of the
> > > ACPI table.  It
> > > > > > > > works because bios happens to allocate tables from high 
memory.
> > > > > > > > But I think this is a problem in practice because
> > > > > > > > bios is allowed to allocate acpi memory differently.
> > > > > > > > On the other hand log presumably needs to reside in
> > > > > > > > physical memory somewhere.
> > > > > > > >
> > > > > > > > If you need bios to allocate this memory, then we will
> > > > > > > > need a new allocation type for this, add it to linker
> > > > > > > > in bios and qemu.
> > > > > > > >
> > > > > > > > Alternatively, find some other way to get hold of
> > > > > > > > physical memory.
> > > > > > > > Is there a way to disable the log completely?
> > > > > > > > As defined in your patch, I doubt there's anything 
> there, ever ..
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >> +    bios_linker_loader_add_pointer(linker, 
> ACPI_BUILD_TABLE_FILE,
> > > > > > > >> + ACPI_BUILD_TABLE_FILE,
> > > > > > > >> +                                   table_data,
> > > > > > > &tcpa->log_area_start_address,
> > > > > > > >> +                                   sizeof
> > > > > (tcpa->log_area_start_address));
> > > > > > > >> +    build_header(linker, table_data,
> > > > > > > >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 
2);
> > > > > > > >> +}
> > > > > > >
> > > > > > > So here's my understanding. The spec referenced above 
> describes three
> > > > > > > ACPI tables: two (client vs. server) for TPM 1.2, and a 
third one
> > > > > > > (usable by both client & server platforms) for TPM 2.0.
> > > > > > >
> > > > > > > The code above prepares a TPM 1.2 table. (Signature: 
"TCPA".)
> > > > > > >
> > > > > > > This table has a field called LASA (Log Area Start Address)
> > > which points
> > > > > > > to somewhere in (guest-)physical memory. The patch adds a
> > > "dummy range"
> > > > > > > to the end of the TCPA table itself, and asks the linker to
> > > set LASA to
> > > > > > > the beginning of that range.
> > > > > > >
> > > > > > > This won't work in OVMF, and not just because of the reason
> > > that Michael
> > > > > > > mentions (ie. because the firmware, in particular SeaBIOS, 
might
> > > > > > > allocate the TCPA table in an area that is unsuitable as 
LASA
> > target).
> > > > > > >
> > > > > > > Rather, in OVMF this won't work because OVMF doesn't 
implement the
> > > > > > > linking part of the linker. The *generic* edk2 protocol
> > > > > > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF)
> > > that OVMF uses
> > > > > > > (as a client) to install ACPI tables in guest-phys memory 
requires
> > > > > > > tables to be passed in one-by-one.
> > > > > > >
> > > > > > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles 
*some*
> > > > > > > well-known tables specially. It has knowledge of their 
internal
> > > > > > > pointers, and when you install an ACPI table, 
> EFI_ACPI_TABLE_PROTOCOL
> > > > > > > updates pointers automatically. (For example when you
> > > install the FACS,
> > > > > > > the protocol links it automatically into FACP.)
> > > > > > >
> > > > > > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't
> > > seem to know
> > > > > > > anything about the TCPA table, let alone the unstructured
> > > (?) TCG event
> > > > > > > log that is pointed-to by TCPA.LASA.
> > > > > > >
> > > > > > > (I grepped for the TCPA signature,
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > 
> 
EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> > > > > > >
> > > > > > > This means that if you pass down a TCPA table, OVMF 
willinstall it
> > > > > > > right now, but TCPA.LASA will be bogus.
> > > > > > >
> > > > > > > If I wanted to implement the complete linker as Michael 
envisioned
> > it,
> > > > > > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, 
> and implement
> > > > > > > ACPI table installation from zero, trying to mimic the 
> SeaBIOS client
> > > > > > > code, but in a way that matches the UEFI environment. 
> I'm not ready
> > to
> > > > > > > do that. Definitely not without an "official" human-language
> > > > > > > specification of the linker-loader interface.
> > > > > > >
> > > > > > > I skimmed the patch but I'm not sure what exactly the 
> TPM emulation
> > in
> > > > > > > qemu depends on. Is it a command line option? Is it 
> default for some
> > > > > > > machine types?
> > > > > > >
> > > > > > > Alternatively, I could recognize the TCPA signature in OVMF
> > > when parsing
> > > > > > > the ACPI blobs for table headers, and filter it out.
> > > > > >
> > > > > > This is the code for what I would call 'pointer relocation'. 
The
> > > > > TCPA table is
> > > > > > not the only place where this is used, but why is it an issue
> > > > > there while not
> > > > > > with the following?
> > > > > >
> > > > > >     fadt->firmware_ctrl = cpu_to_le32(facs);
> > > > > >     /* FACS address to be filled by Guest linker */
> > > > > >     bios_linker_loader_add_pointer(linker, 
ACPI_BUILD_TABLE_FILE, 
> > > > > >                                    ACPI_BUILD_TABLE_FILE,
> > > > > >                                    table_data, 
&fadt->firmware_ctrl,
> > > > > >                                    sizeof 
fadt->firmware_ctrl);
> > > > > >
> > > > > > Regards,
> > > > > >     Stefan
> > > > >
> > > > >
> > > > > Becase FACS is an ACPI table. So BIOS allocates it
> > > > > from E820_RESERVED at the moment but it does not have to,
> > > > > it could mark it with E820_ACPI.
> > > > > Guest can then interpret the tables and then release the
> > > > > memory if it wishes.
> > > > >
> > > > > If you want to do it for TCPA you must tell bios that
> > > > > this is not ACPI memory.
> > > >
> > > > I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT 
> would need a
> > > > similar tag to keep the S3 resume vector around?
> > > >
> > > > Stefan
> > >
> > > Interesting, good point.
> > > Yes ACPI spec says
> > >    The BIOS aligns the FACS on a 64-byte boundary anywhere within 
the
> > >    system’s memory address space. The memory where the FACS 
structure
> > >    resides must not be reported as system AddressRangeMemory in the 
system
> > >    address map. For example, the E820 address map reporting 
> interface would
> > >    report the region as AddressRangeReserved. For more information 
about
> > >    system address map reporting interfaces, see Section 15, 
> “System Address
> > >    Map Interfaces.”
> > >
> > > I don't see where would the requirement for other tables come from.
> > > Can you clarify please?
> > >
> > 
> > Looking at SeaBIOS:
> > 
> > resume.c::s3_resume() calls find_resume_vector()
> > 
> > fw/biostables::find_resume_vector() calls find_fadt()
> > 
> > fw/biostables::find_fadt() accesses the RSDP then the RSDT then 
> traverses its
> > table of pointers to other ACPI tables and checks all their signatures 
until
> > the FACP_SIGNATURE is found.
> > 
> > fw/biostables::find_resume_vector() then accesses the FACS and takes 
the
> > firmware_waking_vector from it.
> > 
> > Had any of the tables been deallocated, S3 resume wouldn't work 
anymore.
> > Besides that the signature checking wouldn't be all that great if the 
memory
> > was now used for something else.
> >    Stefan
> 
> Well this works because it reserves all tables.
> If seabios wanted to stop doing this,
> I guess it would have to stop looking for these things,
> instead store FACS address somewhere else in reserved memory.
> 
> But nothing says it can't right?

The slew of connections is a bit longer:

http://lxr.free-electrons.com/source/drivers/acpi/acpica/hwxfsleep.c#L94

I think the OSes install their waking vector in the original table that 
the BIOS then later on searches again.

   Stefan


[-- Attachment #2: Type: text/html, Size: 18512 bytes --]

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

[Laszlo Ersek]

On 07/30/14 17:10, Stefan Berger wrote:
> Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
> 
>> From: Laszlo Ersek <lersek@redhat.com>
>> To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan Berger/Watson/IBM@IBMUS
>> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Date: 07/30/2014 10:36 AM
>> Subject: Re: [PATCH v2] Add ACPI tables for TPM
>>
>> On 07/30/14 15:20, Michael S. Tsirkin wrote:
>> > On Tue, Jul 29, 2014 at 06:52:19AM -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
>>
>> (Thanks for CC'ing me, Michael.)
>>
>> I skimmed this spec.
>>
>> >> +static void
>> >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
>> >> +{
>> >> +    Acpi20Tcpa *tcpa;
>> >> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
>> >> +    uint64_t log_area_start_address;
>> >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
>> >> +
>> >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
>> >> +
>> >> +    tcpa = acpi_data_push(table_data, len);
>> >> +
>> >> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
>> >> +    tcpa->log_area_minimum_length =
> cpu_to_le32(log_area_minimum_length);
>> >> +    tcpa->log_area_start_address =
> cpu_to_le64(log_area_start_address);
>> >> +
>> >> +    /* LASA address to be filled by Guest linker */
>> >
>> > Hmm, you are simply allocating log area as part of the ACPI table.  It
>> > works because bios happens to allocate tables from high memory.
>> > But I think this is a problem in practice because
>> > bios is allowed to allocate acpi memory differently.
>> > On the other hand log presumably needs to reside in
>> > physical memory somewhere.
>> >
>> > If you need bios to allocate this memory, then we will
>> > need a new allocation type for this, add it to linker
>> > in bios and qemu.
>> >
>> > Alternatively, find some other way to get hold of
>> > physical memory.
>> > Is there a way to disable the log completely?
>> > As defined in your patch, I doubt there's anything there, ever ..
>> >
>> >
>> >
>> >> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> >> +                                   ACPI_BUILD_TABLE_FILE,
>> >> +                                   table_data,
>> &tcpa->log_area_start_address,
>> >> +                                  
> sizeof(tcpa->log_area_start_address));
>> >> +    build_header(linker, table_data,
>> >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
>> >> +}
>>
>> So here's my understanding. The spec referenced above describes three
>> ACPI tables: two (client vs. server) for TPM 1.2, and a third one
>> (usable by both client & server platforms) for TPM 2.0.
>>
>> The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
>>
>> This table has a field called LASA (Log Area Start Address) which points
>> to somewhere in (guest-)physical memory. The patch adds a "dummy range"
>> to the end of the TCPA table itself, and asks the linker to set LASA to
>> the beginning of that range.
>>
>> This won't work in OVMF, and not just because of the reason that Michael
>> mentions (ie. because the firmware, in particular SeaBIOS, might
>> allocate the TCPA table in an area that is unsuitable as LASA target).
>>
>> Rather, in OVMF this won't work because OVMF doesn't implement the
>> linking part of the linker. The *generic* edk2 protocol
>> (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF uses
>> (as a client) to install ACPI tables in guest-phys memory requires
>> tables to be passed in one-by-one.
>>
>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
>> well-known tables specially. It has knowledge of their internal
>> pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
>> updates pointers automatically. (For example when you install the FACS,
>> the protocol links it automatically into FACP.)
>>
>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to know
>> anything about the TCPA table, let alone the unstructured (?) TCG event
>> log that is pointed-to by TCPA.LASA.
>>
>> (I grepped for the TCPA signature,
>>
> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
>>
>> This means that if you pass down a TCPA table, OVMF will install it
>> right now, but TCPA.LASA will be bogus.
>>
>> If I wanted to implement the complete linker as Michael envisioned it,
>> then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
>> ACPI table installation from zero, trying to mimic the SeaBIOS client
>> code, but in a way that matches the UEFI environment. I'm not ready to
>> do that. Definitely not without an "official" human-language
>> specification of the linker-loader interface.
>>
>> I skimmed the patch but I'm not sure what exactly the TPM emulation in
>> qemu depends on. Is it a command line option? Is it default for some
>> machine types?
>>
>> Alternatively, I could recognize the TCPA signature in OVMF when parsing
>> the ACPI blobs for table headers, and filter it out.
> 
> This is the code for what I would call 'pointer relocation'. The TCPA
> table is not the only place where this is used, but why is it an issue
> there while not with the following?
> 
>     fadt->firmware_ctrl = cpu_to_le32(facs);
>     /* FACS address to be filled by Guest linker */
>     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
>                                    ACPI_BUILD_TABLE_FILE,
>                                    table_data, &fadt->firmware_ctrl,
>                                    sizeof fadt->firmware_ctrl);

(Responding wrt. to OVMF:) because EFI_ACPI_TABLE_PROTOCOL special cases
the installation of FACS. This is visible both in the UEFI
specification, and in the edk2 implementation.

That is, OVMF can avoid relocating the "fadt->firmware_ctrl" pointer,
because edk2's EFI_ACPI_TABLE_PROTOCOL will do that automatically, when
OVMF requests the installation of FACS.

No such special case exists for the installation of the TCG event log
area (which is by itself not even an ACPI table as I gather). *If* the
TCG event log were an ACPI table in its own right, *and* edk2's
EFI_ACPI_TABLE_PROTOCOL implementation had a special case for it, then
when OVMF installed the event log, EFI_ACPI_TABLE_PROTOCOL would update
TCPA.LASA to it.

Thanks
Laszlo

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

[Michael S. Tsirkin]

On Wed, Jul 30, 2014 at 05:37:26PM +0200, Laszlo Ersek wrote:
> On 07/30/14 17:10, Stefan Berger wrote:
> > Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
> > 
> >> From: Laszlo Ersek <lersek@redhat.com>
> >> To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan Berger/Watson/IBM@IBMUS
> >> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Date: 07/30/2014 10:36 AM
> >> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >>
> >> On 07/30/14 15:20, Michael S. Tsirkin wrote:
> >> > On Tue, Jul 29, 2014 at 06:52:19AM -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
> >>
> >> (Thanks for CC'ing me, Michael.)
> >>
> >> I skimmed this spec.
> >>
> >> >> +static void
> >> >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> >> >> +{
> >> >> +    Acpi20Tcpa *tcpa;
> >> >> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> >> >> +    uint64_t log_area_start_address;
> >> >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> >> >> +
> >> >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> >> >> +
> >> >> +    tcpa = acpi_data_push(table_data, len);
> >> >> +
> >> >> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> >> >> +    tcpa->log_area_minimum_length =
> > cpu_to_le32(log_area_minimum_length);
> >> >> +    tcpa->log_area_start_address =
> > cpu_to_le64(log_area_start_address);
> >> >> +
> >> >> +    /* LASA address to be filled by Guest linker */
> >> >
> >> > Hmm, you are simply allocating log area as part of the ACPI table.  It
> >> > works because bios happens to allocate tables from high memory.
> >> > But I think this is a problem in practice because
> >> > bios is allowed to allocate acpi memory differently.
> >> > On the other hand log presumably needs to reside in
> >> > physical memory somewhere.
> >> >
> >> > If you need bios to allocate this memory, then we will
> >> > need a new allocation type for this, add it to linker
> >> > in bios and qemu.
> >> >
> >> > Alternatively, find some other way to get hold of
> >> > physical memory.
> >> > Is there a way to disable the log completely?
> >> > As defined in your patch, I doubt there's anything there, ever ..
> >> >
> >> >
> >> >
> >> >> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> >> >> +                                   ACPI_BUILD_TABLE_FILE,
> >> >> +                                   table_data,
> >> &tcpa->log_area_start_address,
> >> >> +                                  
> > sizeof(tcpa->log_area_start_address));
> >> >> +    build_header(linker, table_data,
> >> >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> >> >> +}
> >>
> >> So here's my understanding. The spec referenced above describes three
> >> ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> >> (usable by both client & server platforms) for TPM 2.0.
> >>
> >> The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> >>
> >> This table has a field called LASA (Log Area Start Address) which points
> >> to somewhere in (guest-)physical memory. The patch adds a "dummy range"
> >> to the end of the TCPA table itself, and asks the linker to set LASA to
> >> the beginning of that range.
> >>
> >> This won't work in OVMF, and not just because of the reason that Michael
> >> mentions (ie. because the firmware, in particular SeaBIOS, might
> >> allocate the TCPA table in an area that is unsuitable as LASA target).
> >>
> >> Rather, in OVMF this won't work because OVMF doesn't implement the
> >> linking part of the linker. The *generic* edk2 protocol
> >> (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF uses
> >> (as a client) to install ACPI tables in guest-phys memory requires
> >> tables to be passed in one-by-one.
> >>
> >> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> >> well-known tables specially. It has knowledge of their internal
> >> pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
> >> updates pointers automatically. (For example when you install the FACS,
> >> the protocol links it automatically into FACP.)
> >>
> >> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to know
> >> anything about the TCPA table, let alone the unstructured (?) TCG event
> >> log that is pointed-to by TCPA.LASA.
> >>
> >> (I grepped for the TCPA signature,
> >>
> > EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> >>
> >> This means that if you pass down a TCPA table, OVMF will install it
> >> right now, but TCPA.LASA will be bogus.
> >>
> >> If I wanted to implement the complete linker as Michael envisioned it,
> >> then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
> >> ACPI table installation from zero, trying to mimic the SeaBIOS client
> >> code, but in a way that matches the UEFI environment. I'm not ready to
> >> do that. Definitely not without an "official" human-language
> >> specification of the linker-loader interface.
> >>
> >> I skimmed the patch but I'm not sure what exactly the TPM emulation in
> >> qemu depends on. Is it a command line option? Is it default for some
> >> machine types?
> >>
> >> Alternatively, I could recognize the TCPA signature in OVMF when parsing
> >> the ACPI blobs for table headers, and filter it out.
> > 
> > This is the code for what I would call 'pointer relocation'. The TCPA
> > table is not the only place where this is used, but why is it an issue
> > there while not with the following?
> > 
> >     fadt->firmware_ctrl = cpu_to_le32(facs);
> >     /* FACS address to be filled by Guest linker */
> >     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
> >                                    ACPI_BUILD_TABLE_FILE,
> >                                    table_data, &fadt->firmware_ctrl,
> >                                    sizeof fadt->firmware_ctrl);
> 
> (Responding wrt. to OVMF:) because EFI_ACPI_TABLE_PROTOCOL special cases
> the installation of FACS. This is visible both in the UEFI
> specification, and in the edk2 implementation.
> 
> That is, OVMF can avoid relocating the "fadt->firmware_ctrl" pointer,
> because edk2's EFI_ACPI_TABLE_PROTOCOL will do that automatically, when
> OVMF requests the installation of FACS.
> 
> No such special case exists for the installation of the TCG event log
> area (which is by itself not even an ACPI table as I gather). *If* the
> TCG event log were an ACPI table in its own right, *and* edk2's
> EFI_ACPI_TABLE_PROTOCOL implementation had a special case for it, then
> when OVMF installed the event log, EFI_ACPI_TABLE_PROTOCOL would update
> TCPA.LASA to it.
> 
> Thanks
> Laszlo

How does EFI want to handle TCPA? Does caller allocate it
log and fill in the address?


-- 
MST

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

[Laszlo Ersek]

On 07/30/14 17:52, Michael S. Tsirkin wrote:
> On Wed, Jul 30, 2014 at 05:37:26PM +0200, Laszlo Ersek wrote:
>> On 07/30/14 17:10, Stefan Berger wrote:
>>> Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
>>>
>>>> From: Laszlo Ersek <lersek@redhat.com>
>>>> To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan Berger/Watson/IBM@IBMUS
>>>> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>> Date: 07/30/2014 10:36 AM
>>>> Subject: Re: [PATCH v2] Add ACPI tables for TPM
>>>>
>>>> On 07/30/14 15:20, Michael S. Tsirkin wrote:
>>>>> On Tue, Jul 29, 2014 at 06:52:19AM -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
>>>>
>>>> (Thanks for CC'ing me, Michael.)
>>>>
>>>> I skimmed this spec.
>>>>
>>>>>> +static void
>>>>>> +build_tpm_tcpa(GArray *table_data, GArray *linker)
>>>>>> +{
>>>>>> +    Acpi20Tcpa *tcpa;
>>>>>> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
>>>>>> +    uint64_t log_area_start_address;
>>>>>> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
>>>>>> +
>>>>>> +    log_area_start_address = table_data->len + sizeof(*tcpa);
>>>>>> +
>>>>>> +    tcpa = acpi_data_push(table_data, len);
>>>>>> +
>>>>>> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
>>>>>> +    tcpa->log_area_minimum_length =
>>> cpu_to_le32(log_area_minimum_length);
>>>>>> +    tcpa->log_area_start_address =
>>> cpu_to_le64(log_area_start_address);
>>>>>> +
>>>>>> +    /* LASA address to be filled by Guest linker */
>>>>>
>>>>> Hmm, you are simply allocating log area as part of the ACPI table.  It
>>>>> works because bios happens to allocate tables from high memory.
>>>>> But I think this is a problem in practice because
>>>>> bios is allowed to allocate acpi memory differently.
>>>>> On the other hand log presumably needs to reside in
>>>>> physical memory somewhere.
>>>>>
>>>>> If you need bios to allocate this memory, then we will
>>>>> need a new allocation type for this, add it to linker
>>>>> in bios and qemu.
>>>>>
>>>>> Alternatively, find some other way to get hold of
>>>>> physical memory.
>>>>> Is there a way to disable the log completely?
>>>>> As defined in your patch, I doubt there's anything there, ever ..
>>>>>
>>>>>
>>>>>
>>>>>> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>>>>> +                                   ACPI_BUILD_TABLE_FILE,
>>>>>> +                                   table_data,
>>>> &tcpa->log_area_start_address,
>>>>>> +                                  
>>> sizeof(tcpa->log_area_start_address));
>>>>>> +    build_header(linker, table_data,
>>>>>> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
>>>>>> +}
>>>>
>>>> So here's my understanding. The spec referenced above describes three
>>>> ACPI tables: two (client vs. server) for TPM 1.2, and a third one
>>>> (usable by both client & server platforms) for TPM 2.0.
>>>>
>>>> The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
>>>>
>>>> This table has a field called LASA (Log Area Start Address) which points
>>>> to somewhere in (guest-)physical memory. The patch adds a "dummy range"
>>>> to the end of the TCPA table itself, and asks the linker to set LASA to
>>>> the beginning of that range.
>>>>
>>>> This won't work in OVMF, and not just because of the reason that Michael
>>>> mentions (ie. because the firmware, in particular SeaBIOS, might
>>>> allocate the TCPA table in an area that is unsuitable as LASA target).
>>>>
>>>> Rather, in OVMF this won't work because OVMF doesn't implement the
>>>> linking part of the linker. The *generic* edk2 protocol
>>>> (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF uses
>>>> (as a client) to install ACPI tables in guest-phys memory requires
>>>> tables to be passed in one-by-one.
>>>>
>>>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
>>>> well-known tables specially. It has knowledge of their internal
>>>> pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
>>>> updates pointers automatically. (For example when you install the FACS,
>>>> the protocol links it automatically into FACP.)
>>>>
>>>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to know
>>>> anything about the TCPA table, let alone the unstructured (?) TCG event
>>>> log that is pointed-to by TCPA.LASA.
>>>>
>>>> (I grepped for the TCPA signature,
>>>>
>>> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
>>>>
>>>> This means that if you pass down a TCPA table, OVMF will install it
>>>> right now, but TCPA.LASA will be bogus.
>>>>
>>>> If I wanted to implement the complete linker as Michael envisioned it,
>>>> then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
>>>> ACPI table installation from zero, trying to mimic the SeaBIOS client
>>>> code, but in a way that matches the UEFI environment. I'm not ready to
>>>> do that. Definitely not without an "official" human-language
>>>> specification of the linker-loader interface.
>>>>
>>>> I skimmed the patch but I'm not sure what exactly the TPM emulation in
>>>> qemu depends on. Is it a command line option? Is it default for some
>>>> machine types?
>>>>
>>>> Alternatively, I could recognize the TCPA signature in OVMF when parsing
>>>> the ACPI blobs for table headers, and filter it out.
>>>
>>> This is the code for what I would call 'pointer relocation'. The TCPA
>>> table is not the only place where this is used, but why is it an issue
>>> there while not with the following?
>>>
>>>     fadt->firmware_ctrl = cpu_to_le32(facs);
>>>     /* FACS address to be filled by Guest linker */
>>>     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
>>>                                    ACPI_BUILD_TABLE_FILE,
>>>                                    table_data, &fadt->firmware_ctrl,
>>>                                    sizeof fadt->firmware_ctrl);
>>
>> (Responding wrt. to OVMF:) because EFI_ACPI_TABLE_PROTOCOL special cases
>> the installation of FACS. This is visible both in the UEFI
>> specification, and in the edk2 implementation.
>>
>> That is, OVMF can avoid relocating the "fadt->firmware_ctrl" pointer,
>> because edk2's EFI_ACPI_TABLE_PROTOCOL will do that automatically, when
>> OVMF requests the installation of FACS.
>>
>> No such special case exists for the installation of the TCG event log
>> area (which is by itself not even an ACPI table as I gather). *If* the
>> TCG event log were an ACPI table in its own right, *and* edk2's
>> EFI_ACPI_TABLE_PROTOCOL implementation had a special case for it, then
>> when OVMF installed the event log, EFI_ACPI_TABLE_PROTOCOL would update
>> TCPA.LASA to it.
>>
>> Thanks
>> Laszlo
> 
> How does EFI want to handle TCPA? Does caller allocate it
> log and fill in the address?

TPM 1.2 seems to be completely absent from edk2, so I have no clue. TPM
2.0 appears to be supported, but I already grepped edk2 for how it
handles the related ACPI table(s), and it doesn't. As far as I can see.

In the long term, if you can find a way to make it work with SeaBIOS,
inside the linker/loader framework, that should work under OVMF as well.
I hope. Famous last words.

Laszlo

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

[Stefan Berger]

[-- Attachment #1: Type: text/plain, Size: 8317 bytes --]

Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 12:07:28 PM:

> From: Laszlo Ersek <lersek@redhat.com>
> To: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Stefan Berger/Watson/IBM@IBMUS, qemu-devel@nongnu.org, Stefan 
> Berger <stefanb@linux.vnet.ibm.com>
> Date: 07/30/2014 12:07 PM
> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> 
> On 07/30/14 17:52, Michael S. Tsirkin wrote:
> > On Wed, Jul 30, 2014 at 05:37:26PM +0200, Laszlo Ersek wrote:
> >> On 07/30/14 17:10, Stefan Berger wrote:
> >>> Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
> >>>
> >>>> From: Laszlo Ersek <lersek@redhat.com>
> >>>> To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan 
Berger/Watson/IBM@IBMUS
> >>>> Cc: qemu-devel@nongnu.org, Stefan Berger 
<stefanb@linux.vnet.ibm.com>
> >>>> Date: 07/30/2014 10:36 AM
> >>>> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >>>>
> >>>> On 07/30/14 15:20, Michael S. Tsirkin wrote:
> >>>>> On Tue, Jul 29, 2014 at 06:52:19AM -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
> >>>>
> >>>> (Thanks for CC'ing me, Michael.)
> >>>>
> >>>> I skimmed this spec.
> >>>>
> >>>>>> +static void
> >>>>>> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> >>>>>> +{
> >>>>>> +    Acpi20Tcpa *tcpa;
> >>>>>> +    uint32_t log_area_minimum_length = 
TPM_LOG_AREA_MINIMUM_SIZE;
> >>>>>> +    uint64_t log_area_start_address;
> >>>>>> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> >>>>>> +
> >>>>>> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> >>>>>> +
> >>>>>> +    tcpa = acpi_data_push(table_data, len);
> >>>>>> +
> >>>>>> +    tcpa->platform_class = 
cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> >>>>>> +    tcpa->log_area_minimum_length =
> >>> cpu_to_le32(log_area_minimum_length);
> >>>>>> +    tcpa->log_area_start_address =
> >>> cpu_to_le64(log_area_start_address);
> >>>>>> +
> >>>>>> +    /* LASA address to be filled by Guest linker */
> >>>>>
> >>>>> Hmm, you are simply allocating log area as part of the ACPI table. 
 It
> >>>>> works because bios happens to allocate tables from high memory.
> >>>>> But I think this is a problem in practice because
> >>>>> bios is allowed to allocate acpi memory differently.
> >>>>> On the other hand log presumably needs to reside in
> >>>>> physical memory somewhere.
> >>>>>
> >>>>> If you need bios to allocate this memory, then we will
> >>>>> need a new allocation type for this, add it to linker
> >>>>> in bios and qemu.
> >>>>>
> >>>>> Alternatively, find some other way to get hold of
> >>>>> physical memory.
> >>>>> Is there a way to disable the log completely?
> >>>>> As defined in your patch, I doubt there's anything there, ever ..
> >>>>>
> >>>>>
> >>>>>
> >>>>>> +    bios_linker_loader_add_pointer(linker, 
ACPI_BUILD_TABLE_FILE,
> >>>>>> +                                   ACPI_BUILD_TABLE_FILE,
> >>>>>> +                                   table_data,
> >>>> &tcpa->log_area_start_address,
> >>>>>> + 
> >>> sizeof(tcpa->log_area_start_address));
> >>>>>> +    build_header(linker, table_data,
> >>>>>> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> >>>>>> +}
> >>>>
> >>>> So here's my understanding. The spec referenced above describes 
three
> >>>> ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> >>>> (usable by both client & server platforms) for TPM 2.0.
> >>>>
> >>>> The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> >>>>
> >>>> This table has a field called LASA (Log Area Start Address) which 
points
> >>>> to somewhere in (guest-)physical memory. The patch adds a "dummy 
range"
> >>>> to the end of the TCPA table itself, and asks the linker to set 
LASA to
> >>>> the beginning of that range.
> >>>>
> >>>> This won't work in OVMF, and not just because of the reason that 
Michael
> >>>> mentions (ie. because the firmware, in particular SeaBIOS, might
> >>>> allocate the TCPA table in an area that is unsuitable as LASA 
target).
> >>>>
> >>>> Rather, in OVMF this won't work because OVMF doesn't implement the
> >>>> linking part of the linker. The *generic* edk2 protocol
> >>>> (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF 
uses
> >>>> (as a client) to install ACPI tables in guest-phys memory requires
> >>>> tables to be passed in one-by-one.
> >>>>
> >>>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> >>>> well-known tables specially. It has knowledge of their internal
> >>>> pointers, and when you install an ACPI table, 
EFI_ACPI_TABLE_PROTOCOL
> >>>> updates pointers automatically. (For example when you install the 
FACS,
> >>>> the protocol links it automatically into FACP.)
> >>>>
> >>>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to 
know
> >>>> anything about the TCPA table, let alone the unstructured (?) TCG 
event
> >>>> log that is pointed-to by TCPA.LASA.
> >>>>
> >>>> (I grepped for the TCPA signature,
> >>>>
> >>> 
> 
EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> >>>>
> >>>> This means that if you pass down a TCPA table, OVMF will install it
> >>>> right now, but TCPA.LASA will be bogus.
> >>>>
> >>>> If I wanted to implement the complete linker as Michael envisioned 
it,
> >>>> then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and 
implement
> >>>> ACPI table installation from zero, trying to mimic the SeaBIOS 
client
> >>>> code, but in a way that matches the UEFI environment. I'm not ready 
to
> >>>> do that. Definitely not without an "official" human-language
> >>>> specification of the linker-loader interface.
> >>>>
> >>>> I skimmed the patch but I'm not sure what exactly the TPM emulation 
in
> >>>> qemu depends on. Is it a command line option? Is it default for 
some
> >>>> machine types?
> >>>>
> >>>> Alternatively, I could recognize the TCPA signature in OVMF when 
parsing
> >>>> the ACPI blobs for table headers, and filter it out.
> >>>
> >>> This is the code for what I would call 'pointer relocation'. The 
TCPA
> >>> table is not the only place where this is used, but why is it an 
issue
> >>> there while not with the following?
> >>>
> >>>     fadt->firmware_ctrl = cpu_to_le32(facs);
> >>>     /* FACS address to be filled by Guest linker */
> >>>     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE, 
> >>>                                    ACPI_BUILD_TABLE_FILE,
> >>>                                    table_data, &fadt->firmware_ctrl,
> >>>                                    sizeof fadt->firmware_ctrl);
> >>
> >> (Responding wrt. to OVMF:) because EFI_ACPI_TABLE_PROTOCOL special 
cases
> >> the installation of FACS. This is visible both in the UEFI
> >> specification, and in the edk2 implementation.
> >>
> >> That is, OVMF can avoid relocating the "fadt->firmware_ctrl" pointer,
> >> because edk2's EFI_ACPI_TABLE_PROTOCOL will do that automatically, 
when
> >> OVMF requests the installation of FACS.
> >>
> >> No such special case exists for the installation of the TCG event log
> >> area (which is by itself not even an ACPI table as I gather). *If* 
the
> >> TCG event log were an ACPI table in its own right, *and* edk2's
> >> EFI_ACPI_TABLE_PROTOCOL implementation had a special case for it, 
then
> >> when OVMF installed the event log, EFI_ACPI_TABLE_PROTOCOL would 
update
> >> TCPA.LASA to it.
> >>
> >> Thanks
> >> Laszlo
> > 
> > How does EFI want to handle TCPA? Does caller allocate it
> > log and fill in the address?
> 
> TPM 1.2 seems to be completely absent from edk2, so I have no clue. TPM
> 2.0 appears to be supported, but I already grepped edk2 for how it
> handles the related ACPI table(s), and it doesn't. As far as I can see.
> 
> In the long term, if you can find a way to make it work with SeaBIOS,
> inside the linker/loader framework, that should work under OVMF as well.
> I hope. Famous last words.

The TCPA table works using SeaBIOS's pointer relocation facility.

   Stefan

[-- Attachment #2: Type: text/html, Size: 12780 bytes --]

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

[Michael S. Tsirkin]

On Wed, Jul 30, 2014 at 06:07:28PM +0200, Laszlo Ersek wrote:
> On 07/30/14 17:52, Michael S. Tsirkin wrote:
> > On Wed, Jul 30, 2014 at 05:37:26PM +0200, Laszlo Ersek wrote:
> >> On 07/30/14 17:10, Stefan Berger wrote:
> >>> Laszlo Ersek <lersek@redhat.com> wrote on 07/30/2014 10:36:38 AM:
> >>>
> >>>> From: Laszlo Ersek <lersek@redhat.com>
> >>>> To: "Michael S. Tsirkin" <mst@redhat.com>, Stefan Berger/Watson/IBM@IBMUS
> >>>> Cc: qemu-devel@nongnu.org, Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>>> Date: 07/30/2014 10:36 AM
> >>>> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >>>>
> >>>> On 07/30/14 15:20, Michael S. Tsirkin wrote:
> >>>>> On Tue, Jul 29, 2014 at 06:52:19AM -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
> >>>>
> >>>> (Thanks for CC'ing me, Michael.)
> >>>>
> >>>> I skimmed this spec.
> >>>>
> >>>>>> +static void
> >>>>>> +build_tpm_tcpa(GArray *table_data, GArray *linker)
> >>>>>> +{
> >>>>>> +    Acpi20Tcpa *tcpa;
> >>>>>> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> >>>>>> +    uint64_t log_area_start_address;
> >>>>>> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
> >>>>>> +
> >>>>>> +    log_area_start_address = table_data->len + sizeof(*tcpa);
> >>>>>> +
> >>>>>> +    tcpa = acpi_data_push(table_data, len);
> >>>>>> +
> >>>>>> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> >>>>>> +    tcpa->log_area_minimum_length =
> >>> cpu_to_le32(log_area_minimum_length);
> >>>>>> +    tcpa->log_area_start_address =
> >>> cpu_to_le64(log_area_start_address);
> >>>>>> +
> >>>>>> +    /* LASA address to be filled by Guest linker */
> >>>>>
> >>>>> Hmm, you are simply allocating log area as part of the ACPI table.  It
> >>>>> works because bios happens to allocate tables from high memory.
> >>>>> But I think this is a problem in practice because
> >>>>> bios is allowed to allocate acpi memory differently.
> >>>>> On the other hand log presumably needs to reside in
> >>>>> physical memory somewhere.
> >>>>>
> >>>>> If you need bios to allocate this memory, then we will
> >>>>> need a new allocation type for this, add it to linker
> >>>>> in bios and qemu.
> >>>>>
> >>>>> Alternatively, find some other way to get hold of
> >>>>> physical memory.
> >>>>> Is there a way to disable the log completely?
> >>>>> As defined in your patch, I doubt there's anything there, ever ..
> >>>>>
> >>>>>
> >>>>>
> >>>>>> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> >>>>>> +                                   ACPI_BUILD_TABLE_FILE,
> >>>>>> +                                   table_data,
> >>>> &tcpa->log_area_start_address,
> >>>>>> +                                  
> >>> sizeof(tcpa->log_area_start_address));
> >>>>>> +    build_header(linker, table_data,
> >>>>>> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
> >>>>>> +}
> >>>>
> >>>> So here's my understanding. The spec referenced above describes three
> >>>> ACPI tables: two (client vs. server) for TPM 1.2, and a third one
> >>>> (usable by both client & server platforms) for TPM 2.0.
> >>>>
> >>>> The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
> >>>>
> >>>> This table has a field called LASA (Log Area Start Address) which points
> >>>> to somewhere in (guest-)physical memory. The patch adds a "dummy range"
> >>>> to the end of the TCPA table itself, and asks the linker to set LASA to
> >>>> the beginning of that range.
> >>>>
> >>>> This won't work in OVMF, and not just because of the reason that Michael
> >>>> mentions (ie. because the firmware, in particular SeaBIOS, might
> >>>> allocate the TCPA table in an area that is unsuitable as LASA target).
> >>>>
> >>>> Rather, in OVMF this won't work because OVMF doesn't implement the
> >>>> linking part of the linker. The *generic* edk2 protocol
> >>>> (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that OVMF uses
> >>>> (as a client) to install ACPI tables in guest-phys memory requires
> >>>> tables to be passed in one-by-one.
> >>>>
> >>>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
> >>>> well-known tables specially. It has knowledge of their internal
> >>>> pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
> >>>> updates pointers automatically. (For example when you install the FACS,
> >>>> the protocol links it automatically into FACP.)
> >>>>
> >>>> The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to know
> >>>> anything about the TCPA table, let alone the unstructured (?) TCG event
> >>>> log that is pointed-to by TCPA.LASA.
> >>>>
> >>>> (I grepped for the TCPA signature,
> >>>>
> >>> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
> >>>>
> >>>> This means that if you pass down a TCPA table, OVMF will install it
> >>>> right now, but TCPA.LASA will be bogus.
> >>>>
> >>>> If I wanted to implement the complete linker as Michael envisioned it,
> >>>> then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
> >>>> ACPI table installation from zero, trying to mimic the SeaBIOS client
> >>>> code, but in a way that matches the UEFI environment. I'm not ready to
> >>>> do that. Definitely not without an "official" human-language
> >>>> specification of the linker-loader interface.
> >>>>
> >>>> I skimmed the patch but I'm not sure what exactly the TPM emulation in
> >>>> qemu depends on. Is it a command line option? Is it default for some
> >>>> machine types?
> >>>>
> >>>> Alternatively, I could recognize the TCPA signature in OVMF when parsing
> >>>> the ACPI blobs for table headers, and filter it out.
> >>>
> >>> This is the code for what I would call 'pointer relocation'. The TCPA
> >>> table is not the only place where this is used, but why is it an issue
> >>> there while not with the following?
> >>>
> >>>     fadt->firmware_ctrl = cpu_to_le32(facs);
> >>>     /* FACS address to be filled by Guest linker */
> >>>     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
> >>>                                    ACPI_BUILD_TABLE_FILE,
> >>>                                    table_data, &fadt->firmware_ctrl,
> >>>                                    sizeof fadt->firmware_ctrl);
> >>
> >> (Responding wrt. to OVMF:) because EFI_ACPI_TABLE_PROTOCOL special cases
> >> the installation of FACS. This is visible both in the UEFI
> >> specification, and in the edk2 implementation.
> >>
> >> That is, OVMF can avoid relocating the "fadt->firmware_ctrl" pointer,
> >> because edk2's EFI_ACPI_TABLE_PROTOCOL will do that automatically, when
> >> OVMF requests the installation of FACS.
> >>
> >> No such special case exists for the installation of the TCG event log
> >> area (which is by itself not even an ACPI table as I gather). *If* the
> >> TCG event log were an ACPI table in its own right, *and* edk2's
> >> EFI_ACPI_TABLE_PROTOCOL implementation had a special case for it, then
> >> when OVMF installed the event log, EFI_ACPI_TABLE_PROTOCOL would update
> >> TCPA.LASA to it.
> >>
> >> Thanks
> >> Laszlo
> > 
> > How does EFI want to handle TCPA? Does caller allocate it
> > log and fill in the address?
> 
> TPM 1.2 seems to be completely absent from edk2, so I have no clue. TPM
> 2.0 appears to be supported, but I already grepped edk2 for how it
> handles the related ACPI table(s), and it doesn't. As far as I can see.

So it will basically just add it to RSDT and assume log pointer is
filled in correctly by caller?

> In the long term, if you can find a way to make it work with SeaBIOS,
> inside the linker/loader framework, that should work under OVMF as well.
> I hope. Famous last words.
> 
> Laszlo

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

[Laszlo Ersek]

On 07/30/14 18:11, Michael S. Tsirkin wrote:
> On Wed, Jul 30, 2014 at 06:07:28PM +0200, Laszlo Ersek wrote:
>> On 07/30/14 17:52, Michael S. Tsirkin wrote:

>>> How does EFI want to handle TCPA? Does caller allocate it
>>> log and fill in the address?
>>
>> TPM 1.2 seems to be completely absent from edk2, so I have no clue. TPM
>> 2.0 appears to be supported, but I already grepped edk2 for how it
>> handles the related ACPI table(s), and it doesn't. As far as I can see.
> 
> So it will basically just add it to RSDT and assume log pointer is
> filled in correctly by caller?

Yep.

Laszlo

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

[Stefan Berger]

On 07/30/2014 09:20 AM, Michael S. Tsirkin wrote:
> On Tue, Jul 29, 2014 at 06:52:19AM -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>
>> ---
>>   hw/i386/Makefile.objs |  3 ++-
>>   hw/i386/acpi-build.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>   hw/i386/acpi-defs.h   | 11 +++++++++++
>>   hw/i386/ssdt-tpm.dsl  | 43 +++++++++++++++++++++++++++++++++++++++++++
>>   hw/tpm/tpm_tis.h      |  5 +----
>>   include/hw/acpi/tpm.h | 29 +++++++++++++++++++++++++++++
>>   include/sysemu/tpm.h  |  5 +++++
>>   7 files changed, 137 insertions(+), 5 deletions(-)
>>   create mode 100644 hw/i386/ssdt-tpm.dsl
>>   create mode 100644 include/hw/acpi/tpm.h
>>
>> 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..d767e37 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -38,6 +38,8 @@
>>   #include "hw/loader.h"
>>   #include "hw/isa/isa.h"
>>   #include "hw/acpi/memory_hotplug.h"
>> +#include "sysemu/tpm.h"
>> +#include "hw/acpi/tpm.h"
>>   
>>   /* Supported chipsets: */
>>   #include "hw/acpi/piix4.h"
>> @@ -75,6 +77,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 +196,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 +685,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 +1172,40 @@ 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 log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
>> +    uint64_t log_area_start_address;
>> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
>> +
>> +    log_area_start_address = table_data->len + sizeof(*tcpa);
>> +
>> +    tcpa = acpi_data_push(table_data, len);
>> +
>> +    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
>> +    tcpa->log_area_minimum_length = cpu_to_le32(log_area_minimum_length);
>> +    tcpa->log_area_start_address = cpu_to_le64(log_area_start_address);
>> +
>> +    /* LASA address to be filled by Guest linker */
> Hmm, you are simply allocating log area as part of the ACPI table.  It
> works because bios happens to allocate tables from high memory.
> But I think this is a problem in practice because
> bios is allowed to allocate acpi memory differently.
> On the other hand log presumably needs to reside in
> physical memory somewhere.
>
> If you need bios to allocate this memory, then we will
> need a new allocation type for this, add it to linker
> in bios and qemu.

Why does the BIOS 'need' to allocate it? Why can it not just use the 
memory that QEMU allocates? Obviously I am using the 'pointer 
relocation' feature of the BIOS to bend the pointer in the TCPA table to 
this log area.

>
> Alternatively, find some other way to get hold of
> physical memory.
> Is there a way to disable the log completely?
> As defined in your patch, I doubt there's anything there, ever ..

There is currently no way to disable it. For a machine with a TPM, there 
should be support for an SSDT and this TCPA table for the BIOS to write 
logs into. So I allocate both and Linux for example can then show an 
empty table in /sys/kernel/security/tpm0/ascii_bios_measurements when 
the passthrough driver is used. I am working on a TPM driver for a CUSE 
TPM(CUSE = character device in user space) where we want the BIOS to 
behave exactly like the BIOS on real hardware and write its measurements 
into this log. I know at least that this then works the way it is 
implemented now.

    Stefan

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

[Michael S. Tsirkin]

On Wed, Jul 30, 2014 at 10:54:20AM -0400, Stefan Berger wrote:
> On 07/30/2014 09:20 AM, Michael S. Tsirkin wrote:
> >On Tue, Jul 29, 2014 at 06:52:19AM -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>
> >>---
> >>  hw/i386/Makefile.objs |  3 ++-
> >>  hw/i386/acpi-build.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  hw/i386/acpi-defs.h   | 11 +++++++++++
> >>  hw/i386/ssdt-tpm.dsl  | 43 +++++++++++++++++++++++++++++++++++++++++++
> >>  hw/tpm/tpm_tis.h      |  5 +----
> >>  include/hw/acpi/tpm.h | 29 +++++++++++++++++++++++++++++
> >>  include/sysemu/tpm.h  |  5 +++++
> >>  7 files changed, 137 insertions(+), 5 deletions(-)
> >>  create mode 100644 hw/i386/ssdt-tpm.dsl
> >>  create mode 100644 include/hw/acpi/tpm.h
> >>
> >>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..d767e37 100644
> >>--- a/hw/i386/acpi-build.c
> >>+++ b/hw/i386/acpi-build.c
> >>@@ -38,6 +38,8 @@
> >>  #include "hw/loader.h"
> >>  #include "hw/isa/isa.h"
> >>  #include "hw/acpi/memory_hotplug.h"
> >>+#include "sysemu/tpm.h"
> >>+#include "hw/acpi/tpm.h"
> >>  /* Supported chipsets: */
> >>  #include "hw/acpi/piix4.h"
> >>@@ -75,6 +77,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 +196,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 +685,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 +1172,40 @@ 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 log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> >>+    uint64_t log_area_start_address;
> >>+    size_t len = log_area_minimum_length + sizeof(*tcpa);
> >>+
> >>+    log_area_start_address = table_data->len + sizeof(*tcpa);
> >>+
> >>+    tcpa = acpi_data_push(table_data, len);
> >>+
> >>+    tcpa->platform_class = cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
> >>+    tcpa->log_area_minimum_length = cpu_to_le32(log_area_minimum_length);
> >>+    tcpa->log_area_start_address = cpu_to_le64(log_area_start_address);
> >>+
> >>+    /* LASA address to be filled by Guest linker */
> >Hmm, you are simply allocating log area as part of the ACPI table.  It
> >works because bios happens to allocate tables from high memory.
> >But I think this is a problem in practice because
> >bios is allowed to allocate acpi memory differently.
> >On the other hand log presumably needs to reside in
> >physical memory somewhere.
> >
> >If you need bios to allocate this memory, then we will
> >need a new allocation type for this, add it to linker
> >in bios and qemu.
> 
> Why does the BIOS 'need' to allocate it? Why can it not just use the memory
> that QEMU allocates? Obviously I am using the 'pointer relocation' feature
> of the BIOS to bend the pointer in the TCPA table to this log area.

You tell me - your patches make BIOS allocate it.

> >
> >Alternatively, find some other way to get hold of
> >physical memory.
> >Is there a way to disable the log completely?
> >As defined in your patch, I doubt there's anything there, ever ..
> 
> There is currently no way to disable it. For a machine with a TPM, there
> should be support for an SSDT and this TCPA table for the BIOS to write logs
> into. So I allocate both and Linux for example can then show an empty table
> in /sys/kernel/security/tpm0/ascii_bios_measurements when the passthrough
> driver is used. I am working on a TPM driver for a CUSE TPM(CUSE = character
> device in user space) where we want the BIOS to behave exactly like the BIOS
> on real hardware and write its measurements into this log. I know at least
> that this then works the way it is implemented now.
> 
>    Stefan

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

[Stefan Berger]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

"Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:07:28 AM:


> > >If you need bios to allocate this memory, then we will
> > >need a new allocation type for this, add it to linker
> > >in bios and qemu.
> > 
> > Why does the BIOS 'need' to allocate it? Why can it not just use the 
memory
> > that QEMU allocates? Obviously I am using the 'pointer relocation' 
feature
> > of the BIOS to bend the pointer in the TCPA table to this log area.
> 
> You tell me - your patches make BIOS allocate it.

No, the BIOS does not allocate it, it merely relocates the pointer.

One example here:

    fadt->firmware_ctrl = cpu_to_le32(facs);
    /* FACS address to be filled by Guest linker */
    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE, 
                                   ACPI_BUILD_TABLE_FILE,
                                   table_data, &fadt->firmware_ctrl,
                                   sizeof fadt->firmware_ctrl);

similar example here:

    log_area_start_address = table_data->len + sizeof(*tcpa);
 
    [...]
    tcpa->log_area_start_address = cpu_to_le64(log_area_start_address);

    /* 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->log_area_start_address,
                                   sizeof(tcpa->log_area_start_address));



   Stefan

[-- Attachment #2: Type: text/html, Size: 2945 bytes --]

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

[Michael S. Tsirkin]

On Wed, Jul 30, 2014 at 11:13:07AM -0400, Stefan Berger wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:07:28 AM:
> 
> 
> > > >If you need bios to allocate this memory, then we will
> > > >need a new allocation type for this, add it to linker
> > > >in bios and qemu.
> > >
> > > Why does the BIOS 'need' to allocate it? Why can it not just use the memory
> > > that QEMU allocates? Obviously I am using the 'pointer relocation' feature
> > > of the BIOS to bend the pointer in the TCPA table to this log area.
> >
> > You tell me - your patches make BIOS allocate it.
> 
> No, the BIOS does not allocate it, it merely relocates the pointer.

Yes it does :)
You pushed the log as part of the table_data which bios
allocates memory for, and reads from QEMU.

> One example here:
> 
>     fadt->firmware_ctrl = cpu_to_le32(facs);
>     /* FACS address to be filled by Guest linker */
>     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
>                                    ACPI_BUILD_TABLE_FILE,
>                                    table_data, &fadt->firmware_ctrl,
>                                    sizeof fadt->firmware_ctrl);
> 
> similar example here:
> 
>     log_area_start_address = table_data->len + sizeof(*tcpa);
>  
>     [...]
>     tcpa->log_area_start_address = cpu_to_le64(log_area_start_address);
> 
>     /* 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->log_area_start_address,
>                                    sizeof(tcpa->log_area_start_address));
> 
> 
> 
>    Stefan


It relocates it to within the table that it allocated.

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

[Stefan Berger]

[-- Attachment #1: Type: text/plain, Size: 1607 bytes --]

"Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:25:25 AM:

> From: "Michael S. Tsirkin" <mst@redhat.com>
> To: Stefan Berger/Watson/IBM@IBMUS
> Cc: lersek@redhat.com, qemu-devel@nongnu.org, Stefan Berger 
> <stefanb@linux.vnet.ibm.com>
> Date: 07/30/2014 11:25 AM
> Subject: Re: [PATCH v2] Add ACPI tables for TPM
> 
> On Wed, Jul 30, 2014 at 11:13:07AM -0400, Stefan Berger wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> wrote on 07/30/2014 11:07:28 AM:
> > 
> > 
> > > > >If you need bios to allocate this memory, then we will
> > > > >need a new allocation type for this, add it to linker
> > > > >in bios and qemu.
> > > >
> > > > Why does the BIOS 'need' to allocate it? Why can it not just 
> use the memory
> > > > that QEMU allocates? Obviously I am using the 'pointer 
> relocation' feature
> > > > of the BIOS to bend the pointer in the TCPA table to this log 
area.
> > >
> > > You tell me - your patches make BIOS allocate it.
> > 
> > No, the BIOS does not allocate it, it merely relocates the pointer.
> 
> Yes it does :)

Of course it allocates memory for the ACPI tables, but it doesn't do this 
by an instruction 'allocate memory and put the pointer here.'

> You pushed the log as part of the table_data which bios
> allocates memory for, and reads from QEMU.

Yes, and then it relocates that pointer to the beginning of the log area. 
That log area is part of that allocation.

Following what I stated in the other mail regarding the S3 resume vector 
and the slew of ACPI tables that need to be kept around,
why do we need to treat the TCPA table differently ?

   Stefan

[-- Attachment #2: Type: text/html, Size: 2358 bytes --]