Hi Igor,
On 6/16/20 2:33 PM, Igor Mammedov wrote:
> On Thu, 11 Jun 2020 15:59:13 +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>
>>
>> ---
>>
>> 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
>
>
