Re: [PATCH v4 1/5] acpi: Convert build_tpm2() to build_append* API

[Auger Eric <eric.auger@redhat.com>]

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)
> 
> 
>