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 <bhar...@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. -- Eduardo