[Qemu-devel] [PATCH 1/2] qemu/qdev: type safety in reset handler

[Qemu-devel] [PATCH 1/2] qemu/qdev: type safety in reset handler

[Michael S. Tsirkin]

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);
+}
+
 /* 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),
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/2] qemu/virtio: fix reset with device removal

[Michael S. Tsirkin]

virtio pci registers its own reset handler, but fails to unregister it,
which will lead to crashes after device removal.  Solve this problem by
switching to qdev reset handler, which is automatically unregistered.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-pci.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index f7a51ff..951d1dc 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -155,9 +155,8 @@ static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f)
     return 0;
 }
 
-static void virtio_pci_reset(void *opaque)
+static void virtio_pci_reset(VirtIOPCIProxy *proxy)
 {
-    VirtIOPCIProxy *proxy = opaque;
     virtio_reset(proxy->vdev);
     msix_reset(&proxy->pci_dev);
 }
@@ -429,8 +428,6 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
     pci_register_bar(&proxy->pci_dev, 0, size, PCI_ADDRESS_SPACE_IO,
                            virtio_map);
 
-    qemu_register_reset(virtio_pci_reset, proxy);
-
     virtio_bind_device(vdev, &virtio_pci_bindings, proxy);
 }
 
@@ -514,6 +511,11 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
     return 0;
 }
 
+static void virtio_reset_pci(DeviceState *d)
+{
+    virtio_pci_reset(container_of(d, VirtIOPCIProxy, pci_dev.qdev));
+}
+
 static PCIDeviceInfo virtio_info[] = {
     {
         .qdev.name = "virtio-blk-pci",
@@ -525,6 +527,7 @@ static PCIDeviceInfo virtio_info[] = {
             DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
             DEFINE_PROP_END_OF_LIST(),
         },
+        .qdev.reset = virtio_reset_pci,
     },{
         .qdev.name  = "virtio-net-pci",
         .qdev.size  = sizeof(VirtIOPCIProxy),
@@ -534,6 +537,7 @@ static PCIDeviceInfo virtio_info[] = {
                                NIC_NVECTORS_UNSPECIFIED),
             DEFINE_PROP_END_OF_LIST(),
         },
+        .qdev.reset = virtio_reset_pci,
     },{
         .qdev.name = "virtio-console-pci",
         .qdev.size = sizeof(VirtIOPCIProxy),
@@ -542,10 +546,12 @@ static PCIDeviceInfo virtio_info[] = {
             DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
             DEFINE_PROP_END_OF_LIST(),
         },
+        .qdev.reset = virtio_reset_pci,
     },{
         .qdev.name = "virtio-balloon-pci",
         .qdev.size = sizeof(VirtIOPCIProxy),
         .init      = virtio_balloon_init_pci,
+        .qdev.reset = virtio_reset_pci,
     },{
         /* end of list */
     }
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Gerd Hoffmann]

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

[Qemu-devel] Re: [PATCH 2/2] qemu/virtio: fix reset with device removal

[Gerd Hoffmann]

On 09/15/09 16:33, Michael S. Tsirkin wrote:
> virtio pci registers its own reset handler, but fails to unregister it,
> which will lead to crashes after device removal.  Solve this problem by
> switching to qdev reset handler, which is automatically unregistered.

Looks good.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/2] qemu/qdev: type safety in reset handler

[Blue Swirl]

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);

?

Re: [Qemu-devel] [PATCH 1/2] qemu/qdev: type safety in reset handler

[Michael S. Tsirkin]

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);
> 
> ?

[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Paolo Bonzini]

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

[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Michael S. Tsirkin]

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.

-- 
MST

[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Michael S. Tsirkin]

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);
>      }
>  }
> 

[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Gerd Hoffmann]

   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

[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Michael S. Tsirkin]

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

[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Gerd Hoffmann]

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

[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Michael S. Tsirkin]

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.

[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Gerd Hoffmann]

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

[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Michael S. Tsirkin]

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.

-- 
MST

[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Michael S. Tsirkin]

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

[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Michael S. Tsirkin]

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