| Message ID | 158396721426.58170.2930696192478912976.stgit@naples-babu.amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | APIC ID fixes for AMD EPYC CPU model | expand |
On Wed, 11 Mar 2020 17:53:34 -0500 Babu Moger <babu.moger@amd.com> wrote: > These functions add support for building EPYC mode topology given the smp > details like numa nodes, cores, threads and sockets. > > The new apic id decoding is mostly similar to current apic id decoding > except that it adds a new field node_id when numa configured. Removes all > the hardcoded values. Subsequent patches will use these functions to build > the topology. > > Following functions are added. [...] > x86_topo_ids_from_idx_epyc you forgot to remove unused anymore function. No need to respin whole series for it though, you can post as reply to this patch v8 or do it as a patch on top. [...] > > The topology details are available in Processor Programming Reference (PPR) > for AMD Family 17h Model 01h, Revision B1 Processors. The revision guides are > available from the bugzilla Link below. > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537 > > Signed-off-by: Babu Moger <babu.moger@amd.com> > Acked-by: Igor Mammedov <imammedo@redhat.com> > Acked-by: Michael S. Tsirkin <mst@redhat.com> [...] > +static inline void x86_topo_ids_from_idx_epyc(X86CPUTopoInfo *topo_info, > + unsigned cpu_index, > + X86CPUTopoIDs *topo_ids) > +{ > + unsigned nr_nodes = MAX(topo_info->nodes_per_pkg, 1); > + unsigned nr_dies = topo_info->dies_per_pkg; > + unsigned nr_cores = topo_info->cores_per_die; > + unsigned nr_threads = topo_info->threads_per_core; > + unsigned cores_per_node = DIV_ROUND_UP((nr_dies * nr_cores * nr_threads), > + nr_nodes); > + > + topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads); > + topo_ids->node_id = (cpu_index / cores_per_node) % nr_nodes; > + topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies; > + topo_ids->core_id = cpu_index / nr_threads % nr_cores; > + topo_ids->smt_id = cpu_index % nr_threads; > +} > + > +/* [...]
On 3/12/20 7:39 AM, Igor Mammedov wrote: > On Wed, 11 Mar 2020 17:53:34 -0500 > Babu Moger <babu.moger@amd.com> wrote: > >> These functions add support for building EPYC mode topology given the smp >> details like numa nodes, cores, threads and sockets. >> >> The new apic id decoding is mostly similar to current apic id decoding >> except that it adds a new field node_id when numa configured. Removes all >> the hardcoded values. Subsequent patches will use these functions to build >> the topology. >> >> Following functions are added. > [...] >> x86_topo_ids_from_idx_epyc > you forgot to remove unused anymore function. > No need to respin whole series for it though, you can post as reply to > this patch v8 or do it as a patch on top. Igor, The function x86_topo_ids_from_idx_epyc(or x86_topo_ids_from_idx) is still there. We are using it internally now. It is used by x86_apicid_from_cpu_idx_epyc(or x86_apicid_from_cpu_idx). We removed it as callback function. So, we are good here. Thanks > > > > > [...] >> >> The topology details are available in Processor Programming Reference (PPR) >> for AMD Family 17h Model 01h, Revision B1 Processors. The revision guides are >> available from the bugzilla Link below. >> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.moger%40amd.com%7C3d1032fb1cc94a5a197308d7c68268c5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637196135715465573&sdata=13zSN7AqPGKHFG%2FePmkWTVbwM0qzktrnolEidnNzyhs%3D&reserved=0 >> >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> Acked-by: Igor Mammedov <imammedo@redhat.com> >> Acked-by: Michael S. Tsirkin <mst@redhat.com> > [...] >> +static inline void x86_topo_ids_from_idx_epyc(X86CPUTopoInfo *topo_info, >> + unsigned cpu_index, >> + X86CPUTopoIDs *topo_ids) >> +{ >> + unsigned nr_nodes = MAX(topo_info->nodes_per_pkg, 1); >> + unsigned nr_dies = topo_info->dies_per_pkg; >> + unsigned nr_cores = topo_info->cores_per_die; >> + unsigned nr_threads = topo_info->threads_per_core; >> + unsigned cores_per_node = DIV_ROUND_UP((nr_dies * nr_cores * nr_threads), >> + nr_nodes); >> + >> + topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads); >> + topo_ids->node_id = (cpu_index / cores_per_node) % nr_nodes; >> + topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies; >> + topo_ids->core_id = cpu_index / nr_threads % nr_cores; >> + topo_ids->smt_id = cpu_index % nr_threads; >> +} >> + >> +/* > [...] >
Hi, It looks like this series breaks -device and CPU hotplug: On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote: > These functions add support for building EPYC mode topology given the smp > details like numa nodes, cores, threads and sockets. > > The new apic id decoding is mostly similar to current apic id decoding > except that it adds a new field node_id when numa configured. Removes all > the hardcoded values. Subsequent patches will use these functions to build > the topology. > > Following functions are added. > apicid_llc_width_epyc > apicid_llc_offset_epyc > apicid_pkg_offset_epyc > apicid_from_topo_ids_epyc > x86_topo_ids_from_idx_epyc > x86_topo_ids_from_apicid_epyc > x86_apicid_from_cpu_idx_epyc > > The topology details are available in Processor Programming Reference (PPR) > for AMD Family 17h Model 01h, Revision B1 Processors. The revision guides are > available from the bugzilla Link below. > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537 > > Signed-off-by: Babu Moger <babu.moger@amd.com> > Acked-by: Igor Mammedov <imammedo@redhat.com> > Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- [...] > typedef struct X86CPUTopoIDs { > unsigned pkg_id; > + unsigned node_id; You have added a new field here. > unsigned die_id; > unsigned core_id; > unsigned smt_id; [...] > +static inline apic_id_t > +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > + const X86CPUTopoIDs *topo_ids) > +{ > + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > + (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | You are using the new field here. > + (topo_ids->die_id << apicid_die_offset(topo_info)) | > + (topo_ids->core_id << apicid_core_offset(topo_info)) | > + topo_ids->smt_id; > +} But you are not initializing node_id in one caller of apicid_from_topo_ids(): static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { [...] X86CPUTopoIDs topo_ids; [...] if (cpu->apic_id == UNASSIGNED_APIC_ID) { [...] topo_ids.pkg_id = cpu->socket_id; topo_ids.die_id = cpu->die_id; topo_ids.core_id = cpu->core_id; topo_ids.smt_id = cpu->thread_id; cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); } [...] } Result: -device is broken when using -cpu EPYC: $ qemu-system-x86_64 -machine q35,accel=kvm -smp 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64-cpu,core-id=0,socket-id=1,thread-id=0 qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket-id=1,thread-id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID 21855, valid index range 0:1 This happens because APIC ID is calculated using uninitialized memory.
Oh. Ok.. Will look at it. thanks > -----Original Message----- > From: Eduardo Habkost <ehabkost@redhat.com> > Sent: Tuesday, June 2, 2020 12:19 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > functions > > Hi, > > It looks like this series breaks -device and CPU hotplug: > > On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote: > > These functions add support for building EPYC mode topology given the smp > > details like numa nodes, cores, threads and sockets. > > > > The new apic id decoding is mostly similar to current apic id decoding > > except that it adds a new field node_id when numa configured. Removes all > > the hardcoded values. Subsequent patches will use these functions to build > > the topology. > > > > Following functions are added. > > apicid_llc_width_epyc > > apicid_llc_offset_epyc > > apicid_pkg_offset_epyc > > apicid_from_topo_ids_epyc > > x86_topo_ids_from_idx_epyc > > x86_topo_ids_from_apicid_epyc > > x86_apicid_from_cpu_idx_epyc > > > > The topology details are available in Processor Programming Reference (PPR) > > for AMD Family 17h Model 01h, Revision B1 Processors. The revision guides > are > > available from the bugzilla Link below. > > Link: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla. > kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.m > oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488 > 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&sdata=wE0 > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&reserved=0 > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > Acked-by: Igor Mammedov <imammedo@redhat.com> > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > --- > [...] > > typedef struct X86CPUTopoIDs { > > unsigned pkg_id; > > + unsigned node_id; > > You have added a new field here. > > > unsigned die_id; > > unsigned core_id; > > unsigned smt_id; > [...] > > +static inline apic_id_t > > +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > > + const X86CPUTopoIDs *topo_ids) > > +{ > > + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > > + (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | > > You are using the new field here. > > > + (topo_ids->die_id << apicid_die_offset(topo_info)) | > > + (topo_ids->core_id << apicid_core_offset(topo_info)) | > > + topo_ids->smt_id; > > +} > > But you are not initializing node_id in one caller of apicid_from_topo_ids(): > > static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > [...] > X86CPUTopoIDs topo_ids; > [...] > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > [...] > topo_ids.pkg_id = cpu->socket_id; > topo_ids.die_id = cpu->die_id; > topo_ids.core_id = cpu->core_id; > topo_ids.smt_id = cpu->thread_id; > cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); > } > [...] > } > > Result: -device is broken when using -cpu EPYC: > > $ qemu-system-x86_64 -machine q35,accel=kvm -smp > 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64- > cpu,core-id=0,socket-id=1,thread-id=0 > qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket-id=1,thread- > id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID 21855, > valid index range 0:1 > > This happens because APIC ID is calculated using uninitialized > memory. > > -- > Eduardo
> -----Original Message----- > From: Eduardo Habkost <ehabkost@redhat.com> > Sent: Tuesday, June 2, 2020 12:19 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > functions > > Hi, > > It looks like this series breaks -device and CPU hotplug: > > On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote: > > These functions add support for building EPYC mode topology given the smp > > details like numa nodes, cores, threads and sockets. > > > > The new apic id decoding is mostly similar to current apic id decoding > > except that it adds a new field node_id when numa configured. Removes all > > the hardcoded values. Subsequent patches will use these functions to build > > the topology. > > > > Following functions are added. > > apicid_llc_width_epyc > > apicid_llc_offset_epyc > > apicid_pkg_offset_epyc > > apicid_from_topo_ids_epyc > > x86_topo_ids_from_idx_epyc > > x86_topo_ids_from_apicid_epyc > > x86_apicid_from_cpu_idx_epyc > > > > The topology details are available in Processor Programming Reference (PPR) > > for AMD Family 17h Model 01h, Revision B1 Processors. The revision guides > are > > available from the bugzilla Link below. > > Link: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla. > kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.m > oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488 > 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&sdata=wE0 > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&reserved=0 > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > Acked-by: Igor Mammedov <imammedo@redhat.com> > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > --- > [...] > > typedef struct X86CPUTopoIDs { > > unsigned pkg_id; > > + unsigned node_id; > > You have added a new field here. > > > unsigned die_id; > > unsigned core_id; > > unsigned smt_id; > [...] > > +static inline apic_id_t > > +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > > + const X86CPUTopoIDs *topo_ids) > > +{ > > + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > > + (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | > > You are using the new field here. > > > + (topo_ids->die_id << apicid_die_offset(topo_info)) | > > + (topo_ids->core_id << apicid_core_offset(topo_info)) | > > + topo_ids->smt_id; > > +} > > But you are not initializing node_id in one caller of apicid_from_topo_ids(): > > static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > [...] > X86CPUTopoIDs topo_ids; > [...] > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > [...] > topo_ids.pkg_id = cpu->socket_id; > topo_ids.die_id = cpu->die_id; > topo_ids.core_id = cpu->core_id; > topo_ids.smt_id = cpu->thread_id; > cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); > } > [...] > } > > Result: -device is broken when using -cpu EPYC: > > $ qemu-system-x86_64 -machine q35,accel=kvm -smp > 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64- > cpu,core-id=0,socket-id=1,thread-id=0 > qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket-id=1,thread- > id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID 21855, > valid index range 0:1 > > This happens because APIC ID is calculated using uninitialized > memory. This patch should initialize the node_id. But I am not sure how to reproduce the bug. Can you please send me the full command line to reproduce the problem. Also test different options. diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 2128f3d6fe..047b4b9391 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1539,6 +1539,9 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, if (cpu->apic_id == UNASSIGNED_APIC_ID) { int max_socket = (ms->smp.max_cpus - 1) / smp_threads / smp_cores / x86ms->smp_dies; + unsigned nr_nodes = MAX(topo_info.nodes_per_pkg, 1); + unsigned cores_per_node = DIV_ROUND_UP((x86ms->smp_dies * smp_cores * + smp_threads), nr_nodes); /* * die-id was optional in QEMU 4.0 and older, so keep it optional @@ -1585,6 +1588,7 @@ 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; + topo_ids.node_id = (cpu->core_id / cores_per_node) % nr_nodes; cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); }
On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote: > > > > -----Original Message----- > > From: Eduardo Habkost <ehabkost@redhat.com> > > Sent: Tuesday, June 2, 2020 12:19 PM > > To: Moger, Babu <Babu.Moger@amd.com> > > Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > > mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > > functions > > > > Hi, > > > > It looks like this series breaks -device and CPU hotplug: > > > > On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote: > > > These functions add support for building EPYC mode topology given the smp > > > details like numa nodes, cores, threads and sockets. > > > > > > The new apic id decoding is mostly similar to current apic id decoding > > > except that it adds a new field node_id when numa configured. Removes all > > > the hardcoded values. Subsequent patches will use these functions to build > > > the topology. > > > > > > Following functions are added. > > > apicid_llc_width_epyc > > > apicid_llc_offset_epyc > > > apicid_pkg_offset_epyc > > > apicid_from_topo_ids_epyc > > > x86_topo_ids_from_idx_epyc > > > x86_topo_ids_from_apicid_epyc > > > x86_apicid_from_cpu_idx_epyc > > > > > > The topology details are available in Processor Programming Reference (PPR) > > > for AMD Family 17h Model 01h, Revision B1 Processors. The revision guides > > are > > > available from the bugzilla Link below. > > > Link: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla. > > kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.m > > oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488 > > 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&sdata=wE0 > > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&reserved=0 > > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > > Acked-by: Igor Mammedov <imammedo@redhat.com> > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > --- > > [...] > > > typedef struct X86CPUTopoIDs { > > > unsigned pkg_id; > > > + unsigned node_id; > > > > You have added a new field here. > > > > > unsigned die_id; > > > unsigned core_id; > > > unsigned smt_id; > > [...] > > > +static inline apic_id_t > > > +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > > > + const X86CPUTopoIDs *topo_ids) > > > +{ > > > + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > > > + (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | > > > > You are using the new field here. > > > > > + (topo_ids->die_id << apicid_die_offset(topo_info)) | > > > + (topo_ids->core_id << apicid_core_offset(topo_info)) | > > > + topo_ids->smt_id; > > > +} > > > > But you are not initializing node_id in one caller of apicid_from_topo_ids(): > > > > static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > > DeviceState *dev, Error **errp) > > { > > [...] > > X86CPUTopoIDs topo_ids; > > [...] > > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > [...] > > topo_ids.pkg_id = cpu->socket_id; > > topo_ids.die_id = cpu->die_id; > > topo_ids.core_id = cpu->core_id; > > topo_ids.smt_id = cpu->thread_id; > > cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); > > } > > [...] > > } > > > > Result: -device is broken when using -cpu EPYC: > > > > $ qemu-system-x86_64 -machine q35,accel=kvm -smp > > 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64- > > cpu,core-id=0,socket-id=1,thread-id=0 [1] > > qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket-id=1,thread- > > id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID 21855, > > valid index range 0:1 > > > > This happens because APIC ID is calculated using uninitialized > > memory. > This patch should initialize the node_id. But I am not sure how to > reproduce the bug. Can you please send me the full command line to > reproduce the problem. Also test different options. The full command line is above[1]. > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 2128f3d6fe..047b4b9391 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1539,6 +1539,9 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > int max_socket = (ms->smp.max_cpus - 1) / > smp_threads / smp_cores / x86ms->smp_dies; So, here's the input you are using to calculate topo_ids.node_id: > + unsigned nr_nodes = MAX(topo_info.nodes_per_pkg, 1); When is topo_info.nodes_per_pkg allowed to be 0? > + unsigned cores_per_node = DIV_ROUND_UP((x86ms->smp_dies * smp_cores * > + smp_threads), nr_nodes); x86ms->smp_dies should be available at topo_info.dies_per_pkg, smp_cores should available at topo_info.cores_per_die, smp_threads should be available at topo_info.threads_per_core, nr_nodes should be available at topo_info.nodes_per_pkg. > > /* > * die-id was optional in QEMU 4.0 and older, so keep it optional > @@ -1585,6 +1588,7 @@ 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; > + topo_ids.node_id = (cpu->core_id / cores_per_node) % nr_nodes; apicid_from_topo_ids() have access to topo_info and topo_ids, If all the information you need to calculate node_id is already available inside topo_info + topo_ids, we could be calculating it inside apicid_from_topo_ids(). Why don't we do it? Also, is topo_ids.core_id really allowed to be larger than cores_per_node when calling apicid_from_topo_ids()? > cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); > } >
> -----Original Message----- > From: Eduardo Habkost <ehabkost@redhat.com> > Sent: Tuesday, June 2, 2020 6:01 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > functions > > On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote: > > > > > > > -----Original Message----- > > > From: Eduardo Habkost <ehabkost@redhat.com> > > > Sent: Tuesday, June 2, 2020 12:19 PM > > > To: Moger, Babu <Babu.Moger@amd.com> > > > Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > > > mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > > > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > > > functions > > > > > > Hi, > > > > > > It looks like this series breaks -device and CPU hotplug: > > > > > > On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote: > > > > These functions add support for building EPYC mode topology given the > smp > > > > details like numa nodes, cores, threads and sockets. > > > > > > > > The new apic id decoding is mostly similar to current apic id decoding > > > > except that it adds a new field node_id when numa configured. Removes > all > > > > the hardcoded values. Subsequent patches will use these functions to build > > > > the topology. > > > > > > > > Following functions are added. > > > > apicid_llc_width_epyc > > > > apicid_llc_offset_epyc > > > > apicid_pkg_offset_epyc > > > > apicid_from_topo_ids_epyc > > > > x86_topo_ids_from_idx_epyc > > > > x86_topo_ids_from_apicid_epyc > > > > x86_apicid_from_cpu_idx_epyc > > > > > > > > The topology details are available in Processor Programming Reference > (PPR) > > > > for AMD Family 17h Model 01h, Revision B1 Processors. The revision > guides > > > are > > > > available from the bugzilla Link below. > > > > Link: > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla. > > > > kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.m > > > > oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488 > > > > 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&sdata=wE0 > > > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&reserved=0 > > > > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > > > Acked-by: Igor Mammedov <imammedo@redhat.com> > > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > > --- > > > [...] > > > > typedef struct X86CPUTopoIDs { > > > > unsigned pkg_id; > > > > + unsigned node_id; > > > > > > You have added a new field here. > > > > > > > unsigned die_id; > > > > unsigned core_id; > > > > unsigned smt_id; > > > [...] > > > > +static inline apic_id_t > > > > +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > > > > + const X86CPUTopoIDs *topo_ids) > > > > +{ > > > > + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > > > > + (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | > > > > > > You are using the new field here. > > > > > > > + (topo_ids->die_id << apicid_die_offset(topo_info)) | > > > > + (topo_ids->core_id << apicid_core_offset(topo_info)) | > > > > + topo_ids->smt_id; > > > > +} > > > > > > But you are not initializing node_id in one caller of apicid_from_topo_ids(): > > > > > > static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > > > DeviceState *dev, Error **errp) > > > { > > > [...] > > > X86CPUTopoIDs topo_ids; > > > [...] > > > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > > [...] > > > topo_ids.pkg_id = cpu->socket_id; > > > topo_ids.die_id = cpu->die_id; > > > topo_ids.core_id = cpu->core_id; > > > topo_ids.smt_id = cpu->thread_id; > > > cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); > > > } > > > [...] > > > } > > > > > > Result: -device is broken when using -cpu EPYC: > > > > > > $ qemu-system-x86_64 -machine q35,accel=kvm -smp > > > 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64- > > > cpu,core-id=0,socket-id=1,thread-id=0 > > [1] > > > > qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket- > id=1,thread- > > > id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID > 21855, > > > valid index range 0:1 > > > > > > This happens because APIC ID is calculated using uninitialized > > > memory. > > This patch should initialize the node_id. But I am not sure how to > > reproduce the bug. Can you please send me the full command line to > > reproduce the problem. Also test different options. > > The full command line is above[1]. > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 2128f3d6fe..047b4b9391 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1539,6 +1539,9 @@ static void pc_cpu_pre_plug(HotplugHandler > *hotplug_dev, > > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > int max_socket = (ms->smp.max_cpus - 1) / > > smp_threads / smp_cores / x86ms->smp_dies; > > So, here's the input you are using to calculate topo_ids.node_id: > > > + unsigned nr_nodes = MAX(topo_info.nodes_per_pkg, 1); > > When is topo_info.nodes_per_pkg allowed to be 0? > > > + unsigned cores_per_node = DIV_ROUND_UP((x86ms->smp_dies * > smp_cores * > > + smp_threads), nr_nodes); > > x86ms->smp_dies should be available at topo_info.dies_per_pkg, > smp_cores should available at topo_info.cores_per_die, > smp_threads should be available at topo_info.threads_per_core, > nr_nodes should be available at topo_info.nodes_per_pkg. > > > > > /* > > * die-id was optional in QEMU 4.0 and older, so keep it optional > > @@ -1585,6 +1588,7 @@ 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; > > + topo_ids.node_id = (cpu->core_id / cores_per_node) % nr_nodes; > > apicid_from_topo_ids() have access to topo_info and topo_ids, If > all the information you need to calculate node_id is already > available inside topo_info + topo_ids, we could be calculating it > inside apicid_from_topo_ids(). Why don't we do it? Sure. I will try it. Will post revised patch tomorrow. > > Also, is topo_ids.core_id really allowed to be larger than > cores_per_node when calling apicid_from_topo_ids()? > > > cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); > > } > > > > -- > Eduardo
> -----Original Message----- > From: Eduardo Habkost <ehabkost@redhat.com> > Sent: Tuesday, June 2, 2020 6:01 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > functions > > On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote: > > > > > > > -----Original Message----- > > > From: Eduardo Habkost <ehabkost@redhat.com> > > > Sent: Tuesday, June 2, 2020 12:19 PM > > > To: Moger, Babu <Babu.Moger@amd.com> > > > Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > > > mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > > > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > > > functions > > > > > > Hi, > > > > > > It looks like this series breaks -device and CPU hotplug: > > > > > > On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote: > > > > These functions add support for building EPYC mode topology given the > smp > > > > details like numa nodes, cores, threads and sockets. > > > > > > > > The new apic id decoding is mostly similar to current apic id decoding > > > > except that it adds a new field node_id when numa configured. Removes > all > > > > the hardcoded values. Subsequent patches will use these functions to build > > > > the topology. > > > > > > > > Following functions are added. > > > > apicid_llc_width_epyc > > > > apicid_llc_offset_epyc > > > > apicid_pkg_offset_epyc > > > > apicid_from_topo_ids_epyc > > > > x86_topo_ids_from_idx_epyc > > > > x86_topo_ids_from_apicid_epyc > > > > x86_apicid_from_cpu_idx_epyc > > > > > > > > The topology details are available in Processor Programming Reference > (PPR) > > > > for AMD Family 17h Model 01h, Revision B1 Processors. The revision > guides > > > are > > > > available from the bugzilla Link below. > > > > Link: > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla. > > > > kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.m > > > > oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488 > > > > 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&sdata=wE0 > > > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&reserved=0 > > > > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > > > Acked-by: Igor Mammedov <imammedo@redhat.com> > > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > > --- > > > [...] > > > > typedef struct X86CPUTopoIDs { > > > > unsigned pkg_id; > > > > + unsigned node_id; > > > > > > You have added a new field here. > > > > > > > unsigned die_id; > > > > unsigned core_id; > > > > unsigned smt_id; > > > [...] > > > > +static inline apic_id_t > > > > +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > > > > + const X86CPUTopoIDs *topo_ids) > > > > +{ > > > > + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > > > > + (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | > > > > > > You are using the new field here. > > > > > > > + (topo_ids->die_id << apicid_die_offset(topo_info)) | > > > > + (topo_ids->core_id << apicid_core_offset(topo_info)) | > > > > + topo_ids->smt_id; > > > > +} > > > > > > But you are not initializing node_id in one caller of apicid_from_topo_ids(): > > > > > > static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > > > DeviceState *dev, Error **errp) > > > { > > > [...] > > > X86CPUTopoIDs topo_ids; > > > [...] > > > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > > [...] > > > topo_ids.pkg_id = cpu->socket_id; > > > topo_ids.die_id = cpu->die_id; > > > topo_ids.core_id = cpu->core_id; > > > topo_ids.smt_id = cpu->thread_id; > > > cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); > > > } > > > [...] > > > } > > > > > > Result: -device is broken when using -cpu EPYC: > > > > > > $ qemu-system-x86_64 -machine q35,accel=kvm -smp > > > 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64- > > > cpu,core-id=0,socket-id=1,thread-id=0 > > [1] > > > > qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket- > id=1,thread- > > > id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID > 21855, > > > valid index range 0:1 > > > > > > This happens because APIC ID is calculated using uninitialized > > > memory. > > This patch should initialize the node_id. But I am not sure how to > > reproduce the bug. Can you please send me the full command line to > > reproduce the problem. Also test different options. > > The full command line is above[1]. I just picked up the latest tree from git clone https://git.qemu.org/git/qemu.git qemu Not seeing the problem. ./x86_64-softmmu/qemu-system-x86_64 -machine q35,accel=kvm -smp 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64-cpu,core-id=0,socket-id=1,thread-id=0 VNC server running on ::1:5900 It appears to run ok. > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 2128f3d6fe..047b4b9391 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1539,6 +1539,9 @@ static void pc_cpu_pre_plug(HotplugHandler > *hotplug_dev, > > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > int max_socket = (ms->smp.max_cpus - 1) / > > smp_threads / smp_cores / x86ms->smp_dies; > > So, here's the input you are using to calculate topo_ids.node_id: > > > + unsigned nr_nodes = MAX(topo_info.nodes_per_pkg, 1); > > When is topo_info.nodes_per_pkg allowed to be 0? It will be zero if numa is not configured. Node_id will be zero for all the cores. > > > + unsigned cores_per_node = DIV_ROUND_UP((x86ms->smp_dies * > smp_cores * > > + smp_threads), nr_nodes); > > x86ms->smp_dies should be available at topo_info.dies_per_pkg, > smp_cores should available at topo_info.cores_per_die, > smp_threads should be available at topo_info.threads_per_core, > nr_nodes should be available at topo_info.nodes_per_pkg. > > > > > /* > > * die-id was optional in QEMU 4.0 and older, so keep it optional > > @@ -1585,6 +1588,7 @@ 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; > > + topo_ids.node_id = (cpu->core_id / cores_per_node) % nr_nodes; > > apicid_from_topo_ids() have access to topo_info and topo_ids, If > all the information you need to calculate node_id is already > available inside topo_info + topo_ids, we could be calculating it > inside apicid_from_topo_ids(). Why don't we do it? > > Also, is topo_ids.core_id really allowed to be larger than > cores_per_node when calling apicid_from_topo_ids()? Yes. It is. If we have two numa nodes and 8 cores. Then cores_per_node is 4. Nr_nodes =2. Yes. Core_id can be larger than cores_per_node. > > > cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); > > } > > > > -- > Eduardo
On Wed, Jun 03, 2020 at 10:22:10AM -0500, Babu Moger wrote: > > > > -----Original Message----- > > From: Eduardo Habkost <ehabkost@redhat.com> > > Sent: Tuesday, June 2, 2020 6:01 PM > > To: Moger, Babu <Babu.Moger@amd.com> > > Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > > mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > > functions > > > > On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote: > > > > > > > > > > -----Original Message----- > > > > From: Eduardo Habkost <ehabkost@redhat.com> > > > > Sent: Tuesday, June 2, 2020 12:19 PM > > > > To: Moger, Babu <Babu.Moger@amd.com> > > > > Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > > > > mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > > > > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > > > > functions > > > > > > > > Hi, > > > > > > > > It looks like this series breaks -device and CPU hotplug: > > > > > > > > On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote: > > > > > These functions add support for building EPYC mode topology given the > > smp > > > > > details like numa nodes, cores, threads and sockets. > > > > > > > > > > The new apic id decoding is mostly similar to current apic id decoding > > > > > except that it adds a new field node_id when numa configured. Removes > > all > > > > > the hardcoded values. Subsequent patches will use these functions to build > > > > > the topology. > > > > > > > > > > Following functions are added. > > > > > apicid_llc_width_epyc > > > > > apicid_llc_offset_epyc > > > > > apicid_pkg_offset_epyc > > > > > apicid_from_topo_ids_epyc > > > > > x86_topo_ids_from_idx_epyc > > > > > x86_topo_ids_from_apicid_epyc > > > > > x86_apicid_from_cpu_idx_epyc > > > > > > > > > > The topology details are available in Processor Programming Reference > > (PPR) > > > > > for AMD Family 17h Model 01h, Revision B1 Processors. The revision > > guides > > > > are > > > > > available from the bugzilla Link below. > > > > > Link: > > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla. > > > > > > kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.m > > > > > > oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488 > > > > > > 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&sdata=wE0 > > > > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&reserved=0 > > > > > > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > > > > Acked-by: Igor Mammedov <imammedo@redhat.com> > > > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > > > --- > > > > [...] > > > > > typedef struct X86CPUTopoIDs { > > > > > unsigned pkg_id; > > > > > + unsigned node_id; > > > > > > > > You have added a new field here. > > > > > > > > > unsigned die_id; > > > > > unsigned core_id; > > > > > unsigned smt_id; > > > > [...] > > > > > +static inline apic_id_t > > > > > +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > > > > > + const X86CPUTopoIDs *topo_ids) > > > > > +{ > > > > > + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > > > > > + (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | > > > > > > > > You are using the new field here. > > > > > > > > > + (topo_ids->die_id << apicid_die_offset(topo_info)) | > > > > > + (topo_ids->core_id << apicid_core_offset(topo_info)) | > > > > > + topo_ids->smt_id; > > > > > +} > > > > > > > > But you are not initializing node_id in one caller of apicid_from_topo_ids(): > > > > > > > > static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > > > > DeviceState *dev, Error **errp) > > > > { > > > > [...] > > > > X86CPUTopoIDs topo_ids; > > > > [...] > > > > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > > > [...] > > > > topo_ids.pkg_id = cpu->socket_id; > > > > topo_ids.die_id = cpu->die_id; > > > > topo_ids.core_id = cpu->core_id; > > > > topo_ids.smt_id = cpu->thread_id; > > > > cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); > > > > } > > > > [...] > > > > } > > > > > > > > Result: -device is broken when using -cpu EPYC: > > > > > > > > $ qemu-system-x86_64 -machine q35,accel=kvm -smp > > > > 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64- > > > > cpu,core-id=0,socket-id=1,thread-id=0 > > > > [1] > > > > > > qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket- > > id=1,thread- > > > > id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID > > 21855, > > > > valid index range 0:1 > > > > > > > > This happens because APIC ID is calculated using uninitialized > > > > memory. > > > This patch should initialize the node_id. But I am not sure how to > > > reproduce the bug. Can you please send me the full command line to > > > reproduce the problem. Also test different options. > > > > The full command line is above[1]. > > I just picked up the latest tree from > git clone https://git.qemu.org/git/qemu.git qemu > Not seeing the problem. > > ./x86_64-softmmu/qemu-system-x86_64 -machine q35,accel=kvm -smp > 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device > EPYC-x86_64-cpu,core-id=0,socket-id=1,thread-id=0 > VNC server running on ::1:5900 > > It appears to run ok. > > > > > > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index 2128f3d6fe..047b4b9391 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -1539,6 +1539,9 @@ static void pc_cpu_pre_plug(HotplugHandler > > *hotplug_dev, > > > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > > int max_socket = (ms->smp.max_cpus - 1) / > > > smp_threads / smp_cores / x86ms->smp_dies; > > > > So, here's the input you are using to calculate topo_ids.node_id: > > > > > + unsigned nr_nodes = MAX(topo_info.nodes_per_pkg, 1); > > > > When is topo_info.nodes_per_pkg allowed to be 0? > > It will be zero if numa is not configured. Node_id will be zero for all > the cores. > > > > > > + unsigned cores_per_node = DIV_ROUND_UP((x86ms->smp_dies * > > smp_cores * > > > + smp_threads), nr_nodes); > > > > x86ms->smp_dies should be available at topo_info.dies_per_pkg, > > smp_cores should available at topo_info.cores_per_die, > > smp_threads should be available at topo_info.threads_per_core, > > nr_nodes should be available at topo_info.nodes_per_pkg. > > > > > > > > /* > > > * die-id was optional in QEMU 4.0 and older, so keep it optional > > > @@ -1585,6 +1588,7 @@ 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; > > > + topo_ids.node_id = (cpu->core_id / cores_per_node) % nr_nodes; > > > > apicid_from_topo_ids() have access to topo_info and topo_ids, If > > all the information you need to calculate node_id is already > > available inside topo_info + topo_ids, we could be calculating it > > inside apicid_from_topo_ids(). Why don't we do it? > > > > Also, is topo_ids.core_id really allowed to be larger than > > cores_per_node when calling apicid_from_topo_ids()? > > Yes. It is. If we have two numa nodes and 8 cores. Then cores_per_node is > 4. Nr_nodes =2. Yes. Core_id can be larger than cores_per_node. I assumed core_id identified the core inside a die, and the range would be [0, cores_per_die). This seems to be what apicid_from_topo_ids_epyc() expects. We need clearer documentation on the semantics of each *_id field, to avoid confusion. > > > > > > cpu->apic_id = > > > x86ms->apicid_from_topo_ids(&topo_info, > > > &topo_ids); } > > > > > > > -- Eduardo >
On 6/3/20 10:31 AM, Eduardo Habkost wrote: > On Wed, Jun 03, 2020 at 10:22:10AM -0500, Babu Moger wrote: >> >> >>> -----Original Message----- >>> From: Eduardo Habkost <ehabkost@redhat.com> >>> Sent: Tuesday, June 2, 2020 6:01 PM >>> To: Moger, Babu <Babu.Moger@amd.com> >>> Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; >>> mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org >>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding >>> functions >>> >>> On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Eduardo Habkost <ehabkost@redhat.com> >>>>> Sent: Tuesday, June 2, 2020 12:19 PM >>>>> To: Moger, Babu <Babu.Moger@amd.com> >>>>> Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; >>>>> mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org >>>>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding >>>>> functions >>>>> >>>>> Hi, >>>>> >>>>> It looks like this series breaks -device and CPU hotplug: >>>>> >>>>> On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote: >>>>>> These functions add support for building EPYC mode topology given the >>> smp >>>>>> details like numa nodes, cores, threads and sockets. >>>>>> >>>>>> The new apic id decoding is mostly similar to current apic id decoding >>>>>> except that it adds a new field node_id when numa configured. Removes >>> all >>>>>> the hardcoded values. Subsequent patches will use these functions to build >>>>>> the topology. >>>>>> >>>>>> Following functions are added. >>>>>> apicid_llc_width_epyc >>>>>> apicid_llc_offset_epyc >>>>>> apicid_pkg_offset_epyc >>>>>> apicid_from_topo_ids_epyc >>>>>> x86_topo_ids_from_idx_epyc >>>>>> x86_topo_ids_from_apicid_epyc >>>>>> x86_apicid_from_cpu_idx_epyc >>>>>> >>>>>> The topology details are available in Processor Programming Reference >>> (PPR) >>>>>> for AMD Family 17h Model 01h, Revision B1 Processors. The revision >>> guides >>>>> are >>>>>> available from the bugzilla Link below. >>>>>> Link: >>>>> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla. >>>>> >>> kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.m >>>>> >>> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488 >>>>> >>> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&sdata=wE0 >>>>> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&reserved=0 >>>>>> >>>>>> Signed-off-by: Babu Moger <babu.moger@amd.com> >>>>>> Acked-by: Igor Mammedov <imammedo@redhat.com> >>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> >>>>>> --- >>>>> [...] >>>>>> typedef struct X86CPUTopoIDs { >>>>>> unsigned pkg_id; >>>>>> + unsigned node_id; >>>>> >>>>> You have added a new field here. >>>>> >>>>>> unsigned die_id; >>>>>> unsigned core_id; >>>>>> unsigned smt_id; >>>>> [...] >>>>>> +static inline apic_id_t >>>>>> +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, >>>>>> + const X86CPUTopoIDs *topo_ids) >>>>>> +{ >>>>>> + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | >>>>>> + (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | >>>>> >>>>> You are using the new field here. >>>>> >>>>>> + (topo_ids->die_id << apicid_die_offset(topo_info)) | >>>>>> + (topo_ids->core_id << apicid_core_offset(topo_info)) | >>>>>> + topo_ids->smt_id; >>>>>> +} >>>>> >>>>> But you are not initializing node_id in one caller of apicid_from_topo_ids(): >>>>> >>>>> static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, >>>>> DeviceState *dev, Error **errp) >>>>> { >>>>> [...] >>>>> X86CPUTopoIDs topo_ids; >>>>> [...] >>>>> if (cpu->apic_id == UNASSIGNED_APIC_ID) { >>>>> [...] >>>>> topo_ids.pkg_id = cpu->socket_id; >>>>> topo_ids.die_id = cpu->die_id; >>>>> topo_ids.core_id = cpu->core_id; >>>>> topo_ids.smt_id = cpu->thread_id; >>>>> cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); >>>>> } >>>>> [...] >>>>> } >>>>> >>>>> Result: -device is broken when using -cpu EPYC: >>>>> >>>>> $ qemu-system-x86_64 -machine q35,accel=kvm -smp >>>>> 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64- >>>>> cpu,core-id=0,socket-id=1,thread-id=0 >>> >>> [1] >>> >>>>> qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket- >>> id=1,thread- >>>>> id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID >>> 21855, >>>>> valid index range 0:1 >>>>> >>>>> This happens because APIC ID is calculated using uninitialized >>>>> memory. >>>> This patch should initialize the node_id. But I am not sure how to >>>> reproduce the bug. Can you please send me the full command line to >>>> reproduce the problem. Also test different options. >>> >>> The full command line is above[1]. >> >> I just picked up the latest tree from >> git clone https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.qemu.org%2Fgit%2Fqemu.git&data=02%7C01%7Cbabu.moger%40amd.com%7C7752c36b7ad24df4039d08d807d334fa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637267951007572953&sdata=69rG4w79FnLs9bhNmroxsRckzmy%2BGuH1dMjyP8PPRUo%3D&reserved=0 qemu >> Not seeing the problem. >> >> ./x86_64-softmmu/qemu-system-x86_64 -machine q35,accel=kvm -smp >> 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device >> EPYC-x86_64-cpu,core-id=0,socket-id=1,thread-id=0 >> VNC server running on ::1:5900 >> >> It appears to run ok. [2] >> >>> >>> >>>> >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >>>> index 2128f3d6fe..047b4b9391 100644 >>>> --- a/hw/i386/pc.c >>>> +++ b/hw/i386/pc.c >>>> @@ -1539,6 +1539,9 @@ static void pc_cpu_pre_plug(HotplugHandler >>> *hotplug_dev, >>>> if (cpu->apic_id == UNASSIGNED_APIC_ID) { >>>> int max_socket = (ms->smp.max_cpus - 1) / >>>> smp_threads / smp_cores / x86ms->smp_dies; >>> >>> So, here's the input you are using to calculate topo_ids.node_id: >>> >>>> + unsigned nr_nodes = MAX(topo_info.nodes_per_pkg, 1); >>> >>> When is topo_info.nodes_per_pkg allowed to be 0? >> >> It will be zero if numa is not configured. Node_id will be zero for all >> the cores. >> >>> >>>> + unsigned cores_per_node = DIV_ROUND_UP((x86ms->smp_dies * >>> smp_cores * >>>> + smp_threads), nr_nodes); >>> >>> x86ms->smp_dies should be available at topo_info.dies_per_pkg, >>> smp_cores should available at topo_info.cores_per_die, >>> smp_threads should be available at topo_info.threads_per_core, >>> nr_nodes should be available at topo_info.nodes_per_pkg. >>> >>>> >>>> /* >>>> * die-id was optional in QEMU 4.0 and older, so keep it optional >>>> @@ -1585,6 +1588,7 @@ 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; >>>> + topo_ids.node_id = (cpu->core_id / cores_per_node) % nr_nodes; >>> >>> apicid_from_topo_ids() have access to topo_info and topo_ids, If >>> all the information you need to calculate node_id is already >>> available inside topo_info + topo_ids, we could be calculating it >>> inside apicid_from_topo_ids(). Why don't we do it? >>> >>> Also, is topo_ids.core_id really allowed to be larger than >>> cores_per_node when calling apicid_from_topo_ids()? >> >> Yes. It is. If we have two numa nodes and 8 cores. Then cores_per_node is >> 4. Nr_nodes =2. Yes. Core_id can be larger than cores_per_node. > > I assumed core_id identified the core inside a die, and the range > would be [0, cores_per_die). This seems to be what > apicid_from_topo_ids_epyc() expects. > > We need clearer documentation on the semantics of each *_id > field, to avoid confusion. Ok. Sure. I will add that. Can you please clarify on how to repro the issue. Marked the question at [2]. > >> >>> >>>> cpu->apic_id = >>>> x86ms->apicid_from_topo_ids(&topo_info, >>>> &topo_ids); } >>>> >>> >>> -- Eduardo >> >
On Wed, Jun 03, 2020 at 10:38:46AM -0500, Babu Moger wrote: > > > On 6/3/20 10:31 AM, Eduardo Habkost wrote: > > On Wed, Jun 03, 2020 at 10:22:10AM -0500, Babu Moger wrote: > >> > >> > >>> -----Original Message----- > >>> From: Eduardo Habkost <ehabkost@redhat.com> > >>> Sent: Tuesday, June 2, 2020 6:01 PM > >>> To: Moger, Babu <Babu.Moger@amd.com> > >>> Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > >>> mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > >>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > >>> functions > >>> > >>> On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote: > >>>> > >>>> > >>>>> -----Original Message----- > >>>>> From: Eduardo Habkost <ehabkost@redhat.com> > >>>>> Sent: Tuesday, June 2, 2020 12:19 PM > >>>>> To: Moger, Babu <Babu.Moger@amd.com> > >>>>> Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > >>>>> mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > >>>>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > >>>>> functions > >>>>> > >>>>> Hi, > >>>>> > >>>>> It looks like this series breaks -device and CPU hotplug: > >>>>> > >>>>> On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote: > >>>>>> These functions add support for building EPYC mode topology given the > >>> smp > >>>>>> details like numa nodes, cores, threads and sockets. > >>>>>> > >>>>>> The new apic id decoding is mostly similar to current apic id decoding > >>>>>> except that it adds a new field node_id when numa configured. Removes > >>> all > >>>>>> the hardcoded values. Subsequent patches will use these functions to build > >>>>>> the topology. > >>>>>> > >>>>>> Following functions are added. > >>>>>> apicid_llc_width_epyc > >>>>>> apicid_llc_offset_epyc > >>>>>> apicid_pkg_offset_epyc > >>>>>> apicid_from_topo_ids_epyc > >>>>>> x86_topo_ids_from_idx_epyc > >>>>>> x86_topo_ids_from_apicid_epyc > >>>>>> x86_apicid_from_cpu_idx_epyc > >>>>>> > >>>>>> The topology details are available in Processor Programming Reference > >>> (PPR) > >>>>>> for AMD Family 17h Model 01h, Revision B1 Processors. The revision > >>> guides > >>>>> are > >>>>>> available from the bugzilla Link below. > >>>>>> Link: > >>>>> > >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla. > >>>>> > >>> kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.m > >>>>> > >>> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488 > >>>>> > >>> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&sdata=wE0 > >>>>> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&reserved=0 > >>>>>> > >>>>>> Signed-off-by: Babu Moger <babu.moger@amd.com> > >>>>>> Acked-by: Igor Mammedov <imammedo@redhat.com> > >>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> > >>>>>> --- > >>>>> [...] > >>>>>> typedef struct X86CPUTopoIDs { > >>>>>> unsigned pkg_id; > >>>>>> + unsigned node_id; > >>>>> > >>>>> You have added a new field here. > >>>>> > >>>>>> unsigned die_id; > >>>>>> unsigned core_id; > >>>>>> unsigned smt_id; > >>>>> [...] > >>>>>> +static inline apic_id_t > >>>>>> +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > >>>>>> + const X86CPUTopoIDs *topo_ids) > >>>>>> +{ > >>>>>> + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > >>>>>> + (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | > >>>>> > >>>>> You are using the new field here. > >>>>> > >>>>>> + (topo_ids->die_id << apicid_die_offset(topo_info)) | > >>>>>> + (topo_ids->core_id << apicid_core_offset(topo_info)) | > >>>>>> + topo_ids->smt_id; > >>>>>> +} > >>>>> > >>>>> But you are not initializing node_id in one caller of apicid_from_topo_ids(): > >>>>> > >>>>> static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > >>>>> DeviceState *dev, Error **errp) > >>>>> { > >>>>> [...] > >>>>> X86CPUTopoIDs topo_ids; > >>>>> [...] > >>>>> if (cpu->apic_id == UNASSIGNED_APIC_ID) { > >>>>> [...] > >>>>> topo_ids.pkg_id = cpu->socket_id; > >>>>> topo_ids.die_id = cpu->die_id; > >>>>> topo_ids.core_id = cpu->core_id; > >>>>> topo_ids.smt_id = cpu->thread_id; > >>>>> cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); > >>>>> } > >>>>> [...] > >>>>> } > >>>>> > >>>>> Result: -device is broken when using -cpu EPYC: > >>>>> > >>>>> $ qemu-system-x86_64 -machine q35,accel=kvm -smp > >>>>> 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64- > >>>>> cpu,core-id=0,socket-id=1,thread-id=0 > >>> > >>> [1] > >>> > >>>>> qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket- > >>> id=1,thread- > >>>>> id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID > >>> 21855, > >>>>> valid index range 0:1 > >>>>> > >>>>> This happens because APIC ID is calculated using uninitialized > >>>>> memory. > >>>> This patch should initialize the node_id. But I am not sure how to > >>>> reproduce the bug. Can you please send me the full command line to > >>>> reproduce the problem. Also test different options. > >>> > >>> The full command line is above[1]. > >> > >> I just picked up the latest tree from > >> git clone https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.qemu.org%2Fgit%2Fqemu.git&data=02%7C01%7Cbabu.moger%40amd.com%7C7752c36b7ad24df4039d08d807d334fa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637267951007572953&sdata=69rG4w79FnLs9bhNmroxsRckzmy%2BGuH1dMjyP8PPRUo%3D&reserved=0 qemu > >> Not seeing the problem. > >> > >> ./x86_64-softmmu/qemu-system-x86_64 -machine q35,accel=kvm -smp > >> 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device > >> EPYC-x86_64-cpu,core-id=0,socket-id=1,thread-id=0 > >> VNC server running on ::1:5900 > >> > >> It appears to run ok. > > [2] > > >> > >>> > >>> > >>>> > >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c > >>>> index 2128f3d6fe..047b4b9391 100644 > >>>> --- a/hw/i386/pc.c > >>>> +++ b/hw/i386/pc.c > >>>> @@ -1539,6 +1539,9 @@ static void pc_cpu_pre_plug(HotplugHandler > >>> *hotplug_dev, > >>>> if (cpu->apic_id == UNASSIGNED_APIC_ID) { > >>>> int max_socket = (ms->smp.max_cpus - 1) / > >>>> smp_threads / smp_cores / x86ms->smp_dies; > >>> > >>> So, here's the input you are using to calculate topo_ids.node_id: > >>> > >>>> + unsigned nr_nodes = MAX(topo_info.nodes_per_pkg, 1); > >>> > >>> When is topo_info.nodes_per_pkg allowed to be 0? > >> > >> It will be zero if numa is not configured. Node_id will be zero for all > >> the cores. > >> > >>> > >>>> + unsigned cores_per_node = DIV_ROUND_UP((x86ms->smp_dies * > >>> smp_cores * > >>>> + smp_threads), nr_nodes); > >>> > >>> x86ms->smp_dies should be available at topo_info.dies_per_pkg, > >>> smp_cores should available at topo_info.cores_per_die, > >>> smp_threads should be available at topo_info.threads_per_core, > >>> nr_nodes should be available at topo_info.nodes_per_pkg. > >>> > >>>> > >>>> /* > >>>> * die-id was optional in QEMU 4.0 and older, so keep it optional > >>>> @@ -1585,6 +1588,7 @@ 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; > >>>> + topo_ids.node_id = (cpu->core_id / cores_per_node) % nr_nodes; > >>> > >>> apicid_from_topo_ids() have access to topo_info and topo_ids, If > >>> all the information you need to calculate node_id is already > >>> available inside topo_info + topo_ids, we could be calculating it > >>> inside apicid_from_topo_ids(). Why don't we do it? > >>> > >>> Also, is topo_ids.core_id really allowed to be larger than > >>> cores_per_node when calling apicid_from_topo_ids()? > >> > >> Yes. It is. If we have two numa nodes and 8 cores. Then cores_per_node is > >> 4. Nr_nodes =2. Yes. Core_id can be larger than cores_per_node. > > > > I assumed core_id identified the core inside a die, and the range > > would be [0, cores_per_die). This seems to be what > > apicid_from_topo_ids_epyc() expects. > > > > We need clearer documentation on the semantics of each *_id > > field, to avoid confusion. > > Ok. Sure. I will add that. Can you please clarify on how to repro the > issue. Marked the question at [2]. As it is usage of uninitialized memory, behavior is undefined and not guaranteed to crash. My QEMU binary was built with --enable-debug, maybe it makes it easier to reproduce.
On 6/3/20 10:45 AM, Eduardo Habkost wrote: > On Wed, Jun 03, 2020 at 10:38:46AM -0500, Babu Moger wrote: >> >> >> On 6/3/20 10:31 AM, Eduardo Habkost wrote: >>> On Wed, Jun 03, 2020 at 10:22:10AM -0500, Babu Moger wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Eduardo Habkost <ehabkost@redhat.com> >>>>> Sent: Tuesday, June 2, 2020 6:01 PM >>>>> To: Moger, Babu <Babu.Moger@amd.com> >>>>> Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; >>>>> mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org >>>>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding >>>>> functions >>>>> >>>>> On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote: >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Eduardo Habkost <ehabkost@redhat.com> >>>>>>> Sent: Tuesday, June 2, 2020 12:19 PM >>>>>>> To: Moger, Babu <Babu.Moger@amd.com> >>>>>>> Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; >>>>>>> mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org >>>>>>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding >>>>>>> functions >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> It looks like this series breaks -device and CPU hotplug: >>>>>>> >>>>>>> On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote: >>>>>>>> These functions add support for building EPYC mode topology given the >>>>> smp >>>>>>>> details like numa nodes, cores, threads and sockets. >>>>>>>> >>>>>>>> The new apic id decoding is mostly similar to current apic id decoding >>>>>>>> except that it adds a new field node_id when numa configured. Removes >>>>> all >>>>>>>> the hardcoded values. Subsequent patches will use these functions to build >>>>>>>> the topology. >>>>>>>> >>>>>>>> Following functions are added. >>>>>>>> apicid_llc_width_epyc >>>>>>>> apicid_llc_offset_epyc >>>>>>>> apicid_pkg_offset_epyc >>>>>>>> apicid_from_topo_ids_epyc >>>>>>>> x86_topo_ids_from_idx_epyc >>>>>>>> x86_topo_ids_from_apicid_epyc >>>>>>>> x86_apicid_from_cpu_idx_epyc >>>>>>>> >>>>>>>> The topology details are available in Processor Programming Reference >>>>> (PPR) >>>>>>>> for AMD Family 17h Model 01h, Revision B1 Processors. The revision >>>>> guides >>>>>>> are >>>>>>>> available from the bugzilla Link below. >>>>>>>> Link: >>>>>>> >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla. >>>>>>> >>>>> kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.m >>>>>>> >>>>> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488 >>>>>>> >>>>> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&sdata=wE0 >>>>>>> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&reserved=0 >>>>>>>> >>>>>>>> Signed-off-by: Babu Moger <babu.moger@amd.com> >>>>>>>> Acked-by: Igor Mammedov <imammedo@redhat.com> >>>>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> >>>>>>>> --- >>>>>>> [...] >>>>>>>> typedef struct X86CPUTopoIDs { >>>>>>>> unsigned pkg_id; >>>>>>>> + unsigned node_id; >>>>>>> >>>>>>> You have added a new field here. >>>>>>> >>>>>>>> unsigned die_id; >>>>>>>> unsigned core_id; >>>>>>>> unsigned smt_id; >>>>>>> [...] >>>>>>>> +static inline apic_id_t >>>>>>>> +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, >>>>>>>> + const X86CPUTopoIDs *topo_ids) >>>>>>>> +{ >>>>>>>> + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | >>>>>>>> + (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | >>>>>>> >>>>>>> You are using the new field here. >>>>>>> >>>>>>>> + (topo_ids->die_id << apicid_die_offset(topo_info)) | >>>>>>>> + (topo_ids->core_id << apicid_core_offset(topo_info)) | >>>>>>>> + topo_ids->smt_id; >>>>>>>> +} >>>>>>> >>>>>>> But you are not initializing node_id in one caller of apicid_from_topo_ids(): >>>>>>> >>>>>>> static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, >>>>>>> DeviceState *dev, Error **errp) >>>>>>> { >>>>>>> [...] >>>>>>> X86CPUTopoIDs topo_ids; >>>>>>> [...] >>>>>>> if (cpu->apic_id == UNASSIGNED_APIC_ID) { >>>>>>> [...] >>>>>>> topo_ids.pkg_id = cpu->socket_id; >>>>>>> topo_ids.die_id = cpu->die_id; >>>>>>> topo_ids.core_id = cpu->core_id; >>>>>>> topo_ids.smt_id = cpu->thread_id; >>>>>>> cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids); >>>>>>> } >>>>>>> [...] >>>>>>> } >>>>>>> >>>>>>> Result: -device is broken when using -cpu EPYC: >>>>>>> >>>>>>> $ qemu-system-x86_64 -machine q35,accel=kvm -smp >>>>>>> 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64- >>>>>>> cpu,core-id=0,socket-id=1,thread-id=0 >>>>> >>>>> [1] >>>>> >>>>>>> qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket- >>>>> id=1,thread- >>>>>>> id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID >>>>> 21855, >>>>>>> valid index range 0:1 >>>>>>> >>>>>>> This happens because APIC ID is calculated using uninitialized >>>>>>> memory. >>>>>> This patch should initialize the node_id. But I am not sure how to >>>>>> reproduce the bug. Can you please send me the full command line to >>>>>> reproduce the problem. Also test different options. >>>>> >>>>> The full command line is above[1]. >>>> >>>> I just picked up the latest tree from >>>> git clone https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.qemu.org%2Fgit%2Fqemu.git&data=02%7C01%7Cbabu.moger%40amd.com%7C4b409b77ff2c4c4ed90608d807d53570%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637267959601716206&sdata=ih6bo9Wp0RbJgRpryzSa2D0v6kr3Zfww%2B1uB%2FNyngk8%3D&reserved=0 qemu >>>> Not seeing the problem. >>>> >>>> ./x86_64-softmmu/qemu-system-x86_64 -machine q35,accel=kvm -smp >>>> 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device >>>> EPYC-x86_64-cpu,core-id=0,socket-id=1,thread-id=0 >>>> VNC server running on ::1:5900 >>>> >>>> It appears to run ok. >> >> [2] >> >>>> >>>>> >>>>> >>>>>> >>>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >>>>>> index 2128f3d6fe..047b4b9391 100644 >>>>>> --- a/hw/i386/pc.c >>>>>> +++ b/hw/i386/pc.c >>>>>> @@ -1539,6 +1539,9 @@ static void pc_cpu_pre_plug(HotplugHandler >>>>> *hotplug_dev, >>>>>> if (cpu->apic_id == UNASSIGNED_APIC_ID) { >>>>>> int max_socket = (ms->smp.max_cpus - 1) / >>>>>> smp_threads / smp_cores / x86ms->smp_dies; >>>>> >>>>> So, here's the input you are using to calculate topo_ids.node_id: >>>>> >>>>>> + unsigned nr_nodes = MAX(topo_info.nodes_per_pkg, 1); >>>>> >>>>> When is topo_info.nodes_per_pkg allowed to be 0? >>>> >>>> It will be zero if numa is not configured. Node_id will be zero for all >>>> the cores. >>>> >>>>> >>>>>> + unsigned cores_per_node = DIV_ROUND_UP((x86ms->smp_dies * >>>>> smp_cores * >>>>>> + smp_threads), nr_nodes); >>>>> >>>>> x86ms->smp_dies should be available at topo_info.dies_per_pkg, >>>>> smp_cores should available at topo_info.cores_per_die, >>>>> smp_threads should be available at topo_info.threads_per_core, >>>>> nr_nodes should be available at topo_info.nodes_per_pkg. >>>>> >>>>>> >>>>>> /* >>>>>> * die-id was optional in QEMU 4.0 and older, so keep it optional >>>>>> @@ -1585,6 +1588,7 @@ 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; >>>>>> + topo_ids.node_id = (cpu->core_id / cores_per_node) % nr_nodes; >>>>> >>>>> apicid_from_topo_ids() have access to topo_info and topo_ids, If >>>>> all the information you need to calculate node_id is already >>>>> available inside topo_info + topo_ids, we could be calculating it >>>>> inside apicid_from_topo_ids(). Why don't we do it? >>>>> >>>>> Also, is topo_ids.core_id really allowed to be larger than >>>>> cores_per_node when calling apicid_from_topo_ids()? >>>> >>>> Yes. It is. If we have two numa nodes and 8 cores. Then cores_per_node is >>>> 4. Nr_nodes =2. Yes. Core_id can be larger than cores_per_node. >>> >>> I assumed core_id identified the core inside a die, and the range >>> would be [0, cores_per_die). This seems to be what >>> apicid_from_topo_ids_epyc() expects. >>> >>> We need clearer documentation on the semantics of each *_id >>> field, to avoid confusion. >> >> Ok. Sure. I will add that. Can you please clarify on how to repro the >> issue. Marked the question at [2]. > > As it is usage of uninitialized memory, behavior is undefined and > not guaranteed to crash. My QEMU binary was built with > --enable-debug, maybe it makes it easier to reproduce. Ok. That is fine. Will try..Thanks
> -----Original Message----- > From: Eduardo Habkost <ehabkost@redhat.com> > Sent: Wednesday, June 3, 2020 10:46 AM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > functions > > On Wed, Jun 03, 2020 at 10:38:46AM -0500, Babu Moger wrote: > > > > > > On 6/3/20 10:31 AM, Eduardo Habkost wrote: > > > On Wed, Jun 03, 2020 at 10:22:10AM -0500, Babu Moger wrote: > > >> > > >> > > >>> -----Original Message----- > > >>> From: Eduardo Habkost <ehabkost@redhat.com> > > >>> Sent: Tuesday, June 2, 2020 6:01 PM > > >>> To: Moger, Babu <Babu.Moger@amd.com> > > >>> Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > rth@twiddle.net; > > >>> mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > > >>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > > >>> functions > > >>> > > >>> On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote: > > >>>> > > >>>> > > >>>>> -----Original Message----- > > >>>>> From: Eduardo Habkost <ehabkost@redhat.com> > > >>>>> Sent: Tuesday, June 2, 2020 12:19 PM > > >>>>> To: Moger, Babu <Babu.Moger@amd.com> > > >>>>> Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > rth@twiddle.net; > > >>>>> mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > > >>>>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology > decoding > > >>>>> functions > > >>>>> > > >>>>> Hi, > > >>>>> > > >>>>> It looks like this series breaks -device and CPU hotplug: > > >>>>> > > >>>>> On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote: > > >>>>>> These functions add support for building EPYC mode topology given > the > > >>> smp > > >>>>>> details like numa nodes, cores, threads and sockets. > > >>>>>> > > >>>>>> The new apic id decoding is mostly similar to current apic id decoding > > >>>>>> except that it adds a new field node_id when numa configured. > Removes > > >>> all > > >>>>>> the hardcoded values. Subsequent patches will use these functions to > build > > >>>>>> the topology. > > >>>>>> > > >>>>>> Following functions are added. > > >>>>>> apicid_llc_width_epyc > > >>>>>> apicid_llc_offset_epyc > > >>>>>> apicid_pkg_offset_epyc > > >>>>>> apicid_from_topo_ids_epyc > > >>>>>> x86_topo_ids_from_idx_epyc > > >>>>>> x86_topo_ids_from_apicid_epyc > > >>>>>> x86_apicid_from_cpu_idx_epyc > > >>>>>> > > >>>>>> The topology details are available in Processor Programming > Reference > > >>> (PPR) > > >>>>>> for AMD Family 17h Model 01h, Revision B1 Processors. The revision > > >>> guides > > >>>>> are > > >>>>>> available from the bugzilla Link below. > > >>>>>> Link: > > >>>>> > > >>> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla. > > >>>>> > > >>> > kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.m > > >>>>> > > >>> > oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488 > > >>>>> > > >>> > 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&sdata=wE0 > > >>>>> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&reserved=0 > > >>>>>> > > >>>>>> Signed-off-by: Babu Moger <babu.moger@amd.com> > > >>>>>> Acked-by: Igor Mammedov <imammedo@redhat.com> > > >>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> > > >>>>>> --- > > >>>>> [...] > > >>>>>> typedef struct X86CPUTopoIDs { > > >>>>>> unsigned pkg_id; > > >>>>>> + unsigned node_id; > > >>>>> > > >>>>> You have added a new field here. > > >>>>> > > >>>>>> unsigned die_id; > > >>>>>> unsigned core_id; > > >>>>>> unsigned smt_id; > > >>>>> [...] > > >>>>>> +static inline apic_id_t > > >>>>>> +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > > >>>>>> + const X86CPUTopoIDs *topo_ids) > > >>>>>> +{ > > >>>>>> + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > > >>>>>> + (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | > > >>>>> > > >>>>> You are using the new field here. > > >>>>> > > >>>>>> + (topo_ids->die_id << apicid_die_offset(topo_info)) | > > >>>>>> + (topo_ids->core_id << apicid_core_offset(topo_info)) | > > >>>>>> + topo_ids->smt_id; > > >>>>>> +} > > >>>>> > > >>>>> But you are not initializing node_id in one caller of > apicid_from_topo_ids(): > > >>>>> > > >>>>> static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > > >>>>> DeviceState *dev, Error **errp) > > >>>>> { > > >>>>> [...] > > >>>>> X86CPUTopoIDs topo_ids; > > >>>>> [...] > > >>>>> if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > >>>>> [...] > > >>>>> topo_ids.pkg_id = cpu->socket_id; > > >>>>> topo_ids.die_id = cpu->die_id; > > >>>>> topo_ids.core_id = cpu->core_id; > > >>>>> topo_ids.smt_id = cpu->thread_id; > > >>>>> cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, > &topo_ids); > > >>>>> } > > >>>>> [...] > > >>>>> } > > >>>>> > > >>>>> Result: -device is broken when using -cpu EPYC: > > >>>>> > > >>>>> $ qemu-system-x86_64 -machine q35,accel=kvm -smp > > >>>>> 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC- > x86_64- > > >>>>> cpu,core-id=0,socket-id=1,thread-id=0 > > >>> > > >>> [1] > > >>> > > >>>>> qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket- > > >>> id=1,thread- > > >>>>> id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID > > >>> 21855, > > >>>>> valid index range 0:1 > > >>>>> > > >>>>> This happens because APIC ID is calculated using uninitialized > > >>>>> memory. > > >>>> This patch should initialize the node_id. But I am not sure how to > > >>>> reproduce the bug. Can you please send me the full command line to > > >>>> reproduce the problem. Also test different options. > > >>> > > >>> The full command line is above[1]. > > >> > > >> I just picked up the latest tree from > > >> git clone > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.qem > u.org%2Fgit%2Fqemu.git&data=02%7C01%7Cbabu.moger%40amd.com%7 > C4b409b77ff2c4c4ed90608d807d53570%7C3dd8961fe4884e608e11a82d994e1 > 83d%7C0%7C0%7C637267959601716206&sdata=ih6bo9Wp0RbJgRpryzSa2 > D0v6kr3Zfww%2B1uB%2FNyngk8%3D&reserved=0 qemu > > >> Not seeing the problem. > > >> > > >> ./x86_64-softmmu/qemu-system-x86_64 -machine q35,accel=kvm -smp > > >> 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device > > >> EPYC-x86_64-cpu,core-id=0,socket-id=1,thread-id=0 > > >> VNC server running on ::1:5900 > > >> > > >> It appears to run ok. > > > > [2] > > > > >> > > >>> > > >>> > > >>>> > > >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > >>>> index 2128f3d6fe..047b4b9391 100644 > > >>>> --- a/hw/i386/pc.c > > >>>> +++ b/hw/i386/pc.c > > >>>> @@ -1539,6 +1539,9 @@ static void pc_cpu_pre_plug(HotplugHandler > > >>> *hotplug_dev, > > >>>> if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > >>>> int max_socket = (ms->smp.max_cpus - 1) / > > >>>> smp_threads / smp_cores / x86ms->smp_dies; > > >>> > > >>> So, here's the input you are using to calculate topo_ids.node_id: > > >>> > > >>>> + unsigned nr_nodes = MAX(topo_info.nodes_per_pkg, 1); > > >>> > > >>> When is topo_info.nodes_per_pkg allowed to be 0? > > >> > > >> It will be zero if numa is not configured. Node_id will be zero for all > > >> the cores. > > >> > > >>> > > >>>> + unsigned cores_per_node = DIV_ROUND_UP((x86ms->smp_dies * > > >>> smp_cores * > > >>>> + smp_threads), nr_nodes); > > >>> > > >>> x86ms->smp_dies should be available at topo_info.dies_per_pkg, > > >>> smp_cores should available at topo_info.cores_per_die, > > >>> smp_threads should be available at topo_info.threads_per_core, > > >>> nr_nodes should be available at topo_info.nodes_per_pkg. > > >>> > > >>>> > > >>>> /* > > >>>> * die-id was optional in QEMU 4.0 and older, so keep it optional > > >>>> @@ -1585,6 +1588,7 @@ 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; > > >>>> + topo_ids.node_id = (cpu->core_id / cores_per_node) % nr_nodes; > > >>> > > >>> apicid_from_topo_ids() have access to topo_info and topo_ids, If > > >>> all the information you need to calculate node_id is already > > >>> available inside topo_info + topo_ids, we could be calculating it > > >>> inside apicid_from_topo_ids(). Why don't we do it? > > >>> > > >>> Also, is topo_ids.core_id really allowed to be larger than > > >>> cores_per_node when calling apicid_from_topo_ids()? > > >> > > >> Yes. It is. If we have two numa nodes and 8 cores. Then cores_per_node is > > >> 4. Nr_nodes =2. Yes. Core_id can be larger than cores_per_node. > > > > > > I assumed core_id identified the core inside a die, and the range > > > would be [0, cores_per_die). This seems to be what > > > apicid_from_topo_ids_epyc() expects. > > > > > > We need clearer documentation on the semantics of each *_id > > > field, to avoid confusion. > > > > Ok. Sure. I will add that. Can you please clarify on how to repro the > > issue. Marked the question at [2]. > > As it is usage of uninitialized memory, behavior is undefined and > not guaranteed to crash. My QEMU binary was built with > --enable-debug, maybe it makes it easier to reproduce. This patch works fine. Tested few combination. Will send the formal version if patch looks good. diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h index 07239f95f4..cb28f0a57c 100644 --- a/include/hw/i386/topology.h +++ b/include/hw/i386/topology.h @@ -140,6 +140,17 @@ static inline unsigned apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info) apicid_node_width_epyc(topo_info); } +static inline unsigned x86_node_id_for_epyc(X86CPUTopoInfo *topo_info, + const X86CPUTopoIDs *topo_ids) +{ + unsigned nr_nodes = MAX(topo_info->nodes_per_pkg, 1); + unsigned cores_per_node = DIV_ROUND_UP((topo_info->dies_per_pkg * + topo_info->cores_per_die * + topo_info->threads_per_core), + nr_nodes); + + return ((topo_ids->core_id / cores_per_node) % nr_nodes); +} /* * Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID * @@ -149,8 +160,11 @@ static inline apic_id_t x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, const X86CPUTopoIDs *topo_ids) { + /* node_id can be uninitialized. Initialize here */ + unsigned node_id = x86_node_id_for_epyc(topo_info, topo_ids); + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | - (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | + (node_id << apicid_node_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;
* Babu Moger (babu.moger@amd.com) wrote: > > > > -----Original Message----- > > From: Eduardo Habkost <ehabkost@redhat.com> > > Sent: Wednesday, June 3, 2020 10:46 AM > > To: Moger, Babu <Babu.Moger@amd.com> > > Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net; > > mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > > functions > > > > On Wed, Jun 03, 2020 at 10:38:46AM -0500, Babu Moger wrote: > > > > > > > > > On 6/3/20 10:31 AM, Eduardo Habkost wrote: > > > > On Wed, Jun 03, 2020 at 10:22:10AM -0500, Babu Moger wrote: > > > >> > > > >> > > > >>> -----Original Message----- > > > >>> From: Eduardo Habkost <ehabkost@redhat.com> > > > >>> Sent: Tuesday, June 2, 2020 6:01 PM > > > >>> To: Moger, Babu <Babu.Moger@amd.com> > > > >>> Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > > rth@twiddle.net; > > > >>> mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > > > >>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding > > > >>> functions > > > >>> > > > >>> On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote: > > > >>>> > > > >>>> > > > >>>>> -----Original Message----- > > > >>>>> From: Eduardo Habkost <ehabkost@redhat.com> > > > >>>>> Sent: Tuesday, June 2, 2020 12:19 PM > > > >>>>> To: Moger, Babu <Babu.Moger@amd.com> > > > >>>>> Cc: marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > > rth@twiddle.net; > > > >>>>> mst@redhat.com; imammedo@redhat.com; qemu-devel@nongnu.org > > > >>>>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology > > decoding > > > >>>>> functions > > > >>>>> > > > >>>>> Hi, > > > >>>>> > > > >>>>> It looks like this series breaks -device and CPU hotplug: > > > >>>>> > > > >>>>> On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote: > > > >>>>>> These functions add support for building EPYC mode topology given > > the > > > >>> smp > > > >>>>>> details like numa nodes, cores, threads and sockets. > > > >>>>>> > > > >>>>>> The new apic id decoding is mostly similar to current apic id decoding > > > >>>>>> except that it adds a new field node_id when numa configured. > > Removes > > > >>> all > > > >>>>>> the hardcoded values. Subsequent patches will use these functions to > > build > > > >>>>>> the topology. > > > >>>>>> > > > >>>>>> Following functions are added. > > > >>>>>> apicid_llc_width_epyc > > > >>>>>> apicid_llc_offset_epyc > > > >>>>>> apicid_pkg_offset_epyc > > > >>>>>> apicid_from_topo_ids_epyc > > > >>>>>> x86_topo_ids_from_idx_epyc > > > >>>>>> x86_topo_ids_from_apicid_epyc > > > >>>>>> x86_apicid_from_cpu_idx_epyc > > > >>>>>> > > > >>>>>> The topology details are available in Processor Programming > > Reference > > > >>> (PPR) > > > >>>>>> for AMD Family 17h Model 01h, Revision B1 Processors. The revision > > > >>> guides > > > >>>>> are > > > >>>>>> available from the bugzilla Link below. > > > >>>>>> Link: > > > >>>>> > > > >>> > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla. > > > >>>>> > > > >>> > > kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.m > > > >>>>> > > > >>> > > oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488 > > > >>>>> > > > >>> > > 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&sdata=wE0 > > > >>>>> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&reserved=0 > > > >>>>>> > > > >>>>>> Signed-off-by: Babu Moger <babu.moger@amd.com> > > > >>>>>> Acked-by: Igor Mammedov <imammedo@redhat.com> > > > >>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > >>>>>> --- > > > >>>>> [...] > > > >>>>>> typedef struct X86CPUTopoIDs { > > > >>>>>> unsigned pkg_id; > > > >>>>>> + unsigned node_id; > > > >>>>> > > > >>>>> You have added a new field here. > > > >>>>> > > > >>>>>> unsigned die_id; > > > >>>>>> unsigned core_id; > > > >>>>>> unsigned smt_id; > > > >>>>> [...] > > > >>>>>> +static inline apic_id_t > > > >>>>>> +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > > > >>>>>> + const X86CPUTopoIDs *topo_ids) > > > >>>>>> +{ > > > >>>>>> + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > > > >>>>>> + (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | > > > >>>>> > > > >>>>> You are using the new field here. > > > >>>>> > > > >>>>>> + (topo_ids->die_id << apicid_die_offset(topo_info)) | > > > >>>>>> + (topo_ids->core_id << apicid_core_offset(topo_info)) | > > > >>>>>> + topo_ids->smt_id; > > > >>>>>> +} > > > >>>>> > > > >>>>> But you are not initializing node_id in one caller of > > apicid_from_topo_ids(): > > > >>>>> > > > >>>>> static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > > > >>>>> DeviceState *dev, Error **errp) > > > >>>>> { > > > >>>>> [...] > > > >>>>> X86CPUTopoIDs topo_ids; > > > >>>>> [...] > > > >>>>> if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > > >>>>> [...] > > > >>>>> topo_ids.pkg_id = cpu->socket_id; > > > >>>>> topo_ids.die_id = cpu->die_id; > > > >>>>> topo_ids.core_id = cpu->core_id; > > > >>>>> topo_ids.smt_id = cpu->thread_id; > > > >>>>> cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, > > &topo_ids); > > > >>>>> } > > > >>>>> [...] > > > >>>>> } > > > >>>>> > > > >>>>> Result: -device is broken when using -cpu EPYC: > > > >>>>> > > > >>>>> $ qemu-system-x86_64 -machine q35,accel=kvm -smp > > > >>>>> 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC- > > x86_64- > > > >>>>> cpu,core-id=0,socket-id=1,thread-id=0 > > > >>> > > > >>> [1] > > > >>> > > > >>>>> qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket- > > > >>> id=1,thread- > > > >>>>> id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID > > > >>> 21855, > > > >>>>> valid index range 0:1 > > > >>>>> > > > >>>>> This happens because APIC ID is calculated using uninitialized > > > >>>>> memory. > > > >>>> This patch should initialize the node_id. But I am not sure how to > > > >>>> reproduce the bug. Can you please send me the full command line to > > > >>>> reproduce the problem. Also test different options. > > > >>> > > > >>> The full command line is above[1]. > > > >> > > > >> I just picked up the latest tree from > > > >> git clone > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.qem > > u.org%2Fgit%2Fqemu.git&data=02%7C01%7Cbabu.moger%40amd.com%7 > > C4b409b77ff2c4c4ed90608d807d53570%7C3dd8961fe4884e608e11a82d994e1 > > 83d%7C0%7C0%7C637267959601716206&sdata=ih6bo9Wp0RbJgRpryzSa2 > > D0v6kr3Zfww%2B1uB%2FNyngk8%3D&reserved=0 qemu > > > >> Not seeing the problem. > > > >> > > > >> ./x86_64-softmmu/qemu-system-x86_64 -machine q35,accel=kvm -smp > > > >> 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device > > > >> EPYC-x86_64-cpu,core-id=0,socket-id=1,thread-id=0 > > > >> VNC server running on ::1:5900 > > > >> > > > >> It appears to run ok. > > > > > > [2] > > > > > > >> > > > >>> > > > >>> > > > >>>> > > > >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > >>>> index 2128f3d6fe..047b4b9391 100644 > > > >>>> --- a/hw/i386/pc.c > > > >>>> +++ b/hw/i386/pc.c > > > >>>> @@ -1539,6 +1539,9 @@ static void pc_cpu_pre_plug(HotplugHandler > > > >>> *hotplug_dev, > > > >>>> if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > > >>>> int max_socket = (ms->smp.max_cpus - 1) / > > > >>>> smp_threads / smp_cores / x86ms->smp_dies; > > > >>> > > > >>> So, here's the input you are using to calculate topo_ids.node_id: > > > >>> > > > >>>> + unsigned nr_nodes = MAX(topo_info.nodes_per_pkg, 1); > > > >>> > > > >>> When is topo_info.nodes_per_pkg allowed to be 0? > > > >> > > > >> It will be zero if numa is not configured. Node_id will be zero for all > > > >> the cores. > > > >> > > > >>> > > > >>>> + unsigned cores_per_node = DIV_ROUND_UP((x86ms->smp_dies * > > > >>> smp_cores * > > > >>>> + smp_threads), nr_nodes); > > > >>> > > > >>> x86ms->smp_dies should be available at topo_info.dies_per_pkg, > > > >>> smp_cores should available at topo_info.cores_per_die, > > > >>> smp_threads should be available at topo_info.threads_per_core, > > > >>> nr_nodes should be available at topo_info.nodes_per_pkg. > > > >>> > > > >>>> > > > >>>> /* > > > >>>> * die-id was optional in QEMU 4.0 and older, so keep it optional > > > >>>> @@ -1585,6 +1588,7 @@ 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; > > > >>>> + topo_ids.node_id = (cpu->core_id / cores_per_node) % nr_nodes; > > > >>> > > > >>> apicid_from_topo_ids() have access to topo_info and topo_ids, If > > > >>> all the information you need to calculate node_id is already > > > >>> available inside topo_info + topo_ids, we could be calculating it > > > >>> inside apicid_from_topo_ids(). Why don't we do it? > > > >>> > > > >>> Also, is topo_ids.core_id really allowed to be larger than > > > >>> cores_per_node when calling apicid_from_topo_ids()? > > > >> > > > >> Yes. It is. If we have two numa nodes and 8 cores. Then cores_per_node is > > > >> 4. Nr_nodes =2. Yes. Core_id can be larger than cores_per_node. > > > > > > > > I assumed core_id identified the core inside a die, and the range > > > > would be [0, cores_per_die). This seems to be what > > > > apicid_from_topo_ids_epyc() expects. > > > > > > > > We need clearer documentation on the semantics of each *_id > > > > field, to avoid confusion. > > > > > > Ok. Sure. I will add that. Can you please clarify on how to repro the > > > issue. Marked the question at [2]. > > > > As it is usage of uninitialized memory, behavior is undefined and > > not guaranteed to crash. My QEMU binary was built with > > --enable-debug, maybe it makes it easier to reproduce. > > This patch works fine. Tested few combination. Will send the formal > version if patch looks good. > > diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h > index 07239f95f4..cb28f0a57c 100644 > --- a/include/hw/i386/topology.h > +++ b/include/hw/i386/topology.h > @@ -140,6 +140,17 @@ static inline unsigned > apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info) > apicid_node_width_epyc(topo_info); > } > > +static inline unsigned x86_node_id_for_epyc(X86CPUTopoInfo *topo_info, > + const X86CPUTopoIDs *topo_ids) > +{ > + unsigned nr_nodes = MAX(topo_info->nodes_per_pkg, 1); > + unsigned cores_per_node = DIV_ROUND_UP((topo_info->dies_per_pkg * > + topo_info->cores_per_die * > + topo_info->threads_per_core), > + nr_nodes); > + > + return ((topo_ids->core_id / cores_per_node) % nr_nodes); > +} > /* > * Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID > * > @@ -149,8 +160,11 @@ static inline apic_id_t > x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, > const X86CPUTopoIDs *topo_ids) > { > + /* node_id can be uninitialized. Initialize here */ > + unsigned node_id = x86_node_id_for_epyc(topo_info, topo_ids); > + > return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | > - (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) | > + (node_id << apicid_node_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; > What's the status of this? It seems to fix a hotplug case our QE hit (RH bz 1828750) Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h index b9593b9905..07239f95f4 100644 --- a/include/hw/i386/topology.h +++ b/include/hw/i386/topology.h @@ -47,6 +47,7 @@ typedef uint32_t apic_id_t; typedef struct X86CPUTopoIDs { unsigned pkg_id; + unsigned node_id; unsigned die_id; unsigned core_id; unsigned smt_id; @@ -88,6 +89,11 @@ static inline unsigned apicid_die_width(X86CPUTopoInfo *topo_info) return apicid_bitwidth_for_count(topo_info->dies_per_pkg); } +/* Bit width of the node_id field per socket */ +static inline unsigned apicid_node_width_epyc(X86CPUTopoInfo *topo_info) +{ + return apicid_bitwidth_for_count(MAX(topo_info->nodes_per_pkg, 1)); +} /* Bit offset of the Core_ID field */ static inline unsigned apicid_core_offset(X86CPUTopoInfo *topo_info) @@ -108,6 +114,100 @@ static inline unsigned apicid_pkg_offset(X86CPUTopoInfo *topo_info) return apicid_die_offset(topo_info) + apicid_die_width(topo_info); } +#define NODE_ID_OFFSET 3 /* Minimum node_id offset if numa configured */ + +/* + * Bit offset of the node_id field + * + * Make sure nodes_per_pkg > 0 if numa configured else zero. + */ +static inline unsigned apicid_node_offset_epyc(X86CPUTopoInfo *topo_info) +{ + unsigned offset = apicid_die_offset(topo_info) + + apicid_die_width(topo_info); + + if (topo_info->nodes_per_pkg) { + return MAX(NODE_ID_OFFSET, offset); + } else { + return offset; + } +} + +/* Bit offset of the Pkg_ID (socket ID) field */ +static inline unsigned apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info) +{ + return apicid_node_offset_epyc(topo_info) + + apicid_node_width_epyc(topo_info); +} + +/* + * Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID + * + * The caller must make sure core_id < nr_cores and smt_id < nr_threads. + */ +static inline apic_id_t +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info, + const X86CPUTopoIDs *topo_ids) +{ + return (topo_ids->pkg_id << apicid_pkg_offset_epyc(topo_info)) | + (topo_ids->node_id << apicid_node_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; +} + +static inline void x86_topo_ids_from_idx_epyc(X86CPUTopoInfo *topo_info, + unsigned cpu_index, + X86CPUTopoIDs *topo_ids) +{ + unsigned nr_nodes = MAX(topo_info->nodes_per_pkg, 1); + unsigned nr_dies = topo_info->dies_per_pkg; + unsigned nr_cores = topo_info->cores_per_die; + unsigned nr_threads = topo_info->threads_per_core; + unsigned cores_per_node = DIV_ROUND_UP((nr_dies * nr_cores * nr_threads), + nr_nodes); + + topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads); + topo_ids->node_id = (cpu_index / cores_per_node) % nr_nodes; + topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies; + topo_ids->core_id = cpu_index / nr_threads % nr_cores; + topo_ids->smt_id = cpu_index % nr_threads; +} + +/* + * Calculate thread/core/package IDs for a specific topology, + * based on APIC ID + */ +static inline void x86_topo_ids_from_apicid_epyc(apic_id_t apicid, + X86CPUTopoInfo *topo_info, + X86CPUTopoIDs *topo_ids) +{ + topo_ids->smt_id = apicid & + ~(0xFFFFFFFFUL << apicid_smt_width(topo_info)); + topo_ids->core_id = + (apicid >> apicid_core_offset(topo_info)) & + ~(0xFFFFFFFFUL << apicid_core_width(topo_info)); + topo_ids->die_id = + (apicid >> apicid_die_offset(topo_info)) & + ~(0xFFFFFFFFUL << apicid_die_width(topo_info)); + topo_ids->node_id = + (apicid >> apicid_node_offset_epyc(topo_info)) & + ~(0xFFFFFFFFUL << apicid_node_width_epyc(topo_info)); + topo_ids->pkg_id = apicid >> apicid_pkg_offset_epyc(topo_info); +} + +/* + * Make APIC ID for the CPU 'cpu_index' + * + * 'cpu_index' is a sequential, contiguous ID for the CPU. + */ +static inline apic_id_t x86_apicid_from_cpu_idx_epyc(X86CPUTopoInfo *topo_info, + unsigned cpu_index) +{ + X86CPUTopoIDs topo_ids; + x86_topo_ids_from_idx_epyc(topo_info, cpu_index, &topo_ids); + return x86_apicid_from_topo_ids_epyc(topo_info, &topo_ids); +} /* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID * * The caller must make sure core_id < nr_cores and smt_id < nr_threads.