On Tue, 2 Jun 2020 10:24:03 -0400
Stefan Berger <stef...@linux.ibm.com> wrote:
> 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.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));
> >> 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.
practice showed it was opposite, it was easy to forget about cpu_to_le
build_append_int_noprefix() API we don't have to care about endiannes
issues anymore at places where API is used.
Another reasons for using build_append_int_noprefix() is that one doesn't
have to define packed structures, work around unaligned access and bit plus is
when I review patch it boiles down to comparing it with table in a spec
row by row as API enforces the same order as in spec, which makes reviews
much easier.
>
> >>
> >>> 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.
I guss I'm just used to g_array_append_vals() but build_append_array() looks
nicer
and consistent with build_append_foo API
>
>
> >>> - 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);
> >>
>
>
>
