diff mbox

hw: add .min_cpus and .default_cpus fields to machine_class

Message ID 20171106201332.GA2152@flamenco (mailing list archive)
State New, archived
Headers show

Commit Message

Emilio Cota Nov. 6, 2017, 8:13 p.m. UTC 
On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote:
> IMO, initialization state doesn't belong to CPUClass.  We already
> have a single accelerator object in MachineState::accelerator,
> and tcg_initialized could be moved to a AccelState::initialized
> field.

I don't know how to cleanly get AccelState from a CPUClass pointer
(as I said I'm not familiar with object code / qom) -- suggestions
welcome! The best I could come up in the limited time I have for
this is to use a static bool, as shown below.


---8<---

Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global

55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24)
introduces a per-CPUClass bool that we check so that the target CPU
is initialized for TCG only once. This works well except when
we end up creating more than one CPUClass, in which case we end
up incorrectly initializing TCG more than once, i.e. once for
each CPUClass.

This can be replicated with:
  $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \
      -global driver=xlnx,,zynqmp,property=has_rpu,value=on
In this case the class name of the "RPUs" is prefixed by "cortex-r5-",
whereas the "regular" CPUs are prefixed by "cortex-a53-". This
results in two CPUClass instances being created.

Fix it by introducing a static variable, so that only the first
target CPU being initialized will initialize the target-dependent
part of TCG, regardless of CPUClass instances.

Fixes: 55c3ceef61fcf06fc98ddc752b7cce788ce7680b
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 exec.c            | 5 +++--
 include/qom/cpu.h | 1 -
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Alistair Francis Nov. 7, 2017, 12:43 a.m. UTC | #1 
On Mon, Nov 6, 2017 at 12:13 PM, Emilio G. Cota <cota@braap.org> wrote:
> On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote:
>> IMO, initialization state doesn't belong to CPUClass.  We already
>> have a single accelerator object in MachineState::accelerator,
>> and tcg_initialized could be moved to a AccelState::initialized
>> field.
>
> I don't know how to cleanly get AccelState from a CPUClass pointer
> (as I said I'm not familiar with object code / qom) -- suggestions
> welcome! The best I could come up in the limited time I have for
> this is to use a static bool, as shown below.
>
>
> ---8<---
>
> Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global
>
> 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24)
> introduces a per-CPUClass bool that we check so that the target CPU
> is initialized for TCG only once. This works well except when
> we end up creating more than one CPUClass, in which case we end
> up incorrectly initializing TCG more than once, i.e. once for
> each CPUClass.
>
> This can be replicated with:
>   $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \
>       -global driver=xlnx,,zynqmp,property=has_rpu,value=on
> In this case the class name of the "RPUs" is prefixed by "cortex-r5-",
> whereas the "regular" CPUs are prefixed by "cortex-a53-". This
> results in two CPUClass instances being created.
>
> Fix it by introducing a static variable, so that only the first
> target CPU being initialized will initialize the target-dependent
> part of TCG, regardless of CPUClass instances.
>
> Fixes: 55c3ceef61fcf06fc98ddc752b7cce788ce7680b
> Signed-off-by: Emilio G. Cota <cota@braap.org>

This works great for me, is this ok to be applied?

Thanks,
Alistair

> ---
>  exec.c            | 5 +++--
>  include/qom/cpu.h | 1 -
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 97a24a8..8b579c0 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -792,11 +792,12 @@ void cpu_exec_initfn(CPUState *cpu)
>  void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> +    static bool tcg_target_initialized;
>
>      cpu_list_add(cpu);
>
> -    if (tcg_enabled() && !cc->tcg_initialized) {
> -        cc->tcg_initialized = true;
> +    if (tcg_enabled() && !tcg_target_initialized) {
> +        tcg_target_initialized = true;
>          cc->tcg_initialize();
>      }
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index fa4b0c9..c2fa151 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -209,7 +209,6 @@ typedef struct CPUClass {
>      /* Keep non-pointer data at the end to minimize holes.  */
>      int gdb_num_core_regs;
>      bool gdb_stop_before_watchpoint;
> -    bool tcg_initialized;
>  } CPUClass;
>
>  #ifdef HOST_WORDS_BIGENDIAN
> --
> 2.7.4
>
>
Eduardo Habkost Nov. 7, 2017, 12:31 p.m. UTC | #2 
On Mon, Nov 06, 2017 at 04:43:34PM -0800, Alistair Francis wrote:
> On Mon, Nov 6, 2017 at 12:13 PM, Emilio G. Cota <cota@braap.org> wrote:
> > On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote:
> >> IMO, initialization state doesn't belong to CPUClass.  We already
> >> have a single accelerator object in MachineState::accelerator,
> >> and tcg_initialized could be moved to a AccelState::initialized
> >> field.
> >
> > I don't know how to cleanly get AccelState from a CPUClass pointer
> > (as I said I'm not familiar with object code / qom) -- suggestions
> > welcome! The best I could come up in the limited time I have for
> > this is to use a static bool, as shown below.

I don't believe a TYPE_ACCEL object is created in *-user.  We can
consider changing that, but not during 2.11 freeze.

> >
> >
> > ---8<---
> >
> > Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global
> >
> > 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24)
> > introduces a per-CPUClass bool that we check so that the target CPU
> > is initialized for TCG only once. This works well except when
> > we end up creating more than one CPUClass, in which case we end
> > up incorrectly initializing TCG more than once, i.e. once for
> > each CPUClass.
> >
> > This can be replicated with:
> >   $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \
> >       -global driver=xlnx,,zynqmp,property=has_rpu,value=on
> > In this case the class name of the "RPUs" is prefixed by "cortex-r5-",
> > whereas the "regular" CPUs are prefixed by "cortex-a53-". This
> > results in two CPUClass instances being created.
> >
> > Fix it by introducing a static variable, so that only the first
> > target CPU being initialized will initialize the target-dependent
> > part of TCG, regardless of CPUClass instances.
> >
> > Fixes: 55c3ceef61fcf06fc98ddc752b7cce788ce7680b
> > Signed-off-by: Emilio G. Cota <cota@braap.org>
> 
> This works great for me, is this ok to be applied?

It depends if we are already relying on initialization of CPUs of
different types to be calling two different ->tcg_initialize()
functions.  I think that's the case today, so this looks
reasonable as a quick fix for 2.11.

I would wait for an Acked-by from Richard Henderson before
applying it, though.  Richard, what do you think?

> 
> Thanks,
> Alistair
> 
> > ---
> >  exec.c            | 5 +++--
> >  include/qom/cpu.h | 1 -
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index 97a24a8..8b579c0 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -792,11 +792,12 @@ void cpu_exec_initfn(CPUState *cpu)
> >  void cpu_exec_realizefn(CPUState *cpu, Error **errp)
> >  {
> >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > +    static bool tcg_target_initialized;
> >
> >      cpu_list_add(cpu);
> >
> > -    if (tcg_enabled() && !cc->tcg_initialized) {
> > -        cc->tcg_initialized = true;
> > +    if (tcg_enabled() && !tcg_target_initialized) {
> > +        tcg_target_initialized = true;
> >          cc->tcg_initialize();
> >      }
> >
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index fa4b0c9..c2fa151 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -209,7 +209,6 @@ typedef struct CPUClass {
> >      /* Keep non-pointer data at the end to minimize holes.  */
> >      int gdb_num_core_regs;
> >      bool gdb_stop_before_watchpoint;
> > -    bool tcg_initialized;
> >  } CPUClass;
> >
> >  #ifdef HOST_WORDS_BIGENDIAN
> > --
> > 2.7.4
> >
> >
Richard Henderson Nov. 8, 2017, 9:29 p.m. UTC | #3 
On 11/06/2017 09:13 PM, Emilio G. Cota wrote:
> Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global
> 
> 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24)
> introduces a per-CPUClass bool that we check so that the target CPU
> is initialized for TCG only once. This works well except when
> we end up creating more than one CPUClass, in which case we end
> up incorrectly initializing TCG more than once, i.e. once for
> each CPUClass.
> 
> This can be replicated with:
>   $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \
>       -global driver=xlnx,,zynqmp,property=has_rpu,value=on
> In this case the class name of the "RPUs" is prefixed by "cortex-r5-",
> whereas the "regular" CPUs are prefixed by "cortex-a53-". This
> results in two CPUClass instances being created.
> 
> Fix it by introducing a static variable, so that only the first
> target CPU being initialized will initialize the target-dependent
> part of TCG, regardless of CPUClass instances.

Hah!

So, I had been thinking of the xylinx ARM + Microblaze case, where we really do
need two different initializations.  I never imagined that two different ARM
parts had different CPUClasses.

So I guess it's my initial patch that unified this that's more buggy than not.


r~
Eduardo Habkost Nov. 8, 2017, 9:52 p.m. UTC | #4 
On Wed, Nov 08, 2017 at 10:29:43PM +0100, Richard Henderson wrote:
> On 11/06/2017 09:13 PM, Emilio G. Cota wrote:
> > Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global
> > 
> > 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24)
> > introduces a per-CPUClass bool that we check so that the target CPU
> > is initialized for TCG only once. This works well except when
> > we end up creating more than one CPUClass, in which case we end
> > up incorrectly initializing TCG more than once, i.e. once for
> > each CPUClass.
> > 
> > This can be replicated with:
> >   $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \
> >       -global driver=xlnx,,zynqmp,property=has_rpu,value=on
> > In this case the class name of the "RPUs" is prefixed by "cortex-r5-",
> > whereas the "regular" CPUs are prefixed by "cortex-a53-". This
> > results in two CPUClass instances being created.
> > 
> > Fix it by introducing a static variable, so that only the first
> > target CPU being initialized will initialize the target-dependent
> > part of TCG, regardless of CPUClass instances.
> 
> Hah!
> 
> So, I had been thinking of the xylinx ARM + Microblaze case, where we really do
> need two different initializations.  I never imagined that two different ARM
> parts had different CPUClasses.

Is xylinx ARM + Microblaze something that already works (and
would be broken by this patch), or something planned for the
future?

> 
> So I guess it's my initial patch that unified this that's more buggy than not.

We still have the option of reverting the original patch, but (if
it doesn't break anything) this patch looks like a simpler fix
for 2.11 than a full revert.
Alistair Francis Nov. 8, 2017, 10:08 p.m. UTC | #5 
On Wed, Nov 8, 2017 at 1:52 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Wed, Nov 08, 2017 at 10:29:43PM +0100, Richard Henderson wrote:
>> On 11/06/2017 09:13 PM, Emilio G. Cota wrote:
>> > Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global
>> >
>> > 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24)
>> > introduces a per-CPUClass bool that we check so that the target CPU
>> > is initialized for TCG only once. This works well except when
>> > we end up creating more than one CPUClass, in which case we end
>> > up incorrectly initializing TCG more than once, i.e. once for
>> > each CPUClass.
>> >
>> > This can be replicated with:
>> >   $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \
>> >       -global driver=xlnx,,zynqmp,property=has_rpu,value=on
>> > In this case the class name of the "RPUs" is prefixed by "cortex-r5-",
>> > whereas the "regular" CPUs are prefixed by "cortex-a53-". This
>> > results in two CPUClass instances being created.
>> >
>> > Fix it by introducing a static variable, so that only the first
>> > target CPU being initialized will initialize the target-dependent
>> > part of TCG, regardless of CPUClass instances.
>>
>> Hah!
>>
>> So, I had been thinking of the xylinx ARM + Microblaze case, where we really do
>> need two different initializations.  I never imagined that two different ARM
>> parts had different CPUClasses.
>
> Is xylinx ARM + Microblaze something that already works (and
> would be broken by this patch), or something planned for the
> future?

Something planned for the future, it has never worked.

Alistair

>
>>
>> So I guess it's my initial patch that unified this that's more buggy than not.
>
> We still have the option of reverting the original patch, but (if
> it doesn't break anything) this patch looks like a simpler fix
> for 2.11 than a full revert.
>
> --
> Eduardo
>
diff mbox

Patch

diff --git a/exec.c b/exec.c
index 97a24a8..8b579c0 100644
--- a/exec.c
+++ b/exec.c
@@ -792,11 +792,12 @@  void cpu_exec_initfn(CPUState *cpu)
 void cpu_exec_realizefn(CPUState *cpu, Error **errp)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
+    static bool tcg_target_initialized;
 
     cpu_list_add(cpu);
 
-    if (tcg_enabled() && !cc->tcg_initialized) {
-        cc->tcg_initialized = true;
+    if (tcg_enabled() && !tcg_target_initialized) {
+        tcg_target_initialized = true;
         cc->tcg_initialize();
     }
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index fa4b0c9..c2fa151 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -209,7 +209,6 @@  typedef struct CPUClass {
     /* Keep non-pointer data at the end to minimize holes.  */
     int gdb_num_core_regs;
     bool gdb_stop_before_watchpoint;
-    bool tcg_initialized;
 } CPUClass;
 
 #ifdef HOST_WORDS_BIGENDIAN