| Message ID | 20220609091534.1416909-5-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio: various cleanups to reset code | expand |
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;
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
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;
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(-)