mbox series

[0/4] Remove some qdev_get_machine() calls from CONFIG_USER_ONLY

Message ID 20190425200051.19906-1-ehabkost@redhat.com
Headers show
Series Remove some qdev_get_machine() calls from CONFIG_USER_ONLY | expand

Message

Eduardo Habkost April 25, 2019, 8 p.m. UTC 
This series moves some qdev code outside qdev.o, so it can be
compiled only in CONFIG_SOFTMMU.

The code being moved includes two qdev_get_machine() calls, so
this will make it easier to move qdev_get_machine() to
CONFIG_SOFTMMU later.

After this series, there's one remaining qdev_get_machine() call
that seems more difficult to remove:

    static void device_set_realized(Object *obj, bool value, Error **errp)
    {
        /* [...] */
        if (!obj->parent) {
            gchar *name = g_strdup_printf("device[%d]", unattached_count++);

            object_property_add_child(container_get(qdev_get_machine(),
                                                    "/unattached"),
                                      name, obj, &error_abort);
            unattached_parent = true;
            g_free(name);
        }
        /* [...] */
    }

This one is tricky because on system emulation mode it needs
"/machine" to already exist, but in user-only mode it needs to
implicitly create a "/machine" container.

Eduardo Habkost (4):
  machine: Move gpio code to hw/core/gpio.c
  move qdev hotplug code to qdev-hotplug.c
  qdev: Don't compile hotplug code in user-mode emulation
  qdev-hotplug: Don't check type of qdev_get_machine()

 hw/core/bus.c                |  11 --
 hw/core/gpio.c               | 206 ++++++++++++++++++++++++++++++++
 hw/core/qdev-hotplug-stubs.c |  44 +++++++
 hw/core/qdev-hotplug.c       |  64 ++++++++++
 hw/core/qdev.c               | 219 -----------------------------------
 hw/core/Makefile.objs        |   5 +-
 tests/Makefile.include       |   3 +-
 7 files changed, 320 insertions(+), 232 deletions(-)
 create mode 100644 hw/core/gpio.c
 create mode 100644 hw/core/qdev-hotplug-stubs.c
 create mode 100644 hw/core/qdev-hotplug.c

Comments

no-reply@patchew.org April 25, 2019, 8:09 p.m. UTC | #1 
Patchew URL: https://patchew.org/QEMU/20190425200051.19906-1-ehabkost@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20190425200051.19906-1-ehabkost@redhat.com
Subject: [Qemu-devel] [PATCH 0/4] Remove some qdev_get_machine() calls from CONFIG_USER_ONLY

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20190424004700.12766-1-richardw.yang@linux.intel.com -> patchew/20190424004700.12766-1-richardw.yang@linux.intel.com
 * [new tag]               patchew/20190425200051.19906-1-ehabkost@redhat.com -> patchew/20190425200051.19906-1-ehabkost@redhat.com
Switched to a new branch 'test'
c5c77ea2cc qdev-hotplug: Don't check type of qdev_get_machine()
854ded711d qdev: Don't compile hotplug code in user-mode emulation
1bd33307be move qdev hotplug code to qdev-hotplug.c
02b1f5db6f machine: Move gpio code to hw/core/gpio.c

=== OUTPUT BEGIN ===
1/4 Checking commit 02b1f5db6fcd (machine: Move gpio code to hw/core/gpio.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#26: 
new file mode 100644

WARNING: Block comments use a leading /* on a separate line
#60: FILE: hw/core/gpio.c:30:
+        /* NULL is a valid and matchable name, otherwise do a normal

WARNING: Block comments use a leading /* on a separate line
#156: FILE: hw/core/gpio.c:126:
+        /* We need a name for object_property_set_link to work.  If the

ERROR: "foo * bar" should be "foo *bar"
#205: FILE: hw/core/gpio.c:175:
+void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)

total: 1 errors, 3 warnings, 403 lines checked

Patch 1/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/4 Checking commit 1bd33307be11 (move qdev hotplug code to qdev-hotplug.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#49: 
new file mode 100644

total: 0 errors, 1 warnings, 134 lines checked

Patch 2/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/4 Checking commit 854ded711daa (qdev: Don't compile hotplug code in user-mode emulation)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#48: 
new file mode 100644

total: 0 errors, 1 warnings, 77 lines checked

Patch 3/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/4 Checking commit c5c77ea2ccde (qdev-hotplug: Don't check type of qdev_get_machine())
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190425200051.19906-1-ehabkost@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Like Xu April 26, 2019, 8:55 a.m. UTC | #2 
On 2019/4/26 4:00, Eduardo Habkost wrote:
> This series moves some qdev code outside qdev.o, so it can be
> compiled only in CONFIG_SOFTMMU.
> 
> The code being moved includes two qdev_get_machine() calls, so
> this will make it easier to move qdev_get_machine() to
> CONFIG_SOFTMMU later.
> 
> After this series, there's one remaining qdev_get_machine() call
> that seems more difficult to remove:
> 
>      static void device_set_realized(Object *obj, bool value, Error **errp)
>      {
>          /* [...] */
>          if (!obj->parent) {
>              gchar *name = g_strdup_printf("device[%d]", unattached_count++);
> 
>              object_property_add_child(container_get(qdev_get_machine(),
>                                                      "/unattached"),
>                                        name, obj, &error_abort);
>              unattached_parent = true;
>              g_free(name);
>          }
>          /* [...] */
>      }
> 

I may have an experimental patch to fix device_set_realized issue:

1. in qdev_get_machine():
replace
	dev = container_get(object_get_root(), "/machine");
with
	dev = object_resolve_path("/machine", NULL);

2. in device_set_realized():

Using
	Object *container = qdev_get_machine() ?
		qdev_get_machine() : object_get_root();
and pass it to
	object_property_add_child(
		container_get(container, "/unattached"),
		name, obj, &error_abort);

With this fix, we could say the qdev_get_machine() does
return the "/machine" object (or null) not a confused "/container".

We could continue to use qdev_get_machine() in system emulation mode,
getting rid of its surprising side effect as Markus said.

The return value of qdev_get_machine() in user-only mode
is the same object returned by object_get_root(),
so no semantic changes.


> This one is tricky because on system emulation mode it needs
> "/machine" to already exist, but in user-only mode it needs to
> implicitly create a "/machine" container.
> 
> Eduardo Habkost (4):
>    machine: Move gpio code to hw/core/gpio.c
>    move qdev hotplug code to qdev-hotplug.c
>    qdev: Don't compile hotplug code in user-mode emulation
>    qdev-hotplug: Don't check type of qdev_get_machine()
> 
>   hw/core/bus.c                |  11 --
>   hw/core/gpio.c               | 206 ++++++++++++++++++++++++++++++++
>   hw/core/qdev-hotplug-stubs.c |  44 +++++++
>   hw/core/qdev-hotplug.c       |  64 ++++++++++
>   hw/core/qdev.c               | 219 -----------------------------------
>   hw/core/Makefile.objs        |   5 +-
>   tests/Makefile.include       |   3 +-
>   7 files changed, 320 insertions(+), 232 deletions(-)
>   create mode 100644 hw/core/gpio.c
>   create mode 100644 hw/core/qdev-hotplug-stubs.c
>   create mode 100644 hw/core/qdev-hotplug.c
>
Eduardo Habkost May 2, 2019, 8:24 p.m. UTC | #3 
On Fri, Apr 26, 2019 at 04:55:17PM +0800, Like Xu wrote:
> On 2019/4/26 4:00, Eduardo Habkost wrote:
> > This series moves some qdev code outside qdev.o, so it can be
> > compiled only in CONFIG_SOFTMMU.
> > 
> > The code being moved includes two qdev_get_machine() calls, so
> > this will make it easier to move qdev_get_machine() to
> > CONFIG_SOFTMMU later.
> > 
> > After this series, there's one remaining qdev_get_machine() call
> > that seems more difficult to remove:
> > 
> >      static void device_set_realized(Object *obj, bool value, Error **errp)
> >      {
> >          /* [...] */
> >          if (!obj->parent) {
> >              gchar *name = g_strdup_printf("device[%d]", unattached_count++);
> > 
> >              object_property_add_child(container_get(qdev_get_machine(),
> >                                                      "/unattached"),
> >                                        name, obj, &error_abort);
> >              unattached_parent = true;
> >              g_free(name);
> >          }
> >          /* [...] */
> >      }
> > 
> 
> I may have an experimental patch to fix device_set_realized issue:
> 
> 1. in qdev_get_machine():
> replace
> 	dev = container_get(object_get_root(), "/machine");
> with
> 	dev = object_resolve_path("/machine", NULL);
> 
> 2. in device_set_realized():
> 
> Using
> 	Object *container = qdev_get_machine() ?
> 		qdev_get_machine() : object_get_root();
> and pass it to
> 	object_property_add_child(
> 		container_get(container, "/unattached"),
> 		name, obj, &error_abort);

I wouldn't like to call qdev_get_machine() here (see below[1]),
but trying "/machine" first and falling back to object_get_root()
sounds good, for two reasons:

* It won't require "/machine" to exist at all, on *-user;
* It will allow machines to create unattached devices on
  instance_init without crashing (which is not possible today).

> 
> With this fix, we could say the qdev_get_machine() does
> return the "/machine" object (or null) not a confused "/container".
> 
> We could continue to use qdev_get_machine() in system emulation mode,
> getting rid of its surprising side effect as Markus said.
> 
> The return value of qdev_get_machine() in user-only mode
> is the same object returned by object_get_root(),
> so no semantic changes.

[1] I wouldn't like to have a different qdev_get_machine()
function in user-only mode.  I would like to simply not call
qdev_get_machine() at all unless we're in system emulation mode.
Markus Armbruster May 6, 2019, 2:34 p.m. UTC | #4 
Eduardo Habkost <ehabkost@redhat.com> writes:

> This series moves some qdev code outside qdev.o, so it can be
> compiled only in CONFIG_SOFTMMU.
>
> The code being moved includes two qdev_get_machine() calls, so
> this will make it easier to move qdev_get_machine() to
> CONFIG_SOFTMMU later.
>
> After this series, there's one remaining qdev_get_machine() call
> that seems more difficult to remove:
>
>     static void device_set_realized(Object *obj, bool value, Error **errp)
>     {
>         /* [...] */
>         if (!obj->parent) {
>             gchar *name = g_strdup_printf("device[%d]", unattached_count++);
>
>             object_property_add_child(container_get(qdev_get_machine(),
>                                                     "/unattached"),
>                                       name, obj, &error_abort);
>             unattached_parent = true;
>             g_free(name);
>         }
>         /* [...] */
>     }
>
> This one is tricky because on system emulation mode it needs
> "/machine" to already exist, but in user-only mode it needs to
> implicitly create a "/machine" container.

The patches look good to me on a glance.  Looking forward to v2.
Philippe Mathieu-Daudé Nov. 1, 2021, 6:27 p.m. UTC | #5 
On 4/25/19 22:00, Eduardo Habkost wrote:

> Eduardo Habkost (4):
>   machine: Move gpio code to hw/core/gpio.c
>   move qdev hotplug code to qdev-hotplug.c
>   qdev: Don't compile hotplug code in user-mode emulation
>   qdev-hotplug: Don't check type of qdev_get_machine()

Patch 1 queued to machine-next.