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

[wangyanan (Y)]

Hi Drew,

On 2022/1/3 19:30, Andrew Jones wrote:
> 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.
Hm yes. Currently for clusters alone, a check of 
ms->smp_props.clusters_supported
would be enough. So I guess it's preferred that we should keep the
generic PPTT, right?
> > 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.
Yes, I changed the function parameter "ms" to "vms". That's the
whole update.

Thanks,
Yanan
> > 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
>
> .