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

[Moger, Babu]

Hi Eduardo,
  Thanks for the quick comments. I will look into your comments closely
and will let you know if I have questions.

> -----Original Message-----
> From: Eduardo Habkost <address@hidden>
> Sent: Thursday, August 1, 2019 2:29 PM
> To: Moger, Babu <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden
> Subject: Re: [RFC PATCH 4/5] hw/i386: Generate apicid based on cpu_type
> 
> 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