Re: [PATCH v5 14/16] hw/i386: Move arch_id decode inside x86_cpus_init

From: Igor Mammedov <imammedo@redhat.com>
To: Babu Moger <babu.moger@amd.com>
Cc: ehabkost@redhat.com, mst@redhat.com, qemu-devel@nongnu.org,
	pbonzini@redhat.com, rth@twiddle.net
Subject: Re: [PATCH v5 14/16] hw/i386: Move arch_id decode inside x86_cpus_init
Date: Mon, 9 Mar 2020 16:21:23 +0100	[thread overview]
Message-ID: <20200309162123.5ab6a750@Igors-MacBook-Pro> (raw)
In-Reply-To: <158326550403.40452.15934956681175349815.stgit@naples-babu.amd.com>

On Tue, 03 Mar 2020 13:58:24 -0600
Babu Moger <babu.moger@amd.com> wrote:

> Apicid calculation depends on knowing the total number of numa nodes
> for EPYC cpu models. Right now, we are calculating the arch_id while
> parsing the numa(parse_numa). At this time, it is not known how many
> total numa nodes are configured in the system.
> 
> Move the arch_id inside x86_cpus_init. At this time smp parameter is already
> completed and numa node information is available.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  hw/i386/x86.c |   17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index d46dd4ad9e..66998b065c 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -121,6 +121,9 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
>      MachineState *ms = MACHINE(x86ms);
>      MachineClass *mc = MACHINE_GET_CLASS(x86ms);
>  
> +    /* Initialize apicid handlers first */
> +    cpu_x86_init_apicid_fns(ms);
> +
>      x86_cpu_set_default_version(default_cpu_version);
>  
>      /*
> @@ -134,6 +137,12 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
>      x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
>                                                        ms->smp.max_cpus - 1) + 1;
>      possible_cpus = mc->possible_cpu_arch_ids(ms);
> +
> +    for (i = 0; i < ms->smp.cpus; i++) {
> +        ms->possible_cpus->cpus[i].arch_id =
> +            x86_cpu_apic_id_from_index(x86ms, i);
> +    }
> +
>      for (i = 0; i < ms->smp.cpus; i++) {
>          x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal);
>      }
> @@ -158,8 +167,7 @@ int64_t x86_get_default_cpu_node_id(const MachineState *ms, int idx)
>     init_topo_info(&topo_info, x86ms);
>  
>     assert(idx < ms->possible_cpus->len);
> -   x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id,
> -                               &topo_info, &topo_ids);
> +   x86_topo_ids_from_idx(&topo_info, idx, &topo_ids);
not necessary if default x86ms->topo_ids_from_apicid were initialized from x86 machine class

I also wonder if this default contraption we have is going to work
in case of EPYC cpu (i.e. is would generate valid nodeids).

Bot instead of than trying to fix it if it's broken,
I'd rather deprecate and drop get_default_cpu_node_id() requiring users
to explicitly define CPU mapping to numa nodes.
That would be consistent with req for explicit RAM for numa nodes
(postponed till 5.1 due to libvirt not being ready),
i.e if one wants numa, one should explicitly provide necessary mapping
or machine won't start.

>     return topo_ids.pkg_id % ms->numa_state->num_nodes;
>  }
>  
> @@ -193,10 +201,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
>  
>          ms->possible_cpus->cpus[i].type = ms->cpu_type;
>          ms->possible_cpus->cpus[i].vcpus_count = 1;
> -        ms->possible_cpus->cpus[i].arch_id =
> -            x86_cpu_apic_id_from_index(x86ms, i);
> -        x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id,
> -                                 &topo_info, &topo_ids);
> +        x86_topo_ids_from_idx(&topo_info, i, &topo_ids);
ditto

>          ms->possible_cpus->cpus[i].props.has_socket_id = true;
>          ms->possible_cpus->cpus[i].props.socket_id = topo_ids.pkg_id;
>          if (x86ms->smp_dies > 1) {
> 