| Message ID | 157541986210.46157.5082551407581177819.stgit@naples-babu.amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | APIC ID fixes for AMD EPYC CPU models | expand |
On Tue, 03 Dec 2019 18:37:42 -0600 Babu Moger <babu.moger@amd.com> wrote: > Add a new function init_apicid_fn in MachineClass to initialize the mode > specific handlers to decode the apic ids. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > include/hw/boards.h | 1 + > vl.c | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/include/hw/boards.h b/include/hw/boards.h > index d4fab218e6..ce5aa365cb 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -238,6 +238,7 @@ struct MachineClass { > unsigned cpu_index); > const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); > int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); > + void (*init_apicid_fn)(MachineState *ms); it's x86 specific, so why it wasn put into PCMachineClass? > }; > > /** > diff --git a/vl.c b/vl.c > index a42c24a77f..b6af604e11 100644 > --- a/vl.c > +++ b/vl.c > @@ -4318,6 +4318,9 @@ int main(int argc, char **argv, char **envp) > current_machine->cpu_type = machine_class->default_cpu_type; > if (cpu_option) { > current_machine->cpu_type = parse_cpu_option(cpu_option); > + if (machine_class->init_apicid_fn) { > + machine_class->init_apicid_fn(current_machine); > + } > } > parse_numa_opts(current_machine); > > >
On 1/28/20 10:29 AM, Igor Mammedov wrote: > On Tue, 03 Dec 2019 18:37:42 -0600 > Babu Moger <babu.moger@amd.com> wrote: > >> Add a new function init_apicid_fn in MachineClass to initialize the mode >> specific handlers to decode the apic ids. >> >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> --- >> include/hw/boards.h | 1 + >> vl.c | 3 +++ >> 2 files changed, 4 insertions(+) >> >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index d4fab218e6..ce5aa365cb 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -238,6 +238,7 @@ struct MachineClass { >> unsigned cpu_index); >> const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); >> int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); >> + void (*init_apicid_fn)(MachineState *ms); > it's x86 specific, so why it wasn put into PCMachineClass? Yes. It is x86 specific for now. I tried to make it generic function so other OSes can use it if required(like we have done in possible_cpu_arch_ids). It initializes functions required to build the apicid for each CPUs. We need these functions much early in the initialization. It should be initialized before parse_numa_opts or machine_run_board_init(in v1.c) which are called from generic context. We cannot use PCMachineClass at this time. > > >> }; >> >> /** >> diff --git a/vl.c b/vl.c >> index a42c24a77f..b6af604e11 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -4318,6 +4318,9 @@ int main(int argc, char **argv, char **envp) >> current_machine->cpu_type = machine_class->default_cpu_type; >> if (cpu_option) { >> current_machine->cpu_type = parse_cpu_option(cpu_option); >> + if (machine_class->init_apicid_fn) { >> + machine_class->init_apicid_fn(current_machine); >> + } >> } >> parse_numa_opts(current_machine); >> >> >> >
On Tue, Jan 28, 2020 at 01:45:31PM -0600, Babu Moger wrote: > > > On 1/28/20 10:29 AM, Igor Mammedov wrote: > > On Tue, 03 Dec 2019 18:37:42 -0600 > > Babu Moger <babu.moger@amd.com> wrote: > > > >> Add a new function init_apicid_fn in MachineClass to initialize the mode > >> specific handlers to decode the apic ids. > >> > >> Signed-off-by: Babu Moger <babu.moger@amd.com> > >> --- > >> include/hw/boards.h | 1 + > >> vl.c | 3 +++ > >> 2 files changed, 4 insertions(+) > >> > >> diff --git a/include/hw/boards.h b/include/hw/boards.h > >> index d4fab218e6..ce5aa365cb 100644 > >> --- a/include/hw/boards.h > >> +++ b/include/hw/boards.h > >> @@ -238,6 +238,7 @@ struct MachineClass { > >> unsigned cpu_index); > >> const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); > >> int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); > >> + void (*init_apicid_fn)(MachineState *ms); > > it's x86 specific, so why it wasn put into PCMachineClass? > > Yes. It is x86 specific for now. I tried to make it generic function so > other OSes can use it if required(like we have done in > possible_cpu_arch_ids). It initializes functions required to build the > apicid for each CPUs. We need these functions much early in the > initialization. It should be initialized before parse_numa_opts or > machine_run_board_init(in v1.c) which are called from generic context. We > cannot use PCMachineClass at this time. Even if the only user of the new hook will be x86, you are introducing a generic API, so a x86-specific name doesn't seem appropriate. I suggest using a generic name and documenting its rules and intended usage explicitly. Something like "pre_init" might be good enough, as long as the rules documented clearly (e.g. it will be called before NUMA initialization, but after CPU model lookup). However, I believe we can implement the same functionality without a new generic initialization hook. See my reply to patch 16/18. > > > > > > >> }; > >> > >> /** > >> diff --git a/vl.c b/vl.c > >> index a42c24a77f..b6af604e11 100644 > >> --- a/vl.c > >> +++ b/vl.c > >> @@ -4318,6 +4318,9 @@ int main(int argc, char **argv, char **envp) > >> current_machine->cpu_type = machine_class->default_cpu_type; > >> if (cpu_option) { > >> current_machine->cpu_type = parse_cpu_option(cpu_option); > >> + if (machine_class->init_apicid_fn) { > >> + machine_class->init_apicid_fn(current_machine); > >> + } > >> } > >> parse_numa_opts(current_machine); > >> > >> > >> > > >
On Tue, 28 Jan 2020 13:45:31 -0600 Babu Moger <babu.moger@amd.com> wrote: > On 1/28/20 10:29 AM, Igor Mammedov wrote: > > On Tue, 03 Dec 2019 18:37:42 -0600 > > Babu Moger <babu.moger@amd.com> wrote: > > > >> Add a new function init_apicid_fn in MachineClass to initialize the mode > >> specific handlers to decode the apic ids. > >> > >> Signed-off-by: Babu Moger <babu.moger@amd.com> > >> --- > >> include/hw/boards.h | 1 + > >> vl.c | 3 +++ > >> 2 files changed, 4 insertions(+) > >> > >> diff --git a/include/hw/boards.h b/include/hw/boards.h > >> index d4fab218e6..ce5aa365cb 100644 > >> --- a/include/hw/boards.h > >> +++ b/include/hw/boards.h > >> @@ -238,6 +238,7 @@ struct MachineClass { > >> unsigned cpu_index); > >> const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); > >> int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); > >> + void (*init_apicid_fn)(MachineState *ms); > > it's x86 specific, so why it wasn put into PCMachineClass? > > Yes. It is x86 specific for now. I tried to make it generic function so > other OSes can use it if required(like we have done in > possible_cpu_arch_ids). It initializes functions required to build the > apicid for each CPUs. We need these functions much early in the > initialization. It should be initialized before parse_numa_opts or > machine_run_board_init(in v1.c) which are called from generic context. We > cannot use PCMachineClass at this time. could you point to specific patches in this series that require apic ids being initialized before parse_numa_opts and elaborate why? we already have possible_cpu_arch_ids() which could be called very early and calculates APIC IDs in x86 case, so why not reuse it? > > > > > > >> }; > >> > >> /** > >> diff --git a/vl.c b/vl.c > >> index a42c24a77f..b6af604e11 100644 > >> --- a/vl.c > >> +++ b/vl.c > >> @@ -4318,6 +4318,9 @@ int main(int argc, char **argv, char **envp) > >> current_machine->cpu_type = machine_class->default_cpu_type; > >> if (cpu_option) { > >> current_machine->cpu_type = parse_cpu_option(cpu_option); > >> + if (machine_class->init_apicid_fn) { > >> + machine_class->init_apicid_fn(current_machine); > >> + } > >> } > >> parse_numa_opts(current_machine); > >> > >> > >> > > >
On 1/29/20 3:14 AM, Igor Mammedov wrote: > On Tue, 28 Jan 2020 13:45:31 -0600 > Babu Moger <babu.moger@amd.com> wrote: > >> On 1/28/20 10:29 AM, Igor Mammedov wrote: >>> On Tue, 03 Dec 2019 18:37:42 -0600 >>> Babu Moger <babu.moger@amd.com> wrote: >>> >>>> Add a new function init_apicid_fn in MachineClass to initialize the mode >>>> specific handlers to decode the apic ids. >>>> >>>> Signed-off-by: Babu Moger <babu.moger@amd.com> >>>> --- >>>> include/hw/boards.h | 1 + >>>> vl.c | 3 +++ >>>> 2 files changed, 4 insertions(+) >>>> >>>> diff --git a/include/hw/boards.h b/include/hw/boards.h >>>> index d4fab218e6..ce5aa365cb 100644 >>>> --- a/include/hw/boards.h >>>> +++ b/include/hw/boards.h >>>> @@ -238,6 +238,7 @@ struct MachineClass { >>>> unsigned cpu_index); >>>> const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); >>>> int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); >>>> + void (*init_apicid_fn)(MachineState *ms); >>> it's x86 specific, so why it wasn put into PCMachineClass? >> >> Yes. It is x86 specific for now. I tried to make it generic function so >> other OSes can use it if required(like we have done in >> possible_cpu_arch_ids). It initializes functions required to build the >> apicid for each CPUs. We need these functions much early in the >> initialization. It should be initialized before parse_numa_opts or >> machine_run_board_init(in v1.c) which are called from generic context. We >> cannot use PCMachineClass at this time. > > could you point to specific patches in this series that require > apic ids being initialized before parse_numa_opts and elaborate why? > > we already have possible_cpu_arch_ids() which could be called very > early and calculates APIC IDs in x86 case, so why not reuse it? The current code(before this series) parses the numa information and then sequentially builds the apicid. Both are done together. But this series separates the numa parsing and apicid generation. Numa parsing is done first and after that the apicid is generated. Reason is we need to know the number of numa nodes in advance to decode the apicid. Look at this patch. https://lore.kernel.org/qemu-devel/157541988471.46157.6587693720990965800.stgit@naples-babu.amd.com/ static inline apic_id_t apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, + const X86CPUTopoIDs *topo_ids) +{ + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | + (topo_ids->llc_id << apicid_llc_offset_epyc(topo_info)) | + (topo_ids->die_id << apicid_die_offset(topo_info)) | + (topo_ids->core_id << apicid_core_offset(topo_info)) | + topo_ids->smt_id; +} The function apicid_from_topo_ids_epyc builds the apicid. New decode adds llc_id(which is numa id here) to the current decoding. Other fields are mostly remains same. Details from the bug https://bugzilla.redhat.com/show_bug.cgi?id=1728166 Processor Programming Reference (PPR) for AMD Family 17h Model 01h, Revision B1 Processors: """ 2.1.10.2.1.3 ApicId Enumeration Requirements Operating systems are expected to use Core::X86::Cpuid::SizeId[ApicIdCoreIdSize], the number of least significant bits in the Initial APIC ID that indicate core ID within a processor, in constructing per-core CPUID masks. Core::X86::Cpuid::SizeId[ApicIdCoreIdSize] determines the maximum number of cores (MNC) that the processor could theoretically support, not the actual number of cores that are actually implemented or enabled on the processor, as indicated by Core::X86::Cpuid::SizeId[NC]. Each Core::X86::Apic::ApicId[ApicId] register is preset as follows: • ApicId[6] = Socket ID. • ApicId[5:4] = Node ID. • ApicId[3] = Logical CCX L3 complex ID • ApicId[2:0]= (SMT) ? {LogicalCoreID[1:0],ThreadId} : {1'b0,LogicalCoreID[1:0]}. """ > >> >>> >>> >>>> }; >>>> >>>> /** >>>> diff --git a/vl.c b/vl.c >>>> index a42c24a77f..b6af604e11 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -4318,6 +4318,9 @@ int main(int argc, char **argv, char **envp) >>>> current_machine->cpu_type = machine_class->default_cpu_type; >>>> if (cpu_option) { >>>> current_machine->cpu_type = parse_cpu_option(cpu_option); >>>> + if (machine_class->init_apicid_fn) { >>>> + machine_class->init_apicid_fn(current_machine); >>>> + } >>>> } >>>> parse_numa_opts(current_machine); >>>> >>>> >>>> >>> >> >
On 1/29/20 3:14 AM, Igor Mammedov wrote: > On Tue, 28 Jan 2020 13:45:31 -0600 > Babu Moger <babu.moger@amd.com> wrote: > >> On 1/28/20 10:29 AM, Igor Mammedov wrote: >>> On Tue, 03 Dec 2019 18:37:42 -0600 >>> Babu Moger <babu.moger@amd.com> wrote: >>> >>>> Add a new function init_apicid_fn in MachineClass to initialize the mode >>>> specific handlers to decode the apic ids. >>>> >>>> Signed-off-by: Babu Moger <babu.moger@amd.com> >>>> --- >>>> include/hw/boards.h | 1 + >>>> vl.c | 3 +++ >>>> 2 files changed, 4 insertions(+) >>>> >>>> diff --git a/include/hw/boards.h b/include/hw/boards.h >>>> index d4fab218e6..ce5aa365cb 100644 >>>> --- a/include/hw/boards.h >>>> +++ b/include/hw/boards.h >>>> @@ -238,6 +238,7 @@ struct MachineClass { >>>> unsigned cpu_index); >>>> const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); >>>> int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); >>>> + void (*init_apicid_fn)(MachineState *ms); >>> it's x86 specific, so why it wasn put into PCMachineClass? >> >> Yes. It is x86 specific for now. I tried to make it generic function so >> other OSes can use it if required(like we have done in >> possible_cpu_arch_ids). It initializes functions required to build the >> apicid for each CPUs. We need these functions much early in the >> initialization. It should be initialized before parse_numa_opts or >> machine_run_board_init(in v1.c) which are called from generic context. We >> cannot use PCMachineClass at this time. > > could you point to specific patches in this series that require > apic ids being initialized before parse_numa_opts and elaborate why? > > we already have possible_cpu_arch_ids() which could be called very > early and calculates APIC IDs in x86 case, so why not reuse it? Forgot to respond to this. The possible_cpu_arch_ids does not use the numa information to build the apic id. We cannot re-use it without changing it drastically. > >> >>> >>> >>>> }; >>>> >>>> /** >>>> diff --git a/vl.c b/vl.c >>>> index a42c24a77f..b6af604e11 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -4318,6 +4318,9 @@ int main(int argc, char **argv, char **envp) >>>> current_machine->cpu_type = machine_class->default_cpu_type; >>>> if (cpu_option) { >>>> current_machine->cpu_type = parse_cpu_option(cpu_option); >>>> + if (machine_class->init_apicid_fn) { >>>> + machine_class->init_apicid_fn(current_machine); >>>> + } >>>> } >>>> parse_numa_opts(current_machine); >>>> >>>> >>>> >>> >> >
On Wed, Jan 29, 2020 at 10:32:01AM -0600, Babu Moger wrote: > > > On 1/29/20 3:14 AM, Igor Mammedov wrote: > > On Tue, 28 Jan 2020 13:45:31 -0600 > > Babu Moger <babu.moger@amd.com> wrote: > > > >> On 1/28/20 10:29 AM, Igor Mammedov wrote: > >>> On Tue, 03 Dec 2019 18:37:42 -0600 > >>> Babu Moger <babu.moger@amd.com> wrote: > >>> > >>>> Add a new function init_apicid_fn in MachineClass to initialize the mode > >>>> specific handlers to decode the apic ids. > >>>> > >>>> Signed-off-by: Babu Moger <babu.moger@amd.com> > >>>> --- > >>>> include/hw/boards.h | 1 + > >>>> vl.c | 3 +++ > >>>> 2 files changed, 4 insertions(+) > >>>> > >>>> diff --git a/include/hw/boards.h b/include/hw/boards.h > >>>> index d4fab218e6..ce5aa365cb 100644 > >>>> --- a/include/hw/boards.h > >>>> +++ b/include/hw/boards.h > >>>> @@ -238,6 +238,7 @@ struct MachineClass { > >>>> unsigned cpu_index); > >>>> const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); > >>>> int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); > >>>> + void (*init_apicid_fn)(MachineState *ms); > >>> it's x86 specific, so why it wasn put into PCMachineClass? > >> > >> Yes. It is x86 specific for now. I tried to make it generic function so > >> other OSes can use it if required(like we have done in > >> possible_cpu_arch_ids). It initializes functions required to build the > >> apicid for each CPUs. We need these functions much early in the > >> initialization. It should be initialized before parse_numa_opts or > >> machine_run_board_init(in v1.c) which are called from generic context. We > >> cannot use PCMachineClass at this time. > > > > could you point to specific patches in this series that require > > apic ids being initialized before parse_numa_opts and elaborate why? > > > > we already have possible_cpu_arch_ids() which could be called very > > early and calculates APIC IDs in x86 case, so why not reuse it? > > Forgot to respond to this. The possible_cpu_arch_ids does not use the numa > information to build the apic id. We cannot re-use it without changing it > drastically. I don't get it. I see multiple patches in this series changing pc_possible_cpu_arch_ids() (which is really expected, if you are changing how APIC IDs are generated).
On 1/29/20 10:51 AM, Eduardo Habkost wrote: > On Wed, Jan 29, 2020 at 10:32:01AM -0600, Babu Moger wrote: >> >> >> On 1/29/20 3:14 AM, Igor Mammedov wrote: >>> On Tue, 28 Jan 2020 13:45:31 -0600 >>> Babu Moger <babu.moger@amd.com> wrote: >>> >>>> On 1/28/20 10:29 AM, Igor Mammedov wrote: >>>>> On Tue, 03 Dec 2019 18:37:42 -0600 >>>>> Babu Moger <babu.moger@amd.com> wrote: >>>>> >>>>>> Add a new function init_apicid_fn in MachineClass to initialize the mode >>>>>> specific handlers to decode the apic ids. >>>>>> >>>>>> Signed-off-by: Babu Moger <babu.moger@amd.com> >>>>>> --- >>>>>> include/hw/boards.h | 1 + >>>>>> vl.c | 3 +++ >>>>>> 2 files changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h >>>>>> index d4fab218e6..ce5aa365cb 100644 >>>>>> --- a/include/hw/boards.h >>>>>> +++ b/include/hw/boards.h >>>>>> @@ -238,6 +238,7 @@ struct MachineClass { >>>>>> unsigned cpu_index); >>>>>> const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); >>>>>> int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); >>>>>> + void (*init_apicid_fn)(MachineState *ms); >>>>> it's x86 specific, so why it wasn put into PCMachineClass? >>>> >>>> Yes. It is x86 specific for now. I tried to make it generic function so >>>> other OSes can use it if required(like we have done in >>>> possible_cpu_arch_ids). It initializes functions required to build the >>>> apicid for each CPUs. We need these functions much early in the >>>> initialization. It should be initialized before parse_numa_opts or >>>> machine_run_board_init(in v1.c) which are called from generic context. We >>>> cannot use PCMachineClass at this time. >>> >>> could you point to specific patches in this series that require >>> apic ids being initialized before parse_numa_opts and elaborate why? >>> >>> we already have possible_cpu_arch_ids() which could be called very >>> early and calculates APIC IDs in x86 case, so why not reuse it? >> >> Forgot to respond to this. The possible_cpu_arch_ids does not use the numa >> information to build the apic id. We cannot re-use it without changing it >> drastically. > > I don't get it. I see multiple patches in this series changing > pc_possible_cpu_arch_ids() (which is really expected, if you are > changing how APIC IDs are generated). > My bad. I mispoke on that.I should have said the current decoding logic(x86_apicid_from_cpu_idx, x86_topo_ids_from_apicid, x86_apicid_from_topo_ids) cannot be used as is.
On Wed, 29 Jan 2020 10:17:11 -0600 Babu Moger <babu.moger@amd.com> wrote: > On 1/29/20 3:14 AM, Igor Mammedov wrote: > > On Tue, 28 Jan 2020 13:45:31 -0600 > > Babu Moger <babu.moger@amd.com> wrote: > > > >> On 1/28/20 10:29 AM, Igor Mammedov wrote: > >>> On Tue, 03 Dec 2019 18:37:42 -0600 > >>> Babu Moger <babu.moger@amd.com> wrote: > >>> > >>>> Add a new function init_apicid_fn in MachineClass to initialize the mode > >>>> specific handlers to decode the apic ids. > >>>> > >>>> Signed-off-by: Babu Moger <babu.moger@amd.com> > >>>> --- > >>>> include/hw/boards.h | 1 + > >>>> vl.c | 3 +++ > >>>> 2 files changed, 4 insertions(+) > >>>> > >>>> diff --git a/include/hw/boards.h b/include/hw/boards.h > >>>> index d4fab218e6..ce5aa365cb 100644 > >>>> --- a/include/hw/boards.h > >>>> +++ b/include/hw/boards.h > >>>> @@ -238,6 +238,7 @@ struct MachineClass { > >>>> unsigned cpu_index); > >>>> const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); > >>>> int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); > >>>> + void (*init_apicid_fn)(MachineState *ms); > >>> it's x86 specific, so why it wasn put into PCMachineClass? > >> > >> Yes. It is x86 specific for now. I tried to make it generic function so > >> other OSes can use it if required(like we have done in > >> possible_cpu_arch_ids). It initializes functions required to build the > >> apicid for each CPUs. We need these functions much early in the > >> initialization. It should be initialized before parse_numa_opts or > >> machine_run_board_init(in v1.c) which are called from generic context. We > >> cannot use PCMachineClass at this time. > > > > could you point to specific patches in this series that require > > apic ids being initialized before parse_numa_opts and elaborate why? > > > > we already have possible_cpu_arch_ids() which could be called very > > early and calculates APIC IDs in x86 case, so why not reuse it? > > > The current code(before this series) parses the numa information and then > sequentially builds the apicid. Both are done together. > > But this series separates the numa parsing and apicid generation. Numa > parsing is done first and after that the apicid is generated. Reason is we > need to know the number of numa nodes in advance to decode the apicid. > > Look at this patch. > https://lore.kernel.org/qemu-devel/157541988471.46157.6587693720990965800.stgit@naples-babu.amd.com/ > > static inline apic_id_t apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > + const X86CPUTopoIDs > *topo_ids) > +{ > + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > + (topo_ids->llc_id << apicid_llc_offset_epyc(topo_info)) | > + (topo_ids->die_id << apicid_die_offset(topo_info)) | > + (topo_ids->core_id << apicid_core_offset(topo_info)) | > + topo_ids->smt_id; > +} > > > The function apicid_from_topo_ids_epyc builds the apicid. New decode adds > llc_id(which is numa id here) to the current decoding. Other fields are > mostly remains same. If llc_id is the same as numa id, why not reuse CpuInstanceProperties::node-id instead of llc_id you are adding in previous patch 6/18? > > > Details from the bug https://bugzilla.redhat.com/show_bug.cgi?id=1728166 > > Processor Programming Reference (PPR) for AMD Family 17h Model 01h, > Revision B1 Processors: > > """ > 2.1.10.2.1.3 > ApicId Enumeration Requirements > Operating systems are expected to use > Core::X86::Cpuid::SizeId[ApicIdCoreIdSize], the number of least > significant bits in the Initial APIC ID that indicate core ID within a > processor, in constructing per-core CPUID > masks. Core::X86::Cpuid::SizeId[ApicIdCoreIdSize] determines the maximum > number of cores (MNC) that the > processor could theoretically support, not the actual number of cores that > are actually implemented or enabled on > the processor, as indicated by Core::X86::Cpuid::SizeId[NC]. > Each Core::X86::Apic::ApicId[ApicId] register is preset as follows: > • ApicId[6] = Socket ID. > • ApicId[5:4] = Node ID. > • ApicId[3] = Logical CCX L3 complex ID > • ApicId[2:0]= (SMT) ? {LogicalCoreID[1:0],ThreadId} : > {1'b0,LogicalCoreID[1:0]}. > """ > > > > >> > >>> > >>> > >>>> }; > >>>> > >>>> /** > >>>> diff --git a/vl.c b/vl.c > >>>> index a42c24a77f..b6af604e11 100644 > >>>> --- a/vl.c > >>>> +++ b/vl.c > >>>> @@ -4318,6 +4318,9 @@ int main(int argc, char **argv, char **envp) > >>>> current_machine->cpu_type = machine_class->default_cpu_type; > >>>> if (cpu_option) { > >>>> current_machine->cpu_type = parse_cpu_option(cpu_option); > >>>> + if (machine_class->init_apicid_fn) { > >>>> + machine_class->init_apicid_fn(current_machine); > >>>> + } > >>>> } > >>>> parse_numa_opts(current_machine); > >>>> > >>>> > >>>> > >>> > >> > > >
On 2/3/20 9:17 AM, Igor Mammedov wrote: > On Wed, 29 Jan 2020 10:17:11 -0600 > Babu Moger <babu.moger@amd.com> wrote: > >> On 1/29/20 3:14 AM, Igor Mammedov wrote: >>> On Tue, 28 Jan 2020 13:45:31 -0600 >>> Babu Moger <babu.moger@amd.com> wrote: >>> >>>> On 1/28/20 10:29 AM, Igor Mammedov wrote: >>>>> On Tue, 03 Dec 2019 18:37:42 -0600 >>>>> Babu Moger <babu.moger@amd.com> wrote: >>>>> >>>>>> Add a new function init_apicid_fn in MachineClass to initialize the mode >>>>>> specific handlers to decode the apic ids. >>>>>> >>>>>> Signed-off-by: Babu Moger <babu.moger@amd.com> >>>>>> --- >>>>>> include/hw/boards.h | 1 + >>>>>> vl.c | 3 +++ >>>>>> 2 files changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h >>>>>> index d4fab218e6..ce5aa365cb 100644 >>>>>> --- a/include/hw/boards.h >>>>>> +++ b/include/hw/boards.h >>>>>> @@ -238,6 +238,7 @@ struct MachineClass { >>>>>> unsigned cpu_index); >>>>>> const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); >>>>>> int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); >>>>>> + void (*init_apicid_fn)(MachineState *ms); >>>>> it's x86 specific, so why it wasn put into PCMachineClass? >>>> >>>> Yes. It is x86 specific for now. I tried to make it generic function so >>>> other OSes can use it if required(like we have done in >>>> possible_cpu_arch_ids). It initializes functions required to build the >>>> apicid for each CPUs. We need these functions much early in the >>>> initialization. It should be initialized before parse_numa_opts or >>>> machine_run_board_init(in v1.c) which are called from generic context. We >>>> cannot use PCMachineClass at this time. >>> >>> could you point to specific patches in this series that require >>> apic ids being initialized before parse_numa_opts and elaborate why? >>> >>> we already have possible_cpu_arch_ids() which could be called very >>> early and calculates APIC IDs in x86 case, so why not reuse it? >> >> >> The current code(before this series) parses the numa information and then >> sequentially builds the apicid. Both are done together. >> >> But this series separates the numa parsing and apicid generation. Numa >> parsing is done first and after that the apicid is generated. Reason is we >> need to know the number of numa nodes in advance to decode the apicid. >> >> Look at this patch. >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F157541988471.46157.6587693720990965800.stgit%40naples-babu.amd.com%2F&data=02%7C01%7Cbabu.moger%40amd.com%7C0a643dd978f149acf9d108d7a8bc487a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637163398941923379&sdata=sP2TnNaqNXRGEeQNhJMna3wyeBqN0XbNKqgsCTVDaOQ%3D&reserved=0 >> >> static inline apic_id_t apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, >> + const X86CPUTopoIDs >> *topo_ids) >> +{ >> + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | >> + (topo_ids->llc_id << apicid_llc_offset_epyc(topo_info)) | >> + (topo_ids->die_id << apicid_die_offset(topo_info)) | >> + (topo_ids->core_id << apicid_core_offset(topo_info)) | >> + topo_ids->smt_id; >> +} >> >> >> The function apicid_from_topo_ids_epyc builds the apicid. New decode adds >> llc_id(which is numa id here) to the current decoding. Other fields are >> mostly remains same. > > If llc_id is the same as numa id, why not reuse CpuInstanceProperties::node-id > instead of llc_id you are adding in previous patch 6/18? > I tried to use that earlier. But dropped the idea as it required some changes. Don't remember exactly now. I am going to investigate again if we can use the node_id for our purpose here. Will let you know if I have any issues.
On Mon, 3 Feb 2020 15:49:31 -0600 Babu Moger <babu.moger@amd.com> wrote: > On 2/3/20 9:17 AM, Igor Mammedov wrote: > > On Wed, 29 Jan 2020 10:17:11 -0600 > > Babu Moger <babu.moger@amd.com> wrote: > > > >> On 1/29/20 3:14 AM, Igor Mammedov wrote: > >>> On Tue, 28 Jan 2020 13:45:31 -0600 > >>> Babu Moger <babu.moger@amd.com> wrote: > >>> > >>>> On 1/28/20 10:29 AM, Igor Mammedov wrote: > >>>>> On Tue, 03 Dec 2019 18:37:42 -0600 > >>>>> Babu Moger <babu.moger@amd.com> wrote: > >>>>> > >>>>>> Add a new function init_apicid_fn in MachineClass to initialize the mode > >>>>>> specific handlers to decode the apic ids. > >>>>>> > >>>>>> Signed-off-by: Babu Moger <babu.moger@amd.com> > >>>>>> --- > >>>>>> include/hw/boards.h | 1 + > >>>>>> vl.c | 3 +++ > >>>>>> 2 files changed, 4 insertions(+) > >>>>>> > >>>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h > >>>>>> index d4fab218e6..ce5aa365cb 100644 > >>>>>> --- a/include/hw/boards.h > >>>>>> +++ b/include/hw/boards.h > >>>>>> @@ -238,6 +238,7 @@ struct MachineClass { > >>>>>> unsigned cpu_index); > >>>>>> const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); > >>>>>> int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); > >>>>>> + void (*init_apicid_fn)(MachineState *ms); > >>>>> it's x86 specific, so why it wasn put into PCMachineClass? > >>>> > >>>> Yes. It is x86 specific for now. I tried to make it generic function so > >>>> other OSes can use it if required(like we have done in > >>>> possible_cpu_arch_ids). It initializes functions required to build the > >>>> apicid for each CPUs. We need these functions much early in the > >>>> initialization. It should be initialized before parse_numa_opts or > >>>> machine_run_board_init(in v1.c) which are called from generic context. We > >>>> cannot use PCMachineClass at this time. > >>> > >>> could you point to specific patches in this series that require > >>> apic ids being initialized before parse_numa_opts and elaborate why? > >>> > >>> we already have possible_cpu_arch_ids() which could be called very > >>> early and calculates APIC IDs in x86 case, so why not reuse it? > >> > >> > >> The current code(before this series) parses the numa information and then > >> sequentially builds the apicid. Both are done together. > >> > >> But this series separates the numa parsing and apicid generation. Numa > >> parsing is done first and after that the apicid is generated. Reason is we > >> need to know the number of numa nodes in advance to decode the apicid. > >> > >> Look at this patch. > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F157541988471.46157.6587693720990965800.stgit%40naples-babu.amd.com%2F&data=02%7C01%7Cbabu.moger%40amd.com%7C0a643dd978f149acf9d108d7a8bc487a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637163398941923379&sdata=sP2TnNaqNXRGEeQNhJMna3wyeBqN0XbNKqgsCTVDaOQ%3D&reserved=0 > >> > >> static inline apic_id_t apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > >> + const X86CPUTopoIDs > >> *topo_ids) > >> +{ > >> + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > >> + (topo_ids->llc_id << apicid_llc_offset_epyc(topo_info)) | > >> + (topo_ids->die_id << apicid_die_offset(topo_info)) | > >> + (topo_ids->core_id << apicid_core_offset(topo_info)) | > >> + topo_ids->smt_id; > >> +} > >> > >> > >> The function apicid_from_topo_ids_epyc builds the apicid. New decode adds > >> llc_id(which is numa id here) to the current decoding. Other fields are > >> mostly remains same. > > > > If llc_id is the same as numa id, why not reuse CpuInstanceProperties::node-id > > instead of llc_id you are adding in previous patch 6/18? > > > I tried to use that earlier. But dropped the idea as it required some > changes. Don't remember exactly now. I am going to investigate again if we > can use the node_id for our purpose here. Will let you know if I have any > issues. The reason I'm asking to not add new properties here is that it expands interface visible/used by management tools and it's maintenance burden not only on QEMU but on engagement side as well. So if yo can reuse node-id, it will work out of box with existing users. It should also be less confusing for us since we don't have to keep in mind (or figure out) that llc_id is the same as node id and wonder why the later wasn't used in the first place.
diff --git a/include/hw/boards.h b/include/hw/boards.h index d4fab218e6..ce5aa365cb 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -238,6 +238,7 @@ struct MachineClass { unsigned cpu_index); const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); + void (*init_apicid_fn)(MachineState *ms); }; /** diff --git a/vl.c b/vl.c index a42c24a77f..b6af604e11 100644 --- a/vl.c +++ b/vl.c @@ -4318,6 +4318,9 @@ int main(int argc, char **argv, char **envp) current_machine->cpu_type = machine_class->default_cpu_type; if (cpu_option) { current_machine->cpu_type = parse_cpu_option(cpu_option); + if (machine_class->init_apicid_fn) { + machine_class->init_apicid_fn(current_machine); + } } parse_numa_opts(current_machine);
Add a new function init_apicid_fn in MachineClass to initialize the mode specific handlers to decode the apic ids. Signed-off-by: Babu Moger <babu.moger@amd.com> --- include/hw/boards.h | 1 + vl.c | 3 +++ 2 files changed, 4 insertions(+)