| Message ID | 20241121192202.4155849-9-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QOM: container_get() removal | expand |
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>
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) 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,
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,
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;
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(-)