| Message ID | 158326550403.40452.15934956681175349815.stgit@naples-babu.amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | APIC ID fixes for AMD EPYC CPU model | expand |
On Tue, 03 Mar 2020 13:58:24 -0600 Babu Moger <babu.moger@amd.com> wrote: > Apicid calculation depends on knowing the total number of numa nodes > for EPYC cpu models. Right now, we are calculating the arch_id while > parsing the numa(parse_numa). At this time, it is not known how many > total numa nodes are configured in the system. > > Move the arch_id inside x86_cpus_init. At this time smp parameter is already > completed and numa node information is available. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > hw/i386/x86.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index d46dd4ad9e..66998b065c 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -121,6 +121,9 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version) > MachineState *ms = MACHINE(x86ms); > MachineClass *mc = MACHINE_GET_CLASS(x86ms); > > + /* Initialize apicid handlers first */ > + cpu_x86_init_apicid_fns(ms); > + > x86_cpu_set_default_version(default_cpu_version); > > /* > @@ -134,6 +137,12 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version) > x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms, > ms->smp.max_cpus - 1) + 1; > possible_cpus = mc->possible_cpu_arch_ids(ms); > + > + for (i = 0; i < ms->smp.cpus; i++) { > + ms->possible_cpus->cpus[i].arch_id = > + x86_cpu_apic_id_from_index(x86ms, i); > + } > + > for (i = 0; i < ms->smp.cpus; i++) { > x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal); > } > @@ -158,8 +167,7 @@ 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); > - x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id, > - &topo_info, &topo_ids); > + x86_topo_ids_from_idx(&topo_info, idx, &topo_ids); not necessary if default x86ms->topo_ids_from_apicid were initialized from x86 machine class I also wonder if this default contraption we have is going to work in case of EPYC cpu (i.e. is would generate valid nodeids). Bot instead of than trying to fix it if it's broken, I'd rather deprecate and drop get_default_cpu_node_id() requiring users to explicitly define CPU mapping to numa nodes. That would be consistent with req for explicit RAM for numa nodes (postponed till 5.1 due to libvirt not being ready), i.e if one wants numa, one should explicitly provide necessary mapping or machine won't start. > return topo_ids.pkg_id % ms->numa_state->num_nodes; > } > > @@ -193,10 +201,7 @@ const CPUArchIdList *x86_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(x86ms, i); > - x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id, > - &topo_info, &topo_ids); > + x86_topo_ids_from_idx(&topo_info, i, &topo_ids); ditto > ms->possible_cpus->cpus[i].props.has_socket_id = true; > ms->possible_cpus->cpus[i].props.socket_id = topo_ids.pkg_id; > if (x86ms->smp_dies > 1) { >
On 3/9/20 10:21 AM, Igor Mammedov wrote: > On Tue, 03 Mar 2020 13:58:24 -0600 > Babu Moger <babu.moger@amd.com> wrote: > >> Apicid calculation depends on knowing the total number of numa nodes >> for EPYC cpu models. Right now, we are calculating the arch_id while >> parsing the numa(parse_numa). At this time, it is not known how many >> total numa nodes are configured in the system. >> >> Move the arch_id inside x86_cpus_init. At this time smp parameter is already >> completed and numa node information is available. >> >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> --- >> hw/i386/x86.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/hw/i386/x86.c b/hw/i386/x86.c >> index d46dd4ad9e..66998b065c 100644 >> --- a/hw/i386/x86.c >> +++ b/hw/i386/x86.c >> @@ -121,6 +121,9 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version) >> MachineState *ms = MACHINE(x86ms); >> MachineClass *mc = MACHINE_GET_CLASS(x86ms); >> >> + /* Initialize apicid handlers first */ >> + cpu_x86_init_apicid_fns(ms); >> + >> x86_cpu_set_default_version(default_cpu_version); >> >> /* >> @@ -134,6 +137,12 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version) >> x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms, >> ms->smp.max_cpus - 1) + 1; >> possible_cpus = mc->possible_cpu_arch_ids(ms); >> + >> + for (i = 0; i < ms->smp.cpus; i++) { >> + ms->possible_cpus->cpus[i].arch_id = >> + x86_cpu_apic_id_from_index(x86ms, i); >> + } >> + >> for (i = 0; i < ms->smp.cpus; i++) { >> x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal); >> } >> @@ -158,8 +167,7 @@ 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); >> - x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id, >> - &topo_info, &topo_ids); >> + x86_topo_ids_from_idx(&topo_info, idx, &topo_ids); > not necessary if default x86ms->topo_ids_from_apicid were initialized from x86 machine class > > I also wonder if this default contraption we have is going to work > in case of EPYC cpu (i.e. is would generate valid nodeids). From what I understand, we call this x86_get_default_cpu_node_id only when the user does not specify the numa binding requirements. We tried to generate the default node it for a given config. This works fine for EPYC also. I am not sure about changing this right now. what do you think? > > Bot instead of than trying to fix it if it's broken, > I'd rather deprecate and drop get_default_cpu_node_id() requiring users > to explicitly define CPU mapping to numa nodes. > That would be consistent with req for explicit RAM for numa nodes > (postponed till 5.1 due to libvirt not being ready), > i.e if one wants numa, one should explicitly provide necessary mapping > or machine won't start. > > >> return topo_ids.pkg_id % ms->numa_state->num_nodes; >> } >> >> @@ -193,10 +201,7 @@ const CPUArchIdList *x86_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(x86ms, i); >> - x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id, >> - &topo_info, &topo_ids); >> + x86_topo_ids_from_idx(&topo_info, i, &topo_ids); > ditto > >> ms->possible_cpus->cpus[i].props.has_socket_id = true; >> ms->possible_cpus->cpus[i].props.socket_id = topo_ids.pkg_id; >> if (x86ms->smp_dies > 1) { >> >
On Mon, 9 Mar 2020 14:31:31 -0500 Babu Moger <babu.moger@amd.com> wrote: > On 3/9/20 10:21 AM, Igor Mammedov wrote: > > On Tue, 03 Mar 2020 13:58:24 -0600 > > Babu Moger <babu.moger@amd.com> wrote: > > > >> Apicid calculation depends on knowing the total number of numa nodes > >> for EPYC cpu models. Right now, we are calculating the arch_id while > >> parsing the numa(parse_numa). At this time, it is not known how many > >> total numa nodes are configured in the system. > >> > >> Move the arch_id inside x86_cpus_init. At this time smp parameter is already > >> completed and numa node information is available. > >> > >> Signed-off-by: Babu Moger <babu.moger@amd.com> > >> --- > >> hw/i386/x86.c | 17 +++++++++++------ > >> 1 file changed, 11 insertions(+), 6 deletions(-) > >> > >> diff --git a/hw/i386/x86.c b/hw/i386/x86.c > >> index d46dd4ad9e..66998b065c 100644 > >> --- a/hw/i386/x86.c > >> +++ b/hw/i386/x86.c > >> @@ -121,6 +121,9 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version) > >> MachineState *ms = MACHINE(x86ms); > >> MachineClass *mc = MACHINE_GET_CLASS(x86ms); > >> > >> + /* Initialize apicid handlers first */ > >> + cpu_x86_init_apicid_fns(ms); > >> + > >> x86_cpu_set_default_version(default_cpu_version); > >> > >> /* > >> @@ -134,6 +137,12 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version) > >> x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms, > >> ms->smp.max_cpus - 1) + 1; > >> possible_cpus = mc->possible_cpu_arch_ids(ms); > >> + > >> + for (i = 0; i < ms->smp.cpus; i++) { > >> + ms->possible_cpus->cpus[i].arch_id = > >> + x86_cpu_apic_id_from_index(x86ms, i); > >> + } > >> + > >> for (i = 0; i < ms->smp.cpus; i++) { > >> x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal); > >> } > >> @@ -158,8 +167,7 @@ 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); > >> - x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id, > >> - &topo_info, &topo_ids); > >> + x86_topo_ids_from_idx(&topo_info, idx, &topo_ids); > > not necessary if default x86ms->topo_ids_from_apicid were initialized from x86 machine class > > > > I also wonder if this default contraption we have is going to work > > in case of EPYC cpu (i.e. is would generate valid nodeids). > > From what I understand, we call this x86_get_default_cpu_node_id only when > the user does not specify the numa binding requirements. We tried to > generate the default node it for a given config. This works fine for EPYC > also. I am not sure about changing this right now. what do you think? if it work for EPYC with default x86_topo_ids_from_idx() then it's fine. Just keep callback here, given that callback is always initialized early (class_init) there is no point to create mix of callback/non-callback usage. > > > > Bot instead of than trying to fix it if it's broken, > > I'd rather deprecate and drop get_default_cpu_node_id() requiring users > > to explicitly define CPU mapping to numa nodes. > > That would be consistent with req for explicit RAM for numa nodes > > (postponed till 5.1 due to libvirt not being ready), > > i.e if one wants numa, one should explicitly provide necessary mapping > > or machine won't start. > > > > > >> return topo_ids.pkg_id % ms->numa_state->num_nodes; > >> } > >> > >> @@ -193,10 +201,7 @@ const CPUArchIdList *x86_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(x86ms, i); > >> - x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id, > >> - &topo_info, &topo_ids); > >> + x86_topo_ids_from_idx(&topo_info, i, &topo_ids); > > ditto > > > >> ms->possible_cpus->cpus[i].props.has_socket_id = true; > >> ms->possible_cpus->cpus[i].props.socket_id = topo_ids.pkg_id; > >> if (x86ms->smp_dies > 1) { > >> > > >
On 3/10/20 3:35 AM, Igor Mammedov wrote: > On Mon, 9 Mar 2020 14:31:31 -0500 > Babu Moger <babu.moger@amd.com> wrote: > >> On 3/9/20 10:21 AM, Igor Mammedov wrote: >>> On Tue, 03 Mar 2020 13:58:24 -0600 >>> Babu Moger <babu.moger@amd.com> wrote: >>> >>>> Apicid calculation depends on knowing the total number of numa nodes >>>> for EPYC cpu models. Right now, we are calculating the arch_id while >>>> parsing the numa(parse_numa). At this time, it is not known how many >>>> total numa nodes are configured in the system. >>>> >>>> Move the arch_id inside x86_cpus_init. At this time smp parameter is already >>>> completed and numa node information is available. >>>> >>>> Signed-off-by: Babu Moger <babu.moger@amd.com> >>>> --- >>>> hw/i386/x86.c | 17 +++++++++++------ >>>> 1 file changed, 11 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c >>>> index d46dd4ad9e..66998b065c 100644 >>>> --- a/hw/i386/x86.c >>>> +++ b/hw/i386/x86.c >>>> @@ -121,6 +121,9 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version) >>>> MachineState *ms = MACHINE(x86ms); >>>> MachineClass *mc = MACHINE_GET_CLASS(x86ms); >>>> >>>> + /* Initialize apicid handlers first */ >>>> + cpu_x86_init_apicid_fns(ms); >>>> + >>>> x86_cpu_set_default_version(default_cpu_version); >>>> >>>> /* >>>> @@ -134,6 +137,12 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version) >>>> x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms, >>>> ms->smp.max_cpus - 1) + 1; >>>> possible_cpus = mc->possible_cpu_arch_ids(ms); >>>> + >>>> + for (i = 0; i < ms->smp.cpus; i++) { >>>> + ms->possible_cpus->cpus[i].arch_id = >>>> + x86_cpu_apic_id_from_index(x86ms, i); >>>> + } >>>> + >>>> for (i = 0; i < ms->smp.cpus; i++) { >>>> x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal); >>>> } >>>> @@ -158,8 +167,7 @@ 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); >>>> - x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id, >>>> - &topo_info, &topo_ids); >>>> + x86_topo_ids_from_idx(&topo_info, idx, &topo_ids); >>> not necessary if default x86ms->topo_ids_from_apicid were initialized from x86 machine class >>> >>> I also wonder if this default contraption we have is going to work >>> in case of EPYC cpu (i.e. is would generate valid nodeids). >> >> From what I understand, we call this x86_get_default_cpu_node_id only when >> the user does not specify the numa binding requirements. We tried to >> generate the default node it for a given config. This works fine for EPYC >> also. I am not sure about changing this right now. what do you think? > > if it work for EPYC with default x86_topo_ids_from_idx() then it's fine. > > Just keep callback here, given that callback is always initialized early (class_init) > there is no point to create mix of callback/non-callback usage. Ok. Done. We did not have callback for x86_topo_ids_from_idx explicity. Now, I have added this function as callback and using the callback here. > >>> >>> Bot instead of than trying to fix it if it's broken, >>> I'd rather deprecate and drop get_default_cpu_node_id() requiring users >>> to explicitly define CPU mapping to numa nodes. >>> That would be consistent with req for explicit RAM for numa nodes >>> (postponed till 5.1 due to libvirt not being ready), >>> i.e if one wants numa, one should explicitly provide necessary mapping >>> or machine won't start. >>> >>> >>>> return topo_ids.pkg_id % ms->numa_state->num_nodes; >>>> } >>>> >>>> @@ -193,10 +201,7 @@ const CPUArchIdList *x86_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(x86ms, i); >>>> - x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id, >>>> - &topo_info, &topo_ids); >>>> + x86_topo_ids_from_idx(&topo_info, i, &topo_ids); >>> ditto >>> >>>> ms->possible_cpus->cpus[i].props.has_socket_id = true; >>>> ms->possible_cpus->cpus[i].props.socket_id = topo_ids.pkg_id; >>>> if (x86ms->smp_dies > 1) { >>>> >>> >> >
diff --git a/hw/i386/x86.c b/hw/i386/x86.c index d46dd4ad9e..66998b065c 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -121,6 +121,9 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version) MachineState *ms = MACHINE(x86ms); MachineClass *mc = MACHINE_GET_CLASS(x86ms); + /* Initialize apicid handlers first */ + cpu_x86_init_apicid_fns(ms); + x86_cpu_set_default_version(default_cpu_version); /* @@ -134,6 +137,12 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version) x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms, ms->smp.max_cpus - 1) + 1; possible_cpus = mc->possible_cpu_arch_ids(ms); + + for (i = 0; i < ms->smp.cpus; i++) { + ms->possible_cpus->cpus[i].arch_id = + x86_cpu_apic_id_from_index(x86ms, i); + } + for (i = 0; i < ms->smp.cpus; i++) { x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal); } @@ -158,8 +167,7 @@ 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); - x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id, - &topo_info, &topo_ids); + x86_topo_ids_from_idx(&topo_info, idx, &topo_ids); return topo_ids.pkg_id % ms->numa_state->num_nodes; } @@ -193,10 +201,7 @@ const CPUArchIdList *x86_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(x86ms, i); - x86ms->topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id, - &topo_info, &topo_ids); + x86_topo_ids_from_idx(&topo_info, i, &topo_ids); ms->possible_cpus->cpus[i].props.has_socket_id = true; ms->possible_cpus->cpus[i].props.socket_id = topo_ids.pkg_id; if (x86ms->smp_dies > 1) {
Apicid calculation depends on knowing the total number of numa nodes for EPYC cpu models. Right now, we are calculating the arch_id while parsing the numa(parse_numa). At this time, it is not known how many total numa nodes are configured in the system. Move the arch_id inside x86_cpus_init. At this time smp parameter is already completed and numa node information is available. Signed-off-by: Babu Moger <babu.moger@amd.com> --- hw/i386/x86.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)