Re: [Qemu-devel] [PATCH v5 01/10] vl: Don't allow CPU toplogies with partially filled cores

[Eduardo Habkost]

On Wed, Dec 02, 2015 at 02:52:39PM +0100, Igor Mammedov wrote:
> On Fri, 20 Nov 2015 18:24:30 +0530
> Bharata B Rao <address@hidden> wrote:
> 
> > Prevent guests from booting with CPU topologies that have partially
> > filled CPU cores or can result in partially filled CPU cores after
> > CPU hotplug like
> > 
> > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
> 
> CCing Eduardo who might tell if it's ok wrt x86

I am always afraid of introducing fatal errors for configurations
that are obviously wrong but may already exist in production (so
it would prevent users from migrating existing VMs). But I am
becoming more inclined to be a bit more aggressive and stop
allowing these broken configurations.

But I would rewrite the error message, and just say that "cpus"
and "maxcpus" must be multiples of "threads". It's easier to
understand and explain than what "partially filled cores" mean.
You don't need to mention "sockets" and "cores" in the error
message, either.

Also, please print different error messages to indicate if
"maxcpus" or "cpus" is the culprit.

What about (cpus % (cores * threads) != 0)? It would result in
sockets with different numbers of cores. Should we allow that?

The patch makes QEMU crash with the following command-line:

  $ qemu-system-x86_64 -smp 2,cores=2,sockets=2
  Floating point exception (core dumped)
  $

It can probably be solved by moving the check after
smp_{cpus,cores,threads} are already set.

> 
> > 
> > Signed-off-by: Bharata B Rao <address@hidden>
> > ---
> >  vl.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/vl.c b/vl.c
> > index 7d993a5..23a1a1e 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1248,6 +1248,15 @@ static void smp_parse(QemuOpts *opts)
> >              exit(1);
> >          }
> >  
> > +        if (cpus % threads || max_cpus % threads) {
> > +            error_report("cpu topology: "
> > +                         "sockets (%u) cores (%u) threads (%u) with "
> > +                         "smp_cpus (%u) maxcpus (%u) "
> > +                         "will result in partially filled cores",
> > +                         sockets, cores, threads, cpus, max_cpus);
> > +            exit(1);
> > +        }
> > +
> >          smp_cpus = cpus;
> >          smp_cores = cores > 0 ? cores : 1;
> >          smp_threads = threads > 0 ? threads : 1;
> 



-- 
Eduardo