| Message ID | 20190926151733.25349-12-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw: Convert various reset() handler to DeviceReset | expand |
On 9/26/19 5:17 PM, Philippe Mathieu-Daudé wrote: > Convert the reset handler into a proper Device reset method. Marc-André noticed this one is incorrect, because while being QDEV it is not connected to a QBUS. Maybe we can add a Device::unconnected property, and when set, the parent realize() calls 'qemu_register_reset(dev->reset, dev);'? This might look the same, but at least Devices implementations could stop to use this function... > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/misc/vmcoreinfo.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c > index 326a3ce8f4..a1c4847cdf 100644 > --- a/hw/misc/vmcoreinfo.c > +++ b/hw/misc/vmcoreinfo.c > @@ -13,7 +13,6 @@ > #include "qemu/osdep.h" > #include "qapi/error.h" > #include "qemu/module.h" > -#include "sysemu/reset.h" > #include "hw/nvram/fw_cfg.h" > #include "migration/vmstate.h" > #include "hw/misc/vmcoreinfo.h" > @@ -26,7 +25,7 @@ static void fw_cfg_vmci_write(void *dev, off_t offset, size_t len) > && s->vmcoreinfo.guest_format != FW_CFG_VMCOREINFO_FORMAT_NONE; > } > > -static void vmcoreinfo_reset(void *dev) > +static void vmcoreinfo_reset(DeviceState *dev) > { > VMCoreInfoState *s = VMCOREINFO(dev); > > @@ -61,7 +60,6 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp) > NULL, fw_cfg_vmci_write, s, > &s->vmcoreinfo, sizeof(s->vmcoreinfo), false); > > - qemu_register_reset(vmcoreinfo_reset, dev); > vmcoreinfo_state = s; > } > > @@ -84,6 +82,7 @@ static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data) > DeviceClass *dc = DEVICE_CLASS(klass); > > dc->vmsd = &vmstate_vmcoreinfo; > + dc->reset = vmcoreinfo_reset; > dc->realize = vmcoreinfo_realize; > dc->hotpluggable = false; > set_bit(DEVICE_CATEGORY_MISC, dc->categories); >
On Thu, Sep 26, 2019 at 06:02:47PM +0200, Philippe Mathieu-Daudé wrote: > On 9/26/19 5:17 PM, Philippe Mathieu-Daudé wrote: > > Convert the reset handler into a proper Device reset method. > > Marc-André noticed this one is incorrect, because while being QDEV it is > not connected to a QBUS. > > Maybe we can add a Device::unconnected property, and when set, the > parent realize() calls 'qemu_register_reset(dev->reset, dev);'? > This might look the same, but at least Devices implementations could > stop to use this function... Can we make this automatic instead of requiring another explicit setting? Today we have at least 3 different ways of getting a device to be reset: qemu_register_reset(); explicit device_reset_all() call in another reset handler; and implicit device_reset_all() call done through parent buses/devices. I wouldn't like to create a 4th method. What I really wish for, is a opt-out mechanism for reset (meaning all devices would be guaranteed to be reset unless they explicitly opt out), instead of 3 or 4 different opt-in mechanisms.
On 10/8/19 3:32 PM, Eduardo Habkost wrote: > On Thu, Sep 26, 2019 at 06:02:47PM +0200, Philippe Mathieu-Daudé wrote: >> On 9/26/19 5:17 PM, Philippe Mathieu-Daudé wrote: >>> Convert the reset handler into a proper Device reset method. >> >> Marc-André noticed this one is incorrect, because while being QDEV it is >> not connected to a QBUS. >> >> Maybe we can add a Device::unconnected property, and when set, the >> parent realize() calls 'qemu_register_reset(dev->reset, dev);'? >> This might look the same, but at least Devices implementations could >> stop to use this function... > > Can we make this automatic instead of requiring another explicit > setting? > > Today we have at least 3 different ways of getting a device to be > reset: qemu_register_reset(); explicit device_reset_all() call in > another reset handler; and implicit device_reset_all() call done > through parent buses/devices. I wouldn't like to create a 4th > method. > > What I really wish for, is a opt-out mechanism for reset (meaning > all devices would be guaranteed to be reset unless they > explicitly opt out), instead of 3 or 4 different opt-in > mechanisms. > Sorry for the stupid question, but why would we not reset a device ? Are there some cases when a device must be "initialized" not in its reset state ? Regarding the reset guarantee. Can this be done by doing first qemu_register_reset() on each device and eventually unregistering it in case of opt-out or wanting to reset it by other means (eg when putting it into a bus) ? Damien
On Thu, 26 Sep 2019 at 17:02, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 9/26/19 5:17 PM, Philippe Mathieu-Daudé wrote: > > Convert the reset handler into a proper Device reset method. > > Marc-André noticed this one is incorrect, because while being QDEV it is > not connected to a QBUS. > > Maybe we can add a Device::unconnected property, and when set, the > parent realize() calls 'qemu_register_reset(dev->reset, dev);'? > This might look the same, but at least Devices implementations could > stop to use this function... I'm not in favour of ad-hoc attempts to patch the problem with some devices not being reset like this. I'd rather we figured out a general solution to the design problem (which isn't easy, but on the other hand the set of workarounds we currently have isn't too awful to deal with). thanks -- PMM
diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index 326a3ce8f4..a1c4847cdf 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -13,7 +13,6 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "qemu/module.h" -#include "sysemu/reset.h" #include "hw/nvram/fw_cfg.h" #include "migration/vmstate.h" #include "hw/misc/vmcoreinfo.h" @@ -26,7 +25,7 @@ static void fw_cfg_vmci_write(void *dev, off_t offset, size_t len) && s->vmcoreinfo.guest_format != FW_CFG_VMCOREINFO_FORMAT_NONE; } -static void vmcoreinfo_reset(void *dev) +static void vmcoreinfo_reset(DeviceState *dev) { VMCoreInfoState *s = VMCOREINFO(dev); @@ -61,7 +60,6 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp) NULL, fw_cfg_vmci_write, s, &s->vmcoreinfo, sizeof(s->vmcoreinfo), false); - qemu_register_reset(vmcoreinfo_reset, dev); vmcoreinfo_state = s; } @@ -84,6 +82,7 @@ static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data) DeviceClass *dc = DEVICE_CLASS(klass); dc->vmsd = &vmstate_vmcoreinfo; + dc->reset = vmcoreinfo_reset; dc->realize = vmcoreinfo_realize; dc->hotpluggable = false; set_bit(DEVICE_CATEGORY_MISC, dc->categories);
Convert the reset handler into a proper Device reset method. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/misc/vmcoreinfo.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)