| Message ID | 158161786628.48948.1813866541108702762.stgit@naples-babu.amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | APIC ID fixes for AMD EPYC CPU model | expand |
On Thu, 13 Feb 2020 12:17:46 -0600 Babu Moger <babu.moger@amd.com> wrote: > Check and Load the apicid handlers from X86CPUDefinition if available. > Update the calling convention for the apicid handlers. Previous and this patch look too complicated for the task at the hand. In particular, cpu_x86_init_apicid_fns() from previous patch adds 1 more reference to Machine into i386/cpu.c (even though it's just a helper function) and I think un-necessary hooks to X86CPUDefinition (it's not really CPU's businesses to make up APIC-IDs). I'd rather do opposite and get rid of the last explicit dependency to ms->smp.cpus from cpu.c. But well, it's out of scope of this series, so for this series I'd just try to avoid adding more Machine dependencies. All 11/16 does is basically using hooks as a switch "I'm EPYC" to set epyc specific encoding topo routines. It could be accomplished by a simple Boolean flag like X86CPUDefinition::use_epyc_apic_id_encoding and then cpu_x86_init_apicid_fns() could be replaced with trivial helper like: x86_use_epyc_apic_id_encoding(char *cpu_type) { X86CPUClass *xcc = ... cpu_type ... return xcc->model->cpudef->use_epyc_apic_id_encoding } then machine could override default[1] hooks using this helper as the trigger x86_cpus_init() { // no need in dedicated function as it's the only instance it's going to be called ever if (x86_use_epyc_apic_id_encoding(ms->cpu_type)) { x86ms->apicid_from_cpu_idx = ...epyc... x86ms->topo_ids_from_apicid = ...epyc... x86ms->apicid_from_topo_ids = ...epyc... x86ms->apicid_pkg_offset = ...epyc... } } That would be less invasive and won't create non necessary dependencies. --- 1) defaults should be set in x86_machine_class_init() Eduardo, what's your take on this? > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > hw/i386/pc.c | 6 +++--- > hw/i386/x86.c | 27 +++++++++++++++++++++++---- > 2 files changed, 26 insertions(+), 7 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index be72a49716..93063af6a8 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1808,14 +1808,14 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > topo_ids.die_id = cpu->die_id; > topo_ids.core_id = cpu->core_id; > topo_ids.smt_id = cpu->thread_id; > - cpu->apic_id = x86_apicid_from_topo_ids(&topo_info, &topo_ids); > + cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); > } > > 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, &topo_info, &topo_ids); > + x86ms->topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids); > error_setg(errp, > "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with" > " APIC ID %" PRIu32 ", valid index range 0:%d", > @@ -1836,7 +1836,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > /* TODO: move socket_id/core_id/thread_id checks into x86_cpu_realizefn() > * once -smp refactoring is complete and there will be CPU private > * CPUState::nr_cores and CPUState::nr_threads fields instead of globals */ > - x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids); > + x86ms->topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids); > if (cpu->socket_id != -1 && cpu->socket_id != topo_ids.pkg_id) { > error_setg(errp, "property socket-id: %u doesn't match set apic-id:" > " 0x%x (socket-id: %u)", cpu->socket_id, cpu->apic_id, topo_ids.pkg_id); > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index 3d944f68e6..b825861b85 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -52,6 +52,22 @@ > /* Physical Address of PVH entry point read from kernel ELF NOTE */ > static size_t pvh_start_addr; > > +/* > + * Check for apicid handlers in X86MachineState. Load them if > + * not loaded already. These handlers are loaded from X86CPUDefinition. > + */ > +static void x86_check_apicid_handlers(MachineState *ms) > +{ > + X86MachineState *x86ms = X86_MACHINE(ms); > + > + if (!x86ms->apicid_from_cpu_idx || > + !x86ms->topo_ids_from_apicid || > + !x86ms->apicid_from_topo_ids || > + !x86ms->apicid_pkg_offset) { > + cpu_x86_init_apicid_fns(ms); > + } > +} > + > /* > * Calculates initial APIC ID for a specific CPU index > * > @@ -70,7 +86,7 @@ uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms, > > init_topo_info(&topo_info, x86ms); > > - correct_id = x86_apicid_from_cpu_idx(&topo_info, cpu_index); > + correct_id = x86ms->apicid_from_cpu_idx(&topo_info, cpu_index); > if (x86mc->compat_apic_id_mode) { > if (cpu_index != correct_id && !warned && !qtest_enabled()) { > error_report("APIC IDs set in compatibility mode, " > @@ -148,8 +164,8 @@ int64_t x86_get_default_cpu_node_id(const MachineState *ms, int idx) > init_topo_info(&topo_info, x86ms); > > assert(idx < ms->possible_cpus->len); > - x86_topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id, > - &topo_info, &topo_ids); > + x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id, > + &topo_info, &topo_ids); > return topo_ids.pkg_id % ms->numa_state->num_nodes; > } > > @@ -169,6 +185,9 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms) > return ms->possible_cpus; > } > > + /* Initialize apicid handlers */ > + x86_check_apicid_handlers(ms); > + > ms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) + > sizeof(CPUArchId) * max_cpus); > ms->possible_cpus->len = max_cpus; > @@ -182,7 +201,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms) > ms->possible_cpus->cpus[i].vcpus_count = 1; > ms->possible_cpus->cpus[i].arch_id = > x86_cpu_apic_id_from_index(x86ms, i); > - x86_topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id, > + x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id, > &topo_info, &topo_ids); > ms->possible_cpus->cpus[i].props.has_socket_id = true; > ms->possible_cpus->cpus[i].props.socket_id = topo_ids.pkg_id; > >
On 2/24/20 11:19 AM, Igor Mammedov wrote: > On Thu, 13 Feb 2020 12:17:46 -0600 > Babu Moger <babu.moger@amd.com> wrote: > >> Check and Load the apicid handlers from X86CPUDefinition if available. >> Update the calling convention for the apicid handlers. > > Previous and this patch look too complicated for the task at the hand. > In particular, cpu_x86_init_apicid_fns() from previous patch adds 1 more > reference to Machine into i386/cpu.c (even though it's just a helper function) > and I think un-necessary hooks to X86CPUDefinition (it's not really CPU's > businesses to make up APIC-IDs). > > I'd rather do opposite and get rid of the last explicit dependency to > ms->smp.cpus from cpu.c. But well, it's out of scope of this series, > so for this series I'd just try to avoid adding more Machine dependencies. > > All 11/16 does is basically using hooks as a switch "I'm EPYC" to > set epyc specific encoding topo routines. > > It could be accomplished by a simple Boolean flag like > X86CPUDefinition::use_epyc_apic_id_encoding > > and then cpu_x86_init_apicid_fns() could be replaced with trivial > helper like: > > x86_use_epyc_apic_id_encoding(char *cpu_type) > { > X86CPUClass *xcc = ... cpu_type ... > return xcc->model->cpudef->use_epyc_apic_id_encoding > } > > then machine could override default[1] hooks using this helper > as the trigger > x86_cpus_init() > { > // no need in dedicated function as it's the only instance it's going to be called ever > if (x86_use_epyc_apic_id_encoding(ms->cpu_type)) { > x86ms->apicid_from_cpu_idx = ...epyc... > x86ms->topo_ids_from_apicid = ...epyc... > x86ms->apicid_from_topo_ids = ...epyc... > x86ms->apicid_pkg_offset = ...epyc... > } > } > > That would be less invasive and won't create non necessary dependencies. Yes. We can achieve the task here with your approach mentioned above. But, we still will have a scaling issue. In future if a "new cpu model" comes up its own decoding, then we need to add another bolean flag use_new _cpu_apic_id_encoding. And then do that same check again. In that sense, the current approach is bit generic. Lets also hear from Eduardo. > > --- > 1) defaults should be set in x86_machine_class_init() > > Eduardo, what's your take on this? > >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> --- >> hw/i386/pc.c | 6 +++--- >> hw/i386/x86.c | 27 +++++++++++++++++++++++---- >> 2 files changed, 26 insertions(+), 7 deletions(-) >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index be72a49716..93063af6a8 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -1808,14 +1808,14 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, >> topo_ids.die_id = cpu->die_id; >> topo_ids.core_id = cpu->core_id; >> topo_ids.smt_id = cpu->thread_id; >> - cpu->apic_id = x86_apicid_from_topo_ids(&topo_info, &topo_ids); >> + cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); >> } >> >> 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, &topo_info, &topo_ids); >> + x86ms->topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids); >> error_setg(errp, >> "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with" >> " APIC ID %" PRIu32 ", valid index range 0:%d", >> @@ -1836,7 +1836,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, >> /* TODO: move socket_id/core_id/thread_id checks into x86_cpu_realizefn() >> * once -smp refactoring is complete and there will be CPU private >> * CPUState::nr_cores and CPUState::nr_threads fields instead of globals */ >> - x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids); >> + x86ms->topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids); >> if (cpu->socket_id != -1 && cpu->socket_id != topo_ids.pkg_id) { >> error_setg(errp, "property socket-id: %u doesn't match set apic-id:" >> " 0x%x (socket-id: %u)", cpu->socket_id, cpu->apic_id, topo_ids.pkg_id); >> diff --git a/hw/i386/x86.c b/hw/i386/x86.c >> index 3d944f68e6..b825861b85 100644 >> --- a/hw/i386/x86.c >> +++ b/hw/i386/x86.c >> @@ -52,6 +52,22 @@ >> /* Physical Address of PVH entry point read from kernel ELF NOTE */ >> static size_t pvh_start_addr; >> >> +/* >> + * Check for apicid handlers in X86MachineState. Load them if >> + * not loaded already. These handlers are loaded from X86CPUDefinition. >> + */ >> +static void x86_check_apicid_handlers(MachineState *ms) >> +{ >> + X86MachineState *x86ms = X86_MACHINE(ms); >> + >> + if (!x86ms->apicid_from_cpu_idx || >> + !x86ms->topo_ids_from_apicid || >> + !x86ms->apicid_from_topo_ids || >> + !x86ms->apicid_pkg_offset) { >> + cpu_x86_init_apicid_fns(ms); >> + } >> +} >> + >> /* >> * Calculates initial APIC ID for a specific CPU index >> * >> @@ -70,7 +86,7 @@ uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms, >> >> init_topo_info(&topo_info, x86ms); >> >> - correct_id = x86_apicid_from_cpu_idx(&topo_info, cpu_index); >> + correct_id = x86ms->apicid_from_cpu_idx(&topo_info, cpu_index); >> if (x86mc->compat_apic_id_mode) { >> if (cpu_index != correct_id && !warned && !qtest_enabled()) { >> error_report("APIC IDs set in compatibility mode, " >> @@ -148,8 +164,8 @@ int64_t x86_get_default_cpu_node_id(const MachineState *ms, int idx) >> init_topo_info(&topo_info, x86ms); >> >> assert(idx < ms->possible_cpus->len); >> - x86_topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id, >> - &topo_info, &topo_ids); >> + x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id, >> + &topo_info, &topo_ids); >> return topo_ids.pkg_id % ms->numa_state->num_nodes; >> } >> >> @@ -169,6 +185,9 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms) >> return ms->possible_cpus; >> } >> >> + /* Initialize apicid handlers */ >> + x86_check_apicid_handlers(ms); >> + >> ms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) + >> sizeof(CPUArchId) * max_cpus); >> ms->possible_cpus->len = max_cpus; >> @@ -182,7 +201,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms) >> ms->possible_cpus->cpus[i].vcpus_count = 1; >> ms->possible_cpus->cpus[i].arch_id = >> x86_cpu_apic_id_from_index(x86ms, i); >> - x86_topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id, >> + x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id, >> &topo_info, &topo_ids); >> ms->possible_cpus->cpus[i].props.has_socket_id = true; >> ms->possible_cpus->cpus[i].props.socket_id = topo_ids.pkg_id; >> >> >
On Mon, Feb 24, 2020 at 11:58:09AM -0600, Babu Moger wrote: > > > On 2/24/20 11:19 AM, Igor Mammedov wrote: > > On Thu, 13 Feb 2020 12:17:46 -0600 > > Babu Moger <babu.moger@amd.com> wrote: > > > >> Check and Load the apicid handlers from X86CPUDefinition if available. > >> Update the calling convention for the apicid handlers. > > > > Previous and this patch look too complicated for the task at the hand. > > In particular, cpu_x86_init_apicid_fns() from previous patch adds 1 more > > reference to Machine into i386/cpu.c (even though it's just a helper function) > > and I think un-necessary hooks to X86CPUDefinition (it's not really CPU's > > businesses to make up APIC-IDs). > > > > I'd rather do opposite and get rid of the last explicit dependency to > > ms->smp.cpus from cpu.c. But well, it's out of scope of this series, > > so for this series I'd just try to avoid adding more Machine dependencies. > > > > All 11/16 does is basically using hooks as a switch "I'm EPYC" to > > set epyc specific encoding topo routines. > > > > It could be accomplished by a simple Boolean flag like > > X86CPUDefinition::use_epyc_apic_id_encoding > > > > and then cpu_x86_init_apicid_fns() could be replaced with trivial > > helper like: > > > > x86_use_epyc_apic_id_encoding(char *cpu_type) > > { > > X86CPUClass *xcc = ... cpu_type ... > > return xcc->model->cpudef->use_epyc_apic_id_encoding > > } > > > > then machine could override default[1] hooks using this helper > > as the trigger > > x86_cpus_init() > > { > > // no need in dedicated function as it's the only instance it's going to be called ever > > if (x86_use_epyc_apic_id_encoding(ms->cpu_type)) { > > x86ms->apicid_from_cpu_idx = ...epyc... > > x86ms->topo_ids_from_apicid = ...epyc... > > x86ms->apicid_from_topo_ids = ...epyc... > > x86ms->apicid_pkg_offset = ...epyc... > > } > > } > > > > That would be less invasive and won't create non necessary dependencies. > > Yes. We can achieve the task here with your approach mentioned above. But, > we still will have a scaling issue. In future if a "new cpu model" comes > up its own decoding, then we need to add another bolean flag use_new > _cpu_apic_id_encoding. And then do that same check again. In that sense, > the current approach is bit generic. Lets also hear from Eduardo. To be honest, I really hope the number of APIC ID initialization variations won't grow in the future. In either case, X86MachineState really doesn't seem to be the right place to save the function pointers. Whether we choose a boolean flag or a collection of function pointers, model-specific information belong to x86CPUClass and/or X86CPUDefinition, not MachineState. See the reply I sent at: https://lore.kernel.org/qemu-devel/20200128200438.GJ18770@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?
On 2/24/20 4:31 PM, Eduardo Habkost wrote: > On Mon, Feb 24, 2020 at 11:58:09AM -0600, Babu Moger wrote: >> >> >> On 2/24/20 11:19 AM, Igor Mammedov wrote: >>> On Thu, 13 Feb 2020 12:17:46 -0600 >>> Babu Moger <babu.moger@amd.com> wrote: >>> >>>> Check and Load the apicid handlers from X86CPUDefinition if available. >>>> Update the calling convention for the apicid handlers. >>> >>> Previous and this patch look too complicated for the task at the hand. >>> In particular, cpu_x86_init_apicid_fns() from previous patch adds 1 more >>> reference to Machine into i386/cpu.c (even though it's just a helper function) >>> and I think un-necessary hooks to X86CPUDefinition (it's not really CPU's >>> businesses to make up APIC-IDs). >>> >>> I'd rather do opposite and get rid of the last explicit dependency to >>> ms->smp.cpus from cpu.c. But well, it's out of scope of this series, >>> so for this series I'd just try to avoid adding more Machine dependencies. >>> >>> All 11/16 does is basically using hooks as a switch "I'm EPYC" to >>> set epyc specific encoding topo routines. >>> >>> It could be accomplished by a simple Boolean flag like >>> X86CPUDefinition::use_epyc_apic_id_encoding >>> >>> and then cpu_x86_init_apicid_fns() could be replaced with trivial >>> helper like: >>> >>> x86_use_epyc_apic_id_encoding(char *cpu_type) >>> { >>> X86CPUClass *xcc = ... cpu_type ... >>> return xcc->model->cpudef->use_epyc_apic_id_encoding >>> } >>> >>> then machine could override default[1] hooks using this helper >>> as the trigger >>> x86_cpus_init() >>> { >>> // no need in dedicated function as it's the only instance it's going to be called ever >>> if (x86_use_epyc_apic_id_encoding(ms->cpu_type)) { >>> x86ms->apicid_from_cpu_idx = ...epyc... >>> x86ms->topo_ids_from_apicid = ...epyc... >>> x86ms->apicid_from_topo_ids = ...epyc... >>> x86ms->apicid_pkg_offset = ...epyc... >>> } >>> } >>> >>> That would be less invasive and won't create non necessary dependencies. >> >> Yes. We can achieve the task here with your approach mentioned above. But, >> we still will have a scaling issue. In future if a "new cpu model" comes >> up its own decoding, then we need to add another bolean flag use_new >> _cpu_apic_id_encoding. And then do that same check again. In that sense, >> the current approach is bit generic. Lets also hear from Eduardo. > > To be honest, I really hope the number of APIC ID initialization > variations won't grow in the future. > > In either case, X86MachineState really doesn't seem to be the > right place to save the function pointers. Whether we choose a > boolean flag or a collection of function pointers, model-specific > information belong to x86CPUClass and/or X86CPUDefinition, not > MachineState. My bad. I completely missed that part. Yes. You mentioned that earlier. I can move the functions pointers to X86CPUClass and initialize the pointers from X86CPUDefinition. Thanks > > See the reply I sent at: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F20200128200438.GJ18770%40habkost.net%2F&data=02%7C01%7Cbabu.moger%40amd.com%7Cda1d1c9f34af4475596108d7b9795fef%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637181803279890359&sdata=Z%2B%2BA2%2FMIVQZfGtUe1aBLzttCQnCpZKEwshOhoVAg1%2BU%3D&reserved=0 > > ] 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? >
On Mon, 24 Feb 2020 17:31:49 -0500 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Feb 24, 2020 at 11:58:09AM -0600, Babu Moger wrote: > > > > > > On 2/24/20 11:19 AM, Igor Mammedov wrote: > > > On Thu, 13 Feb 2020 12:17:46 -0600 > > > Babu Moger <babu.moger@amd.com> wrote: > > > > > >> Check and Load the apicid handlers from X86CPUDefinition if available. > > >> Update the calling convention for the apicid handlers. > > > > > > Previous and this patch look too complicated for the task at the hand. > > > In particular, cpu_x86_init_apicid_fns() from previous patch adds 1 more > > > reference to Machine into i386/cpu.c (even though it's just a helper function) > > > and I think un-necessary hooks to X86CPUDefinition (it's not really CPU's > > > businesses to make up APIC-IDs). > > > > > > I'd rather do opposite and get rid of the last explicit dependency to > > > ms->smp.cpus from cpu.c. But well, it's out of scope of this series, > > > so for this series I'd just try to avoid adding more Machine dependencies. > > > > > > All 11/16 does is basically using hooks as a switch "I'm EPYC" to > > > set epyc specific encoding topo routines. > > > > > > It could be accomplished by a simple Boolean flag like > > > X86CPUDefinition::use_epyc_apic_id_encoding > > > > > > and then cpu_x86_init_apicid_fns() could be replaced with trivial > > > helper like: > > > > > > x86_use_epyc_apic_id_encoding(char *cpu_type) > > > { > > > X86CPUClass *xcc = ... cpu_type ... > > > return xcc->model->cpudef->use_epyc_apic_id_encoding > > > } > > > > > > then machine could override default[1] hooks using this helper > > > as the trigger > > > x86_cpus_init() > > > { > > > // no need in dedicated function as it's the only instance it's going to be called ever > > > if (x86_use_epyc_apic_id_encoding(ms->cpu_type)) { > > > x86ms->apicid_from_cpu_idx = ...epyc... > > > x86ms->topo_ids_from_apicid = ...epyc... > > > x86ms->apicid_from_topo_ids = ...epyc... > > > x86ms->apicid_pkg_offset = ...epyc... > > > } > > > } > > > > > > That would be less invasive and won't create non necessary dependencies. > > > > Yes. We can achieve the task here with your approach mentioned above. But, > > we still will have a scaling issue. In future if a "new cpu model" comes > > up its own decoding, then we need to add another bolean flag use_new > > _cpu_apic_id_encoding. And then do that same check again. In that sense, > > the current approach is bit generic. Lets also hear from Eduardo. Without another encoding on horizon, it looks like over-engineering. So lets think of a more generic approach later when need arises. > To be honest, I really hope the number of APIC ID initialization > variations won't grow in the future. > In either case, X86MachineState really doesn't seem to be the > right place to save the function pointers. Whether we choose a > boolean flag or a collection of function pointers, model-specific > information belong to x86CPUClass and/or X86CPUDefinition, not > MachineState. That's where I disagree, generating APIC ID and it's assignment shouldn't be part of CPU model. On real machines it's done by board/firmware, there board is purpose build for specific CPU. The same should be in QEMU case, where universal QEMU board adapts its APIC initialization to used CPU and not other way around (i.e. it's not CPU's job to generate IDs). So hooks in machine look like a good approach to me. I's fine to add small helper to CPU code to help board decide what APIC encoding to use, but I strongly disagree in putting more logic/data than that into CPU model. What CPU does inside (I mean CPUID handling) it's separate business and in that case CPU usually knows what it's encoding and can use epyc/non_epyc functions directly if necessary. > See the reply I sent at: > https://lore.kernel.org/qemu-devel/20200128200438.GJ18770@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? >
On Tue, Feb 25, 2020 at 09:16:32AM +0100, Igor Mammedov wrote: > On Mon, 24 Feb 2020 17:31:49 -0500 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, Feb 24, 2020 at 11:58:09AM -0600, Babu Moger wrote: > > > > > > > > > On 2/24/20 11:19 AM, Igor Mammedov wrote: > > > > On Thu, 13 Feb 2020 12:17:46 -0600 > > > > Babu Moger <babu.moger@amd.com> wrote: > > > > > > > >> Check and Load the apicid handlers from X86CPUDefinition if available. > > > >> Update the calling convention for the apicid handlers. > > > > > > > > Previous and this patch look too complicated for the task at the hand. > > > > In particular, cpu_x86_init_apicid_fns() from previous patch adds 1 more > > > > reference to Machine into i386/cpu.c (even though it's just a helper function) > > > > and I think un-necessary hooks to X86CPUDefinition (it's not really CPU's > > > > businesses to make up APIC-IDs). > > > > > > > > I'd rather do opposite and get rid of the last explicit dependency to > > > > ms->smp.cpus from cpu.c. But well, it's out of scope of this series, > > > > so for this series I'd just try to avoid adding more Machine dependencies. > > > > > > > > All 11/16 does is basically using hooks as a switch "I'm EPYC" to > > > > set epyc specific encoding topo routines. > > > > > > > > It could be accomplished by a simple Boolean flag like > > > > X86CPUDefinition::use_epyc_apic_id_encoding > > > > > > > > and then cpu_x86_init_apicid_fns() could be replaced with trivial > > > > helper like: > > > > > > > > x86_use_epyc_apic_id_encoding(char *cpu_type) > > > > { > > > > X86CPUClass *xcc = ... cpu_type ... > > > > return xcc->model->cpudef->use_epyc_apic_id_encoding > > > > } > > > > > > > > then machine could override default[1] hooks using this helper > > > > as the trigger > > > > x86_cpus_init() > > > > { > > > > // no need in dedicated function as it's the only instance it's going to be called ever > > > > if (x86_use_epyc_apic_id_encoding(ms->cpu_type)) { > > > > x86ms->apicid_from_cpu_idx = ...epyc... > > > > x86ms->topo_ids_from_apicid = ...epyc... > > > > x86ms->apicid_from_topo_ids = ...epyc... > > > > x86ms->apicid_pkg_offset = ...epyc... [1] > > > > } > > > > } > > > > > > > > That would be less invasive and won't create non necessary dependencies. > > > > > > Yes. We can achieve the task here with your approach mentioned above. But, > > > we still will have a scaling issue. In future if a "new cpu model" comes > > > up its own decoding, then we need to add another bolean flag use_new > > > _cpu_apic_id_encoding. And then do that same check again. In that sense, > > > the current approach is bit generic. Lets also hear from Eduardo. > > Without another encoding on horizon, it looks like over-engineering. > So lets think of a more generic approach later when need arises. > > > > To be honest, I really hope the number of APIC ID initialization > > variations won't grow in the future. > > In either case, X86MachineState really doesn't seem to be the > > right place to save the function pointers. Whether we choose a > > boolean flag or a collection of function pointers, model-specific > > information belong to x86CPUClass and/or X86CPUDefinition, not > > MachineState. > > That's where I disagree, generating APIC ID and it's assignment > shouldn't be part of CPU model. On real machines it's done by > board/firmware, there board is purpose build for specific CPU. > > The same should be in QEMU case, where universal QEMU board > adapts its APIC initialization to used CPU and not other way > around (i.e. it's not CPU's job to generate IDs). > So hooks in machine look like a good approach to me. > > I's fine to add small helper to CPU code to help board decide > what APIC encoding to use, but I strongly disagree in putting > more logic/data than that into CPU model. > > What CPU does inside (I mean CPUID handling) it's separate > business and in that case CPU usually knows what it's encoding > and can use epyc/non_epyc functions directly if necessary. I see your point. I was assuming that the function pointers would stay in cpu.c/X86CPUDefinition, then it wouldn't make sense to copy the pointers to X86MachineClass. If the functions are going to live in pc.c, then adding the function pointers to X86MachineState might make sense and your suggestion above[1] sounds reasonable.
On Mon, Feb 24, 2020 at 05:13:18PM -0600, Babu Moger wrote: > > > On 2/24/20 4:31 PM, Eduardo Habkost wrote: > > On Mon, Feb 24, 2020 at 11:58:09AM -0600, Babu Moger wrote: > >> > >> > >> On 2/24/20 11:19 AM, Igor Mammedov wrote: > >>> On Thu, 13 Feb 2020 12:17:46 -0600 > >>> Babu Moger <babu.moger@amd.com> wrote: > >>> > >>>> Check and Load the apicid handlers from X86CPUDefinition if available. > >>>> Update the calling convention for the apicid handlers. > >>> > >>> Previous and this patch look too complicated for the task at the hand. > >>> In particular, cpu_x86_init_apicid_fns() from previous patch adds 1 more > >>> reference to Machine into i386/cpu.c (even though it's just a helper function) > >>> and I think un-necessary hooks to X86CPUDefinition (it's not really CPU's > >>> businesses to make up APIC-IDs). > >>> > >>> I'd rather do opposite and get rid of the last explicit dependency to > >>> ms->smp.cpus from cpu.c. But well, it's out of scope of this series, > >>> so for this series I'd just try to avoid adding more Machine dependencies. > >>> > >>> All 11/16 does is basically using hooks as a switch "I'm EPYC" to > >>> set epyc specific encoding topo routines. > >>> > >>> It could be accomplished by a simple Boolean flag like > >>> X86CPUDefinition::use_epyc_apic_id_encoding > >>> > >>> and then cpu_x86_init_apicid_fns() could be replaced with trivial > >>> helper like: > >>> > >>> x86_use_epyc_apic_id_encoding(char *cpu_type) > >>> { > >>> X86CPUClass *xcc = ... cpu_type ... > >>> return xcc->model->cpudef->use_epyc_apic_id_encoding > >>> } > >>> > >>> then machine could override default[1] hooks using this helper > >>> as the trigger > >>> x86_cpus_init() > >>> { > >>> // no need in dedicated function as it's the only instance it's going to be called ever > >>> if (x86_use_epyc_apic_id_encoding(ms->cpu_type)) { > >>> x86ms->apicid_from_cpu_idx = ...epyc... > >>> x86ms->topo_ids_from_apicid = ...epyc... > >>> x86ms->apicid_from_topo_ids = ...epyc... > >>> x86ms->apicid_pkg_offset = ...epyc... > >>> } > >>> } > >>> > >>> That would be less invasive and won't create non necessary dependencies. > >> > >> Yes. We can achieve the task here with your approach mentioned above. But, > >> we still will have a scaling issue. In future if a "new cpu model" comes > >> up its own decoding, then we need to add another bolean flag use_new > >> _cpu_apic_id_encoding. And then do that same check again. In that sense, > >> the current approach is bit generic. Lets also hear from Eduardo. > > > > To be honest, I really hope the number of APIC ID initialization > > variations won't grow in the future. > > > > In either case, X86MachineState really doesn't seem to be the > > right place to save the function pointers. Whether we choose a > > boolean flag or a collection of function pointers, model-specific > > information belong to x86CPUClass and/or X86CPUDefinition, not > > MachineState. > > My bad. I completely missed that part. Yes. You mentioned that earlier. > I can move the functions pointers to X86CPUClass and initialize the > pointers from X86CPUDefinition. Thanks See my reply to Igor before doing that. Summary is: if the function implementations are provided by the CPU, the pointers belong to X86CPUClass. If the APIC ID calculation logic lives in pc.c, the pointers probably can stay in X86MachineState.
On 2/25/20 9:32 AM, Eduardo Habkost wrote: > On Mon, Feb 24, 2020 at 05:13:18PM -0600, Babu Moger wrote: >> >> >> On 2/24/20 4:31 PM, Eduardo Habkost wrote: >>> On Mon, Feb 24, 2020 at 11:58:09AM -0600, Babu Moger wrote: >>>> >>>> >>>> On 2/24/20 11:19 AM, Igor Mammedov wrote: >>>>> On Thu, 13 Feb 2020 12:17:46 -0600 >>>>> Babu Moger <babu.moger@amd.com> wrote: >>>>> >>>>>> Check and Load the apicid handlers from X86CPUDefinition if available. >>>>>> Update the calling convention for the apicid handlers. >>>>> >>>>> Previous and this patch look too complicated for the task at the hand. >>>>> In particular, cpu_x86_init_apicid_fns() from previous patch adds 1 more >>>>> reference to Machine into i386/cpu.c (even though it's just a helper function) >>>>> and I think un-necessary hooks to X86CPUDefinition (it's not really CPU's >>>>> businesses to make up APIC-IDs). >>>>> >>>>> I'd rather do opposite and get rid of the last explicit dependency to >>>>> ms->smp.cpus from cpu.c. But well, it's out of scope of this series, >>>>> so for this series I'd just try to avoid adding more Machine dependencies. >>>>> >>>>> All 11/16 does is basically using hooks as a switch "I'm EPYC" to >>>>> set epyc specific encoding topo routines. >>>>> >>>>> It could be accomplished by a simple Boolean flag like >>>>> X86CPUDefinition::use_epyc_apic_id_encoding >>>>> >>>>> and then cpu_x86_init_apicid_fns() could be replaced with trivial >>>>> helper like: >>>>> >>>>> x86_use_epyc_apic_id_encoding(char *cpu_type) >>>>> { >>>>> X86CPUClass *xcc = ... cpu_type ... >>>>> return xcc->model->cpudef->use_epyc_apic_id_encoding >>>>> } >>>>> >>>>> then machine could override default[1] hooks using this helper >>>>> as the trigger >>>>> x86_cpus_init() >>>>> { >>>>> // no need in dedicated function as it's the only instance it's going to be called ever >>>>> if (x86_use_epyc_apic_id_encoding(ms->cpu_type)) { >>>>> x86ms->apicid_from_cpu_idx = ...epyc... >>>>> x86ms->topo_ids_from_apicid = ...epyc... >>>>> x86ms->apicid_from_topo_ids = ...epyc... >>>>> x86ms->apicid_pkg_offset = ...epyc... >>>>> } >>>>> } >>>>> >>>>> That would be less invasive and won't create non necessary dependencies. >>>> >>>> Yes. We can achieve the task here with your approach mentioned above. But, >>>> we still will have a scaling issue. In future if a "new cpu model" comes >>>> up its own decoding, then we need to add another bolean flag use_new >>>> _cpu_apic_id_encoding. And then do that same check again. In that sense, >>>> the current approach is bit generic. Lets also hear from Eduardo. >>> >>> To be honest, I really hope the number of APIC ID initialization >>> variations won't grow in the future. >>> >>> In either case, X86MachineState really doesn't seem to be the >>> right place to save the function pointers. Whether we choose a >>> boolean flag or a collection of function pointers, model-specific >>> information belong to x86CPUClass and/or X86CPUDefinition, not >>> MachineState. >> >> My bad. I completely missed that part. Yes. You mentioned that earlier. >> I can move the functions pointers to X86CPUClass and initialize the >> pointers from X86CPUDefinition. Thanks > > See my reply to Igor before doing that. > > Summary is: if the function implementations are provided by the > CPU, the pointers belong to X86CPUClass. If the APIC ID > calculation logic lives in pc.c, the pointers probably can stay > in X86MachineState. > Ok. Sure. I will leave those pointers in X86MachineState.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index be72a49716..93063af6a8 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1808,14 +1808,14 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, topo_ids.die_id = cpu->die_id; topo_ids.core_id = cpu->core_id; topo_ids.smt_id = cpu->thread_id; - cpu->apic_id = x86_apicid_from_topo_ids(&topo_info, &topo_ids); + cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); } 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, &topo_info, &topo_ids); + x86ms->topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids); error_setg(errp, "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with" " APIC ID %" PRIu32 ", valid index range 0:%d", @@ -1836,7 +1836,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, /* TODO: move socket_id/core_id/thread_id checks into x86_cpu_realizefn() * once -smp refactoring is complete and there will be CPU private * CPUState::nr_cores and CPUState::nr_threads fields instead of globals */ - x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids); + x86ms->topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids); if (cpu->socket_id != -1 && cpu->socket_id != topo_ids.pkg_id) { error_setg(errp, "property socket-id: %u doesn't match set apic-id:" " 0x%x (socket-id: %u)", cpu->socket_id, cpu->apic_id, topo_ids.pkg_id); diff --git a/hw/i386/x86.c b/hw/i386/x86.c index 3d944f68e6..b825861b85 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -52,6 +52,22 @@ /* Physical Address of PVH entry point read from kernel ELF NOTE */ static size_t pvh_start_addr; +/* + * Check for apicid handlers in X86MachineState. Load them if + * not loaded already. These handlers are loaded from X86CPUDefinition. + */ +static void x86_check_apicid_handlers(MachineState *ms) +{ + X86MachineState *x86ms = X86_MACHINE(ms); + + if (!x86ms->apicid_from_cpu_idx || + !x86ms->topo_ids_from_apicid || + !x86ms->apicid_from_topo_ids || + !x86ms->apicid_pkg_offset) { + cpu_x86_init_apicid_fns(ms); + } +} + /* * Calculates initial APIC ID for a specific CPU index * @@ -70,7 +86,7 @@ uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms, init_topo_info(&topo_info, x86ms); - correct_id = x86_apicid_from_cpu_idx(&topo_info, cpu_index); + correct_id = x86ms->apicid_from_cpu_idx(&topo_info, cpu_index); if (x86mc->compat_apic_id_mode) { if (cpu_index != correct_id && !warned && !qtest_enabled()) { error_report("APIC IDs set in compatibility mode, " @@ -148,8 +164,8 @@ int64_t x86_get_default_cpu_node_id(const MachineState *ms, int idx) init_topo_info(&topo_info, x86ms); assert(idx < ms->possible_cpus->len); - x86_topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id, - &topo_info, &topo_ids); + x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id, + &topo_info, &topo_ids); return topo_ids.pkg_id % ms->numa_state->num_nodes; } @@ -169,6 +185,9 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms) return ms->possible_cpus; } + /* Initialize apicid handlers */ + x86_check_apicid_handlers(ms); + ms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) + sizeof(CPUArchId) * max_cpus); ms->possible_cpus->len = max_cpus; @@ -182,7 +201,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms) ms->possible_cpus->cpus[i].vcpus_count = 1; ms->possible_cpus->cpus[i].arch_id = x86_cpu_apic_id_from_index(x86ms, i); - x86_topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id, + x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id, &topo_info, &topo_ids); ms->possible_cpus->cpus[i].props.has_socket_id = true; ms->possible_cpus->cpus[i].props.socket_id = topo_ids.pkg_id;
Check and Load the apicid handlers from X86CPUDefinition if available. Update the calling convention for the apicid handlers. Signed-off-by: Babu Moger <babu.moger@amd.com> --- hw/i386/pc.c | 6 +++--- hw/i386/x86.c | 27 +++++++++++++++++++++++---- 2 files changed, 26 insertions(+), 7 deletions(-)