[libvirt] [Qemu-devel] [PATCH] vl.c deprecate incorrect CPUs topology

[Igor Mammedov]

On Mon, 27 Aug 2018 14:13:43 +0200
Andrew Jones <drjones at redhat.com> wrote:

> On Mon, Aug 27, 2018 at 01:56:08PM +0200, Igor Mammedov wrote:
> > -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> > so that total number of logical CPUs [sockets * cores * threads]
> > would be equal to [maxcpus], however historically we didn't have
> > such check in QEMU and it is possible to start VM with an invalid
> > topology.
> > Deprecate invalid options combination so we can make sure that
> > the topology VM started with is always correct in the future.
> > Users with an invalid sockets/cores/threads/maxcpus values should
> > fix their CLI to make sure that
> >    [sockets * cores * threads] == [maxcpus]
> > 
> > Signed-off-by: Igor Mammedov <imammedo at redhat.com>
> > ---
> >  qemu-deprecated.texi | 11 +++++++++++
> >  vl.c                 |  6 ++++++
> >  2 files changed, 17 insertions(+)
> > 
> > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > index 87212b6..d5d9ce6 100644
> > --- a/qemu-deprecated.texi
> > +++ b/qemu-deprecated.texi
> > @@ -159,6 +159,17 @@ The 'file' driver for drives is no longer appropriate for character or host
> >  devices and will only accept regular files (S_IFREG). The correct driver
> >  for these file types is 'host_cdrom' or 'host_device' as appropriate.
> >  
> > + at subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)  
> 
> sockets, cores, threads
> 
> > +
> > +CPU topology properties should describe whole machine topology including
> > +possible CPUs. but historically it was possible to start QEMU with  
> 
> /./,/
> 
> > +an incorrect topology where
> > +  socket * core * thread >= X && < maxcpus  
> 
> sockets * cores * threads
> 
> && X < maxcpus
> 
> > +which could lead to incorrect topology enumeration by the guest.
> > +Support for invalid topology will be removed, end user must ensure  
> topologies
> /end user/the user/
> 
> > +that topology described with -smp includes all possible cpus, i.e.:  
> /that/the
> 
> > +  socket * core * thread == maxcpus  
> 
> sockets * cores * threads
> 
> > +
> >  @section QEMU Machine Protocol (QMP) commands
> >  
> >  @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
> > diff --git a/vl.c b/vl.c
> > index 5ba06ad..bc53828 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1238,6 +1238,12 @@ static void smp_parse(QemuOpts *opts)
> >              exit(1);
> >          }
> >  
> > +        if (sockets * cores * threads != max_cpus) {
> > +            warn_report("Invalid CPUs topology deprecated. "  
> 
> /CPUs/CPU/
> 
> Not sure we want a period after deprecated. Would ':' be more appropriate?
> 
> > +                        "sockets (%u) * cores (%u) * threads (%u) "
> > +                        "!= maxcpus (%u)",
> > +                        sockets, cores, threads, max_cpus);
> > +        }
> >          if (sockets * cores * threads > max_cpus) {
> >              error_report("cpu topology: "
> >                           "sockets (%u) * cores (%u) * threads (%u) > "
> > -- 
> > 2.7.4
> >  
> 
> Thanks,
> drew 
> 

Thanks for corrections, v2 is on the way