From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34383) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bSKSm-0005ST-HD for qemu-devel@nongnu.org; Wed, 27 Jul 2016 04:45:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bSKSf-0007hd-CP for qemu-devel@nongnu.org; Wed, 27 Jul 2016 04:45:11 -0400 Date: Wed, 27 Jul 2016 10:45:00 +0200 From: Igor Mammedov Message-ID: <20160727104500.3ee6be9e@nial.brq.redhat.com> In-Reply-To: <20160726182813.GB3337@thinpad.lan.raisama.net> References: <1469440764-61619-1-git-send-email-imammedo@redhat.com> <1469440764-61619-4-git-send-email-imammedo@redhat.com> <20160726182813.GB3337@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 3/6] exec: set cpu_index only if it's not been explictly set List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Riku Voipio , "Michael S. Tsirkin" , Peter Crosthwaite , qemu-devel@nongnu.org, Alexander Graf , qemu-ppc@nongnu.org, Bharata B Rao , Paolo Bonzini , David Gibson , Richard Henderson On Tue, 26 Jul 2016 15:28:13 -0300 Eduardo Habkost wrote: > On Mon, Jul 25, 2016 at 11:59:21AM +0200, Igor Mammedov wrote: > > it keeps the legacy behavior for all users that doesn't care > > about stable cpu_index value, but would allow boards that > > would support device_add/device_del to set stable cpu_index > > that won't depend on order in which cpus are created/destroyed. > > > > While at that simplify cpu_get_free_index() as cpu_index > > generated by USER_ONLY and softmmu variants is the same > > since none of the users support cpu-remove so far, except > > of not yet released spapr/x86 device_add/delr, which > > will be altered by follow up patches to set stable > > cpu_index manually. > > So, cpu_get_free_index() behavior is exactly the same because > cpu-remove is either unsupported, or only supported for the last > CPU. But I worry that this will easily break if anybody starts > implementing CPU removal in other machines without setting > cpu_index explicitly in the board code. Then we can make > cpu_get_free_index() generate a duplicate cpu_index. > > I wonder if there any way we can add an assert() somewhere to > ensure no machine will ever allow CPU removal while not > initializing cpu_index explicitly. > > (This shouldn't hold this patch, it's just a suggestion for a > possible follow-up patch). I'll try to post it shortly on top of this patch > > Additional comment: > > > > > Signed-off-by: Igor Mammedov > > Reviewed-by: David Gibson > [...] > > -static int cpu_get_free_index(Error **errp) > > -{ > > - int cpu = find_first_zero_bit(cpu_index_map, MAX_CPUMASK_BITS); > > - > > - if (cpu >= MAX_CPUMASK_BITS) { > > - error_setg(errp, "Trying to use more CPUs than max of %d", > > - MAX_CPUMASK_BITS); > > - return -1; > > - } > > We are now relying on the rest of the QEMU code to make sure > cpu_index will be always < MAX_CPUMASK_BITS. In this case, I > suggest we add an assert() below: > > [...] > > - cpu->cpu_index = cpu_get_free_index(&local_err); > > - if (local_err) { > > - error_propagate(errp, local_err); > > - cpu_list_unlock(); > > - return; > > + if (cpu->cpu_index == UNASSIGNED_CPU_INDEX) { > > + cpu->cpu_index = cpu_get_free_index(); > > + assert(cpu->cpu_index != UNASSIGNED_CPU_INDEX); > > } > > Here: > assert(cpu->cpu_index <= MAX_CPUMASK_BITS) I'd rather get rid of MAX_CPUMASK_BITS but I haven't looked at how hard it will be yet. > > > Both comments can be addressed in a follow-up patch, so: > > Reviewed-by: Eduardo Habkost >