On Mon, 1 Jun 2020 11:57:34 +0200
Eric Auger <eric.au...@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.au...@redhat.com>
> ---
> hw/i386/acpi-build.c | 28 +++++++++++++++++++---------
> 1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b5669d6c65..f0d35d7b17 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2298,30 +2298,40 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker
> *linker, GArray *tcpalog)
> static void
> build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
> {
> - Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data, sizeof *tpm2_ptr);
> + Acpi20TPM2 *tpm2_ptr = acpi_data_push(table_data,
> sizeof(AcpiTableHeader));
> unsigned log_addr_size = sizeof(tpm2_ptr->log_area_start_address);
> unsigned log_addr_offset =
> (char *)&tpm2_ptr->log_area_start_address - table_data->data;
Is this the reason why you've kept Acpi20TPM2 around?
> + uint8_t start_method_params[12] = {};
>
> - tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
> + /* platform class */
pls verbatim filed names from spec in comments
> + build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
> + /* reserved */
> + build_append_int_noprefix(table_data, 0, 2);
> if (TPM_IS_TIS_ISA(tpm_find())) {
> - tpm2_ptr->control_area_address = cpu_to_le64(0);
> - tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_MMIO);
> + /* address of control area */
> + build_append_int_noprefix(table_data, 0, 8);
> + /* start method */
> + build_append_int_noprefix(table_data, TPM2_START_METHOD_MMIO, 4);
> } else if (TPM_IS_CRB(tpm_find())) {
> - tpm2_ptr->control_area_address = cpu_to_le64(TPM_CRB_ADDR_CTRL);
> - tpm2_ptr->start_method = cpu_to_le32(TPM2_START_METHOD_CRB);
missing field name comments
> + build_append_int_noprefix(table_data, TPM_CRB_ADDR_CTRL, 8);
> + build_append_int_noprefix(table_data, TPM2_START_METHOD_CRB, 4);
> } else {
> g_warn_if_reached();
> }
considering fields are the same I'd also restructure above as
if () {
control_area_address =
start_method =
...
}
/* address of control area */
build_append_int_noprefix(table_data, control_area_address, 8);
/* start method */
build_append_int_noprefix(table_data, start_method, 4);
which is bit easier to read
>
> - tpm2_ptr->log_area_minimum_length =
> - cpu_to_le32(TPM_LOG_AREA_MINIMUM_SIZE);
> + /* platform specific parameters */
> + g_array_append_vals(table_data, &start_method_params, 12);
>
> - acpi_data_push(tcpalog, le32_to_cpu(tpm2_ptr->log_area_minimum_length));
> + /* log area minimum length */
> + build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
> +
> + acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
> bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
> false);
I suggest to drop Acpi20TPM2 with pointer math above and use approach similar
to build_ghes_v2/address_offset, i.e. get actual offest here:
log_addr_offset = table_data->len
and s/log_addr_size/8/
> /* log area start address to be filled by Guest linker */
> + build_append_int_noprefix(table_data, 0, 8);
> bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> log_addr_offset, log_addr_size,
> ACPI_BUILD_TPMLOG_FILE, 0);
