Currently, qdev_get_machine() has a slight misuse on container_get(), as
the helper says "get a container" but in reality the goal is to get the
machine object. It is still a "container" but not strictly.
Note that it _may_ get a container (at "/machine") in our current unit test
of test-qdev-global-props.c before all these changes, but it's probably
unexpected and worked by accident.
Switch to an explicit object_resolve_path_component(), with a side benefit
that qdev_get_machine() can happen a lot, and we don't need to split the
string ("/machine") every time. This also paves way for making the helper
container_get() never try to return a non-container at all.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
hw/core/qdev.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 5f13111b77..b622be15ee 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
static Object *dev;
if (dev == NULL) {
- dev = container_get(object_get_root(), "/machine");
+ dev = object_resolve_path_component(object_get_root(), "machine");
+ /*
+ * Any call to this function before machine is created is treated
+ * as a programming error as of now.
+ */
+ assert(dev);
}
return dev;
--
2.45.0On 21/11/24 20:21, Peter Xu wrote:
> Currently, qdev_get_machine() has a slight misuse on container_get(), as
> the helper says "get a container" but in reality the goal is to get the
> machine object. It is still a "container" but not strictly.
>
> Note that it _may_ get a container (at "/machine") in our current unit test
> of test-qdev-global-props.c before all these changes, but it's probably
> unexpected and worked by accident.
>
> Switch to an explicit object_resolve_path_component(), with a side benefit
> that qdev_get_machine() can happen a lot, and we don't need to split the
> string ("/machine") every time. This also paves way for making the helper
> container_get() never try to return a non-container at all.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> hw/core/qdev.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 5f13111b77..b622be15ee 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
> static Object *dev;
>
> if (dev == NULL) {
> - dev = container_get(object_get_root(), "/machine");
> + dev = object_resolve_path_component(object_get_root(), "machine");
> + /*
> + * Any call to this function before machine is created is treated
> + * as a programming error as of now.
> + */
> + assert(dev);
This fails for user-emulation:
./qemu-x86_64 /bin/echo foo
qemu-x86_64: ../../hw/core/qdev.c:825: qdev_get_machine: Assertion `dev'
failed.
Aborted (core dumped)
We need to skip this test for user emulation, but this file is in
hwcore_ss[] so the CONFIG_USER_ONLY definitions is not available.
Any simple enough idea to not block this?
> }
>
> return dev;
On 19/12/24 19:20, Philippe Mathieu-Daudé wrote:
> On 21/11/24 20:21, Peter Xu wrote:
>> Currently, qdev_get_machine() has a slight misuse on container_get(), as
>> the helper says "get a container" but in reality the goal is to get the
>> machine object. It is still a "container" but not strictly.
>>
>> Note that it _may_ get a container (at "/machine") in our current unit
>> test
>> of test-qdev-global-props.c before all these changes, but it's probably
>> unexpected and worked by accident.
>>
>> Switch to an explicit object_resolve_path_component(), with a side
>> benefit
>> that qdev_get_machine() can happen a lot, and we don't need to split the
>> string ("/machine") every time. This also paves way for making the
>> helper
>> container_get() never try to return a non-container at all.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>> hw/core/qdev.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 5f13111b77..b622be15ee 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
>> static Object *dev;
>> if (dev == NULL) {
>> - dev = container_get(object_get_root(), "/machine");
>> + dev = object_resolve_path_component(object_get_root(),
>> "machine");
>> + /*
>> + * Any call to this function before machine is created is
>> treated
>> + * as a programming error as of now.
>> + */
>> + assert(dev);
>
> This fails for user-emulation:
>
> ./qemu-x86_64 /bin/echo foo
> qemu-x86_64: ../../hw/core/qdev.c:825: qdev_get_machine: Assertion `dev'
> failed.
> Aborted (core dumped)
(gdb) bt
#5 0x00007ffff747171b in __assert_fail_base (fmt=0x7ffff7626130
"%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x555555725150
"dev",
file=0x55555571aff9 "../../hw/core/qdev.c", line=824,
function=<optimized out>) at ./assert/assert.c:92
#8 0x000055555565e400 in qdev_get_machine () at ../../hw/core/qdev.c:824
#9 machine_get_container (name=0x55555571b052 "unattached") at
../../hw/core/qdev.c:834
#10 0x000055555565ea2d in device_set_realized (obj=0x5555558b6760,
value=<optimized out>, errp=0x7fffffffdb50) at ../../hw/core/qdev.c:479
#11 0x000055555566181a in property_set_bool (obj=0x5555558b6760,
v=<optimized out>, name=<optimized out>, opaque=0x555555813350,
errp=0x7fffffffdb50)
at ../../qom/object.c:2375
#12 0x00005555556649f8 in object_property_set
(obj=obj@entry=0x5555558b6760, name=name@entry=0x55555571b03e
"realized", v=v@entry=0x5555558c0680,
errp=errp@entry=0x7fffffffdb50) at ../../qom/object.c:1450
#13 0x0000555555668754 in object_property_set_qobject
(obj=obj@entry=0x5555558b6760, name=name@entry=0x55555571b03e
"realized", value=value@entry=0x5555558be490,
errp=errp@entry=0x7fffffffdb50) at ../../qom/qom-qobject.c:28
#14 0x00005555556650c9 in object_property_set_bool (obj=0x5555558b6760,
name=name@entry=0x55555571b03e "realized", value=value@entry=true,
errp=errp@entry=0x7fffffffdb50) at ../../qom/object.c:1520
#15 0x000055555565dd52 in qdev_realize (dev=<optimized out>,
bus=bus@entry=0x0, errp=errp@entry=0x7fffffffdb50) at
../../hw/core/qdev.c:276
#16 0x0000555555593dc9 in cpu_create (typename=<optimized out>) at
../../hw/core/cpu-common.c:61
#17 0x00005555555925de in main (argc=3, argv=0x7fffffffe308,
envp=<optimized out>) at ../../linux-user/main.c:823
>
> We need to skip this test for user emulation, but this file is in
> hwcore_ss[] so the CONFIG_USER_ONLY definitions is not available.
>
> Any simple enough idea to not block this?
>
>> }
>> return dev;
>
On 19/12/24 19:27, Philippe Mathieu-Daudé wrote:
> On 19/12/24 19:20, Philippe Mathieu-Daudé wrote:
>> On 21/11/24 20:21, Peter Xu wrote:
>>> Currently, qdev_get_machine() has a slight misuse on container_get(), as
>>> the helper says "get a container" but in reality the goal is to get the
>>> machine object. It is still a "container" but not strictly.
>>>
>>> Note that it _may_ get a container (at "/machine") in our current
>>> unit test
>>> of test-qdev-global-props.c before all these changes, but it's probably
>>> unexpected and worked by accident.
>>>
>>> Switch to an explicit object_resolve_path_component(), with a side
>>> benefit
>>> that qdev_get_machine() can happen a lot, and we don't need to split the
>>> string ("/machine") every time. This also paves way for making the
>>> helper
>>> container_get() never try to return a non-container at all.
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>> hw/core/qdev.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 5f13111b77..b622be15ee 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
>>> static Object *dev;
>>> if (dev == NULL) {
>>> - dev = container_get(object_get_root(), "/machine");
>>> + dev = object_resolve_path_component(object_get_root(),
>>> "machine");
>>> + /*
>>> + * Any call to this function before machine is created is
>>> treated
>>> + * as a programming error as of now.
>>> + */
>>> + assert(dev);
>>
>> This fails for user-emulation:
>>
>> ./qemu-x86_64 /bin/echo foo
>> qemu-x86_64: ../../hw/core/qdev.c:825: qdev_get_machine: Assertion
>> `dev' failed.
OK so I guess I might have found a "fix" which is to simply not
call qdev_get_machine() for user emulation, but this involves some
invasive refactoring -- so will take time --.
I'm dropping this series for now, planning to merge it again on top
of my refactor once it is ready. Any clever / simpler fix is
obviously welcomed first.
Regards,
Phil.
>> Aborted (core dumped)
>
> (gdb) bt
> #5 0x00007ffff747171b in __assert_fail_base (fmt=0x7ffff7626130
> "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x555555725150
> "dev",
> file=0x55555571aff9 "../../hw/core/qdev.c", line=824,
> function=<optimized out>) at ./assert/assert.c:92
> #8 0x000055555565e400 in qdev_get_machine () at ../../hw/core/qdev.c:824
> #9 machine_get_container (name=0x55555571b052 "unattached") at ../../
> hw/core/qdev.c:834
> #10 0x000055555565ea2d in device_set_realized (obj=0x5555558b6760,
> value=<optimized out>, errp=0x7fffffffdb50) at ../../hw/core/qdev.c:479
> #11 0x000055555566181a in property_set_bool (obj=0x5555558b6760,
> v=<optimized out>, name=<optimized out>, opaque=0x555555813350,
> errp=0x7fffffffdb50)
> at ../../qom/object.c:2375
> #12 0x00005555556649f8 in object_property_set
> (obj=obj@entry=0x5555558b6760, name=name@entry=0x55555571b03e
> "realized", v=v@entry=0x5555558c0680,
> errp=errp@entry=0x7fffffffdb50) at ../../qom/object.c:1450
> #13 0x0000555555668754 in object_property_set_qobject
> (obj=obj@entry=0x5555558b6760, name=name@entry=0x55555571b03e
> "realized", value=value@entry=0x5555558be490,
> errp=errp@entry=0x7fffffffdb50) at ../../qom/qom-qobject.c:28
> #14 0x00005555556650c9 in object_property_set_bool (obj=0x5555558b6760,
> name=name@entry=0x55555571b03e "realized", value=value@entry=true,
> errp=errp@entry=0x7fffffffdb50) at ../../qom/object.c:1520
> #15 0x000055555565dd52 in qdev_realize (dev=<optimized out>,
> bus=bus@entry=0x0, errp=errp@entry=0x7fffffffdb50) at ../../hw/core/
> qdev.c:276
> #16 0x0000555555593dc9 in cpu_create (typename=<optimized out>)
> at ../../hw/core/cpu-common.c:61
> #17 0x00005555555925de in main (argc=3, argv=0x7fffffffe308,
> envp=<optimized out>) at ../../linux-user/main.c:823
>
>
>>
>> We need to skip this test for user emulation, but this file is in
>> hwcore_ss[] so the CONFIG_USER_ONLY definitions is not available.
>>
>> Any simple enough idea to not block this?
>>
>>> }
>>> return dev;
>>
>
On Fri, Dec 20, 2024 at 12:25:44PM +0100, Philippe Mathieu-Daudé wrote:
> On 19/12/24 19:27, Philippe Mathieu-Daudé wrote:
> > On 19/12/24 19:20, Philippe Mathieu-Daudé wrote:
> > > On 21/11/24 20:21, Peter Xu wrote:
> > > > Currently, qdev_get_machine() has a slight misuse on container_get(), as
> > > > the helper says "get a container" but in reality the goal is to get the
> > > > machine object. It is still a "container" but not strictly.
> > > >
> > > > Note that it _may_ get a container (at "/machine") in our
> > > > current unit test
> > > > of test-qdev-global-props.c before all these changes, but it's probably
> > > > unexpected and worked by accident.
> > > >
> > > > Switch to an explicit object_resolve_path_component(), with a
> > > > side benefit
> > > > that qdev_get_machine() can happen a lot, and we don't need to split the
> > > > string ("/machine") every time. This also paves way for making
> > > > the helper
> > > > container_get() never try to return a non-container at all.
> > > >
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > > hw/core/qdev.c | 7 ++++++-
> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > index 5f13111b77..b622be15ee 100644
> > > > --- a/hw/core/qdev.c
> > > > +++ b/hw/core/qdev.c
> > > > @@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
> > > > static Object *dev;
> > > > if (dev == NULL) {
> > > > - dev = container_get(object_get_root(), "/machine");
> > > > + dev = object_resolve_path_component(object_get_root(),
> > > > "machine");
> > > > + /*
> > > > + * Any call to this function before machine is created
> > > > is treated
> > > > + * as a programming error as of now.
> > > > + */
> > > > + assert(dev);
> > >
> > > This fails for user-emulation:
> > >
> > > ./qemu-x86_64 /bin/echo foo
> > > qemu-x86_64: ../../hw/core/qdev.c:825: qdev_get_machine: Assertion
> > > `dev' failed.
>
> OK so I guess I might have found a "fix" which is to simply not
> call qdev_get_machine() for user emulation, but this involves some
> invasive refactoring -- so will take time --.
Thanks for taking a look, Phil. Yes this sounds clean.
>
> I'm dropping this series for now, planning to merge it again on top
> of my refactor once it is ready. Any clever / simpler fix is
> obviously welcomed first.
I initially thought about this, which could also be clean but I then
noticed LINUX_USER is poisoned..
===8<===
diff --git a/qom/object.c b/qom/object.c
index 58897a79a7..da26e8d69b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1729,7 +1729,19 @@ const char *object_property_get_type(Object *obj, const char *name, Error **errp
return prop->type;
}
+/*
+ * Create all QEMU default containers.
+ *
+ * For system emulations, "machine" and its sub-containers are only created
+ * when machine initializes (qemu_create_machine()).
+ *
+ * For user emulations, create "machine" before hand to make qdev realize()
+ * work by default.
+ */
static const char *const root_containers[] = {
+#ifdef CONFIG_LINUX_USER
+ "machine",
+#endif
"chardevs",
"objects",
"backend"
@@ -1740,10 +1752,6 @@ static Object *object_root_initialize(void)
Object *root = object_new(TYPE_CONTAINER);
int i;
- /*
- * Create all QEMU system containers. "machine" and its sub-containers
- * are only created when machine initializes (qemu_create_machine()).
- */
for (i = 0; i < ARRAY_SIZE(root_containers); i++) {
object_property_add_new_container(root, root_containers[i]);
}
===8<===
Maybe we could still move it somewhere that LINUX_USER is not poisoned
(plus "unattached" be created too, more below)?
OTOH, this works for me:
===8<===
diff --git a/linux-user/main.c b/linux-user/main.c
index b09af8d436..009b7695f2 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -819,6 +819,11 @@ int main(int argc, char **argv, char **envp)
set_preferred_target_page_bits(ctz32(host_page_size));
finalize_target_page_bits();
+ Object *fake_obj = object_property_add_new_container(object_get_root(),
+ "machine");
+ object_property_add_new_container(fake_obj, "unattached");
+
cpu = cpu_create(cpu_type);
env = cpu_env(cpu);
cpu_reset(cpu);
===8<===
So we need both "/machine" and "/machine/unattached" so far to make
linux-user work. Not sure if bsd-user/main.c needs similar care, but none
of these look as clean.
Thanks,
--
Peter Xu
On 20/12/24 18:24, Peter Xu wrote:
> On Fri, Dec 20, 2024 at 12:25:44PM +0100, Philippe Mathieu-Daudé wrote:
>> On 19/12/24 19:27, Philippe Mathieu-Daudé wrote:
>>> On 19/12/24 19:20, Philippe Mathieu-Daudé wrote:
>>>> On 21/11/24 20:21, Peter Xu wrote:
>>>>> Currently, qdev_get_machine() has a slight misuse on container_get(), as
>>>>> the helper says "get a container" but in reality the goal is to get the
>>>>> machine object. It is still a "container" but not strictly.
>>>>>
>>>>> Note that it _may_ get a container (at "/machine") in our
>>>>> current unit test
>>>>> of test-qdev-global-props.c before all these changes, but it's probably
>>>>> unexpected and worked by accident.
>>>>>
>>>>> Switch to an explicit object_resolve_path_component(), with a
>>>>> side benefit
>>>>> that qdev_get_machine() can happen a lot, and we don't need to split the
>>>>> string ("/machine") every time. This also paves way for making
>>>>> the helper
>>>>> container_get() never try to return a non-container at all.
>>>>>
>>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>>>> ---
>>>>> hw/core/qdev.c | 7 ++++++-
>>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>>> index 5f13111b77..b622be15ee 100644
>>>>> --- a/hw/core/qdev.c
>>>>> +++ b/hw/core/qdev.c
>>>>> @@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
>>>>> static Object *dev;
>>>>> if (dev == NULL) {
>>>>> - dev = container_get(object_get_root(), "/machine");
>>>>> + dev = object_resolve_path_component(object_get_root(),
>>>>> "machine");
>>>>> + /*
>>>>> + * Any call to this function before machine is created
>>>>> is treated
>>>>> + * as a programming error as of now.
>>>>> + */
>>>>> + assert(dev);
>>>>
>>>> This fails for user-emulation:
>>>>
>>>> ./qemu-x86_64 /bin/echo foo
>>>> qemu-x86_64: ../../hw/core/qdev.c:825: qdev_get_machine: Assertion
>>>> `dev' failed.
>>
>> OK so I guess I might have found a "fix" which is to simply not
>> call qdev_get_machine() for user emulation, but this involves some
>> invasive refactoring -- so will take time --.
>
> Thanks for taking a look, Phil. Yes this sounds clean.
>
>>
>> I'm dropping this series for now, planning to merge it again on top
>> of my refactor once it is ready. Any clever / simpler fix is
>> obviously welcomed first.
>
> I initially thought about this, which could also be clean but I then
> noticed LINUX_USER is poisoned..
>
> ===8<===
> diff --git a/qom/object.c b/qom/object.c
> index 58897a79a7..da26e8d69b 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1729,7 +1729,19 @@ const char *object_property_get_type(Object *obj, const char *name, Error **errp
> return prop->type;
> }
>
> +/*
> + * Create all QEMU default containers.
> + *
> + * For system emulations, "machine" and its sub-containers are only created
> + * when machine initializes (qemu_create_machine()).
> + *
> + * For user emulations, create "machine" before hand to make qdev realize()
> + * work by default.
> + */
> static const char *const root_containers[] = {
> +#ifdef CONFIG_LINUX_USER
> + "machine",
> +#endif
> "chardevs",
> "objects",
> "backend"
> @@ -1740,10 +1752,6 @@ static Object *object_root_initialize(void)
> Object *root = object_new(TYPE_CONTAINER);
> int i;
>
> - /*
> - * Create all QEMU system containers. "machine" and its sub-containers
> - * are only created when machine initializes (qemu_create_machine()).
> - */
> for (i = 0; i < ARRAY_SIZE(root_containers); i++) {
> object_property_add_new_container(root, root_containers[i]);
> }
> ===8<===
>
> Maybe we could still move it somewhere that LINUX_USER is not poisoned
> (plus "unattached" be created too, more below)?
>
> OTOH, this works for me:
>
> ===8<===
> diff --git a/linux-user/main.c b/linux-user/main.c
> index b09af8d436..009b7695f2 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -819,6 +819,11 @@ int main(int argc, char **argv, char **envp)
> set_preferred_target_page_bits(ctz32(host_page_size));
> finalize_target_page_bits();
>
> + Object *fake_obj = object_property_add_new_container(object_get_root(),
> + "machine");
> + object_property_add_new_container(fake_obj, "unattached");
> +
> cpu = cpu_create(cpu_type);
> env = cpu_env(cpu);
> cpu_reset(cpu);
> ===8<===
I like it, simple enough, allowing to remove container_get() now.
>
> So we need both "/machine" and "/machine/unattached" so far to make
> linux-user work. Not sure if bsd-user/main.c needs similar care, but none
> of these look as clean.
Maybe add a common method in hw/core/qdev-user.c?
qemu_create_machine() or qdev_create_fake_machine()?
On Fri, Dec 20, 2024 at 10:38:40PM +0100, Philippe Mathieu-Daudé wrote: > > OTOH, this works for me: > > > > ===8<=== > > diff --git a/linux-user/main.c b/linux-user/main.c > > index b09af8d436..009b7695f2 100644 > > --- a/linux-user/main.c > > +++ b/linux-user/main.c > > @@ -819,6 +819,11 @@ int main(int argc, char **argv, char **envp) > > set_preferred_target_page_bits(ctz32(host_page_size)); > > finalize_target_page_bits(); > > + Object *fake_obj = object_property_add_new_container(object_get_root(), > > + "machine"); > > + object_property_add_new_container(fake_obj, "unattached"); > > + > > cpu = cpu_create(cpu_type); > > env = cpu_env(cpu); > > cpu_reset(cpu); > > ===8<=== > > I like it, simple enough, allowing to remove container_get() now. > > > > > So we need both "/machine" and "/machine/unattached" so far to make > > linux-user work. Not sure if bsd-user/main.c needs similar care, but none > > of these look as clean. > > Maybe add a common method in hw/core/qdev-user.c? > qemu_create_machine() or qdev_create_fake_machine()? Sounds good at least to me. I'd vote for the latter to reflect it's unreal. Thanks, -- Peter Xu
On 21/11/24 20:21, Peter Xu wrote:
> Currently, qdev_get_machine() has a slight misuse on container_get(), as
> the helper says "get a container" but in reality the goal is to get the
> machine object. It is still a "container" but not strictly.
>
> Note that it _may_ get a container (at "/machine") in our current unit test
> of test-qdev-global-props.c before all these changes, but it's probably
> unexpected and worked by accident.
>
> Switch to an explicit object_resolve_path_component(), with a side benefit
> that qdev_get_machine() can happen a lot, and we don't need to split the
> string ("/machine") every time. This also paves way for making the helper
> container_get() never try to return a non-container at all.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> hw/core/qdev.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
© 2016 - 2026 Red Hat, Inc.