[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel] [PATCH v2 3/6] exec: set cpu_index only if i
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-ppc] [Qemu-devel] [PATCH v2 3/6] exec: set cpu_index only if it's not been explictly set |
Date: |
Wed, 27 Jul 2016 10:45:00 +0200 |
On Tue, 26 Jul 2016 15:28:13 -0300
Eduardo Habkost <address@hidden> 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 <address@hidden>
> > Reviewed-by: David Gibson <address@hidden>
> [...]
> > -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 <address@hidden>
>
- [Qemu-ppc] [PATCH v2 0/6] Fix migration issues with arbitrary cpu-hot(un)plug, Igor Mammedov, 2016/07/25
- [Qemu-ppc] [PATCH v2 2/6] exec: don't use cpu_index to detect if cpu_exec_init()'s been called for cpu, Igor Mammedov, 2016/07/25
- [Qemu-ppc] [PATCH v2 1/6] exec: reduce CONFIG_USER_ONLY ifdeffenery, Igor Mammedov, 2016/07/25
- [Qemu-ppc] [PATCH v2 3/6] exec: set cpu_index only if it's not been explictly set, Igor Mammedov, 2016/07/25
- [Qemu-ppc] [PATCH v2 4/6] qdev: fix object reference leak in case device.realize() fails, Igor Mammedov, 2016/07/25
- [Qemu-ppc] [PATCH v2 6/6] Revert "pc: Enforce adding CPUs contiguously and removing them in opposite order", Igor Mammedov, 2016/07/25
- [Qemu-ppc] [PATCH v2 5/6] pc: init CPUState->cpu_index with index in possible_cpus[], Igor Mammedov, 2016/07/25
- Re: [Qemu-ppc] [PATCH v2 0/6] Fix migration issues with arbitrary cpu-hot(un)plug, David Gibson, 2016/07/25
- Re: [Qemu-ppc] [PATCH v2 0/6] Fix migration issues with arbitrary cpu-hot(un)plug, David Gibson, 2016/07/26
- Re: [Qemu-ppc] [PATCH v2 0/6] Fix migration issues with arbitrary cpu-hot(un)plug, Michael S. Tsirkin, 2016/07/26
- Re: [Qemu-ppc] [PATCH v2 0/6] Fix migration issues with arbitrary cpu-hot(un)plug, Eduardo Habkost, 2016/07/26