[PATCH v4 0/5] vTPM/aarch64 ACPI support

[PATCH v4 0/5] 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-v4
(includes the related DSDT and TPM2 ACPI table tests)

History:
v3 -> v4:
- some rework in build_tpm2() as suggested by Igor
- Restored tpm presence check in acpi_dsdt_add_tpm()
- add the doc related patch

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 (5):
  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
  docs/specs/tpm: ACPI boot now supported for TPM/ARM

 docs/specs/tpm.rst          |  2 --
 include/hw/acpi/aml-build.h |  2 ++
 include/sysemu/tpm.h        |  2 ++
 hw/acpi/aml-build.c         | 52 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 41 +++++++++++++++++++++++++++++
 hw/i386/acpi-build.c        | 34 ------------------------
 6 files changed, 97 insertions(+), 36 deletions(-)

-- 
2.20.1

[PATCH v4 1/5] 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>

---

v3 -> v4:
- Don't use Acpi20TPM2 *tpm2_ptr anymore
- Use variables for control area start address and start method
- Simplified arg values passed to bios_linker_loader_add_pointer
- use g_assert_not_reached()
---
 hw/i386/acpi-build.c | 49 +++++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b5669d6c65..f150d95ecc 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2298,35 +2298,52 @@ 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);
-    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] = {};
+    unsigned log_addr_offset, tpm2_start;
+    uint64_t control_area_start_address;
+    uint32_t start_method;
+    void *tpm2_ptr;
 
-    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
+    tpm2_start = table_data->len;
+    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
+
+    /* 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);
+        control_area_start_address = 0;
+        start_method = TPM2_START_METHOD_MMIO;
     } 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);
+        control_area_start_address = TPM_CRB_ADDR_CTRL;
+        start_method = TPM2_START_METHOD_CRB;
     } else {
-        g_warn_if_reached();
+        g_assert_not_reached();
     }
+    /* Address of Control Area */
+    build_append_int_noprefix(table_data, control_area_start_address, 8);
+    /* Start Method */
+    build_append_int_noprefix(table_data, start_method, 4);
 
-    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,
+                        ARRAY_SIZE(start_method_params));
 
-    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 */
+    log_addr_offset = table_data->len;
+    build_append_int_noprefix(table_data, 0, 8);
+    /* Log Area Start Address to be filled by Guest linker */
     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
-                                   log_addr_offset, log_addr_size,
+                                   log_addr_offset, 8,
                                    ACPI_BUILD_TPMLOG_FILE, 0);
     build_header(linker, table_data,
-                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
+                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4, NULL, NULL);
 }
 
 #define HOLE_640K_START  (640 * KiB)
-- 
2.20.1

[PATCH v4 2/5] 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         | 51 +++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c        | 51 -------------------------------------
 3 files changed, 53 insertions(+), 51 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..2a1d9fc839 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,56 @@ build_hdr:
                  "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
 }
 
+void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
+{
+    uint8_t start_method_params[12] = {};
+    unsigned log_addr_offset, tpm2_start;
+    uint64_t control_area_start_address;
+    uint32_t start_method;
+    void *tpm2_ptr;
+
+    tpm2_start = table_data->len;
+    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
+
+    /* 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())) {
+        control_area_start_address = 0;
+        start_method = TPM2_START_METHOD_MMIO;
+    } else if (TPM_IS_CRB(tpm_find())) {
+        control_area_start_address = TPM_CRB_ADDR_CTRL;
+        start_method = TPM2_START_METHOD_CRB;
+    } else {
+        g_assert_not_reached();
+    }
+    /* Address of Control Area */
+    build_append_int_noprefix(table_data, control_area_start_address, 8);
+    /* Start Method */
+    build_append_int_noprefix(table_data, start_method, 4);
+
+    /* Platform Specific Parameters */
+    g_array_append_vals(table_data, &start_method_params,
+                        ARRAY_SIZE(start_method_params));
+
+    /* 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_addr_offset = table_data->len;
+    build_append_int_noprefix(table_data, 0, 8);
+    /* Log Area Start Address to be filled by Guest linker */
+    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
+                                   log_addr_offset, 8,
+                                   ACPI_BUILD_TPMLOG_FILE, 0);
+    build_header(linker, table_data,
+                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 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 f150d95ecc..b7c7583b5f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2295,57 +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)
-{
-    uint8_t start_method_params[12] = {};
-    unsigned log_addr_offset, tpm2_start;
-    uint64_t control_area_start_address;
-    uint32_t start_method;
-    void *tpm2_ptr;
-
-    tpm2_start = table_data->len;
-    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
-
-    /* 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())) {
-        control_area_start_address = 0;
-        start_method = TPM2_START_METHOD_MMIO;
-    } else if (TPM_IS_CRB(tpm_find())) {
-        control_area_start_address = TPM_CRB_ADDR_CTRL;
-        start_method = TPM2_START_METHOD_CRB;
-    } else {
-        g_assert_not_reached();
-    }
-    /* Address of Control Area */
-    build_append_int_noprefix(table_data, control_area_start_address, 8);
-    /* Start Method */
-    build_append_int_noprefix(table_data, start_method, 4);
-
-    /* Platform Specific Parameters */
-    g_array_append_vals(table_data, &start_method_params,
-                        ARRAY_SIZE(start_method_params));
-
-    /* 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_addr_offset = table_data->len;
-    build_append_int_noprefix(table_data, 0, 8);
-    /* Log Area Start Address to be filled by Guest linker */
-    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
-                                   log_addr_offset, 8,
-                                   ACPI_BUILD_TPMLOG_FILE, 0);
-    build_header(linker, table_data,
-                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4, NULL, NULL);
-}
-
 #define HOLE_640K_START  (640 * KiB)
 #define HOLE_640K_END   (1 * MiB)
 
-- 
2.20.1

[PATCH v4 3/5] 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 2a1d9fc839..a0c4d71fe9 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1883,6 +1883,7 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
     uint8_t start_method_params[12] = {};
     unsigned log_addr_offset, tpm2_start;
     uint64_t control_area_start_address;
+    TPMIf *tpmif = tpm_find();
     uint32_t start_method;
     void *tpm2_ptr;
 
@@ -1893,10 +1894,10 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
     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)) {
         control_area_start_address = 0;
         start_method = TPM2_START_METHOD_MMIO;
-    } else if (TPM_IS_CRB(tpm_find())) {
+    } else if (TPM_IS_CRB(tpmif)) {
         control_area_start_address = TPM_CRB_ADDR_CTRL;
         start_method = TPM2_START_METHOD_CRB;
     } 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 v4 4/5] 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>

---

v3 -> v4:
- check the presence of the tpm in acpi_dsdt_add_tpm
  as it was done in v2

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 | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 6d152ab481..b7277233b3 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,38 @@ static void acpi_dsdt_add_power_button(Aml *scope)
     aml_append(scope, dev);
 }
 
+static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
+{
+    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
+    hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
+    SysBusDevice *sbdev = SYS_BUS_DEVICE(tpm_find());
+    MemoryRegion *sbdev_mr;
+    hwaddr tpm_base;
+
+    if (!sbdev) {
+        return;
+    }
+
+    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 +791,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

[PATCH v4 5/5] docs/specs/tpm: ACPI boot now supported for TPM/ARM

[Eric Auger]

ACPI boot now is supported. Let's remove the comment
saying it is not.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 docs/specs/tpm.rst | 2 --
 1 file changed, 2 deletions(-)

diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
index 5e61238bc5..eeeb93730a 100644
--- a/docs/specs/tpm.rst
+++ b/docs/specs/tpm.rst
@@ -346,8 +346,6 @@ In case an Arm virt machine is emulated, use the following command line:
     -drive if=pflash,format=raw,file=flash0.img,readonly \
     -drive if=pflash,format=raw,file=flash1.img
 
-  On Arm, ACPI boot with TPM is not yet supported.
-
 In case SeaBIOS is used as firmware, it should show the TPM menu item
 after entering the menu with 'ESC'.
 
-- 
2.20.1

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

[Stefan Berger]

On 6/11/20 9:59 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>
>
> ---
>
> v3 -> v4:
> - Don't use Acpi20TPM2 *tpm2_ptr anymore
> - Use variables for control area start address and start method
> - Simplified arg values passed to bios_linker_loader_add_pointer
> - use g_assert_not_reached()
> ---
>   hw/i386/acpi-build.c | 49 +++++++++++++++++++++++++++++---------------
>   1 file changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b5669d6c65..f150d95ecc 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2298,35 +2298,52 @@ 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);
> -    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] = {};
> +    unsigned log_addr_offset, tpm2_start;
> +    uint64_t control_area_start_address;
> +    uint32_t start_method;
> +    void *tpm2_ptr;
>   
> -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> +    tpm2_start = table_data->len;
> +    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
> +
> +    /* 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);
> +        control_area_start_address = 0;
> +        start_method = TPM2_START_METHOD_MMIO;
>       } 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);
> +        control_area_start_address = TPM_CRB_ADDR_CTRL;
> +        start_method = TPM2_START_METHOD_CRB;
>       } else {
> -        g_warn_if_reached();
> +        g_assert_not_reached();
>       }
> +    /* Address of Control Area */
> +    build_append_int_noprefix(table_data, control_area_start_address, 8);
> +    /* Start Method */
> +    build_append_int_noprefix(table_data, start_method, 4);
>   
> -    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,
> +                        ARRAY_SIZE(start_method_params));
>   
> -    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);

Here you push data related to TPM2 table...


> +
> +    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);

... here you push log area memory ...


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


... here you push TPM2 table related data again. Is this right or did we 
just mess up the TPM 2 table?


> +    /* Log Area Start Address to be filled by Guest linker */
>       bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> -                                   log_addr_offset, log_addr_size,
> +                                   log_addr_offset, 8,
>                                      ACPI_BUILD_TPMLOG_FILE, 0);
>       build_header(linker, table_data,
> -                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> +                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4, NULL, NULL);
>   }
>   
>   #define HOLE_640K_START  (640 * KiB)

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

[Auger Eric]

Hi Stefan,

On 6/11/20 4:25 PM, Stefan Berger wrote:
> On 6/11/20 9:59 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>
>>
>> ---
>>
>> v3 -> v4:
>> - Don't use Acpi20TPM2 *tpm2_ptr anymore
>> - Use variables for control area start address and start method
>> - Simplified arg values passed to bios_linker_loader_add_pointer
>> - use g_assert_not_reached()
>> ---
>>   hw/i386/acpi-build.c | 49 +++++++++++++++++++++++++++++---------------
>>   1 file changed, 33 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index b5669d6c65..f150d95ecc 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2298,35 +2298,52 @@ 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);
>> -    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] = {};
>> +    unsigned log_addr_offset, tpm2_start;
>> +    uint64_t control_area_start_address;
>> +    uint32_t start_method;
>> +    void *tpm2_ptr;
>>   -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
>> +    tpm2_start = table_data->len;
>> +    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
>> +
>> +    /* 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);
>> +        control_area_start_address = 0;
>> +        start_method = TPM2_START_METHOD_MMIO;
>>       } 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);
>> +        control_area_start_address = TPM_CRB_ADDR_CTRL;
>> +        start_method = TPM2_START_METHOD_CRB;
>>       } else {
>> -        g_warn_if_reached();
>> +        g_assert_not_reached();
>>       }
>> +    /* Address of Control Area */
>> +    build_append_int_noprefix(table_data, control_area_start_address,
>> 8);
>> +    /* Start Method */
>> +    build_append_int_noprefix(table_data, start_method, 4);
>>   -    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,
>> +                        ARRAY_SIZE(start_method_params));
>>   -    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);
> 
> Here you push data related to TPM2 table...
> 
> 
>> +
>> +    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);

This was:
    tpm2_ptr->log_area_minimum_length =
        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
> 
> ... here you push log area memory ...
yes.
> 
> 
>>       bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE,
>> tcpalog, 1,
>>                                false);

after "acpi: tpm: Do not build TCPA table for TPM 2" we had:

1) acpi_data_push(tcpalog, le32_to_cpu(tpm2_ptr->log_area_minimum_length));
2) bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
                             false);

so 1) also pushed log area memory

>>   -    /* log area start address to be filled by Guest linker */
>> +    log_addr_offset = table_data->len;
>> +    build_append_int_noprefix(table_data, 0, 8);
> 
> 
> ... here you push TPM2 table related data again. Is this right or did we
> just mess up the TPM 2 table?
this is to increment the table_data->len according to the
log_addr_offset length. So yes it updates tpm2

The bios-table-tests passes so at least the tpm2 shall be identical.

Thanks

Eric
> 
> 
>> +    /* Log Area Start Address to be filled by Guest linker */
>>       bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> -                                   log_addr_offset, log_addr_size,
>> +                                   log_addr_offset, 8,
>>                                      ACPI_BUILD_TPMLOG_FILE, 0);
>>       build_header(linker, table_data,
>> -                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4,
>> NULL, NULL);
>> +                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4,
>> NULL, NULL);
>>   }
>>     #define HOLE_640K_START  (640 * KiB)
> 
> 
> 

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

[Auger Eric]

Hi Stefan,

On 6/11/20 4:25 PM, Stefan Berger wrote:
> On 6/11/20 9:59 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>
>>
>> ---
>>
>> v3 -> v4:
>> - Don't use Acpi20TPM2 *tpm2_ptr anymore
>> - Use variables for control area start address and start method
>> - Simplified arg values passed to bios_linker_loader_add_pointer
>> - use g_assert_not_reached()
>> ---
>>   hw/i386/acpi-build.c | 49 +++++++++++++++++++++++++++++---------------
>>   1 file changed, 33 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index b5669d6c65..f150d95ecc 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2298,35 +2298,52 @@ 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);
>> -    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] = {};
>> +    unsigned log_addr_offset, tpm2_start;
>> +    uint64_t control_area_start_address;
>> +    uint32_t start_method;
>> +    void *tpm2_ptr;
>>   -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
>> +    tpm2_start = table_data->len;
>> +    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
>> +
>> +    /* 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);
>> +        control_area_start_address = 0;
>> +        start_method = TPM2_START_METHOD_MMIO;
>>       } 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);
>> +        control_area_start_address = TPM_CRB_ADDR_CTRL;
>> +        start_method = TPM2_START_METHOD_CRB;
>>       } else {
>> -        g_warn_if_reached();
>> +        g_assert_not_reached();
>>       }
>> +    /* Address of Control Area */
>> +    build_append_int_noprefix(table_data, control_area_start_address,
>> 8);
>> +    /* Start Method */
>> +    build_append_int_noprefix(table_data, start_method, 4);
>>   -    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,
>> +                        ARRAY_SIZE(start_method_params));
>>   -    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);
> 
> Here you push data related to TPM2 table...
yes it was
    tpm2_ptr->log_area_minimum_length =
        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
altering tpm2
> 
> 
>> +
>> +    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
> 
> ... here you push log area memory ...
in "acpi: tpm: Do not build TCPA table for TPM 2" there is
acpi_data_push(tcpalog, le32_to_cpu(tpm2_ptr->log_area_minimum_length));
bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
                         false);

So to me this matches
> 
> 
>>       bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE,
>> tcpalog, 1,
>>                                false);
>>   -    /* log area start address to be filled by Guest linker */
>> +    log_addr_offset = table_data->len;
>> +    build_append_int_noprefix(table_data, 0, 8);
> 
> 
> ... here you push TPM2 table related data again. Is this right or did we
> just mess up the TPM 2 table?
here I increment the table_data->len to take into account the last
field, ie. log_addr_offset
So I think it is correct.
bios-tables-test pass so at least TPM2 ACPI tables shouldn't have veen
altered.

Thanks

Eric
> 
> 
>> +    /* Log Area Start Address to be filled by Guest linker */
>>       bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> -                                   log_addr_offset, log_addr_size,
>> +                                   log_addr_offset, 8,
>>                                      ACPI_BUILD_TPMLOG_FILE, 0);
>>       build_header(linker, table_data,
>> -                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4,
>> NULL, NULL);
>> +                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4,
>> NULL, NULL);
>>   }
>>     #define HOLE_640K_START  (640 * KiB)
> 
> 
> 

Re: [PATCH v4 2/5] acpi: Move build_tpm2() in the generic part

[Stefan Berger]

On 6/11/20 9:59 AM, Eric Auger wrote:
> 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>


Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>


> ---
>   include/hw/acpi/aml-build.h |  2 ++
>   hw/acpi/aml-build.c         | 51 +++++++++++++++++++++++++++++++++++++
>   hw/i386/acpi-build.c        | 51 -------------------------------------
>   3 files changed, 53 insertions(+), 51 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..2a1d9fc839 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,56 @@ build_hdr:
>                    "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
>   }
>   
> +void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
> +{
> +    uint8_t start_method_params[12] = {};
> +    unsigned log_addr_offset, tpm2_start;
> +    uint64_t control_area_start_address;
> +    uint32_t start_method;
> +    void *tpm2_ptr;
> +
> +    tpm2_start = table_data->len;
> +    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
> +
> +    /* 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())) {
> +        control_area_start_address = 0;
> +        start_method = TPM2_START_METHOD_MMIO;
> +    } else if (TPM_IS_CRB(tpm_find())) {
> +        control_area_start_address = TPM_CRB_ADDR_CTRL;
> +        start_method = TPM2_START_METHOD_CRB;
> +    } else {
> +        g_assert_not_reached();
> +    }
> +    /* Address of Control Area */
> +    build_append_int_noprefix(table_data, control_area_start_address, 8);
> +    /* Start Method */
> +    build_append_int_noprefix(table_data, start_method, 4);
> +
> +    /* Platform Specific Parameters */
> +    g_array_append_vals(table_data, &start_method_params,
> +                        ARRAY_SIZE(start_method_params));
> +
> +    /* 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_addr_offset = table_data->len;
> +    build_append_int_noprefix(table_data, 0, 8);
> +    /* Log Area Start Address to be filled by Guest linker */
> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> +                                   log_addr_offset, 8,
> +                                   ACPI_BUILD_TPMLOG_FILE, 0);
> +    build_header(linker, table_data,
> +                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 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 f150d95ecc..b7c7583b5f 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2295,57 +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)
> -{
> -    uint8_t start_method_params[12] = {};
> -    unsigned log_addr_offset, tpm2_start;
> -    uint64_t control_area_start_address;
> -    uint32_t start_method;
> -    void *tpm2_ptr;
> -
> -    tpm2_start = table_data->len;
> -    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
> -
> -    /* 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())) {
> -        control_area_start_address = 0;
> -        start_method = TPM2_START_METHOD_MMIO;
> -    } else if (TPM_IS_CRB(tpm_find())) {
> -        control_area_start_address = TPM_CRB_ADDR_CTRL;
> -        start_method = TPM2_START_METHOD_CRB;
> -    } else {
> -        g_assert_not_reached();
> -    }
> -    /* Address of Control Area */
> -    build_append_int_noprefix(table_data, control_area_start_address, 8);
> -    /* Start Method */
> -    build_append_int_noprefix(table_data, start_method, 4);
> -
> -    /* Platform Specific Parameters */
> -    g_array_append_vals(table_data, &start_method_params,
> -                        ARRAY_SIZE(start_method_params));
> -
> -    /* 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_addr_offset = table_data->len;
> -    build_append_int_noprefix(table_data, 0, 8);
> -    /* Log Area Start Address to be filled by Guest linker */
> -    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> -                                   log_addr_offset, 8,
> -                                   ACPI_BUILD_TPMLOG_FILE, 0);
> -    build_header(linker, table_data,
> -                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4, NULL, NULL);
> -}
> -
>   #define HOLE_640K_START  (640 * KiB)
>   #define HOLE_640K_END   (1 * MiB)
>   

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

[Stefan Berger]

On 6/11/20 9:59 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>

>
> ---
>
> v3 -> v4:
> - Don't use Acpi20TPM2 *tpm2_ptr anymore
> - Use variables for control area start address and start method
> - Simplified arg values passed to bios_linker_loader_add_pointer
> - use g_assert_not_reached()
> ---
>   hw/i386/acpi-build.c | 49 +++++++++++++++++++++++++++++---------------
>   1 file changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b5669d6c65..f150d95ecc 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2298,35 +2298,52 @@ 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);
> -    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] = {};
> +    unsigned log_addr_offset, tpm2_start;
> +    uint64_t control_area_start_address;
> +    uint32_t start_method;
> +    void *tpm2_ptr;
>   
> -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> +    tpm2_start = table_data->len;
> +    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
> +
> +    /* 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);
> +        control_area_start_address = 0;
> +        start_method = TPM2_START_METHOD_MMIO;
>       } 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);
> +        control_area_start_address = TPM_CRB_ADDR_CTRL;
> +        start_method = TPM2_START_METHOD_CRB;
>       } else {
> -        g_warn_if_reached();
> +        g_assert_not_reached();
>       }
> +    /* Address of Control Area */
> +    build_append_int_noprefix(table_data, control_area_start_address, 8);
> +    /* Start Method */
> +    build_append_int_noprefix(table_data, start_method, 4);
>   
> -    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,
> +                        ARRAY_SIZE(start_method_params));
>   
> -    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 */
> +    log_addr_offset = table_data->len;
> +    build_append_int_noprefix(table_data, 0, 8);
> +    /* Log Area Start Address to be filled by Guest linker */
>       bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> -                                   log_addr_offset, log_addr_size,
> +                                   log_addr_offset, 8,


8 => sizeof(tpm2_ptr->log_area_start_address);


>                                      ACPI_BUILD_TPMLOG_FILE, 0);
>       build_header(linker, table_data,
> -                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> +                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4, NULL, NULL);
>   }
>   
>   #define HOLE_640K_START  (640 * KiB)


Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

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

[Auger Eric]

Hi Stefan,

On 6/11/20 5:19 PM, Stefan Berger wrote:
> On 6/11/20 9:59 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>
> 
>>
>> ---
>>
>> v3 -> v4:
>> - Don't use Acpi20TPM2 *tpm2_ptr anymore
>> - Use variables for control area start address and start method
>> - Simplified arg values passed to bios_linker_loader_add_pointer
>> - use g_assert_not_reached()
>> ---
>>   hw/i386/acpi-build.c | 49 +++++++++++++++++++++++++++++---------------
>>   1 file changed, 33 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index b5669d6c65..f150d95ecc 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2298,35 +2298,52 @@ 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);
>> -    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] = {};
>> +    unsigned log_addr_offset, tpm2_start;
>> +    uint64_t control_area_start_address;
>> +    uint32_t start_method;
>> +    void *tpm2_ptr;
>>   -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
>> +    tpm2_start = table_data->len;
>> +    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
>> +
>> +    /* 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);
>> +        control_area_start_address = 0;
>> +        start_method = TPM2_START_METHOD_MMIO;
>>       } 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);
>> +        control_area_start_address = TPM_CRB_ADDR_CTRL;
>> +        start_method = TPM2_START_METHOD_CRB;
>>       } else {
>> -        g_warn_if_reached();
>> +        g_assert_not_reached();
>>       }
>> +    /* Address of Control Area */
>> +    build_append_int_noprefix(table_data, control_area_start_address,
>> 8);
>> +    /* Start Method */
>> +    build_append_int_noprefix(table_data, start_method, 4);
>>   -    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,
>> +                        ARRAY_SIZE(start_method_params));
>>   -    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 */
>> +    log_addr_offset = table_data->len;
>> +    build_append_int_noprefix(table_data, 0, 8);
>> +    /* Log Area Start Address to be filled by Guest linker */
>>       bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> -                                   log_addr_offset, log_addr_size,
>> +                                   log_addr_offset, 8,
> 
> 
> 8 => sizeof(tpm2_ptr->log_area_start_address);
Igor asked for sizeof(tpm2_ptr->log_area_start_address) => 8 ;-)

Also he asked for Acpi20TPM2 removal so tpm2_ptr now is a void *.

I don't have a strong opinion on that so I will follow the consensus, if
any.


> 
> 
>>                                      ACPI_BUILD_TPMLOG_FILE, 0);
>>       build_header(linker, table_data,
>> -                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4,
>> NULL, NULL);
>> +                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4,
>> NULL, NULL);
>>   }
>>     #define HOLE_640K_START  (640 * KiB)
> 
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
thanks!

Thanks

Eric
> 
> 
> 
> 

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

[Igor Mammedov]

On Thu, 11 Jun 2020 10:25:38 -0400
Stefan Berger <stefanb@linux.ibm.com> wrote:

> On 6/11/20 9:59 AM, Eric Auger wrote:
[...]
> > -    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,
> > +                        ARRAY_SIZE(start_method_params));
> >   
> > -    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);  
> 
> Here you push data related to TPM2 table...
> 
> 
> > +
> > +    acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);  
> 
> ... here you push log area memory ...
> 
> 
> >       bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
> >                                false);
> >   
> > -    /* log area start address to be filled by Guest linker */
> > +    log_addr_offset = table_data->len;
> > +    build_append_int_noprefix(table_data, 0, 8);  
> 
> 
> ... here you push TPM2 table related data again. Is this right or did we 
> just mess up the TPM 2 table?

it's 2 differnt blobs tcpalog and table_data

> 
> 
> > +    /* Log Area Start Address to be filled by Guest linker */
> >       bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> > -                                   log_addr_offset, log_addr_size,
> > +                                   log_addr_offset, 8,
> >                                      ACPI_BUILD_TPMLOG_FILE, 0);
> >       build_header(linker, table_data,
> > -                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> > +                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4, NULL, NULL);
> >   }
> >   
> >   #define HOLE_640K_START  (640 * KiB)  
> 
> 

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

[Igor Mammedov]

On Thu, 11 Jun 2020 15:59:13 +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>
> 
> ---
> 
> v3 -> v4:
> - Don't use Acpi20TPM2 *tpm2_ptr anymore
> - Use variables for control area start address and start method
> - Simplified arg values passed to bios_linker_loader_add_pointer
> - use g_assert_not_reached()
> ---
>  hw/i386/acpi-build.c | 49 +++++++++++++++++++++++++++++---------------
>  1 file changed, 33 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b5669d6c65..f150d95ecc 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2298,35 +2298,52 @@ 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);
> -    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] = {};
> +    unsigned log_addr_offset, tpm2_start;
> +    uint64_t control_area_start_address;
> +    uint32_t start_method;
> +    void *tpm2_ptr;
>  
> -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> +    tpm2_start = table_data->len;
> +    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
> +
> +    /* 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);
> +        control_area_start_address = 0;
> +        start_method = TPM2_START_METHOD_MMIO;
>      } 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);
> +        control_area_start_address = TPM_CRB_ADDR_CTRL;
> +        start_method = TPM2_START_METHOD_CRB;
>      } else {
> -        g_warn_if_reached();
> +        g_assert_not_reached();
>      }
> +    /* Address of Control Area */
> +    build_append_int_noprefix(table_data, control_area_start_address, 8);
> +    /* Start Method */
> +    build_append_int_noprefix(table_data, start_method, 4);
>  
> -    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,
> +                        ARRAY_SIZE(start_method_params));
>  
> -    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);

question not related to conversion:
Is it a part of 'Platform Specific Parameters'?
(as per spec table ends with it. if yes, then probably add pointer to place in spec
wher its documented.

> +
> +    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 */
> +    log_addr_offset = table_data->len;
> +    build_append_int_noprefix(table_data, 0, 8);
> +    /* Log Area Start Address to be filled by Guest linker */
move this line to where it used to be or at least above build_append_int_noprefix()

>      bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> -                                   log_addr_offset, log_addr_size,
> +                                   log_addr_offset, 8,
>                                     ACPI_BUILD_TPMLOG_FILE, 0);
>      build_header(linker, table_data,
> -                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> +                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4, NULL, NULL);
>  }
>  
>  #define HOLE_640K_START  (640 * KiB)

nevertheless looks like faithfull conversion,
btw why you didn't drop Acpi20TPM2 structure definition?

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

[Auger Eric]

Hi Igor,
On 6/16/20 2:33 PM, Igor Mammedov wrote:
> On Thu, 11 Jun 2020 15:59:13 +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>
>>
>> ---
>>
>> v3 -> v4:
>> - Don't use Acpi20TPM2 *tpm2_ptr anymore
>> - Use variables for control area start address and start method
>> - Simplified arg values passed to bios_linker_loader_add_pointer
>> - use g_assert_not_reached()
>> ---
>>  hw/i386/acpi-build.c | 49 +++++++++++++++++++++++++++++---------------
>>  1 file changed, 33 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index b5669d6c65..f150d95ecc 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2298,35 +2298,52 @@ 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);
>> -    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] = {};
>> +    unsigned log_addr_offset, tpm2_start;
>> +    uint64_t control_area_start_address;
>> +    uint32_t start_method;
>> +    void *tpm2_ptr;
>>  
>> -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
>> +    tpm2_start = table_data->len;
>> +    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
>> +
>> +    /* 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);
>> +        control_area_start_address = 0;
>> +        start_method = TPM2_START_METHOD_MMIO;
>>      } 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);
>> +        control_area_start_address = TPM_CRB_ADDR_CTRL;
>> +        start_method = TPM2_START_METHOD_CRB;
>>      } else {
>> -        g_warn_if_reached();
>> +        g_assert_not_reached();
>>      }
>> +    /* Address of Control Area */
>> +    build_append_int_noprefix(table_data, control_area_start_address, 8);
>> +    /* Start Method */
>> +    build_append_int_noprefix(table_data, start_method, 4);
>>  
>> -    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,
>> +                        ARRAY_SIZE(start_method_params));
>>  
>> -    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);
> 
> question not related to conversion:
> Is it a part of 'Platform Specific Parameters'?
no I don't think so.
> (as per spec table ends with it. if yes, then probably add pointer to place in spec
> wher its documented.
Actually I failed to identify the place in the documentation. I looked
at the Acpi20TPM2 struct instead :-)
> 
>> +
>> +    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 */
>> +    log_addr_offset = table_data->len;
>> +    build_append_int_noprefix(table_data, 0, 8);
>> +    /* Log Area Start Address to be filled by Guest linker */
> move this line to where it used to be or at least above build_append_int_noprefix()
ok
> 
>>      bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> -                                   log_addr_offset, log_addr_size,
>> +                                   log_addr_offset, 8,
>>                                     ACPI_BUILD_TPMLOG_FILE, 0);
>>      build_header(linker, table_data,
>> -                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
>> +                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4, NULL, NULL);
>>  }
>>  
>>  #define HOLE_640K_START  (640 * KiB)
> 
> nevertheless looks like faithfull conversion,
> btw why you didn't drop Acpi20TPM2 structure definition?
I did not look if it were used elsewhere and also it reflects a state of
the spec. As I mentioned above, I was not able to find some info in the
spec.

Please note that the conversion was already pulled upstream but Michael
took an ancient version of this patch. So I sent "[PATCH v5 0/3]
vTPM/aarch64 ACPI support" this morning to fix the situation.

Thanks

Eric
> 
> 

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

[Stefan Berger]

On 6/16/20 8:33 AM, Igor Mammedov wrote:
>
> nevertheless looks like faithfull conversion,
> btw why you didn't drop Acpi20TPM2 structure definition?
>
If we get rid of the table we should keep a reference to this document, 
table 7: "TCG ACPI Specification; Family 1.2 and 2.0; Level 00 Revision 
00.37, December 19, 2014"

https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_1-10_0-37-Published.pdf

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

[Auger Eric]

Hi Stefan, Igor,

On 6/16/20 4:11 PM, Stefan Berger wrote:
> On 6/16/20 8:33 AM, Igor Mammedov wrote:
>>
>> nevertheless looks like faithfull conversion,
>> btw why you didn't drop Acpi20TPM2 structure definition?
>>
> If we get rid of the table we should keep a reference to this document,
> table 7: "TCG ACPI Specification; Family 1.2 and 2.0; Level 00 Revision
> 00.37, December 19, 2014"
> 
> https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_1-10_0-37-Published.pdf
> 
> 
> 
Further looking at this spec, the log_area_minimum_length and
log_area_start_address only are described in
- Table 2 (TCG Hardware InterfaceDescription Table Format for TPM 1.2
Clients)
- Table 4 (TCG Hardware Interface Description Table Format for TPM 1.2
Servers)
but not in Table 7, ie. not for TPM 2.0.

Are they really needed for TPM2 or what do I miss?

Thanks

Eric

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

[Laszlo Ersek]

On 06/18/20 09:50, Auger Eric wrote:
> Hi Stefan, Igor,
> 
> On 6/16/20 4:11 PM, Stefan Berger wrote:
>> On 6/16/20 8:33 AM, Igor Mammedov wrote:
>>>
>>> nevertheless looks like faithfull conversion,
>>> btw why you didn't drop Acpi20TPM2 structure definition?
>>>
>> If we get rid of the table we should keep a reference to this document,
>> table 7: "TCG ACPI Specification; Family 1.2 and 2.0; Level 00 Revision
>> 00.37, December 19, 2014"
>>
>> https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_1-10_0-37-Published.pdf
>>
>>
>>
> Further looking at this spec, the log_area_minimum_length and
> log_area_start_address only are described in
> - Table 2 (TCG Hardware InterfaceDescription Table Format for TPM 1.2
> Clients)
> - Table 4 (TCG Hardware Interface Description Table Format for TPM 1.2
> Servers)
> but not in Table 7, ie. not for TPM 2.0.
> 
> Are they really needed for TPM2 or what do I miss?

(side comment:

LASA and LAML are optional with TPM-2.0. From the discussion at
<https://bugzilla.tianocore.org/show_bug.cgi?id=978>.

)

Thanks
Laszlo

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

[Auger Eric]

Hi Laszlo,

On 6/19/20 11:38 AM, Laszlo Ersek wrote:
> On 06/18/20 09:50, Auger Eric wrote:
>> Hi Stefan, Igor,
>>
>> On 6/16/20 4:11 PM, Stefan Berger wrote:
>>> On 6/16/20 8:33 AM, Igor Mammedov wrote:
>>>>
>>>> nevertheless looks like faithfull conversion,
>>>> btw why you didn't drop Acpi20TPM2 structure definition?
>>>>
>>> If we get rid of the table we should keep a reference to this document,
>>> table 7: "TCG ACPI Specification; Family 1.2 and 2.0; Level 00 Revision
>>> 00.37, December 19, 2014"
>>>
>>> https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_1-10_0-37-Published.pdf
>>>
>>>
>>>
>> Further looking at this spec, the log_area_minimum_length and
>> log_area_start_address only are described in
>> - Table 2 (TCG Hardware InterfaceDescription Table Format for TPM 1.2
>> Clients)
>> - Table 4 (TCG Hardware Interface Description Table Format for TPM 1.2
>> Servers)
>> but not in Table 7, ie. not for TPM 2.0.
>>
>> Are they really needed for TPM2 or what do I miss?
> 
> (side comment:
> 
> LASA and LAML are optional with TPM-2.0. From the discussion at
> <https://bugzilla.tianocore.org/show_bug.cgi?id=978>.

Thank you for the pointer and info. I failed to find this info in the
spec. Given the risk of confusion, I would personally keep struct
Acpi20TPM2 and maybe add a comment. Stefan?

Thanks

Eric
> 
> )
> 
> Thanks
> Laszlo
> 

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

[Stefan Berger]

On 6/19/20 5:43 AM, Auger Eric wrote:
> Hi Laszlo,
>
> On 6/19/20 11:38 AM, Laszlo Ersek wrote:
>> On 06/18/20 09:50, Auger Eric wrote:
>>> Hi Stefan, Igor,
>>>
>>> On 6/16/20 4:11 PM, Stefan Berger wrote:
>>>> On 6/16/20 8:33 AM, Igor Mammedov wrote:
>>>>> nevertheless looks like faithfull conversion,
>>>>> btw why you didn't drop Acpi20TPM2 structure definition?
>>>>>
>>>> If we get rid of the table we should keep a reference to this document,
>>>> table 7: "TCG ACPI Specification; Family 1.2 and 2.0; Level 00 Revision
>>>> 00.37, December 19, 2014"
>>>>
>>>> https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_1-10_0-37-Published.pdf
>>>>
>>>>
>>>>
>>> Further looking at this spec, the log_area_minimum_length and
>>> log_area_start_address only are described in
>>> - Table 2 (TCG Hardware InterfaceDescription Table Format for TPM 1.2
>>> Clients)
>>> - Table 4 (TCG Hardware Interface Description Table Format for TPM 1.2
>>> Servers)
>>> but not in Table 7, ie. not for TPM 2.0.
>>>
>>> Are they really needed for TPM2 or what do I miss?
>> (side comment:
>>
>> LASA and LAML are optional with TPM-2.0. From the discussion at
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=978>.


They are needed for (x86) BIOS, such as SeaBIOS, not for UEFI, though. I 
do not know about ARM.


> Thank you for the pointer and info. I failed to find this info in the
> spec. Given the risk of confusion, I would personally keep struct
> Acpi20TPM2 and maybe add a comment. Stefan?

Either way is fine with me for as long as we know where to find the 
layout of the structure.

   Stefan

>
> Thanks
>
> Eric
>> )
>>
>> Thanks
>> Laszlo
>>

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

[Igor Mammedov]

On Fri, 19 Jun 2020 07:19:51 -0400
Stefan Berger <stefanb@linux.ibm.com> wrote:

> On 6/19/20 5:43 AM, Auger Eric wrote:
> > Hi Laszlo,
> >
> > On 6/19/20 11:38 AM, Laszlo Ersek wrote:  
> >> On 06/18/20 09:50, Auger Eric wrote:  
> >>> Hi Stefan, Igor,
> >>>
> >>> On 6/16/20 4:11 PM, Stefan Berger wrote:  
> >>>> On 6/16/20 8:33 AM, Igor Mammedov wrote:  
> >>>>> nevertheless looks like faithfull conversion,
> >>>>> btw why you didn't drop Acpi20TPM2 structure definition?
> >>>>>  
> >>>> If we get rid of the table we should keep a reference to this document,
> >>>> table 7: "TCG ACPI Specification; Family 1.2 and 2.0; Level 00 Revision
> >>>> 00.37, December 19, 2014"
> >>>>
> >>>> https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_1-10_0-37-Published.pdf
> >>>>
> >>>>
> >>>>  
> >>> Further looking at this spec, the log_area_minimum_length and
> >>> log_area_start_address only are described in
> >>> - Table 2 (TCG Hardware InterfaceDescription Table Format for TPM 1.2
> >>> Clients)
> >>> - Table 4 (TCG Hardware Interface Description Table Format for TPM 1.2
> >>> Servers)
> >>> but not in Table 7, ie. not for TPM 2.0.
> >>>
> >>> Are they really needed for TPM2 or what do I miss?  
> >> (side comment:
> >>
> >> LASA and LAML are optional with TPM-2.0. From the discussion at
> >> <https://bugzilla.tianocore.org/show_bug.cgi?id=978>.  
> 
> 
> They are needed for (x86) BIOS, such as SeaBIOS, not for UEFI, though. I 
> do not know about ARM.
> 
> 
> > Thank you for the pointer and info. I failed to find this info in the
> > spec. Given the risk of confusion, I would personally keep struct
> > Acpi20TPM2 and maybe add a comment. Stefan?  
> 
> Either way is fine with me for as long as we know where to find the 
> layout of the structure.
I'd remove Acpi20TPM2 as it hardly documents anything, and add a comment
pointing to the concrete spec that has these fields.

TCGTCG ACPI SpecificationFamily “1.2” and “2.0”Version 1.2,Revision 8

> 
>    Stefan
> 
> >
> > Thanks
> >
> > Eric  
> >> )
> >>
> >> Thanks
> >> Laszlo
> >>  
> 

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

[Auger Eric]

Hi Igor,

On 6/22/20 11:39 AM, Igor Mammedov wrote:
> On Fri, 19 Jun 2020 07:19:51 -0400
> Stefan Berger <stefanb@linux.ibm.com> wrote:
> 
>> On 6/19/20 5:43 AM, Auger Eric wrote:
>>> Hi Laszlo,
>>>
>>> On 6/19/20 11:38 AM, Laszlo Ersek wrote:  
>>>> On 06/18/20 09:50, Auger Eric wrote:  
>>>>> Hi Stefan, Igor,
>>>>>
>>>>> On 6/16/20 4:11 PM, Stefan Berger wrote:  
>>>>>> On 6/16/20 8:33 AM, Igor Mammedov wrote:  
>>>>>>> nevertheless looks like faithfull conversion,
>>>>>>> btw why you didn't drop Acpi20TPM2 structure definition?
>>>>>>>  
>>>>>> If we get rid of the table we should keep a reference to this document,
>>>>>> table 7: "TCG ACPI Specification; Family 1.2 and 2.0; Level 00 Revision
>>>>>> 00.37, December 19, 2014"
>>>>>>
>>>>>> https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_1-10_0-37-Published.pdf
>>>>>>
>>>>>>
>>>>>>  
>>>>> Further looking at this spec, the log_area_minimum_length and
>>>>> log_area_start_address only are described in
>>>>> - Table 2 (TCG Hardware InterfaceDescription Table Format for TPM 1.2
>>>>> Clients)
>>>>> - Table 4 (TCG Hardware Interface Description Table Format for TPM 1.2
>>>>> Servers)
>>>>> but not in Table 7, ie. not for TPM 2.0.
>>>>>
>>>>> Are they really needed for TPM2 or what do I miss?  
>>>> (side comment:
>>>>
>>>> LASA and LAML are optional with TPM-2.0. From the discussion at
>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=978>.  
>>
>>
>> They are needed for (x86) BIOS, such as SeaBIOS, not for UEFI, though. I 
>> do not know about ARM.
>>
>>
>>> Thank you for the pointer and info. I failed to find this info in the
>>> spec. Given the risk of confusion, I would personally keep struct
>>> Acpi20TPM2 and maybe add a comment. Stefan?  
>>
>> Either way is fine with me for as long as we know where to find the 
>> layout of the structure.
> I'd remove Acpi20TPM2 as it hardly documents anything, and add a comment
> pointing to the concrete spec that has these fields.
> 
> TCGTCG ACPI SpecificationFamily “1.2” and “2.0”Version 1.2,Revision 8

[PATCH v6 0/3] vTPM/aarch64 ACPI support was posted.

As documented in the cover letter (history log), the presence of the
LAML and LASA fields in the TPM2 table is not clearly documented in the
spec (at least I failed to find it). It is for TPM 1.2. On the other
hand, Stefan said it is mandated for some x86 BIOS to work. Given this
weirdness I think keeping the  Acpi20TPM2 struct is not too bad. See v6 ...

Thanks

Eric
> 
>>
>>    Stefan
>>
>>>
>>> Thanks
>>>
>>> Eric  
>>>> )
>>>>
>>>> Thanks
>>>> Laszlo
>>>>  
>>
> 
> 

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

[Igor Mammedov]

On Mon, 22 Jun 2020 11:47:26 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Igor,
> 
> On 6/22/20 11:39 AM, Igor Mammedov wrote:
> > On Fri, 19 Jun 2020 07:19:51 -0400
> > Stefan Berger <stefanb@linux.ibm.com> wrote:
> >   
> >> On 6/19/20 5:43 AM, Auger Eric wrote:  
> >>> Hi Laszlo,
> >>>
> >>> On 6/19/20 11:38 AM, Laszlo Ersek wrote:    
> >>>> On 06/18/20 09:50, Auger Eric wrote:    
> >>>>> Hi Stefan, Igor,
> >>>>>
> >>>>> On 6/16/20 4:11 PM, Stefan Berger wrote:    
> >>>>>> On 6/16/20 8:33 AM, Igor Mammedov wrote:    
> >>>>>>> nevertheless looks like faithfull conversion,
> >>>>>>> btw why you didn't drop Acpi20TPM2 structure definition?
> >>>>>>>    
> >>>>>> If we get rid of the table we should keep a reference to this document,
> >>>>>> table 7: "TCG ACPI Specification; Family 1.2 and 2.0; Level 00 Revision
> >>>>>> 00.37, December 19, 2014"
> >>>>>>
> >>>>>> https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_1-10_0-37-Published.pdf
> >>>>>>
> >>>>>>
> >>>>>>    
> >>>>> Further looking at this spec, the log_area_minimum_length and
> >>>>> log_area_start_address only are described in
> >>>>> - Table 2 (TCG Hardware InterfaceDescription Table Format for TPM 1.2
> >>>>> Clients)
> >>>>> - Table 4 (TCG Hardware Interface Description Table Format for TPM 1.2
> >>>>> Servers)
> >>>>> but not in Table 7, ie. not for TPM 2.0.
> >>>>>
> >>>>> Are they really needed for TPM2 or what do I miss?    
> >>>> (side comment:
> >>>>
> >>>> LASA and LAML are optional with TPM-2.0. From the discussion at
> >>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=978>.    
> >>
> >>
> >> They are needed for (x86) BIOS, such as SeaBIOS, not for UEFI, though. I 
> >> do not know about ARM.
> >>
> >>  
> >>> Thank you for the pointer and info. I failed to find this info in the
> >>> spec. Given the risk of confusion, I would personally keep struct
> >>> Acpi20TPM2 and maybe add a comment. Stefan?    
> >>
> >> Either way is fine with me for as long as we know where to find the 
> >> layout of the structure.  
> > I'd remove Acpi20TPM2 as it hardly documents anything, and add a comment
> > pointing to the concrete spec that has these fields.
> > 
> > TCGTCG ACPI SpecificationFamily “1.2” and “2.0”Version 1.2,Revision 8  
> 
> [PATCH v6 0/3] vTPM/aarch64 ACPI support was posted.
> 
> As documented in the cover letter (history log), the presence of the
> LAML and LASA fields in the TPM2 table is not clearly documented in the
> spec (at least I failed to find it). It is for TPM 1.2. On the other
> hand, Stefan said it is mandated for some x86 BIOS to work. Given this
> weirdness I think keeping the  Acpi20TPM2 struct is not too bad. See v6 ...

Laszlo pointed to spec version where LAML/LASA in TPM2 are documented,
so I'd just use that as a spec this code is based on.

PS:
Acpi20TPM2 struct doesn't document anything, it's just another way to do
the same thing as build_appen_* calls do. Having it just adds to confusion. 

> 
> Thanks
> 
> Eric
> >   
> >>
> >>    Stefan
> >>  
> >>>
> >>> Thanks
> >>>
> >>> Eric    
> >>>> )
> >>>>
> >>>> Thanks
> >>>> Laszlo
> >>>>    
> >>  
> > 
> >   

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

[Auger Eric]

Hi Igor,

On 6/22/20 2:14 PM, Igor Mammedov wrote:
> On Mon, 22 Jun 2020 11:47:26 +0200
> Auger Eric <eric.auger@redhat.com> wrote:
> 
>> Hi Igor,
>>
>> On 6/22/20 11:39 AM, Igor Mammedov wrote:
>>> On Fri, 19 Jun 2020 07:19:51 -0400
>>> Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>   
>>>> On 6/19/20 5:43 AM, Auger Eric wrote:  
>>>>> Hi Laszlo,
>>>>>
>>>>> On 6/19/20 11:38 AM, Laszlo Ersek wrote:    
>>>>>> On 06/18/20 09:50, Auger Eric wrote:    
>>>>>>> Hi Stefan, Igor,
>>>>>>>
>>>>>>> On 6/16/20 4:11 PM, Stefan Berger wrote:    
>>>>>>>> On 6/16/20 8:33 AM, Igor Mammedov wrote:    
>>>>>>>>> nevertheless looks like faithfull conversion,
>>>>>>>>> btw why you didn't drop Acpi20TPM2 structure definition?
>>>>>>>>>    
>>>>>>>> If we get rid of the table we should keep a reference to this document,
>>>>>>>> table 7: "TCG ACPI Specification; Family 1.2 and 2.0; Level 00 Revision
>>>>>>>> 00.37, December 19, 2014"
>>>>>>>>
>>>>>>>> https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_1-10_0-37-Published.pdf
>>>>>>>>
>>>>>>>>
>>>>>>>>    
>>>>>>> Further looking at this spec, the log_area_minimum_length and
>>>>>>> log_area_start_address only are described in
>>>>>>> - Table 2 (TCG Hardware InterfaceDescription Table Format for TPM 1.2
>>>>>>> Clients)
>>>>>>> - Table 4 (TCG Hardware Interface Description Table Format for TPM 1.2
>>>>>>> Servers)
>>>>>>> but not in Table 7, ie. not for TPM 2.0.
>>>>>>>
>>>>>>> Are they really needed for TPM2 or what do I miss?    
>>>>>> (side comment:
>>>>>>
>>>>>> LASA and LAML are optional with TPM-2.0. From the discussion at
>>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=978>.    
>>>>
>>>>
>>>> They are needed for (x86) BIOS, such as SeaBIOS, not for UEFI, though. I 
>>>> do not know about ARM.
>>>>
>>>>  
>>>>> Thank you for the pointer and info. I failed to find this info in the
>>>>> spec. Given the risk of confusion, I would personally keep struct
>>>>> Acpi20TPM2 and maybe add a comment. Stefan?    
>>>>
>>>> Either way is fine with me for as long as we know where to find the 
>>>> layout of the structure.  
>>> I'd remove Acpi20TPM2 as it hardly documents anything, and add a comment
>>> pointing to the concrete spec that has these fields.
>>>
>>> TCGTCG ACPI SpecificationFamily “1.2” and “2.0”Version 1.2,Revision 8  
>>
>> [PATCH v6 0/3] vTPM/aarch64 ACPI support was posted.
>>
>> As documented in the cover letter (history log), the presence of the
>> LAML and LASA fields in the TPM2 table is not clearly documented in the
>> spec (at least I failed to find it). It is for TPM 1.2. On the other
>> hand, Stefan said it is mandated for some x86 BIOS to work. Given this
>> weirdness I think keeping the  Acpi20TPM2 struct is not too bad. See v6 ...
> 
> Laszlo pointed to spec version where LAML/LASA in TPM2 are documented,
> so I'd just use that as a spec this code is based on.

OK I missed that, indeed in the version the 2 fields are documented.
https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf

in table 7:TCG Hardware Interface Description Table Format for TPM 2.0

I will use that ref and remove the Acpi20TPM2 struct then.

Thanks

Eric

> 
> PS:
> Acpi20TPM2 struct doesn't document anything, it's just another way to do
> the same thing as build_appen_* calls do. Having it just adds to confusion. 
> 
>>
>> Thanks
>>
>> Eric
>>>   
>>>>
>>>>    Stefan
>>>>  
>>>>>
>>>>> Thanks
>>>>>
>>>>> Eric    
>>>>>> )
>>>>>>
>>>>>> Thanks
>>>>>> Laszlo
>>>>>>    
>>>>  
>>>
>>>   
>