Re: [Qemu-devel] [RFC PATCH 4/5] hw/i386: Generate apicid based on cpu_type

[Eduardo Habkost]

Thanks for the patches.

I still haven't looked closely at all patches in the series, but
patches 1-3 seem good on the first look.  A few comments on this
one:

On Wed, Jul 31, 2019 at 11:20:50PM +0000, Moger, Babu wrote:
> Check the cpu_type before calling the apicid functions
> from topology.h.
> 
> Signed-off-by: Babu Moger <address@hidden>
> ---
[...]
> @@ -2437,16 +2478,26 @@ static void pc_cpu_pre_plug(HotplugHandler 
> *hotplug_dev,
>          topo.die_id = cpu->die_id;
>          topo.core_id = cpu->core_id;
>          topo.smt_id = cpu->thread_id;
> -        cpu->apic_id = apicid_from_topo_ids(pcms->smp_dies, smp_cores,
> -                                            smp_threads, &topo);
> +     if (!strncmp(ms->cpu_type, "EPYC", 4)) {

Please don't add semantics to the CPU type name.  If you want
some behavior to be configurable per CPU type, please do it at
the X86CPUDefinition struct.

In this specific case, maybe the new APIC ID calculation code
could
be conditional on:
  (vendor == AMD) && (env->features[...] & TOPOEXT).

Also, we must keep compatibility with the old APIC ID calculation
code on older machine types.  We need a compatibility flag to
enable the existing APIC ID calculation.


> +            x86_topo_ids_from_idx_epyc(nb_numa_nodes, smp_sockets, smp_cores,
> +                                       smp_threads, idx, &topo);
> +            cpu->apic_id = apicid_from_topo_ids_epyc(smp_cores, smp_threads,
> +                                                     &topo);
> +     } else

There's a tab character here.  Please use spaces instead of tabs.

> +            cpu->apic_id = apicid_from_topo_ids(pcms->smp_dies, smp_cores,
> +                                                smp_threads, &topo);

I see you are duplicating very similar logic in 3 different
places, to call apicid_from_topo_ids() and
x86_topo_ids_from_apicid().

Also, apicid_from_topo_ids() and x86_topo_ids_from_apicid() have very generic
names, and people could call them expecting them to work for every CPU model
(which they don't).  This makes the topology API very easy to misuse.

Why don't we make the existing generic
apicid_from_topo_ids()/x86_topo_ids_from_apicid() functions work
on all cases?  If they need additional input to handle EPYC and
call EPYC-specific functions, we can make them get additional
arguments.  This way we'll be sure that we'll never call the
wrong implementation by accident.

This might make the list of arguments for
x86_topo_ids_from_apicid() and apicid_from_topo_ids() become
large.  We can address this by making them get a CpuTopology
argument.


In other words, the API could look like this:


static inline apic_id_t apicid_from_topo_ids(const X86CPUTopology *topo,
                                             const X86CPUTopologyIds *ids)
{
    if (topo->epyc_mode) {
        return apicid_from_topo_ids_epyc(topo, ids);
    }

    /* existing QEMU 4.1 logic: */
    return (ids->pkg_id  << apicid_pkg_offset(topo)) |
           (ids->die_id  << apicid_die_offset(topo)) |
           (ids->core_id << apicid_core_offset(topo)) |
           ids->smt_id;
}

static inline void x86_topo_ids_from_apicid(apic_id_t apicid,
                                            const X86CPUTopology *topo,
                                            X86CPUTopologyIds *ids)
{
    if (topo->epyc_mode) {
        x86_topo_ids_from_apicid_epyc(apicid, topo, ids);
        return;
    }

    /* existing QEMU 4.1 logic: */
    ids->smt_id =
            apicid &
            ~(0xFFFFFFFFUL << apicid_smt_width(topo));
    ids->core_id =
            (apicid >> apicid_core_offset(topo)) &
            ~(0xFFFFFFFFUL << apicid_core_width(topo));
    ids->die_id =
            (apicid >> apicid_die_offset(topo)) &
            ~(0xFFFFFFFFUL << apicid_die_width(topo));
    ids->pkg_id = apicid >> apicid_pkg_offset(topo);
}

 
>      }
>  
>      cpu_slot = pc_find_cpu_slot(MACHINE(pcms), cpu->apic_id, &idx);
>      if (!cpu_slot) {
>          MachineState *ms = MACHINE(pcms);
>  
> -        x86_topo_ids_from_apicid(cpu->apic_id, pcms->smp_dies,
> -                                 smp_cores, smp_threads, &topo);
> +        if(!strncmp(ms->cpu_type, "EPYC", 4))
> +            x86_topo_ids_from_apicid_epyc(cpu->apic_id, pcms->smp_dies,
> +                                          smp_cores, smp_threads, &topo);
> +        else
> +            x86_topo_ids_from_apicid(cpu->apic_id, pcms->smp_dies,
> +                                     smp_cores, smp_threads, &topo);
>          error_setg(errp,
>              "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
>              " APIC ID %" PRIu32 ", valid index range 0:%d",
> @@ -2874,10 +2925,18 @@ static const CPUArchIdList 
> *pc_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(pcms, i);
> -        x86_topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id,
> -                                 pcms->smp_dies, ms->smp.cores,
> -                                 ms->smp.threads, &topo);
> +     if(!strncmp(ms->cpu_type, "EPYC", 4)) {
> +            ms->possible_cpus->cpus[i].arch_id =
> +                            x86_cpu_apic_id_from_index_epyc(pcms, i);
> +            x86_topo_ids_from_apicid_epyc(ms->possible_cpus->cpus[i].arch_id,
> +                                          pcms->smp_dies, ms->smp.cores,
> +                                       ms->smp.threads, &topo);
> +     } else {
> +            ms->possible_cpus->cpus[i].arch_id = 
> x86_cpu_apic_id_from_index(pcms, i);
> +            x86_topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id,
> +                                     pcms->smp_dies, ms->smp.cores,
> +                                     ms->smp.threads, &topo);
> +     }
>          ms->possible_cpus->cpus[i].props.has_socket_id = true;
>          ms->possible_cpus->cpus[i].props.socket_id = topo.pkg_id;
>          ms->possible_cpus->cpus[i].props.has_die_id = true;
> -- 
> 2.20.1
> 

-- 
Eduardo