[PATCH v3 0/4] vTPM/aarch64 ACPI support

[PATCH v3 0/4] vTPM/aarch64 ACPI support

[Eric Auger]

Those patches bring MMIO TPM TIS ACPI support in machvirt.
The TPM2 build function is converted to build_append style.
Then the code is moved to the generic part.

On ARM, the TPM2 table is added when the TPM TIS sysbus
device is dynamically instantiated in machvirt.

Also the TPM2 device object is described in the DSDT.

Many thanks to Ard for his support.

Tested with LUKS partition automatic decryption. Also
tested with new bios-tables-test dedicated tests,
sent separately.

Depends on "acpi: tpm: Do not build TCPA table for TPM 2"

Best Regards

Eric

This series can be found at:
https://github.com/eauger/qemu/tree/v5.0-tpm-acpi-v3

History:
v2 -> v3:
- Rebase on top of Stefan's
  "acpi: tpm: Do not build TCPA table for TPM 2"
- brings conversion to build_append

v1 -> v2:
- move build_tpm2() in the generic code (Michael)
- collect Stefan's R-b on 3/3

Eric Auger (4):
  acpi: Convert build_tpm2() to build_append* API
  acpi: Move build_tpm2() in the generic part
  arm/acpi: TPM2 ACPI table support
  arm/acpi: Add the TPM2.0 device under the DSDT

 include/hw/acpi/aml-build.h |  2 ++
 include/sysemu/tpm.h        |  2 ++
 hw/acpi/aml-build.c         | 45 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 39 ++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c        | 34 ----------------------------
 5 files changed, 88 insertions(+), 34 deletions(-)

-- 
2.20.1

[PATCH v3 1/4] acpi: Convert build_tpm2() to build_append* API

[Eric Auger]

In preparation of its move to the generic acpi code,
let's convert build_tpm2() to use build_append API. This
latter now is prefered in place of direct ACPI struct field
settings with manual endianness conversion.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/i386/acpi-build.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b5669d6c65..f0d35d7b17 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2298,30 +2298,40 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
 static void
 build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
 {
-    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *tpm2_ptr);
+    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
     unsigned log_addr_size = sizeof(tpm2_ptr->log_area_start_address);
     unsigned log_addr_offset =
         (char *)&tpm2_ptr->log_area_start_address - table_data->data;
+    uint8_t start_method_params[12] = {};
 
-    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
+    /* platform class */
+    build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
+    /* reserved */
+    build_append_int_noprefix(table_data, 0, 2);
     if (TPM_IS_TIS_ISA(tpm_find())) {
-        tpm2_ptr->control_area_address = cpu_to_le64(0);
-        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
+        /* address of control area */
+        build_append_int_noprefix(table_data, 0, 8);
+        /* start method */
+        build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO, 4);
     } else if (TPM_IS_CRB(tpm_find())) {
-        tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
-        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);
+        build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
+        build_append_int_noprefix(table_data, TPM2_START_METHOD_CRB, 4);
     } else {
         g_warn_if_reached();
     }
 
-    tpm2_ptr->log_area_minimum_length =
-        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
+    /* platform specific parameters */
+    g_array_append_vals(table_data, &start_method_params, 12);
 
-    acpi_data_push(tcpalog, le32_to_cpu(tpm2_ptr->log_area_minimum_length));
+    /* log area minimum length */
+    build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
+
+    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
     bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
                              false);
 
     /* log area start address to be filled by Guest linker */
+    build_append_int_noprefix(table_data, 0, 8);
     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
                                    log_addr_offset, log_addr_size,
                                    ACPI_BUILD_TPMLOG_FILE, 0);
-- 
2.20.1

[PATCH v3 2/4] acpi: Move build_tpm2() in the generic part

[Eric Auger]

We plan to build the TPM2 table on ARM too. In order to reuse the
generation code, let's move build_tpm2() to aml-build.c.

No change in the implementation.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/hw/acpi/aml-build.h |  2 ++
 hw/acpi/aml-build.c         | 44 +++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c        | 44 -------------------------------------
 3 files changed, 46 insertions(+), 44 deletions(-)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index ed7c89309e..d27da03d64 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -437,4 +437,6 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms);
 
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id);
+
+void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog);
 #endif
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 3681ec6e3d..b37052c1b4 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -26,6 +26,7 @@
 #include "qemu/bitops.h"
 #include "sysemu/numa.h"
 #include "hw/boards.h"
+#include "hw/acpi/tpm.h"
 
 static GArray *build_alloc_array(void)
 {
@@ -1877,6 +1878,49 @@ build_hdr:
                  "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
 }
 
+void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
+{
+    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
+    unsigned log_addr_size = sizeof(tpm2_ptr->log_area_start_address);
+    unsigned log_addr_offset =
+        (char *)&tpm2_ptr->log_area_start_address - table_data->data;
+    uint8_t start_method_params[12] = {};
+
+    /* platform class */
+    build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
+    /* reserved */
+    build_append_int_noprefix(table_data, 0, 2);
+    if (TPM_IS_TIS_ISA(tpm_find())) {
+        /* address of control area */
+        build_append_int_noprefix(table_data, 0, 8);
+        /* start method */
+        build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO, 4);
+    } else if (TPM_IS_CRB(tpm_find())) {
+        build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
+        build_append_int_noprefix(table_data, TPM2_START_METHOD_CRB, 4);
+    } else {
+        g_warn_if_reached();
+    }
+
+    /* platform specific parameters */
+    g_array_append_vals(table_data, &start_method_params, 12);
+
+    /* log area minimum length */
+    build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
+
+    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
+    bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
+                             false);
+
+    /* log area start address to be filled by Guest linker */
+    build_append_int_noprefix(table_data, 0, 8);
+    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
+                                   log_addr_offset, log_addr_size,
+                                   ACPI_BUILD_TPMLOG_FILE, 0);
+    build_header(linker, table_data,
+                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
+}
+
 /* ACPI 5.0: 6.4.3.8.2 Serial Bus Connection Descriptors */
 static Aml *aml_serial_bus_device(uint8_t serial_bus_type, uint8_t flags,
                                   uint16_t type_flags,
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f0d35d7b17..b7c7583b5f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2295,50 +2295,6 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
                  (void *)tcpa, "TCPA", sizeof(*tcpa), 2, NULL, NULL);
 }
 
-static void
-build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
-{
-    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
-    unsigned log_addr_size = sizeof(tpm2_ptr->log_area_start_address);
-    unsigned log_addr_offset =
-        (char *)&tpm2_ptr->log_area_start_address - table_data->data;
-    uint8_t start_method_params[12] = {};
-
-    /* platform class */
-    build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
-    /* reserved */
-    build_append_int_noprefix(table_data, 0, 2);
-    if (TPM_IS_TIS_ISA(tpm_find())) {
-        /* address of control area */
-        build_append_int_noprefix(table_data, 0, 8);
-        /* start method */
-        build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO, 4);
-    } else if (TPM_IS_CRB(tpm_find())) {
-        build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
-        build_append_int_noprefix(table_data, TPM2_START_METHOD_CRB, 4);
-    } else {
-        g_warn_if_reached();
-    }
-
-    /* platform specific parameters */
-    g_array_append_vals(table_data, &start_method_params, 12);
-
-    /* log area minimum length */
-    build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
-
-    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
-    bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
-                             false);
-
-    /* log area start address to be filled by Guest linker */
-    build_append_int_noprefix(table_data, 0, 8);
-    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
-                                   log_addr_offset, log_addr_size,
-                                   ACPI_BUILD_TPMLOG_FILE, 0);
-    build_header(linker, table_data,
-                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
-}
-
 #define HOLE_640K_START  (640 * KiB)
 #define HOLE_640K_END   (1 * MiB)
 
-- 
2.20.1

[PATCH v3 3/4] arm/acpi: TPM2 ACPI table support

[Eric Auger]

Add a TPM2 ACPI table if a TPM2.0 sysbus device has been
dynamically instantiated.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v2 -> v3:
- do not to need to create the log area anymore

v1 -> v2:
- reuse generic build_tpm2() and alloc log area externally
- call tpm_find() once in build_tpm2()
---
 include/sysemu/tpm.h     | 2 ++
 hw/acpi/aml-build.c      | 5 +++--
 hw/arm/virt-acpi-build.c | 7 +++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index f37851b1aa..03fb25941c 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -50,6 +50,8 @@ typedef struct TPMIfClass {
 
 #define TPM_IS_TIS_ISA(chr)                         \
     object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
+#define TPM_IS_TIS_SYSBUS(chr)                      \
+    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
 #define TPM_IS_CRB(chr)                             \
     object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
 #define TPM_IS_SPAPR(chr)                           \
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index b37052c1b4..d24e9e6c3a 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1885,17 +1885,18 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
     unsigned log_addr_offset =
         (char *)&tpm2_ptr->log_area_start_address - table_data->data;
     uint8_t start_method_params[12] = {};
+    TPMIf *tpmif = tpm_find();
 
     /* platform class */
     build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
     /* reserved */
     build_append_int_noprefix(table_data, 0, 2);
-    if (TPM_IS_TIS_ISA(tpm_find())) {
+    if (TPM_IS_TIS_ISA(tpmif) || TPM_IS_TIS_SYSBUS(tpmif)) {
         /* address of control area */
         build_append_int_noprefix(table_data, 0, 8);
         /* start method */
         build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO, 4);
-    } else if (TPM_IS_CRB(tpm_find())) {
+    } else if (TPM_IS_CRB(tpmif)) {
         build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
         build_append_int_noprefix(table_data, TPM2_START_METHOD_CRB, 4);
     } else {
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 1b0a584c7b..6d152ab481 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -41,12 +41,14 @@
 #include "hw/acpi/pci.h"
 #include "hw/acpi/memory_hotplug.h"
 #include "hw/acpi/generic_event_device.h"
+#include "hw/acpi/tpm.h"
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
 #include "hw/arm/virt.h"
 #include "hw/mem/nvdimm.h"
 #include "sysemu/numa.h"
 #include "sysemu/reset.h"
+#include "sysemu/tpm.h"
 #include "kvm_arm.h"
 #include "migration/vmstate.h"
 #include "hw/acpi/ghes.h"
@@ -844,6 +846,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
         build_iort(tables_blob, tables->linker, vms);
     }
 
+    if (tpm_get_version(tpm_find()) == TPM_VERSION_2_0) {
+        acpi_add_table(table_offsets, tables_blob);
+        build_tpm2(tables_blob, tables->linker, tables->tcpalog);
+    }
+
     /* XSDT is pointed to by RSDP */
     xsdt = tables_blob->len;
     build_xsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
-- 
2.20.1

[PATCH v3 4/4] arm/acpi: Add the TPM2.0 device under the DSDT

[Eric Auger]

In case it is dynamically instantiated, add the TPM 2.0 device object
under the DSDT table in the ACPI namespace. Its HID is MSFT0101
while its current resource settings (CRS) property is initialized
with the guest physical address and MMIO size of the device.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

---

v2 -> v3:
- use SYS_BUS_DEVICE() instead of
  (SysBusDevice *)object_dynamic_cast(OBJECT())

v1 -> v2:
- use memory_region_size
- fix mingw compilation issue by casting to uint32_t
- added Stefan's R-b
---
 hw/arm/virt-acpi-build.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 6d152ab481..05a3028500 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -46,6 +46,7 @@
 #include "hw/pci/pci.h"
 #include "hw/arm/virt.h"
 #include "hw/mem/nvdimm.h"
+#include "hw/platform-bus.h"
 #include "sysemu/numa.h"
 #include "sysemu/reset.h"
 #include "sysemu/tpm.h"
@@ -364,6 +365,36 @@ static void acpi_dsdt_add_power_button(Aml *scope)
     aml_append(scope, dev);
 }
 
+static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
+{
+    hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
+    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
+    MemoryRegion *sbdev_mr;
+    SysBusDevice *sbdev;
+    hwaddr tpm_base;
+
+    sbdev = SYS_BUS_DEVICE(tpm_find());
+
+    tpm_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
+    assert(tpm_base != -1);
+
+    tpm_base += pbus_base;
+
+    sbdev_mr = sysbus_mmio_get_region(sbdev, 0);
+
+    Aml *dev = aml_device("TPM0");
+    aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
+    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
+
+    Aml *crs = aml_resource_template();
+    aml_append(crs,
+               aml_memory32_fixed(tpm_base,
+                                  (uint32_t)memory_region_size(sbdev_mr),
+                                  AML_READ_WRITE));
+    aml_append(dev, aml_name_decl("_CRS", crs));
+    aml_append(scope, dev);
+}
+
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
@@ -758,6 +789,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     }
 
     acpi_dsdt_add_power_button(scope);
+    acpi_dsdt_add_tpm(scope, vms);
 
     aml_append(dsdt, scope);
 
-- 
2.20.1

Re: [PATCH v3 1/4] acpi: Convert build_tpm2() to build_append* API

[Stefan Berger]

On 6/1/20 5:57 AM, Eric Auger wrote:
> In preparation of its move to the generic acpi code,
> let's convert build_tpm2() to use build_append API. This
> latter now is prefered in place of direct ACPI struct field
> settings with manual endianness conversion.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   hw/i386/acpi-build.c | 28 +++++++++++++++++++---------
>   1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b5669d6c65..f0d35d7b17 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2298,30 +2298,40 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>   static void
>   build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>   {
> -    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *tpm2_ptr);
> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));

And now you want to build the data structure by pushing fields? I would 
definitely NOT do this.


>       unsigned log_addr_size = sizeof(tpm2_ptr->log_area_start_address);
>       unsigned log_addr_offset =
>           (char *)&tpm2_ptr->log_area_start_address - table_data->data;
> +    uint8_t start_method_params[12] = {};
>   
> -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> +    /* platform class */
> +    build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
> +    /* reserved */
> +    build_append_int_noprefix(table_data, 0, 2);
>       if (TPM_IS_TIS_ISA(tpm_find())) {
> -        tpm2_ptr->control_area_address = cpu_to_le64(0);
> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
> +        /* address of control area */
> +        build_append_int_noprefix(table_data, 0, 8);
> +        /* start method */
> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO, 4);
>       } else if (TPM_IS_CRB(tpm_find())) {
> -        tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);
> +        build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_CRB, 4);
>       } else {
>           g_warn_if_reached();
>       }
>   
> -    tpm2_ptr->log_area_minimum_length =
> -        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
> +    /* platform specific parameters */
> +    g_array_append_vals(table_data, &start_method_params, 12);
>   
> -    acpi_data_push(tcpalog, le32_to_cpu(tpm2_ptr->log_area_minimum_length));
> +    /* log area minimum length */
> +    build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
> +
> +    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
>       bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
>                                false);
>   
>       /* log area start address to be filled by Guest linker */
> +    build_append_int_noprefix(table_data, 0, 8);
>       bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>                                      log_addr_offset, log_addr_size,
>                                      ACPI_BUILD_TPMLOG_FILE, 0);

Re: [PATCH v3 1/4] acpi: Convert build_tpm2() to build_append* API

[Auger Eric]

Hi Stefan,
On 6/2/20 3:30 PM, Stefan Berger wrote:
> On 6/1/20 5:57 AM, Eric Auger wrote:
>> In preparation of its move to the generic acpi code,
>> let's convert build_tpm2() to use build_append API. This
>> latter now is prefered in place of direct ACPI struct field
>> settings with manual endianness conversion.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>   hw/i386/acpi-build.c | 28 +++++++++++++++++++---------
>>   1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index b5669d6c65..f0d35d7b17 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2298,30 +2298,40 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker
>> *linker, GArray *tcpalog)
>>   static void
>>   build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>>   {
>> -    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *tpm2_ptr);
>> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data,
>> sizeof(AcpiTableHeader));
> 
> And now you want to build the data structure by pushing fields? I would
> definitely NOT do this.

If I didn't misinterpret things, this was recommended by Drew and Igor
as buid_append* API avoids to take care of endianness and this is the
API now used in the generic ACPI code. Besides I also think that in that
case it does not simplify things but maybe I did that the wrong way? Or
maybe I didn't understand your remark?

Thanks

Eric
> 
> 
>>       unsigned log_addr_size = sizeof(tpm2_ptr->log_area_start_address);
>>       unsigned log_addr_offset =
>>           (char *)&tpm2_ptr->log_area_start_address - table_data->data;
>> +    uint8_t start_method_params[12] = {};
>>   -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
>> +    /* platform class */
>> +    build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
>> +    /* reserved */
>> +    build_append_int_noprefix(table_data, 0, 2);
>>       if (TPM_IS_TIS_ISA(tpm_find())) {
>> -        tpm2_ptr->control_area_address = cpu_to_le64(0);
>> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
>> +        /* address of control area */
>> +        build_append_int_noprefix(table_data, 0, 8);
>> +        /* start method */
>> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO,
>> 4);
>>       } else if (TPM_IS_CRB(tpm_find())) {
>> -        tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
>> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);
>> +        build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
>> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_CRB, 4);
>>       } else {
>>           g_warn_if_reached();
>>       }
>>   -    tpm2_ptr->log_area_minimum_length =
>> -        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
>> +    /* platform specific parameters */
>> +    g_array_append_vals(table_data, &start_method_params, 12);
>>   -    acpi_data_push(tcpalog,
>> le32_to_cpu(tpm2_ptr->log_area_minimum_length));
>> +    /* log area minimum length */
>> +    build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
>> +
>> +    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
>>       bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE,
>> tcpalog, 1,
>>                                false);
>>         /* log area start address to be filled by Guest linker */
>> +    build_append_int_noprefix(table_data, 0, 8);
>>       bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>                                      log_addr_offset, log_addr_size,
>>                                      ACPI_BUILD_TPMLOG_FILE, 0);
> 
> 

Re: [PATCH v3 1/4] acpi: Convert build_tpm2() to build_append* API

[Stefan Berger]

On 6/2/20 9:55 AM, Auger Eric wrote:
> Hi Stefan,
> On 6/2/20 3:30 PM, Stefan Berger wrote:
>> On 6/1/20 5:57 AM, Eric Auger wrote:
>>> In preparation of its move to the generic acpi code,
>>> let's convert build_tpm2() to use build_append API. This
>>> latter now is prefered in place of direct ACPI struct field
>>> settings with manual endianness conversion.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>    hw/i386/acpi-build.c | 28 +++++++++++++++++++---------
>>>    1 file changed, 19 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index b5669d6c65..f0d35d7b17 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -2298,30 +2298,40 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker
>>> *linker, GArray *tcpalog)
>>>    static void
>>>    build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>>>    {
>>> -    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *tpm2_ptr);
>>> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data,
>>> sizeof(AcpiTableHeader));
>> And now you want to build the data structure by pushing fields? I would
>> definitely NOT do this.
> If I didn't misinterpret things, this was recommended by Drew and Igor
> as buid_append* API avoids to take care of endianness and this is the
> API now used in the generic ACPI code. Besides I also think that in that
> case it does not simplify things but maybe I did that the wrong way? Or
> maybe I didn't understand your remark?


If that's what they are saying... I would prefer filling out data 
structures with functions like cpu_to_acpi16() because that seems to be 
less error prone.


>>
>>>        unsigned log_addr_size = sizeof(tpm2_ptr->log_area_start_address);
>>>        unsigned log_addr_offset =
>>>            (char *)&tpm2_ptr->log_area_start_address - table_data->data;
>>> +    uint8_t start_method_params[12] = {};
>>>    -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
>>> +    /* platform class */
>>> +    build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
>>> +    /* reserved */
>>> +    build_append_int_noprefix(table_data, 0, 2);
>>>        if (TPM_IS_TIS_ISA(tpm_find())) {
>>> -        tpm2_ptr->control_area_address = cpu_to_le64(0);
>>> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
>>> +        /* address of control area */
>>> +        build_append_int_noprefix(table_data, 0, 8);
>>> +        /* start method */
>>> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO,
>>> 4);
>>>        } else if (TPM_IS_CRB(tpm_find())) {
>>> -        tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
>>> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);
>>> +        build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
>>> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_CRB, 4);
>>>        } else {
>>>            g_warn_if_reached();
>>>        }
>>>    -    tpm2_ptr->log_area_minimum_length =
>>> -        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
>>> +    /* platform specific parameters */
>>> +    g_array_append_vals(table_data, &start_method_params, 12);

Maybe this should be wrapped in an inline function like 
build_append_array() or so.


>>>    -    acpi_data_push(tcpalog,
>>> le32_to_cpu(tpm2_ptr->log_area_minimum_length));
>>> +    /* log area minimum length */
>>> +    build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
>>> +
>>> +    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);


At this point we have a double-allocation of log memory on x86_64. You'd 
need the patch I posted to create the TCPA table only for TPM 1.2.



>>>        bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE,
>>> tcpalog, 1,
>>>                                 false);
>>>          /* log area start address to be filled by Guest linker */
>>> +    build_append_int_noprefix(table_data, 0, 8);
>>>        bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>>                                       log_addr_offset, log_addr_size,
>>>                                       ACPI_BUILD_TPMLOG_FILE, 0);
>>

Re: [PATCH v3 1/4] acpi: Convert build_tpm2() to build_append* API

[Auger Eric]

Hi Stefan,

On 6/2/20 4:24 PM, Stefan Berger wrote:
> On 6/2/20 9:55 AM, Auger Eric wrote:
>> Hi Stefan,
>> On 6/2/20 3:30 PM, Stefan Berger wrote:
>>> On 6/1/20 5:57 AM, Eric Auger wrote:
>>>> In preparation of its move to the generic acpi code,
>>>> let's convert build_tpm2() to use build_append API. This
>>>> latter now is prefered in place of direct ACPI struct field
>>>> settings with manual endianness conversion.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>    hw/i386/acpi-build.c | 28 +++++++++++++++++++---------
>>>>    1 file changed, 19 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>> index b5669d6c65..f0d35d7b17 100644
>>>> --- a/hw/i386/acpi-build.c
>>>> +++ b/hw/i386/acpi-build.c
>>>> @@ -2298,30 +2298,40 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker
>>>> *linker, GArray *tcpalog)
>>>>    static void
>>>>    build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>>>>    {
>>>> -    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof
>>>> *tpm2_ptr);
>>>> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data,
>>>> sizeof(AcpiTableHeader));
>>> And now you want to build the data structure by pushing fields? I would
>>> definitely NOT do this.
>> If I didn't misinterpret things, this was recommended by Drew and Igor
>> as buid_append* API avoids to take care of endianness and this is the
>> API now used in the generic ACPI code. Besides I also think that in that
>> case it does not simplify things but maybe I did that the wrong way? Or
>> maybe I didn't understand your remark?
> 
> 
> If that's what they are saying... I would prefer filling out data
> structures with functions like cpu_to_acpi16() because that seems to be
> less error prone.
understood. Let's wait for other comments and we will make a decision
wrt direction.
> 
> 
>>>
>>>>        unsigned log_addr_size =
>>>> sizeof(tpm2_ptr->log_area_start_address);
>>>>        unsigned log_addr_offset =
>>>>            (char *)&tpm2_ptr->log_area_start_address -
>>>> table_data->data;
>>>> +    uint8_t start_method_params[12] = {};
>>>>    -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
>>>> +    /* platform class */
>>>> +    build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
>>>> +    /* reserved */
>>>> +    build_append_int_noprefix(table_data, 0, 2);
>>>>        if (TPM_IS_TIS_ISA(tpm_find())) {
>>>> -        tpm2_ptr->control_area_address = cpu_to_le64(0);
>>>> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
>>>> +        /* address of control area */
>>>> +        build_append_int_noprefix(table_data, 0, 8);
>>>> +        /* start method */
>>>> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO,
>>>> 4);
>>>>        } else if (TPM_IS_CRB(tpm_find())) {
>>>> -        tpm2_ptr->control_area_address =
>>>> cpu_to_le64(TPM_CRB_ADDR_CTRL);
>>>> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);
>>>> +        build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
>>>> +        build_append_int_noprefix(table_data,
>>>> TPM2_START_METHOD_CRB, 4);
>>>>        } else {
>>>>            g_warn_if_reached();
>>>>        }
>>>>    -    tpm2_ptr->log_area_minimum_length =
>>>> -        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
>>>> +    /* platform specific parameters */
>>>> +    g_array_append_vals(table_data, &start_method_params, 12);
> 
> Maybe this should be wrapped in an inline function like
> build_append_array() or so.
Hum OK
> 
> 
>>>>    -    acpi_data_push(tcpalog,
>>>> le32_to_cpu(tpm2_ptr->log_area_minimum_length));
>>>> +    /* log area minimum length */
>>>> +    build_append_int_noprefix(table_data,
>>>> TPM_LOG_AREA_MINIMUM_SIZE, 4);
>>>> +
>>>> +    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
> 
> 
> At this point we have a double-allocation of log memory on x86_64. You'd
> need the patch I posted to create the TCPA table only for TPM 1.2.
This series applies on top of this patch (mentionned in the cover letter).
> 
> 
> 
>>>>        bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE,
>>>> tcpalog, 1,
>>>>                                 false);
>>>>          /* log area start address to be filled by Guest linker */
>>>> +    build_append_int_noprefix(table_data, 0, 8);
>>>>        bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>>>                                       log_addr_offset, log_addr_size,
>>>>                                       ACPI_BUILD_TPMLOG_FILE, 0);
>>>
> 
> 
> 
Thanks

Eric

Re: [PATCH v3 1/4] acpi: Convert build_tpm2() to build_append* API

[Igor Mammedov]

On Mon,  1 Jun 2020 11:57:34 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> In preparation of its move to the generic acpi code,
> let's convert build_tpm2() to use build_append API. This
> latter now is prefered in place of direct ACPI struct field
> settings with manual endianness conversion.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/i386/acpi-build.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b5669d6c65..f0d35d7b17 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2298,30 +2298,40 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>  static void
>  build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>  {
> -    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *tpm2_ptr);
> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));

>      unsigned log_addr_size = sizeof(tpm2_ptr->log_area_start_address);
>      unsigned log_addr_offset =
>          (char *)&tpm2_ptr->log_area_start_address - table_data->data;

Is this the reason why you've kept Acpi20TPM2 around?


> +    uint8_t start_method_params[12] = {};
>  
> -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> +    /* platform class */
pls verbatim filed names from spec in comments

> +    build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
> +    /* reserved */
> +    build_append_int_noprefix(table_data, 0, 2);
>      if (TPM_IS_TIS_ISA(tpm_find())) {
> -        tpm2_ptr->control_area_address = cpu_to_le64(0);
> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
> +        /* address of control area */
> +        build_append_int_noprefix(table_data, 0, 8);
> +        /* start method */
> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO, 4);
>      } else if (TPM_IS_CRB(tpm_find())) {
> -        tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);

missing field name comments

> +        build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_CRB, 4);
>      } else {
>          g_warn_if_reached();
>      }

considering fields are the same I'd also restructure above as
    if () {
       control_area_address = 
       start_method =
    ...
    }
    /* address of control area */
    build_append_int_noprefix(table_data, control_area_address, 8);
    /* start method */
    build_append_int_noprefix(table_data, start_method, 4); 

which is bit easier to read 
    
>  
> -    tpm2_ptr->log_area_minimum_length =
> -        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
> +    /* platform specific parameters */
> +    g_array_append_vals(table_data, &start_method_params, 12);
>  
> -    acpi_data_push(tcpalog, le32_to_cpu(tpm2_ptr->log_area_minimum_length));
> +    /* log area minimum length */
> +    build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
> +
> +    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
>      bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
>                               false);

I suggest to drop Acpi20TPM2 with pointer math above and use approach similar
to build_ghes_v2/address_offset, i.e. get actual offest here:

      log_addr_offset = table_data->len

and s/log_addr_size/8/

>      /* log area start address to be filled by Guest linker */
> +    build_append_int_noprefix(table_data, 0, 8);
>      bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>                                     log_addr_offset, log_addr_size,
>                                     ACPI_BUILD_TPMLOG_FILE, 0);

Re: [PATCH v3 1/4] acpi: Convert build_tpm2() to build_append* API

[Igor Mammedov]

On Tue, 2 Jun 2020 10:24:03 -0400
Stefan Berger <stefanb@linux.ibm.com> wrote:

> On 6/2/20 9:55 AM, Auger Eric wrote:
> > Hi Stefan,
> > On 6/2/20 3:30 PM, Stefan Berger wrote:  
> >> On 6/1/20 5:57 AM, Eric Auger wrote:  
> >>> In preparation of its move to the generic acpi code,
> >>> let's convert build_tpm2() to use build_append API. This
> >>> latter now is prefered in place of direct ACPI struct field
> >>> settings with manual endianness conversion.
> >>>
> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>> ---
> >>>    hw/i386/acpi-build.c | 28 +++++++++++++++++++---------
> >>>    1 file changed, 19 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>> index b5669d6c65..f0d35d7b17 100644
> >>> --- a/hw/i386/acpi-build.c
> >>> +++ b/hw/i386/acpi-build.c
> >>> @@ -2298,30 +2298,40 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker
> >>> *linker, GArray *tcpalog)
> >>>    static void
> >>>    build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
> >>>    {
> >>> -    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *tpm2_ptr);
> >>> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data,
> >>> sizeof(AcpiTableHeader));  
> >> And now you want to build the data structure by pushing fields? I would
> >> definitely NOT do this.  
> > If I didn't misinterpret things, this was recommended by Drew and Igor
> > as buid_append* API avoids to take care of endianness and this is the
> > API now used in the generic ACPI code. Besides I also think that in that
> > case it does not simplify things but maybe I did that the wrong way? Or
> > maybe I didn't understand your remark?  
> 
> 
> If that's what they are saying... I would prefer filling out data 
> structures with functions like cpu_to_acpi16() because that seems to be 
> less error prone.

practice showed it was opposite, it was easy to forget about cpu_to_le
build_append_int_noprefix() API we don't have to care about endiannes
issues anymore at places where API is used.
Another reasons for using build_append_int_noprefix() is that one doesn't
have to define packed structures, work around unaligned access and bit plus is
when I review patch it boiles down to comparing it with table in a spec
row by row as API enforces the same order as in spec, which makes reviews
much easier.

> 
> >>  
> >>>        unsigned log_addr_size = sizeof(tpm2_ptr->log_area_start_address);
> >>>        unsigned log_addr_offset =
> >>>            (char *)&tpm2_ptr->log_area_start_address - table_data->data;
> >>> +    uint8_t start_method_params[12] = {};
> >>>    -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> >>> +    /* platform class */
> >>> +    build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
> >>> +    /* reserved */
> >>> +    build_append_int_noprefix(table_data, 0, 2);
> >>>        if (TPM_IS_TIS_ISA(tpm_find())) {
> >>> -        tpm2_ptr->control_area_address = cpu_to_le64(0);
> >>> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
> >>> +        /* address of control area */
> >>> +        build_append_int_noprefix(table_data, 0, 8);
> >>> +        /* start method */
> >>> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO,
> >>> 4);
> >>>        } else if (TPM_IS_CRB(tpm_find())) {
> >>> -        tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
> >>> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);
> >>> +        build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
> >>> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_CRB, 4);
> >>>        } else {
> >>>            g_warn_if_reached();
> >>>        }
> >>>    -    tpm2_ptr->log_area_minimum_length =
> >>> -        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
> >>> +    /* platform specific parameters */
> >>> +    g_array_append_vals(table_data, &start_method_params, 12);  
> 
> Maybe this should be wrapped in an inline function like 
> build_append_array() or so.
I guss I'm just used to g_array_append_vals() but build_append_array() looks nicer
and consistent with build_append_foo API

> 
> 
> >>>    -    acpi_data_push(tcpalog,
> >>> le32_to_cpu(tpm2_ptr->log_area_minimum_length));
> >>> +    /* log area minimum length */
> >>> +    build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
> >>> +
> >>> +    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);  
> 
> 
> At this point we have a double-allocation of log memory on x86_64. You'd 
> need the patch I posted to create the TCPA table only for TPM 1.2.
> 
> 
> 
> >>>        bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE,
> >>> tcpalog, 1,
> >>>                                 false);
> >>>          /* log area start address to be filled by Guest linker */
> >>> +    build_append_int_noprefix(table_data, 0, 8);
> >>>        bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> >>>                                       log_addr_offset, log_addr_size,
> >>>                                       ACPI_BUILD_TPMLOG_FILE, 0);  
> >>  
> 
> 
> 

Re: [PATCH v3 4/4] arm/acpi: Add the TPM2.0 device under the DSDT

[Igor Mammedov]

On Mon,  1 Jun 2020 11:57:37 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> In case it is dynamically instantiated, add the TPM 2.0 device object
> under the DSDT table in the ACPI namespace. Its HID is MSFT0101
> while its current resource settings (CRS) property is initialized
> with the guest physical address and MMIO size of the device.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> ---
> 
> v2 -> v3:
> - use SYS_BUS_DEVICE() instead of
>   (SysBusDevice *)object_dynamic_cast(OBJECT())
> 
> v1 -> v2:
> - use memory_region_size
> - fix mingw compilation issue by casting to uint32_t
> - added Stefan's R-b
> ---
>  hw/arm/virt-acpi-build.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 6d152ab481..05a3028500 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -46,6 +46,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/arm/virt.h"
>  #include "hw/mem/nvdimm.h"
> +#include "hw/platform-bus.h"
>  #include "sysemu/numa.h"
>  #include "sysemu/reset.h"
>  #include "sysemu/tpm.h"
> @@ -364,6 +365,36 @@ static void acpi_dsdt_add_power_button(Aml *scope)
>      aml_append(scope, dev);
>  }
>  
> +static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
> +{
> +    hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
> +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> +    MemoryRegion *sbdev_mr;
> +    SysBusDevice *sbdev;
> +    hwaddr tpm_base;
> +
> +    sbdev = SYS_BUS_DEVICE(tpm_find());
> +
> +    tpm_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> +    assert(tpm_base != -1);
> +
> +    tpm_base += pbus_base;
> +
> +    sbdev_mr = sysbus_mmio_get_region(sbdev, 0);
> +
> +    Aml *dev = aml_device("TPM0");
> +    aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> +
> +    Aml *crs = aml_resource_template();
> +    aml_append(crs,
> +               aml_memory32_fixed(tpm_base,
> +                                  (uint32_t)memory_region_size(sbdev_mr),
> +                                  AML_READ_WRITE));
> +    aml_append(dev, aml_name_decl("_CRS", crs));
> +    aml_append(scope, dev);
> +}
> +
>  static void
>  build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> @@ -758,6 +789,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      }
>  
>      acpi_dsdt_add_power_button(scope);
> +    acpi_dsdt_add_tpm(scope, vms);
shouldn't be this guarded by check if TPM device is present?

perhaps pass found here tpm to acpi_dsdt_add_tpm() as an argument

>  
>      aml_append(dsdt, scope);
>  

Re: [PATCH v3 1/4] acpi: Convert build_tpm2() to build_append* API

[Auger Eric]

Hi Igor,

On 6/5/20 4:23 PM, Igor Mammedov wrote:
> On Mon,  1 Jun 2020 11:57:34 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
> 
>> In preparation of its move to the generic acpi code,
>> let's convert build_tpm2() to use build_append API. This
>> latter now is prefered in place of direct ACPI struct field
>> settings with manual endianness conversion.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/i386/acpi-build.c | 28 +++++++++++++++++++---------
>>  1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index b5669d6c65..f0d35d7b17 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2298,30 +2298,40 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>>  static void
>>  build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>>  {
>> -    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *tpm2_ptr);
>> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
> 
>>      unsigned log_addr_size = sizeof(tpm2_ptr->log_area_start_address);
>>      unsigned log_addr_offset =
>>          (char *)&tpm2_ptr->log_area_start_address - table_data->data;
> 
> Is this the reason why you've kept Acpi20TPM2 around?
Yes it is
> 
> 
>> +    uint8_t start_method_params[12] = {};
>>  
>> -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
>> +    /* platform class */
> pls verbatim filed names from spec in comments
Do you want "Platform Class" instead?
> 
>> +    build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
>> +    /* reserved */
>> +    build_append_int_noprefix(table_data, 0, 2);
>>      if (TPM_IS_TIS_ISA(tpm_find())) {
>> -        tpm2_ptr->control_area_address = cpu_to_le64(0);
>> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
>> +        /* address of control area */
>> +        build_append_int_noprefix(table_data, 0, 8);
>> +        /* start method */
>> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO, 4);
>>      } else if (TPM_IS_CRB(tpm_find())) {
>> -        tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
>> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);
> 
> missing field name comments
OK
> 
>> +        build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
>> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_CRB, 4);
>>      } else {
>>          g_warn_if_reached();
>>      }
> 
> considering fields are the same I'd also restructure above as
>     if () {
>        control_area_address = 
>        start_method =
>     ...
>     }
>     /* address of control area */
>     build_append_int_noprefix(table_data, control_area_address, 8);
>     /* start method */
>     build_append_int_noprefix(table_data, start_method, 4); 
> 
> which is bit easier to read 
OK
>     
>>  
>> -    tpm2_ptr->log_area_minimum_length =
>> -        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
>> +    /* platform specific parameters */
>> +    g_array_append_vals(table_data, &start_method_params, 12);
>>  
>> -    acpi_data_push(tcpalog, le32_to_cpu(tpm2_ptr->log_area_minimum_length));
>> +    /* log area minimum length */
>> +    build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
>> +
>> +    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
>>      bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
>>                               false);
> 
> I suggest to drop Acpi20TPM2 with pointer math above and use approach similar
> to build_ghes_v2/address_offset, i.e. get actual offest here:
> 
>       log_addr_offset = table_data->len
yes, that's simpler. Thank you for the suggestion.
> 
> and s/log_addr_size/8/
OK

Thanks

Eric
> 
>>      /* log area start address to be filled by Guest linker */
>> +    build_append_int_noprefix(table_data, 0, 8);
>>      bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>                                     log_addr_offset, log_addr_size,
>>                                     ACPI_BUILD_TPMLOG_FILE, 0);
> 

Re: [PATCH v3 1/4] acpi: Convert build_tpm2() to build_append* API

[Igor Mammedov]

On Wed, 10 Jun 2020 18:15:07 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Igor,
> 
> On 6/5/20 4:23 PM, Igor Mammedov wrote:
> > On Mon,  1 Jun 2020 11:57:34 +0200
> > Eric Auger <eric.auger@redhat.com> wrote:
> >   
> >> In preparation of its move to the generic acpi code,
> >> let's convert build_tpm2() to use build_append API. This
> >> latter now is prefered in place of direct ACPI struct field
> >> settings with manual endianness conversion.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> ---
> >>  hw/i386/acpi-build.c | 28 +++++++++++++++++++---------
> >>  1 file changed, 19 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index b5669d6c65..f0d35d7b17 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -2298,30 +2298,40 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
> >>  static void
> >>  build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
> >>  {
> >> -    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *tpm2_ptr);
> >> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));  
> >   
> >>      unsigned log_addr_size = sizeof(tpm2_ptr->log_area_start_address);
> >>      unsigned log_addr_offset =
> >>          (char *)&tpm2_ptr->log_area_start_address - table_data->data;  
> > 
> > Is this the reason why you've kept Acpi20TPM2 around?  
> Yes it is
> > 
> >   
> >> +    uint8_t start_method_params[12] = {};
> >>  
> >> -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> >> +    /* platform class */  
> > pls verbatim filed names from spec in comments  
> Do you want "Platform Class" instead?
yep, just copy past field names from spec as is.

> >   
> >> +    build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
> >> +    /* reserved */
> >> +    build_append_int_noprefix(table_data, 0, 2);
> >>      if (TPM_IS_TIS_ISA(tpm_find())) {
> >> -        tpm2_ptr->control_area_address = cpu_to_le64(0);
> >> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
> >> +        /* address of control area */
> >> +        build_append_int_noprefix(table_data, 0, 8);
> >> +        /* start method */
> >> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO, 4);
> >>      } else if (TPM_IS_CRB(tpm_find())) {
> >> -        tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
> >> -        tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);  
> > 
> > missing field name comments  
> OK
> >   
> >> +        build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
> >> +        build_append_int_noprefix(table_data, TPM2_START_METHOD_CRB, 4);
> >>      } else {
> >>          g_warn_if_reached();
> >>      }  
> > 
> > considering fields are the same I'd also restructure above as
> >     if () {
> >        control_area_address = 
> >        start_method =
> >     ...
> >     }
> >     /* address of control area */
> >     build_append_int_noprefix(table_data, control_area_address, 8);
> >     /* start method */
> >     build_append_int_noprefix(table_data, start_method, 4); 
> > 
> > which is bit easier to read   
> OK
> >       
> >>  
> >> -    tpm2_ptr->log_area_minimum_length =
> >> -        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
> >> +    /* platform specific parameters */
> >> +    g_array_append_vals(table_data, &start_method_params, 12);
> >>  
> >> -    acpi_data_push(tcpalog, le32_to_cpu(tpm2_ptr->log_area_minimum_length));
> >> +    /* log area minimum length */
> >> +    build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
> >> +
> >> +    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
> >>      bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
> >>                               false);  
> > 
> > I suggest to drop Acpi20TPM2 with pointer math above and use approach similar
> > to build_ghes_v2/address_offset, i.e. get actual offest here:
> > 
> >       log_addr_offset = table_data->len  
> yes, that's simpler. Thank you for the suggestion.
> > 
> > and s/log_addr_size/8/  
> OK
> 
> Thanks
> 
> Eric
> >   
> >>      /* log area start address to be filled by Guest linker */
> >> +    build_append_int_noprefix(table_data, 0, 8);
> >>      bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> >>                                     log_addr_offset, log_addr_size,
> >>                                     ACPI_BUILD_TPMLOG_FILE, 0);  
> >   

Re: [PATCH v3 4/4] arm/acpi: Add the TPM2.0 device under the DSDT

[Auger Eric]

Hi Igor,

On 6/5/20 4:45 PM, Igor Mammedov wrote:
> On Mon,  1 Jun 2020 11:57:37 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
> 
>> In case it is dynamically instantiated, add the TPM 2.0 device object
>> under the DSDT table in the ACPI namespace. Its HID is MSFT0101
>> while its current resource settings (CRS) property is initialized
>> with the guest physical address and MMIO size of the device.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>>
>> ---
>>
>> v2 -> v3:
>> - use SYS_BUS_DEVICE() instead of
>>   (SysBusDevice *)object_dynamic_cast(OBJECT())
>>
>> v1 -> v2:
>> - use memory_region_size
>> - fix mingw compilation issue by casting to uint32_t
>> - added Stefan's R-b
>> ---
>>  hw/arm/virt-acpi-build.c | 32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 6d152ab481..05a3028500 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -46,6 +46,7 @@
>>  #include "hw/pci/pci.h"
>>  #include "hw/arm/virt.h"
>>  #include "hw/mem/nvdimm.h"
>> +#include "hw/platform-bus.h"
>>  #include "sysemu/numa.h"
>>  #include "sysemu/reset.h"
>>  #include "sysemu/tpm.h"
>> @@ -364,6 +365,36 @@ static void acpi_dsdt_add_power_button(Aml *scope)
>>      aml_append(scope, dev);
>>  }
>>  
>> +static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
>> +{
>> +    hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
>> +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
>> +    MemoryRegion *sbdev_mr;
>> +    SysBusDevice *sbdev;
>> +    hwaddr tpm_base;
>> +
>> +    sbdev = SYS_BUS_DEVICE(tpm_find());
>> +
>> +    tpm_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
>> +    assert(tpm_base != -1);
>> +
>> +    tpm_base += pbus_base;
>> +
>> +    sbdev_mr = sysbus_mmio_get_region(sbdev, 0);
>> +
>> +    Aml *dev = aml_device("TPM0");
>> +    aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
>> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>> +
>> +    Aml *crs = aml_resource_template();
>> +    aml_append(crs,
>> +               aml_memory32_fixed(tpm_base,
>> +                                  (uint32_t)memory_region_size(sbdev_mr),
>> +                                  AML_READ_WRITE));
>> +    aml_append(dev, aml_name_decl("_CRS", crs));
>> +    aml_append(scope, dev);
>> +}
>> +
>>  static void
>>  build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>  {
>> @@ -758,6 +789,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>      }
>>  
>>      acpi_dsdt_add_power_button(scope);
>> +    acpi_dsdt_add_tpm(scope, vms);
> shouldn't be this guarded by check if TPM device is present?
Yes I should. the check was in v2 and was dropped in v3.

thanks

Eric
> 
> perhaps pass found here tpm to acpi_dsdt_add_tpm() as an argument
> 
>>  
>>      aml_append(dsdt, scope);
>>  
>