| Message ID | 20190731232032.51786-5-babu.moger@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | APIC ID fixes for AMD EPYC CPU models | expand |
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.moger@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 >
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 <ehabkost@redhat.com> > Sent: Thursday, August 1, 2019 2:29 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: marcel.apfelbaum@gmail.com; mst@redhat.com; pbonzini@redhat.com; > rth@twiddle.net; imammedo@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.moger@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
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index ef39463fd5..dad55c940f 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -947,6 +947,36 @@ static uint32_t x86_cpu_apic_id_from_index(PCMachineState *pcms, } } +/* Calculates initial APIC ID for a specific CPU index + * + * Currently we need to be able to calculate the APIC ID from the CPU index + * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces have + * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of + * all CPUs up to max_cpus. + */ +static uint32_t x86_cpu_apic_id_from_index_epyc(PCMachineState *pcms, + unsigned int cpu_index) +{ + MachineState *ms = MACHINE(pcms); + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); + uint32_t correct_id; + static bool warned; + + correct_id = x86_apicid_from_cpu_idx_epyc(nb_numa_nodes, ms->smp.sockets, + ms->smp.cores, ms->smp.threads, + cpu_index); + if (pcmc->compat_apic_id_mode) { + if (cpu_index != correct_id && !warned && !qtest_enabled()) { + error_report("APIC IDs set in compatibility mode, " + "CPU topology won't match the configuration"); + warned = true; + } + return cpu_index; + } else { + return correct_id; + } +} + static void pc_build_smbios(PCMachineState *pcms) { uint8_t *smbios_tables, *smbios_anchor; @@ -1619,7 +1649,7 @@ void pc_smp_parse(MachineState *ms, QemuOpts *opts) void pc_hot_add_cpu(MachineState *ms, const int64_t id, Error **errp) { PCMachineState *pcms = PC_MACHINE(ms); - int64_t apic_id = x86_cpu_apic_id_from_index(pcms, id); + int64_t apic_id; Error *local_err = NULL; if (id < 0) { @@ -1627,6 +1657,11 @@ void pc_hot_add_cpu(MachineState *ms, const int64_t id, Error **errp) return; } + if(!strncmp(ms->cpu_type, "EPYC", 4)) + apic_id = x86_cpu_apic_id_from_index_epyc(pcms, id); + else + apic_id = x86_cpu_apic_id_from_index(pcms, id); + if (apic_id >= ACPI_CPU_HOTPLUG_ID_LIMIT) { error_setg(errp, "Unable to add CPU: %" PRIi64 ", resulting APIC ID (%" PRIi64 ") is too large", @@ -1658,8 +1693,13 @@ void pc_cpus_init(PCMachineState *pcms) * * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init(). */ - pcms->apic_id_limit = x86_cpu_apic_id_from_index(pcms, - ms->smp.max_cpus - 1) + 1; + if(!strncmp(ms->cpu_type, "EPYC", 4)) + pcms->apic_id_limit = x86_cpu_apic_id_from_index_epyc(pcms, + ms->smp.max_cpus - 1) + 1; + else + pcms->apic_id_limit = x86_cpu_apic_id_from_index(pcms, + ms->smp.max_cpus - 1) + 1; + possible_cpus = mc->possible_cpu_arch_ids(ms); for (i = 0; i < ms->smp.cpus; i++) { pc_new_cpu(pcms, possible_cpus->cpus[i].arch_id, &error_fatal); @@ -2387,6 +2427,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, PCMachineState *pcms = PC_MACHINE(hotplug_dev); unsigned int smp_cores = ms->smp.cores; unsigned int smp_threads = ms->smp.threads; + unsigned int smp_sockets = ms->smp.sockets; if(!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) { error_setg(errp, "Invalid CPU type, expected cpu type: '%s'", @@ -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)) { + 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 + cpu->apic_id = apicid_from_topo_ids(pcms->smp_dies, smp_cores, + smp_threads, &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;
Check the cpu_type before calling the apicid functions from topology.h. Signed-off-by: Babu Moger <babu.moger@amd.com> --- hw/i386/pc.c | 81 +++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 70 insertions(+), 11 deletions(-)