[Qemu-devel] [PULL v3 03/22] cpu: Reorder cpu->as, cpu->thread_id, cpu->memory_dispatch init

[Qemu-devel] [PULL v3 03/22] cpu: Reorder cpu->as, cpu->thread_id, cpu->memory_dispatch init

[Andreas Färber]

From: Eduardo Habkost <ehabkost@redhat.com>

Instead of initializing cpu->as, cpu->thread_id, and reloading memory
map while holding cpu_list_lock(), do it earlier, before locking the CPU
list and initializing cpu_index.

This allows the code handling cpu_index and global CPU list to be
isolated from the rest.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 exec.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/exec.c b/exec.c
index e2caee9..442df0d 100644
--- a/exec.c
+++ b/exec.c
@@ -533,6 +533,12 @@ void cpu_exec_init(CPUArchState *env)
     CPUState *some_cpu;
     int cpu_index;
 
+#ifndef CONFIG_USER_ONLY
+    cpu->as = &address_space_memory;
+    cpu->thread_id = qemu_get_thread_id();
+    cpu_reload_memory_map(cpu);
+#endif
+
 #if defined(CONFIG_USER_ONLY)
     cpu_list_lock();
 #endif
@@ -541,11 +547,6 @@ void cpu_exec_init(CPUArchState *env)
         cpu_index++;
     }
     cpu->cpu_index = cpu_index;
-#ifndef CONFIG_USER_ONLY
-    cpu->as = &address_space_memory;
-    cpu->thread_id = qemu_get_thread_id();
-    cpu_reload_memory_map(cpu);
-#endif
     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
-- 
2.1.4

[Qemu-devel] [PULL v3 13/22] gdbstub: Use cpu_set_pc() helper

[Andreas Färber]

From: Peter Crosthwaite <crosthwaitepeter@gmail.com>

Use the cpu_set_pc() helper which will take care of CPUClass retrieval
for us.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 gdbstub.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index aa5ba51..92b2f81 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -754,12 +754,9 @@ static void gdb_breakpoint_remove_all(void)
 static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
 {
     CPUState *cpu = s->c_cpu;
-    CPUClass *cc = CPU_GET_CLASS(cpu);
 
     cpu_synchronize_state(cpu);
-    if (cc->set_pc) {
-        cc->set_pc(cpu, pc);
-    }
+    cpu_set_pc(cpu, pc);
 }
 
 static CPUState *find_cpu(uint32_t thread_id)
-- 
2.1.4

[Qemu-devel] [PULL v3 21/22] disas: cris: Fix 0 buffer length case

[Andreas Färber]

From: Peter Crosthwaite <crosthwaitepeter@gmail.com>

Cris has the complication of variable length instructions and has
a check in place to clamp memory reads in case the disas request
doesn't have enough bytes for the instruction being disas'd. This
breaks down in the case where disassembling for the monitor where
the buffer length is defaulted to 0.

The buffer length should never be zero for a regular target_disas,
so we can safely assume the 0 case is for the monitor in which case
consider the buffer length to be the max for cris instructions.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 disas/cris.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/disas/cris.c b/disas/cris.c
index e6cff7a..1b76a09 100644
--- a/disas/cris.c
+++ b/disas/cris.c
@@ -2575,9 +2575,9 @@ print_insn_cris_generic (bfd_vma memaddr,
      If we can't get any data, or we do not get enough data, we print
      the error message.  */
 
-  nbytes = info->buffer_length;
-  if (nbytes > MAX_BYTES_PER_CRIS_INSN)
-	  nbytes = MAX_BYTES_PER_CRIS_INSN;
+  nbytes = info->buffer_length ? info->buffer_length
+                               : MAX_BYTES_PER_CRIS_INSN;
+  nbytes = MIN(nbytes, MAX_BYTES_PER_CRIS_INSN);
   status = (*info->read_memory_func) (memaddr, buffer, nbytes, info);  
 
   /* If we did not get all we asked for, then clear the rest.
-- 
2.1.4

[Qemu-devel] [PULL v3 22/22] disas: cris: QOMify target specific disas setup

[Andreas Färber]

From: Peter Crosthwaite <crosthwaitepeter@gmail.com>

Move the target_disas() cris specifics to the QOM disas_set_info() hook
and delete the cris specific code in disas.c.

This also now adds support for monitor_disas() to cris.

E.g.
(qemu) xp 0x40004000
0000000040004000: 0x1e6f25f0

And before this patch:
(qemu) xp/i 0x40004000
0x40004000: Asm output not supported on this arch

After:
(qemu) xp/i 0x40004000
0x40004000:  di
(qemu) xp/i 0x40004002
0x40004002:  move.d 0xb003c004,$r1

Note: second example is 6-byte misaligned instruction!

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 disas.c           |  8 --------
 target-cris/cpu.c | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/disas.c b/disas.c
index 937e08b..69a6066 100644
--- a/disas.c
+++ b/disas.c
@@ -257,14 +257,6 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
 #elif defined(TARGET_ALPHA)
     s.info.mach = bfd_mach_alpha_ev6;
     s.info.print_insn = print_insn_alpha;
-#elif defined(TARGET_CRIS)
-    if (flags != 32) {
-        s.info.mach = bfd_mach_cris_v0_v10;
-        s.info.print_insn = print_insn_crisv10;
-    } else {
-        s.info.mach = bfd_mach_cris_v32;
-        s.info.print_insn = print_insn_crisv32;
-    }
 #elif defined(TARGET_S390X)
     s.info.mach = bfd_mach_s390_64;
     s.info.print_insn = print_insn_s390;
diff --git a/target-cris/cpu.c b/target-cris/cpu.c
index 0db209b..b17e849 100644
--- a/target-cris/cpu.c
+++ b/target-cris/cpu.c
@@ -161,6 +161,20 @@ static void cris_cpu_set_irq(void *opaque, int irq, int level)
 }
 #endif
 
+static void cris_disas_set_info(CPUState *cpu, disassemble_info *info)
+{
+    CRISCPU *cc = CRIS_CPU(cpu);
+    CPUCRISState *env = &cc->env;
+
+    if (env->pregs[PR_VR] != 32) {
+        info->mach = bfd_mach_cris_v0_v10;
+        info->print_insn = print_insn_crisv10;
+    } else {
+        info->mach = bfd_mach_cris_v32;
+        info->print_insn = print_insn_crisv32;
+    }
+}
+
 static void cris_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -292,6 +306,8 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data)
 
     cc->gdb_num_core_regs = 49;
     cc->gdb_stop_before_watchpoint = true;
+
+    cc->disas_set_info = cris_disas_set_info;
 }
 
 static const TypeInfo cris_cpu_type_info = {
-- 
2.1.4

Re: [Qemu-devel] [PULL v3 00/22] QOM CPUState patch queue 2015-07-09

[Peter Maydell]

On 9 July 2015 at 14:23, Andreas Färber <afaerber@suse.de> wrote:
> Hello Peter,
>
> This is my QOM CPU patch queue. Please pull.
>
> v3 fixes ppc and linux-user related bugs.
>
> Regards,
> Andreas
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> The following changes since commit acf7b7fdf31fa76b53803790917c8acf23a2badb:
>
>   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2015-07-08 20:46:35 +0100)
>
> are available in the git repository at:
>
>   git://github.com/afaerber/qemu-cpu.git tags/qom-cpu-for-peter
>
> for you to fetch changes up to 6b625fde5eb8d1c969969392f1c92b58beed2183:
>
>   disas: cris: QOMify target specific disas setup (2015-07-09 15:20:41 +0200)
>
> ----------------------------------------------------------------
> QOM CPUState and X86CPU
>
> * Further QOM'ification of CPU initialization
> * Propagation of CPUState arguments and elimination of ENV_GET_CPU() usage
> * cpu_set_pc() abstraction
> * CPUClass::disas_set_info() hook
>

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL v3 00/22] QOM CPUState patch queue 2015-07-09

[Peter Maydell]

On 9 July 2015 at 16:22, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 July 2015 at 14:23, Andreas Färber <afaerber@suse.de> wrote:
>> Hello Peter,
>>
>> This is my QOM CPU patch queue. Please pull.
>>
>> v3 fixes ppc and linux-user related bugs.
>>
>> Regards,
>> Andreas
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> The following changes since commit acf7b7fdf31fa76b53803790917c8acf23a2badb:
>>
>>   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2015-07-08 20:46:35 +0100)
>>
>> are available in the git repository at:
>>
>>   git://github.com/afaerber/qemu-cpu.git tags/qom-cpu-for-peter
>>
>> for you to fetch changes up to 6b625fde5eb8d1c969969392f1c92b58beed2183:
>>
>>   disas: cris: QOMify target specific disas setup (2015-07-09 15:20:41 +0200)
>>
>> ----------------------------------------------------------------
>> QOM CPUState and X86CPU
>>
>> * Further QOM'ification of CPU initialization
>> * Propagation of CPUState arguments and elimination of ENV_GET_CPU() usage
>> * cpu_set_pc() abstraction
>> * CPUClass::disas_set_info() hook
>>
>
> Applied, thanks.

...but it turns out to have broken compiling TCI:

  CC    x86_64-softmmu/arch_init.o
In file included from
/home/petmay01/linaro/qemu-for-merges/target-i386/cpu-qom.h:23:0,
                 from
/home/petmay01/linaro/qemu-for-merges/target-i386/cpu.h:986,
                 from
/home/petmay01/linaro/qemu-for-merges/include/qemu-common.h:122,
                 from
/home/petmay01/linaro/qemu-for-merges/include/disas/bfd.h:12,
                 from /home/petmay01/linaro/qemu-for-merges/disas/tci.c:20:
/home/petmay01/linaro/qemu-for-merges/include/qom/cpu.h:178:43: error:
unknown type name ‘disassemble_info’
     void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
                                           ^
/home/petmay01/linaro/qemu-for-merges/include/qom/cpu.h:179:1: error:
no semicolon at end of struct or union [-Werror]
 } CPUClass;
 ^
cc1: all warnings being treated as errors

Maybe I should put that config in my standard build test set...

-- PMM

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

[Bharata B Rao]

On Thu, Jul 09, 2015 at 03:23:55PM +0200, Andreas Färber wrote:
> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> 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 <bharata@linux.vnet.ibm.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> [AF: max_cpus -> MAX_CPUMASK_BITS]
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  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 ?

Also, is it possible to revisit the problem that use of max_cpus instead
of MAX_CPUMASK_BITS caused to xlnx-ep108 ?

Regards,
Bharata.

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

[Igor Mammedov]

On Tue, 14 Jul 2015 16:08:54 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Thu, Jul 09, 2015 at 03:23:55PM +0200, Andreas Färber wrote:
> > From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > 
> > 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 <bharata@linux.vnet.ibm.com>
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> > [AF: max_cpus -> MAX_CPUMASK_BITS]
> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > ---
> >  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

> 
> Also, is it possible to revisit the problem that use of max_cpus instead
> of MAX_CPUMASK_BITS caused to xlnx-ep108 ?
> 
> Regards,
> Bharata.
> 
> 

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 <imammedo@redhat.com> wrote:
> On Tue, 14 Jul 2015 16:08:54 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
>
>> On Thu, Jul 09, 2015 at 03:23:55PM +0200, Andreas Färber wrote:
>> > From: Bharata B Rao <bharata@linux.vnet.ibm.com>
>> >
>> > 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 <bharata@linux.vnet.ibm.com>
>> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> > Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>> > Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> > [AF: max_cpus -> MAX_CPUMASK_BITS]
>> > Signed-off-by: Andreas Färber <afaerber@suse.de>
>> > ---
>> >  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.

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

[Igor Mammedov]

On Wed, 15 Jul 2015 09:12:48 +0530
Bharata B Rao <bharata.rao@gmail.com> wrote:

> On Tue, Jul 14, 2015 at 5:17 PM, Igor Mammedov <imammedo@redhat.com>
> wrote:
> > On Tue, 14 Jul 2015 16:08:54 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >
> >> On Thu, Jul 09, 2015 at 03:23:55PM +0200, Andreas Färber wrote:
> >> > From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >> >
> >> > 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 <bharata@linux.vnet.ibm.com>
> >> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> >> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >> > Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> >> > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> >> > Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> >> > [AF: max_cpus -> MAX_CPUMASK_BITS]
> >> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> > ---
> >> >  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 ?
yep, along the way move related  cpus,threads,sockets,cores from vl.c to
MachineState

> 
> Regards,
> Bharata.