| Message ID | 1350414523-8117-8-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eduardo Habkost <ehabkost@redhat.com> writes: > The reset register/unregister code is specific to qemu-system-*, so > isolate it so it can be moved to qdev-system.c. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/qdev-core.c | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/hw/qdev-core.c b/hw/qdev-core.c > index af0af52..a105679 100644 > --- a/hw/qdev-core.c > +++ b/hw/qdev-core.c > @@ -47,6 +47,21 @@ void GCC_WEAK qdev_finalize_vmstate(DeviceState *dev) > { > } > > +static void qbus_register_reset(BusState *bus) > +{ > + if (bus != sysbus_get_default()) { > + /* TODO: once all bus devices are qdevified, > + only reset handler for main_system_bus should be registered here. */ > + qemu_register_reset(qbus_reset_all_fn, bus); > + } > +} > + > +static void qbus_unregister_reset(BusState *bus) > +{ > + assert(bus != sysbus_get_default()); /* main_system_bus is never freed */ > + qemu_unregister_reset(qbus_reset_all_fn, bus); > +} > + Again, I'd suggest stubbing out qemu_[un]register_reset instead of trying to cope with it's users. Regards, Anthony Liguori > const char *qdev_fw_name(DeviceState *dev) > { > DeviceClass *dc = DEVICE_GET_CLASS(dev); > @@ -355,10 +370,8 @@ static void qbus_realize(BusState *bus) > QLIST_INSERT_HEAD(&bus->parent->child_bus, bus, sibling); > bus->parent->num_child_bus++; > object_property_add_child(OBJECT(bus->parent), bus->name, OBJECT(bus), NULL); > - } else if (bus != sysbus_get_default()) { > - /* TODO: once all bus devices are qdevified, > - only reset handler for main_system_bus should be registered here. */ > - qemu_register_reset(qbus_reset_all_fn, bus); > + } else { > + qbus_register_reset(bus); > } > } > > @@ -692,8 +705,7 @@ static void qbus_finalize(Object *obj) > QLIST_REMOVE(bus, sibling); > bus->parent->num_child_bus--; > } else { > - assert(bus != sysbus_get_default()); /* main_system_bus is never freed */ > - qemu_unregister_reset(qbus_reset_all_fn, bus); > + qbus_unregister_reset(bus); > } > g_free((char *)bus->name); > } > -- > 1.7.11.7
On Wed, Oct 17, 2012 at 01:08:23PM -0500, Anthony Liguori wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > The reset register/unregister code is specific to qemu-system-*, so > > isolate it so it can be moved to qdev-system.c. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > hw/qdev-core.c | 24 ++++++++++++++++++------ > > 1 file changed, 18 insertions(+), 6 deletions(-) > > > > diff --git a/hw/qdev-core.c b/hw/qdev-core.c > > index af0af52..a105679 100644 > > --- a/hw/qdev-core.c > > +++ b/hw/qdev-core.c > > @@ -47,6 +47,21 @@ void GCC_WEAK qdev_finalize_vmstate(DeviceState *dev) > > { > > } > > > > +static void qbus_register_reset(BusState *bus) > > +{ > > + if (bus != sysbus_get_default()) { > > + /* TODO: once all bus devices are qdevified, > > + only reset handler for main_system_bus should be registered here. */ > > + qemu_register_reset(qbus_reset_all_fn, bus); > > + } > > +} > > + > > +static void qbus_unregister_reset(BusState *bus) > > +{ > > + assert(bus != sysbus_get_default()); /* main_system_bus is never freed */ > > + qemu_unregister_reset(qbus_reset_all_fn, bus); > > +} > > + > > Again, I'd suggest stubbing out qemu_[un]register_reset instead of > trying to cope with it's users. I agree on the vmstate case (the other patch), but on this case this would require implementing a sysbus_get_default() stub as well. Skipping the whole check for sysbus_get_default() on *-user looks like the right thing to do, to me. Anyway, I am actually wondering if we really need to include the qbus code on *-user. Do you think *-user will eventually use BusState objects too, or it will use only DeviceState objects in the foreseeable future? > > Regards, > > Anthony Liguori > > > const char *qdev_fw_name(DeviceState *dev) > > { > > DeviceClass *dc = DEVICE_GET_CLASS(dev); > > @@ -355,10 +370,8 @@ static void qbus_realize(BusState *bus) > > QLIST_INSERT_HEAD(&bus->parent->child_bus, bus, sibling); > > bus->parent->num_child_bus++; > > object_property_add_child(OBJECT(bus->parent), bus->name, OBJECT(bus), NULL); > > - } else if (bus != sysbus_get_default()) { > > - /* TODO: once all bus devices are qdevified, > > - only reset handler for main_system_bus should be registered here. */ > > - qemu_register_reset(qbus_reset_all_fn, bus); > > + } else { > > + qbus_register_reset(bus); > > } > > } > > > > @@ -692,8 +705,7 @@ static void qbus_finalize(Object *obj) > > QLIST_REMOVE(bus, sibling); > > bus->parent->num_child_bus--; > > } else { > > - assert(bus != sysbus_get_default()); /* main_system_bus is never freed */ > > - qemu_unregister_reset(qbus_reset_all_fn, bus); > > + qbus_unregister_reset(bus); > > } > > g_free((char *)bus->name); > > } > > -- > > 1.7.11.7
Am 17.10.2012 20:32, schrieb Eduardo Habkost: > Anyway, I am actually wondering if we really need to include the qbus > code on *-user. Do you think *-user will eventually use BusState objects > too, or it will use only DeviceState objects in the foreseeable future? My understanding is that we want DeviceState as a consistent parent and its static properties mechanisms. Most of it, including BusState, is going to be unused. At runtime *-user only needs the TCG fields in CPUArchState plus the name -> object mapping mechanisms of the day for initialization. Andreas
On Wed, Oct 17, 2012 at 01:08:23PM -0500, Anthony Liguori wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > The reset register/unregister code is specific to qemu-system-*, so > > isolate it so it can be moved to qdev-system.c. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > hw/qdev-core.c | 24 ++++++++++++++++++------ > > 1 file changed, 18 insertions(+), 6 deletions(-) > > > > diff --git a/hw/qdev-core.c b/hw/qdev-core.c > > index af0af52..a105679 100644 > > --- a/hw/qdev-core.c > > +++ b/hw/qdev-core.c > > @@ -47,6 +47,21 @@ void GCC_WEAK qdev_finalize_vmstate(DeviceState *dev) > > { > > } > > > > +static void qbus_register_reset(BusState *bus) > > +{ > > + if (bus != sysbus_get_default()) { > > + /* TODO: once all bus devices are qdevified, > > + only reset handler for main_system_bus should be registered here. */ > > + qemu_register_reset(qbus_reset_all_fn, bus); > > + } > > +} > > + > > +static void qbus_unregister_reset(BusState *bus) > > +{ > > + assert(bus != sysbus_get_default()); /* main_system_bus is never freed */ > > + qemu_unregister_reset(qbus_reset_all_fn, bus); > > +} > > + > > Again, I'd suggest stubbing out qemu_[un]register_reset instead of > trying to cope with it's users. I was going to implement it the way you suggested. But then I noticed that I _ *-user will need to request the devices to be reset once, too (even if the only devices available are the CPU objects). On the next version, I will move the reset-function list from vl.c to qdev-core.c. Code that uses qdev will need to take care of calling qemu_devices_reset() eventually (qemu-system does that on vl.c *-user will need to that somewhere else). > > Regards, > > Anthony Liguori > > > const char *qdev_fw_name(DeviceState *dev) > > { > > DeviceClass *dc = DEVICE_GET_CLASS(dev); > > @@ -355,10 +370,8 @@ static void qbus_realize(BusState *bus) > > QLIST_INSERT_HEAD(&bus->parent->child_bus, bus, sibling); > > bus->parent->num_child_bus++; > > object_property_add_child(OBJECT(bus->parent), bus->name, OBJECT(bus), NULL); > > - } else if (bus != sysbus_get_default()) { > > - /* TODO: once all bus devices are qdevified, > > - only reset handler for main_system_bus should be registered here. */ > > - qemu_register_reset(qbus_reset_all_fn, bus); > > + } else { > > + qbus_register_reset(bus); > > } > > } > > > > @@ -692,8 +705,7 @@ static void qbus_finalize(Object *obj) > > QLIST_REMOVE(bus, sibling); > > bus->parent->num_child_bus--; > > } else { > > - assert(bus != sysbus_get_default()); /* main_system_bus is never freed */ > > - qemu_unregister_reset(qbus_reset_all_fn, bus); > > + qbus_unregister_reset(bus); > > } > > g_free((char *)bus->name); > > } > > -- > > 1.7.11.7 >
On Tue, Oct 23, 2012 at 12:56:18PM -0200, Eduardo Habkost wrote: > On Wed, Oct 17, 2012 at 01:08:23PM -0500, Anthony Liguori wrote: > > Eduardo Habkost <ehabkost@redhat.com> writes: > > > > > The reset register/unregister code is specific to qemu-system-*, so > > > isolate it so it can be moved to qdev-system.c. > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > > hw/qdev-core.c | 24 ++++++++++++++++++------ > > > 1 file changed, 18 insertions(+), 6 deletions(-) > > > > > > diff --git a/hw/qdev-core.c b/hw/qdev-core.c > > > index af0af52..a105679 100644 > > > --- a/hw/qdev-core.c > > > +++ b/hw/qdev-core.c > > > @@ -47,6 +47,21 @@ void GCC_WEAK qdev_finalize_vmstate(DeviceState *dev) > > > { > > > } > > > > > > +static void qbus_register_reset(BusState *bus) > > > +{ > > > + if (bus != sysbus_get_default()) { > > > + /* TODO: once all bus devices are qdevified, > > > + only reset handler for main_system_bus should be registered here. */ > > > + qemu_register_reset(qbus_reset_all_fn, bus); > > > + } > > > +} > > > + > > > +static void qbus_unregister_reset(BusState *bus) > > > +{ > > > + assert(bus != sysbus_get_default()); /* main_system_bus is never freed */ > > > + qemu_unregister_reset(qbus_reset_all_fn, bus); > > > +} > > > + > > > > Again, I'd suggest stubbing out qemu_[un]register_reset instead of > > trying to cope with it's users. > > I was going to implement it the way you suggested. But then I noticed > that I _ *-user will need to request the devices to be reset once, too > (even if the only devices available are the CPU objects). > > On the next version, I will move the reset-function list from vl.c to > qdev-core.c. Code that uses qdev will need to take care of calling > qemu_devices_reset() eventually (qemu-system does that on vl.c *-user > will need to that somewhere else). I don't usually reply to myself to correct typos, but the message above was almost impossible to parse. So, rewriting it: I was going to implement it the way you suggested. But then I noticed that *-user will need to request the devices to be reset once, too (even if the only devices available are the CPU objects). On the next version, I will move the reset-function list from vl.c to qdev-core.c. Code that uses qdev will need to take care of calling qemu_devices_reset() eventually (qemu-system does that on vl.c, and *-user will need to that somewhere else). > > > > > Regards, > > > > Anthony Liguori > > > > > const char *qdev_fw_name(DeviceState *dev) > > > { > > > DeviceClass *dc = DEVICE_GET_CLASS(dev); > > > @@ -355,10 +370,8 @@ static void qbus_realize(BusState *bus) > > > QLIST_INSERT_HEAD(&bus->parent->child_bus, bus, sibling); > > > bus->parent->num_child_bus++; > > > object_property_add_child(OBJECT(bus->parent), bus->name, OBJECT(bus), NULL); > > > - } else if (bus != sysbus_get_default()) { > > > - /* TODO: once all bus devices are qdevified, > > > - only reset handler for main_system_bus should be registered here. */ > > > - qemu_register_reset(qbus_reset_all_fn, bus); > > > + } else { > > > + qbus_register_reset(bus); > > > } > > > } > > > > > > @@ -692,8 +705,7 @@ static void qbus_finalize(Object *obj) > > > QLIST_REMOVE(bus, sibling); > > > bus->parent->num_child_bus--; > > > } else { > > > - assert(bus != sysbus_get_default()); /* main_system_bus is never freed */ > > > - qemu_unregister_reset(qbus_reset_all_fn, bus); > > > + qbus_unregister_reset(bus); > > > } > > > g_free((char *)bus->name); > > > } > > > -- > > > 1.7.11.7 > > > > -- > Eduardo >
diff --git a/hw/qdev-core.c b/hw/qdev-core.c index af0af52..a105679 100644 --- a/hw/qdev-core.c +++ b/hw/qdev-core.c @@ -47,6 +47,21 @@ void GCC_WEAK qdev_finalize_vmstate(DeviceState *dev) { } +static void qbus_register_reset(BusState *bus) +{ + if (bus != sysbus_get_default()) { + /* TODO: once all bus devices are qdevified, + only reset handler for main_system_bus should be registered here. */ + qemu_register_reset(qbus_reset_all_fn, bus); + } +} + +static void qbus_unregister_reset(BusState *bus) +{ + assert(bus != sysbus_get_default()); /* main_system_bus is never freed */ + qemu_unregister_reset(qbus_reset_all_fn, bus); +} + const char *qdev_fw_name(DeviceState *dev) { DeviceClass *dc = DEVICE_GET_CLASS(dev); @@ -355,10 +370,8 @@ static void qbus_realize(BusState *bus) QLIST_INSERT_HEAD(&bus->parent->child_bus, bus, sibling); bus->parent->num_child_bus++; object_property_add_child(OBJECT(bus->parent), bus->name, OBJECT(bus), NULL); - } else if (bus != sysbus_get_default()) { - /* TODO: once all bus devices are qdevified, - only reset handler for main_system_bus should be registered here. */ - qemu_register_reset(qbus_reset_all_fn, bus); + } else { + qbus_register_reset(bus); } } @@ -692,8 +705,7 @@ static void qbus_finalize(Object *obj) QLIST_REMOVE(bus, sibling); bus->parent->num_child_bus--; } else { - assert(bus != sysbus_get_default()); /* main_system_bus is never freed */ - qemu_unregister_reset(qbus_reset_all_fn, bus); + qbus_unregister_reset(bus); } g_free((char *)bus->name); }
The reset register/unregister code is specific to qemu-system-*, so isolate it so it can be moved to qdev-system.c. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/qdev-core.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)