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

[Igor Mammedov]

On Thu, 14 Apr 2022 08:33:29 +0800
Gavin Shan <gshan@redhat.com> wrote:

> 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 :)


comment is not enough, as it will break silently that's why I suggested
sprinkling asserts() here.

[...]
> Thanks,
> Gavin
>