Re: [PATCH v5 4/4] hw/acpi/aml-build: Use existing CPU topology to build PPTT table

[Gavin Shan]

Hi Igor,

On 4/13/22 9:52 PM, Igor Mammedov wrote:
> On Sun,  3 Apr 2022 22:59:53 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>
> > When the PPTT table is built, the CPU topology is re-calculated, but
> > it's unecessary because the CPU topology has been populated in
> > virt_possible_cpu_arch_ids() on arm/virt machine.
> >
> > This reworks build_pptt() to avoid by reusing the existing one in
> > ms->possible_cpus. Currently, the only user of build_pptt() is
> > arm/virt machine.
> >
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > ---
> >   hw/acpi/aml-build.c | 100 +++++++++++++++++---------------------------
> >   1 file changed, 38 insertions(+), 62 deletions(-)
> >
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index 4086879ebf..4b0f9df3e3 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -2002,86 +2002,62 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, 
> > MachineState *ms,
> >                   const char *oem_id, const char *oem_table_id)
> >   {
> >       MachineClass *mc = MACHINE_GET_CLASS(ms);
> > -    GQueue *list = g_queue_new();
> > -    guint pptt_start = table_data->len;
> > -    guint parent_offset;
> > -    guint length, i;
> > -    int uid = 0;
> > -    int socket;
> > +    CPUArchIdList *cpus = ms->possible_cpus;
> > +    int64_t socket_id = -1, cluster_id = -1, core_id = -1;
> > +    uint32_t socket_offset, cluster_offset, core_offset;
> > +    uint32_t pptt_start = table_data->len;
> > +    int n;
> >       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);
> > -    }
> > +    for (n = 0; n < cpus->len; n++) {
>
> > +        if (cpus->cpus[n].props.socket_id != socket_id) {
> > +            socket_id = cpus->cpus[n].props.socket_id;
>
> this relies on cpus->cpus[n].props.*_id being sorted form top to down levels
> I'd add here and for other container_id an assert() that checks for that
> specific ID goes in only one direction, to be able to detect when rule is 
> broken.
>
> otherwise on may end up with duplicate containers silently.

Exactly. cpus->cpus[n].props.*_id is sorted as you said in 
virt_possible_cpu_arch_ids().
The only user of build_pptt() is arm/virt machine. So it's fine. However, I 
think I
may need add comments for this in v6.

    /*
     * This works with the assumption that cpus[n].props.*_id has been
     * sorted from top to down levels in mc->possible_cpu_arch_ids().
     * Otherwise, the unexpected and duplicate containers will be created.
     */

The implementation in v3 looks complicated, but comprehensive. The one
in this revision (v6) looks simple, but the we're losing flexibility :)


> > +            cluster_id = -1;
> > +            core_id = -1;
> > +            socket_offset = table_data->len - pptt_start;
> > +            build_processor_hierarchy_node(table_data,
> > +                (1 << 0), /* Physical package */
> > +                0, socket_id, NULL, 0);
> > +        }
> >   
> > -    if (mc->smp_props.clusters_supported) {
> > -        length = g_queue_get_length(list);
> > -        for (i = 0; i < length; i++) {
> > -            int cluster;
> > -
> > -            parent_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> > -            for (cluster = 0; cluster < ms->smp.clusters; cluster++) {
> > -                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 */
> > -                    parent_offset, cluster, NULL, 0);
> > +        if (mc->smp_props.clusters_supported) {
> > +            if (cpus->cpus[n].props.cluster_id != cluster_id) {
> > +                cluster_id = cpus->cpus[n].props.cluster_id;
> > +                core_id = -1;
> > +                cluster_offset = table_data->len - pptt_start;
> > +                build_processor_hierarchy_node(table_data,
> > +                    (0 << 0), /* Not a physical package */
> > +                    socket_offset, cluster_id, NULL, 0);
> >               }
> > +        } else {
> > +            cluster_offset = socket_offset;
> >           }
> > -    }
> >   
> > -    length = g_queue_get_length(list);
> > -    for (i = 0; i < length; i++) {
> > -        int core;
> > -
> > -        parent_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,
> > +        if (ms->smp.threads <= 1) {
>
> why <= instead of < is used here?

It's the counterpart to the one in the original implementation,
which is "if (ms->smp.threads > 1)". the nodes for threads won't
be created until ms->smp.threads is bigger than 1. If we want
to use "<" here, then the code will be changed like below in v6.
However, I really don't think it's necessary.

           if (ms->smp.threads < 2) {
               :
           }


> > +            build_processor_hierarchy_node(table_data,
> > +                (1 << 1) | /* ACPI Processor ID valid */
> > +                (1 << 3),  /* Node is a Leaf */
> > +                cluster_offset, n, NULL, 0);
> > +        } else {
> > +            if (cpus->cpus[n].props.core_id != core_id) {
> > +                core_id = cpus->cpus[n].props.core_id;
> > +                core_offset = table_data->len - pptt_start;
> > +                build_processor_hierarchy_node(table_data,
> >                       (0 << 0), /* not a physical package */
> > -                    parent_offset, core, NULL, 0);
> > -            } else {
> > -                build_processor_hierarchy_node(
> > -                    table_data,
> > -                    (1 << 1) | /* ACPI Processor ID valid */
> > -                    (1 << 3),  /* Node is a Leaf */
> > -                    parent_offset, uid++, NULL, 0);
> > +                    cluster_offset, core_id, NULL, 0);
> >               }
> > -        }
> > -    }
> > -
> > -    length = g_queue_get_length(list);
> > -    for (i = 0; i < length; i++) {
> > -        int thread;
> >   
> > -        parent_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> > -        for (thread = 0; thread < ms->smp.threads; thread++) {
> > -            build_processor_hierarchy_node(
> > -                table_data,
> > +            build_processor_hierarchy_node(table_data,
> >                   (1 << 1) | /* ACPI Processor ID valid */
> >                   (1 << 2) | /* Processor is a Thread */
> >                   (1 << 3),  /* Node is a Leaf */
> > -                parent_offset, uid++, NULL, 0);
> > +                core_offset, n, NULL, 0);
> >           }
> >       }
> >   
> > -    g_queue_free(list);
> >       acpi_table_end(linker, &table);
> >   }
> >   

Thanks,
Gavin