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 IDs 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 | 109 +++++++++++++++++++-------------------------
1 file changed, 47 insertions(+), 62 deletions(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 4086879ebf..73f4e69c29 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2002,86 +2002,71 @@ 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 = 0, cluster_offset = 0, core_offset = 0;
+ 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);
- }
+ /*
+ * 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.
+ */
+ for (n = 0; n < cpus->len; n++) {
+ if (cpus->cpus[n].props.socket_id != socket_id) {
+ assert(cpus->cpus[n].props.socket_id > socket_id);
+ socket_id = cpus->cpus[n].props.socket_id;
+ 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) {
+ assert(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) {
+ 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) {
+ assert(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);
}
--
2.23.0Hi Gavin,
On 2022/4/20 18:49, Gavin Shan 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 IDs 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 | 109 +++++++++++++++++++-------------------------
> 1 file changed, 47 insertions(+), 62 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 4086879ebf..73f4e69c29 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2002,86 +2002,71 @@ 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 = 0, cluster_offset = 0, core_offset = 0;
> + 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);
> - }
> + /*
> + * 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.
> + */
s/duplicate/duplicated
> + for (n = 0; n < cpus->len; n++) {
> + if (cpus->cpus[n].props.socket_id != socket_id) {
> + assert(cpus->cpus[n].props.socket_id > socket_id);
> + socket_id = cpus->cpus[n].props.socket_id;
> + 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) {
> + assert(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) {
> + 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) {
> + assert(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 */
nit: For consistency, maybe "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);
> }
>
For the function of exposing vCPU topology to guest:
Tested-by: Yanan Wang <wangyanan55@huawei.com>
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Thanks,
Yanan
Hi Yanan,
On 4/21/22 7:50 PM, wangyanan (Y) wrote:
> On 2022/4/20 18:49, Gavin Shan 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 IDs 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 | 109 +++++++++++++++++++-------------------------
>> 1 file changed, 47 insertions(+), 62 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 4086879ebf..73f4e69c29 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2002,86 +2002,71 @@ 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 = 0, cluster_offset = 0, core_offset = 0;
>> + 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);
>> - }
>> + /*
>> + * 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.
>> + */
> s/duplicate/duplicated
Both should be fine, but I will replace it with the one you prefer
in v8.
>> + for (n = 0; n < cpus->len; n++) {
>> + if (cpus->cpus[n].props.socket_id != socket_id) {
>> + assert(cpus->cpus[n].props.socket_id > socket_id);
>> + socket_id = cpus->cpus[n].props.socket_id;
>> + 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) {
>> + assert(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) {
>> + 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) {
>> + assert(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 */
> nit: For consistency, maybe "Not a physical package"
Sure. Will do in v8.
>> - 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);
>> }
> For the function of exposing vCPU topology to guest:
> Tested-by: Yanan Wang <wangyanan55@huawei.com>
> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>
Thanks for testing it and your time to review this series.
Thanks,
Gavin
On Wed, 20 Apr 2022 18:49:09 +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 IDs in
> ms->possible_cpus. Currently, the only user of build_pptt() is
> arm/virt machine.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
Looks fine to me, so
Acked-by: Igor Mammedov <imammedo@redhat.com>
Also do we have a bios-tables tests that watches over/tests PPTT table?
if not please add one as a patch on top.
> ---
> hw/acpi/aml-build.c | 109 +++++++++++++++++++-------------------------
> 1 file changed, 47 insertions(+), 62 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 4086879ebf..73f4e69c29 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2002,86 +2002,71 @@ 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 = 0, cluster_offset = 0, core_offset = 0;
> + 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);
> - }
> + /*
> + * 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.
> + */
> + for (n = 0; n < cpus->len; n++) {
> + if (cpus->cpus[n].props.socket_id != socket_id) {
> + assert(cpus->cpus[n].props.socket_id > socket_id);
> + socket_id = cpus->cpus[n].props.socket_id;
> + 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) {
> + assert(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) {
> + 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) {
> + assert(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);
> }
>
Hi Igor,
On 4/20/22 10:56 PM, Igor Mammedov wrote:
> On Wed, 20 Apr 2022 18:49:09 +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 IDs in
>> ms->possible_cpus. Currently, the only user of build_pptt() is
>> arm/virt machine.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>
> Looks fine to me, so
>
> Acked-by: Igor Mammedov <imammedo@redhat.com>
>
>
> Also do we have a bios-tables tests that watches over/tests PPTT table?
> if not please add one as a patch on top.
>
Thanks a lot for your continuous reviews. We won't have PPTT table included
in bios-table tests. I will add one after this series is merged because
our downstream release needs this series :)
>> ---
>> hw/acpi/aml-build.c | 109 +++++++++++++++++++-------------------------
>> 1 file changed, 47 insertions(+), 62 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 4086879ebf..73f4e69c29 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2002,86 +2002,71 @@ 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 = 0, cluster_offset = 0, core_offset = 0;
>> + 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);
>> - }
>> + /*
>> + * 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.
>> + */
>> + for (n = 0; n < cpus->len; n++) {
>> + if (cpus->cpus[n].props.socket_id != socket_id) {
>> + assert(cpus->cpus[n].props.socket_id > socket_id);
>> + socket_id = cpus->cpus[n].props.socket_id;
>> + 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) {
>> + assert(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) {
>> + 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) {
>> + assert(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
© 2016 - 2026 Red Hat, Inc.