| Message ID | 20200611135917.18300-2-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vTPM/aarch64 ACPI support | expand |
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)
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) > > >
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) > > >
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>
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 > > > >
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) > >
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?
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 > >
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
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
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
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 >
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 >>
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 > >> >
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 >>>> >> > >
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 > >>>> > >> > > > >
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 >>>>>> >>>> >>> >>> >
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)
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(-)