[Qemu-devel] [PATCH 0/2] virtio: reset all qbuses too when writing to the status field

[Qemu-devel] [PATCH 0/2] virtio: reset all qbuses too when writing to the status field

[Paolo Bonzini]

virtio devices are not performing a full reset when zero is written
to the status field, because the reset does not propagate down the qdev
bus hierarchy.

These patches fix this problem by calling qdev_reset_all when zero
is written to the status field.

Paolo Bonzini (2):
  virtio-pci: reset all qbuses too when writing to the status field
  virtio-s390: reset all qbuses too when writing to the status field

 hw/s390-virtio-bus.c |  8 +++++++-
 hw/virtio-pci.c      | 25 ++++++++++---------------
 2 files changed, 17 insertions(+), 16 deletions(-)

-- 
1.8.0.1

[Qemu-devel] [PATCH 1/2] 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 field.  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).

The patch calls qdev_reset_all, which calls virtio_pci_reset,
instead of basically inlining the contents of the latter.

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 | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 71f4fb5..a1685f1 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_pci_stop_ioeventfd(proxy);
-            virtio_reset(proxy->vdev);
-            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,19 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         }
         break;
     case VIRTIO_PCI_STATUS:
-        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) {
-            virtio_reset(proxy->vdev);
-            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.1

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

[Michael S. Tsirkin]

On Wed, Dec 12, 2012 at 03:26:35PM +0100, Paolo Bonzini wrote:
> virtio-pci devices do not perform a full reset when zero is written
> to the status field.  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).
> 
> The patch calls qdev_reset_all, which calls virtio_pci_reset,
> instead of basically inlining the contents of the latter.
> 
> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

This is a device specific register, IMO it should reset
very specific things not what happens to be on the bus.
For example qdev resets the PCI header: will or
will not this reset it? It should not but no easy way to figure out.

Can't the required code just go into the virtio-scsi
reset callback?

> ---
>  hw/virtio-pci.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 71f4fb5..a1685f1 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_pci_stop_ioeventfd(proxy);
> -            virtio_reset(proxy->vdev);
> -            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,19 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>          }
>          break;
>      case VIRTIO_PCI_STATUS:
> -        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) {
> -            virtio_reset(proxy->vdev);
> -            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.1
> 

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

[Paolo Bonzini]

Il 12/12/2012 15:44, Michael S. Tsirkin ha scritto:
> On Wed, Dec 12, 2012 at 03:26:35PM +0100, Paolo Bonzini wrote:
>> virtio-pci devices do not perform a full reset when zero is written
>> to the status field.  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).
>>
>> The patch calls qdev_reset_all, which calls virtio_pci_reset,
>> instead of basically inlining the contents of the latter.
>>
>> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> This is a device specific register, IMO it should reset
> very specific things not what happens to be on the bus.
> For example qdev resets the PCI header: will or
> will not this reset it?

qdev does not reset the PCI header.  Only pci_device_reset (called when
resetting the whole bus and also by FLR) does.

> It should not but no easy way to figure out.

qdev_reset_all is not FLR.  If you don't like direct usage of
qdev_reset_all, let's add a pci_device_soft_reset function that just
calls qdev_reset_all.  This way it is more self-documenting.

Other virtio implementations may not have an equivalent of FLR on their
bus and do a hard-reset when 0 is written to the status field.  The
virtio spec is silent on this---they can do it if they want.

> Can't the required code just go into the virtio-scsi
> reset callback?

Of course yes, but it'd be different from all other SCSI adapters then.
 They don't expect that they need to do anything to reset devices on
their SCSI bus.

Paolo

>> ---
>>  hw/virtio-pci.c | 25 ++++++++++---------------
>>  1 file changed, 10 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index 71f4fb5..a1685f1 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_pci_stop_ioeventfd(proxy);
>> -            virtio_reset(proxy->vdev);
>> -            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,19 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>          }
>>          break;
>>      case VIRTIO_PCI_STATUS:
>> -        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) {
>> -            virtio_reset(proxy->vdev);
>> -            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.1
>>

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

[Michael S. Tsirkin]

On Wed, Dec 12, 2012 at 04:20:08PM +0100, Paolo Bonzini wrote:
> Il 12/12/2012 15:44, Michael S. Tsirkin ha scritto:
> > On Wed, Dec 12, 2012 at 03:26:35PM +0100, Paolo Bonzini wrote:
> >> virtio-pci devices do not perform a full reset when zero is written
> >> to the status field.  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).
> >>
> >> The patch calls qdev_reset_all, which calls virtio_pci_reset,
> >> instead of basically inlining the contents of the latter.
> >>
> >> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > This is a device specific register, IMO it should reset
> > very specific things not what happens to be on the bus.
> > For example qdev resets the PCI header: will or
> > will not this reset it?
> 
> qdev does not reset the PCI header.  Only pci_device_reset (called when
> resetting the whole bus and also by FLR) does.

Point is it's a simple register, the easier it is to understand
what happens on this write the better.

> > It should not but no easy way to figure out.
> 
> qdev_reset_all is not FLR.  If you don't like direct usage of
> qdev_reset_all, let's add a pci_device_soft_reset function that just
> calls qdev_reset_all.  This way it is more self-documenting.
> 
> Other virtio implementations may not have an equivalent of FLR on their
> bus and do a hard-reset when 0 is written to the status field.  The
> virtio spec is silent on this---they can do it if they want.

guests expect sane behaviour, losing bios-assigned registers
on a device specific write wouldn't be sane.


> > Can't the required code just go into the virtio-scsi
> > reset callback?
> 
> Of course yes, but it'd be different from all other SCSI adapters then.
> They don't expect that they need to do anything to reset devices on
> their SCSI bus.
> 
> Paolo

On virtio level, it's a device specific register, there's nothing
standard about it.
If you are talking about some scsi thing here it belongs in
virtio scsi, but apparently same applies to virtio-blk which
really has no block bus.

> >> ---
> >>  hw/virtio-pci.c | 25 ++++++++++---------------
> >>  1 file changed, 10 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >> index 71f4fb5..a1685f1 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_pci_stop_ioeventfd(proxy);
> >> -            virtio_reset(proxy->vdev);
> >> -            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,19 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>          }
> >>          break;
> >>      case VIRTIO_PCI_STATUS:
> >> -        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) {
> >> -            virtio_reset(proxy->vdev);
> >> -            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.1
> >>

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

[Paolo Bonzini]

Il 12/12/2012 16:33, Michael S. Tsirkin ha scritto:
>>> This is a device specific register, IMO it should reset
>>> very specific things not what happens to be on the bus.
>>> For example qdev resets the PCI header: will or
>>> will not this reset it?
>>
>> qdev does not reset the PCI header.  Only pci_device_reset (called when
>> resetting the whole bus and also by FLR) does.
> 
> Point is it's a simple register, the easier it is to understand
> what happens on this write the better.

The easiest way to understand what happens is to have a definition in
the spec.  There is none, so to me the simplest definition of resetting
the device is "soft-reset the device", and in qdev a soft-reset is
qdev_reset_all (see below).

Whether it involves function pointers or not, I don't care.

>>> Can't the required code just go into the virtio-scsi
>>> reset callback?
>>
>> Of course yes, but it'd be different from all other SCSI adapters then.
>> They don't expect that they need to do anything to reset devices on
>> their SCSI bus.
> 
> On virtio level, it's a device specific register, there's nothing 
> standard about it. If you are talking about some scsi thing here it
> belongs in virtio scsi

No, it's a generic thing.  Other virtio devices may have a bus, and they
should reset it just as well.  virtio-serial's guest_reset function
closes each port from its own reset callback manually (since commit
4399722, virtio-serial-bus: Unset guest_connected at reset and driver
reset, 2012-04-24).  That shouldn't even exist.  The ports should close
themselves when a reset signal reaches them, and the propagation of the
reset should happen automatically just like IMO it should for virtio-scsi.

Let's not hide the basic concepts behind "it's a SCSI thing".  The
concept here is that of a soft and hard reset, and these is the definition:

- you soft-reset a device by writing to a register which is in the spec
of the device (e.g., write zero to the status register);

- you hard-reset a device by writing to a register which is in the spec
of the bus (e.g., FLR);

- hard reset is a superset of soft reset;

- soft-resetting device X will also do a hard reset of all devices below X.

For example, a soft-reset of a PCI-to-PCI bridge does a hard-reset of
all devices below (qdev_reset_all calls pcibus_reset calls
pci_device_reset).

In QEMU, qdev_reset_all is a soft reset of a device.  qbus_reset_all_fn
is a hard reset of all devices below a particular device.  There is no
generic qdev function for a hard reset of a particular device
(pci_device_reset is it for the PCI case, just to complete the
nomenclature with something you're familiar with).

These are all generic concepts.  Nothing virtio-specific, nothing
SCSI-specific.  It's not open for questioning. :)

In the virtio case you have (logically, not at the qdev level):

           virtio-pci
                |
           virtio-scsi
          /     |     \
        disk   disk   disk

Writing zero to the status register does a soft reset of the virtio-pci
device.  This includes a hard reset of the virtio-scsi device, and a
hard reset of the disks.  That can be easily modeled by
qdev_reset_all(pci_dev).

What we actually have at the qdev level is:

           virtio-pci  ---> virtio-scsi (via ->vdev pointer)
          /     |    \
       disk   disk   disk

The soft reset of the virtio-scsi device is done by virtio_reset instead
of walking the qdev bus, but even in this case the soft reset is
perfectly described by qdev_reset_all.

Hence, writing zero to the status register should use qdev_reset_all.

> but apparently same applies to virtio-blk which
> really has no block bus.

virtio-blk has no bus, so it does the reset from its own virtio_reset
callback.

virtio-scsi needs to ask the SCSIDevices to reset themselves.  Whatever
virtio-blk does in its reset callback, the SCSIDevices will do---only in
a "regular" qdev reset rather than a special-purpose reset callback.

I can of course iterate on all the devices, but it is pointless when
there is already a perfectly good callback to do a soft reset, and it's
called qdev_reset_all.

Paolo

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

[Michael S. Tsirkin]

On Wed, Dec 12, 2012 at 05:37:15PM +0100, Paolo Bonzini wrote:
> Il 12/12/2012 16:33, Michael S. Tsirkin ha scritto:
> >>> This is a device specific register, IMO it should reset
> >>> very specific things not what happens to be on the bus.
> >>> For example qdev resets the PCI header: will or
> >>> will not this reset it?
> >>
> >> qdev does not reset the PCI header.  Only pci_device_reset (called when
> >> resetting the whole bus and also by FLR) does.
> > 
> > Point is it's a simple register, the easier it is to understand
> > what happens on this write the better.
> 
> The easiest way to understand what happens is to have a definition in
> the spec.  There is none,

I think we should improve the spec.

> so to me the simplest definition of resetting
> the device is "soft-reset the device", and in qdev a soft-reset is
> qdev_reset_all (see below).

That's kind of vague.

> Whether it involves function pointers or not, I don't care.
> 
> >>> Can't the required code just go into the virtio-scsi
> >>> reset callback?
> >>
> >> Of course yes, but it'd be different from all other SCSI adapters then.
> >> They don't expect that they need to do anything to reset devices on
> >> their SCSI bus.
> > 
> > On virtio level, it's a device specific register, there's nothing 
> > standard about it. If you are talking about some scsi thing here it
> > belongs in virtio scsi
> 
> No, it's a generic thing.  Other virtio devices may have a bus, and they
> should reset it just as well.  virtio-serial's guest_reset function
> closes each port from its own reset callback manually (since commit
> 4399722, virtio-serial-bus: Unset guest_connected at reset and driver
> reset, 2012-04-24).  That shouldn't even exist.  The ports should close
> themselves when a reset signal reaches them, and the propagation of the
> reset should happen automatically just like IMO it should for virtio-scsi.
> 
> Let's not hide the basic concepts behind "it's a SCSI thing".  The
> concept here is that of a soft and hard reset, and these is the definition:
> 
> - you soft-reset a device by writing to a register which is in the spec
> of the device (e.g., write zero to the status register);
> 
> - you hard-reset a device by writing to a register which is in the spec
> of the bus (e.g., FLR);
>
> - hard reset is a superset of soft reset;
> 
> - soft-resetting device X will also do a hard reset of all devices below X.
> 
> For example, a soft-reset of a PCI-to-PCI bridge does a hard-reset of
> all devices below (qdev_reset_all calls pcibus_reset calls
> pci_device_reset).
> 
> In QEMU, qdev_reset_all is a soft reset of a device.
>  qbus_reset_all_fn
> is a hard reset of all devices below a particular device.

I might be convinced if it's renamed qdev_reset_soft,
qbus_reset_hard.
As it is it, these look like internal interfaces
that one needs to poke at implementation to figure out.

> There is no
> generic qdev function for a hard reset of a particular device
> (pci_device_reset is it for the PCI case, just to complete the
> nomenclature with something you're familiar with).
> 
> These are all generic concepts.  Nothing virtio-specific, nothing
> SCSI-specific.  It's not open for questioning. :)

When we have a similar issue in another device it
will be easier to see how to do it right.
At the moment let's fix it locally.

> In the virtio case you have (logically, not at the qdev level):
> 
>            virtio-pci
>                 |
>            virtio-scsi
>           /     |     \
>         disk   disk   disk
> 
> Writing zero to the status register does a soft reset of the virtio-pci
> device.  This includes a hard reset of the virtio-scsi device, and a
> hard reset of the disks.  That can be easily modeled by
> qdev_reset_all(pci_dev).
> 
> What we actually have at the qdev level is:
> 
>            virtio-pci  ---> virtio-scsi (via ->vdev pointer)
>           /     |    \
>        disk   disk   disk
> 
> The soft reset of the virtio-scsi device is done by virtio_reset instead
> of walking the qdev bus, but even in this case the soft reset is
> perfectly described by qdev_reset_all.
> 
> Hence, writing zero to the status register should use qdev_reset_all.

OK the right thing would be to to qdev_reset_all
at the virtio scsi level. The modeling
is wrong though and the devices are not the
children of the right device.

Sticking the reset in virtio-pci just propagates this bug.

> > but apparently same applies to virtio-blk which
> > really has no block bus.
> 
> virtio-blk has no bus, so it does the reset from its own virtio_reset
> callback.
> 
> virtio-scsi needs to ask the SCSIDevices to reset themselves.  Whatever
> virtio-blk does in its reset callback, the SCSIDevices will do---only in
> a "regular" qdev reset rather than a special-purpose reset callback.
> 
> I can of course iterate on all the devices, but it is pointless when
> there is already a perfectly good callback to do a soft reset, and it's
> called qdev_reset_all.
> 
> Paolo

Fine bug call it from virtio_scsi_reset.

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

[Paolo Bonzini]

>> No, it's a generic thing.  Other virtio devices may have a bus, and they
>> should reset it just as well.  virtio-serial's guest_reset function
>> closes each port from its own reset callback manually (since commit
>> 4399722, virtio-serial-bus: Unset guest_connected at reset and driver
>> reset, 2012-04-24).  That shouldn't even exist.  The ports should close
>> themselves when a reset signal reaches them, and the propagation of the
>> reset should happen automatically just like IMO it should for virtio-scsi.
>>
>> Let's not hide the basic concepts behind "it's a SCSI thing".  The
>> concept here is that of a soft and hard reset, and these is the definition:
>>
>> - you soft-reset a device by writing to a register which is in the spec
>> of the device (e.g., write zero to the status register);
>>
>> - you hard-reset a device by writing to a register which is in the spec
>> of the bus (e.g., FLR);
>>
>> - hard reset is a superset of soft reset;
>>
>> - soft-resetting device X will also do a hard reset of all devices below X.
>>
>> For example, a soft-reset of a PCI-to-PCI bridge does a hard-reset of
>> all devices below (qdev_reset_all calls pcibus_reset calls
>> pci_device_reset).
>>
>> In QEMU, qdev_reset_all is a soft reset of a device.  qbus_reset_all_fn
>> is a hard reset of all devices below a particular device.
> 
> I might be convinced if it's renamed qdev_reset_soft,
> qbus_reset_hard.

Let's rename it.

> As it is it, these look like internal interfaces
> that one needs to poke at implementation to figure out.

Point taken.

>> There is no
>> generic qdev function for a hard reset of a particular device
>> (pci_device_reset is it for the PCI case, just to complete the
>> nomenclature with something you're familiar with).
>>
>> These are all generic concepts.  Nothing virtio-specific, nothing
>> SCSI-specific.  It's not open for questioning. :)
> 
> When we have a similar issue in another device it
> will be easier to see how to do it right.
> At the moment let's fix it locally.

We have a similar issue in virtio-serial.c, and with my patch you can
do this:

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;
 

i.e. just implement a method on the bus to do the hard-reset, and let
generic infrastructure call it.

> OK the right thing would be to to qdev_reset_all
> at the virtio scsi level. The modeling
> is wrong though and the devices are not the
> children of the right device.
> 
> Sticking the reset in virtio-pci just propagates this bug.

Sticking a bus reset in virtio_pci_reset would.  But that's not
what my patch does.

>>> but apparently same applies to virtio-blk which
>>> really has no block bus.
>>
>> virtio-blk has no bus, so it does the reset from its own virtio_reset
>> callback.
>>
>> virtio-scsi needs to ask the SCSIDevices to reset themselves.  Whatever
>> virtio-blk does in its reset callback, the SCSIDevices will do---only in
>> a "regular" qdev reset rather than a special-purpose reset callback.
>>
>> I can of course iterate on all the devices, but it is pointless when
>> there is already a perfectly good callback to do a soft reset, and it's
>> called qdev_reset_all.
> 
> Fine bug call it from virtio_scsi_reset.

No, because that would also reset the virtio-pci bits (ioeventfd etc.) which
virtio-scsi has no business with.  What I could do is to call qbus_reset_all,
but it makes no sense to me when there is a generic solution.

Paolo

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

[Michael S. Tsirkin]

On Wed, Dec 12, 2012 at 06:29:54PM +0100, Paolo Bonzini wrote:
> 
> >> No, it's a generic thing.  Other virtio devices may have a bus, and they
> >> should reset it just as well.  virtio-serial's guest_reset function
> >> closes each port from its own reset callback manually (since commit
> >> 4399722, virtio-serial-bus: Unset guest_connected at reset and driver
> >> reset, 2012-04-24).  That shouldn't even exist.  The ports should close
> >> themselves when a reset signal reaches them, and the propagation of the
> >> reset should happen automatically just like IMO it should for virtio-scsi.
> >>
> >> Let's not hide the basic concepts behind "it's a SCSI thing".  The
> >> concept here is that of a soft and hard reset, and these is the definition:
> >>
> >> - you soft-reset a device by writing to a register which is in the spec
> >> of the device (e.g., write zero to the status register);
> >>
> >> - you hard-reset a device by writing to a register which is in the spec
> >> of the bus (e.g., FLR);
> >>
> >> - hard reset is a superset of soft reset;
> >>
> >> - soft-resetting device X will also do a hard reset of all devices below X.
> >>
> >> For example, a soft-reset of a PCI-to-PCI bridge does a hard-reset of
> >> all devices below (qdev_reset_all calls pcibus_reset calls
> >> pci_device_reset).
> >>
> >> In QEMU, qdev_reset_all is a soft reset of a device.  qbus_reset_all_fn
> >> is a hard reset of all devices below a particular device.
> > 
> > I might be convinced if it's renamed qdev_reset_soft,
> > qbus_reset_hard.
> 
> Let's rename it.
> 
> > As it is it, these look like internal interfaces
> > that one needs to poke at implementation to figure out.
> 
> Point taken.
> 
> >> There is no
> >> generic qdev function for a hard reset of a particular device
> >> (pci_device_reset is it for the PCI case, just to complete the
> >> nomenclature with something you're familiar with).
> >>
> >> These are all generic concepts.  Nothing virtio-specific, nothing
> >> SCSI-specific.  It's not open for questioning. :)
> > 
> > When we have a similar issue in another device it
> > will be easier to see how to do it right.
> > At the moment let's fix it locally.
> 
> We have a similar issue in virtio-serial.c, and with my patch you can
> do this:
> 
> 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;
>  
> 
> i.e. just implement a method on the bus to do the hard-reset, and let
> generic infrastructure call it.

I dislike this approach.
A specific function call is easier to follow.

> > OK the right thing would be to to qdev_reset_all
> > at the virtio scsi level. The modeling
> > is wrong though and the devices are not the
> > children of the right device.
> > 
> > Sticking the reset in virtio-pci just propagates this bug.
> 
> Sticking a bus reset in virtio_pci_reset would.  But that's not
> what my patch does.
> 
> >>> but apparently same applies to virtio-blk which
> >>> really has no block bus.
> >>
> >> virtio-blk has no bus, so it does the reset from its own virtio_reset
> >> callback.
> >>
> >> virtio-scsi needs to ask the SCSIDevices to reset themselves.  Whatever
> >> virtio-blk does in its reset callback, the SCSIDevices will do---only in
> >> a "regular" qdev reset rather than a special-purpose reset callback.
> >>
> >> I can of course iterate on all the devices, but it is pointless when
> >> there is already a perfectly good callback to do a soft reset, and it's
> >> called qdev_reset_all.
> > 
> > Fine bug call it from virtio_scsi_reset.
> 
> No, because that would also reset the virtio-pci bits (ioeventfd etc.) which
> virtio-scsi has no business with.  What I could do is to call qbus_reset_all,
> but it makes no sense to me when there is a generic solution.
> 
> Paolo

The virtio reset just resets virtio registers and stops DMA
and interrupts. That is all. If it's not clear from spec
we should make it clear.

The fact that virtio scsi needs to do something special
with qdev or whatever for this to happen is it's own business.
E.g. -net does it differently it checks state
before processing packets.
Generic  virtio core does not care.

-- 
MST

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

[Paolo Bonzini]

Il 12/12/2012 22:32, Michael S. Tsirkin ha scritto:
> > i.e. just implement a method on the bus to do the hard-reset, and let
> > generic infrastructure call it.
> 
> I dislike this approach.
> A specific function call is easier to follow.

"I dislike reading documentation, so I prefer to have ugly code than
good documentation".

The right way to fix it is to document the reset semantics, and use them.

> > No, because that would also reset the virtio-pci bits (ioeventfd etc.) which
> > virtio-scsi has no business with.  What I could do is to call qbus_reset_all,
> > but it makes no sense to me when there is a generic solution.
> > 
>
> The virtio reset just resets virtio registers and stops DMA
> and interrupts. That is all. If it's not clear from spec
> we should make it clear.
> 
> The fact that virtio scsi needs to do something special
> with qdev or whatever for this to happen is it's own business.

Same for virtio-console.  If you don't call qemu_chr_fe_close, the char
device layer ends up calling chr_read in virtio-console.c which
ultimately does DMA.

It's how most qdev buses work.  The bus callbacks includes a
parent->child interface to submit request and a child->parent interface
to DMA.  If you do not reset the children, you do not reset DMA.  Period.

> E.g. -net does it differently it checks state
> before processing packets.
> Generic  virtio core does not care.

Generic qdev core cares.  virtio is a qdev device and it should use
services provided by the generic framework, as much as you hate it.

Paolo

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

[Paolo Bonzini]

virtio-s390 devices do not perform a full reset when zero is written
to the status field.  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.

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 e0ac2d1..91d5a3a 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.1

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

[Michael S. Tsirkin]

On Wed, Dec 12, 2012 at 03:26:34PM +0100, Paolo Bonzini wrote:
> virtio devices are not performing a full reset when zero is written
> to the status field, because the reset does not propagate down the qdev
> bus hierarchy.
> 
> These patches fix this problem by calling qdev_reset_all when zero
> is written to the status field.

Looks like this is a virtio-scsi thing - others don't have a hierarchy.
Let's just stick this code in virtio_scsi_reset then?
It likely can access the scsi bus without going through virtio-pci
and s390 right?

> Paolo Bonzini (2):
>   virtio-pci: reset all qbuses too when writing to the status field
>   virtio-s390: reset all qbuses too when writing to the status field
> 
>  hw/s390-virtio-bus.c |  8 +++++++-
>  hw/virtio-pci.c      | 25 ++++++++++---------------
>  2 files changed, 17 insertions(+), 16 deletions(-)
> 
> -- 
> 1.8.0.1

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

[Paolo Bonzini]

Il 12/12/2012 16:38, Michael S. Tsirkin ha scritto:
>> > These patches fix this problem by calling qdev_reset_all when zero
>> > is written to the status field.
> Looks like this is a virtio-scsi thing - others don't have a hierarchy.

virtio-serial does, and is doing the same walk manually.

> Let's just stick this code in virtio_scsi_reset then?

No.

> It likely can access the scsi bus without going through virtio-pci
> and s390 right?

Yes, it can, but it's not a good reason to do it there.

Paolo

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

[Michael S. Tsirkin]

On Wed, Dec 12, 2012 at 05:38:21PM +0100, Paolo Bonzini wrote:
> Il 12/12/2012 16:38, Michael S. Tsirkin ha scritto:
> >> > These patches fix this problem by calling qdev_reset_all when zero
> >> > is written to the status field.
> > Looks like this is a virtio-scsi thing - others don't have a hierarchy.
> 
> virtio-serial does, and is doing the same walk manually.

So just do the same.

> > Let's just stick this code in virtio_scsi_reset then?
> 
> No.
> 
> > It likely can access the scsi bus without going through virtio-pci
> > and s390 right?
> 
> Yes, it can, but it's not a good reason to do it there.
> 
> Paolo

Maybe it's obvious to you that qdev_reset_all(x)
does a soft reset and what soft reset means
for each bus type, but it is not for me,
it is in particular not well defined for classical PCI,
and you said youself the hierarchy is wrong,
disks should be under virtio-scsi.
So until we have better qdev interfaces and modeling
in virtio I think it's better to avoid relying on it
in common virtio code. You maintain and understand
virtio scsi better maybe it makes sense there.

-- 
MST

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

[Paolo Bonzini]

Il 12/12/2012 22:27, Michael S. Tsirkin ha scritto:
>>> Maybe it's obvious to you that qdev_reset_all(x)
>>> does a soft reset and what soft reset means
>>> for each bus type
>>
>> We can define soft reset to be *independent* of the bus type.  As you
>> said, you access it with a device register.
> 
> I think qemu has one type of reset ATM which is the hard reset.

A hard reset would kill BARs and configuration space too.
qdev_reset_all doesn't.  Ergo, it is not a hard reset.

But hey, I'm not wed to names.  Let's call it device-level reset and
bus-level reset.  Whatever.

> Any other kind is device specific now.
> The fact that qdev has two APIs which are kind of but not
> exactly the same is a bug not a feature.

BusState reset and DeviceState reset are not the same because they are
triggered differently, one by bus-level functionality (e.g. FLR) and the
other by device-level functionality (e.g. a register).

That's neither a bug nor a feature.  It's just obvious.

> And relying on it in generic virtio is just going to
> confuse things.

It is going to confuse you perhaps, but it will not confuse whoever will
write the next virtio device with a bus.  Who knows, virtio-i2c.

> Further it does not follow that all backends are children
> of the frontend.

Backends are not children of the frontend.  Seriously, this is qdev 101.

virtio-scsi has no backend.  Frontends (disks) are children of
virtio-scsi.  Each backend (host block device) is connected to a child
of virtio-scsi.

virtio-scsi does not need to reset back-ends.  It needs to reset front-ends.

virtio-serial does not know it needs to reset back-ends.  It knows it
needs to reset front-ends.  Each reset of the front-end will also
propagate to a back-end, but virtio-serial need not know that.

> So please just fix the virtio-scsi bug for now and we can
> address the bigger issue if any later.

I'm refusing to fix the bug in virtio-scsi.  It is not a virtio-scsi
bug, as proved by the virtio-serial bus having to do the same things.
Two wrongs do not make a right, and here we have three wrongs already:
virtio-pci, virtio-s390, virtio-serial all reinventing the wheel.

Paolo

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

[Michael S. Tsirkin]

On Thu, Dec 13, 2012 at 09:54:23AM +0100, Paolo Bonzini wrote:
> Il 12/12/2012 22:27, Michael S. Tsirkin ha scritto:
> >>> Maybe it's obvious to you that qdev_reset_all(x)
> >>> does a soft reset and what soft reset means
> >>> for each bus type
> >>
> >> We can define soft reset to be *independent* of the bus type.  As you
> >> said, you access it with a device register.
> > 
> > I think qemu has one type of reset ATM which is the hard reset.
> 
> A hard reset would kill BARs and configuration space too.
> qdev_reset_all doesn't.  Ergo, it is not a hard reset.
> 
> But hey, I'm not wed to names.  Let's call it device-level reset and
> bus-level reset.  Whatever.

It's not a question of a name.

ATM qemu supports one kind of reset because it's a kind of reset all
hardware supports.  The moment we start inventing
our own one we need to document exactly what it means.


> > Any other kind is device specific now.
> > The fact that qdev has two APIs which are kind of but not
> > exactly the same is a bug not a feature.
> 
> BusState reset and DeviceState reset are not the same because they are
> triggered differently, one by bus-level functionality (e.g. FLR) and the
> other by device-level functionality (e.g. a register).
> 
> That's neither a bug nor a feature.  It's just obvious.
> 
> > And relying on it in generic virtio is just going to
> > confuse things.
> 
> It is going to confuse you perhaps, but it will not confuse whoever will
> write the next virtio device with a bus.  Who knows, virtio-i2c.
> 
> > Further it does not follow that all backends are children
> > of the frontend.
> 
> Backends are not children of the frontend.  Seriously, this is qdev 101.
> 
> virtio-scsi has no backend.  Frontends (disks) are children of
> virtio-scsi.  Each backend (host block device) is connected to a child
> of virtio-scsi.
> 
> virtio-scsi does not need to reset back-ends.  It needs to reset front-ends.
> 
> virtio-serial does not know it needs to reset back-ends.  It knows it
> needs to reset front-ends.  Each reset of the front-end will also
> propagate to a back-end, but virtio-serial need not know that.
> 
> > So please just fix the virtio-scsi bug for now and we can
> > address the bigger issue if any later.
> 
> I'm refusing to fix the bug in virtio-scsi.  It is not a virtio-scsi
> bug, as proved by the virtio-serial bus having to do the same things.
> Two wrongs do not make a right, and here we have three wrongs already:
> virtio-pci, virtio-s390, virtio-serial all reinventing the wheel.
> 
> Paolo

You have a point about a problem.

My problem is with the solution, this solution depends on the exact
modeling for correctness and that I don't want to do since we seem to be
re-shuffling what inherits what so often.

For example I could not figure out how the reset function for virtio pci
(which clears pending msix vectors so is required) was called by this.

Another thing that bothers me is that during regular PCI bus reset
virtio does not invoke qdev_reset_all but with this reset, it does.
Inconsistent.

-- 
MST

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

[Paolo Bonzini]

Il 16/12/2012 18:04, Michael S. Tsirkin ha scritto:
> On Thu, Dec 13, 2012 at 09:54:23AM +0100, Paolo Bonzini wrote:
>> Il 12/12/2012 22:27, Michael S. Tsirkin ha scritto:
>>>>> Maybe it's obvious to you that qdev_reset_all(x)
>>>>> does a soft reset and what soft reset means
>>>>> for each bus type
>>>>
>>>> We can define soft reset to be *independent* of the bus type.  As you
>>>> said, you access it with a device register.
>>>
>>> I think qemu has one type of reset ATM which is the hard reset.
>>
>> A hard reset would kill BARs and configuration space too.
>> qdev_reset_all doesn't.  Ergo, it is not a hard reset.
>>
>> But hey, I'm not wed to names.  Let's call it device-level reset and
>> bus-level reset.  Whatever.
> 
> It's not a question of a name.
> 
> ATM qemu supports one kind of reset because it's a kind of reset all
> hardware supports.  The moment we start inventing
> our own one we need to document exactly what it means.

We have two, DeviceClass's and BusClass's reset members.

I'll make a patch to document them.

> You have a point about a problem.
> 
> My problem is with the solution, this solution depends on the exact
> modeling for correctness and that I don't want to do since we seem to be
> re-shuffling what inherits what so often.
> 
> For example I could not figure out how the reset function for virtio pci
> (which clears pending msix vectors so is required) was called by this.

The same way a PCI bus reset clears pending MSIX vectors.

qdev_reset_all(pci_dev)
   -> qdev_walk_children(pci_dev, qdev_reset_one, qbus_reset_one, NULL);
   -> qdev_reset_one(pci_dev, NULL);
   -> device_reset(pci_dev);
   -> calls dc->reset member set for virtio-*-pci, i.e. virtio_pci_reset

> Another thing that bothers me is that during regular PCI bus reset
> virtio does not invoke qdev_reset_all but with this reset, it does.
> Inconsistent.

It does.

A PCI bus reset (or FLR) calls pci_device_reset which does this

void pci_device_reset(PCIDevice *dev)
{
    int r;

    qdev_reset_all(&dev->qdev);
    ...
}

This is exactly how a PCI bus reset clears pending MSIX vectors.

Paolo

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

[Michael S. Tsirkin]

On Sun, Dec 16, 2012 at 08:31:16PM +0100, Paolo Bonzini wrote:
> Il 16/12/2012 18:04, Michael S. Tsirkin ha scritto:
> > On Thu, Dec 13, 2012 at 09:54:23AM +0100, Paolo Bonzini wrote:
> >> Il 12/12/2012 22:27, Michael S. Tsirkin ha scritto:
> >>>>> Maybe it's obvious to you that qdev_reset_all(x)
> >>>>> does a soft reset and what soft reset means
> >>>>> for each bus type
> >>>>
> >>>> We can define soft reset to be *independent* of the bus type.  As you
> >>>> said, you access it with a device register.
> >>>
> >>> I think qemu has one type of reset ATM which is the hard reset.
> >>
> >> A hard reset would kill BARs and configuration space too.
> >> qdev_reset_all doesn't.  Ergo, it is not a hard reset.
> >>
> >> But hey, I'm not wed to names.  Let's call it device-level reset and
> >> bus-level reset.  Whatever.
> > 
> > It's not a question of a name.
> > 
> > ATM qemu supports one kind of reset because it's a kind of reset all
> > hardware supports.  The moment we start inventing
> > our own one we need to document exactly what it means.
> 
> We have two, DeviceClass's and BusClass's reset members.
> 
> I'll make a patch to document them.
> 
> > You have a point about a problem.
> > 
> > My problem is with the solution, this solution depends on the exact
> > modeling for correctness and that I don't want to do since we seem to be
> > re-shuffling what inherits what so often.
> > 
> > For example I could not figure out how the reset function for virtio pci
> > (which clears pending msix vectors so is required) was called by this.
> 
> The same way a PCI bus reset clears pending MSIX vectors.
> 
> qdev_reset_all(pci_dev)
>    -> qdev_walk_children(pci_dev, qdev_reset_one, qbus_reset_one, NULL);
>    -> qdev_reset_one(pci_dev, NULL);
>    -> device_reset(pci_dev);
>    -> calls dc->reset member set for virtio-*-pci, i.e. virtio_pci_reset
> 
> > Another thing that bothers me is that during regular PCI bus reset
> > virtio does not invoke qdev_reset_all but with this reset, it does.
> > Inconsistent.
> 
> It does.
> 
> A PCI bus reset (or FLR) calls pci_device_reset which does this
> 
> void pci_device_reset(PCIDevice *dev)
> {
>     int r;
> 
>     qdev_reset_all(&dev->qdev);
>     ...
> }
> 
> This is exactly how a PCI bus reset clears pending MSIX vectors.
> 
> Paolo

Yes but I mean virtio-pci device is not a bus.
virtio could be a bus.
Our modeling seems incorrect and relying on it for correctness,
looks a bit wrong.

-- 
MST

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

[Paolo Bonzini]

Il 16/12/2012 22:15, Michael S. Tsirkin ha scritto:
>> A PCI bus reset (or FLR) calls pci_device_reset which does this
>>
>> void pci_device_reset(PCIDevice *dev)
>> {
>>     int r;
>>
>>     qdev_reset_all(&dev->qdev);
>>     ...
>> }
>>
>> This is exactly how a PCI bus reset clears pending MSIX vectors.
>>
>> Paolo
> 
> Yes but I mean virtio-pci device is not a bus.
> virtio could be a bus.
> Our modeling seems incorrect and relying on it for correctness,
> looks a bit wrong.

If virtio were a bus, everything would work just as well.  The bus reset
method would not be implemented and qdev_reset_all would proceed past
the bus and reset the virtio device.  So virtio_reset would not be
needed either.

Paolo

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

[Michael S. Tsirkin]

On Sun, Dec 16, 2012 at 08:31:16PM +0100, Paolo Bonzini wrote:
> Il 16/12/2012 18:04, Michael S. Tsirkin ha scritto:
> > On Thu, Dec 13, 2012 at 09:54:23AM +0100, Paolo Bonzini wrote:
> >> Il 12/12/2012 22:27, Michael S. Tsirkin ha scritto:
> >>>>> Maybe it's obvious to you that qdev_reset_all(x)
> >>>>> does a soft reset and what soft reset means
> >>>>> for each bus type
> >>>>
> >>>> We can define soft reset to be *independent* of the bus type.  As you
> >>>> said, you access it with a device register.
> >>>
> >>> I think qemu has one type of reset ATM which is the hard reset.
> >>
> >> A hard reset would kill BARs and configuration space too.
> >> qdev_reset_all doesn't.  Ergo, it is not a hard reset.
> >>
> >> But hey, I'm not wed to names.  Let's call it device-level reset and
> >> bus-level reset.  Whatever.
> > 
> > It's not a question of a name.
> > 
> > ATM qemu supports one kind of reset because it's a kind of reset all
> > hardware supports.  The moment we start inventing
> > our own one we need to document exactly what it means.
> 
> We have two, DeviceClass's and BusClass's reset members.
> 
> I'll make a patch to document them.

And qdev_reset_all needs documentation too.
Another issue is that this really should be
part of generic code, not transport-specific one.

The reason we can't do this is because virtio
does not have access to DeviceState: all it has
is a void * binding opaque.
However this is not really a feature - it seems we
should be able to pass DeviceState instead.

How about the following? Then we can put reset
in generic code where it belongs.
It's untested - really kind of pseudo code - and
s390 is still to be updated.

Posting to see what does everyone thinks.

---

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 3ea4140..63ae888 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);
 
 /* virtio device */
 
-static void virtio_pci_notify(void *opaque, uint16_t vector)
+static void virtio_pci_notify(DeviceState *d, uint16_t vector)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     if (msix_enabled(&proxy->pci_dev))
         msix_notify(&proxy->pci_dev, vector);
     else
         qemu_set_irq(proxy->pci_dev.irq[0], proxy->vdev->isr & 1);
 }
 
-static void virtio_pci_save_config(void * opaque, QEMUFile *f)
+static void virtio_pci_save_config(DeviceState *d, QEMUFile *f)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     pci_device_save(&proxy->pci_dev, f);
     msix_save(&proxy->pci_dev, f);
     if (msix_present(&proxy->pci_dev))
         qemu_put_be16(f, proxy->vdev->config_vector);
 }
 
-static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
+static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     if (msix_present(&proxy->pci_dev))
         qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n));
 }
 
-static int virtio_pci_load_config(void * opaque, QEMUFile *f)
+static int virtio_pci_load_config(DeviceState *d, QEMUFile *f)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     int ret;
     ret = pci_device_load(&proxy->pci_dev, f);
     if (ret) {
@@ -144,9 +144,9 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
     return 0;
 }
 
-static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f)
+static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     uint16_t vector;
     if (msix_present(&proxy->pci_dev)) {
         qemu_get_be16s(f, &vector);
@@ -464,9 +464,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
     }
 }
 
-static unsigned virtio_pci_get_features(void *opaque)
+static unsigned virtio_pci_get_features(DeviceState *d)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     return proxy->host_features;
 }
 
@@ -568,9 +568,9 @@ static void kvm_virtio_pci_vector_release(PCIDevice *dev, unsigned vector)
     }
 }
 
-static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign)
+static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     VirtQueue *vq = virtio_get_queue(proxy->vdev, n);
     EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
 
@@ -588,15 +588,15 @@ static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign)
     return 0;
 }
 
-static bool virtio_pci_query_guest_notifiers(void *opaque)
+static bool virtio_pci_query_guest_notifiers(DeviceState *d)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     return msix_enabled(&proxy->pci_dev);
 }
 
-static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
+static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     VirtIODevice *vdev = proxy->vdev;
     int r, n;
 
@@ -612,7 +612,7 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
             break;
         }
 
-        r = virtio_pci_set_guest_notifier(opaque, n, assign);
+        r = virtio_pci_set_guest_notifier(d, n, assign);
         if (r < 0) {
             goto assign_error;
         }
@@ -638,14 +638,14 @@ assign_error:
     /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */
     assert(assign);
     while (--n >= 0) {
-        virtio_pci_set_guest_notifier(opaque, n, !assign);
+        virtio_pci_set_guest_notifier(d, n, !assign);
     }
     return r;
 }
 
-static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign)
+static int virtio_pci_set_host_notifier(DeviceState *d, int n, bool assign)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
 
     /* Stop using ioeventfd for virtqueue kick if the device starts using host
      * notifiers.  This makes it easy to avoid stepping on each others' toes.
@@ -661,9 +661,9 @@ static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign)
     return virtio_pci_set_host_notifier_internal(proxy, n, assign, false);
 }
 
-static void virtio_pci_vmstate_change(void *opaque, bool running)
+static void virtio_pci_vmstate_change(DeviceState *d, bool running)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
 
     if (running) {
         /* Try to find out if the guest has bus master disabled, but is
diff --git a/hw/virtio.h b/hw/virtio.h
index 7c17f7b..b9eeaa5 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -91,17 +91,17 @@ typedef struct VirtQueueElement
 } VirtQueueElement;
 
 typedef struct {
-    void (*notify)(void * opaque, uint16_t vector);
-    void (*save_config)(void * opaque, QEMUFile *f);
-    void (*save_queue)(void * opaque, int n, QEMUFile *f);
-    int (*load_config)(void * opaque, QEMUFile *f);
-    int (*load_queue)(void * opaque, int n, QEMUFile *f);
-    int (*load_done)(void * opaque, QEMUFile *f);
-    unsigned (*get_features)(void * opaque);
-    bool (*query_guest_notifiers)(void * opaque);
-    int (*set_guest_notifiers)(void * opaque, bool assigned);
-    int (*set_host_notifier)(void * opaque, int n, bool assigned);
-    void (*vmstate_change)(void * opaque, bool running);
+    void (*notify)(DeviceState *d, uint16_t vector);
+    void (*save_config)(DeviceState *d, QEMUFile *f);
+    void (*save_queue)(DeviceState *d, int n, QEMUFile *f);
+    int (*load_config)(DeviceState *d, QEMUFile *f);
+    int (*load_queue)(DeviceState *d, int n, QEMUFile *f);
+    int (*load_done)(DeviceState *d, QEMUFile *f);
+    unsigned (*get_features)(DeviceState *d);
+    bool (*query_guest_notifiers)(DeviceState *d);
+    int (*set_guest_notifiers)(DeviceState *d, bool assigned);
+    int (*set_host_notifier)(DeviceState *d, int n, bool assigned);
+    void (*vmstate_change)(DeviceState *d, bool running);
 } VirtIOBindings;
 
 #define VIRTIO_PCI_QUEUE_MAX 64
@@ -128,7 +128,7 @@ struct VirtIODevice
     void (*set_status)(VirtIODevice *vdev, uint8_t val);
     VirtQueue *vq;
     const VirtIOBindings *binding;
-    void *binding_opaque;
+    DeviceState *binding_opaque;
     uint16_t device_id;
     bool vm_running;
     VMChangeStateEntry *vmstate;

-- 
MST

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

[Paolo Bonzini]

Il 17/12/2012 11:40, Michael S. Tsirkin ha scritto:
> How about the following? Then we can put reset
> in generic code where it belongs.
> It's untested - really kind of pseudo code - and
> s390 is still to be updated.
> 
> Posting to see what does everyone thinks.

I'm not (yet) sure how that helps my problem, but it is definitely a
step in the right direction!

Paolo

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

[Michael S. Tsirkin]

On Mon, Dec 17, 2012 at 04:14:00PM +0100, Paolo Bonzini wrote:
> Il 17/12/2012 11:40, Michael S. Tsirkin ha scritto:
> > How about the following? Then we can put reset
> > in generic code where it belongs.
> > It's untested - really kind of pseudo code - and
> > s390 is still to be updated.
> > 
> > Posting to see what does everyone thinks.
> 
> I'm not (yet) sure how that helps my problem,

It makes it possible for virtio.c to get at the
device state through the binding pointer.
So you will be able to qdev_reset_all from virtio.c
where it belongs, instead of duplicating code
in all bindings.

> but it is definitely a
> step in the right direction!
> 
> Paolo

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

[Paolo Bonzini]

Il 17/12/2012 16:24, Michael S. Tsirkin ha scritto:
> On Mon, Dec 17, 2012 at 04:14:00PM +0100, Paolo Bonzini wrote:
>> Il 17/12/2012 11:40, Michael S. Tsirkin ha scritto:
>>> How about the following? Then we can put reset
>>> in generic code where it belongs.
>>> It's untested - really kind of pseudo code - and
>>> s390 is still to be updated.
>>>
>>> Posting to see what does everyone thinks.
>>
>> I'm not (yet) sure how that helps my problem,
> 
> It makes it possible for virtio.c to get at the
> device state through the binding pointer.
> So you will be able to qdev_reset_all from virtio.c
> where it belongs, instead of duplicating code
> in all bindings.

Yes, but where does it belong?  Do you want to move handling of the
status register (and others) to hw/virtio.c?

Also, you're proposing that I do qdev_reset_all(vdev->binding_opaque)
but that would be a layering violation.  Generic virtio code should not
be able to reset the transport-specific setup (e.g. MSIs).

Paolo

>> but it is definitely a
>> step in the right direction!
>>
>> Paolo

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

[Michael S. Tsirkin]

On Mon, Dec 17, 2012 at 04:37:36PM +0100, Paolo Bonzini wrote:
> Il 17/12/2012 16:24, Michael S. Tsirkin ha scritto:
> > On Mon, Dec 17, 2012 at 04:14:00PM +0100, Paolo Bonzini wrote:
> >> Il 17/12/2012 11:40, Michael S. Tsirkin ha scritto:
> >>> How about the following? Then we can put reset
> >>> in generic code where it belongs.
> >>> It's untested - really kind of pseudo code - and
> >>> s390 is still to be updated.
> >>>
> >>> Posting to see what does everyone thinks.
> >>
> >> I'm not (yet) sure how that helps my problem,
> > 
> > It makes it possible for virtio.c to get at the
> > device state through the binding pointer.
> > So you will be able to qdev_reset_all from virtio.c
> > where it belongs, instead of duplicating code
> > in all bindings.
> 
> Yes, but where does it belong?  Do you want to move handling of the
> status register (and others) to hw/virtio.c?

I thought we can have some kind of generic function that all
transports can call. It would call qdev_reset_all internally,
and we would invoke it from all transports.

> Also, you're proposing that I do qdev_reset_all(vdev->binding_opaque)
> but that would be a layering violation.  Generic virtio code should not
> be able to reset the transport-specific setup (e.g. MSIs).
> 
> Paolo

Bus reset looks like this:

	qdev -> pci -> virtio pci reset -> virtio reset

status reset looks like this:

	 virtio pci reset -> virtio reset

You original patch was basically calling back to
qdev from virtio pci (bypassing pci).

If that is OK and not a layering violation,
why calling from virtio back to virtio pci not OK?

How do you think reset should be layered?


> >> but it is definitely a
> >> step in the right direction!
> >>
> >> Paolo

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

[Paolo Bonzini]

Il 17/12/2012 17:01, Michael S. Tsirkin ha scritto:
> On Mon, Dec 17, 2012 at 04:37:36PM +0100, Paolo Bonzini wrote:
>> Il 17/12/2012 16:24, Michael S. Tsirkin ha scritto:
>>> On Mon, Dec 17, 2012 at 04:14:00PM +0100, Paolo Bonzini wrote:
>>>> Il 17/12/2012 11:40, Michael S. Tsirkin ha scritto:
>>>>> How about the following? Then we can put reset
>>>>> in generic code where it belongs.
>>>>> It's untested - really kind of pseudo code - and
>>>>> s390 is still to be updated.
>>>>>
>>>>> Posting to see what does everyone thinks.
>>>>
>>>> I'm not (yet) sure how that helps my problem,
>>>
>>> It makes it possible for virtio.c to get at the
>>> device state through the binding pointer.
>>> So you will be able to qdev_reset_all from virtio.c
>>> where it belongs, instead of duplicating code
>>> in all bindings.
>>
>> Yes, but where does it belong?  Do you want to move handling of the
>> status register (and others) to hw/virtio.c?
> 
> I thought we can have some kind of generic function that all
> transports can call. It would call qdev_reset_all internally,
> and we would invoke it from all transports.
> 
>> Also, you're proposing that I do qdev_reset_all(vdev->binding_opaque)
>> but that would be a layering violation.  Generic virtio code should not
>> be able to reset the transport-specific setup (e.g. MSIs).
> 
> Bus reset looks like this:
> 
> 	qdev -> pci -> virtio pci reset -> virtio reset
> 
> status reset looks like this:
> 
> 	 virtio pci reset -> virtio reset
> 
> You original patch was basically calling back to
> qdev from virtio pci (bypassing pci).

Because it is actually correct to not involve PCI.  This is not
bypassing: PCI is above in the qdev tree, and never learns about a reset
that is triggered by a register write.  So, a device can ask qdev and be
reset, but it device cannot ask its parent to do a bus reset of itself.
 That would be like doing an FLR when writing zero to status.  Wrong,
and a layering violation.

> If that is OK and not a layering violation,
> why calling from virtio back to virtio pci not OK?

Because I'm calling qdev_reset_all on the same device that received the
reset.  I'm not calling qdev_reset_all on a parent device.  Calling
qdev_reset_all(vdev->binding_opaque) is equivalent calling it on a
parent device.

Still, the extra typesafety of your patch is good to have.

Paolo

> How do you think reset should be layered?
> 
> 
>>>> but it is definitely a
>>>> step in the right direction!
>>>>
>>>> Paolo