Re: [Qemu-devel] [RFC PATCH v0 1/9] vl: Don't allow CPU toplogies with partially filled cores

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Mon, Dec 14, 2015 at 03:37:52PM -0200, Eduardo Habkost wrote:
> On Fri, Dec 11, 2015 at 08:54:31AM +0530, Bharata B Rao wrote:
> > On Thu, Dec 10, 2015 at 10:25:28AM +0000, Daniel P. Berrange wrote:
> > > On Thu, Dec 10, 2015 at 11:45:36AM +0530, Bharata B Rao 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 or
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  vl.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > > 
> > > > diff --git a/vl.c b/vl.c
> > > > index 525929b..e656f53 100644
> > > > --- a/vl.c
> > > > +++ b/vl.c
> > > > @@ -1252,6 +1252,19 @@ static void smp_parse(QemuOpts *opts)
> > > >          smp_cores = cores > 0 ? cores : 1;
> > > >          smp_threads = threads > 0 ? threads : 1;
> > > >  
> > > > +        if (smp_cpus % smp_threads) {
> > > > +            error_report("cpu topology: "
> > > > +                         "smp_cpus (%u) should be multiple of threads (%u)",
> > > > +                         smp_cpus, smp_threads);
> > > > +            exit(1);
> > > > +        }
> > > > +
> > > > +        if (max_cpus % smp_threads) {
> > > > +            error_report("cpu topology: "
> > > > +                         "maxcpus (%u) should be multiple of threads (%u)",
> > > > +                         max_cpus, smp_threads);
> > > > +            exit(1);
> > > > +        }
> > > >      }
> > > 
> > > Adding this seems like it has a pretty high chance of causing regression,
> > > ie preventing previously working guests from booting with new QEMU. I
> > > know adding the check makes sense from a semantic POV, but are we willing
> > > to risk breaking people with such odd configurations ?
> > 
> > I wasn't sure about how much risk that would be and hence in my older
> > version of PowerPC CPU hotplug patchset, I indeed supported such topologies:
> > 
> > https://lists.gnu.org/archive/html/qemu-ppc/2015-09/msg00102.html
> > 
> > But the code indeed looked ugly to support such special case.
> > 
> > There was some discussion about this recently here:
> > 
> > http://lists.gnu.org/archive/html/qemu-devel/2015-12/msg00396.html
> > 
> > from where I sensed that it may be ok to dis-allow such topologies.
> 
> I want to be as strict as possible and disallow such topologies,
> but Daniel has a point. Maybe we should make those checks
> machine-specific, so we can make pc-*-2.5 and older allow those
> broken configs.
> 
> If we make it a MachineClass::validate_smp_config() method, for
> example, we could make TYPE_MACHINE point to a generic function
> containing the checks you implemented above (so all machines have
> those checks enabled by default), but let pc <= 2.5 override the
> method.

Nice suggestion, will give it a try in the next iteration.

Regards,
Bharata.