Re: [PATCH v6 4/7] hw/arm/virt-acpi-build: Make an ARM specific PPTT generator

[Andrew Jones]

On Mon, Jan 03, 2022 at 04:46:33PM +0800, Yanan Wang wrote:
> We have a generic build_pptt() in hw/acpi/aml-build.c but it's
> currently only used in ARM acpi initialization. Now we are going
> to support the new CPU cluster parameter which is currently only
> supported by ARM, it won't be a very good idea to add it to the
> generic build_pptt() as it will make the code complex and hard
> to maintain especially when we also support CPU cache topology
> hierarchy in build_pptt() too. Note that the cache topology
> design also varies between different CPU targets.

It's a shame to remove generic ACPI table. I'm not sure what
will be best when addressing the cache topology, but for the
clusters it seems like we should be able to easily skip them
when has_clusters is false.

> 
> So an ARM specific PPTT generator becomes necessary now. Given
> that the generic one is currently only used by ARM, let's just
> move build_pptt() from aml-build.c to virt-acpi-build.c with
> minor update.

Please state what the minor update is? I see the oem parameter
change, but I want to be sure that's the one you're referring
to.

> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/acpi/aml-build.c         | 80 ++-----------------------------------
>  hw/arm/virt-acpi-build.c    | 77 ++++++++++++++++++++++++++++++++++-
>  include/hw/acpi/aml-build.h |  5 ++-
>  3 files changed, 81 insertions(+), 81 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index be3851be36..040fbc9b4b 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1968,10 +1968,9 @@ void build_slit(GArray *table_data, BIOSLinker 
> *linker, MachineState *ms,
>   * ACPI spec, Revision 6.3
>   * 5.2.29.1 Processor hierarchy node structure (Type 0)
>   */
> -static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> -                                           uint32_t parent, uint32_t id,
> -                                           uint32_t *priv_rsrc,
> -                                           uint32_t priv_num)
> +void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> +                                    uint32_t parent, uint32_t id,
> +                                    uint32_t *priv_rsrc, uint32_t priv_num)
>  {
>      int i;
>  
> @@ -1994,79 +1993,6 @@ static void build_processor_hierarchy_node(GArray 
> *tbl, uint32_t flags,
>      }
>  }
>  
> -/*
> - * ACPI spec, Revision 6.3
> - * 5.2.29 Processor Properties Topology Table (PPTT)
> - */
> -void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> -                const char *oem_id, const char *oem_table_id)
> -{
> -    GQueue *list = g_queue_new();
> -    guint pptt_start = table_data->len;
> -    guint father_offset;
> -    guint length, i;
> -    int uid = 0;
> -    int socket;
> -    AcpiTable table = { .sig = "PPTT", .rev = 2,
> -                        .oem_id = oem_id, .oem_table_id = oem_table_id };
> -
> -    acpi_table_begin(&table, table_data);
> -
> -    for (socket = 0; socket < ms->smp.sockets; socket++) {
> -        g_queue_push_tail(list,
> -            GUINT_TO_POINTER(table_data->len - pptt_start));
> -        build_processor_hierarchy_node(
> -            table_data,
> -            /*
> -             * Physical package - represents the boundary
> -             * of a physical package
> -             */
> -            (1 << 0),
> -            0, socket, NULL, 0);
> -    }
> -
> -    length = g_queue_get_length(list);
> -    for (i = 0; i < length; i++) {
> -        int core;
> -
> -        father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> -        for (core = 0; core < ms->smp.cores; core++) {
> -            if (ms->smp.threads > 1) {
> -                g_queue_push_tail(list,
> -                    GUINT_TO_POINTER(table_data->len - pptt_start));
> -                build_processor_hierarchy_node(
> -                    table_data,
> -                    (0 << 0), /* not a physical package */
> -                    father_offset, core, NULL, 0);
> -            } else {
> -                build_processor_hierarchy_node(
> -                    table_data,
> -                    (1 << 1) | /* ACPI Processor ID valid */
> -                    (1 << 3),  /* Node is a Leaf */
> -                    father_offset, uid++, NULL, 0);
> -            }
> -        }
> -    }
> -
> -    length = g_queue_get_length(list);
> -    for (i = 0; i < length; i++) {
> -        int thread;
> -
> -        father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> -        for (thread = 0; thread < ms->smp.threads; thread++) {
> -            build_processor_hierarchy_node(
> -                table_data,
> -                (1 << 1) | /* ACPI Processor ID valid */
> -                (1 << 2) | /* Processor is a Thread */
> -                (1 << 3),  /* Node is a Leaf */
> -                father_offset, uid++, NULL, 0);
> -        }
> -    }
> -
> -    g_queue_free(list);
> -    acpi_table_end(linker, &table);
> -}
> -
>  /* build rev1/rev3/rev5.1 FADT */
>  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>                  const char *oem_id, const char *oem_table_id)
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index d0f4867fdf..3ce7680393 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -808,6 +808,80 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>      acpi_table_end(linker, &table);
>  }
>  
> +/*
> + * ACPI spec, Revision 6.3
> + * 5.2.29 Processor Properties Topology Table (PPTT)
> + */
> +static void
> +build_pptt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> +{
> +    MachineState *ms = MACHINE(vms);
> +    GQueue *list = g_queue_new();
> +    guint pptt_start = table_data->len;
> +    guint father_offset;
> +    guint length, i;
> +    int uid = 0;
> +    int socket;
> +    AcpiTable table = { .sig = "PPTT", .rev = 2, .oem_id = vms->oem_id,
> +                        .oem_table_id = vms->oem_table_id };
> +
> +    acpi_table_begin(&table, table_data);
> +
> +    for (socket = 0; socket < ms->smp.sockets; socket++) {
> +        g_queue_push_tail(list,
> +            GUINT_TO_POINTER(table_data->len - pptt_start));
> +        build_processor_hierarchy_node(
> +            table_data,
> +            /*
> +             * Physical package - represents the boundary
> +             * of a physical package
> +             */
> +            (1 << 0),
> +            0, socket, NULL, 0);
> +    }
> +
> +    length = g_queue_get_length(list);
> +    for (i = 0; i < length; i++) {
> +        int core;
> +
> +        father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> +        for (core = 0; core < ms->smp.cores; core++) {
> +            if (ms->smp.threads > 1) {
> +                g_queue_push_tail(list,
> +                    GUINT_TO_POINTER(table_data->len - pptt_start));
> +                build_processor_hierarchy_node(
> +                    table_data,
> +                    (0 << 0), /* not a physical package */
> +                    father_offset, core, NULL, 0);
> +            } else {
> +                build_processor_hierarchy_node(
> +                    table_data,
> +                    (1 << 1) | /* ACPI Processor ID valid */
> +                    (1 << 3),  /* Node is a Leaf */
> +                    father_offset, uid++, NULL, 0);
> +            }
> +        }
> +    }
> +
> +    length = g_queue_get_length(list);
> +    for (i = 0; i < length; i++) {
> +        int thread;
> +
> +        father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> +        for (thread = 0; thread < ms->smp.threads; thread++) {
> +            build_processor_hierarchy_node(
> +                table_data,
> +                (1 << 1) | /* ACPI Processor ID valid */
> +                (1 << 2) | /* Processor is a Thread */
> +                (1 << 3),  /* Node is a Leaf */
> +                father_offset, uid++, NULL, 0);
> +        }
> +    }
> +
> +    g_queue_free(list);
> +    acpi_table_end(linker, &table);
> +}
> +
>  /* FADT */
>  static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
>                              VirtMachineState *vms, unsigned dsdt_tbl_offset)
> @@ -953,8 +1027,7 @@ void virt_acpi_build(VirtMachineState *vms, 
> AcpiBuildTables *tables)
>  
>      if (!vmc->no_cpu_topology) {
>          acpi_add_table(table_offsets, tables_blob);
> -        build_pptt(tables_blob, tables->linker, ms,
> -                   vms->oem_id, vms->oem_table_id);
> +        build_pptt(tables_blob, tables->linker, vms);
>      }
>  
>      acpi_add_table(table_offsets, tables_blob);
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 8346003a22..2c457c8f17 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -489,8 +489,9 @@ void build_srat_memory(GArray *table_data, uint64_t base,
>  void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>                  const char *oem_id, const char *oem_table_id);
>  
> -void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> -                const char *oem_id, const char *oem_table_id);
> +void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> +                                    uint32_t parent, uint32_t id,
> +                                    uint32_t *priv_rsrc, uint32_t priv_num);
>  
>  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>                  const char *oem_id, const char *oem_table_id);
> -- 
> 2.27.0
>

Thanks,
drew