From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40113) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YY7UE-0003QH-Ry for qemu-devel@nongnu.org; Wed, 18 Mar 2015 02:29:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YY7UB-0001zT-Ld for qemu-devel@nongnu.org; Wed, 18 Mar 2015 02:29:50 -0400 Received: from e23smtp09.au.ibm.com ([202.81.31.142]:36218) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YY7UB-0001uk-0i for qemu-devel@nongnu.org; Wed, 18 Mar 2015 02:29:47 -0400 Received: from /spool/local by e23smtp09.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 18 Mar 2015 16:29:44 +1000 Received: from d23relay08.au.ibm.com (d23relay08.au.ibm.com [9.185.71.33]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id 2F1DA2BB0054 for ; Wed, 18 Mar 2015 17:29:42 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay08.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t2I6TXBv22675546 for ; Wed, 18 Mar 2015 17:29:42 +1100 Received: from d23av03.au.ibm.com (localhost [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t2I6T82A027151 for ; Wed, 18 Mar 2015 17:29:08 +1100 Date: Wed, 18 Mar 2015 11:58:47 +0530 From: Bharata B Rao Message-ID: <20150318062847.GD21610@in.ibm.com> References: <1426247796-1657-1-git-send-email-bharata@linux.vnet.ibm.com> <55080738.2060705@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <55080738.2060705@suse.de> Subject: Re: [Qemu-devel] [RFC v0 PATCH] cpus: Convert cpu_index into a bitmap Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: zhugh.fnst@cn.fujitsu.com, qemu-devel@nongnu.org, agraf@suse.de, Bharata B Rao , imammedo@redhat.com, david@gibson.dropbear.id.au On Tue, Mar 17, 2015 at 11:51:36AM +0100, Andreas Färber wrote: > Am 13.03.2015 um 12:56 schrieb Bharata B Rao: > > From: Bharata B Rao > > > > Currently CPUState.cpu_index is monotonically increasing and a newly > > created CPU always gets the next higher index. The next available > > index is calculated by counting the existing number of CPUs. This is > > fine as long as we only add CPUs, but there are architectures which > > are starting to support CPU removal too. For an architecture like PowerPC > > which derives its CPU identifier (device tree ID) from cpu_index, the > > existing logic of generating cpu_index values causes problems. > > > > With the currently proposed method of handling vCPU removal by parking > > the vCPU fd in QEMU > > (Ref: http://lists.gnu.org/archive/html/qemu-devel/2015-02/msg02604.html), > > generating cpu_index this way will not work for PowerPC. > > > > This patch changes the way cpu_index is handed out by maintaining > > a bit map of the CPUs that tracks both addition and removal of CPUs. > > > > I am not sure if this is the right and an acceptable approach. The > > alternative is to do something similar for PowerPC alone and not > > depend on cpu_index. > > > > I have tested this with out-of-the-tree patches for CPU hot plug and > > removal on x86 and sPAPR PowerPC. > > > > Signed-off-by: Bharata B Rao > > --- > > exec.c | 39 +++++++++++++++++++++++++++++---------- > > include/exec/exec-all.h | 1 + > > target-alpha/cpu.c | 6 ++++++ > > target-arm/cpu.c | 1 + > > target-cris/cpu.c | 6 ++++++ > > target-i386/cpu.c | 6 ++++++ > > target-lm32/cpu.c | 6 ++++++ > > target-m68k/cpu.c | 6 ++++++ > > target-microblaze/cpu.c | 6 ++++++ > > target-mips/cpu.c | 6 ++++++ > > target-moxie/cpu.c | 6 ++++++ > > target-openrisc/cpu.c | 6 ++++++ > > target-ppc/translate_init.c | 6 ++++++ > > target-s390x/cpu.c | 1 + > > target-sh4/cpu.c | 6 ++++++ > > target-sparc/cpu.c | 1 + > > target-tricore/cpu.c | 5 +++++ > > target-unicore32/cpu.c | 6 ++++++ > > target-xtensa/cpu.c | 6 ++++++ > > 19 files changed, 116 insertions(+), 10 deletions(-) > > > > diff --git a/exec.c b/exec.c > > index e97071a..7760f2d 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -530,21 +530,40 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as) > > } > > #endif > > > > +static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS); > > I don't see this constant being defined in this patch. How large is it? > I wonder whether this might be stolen from an x86 ACPI/NUMA context and > forced onto all architectures now? I thought MAX_CPUMASK_BITS defines the max cpus possible. >>From include/sysemu/sysemu.h /* The following shall be true for all CPUs: * cpu->cpu_index < max_cpus <= MAX_CPUMASK_BITS * * Note that cpu->get_arch_id() may be larger than MAX_CPUMASK_BITS. */ #define MAX_CPUMASK_BITS 255 > > > + > > +#ifdef CONFIG_USER_ONLY > > +int max_cpus = 1; /* TODO: Check if this is correct ? */ > > This strikes me as wrong, as forking will create a copy of the initial > CPUState, see cpu_copy(). The cpu_index might get overwritten inside the > CPUState, not sure about that, but this here is global state. > > Can't we just keep the current code for CONFIG_USER_ONLY and skip all > the bitmap functions? Yes that would better than to touch CONFIG_USER_ONLY. > > > +#endif > > + > > +static int cpu_get_free_index(void) > > +{ > > + int cpu = find_first_zero_bit(cpu_index_map, max_cpus); > > + > > + if (cpu == max_cpus) { > > + fprintf(stderr, "WARNING: qemu: Trying to use more " > > + "CPUs than allowed max of %d\n", max_cpus); > > This is a bad API. :) If we can't handle it, use an Error** errp > argument to pass that info outwards. Imagine this happening from QMP > device-add, then this warning will go unnoticed on stderr. I wanted to have an Error argument, but realized that instance_init doesn't take an Error argument and as a consequence, it appears that object_new() can't fail. So I depended on (cpu->cpu_index >= max_cpus) or equivalent check in realize call to catch this error. But as being discussed in other thread on the same subject, if cpu_exec_init call moves to realize, this will get solved neatly. Regards, Bharata.