diff mbox series

[v5,14/16] hw/i386: Move arch_id decode inside x86_cpus_init

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

Commit Message

Babu Moger March 3, 2020, 7:58 p.m. UTC
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(-)

Comments

Igor Mammedov March 9, 2020, 3:21 p.m. UTC | #1
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) {
>
Babu Moger March 9, 2020, 7:31 p.m. UTC | #2
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) {
>>
>
Igor Mammedov March 10, 2020, 8:35 a.m. UTC | #3
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) {
> >>  
> >   
>
Babu Moger March 10, 2020, 8:05 p.m. UTC | #4
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 mbox series

Patch

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) {