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 <ehabk...@redhat.com>
> Sent: Thursday, August 1, 2019 2:29 PM
> To: Moger, Babu <babu.mo...@amd.com>
> Cc: marcel.apfelb...@gmail.com; m...@redhat.com; pbonz...@redhat.com;
> r...@twiddle.net; imamm...@redhat.com; qemu-devel@nongnu.org
> 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 <babu.mo...@amd.com>
> > ---
> [...]
> > @@ -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
