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 <pet...@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
