From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34671) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8lB7-00077o-Bb for qemu-devel@nongnu.org; Tue, 15 Dec 2015 03:41:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8lB2-000598-B7 for qemu-devel@nongnu.org; Tue, 15 Dec 2015 03:41:49 -0500 Received: from e28smtp03.in.ibm.com ([125.16.236.3]:42955) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8lB1-00055m-KP for qemu-devel@nongnu.org; Tue, 15 Dec 2015 03:41:44 -0500 Received: from localhost by e28smtp03.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 15 Dec 2015 14:11:40 +0530 Received: from d28relay02.in.ibm.com (d28relay02.in.ibm.com [9.184.220.59]) by d28dlp03.in.ibm.com (Postfix) with ESMTP id 525BB125805D for ; Tue, 15 Dec 2015 14:12:05 +0530 (IST) Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay02.in.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id tBF8fXan11993426 for ; Tue, 15 Dec 2015 14:11:34 +0530 Received: from d28av01.in.ibm.com (localhost [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id tBF8fWWf013183 for ; Tue, 15 Dec 2015 14:11:33 +0530 Date: Tue, 15 Dec 2015 14:11:06 +0530 From: Bharata B Rao Message-ID: <20151215084106.GJ18759@in.ibm.com> References: <1449728144-6223-1-git-send-email-bharata@linux.vnet.ibm.com> <1449728144-6223-2-git-send-email-bharata@linux.vnet.ibm.com> <20151210102528.GB28508@redhat.com> <20151211032431.GB18759@in.ibm.com> <20151214173752.GD3774@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151214173752.GD3774@thinpad.lan.raisama.net> Subject: Re: [Qemu-devel] [RFC PATCH v0 1/9] vl: Don't allow CPU toplogies with partially filled cores Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org, agraf@suse.de, borntraeger@de.ibm.com, pbonzini@redhat.com, imammedo@redhat.com, afaerber@suse.de, david@gibson.dropbear.id.au 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 > > > > --- > > > > 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.