Re: [PATCH v8 4/6] hw/acpi/aml-build.c: add cache hierarchy to pptt table

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Mon, 10 Mar 2025 16:23:35 +0000
Alireza Sanaee <alireza.sanaee@huawei.com> wrote:

> Add cache topology to PPTT table. With this patch, both ACPI PPTT table
> and device tree will represent the same cache topology given users
> input.
> 
> Signed-off-by: Alireza Sanaee <alireza.sanaee@huawei.com>
> Co-developed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Hi Ali,

A few trivial style things. You'll need to rebase after 10.0 is out
anyway, so just tidy these up when you do that.

Jonathan

> ---
>  hw/acpi/aml-build.c            | 205 ++++++++++++++++++++++++++++++++-
>  hw/arm/virt-acpi-build.c       |   8 +-
>  hw/loongarch/virt-acpi-build.c |   2 +-
>  include/hw/acpi/aml-build.h    |   4 +-
>  include/hw/cpu/core.h          |   1 +
>  5 files changed, 212 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index e5401dfdb1a8..e2568522660d 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
...

>  /*
>   * ACPI spec, Revision 6.3
>   * 5.2.29.1 Processor hierarchy node structure (Type 0)
> @@ -2146,15 +2247,24 @@ void build_spcr(GArray *table_data, BIOSLinker *linker,
>   * 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)
> +                const char *oem_id, const char *oem_table_id,
...

> @@ -2184,11 +2294,35 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>              socket_id = cpus->cpus[n].props.socket_id;
>              cluster_id = -1;
>              core_id = -1;
> +            bottom_node = top_node;
> +            num_priv = 0;
> +            if (cache_described_at(ms, CPU_TOPOLOGY_LEVEL_SOCKET) &&
> +                find_the_lowest_level_cache_defined_at_level(
> +                    ms,
> +                    &bottom_node,
> +                    CPU_TOPOLOGY_LEVEL_SOCKET))
> +            {
As below.

> +                build_caches(table_data, pptt_start,
> +                             num_caches, caches,
> +                             n, top_node, bottom_node,
> +                             &node_data_offset, &node_instr_offset);
> +
> +                priv_rsrc[0] = node_instr_offset;
> +                priv_rsrc[1] = node_data_offset;
> +
> +                if (node_instr_offset || node_data_offset) {
> +                    num_priv = node_instr_offset == node_data_offset ? 1 : 2;
> +                }
> +
> +                top_cluster = bottom_node - 1;
> +            }
> +
>              socket_offset = table_data->len - pptt_start;
>              build_processor_hierarchy_node(table_data,
>                  (1 << 0) | /* Physical package */
>                  (1 << 4), /* Identical Implementation */
> -                root_offset, socket_id, NULL, 0);
> +                root_offset, socket_id,
> +                priv_rsrc, num_priv);
>          }
>  
>          if (mc->smp_props.clusters_supported && mc->smp_props.has_clusters) {
> @@ -2196,21 +2330,81 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>                  assert(cpus->cpus[n].props.cluster_id > cluster_id);
>                  cluster_id = cpus->cpus[n].props.cluster_id;
>                  core_id = -1;
> +                bottom_cluster = top_cluster;
> +                num_priv = 0;
> +
> +                if (cache_described_at(ms, CPU_TOPOLOGY_LEVEL_CLUSTER) &&
> +                    find_the_lowest_level_cache_defined_at_level(
> +                        ms,
> +                        &bottom_cluster,
> +                        CPU_TOPOLOGY_LEVEL_CLUSTER))
> +                {

As below, style not correct.  Please check for other cases.

> +
> +                    build_caches(table_data, pptt_start,
> +                                 num_caches, caches, n, top_cluster,
> +                                 bottom_cluster, &cluster_data_offset,
> +                                 &cluster_instr_offset);
> +
> +                    priv_rsrc[0] = cluster_instr_offset;
> +                    priv_rsrc[1] = cluster_data_offset;
> +
> +                    if (cluster_instr_offset || cluster_data_offset) {
> +                        num_priv =
> +                        cluster_instr_offset == cluster_data_offset ? 1 : 2;
> +                    }
> +
> +                    top_core = bottom_cluster - 1;
> +                } else if (top_cluster == bottom_node - 1) {
> +                    /* socket cache but no cluster cache */
> +                    top_core = bottom_node - 1;
> +                }

...

> +        if (cpus->cpus[n].props.core_id != core_id) {
> +            bottom_core = top_core;
> +            num_priv = 0;
> +
> +            if (cache_described_at(ms, CPU_TOPOLOGY_LEVEL_CORE) &&
> +                find_the_lowest_level_cache_defined_at_level(
> +                    ms,
> +                    &bottom_core,
> +                    CPU_TOPOLOGY_LEVEL_CORE))
> +            {

Trivial but that's not qemu coding style.  Bracket needs to go on end of
line above.

> +                build_caches(table_data, pptt_start,
> +                             num_caches, caches,
> +                             n, top_core, bottom_core,
> +                             &core_data_offset, &core_instr_offset);
> +
> +                priv_rsrc[0] = core_instr_offset;
> +                priv_rsrc[1] = core_data_offset;
> +
> +                num_priv = core_instr_offset == core_data_offset ? 1 : 2;
> +            }
> +        }
> +
> +

One blank line enough.

>          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);
> +                cluster_offset, n,
> +                priv_rsrc, num_priv);