[v7,07/13] hw/386: Add EPYC mode topology decoding functions

Commit Message

Babu Moger March 11, 2020, 10:53 p.m. UTC

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 <[e-mail]>
Acked-by: Igor Mammedov <[e-mail]>
Acked-by: Michael S. Tsirkin <[e-mail]>
---
 include/hw/i386/topology.h |  100 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

Comments

Igor Mammedov March 12, 2020, 12:39 p.m. UTC | #1

On Wed, 11 Mar 2020 17:53:34 -0500
Babu Moger <[e-mail]> 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 <[e-mail]>
> Acked-by: Igor Mammedov <[e-mail]>
> Acked-by: Michael S. Tsirkin <[e-mail]>
[...]
> +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;
> +}
> +
> +/*
[...]

Babu Moger March 12, 2020, 1:44 p.m. UTC | #2

On 3/12/20 7:39 AM, Igor Mammedov wrote:
> On Wed, 11 Mar 2020 17:53:34 -0500
> Babu Moger <[e-mail]> 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&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C3d1032fb1cc94a5a197308d7c68268c5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637196135715465573&amp;sdata=13zSN7AqPGKHFG%2FePmkWTVbwM0qzktrnolEidnNzyhs%3D&amp;reserved=0
>>
>> Signed-off-by: Babu Moger <[e-mail]>
>> Acked-by: Igor Mammedov <[e-mail]>
>> Acked-by: Michael S. Tsirkin <[e-mail]>
> [...]
>> +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;
>> +}
>> +
>> +/*
> [...]
>

Eduardo Habkost June 2, 2020, 5:18 p.m. UTC | #3

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 <[e-mail]>
> Acked-by: Igor Mammedov <[e-mail]>
> Acked-by: Michael S. Tsirkin <[e-mail]>
> ---
[...]
>  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.

Babu Moger June 2, 2020, 5:27 p.m. UTC | #4

Oh. Ok.. Will look at it. thanks

> -----Original Message-----
> From: Eduardo Habkost <[e-mail]>
> Sent: Tuesday, June 2, 2020 12:19 PM
> To: Moger, Babu <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> 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&amp;data=02%7C01%7Cbabu.m
> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&amp;sdata=wE0
> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&amp;reserved=0
> >
> > Signed-off-by: Babu Moger <[e-mail]>
> > Acked-by: Igor Mammedov <[e-mail]>
> > Acked-by: Michael S. Tsirkin <[e-mail]>
> > ---
> [...]
> >  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

Babu Moger June 2, 2020, 9:59 p.m. UTC | #5

> -----Original Message-----
> From: Eduardo Habkost <[e-mail]>
> Sent: Tuesday, June 2, 2020 12:19 PM
> To: Moger, Babu <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> 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&amp;data=02%7C01%7Cbabu.m
> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&amp;sdata=wE0
> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&amp;reserved=0
> >
> > Signed-off-by: Babu Moger <[e-mail]>
> > Acked-by: Igor Mammedov <[e-mail]>
> > Acked-by: Michael S. Tsirkin <[e-mail]>
> > ---
> [...]
> >  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);
     }

Eduardo Habkost June 2, 2020, 11:01 p.m. UTC | #6

On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote:
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost <[e-mail]>
> > Sent: Tuesday, June 2, 2020 12:19 PM
> > To: Moger, Babu <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]
> > 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&amp;data=02%7C01%7Cbabu.m
> > oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> > 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&amp;sdata=wE0
> > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&amp;reserved=0
> > >
> > > Signed-off-by: Babu Moger <[e-mail]>
> > > Acked-by: Igor Mammedov <[e-mail]>
> > > Acked-by: Michael S. Tsirkin <[e-mail]>
> > > ---
> > [...]
> > >  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);
>      }
>

Babu Moger June 3, 2020, 12:07 a.m. UTC | #7

> -----Original Message-----
> From: Eduardo Habkost <[e-mail]>
> Sent: Tuesday, June 2, 2020 6:01 PM
> To: Moger, Babu <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> 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 <[e-mail]>
> > > Sent: Tuesday, June 2, 2020 12:19 PM
> > > To: Moger, Babu <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail]
> > > 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&amp;data=02%7C01%7Cbabu.m
> > >
> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> > >
> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&amp;sdata=wE0
> > > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&amp;reserved=0
> > > >
> > > > Signed-off-by: Babu Moger <[e-mail]>
> > > > Acked-by: Igor Mammedov <[e-mail]>
> > > > Acked-by: Michael S. Tsirkin <[e-mail]>
> > > > ---
> > > [...]
> > > >  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

Babu Moger June 3, 2020, 3:22 p.m. UTC | #8

> -----Original Message-----
> From: Eduardo Habkost <[e-mail]>
> Sent: Tuesday, June 2, 2020 6:01 PM
> To: Moger, Babu <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> 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 <[e-mail]>
> > > Sent: Tuesday, June 2, 2020 12:19 PM
> > > To: Moger, Babu <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail]
> > > 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&amp;data=02%7C01%7Cbabu.m
> > >
> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> > >
> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&amp;sdata=wE0
> > > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&amp;reserved=0
> > > >
> > > > Signed-off-by: Babu Moger <[e-mail]>
> > > > Acked-by: Igor Mammedov <[e-mail]>
> > > > Acked-by: Michael S. Tsirkin <[e-mail]>
> > > > ---
> > > [...]
> > > >  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

Eduardo Habkost June 3, 2020, 3:31 p.m. UTC | #9

On Wed, Jun 03, 2020 at 10:22:10AM -0500, Babu Moger wrote:
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost <[e-mail]>
> > Sent: Tuesday, June 2, 2020 6:01 PM
> > To: Moger, Babu <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]
> > 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 <[e-mail]>
> > > > Sent: Tuesday, June 2, 2020 12:19 PM
> > > > To: Moger, Babu <[e-mail]>
> > > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail]
> > > > 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&amp;data=02%7C01%7Cbabu.m
> > > >
> > oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> > > >
> > 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&amp;sdata=wE0
> > > > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&amp;reserved=0
> > > > >
> > > > > Signed-off-by: Babu Moger <[e-mail]>
> > > > > Acked-by: Igor Mammedov <[e-mail]>
> > > > > Acked-by: Michael S. Tsirkin <[e-mail]>
> > > > > ---
> > > > [...]
> > > > >  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
>

Babu Moger June 3, 2020, 3:38 p.m. UTC | #10

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 <[e-mail]>
>>> Sent: Tuesday, June 2, 2020 6:01 PM
>>> To: Moger, Babu <[e-mail]>
>>> Cc: [e-mail]; [e-mail]; [e-mail];
>>> [e-mail]; [e-mail]; [e-mail]
>>> 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 <[e-mail]>
>>>>> Sent: Tuesday, June 2, 2020 12:19 PM
>>>>> To: Moger, Babu <[e-mail]>
>>>>> Cc: [e-mail]; [e-mail]; [e-mail];
>>>>> [e-mail]; [e-mail]; [e-mail]
>>>>> 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&amp;data=02%7C01%7Cbabu.m
>>>>>
>>> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
>>>>>
>>> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&amp;sdata=wE0
>>>>> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&amp;reserved=0
>>>>>>
>>>>>> Signed-off-by: Babu Moger <[e-mail]>
>>>>>> Acked-by: Igor Mammedov <[e-mail]>
>>>>>> Acked-by: Michael S. Tsirkin <[e-mail]>
>>>>>> ---
>>>>> [...]
>>>>>>  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&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C7752c36b7ad24df4039d08d807d334fa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637267951007572953&amp;sdata=69rG4w79FnLs9bhNmroxsRckzmy%2BGuH1dMjyP8PPRUo%3D&amp;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
>>
>

Eduardo Habkost June 3, 2020, 3:45 p.m. UTC | #11

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 <[e-mail]>
> >>> Sent: Tuesday, June 2, 2020 6:01 PM
> >>> To: Moger, Babu <[e-mail]>
> >>> Cc: [e-mail]; [e-mail]; [e-mail];
> >>> [e-mail]; [e-mail]; [e-mail]
> >>> 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 <[e-mail]>
> >>>>> Sent: Tuesday, June 2, 2020 12:19 PM
> >>>>> To: Moger, Babu <[e-mail]>
> >>>>> Cc: [e-mail]; [e-mail]; [e-mail];
> >>>>> [e-mail]; [e-mail]; [e-mail]
> >>>>> 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&amp;data=02%7C01%7Cbabu.m
> >>>>>
> >>> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> >>>>>
> >>> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&amp;sdata=wE0
> >>>>> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&amp;reserved=0
> >>>>>>
> >>>>>> Signed-off-by: Babu Moger <[e-mail]>
> >>>>>> Acked-by: Igor Mammedov <[e-mail]>
> >>>>>> Acked-by: Michael S. Tsirkin <[e-mail]>
> >>>>>> ---
> >>>>> [...]
> >>>>>>  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&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C7752c36b7ad24df4039d08d807d334fa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637267951007572953&amp;sdata=69rG4w79FnLs9bhNmroxsRckzmy%2BGuH1dMjyP8PPRUo%3D&amp;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.

Babu Moger June 3, 2020, 3:49 p.m. UTC | #12

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 <[e-mail]>
>>>>> Sent: Tuesday, June 2, 2020 6:01 PM
>>>>> To: Moger, Babu <[e-mail]>
>>>>> Cc: [e-mail]; [e-mail]; [e-mail];
>>>>> [e-mail]; [e-mail]; [e-mail]
>>>>> 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 <[e-mail]>
>>>>>>> Sent: Tuesday, June 2, 2020 12:19 PM
>>>>>>> To: Moger, Babu <[e-mail]>
>>>>>>> Cc: [e-mail]; [e-mail]; [e-mail];
>>>>>>> [e-mail]; [e-mail]; [e-mail]
>>>>>>> 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&amp;data=02%7C01%7Cbabu.m
>>>>>>>
>>>>> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
>>>>>>>
>>>>> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&amp;sdata=wE0
>>>>>>> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&amp;reserved=0
>>>>>>>>
>>>>>>>> Signed-off-by: Babu Moger <[e-mail]>
>>>>>>>> Acked-by: Igor Mammedov <[e-mail]>
>>>>>>>> Acked-by: Michael S. Tsirkin <[e-mail]>
>>>>>>>> ---
>>>>>>> [...]
>>>>>>>>  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&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C4b409b77ff2c4c4ed90608d807d53570%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637267959601716206&amp;sdata=ih6bo9Wp0RbJgRpryzSa2D0v6kr3Zfww%2B1uB%2FNyngk8%3D&amp;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

Babu Moger June 3, 2020, 9:49 p.m. UTC | #13

> -----Original Message-----
> From: Eduardo Habkost <[e-mail]>
> Sent: Wednesday, June 3, 2020 10:46 AM
> To: Moger, Babu <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> 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 <[e-mail]>
> > >>> Sent: Tuesday, June 2, 2020 6:01 PM
> > >>> To: Moger, Babu <[e-mail]>
> > >>> Cc: [e-mail]; [e-mail];
> [e-mail];
> > >>> [e-mail]; [e-mail]; [e-mail]
> > >>> 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 <[e-mail]>
> > >>>>> Sent: Tuesday, June 2, 2020 12:19 PM
> > >>>>> To: Moger, Babu <[e-mail]>
> > >>>>> Cc: [e-mail]; [e-mail];
> [e-mail];
> > >>>>> [e-mail]; [e-mail]; [e-mail]
> > >>>>> 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&amp;data=02%7C01%7Cbabu.m
> > >>>>>
> > >>>
> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> > >>>>>
> > >>>
> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&amp;sdata=wE0
> > >>>>> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&amp;reserved=0
> > >>>>>>
> > >>>>>> Signed-off-by: Babu Moger <[e-mail]>
> > >>>>>> Acked-by: Igor Mammedov <[e-mail]>
> > >>>>>> Acked-by: Michael S. Tsirkin <[e-mail]>
> > >>>>>> ---
> > >>>>> [...]
> > >>>>>>  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&amp;data=02%7C01%7Cbabu.moger%40amd.com%7
> C4b409b77ff2c4c4ed90608d807d53570%7C3dd8961fe4884e608e11a82d994e1
> 83d%7C0%7C0%7C637267959601716206&amp;sdata=ih6bo9Wp0RbJgRpryzSa2
> D0v6kr3Zfww%2B1uB%2FNyngk8%3D&amp;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;

Dr. David Alan Gilbert June 15, 2020, 11:44 a.m. UTC | #14

* Babu Moger ([e-mail]) wrote:
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost <[e-mail]>
> > Sent: Wednesday, June 3, 2020 10:46 AM
> > To: Moger, Babu <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]
> > 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 <[e-mail]>
> > > >>> Sent: Tuesday, June 2, 2020 6:01 PM
> > > >>> To: Moger, Babu <[e-mail]>
> > > >>> Cc: [e-mail]; [e-mail];
> > [e-mail];
> > > >>> [e-mail]; [e-mail]; [e-mail]
> > > >>> 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 <[e-mail]>
> > > >>>>> Sent: Tuesday, June 2, 2020 12:19 PM
> > > >>>>> To: Moger, Babu <[e-mail]>
> > > >>>>> Cc: [e-mail]; [e-mail];
> > [e-mail];
> > > >>>>> [e-mail]; [e-mail]; [e-mail]
> > > >>>>> 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&amp;data=02%7C01%7Cbabu.m
> > > >>>>>
> > > >>>
> > oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> > > >>>>>
> > > >>>
> > 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739&amp;sdata=wE0
> > > >>>>> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3D&amp;reserved=0
> > > >>>>>>
> > > >>>>>> Signed-off-by: Babu Moger <[e-mail]>
> > > >>>>>> Acked-by: Igor Mammedov <[e-mail]>
> > > >>>>>> Acked-by: Michael S. Tsirkin <[e-mail]>
> > > >>>>>> ---
> > > >>>>> [...]
> > > >>>>>>  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&amp;data=02%7C01%7Cbabu.moger%40amd.com%7
> > C4b409b77ff2c4c4ed90608d807d53570%7C3dd8961fe4884e608e11a82d994e1
> > 83d%7C0%7C0%7C637267959601716206&amp;sdata=ih6bo9Wp0RbJgRpryzSa2
> > D0v6kr3Zfww%2B1uB%2FNyngk8%3D&amp;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 / [e-mail] / Manchester, UK

Patch

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.