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

[Igor Mammedov]

On Wed, 10 Jun 2020 18:15:07 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Igor,
> 
> On 6/5/20 4:23 PM, Igor Mammedov wrote:
> > On Mon,  1 Jun 2020 11:57:34 +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>
> >> ---
> >>  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?  
> Yes it is
> > 
> >   
> >> +    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  
> Do you want "Platform Class" instead?
yep, just copy past field names from spec as is.

> >   
> >> +    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  
> OK
> >   
> >> +        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   
> OK
> >       
> >>  
> >> -    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  
> yes, that's simpler. Thank you for the suggestion.
> > 
> > and s/log_addr_size/8/  
> OK
> 
> Thanks
> 
> Eric
> >   
> >>      /* 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);  
> >