Re: [Qemu-devel] [RFC 2 PATCH 08/16] i386: Cleanup and use the new epyc mode topology functions

[Eduardo Habkost]

On Fri, Sep 06, 2019 at 07:12:33PM +0000, Moger, Babu wrote:
> Use the new epyc mode functions and delete the unused code.
> 
> Signed-off-by: Babu Moger <address@hidden>
> ---
>  target/i386/cpu.c |  171 
> +++++++++++++++--------------------------------------
>  1 file changed, 48 insertions(+), 123 deletions(-)
> 
[...]
>  /* Encode cache info for CPUID[8000001E] */
> -static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
> -                                       uint32_t *eax, uint32_t *ebx,
> -                                       uint32_t *ecx, uint32_t *edx)
> +static void encode_topo_cpuid8000001e(CPUX86State *env,
> +                                      uint32_t *eax, uint32_t *ebx,
> +                                      uint32_t *ecx, uint32_t *edx)
>  {
> -    struct core_topology topo = {0};
> -    unsigned long nodes;
> -    int shift;
> +    X86CPUTopoIDs topo_ids = { 0 };
> +    unsigned long nodes, shift;
> +    X86CPU *cpu = env_archcpu(env);
> +    CPUState *cs = env_cpu(env);
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    X86CPUTopoInfo topo_info = {
> +        .numa_nodes = nb_numa_nodes,
> +        .nr_sockets = ms->smp.sockets,
> +        .nr_cores = ms->smp.cores,
> +        .nr_threads = ms->smp.threads,
> +    };
> +
> +    nodes = nodes_in_pkg(&topo_info);
> +    x86_topo_ids_from_idx_epyc(&topo_info, cs->cpu_index, &topo_ids);

You probably want to use x86_topo_ids_from_apicid() instead.
cpu_index is a deprecated identifier for a CPU, and may not match
the actual CPU topology if using CPU hotplug.

>  
> -    build_core_topology(cs->nr_cores, cpu->core_id, &topo);
>      *eax = cpu->apic_id;
> +
>      /*
>       * CPUID_Fn8000001E_EBX
>       * 31:16 Reserved
> @@ -496,11 +421,12 @@ static void encode_topo_cpuid8000001e(CPUState *cs, 
> X86CPU *cpu,
>       *             3 Core complex id
>       *           1:0 Core id
>       */
> -    if (cs->nr_threads - 1) {
> -        *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
> -                (topo.ccx_id << 2) | topo.core_id;
> +    if (topo_info.nr_threads - 1) {
> +        *ebx = ((topo_info.nr_threads - 1) << 8) | (topo_ids.node_id << 3) |
> +                (topo_ids.ccx_id << 2) | topo_ids.core_id;
>      } else {
> -        *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
> +        *ebx = (topo_ids.node_id << 4) | (topo_ids.ccx_id << 3) |
> +                topo_ids.core_id;

It's nice to see we are deleting old code and reusing the same
topology functions.  However, I'm worried that the relationship
between the topology IDs in APIC ID (EAX) and the topology IDs in
EBX and ECX isn't clear.

I'm also worried there's no code enforcing a limit for
node_id/ccx_id/core_id to ensure they fit in the fields defined
above.  These hardcoded limits will become painful to maintain
once we have new CPUs with different constraints.  We need to
make the limits explicit in the code, and also be prepared for
them to change in the future depending on the CPU model.

>      }
>      /*
>       * CPUID_Fn8000001E_ECX
> @@ -510,9 +436,8 @@ static void encode_topo_cpuid8000001e(CPUState *cs, 
> X86CPU *cpu,
>       *         2  Socket id
>       *       1:0  Node id
>       */
> -    if (topo.num_nodes <= 4) {
> -        *ecx = ((topo.num_nodes - 1) << 8) | (cpu->socket_id << 2) |
> -                topo.node_id;
> +    if (nodes <= 4) {
> +        *ecx = ((nodes - 1) << 8) | (topo_ids.pkg_id << 2) | 
> topo_ids.node_id;
>      } else {
>          /*
>           * Node id fix up. Actual hardware supports up to 4 nodes. But with
> @@ -527,12 +452,12 @@ static void encode_topo_cpuid8000001e(CPUState *cs, 
> X86CPU *cpu,
>           * number of nodes. find_last_bit returns last set bit(0 based). Left
>           * shift(+1) the socket id to represent all the nodes.
>           */
> -        nodes = topo.num_nodes - 1;
> +        nodes = nodes - 1;
>          shift = find_last_bit(&nodes, 8);
> -        *ecx = ((topo.num_nodes - 1) << 8) | (cpu->socket_id << (shift + 1)) 
> |
> -                topo.node_id;
> +        *ecx = (nodes << 8) | (topo_ids.pkg_id << (shift + 1)) | 
> topo_ids.node_id;

Like above, we also need to ensure pkg_id and node_id are withing
reasonable limits, somewhere in the code, and that the limits can
change in the future depending on the CPU model.

>      }
>      *edx = 0;
> +
>  }
>  
>  /*
> @@ -4580,19 +4505,19 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> uint32_t count,
>          }
>          switch (count) {
>          case 0: /* L1 dcache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache, cs,
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache,
>                                         eax, ebx, ecx, edx);
>              break;
>          case 1: /* L1 icache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache, cs,
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache,
>                                         eax, ebx, ecx, edx);
>              break;
>          case 2: /* L2 cache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache, cs,
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache,
>                                         eax, ebx, ecx, edx);
>              break;
>          case 3: /* L3 cache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache, cs,
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache,
>                                         eax, ebx, ecx, edx);
>              break;
>          default: /* end of info */
> @@ -4602,7 +4527,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> uint32_t count,
>          break;
>      case 0x8000001E:
>          assert(cpu->core_id <= 255);
> -        encode_topo_cpuid8000001e(cs, cpu,
> +        encode_topo_cpuid8000001e(env,
>                                    eax, ebx, ecx, edx);
>          break;
>      case 0xC0000000:
> 

-- 
Eduardo