Re: [PATCH v3 28/35] acpi: arm: virt: build_iort: use acpi_table_begin()/acpi_table_end() instead of build_header()

[Eric Auger]

Hi Igor,

On 9/7/21 4:48 PM, Igor Mammedov wrote:
> it replaces error-prone pointer arithmetic for build_header() API,
> with 2 calls to start and finish table creation,
> which hides offsets magic from API user.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v3:
>   * fix conflicts due to
>       42e0f050e3a51 'hw/arm/virt-acpi-build: Add IORT support to bypass 
> SMMUv3'
>   *  * s/acpi_init_table|acpi_table_composed/acpi_table_begin|acpi_table_end/
> 
> CC: drjones@redhat.com
> CC: peter.maydell@linaro.org
> CC: shannon.zhaosl@gmail.com
> CC: qemu-arm@nongnu.org
> CC: eauger@redhat.com
> ---
>  include/hw/acpi/acpi-defs.h | 14 ------------
>  hw/arm/virt-acpi-build.c    | 45 ++++++++++++++++---------------------
>  2 files changed, 19 insertions(+), 40 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index bcada37601..195f90caf6 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -188,20 +188,6 @@ struct AcpiGenericTimerTable {
>  } QEMU_PACKED;
>  typedef struct AcpiGenericTimerTable AcpiGenericTimerTable;
>  
> -/*
> - * Input Output Remapping Table (IORT)
> - * Conforms to "IO Remapping Table System Software on ARM Platforms",
> - * Document number: ARM DEN 0049B, October 2015
> - */
> -
> -struct AcpiIortTable {
> -    ACPI_TABLE_HEADER_DEF     /* ACPI common table header */
> -    uint32_t node_count;
> -    uint32_t node_offset;
> -    uint32_t reserved;
> -} QEMU_PACKED;
> -typedef struct AcpiIortTable AcpiIortTable;
> -
>  /*
>   * IORT node types
>   */
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 4b9687439d..bceb1b3b7e 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -273,22 +273,29 @@ static int iort_idmap_compare(gconstpointer a, 
> gconstpointer b)
>      return idmap_a->input_base - idmap_b->input_base;
>  }
>  
> +/*
> + * Input Output Remapping Table (IORT)
> + * Conforms to "IO Remapping Table System Software on ARM Platforms",
> + * Document number: ARM DEN 0049B, October 2015
> + */
>  static void
>  build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> -    int i, nb_nodes, rc_mapping_count, iort_start = table_data->len;
> +    int i, nb_nodes, rc_mapping_count;
>      AcpiIortIdMapping *idmap;
>      AcpiIortItsGroup *its;
> -    AcpiIortTable *iort;
>      AcpiIortSmmu3 *smmu;
> -    size_t node_size, iort_node_offset, iort_length, smmu_offset = 0;
>      AcpiIortRC *rc;
> +    const uint32_t iort_node_offset = 48;
> +    size_t node_size, smmu_offset = 0;
>      GArray *smmu_idmaps = g_array_new(false, true, 
> sizeof(AcpiIortIdMapping));
>      GArray *its_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
>  
> -    iort = acpi_data_push(table_data, sizeof(*iort));
> +    AcpiTable table = { .sig = "IORT", .rev = 0, .oem_id = vms->oem_id,
> +                        .oem_table_id = vms->oem_table_id };
>  
>      if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> +
forgot this minor comment
nit: spurious change

Eric
>          AcpiIortIdMapping next_range = {0};
>  
>          object_child_foreach_recursive(object_get_root(),
> @@ -325,18 +332,16 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>          rc_mapping_count = 1;
>      }
>  
> -    iort_length = sizeof(*iort);
> -    iort->node_count = cpu_to_le32(nb_nodes);
> -    /*
> -     * Use a copy in case table_data->data moves during acpi_data_push
> -     * operations.
> -     */
> -    iort_node_offset = sizeof(*iort);
> -    iort->node_offset = cpu_to_le32(iort_node_offset);
> +    /* Table 2 The IORT */
> +    acpi_table_begin(&table, table_data);
> +    /* Number of IORT Nodes */
> +    build_append_int_noprefix(table_data, nb_nodes, 4);
> +    /* Offset to Array of IORT Nodes */
> +    build_append_int_noprefix(table_data, iort_node_offset, 4);
> +    build_append_int_noprefix(table_data, 0, 4); /* Reserved */
>  
>      /* ITS group node */
>      node_size =  sizeof(*its) + sizeof(uint32_t);
> -    iort_length += node_size;
>      its = acpi_data_push(table_data, node_size);
>  
>      its->type = ACPI_IORT_NODE_ITS_GROUP;
> @@ -350,7 +355,6 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>          /* SMMUv3 node */
>          smmu_offset = iort_node_offset + node_size;
>          node_size = sizeof(*smmu) + sizeof(*idmap);
> -        iort_length += node_size;
>          smmu = acpi_data_push(table_data, node_size);
>  
>          smmu->type = ACPI_IORT_NODE_SMMU_V3;
> @@ -375,7 +379,6 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>  
>      /* Root Complex Node */
>      node_size = sizeof(*rc) + sizeof(*idmap) * rc_mapping_count;
> -    iort_length += node_size;
>      rc = acpi_data_push(table_data, node_size);
>  
>      rc->type = ACPI_IORT_NODE_PCI_ROOT_COMPLEX;
> @@ -424,19 +427,9 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>          idmap->output_reference = cpu_to_le32(iort_node_offset);
>      }
>  
> +    acpi_table_end(linker, &table);
>      g_array_free(smmu_idmaps, true);
>      g_array_free(its_idmaps, true);
> -
> -    /*
> -     * Update the pointer address in case table_data->data moves during above
> -     * acpi_data_push operations.
> -     */
> -    iort = (AcpiIortTable *)(table_data->data + iort_start);
> -    iort->length = cpu_to_le32(iort_length);
> -
> -    build_header(linker, table_data, (void *)(table_data->data + iort_start),
> -                 "IORT", table_data->len - iort_start, 0, vms->oem_id,
> -                 vms->oem_table_id);
>  }
>  
>  static void
>