| Message ID | 157541992659.46157.18191224973398213624.stgit@naples-babu.amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | APIC ID fixes for AMD EPYC CPU models | expand |
Hi, Sorry for taking so long. I was away from the office for a month, and now I'm finally back. On Tue, Dec 03, 2019 at 06:38:46PM -0600, Babu Moger wrote: > Introduce following handlers for new epyc mode. > x86_apicid_from_cpu_idx_epyc: Generate apicid from cpu index. > x86_topo_ids_from_apicid_epyc: Generate topo ids from apic id. > x86_apicid_from_topo_ids_epyc: Generate apicid from topo ids. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > hw/i386/pc.c | 12 ++++++++++++ > include/hw/i386/topology.h | 4 ++-- > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index e6c8a458e7..64e3658873 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -2819,6 +2819,17 @@ static bool pc_hotplug_allowed(MachineState *ms, DeviceState *dev, Error **errp) > return true; > } > > +static void pc_init_apicid_fn(MachineState *ms) > +{ > + PCMachineState *pcms = PC_MACHINE(ms); > + > + if (!strncmp(ms->cpu_type, "EPYC", 4)) { Please never use string comparison to introduce device-specific behavior. I had already pointed this out at https://lore.kernel.org/qemu-devel/20190801192830.GD20035@habkost.net/ If you need a CPU model to provide special behavior, you have two options: * Add a method pointer to X86CPUClass and/or X86CPUDefinition * Add a QOM property to enable/disable special behavior, and include the property in the CPU model definition. The second option might be preferable long term, but might require more work because the property would become visible in query-cpu-model-expansion and in the command line. The first option may be acceptable to avoid extra user-visible complexity in the first version. > + pcms->apicid_from_cpu_idx = x86_apicid_from_cpu_idx_epyc; > + pcms->topo_ids_from_apicid = x86_topo_ids_from_apicid_epyc; > + pcms->apicid_from_topo_ids = x86_apicid_from_topo_ids_epyc; Why do you need to override the function pointers in PCMachineState instead of just looking up the relevant info at X86CPUClass? If both machine-types and CPU models are supposed to override the APIC ID calculation functions, the interaction between machine-type and CPU model needs to be better documented (preferably with simple test cases) to ensure we won't break compatibility later. > + } > +} > + > static void pc_machine_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > @@ -2847,6 +2858,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > mc->cpu_index_to_instance_props = pc_cpu_index_to_props; > mc->get_default_cpu_node_id = pc_get_default_cpu_node_id; > mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids; > + mc->init_apicid_fn = pc_init_apicid_fn; > mc->auto_enable_numa_with_memhp = true; > mc->has_hotpluggable_cpus = true; > mc->default_boot_order = "cad"; > diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h > index b2b9e93a06..f028d2332a 100644 > --- a/include/hw/i386/topology.h > +++ b/include/hw/i386/topology.h > @@ -140,7 +140,7 @@ static inline unsigned apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info) > * > * The caller must make sure core_id < nr_cores and smt_id < nr_threads. > */ > -static inline apic_id_t apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > +static inline apic_id_t x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > const X86CPUTopoIDs *topo_ids) > { > return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > @@ -200,7 +200,7 @@ static inline apic_id_t x86_apicid_from_cpu_idx_epyc(X86CPUTopoInfo *topo_info, > { > X86CPUTopoIDs topo_ids; > x86_topo_ids_from_idx_epyc(topo_info, cpu_index, &topo_ids); > - return apicid_from_topo_ids_epyc(topo_info, &topo_ids); > + return x86_apicid_from_topo_ids_epyc(topo_info, &topo_ids); > } > /* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID > * > >
On 1/28/20 2:04 PM, Eduardo Habkost wrote: > Hi, > > Sorry for taking so long. I was away from the office for a > month, and now I'm finally back. no worries. > > On Tue, Dec 03, 2019 at 06:38:46PM -0600, Babu Moger wrote: >> Introduce following handlers for new epyc mode. >> x86_apicid_from_cpu_idx_epyc: Generate apicid from cpu index. >> x86_topo_ids_from_apicid_epyc: Generate topo ids from apic id. >> x86_apicid_from_topo_ids_epyc: Generate apicid from topo ids. >> >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> --- >> hw/i386/pc.c | 12 ++++++++++++ >> include/hw/i386/topology.h | 4 ++-- >> 2 files changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index e6c8a458e7..64e3658873 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -2819,6 +2819,17 @@ static bool pc_hotplug_allowed(MachineState *ms, DeviceState *dev, Error **errp) >> return true; >> } >> >> +static void pc_init_apicid_fn(MachineState *ms) >> +{ >> + PCMachineState *pcms = PC_MACHINE(ms); >> + >> + if (!strncmp(ms->cpu_type, "EPYC", 4)) { > > Please never use string comparison to introduce device-specific > behavior. I had already pointed this out at Yes. you did mention before. I was not sure how to achieve without comparing the model string > > If you need a CPU model to provide special behavior, > you have two options: > > * Add a method pointer to X86CPUClass and/or X86CPUDefinition > * Add a QOM property to enable/disable special behavior, and > include the property in the CPU model definition. > > The second option might be preferable long term, but might > require more work because the property would become visible in > query-cpu-model-expansion and in the command line. The first > option may be acceptable to avoid extra user-visible complexity > in the first version. Yes. We need to have a special behavior for specific model. I will look at both these above approaches closely. Challenge is this needs to be done much early in the initialization(before parse_numa_opts or machine_run_board_init). Will research more on this. > > > >> + pcms->apicid_from_cpu_idx = x86_apicid_from_cpu_idx_epyc; >> + pcms->topo_ids_from_apicid = x86_topo_ids_from_apicid_epyc; >> + pcms->apicid_from_topo_ids = x86_apicid_from_topo_ids_epyc; > > Why do you need to override the function pointers in > PCMachineState instead of just looking up the relevant info at > X86CPUClass? > > If both machine-types and CPU models are supposed to override the > APIC ID calculation functions, the interaction between > machine-type and CPU model needs to be better documented > (preferably with simple test cases) to ensure we won't break > compatibility later. > >> + } >> +} >> + >> static void pc_machine_class_init(ObjectClass *oc, void *data) >> { >> MachineClass *mc = MACHINE_CLASS(oc); >> @@ -2847,6 +2858,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) >> mc->cpu_index_to_instance_props = pc_cpu_index_to_props; >> mc->get_default_cpu_node_id = pc_get_default_cpu_node_id; >> mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids; >> + mc->init_apicid_fn = pc_init_apicid_fn; >> mc->auto_enable_numa_with_memhp = true; >> mc->has_hotpluggable_cpus = true; >> mc->default_boot_order = "cad"; >> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h >> index b2b9e93a06..f028d2332a 100644 >> --- a/include/hw/i386/topology.h >> +++ b/include/hw/i386/topology.h >> @@ -140,7 +140,7 @@ static inline unsigned apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info) >> * >> * The caller must make sure core_id < nr_cores and smt_id < nr_threads. >> */ >> -static inline apic_id_t apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, >> +static inline apic_id_t x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, >> const X86CPUTopoIDs *topo_ids) >> { >> return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | >> @@ -200,7 +200,7 @@ static inline apic_id_t x86_apicid_from_cpu_idx_epyc(X86CPUTopoInfo *topo_info, >> { >> X86CPUTopoIDs topo_ids; >> x86_topo_ids_from_idx_epyc(topo_info, cpu_index, &topo_ids); >> - return apicid_from_topo_ids_epyc(topo_info, &topo_ids); >> + return x86_apicid_from_topo_ids_epyc(topo_info, &topo_ids); >> } >> /* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID >> * >> >> >
On Tue, Jan 28, 2020 at 03:48:15PM -0600, Babu Moger wrote: > On 1/28/20 2:04 PM, Eduardo Habkost wrote: [...] > > If you need a CPU model to provide special behavior, > > you have two options: > > > > * Add a method pointer to X86CPUClass and/or X86CPUDefinition > > * Add a QOM property to enable/disable special behavior, and > > include the property in the CPU model definition. > > > > The second option might be preferable long term, but might > > require more work because the property would become visible in > > query-cpu-model-expansion and in the command line. The first > > option may be acceptable to avoid extra user-visible complexity > > in the first version. > > Yes. We need to have a special behavior for specific model. > I will look at both these above approaches closely. Challenge is this > needs to be done much early in the initialization(before parse_numa_opts > or machine_run_board_init). Will research more on this. You should be able to look up the requested CPU model using object_class_by_name(machine->cpu_type). If you do this inside x86-specific code before calling apicid_from_cpu_idx/topo_ids_from_apicid/apicid_from_topo_ids, you probably won't need a init_apicid_fn hook.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index e6c8a458e7..64e3658873 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -2819,6 +2819,17 @@ static bool pc_hotplug_allowed(MachineState *ms, DeviceState *dev, Error **errp) return true; } +static void pc_init_apicid_fn(MachineState *ms) +{ + PCMachineState *pcms = PC_MACHINE(ms); + + if (!strncmp(ms->cpu_type, "EPYC", 4)) { + pcms->apicid_from_cpu_idx = x86_apicid_from_cpu_idx_epyc; + pcms->topo_ids_from_apicid = x86_topo_ids_from_apicid_epyc; + pcms->apicid_from_topo_ids = x86_apicid_from_topo_ids_epyc; + } +} + static void pc_machine_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); @@ -2847,6 +2858,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) mc->cpu_index_to_instance_props = pc_cpu_index_to_props; mc->get_default_cpu_node_id = pc_get_default_cpu_node_id; mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids; + mc->init_apicid_fn = pc_init_apicid_fn; mc->auto_enable_numa_with_memhp = true; mc->has_hotpluggable_cpus = true; mc->default_boot_order = "cad"; diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h index b2b9e93a06..f028d2332a 100644 --- a/include/hw/i386/topology.h +++ b/include/hw/i386/topology.h @@ -140,7 +140,7 @@ static inline unsigned apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info) * * The caller must make sure core_id < nr_cores and smt_id < nr_threads. */ -static inline apic_id_t apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, +static inline apic_id_t x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, const X86CPUTopoIDs *topo_ids) { return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | @@ -200,7 +200,7 @@ static inline apic_id_t x86_apicid_from_cpu_idx_epyc(X86CPUTopoInfo *topo_info, { X86CPUTopoIDs topo_ids; x86_topo_ids_from_idx_epyc(topo_info, cpu_index, &topo_ids); - return apicid_from_topo_ids_epyc(topo_info, &topo_ids); + return x86_apicid_from_topo_ids_epyc(topo_info, &topo_ids); } /* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID *
Introduce following handlers for new epyc mode. x86_apicid_from_cpu_idx_epyc: Generate apicid from cpu index. x86_topo_ids_from_apicid_epyc: Generate topo ids from apic id. x86_apicid_from_topo_ids_epyc: Generate apicid from topo ids. Signed-off-by: Babu Moger <babu.moger@amd.com> --- hw/i386/pc.c | 12 ++++++++++++ include/hw/i386/topology.h | 4 ++-- 2 files changed, 14 insertions(+), 2 deletions(-)