Re: [Qemu-devel] [PULL v3 05/22] cpu: Convert cpu_index into a bitmap

[Bharata B Rao]

On Tue, Jul 14, 2015 at 5:17 PM, Igor Mammedov <address@hidden> wrote:
> On Tue, 14 Jul 2015 16:08:54 +0530
> Bharata B Rao <address@hidden> wrote:
>
>> On Thu, Jul 09, 2015 at 03:23:55PM +0200, Andreas Färber wrote:
>> > From: Bharata B Rao <address@hidden>
>> >
>> > 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.
>> >
>> > The CPU bitmap allocation logic is part of cpu_exec_init(), which is
>> > called by instance_init routines of various CPU targets. Newly added
>> > cpu_exec_exit() API handles the deallocation part and this routine is
>> > called from generic CPU instance_finalize.
>> >
>> > Note: This new CPU enumeration is for !CONFIG_USER_ONLY only.
>> > CONFIG_USER_ONLY continues to have the old enumeration logic.
>> >
>> > Signed-off-by: Bharata B Rao <address@hidden>
>> > Reviewed-by: Eduardo Habkost <address@hidden>
>> > Reviewed-by: Igor Mammedov <address@hidden>
>> > Reviewed-by: David Gibson <address@hidden>
>> > Reviewed-by: Peter Crosthwaite <address@hidden>
>> > Acked-by: Paolo Bonzini <address@hidden>
>> > Signed-off-by: Peter Crosthwaite <address@hidden>
>> > [AF: max_cpus -> MAX_CPUMASK_BITS]
>> > Signed-off-by: Andreas Färber <address@hidden>
>> > ---
>> >  exec.c            | 58 
>> > ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>> >  include/qom/cpu.h |  1 +
>> >  qom/cpu.c         |  7 +++++++
>> >  3 files changed, 61 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/exec.c b/exec.c
>> > index ce5fadd..d817e5f 100644
>> > --- a/exec.c
>> > +++ b/exec.c
>> > @@ -526,12 +526,57 @@ void tcg_cpu_address_space_init(CPUState *cpu, 
>> > AddressSpace *as)
>> >  }
>> >  #endif
>> >
>> > +#ifndef CONFIG_USER_ONLY
>> > +static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
>> > +
>> > +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;
>> > +    }
>>
>> If this routine and hence cpu_exec_init() (which is called from realize
>> routine) don't error out when max_cpus is reached, archs supporting CPU
>> hotplug using device_add will find it difficult to fail the realization of
>> CPU when hotplugging of more than max_cpus is attempted.
>>
>> An alternative is to explicitly check for the returned cpu_index
>> in realize call within each arch and fail if the cpu_index obtained
>> is greater than max_cpus. So for ppc, I could put such a check in
>> target-ppc/translate_init:ppc_cpu_realizefn(), but ppc_cpu_realizefn()
>> is a common routine for all targets under ppc and some targets like
>> ppc64abi32-linux-user don't have visibility to max_cpus which is
>> in vl.c.
>>
>> Any thoughts on the above problem ?
> we already have MachineClass.max_cpus which is max
> supported limit of machine type.
> Perhaps make max_cpus a property of MashineState

MachineClass.max_cpus is the maximum number of CPUs supported for the
machine. What we want to check here is against the max_cpus that the
guest has booted with.

So are you suggesting that we move vl.c:max_cpus to
MachineState.max_cpus and use that from all archs instead of using
vl.c:max_cpus directly ?

Regards,
Bharata.