[PATCH 0/2] cpu/core: Fix "help" of CPU core device types

[PATCH 0/2] cpu/core: Fix "help" of CPU core device types

[Greg Kurz]

Thomas Huth recently reported an annoying crash:

$ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
/home/thuth/devel/qemu/include/hw/boards.h:24:
 MACHINE: Object 0x5635bd53af10 is not an instance of type machine
Aborted (core dumped)

This is caused by an early use of qdev_get_machine(), before the
machine creation, which triggers a side-effect of creating a
dummy "container" object instead of the machine. This is needed
by user mode emulation, which doesn't really care about the
type of the parent of the CPU model. This is toxic for system
mode though because the system mode specific code usually assume
MACHINE(qdev_get_machine()).

This series brings separate implementations between user and
system mode. The breakage with "cpu-code,help" is fixed by using
current_machine.

Greg Kurz (2):
  qdev: Separate implementations of qdev_get_machine() for user and
    system
  cpu/core: Fix "help" of CPU core device types

 hw/core/machine.c        | 14 ++++++++++++++
 hw/core/qdev.c           |  2 +-
 hw/cpu/core.c            | 10 ++++++++--
 include/hw/qdev-core.h   |  1 +
 stubs/meson.build        |  1 +
 stubs/qdev-get-machine.c | 11 +++++++++++
 6 files changed, 36 insertions(+), 3 deletions(-)
 create mode 100644 stubs/qdev-get-machine.c

-- 
2.26.3

[PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Greg Kurz]

Despite its simple name and common usage of "getting a pointer to
the machine" in system-mode emulation, qdev_get_machine() has some
subtilities.

First, it can be called when running user-mode emulation : this is
because user-mode partly relies on qdev to instantiate its CPU
model.

Second, but not least, it has a side-effect : if it cannot find an
object at "/machine" in the QOM tree, it creates a dummy "container"
object and put it there. A simple check on the type returned by
qdev_get_machine() allows user-mode to run the common qdev code,
skipping the parts that only make sense for system-mode.

This side-effect turns out to complicate the use of qdev_get_machine()
for the system-mode case though. Most notably, qdev_get_machine() must
not be called before the machine object is added to the QOM tree by
qemu_create_machine(), otherwise the existing dummy "container" object
would cause qemu_create_machine() to fail with something like :

Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
qemu-system-ppc64: attempt to add duplicate property 'machine' to
 object (type 'container')
Aborted (core dumped)

This situation doesn't exist in the current code base, mostly because
of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
and e2fb3fbbf9c for details).

A new kind of breakage was spotted very recently though :

$ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
/home/thuth/devel/qemu/include/hw/boards.h:24:
 MACHINE: Object 0x5635bd53af10 is not an instance of type machine
Aborted (core dumped)

This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
added a new condition for qdev_get_machine() to be called too early,
breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
time.

In order to avoid further subtle breakages like this, change the
implentation of qdev_get_machine() to:
- keep the existing behaviour of creating the dummy "container"
  object for the user-mode case only ;
- abort() if the machine doesn't exist yet in the QOM tree for
  the system-mode case. This gives a precise hint to developpers
  that calling qdev_get_machine() too early is a programming bug.

This is achieved with a new do_qdev_get_machine() function called
from qdev_get_machine(), with different implementations for system
and user mode.

$ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
qemu-system-ppc64: ../../hw/core/machine.c:1290:
 qdev_get_machine: Assertion `machine != NULL' failed.
Aborted (core dumped)

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/core/machine.c        | 14 ++++++++++++++
 hw/core/qdev.c           |  2 +-
 include/hw/qdev-core.h   |  1 +
 stubs/meson.build        |  1 +
 stubs/qdev-get-machine.c | 11 +++++++++++
 5 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 stubs/qdev-get-machine.c

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 40def78183a7..fecca4023105 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1293,6 +1293,20 @@ void qdev_machine_creation_done(void)
     register_global_state();
 }
 
+Object *do_qdev_get_machine(void)
+{
+    Object *machine;
+
+    machine = object_resolve_path_component(object_get_root(), "machine");
+    /*
+     * qdev_get_machine() shouldn't be called before qemu_create_machine()
+     * has created the "/machine" path.
+     */
+    assert(machine != NULL);
+
+    return machine;
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a92..1122721b2bf0 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1131,7 +1131,7 @@ Object *qdev_get_machine(void)
     static Object *dev;
 
     if (dev == NULL) {
-        dev = container_get(object_get_root(), "/machine");
+        dev = do_qdev_get_machine();
     }
 
     return dev;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bafc311bfa1b..90e295e0bc1a 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -782,6 +782,7 @@ const char *qdev_fw_name(DeviceState *dev);
 
 void qdev_assert_realized_properly(void);
 Object *qdev_get_machine(void);
+Object *do_qdev_get_machine(void);
 
 /* FIXME: make this a link<> */
 bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
diff --git a/stubs/meson.build b/stubs/meson.build
index be6f6d609e58..b99ee2b33e94 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -54,3 +54,4 @@ if have_system
 else
   stub_ss.add(files('qdev.c'))
 endif
+stub_ss.add(files('qdev-get-machine.c'))
diff --git a/stubs/qdev-get-machine.c b/stubs/qdev-get-machine.c
new file mode 100644
index 000000000000..ed4cdaa01900
--- /dev/null
+++ b/stubs/qdev-get-machine.c
@@ -0,0 +1,11 @@
+#include "qemu/osdep.h"
+#include "hw/qdev-core.h"
+
+Object *do_qdev_get_machine(void)
+{
+    /*
+     * This will create a "container" and add it to the QOM tree, if there
+     * isn't one already.
+     */
+    return container_get(object_get_root(), "/machine");
+}
-- 
2.26.3

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Eduardo Habkost]

On Fri, Apr 09, 2021 at 06:03:38PM +0200, Greg Kurz wrote:
[...]
> In order to avoid further subtle breakages like this, change the
> implentation of qdev_get_machine() to:
> - keep the existing behaviour of creating the dummy "container"
>   object for the user-mode case only ;
> - abort() if the machine doesn't exist yet in the QOM tree for
>   the system-mode case. This gives a precise hint to developpers
>   that calling qdev_get_machine() too early is a programming bug.
> 
> This is achieved with a new do_qdev_get_machine() function called
> from qdev_get_machine(), with different implementations for system
> and user mode.
> 
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>  qdev_get_machine: Assertion `machine != NULL' failed.
> Aborted (core dumped)

Should this to be considered for 6.0?  It doesn't seem to be a
bug fix, but just a preventive measure.

-- 
Eduardo

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Greg Kurz]

On Fri, 9 Apr 2021 16:14:41 -0400
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Apr 09, 2021 at 06:03:38PM +0200, Greg Kurz wrote:
> [...]
> > In order to avoid further subtle breakages like this, change the
> > implentation of qdev_get_machine() to:
> > - keep the existing behaviour of creating the dummy "container"
> >   object for the user-mode case only ;
> > - abort() if the machine doesn't exist yet in the QOM tree for
> >   the system-mode case. This gives a precise hint to developpers
> >   that calling qdev_get_machine() too early is a programming bug.
> > 
> > This is achieved with a new do_qdev_get_machine() function called
> > from qdev_get_machine(), with different implementations for system
> > and user mode.
> > 
> > $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> > qemu-system-ppc64: ../../hw/core/machine.c:1290:
> >  qdev_get_machine: Assertion `machine != NULL' failed.
> > Aborted (core dumped)
> 
> Should this to be considered for 6.0?  It doesn't seem to be a
> bug fix, but just a preventive measure.
> 

Correct. I forgot to add a for-6.1 prefix but it is definitely too
late for 6.0.

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Thomas Huth]

On 09/04/2021 18.03, Greg Kurz wrote:
> Despite its simple name and common usage of "getting a pointer to
> the machine" in system-mode emulation, qdev_get_machine() has some
> subtilities.
> 
> First, it can be called when running user-mode emulation : this is
> because user-mode partly relies on qdev to instantiate its CPU
> model.
> 
> Second, but not least, it has a side-effect : if it cannot find an
> object at "/machine" in the QOM tree, it creates a dummy "container"
> object and put it there. A simple check on the type returned by
> qdev_get_machine() allows user-mode to run the common qdev code,
> skipping the parts that only make sense for system-mode.
> 
> This side-effect turns out to complicate the use of qdev_get_machine()
> for the system-mode case though. Most notably, qdev_get_machine() must
> not be called before the machine object is added to the QOM tree by
> qemu_create_machine(), otherwise the existing dummy "container" object
> would cause qemu_create_machine() to fail with something like :
> 
> Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
> qemu-system-ppc64: attempt to add duplicate property 'machine' to
>   object (type 'container')
> Aborted (core dumped)
> 
> This situation doesn't exist in the current code base, mostly because
> of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
> and e2fb3fbbf9c for details).
> 
> A new kind of breakage was spotted very recently though :
> 
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> /home/thuth/devel/qemu/include/hw/boards.h:24:
>   MACHINE: Object 0x5635bd53af10 is not an instance of type machine
> Aborted (core dumped)
> 
> This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
> added a new condition for qdev_get_machine() to be called too early,
> breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
> time.
> 
> In order to avoid further subtle breakages like this, change the
> implentation of qdev_get_machine() to:
> - keep the existing behaviour of creating the dummy "container"
>    object for the user-mode case only ;
> - abort() if the machine doesn't exist yet in the QOM tree for
>    the system-mode case. This gives a precise hint to developpers
>    that calling qdev_get_machine() too early is a programming bug.
> 
> This is achieved with a new do_qdev_get_machine() function called
> from qdev_get_machine(), with different implementations for system
> and user mode.
> 
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>   qdev_get_machine: Assertion `machine != NULL' failed.
> Aborted (core dumped)
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   hw/core/machine.c        | 14 ++++++++++++++
>   hw/core/qdev.c           |  2 +-
>   include/hw/qdev-core.h   |  1 +
>   stubs/meson.build        |  1 +
>   stubs/qdev-get-machine.c | 11 +++++++++++
>   5 files changed, 28 insertions(+), 1 deletion(-)
>   create mode 100644 stubs/qdev-get-machine.c
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 40def78183a7..fecca4023105 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1293,6 +1293,20 @@ void qdev_machine_creation_done(void)
>       register_global_state();
>   }
>   
> +Object *do_qdev_get_machine(void)
> +{
> +    Object *machine;
> +
> +    machine = object_resolve_path_component(object_get_root(), "machine");
> +    /*
> +     * qdev_get_machine() shouldn't be called before qemu_create_machine()
> +     * has created the "/machine" path.
> +     */
> +    assert(machine != NULL);
> +
> +    return machine;
> +}
> +
>   static const TypeInfo machine_info = {
>       .name = TYPE_MACHINE,
>       .parent = TYPE_OBJECT,
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index cefc5eaa0a92..1122721b2bf0 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1131,7 +1131,7 @@ Object *qdev_get_machine(void)
>       static Object *dev;
>   
>       if (dev == NULL) {
> -        dev = container_get(object_get_root(), "/machine");
> +        dev = do_qdev_get_machine();
>       }
>   
>       return dev;
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index bafc311bfa1b..90e295e0bc1a 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -782,6 +782,7 @@ const char *qdev_fw_name(DeviceState *dev);
>   
>   void qdev_assert_realized_properly(void);
>   Object *qdev_get_machine(void);
> +Object *do_qdev_get_machine(void);
>   
>   /* FIXME: make this a link<> */
>   bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
> diff --git a/stubs/meson.build b/stubs/meson.build
> index be6f6d609e58..b99ee2b33e94 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -54,3 +54,4 @@ if have_system
>   else
>     stub_ss.add(files('qdev.c'))
>   endif
> +stub_ss.add(files('qdev-get-machine.c'))
> diff --git a/stubs/qdev-get-machine.c b/stubs/qdev-get-machine.c
> new file mode 100644
> index 000000000000..ed4cdaa01900
> --- /dev/null
> +++ b/stubs/qdev-get-machine.c
> @@ -0,0 +1,11 @@
> +#include "qemu/osdep.h"
> +#include "hw/qdev-core.h"
> +
> +Object *do_qdev_get_machine(void)
> +{
> +    /*
> +     * This will create a "container" and add it to the QOM tree, if there
> +     * isn't one already.
> +     */
> +    return container_get(object_get_root(), "/machine");
> +}
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

... but I think I agree with Eduardo, we should likely add this only after 
6.0 has been released.

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> Despite its simple name and common usage of "getting a pointer to
> the machine" in system-mode emulation, qdev_get_machine() has some
> subtilities.
>
> First, it can be called when running user-mode emulation : this is
> because user-mode partly relies on qdev to instantiate its CPU
> model.
>
> Second, but not least, it has a side-effect : if it cannot find an
> object at "/machine" in the QOM tree, it creates a dummy "container"
> object and put it there. A simple check on the type returned by
> qdev_get_machine() allows user-mode to run the common qdev code,
> skipping the parts that only make sense for system-mode.
>
> This side-effect turns out to complicate the use of qdev_get_machine()
> for the system-mode case though. Most notably, qdev_get_machine() must
> not be called before the machine object is added to the QOM tree by
> qemu_create_machine(), otherwise the existing dummy "container" object
> would cause qemu_create_machine() to fail with something like :

Stupid trap.

> Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
> qemu-system-ppc64: attempt to add duplicate property 'machine' to
>  object (type 'container')
> Aborted (core dumped)
>
> This situation doesn't exist in the current code base, mostly because
> of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
> and e2fb3fbbf9c for details).

I lacked the stamina to address the root problem: automatic creation of
dummy containers where real ones may be needed.

Is /machine the only such container?  Have you reviewed the other uses
of container_get()?

> A new kind of breakage was spotted very recently though :
>
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> /home/thuth/devel/qemu/include/hw/boards.h:24:
>  MACHINE: Object 0x5635bd53af10 is not an instance of type machine
> Aborted (core dumped)
>
> This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
> added a new condition for qdev_get_machine() to be called too early,
> breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
> time.
>
> In order to avoid further subtle breakages like this, change the
> implentation of qdev_get_machine() to:
> - keep the existing behaviour of creating the dummy "container"
>   object for the user-mode case only ;
> - abort() if the machine doesn't exist yet in the QOM tree for
>   the system-mode case. This gives a precise hint to developpers
>   that calling qdev_get_machine() too early is a programming bug.

In other words, we fail right away instead of planting a landmine for
later.  Good.

The alternative would be mandating "must create /machine before first
use" for all programs, not just qemu-system-FOO, but that might be more
invasive.  Not sure.

> This is achieved with a new do_qdev_get_machine() function called

container_get() is a suboptimal name for a function that creates
containers, qdev_get_machine() is a suboptimal name for a function that
creates /machine, and so is do_qdev_get_machine().  Observation, not
demand.

> from qdev_get_machine(), with different implementations for system
> and user mode.
>
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>  qdev_get_machine: Assertion `machine != NULL' failed.
> Aborted (core dumped)
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Greg Kurz]

On Sat, 10 Apr 2021 10:59:25 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Greg Kurz <groug@kaod.org> writes:
> 
> > Despite its simple name and common usage of "getting a pointer to
> > the machine" in system-mode emulation, qdev_get_machine() has some
> > subtilities.
> >
> > First, it can be called when running user-mode emulation : this is
> > because user-mode partly relies on qdev to instantiate its CPU
> > model.
> >
> > Second, but not least, it has a side-effect : if it cannot find an
> > object at "/machine" in the QOM tree, it creates a dummy "container"
> > object and put it there. A simple check on the type returned by
> > qdev_get_machine() allows user-mode to run the common qdev code,
> > skipping the parts that only make sense for system-mode.
> >
> > This side-effect turns out to complicate the use of qdev_get_machine()
> > for the system-mode case though. Most notably, qdev_get_machine() must
> > not be called before the machine object is added to the QOM tree by
> > qemu_create_machine(), otherwise the existing dummy "container" object
> > would cause qemu_create_machine() to fail with something like :
> 
> Stupid trap.
> 

Still armed and ready for subtle bugs.

> > Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
> > qemu-system-ppc64: attempt to add duplicate property 'machine' to
> >  object (type 'container')
> > Aborted (core dumped)
> >
> > This situation doesn't exist in the current code base, mostly because
> > of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
> > and e2fb3fbbf9c for details).
> 
> I lacked the stamina to address the root problem: automatic creation of
> dummy containers where real ones may be needed.
> 
> Is /machine the only such container?  Have you reviewed the other uses
> of container_get()?
> 

No. I've only looked at the /machine case.

> > A new kind of breakage was spotted very recently though :
> >
> > $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> > /home/thuth/devel/qemu/include/hw/boards.h:24:
> >  MACHINE: Object 0x5635bd53af10 is not an instance of type machine
> > Aborted (core dumped)
> >
> > This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
> > added a new condition for qdev_get_machine() to be called too early,
> > breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
> > time.
> >
> > In order to avoid further subtle breakages like this, change the
> > implentation of qdev_get_machine() to:
> > - keep the existing behaviour of creating the dummy "container"
> >   object for the user-mode case only ;
> > - abort() if the machine doesn't exist yet in the QOM tree for
> >   the system-mode case. This gives a precise hint to developpers
> >   that calling qdev_get_machine() too early is a programming bug.
> 
> In other words, we fail right away instead of planting a landmine for
> later.  Good.
> 
> The alternative would be mandating "must create /machine before first
> use" for all programs, not just qemu-system-FOO, but that might be more
> invasive.  Not sure.
> 

This would mean all user emulation binaries and a bunch of test
programs as well. I'll give a try in this direction.

> > This is achieved with a new do_qdev_get_machine() function called
> 
> container_get() is a suboptimal name for a function that creates
> containers, qdev_get_machine() is a suboptimal name for a function that
> creates /machine, and so is do_qdev_get_machine().  Observation, not
> demand.
> 

/**
 * container_get:
 * @root: root of the #path, e.g., object_get_root()
 * @path: path to the container
 *
 * Return a container object whose path is @path.  Create more containers
 * along the path if necessary.
 *
 * Returns: the container object.
 */
Object *container_get(Object *root, const char *path);

My understanding is that container_get()'s main mission is to
return a "container" object. The creation part looks like
a fallback to "fill the holes" in the QOM tree...

I'd rather try to get rid of that side-effect entirely rather
than coming up with a sensible name => auditing other users
of container_get() as you asked above seems to be the next
step :)

Thanks!

> > from qdev_get_machine(), with different implementations for system
> > and user mode.
> >
> > $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> > qemu-system-ppc64: ../../hw/core/machine.c:1290:
> >  qdev_get_machine: Assertion `machine != NULL' failed.
> > Aborted (core dumped)
> >
> > Reported-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Eduardo Habkost]

On Fri, Apr 09, 2021 at 06:03:38PM +0200, Greg Kurz wrote:
> Despite its simple name and common usage of "getting a pointer to
> the machine" in system-mode emulation, qdev_get_machine() has some
> subtilities.
> 
> First, it can be called when running user-mode emulation : this is
> because user-mode partly relies on qdev to instantiate its CPU
> model.

Do we know exactly which user mode emulation code needs to call
qdev_get_machine(), and why?

If there's no real MachineState object in user mode, what the
return value of qdev_get_machine() is used for?

> 
> Second, but not least, it has a side-effect : if it cannot find an
> object at "/machine" in the QOM tree, it creates a dummy "container"
> object and put it there. A simple check on the type returned by
> qdev_get_machine() allows user-mode to run the common qdev code,
> skipping the parts that only make sense for system-mode.
> 
> This side-effect turns out to complicate the use of qdev_get_machine()
> for the system-mode case though. Most notably, qdev_get_machine() must
> not be called before the machine object is added to the QOM tree by
> qemu_create_machine(), otherwise the existing dummy "container" object
> would cause qemu_create_machine() to fail with something like :
> 
> Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
> qemu-system-ppc64: attempt to add duplicate property 'machine' to
>  object (type 'container')
> Aborted (core dumped)
> 
> This situation doesn't exist in the current code base, mostly because
> of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
> and e2fb3fbbf9c for details).
> 
> A new kind of breakage was spotted very recently though :
> 
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> /home/thuth/devel/qemu/include/hw/boards.h:24:
>  MACHINE: Object 0x5635bd53af10 is not an instance of type machine
> Aborted (core dumped)
> 
> This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
> added a new condition for qdev_get_machine() to be called too early,
> breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
> time.
> 
> In order to avoid further subtle breakages like this, change the
> implentation of qdev_get_machine() to:
> - keep the existing behaviour of creating the dummy "container"
>   object for the user-mode case only ;
> - abort() if the machine doesn't exist yet in the QOM tree for
>   the system-mode case. This gives a precise hint to developpers
>   that calling qdev_get_machine() too early is a programming bug.
> 
> This is achieved with a new do_qdev_get_machine() function called
> from qdev_get_machine(), with different implementations for system
> and user mode.
> 
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>  qdev_get_machine: Assertion `machine != NULL' failed.
> Aborted (core dumped)
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/core/machine.c        | 14 ++++++++++++++
>  hw/core/qdev.c           |  2 +-
>  include/hw/qdev-core.h   |  1 +
>  stubs/meson.build        |  1 +
>  stubs/qdev-get-machine.c | 11 +++++++++++
>  5 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100644 stubs/qdev-get-machine.c
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 40def78183a7..fecca4023105 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1293,6 +1293,20 @@ void qdev_machine_creation_done(void)
>      register_global_state();
>  }
>  
> +Object *do_qdev_get_machine(void)
> +{
> +    Object *machine;
> +
> +    machine = object_resolve_path_component(object_get_root(), "machine");

What about just replacing this expression with `current_machine`?

> +    /*
> +     * qdev_get_machine() shouldn't be called before qemu_create_machine()
> +     * has created the "/machine" path.
> +     */
> +    assert(machine != NULL);
> +
> +    return machine;

> +}
> +
>  static const TypeInfo machine_info = {
>      .name = TYPE_MACHINE,
>      .parent = TYPE_OBJECT,
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index cefc5eaa0a92..1122721b2bf0 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1131,7 +1131,7 @@ Object *qdev_get_machine(void)
>      static Object *dev;

It's interesting how this variable simply duplicates the purpose
of `current_machine`.

>  
>      if (dev == NULL) {
> -        dev = container_get(object_get_root(), "/machine");
> +        dev = do_qdev_get_machine();
>      }
>  
>      return dev;
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index bafc311bfa1b..90e295e0bc1a 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -782,6 +782,7 @@ const char *qdev_fw_name(DeviceState *dev);
>  
>  void qdev_assert_realized_properly(void);
>  Object *qdev_get_machine(void);
> +Object *do_qdev_get_machine(void);
>  
>  /* FIXME: make this a link<> */
>  bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
> diff --git a/stubs/meson.build b/stubs/meson.build
> index be6f6d609e58..b99ee2b33e94 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -54,3 +54,4 @@ if have_system
>  else
>    stub_ss.add(files('qdev.c'))
>  endif
> +stub_ss.add(files('qdev-get-machine.c'))
> diff --git a/stubs/qdev-get-machine.c b/stubs/qdev-get-machine.c
> new file mode 100644
> index 000000000000..ed4cdaa01900
> --- /dev/null
> +++ b/stubs/qdev-get-machine.c
> @@ -0,0 +1,11 @@
> +#include "qemu/osdep.h"
> +#include "hw/qdev-core.h"
> +
> +Object *do_qdev_get_machine(void)
> +{
> +    /*
> +     * This will create a "container" and add it to the QOM tree, if there
> +     * isn't one already.
> +     */
> +    return container_get(object_get_root(), "/machine");
> +}

I'm curious to understand when exactly this stub is useful.

-- 
Eduardo

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Greg Kurz]

On Tue, 13 Apr 2021 18:25:42 -0400
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Apr 09, 2021 at 06:03:38PM +0200, Greg Kurz wrote:
> > Despite its simple name and common usage of "getting a pointer to
> > the machine" in system-mode emulation, qdev_get_machine() has some
> > subtilities.
> > 
> > First, it can be called when running user-mode emulation : this is
> > because user-mode partly relies on qdev to instantiate its CPU
> > model.
> 
> Do we know exactly which user mode emulation code needs to call
> qdev_get_machine(), and why?
> 

User mode emulation uses qdev for the CPU model, thus indirectly calling
qdev_get_machine() in several paths during startup:

#0  qdev_get_machine () at ../../hw/core/qdev.c:1133
#1  0x0000000100209ddc in device_set_realized (obj=0x100485040, value=<optimized out>, errp=0x7fffffffd640) at ../../hw/core/qdev.c:745
#2  0x00000001001faa1c in property_set_bool (obj=0x100485040, v=<optimized out>, name=<optimized out>, opaque=0x100424410, errp=0x7fffffffd640) at ../../qom/object.c:2257
#3  0x00000001001fe85c in object_property_set (obj=0x100485040, name=0x1002be788 "realized", v=0x10049a430, errp=0x7fffffffd750) at ../../qom/object.c:1402
#4  0x00000001001f9f6c in object_property_set_qobject (obj=0x100485040, name=0x1002be788 "realized", value=<optimized out>, errp=0x7fffffffd750) at ../../qom/qom-qobject.c:28
#5  0x00000001001febe0 in object_property_set_bool (obj=0x100485040, name=0x1002be788 "realized", value=<optimized out>, errp=0x7fffffffd750) at ../../qom/object.c:1472
#6  0x00000001002079ac in qdev_realize (dev=0x100485040, bus=<optimized out>, errp=0x7fffffffd750) at ../../hw/core/qdev.c:389
#7  0x0000000100076db8 in cpu_create (typename=<optimized out>) at /home/greg/Work/qemu/qemu-master/include/hw/qdev-core.h:17
#8  0x0000000100072a04 in main (argc=<optimized out>, argv=0x7fffffffe318, envp=<optimized out>) at ../../linux-user/main.c:736


#0  qdev_get_machine () at ../../hw/core/qdev.c:1133
#1  0x0000000100209220 in qdev_get_machine_hotplug_handler (dev=<optimized out>) at ../../hw/core/qdev.c:241
#2  0x0000000100209340 in qdev_get_hotplug_handler (dev=<optimized out>) at ../../hw/core/qdev.c:281
#3  0x0000000100209a1c in device_set_realized (obj=0x100485040, value=<optimized out>, errp=0x7fffffffd640) at ../../hw/core/qdev.c:752
#4  0x00000001001faa1c in property_set_bool (obj=0x100485040, v=<optimized out>, name=<optimized out>, opaque=0x100424410, errp=0x7fffffffd640) at ../../qom/object.c:2257
#5  0x00000001001fe85c in object_property_set (obj=0x100485040, name=0x1002be788 "realized", v=0x10049a430, errp=0x7fffffffd750) at ../../qom/object.c:1402
#6  0x00000001001f9f6c in object_property_set_qobject (obj=0x100485040, name=0x1002be788 "realized", value=<optimized out>, errp=0x7fffffffd750) at ../../qom/qom-qobject.c:28
#7  0x00000001001febe0 in object_property_set_bool (obj=0x100485040, name=0x1002be788 "realized", value=<optimized out>, errp=0x7fffffffd750) at ../../qom/object.c:1472
#8  0x00000001002079ac in qdev_realize (dev=0x100485040, bus=<optimized out>, errp=0x7fffffffd750) at ../../hw/core/qdev.c:389
#9  0x0000000100076db8 in cpu_create (typename=<optimized out>) at /home/greg/Work/qemu/qemu-master/include/hw/qdev-core.h:17
#10 0x0000000100072a04 in main (argc=<optimized out>, argv=0x7fffffffe318, envp=<optimized out>) at ../../linux-user/main.c:736


#0  qdev_get_machine () at ../../hw/core/qdev.c:1133
#1  0x0000000100076700 in cpu_common_realizefn (dev=<optimized out>, errp=<optimized out>) at ../../hw/core/cpu.c:311
#2  0x00000001000c7a2c in ppc_cpu_realize (dev=0x100485040, errp=0x7fffffffd530) at ../../target/ppc/translate_init.c.inc:10199
#3  0x0000000100209ae0 in device_set_realized (obj=0x100485040, value=<optimized out>, errp=0x7fffffffd640) at ../../hw/core/qdev.c:761
#4  0x00000001001faa1c in property_set_bool (obj=0x100485040, v=<optimized out>, name=<optimized out>, opaque=0x100424410, errp=0x7fffffffd640) at ../../qom/object.c:2257
#5  0x00000001001fe85c in object_property_set (obj=0x100485040, name=0x1002be788 "realized", v=0x10049a430, errp=0x7fffffffd750) at ../../qom/object.c:1402
#6  0x00000001001f9f6c in object_property_set_qobject (obj=0x100485040, name=0x1002be788 "realized", value=<optimized out>, errp=0x7fffffffd750) at ../../qom/qom-qobject.c:28
#7  0x00000001001febe0 in object_property_set_bool (obj=0x100485040, name=0x1002be788 "realized", value=<optimized out>, errp=0x7fffffffd750) at ../../qom/object.c:1472
#8  0x00000001002079ac in qdev_realize (dev=0x100485040, bus=<optimized out>, errp=0x7fffffffd750) at ../../hw/core/qdev.c:389
#9  0x0000000100076db8 in cpu_create (typename=<optimized out>) at /home/greg/Work/qemu/qemu-master/include/hw/qdev-core.h:17
#10 0x0000000100072a04 in main (argc=<optimized out>, argv=0x7fffffffe318, envp=<optimized out>) at ../../linux-user/main.c:736

All these paths are also followed when the target spawns a new thread:

#0  qdev_get_machine () at ../../hw/core/qdev.c:1133
#1  0x0000000100209ddc in device_set_realized (obj=0x10051e3f0, value=<optimized out>, errp=0x7fffffffbc40) at ../../hw/core/qdev.c:745
#2  0x00000001001faa1c in property_set_bool (obj=0x10051e3f0, v=<optimized out>, name=<optimized out>, opaque=0x100424410, errp=0x7fffffffbc40) at ../../qom/object.c:2257
#3  0x00000001001fe85c in object_property_set (obj=0x10051e3f0, name=0x1002be788 "realized", v=0x1005317e0, errp=0x7fffffffbd50) at ../../qom/object.c:1402
#4  0x00000001001f9f6c in object_property_set_qobject (obj=0x10051e3f0, name=0x1002be788 "realized", value=<optimized out>, errp=0x7fffffffbd50) at ../../qom/qom-qobject.c:28
#5  0x00000001001febe0 in object_property_set_bool (obj=0x10051e3f0, name=0x1002be788 "realized", value=<optimized out>, errp=0x7fffffffbd50) at ../../qom/object.c:1472
#6  0x00000001002079ac in qdev_realize (dev=0x10051e3f0, bus=<optimized out>, errp=0x7fffffffbd50) at ../../hw/core/qdev.c:389
#7  0x0000000100076db8 in cpu_create (typename=<optimized out>) at /home/greg/Work/qemu/qemu-master/include/hw/qdev-core.h:17
#8  0x00000001001b57a4 in cpu_copy (env=0x10048d4f0) at ../../linux-user/main.c:206
#9  0x000000010018e844 in do_fork (env=0x10048d4f0, flags=flags@entry=4001536, newsp=newsp@entry=274897824272, parent_tidptr=parent_tidptr@entry=274897826368, newtls=newtls@entry=274897856736, 
    child_tidptr=child_tidptr@entry=274897826368) at ../../linux-user/syscall.c:6500
#10 0x000000010019e7e0 in do_syscall1 (cpu_env=cpu_env@entry=0x10048d4f0, num=num@entry=120, arg1=arg1@entry=4001536, arg2=arg2@entry=274897824272, arg3=arg3@entry=274897826368, arg4=arg4@entry=274897856736, 
    arg5=arg5@entry=274897826368, arg6=arg6@entry=274897856736, arg8=<optimized out>, arg7=<optimized out>) at ../../linux-user/syscall.c:10267
#11 0x00000001001a6374 in do_syscall (cpu_env=0x10048d4f0, num=<optimized out>, arg1=4001536, arg2=274897824272, arg3=274897826368, arg4=274897856736, arg5=274897826368, arg6=274897856736, arg7=0, arg8=0)
    at ../../linux-user/syscall.c:13302
#12 0x0000000100079bd8 in cpu_loop (env=0x10048d4f0) at ../../linux-user/ppc/cpu_loop.c:440
#13 0x0000000100072cdc in main (argc=<optimized out>, argv=0x7fffffffe2f8, envp=<optimized out>) at ../../linux-user/main.c:889


> If there's no real MachineState object in user mode, what the
> return value of qdev_get_machine() is used for?
> 

It is used to rule out paths that are meaningless for user-mode, 
in qdev_get_machine_hotplug_handler() :

    Object *m_obj = qdev_get_machine();

    if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {

and in cpu_common_realizefn() :

    Object *machine = qdev_get_machine();

    /* qdev_get_machine() can return something that's not TYPE_MACHINE
     * if this is one of the user-only emulators; in that case there's
     * no need to check the ignore_memory_transaction_failures board flag.
     */
    if (object_dynamic_cast(machine, TYPE_MACHINE)) {

Then comes device_set_realized() :

        if (!obj->parent) {
            gchar *name = g_strdup_printf("device[%d]", unattached_count++);

            object_property_add_child(container_get(qdev_get_machine(),
                                                    "/unattached"),
                                      name, obj);
            unattached_parent = true;
            g_free(name);
        }

This perfectly makes sense for system-mode again, but I can't tell
what it buys for user-mode...

> > 
> > Second, but not least, it has a side-effect : if it cannot find an
> > object at "/machine" in the QOM tree, it creates a dummy "container"
> > object and put it there. A simple check on the type returned by
> > qdev_get_machine() allows user-mode to run the common qdev code,
> > skipping the parts that only make sense for system-mode.
> > 
> > This side-effect turns out to complicate the use of qdev_get_machine()
> > for the system-mode case though. Most notably, qdev_get_machine() must
> > not be called before the machine object is added to the QOM tree by
> > qemu_create_machine(), otherwise the existing dummy "container" object
> > would cause qemu_create_machine() to fail with something like :
> > 
> > Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
> > qemu-system-ppc64: attempt to add duplicate property 'machine' to
> >  object (type 'container')
> > Aborted (core dumped)
> > 
> > This situation doesn't exist in the current code base, mostly because
> > of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
> > and e2fb3fbbf9c for details).
> > 
> > A new kind of breakage was spotted very recently though :
> > 
> > $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> > /home/thuth/devel/qemu/include/hw/boards.h:24:
> >  MACHINE: Object 0x5635bd53af10 is not an instance of type machine
> > Aborted (core dumped)
> > 
> > This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
> > added a new condition for qdev_get_machine() to be called too early,
> > breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
> > time.
> > 
> > In order to avoid further subtle breakages like this, change the
> > implentation of qdev_get_machine() to:
> > - keep the existing behaviour of creating the dummy "container"
> >   object for the user-mode case only ;
> > - abort() if the machine doesn't exist yet in the QOM tree for
> >   the system-mode case. This gives a precise hint to developpers
> >   that calling qdev_get_machine() too early is a programming bug.
> > 
> > This is achieved with a new do_qdev_get_machine() function called
> > from qdev_get_machine(), with different implementations for system
> > and user mode.
> > 
> > $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> > qemu-system-ppc64: ../../hw/core/machine.c:1290:
> >  qdev_get_machine: Assertion `machine != NULL' failed.
> > Aborted (core dumped)
> > 
> > Reported-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/core/machine.c        | 14 ++++++++++++++
> >  hw/core/qdev.c           |  2 +-
> >  include/hw/qdev-core.h   |  1 +
> >  stubs/meson.build        |  1 +
> >  stubs/qdev-get-machine.c | 11 +++++++++++
> >  5 files changed, 28 insertions(+), 1 deletion(-)
> >  create mode 100644 stubs/qdev-get-machine.c
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 40def78183a7..fecca4023105 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -1293,6 +1293,20 @@ void qdev_machine_creation_done(void)
> >      register_global_state();
> >  }
> >  
> > +Object *do_qdev_get_machine(void)
> > +{
> > +    Object *machine;
> > +
> > +    machine = object_resolve_path_component(object_get_root(), "machine");
> 
> What about just replacing this expression with `current_machine`?
> 

Yes, this is an option.

> > +    /*
> > +     * qdev_get_machine() shouldn't be called before qemu_create_machine()
> > +     * has created the "/machine" path.
> > +     */
> > +    assert(machine != NULL);
> > +
> > +    return machine;
> 
> > +}
> > +
> >  static const TypeInfo machine_info = {
> >      .name = TYPE_MACHINE,
> >      .parent = TYPE_OBJECT,
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index cefc5eaa0a92..1122721b2bf0 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -1131,7 +1131,7 @@ Object *qdev_get_machine(void)
> >      static Object *dev;
> 
> It's interesting how this variable simply duplicates the purpose
> of `current_machine`.
> 

Agreed for system-mode. User-mode doesn't have current_machine
though : we cannot use it in hw/core/qdev.c .

> >  
> >      if (dev == NULL) {
> > -        dev = container_get(object_get_root(), "/machine");
> > +        dev = do_qdev_get_machine();
> >      }
> >  
> >      return dev;
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index bafc311bfa1b..90e295e0bc1a 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -782,6 +782,7 @@ const char *qdev_fw_name(DeviceState *dev);
> >  
> >  void qdev_assert_realized_properly(void);
> >  Object *qdev_get_machine(void);
> > +Object *do_qdev_get_machine(void);
> >  
> >  /* FIXME: make this a link<> */
> >  bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
> > diff --git a/stubs/meson.build b/stubs/meson.build
> > index be6f6d609e58..b99ee2b33e94 100644
> > --- a/stubs/meson.build
> > +++ b/stubs/meson.build
> > @@ -54,3 +54,4 @@ if have_system
> >  else
> >    stub_ss.add(files('qdev.c'))
> >  endif
> > +stub_ss.add(files('qdev-get-machine.c'))
> > diff --git a/stubs/qdev-get-machine.c b/stubs/qdev-get-machine.c
> > new file mode 100644
> > index 000000000000..ed4cdaa01900
> > --- /dev/null
> > +++ b/stubs/qdev-get-machine.c
> > @@ -0,0 +1,11 @@
> > +#include "qemu/osdep.h"
> > +#include "hw/qdev-core.h"
> > +
> > +Object *do_qdev_get_machine(void)
> > +{
> > +    /*
> > +     * This will create a "container" and add it to the QOM tree, if there
> > +     * isn't one already.
> > +     */
> > +    return container_get(object_get_root(), "/machine");
> > +}
> 
> I'm curious to understand when exactly this stub is useful.
> 

It gets called from the code snippet in device_set_realized() already
mentioned above:

            object_property_add_child(container_get(qdev_get_machine(),
                                                    "/unattached"),

The only usefulness I see in this case is to accommodate container_get()
which expects an actual object and crashes if passed NULL :

#0  object_get_class (obj=0x0) at ../../qom/object.c:1001
#1  0x00000001002084d8 in object_property_find (obj=<optimized out>, name=<optimized out>) at ../../qom/object.c:1283
#2  0x000000010020ae68 in object_resolve_path_component (parent=<optimized out>, part=<optimized out>) at ../../qom/object.c:2046
#3  0x000000010020c528 in container_get (root=<optimized out>, path=<optimized out>) at ../../qom/container.c:38
#4  0x000000010020088c in device_set_realized (obj=0x100485080, value=<optimized out>, errp=0x7fffffffd620) at ../../hw/core/qdev.c:745

but as said above, I cannot tell if the subset of qdev/qom code used in
user-mode assumes that any device must have a parent... Any input in
this area would be appreciated.

Cheers,

--
Greg

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Philippe Mathieu-Daudé]

On 4/9/21 6:03 PM, Greg Kurz wrote:
> Despite its simple name and common usage of "getting a pointer to
> the machine" in system-mode emulation, qdev_get_machine() has some
> subtilities.
> 
> First, it can be called when running user-mode emulation : this is
> because user-mode partly relies on qdev to instantiate its CPU
> model.
> 
> Second, but not least, it has a side-effect : if it cannot find an
> object at "/machine" in the QOM tree, it creates a dummy "container"
> object and put it there. A simple check on the type returned by
> qdev_get_machine() allows user-mode to run the common qdev code,
> skipping the parts that only make sense for system-mode.
> 
> This side-effect turns out to complicate the use of qdev_get_machine()
> for the system-mode case though. Most notably, qdev_get_machine() must
> not be called before the machine object is added to the QOM tree by
> qemu_create_machine(), otherwise the existing dummy "container" object
> would cause qemu_create_machine() to fail with something like :
> 
> Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
> qemu-system-ppc64: attempt to add duplicate property 'machine' to
>  object (type 'container')
> Aborted (core dumped)
> 
> This situation doesn't exist in the current code base, mostly because
> of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
> and e2fb3fbbf9c for details).
> 
> A new kind of breakage was spotted very recently though :
> 
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> /home/thuth/devel/qemu/include/hw/boards.h:24:
>  MACHINE: Object 0x5635bd53af10 is not an instance of type machine
> Aborted (core dumped)
> 
> This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
> added a new condition for qdev_get_machine() to be called too early,
> breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
> time.
> 
> In order to avoid further subtle breakages like this, change the
> implentation of qdev_get_machine() to:
> - keep the existing behaviour of creating the dummy "container"
>   object for the user-mode case only ;
> - abort() if the machine doesn't exist yet in the QOM tree for
>   the system-mode case. This gives a precise hint to developpers
>   that calling qdev_get_machine() too early is a programming bug.
> 
> This is achieved with a new do_qdev_get_machine() function called
> from qdev_get_machine(), with different implementations for system
> and user mode.
> 
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>  qdev_get_machine: Assertion `machine != NULL' failed.
> Aborted (core dumped)
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/core/machine.c        | 14 ++++++++++++++
>  hw/core/qdev.c           |  2 +-
>  include/hw/qdev-core.h   |  1 +
>  stubs/meson.build        |  1 +
>  stubs/qdev-get-machine.c | 11 +++++++++++
>  5 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100644 stubs/qdev-get-machine.c
...

> diff --git a/stubs/meson.build b/stubs/meson.build
> index be6f6d609e58..b99ee2b33e94 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -54,3 +54,4 @@ if have_system
>  else
>    stub_ss.add(files('qdev.c'))
>  endif
> +stub_ss.add(files('qdev-get-machine.c'))

Adding this as a stub looks suspicious...
Why not add it in to user_ss in hw/core/meson.build?
Maybe name the new file hw/core/qdev-user.c?

-- >8 --
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -24,6 +24,8 @@
 common_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
 common_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))

+user_ss.add(files('qdev-user.c'))
+
 softmmu_ss.add(files(
   'fw-path-provider.c',
   'loader.c',
---

Thanks,

Phil.

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Greg Kurz]

On Thu, 15 Apr 2021 14:39:55 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 4/9/21 6:03 PM, Greg Kurz wrote:
> > Despite its simple name and common usage of "getting a pointer to
> > the machine" in system-mode emulation, qdev_get_machine() has some
> > subtilities.
> > 
> > First, it can be called when running user-mode emulation : this is
> > because user-mode partly relies on qdev to instantiate its CPU
> > model.
> > 
> > Second, but not least, it has a side-effect : if it cannot find an
> > object at "/machine" in the QOM tree, it creates a dummy "container"
> > object and put it there. A simple check on the type returned by
> > qdev_get_machine() allows user-mode to run the common qdev code,
> > skipping the parts that only make sense for system-mode.
> > 
> > This side-effect turns out to complicate the use of qdev_get_machine()
> > for the system-mode case though. Most notably, qdev_get_machine() must
> > not be called before the machine object is added to the QOM tree by
> > qemu_create_machine(), otherwise the existing dummy "container" object
> > would cause qemu_create_machine() to fail with something like :
> > 
> > Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
> > qemu-system-ppc64: attempt to add duplicate property 'machine' to
> >  object (type 'container')
> > Aborted (core dumped)
> > 
> > This situation doesn't exist in the current code base, mostly because
> > of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
> > and e2fb3fbbf9c for details).
> > 
> > A new kind of breakage was spotted very recently though :
> > 
> > $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> > /home/thuth/devel/qemu/include/hw/boards.h:24:
> >  MACHINE: Object 0x5635bd53af10 is not an instance of type machine
> > Aborted (core dumped)
> > 
> > This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
> > added a new condition for qdev_get_machine() to be called too early,
> > breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
> > time.
> > 
> > In order to avoid further subtle breakages like this, change the
> > implentation of qdev_get_machine() to:
> > - keep the existing behaviour of creating the dummy "container"
> >   object for the user-mode case only ;
> > - abort() if the machine doesn't exist yet in the QOM tree for
> >   the system-mode case. This gives a precise hint to developpers
> >   that calling qdev_get_machine() too early is a programming bug.
> > 
> > This is achieved with a new do_qdev_get_machine() function called
> > from qdev_get_machine(), with different implementations for system
> > and user mode.
> > 
> > $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> > qemu-system-ppc64: ../../hw/core/machine.c:1290:
> >  qdev_get_machine: Assertion `machine != NULL' failed.
> > Aborted (core dumped)
> > 
> > Reported-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/core/machine.c        | 14 ++++++++++++++
> >  hw/core/qdev.c           |  2 +-
> >  include/hw/qdev-core.h   |  1 +
> >  stubs/meson.build        |  1 +
> >  stubs/qdev-get-machine.c | 11 +++++++++++
> >  5 files changed, 28 insertions(+), 1 deletion(-)
> >  create mode 100644 stubs/qdev-get-machine.c
> ...
> 
> > diff --git a/stubs/meson.build b/stubs/meson.build
> > index be6f6d609e58..b99ee2b33e94 100644
> > --- a/stubs/meson.build
> > +++ b/stubs/meson.build
> > @@ -54,3 +54,4 @@ if have_system
> >  else
> >    stub_ss.add(files('qdev.c'))
> >  endif
> > +stub_ss.add(files('qdev-get-machine.c'))
> 
> Adding this as a stub looks suspicious...
> Why not add it in to user_ss in hw/core/meson.build?
> Maybe name the new file hw/core/qdev-user.c?
> 

It turns out that this isn't specific to user-mode but rather
to any non-qemu-system-FOO binary built with qdev, e.g.
test-qdev-global-props :

#0  do_qdev_get_machine () at ../../stubs/qdev-get-machine.c:10
#1  0x0000000100017938 in qdev_get_machine () at ../../hw/core/qdev.c:1134
#2  0x000000010001855c in device_set_realized (obj=0x100128b60, value=<optimized out>, errp=0x7fffffffd4e0) at ../../hw/core/qdev.c:745
#3  0x000000010001cc5c in property_set_bool (obj=0x100128b60, v=<optimized out>, name=<optimized out>, opaque=0x1000f33f0, errp=0x7fffffffd4e0) at ../../qom/object.c:2257
#4  0x0000000100020a9c in object_property_set (obj=0x100128b60, name=0x100093f78 "realized", v=0x100136d30, errp=0x1000e3af8 <error_fatal>) at ../../qom/object.c:1402
#5  0x000000010001c38c in object_property_set_qobject (obj=0x100128b60, name=0x100093f78 "realized", value=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../qom/qom-qobject.c:28
#6  0x0000000100020e20 in object_property_set_bool (obj=0x100128b60, name=0x100093f78 "realized", value=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../qom/object.c:1472
#7  0x000000010001612c in qdev_realize (dev=0x100128b60, bus=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../hw/core/qdev.c:389
#8  0x000000010000fb10 in test_static_prop_subprocess () at /home/greg/Work/qemu/qemu-master/include/hw/qdev-core.h:17
#9  0x00007ffff7e95654 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
#10 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
#11 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
#12 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
#13 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
#14 0x00007ffff7e959cc in g_test_run_suite () from /lib64/libglib-2.0.so.0
#15 0x00007ffff7e95a80 in g_test_run () from /lib64/libglib-2.0.so.0
#16 0x000000010000ecec in main (argc=<optimized out>, argv=<optimized out>) at ../../tests/unit/test-qdev-global-props.c:316

Is there a meson thingy to handle this dependency ?

> -- >8 --
> --- a/hw/core/meson.build
> +++ b/hw/core/meson.build
> @@ -24,6 +24,8 @@
>  common_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
>  common_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
> 
> +user_ss.add(files('qdev-user.c'))
> +
>  softmmu_ss.add(files(
>    'fw-path-provider.c',
>    'loader.c',
> ---
> 
> Thanks,
> 
> Phil.
> 

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Philippe Mathieu-Daudé]

On 4/15/21 3:30 PM, Greg Kurz wrote:
> On Thu, 15 Apr 2021 14:39:55 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> On 4/9/21 6:03 PM, Greg Kurz wrote:
>>> Despite its simple name and common usage of "getting a pointer to
>>> the machine" in system-mode emulation, qdev_get_machine() has some
>>> subtilities.
>>>
>>> First, it can be called when running user-mode emulation : this is
>>> because user-mode partly relies on qdev to instantiate its CPU
>>> model.
>>>
>>> Second, but not least, it has a side-effect : if it cannot find an
>>> object at "/machine" in the QOM tree, it creates a dummy "container"
>>> object and put it there. A simple check on the type returned by
>>> qdev_get_machine() allows user-mode to run the common qdev code,
>>> skipping the parts that only make sense for system-mode.
>>>
>>> This side-effect turns out to complicate the use of qdev_get_machine()
>>> for the system-mode case though. Most notably, qdev_get_machine() must
>>> not be called before the machine object is added to the QOM tree by
>>> qemu_create_machine(), otherwise the existing dummy "container" object
>>> would cause qemu_create_machine() to fail with something like :
>>>
>>> Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
>>> qemu-system-ppc64: attempt to add duplicate property 'machine' to
>>>  object (type 'container')
>>> Aborted (core dumped)
>>>
>>> This situation doesn't exist in the current code base, mostly because
>>> of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
>>> and e2fb3fbbf9c for details).
>>>
>>> A new kind of breakage was spotted very recently though :
>>>
>>> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
>>> /home/thuth/devel/qemu/include/hw/boards.h:24:
>>>  MACHINE: Object 0x5635bd53af10 is not an instance of type machine
>>> Aborted (core dumped)
>>>
>>> This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
>>> added a new condition for qdev_get_machine() to be called too early,
>>> breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
>>> time.
>>>
>>> In order to avoid further subtle breakages like this, change the
>>> implentation of qdev_get_machine() to:
>>> - keep the existing behaviour of creating the dummy "container"
>>>   object for the user-mode case only ;
>>> - abort() if the machine doesn't exist yet in the QOM tree for
>>>   the system-mode case. This gives a precise hint to developpers
>>>   that calling qdev_get_machine() too early is a programming bug.
>>>
>>> This is achieved with a new do_qdev_get_machine() function called
>>> from qdev_get_machine(), with different implementations for system
>>> and user mode.
>>>
>>> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
>>> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>>>  qdev_get_machine: Assertion `machine != NULL' failed.
>>> Aborted (core dumped)
>>>
>>> Reported-by: Thomas Huth <thuth@redhat.com>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>>  hw/core/machine.c        | 14 ++++++++++++++
>>>  hw/core/qdev.c           |  2 +-
>>>  include/hw/qdev-core.h   |  1 +
>>>  stubs/meson.build        |  1 +
>>>  stubs/qdev-get-machine.c | 11 +++++++++++
>>>  5 files changed, 28 insertions(+), 1 deletion(-)
>>>  create mode 100644 stubs/qdev-get-machine.c
>> ...
>>
>>> diff --git a/stubs/meson.build b/stubs/meson.build
>>> index be6f6d609e58..b99ee2b33e94 100644
>>> --- a/stubs/meson.build
>>> +++ b/stubs/meson.build
>>> @@ -54,3 +54,4 @@ if have_system
>>>  else
>>>    stub_ss.add(files('qdev.c'))
>>>  endif
>>> +stub_ss.add(files('qdev-get-machine.c'))
>>
>> Adding this as a stub looks suspicious...
>> Why not add it in to user_ss in hw/core/meson.build?
>> Maybe name the new file hw/core/qdev-user.c?
>>
> 
> It turns out that this isn't specific to user-mode but rather
> to any non-qemu-system-FOO binary built with qdev, e.g.
> test-qdev-global-props :
> 
> #0  do_qdev_get_machine () at ../../stubs/qdev-get-machine.c:10
> #1  0x0000000100017938 in qdev_get_machine () at ../../hw/core/qdev.c:1134
> #2  0x000000010001855c in device_set_realized (obj=0x100128b60, value=<optimized out>, errp=0x7fffffffd4e0) at ../../hw/core/qdev.c:745
> #3  0x000000010001cc5c in property_set_bool (obj=0x100128b60, v=<optimized out>, name=<optimized out>, opaque=0x1000f33f0, errp=0x7fffffffd4e0) at ../../qom/object.c:2257
> #4  0x0000000100020a9c in object_property_set (obj=0x100128b60, name=0x100093f78 "realized", v=0x100136d30, errp=0x1000e3af8 <error_fatal>) at ../../qom/object.c:1402
> #5  0x000000010001c38c in object_property_set_qobject (obj=0x100128b60, name=0x100093f78 "realized", value=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../qom/qom-qobject.c:28
> #6  0x0000000100020e20 in object_property_set_bool (obj=0x100128b60, name=0x100093f78 "realized", value=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../qom/object.c:1472
> #7  0x000000010001612c in qdev_realize (dev=0x100128b60, bus=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../hw/core/qdev.c:389
> #8  0x000000010000fb10 in test_static_prop_subprocess () at /home/greg/Work/qemu/qemu-master/include/hw/qdev-core.h:17
> #9  0x00007ffff7e95654 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> #10 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> #11 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> #12 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> #13 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> #14 0x00007ffff7e959cc in g_test_run_suite () from /lib64/libglib-2.0.so.0
> #15 0x00007ffff7e95a80 in g_test_run () from /lib64/libglib-2.0.so.0
> #16 0x000000010000ecec in main (argc=<optimized out>, argv=<optimized out>) at ../../tests/unit/test-qdev-global-props.c:316
> 
> Is there a meson thingy to handle this dependency ?

  if not have_system
    common_ss.add(files('qdev-machine-stubs.c'))
  endif

This is not pretty, but better than a generic stubs/qdev-get-machine.c
IMO...

> 
>> -- >8 --
>> --- a/hw/core/meson.build
>> +++ b/hw/core/meson.build
>> @@ -24,6 +24,8 @@
>>  common_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
>>  common_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
>>
>> +user_ss.add(files('qdev-user.c'))
>> +
>>  softmmu_ss.add(files(
>>    'fw-path-provider.c',
>>    'loader.c',
>> ---
>>
>> Thanks,
>>
>> Phil.
>>
> 

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Greg Kurz]

On Thu, 15 Apr 2021 18:45:45 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 4/15/21 3:30 PM, Greg Kurz wrote:
> > On Thu, 15 Apr 2021 14:39:55 +0200
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > 
> >> On 4/9/21 6:03 PM, Greg Kurz wrote:
> >>> Despite its simple name and common usage of "getting a pointer to
> >>> the machine" in system-mode emulation, qdev_get_machine() has some
> >>> subtilities.
> >>>
> >>> First, it can be called when running user-mode emulation : this is
> >>> because user-mode partly relies on qdev to instantiate its CPU
> >>> model.
> >>>
> >>> Second, but not least, it has a side-effect : if it cannot find an
> >>> object at "/machine" in the QOM tree, it creates a dummy "container"
> >>> object and put it there. A simple check on the type returned by
> >>> qdev_get_machine() allows user-mode to run the common qdev code,
> >>> skipping the parts that only make sense for system-mode.
> >>>
> >>> This side-effect turns out to complicate the use of qdev_get_machine()
> >>> for the system-mode case though. Most notably, qdev_get_machine() must
> >>> not be called before the machine object is added to the QOM tree by
> >>> qemu_create_machine(), otherwise the existing dummy "container" object
> >>> would cause qemu_create_machine() to fail with something like :
> >>>
> >>> Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
> >>> qemu-system-ppc64: attempt to add duplicate property 'machine' to
> >>>  object (type 'container')
> >>> Aborted (core dumped)
> >>>
> >>> This situation doesn't exist in the current code base, mostly because
> >>> of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
> >>> and e2fb3fbbf9c for details).
> >>>
> >>> A new kind of breakage was spotted very recently though :
> >>>
> >>> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> >>> /home/thuth/devel/qemu/include/hw/boards.h:24:
> >>>  MACHINE: Object 0x5635bd53af10 is not an instance of type machine
> >>> Aborted (core dumped)
> >>>
> >>> This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
> >>> added a new condition for qdev_get_machine() to be called too early,
> >>> breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
> >>> time.
> >>>
> >>> In order to avoid further subtle breakages like this, change the
> >>> implentation of qdev_get_machine() to:
> >>> - keep the existing behaviour of creating the dummy "container"
> >>>   object for the user-mode case only ;
> >>> - abort() if the machine doesn't exist yet in the QOM tree for
> >>>   the system-mode case. This gives a precise hint to developpers
> >>>   that calling qdev_get_machine() too early is a programming bug.
> >>>
> >>> This is achieved with a new do_qdev_get_machine() function called
> >>> from qdev_get_machine(), with different implementations for system
> >>> and user mode.
> >>>
> >>> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> >>> qemu-system-ppc64: ../../hw/core/machine.c:1290:
> >>>  qdev_get_machine: Assertion `machine != NULL' failed.
> >>> Aborted (core dumped)
> >>>
> >>> Reported-by: Thomas Huth <thuth@redhat.com>
> >>> Signed-off-by: Greg Kurz <groug@kaod.org>
> >>> ---
> >>>  hw/core/machine.c        | 14 ++++++++++++++
> >>>  hw/core/qdev.c           |  2 +-
> >>>  include/hw/qdev-core.h   |  1 +
> >>>  stubs/meson.build        |  1 +
> >>>  stubs/qdev-get-machine.c | 11 +++++++++++
> >>>  5 files changed, 28 insertions(+), 1 deletion(-)
> >>>  create mode 100644 stubs/qdev-get-machine.c
> >> ...
> >>
> >>> diff --git a/stubs/meson.build b/stubs/meson.build
> >>> index be6f6d609e58..b99ee2b33e94 100644
> >>> --- a/stubs/meson.build
> >>> +++ b/stubs/meson.build
> >>> @@ -54,3 +54,4 @@ if have_system
> >>>  else
> >>>    stub_ss.add(files('qdev.c'))
> >>>  endif
> >>> +stub_ss.add(files('qdev-get-machine.c'))
> >>
> >> Adding this as a stub looks suspicious...
> >> Why not add it in to user_ss in hw/core/meson.build?
> >> Maybe name the new file hw/core/qdev-user.c?
> >>
> > 
> > It turns out that this isn't specific to user-mode but rather
> > to any non-qemu-system-FOO binary built with qdev, e.g.
> > test-qdev-global-props :
> > 
> > #0  do_qdev_get_machine () at ../../stubs/qdev-get-machine.c:10
> > #1  0x0000000100017938 in qdev_get_machine () at ../../hw/core/qdev.c:1134
> > #2  0x000000010001855c in device_set_realized (obj=0x100128b60, value=<optimized out>, errp=0x7fffffffd4e0) at ../../hw/core/qdev.c:745
> > #3  0x000000010001cc5c in property_set_bool (obj=0x100128b60, v=<optimized out>, name=<optimized out>, opaque=0x1000f33f0, errp=0x7fffffffd4e0) at ../../qom/object.c:2257
> > #4  0x0000000100020a9c in object_property_set (obj=0x100128b60, name=0x100093f78 "realized", v=0x100136d30, errp=0x1000e3af8 <error_fatal>) at ../../qom/object.c:1402
> > #5  0x000000010001c38c in object_property_set_qobject (obj=0x100128b60, name=0x100093f78 "realized", value=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../qom/qom-qobject.c:28
> > #6  0x0000000100020e20 in object_property_set_bool (obj=0x100128b60, name=0x100093f78 "realized", value=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../qom/object.c:1472
> > #7  0x000000010001612c in qdev_realize (dev=0x100128b60, bus=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../hw/core/qdev.c:389
> > #8  0x000000010000fb10 in test_static_prop_subprocess () at /home/greg/Work/qemu/qemu-master/include/hw/qdev-core.h:17
> > #9  0x00007ffff7e95654 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> > #10 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> > #11 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> > #12 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> > #13 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> > #14 0x00007ffff7e959cc in g_test_run_suite () from /lib64/libglib-2.0.so.0
> > #15 0x00007ffff7e95a80 in g_test_run () from /lib64/libglib-2.0.so.0
> > #16 0x000000010000ecec in main (argc=<optimized out>, argv=<optimized out>) at ../../tests/unit/test-qdev-global-props.c:316
> > 
> > Is there a meson thingy to handle this dependency ?
> 
>   if not have_system
>     common_ss.add(files('qdev-machine-stubs.c'))
>   endif
> 
> This is not pretty, but better than a generic stubs/qdev-get-machine.c
> IMO...
> 

Yeah it isn't that much different, except maybe an improvement on the
file location. Thanks for tip !

> > 
> >> -- >8 --
> >> --- a/hw/core/meson.build
> >> +++ b/hw/core/meson.build
> >> @@ -24,6 +24,8 @@
> >>  common_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
> >>  common_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
> >>
> >> +user_ss.add(files('qdev-user.c'))
> >> +
> >>  softmmu_ss.add(files(
> >>    'fw-path-provider.c',
> >>    'loader.c',
> >> ---
> >>
> >> Thanks,
> >>
> >> Phil.
> >>
> > 
> 

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Philippe Mathieu-Daudé]

On 4/15/21 6:56 PM, Greg Kurz wrote:
> On Thu, 15 Apr 2021 18:45:45 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> On 4/15/21 3:30 PM, Greg Kurz wrote:
>>> On Thu, 15 Apr 2021 14:39:55 +0200
>>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>
>>>> On 4/9/21 6:03 PM, Greg Kurz wrote:
>>>>> Despite its simple name and common usage of "getting a pointer to
>>>>> the machine" in system-mode emulation, qdev_get_machine() has some
>>>>> subtilities.
>>>>>
>>>>> First, it can be called when running user-mode emulation : this is
>>>>> because user-mode partly relies on qdev to instantiate its CPU
>>>>> model.
>>>>>
>>>>> Second, but not least, it has a side-effect : if it cannot find an
>>>>> object at "/machine" in the QOM tree, it creates a dummy "container"
>>>>> object and put it there. A simple check on the type returned by
>>>>> qdev_get_machine() allows user-mode to run the common qdev code,
>>>>> skipping the parts that only make sense for system-mode.
>>>>>
>>>>> This side-effect turns out to complicate the use of qdev_get_machine()
>>>>> for the system-mode case though. Most notably, qdev_get_machine() must
>>>>> not be called before the machine object is added to the QOM tree by
>>>>> qemu_create_machine(), otherwise the existing dummy "container" object
>>>>> would cause qemu_create_machine() to fail with something like :
>>>>>
>>>>> Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
>>>>> qemu-system-ppc64: attempt to add duplicate property 'machine' to
>>>>>  object (type 'container')
>>>>> Aborted (core dumped)
>>>>>
>>>>> This situation doesn't exist in the current code base, mostly because
>>>>> of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
>>>>> and e2fb3fbbf9c for details).
>>>>>
>>>>> A new kind of breakage was spotted very recently though :
>>>>>
>>>>> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
>>>>> /home/thuth/devel/qemu/include/hw/boards.h:24:
>>>>>  MACHINE: Object 0x5635bd53af10 is not an instance of type machine
>>>>> Aborted (core dumped)
>>>>>
>>>>> This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
>>>>> added a new condition for qdev_get_machine() to be called too early,
>>>>> breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
>>>>> time.
>>>>>
>>>>> In order to avoid further subtle breakages like this, change the
>>>>> implentation of qdev_get_machine() to:
>>>>> - keep the existing behaviour of creating the dummy "container"
>>>>>   object for the user-mode case only ;
>>>>> - abort() if the machine doesn't exist yet in the QOM tree for
>>>>>   the system-mode case. This gives a precise hint to developpers
>>>>>   that calling qdev_get_machine() too early is a programming bug.
>>>>>
>>>>> This is achieved with a new do_qdev_get_machine() function called
>>>>> from qdev_get_machine(), with different implementations for system
>>>>> and user mode.
>>>>>
>>>>> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
>>>>> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>>>>>  qdev_get_machine: Assertion `machine != NULL' failed.
>>>>> Aborted (core dumped)
>>>>>
>>>>> Reported-by: Thomas Huth <thuth@redhat.com>
>>>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>>>> ---
>>>>>  hw/core/machine.c        | 14 ++++++++++++++
>>>>>  hw/core/qdev.c           |  2 +-
>>>>>  include/hw/qdev-core.h   |  1 +
>>>>>  stubs/meson.build        |  1 +
>>>>>  stubs/qdev-get-machine.c | 11 +++++++++++
>>>>>  5 files changed, 28 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 stubs/qdev-get-machine.c
>>>> ...
>>>>
>>>>> diff --git a/stubs/meson.build b/stubs/meson.build
>>>>> index be6f6d609e58..b99ee2b33e94 100644
>>>>> --- a/stubs/meson.build
>>>>> +++ b/stubs/meson.build
>>>>> @@ -54,3 +54,4 @@ if have_system
>>>>>  else
>>>>>    stub_ss.add(files('qdev.c'))
>>>>>  endif
>>>>> +stub_ss.add(files('qdev-get-machine.c'))
>>>>
>>>> Adding this as a stub looks suspicious...
>>>> Why not add it in to user_ss in hw/core/meson.build?
>>>> Maybe name the new file hw/core/qdev-user.c?
>>>>
>>>
>>> It turns out that this isn't specific to user-mode but rather
>>> to any non-qemu-system-FOO binary built with qdev, e.g.
>>> test-qdev-global-props :
>>>
>>> #0  do_qdev_get_machine () at ../../stubs/qdev-get-machine.c:10
>>> #1  0x0000000100017938 in qdev_get_machine () at ../../hw/core/qdev.c:1134
>>> #2  0x000000010001855c in device_set_realized (obj=0x100128b60, value=<optimized out>, errp=0x7fffffffd4e0) at ../../hw/core/qdev.c:745
>>> #3  0x000000010001cc5c in property_set_bool (obj=0x100128b60, v=<optimized out>, name=<optimized out>, opaque=0x1000f33f0, errp=0x7fffffffd4e0) at ../../qom/object.c:2257
>>> #4  0x0000000100020a9c in object_property_set (obj=0x100128b60, name=0x100093f78 "realized", v=0x100136d30, errp=0x1000e3af8 <error_fatal>) at ../../qom/object.c:1402
>>> #5  0x000000010001c38c in object_property_set_qobject (obj=0x100128b60, name=0x100093f78 "realized", value=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../qom/qom-qobject.c:28
>>> #6  0x0000000100020e20 in object_property_set_bool (obj=0x100128b60, name=0x100093f78 "realized", value=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../qom/object.c:1472
>>> #7  0x000000010001612c in qdev_realize (dev=0x100128b60, bus=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../hw/core/qdev.c:389
>>> #8  0x000000010000fb10 in test_static_prop_subprocess () at /home/greg/Work/qemu/qemu-master/include/hw/qdev-core.h:17
>>> #9  0x00007ffff7e95654 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
>>> #10 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
>>> #11 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
>>> #12 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
>>> #13 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
>>> #14 0x00007ffff7e959cc in g_test_run_suite () from /lib64/libglib-2.0.so.0
>>> #15 0x00007ffff7e95a80 in g_test_run () from /lib64/libglib-2.0.so.0
>>> #16 0x000000010000ecec in main (argc=<optimized out>, argv=<optimized out>) at ../../tests/unit/test-qdev-global-props.c:316
>>>
>>> Is there a meson thingy to handle this dependency ?
>>
>>   if not have_system
>>     common_ss.add(files('qdev-machine-stubs.c'))
>>   endif
>>
>> This is not pretty, but better than a generic stubs/qdev-get-machine.c
>> IMO...
>>
> 
> Yeah it isn't that much different,

I'd expect symbols in stubs/ to be declared weak, while not with this
approach, so we'd catch clash for incorrect configs. Maybe I'm wrong...

> except maybe an improvement on the
> file location. Thanks for tip !

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Greg Kurz]

On Thu, 15 Apr 2021 21:07:33 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 4/15/21 6:56 PM, Greg Kurz wrote:
> > On Thu, 15 Apr 2021 18:45:45 +0200
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > 
> >> On 4/15/21 3:30 PM, Greg Kurz wrote:
> >>> On Thu, 15 Apr 2021 14:39:55 +0200
> >>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >>>
> >>>> On 4/9/21 6:03 PM, Greg Kurz wrote:
> >>>>> Despite its simple name and common usage of "getting a pointer to
> >>>>> the machine" in system-mode emulation, qdev_get_machine() has some
> >>>>> subtilities.
> >>>>>
> >>>>> First, it can be called when running user-mode emulation : this is
> >>>>> because user-mode partly relies on qdev to instantiate its CPU
> >>>>> model.
> >>>>>
> >>>>> Second, but not least, it has a side-effect : if it cannot find an
> >>>>> object at "/machine" in the QOM tree, it creates a dummy "container"
> >>>>> object and put it there. A simple check on the type returned by
> >>>>> qdev_get_machine() allows user-mode to run the common qdev code,
> >>>>> skipping the parts that only make sense for system-mode.
> >>>>>
> >>>>> This side-effect turns out to complicate the use of qdev_get_machine()
> >>>>> for the system-mode case though. Most notably, qdev_get_machine() must
> >>>>> not be called before the machine object is added to the QOM tree by
> >>>>> qemu_create_machine(), otherwise the existing dummy "container" object
> >>>>> would cause qemu_create_machine() to fail with something like :
> >>>>>
> >>>>> Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
> >>>>> qemu-system-ppc64: attempt to add duplicate property 'machine' to
> >>>>>  object (type 'container')
> >>>>> Aborted (core dumped)
> >>>>>
> >>>>> This situation doesn't exist in the current code base, mostly because
> >>>>> of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
> >>>>> and e2fb3fbbf9c for details).
> >>>>>
> >>>>> A new kind of breakage was spotted very recently though :
> >>>>>
> >>>>> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> >>>>> /home/thuth/devel/qemu/include/hw/boards.h:24:
> >>>>>  MACHINE: Object 0x5635bd53af10 is not an instance of type machine
> >>>>> Aborted (core dumped)
> >>>>>
> >>>>> This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
> >>>>> added a new condition for qdev_get_machine() to be called too early,
> >>>>> breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
> >>>>> time.
> >>>>>
> >>>>> In order to avoid further subtle breakages like this, change the
> >>>>> implentation of qdev_get_machine() to:
> >>>>> - keep the existing behaviour of creating the dummy "container"
> >>>>>   object for the user-mode case only ;
> >>>>> - abort() if the machine doesn't exist yet in the QOM tree for
> >>>>>   the system-mode case. This gives a precise hint to developpers
> >>>>>   that calling qdev_get_machine() too early is a programming bug.
> >>>>>
> >>>>> This is achieved with a new do_qdev_get_machine() function called
> >>>>> from qdev_get_machine(), with different implementations for system
> >>>>> and user mode.
> >>>>>
> >>>>> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> >>>>> qemu-system-ppc64: ../../hw/core/machine.c:1290:
> >>>>>  qdev_get_machine: Assertion `machine != NULL' failed.
> >>>>> Aborted (core dumped)
> >>>>>
> >>>>> Reported-by: Thomas Huth <thuth@redhat.com>
> >>>>> Signed-off-by: Greg Kurz <groug@kaod.org>
> >>>>> ---
> >>>>>  hw/core/machine.c        | 14 ++++++++++++++
> >>>>>  hw/core/qdev.c           |  2 +-
> >>>>>  include/hw/qdev-core.h   |  1 +
> >>>>>  stubs/meson.build        |  1 +
> >>>>>  stubs/qdev-get-machine.c | 11 +++++++++++
> >>>>>  5 files changed, 28 insertions(+), 1 deletion(-)
> >>>>>  create mode 100644 stubs/qdev-get-machine.c
> >>>> ...
> >>>>
> >>>>> diff --git a/stubs/meson.build b/stubs/meson.build
> >>>>> index be6f6d609e58..b99ee2b33e94 100644
> >>>>> --- a/stubs/meson.build
> >>>>> +++ b/stubs/meson.build
> >>>>> @@ -54,3 +54,4 @@ if have_system
> >>>>>  else
> >>>>>    stub_ss.add(files('qdev.c'))
> >>>>>  endif
> >>>>> +stub_ss.add(files('qdev-get-machine.c'))
> >>>>
> >>>> Adding this as a stub looks suspicious...
> >>>> Why not add it in to user_ss in hw/core/meson.build?
> >>>> Maybe name the new file hw/core/qdev-user.c?
> >>>>
> >>>
> >>> It turns out that this isn't specific to user-mode but rather
> >>> to any non-qemu-system-FOO binary built with qdev, e.g.
> >>> test-qdev-global-props :
> >>>
> >>> #0  do_qdev_get_machine () at ../../stubs/qdev-get-machine.c:10
> >>> #1  0x0000000100017938 in qdev_get_machine () at ../../hw/core/qdev.c:1134
> >>> #2  0x000000010001855c in device_set_realized (obj=0x100128b60, value=<optimized out>, errp=0x7fffffffd4e0) at ../../hw/core/qdev.c:745
> >>> #3  0x000000010001cc5c in property_set_bool (obj=0x100128b60, v=<optimized out>, name=<optimized out>, opaque=0x1000f33f0, errp=0x7fffffffd4e0) at ../../qom/object.c:2257
> >>> #4  0x0000000100020a9c in object_property_set (obj=0x100128b60, name=0x100093f78 "realized", v=0x100136d30, errp=0x1000e3af8 <error_fatal>) at ../../qom/object.c:1402
> >>> #5  0x000000010001c38c in object_property_set_qobject (obj=0x100128b60, name=0x100093f78 "realized", value=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../qom/qom-qobject.c:28
> >>> #6  0x0000000100020e20 in object_property_set_bool (obj=0x100128b60, name=0x100093f78 "realized", value=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../qom/object.c:1472
> >>> #7  0x000000010001612c in qdev_realize (dev=0x100128b60, bus=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../hw/core/qdev.c:389
> >>> #8  0x000000010000fb10 in test_static_prop_subprocess () at /home/greg/Work/qemu/qemu-master/include/hw/qdev-core.h:17
> >>> #9  0x00007ffff7e95654 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> >>> #10 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> >>> #11 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> >>> #12 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> >>> #13 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
> >>> #14 0x00007ffff7e959cc in g_test_run_suite () from /lib64/libglib-2.0.so.0
> >>> #15 0x00007ffff7e95a80 in g_test_run () from /lib64/libglib-2.0.so.0
> >>> #16 0x000000010000ecec in main (argc=<optimized out>, argv=<optimized out>) at ../../tests/unit/test-qdev-global-props.c:316
> >>>
> >>> Is there a meson thingy to handle this dependency ?
> >>
> >>   if not have_system
> >>     common_ss.add(files('qdev-machine-stubs.c'))
> >>   endif
> >>
> >> This is not pretty, but better than a generic stubs/qdev-get-machine.c
> >> IMO...
> >>
> > 
> > Yeah it isn't that much different,
> 
> I'd expect symbols in stubs/ to be declared weak, while not with this
> approach, so we'd catch clash for incorrect configs. Maybe I'm wrong...
> 

Ah yes, you're certainly right. I'm convinced :)

> > except maybe an improvement on the
> > file location. Thanks for tip !
> 

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Thomas Huth]

On 15/04/2021 21.07, Philippe Mathieu-Daudé wrote:
> On 4/15/21 6:56 PM, Greg Kurz wrote:
>> On Thu, 15 Apr 2021 18:45:45 +0200
>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>>> On 4/15/21 3:30 PM, Greg Kurz wrote:
>>>> On Thu, 15 Apr 2021 14:39:55 +0200
>>>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>>
>>>>> On 4/9/21 6:03 PM, Greg Kurz wrote:
>>>>>> Despite its simple name and common usage of "getting a pointer to
>>>>>> the machine" in system-mode emulation, qdev_get_machine() has some
>>>>>> subtilities.
>>>>>>
>>>>>> First, it can be called when running user-mode emulation : this is
>>>>>> because user-mode partly relies on qdev to instantiate its CPU
>>>>>> model.
>>>>>>
>>>>>> Second, but not least, it has a side-effect : if it cannot find an
>>>>>> object at "/machine" in the QOM tree, it creates a dummy "container"
>>>>>> object and put it there. A simple check on the type returned by
>>>>>> qdev_get_machine() allows user-mode to run the common qdev code,
>>>>>> skipping the parts that only make sense for system-mode.
>>>>>>
>>>>>> This side-effect turns out to complicate the use of qdev_get_machine()
>>>>>> for the system-mode case though. Most notably, qdev_get_machine() must
>>>>>> not be called before the machine object is added to the QOM tree by
>>>>>> qemu_create_machine(), otherwise the existing dummy "container" object
>>>>>> would cause qemu_create_machine() to fail with something like :
>>>>>>
>>>>>> Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
>>>>>> qemu-system-ppc64: attempt to add duplicate property 'machine' to
>>>>>>   object (type 'container')
>>>>>> Aborted (core dumped)
>>>>>>
>>>>>> This situation doesn't exist in the current code base, mostly because
>>>>>> of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
>>>>>> and e2fb3fbbf9c for details).
>>>>>>
>>>>>> A new kind of breakage was spotted very recently though :
>>>>>>
>>>>>> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
>>>>>> /home/thuth/devel/qemu/include/hw/boards.h:24:
>>>>>>   MACHINE: Object 0x5635bd53af10 is not an instance of type machine
>>>>>> Aborted (core dumped)
>>>>>>
>>>>>> This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
>>>>>> added a new condition for qdev_get_machine() to be called too early,
>>>>>> breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
>>>>>> time.
>>>>>>
>>>>>> In order to avoid further subtle breakages like this, change the
>>>>>> implentation of qdev_get_machine() to:
>>>>>> - keep the existing behaviour of creating the dummy "container"
>>>>>>    object for the user-mode case only ;
>>>>>> - abort() if the machine doesn't exist yet in the QOM tree for
>>>>>>    the system-mode case. This gives a precise hint to developpers
>>>>>>    that calling qdev_get_machine() too early is a programming bug.
>>>>>>
>>>>>> This is achieved with a new do_qdev_get_machine() function called
>>>>>> from qdev_get_machine(), with different implementations for system
>>>>>> and user mode.
>>>>>>
>>>>>> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
>>>>>> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>>>>>>   qdev_get_machine: Assertion `machine != NULL' failed.
>>>>>> Aborted (core dumped)
>>>>>>
>>>>>> Reported-by: Thomas Huth <thuth@redhat.com>
>>>>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>>>>> ---
>>>>>>   hw/core/machine.c        | 14 ++++++++++++++
>>>>>>   hw/core/qdev.c           |  2 +-
>>>>>>   include/hw/qdev-core.h   |  1 +
>>>>>>   stubs/meson.build        |  1 +
>>>>>>   stubs/qdev-get-machine.c | 11 +++++++++++
>>>>>>   5 files changed, 28 insertions(+), 1 deletion(-)
>>>>>>   create mode 100644 stubs/qdev-get-machine.c
>>>>> ...
>>>>>
>>>>>> diff --git a/stubs/meson.build b/stubs/meson.build
>>>>>> index be6f6d609e58..b99ee2b33e94 100644
>>>>>> --- a/stubs/meson.build
>>>>>> +++ b/stubs/meson.build
>>>>>> @@ -54,3 +54,4 @@ if have_system
>>>>>>   else
>>>>>>     stub_ss.add(files('qdev.c'))
>>>>>>   endif
>>>>>> +stub_ss.add(files('qdev-get-machine.c'))
>>>>>
>>>>> Adding this as a stub looks suspicious...
>>>>> Why not add it in to user_ss in hw/core/meson.build?
>>>>> Maybe name the new file hw/core/qdev-user.c?
>>>>>
>>>>
>>>> It turns out that this isn't specific to user-mode but rather
>>>> to any non-qemu-system-FOO binary built with qdev, e.g.
>>>> test-qdev-global-props :
>>>>
>>>> #0  do_qdev_get_machine () at ../../stubs/qdev-get-machine.c:10
>>>> #1  0x0000000100017938 in qdev_get_machine () at ../../hw/core/qdev.c:1134
>>>> #2  0x000000010001855c in device_set_realized (obj=0x100128b60, value=<optimized out>, errp=0x7fffffffd4e0) at ../../hw/core/qdev.c:745
>>>> #3  0x000000010001cc5c in property_set_bool (obj=0x100128b60, v=<optimized out>, name=<optimized out>, opaque=0x1000f33f0, errp=0x7fffffffd4e0) at ../../qom/object.c:2257
>>>> #4  0x0000000100020a9c in object_property_set (obj=0x100128b60, name=0x100093f78 "realized", v=0x100136d30, errp=0x1000e3af8 <error_fatal>) at ../../qom/object.c:1402
>>>> #5  0x000000010001c38c in object_property_set_qobject (obj=0x100128b60, name=0x100093f78 "realized", value=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../qom/qom-qobject.c:28
>>>> #6  0x0000000100020e20 in object_property_set_bool (obj=0x100128b60, name=0x100093f78 "realized", value=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../qom/object.c:1472
>>>> #7  0x000000010001612c in qdev_realize (dev=0x100128b60, bus=<optimized out>, errp=0x1000e3af8 <error_fatal>) at ../../hw/core/qdev.c:389
>>>> #8  0x000000010000fb10 in test_static_prop_subprocess () at /home/greg/Work/qemu/qemu-master/include/hw/qdev-core.h:17
>>>> #9  0x00007ffff7e95654 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
>>>> #10 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
>>>> #11 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
>>>> #12 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
>>>> #13 0x00007ffff7e954b8 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0
>>>> #14 0x00007ffff7e959cc in g_test_run_suite () from /lib64/libglib-2.0.so.0
>>>> #15 0x00007ffff7e95a80 in g_test_run () from /lib64/libglib-2.0.so.0
>>>> #16 0x000000010000ecec in main (argc=<optimized out>, argv=<optimized out>) at ../../tests/unit/test-qdev-global-props.c:316
>>>>
>>>> Is there a meson thingy to handle this dependency ?
>>>
>>>    if not have_system
>>>      common_ss.add(files('qdev-machine-stubs.c'))
>>>    endif
>>>
>>> This is not pretty, but better than a generic stubs/qdev-get-machine.c
>>> IMO...
>>>
>>
>> Yeah it isn't that much different,
> 
> I'd expect symbols in stubs/ to be declared weak, while not with this
> approach, so we'd catch clash for incorrect configs. Maybe I'm wrong...

See https://gitlab.com/qemu-project/qemu/-/commit/3bc2f570ec9fc93 for the 
reason why symbols in stubs are *not* declared as weak anymore.

  Thomas

[PATCH 2/2] cpu/core: Fix "help" of CPU core device types

[Greg Kurz]

Calling qdev_get_machine() from a QOM instance_init function is
fragile because we can't be sure the machine object actually
exists. And this happens to break when passing ",help" on the
command line to get the list of properties for a CPU core
device types :

$ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
qemu-system-ppc64: ../../hw/core/machine.c:1290:
 qdev_get_machine: Assertion `machine != NULL' failed.
Aborted (core dumped)

This used to work before QEMU 5.0, but commit 3df261b6676b
unwillingly introduced a subtle regression : the above command
line needs to create an instance but the instance_init function
of the base class calls qdev_get_machine() before
qemu_create_machine() has been called, which is a programming bug.

Use current_machine instead. It is okay to skip the setting of
nr_thread in this case since only its type is displayed.

Reported-by: Thomas Huth <thuth@redhat.com>
Fixes: 3df261b6676b ("softmmu/vl.c: Handle '-cpu help' and '-device help' before 'no default machine'")
Cc: peter.maydell@linaro.org
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/cpu/core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/cpu/core.c b/hw/cpu/core.c
index 92d3b2fbad62..987607515574 100644
--- a/hw/cpu/core.c
+++ b/hw/cpu/core.c
@@ -66,10 +66,16 @@ static void core_prop_set_nr_threads(Object *obj, Visitor *v, const char *name,
 
 static void cpu_core_instance_init(Object *obj)
 {
-    MachineState *ms = MACHINE(qdev_get_machine());
     CPUCore *core = CPU_CORE(obj);
 
-    core->nr_threads = ms->smp.threads;
+    /*
+     * Only '-device something-cpu-core,help' can get us there before
+     * the machine has been created. We don't care to set nr_threads
+     * in this case since it isn't used afterwards.
+     */
+    if (current_machine) {
+        core->nr_threads = current_machine->smp.threads;
+    }
 }
 
 static void cpu_core_class_init(ObjectClass *oc, void *data)
-- 
2.26.3

Re: [PATCH 2/2] cpu/core: Fix "help" of CPU core device types

[Eduardo Habkost]

On Fri, Apr 09, 2021 at 06:03:39PM +0200, Greg Kurz wrote:
> Calling qdev_get_machine() from a QOM instance_init function is
> fragile because we can't be sure the machine object actually
> exists. And this happens to break when passing ",help" on the
> command line to get the list of properties for a CPU core
> device types :
> 
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>  qdev_get_machine: Assertion `machine != NULL' failed.
> Aborted (core dumped)
> 
> This used to work before QEMU 5.0, but commit 3df261b6676b
> unwillingly introduced a subtle regression : the above command
> line needs to create an instance but the instance_init function
> of the base class calls qdev_get_machine() before
> qemu_create_machine() has been called, which is a programming bug.
> 
> Use current_machine instead. It is okay to skip the setting of
> nr_thread in this case since only its type is displayed.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Fixes: 3df261b6676b ("softmmu/vl.c: Handle '-cpu help' and '-device help' before 'no default machine'")
> Cc: peter.maydell@linaro.org
> Signed-off-by: Greg Kurz <groug@kaod.org>

Thanks!  I'm queueing this one (without patch 1/2) for QEMU 6.0.

-- 
Eduardo

Re: [PATCH 2/2] cpu/core: Fix "help" of CPU core device types

[Thomas Huth]

On 09/04/2021 18.03, Greg Kurz wrote:
> Calling qdev_get_machine() from a QOM instance_init function is
> fragile because we can't be sure the machine object actually
> exists. And this happens to break when passing ",help" on the
> command line to get the list of properties for a CPU core
> device types :
> 
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>   qdev_get_machine: Assertion `machine != NULL' failed.
> Aborted (core dumped)
> 
> This used to work before QEMU 5.0, but commit 3df261b6676b
> unwillingly introduced a subtle regression : the above command
> line needs to create an instance but the instance_init function
> of the base class calls qdev_get_machine() before
> qemu_create_machine() has been called, which is a programming bug.
> 
> Use current_machine instead. It is okay to skip the setting of
> nr_thread in this case since only its type is displayed.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Fixes: 3df261b6676b ("softmmu/vl.c: Handle '-cpu help' and '-device help' before 'no default machine'")
> Cc: peter.maydell@linaro.org
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   hw/cpu/core.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> index 92d3b2fbad62..987607515574 100644
> --- a/hw/cpu/core.c
> +++ b/hw/cpu/core.c
> @@ -66,10 +66,16 @@ static void core_prop_set_nr_threads(Object *obj, Visitor *v, const char *name,
>   
>   static void cpu_core_instance_init(Object *obj)
>   {
> -    MachineState *ms = MACHINE(qdev_get_machine());
>       CPUCore *core = CPU_CORE(obj);
>   
> -    core->nr_threads = ms->smp.threads;
> +    /*
> +     * Only '-device something-cpu-core,help' can get us there before
> +     * the machine has been created. We don't care to set nr_threads
> +     * in this case since it isn't used afterwards.
> +     */
> +    if (current_machine) {
> +        core->nr_threads = current_machine->smp.threads;
> +    }
>   }

Ack for QEMU 6.0 to get rid of the crash. But note that using 
current_machine might also be wrong in some cases. It's e.g. possible that 
the user started QEMU with a different machine type (e.g. -M g3beige) and 
then uses some QOM commands to introspect the available devices - in that 
case, this instance_init function will be executed with current_machine 
pointing to a G3 Mac - which is certainly also not what we want here. It 
likely does not crash, but still ... using current_machine or 
qdev_get_machine() in an instance_init() function is just wrong. This 
nr_thread stuff should likely be done in the realize function instead.

  Thomas