| Message ID | 20090915143319.GB24708@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On 09/15/09 16:33, Michael S. Tsirkin wrote: > Add type safety to qdev reset handlers, by declaring them as > DeviceState * rather than void *. > > Signed-off-by: Michael S. Tsirkin<mst@redhat.com> Nice. Acked-by: Gerd Hoffmann <kraxel@redhat.com> cheers, Gerd
On Tue, Sep 15, 2009 at 5:33 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > Add type safety to qdev reset handlers, by declaring them as > DeviceState * rather than void *. The function seems a bit unnecessary, how about instead: static void rtl8139_reset(struct DeviceState *d) { RTL8139State *s = container_of(d, RTL8139State, dev.qdev); ?
On Tue, Sep 15, 2009 at 11:20:25PM +0300, Blue Swirl wrote: > On Tue, Sep 15, 2009 at 5:33 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > Add type safety to qdev reset handlers, by declaring them as > > DeviceState * rather than void *. > > The function seems a bit unnecessary, which function? > how about instead: instead of which one? > static void rtl8139_reset(struct DeviceState *d) > { > RTL8139State *s = container_of(d, RTL8139State, dev.qdev); > > ?
On 09/15/2009 10:42 PM, Michael S. Tsirkin wrote: > On Tue, Sep 15, 2009 at 11:20:25PM +0300, Blue Swirl wrote: >> On Tue, Sep 15, 2009 at 5:33 PM, Michael S. Tsirkin<mst@redhat.com> wrote: >>> Add type safety to qdev reset handlers, by declaring them as >>> DeviceState * rather than void *. >> >> The function seems a bit unnecessary, > > which function? > >> how about instead: > > instead of which one? > >> static void rtl8139_reset(struct DeviceState *d) >> { >> RTL8139State *s = container_of(d, RTL8139State, dev.qdev); He means not introducing pci_rtl8139_reset. Paolo
On Tue, Sep 15, 2009 at 11:16:42PM +0200, Paolo Bonzini wrote: > On 09/15/2009 10:42 PM, Michael S. Tsirkin wrote: >> On Tue, Sep 15, 2009 at 11:20:25PM +0300, Blue Swirl wrote: >>> On Tue, Sep 15, 2009 at 5:33 PM, Michael S. Tsirkin<mst@redhat.com> wrote: >>>> Add type safety to qdev reset handlers, by declaring them as >>>> DeviceState * rather than void *. >>> >>> The function seems a bit unnecessary, >> >> which function? >> >>> how about instead: >> >> instead of which one? >> >>> static void rtl8139_reset(struct DeviceState *d) >>> { >>> RTL8139State *s = container_of(d, RTL8139State, dev.qdev); > > He means not introducing pci_rtl8139_reset. > > Paolo Several places in this file use the variant that gets RTL8139State, to me it seems nicer to have that in a single place.
On Wed, Sep 16, 2009 at 11:21:41AM +0200, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > Add type safety to qdev reset handlers, by declaring them as > > DeviceState * rather than void *. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > hw/qdev.c | 10 ++++++++-- > > hw/qdev.h | 3 ++- > > hw/rtl8139.c | 10 +++++++--- > > hw/tcx.c | 11 +++++++---- > > 4 files changed, 24 insertions(+), 10 deletions(-) > > > > diff --git a/hw/qdev.c b/hw/qdev.c > > index 43b1beb..4913f88 100644 > > --- a/hw/qdev.c > > +++ b/hw/qdev.c > > @@ -209,6 +209,12 @@ DeviceState *qdev_device_add(QemuOpts *opts) > > return qdev; > > } > > > > +static void qdev_reset(void *opaque) > > +{ > > + DeviceState *dev = opaque; > > + dev->info->reset(dev); > > +} > > + > > Is there a plan to remove qdev_reset in the future? When all devices are converted to qdev, we can convert opaque to DeviceState. > > /* Initialize a device. Device properties should be set before calling > > this function. IRQs and MMIO regions should be connected/mapped after > > calling this function. */ > > @@ -220,7 +226,7 @@ int qdev_init(DeviceState *dev) > > if (rc < 0) > > return rc; > > if (dev->info->reset) > > - qemu_register_reset(dev->info->reset, dev); > > + qemu_register_reset(qdev_reset, dev); > > Otherwise, I prefer the old version. We test if there exist > dev->info->reset > and just after that, we use > dev->info->reset() Good point. I think I'll just move the if (dev->info->reset) into qdev_reset. > If the plan is to add type checking to the system, they I would like to > do something like the patch attached (not compiled, just the idea). > > Add a qdev_register_reset() that takes a dev. > And at reset time, if there is a dev parameter, we call its reset > function (all with typecheking), and we maintain the func + opaque until > everyting is ported (no, I don't now if that will ever happens either) > > For users, normal DO_UPCAST() will do the trick on the reset handler. As you say, we don't know if everything will ever be ported to your proposed way. Keeping both around seems much worse than using a void *. > Later, Juan. > diff --git a/vl.c b/vl.c > index eb01da7..facfc67 100644 > --- a/vl.c > +++ b/vl.c > @@ -3198,6 +3198,7 @@ typedef struct QEMUResetEntry { > QTAILQ_ENTRY(QEMUResetEntry) entry; > QEMUResetHandler *func; > void *opaque; > + DeviceState *dev; So we have both opaque and dev, pointing to the same device. dev is used only for reset, and opaque for everything else. And dev takes precedence if it is set, opaque is ignored on reset, but if not, opaque is used. At a minimum, all these rules should be documented? > } QEMUResetEntry; > > static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers = > @@ -3259,6 +3260,18 @@ void qemu_register_reset(QEMUResetHandler *func, void *opaque) > > re->func = func; > re->opaque = opaque; > + re->dev = NULL; > + QTAILQ_INSERT_TAIL(&reset_handlers, re, entry); > +} > + > +void qdev_register_reset(DeviceState *dev) > +{ > + QEMUResetEntry *re = qemu_mallocz(sizeof(QEMUResetEntry)); > + > + re->func = NULL; > + re->opaque = NULL; > + re->dev = dev; > + > QTAILQ_INSERT_TAIL(&reset_handlers, re, entry); > } > > @@ -3281,7 +3294,10 @@ void qemu_system_reset(void) > > /* reset all devices */ > QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) { > - re->func(re->opaque); > + if (re->dev) > + re->dev->info->reset(re->dev); > + else > + re->func(re->opaque); > } > } >
Hi, >> Otherwise, I prefer the old version. We test if there exist >> dev->info->reset >> and just after that, we use >> dev->info->reset() > > Good point. I think I'll just move the if (dev->info->reset) > into qdev_reset. How about going one step further? Register *one* qdev_reset instance which then walks the qdev tree and calls ->reset() for every device? cheers, Gerd
On Wed, Sep 16, 2009 at 12:05:41PM +0200, Gerd Hoffmann wrote: > Hi, > >>> Otherwise, I prefer the old version. We test if there exist >>> dev->info->reset >>> and just after that, we use >>> dev->info->reset() >> >> Good point. I think I'll just move the if (dev->info->reset) >> into qdev_reset. > > How about going one step further? Register *one* qdev_reset instance > which then walks the qdev tree and calls ->reset() for every device? Will be much more code. Why not reuse the existing queue? > cheers, > Gerd
On 09/15/09 23:37, Michael S. Tsirkin wrote: > On Tue, Sep 15, 2009 at 11:16:42PM +0200, Paolo Bonzini wrote: >> >> He means not introducing pci_rtl8139_reset. >> >> Paolo > > Several places in this file use the variant that gets RTL8139State, > to me it seems nicer to have that in a single place. How about creating a helper macro to go from ${device}State to DeviceState, then kill the wrapper function? i.e something like this: #define TO_QDEV_STATE(state) (&((state)->dev.qdev)) Then have one reset function which accepts DeviceState. The call sites which have RTL8139State at hand can use rtl8139_reset(TO_QDEV_STATE(s)); cheers, Gerd
On Wed, Sep 16, 2009 at 12:13:31PM +0200, Gerd Hoffmann wrote: > On 09/15/09 23:37, Michael S. Tsirkin wrote: >> On Tue, Sep 15, 2009 at 11:16:42PM +0200, Paolo Bonzini wrote: >>> >>> He means not introducing pci_rtl8139_reset. >>> >>> Paolo >> >> Several places in this file use the variant that gets RTL8139State, >> to me it seems nicer to have that in a single place. > > How about creating a helper macro to go from ${device}State to > DeviceState, then kill the wrapper function? i.e something like this: > > #define TO_QDEV_STATE(state) (&((state)->dev.qdev)) > > Then have one reset function which accepts DeviceState. The call sites > which have RTL8139State at hand can use rtl8139_reset(TO_QDEV_STATE(s)); > > cheers, > Gerd OK, since people feel strongly about it, I killed the reset function.
On 09/16/09 12:06, Michael S. Tsirkin wrote: >> How about going one step further? Register *one* qdev_reset instance >> which then walks the qdev tree and calls ->reset() for every device? > > Will be much more code. Why not reuse the existing queue? I think we'll need such a tree walker anyway sooner or later. Thus you'll get bonus points for making it generic, so it could be used for a -- say -- late_init() callback too. Also the reset() callbacks order will be based on the position of the device in the tree instead of being more or less random. cheers, Gerd
On Wed, Sep 16, 2009 at 12:22:28PM +0200, Gerd Hoffmann wrote: > On 09/16/09 12:06, Michael S. Tsirkin wrote: >>> How about going one step further? Register *one* qdev_reset instance >>> which then walks the qdev tree and calls ->reset() for every device? >> >> Will be much more code. Why not reuse the existing queue? > > I think we'll need such a tree walker anyway sooner or later. Thus > you'll get bonus points for making it generic, so it could be used for a > -- say -- late_init() callback too. > > Also the reset() callbacks order will be based on the position of the > device in the tree instead of being more or less random. > > cheers, > Gerd Better make it a separate patch, later. For now, I'm just addressing the type safety.
On Wed, Sep 16, 2009 at 01:30:51PM +0300, Michael S. Tsirkin wrote: > On Wed, Sep 16, 2009 at 12:22:28PM +0200, Gerd Hoffmann wrote: > > On 09/16/09 12:06, Michael S. Tsirkin wrote: > >>> How about going one step further? Register *one* qdev_reset instance > >>> which then walks the qdev tree and calls ->reset() for every device? > >> > >> Will be much more code. Why not reuse the existing queue? > > > > I think we'll need such a tree walker anyway sooner or later. Thus > > you'll get bonus points for making it generic, so it could be used for a > > -- say -- late_init() callback too. > > > > Also the reset() callbacks order will be based on the position of the > > device in the tree instead of being more or less random. > > > > cheers, > > Gerd > > Better make it a separate patch, later. > For now, I'm just addressing the type safety. To clarify: I don't know enough about qdev to see how easy it is. Care to send a patch on top of mine? > -- > MST
On Wed, Sep 16, 2009 at 12:22:28PM +0200, Gerd Hoffmann wrote: > On 09/16/09 12:06, Michael S. Tsirkin wrote: >>> How about going one step further? Register *one* qdev_reset instance >>> which then walks the qdev tree and calls ->reset() for every device? >> >> Will be much more code. Why not reuse the existing queue? > > I think we'll need such a tree walker anyway sooner or later. Thus > you'll get bonus points for making it generic, so it could be used for a > -- say -- late_init() callback too. By the way, for save/restore, we have there: void qdev_free(DeviceState *dev) { #if 0 /* FIXME: need sane vmstate_unregister function */ if (dev->info->vmsd) vmstate_unregister(dev->info->vmsd, dev); #endif QLIST_REMOVE(dev, sibling); qemu_free(dev); } which likely means that qemu will crash on migration if a qdev device has been removed. Anyone looking at fixing this? > > Also the reset() callbacks order will be based on the position of the > device in the tree instead of being more or less random. Not sure whether this can break anything. If no because no one cares about the order, why change it now? > cheers, > Gerd
diff --git a/hw/qdev.c b/hw/qdev.c index 43b1beb..4913f88 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -209,6 +209,12 @@ DeviceState *qdev_device_add(QemuOpts *opts) return qdev; } +static void qdev_reset(void *opaque) +{ + DeviceState *dev = opaque; + dev->info->reset(dev); +} + /* Initialize a device. Device properties should be set before calling this function. IRQs and MMIO regions should be connected/mapped after calling this function. */ @@ -220,7 +226,7 @@ int qdev_init(DeviceState *dev) if (rc < 0) return rc; if (dev->info->reset) - qemu_register_reset(dev->info->reset, dev); + qemu_register_reset(qdev_reset, dev); if (dev->info->vmsd) vmstate_register(-1, dev->info->vmsd, dev); return 0; @@ -234,7 +240,7 @@ void qdev_free(DeviceState *dev) vmstate_unregister(dev->info->vmsd, dev); #endif if (dev->info->reset) - qemu_unregister_reset(dev->info->reset, dev); + qemu_unregister_reset(qdev_reset, dev); QLIST_REMOVE(dev, sibling); qemu_free(dev); } diff --git a/hw/qdev.h b/hw/qdev.h index 623ded5..61a252c 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -100,6 +100,7 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name); /*** Device API. ***/ typedef int (*qdev_initfn)(DeviceState *dev, DeviceInfo *info); +typedef void (*qdev_resetfn)(DeviceState *dev); struct DeviceInfo { const char *name; @@ -110,7 +111,7 @@ struct DeviceInfo { int no_user; /* callbacks */ - QEMUResetHandler *reset; + qdev_resetfn reset; /* device state */ const VMStateDescription *vmsd; diff --git a/hw/rtl8139.c b/hw/rtl8139.c index 83cb1ff..fade985 100644 --- a/hw/rtl8139.c +++ b/hw/rtl8139.c @@ -1173,9 +1173,8 @@ static void rtl8139_reset_rxring(RTL8139State *s, uint32_t bufferSize) s->RxBufAddr = 0; } -static void rtl8139_reset(void *opaque) +static void rtl8139_reset(RTL8139State *s) { - RTL8139State *s = opaque; int i; /* restore MAC address */ @@ -3488,10 +3487,15 @@ static int pci_rtl8139_init(PCIDevice *dev) return 0; } +static void pci_rtl8139_reset(struct DeviceState *d) +{ + rtl8139_reset(container_of(d, RTL8139State, dev.qdev)); +} + static PCIDeviceInfo rtl8139_info = { .qdev.name = "rtl8139", .qdev.size = sizeof(RTL8139State), - .qdev.reset = rtl8139_reset, + .qdev.reset = pci_rtl8139_reset, .init = pci_rtl8139_init, }; diff --git a/hw/tcx.c b/hw/tcx.c index 012d01b..20fc2c7 100644 --- a/hw/tcx.c +++ b/hw/tcx.c @@ -411,10 +411,8 @@ static const VMStateDescription vmstate_tcx = { } }; -static void tcx_reset(void *opaque) +static void tcx_reset(TCXState *s) { - TCXState *s = opaque; - /* Initialize palette */ memset(s->r, 0, 256); memset(s->g, 0, 256); @@ -628,11 +626,16 @@ static void tcx24_screen_dump(void *opaque, const char *filename) return; } +static void reset_tcx(struct DeviceState *d) +{ + tcx_reset(container_of(d, TCXState, busdev.qdev)); +} + static SysBusDeviceInfo tcx_info = { .init = tcx_init1, .qdev.name = "SUNW,tcx", .qdev.size = sizeof(TCXState), - .qdev.reset = tcx_reset, + .qdev.reset = reset_tcx, .qdev.vmsd = &vmstate_tcx, .qdev.props = (Property[]) { DEFINE_PROP_TADDR("addr", TCXState, addr, -1),
Add type safety to qdev reset handlers, by declaring them as DeviceState * rather than void *. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/qdev.c | 10 ++++++++-- hw/qdev.h | 3 ++- hw/rtl8139.c | 10 +++++++--- hw/tcx.c | 11 +++++++---- 4 files changed, 24 insertions(+), 10 deletions(-)