[PATCH 0/4] virtio: various cleanups to reset code

[PATCH 0/4] virtio: various cleanups to reset code

[Paolo Bonzini]

Patches 1, 3 and 4 are cleanups with no functional changes (intended, at
least).  Patch 2 is a small fix to legacy virtio-mmio reset, whose
behavior differed slightly compared to zeroing the status of the device.

Paolo

Paolo Bonzini (4):
  s390x: simplify virtio_ccw_reset_virtio
  virtio-mmio: stop ioeventfd on legacy reset
  virtio: stop ioeventfd on reset
  virtio-mmio: cleanup reset

 hw/s390x/virtio-ccw.c   | 12 +++++-------
 hw/virtio/virtio-bus.c  |  1 +
 hw/virtio/virtio-mmio.c | 18 ++++++++----------
 hw/virtio/virtio-pci.c  |  1 -
 4 files changed, 14 insertions(+), 18 deletions(-)

-- 
2.36.1

[PATCH 1/4] s390x: simplify virtio_ccw_reset_virtio

[Paolo Bonzini]

Call virtio_bus_reset instead of virtio_reset, so that the function
need not receive the VirtIODevice.

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

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 15b458527e..066a387802 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -249,12 +249,12 @@ static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info,
     return 0;
 }
 
-static void virtio_ccw_reset_virtio(VirtioCcwDevice *dev, VirtIODevice *vdev)
+static void virtio_ccw_reset_virtio(VirtioCcwDevice *dev)
 {
     CcwDevice *ccw_dev = CCW_DEVICE(dev);
 
     virtio_ccw_stop_ioeventfd(dev);
-    virtio_reset(vdev);
+    virtio_bus_reset(&dev->bus);
     if (dev->indicators) {
         release_indicator(&dev->routes.adapter, dev->indicators);
         dev->indicators = NULL;
@@ -359,7 +359,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         ret = virtio_ccw_handle_set_vq(sch, ccw, check_len, dev->revision < 1);
         break;
     case CCW_CMD_VDEV_RESET:
-        virtio_ccw_reset_virtio(dev, vdev);
+        virtio_ccw_reset_virtio(dev);
         ret = 0;
         break;
     case CCW_CMD_READ_FEAT:
@@ -536,7 +536,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             }
             if (virtio_set_status(vdev, status) == 0) {
                 if (vdev->status == 0) {
-                    virtio_ccw_reset_virtio(dev, vdev);
+                    virtio_ccw_reset_virtio(dev);
                 }
                 if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
                     virtio_ccw_start_ioeventfd(dev);
@@ -921,10 +921,9 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
 static void virtio_ccw_reset(DeviceState *d)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
-    VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
     VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
 
-    virtio_ccw_reset_virtio(dev, vdev);
+    virtio_ccw_reset_virtio(dev);
     if (vdc->parent_reset) {
         vdc->parent_reset(d);
     }
-- 
2.36.1

[PATCH 2/4] virtio-mmio: stop ioeventfd on legacy reset

[Paolo Bonzini]

If the queue PFN is set to zero on a virtio-mmio device, the device is reset.
In that case however the virtio_bus_stop_ioeventfd function was not
called; add it so that the behavior is similar to when status is set to 0.

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

diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 688eccda94..41a35d31c8 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -376,6 +376,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
             return;
         }
         if (value == 0) {
+            virtio_mmio_stop_ioeventfd(proxy);
             virtio_reset(vdev);
         } else {
             virtio_queue_set_addr(vdev, vdev->queue_sel,
-- 
2.36.1

[PATCH 3/4] virtio: stop ioeventfd on reset

[Paolo Bonzini]

All calls to virtio_bus_reset are preceded by virtio_bus_stop_ioeventfd,
move the call in virtio_bus_reset: that makes sense and clarifies
that the vdc->reset function is called with ioeventfd already stopped.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/s390x/virtio-ccw.c   | 1 -
 hw/virtio/virtio-bus.c  | 1 +
 hw/virtio/virtio-mmio.c | 4 +---
 hw/virtio/virtio-pci.c  | 1 -
 4 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 066a387802..e33e5207ab 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -253,7 +253,6 @@ static void virtio_ccw_reset_virtio(VirtioCcwDevice *dev)
 {
     CcwDevice *ccw_dev = CCW_DEVICE(dev);
 
-    virtio_ccw_stop_ioeventfd(dev);
     virtio_bus_reset(&dev->bus);
     if (dev->indicators) {
         release_indicator(&dev->routes.adapter, dev->indicators);
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index d7ec023adf..896feb37a1 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -104,6 +104,7 @@ void virtio_bus_reset(VirtioBusState *bus)
     VirtIODevice *vdev = virtio_bus_get_device(bus);
 
     DPRINTF("%s: reset device.\n", BUS(bus)->name);
+    virtio_bus_stop_ioeventfd(bus);
     if (vdev != NULL) {
         virtio_reset(vdev);
     }
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 41a35d31c8..6d81a26473 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -376,8 +376,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
             return;
         }
         if (value == 0) {
-            virtio_mmio_stop_ioeventfd(proxy);
-            virtio_reset(vdev);
+            virtio_bus_reset(&vdev->bus);
         } else {
             virtio_queue_set_addr(vdev, vdev->queue_sel,
                                   value << proxy->guest_page_shift);
@@ -628,7 +627,6 @@ static void virtio_mmio_reset(DeviceState *d)
     VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
     int i;
 
-    virtio_mmio_stop_ioeventfd(proxy);
     virtio_bus_reset(&proxy->bus);
     proxy->host_features_sel = 0;
     proxy->guest_features_sel = 0;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 0566ad7d00..45327f0b31 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1945,7 +1945,6 @@ static void virtio_pci_reset(DeviceState *qdev)
     PCIDevice *dev = PCI_DEVICE(qdev);
     int i;
 
-    virtio_pci_stop_ioeventfd(proxy);
     virtio_bus_reset(bus);
     msix_unuse_all_vectors(&proxy->pci_dev);
 
-- 
2.36.1

[PATCH 4/4] virtio-mmio: cleanup reset

[Paolo Bonzini]

Make virtio_mmio_soft_reset reset the virtio device, which is performed by
both the "soft" and the "hard" reset; and then call virtio_mmio_soft_reset
from virtio_mmio_reset to emphasize that the latter is a superset of the
former.

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

diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 6d81a26473..d240efef97 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -72,12 +72,12 @@ static void virtio_mmio_soft_reset(VirtIOMMIOProxy *proxy)
 {
     int i;
 
-    if (proxy->legacy) {
-        return;
-    }
+    virtio_bus_reset(&proxy->bus);
 
-    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-        proxy->vqs[i].enabled = 0;
+    if (!proxy->legacy) {
+        for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+            proxy->vqs[i].enabled = 0;
+        }
     }
 }
 
@@ -376,7 +376,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
             return;
         }
         if (value == 0) {
-            virtio_bus_reset(&vdev->bus);
+            virtio_mmio_soft_reset(proxy);
         } else {
             virtio_queue_set_addr(vdev, vdev->queue_sel,
                                   value << proxy->guest_page_shift);
@@ -432,7 +432,6 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
         }
 
         if (vdev->status == 0) {
-            virtio_reset(vdev);
             virtio_mmio_soft_reset(proxy);
         }
         break;
@@ -627,7 +626,8 @@ static void virtio_mmio_reset(DeviceState *d)
     VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
     int i;
 
-    virtio_bus_reset(&proxy->bus);
+    virtio_mmio_soft_reset(proxy);
+
     proxy->host_features_sel = 0;
     proxy->guest_features_sel = 0;
     proxy->guest_page_shift = 0;
@@ -636,7 +636,6 @@ static void virtio_mmio_reset(DeviceState *d)
         proxy->guest_features[0] = proxy->guest_features[1] = 0;
 
         for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-            proxy->vqs[i].enabled = 0;
             proxy->vqs[i].num = 0;
             proxy->vqs[i].desc[0] = proxy->vqs[i].desc[1] = 0;
             proxy->vqs[i].avail[0] = proxy->vqs[i].avail[1] = 0;
-- 
2.36.1

Re: [PATCH 1/4] s390x: simplify virtio_ccw_reset_virtio

[Cornelia Huck]

On Thu, Jun 09 2022, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Call virtio_bus_reset instead of virtio_reset, so that the function
> need not receive the VirtIODevice.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/s390x/virtio-ccw.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 2/4] virtio-mmio: stop ioeventfd on legacy reset

[Cornelia Huck]

On Thu, Jun 09 2022, Paolo Bonzini <pbonzini@redhat.com> wrote:

> If the queue PFN is set to zero on a virtio-mmio device, the device is reset.
> In that case however the virtio_bus_stop_ioeventfd function was not
> called; add it so that the behavior is similar to when status is set to 0.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/virtio-mmio.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 3/4] virtio: stop ioeventfd on reset

[Cornelia Huck]

On Thu, Jun 09 2022, Paolo Bonzini <pbonzini@redhat.com> wrote:

> All calls to virtio_bus_reset are preceded by virtio_bus_stop_ioeventfd,
> move the call in virtio_bus_reset: that makes sense and clarifies
> that the vdc->reset function is called with ioeventfd already stopped.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/s390x/virtio-ccw.c   | 1 -
>  hw/virtio/virtio-bus.c  | 1 +
>  hw/virtio/virtio-mmio.c | 4 +---
>  hw/virtio/virtio-pci.c  | 1 -
>  4 files changed, 2 insertions(+), 5 deletions(-)
>

(...)

> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index d7ec023adf..896feb37a1 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -104,6 +104,7 @@ void virtio_bus_reset(VirtioBusState *bus)
>      VirtIODevice *vdev = virtio_bus_get_device(bus);
>  
>      DPRINTF("%s: reset device.\n", BUS(bus)->name);
> +    virtio_bus_stop_ioeventfd(bus);
>      if (vdev != NULL) {
>          virtio_reset(vdev);

I looked at the code and I'm wondering under which conditions we could
arrive here with vdev == NULL... virtio_bus_stop_ioeventfd() assumes
that a vdev is there, at least if the ioeventfd has been started.

The patch looks correct, though.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 4/4] virtio-mmio: cleanup reset

[Cornelia Huck]

On Thu, Jun 09 2022, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Make virtio_mmio_soft_reset reset the virtio device, which is performed by
> both the "soft" and the "hard" reset; and then call virtio_mmio_soft_reset
> from virtio_mmio_reset to emphasize that the latter is a superset of the
> former.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/virtio-mmio.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index 6d81a26473..d240efef97 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -72,12 +72,12 @@ static void virtio_mmio_soft_reset(VirtIOMMIOProxy *proxy)
>  {
>      int i;
>  
> -    if (proxy->legacy) {
> -        return;
> -    }
> +    virtio_bus_reset(&proxy->bus);
>  
> -    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> -        proxy->vqs[i].enabled = 0;
> +    if (!proxy->legacy) {
> +        for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +            proxy->vqs[i].enabled = 0;
> +        }
>      }
>  }

The more I look at this, the more confused I get.

The current code calls soft_reset when the driver sets the status to 0,
after already having called virtio_reset(). But doesn't virtio_reset()
ultimately already trigger the virtio-mmio reset routine, which sets
enabled to 0 for all queues? Why do that again? (And why is soft_reset a
"soft reset"?)

Maybe I'm missing something obvious, or it is simply -ENOCOFFEE on my side.

>  
> @@ -376,7 +376,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
>              return;
>          }
>          if (value == 0) {
> -            virtio_bus_reset(&vdev->bus);
> +            virtio_mmio_soft_reset(proxy);
>          } else {
>              virtio_queue_set_addr(vdev, vdev->queue_sel,
>                                    value << proxy->guest_page_shift);
> @@ -432,7 +432,6 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
>          }
>  
>          if (vdev->status == 0) {
> -            virtio_reset(vdev);
>              virtio_mmio_soft_reset(proxy);
>          }
>          break;
> @@ -627,7 +626,8 @@ static void virtio_mmio_reset(DeviceState *d)
>      VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
>      int i;
>  
> -    virtio_bus_reset(&proxy->bus);
> +    virtio_mmio_soft_reset(proxy);
> +
>      proxy->host_features_sel = 0;
>      proxy->guest_features_sel = 0;
>      proxy->guest_page_shift = 0;
> @@ -636,7 +636,6 @@ static void virtio_mmio_reset(DeviceState *d)
>          proxy->guest_features[0] = proxy->guest_features[1] = 0;
>  
>          for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> -            proxy->vqs[i].enabled = 0;
>              proxy->vqs[i].num = 0;
>              proxy->vqs[i].desc[0] = proxy->vqs[i].desc[1] = 0;
>              proxy->vqs[i].avail[0] = proxy->vqs[i].avail[1] = 0;

Re: [PATCH 4/4] virtio-mmio: cleanup reset

[Paolo Bonzini]

On 6/9/22 14:22, Cornelia Huck wrote:
>> -    if (proxy->legacy) {
>> -        return;
>> -    }
>> +    virtio_bus_reset(&proxy->bus);
>>   
>> -    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
>> -        proxy->vqs[i].enabled = 0;
>> +    if (!proxy->legacy) {
>> +        for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
>> +            proxy->vqs[i].enabled = 0;
>> +        }
>>       }
>>   }
>
> The more I look at this, the more confused I get.
> 
> The current code calls soft_reset when the driver sets the status to 0,
> after already having called virtio_reset().

Yes, that's before the patch.

> But doesn't virtio_reset() ultimately already trigger the virtio-mmio
> reset routine, which sets enabled to 0 for all queues? Why do that
> again? (And why is soft_reset a "soft reset"?)
No, it does not set enabled = 0 because "enabled" is specific to 
virtio-mmio (it is read by VIRTIO_MMIO_QUEUE_READY, which is only 
available in virtio-mmio 1.0 devices).  In fact it is stored in 
proxy->vqs[], while virtio_reset only resets fields in vdev->vq[].

k->reset() instead triggers the *device* reset routine (e.g. 
virtio_blk_reset).  So what makes it "soft" is that the various queue 
addresses remain there, and can be enabled just by writing 1 to 
VIRTIO_MMIO_QUEUE_READY for every queue.

Paolo