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

[Stefan Berger <stefanb@linux.ibm.com>]

On 6/2/20 9:55 AM, Auger Eric wrote:
> Hi Stefan,
> On 6/2/20 3:30 PM, Stefan Berger wrote:
>> On 6/1/20 5:57 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>
>>> ---
>>>    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));
>> And now you want to build the data structure by pushing fields? I would
>> definitely NOT do this.
> If I didn't misinterpret things, this was recommended by Drew and Igor
> as buid_append* API avoids to take care of endianness and this is the
> API now used in the generic ACPI code. Besides I also think that in that
> case it does not simplify things but maybe I did that the wrong way? Or
> maybe I didn't understand your remark?


If that's what they are saying... I would prefer filling out data 
structures with functions like cpu_to_acpi16() because that seems to be 
less error prone.


>>
>>>        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] = {};
>>>    -    tpm2_ptr->platform_class = cpu_to_le16(TPM2_ACPI_CLASS_CLIENT);
>>> +    /* 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);
>>> +        /* 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);
>>> +        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();
>>>        }
>>>    -    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);

Maybe this should be wrapped in an inline function like 
build_append_array() or so.


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


At this point we have a double-allocation of log memory on x86_64. You'd 
need the patch I posted to create the TCPA table only for TPM 1.2.



>>>        bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE,
>>> tcpalog, 1,
>>>                                 false);
>>>          /* 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);
>>