RE: [PATCH v4 2/3] hw/i386: Update the EPYC topology to use socket/dies/core/thread model

[Babu Moger <babu.moger@amd.com>]

> -----Original Message-----
> From: Igor Mammedov <imammedo@redhat.com>
> Sent: Wednesday, August 19, 2020 6:26 AM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: pbonzini@redhat.com; rth@twiddle.net; ehabkost@redhat.com;
> mst@redhat.com; marcel.apfelbaum@gmail.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v4 2/3] hw/i386: Update the EPYC topology to use
> socket/dies/core/thread model
> 
> On Fri, 14 Aug 2020 16:39:33 -0500
> Babu Moger <babu.moger@amd.com> wrote:
> 
> > Update the EPYC topology to use socket/dies/core/thread model. The
> > EPYC model does not use the smp dies to build the topology. Instead,
> > it uses numa nodes to build the topology. Internally both are similar
> > concept which divides the cores on L3 boundary. Combining both into
> > one terminology makes it simple to program.
> >
> > Add a new check to error out when smp dies are not provided when EPYC
> > model is numa configured. Next task is to remove node_id, nr_nodes and
> > nodes_per_pkg from EPYC topology which will be done in next patch.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  hw/i386/x86.c |    8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/hw/i386/x86.c b/hw/i386/x86.c index
> > 67bee1bcb8..e90c42d2fc 100644
> > --- a/hw/i386/x86.c
> > +++ b/hw/i386/x86.c
> > @@ -138,6 +138,14 @@ void x86_cpus_init(X86MachineState *x86ms, int
> > default_cpu_version)
> >
> >      /* Check for apicid encoding */
> >      if (cpu_x86_use_epyc_apic_id_encoding(ms->cpu_type)) {
> > +        if ((ms->numa_state->num_nodes > 0) &&
> 
> > +            ms->numa_state->num_nodes != (ms->smp.sockets *
> > + x86ms->smp_dies)) {
> this case is gated by (ms->numa_state->num_nodes > 0) so it won't work in case
> -smp dies=>1 but there is no -numa options at all

Looking back again, this check does not appear to be a good idea. The numa
information is coming from ACPI and die information is coming from CPU
internal hardware config. It may not match. I don't think this check will
be good option. Sorry. I did not think about that earlier.

> 
> we need to error out and ask to provide numa nodes corresponding to
>    (ms->numa_state->num_nodes == 0) && (ms->smp.sockets * x86ms-
> >smp_dies)
> 
> or better alternative would be to enable autonuma when EPYC cpu is enabled,
> that will insure that this patch will work even if user hasn't specified -numa
> option, since it will create a single numa node automatically.
> 
> The later will take care of (-smp 1,dies=1) case, which is broken due to lack of
> explicit -numa we end up with CPU_UNSET_NUMA_NODE_ID in
> CPUID_Fn8000001E_ECX.
> 
> > +            error_setg(&error_fatal, "Numa configuration here requires smp "
> > +                       "'dies' parameter. Configure the cpu topology properly "
> > +                       "with max_cpus = sockets * dies * cores * threads. Dies"
> > +                       " is equivalent to number of numa nodes in a socket.");
> > +            return;
> > +        }
> >          x86_set_epyc_topo_handlers(ms);
> >      }
> >
> >