[Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

After discussion with mst on the topic of resetting virtio devices,
here is a series that hopefully clarifies the semantics of bus and
device resets.

After this series, there are two kinds of resets:

1) device-level reset is the kind of reset that you get with a register
write on the device.  It will clear interrupts and DMAs among other things,
but not any bus-level state, for example it will not clear PCI BARs and
other configuration space data.  It is done with qdev_reset_all.

2) bus-level reset is the kind of reset that you get with a register
write on the device that exports the bus (including triggering a device-level
reset on the device that exports the bus).  It will do a device-level
reset on the child, but also clear bus-level state such as PCI BARs and
other configuration space data.  It can be triggered for all devices
on a bus with qbus_reset_all.  There is still no API for a bus-level
reset of a single device (like PCI FLR), this can be added later.

Patches 1-5 are miscellaneous fixes to the reset paths.

Patches 6-8 introduce qbus_reset_all and uses it.

Patches 9-12 switch qdev_reset_all and qbus_reset_all from pre-order
to post-order, and document the resulting semantics.

Finally, patches 13-15 adjust the virtio implementations to use qdev
device-level reset more extensively.

Paolo Bonzini (15):
  qdev: do not reset a device until the parent has been initialized
  intel-hda: do not reset codecs from intel_hda_reset
  pci: clean up resetting of IRQs
  virtio-pci: reset device before PCI layer
  virtio-s390: add a reset function to virtio-s390 devices
  qdev: add qbus_reset_all
  pci: do not export pci_bus_reset
  lsi: use qbus_reset_all to reset SCSI bus
  qdev: allow both pre- and post-order vists in qdev walking functions
  qdev: switch reset to post-order
  qdev: remove device_reset
  qdev: document reset semantics
  virtio-pci: reset all qbuses too when writing to the status field
  virtio-s390: reset all qbuses too when writing to the status field
  virtio-serial: do not perform bus reset by hand

 hw/intel-hda.c         |  1 -
 hw/lsi53c895a.c        |  7 +----
 hw/pci.c               | 16 ++++------
 hw/pci.h               |  1 -
 hw/pci_bridge.c        |  2 +-
 hw/qdev-core.h         | 83 ++++++++++++++++++++++++++++++++++++++++++--------
 hw/qdev.c              | 70 +++++++++++++++++++++++++++---------------
 hw/s390-virtio-bus.c   | 16 +++++++++-
 hw/virtio-pci.c        | 27 +++++++---------
 hw/virtio-serial-bus.c | 19 +++---------
 10 files changed, 156 insertions(+), 86 deletions(-)

-- 
1.8.0.2

[Qemu-devel] [PATCH 01/15] qdev: do not reset a device until the parent has been initialized

[Paolo Bonzini]

When a device creates a bus and more devices as part of its init callback, the
child device could be reset while the parent is still only partly initialized.
In this case, the right thing to do is to delay resetting the child.  Do not
do it at all in qdev_init, instead use qdev_reset_all to reset already-created
devices when the state goes from CREATED to INITIALIZED.

This happens when hotplugging a usb-storage device.  Without this patch,
initialization of a hotplugged usb-storage device would run in pre-order.
Initialization of a coldplugged usb-storage device would run according to
qdev_reset_all semantics (pre-order right now, post-order later in the
series).  This patch makes things consistent.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 599382c..2fdf4ac 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -176,8 +176,9 @@ int qdev_init(DeviceState *dev)
                                        dev->alias_required_for_version);
     }
     dev->state = DEV_STATE_INITIALIZED;
-    if (dev->hotplugged) {
-        device_reset(dev);
+    if (dev->hotplugged &&
+        dev->parent_bus->parent->state == DEV_STATE_INITIALIZED) {
+        qdev_reset_all(dev);
     }
     return 0;
 }
-- 
1.8.0.2

Re: [Qemu-devel] [PATCH 01/15] qdev: do not reset a device until the parent has been initialized

[Michael S. Tsirkin]

On Mon, Dec 17, 2012 at 05:24:36PM +0100, Paolo Bonzini wrote:
> When a device creates a bus and more devices as part of its init callback, the
> child device could be reset while the parent is still only partly initialized.
> In this case, the right thing to do is to delay resetting the child.  Do not
> do it at all in qdev_init, instead use qdev_reset_all to reset already-created
> devices when the state goes from CREATED to INITIALIZED.
> 
> This happens when hotplugging a usb-storage device.  Without this patch,
> initialization of a hotplugged usb-storage device would run in pre-order.
> Initialization of a coldplugged usb-storage device would run according to
> qdev_reset_all semantics (pre-order right now, post-order later in the
> series).

Is this a problem or just not pretty?

>  This patch makes things consistent.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/qdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 599382c..2fdf4ac 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -176,8 +176,9 @@ int qdev_init(DeviceState *dev)
>                                         dev->alias_required_for_version);
>      }
>      dev->state = DEV_STATE_INITIALIZED;
> -    if (dev->hotplugged) {
> -        device_reset(dev);
> +    if (dev->hotplugged &&
> +        dev->parent_bus->parent->state == DEV_STATE_INITIALIZED) {
> +        qdev_reset_all(dev);

Let's add a comment explaining the why of this test, and when
will reset happen if it does not trigger here.

>      }
>      return 0;
>  }
> -- 
> 1.8.0.2
> 

Re: [Qemu-devel] [PATCH 01/15] qdev: do not reset a device until the parent has been initialized

[Michael S. Tsirkin]

On Mon, Dec 17, 2012 at 06:52:03PM +0200, Michael S. Tsirkin wrote:
> On Mon, Dec 17, 2012 at 05:24:36PM +0100, Paolo Bonzini wrote:
> > When a device creates a bus and more devices as part of its init callback, the
> > child device could be reset while the parent is still only partly initialized.
> > In this case, the right thing to do is to delay resetting the child.  Do not
> > do it at all in qdev_init, instead use qdev_reset_all to reset already-created
> > devices when the state goes from CREATED to INITIALIZED.
> > 
> > This happens when hotplugging a usb-storage device.  Without this patch,
> > initialization of a hotplugged usb-storage device would run in pre-order.
> > Initialization of a coldplugged usb-storage device would run according to
> > qdev_reset_all semantics (pre-order right now, post-order later in the
> > series).
> 
> Is this a problem or just not pretty?
> 
> >  This patch makes things consistent.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  hw/qdev.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/qdev.c b/hw/qdev.c
> > index 599382c..2fdf4ac 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> > @@ -176,8 +176,9 @@ int qdev_init(DeviceState *dev)
> >                                         dev->alias_required_for_version);
> >      }
> >      dev->state = DEV_STATE_INITIALIZED;
> > -    if (dev->hotplugged) {
> > -        device_reset(dev);
> > +    if (dev->hotplugged &&
> > +        dev->parent_bus->parent->state == DEV_STATE_INITIALIZED) {
> > +        qdev_reset_all(dev);
> 
> Let's add a comment explaining the why of this test, and when
> will reset happen if it does not trigger here.

Also - shouldn't device init be delayed as well?
What do you think?

> >      }
> >      return 0;
> >  }
> > -- 
> > 1.8.0.2
> > 

Re: [Qemu-devel] [PATCH 01/15] qdev: do not reset a device until the parent has been initialized

[Paolo Bonzini]

Il 17/12/2012 17:53, Michael S. Tsirkin ha scritto:
>>> This happens when hotplugging a usb-storage device.  Without this patch,
>>> initialization of a hotplugged usb-storage device would run in pre-order.
>>> Initialization of a coldplugged usb-storage device would run according to
>>> qdev_reset_all semantics (pre-order right now, post-order later in the
>>> series).
>>
>> Is this a problem or just not pretty?

Not pretty.  The child device doesn't exist until the parent has been
initialized, it doesn't make sense to reset it.

I can add an assertion to make it a problem, of course. :)

>>>  This patch makes things consistent.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  hw/qdev.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>> index 599382c..2fdf4ac 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -176,8 +176,9 @@ int qdev_init(DeviceState *dev)
>>>                                         dev->alias_required_for_version);
>>>      }
>>>      dev->state = DEV_STATE_INITIALIZED;
>>> -    if (dev->hotplugged) {
>>> -        device_reset(dev);
>>> +    if (dev->hotplugged &&
>>> +        dev->parent_bus->parent->state == DEV_STATE_INITIALIZED) {
>>> +        qdev_reset_all(dev);
>>
>> Let's add a comment explaining the why of this test, and when
>> will reset happen if it does not trigger here.
> 
> Also - shouldn't device init be delayed as well?
> What do you think?

Initialization should run in pre-order (unlike reset): the parent needs
to be ready in order to start the child.  What you typically have, is
that the parent device initializes itself, and then calls
qdev_create/qdev_init on the child before returning.  So it is already
being delayed.  The delay is explicit in program flow, not hidden in
qdev semantics.

Paolo


>>>      }
>>>      return 0;
>>>  }
>>> -- 
>>> 1.8.0.2
>>>

Re: [Qemu-devel] [PATCH 01/15] qdev: do not reset a device until the parent has been initialized

[Andreas Färber]

Am 17.12.2012 17:24, schrieb Paolo Bonzini:
> When a device creates a bus and more devices as part of its init callback, the
> child device could be reset while the parent is still only partly initialized.
> In this case, the right thing to do is to delay resetting the child.  Do not
> do it at all in qdev_init, instead use qdev_reset_all to reset already-created
> devices when the state goes from CREATED to INITIALIZED.
> 
> This happens when hotplugging a usb-storage device.  Without this patch,
> initialization of a hotplugged usb-storage device would run in pre-order.
> Initialization of a coldplugged usb-storage device would run according to
> qdev_reset_all semantics (pre-order right now, post-order later in the
> series).  This patch makes things consistent.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/qdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 599382c..2fdf4ac 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -176,8 +176,9 @@ int qdev_init(DeviceState *dev)
>                                         dev->alias_required_for_version);
>      }
>      dev->state = DEV_STATE_INITIALIZED;
> -    if (dev->hotplugged) {
> -        device_reset(dev);
> +    if (dev->hotplugged &&
> +        dev->parent_bus->parent->state == DEV_STATE_INITIALIZED) {

This assumes that parent_bus != NULL, when we are trying to let our
fallback bus SysBus die out. The CPU in the current proposal is not a
SysBus device, to avoid compiling SysBus unnecessarily into *-user.

Your direct access into the parent's state also conflicts with my QOM
realize series IIUC.

So please at least add appropriate checks to do the below reset only
when there is a bus and leave the old behavior otherwise.

Andreas

> +        qdev_reset_all(dev);
>      }
>      return 0;
>  }
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 02/15] intel-hda: do not reset codecs from intel_hda_reset

[Paolo Bonzini]

They are reset implicitly by walking the qdev tree, before calling intel_hda_reset.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/intel-hda.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index a68c368..e7c9dd9 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -1112,7 +1112,6 @@ static void intel_hda_reset(DeviceState *dev)
     QTAILQ_FOREACH(kid, &d->codecs.qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         cdev = DO_UPCAST(HDACodecDevice, qdev, qdev);
-        device_reset(DEVICE(cdev));
         d->state_sts |= (1 << cdev->cad);
     }
     intel_hda_update_irq(d);
-- 
1.8.0.2

[Qemu-devel] [PATCH 03/15] pci: clean up resetting of IRQs

[Paolo Bonzini]

A PCI bus reset will deassert the INTX pins, and this will make the
irq_count array all-zeroes.  Check that this is the case, and remove
the existing loop which might even unsync irq_count and irq_state.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 97a0cd7..ace9368 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -213,14 +213,14 @@ void pci_bus_reset(PCIBus *bus)
 {
     int i;
 
-    for (i = 0; i < bus->nirq; i++) {
-        bus->irq_count[i] = 0;
-    }
     for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
         if (bus->devices[i]) {
             pci_device_reset(bus->devices[i]);
         }
     }
+    for (i = 0; i < bus->nirq; i++) {
+        assert(bus->irq_count[i] == 0);
+    }
 }
 
 static int pcibus_reset(BusState *qbus)
-- 
1.8.0.2

[Qemu-devel] [PATCH 04/15] virtio-pci: reset device before PCI layer

[Paolo Bonzini]

At the end of this series, qdev will reset devices in post-order: first
the children, then the parents.  The ->vdev link is logically a child
of the virtio-pci device and, even though it is not explicitly modeled
like that, we want to use the same reset semantics as qdev.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-pci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 71f4fb5..a7c75fe 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -245,8 +245,8 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy)
 void virtio_pci_reset(DeviceState *d)
 {
     VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
-    virtio_pci_stop_ioeventfd(proxy);
     virtio_reset(proxy->vdev);
+    virtio_pci_stop_ioeventfd(proxy);
     msix_unuse_all_vectors(&proxy->pci_dev);
     proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
 }
@@ -268,8 +268,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
     case VIRTIO_PCI_QUEUE_PFN:
         pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
         if (pa == 0) {
-            virtio_pci_stop_ioeventfd(proxy);
             virtio_reset(proxy->vdev);
+            virtio_pci_stop_ioeventfd(proxy);
             msix_unuse_all_vectors(&proxy->pci_dev);
         }
         else
@@ -285,6 +285,10 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         }
         break;
     case VIRTIO_PCI_STATUS:
+        if (vdev->status == 0) {
+            virtio_reset(proxy->vdev);
+        }
+
         if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
             virtio_pci_stop_ioeventfd(proxy);
         }
@@ -296,7 +300,6 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         }
 
         if (vdev->status == 0) {
-            virtio_reset(proxy->vdev);
             msix_unuse_all_vectors(&proxy->pci_dev);
         }
 
-- 
1.8.0.2

[Qemu-devel] [PATCH 05/15] virtio-s390: add a reset function to virtio-s390 devices

[Paolo Bonzini]

virtio-s390 devices are not being reset when their bus is.  To fix
this, add a reset method that forwards to virtio_reset.  This is
only needed because of the "strange" modeling of virtio devices;
the ->vdev link is being handled manually rather than through qdev.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/s390-virtio-bus.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index e0ac2d1..34173a7 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -492,6 +492,13 @@ static int s390_virtio_busdev_init(DeviceState *dev)
     return _info->init(_dev);
 }
 
+static void s390_virtio_busdev_reset(DeviceState *dev)
+{
+    VirtIOS390Device *_dev = (VirtIOS390Device *)dev;
+
+    virtio_reset(_dev->vdev);
+}
+
 static void virtio_s390_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -499,6 +506,7 @@ static void virtio_s390_device_class_init(ObjectClass *klass, void *data)
     dc->init = s390_virtio_busdev_init;
     dc->bus_type = TYPE_S390_VIRTIO_BUS;
     dc->unplug = qdev_simple_unplug_cb;
+    dc->reset = s390_virtio_busdev_reset;
 }
 
 static TypeInfo virtio_s390_device_info = {
-- 
1.8.0.2

[Qemu-devel] [PATCH 06/15] qdev: add qbus_reset_all

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-core.h | 12 ++++++++++++
 hw/qdev.c      |  7 ++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index d672cca..f8bf9dd 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -182,6 +182,18 @@ int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
 int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
                        qbus_walkerfn *busfn, void *opaque);
 void qdev_reset_all(DeviceState *dev);
+
+/**
+ * @qbus_reset_all:
+ * @bus: Bus to be reset.
+ *
+ * Reset @bus and perform a bus-level ("hard") reset of all devices connected
+ * to it, including recursive processing of all buses below @bus itself.  A
+ * hard reset means that qbus_reset_all will reset all state of the device.
+ * For PCI devices, for example, this will include the base address registers
+ * or configuration space.
+ */
+void qbus_reset_all(BusState *bus);
 void qbus_reset_all_fn(void *opaque);
 
 void qbus_free(BusState *bus);
diff --git a/hw/qdev.c b/hw/qdev.c
index 2fdf4ac..87ffad8 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -230,10 +230,15 @@ void qdev_reset_all(DeviceState *dev)
     qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
 }
 
+void qbus_reset_all(BusState *bus)
+{
+    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
+}
+
 void qbus_reset_all_fn(void *opaque)
 {
     BusState *bus = opaque;
-    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
+    qbus_reset_all(bus);
 }
 
 /* can be used as ->unplug() callback for the simple cases */
-- 
1.8.0.2

[Qemu-devel] [PATCH 07/15] pci: do not export pci_bus_reset

[Paolo Bonzini]

qbus_reset_all can be used instead.  There is no semantic change
because pcibus_reset returns 1 and takes care of the device
tree traversal.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci.c        | 10 +++-------
 hw/pci.h        |  1 -
 hw/pci_bridge.c |  2 +-
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index ace9368..436982e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -209,8 +209,9 @@ void pci_device_reset(PCIDevice *dev)
  * Trigger pci bus reset under a given bus.
  * To be called on RST# assert.
  */
-void pci_bus_reset(PCIBus *bus)
+static int pcibus_reset(BusState *qbus)
 {
+    PCIBus *bus = DO_UPCAST(PCIBus, qbus, qbus);
     int i;
 
     for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
@@ -221,13 +222,8 @@ void pci_bus_reset(PCIBus *bus)
     for (i = 0; i < bus->nirq; i++) {
         assert(bus->irq_count[i] == 0);
     }
-}
-
-static int pcibus_reset(BusState *qbus)
-{
-    pci_bus_reset(DO_UPCAST(PCIBus, qbus, qbus));
 
-    /* topology traverse is done by pci_bus_reset().
+    /* topology traverse is done in the pci_device_reset() loop above.
        Tell qbus/qdev walker not to traverse the tree */
     return 1;
 }
diff --git a/hw/pci.h b/hw/pci.h
index 4da0c2a..88fd4a3 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -334,7 +334,6 @@ void pci_bus_fire_intx_routing_notifier(PCIBus *bus);
 void pci_device_set_intx_routing_notifier(PCIDevice *dev,
                                           PCIINTxRoutingNotifier notifier);
 void pci_device_reset(PCIDevice *dev);
-void pci_bus_reset(PCIBus *bus);
 
 PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
                         const char *default_devaddr);
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index 4680501..30ed34e 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -234,7 +234,7 @@ void pci_bridge_write_config(PCIDevice *d,
     newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
     if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
         /* Trigger hot reset on 0->1 transition. */
-        pci_bus_reset(&s->sec_bus);
+        qbus_reset_all(&s->sec_bus.qbus);
     }
 }
 
-- 
1.8.0.2

[Qemu-devel] [PATCH 08/15] lsi: use qbus_reset_all to reset SCSI bus

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/lsi53c895a.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 04f2fae..83ac5e9 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -1670,12 +1670,7 @@ static void lsi_reg_writeb(LSIState *s, int offset, uint8_t val)
         }
         if (val & LSI_SCNTL1_RST) {
             if (!(s->sstat0 & LSI_SSTAT0_RST)) {
-                BusChild *kid;
-
-                QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
-                    DeviceState *dev = kid->child;
-                    device_reset(dev);
-                }
+                qbus_reset_all(&s->bus.qbus);
                 s->sstat0 |= LSI_SSTAT0_RST;
                 lsi_script_scsi_interrupt(s, LSI_SIST0_RST, 0);
             }
-- 
1.8.0.2

[Qemu-devel] [PATCH 09/15] qdev: allow both pre- and post-order vists in qdev walking functions

[Paolo Bonzini]

Resetting should be done in post-order, not pre-order.  However,
qdev_walk_children and qbus_walk_children do not allow this.  Fix
it by adding two extra arguments to the functions.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-core.h | 13 +++++++++----
 hw/qdev.c      | 45 +++++++++++++++++++++++++++++++++------------
 2 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index f8bf9dd..7c2598e 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -177,10 +177,15 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
 /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion,
  *         < 0 if either devfn or busfn terminate walk somewhere in cursion,
  *           0 otherwise. */
-int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque);
-int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque);
+int qbus_walk_children(BusState *bus,
+                       qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
+                       qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
+                       void *opaque);
+int qdev_walk_children(DeviceState *dev,
+                       qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
+                       qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
+                       void *opaque);
+
 void qdev_reset_all(DeviceState *dev);
 
 /**
diff --git a/hw/qdev.c b/hw/qdev.c
index 87ffad8..457f7bf 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -227,12 +227,12 @@ static int qbus_reset_one(BusState *bus, void *opaque)
 
 void qdev_reset_all(DeviceState *dev)
 {
-    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
+    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL);
 }
 
 void qbus_reset_all(BusState *bus)
 {
-    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
+    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL);
 }
 
 void qbus_reset_all_fn(void *opaque)
@@ -342,49 +342,70 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
     return NULL;
 }
 
-int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque)
+int qbus_walk_children(BusState *bus,
+                       qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
+                       qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
+                       void *opaque)
 {
     BusChild *kid;
     int err;
 
-    if (busfn) {
-        err = busfn(bus, opaque);
+    if (pre_busfn) {
+        err = pre_busfn(bus, opaque);
         if (err) {
             return err;
         }
     }
 
     QTAILQ_FOREACH(kid, &bus->children, sibling) {
-        err = qdev_walk_children(kid->child, devfn, busfn, opaque);
+        err = qdev_walk_children(kid->child,
+                                 pre_devfn, pre_busfn,
+                                 post_devfn, post_busfn, opaque);
         if (err < 0) {
             return err;
         }
     }
 
+    if (post_busfn) {
+        err = post_busfn(bus, opaque);
+        if (err) {
+            return err;
+        }
+    }
+
     return 0;
 }
 
-int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque)
+int qdev_walk_children(DeviceState *dev,
+                       qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
+                       qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
+                       void *opaque)
 {
     BusState *bus;
     int err;
 
-    if (devfn) {
-        err = devfn(dev, opaque);
+    if (pre_devfn) {
+        err = pre_devfn(dev, opaque);
         if (err) {
             return err;
         }
     }
 
     QLIST_FOREACH(bus, &dev->child_bus, sibling) {
-        err = qbus_walk_children(bus, devfn, busfn, opaque);
+        err = qbus_walk_children(bus, pre_devfn, pre_busfn,
+                                 post_devfn, post_busfn, opaque);
         if (err < 0) {
             return err;
         }
     }
 
+    if (post_devfn) {
+        err = post_devfn(dev, opaque);
+        if (err) {
+            return err;
+        }
+    }
+
     return 0;
 }
 
-- 
1.8.0.2

[Qemu-devel] [PATCH 10/15] qdev: switch reset to post-order

[Paolo Bonzini]

Post-order is the only sensible direction for the reset signals.  For example,
suppose pre-order is used and the parent has some data structures that cache
children state (for example a list of active requests).  When the reset
method is invoked on the parent, these caches cannot be left in a consistent
state, nor asserted to be in a consistent state.

If post-order is used, on the other hand, these will be in a known state
when the reset method is invoked on the parent.

I checked the existing bus-reset implementation and they are all fine with
post-order.  USB doesn't use qdev reset, but it should be fine with post-order
too.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 457f7bf..833d571 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -227,12 +227,12 @@ static int qbus_reset_one(BusState *bus, void *opaque)
 
 void qdev_reset_all(DeviceState *dev)
 {
-    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL);
+    qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
 }
 
 void qbus_reset_all(BusState *bus)
 {
-    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL);
+    qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
 }
 
 void qbus_reset_all_fn(void *opaque)
-- 
1.8.0.2

[Qemu-devel] [PATCH 11/15] qdev: remove device_reset

[Paolo Bonzini]

The right thing to do is always to use qdev_reset_all (or qbus_reset_all).
Call the reset method straight from qdev_reset_one.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-core.h |  7 -------
 hw/qdev.c      | 15 +++++----------
 2 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index 7c2598e..28f12a4 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -218,13 +218,6 @@ char *qdev_get_fw_dev_path(DeviceState *dev);
  */
 void qdev_machine_init(void);
 
-/**
- * @device_reset
- *
- * Reset a single device (by calling the reset method).
- */
-void device_reset(DeviceState *dev);
-
 const struct VMStateDescription *qdev_get_vmsd(DeviceState *dev);
 
 const char *qdev_fw_name(DeviceState *dev);
diff --git a/hw/qdev.c b/hw/qdev.c
index 833d571..1736d6f 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -211,7 +211,11 @@ void qdev_unplug(DeviceState *dev, Error **errp)
 
 static int qdev_reset_one(DeviceState *dev, void *opaque)
 {
-    device_reset(dev);
+    DeviceClass *klass = DEVICE_GET_CLASS(dev);
+
+    if (klass->reset) {
+        klass->reset(dev);
+    }
 
     return 0;
 }
@@ -750,15 +754,6 @@ static void device_class_init(ObjectClass *class, void *data)
     class->unparent = qdev_remove_from_bus;
 }
 
-void device_reset(DeviceState *dev)
-{
-    DeviceClass *klass = DEVICE_GET_CLASS(dev);
-
-    if (klass->reset) {
-        klass->reset(dev);
-    }
-}
-
 Object *qdev_get_machine(void)
 {
     static Object *dev;
-- 
1.8.0.2

[Qemu-devel] [PATCH 12/15] qdev: document reset semantics

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-core.h | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index 28f12a4..1274994 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -36,7 +36,24 @@ typedef struct DeviceClass {
     Property *props;
     int no_user;
 
-    /* callbacks */
+    /* reset:
+     * @dev: Device being reset.
+     *
+     * Performs the device-specific part of a device-level ("soft") reset
+     * of @dev.  This is called by QEMU internally as part of initialization
+     * or resetting a bus, but it is often accessible using a device register
+     * as well.  Devices can access it using qdev_reset_all.
+     *
+     * In general, a soft reset will not reset any state that depends on the
+     * type of bus that the device resides on.  For example, PCI devices do not
+     * reset their base address registers or configuration space in the reset
+     * callback.  Resetting these registers is handled by the bus, not by the
+     * device.
+     *
+     * A device-level reset also includes a hard reset of all the buses exposed
+     * by @dev (and all devices below those, recursively).  However, this is
+     * handled by qdev_reset_all and this callback need not care about it.
+     */
     void (*reset)(DeviceState *dev);
 
     /* device state */
@@ -86,6 +103,23 @@ struct BusClass {
      * bindings can be found at http://playground.sun.com/1275/bindings/.
      */
     char *(*get_fw_dev_path)(DeviceState *dev);
+
+    /* reset:
+     * @bus: Bus being reset.
+     *
+     * This callback performs a reset of @bus.  It is usually done when
+     * qdev_reset_all is called on the parent of @bus.
+     *
+     * Resetting a bus includes a bus-level ("hard") reset of all devices on
+     * the bus itself.  Compared to a device-level ("soft") reset, this will
+     * also remove all the configuration for devices.  In the case of PCI, for
+     * example, this means the base address registers, configuration space,
+     * etc.
+     *
+     * The callback may take care of calling qdev_reset_all on all devices
+     * to perform the soft reset, or not.  In the first case, it should return
+     * 1; in the second case, it should return 0 (see also qbus_walkerfn).
+     */
     int (*reset)(BusState *bus);
 };
 
@@ -186,6 +220,21 @@ int qdev_walk_children(DeviceState *dev,
                        qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
                        void *opaque);
 
+/**
+ * @qdev_reset_all:
+ * @dev: Device to be reset.
+ *
+ * Perform a device-level ("soft") reset of dev, including all buses
+ * and all devices connected to those buses.  A soft reset means that
+ * qdev_reset_all will not reset any state that depends on the type of
+ * bus that the device resides on.  For example, PCI devices will not
+ * reset their base address registers or configuration space.
+ *
+ * However, this is not true for device connected to @dev's buses; these are
+ * reset completely.  For example, if @dev is a PCI-to-PCI bridge, the base
+ * address registers will be reset for devices on the bridge, but not for @dev
+ * itself.
+ */
 void qdev_reset_all(DeviceState *dev);
 
 /**
-- 
1.8.0.2

[Qemu-devel] [PATCH 13/15] virtio-pci: reset all qbuses too when writing to the status field

[Paolo Bonzini]

virtio-pci devices do not perform a full reset when zero is written
to the status byte.  While PCI-specific status is initialized, the
reset does not propagate down the qdev bus hierarchy.  Because of
this, a virtio reset does not cancel in-flight I/O for virtio-scsi
(where the cancellation is handled automatically by the SCSI
devices underneath virtio-scsi-pci).

Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-pci.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index a7c75fe..2cf5282 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -268,12 +268,10 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
     case VIRTIO_PCI_QUEUE_PFN:
         pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
         if (pa == 0) {
-            virtio_reset(proxy->vdev);
-            virtio_pci_stop_ioeventfd(proxy);
-            msix_unuse_all_vectors(&proxy->pci_dev);
-        }
-        else
+            qdev_reset_all(&proxy->pci_dev.qdev);
+        } else {
             virtio_queue_set_addr(vdev, vdev->queue_sel, pa);
+        }
         break;
     case VIRTIO_PCI_QUEUE_SEL:
         if (val < VIRTIO_PCI_QUEUE_MAX)
@@ -285,22 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         }
         break;
     case VIRTIO_PCI_STATUS:
-        if (vdev->status == 0) {
-            virtio_reset(proxy->vdev);
-        }
-
-        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
-            virtio_pci_stop_ioeventfd(proxy);
-        }
-
         virtio_set_status(vdev, val & 0xFF);
 
-        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
-            virtio_pci_start_ioeventfd(proxy);
-        }
-
         if (vdev->status == 0) {
-            msix_unuse_all_vectors(&proxy->pci_dev);
+            qdev_reset_all(&proxy->pci_dev.qdev);
+        } else {
+            if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
+                virtio_pci_stop_ioeventfd(proxy);
+            } else {
+                virtio_pci_start_ioeventfd(proxy);
+            }
         }
 
         /* Linux before 2.6.34 sets the device as OK without enabling
-- 
1.8.0.2

Re: [Qemu-devel] [PATCH 13/15] virtio-pci: reset all qbuses too when writing to the status field

[Michael S. Tsirkin]

On Mon, Dec 17, 2012 at 05:24:48PM +0100, Paolo Bonzini wrote:
> virtio-pci devices do not perform a full reset when zero is written
> to the status byte.  While PCI-specific status is initialized, the
> reset does not propagate down the qdev bus hierarchy.  Because of
> this, a virtio reset does not cancel in-flight I/O for virtio-scsi
> (where the cancellation is handled automatically by the SCSI
> devices underneath virtio-scsi-pci).
> 
> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I would still prefer this logic to reside in virtio.c instead of being
duplicated in each bus.
My idea was to simply call qdev_reset_all on the binding from virtio.c
but other ideas wellcome.


> ---
>  hw/virtio-pci.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index a7c75fe..2cf5282 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -268,12 +268,10 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>      case VIRTIO_PCI_QUEUE_PFN:
>          pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
>          if (pa == 0) {
> -            virtio_reset(proxy->vdev);
> -            virtio_pci_stop_ioeventfd(proxy);
> -            msix_unuse_all_vectors(&proxy->pci_dev);
> -        }
> -        else
> +            qdev_reset_all(&proxy->pci_dev.qdev);
> +        } else {
>              virtio_queue_set_addr(vdev, vdev->queue_sel, pa);
> +        }
>          break;
>      case VIRTIO_PCI_QUEUE_SEL:
>          if (val < VIRTIO_PCI_QUEUE_MAX)
> @@ -285,22 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>          }
>          break;
>      case VIRTIO_PCI_STATUS:
> -        if (vdev->status == 0) {
> -            virtio_reset(proxy->vdev);
> -        }
> -
> -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> -            virtio_pci_stop_ioeventfd(proxy);
> -        }
> -
>          virtio_set_status(vdev, val & 0xFF);
>  
> -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> -            virtio_pci_start_ioeventfd(proxy);
> -        }
> -
>          if (vdev->status == 0) {
> -            msix_unuse_all_vectors(&proxy->pci_dev);
> +            qdev_reset_all(&proxy->pci_dev.qdev);
> +        } else {
> +            if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> +                virtio_pci_stop_ioeventfd(proxy);
> +            } else {
> +                virtio_pci_start_ioeventfd(proxy);
> +            }
>          }
>  
>          /* Linux before 2.6.34 sets the device as OK without enabling
> -- 
> 1.8.0.2
> 

Re: [Qemu-devel] [PATCH 13/15] virtio-pci: reset all qbuses too when writing to the status field

[Paolo Bonzini]

Il 17/12/2012 17:48, Michael S. Tsirkin ha scritto:
> On Mon, Dec 17, 2012 at 05:24:48PM +0100, Paolo Bonzini wrote:
>> virtio-pci devices do not perform a full reset when zero is written
>> to the status byte.  While PCI-specific status is initialized, the
>> reset does not propagate down the qdev bus hierarchy.  Because of
>> this, a virtio reset does not cancel in-flight I/O for virtio-scsi
>> (where the cancellation is handled automatically by the SCSI
>> devices underneath virtio-scsi-pci).
>>
>> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> I would still prefer this logic to reside in virtio.c instead of being
> duplicated in each bus.
> My idea was to simply call qdev_reset_all on the binding from virtio.c
> but other ideas wellcome.

I think you're confusing "in the common superclass of all virtio
transports" vs "in the common superclass of all virtio devices".
virtio.c only implements a common superclass of all virtio devices; in
fact, there is no common superclass of all virtio transports, and it is
not possible without multiple inheritance or stuff like traits (you're
already inheriting from PCIDevice for virtio-*-pci).

Such common superclass, if it existed, would abstract stuff like "write
zero to the status register" and would call qdev_reset_all.  But again,
we don't have this concept.

Paolo

> 
> 
>> ---
>>  hw/virtio-pci.c | 28 ++++++++++------------------
>>  1 file changed, 10 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index a7c75fe..2cf5282 100644
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -268,12 +268,10 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>      case VIRTIO_PCI_QUEUE_PFN:
>>          pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
>>          if (pa == 0) {
>> -            virtio_reset(proxy->vdev);
>> -            virtio_pci_stop_ioeventfd(proxy);
>> -            msix_unuse_all_vectors(&proxy->pci_dev);
>> -        }
>> -        else
>> +            qdev_reset_all(&proxy->pci_dev.qdev);
>> +        } else {
>>              virtio_queue_set_addr(vdev, vdev->queue_sel, pa);
>> +        }
>>          break;
>>      case VIRTIO_PCI_QUEUE_SEL:
>>          if (val < VIRTIO_PCI_QUEUE_MAX)
>> @@ -285,22 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>          }
>>          break;
>>      case VIRTIO_PCI_STATUS:
>> -        if (vdev->status == 0) {
>> -            virtio_reset(proxy->vdev);
>> -        }
>> -
>> -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>> -            virtio_pci_stop_ioeventfd(proxy);
>> -        }
>> -
>>          virtio_set_status(vdev, val & 0xFF);
>>  
>> -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
>> -            virtio_pci_start_ioeventfd(proxy);
>> -        }
>> -
>>          if (vdev->status == 0) {
>> -            msix_unuse_all_vectors(&proxy->pci_dev);
>> +            qdev_reset_all(&proxy->pci_dev.qdev);
>> +        } else {
>> +            if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>> +                virtio_pci_stop_ioeventfd(proxy);
>> +            } else {
>> +                virtio_pci_start_ioeventfd(proxy);
>> +            }
>>          }
>>  
>>          /* Linux before 2.6.34 sets the device as OK without enabling
>> -- 
>> 1.8.0.2
>>

Re: [Qemu-devel] [PATCH 13/15] virtio-pci: reset all qbuses too when writing to the status field

[Michael S. Tsirkin]

On Mon, Dec 17, 2012 at 05:54:06PM +0100, Paolo Bonzini wrote:
> Il 17/12/2012 17:48, Michael S. Tsirkin ha scritto:
> > On Mon, Dec 17, 2012 at 05:24:48PM +0100, Paolo Bonzini wrote:
> >> virtio-pci devices do not perform a full reset when zero is written
> >> to the status byte.  While PCI-specific status is initialized, the
> >> reset does not propagate down the qdev bus hierarchy.  Because of
> >> this, a virtio reset does not cancel in-flight I/O for virtio-scsi
> >> (where the cancellation is handled automatically by the SCSI
> >> devices underneath virtio-scsi-pci).
> >>
> >> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > I would still prefer this logic to reside in virtio.c instead of being
> > duplicated in each bus.
> > My idea was to simply call qdev_reset_all on the binding from virtio.c
> > but other ideas wellcome.
> 
> I think you're confusing "in the common superclass of all virtio
> transports" vs "in the common superclass of all virtio devices".
> virtio.c only implements a common superclass of all virtio devices; in
> fact, there is no common superclass of all virtio transports, and it is
> not possible without multiple inheritance or stuff like traits (you're
> already inheriting from PCIDevice for virtio-*-pci).
> 
> Such common superclass, if it existed, would abstract stuff like "write
> zero to the status register" and would call qdev_reset_all.  But again,
> we don't have this concept.
> 
> Paolo

There's some misunderstanding here.

This is not about classes and stuff.

Common code should be in a single place so everyone can reuse it through
function calls, not duplicated in all transports.
And yes we do have examples of this if you are asking for examples;
examples are functions like virtio_reset, virtio_set_status, etc.


> > 
> > 
> >> ---
> >>  hw/virtio-pci.c | 28 ++++++++++------------------
> >>  1 file changed, 10 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >> index a7c75fe..2cf5282 100644
> >> --- a/hw/virtio-pci.c
> >> +++ b/hw/virtio-pci.c
> >> @@ -268,12 +268,10 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>      case VIRTIO_PCI_QUEUE_PFN:
> >>          pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
> >>          if (pa == 0) {
> >> -            virtio_reset(proxy->vdev);
> >> -            virtio_pci_stop_ioeventfd(proxy);
> >> -            msix_unuse_all_vectors(&proxy->pci_dev);
> >> -        }
> >> -        else
> >> +            qdev_reset_all(&proxy->pci_dev.qdev);
> >> +        } else {
> >>              virtio_queue_set_addr(vdev, vdev->queue_sel, pa);
> >> +        }
> >>          break;
> >>      case VIRTIO_PCI_QUEUE_SEL:
> >>          if (val < VIRTIO_PCI_QUEUE_MAX)
> >> @@ -285,22 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>          }
> >>          break;
> >>      case VIRTIO_PCI_STATUS:
> >> -        if (vdev->status == 0) {
> >> -            virtio_reset(proxy->vdev);
> >> -        }
> >> -
> >> -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> >> -            virtio_pci_stop_ioeventfd(proxy);
> >> -        }
> >> -
> >>          virtio_set_status(vdev, val & 0xFF);
> >>  
> >> -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> >> -            virtio_pci_start_ioeventfd(proxy);
> >> -        }
> >> -
> >>          if (vdev->status == 0) {
> >> -            msix_unuse_all_vectors(&proxy->pci_dev);
> >> +            qdev_reset_all(&proxy->pci_dev.qdev);
> >> +        } else {
> >> +            if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> >> +                virtio_pci_stop_ioeventfd(proxy);
> >> +            } else {
> >> +                virtio_pci_start_ioeventfd(proxy);
> >> +            }
> >>          }
> >>  
> >>          /* Linux before 2.6.34 sets the device as OK without enabling
> >> -- 
> >> 1.8.0.2
> >>

Re: [Qemu-devel] [PATCH 13/15] virtio-pci: reset all qbuses too when writing to the status field

[Paolo Bonzini]

Il 17/12/2012 18:08, Michael S. Tsirkin ha scritto:
>>> I would still prefer this logic to reside in virtio.c instead of being
>>> duplicated in each bus.
>>> My idea was to simply call qdev_reset_all on the binding from virtio.c
>>> but other ideas wellcome.
>>
>> I think you're confusing "in the common superclass of all virtio
>> transports" vs "in the common superclass of all virtio devices".
>> virtio.c only implements a common superclass of all virtio devices; in
>> fact, there is no common superclass of all virtio transports, and it is
>> not possible without multiple inheritance or stuff like traits (you're
>> already inheriting from PCIDevice for virtio-*-pci).
>>
>> Such common superclass, if it existed, would abstract stuff like "write
>> zero to the status register" and would call qdev_reset_all.  But again,
>> we don't have this concept.
> 
> There's some misunderstanding here.
> 
> This is not about classes and stuff.
> 
> Common code should be in a single place so everyone can reuse it through
> function calls, not duplicated in all transports.
> And yes we do have examples of this if you are asking for examples;
> examples are functions like virtio_reset, virtio_set_status, etc.

We shouldn't need a virtio_reset at all.  It should be invoked
automatically by virtue of having a correct, complete description of
virtio in terms of qdev buses.

In other words, qdev_reset_all _is_ the common code you're seeking.

Paolo

[Qemu-devel] [PATCH 14/15] virtio-s390: reset all qbuses too when writing to the status field

[Paolo Bonzini]

virtio-s390 devices do not perform a reset when zero is written to the
status byte.  Because of this, a virtio reset does not cancel in-flight
I/O for virtio-scsi.

Use qdev_reset_all so that the reset correctly propagates down the qdev
bus hierarchy.

Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/s390-virtio-bus.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index 34173a7..b8465d1 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -309,8 +309,14 @@ void s390_virtio_device_update_status(VirtIOS390Device *dev)
 {
     VirtIODevice *vdev = dev->vdev;
     uint32_t features;
+    unsigned char status;
 
-    virtio_set_status(vdev, ldub_phys(dev->dev_offs + VIRTIO_DEV_OFFS_STATUS));
+    status = ldub_phys(dev->dev_offs + VIRTIO_DEV_OFFS_STATUS);
+    if (status == 0) {
+        qdev_reset_all((DeviceState *)dev);
+    }
+
+    virtio_set_status(vdev, status);
 
     /* Update guest supported feature bitmap */
 
-- 
1.8.0.2

Re: [Qemu-devel] [PATCH 14/15] virtio-s390: reset all qbuses too when writing to the status field

[Alexander Graf]

On 17.12.2012, at 17:24, Paolo Bonzini wrote:

> virtio-s390 devices do not perform a reset when zero is written to the
> status byte.  Because of this, a virtio reset does not cancel in-flight
> I/O for virtio-scsi.
> 
> Use qdev_reset_all so that the reset correctly propagates down the qdev
> bus hierarchy.

Works for me.

Alex

> 
> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/s390-virtio-bus.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> index 34173a7..b8465d1 100644
> --- a/hw/s390-virtio-bus.c
> +++ b/hw/s390-virtio-bus.c
> @@ -309,8 +309,14 @@ void s390_virtio_device_update_status(VirtIOS390Device *dev)
> {
>     VirtIODevice *vdev = dev->vdev;
>     uint32_t features;
> +    unsigned char status;
> 
> -    virtio_set_status(vdev, ldub_phys(dev->dev_offs + VIRTIO_DEV_OFFS_STATUS));
> +    status = ldub_phys(dev->dev_offs + VIRTIO_DEV_OFFS_STATUS);
> +    if (status == 0) {
> +        qdev_reset_all((DeviceState *)dev);
> +    }
> +
> +    virtio_set_status(vdev, status);
> 
>     /* Update guest supported feature bitmap */
> 
> -- 
> 1.8.0.2
> 
> 

[Qemu-devel] [PATCH 15/15] virtio-serial: do not perform bus reset by hand

[Paolo Bonzini]

Implement guest_reset as a bus-level reset callback.  Then, the qdev
and virtio core will call it when needed every time the device is
reset or the status field is set to 0.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-serial-bus.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 155da58..1564482 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -532,12 +532,13 @@ static void set_config(VirtIODevice *vdev, const uint8_t *config_data)
     memcpy(&config, config_data, sizeof(config));
 }
 
-static void guest_reset(VirtIOSerial *vser)
+static int virtser_bus_reset(BusState *qbus)
 {
+    VirtIOSerialBus *bus = DO_UPCAST(VirtIOSerialBus, qbus, qbus);
     VirtIOSerialPort *port;
     VirtIOSerialPortClass *vsc;
 
-    QTAILQ_FOREACH(port, &vser->ports, next) {
+    QTAILQ_FOREACH(port, &bus->vser->ports, next) {
         vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
         if (port->guest_connected) {
             port->guest_connected = false;
@@ -546,6 +547,7 @@ static void guest_reset(VirtIOSerial *vser)
                 vsc->guest_close(port);
         }
     }
+    return 0;
 }
 
 static void set_status(VirtIODevice *vdev, uint8_t status)
@@ -566,17 +568,6 @@ static void set_status(VirtIODevice *vdev, uint8_t status)
          */
         port->guest_connected = true;
     }
-    if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
-        guest_reset(vser);
-    }
-}
-
-static void vser_reset(VirtIODevice *vdev)
-{
-    VirtIOSerial *vser;
-
-    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
-    guest_reset(vser);
 }
 
 static void virtio_serial_save(QEMUFile *f, void *opaque)
@@ -766,6 +757,7 @@ static void virtser_bus_class_init(ObjectClass *klass, void *data)
 {
     BusClass *k = BUS_CLASS(klass);
     k->print_dev = virtser_bus_dev_print;
+    k->reset = virtser_bus_reset;
 }
 
 static const TypeInfo virtser_bus_info = {
@@ -985,7 +977,6 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *conf)
     vser->vdev.get_config = get_config;
     vser->vdev.set_config = set_config;
     vser->vdev.set_status = set_status;
-    vser->vdev.reset = vser_reset;
 
     vser->qdev = dev;
 
-- 
1.8.0.2

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Mon, Dec 17, 2012 at 05:24:35PM +0100, Paolo Bonzini wrote:
> After discussion with mst on the topic of resetting virtio devices,
> here is a series that hopefully clarifies the semantics of bus and
> device resets.
> 
> After this series, there are two kinds of resets:

So just to clarify, what I proposed was this
(on top of my type safety patch). Then
all transports can call virtio_config_reset
when appropriate (e.g. when PA is set to 0).

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/hw/virtio.c b/hw/virtio.c
index f40a8c5..e65d7c8 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -554,6 +554,14 @@ void virtio_reset(void *opaque)
     }
 }
 
+/* Device-specific reset through virtio config space.
+ * Reset virtio config and backend child devices if any.
+ */
+void virtio_config_reset(VirtIODevice *vdev)
+{
+    qdev_reset_all(vdev->binding_opaque);
+}
+
 uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
 {
     uint8_t val;
diff --git a/hw/virtio.h b/hw/virtio.h
index 7c17f7b..e2e57a4 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -187,11 +187,12 @@ uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
 void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
 void virtio_set_status(VirtIODevice *vdev, uint8_t val);
 void virtio_reset(void *opaque);
+void virtio_config_reset(VirtIODevice *vdev);
 void virtio_update_irq(VirtIODevice *vdev);
 int virtio_set_features(VirtIODevice *vdev, uint32_t val);
 
 void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
                         DeviceState *opaque);
 
 /* Base devices.  */
 typedef struct VirtIOBlkConf VirtIOBlkConf;

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 17/12/2012 22:43, Michael S. Tsirkin ha scritto:
> On Mon, Dec 17, 2012 at 05:24:35PM +0100, Paolo Bonzini wrote:
>> After discussion with mst on the topic of resetting virtio devices,
>> here is a series that hopefully clarifies the semantics of bus and
>> device resets.
>>
>> After this series, there are two kinds of resets:
> 
> So just to clarify, what I proposed was this
> (on top of my type safety patch). Then
> all transports can call virtio_config_reset
> when appropriate (e.g. when PA is set to 0).
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> diff --git a/hw/virtio.c b/hw/virtio.c
> index f40a8c5..e65d7c8 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -554,6 +554,14 @@ void virtio_reset(void *opaque)
>      }
>  }
>  
> +/* Device-specific reset through virtio config space.
> + * Reset virtio config and backend child devices if any.
> + */
> +void virtio_config_reset(VirtIODevice *vdev)
> +{
> +    qdev_reset_all(vdev->binding_opaque);
> +}

Yes, I had understood.  As I said, this is the wrong direction.
Resetting happens from vdev->binding_opaque, it can just do
qdev_reset_all(myself).

Paolo

>  uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
>  {
>      uint8_t val;
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 7c17f7b..e2e57a4 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -187,11 +187,12 @@ uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
>  void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
>  void virtio_set_status(VirtIODevice *vdev, uint8_t val);
>  void virtio_reset(void *opaque);
> +void virtio_config_reset(VirtIODevice *vdev);
>  void virtio_update_irq(VirtIODevice *vdev);
>  int virtio_set_features(VirtIODevice *vdev, uint32_t val);
>  
>  void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
>                          DeviceState *opaque);
>  
>  /* Base devices.  */
>  typedef struct VirtIOBlkConf VirtIOBlkConf;
> 
> 

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 18/12/2012 08:27, Paolo Bonzini ha scritto:
> Il 17/12/2012 22:43, Michael S. Tsirkin ha scritto:
>> On Mon, Dec 17, 2012 at 05:24:35PM +0100, Paolo Bonzini wrote:
>>> After discussion with mst on the topic of resetting virtio devices,
>>> here is a series that hopefully clarifies the semantics of bus and
>>> device resets.
>>>
>>> After this series, there are two kinds of resets:
>>
>> So just to clarify, what I proposed was this
>> (on top of my type safety patch). Then
>> all transports can call virtio_config_reset
>> when appropriate (e.g. when PA is set to 0).
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> diff --git a/hw/virtio.c b/hw/virtio.c
>> index f40a8c5..e65d7c8 100644
>> --- a/hw/virtio.c
>> +++ b/hw/virtio.c
>> @@ -554,6 +554,14 @@ void virtio_reset(void *opaque)
>>      }
>>  }
>>  
>> +/* Device-specific reset through virtio config space.
>> + * Reset virtio config and backend child devices if any.
>> + */
>> +void virtio_config_reset(VirtIODevice *vdev)
>> +{
>> +    qdev_reset_all(vdev->binding_opaque);
>> +}
> 
> Yes, I had understood.  As I said, this is the wrong direction.
> Resetting happens from vdev->binding_opaque, it can just do
> qdev_reset_all(myself).

... besides, this only works if the reset callback of
vdev->binding_opaque remembers to call virtio_reset (in the s390
bindings, it doesn't and this series fixes it).  So IMO it is not only
useless, it is also misleading.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Tue, Dec 18, 2012 at 09:35:02AM +0100, Paolo Bonzini wrote:
> Il 18/12/2012 08:27, Paolo Bonzini ha scritto:
> > Il 17/12/2012 22:43, Michael S. Tsirkin ha scritto:
> >> On Mon, Dec 17, 2012 at 05:24:35PM +0100, Paolo Bonzini wrote:
> >>> After discussion with mst on the topic of resetting virtio devices,
> >>> here is a series that hopefully clarifies the semantics of bus and
> >>> device resets.
> >>>
> >>> After this series, there are two kinds of resets:
> >>
> >> So just to clarify, what I proposed was this
> >> (on top of my type safety patch). Then
> >> all transports can call virtio_config_reset
> >> when appropriate (e.g. when PA is set to 0).
> >>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >> diff --git a/hw/virtio.c b/hw/virtio.c
> >> index f40a8c5..e65d7c8 100644
> >> --- a/hw/virtio.c
> >> +++ b/hw/virtio.c
> >> @@ -554,6 +554,14 @@ void virtio_reset(void *opaque)
> >>      }
> >>  }
> >>  
> >> +/* Device-specific reset through virtio config space.
> >> + * Reset virtio config and backend child devices if any.
> >> + */
> >> +void virtio_config_reset(VirtIODevice *vdev)
> >> +{
> >> +    qdev_reset_all(vdev->binding_opaque);
> >> +}
> > 
> > Yes, I had understood.  As I said, this is the wrong direction.
> > Resetting happens from vdev->binding_opaque, it can just do
> > qdev_reset_all(myself).

It can but it's the wrong thing for transport to know about.
Let PCI worry about PCI things. This is not
a transport specific thing so belongs in virtio.c

> ... besides, this only works if the reset callback of
> vdev->binding_opaque remembers to call virtio_reset (in the s390
> bindings, it doesn't and this series fixes it).

That's a separate bug I think.

>  So IMO it is not only
> useless, it is also misleading.
> 
> Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 18/12/2012 10:49, Michael S. Tsirkin ha scritto:
>>>> +/* Device-specific reset through virtio config space.
>>>> + * Reset virtio config and backend child devices if any.
>>>> + */
>>>> +void virtio_config_reset(VirtIODevice *vdev)
>>>> +{
>>>> +    qdev_reset_all(vdev->binding_opaque);
>>>> +}
>>>
>>> Yes, I had understood.  As I said, this is the wrong direction.
>>> Resetting happens from vdev->binding_opaque, it can just do
>>> qdev_reset_all(myself).
> 
> It can but it's the wrong thing for transport to know about.

The transport provides an implementation of dc->reset, not virtio.c
(e.g. virtio_pci_reset).  Sure it knows what the effect of
qdev_reset_all are on itself.

> Let PCI worry about PCI things. This is not
> a transport specific thing so belongs in virtio.c

This _is_ a transport specific thing.  Sure it will reset the virtio
device (virtio_reset), but it will also reset things such as MSI-X
vectors and VIRTIO_PCI_FLAG_BUS_MASTER_BUG.  It doesn't belong in virtio.c.

>> ... besides, this only works if the reset callback of
>> vdev->binding_opaque remembers to call virtio_reset (in the s390
>> bindings, it doesn't and this series fixes it).
> 
> That's a separate bug I think.

Yes, I agree.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Tue, Dec 18, 2012 at 12:40:36PM +0100, Paolo Bonzini wrote:
> >> ... besides, this only works if the reset callback of
> >> vdev->binding_opaque remembers to call virtio_reset (in the s390
> >> bindings, it doesn't and this series fixes it).
> > 
> > That's a separate bug I think.
> 
> Yes, I agree.
> 
> Paolo

Anyway, I think the only argument is about a bit of code duplication,
we can fix it up later.
Andreas Färber pointed out what looks like a buglet in this patchset,
were you going to repost a fixed one?

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> After discussion with mst on the topic of resetting virtio devices,
> here is a series that hopefully clarifies the semantics of bus and
> device resets.
>
> After this series, there are two kinds of resets:
>
> 1) device-level reset is the kind of reset that you get with a register
> write on the device.  It will clear interrupts and DMAs among other things,
> but not any bus-level state, for example it will not clear PCI BARs and
> other configuration space data.  It is done with qdev_reset_all.
>
> 2) bus-level reset is the kind of reset that you get with a register
> write on the device that exports the bus (including triggering a device-level
> reset on the device that exports the bus).  It will do a device-level
> reset on the child, but also clear bus-level state such as PCI BARs and
> other configuration space data.  It can be triggered for all devices
> on a bus with qbus_reset_all.  There is still no API for a bus-level
> reset of a single device (like PCI FLR), this can be added later.

I don't really understand this dual abstraction.  I suspect it's
overgeneralizing something that's the result of poor modeling.

Very often with PCI devices, you have an otherwise independent chipset
embedded on a PCI card  There's a clear separate between the chipset and
the card.

It may be possible to poke the chipset directly to do a reset and that
may only affect state that's on the chipset but not any card-specific
state.

But this has nothing to do with busses, this is just about
encapsulation.  IOW, what you have is:

E1000 is-a PCIDevice
  has-a E1000 chipset
  PCIDevice::reset()
    -> calls chipset->reset()

But with the right register write, chipset->reset() can be called w/o
PCIDevice::reset().  But again, this has nothing to do with busses.

What I'm missing with this series is what problem are we trying to
solve?  I don't think we model reset correctly today because I don't
think there's a single notion of reset.

I think reset really ought to just be a bus level concept with
individual implementations for each bus.

Regards,

Anthony Liguori

>
> Patches 1-5 are miscellaneous fixes to the reset paths.
>
> Patches 6-8 introduce qbus_reset_all and uses it.
>
> Patches 9-12 switch qdev_reset_all and qbus_reset_all from pre-order
> to post-order, and document the resulting semantics.
>
> Finally, patches 13-15 adjust the virtio implementations to use qdev
> device-level reset more extensively.
>
> Paolo Bonzini (15):
>   qdev: do not reset a device until the parent has been initialized
>   intel-hda: do not reset codecs from intel_hda_reset
>   pci: clean up resetting of IRQs
>   virtio-pci: reset device before PCI layer
>   virtio-s390: add a reset function to virtio-s390 devices
>   qdev: add qbus_reset_all
>   pci: do not export pci_bus_reset
>   lsi: use qbus_reset_all to reset SCSI bus
>   qdev: allow both pre- and post-order vists in qdev walking functions
>   qdev: switch reset to post-order
>   qdev: remove device_reset
>   qdev: document reset semantics
>   virtio-pci: reset all qbuses too when writing to the status field
>   virtio-s390: reset all qbuses too when writing to the status field
>   virtio-serial: do not perform bus reset by hand
>
>  hw/intel-hda.c         |  1 -
>  hw/lsi53c895a.c        |  7 +----
>  hw/pci.c               | 16 ++++------
>  hw/pci.h               |  1 -
>  hw/pci_bridge.c        |  2 +-
>  hw/qdev-core.h         | 83 ++++++++++++++++++++++++++++++++++++++++++--------
>  hw/qdev.c              | 70 +++++++++++++++++++++++++++---------------
>  hw/s390-virtio-bus.c   | 16 +++++++++-
>  hw/virtio-pci.c        | 27 +++++++---------
>  hw/virtio-serial-bus.c | 19 +++---------
>  10 files changed, 156 insertions(+), 86 deletions(-)
>
> -- 
> 1.8.0.2

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Peter Maydell]

On 7 January 2013 19:10, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 1) device-level reset is the kind of reset that you get with a register
>> write on the device.  It will clear interrupts and DMAs among other things,
>> but not any bus-level state, for example it will not clear PCI BARs and
>> other configuration space data.  It is done with qdev_reset_all.

This isn't really right -- often writing the register on the device will
reset some things but not the whole device state the way a qdev
reset does. qdev reset (to the extent it's modelling anything) is more
like yanking power to the device and reapplying it.

>> 2) bus-level reset is the kind of reset that you get with a register
>> write on the device that exports the bus (including triggering a device-level
>> reset on the device that exports the bus).  It will do a device-level
>> reset on the child, but also clear bus-level state such as PCI BARs and
>> other configuration space data.  It can be triggered for all devices
>> on a bus with qbus_reset_all.  There is still no API for a bus-level
>> reset of a single device (like PCI FLR), this can be added later.

This doesn't sound very plausible: when would you do a bus level
reset anyway?

> I don't really understand this dual abstraction.  I suspect it's
> overgeneralizing something that's the result of poor modeling.

Agreed.

> What I'm missing with this series is what problem are we trying to
> solve?  I don't think we model reset correctly today because I don't
> think there's a single notion of reset.

Also agreed.

> I think reset really ought to just be a bus level concept with
> individual implementations for each bus.

I'm not sure I really agree here, especially since QOM/qdev are
moving away from the idea that there is a single bus tree and every
device is on a single bus. It's quite common for a bus to include a
reset signal but not all device reset is handled by a signal on a bus.

If we want to model reset properly we should model actual reset
lines (and/or power-cycling). If we don't care we can continue with
whatever fudge we like :-)

-- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 January 2013 19:10, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> 1) device-level reset is the kind of reset that you get with a register
>>> write on the device.  It will clear interrupts and DMAs among other things,
>>> but not any bus-level state, for example it will not clear PCI BARs and
>>> other configuration space data.  It is done with qdev_reset_all.
>
> This isn't really right -- often writing the register on the device will
> reset some things but not the whole device state the way a qdev
> reset does. qdev reset (to the extent it's modelling anything) is more
> like yanking power to the device and reapplying it.
>
>>> 2) bus-level reset is the kind of reset that you get with a register
>>> write on the device that exports the bus (including triggering a device-level
>>> reset on the device that exports the bus).  It will do a device-level
>>> reset on the child, but also clear bus-level state such as PCI BARs and
>>> other configuration space data.  It can be triggered for all devices
>>> on a bus with qbus_reset_all.  There is still no API for a bus-level
>>> reset of a single device (like PCI FLR), this can be added later.
>
> This doesn't sound very plausible: when would you do a bus level
> reset anyway?
>
>> I don't really understand this dual abstraction.  I suspect it's
>> overgeneralizing something that's the result of poor modeling.
>
> Agreed.
>
>> What I'm missing with this series is what problem are we trying to
>> solve?  I don't think we model reset correctly today because I don't
>> think there's a single notion of reset.
>
> Also agreed.
>
>> I think reset really ought to just be a bus level concept with
>> individual implementations for each bus.
>
> I'm not sure I really agree here, especially since QOM/qdev are
> moving away from the idea that there is a single bus tree and every
> device is on a single bus.

I don't mean a BusState level concept, I mean a PCIBus concept.

There is clearly such a thing as a PCI bus reset.  In fact, there are
multiple types of PCI bus resets.  There should be a PCIBus method that
calls out to PCIDevices on the bus.

But that isn't something that should be fitted into generalized to a
BusState::reset method.

> It's quite common for a bus to include a
> reset signal but not all device reset is handled by a signal on a bus.

Agreed.

>
> If we want to model reset properly we should model actual reset
> lines (and/or power-cycling). If we don't care we can continue with
> whatever fudge we like :-)

Yes, and that's basically what qemu_system_reset() is.  Of course, we
model it like everything is directly connected to a single power source
which is true 99% of the time.  That's why we've gotten away with it for
so long.

Regards,

Anthony Liguori

> -- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Peter Maydell]

On 7 January 2013 20:20, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> On 7 January 2013 19:10, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>> I think reset really ought to just be a bus level concept with
>>> individual implementations for each bus.
>>
>> I'm not sure I really agree here, especially since QOM/qdev are
>> moving away from the idea that there is a single bus tree and every
>> device is on a single bus.
>
> I don't mean a BusState level concept, I mean a PCIBus concept.
>
> There is clearly such a thing as a PCI bus reset.  In fact, there are
> multiple types of PCI bus resets.  There should be a PCIBus method that
> calls out to PCIDevices on the bus.
>
> But that isn't something that should be fitted into generalized to a
> BusState::reset method.

Ah, I see what you mean now -- yes, definitely.

>> If we want to model reset properly we should model actual reset
>> lines (and/or power-cycling). If we don't care we can continue with
>> whatever fudge we like :-)
>
> Yes, and that's basically what qemu_system_reset() is.  Of course, we
> model it like everything is directly connected to a single power source
> which is true 99% of the time.  That's why we've gotten away with it for
> so long.

I think the bit that's most creaky here is what you do about devices
that want to assert outgoing irq/whatever lines immediately on powerup.

-- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 January 2013 20:20, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> On 7 January 2013 19:10, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>>> I think reset really ought to just be a bus level concept with
>>>> individual implementations for each bus.
>>>
>>> I'm not sure I really agree here, especially since QOM/qdev are
>>> moving away from the idea that there is a single bus tree and every
>>> device is on a single bus.
>>
>> I don't mean a BusState level concept, I mean a PCIBus concept.
>>
>> There is clearly such a thing as a PCI bus reset.  In fact, there are
>> multiple types of PCI bus resets.  There should be a PCIBus method that
>> calls out to PCIDevices on the bus.
>>
>> But that isn't something that should be fitted into generalized to a
>> BusState::reset method.
>
> Ah, I see what you mean now -- yes, definitely.
>
>>> If we want to model reset properly we should model actual reset
>>> lines (and/or power-cycling). If we don't care we can continue with
>>> whatever fudge we like :-)
>>
>> Yes, and that's basically what qemu_system_reset() is.  Of course, we
>> model it like everything is directly connected to a single power source
>> which is true 99% of the time.  That's why we've gotten away with it for
>> so long.
>
> I think the bit that's most creaky here is what you do about devices
> that want to assert outgoing irq/whatever lines immediately on
> powerup.

This isn't so much a reset problem as a consequence of stateless IRQ
lines.  If IRQ lines were stateful, this problem wouldn't exist.

And yes, I will post Pins one day...

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 07/01/2013 20:10, Anthony Liguori ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> After discussion with mst on the topic of resetting virtio devices,
>> here is a series that hopefully clarifies the semantics of bus and
>> device resets.
>>
>> After this series, there are two kinds of resets:
>>
>> 1) device-level reset is the kind of reset that you get with a register
>> write on the device.  It will clear interrupts and DMAs among other things,
>> but not any bus-level state, for example it will not clear PCI BARs and
>> other configuration space data.  It is done with qdev_reset_all.
>>
>> 2) bus-level reset is the kind of reset that you get with a register
>> write on the device that exports the bus (including triggering a device-level
>> reset on the device that exports the bus).  It will do a device-level
>> reset on the child, but also clear bus-level state such as PCI BARs and
>> other configuration space data.  It can be triggered for all devices
>> on a bus with qbus_reset_all.  There is still no API for a bus-level
>> reset of a single device (like PCI FLR), this can be added later.
> 
> I don't really understand this dual abstraction.  I suspect it's
> overgeneralizing something that's the result of poor modeling.

It's possible.  I'll move the SCSI bus away from qdev reset.
Anthony/Michael, can you help doing the same with PCIDevice?  And
perhaps Peter and Andreas with sysbus?

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Wed, Jan 09, 2013 at 10:33:37AM +0100, Paolo Bonzini wrote:
> Il 07/01/2013 20:10, Anthony Liguori ha scritto:
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> > 
> >> After discussion with mst on the topic of resetting virtio devices,
> >> here is a series that hopefully clarifies the semantics of bus and
> >> device resets.
> >>
> >> After this series, there are two kinds of resets:
> >>
> >> 1) device-level reset is the kind of reset that you get with a register
> >> write on the device.  It will clear interrupts and DMAs among other things,
> >> but not any bus-level state, for example it will not clear PCI BARs and
> >> other configuration space data.  It is done with qdev_reset_all.
> >>
> >> 2) bus-level reset is the kind of reset that you get with a register
> >> write on the device that exports the bus (including triggering a device-level
> >> reset on the device that exports the bus).  It will do a device-level
> >> reset on the child, but also clear bus-level state such as PCI BARs and
> >> other configuration space data.  It can be triggered for all devices
> >> on a bus with qbus_reset_all.  There is still no API for a bus-level
> >> reset of a single device (like PCI FLR), this can be added later.
> > 
> > I don't really understand this dual abstraction.  I suspect it's
> > overgeneralizing something that's the result of poor modeling.
> 
> It's possible.  I'll move the SCSI bus away from qdev reset.
> Anthony/Michael, can you help doing the same with PCIDevice?  And
> perhaps Peter and Andreas with sysbus?
> 
> Paolo

I'm not sure what would you like to change with PCIDevice.

-- 
MST

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 09/01/2013 11:22, Michael S. Tsirkin ha scritto:
> > It's possible.  I'll move the SCSI bus away from qdev reset.
> > Anthony/Michael, can you help doing the same with PCIDevice?  And
> > perhaps Peter and Andreas with sysbus?
> 
> I'm not sure what would you like to change with PCIDevice.

Replace the DeviceState reset method with one in PCIDevice, and call it
from the PCI bus reset.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Wed, Jan 09, 2013 at 11:53:00AM +0100, Paolo Bonzini wrote:
> Il 09/01/2013 11:22, Michael S. Tsirkin ha scritto:
> > > It's possible.  I'll move the SCSI bus away from qdev reset.
> > > Anthony/Michael, can you help doing the same with PCIDevice?  And
> > > perhaps Peter and Andreas with sysbus?
> > 
> > I'm not sure what would you like to change with PCIDevice.
> 
> Replace the DeviceState reset method with one in PCIDevice, and call it
> from the PCI bus reset.
> 
> Paolo

You are talking about the call to qdev_reset_all(&dev->qdev)
in pci_device_reset, and you want to detect, there, that device is a bridge
and replace qdev_reset_all(&dev->qdev) with cast and call to pci_bus_reset
for the secondary bus?

-- 
MST

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 09/01/2013 12:09, Michael S. Tsirkin ha scritto:
> On Wed, Jan 09, 2013 at 11:53:00AM +0100, Paolo Bonzini wrote:
>> Il 09/01/2013 11:22, Michael S. Tsirkin ha scritto:
>>>> It's possible.  I'll move the SCSI bus away from qdev reset.
>>>> Anthony/Michael, can you help doing the same with PCIDevice?  And
>>>> perhaps Peter and Andreas with sysbus?
>>>
>>> I'm not sure what would you like to change with PCIDevice.
>>
>> Replace the DeviceState reset method with one in PCIDevice, and call it
>> from the PCI bus reset.
>>
>> Paolo
> 
> You are talking about the call to qdev_reset_all(&dev->qdev)
> in pci_device_reset

Yes.

> , and you want to detect, there, that device is a bridge
> and replace qdev_reset_all(&dev->qdev) with cast and call to pci_bus_reset
> for the secondary bus?

The bridge would call pci_bus_reset for its secondary bus in the
PCIDevice reset method.

I.e. all bus navigation would have to be explicit in the reset
callbacks.  I'm going to do that for the SCSI HBAs.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Wed, Jan 09, 2013 at 12:12:55PM +0100, Paolo Bonzini wrote:
> Il 09/01/2013 12:09, Michael S. Tsirkin ha scritto:
> > On Wed, Jan 09, 2013 at 11:53:00AM +0100, Paolo Bonzini wrote:
> >> Il 09/01/2013 11:22, Michael S. Tsirkin ha scritto:
> >>>> It's possible.  I'll move the SCSI bus away from qdev reset.
> >>>> Anthony/Michael, can you help doing the same with PCIDevice?  And
> >>>> perhaps Peter and Andreas with sysbus?
> >>>
> >>> I'm not sure what would you like to change with PCIDevice.
> >>
> >> Replace the DeviceState reset method with one in PCIDevice, and call it
> >> from the PCI bus reset.
> >>
> >> Paolo
> > 
> > You are talking about the call to qdev_reset_all(&dev->qdev)
> > in pci_device_reset
> 
> Yes.
> 
> > , and you want to detect, there, that device is a bridge
> > and replace qdev_reset_all(&dev->qdev) with cast and call to pci_bus_reset
> > for the secondary bus?
> 
> The bridge would call pci_bus_reset for its secondary bus in the
> PCIDevice reset method.
> 
> I.e. all bus navigation would have to be explicit in the reset
> callbacks.  I'm going to do that for the SCSI HBAs.
> 
> Paolo

OK you mean like this this? Completely untested - if you
and Anthony think we need this change please let me know
and I'll test and repost.

-->

pci: make secondary bus reset explicit

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 94840c4..ad81040 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -172,7 +172,7 @@ void pci_device_reset(PCIDevice *dev)
 {
     int r;
 
-    qdev_reset_all(&dev->qdev);
+    device_reset(&dev->qdev);
 
     dev->irq_state = 0;
     pci_update_irq_status(dev);
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 995842a..a08b479 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -258,10 +258,12 @@ void pci_bridge_disable_base_limit(PCIDevice *dev)
     pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0);
 }
 
-/* reset bridge specific configuration registers */
+/* reset bridge specific configuration registers.
+ * Propagate reset on the secondary bus. */
 void pci_bridge_reset(DeviceState *qdev)
 {
     PCIDevice *dev = PCI_DEVICE(qdev);
+    PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev);
     uint8_t *conf = dev->config;
 
     conf[PCI_PRIMARY_BUS] = 0;
@@ -295,6 +297,8 @@ void pci_bridge_reset(DeviceState *qdev)
     pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0);
 
     pci_set_word(conf + PCI_BRIDGE_CONTROL, 0);
+
+    pci_bus_reset(&br->sec_bus);
 }
 
 /* default qdev initialization function for PCI-to-PCI bridge */

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 09/01/2013 13:10, Michael S. Tsirkin ha scritto:
> OK you mean like this this? Completely untested - if you
> and Anthony think we need this change please let me know
> and I'll test and repost.
> 
> -->
> 
> pci: make secondary bus reset explicit
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Yes, but this can only be done after looking at all PCI devices that
have buses below (of which the bridge is a special case).  And Anthony
also metioned using a new method of PCIDevice instead of device_reset.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 09/01/2013 13:10, Michael S. Tsirkin ha scritto:
>> OK you mean like this this? Completely untested - if you
>> and Anthony think we need this change please let me know
>> and I'll test and repost.
>> 
>> -->
>> 
>> pci: make secondary bus reset explicit
>> 
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Yes, but this can only be done after looking at all PCI devices that
> have buses below (of which the bridge is a special case).  And Anthony
> also metioned using a new method of PCIDevice instead of device_reset.

Sorry, what's the bug here?  Is this a reset bug with cold reset or with
a form of warm reset?

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 09/01/2013 21:40, Anthony Liguori ha scritto:
>> > Yes, but this can only be done after looking at all PCI devices that
>> > have buses below (of which the bridge is a special case).  And Anthony
>> > also metioned using a new method of PCIDevice instead of device_reset.
> Sorry, what's the bug here?  Is this a reset bug with cold reset or with
> a form of warm reset?

Warm reset (virtio status register reset) of virtio-scsi wasn't
propagated down to the SCSI bus.  I fixed it using qdev_reset_all, but
now I'll do it manually in the HBA.  I'm fine with that as long as all
buses move away from device_reset.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Wed, Jan 09, 2013 at 10:22:58PM +0100, Paolo Bonzini wrote:
> Il 09/01/2013 21:40, Anthony Liguori ha scritto:
> >> > Yes, but this can only be done after looking at all PCI devices that
> >> > have buses below (of which the bridge is a special case).  And Anthony
> >> > also metioned using a new method of PCIDevice instead of device_reset.
> > Sorry, what's the bug here?  Is this a reset bug with cold reset or with
> > a form of warm reset?
> 
> Warm reset (virtio status register reset) of virtio-scsi wasn't
> propagated down to the SCSI bus.  I fixed it using qdev_reset_all, but
> now I'll do it manually in the HBA.  I'm fine with that as long as all
> buses move away from device_reset.
> 
> Paolo

You also pointed out some bug related to status register handling in virtio
on s390, right?

-- 
MST

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 09/01/2013 22:40, Michael S. Tsirkin ha scritto:
> > Warm reset (virtio status register reset) of virtio-scsi wasn't
> > propagated down to the SCSI bus.  I fixed it using qdev_reset_all, but
> > now I'll do it manually in the HBA.  I'm fine with that as long as all
> > buses move away from device_reset.
> 
> You also pointed out some bug related to status register handling in virtio
> on s390, right?

Yes, that would be a separate patch.  A large part of this series can
also be kept.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Thu, Jan 10, 2013 at 09:31:05AM +0100, Paolo Bonzini wrote:
> Il 09/01/2013 22:40, Michael S. Tsirkin ha scritto:
> > > Warm reset (virtio status register reset) of virtio-scsi wasn't
> > > propagated down to the SCSI bus.  I fixed it using qdev_reset_all, but
> > > now I'll do it manually in the HBA.  I'm fine with that as long as all
> > > buses move away from device_reset.
> > 
> > You also pointed out some bug related to status register handling in virtio
> > on s390, right?
> 
> Yes, that would be a separate patch.  A large part of this series can
> also be kept.
> 
> Paolo

Let's start with bugfixes, no need to bundle them in a series.

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Peter Maydell]

On 9 January 2013 09:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
> It's possible.  I'll move the SCSI bus away from qdev reset.
> Anthony/Michael, can you help doing the same with PCIDevice?  And
> perhaps Peter and Andreas with sysbus?

What does it even mean to reset a sysbus? Do we do it anywhere?
(it looks like vl.c does, just as a shortcut so memory mapped devices
get their reset hooks called?)

-- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 10/01/2013 12:46, Peter Maydell ha scritto:
>> > It's possible.  I'll move the SCSI bus away from qdev reset.
>> > Anthony/Michael, can you help doing the same with PCIDevice?  And
>> > perhaps Peter and Andreas with sysbus?
> What does it even mean to reset a sysbus? Do we do it anywhere?
> (it looks like vl.c does, just as a shortcut so memory mapped devices
> get their reset hooks called?)

Yes, exactly.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Peter Maydell]

On 10 January 2013 11:47, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/01/2013 12:46, Peter Maydell ha scritto:
>>> > It's possible.  I'll move the SCSI bus away from qdev reset.
>>> > Anthony/Michael, can you help doing the same with PCIDevice?  And
>>> > perhaps Peter and Andreas with sysbus?
>> What does it even mean to reset a sysbus? Do we do it anywhere?
>> (it looks like vl.c does, just as a shortcut so memory mapped devices
>> get their reset hooks called?)

So how should it work instead? I kind of feel like all qdev devices should
get their reset hook called on machine reset, regardless of bus [since it's
modelling power cycling the whole system], but would that break
something?

-- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 10/01/2013 12:59, Peter Maydell ha scritto:
>>>>> >>> > It's possible.  I'll move the SCSI bus away from qdev reset.
>>>>> >>> > Anthony/Michael, can you help doing the same with PCIDevice?  And
>>>>> >>> > perhaps Peter and Andreas with sysbus?
>>> >> What does it even mean to reset a sysbus? Do we do it anywhere?
>>> >> (it looks like vl.c does, just as a shortcut so memory mapped devices
>>> >> get their reset hooks called?)
> So how should it work instead? I kind of feel like all qdev devices should
> get their reset hook called on machine reset, regardless of bus [since it's
> modelling power cycling the whole system], but would that break
> something?

It's just an implementation detail.  Right now we have a common
callback.  The idea is to give each bus its own callback.  In the case
of sysbus it would just call a method; for PCI it would reset some
configuration and then call a method; for SCSI there is no need to call
a method at all; and so on.

In addition, navigating the qdev tree should be explicit in the methods.
 It will not happen anymore via the "magic" qdev_reset_all.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Peter Maydell]

On 10 January 2013 12:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/01/2013 12:59, Peter Maydell ha scritto:
>>>>>> >>> > It's possible.  I'll move the SCSI bus away from qdev reset.
>>>>>> >>> > Anthony/Michael, can you help doing the same with PCIDevice?  And
>>>>>> >>> > perhaps Peter and Andreas with sysbus?
>>>> >> What does it even mean to reset a sysbus? Do we do it anywhere?
>>>> >> (it looks like vl.c does, just as a shortcut so memory mapped devices
>>>> >> get their reset hooks called?)
>> So how should it work instead? I kind of feel like all qdev devices should
>> get their reset hook called on machine reset, regardless of bus [since it's
>> modelling power cycling the whole system], but would that break
>> something?
>
> It's just an implementation detail.  Right now we have a common
> callback.  The idea is to give each bus its own callback.  In the case
> of sysbus it would just call a method; for PCI it would reset some
> configuration and then call a method; for SCSI there is no need to call
> a method at all; and so on.

But machine reset shouldn't call bus specific PCI or SCSI reset
methods -- we've just effectively yanked the power to the VM
so everything should just reset as if it was freshly constructed.

A bus-specific reset method would be for buses where the bus
itself has some sort of guest-triggerable reset (by prodding the
chipset, for instance).

> In addition, navigating the qdev tree should be explicit in the methods.
>  It will not happen anymore via the "magic" qdev_reset_all.

*Something* has to say "call reset for every qdev object in the
system", surely?

-- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 10/01/2013 13:31, Peter Maydell ha scritto:
> On 10 January 2013 12:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 10/01/2013 12:59, Peter Maydell ha scritto:
>>>>>>>>>>> It's possible.  I'll move the SCSI bus away from qdev reset.
>>>>>>>>>>> Anthony/Michael, can you help doing the same with PCIDevice?  And
>>>>>>>>>>> perhaps Peter and Andreas with sysbus?
>>>>>>> What does it even mean to reset a sysbus? Do we do it anywhere?
>>>>>>> (it looks like vl.c does, just as a shortcut so memory mapped devices
>>>>>>> get their reset hooks called?)
>>> So how should it work instead? I kind of feel like all qdev devices should
>>> get their reset hook called on machine reset, regardless of bus [since it's
>>> modelling power cycling the whole system], but would that break
>>> something?
>>
>> It's just an implementation detail.  Right now we have a common
>> callback.  The idea is to give each bus its own callback.  In the case
>> of sysbus it would just call a method; for PCI it would reset some
>> configuration and then call a method; for SCSI there is no need to call
>> a method at all; and so on.
> 
> But machine reset shouldn't call bus specific PCI or SCSI reset
> methods -- we've just effectively yanked the power to the VM
> so everything should just reset as if it was freshly constructed.

Yes, but we do not have a way to do that, so QEMU resorts to a warm reset.

> A bus-specific reset method would be for buses where the bus
> itself has some sort of guest-triggerable reset (by prodding the
> chipset, for instance).

Yes.

>> In addition, navigating the qdev tree should be explicit in the methods.
>>  It will not happen anymore via the "magic" qdev_reset_all.
> 
> *Something* has to say "call reset for every qdev object in the
> system", surely?

It will call it on sysbus, which will call it on every sysbus child.
Devices that have a child bus will propagate it further down.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Peter Maydell]

On 10 January 2013 12:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/01/2013 13:31, Peter Maydell ha scritto:
>> On 10 January 2013 12:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> It's just an implementation detail.  Right now we have a common
>>> callback.  The idea is to give each bus its own callback.  In the case
>>> of sysbus it would just call a method; for PCI it would reset some
>>> configuration and then call a method; for SCSI there is no need to call
>>> a method at all; and so on.
>>
>> But machine reset shouldn't call bus specific PCI or SCSI reset
>> methods -- we've just effectively yanked the power to the VM
>> so everything should just reset as if it was freshly constructed.
>
> Yes, but we do not have a way to do that, so QEMU resorts to a warm reset.

qdev reset is cold reset, not warm reset.

>> A bus-specific reset method would be for buses where the bus
>> itself has some sort of guest-triggerable reset (by prodding the
>> chipset, for instance).
>
> Yes.
>
>>> In addition, navigating the qdev tree should be explicit in the methods.
>>>  It will not happen anymore via the "magic" qdev_reset_all.
>>
>> *Something* has to say "call reset for every qdev object in the
>> system", surely?
>
> It will call it on sysbus, which will call it on every sysbus child.
> Devices that have a child bus will propagate it further down.

This doesn't work when we get rid of the idea that every qdev
device is attached to a tree of buses.

(At this point in the conversation I'm pretty confused about exactly
what we're trying to do :-))

-- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 10/01/2013 14:01, Peter Maydell ha scritto:
> On 10 January 2013 12:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 10/01/2013 13:31, Peter Maydell ha scritto:
>>> On 10 January 2013 12:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> It's just an implementation detail.  Right now we have a common
>>>> callback.  The idea is to give each bus its own callback.  In the case
>>>> of sysbus it would just call a method; for PCI it would reset some
>>>> configuration and then call a method; for SCSI there is no need to call
>>>> a method at all; and so on.
>>>
>>> But machine reset shouldn't call bus specific PCI or SCSI reset
>>> methods -- we've just effectively yanked the power to the VM
>>> so everything should just reset as if it was freshly constructed.
>>
>> Yes, but we do not have a way to do that, so QEMU resorts to a warm reset.
> 
> qdev reset is cold reset, not warm reset.

Whatever. :)  It resorts to a reset (as if it was freshly constructed)
instead of _really_ doing a fresh construction.

>>> A bus-specific reset method would be for buses where the bus
>>> itself has some sort of guest-triggerable reset (by prodding the
>>> chipset, for instance).
>>
>> Yes.
>>
>>>> In addition, navigating the qdev tree should be explicit in the methods.
>>>>  It will not happen anymore via the "magic" qdev_reset_all.
>>>
>>> *Something* has to say "call reset for every qdev object in the
>>> system", surely?
>>
>> It will call it on sysbus, which will call it on every sysbus child.
>> Devices that have a child bus will propagate it further down.
> 
> This doesn't work when we get rid of the idea that every qdev
> device is attached to a tree of buses.

At least for now, devices that should be bus-free are sysbus devices,
aren't they?

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 January 2013 12:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 10/01/2013 12:59, Peter Maydell ha scritto:
>>>>>>> >>> > It's possible.  I'll move the SCSI bus away from qdev reset.
>>>>>>> >>> > Anthony/Michael, can you help doing the same with PCIDevice?  And
>>>>>>> >>> > perhaps Peter and Andreas with sysbus?
>>>>> >> What does it even mean to reset a sysbus? Do we do it anywhere?
>>>>> >> (it looks like vl.c does, just as a shortcut so memory mapped devices
>>>>> >> get their reset hooks called?)
>>> So how should it work instead? I kind of feel like all qdev devices should
>>> get their reset hook called on machine reset, regardless of bus [since it's
>>> modelling power cycling the whole system], but would that break
>>> something?
>>
>> It's just an implementation detail.  Right now we have a common
>> callback.  The idea is to give each bus its own callback.  In the case
>> of sysbus it would just call a method; for PCI it would reset some
>> configuration and then call a method; for SCSI there is no need to call
>> a method at all; and so on.
>
> But machine reset shouldn't call bus specific PCI or SCSI reset
> methods -- we've just effectively yanked the power to the VM
> so everything should just reset as if it was freshly constructed.
>
> A bus-specific reset method would be for buses where the bus
> itself has some sort of guest-triggerable reset (by prodding the
> chipset, for instance).

The challenge is how we go from what we have to what we want.

Right now we have DeviceState::reset.  This is used both as a soft and
hard reset.

What I would propose is that we:

  s/DeviceState::reset/DeviceState::hard_reset/g

Then introduce PCIDevice::soft_reset.  We can convert the PCI layer to
call soft_reset() instead of hard_reset.

Over time, it would be great if we could find a way to implement
hard_reset in terms of device destruction/recreation but we're not there
yet.

I think the reset/hard_reset rename can be done via sed mostly.

Would this solve the bug that you're trying to fix Michael/Paolo?

Regards,

Anthony Liguori


>> In addition, navigating the qdev tree should be explicit in the methods.
>>  It will not happen anymore via the "magic" qdev_reset_all.
>
> *Something* has to say "call reset for every qdev object in the
> system", surely?
>
> -- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 10/01/2013 15:14, Anthony Liguori ha scritto:
> What I would propose is that we:
> 
>   s/DeviceState::reset/DeviceState::hard_reset/g
> 
> Then introduce PCIDevice::soft_reset.  We can convert the PCI layer to
> call soft_reset() instead of hard_reset.
> 
> Over time, it would be great if we could find a way to implement
> hard_reset in terms of device destruction/recreation but we're not there
> yet.
> 
> I think the reset/hard_reset rename can be done via sed mostly.
> 
> Would this solve the bug that you're trying to fix Michael/Paolo?

I can fix the bug easily just in virtio-scsi.  I don't want anywone to
trip on it again due to false expectations about reset, though.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Thu, Jan 10, 2013 at 08:14:43AM -0600, Anthony Liguori wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On 10 January 2013 12:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> Il 10/01/2013 12:59, Peter Maydell ha scritto:
> >>>>>>> >>> > It's possible.  I'll move the SCSI bus away from qdev reset.
> >>>>>>> >>> > Anthony/Michael, can you help doing the same with PCIDevice?  And
> >>>>>>> >>> > perhaps Peter and Andreas with sysbus?
> >>>>> >> What does it even mean to reset a sysbus? Do we do it anywhere?
> >>>>> >> (it looks like vl.c does, just as a shortcut so memory mapped devices
> >>>>> >> get their reset hooks called?)
> >>> So how should it work instead? I kind of feel like all qdev devices should
> >>> get their reset hook called on machine reset, regardless of bus [since it's
> >>> modelling power cycling the whole system], but would that break
> >>> something?
> >>
> >> It's just an implementation detail.  Right now we have a common
> >> callback.  The idea is to give each bus its own callback.  In the case
> >> of sysbus it would just call a method; for PCI it would reset some
> >> configuration and then call a method; for SCSI there is no need to call
> >> a method at all; and so on.
> >
> > But machine reset shouldn't call bus specific PCI or SCSI reset
> > methods -- we've just effectively yanked the power to the VM
> > so everything should just reset as if it was freshly constructed.
> >
> > A bus-specific reset method would be for buses where the bus
> > itself has some sort of guest-triggerable reset (by prodding the
> > chipset, for instance).
> 
> The challenge is how we go from what we have to what we want.
> 
> Right now we have DeviceState::reset.  This is used both as a soft and
> hard reset.
> 
> What I would propose is that we:
> 
>   s/DeviceState::reset/DeviceState::hard_reset/g
> 
> Then introduce PCIDevice::soft_reset.  We can convert the PCI layer to
> call soft_reset() instead of hard_reset.
> 
> Over time, it would be great if we could find a way to implement
> hard_reset in terms of device destruction/recreation but we're not there
> yet.
> 
> I think the reset/hard_reset rename can be done via sed mostly.
> 
> Would this solve the bug that you're trying to fix Michael/Paolo?
> 
> Regards,
> 
> Anthony Liguori

I don't think we need a rename to fix a specific bug.
The bugs Paolo found can be fixed in virtio and virtio-scsi.

> 
> >> In addition, navigating the qdev tree should be explicit in the methods.
> >>  It will not happen anymore via the "magic" qdev_reset_all.
> >
> > *Something* has to say "call reset for every qdev object in the
> > system", surely?
> >
> > -- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Mon, Dec 17, 2012 at 05:24:35PM +0100, Paolo Bonzini wrote:
> After discussion with mst on the topic of resetting virtio devices,
> here is a series that hopefully clarifies the semantics of bus and
> device resets.

what started all this is a bug that affects virtio scsi.
Since there's still a lot of argument about reworking reset,
how about we fix the bug first, and attempt to rework reset
on top?
Will help stable etc as well.

-- 
MST