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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Sep 22, 2022 at 09:11:40PM +0800, 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.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

This is an example of a commit log repeating what the patch does.
Which is ok but the important thing is to explain the motivation -
why is it a bug to generate a cluster node without '-smp clusters'?


> ---
>  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;
>  
>  /**
> -- 
> 2.24.0