Re: [PATCH 1/4] hw/acpi/aml-build: Only generate cluster node in PPTT when specified

[wangyanan (Y)]

Hi Yicong,

On 2022/9/22 21:11, Yicong Yang wrote:
> From: Yicong Yang<yangyicong@hisilicon.com>
>
> Currently we'll always generate a cluster node no matter user has
> specified '-smp clusters=X' or not. Cluster is an optional level
> and it's unncessary to build it if user don't need. So only generate
> it when user specify explicitly.
>
> Also update the test ACPI tables.
It would be much more helpful to explain the problem you
have met in practice without this patch. (maybe have some
description or a link of the issue in the cover-letter if we
need a v2).

In qemu which behaves as like a firmware vendor for VM,
the ACPI PPTT is built based on the topology info produced
by machine_parse_smp_config(). And machine_parse_smp_config
will always calculate a complete topology hierarchy using its
algorithm, if the user gives an incomplete -smp CLI.

I think there are two options for us to chose:
1) approach described in this patch
2) qemu will always generate a full topology hierarchy in PPTT
with all the topo members it currently supports. While users
need to consider the necessity to use an incomplete -smp or
an complete one according to their specific scenario, and
should be aware of the kernel behavior resulted from the
config.

There is some Doc for users to explain how qemu will
parse user-specified -smp in [1].
[1] https://www.mankier.com/1/qemu#Options

Thanks,
Yanan
> Signed-off-by: Yicong Yang<yangyicong@hisilicon.com>
> ---
>   hw/acpi/aml-build.c   | 2 +-
>   hw/core/machine-smp.c | 3 +++
>   include/hw/boards.h   | 2 ++
>   3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index e6bfac95c7..aab73af66d 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2030,7 +2030,7 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, 
> MachineState *ms,
>                   0, socket_id, NULL, 0);
>           }
>   
> -        if (mc->smp_props.clusters_supported) {
> +        if (mc->smp_props.clusters_supported && ms->smp.build_cluster) {
>               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;
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index b39ed21e65..5d37e8d07a 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -158,6 +158,9 @@ void machine_parse_smp_config(MachineState *ms,
>       ms->smp.threads = threads;
>       ms->smp.max_cpus = maxcpus;
>   
> +    if (config->has_clusters)
> +        ms->smp.build_cluster = true;
> +
>       /* sanity-check of the computed topology */
>       if (sockets * dies * clusters * cores * threads != maxcpus) {
>           g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 7b416c9787..24aafc213d 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -305,6 +305,7 @@ typedef struct DeviceMemoryState {
>    * @cores: the number of cores in one cluster
>    * @threads: the number of threads in one core
>    * @max_cpus: the maximum number of logical processors on the machine
> + * @build_cluster: build cluster topology or not
>    */
>   typedef struct CpuTopology {
>       unsigned int cpus;
> @@ -314,6 +315,7 @@ typedef struct CpuTopology {
>       unsigned int cores;
>       unsigned int threads;
>       unsigned int max_cpus;
> +    bool build_cluster;
>   } CpuTopology;
>   
>   /**