Re: [PATCH v3 2/3] hw/i386: Add a new check to configure smp dies for EPYC

From: Igor Mammedov <imammedo@redhat.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: ehabkost@redhat.com, mst@redhat.com, qemu-devel@nongnu.org,
	Babu Moger <babu.moger@amd.com>,
	pbonzini@redhat.com, rth@twiddle.net
Subject: Re: [PATCH v3 2/3] hw/i386: Add a new check to configure smp dies for EPYC
Date: Fri, 7 Aug 2020 21:11:48 +0200	[thread overview]
Message-ID: <20200807211148.77749e6c@redhat.com> (raw)
In-Reply-To: <20200807165222.GB120942@redhat.com>

On Fri, 7 Aug 2020 17:52:22 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Fri, Aug 07, 2020 at 11:32:51AM -0500, Babu Moger wrote:
> > Adding a new check to warn the users to configure 'dies' when
> > topology is numa configured. It makes it easy to build the
> > topology for EPYC models.  
> 
> This says you're adding a warning....
> 
> > 
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  hw/i386/x86.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> > index 67bee1bcb8..2a6ce56ef1 100644
> > --- a/hw/i386/x86.c
> > +++ b/hw/i386/x86.c
> > @@ -138,6 +138,13 @@ 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)) {
> > +            error_setg(&error_fatal, "Numa configuration requires smp 'dies' "
> > +                       "parameter. Configure the cpu topology properly with "
> > +                       "max_cpus = sockets * dies * cores * threads");  
> 
> ...but you're actually making this a fatal error, not a warning.
> 
> I'm not sure this is really OK. Wouldn't this mean that existing VMs
> deployed today, risk triggering this fatal error next time they
> are booted, or live migrated.  If it is possible someone is using
> such a config today, I don't think we can break it.

to begin with, users shouldn't have used 'dies' with initial impl. at all.
(it was Intel introduced option and EPYC's added very similar internal node_id
(removed by the next patch)).
Now we are trying to consolidate this mess and reuse dies for EPYC.

EPYC was out in the since with 5.0 (though broken), users could start a VM with
such config but that would not be correct EPYC from apicid and cpuid point of view.
Guest OS might run if it doesn't know about EPYCs or behave wierdly (sub optimal|crash|whatever)
on seeing unexpected values.

If we are hell bound on keeping bugs of initial impl, then we should keep it to 5.1<=
machine version and do the right thing for newer ones.
Though I'm not sure we should keep broken variant around (all we would get from it is
bug reports*/complains from users with end result of their config anyways).
I'd rather error out with clear error message so user could fix their broken config.

*) there is at least one thread/bz on qemu-devel where users are trying to run
with EPYC and pick up options combination so it would produce sensible topology.

> Regards,
> Daniel