Remove any reference to Acpi20TPM2 and adopt an implementation
similar to build_ghes_v2().

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>

---

v5 -> v6:
- add reference to the spec + comment about LAML and LASA fields
- also moved LASA intro comment above build_append_int_noprefix()
  as requested by Igor
---
 hw/acpi/aml-build.c | 54 +++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 2cb7b991ef..1cc08a3eb9 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1878,48 +1878,64 @@ build_hdr:
                  "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
 }
 
+/*
+ * build_tpm2 - Build the TPM2 table as specified in table 7 of
+ * "TCG ACPI Specification; Family 1.2 and 2.0;
+ * Level 00 Revision 00.37, December 19, 2014"
+ * Note: the LASA and LAML fields are optional for TPM-2.0 (the above
+ * table does not mention them) but are needed at least for SeaBIOS.
+ * See the Acpi20TPM2 struct for the corresponding layout.
+ */
 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] = {};
+    unsigned log_addr_offset, tpm2_start;
+    uint64_t control_area_start_address;
     TPMIf *tpmif = tpm_find();
+    uint32_t start_method;
+    void *tpm2_ptr;
 
-    /* platform class */
+    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 */
+    /* Reserved */
     build_append_int_noprefix(table_data, 0, 2);
     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);
+        control_area_start_address = 0;
+        start_method = TPM2_START_METHOD_MMIO;
     } 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);
+        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);
 
-    /* platform specific parameters */
-    g_array_append_vals(table_data, &start_method_params, 12);
+    /* Platform Specific Parameters */
+    g_array_append_vals(table_data, &start_method_params,
+                        ARRAY_SIZE(start_method_params));
 
-    /* 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;
+
+    /* 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,
+                                   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);
 }
 
 /* ACPI 5.0: 6.4.3.8.2 Serial Bus Connection Descriptors */
-- 
2.20.1

On 6/19/20 10:18 AM, Eric Auger wrote:
> Remove any reference to Acpi20TPM2 and adopt an implementation
> similar to build_ghes_v2().
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>
> ---
>
> v5 -> v6:
> - add reference to the spec + comment about LAML and LASA fields
> - also moved LASA intro comment above build_append_int_noprefix()
>    as requested by Igor
> ---
>   hw/acpi/aml-build.c | 54 +++++++++++++++++++++++++++++----------------
>   1 file changed, 35 insertions(+), 19 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 2cb7b991ef..1cc08a3eb9 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1878,48 +1878,64 @@ build_hdr:
>                    "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
>   }
>   
> +/*
> + * build_tpm2 - Build the TPM2 table as specified in table 7 of
> + * "TCG ACPI Specification; Family 1.2 and 2.0;
> + * Level 00 Revision 00.37, December 19, 2014"
> + * Note: the LASA and LAML fields are optional for TPM-2.0 (the above
> + * table does not mention them) but are needed at least for SeaBIOS.
> + * See the Acpi20TPM2 struct for the corresponding layout.
> + */
>   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] = {};
> +    unsigned log_addr_offset, tpm2_start;
> +    uint64_t control_area_start_address;
>       TPMIf *tpmif = tpm_find();
> +    uint32_t start_method;
> +    void *tpm2_ptr;
>   
> -    /* platform class */
> +    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 */
> +    /* Reserved */
>       build_append_int_noprefix(table_data, 0, 2);
>       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);
> +        control_area_start_address = 0;
> +        start_method = TPM2_START_METHOD_MMIO;
>       } 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);
> +        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);
>   
> -    /* platform specific parameters */
> -    g_array_append_vals(table_data, &start_method_params, 12);
> +    /* Platform Specific Parameters */
> +    g_array_append_vals(table_data, &start_method_params,
> +                        ARRAY_SIZE(start_method_params));
>   
> -    /* 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;
> +

Don't have to subtract tpm2_start from it ?


> +    /* 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,
> +                                   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);
>   }
>   
>   /* ACPI 5.0: 6.4.3.8.2 Serial Bus Connection Descriptors */

Hi Stefan,

On 6/19/20 5:07 PM, Stefan Berger wrote:
> On 6/19/20 10:18 AM, Eric Auger wrote:
>> Remove any reference to Acpi20TPM2 and adopt an implementation
>> similar to build_ghes_v2().
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>>
>> ---
>>
>> v5 -> v6:
>> - add reference to the spec + comment about LAML and LASA fields
>> - also moved LASA intro comment above build_append_int_noprefix()
>>    as requested by Igor
>> ---
>>   hw/acpi/aml-build.c | 54 +++++++++++++++++++++++++++++----------------
>>   1 file changed, 35 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 2cb7b991ef..1cc08a3eb9 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1878,48 +1878,64 @@ build_hdr:
>>                    "FACP", tbl->len - fadt_start, f->rev, oem_id,
>> oem_table_id);
>>   }
>>   +/*
>> + * build_tpm2 - Build the TPM2 table as specified in table 7 of
>> + * "TCG ACPI Specification; Family 1.2 and 2.0;
>> + * Level 00 Revision 00.37, December 19, 2014"
>> + * Note: the LASA and LAML fields are optional for TPM-2.0 (the above
>> + * table does not mention them) but are needed at least for SeaBIOS.
>> + * See the Acpi20TPM2 struct for the corresponding layout.
>> + */
>>   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] = {};
>> +    unsigned log_addr_offset, tpm2_start;
>> +    uint64_t control_area_start_address;
>>       TPMIf *tpmif = tpm_find();
>> +    uint32_t start_method;
>> +    void *tpm2_ptr;
>>   -    /* platform class */
>> +    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 */
>> +    /* Reserved */
>>       build_append_int_noprefix(table_data, 0, 2);
>>       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);
>> +        control_area_start_address = 0;
>> +        start_method = TPM2_START_METHOD_MMIO;
>>       } 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);
>> +        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);
>>   -    /* platform specific parameters */
>> -    g_array_append_vals(table_data, &start_method_params, 12);
>> +    /* Platform Specific Parameters */
>> +    g_array_append_vals(table_data, &start_method_params,
>> +                        ARRAY_SIZE(start_method_params));
>>   -    /* 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;
>> +
> 
> Don't have to subtract tpm2_start from it ?
no that's the absolute offset. It was

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

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,
>> +                                   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);
>>   }
>>     /* ACPI 5.0: 6.4.3.8.2 Serial Bus Connection Descriptors */
> 
> 
> 

On Fri, 19 Jun 2020 16:18:49 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> Remove any reference to Acpi20TPM2 and adopt an implementation
> similar to build_ghes_v2().
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>

with struct Acpi20TPM2 removed completely and pointer to the spec as we discussed ealier

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> 
> ---
> 
> v5 -> v6:
> - add reference to the spec + comment about LAML and LASA fields
> - also moved LASA intro comment above build_append_int_noprefix()
>   as requested by Igor
> ---
>  hw/acpi/aml-build.c | 54 +++++++++++++++++++++++++++++----------------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 2cb7b991ef..1cc08a3eb9 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1878,48 +1878,64 @@ build_hdr:
>                   "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
>  }
>  
> +/*
> + * build_tpm2 - Build the TPM2 table as specified in table 7 of
> + * "TCG ACPI Specification; Family 1.2 and 2.0;
> + * Level 00 Revision 00.37, December 19, 2014"
> + * Note: the LASA and LAML fields are optional for TPM-2.0 (the above
> + * table does not mention them) but are needed at least for SeaBIOS.
> + * See the Acpi20TPM2 struct for the corresponding layout.
> + */
>  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] = {};
> +    unsigned log_addr_offset, tpm2_start;
> +    uint64_t control_area_start_address;
>      TPMIf *tpmif = tpm_find();
> +    uint32_t start_method;
> +    void *tpm2_ptr;
>  
> -    /* platform class */
> +    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 */
> +    /* Reserved */
>      build_append_int_noprefix(table_data, 0, 2);
>      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);
> +        control_area_start_address = 0;
> +        start_method = TPM2_START_METHOD_MMIO;
>      } 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);
> +        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);
>  
> -    /* platform specific parameters */
> -    g_array_append_vals(table_data, &start_method_params, 12);
> +    /* Platform Specific Parameters */
> +    g_array_append_vals(table_data, &start_method_params,
> +                        ARRAY_SIZE(start_method_params));
>  
> -    /* 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;
> +
> +    /* 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,
> +                                   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);
>  }
>  
>  /* ACPI 5.0: 6.4.3.8.2 Serial Bus Connection Descriptors */

On 6/19/20 10:18 AM, Eric Auger wrote:
> Remove any reference to Acpi20TPM2 and adopt an implementation
> similar to build_ghes_v2().
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>

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

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


>
> ---
>
> v5 -> v6:
> - add reference to the spec + comment about LAML and LASA fields
> - also moved LASA intro comment above build_append_int_noprefix()
>    as requested by Igor
> ---
>   hw/acpi/aml-build.c | 54 +++++++++++++++++++++++++++++----------------
>   1 file changed, 35 insertions(+), 19 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 2cb7b991ef..1cc08a3eb9 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1878,48 +1878,64 @@ build_hdr:
>                    "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
>   }
>   
> +/*
> + * build_tpm2 - Build the TPM2 table as specified in table 7 of
> + * "TCG ACPI Specification; Family 1.2 and 2.0;
> + * Level 00 Revision 00.37, December 19, 2014"
> + * Note: the LASA and LAML fields are optional for TPM-2.0 (the above
> + * table does not mention them) but are needed at least for SeaBIOS.
> + * See the Acpi20TPM2 struct for the corresponding layout.
> + */
>   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] = {};
> +    unsigned log_addr_offset, tpm2_start;
> +    uint64_t control_area_start_address;
>       TPMIf *tpmif = tpm_find();
> +    uint32_t start_method;
> +    void *tpm2_ptr;
>   
> -    /* platform class */
> +    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 */
> +    /* Reserved */
>       build_append_int_noprefix(table_data, 0, 2);
>       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);
> +        control_area_start_address = 0;
> +        start_method = TPM2_START_METHOD_MMIO;
>       } 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);
> +        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);
>   
> -    /* platform specific parameters */
> -    g_array_append_vals(table_data, &start_method_params, 12);
> +    /* Platform Specific Parameters */
> +    g_array_append_vals(table_data, &start_method_params,
> +                        ARRAY_SIZE(start_method_params));
>   
> -    /* 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;
> +
> +    /* 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,
> +                                   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);
>   }
>   
>   /* ACPI 5.0: 6.4.3.8.2 Serial Bus Connection Descriptors */

Hi Stefan,

On 6/19/20 6:09 PM, Stefan Berger wrote:
> On 6/19/20 10:18 AM, Eric Auger wrote:
>> Remove any reference to Acpi20TPM2 and adopt an implementation
>> similar to build_ghes_v2().
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> Tested-by: Stefan Berger <stefanb@linux.ibm.com>

Thanks!

Eric
> 
> 
>>
>> ---
>>
>> v5 -> v6:
>> - add reference to the spec + comment about LAML and LASA fields
>> - also moved LASA intro comment above build_append_int_noprefix()
>>    as requested by Igor
>> ---
>>   hw/acpi/aml-build.c | 54 +++++++++++++++++++++++++++++----------------
>>   1 file changed, 35 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 2cb7b991ef..1cc08a3eb9 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1878,48 +1878,64 @@ build_hdr:
>>                    "FACP", tbl->len - fadt_start, f->rev, oem_id,
>> oem_table_id);
>>   }
>>   +/*
>> + * build_tpm2 - Build the TPM2 table as specified in table 7 of
>> + * "TCG ACPI Specification; Family 1.2 and 2.0;
>> + * Level 00 Revision 00.37, December 19, 2014"
>> + * Note: the LASA and LAML fields are optional for TPM-2.0 (the above
>> + * table does not mention them) but are needed at least for SeaBIOS.
>> + * See the Acpi20TPM2 struct for the corresponding layout.
>> + */
>>   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] = {};
>> +    unsigned log_addr_offset, tpm2_start;
>> +    uint64_t control_area_start_address;
>>       TPMIf *tpmif = tpm_find();
>> +    uint32_t start_method;
>> +    void *tpm2_ptr;
>>   -    /* platform class */
>> +    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 */
>> +    /* Reserved */
>>       build_append_int_noprefix(table_data, 0, 2);
>>       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);
>> +        control_area_start_address = 0;
>> +        start_method = TPM2_START_METHOD_MMIO;
>>       } 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);
>> +        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);
>>   -    /* platform specific parameters */
>> -    g_array_append_vals(table_data, &start_method_params, 12);
>> +    /* Platform Specific Parameters */
>> +    g_array_append_vals(table_data, &start_method_params,
>> +                        ARRAY_SIZE(start_method_params));
>>   -    /* 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;
>> +
>> +    /* 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,
>> +                                   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);
>>   }
>>     /* ACPI 5.0: 6.4.3.8.2 Serial Bus Connection Descriptors */
> 
> 
>