[PATCH v2 0/3] vTPM/aarch64 ACPI support

[PATCH v2 0/3] vTPM/aarch64 ACPI support

[Eric Auger]

Those patches bring MMIO TPM TIS ACPI support in machvirt.
The first patch moves the TPM2 ACPI table generation code
in the generic code. Then the table is added if the TPM2
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.

Best Regards

Eric

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

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

Eric Auger (3):
  acpi: Move build_tpm2() in the generic part
  arm/acpi: TPM2 ACPI table support
  arm/acpi: Add the TPM2.0 device under the DSDT

 include/hw/acpi/aml-build.h |  2 ++
 include/sysemu/tpm.h        |  2 ++
 hw/acpi/aml-build.c         | 31 ++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 47 +++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c        | 30 -----------------------
 5 files changed, 82 insertions(+), 30 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] 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         | 30 ++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c        | 30 ------------------------------
 3 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 0f4ed53d7f..a67ab4618a 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 2c3702b882..1f7fd09112 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)
 {
@@ -1875,6 +1876,35 @@ build_hdr:
                  "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
 }
 
+void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
+{
+    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *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;
+
+    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
+    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);
+    } 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);
+    } else {
+        g_warn_if_reached();
+    }
+
+    tpm2_ptr->log_area_minimum_length =
+        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
+
+    /* 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,
+                                   ACPI_BUILD_TPMLOG_FILE, 0);
+    build_header(linker, table_data,
+                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
+}
+
 /* ACPI 5.0: 6.4.3.8.2 Serial Bus Connection Descriptors */
 static Aml *aml_serial_bus_device(uint8_t serial_bus_type, uint8_t flags,
                                   uint16_t type_flags,
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 23c77eeb95..c7c560e269 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2296,36 +2296,6 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
                  (void *)tcpa, "TCPA", sizeof(*tcpa), 2, NULL, NULL);
 }
 
-static void
-build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
-{
-    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *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;
-
-    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
-    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);
-    } 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);
-    } else {
-        g_warn_if_reached();
-    }
-
-    tpm2_ptr->log_area_minimum_length =
-        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
-
-    /* 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,
-                                   ACPI_BUILD_TPMLOG_FILE, 0);
-    build_header(linker, table_data,
-                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
-}
-
 #define HOLE_640K_START  (640 * KiB)
 #define HOLE_640K_END   (1 * MiB)
 
-- 
2.20.1

Re: [PATCH v2 1/3] acpi: Move build_tpm2() in the generic part

[Stefan Berger]

On 5/5/20 10:44 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         | 30 ++++++++++++++++++++++++++++++
>   hw/i386/acpi-build.c        | 30 ------------------------------
>   3 files changed, 32 insertions(+), 30 deletions(-)
>
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 0f4ed53d7f..a67ab4618a 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 2c3702b882..1f7fd09112 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)
>   {
> @@ -1875,6 +1876,35 @@ build_hdr:
>                    "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
>   }
>   
> +void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
> +{
> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *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;
> +
> +    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> +    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);
> +    } 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);
> +    } else {
> +        g_warn_if_reached();
> +    }
> +
> +    tpm2_ptr->log_area_minimum_length =
> +        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
> +
> +    /* 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,
> +                                   ACPI_BUILD_TPMLOG_FILE, 0);
> +    build_header(linker, table_data,
> +                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> +}
> +
>   /* ACPI 5.0: 6.4.3.8.2 Serial Bus Connection Descriptors */
>   static Aml *aml_serial_bus_device(uint8_t serial_bus_type, uint8_t flags,
>                                     uint16_t type_flags,
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 23c77eeb95..c7c560e269 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2296,36 +2296,6 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>                    (void *)tcpa, "TCPA", sizeof(*tcpa), 2, NULL, NULL);
>   }
>   
> -static void
> -build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
> -{
> -    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *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;
> -
> -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> -    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);
> -    } 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);
> -    } else {
> -        g_warn_if_reached();
> -    }
> -
> -    tpm2_ptr->log_area_minimum_length =
> -        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
> -
> -    /* 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,
> -                                   ACPI_BUILD_TPMLOG_FILE, 0);
> -    build_header(linker, table_data,
> -                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> -}
> -
>   #define HOLE_640K_START  (640 * KiB)
>   #define HOLE_640K_END   (1 * MiB)
>   

Re: [PATCH v2 1/3] acpi: Move build_tpm2() in the generic part

[Andrew Jones]

On Tue, May 05, 2020 at 04:44:17PM +0200, 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>
> ---
>  include/hw/acpi/aml-build.h |  2 ++
>  hw/acpi/aml-build.c         | 30 ++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c        | 30 ------------------------------
>  3 files changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 0f4ed53d7f..a67ab4618a 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 2c3702b882..1f7fd09112 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)
>  {
> @@ -1875,6 +1876,35 @@ build_hdr:
>                   "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
>  }
>  
> +void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
> +{
> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *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;
> +
> +    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> +    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);
> +    } 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);
> +    } else {
> +        g_warn_if_reached();
> +    }
> +
> +    tpm2_ptr->log_area_minimum_length =
> +        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
> +
> +    /* 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,
> +                                   ACPI_BUILD_TPMLOG_FILE, 0);
> +    build_header(linker, table_data,
> +                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> +}
> +

I'll let Igor and mst confirm/deny this, but my understanding was that the
build_append* API was the preferred way to create the table. Indeed, I
don't see too many table.field = cpu_to_le(...) lines in aml-build.c

I realize this function is just getting moved, but maybe it should get
converted to the build_append* API while being moved?

Thanks,
drew

Re: [PATCH v2 1/3] acpi: Move build_tpm2() in the generic part

[Auger Eric]

Hi,

On 5/6/20 8:33 AM, Andrew Jones wrote:
> On Tue, May 05, 2020 at 04:44:17PM +0200, 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>
>> ---
>>  include/hw/acpi/aml-build.h |  2 ++
>>  hw/acpi/aml-build.c         | 30 ++++++++++++++++++++++++++++++
>>  hw/i386/acpi-build.c        | 30 ------------------------------
>>  3 files changed, 32 insertions(+), 30 deletions(-)
>>
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 0f4ed53d7f..a67ab4618a 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 2c3702b882..1f7fd09112 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)
>>  {
>> @@ -1875,6 +1876,35 @@ build_hdr:
>>                   "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
>>  }
>>  
>> +void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>> +{
>> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *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;
>> +
>> +    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
>> +    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);
>> +    } 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);
>> +    } else {
>> +        g_warn_if_reached();
>> +    }
>> +
>> +    tpm2_ptr->log_area_minimum_length =
>> +        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
>> +
>> +    /* 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,
>> +                                   ACPI_BUILD_TPMLOG_FILE, 0);
>> +    build_header(linker, table_data,
>> +                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
>> +}
>> +
> 
> I'll let Igor and mst confirm/deny this, but my understanding was that the
> build_append* API was the preferred way to create the table. Indeed, I
> don't see too many table.field = cpu_to_le(...) lines in aml-build.c
> 
> I realize this function is just getting moved, but maybe it should get
> converted to the build_append* API while being moved?

The reason I did not convert is that the struct is as follows

struct Acpi20TPM2 {
    ACPI_TABLE_HEADER_DEF
    uint16_t platform_class;
    uint16_t reserved;
    uint64_t control_area_address;
    uint32_t start_method;
    uint8_t start_method_params[12];
    uint32_t log_area_minimum_length;
    uint64_t log_area_start_address;
} QEMU_PACKED;


If I understand correctly the build_append* adds the fields
contiguously. It was not straightforward to me how to skip the
start_method_params array.

While we are at it the tcpalog arg is not used. Shall I remove it?

Thanks

Eric

> 
> Thanks,
> drew
> 
> 

Re: [PATCH v2 1/3] acpi: Move build_tpm2() in the generic part

[Igor Mammedov]

On Wed, 6 May 2020 11:50:09 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi,
> 
> On 5/6/20 8:33 AM, Andrew Jones wrote:
> > On Tue, May 05, 2020 at 04:44:17PM +0200, 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>
> >> ---
> >>  include/hw/acpi/aml-build.h |  2 ++
> >>  hw/acpi/aml-build.c         | 30 ++++++++++++++++++++++++++++++
> >>  hw/i386/acpi-build.c        | 30 ------------------------------
> >>  3 files changed, 32 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> >> index 0f4ed53d7f..a67ab4618a 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 2c3702b882..1f7fd09112 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)
> >>  {
> >> @@ -1875,6 +1876,35 @@ build_hdr:
> >>                   "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
> >>  }
> >>  
> >> +void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
> >> +{
> >> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *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;
> >> +
> >> +    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> >> +    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);
> >> +    } 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);
> >> +    } else {
> >> +        g_warn_if_reached();
> >> +    }
> >> +
> >> +    tpm2_ptr->log_area_minimum_length =
> >> +        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
> >> +
> >> +    /* 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,
> >> +                                   ACPI_BUILD_TPMLOG_FILE, 0);
> >> +    build_header(linker, table_data,
> >> +                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> >> +}
> >> +  
> > 
> > I'll let Igor and mst confirm/deny this, but my understanding was that the
> > build_append* API was the preferred way to create the table. Indeed, I
> > don't see too many table.field = cpu_to_le(...) lines in aml-build.c
> > 
> > I realize this function is just getting moved, but maybe it should get
> > converted to the build_append* API while being moved?  
> 
> The reason I did not convert is that the struct is as follows
> 
> struct Acpi20TPM2 {
>     ACPI_TABLE_HEADER_DEF
>     uint16_t platform_class;
>     uint16_t reserved;
>     uint64_t control_area_address;
>     uint32_t start_method;
>     uint8_t start_method_params[12];
>     uint32_t log_area_minimum_length;
>     uint64_t log_area_start_address;
> } QEMU_PACKED;
> 
> 
> If I understand correctly the build_append* adds the fields
> contiguously. It was not straightforward to me how to skip the
> start_method_params array.

you can use g_array_append_vals() for adding byte array (even is it's all zeros)

> While we are at it the tcpalog arg is not used. Shall I remove it?
> 
> Thanks
> 
> Eric
> 
> > 
> > Thanks,
> > drew
> > 
> >   

Re: [PATCH v2 1/3] acpi: Move build_tpm2() in the generic part

[Auger Eric]

Hi Igor,
On 5/12/20 4:10 PM, Igor Mammedov wrote:
> On Wed, 6 May 2020 11:50:09 +0200
> Auger Eric <eric.auger@redhat.com> wrote:
> 
>> Hi,
>>
>> On 5/6/20 8:33 AM, Andrew Jones wrote:
>>> On Tue, May 05, 2020 at 04:44:17PM +0200, 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>
>>>> ---
>>>>  include/hw/acpi/aml-build.h |  2 ++
>>>>  hw/acpi/aml-build.c         | 30 ++++++++++++++++++++++++++++++
>>>>  hw/i386/acpi-build.c        | 30 ------------------------------
>>>>  3 files changed, 32 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>>>> index 0f4ed53d7f..a67ab4618a 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 2c3702b882..1f7fd09112 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)
>>>>  {
>>>> @@ -1875,6 +1876,35 @@ build_hdr:
>>>>                   "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
>>>>  }
>>>>  
>>>> +void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>>>> +{
>>>> +    Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *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;
>>>> +
>>>> +    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
>>>> +    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);
>>>> +    } 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);
>>>> +    } else {
>>>> +        g_warn_if_reached();
>>>> +    }
>>>> +
>>>> +    tpm2_ptr->log_area_minimum_length =
>>>> +        cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
>>>> +
>>>> +    /* 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,
>>>> +                                   ACPI_BUILD_TPMLOG_FILE, 0);
>>>> +    build_header(linker, table_data,
>>>> +                 (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
>>>> +}
>>>> +  
>>>
>>> I'll let Igor and mst confirm/deny this, but my understanding was that the
>>> build_append* API was the preferred way to create the table. Indeed, I
>>> don't see too many table.field = cpu_to_le(...) lines in aml-build.c
>>>
>>> I realize this function is just getting moved, but maybe it should get
>>> converted to the build_append* API while being moved?  
>>
>> The reason I did not convert is that the struct is as follows
>>
>> struct Acpi20TPM2 {
>>     ACPI_TABLE_HEADER_DEF
>>     uint16_t platform_class;
>>     uint16_t reserved;
>>     uint64_t control_area_address;
>>     uint32_t start_method;
>>     uint8_t start_method_params[12];
>>     uint32_t log_area_minimum_length;
>>     uint64_t log_area_start_address;
>> } QEMU_PACKED;
>>
>>
>> If I understand correctly the build_append* adds the fields
>> contiguously. It was not straightforward to me how to skip the
>> start_method_params array.
> 
> you can use g_array_append_vals() for adding byte array (even is it's all zeros)

OK I will do.

Thank you for your input.

Eric
> 
>> While we are at it the tcpalog arg is not used. Shall I remove it?
>>
>> Thanks
>>
>> Eric
>>
>>>
>>> Thanks,
>>> drew
>>>
>>>   
> 
> 

Re: [PATCH v2 1/3] acpi: Move build_tpm2() in the generic part

[Michael S. Tsirkin]

On Wed, May 06, 2020 at 08:33:14AM +0200, Andrew Jones wrote:
> I realize this function is just getting moved, but maybe it should get
> converted to the build_append* API while being moved?

I'd rather refactoring was done in a separate patch -
easier to review.

-- 
MST

Re: [PATCH v2 1/3] acpi: Move build_tpm2() in the generic part

[Igor Mammedov]

On Wed, 6 May 2020 05:58:25 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, May 06, 2020 at 08:33:14AM +0200, Andrew Jones wrote:
> > I realize this function is just getting moved, but maybe it should get
> > converted to the build_append* API while being moved?  
> 
> I'd rather refactoring was done in a separate patch -
> easier to review.
maybe first convert and then move

PS:
me wonders if we have test with TPM enabled, if not maybe it's time to add them
i.e. first goes testcase in bios-tables and then refactoring/moving
in that case review is simpler.

Re: [PATCH v2 1/3] acpi: Move build_tpm2() in the generic part

[Auger Eric]

Hi Igor,

On 5/12/20 4:14 PM, Igor Mammedov wrote:
> On Wed, 6 May 2020 05:58:25 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>> On Wed, May 06, 2020 at 08:33:14AM +0200, Andrew Jones wrote:
>>> I realize this function is just getting moved, but maybe it should get
>>> converted to the build_append* API while being moved?  
>>
>> I'd rather refactoring was done in a separate patch -
>> easier to review.
> maybe first convert and then move
> 
> PS:
> me wonders if we have test with TPM enabled, if not maybe it's time to add them
> i.e. first goes testcase in bios-tables and then refactoring/moving
> in that case review is simpler.
Do you mean tests checking the ACPI table content when TPM is
instantiated? I don't think so otherwise it would have failed I guess.
Otherwise we have functional tests with TPM (MMIO access), ie qtest
tests  tpm-tis-device-test and tpm-tis-device-swtpm-test.

Thanks

Eric
> 
> 

Re: [PATCH v2 1/3] acpi: Move build_tpm2() in the generic part

[Igor Mammedov]

On Tue, 12 May 2020 17:59:25 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Igor,
> 
> On 5/12/20 4:14 PM, Igor Mammedov wrote:
> > On Wed, 6 May 2020 05:58:25 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> >> On Wed, May 06, 2020 at 08:33:14AM +0200, Andrew Jones wrote:  
> >>> I realize this function is just getting moved, but maybe it should get
> >>> converted to the build_append* API while being moved?    
> >>
> >> I'd rather refactoring was done in a separate patch -
> >> easier to review.  
> > maybe first convert and then move
> > 
> > PS:
> > me wonders if we have test with TPM enabled, if not maybe it's time to add them
> > i.e. first goes testcase in bios-tables and then refactoring/moving
> > in that case review is simpler.  
> Do you mean tests checking the ACPI table content when TPM is
> instantiated? I don't think so otherwise it would have failed I guess.
yes, I've meant that.

> Otherwise we have functional tests with TPM (MMIO access), ie qtest
> tests  tpm-tis-device-test and tpm-tis-device-swtpm-test.
> 
> Thanks
> 
> Eric
> > 
> >   

[PATCH v2 2/3] 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>

---

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 | 11 +++++++++++
 3 files changed, 16 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 1f7fd09112..4224675cb2 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1882,12 +1882,13 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
     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;
+    TPMIf *tpmif = tpm_find();
 
     tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
-    if (TPM_IS_TIS_ISA(tpm_find())) {
+    if (TPM_IS_TIS_ISA(tpmif) || TPM_IS_TIS_SYSBUS(tpmif)) {
         tpm2_ptr->control_area_address = cpu_to_le64(0);
         tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
-    } else if (TPM_IS_CRB(tpm_find())) {
+    } else if (TPM_IS_CRB(tpmif)) {
         tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
         tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);
     } else {
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 81d41a3990..1a2ec10c8f 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -41,11 +41,13 @@
 #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 "sysemu/numa.h"
 #include "sysemu/reset.h"
+#include "sysemu/tpm.h"
 #include "kvm_arm.h"
 #include "migration/vmstate.h"
 
@@ -831,6 +833,15 @@ 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_data_push(tables->tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
+        bios_linker_loader_alloc(tables->linker, ACPI_BUILD_TPMLOG_FILE,
+                                 tables->tcpalog, 1, false);
+
+        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

Re: [PATCH v2 2/3] arm/acpi: TPM2 ACPI table support

[Stefan Berger]

On 5/5/20 10:44 AM, Eric Auger wrote:
> Add a TPM2 ACPI table if a TPM2.0 sysbus device has been
> dynamically instantiated.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

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


>
> ---
>
> 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 | 11 +++++++++++
>   3 files changed, 16 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 1f7fd09112..4224675cb2 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1882,12 +1882,13 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>       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;
> +    TPMIf *tpmif = tpm_find();
>   
>       tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> -    if (TPM_IS_TIS_ISA(tpm_find())) {
> +    if (TPM_IS_TIS_ISA(tpmif) || TPM_IS_TIS_SYSBUS(tpmif)) {
>           tpm2_ptr->control_area_address = cpu_to_le64(0);
>           tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
> -    } else if (TPM_IS_CRB(tpm_find())) {
> +    } else if (TPM_IS_CRB(tpmif)) {
>           tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
>           tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);
>       } else {
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 81d41a3990..1a2ec10c8f 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -41,11 +41,13 @@
>   #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 "sysemu/numa.h"
>   #include "sysemu/reset.h"
> +#include "sysemu/tpm.h"
>   #include "kvm_arm.h"
>   #include "migration/vmstate.h"
>   
> @@ -831,6 +833,15 @@ 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_data_push(tables->tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
> +        bios_linker_loader_alloc(tables->linker, ACPI_BUILD_TPMLOG_FILE,
> +                                 tables->tcpalog, 1, false);
> +
> +        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);

Re: [PATCH v2 2/3] arm/acpi: TPM2 ACPI table support

[Igor Mammedov]

On Tue,  5 May 2020 16:44:18 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> Add a TPM2 ACPI table if a TPM2.0 sysbus device has been
> dynamically instantiated.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

on x86 we also do:

  fw_cfg_add_file(x86ms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,                       
                  tables.tcpalog->data, acpi_data_len(tables.tcpalog)); 

question is why it's not necessary in case of ARM?

> 
> ---
> 
> 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 | 11 +++++++++++
>  3 files changed, 16 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 1f7fd09112..4224675cb2 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1882,12 +1882,13 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>      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;
> +    TPMIf *tpmif = tpm_find();
>  
>      tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> -    if (TPM_IS_TIS_ISA(tpm_find())) {
> +    if (TPM_IS_TIS_ISA(tpmif) || TPM_IS_TIS_SYSBUS(tpmif)) {
>          tpm2_ptr->control_area_address = cpu_to_le64(0);
>          tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
> -    } else if (TPM_IS_CRB(tpm_find())) {
> +    } else if (TPM_IS_CRB(tpmif)) {
>          tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
>          tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);
>      } else {
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 81d41a3990..1a2ec10c8f 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -41,11 +41,13 @@
>  #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 "sysemu/numa.h"
>  #include "sysemu/reset.h"
> +#include "sysemu/tpm.h"
>  #include "kvm_arm.h"
>  #include "migration/vmstate.h"
>  
> @@ -831,6 +833,15 @@ 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_data_push(tables->tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
> +        bios_linker_loader_alloc(tables->linker, ACPI_BUILD_TPMLOG_FILE,
> +                                 tables->tcpalog, 1, false);
> +
> +        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);

Re: [PATCH v2 2/3] arm/acpi: TPM2 ACPI table support

[Auger Eric]

Hi Igor,

On 5/12/20 4:27 PM, Igor Mammedov wrote:
> On Tue,  5 May 2020 16:44:18 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
> 
>> Add a TPM2 ACPI table if a TPM2.0 sysbus device has been
>> dynamically instantiated.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> on x86 we also do:
> 
>   fw_cfg_add_file(x86ms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,                       
>                   tables.tcpalog->data, acpi_data_len(tables.tcpalog)); 
> 
> question is why it's not necessary in case of ARM?
you have it in virt_acpi_setup():
fw_cfg_add_file(vms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
tables.tcpalog->data, acpi_data_len(tables.tcpalog));

Thanks

Eric
> 
>>
>> ---
>>
>> 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 | 11 +++++++++++
>>  3 files changed, 16 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 1f7fd09112..4224675cb2 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1882,12 +1882,13 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>>      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;
>> +    TPMIf *tpmif = tpm_find();
>>  
>>      tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
>> -    if (TPM_IS_TIS_ISA(tpm_find())) {
>> +    if (TPM_IS_TIS_ISA(tpmif) || TPM_IS_TIS_SYSBUS(tpmif)) {
>>          tpm2_ptr->control_area_address = cpu_to_le64(0);
>>          tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
>> -    } else if (TPM_IS_CRB(tpm_find())) {
>> +    } else if (TPM_IS_CRB(tpmif)) {
>>          tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
>>          tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);
>>      } else {
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 81d41a3990..1a2ec10c8f 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -41,11 +41,13 @@
>>  #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 "sysemu/numa.h"
>>  #include "sysemu/reset.h"
>> +#include "sysemu/tpm.h"
>>  #include "kvm_arm.h"
>>  #include "migration/vmstate.h"
>>  
>> @@ -831,6 +833,15 @@ 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_data_push(tables->tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
>> +        bios_linker_loader_alloc(tables->linker, ACPI_BUILD_TPMLOG_FILE,
>> +                                 tables->tcpalog, 1, false);
>> +
>> +        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);
> 

[PATCH v2 3/3] 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>

---

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

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 1a2ec10c8f..8534d14e20 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -45,6 +45,7 @@
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
 #include "hw/arm/virt.h"
+#include "hw/platform-bus.h"
 #include "sysemu/numa.h"
 #include "sysemu/reset.h"
 #include "sysemu/tpm.h"
@@ -362,6 +363,40 @@ static void acpi_dsdt_add_power_button(Aml *scope)
     aml_append(scope, dev);
 }
 
+static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
+{
+    hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
+    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
+    MemoryRegion *sbdev_mr;
+    SysBusDevice *sbdev;
+    hwaddr tpm_base;
+
+    sbdev = (SysBusDevice *)object_dynamic_cast(OBJECT(tpm_find()),
+                                                TYPE_SYS_BUS_DEVICE);
+    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)
 {
@@ -756,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

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

[Shannon Zhao]

Hi,

On 2020/5/5 22:44, Eric Auger wrote:
> +static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
> +{
> +    hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
> +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> +    MemoryRegion *sbdev_mr;
> +    SysBusDevice *sbdev;
> +    hwaddr tpm_base;
> +
> +    sbdev = (SysBusDevice *)object_dynamic_cast(OBJECT(tpm_find()),
> +                                                TYPE_SYS_BUS_DEVICE);

Does it need to check the tpm version like you do in previous patch?

tpm_get_version(tpm_find()) == TPM_VERSION_2_0

Thanks,
Shannon

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

[Ard Biesheuvel]

On Fri, 8 May 2020 at 17:24, Shannon Zhao <shannon.zhaosl@gmail.com> wrote:
>
> Hi,
>
> On 2020/5/5 22:44, Eric Auger wrote:
> > +static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
> > +{
> > +    hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
> > +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> > +    MemoryRegion *sbdev_mr;
> > +    SysBusDevice *sbdev;
> > +    hwaddr tpm_base;
> > +
> > +    sbdev = (SysBusDevice *)object_dynamic_cast(OBJECT(tpm_find()),
> > +                                                TYPE_SYS_BUS_DEVICE);
>
> Does it need to check the tpm version like you do in previous patch?
>
> tpm_get_version(tpm_find()) == TPM_VERSION_2_0
>

I don't think so. The device node could in theory be used to describe
a TPM 1.2/1.3 as well, even though we never actually do that.

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

[Stefan Berger]

On 5/8/20 11:25 AM, Ard Biesheuvel wrote:
> On Fri, 8 May 2020 at 17:24, Shannon Zhao <shannon.zhaosl@gmail.com> wrote:
>> Hi,
>>
>> On 2020/5/5 22:44, Eric Auger wrote:
>>> +static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
>>> +{
>>> +    hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
>>> +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
>>> +    MemoryRegion *sbdev_mr;
>>> +    SysBusDevice *sbdev;
>>> +    hwaddr tpm_base;
>>> +
>>> +    sbdev = (SysBusDevice *)object_dynamic_cast(OBJECT(tpm_find()),
>>> +                                                TYPE_SYS_BUS_DEVICE);
>> Does it need to check the tpm version like you do in previous patch?
>>
>> tpm_get_version(tpm_find()) == TPM_VERSION_2_0
>>
> I don't think so. The device node could in theory be used to describe
> a TPM 1.2/1.3 as well, even though we never actually do that.

There is no TPM 1.3. There may be a TIS v1.3.

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

[Igor Mammedov]

On Tue,  5 May 2020 16:44:19 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> In case it is dynamically instantiated, add the TPM 2.0 device object
> under the DSDT table in the ACPI namespace. Its HID is MSFT0101
> while its current resource settings (CRS) property is initialized
> with the guest physical address and MMIO size of the device.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> ---
> 
> 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 | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1a2ec10c8f..8534d14e20 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -45,6 +45,7 @@
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pci.h"
>  #include "hw/arm/virt.h"
> +#include "hw/platform-bus.h"
>  #include "sysemu/numa.h"
>  #include "sysemu/reset.h"
>  #include "sysemu/tpm.h"
> @@ -362,6 +363,40 @@ static void acpi_dsdt_add_power_button(Aml *scope)
>      aml_append(scope, dev);
>  }
>  
> +static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
> +{
> +    hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
> +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> +    MemoryRegion *sbdev_mr;
> +    SysBusDevice *sbdev;
> +    hwaddr tpm_base;
> +
> +    sbdev = (SysBusDevice *)object_dynamic_cast(OBJECT(tpm_find()),
> +                                                TYPE_SYS_BUS_DEVICE);
SYS_BUS_DEVICE(object_dynamic_cast())
      

> +    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)
>  {
> @@ -756,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);
>  

Re: [PATCH v2 0/3] vTPM/aarch64 ACPI support

[Ard Biesheuvel]

On Tue, 5 May 2020 at 16:44, Eric Auger <eric.auger@redhat.com> wrote:
>
> Those patches bring MMIO TPM TIS ACPI support in machvirt.
> The first patch moves the TPM2 ACPI table generation code
> in the generic code. Then the table is added if the TPM2
> 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.
>

Thanks Eric

Tested-by: Ard Biesheuvel <ardb@kernel.org>