| Message ID | 20250410-reset-v1-1-751cd0064395@daynix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | virtio: Call set_features during reset | expand |
On Thu, Apr 10, 2025 at 04:42:06PM +0900, Akihiko Odaki wrote: > virtio-net expects set_features() will be called when the feature set > used by the guest changes to update the number of virtqueues. Call it > during reset as reset clears all features and the queues added for > VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS will need to be removed. > > Fixes: f9d6dbf0bf6e ("virtio-net: remove virtio queues if the guest doesn't support multiqueue") > Buglink: https://issues.redhat.com/browse/RHEL-73842 > Cc: qemu-stable@nongnu.org > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> The issue seems specific to virtio net: rset is reset, it is distict from set features. Why not just call the necessary functionality from virtio_net_reset? > --- > hw/virtio/virtio.c | 86 +++++++++++++++++++++++++++--------------------------- > 1 file changed, 43 insertions(+), 43 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 85110bce3744..033e87cdd3b9 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2316,49 +2316,6 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index) > } > } > > -void virtio_reset(void *opaque) > -{ > - VirtIODevice *vdev = opaque; > - VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); > - int i; > - > - virtio_set_status(vdev, 0); > - if (current_cpu) { > - /* Guest initiated reset */ > - vdev->device_endian = virtio_current_cpu_endian(); > - } else { > - /* System reset */ > - vdev->device_endian = virtio_default_endian(); > - } > - > - if (k->get_vhost) { > - struct vhost_dev *hdev = k->get_vhost(vdev); > - /* Only reset when vhost back-end is connected */ > - if (hdev && hdev->vhost_ops) { > - vhost_reset_device(hdev); > - } > - } > - > - if (k->reset) { > - k->reset(vdev); > - } > - > - vdev->start_on_kick = false; > - vdev->started = false; > - vdev->broken = false; > - vdev->guest_features = 0; > - vdev->queue_sel = 0; > - vdev->status = 0; > - vdev->disabled = false; > - qatomic_set(&vdev->isr, 0); > - vdev->config_vector = VIRTIO_NO_VECTOR; > - virtio_notify_vector(vdev, vdev->config_vector); > - > - for(i = 0; i < VIRTIO_QUEUE_MAX; i++) { > - __virtio_queue_reset(vdev, i); > - } > -} > - > void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr) > { > if (!vdev->vq[n].vring.num) { > @@ -3169,6 +3126,49 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val) > return ret; > } > > +void virtio_reset(void *opaque) > +{ > + VirtIODevice *vdev = opaque; > + VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); > + int i; > + > + virtio_set_status(vdev, 0); > + if (current_cpu) { > + /* Guest initiated reset */ > + vdev->device_endian = virtio_current_cpu_endian(); > + } else { > + /* System reset */ > + vdev->device_endian = virtio_default_endian(); > + } > + > + if (k->get_vhost) { > + struct vhost_dev *hdev = k->get_vhost(vdev); > + /* Only reset when vhost back-end is connected */ > + if (hdev && hdev->vhost_ops) { > + vhost_reset_device(hdev); > + } > + } > + > + if (k->reset) { > + k->reset(vdev); > + } > + > + vdev->start_on_kick = false; > + vdev->started = false; > + vdev->broken = false; > + virtio_set_features_nocheck(vdev, 0); > + vdev->queue_sel = 0; > + vdev->status = 0; > + vdev->disabled = false; > + qatomic_set(&vdev->isr, 0); > + vdev->config_vector = VIRTIO_NO_VECTOR; > + virtio_notify_vector(vdev, vdev->config_vector); > + > + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { > + __virtio_queue_reset(vdev, i); > + } > +} > + > static void virtio_device_check_notification_compatibility(VirtIODevice *vdev, > Error **errp) > { > > --- > base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32 > change-id: 20250406-reset-5ed5248ee3c1 > > Best regards, > -- > Akihiko Odaki <akihiko.odaki@daynix.com>
On 2025/04/10 16:48, 'Michael S. Tsirkin' via devel wrote: > On Thu, Apr 10, 2025 at 04:42:06PM +0900, Akihiko Odaki wrote: >> virtio-net expects set_features() will be called when the feature set >> used by the guest changes to update the number of virtqueues. Call it >> during reset as reset clears all features and the queues added for >> VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS will need to be removed. >> >> Fixes: f9d6dbf0bf6e ("virtio-net: remove virtio queues if the guest doesn't support multiqueue") >> Buglink: https://issues.redhat.com/browse/RHEL-73842 >> Cc: qemu-stable@nongnu.org >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > The issue seems specific to virtio net: rset is reset, > it is distict from set features. > Why not just call the necessary functionality from virtio_net_reset? set_features is currently implemented only in virtio-net; virtio-gpu-base also have a function set but it only has code to trace. If another device implements the function in the future, I think the device will also want to have it called during reset for the same reason with virtio-net. virtio_reset() also calls set_status to update the status field so calling set_features() is more aligned with the handling of the status field. > > >> --- >> hw/virtio/virtio.c | 86 +++++++++++++++++++++++++++--------------------------- >> 1 file changed, 43 insertions(+), 43 deletions(-) >> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 85110bce3744..033e87cdd3b9 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -2316,49 +2316,6 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index) >> } >> } >> >> -void virtio_reset(void *opaque) >> -{ >> - VirtIODevice *vdev = opaque; >> - VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); >> - int i; >> - >> - virtio_set_status(vdev, 0); >> - if (current_cpu) { >> - /* Guest initiated reset */ >> - vdev->device_endian = virtio_current_cpu_endian(); >> - } else { >> - /* System reset */ >> - vdev->device_endian = virtio_default_endian(); >> - } >> - >> - if (k->get_vhost) { >> - struct vhost_dev *hdev = k->get_vhost(vdev); >> - /* Only reset when vhost back-end is connected */ >> - if (hdev && hdev->vhost_ops) { >> - vhost_reset_device(hdev); >> - } >> - } >> - >> - if (k->reset) { >> - k->reset(vdev); >> - } >> - >> - vdev->start_on_kick = false; >> - vdev->started = false; >> - vdev->broken = false; >> - vdev->guest_features = 0; >> - vdev->queue_sel = 0; >> - vdev->status = 0; >> - vdev->disabled = false; >> - qatomic_set(&vdev->isr, 0); >> - vdev->config_vector = VIRTIO_NO_VECTOR; >> - virtio_notify_vector(vdev, vdev->config_vector); >> - >> - for(i = 0; i < VIRTIO_QUEUE_MAX; i++) { >> - __virtio_queue_reset(vdev, i); >> - } >> -} >> - >> void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr) >> { >> if (!vdev->vq[n].vring.num) { >> @@ -3169,6 +3126,49 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val) >> return ret; >> } >> >> +void virtio_reset(void *opaque) >> +{ >> + VirtIODevice *vdev = opaque; >> + VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); >> + int i; >> + >> + virtio_set_status(vdev, 0); >> + if (current_cpu) { >> + /* Guest initiated reset */ >> + vdev->device_endian = virtio_current_cpu_endian(); >> + } else { >> + /* System reset */ >> + vdev->device_endian = virtio_default_endian(); >> + } >> + >> + if (k->get_vhost) { >> + struct vhost_dev *hdev = k->get_vhost(vdev); >> + /* Only reset when vhost back-end is connected */ >> + if (hdev && hdev->vhost_ops) { >> + vhost_reset_device(hdev); >> + } >> + } >> + >> + if (k->reset) { >> + k->reset(vdev); >> + } >> + >> + vdev->start_on_kick = false; >> + vdev->started = false; >> + vdev->broken = false; >> + virtio_set_features_nocheck(vdev, 0); >> + vdev->queue_sel = 0; >> + vdev->status = 0; >> + vdev->disabled = false; >> + qatomic_set(&vdev->isr, 0); >> + vdev->config_vector = VIRTIO_NO_VECTOR; >> + virtio_notify_vector(vdev, vdev->config_vector); >> + >> + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { >> + __virtio_queue_reset(vdev, i); >> + } >> +} >> + >> static void virtio_device_check_notification_compatibility(VirtIODevice *vdev, >> Error **errp) >> { >> >> --- >> base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32 >> change-id: 20250406-reset-5ed5248ee3c1 >> >> Best regards, >> -- >> Akihiko Odaki <akihiko.odaki@daynix.com> >
On Thu, Apr 10, 2025 at 04:54:41PM +0900, Akihiko Odaki wrote: > On 2025/04/10 16:48, 'Michael S. Tsirkin' via devel wrote: > > On Thu, Apr 10, 2025 at 04:42:06PM +0900, Akihiko Odaki wrote: > > > virtio-net expects set_features() will be called when the feature set > > > used by the guest changes to update the number of virtqueues. Call it > > > during reset as reset clears all features and the queues added for > > > VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS will need to be removed. > > > > > > Fixes: f9d6dbf0bf6e ("virtio-net: remove virtio queues if the guest doesn't support multiqueue") > > > Buglink: https://issues.redhat.com/browse/RHEL-73842 > > > Cc: qemu-stable@nongnu.org > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > > The issue seems specific to virtio net: rset is reset, > > it is distict from set features. > > Why not just call the necessary functionality from virtio_net_reset? > > set_features is currently implemented only in virtio-net; virtio-gpu-base > also have a function set but it only has code to trace. If another device > implements the function in the future, I think the device will also want to > have it called during reset for the same reason with virtio-net. > > virtio_reset() also calls set_status to update the status field so calling > set_features() is more aligned with the handling of the status field. That came to be because writing 0 to status resets the virtio device. For a while, this was the only way to reset vhost-user so we just went along with it. > > > > > > > --- > > > hw/virtio/virtio.c | 86 +++++++++++++++++++++++++++--------------------------- > > > 1 file changed, 43 insertions(+), 43 deletions(-) > > > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > > index 85110bce3744..033e87cdd3b9 100644 > > > --- a/hw/virtio/virtio.c > > > +++ b/hw/virtio/virtio.c > > > @@ -2316,49 +2316,6 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index) > > > } > > > } > > > -void virtio_reset(void *opaque) > > > -{ > > > - VirtIODevice *vdev = opaque; > > > - VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); > > > - int i; > > > - > > > - virtio_set_status(vdev, 0); > > > - if (current_cpu) { > > > - /* Guest initiated reset */ > > > - vdev->device_endian = virtio_current_cpu_endian(); > > > - } else { > > > - /* System reset */ > > > - vdev->device_endian = virtio_default_endian(); > > > - } > > > - > > > - if (k->get_vhost) { > > > - struct vhost_dev *hdev = k->get_vhost(vdev); > > > - /* Only reset when vhost back-end is connected */ > > > - if (hdev && hdev->vhost_ops) { > > > - vhost_reset_device(hdev); > > > - } > > > - } > > > - > > > - if (k->reset) { > > > - k->reset(vdev); > > > - } > > > - > > > - vdev->start_on_kick = false; > > > - vdev->started = false; > > > - vdev->broken = false; > > > - vdev->guest_features = 0; > > > - vdev->queue_sel = 0; > > > - vdev->status = 0; > > > - vdev->disabled = false; > > > - qatomic_set(&vdev->isr, 0); > > > - vdev->config_vector = VIRTIO_NO_VECTOR; > > > - virtio_notify_vector(vdev, vdev->config_vector); > > > - > > > - for(i = 0; i < VIRTIO_QUEUE_MAX; i++) { > > > - __virtio_queue_reset(vdev, i); > > > - } > > > -} > > > - > > > void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr) > > > { > > > if (!vdev->vq[n].vring.num) { > > > @@ -3169,6 +3126,49 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val) > > > return ret; > > > } > > > +void virtio_reset(void *opaque) > > > +{ > > > + VirtIODevice *vdev = opaque; > > > + VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); > > > + int i; > > > + > > > + virtio_set_status(vdev, 0); > > > + if (current_cpu) { > > > + /* Guest initiated reset */ > > > + vdev->device_endian = virtio_current_cpu_endian(); > > > + } else { > > > + /* System reset */ > > > + vdev->device_endian = virtio_default_endian(); > > > + } > > > + > > > + if (k->get_vhost) { > > > + struct vhost_dev *hdev = k->get_vhost(vdev); > > > + /* Only reset when vhost back-end is connected */ > > > + if (hdev && hdev->vhost_ops) { > > > + vhost_reset_device(hdev); > > > + } > > > + } > > > + > > > + if (k->reset) { > > > + k->reset(vdev); > > > + } > > > + > > > + vdev->start_on_kick = false; > > > + vdev->started = false; > > > + vdev->broken = false; > > > + virtio_set_features_nocheck(vdev, 0); > > > + vdev->queue_sel = 0; > > > + vdev->status = 0; > > > + vdev->disabled = false; > > > + qatomic_set(&vdev->isr, 0); > > > + vdev->config_vector = VIRTIO_NO_VECTOR; > > > + virtio_notify_vector(vdev, vdev->config_vector); > > > + > > > + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { > > > + __virtio_queue_reset(vdev, i); > > > + } > > > +} > > > + > > > static void virtio_device_check_notification_compatibility(VirtIODevice *vdev, > > > Error **errp) > > > { > > > > > > --- > > > base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32 > > > change-id: 20250406-reset-5ed5248ee3c1 > > > > > > Best regards, > > > -- > > > Akihiko Odaki <akihiko.odaki@daynix.com> > >
On 2025/04/10 17:02, Michael S. Tsirkin wrote: > On Thu, Apr 10, 2025 at 04:54:41PM +0900, Akihiko Odaki wrote: >> On 2025/04/10 16:48, 'Michael S. Tsirkin' via devel wrote: >>> On Thu, Apr 10, 2025 at 04:42:06PM +0900, Akihiko Odaki wrote: >>>> virtio-net expects set_features() will be called when the feature set >>>> used by the guest changes to update the number of virtqueues. Call it >>>> during reset as reset clears all features and the queues added for >>>> VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS will need to be removed. >>>> >>>> Fixes: f9d6dbf0bf6e ("virtio-net: remove virtio queues if the guest doesn't support multiqueue") >>>> Buglink: https://issues.redhat.com/browse/RHEL-73842 >>>> Cc: qemu-stable@nongnu.org >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> >>> The issue seems specific to virtio net: rset is reset, >>> it is distict from set features. >>> Why not just call the necessary functionality from virtio_net_reset? >> >> set_features is currently implemented only in virtio-net; virtio-gpu-base >> also have a function set but it only has code to trace. If another device >> implements the function in the future, I think the device will also want to >> have it called during reset for the same reason with virtio-net. >> >> virtio_reset() also calls set_status to update the status field so calling >> set_features() is more aligned with the handling of the status field. > > That came to be because writing 0 to status resets the virtio device. > For a while, this was the only way to reset vhost-user so we just > went along with it. It is possible to have code to send a command to write 0 to status to vhost-user in reset(), but calling set_status() in virtio_reset() is more convenient and makes sense as the status is indeed being set to 0. I think the same reasoning applies to features. > > >>> >>> >>>> --- >>>> hw/virtio/virtio.c | 86 +++++++++++++++++++++++++++--------------------------- >>>> 1 file changed, 43 insertions(+), 43 deletions(-) >>>> >>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>>> index 85110bce3744..033e87cdd3b9 100644 >>>> --- a/hw/virtio/virtio.c >>>> +++ b/hw/virtio/virtio.c >>>> @@ -2316,49 +2316,6 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index) >>>> } >>>> } >>>> -void virtio_reset(void *opaque) >>>> -{ >>>> - VirtIODevice *vdev = opaque; >>>> - VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); >>>> - int i; >>>> - >>>> - virtio_set_status(vdev, 0); >>>> - if (current_cpu) { >>>> - /* Guest initiated reset */ >>>> - vdev->device_endian = virtio_current_cpu_endian(); >>>> - } else { >>>> - /* System reset */ >>>> - vdev->device_endian = virtio_default_endian(); >>>> - } >>>> - >>>> - if (k->get_vhost) { >>>> - struct vhost_dev *hdev = k->get_vhost(vdev); >>>> - /* Only reset when vhost back-end is connected */ >>>> - if (hdev && hdev->vhost_ops) { >>>> - vhost_reset_device(hdev); >>>> - } >>>> - } >>>> - >>>> - if (k->reset) { >>>> - k->reset(vdev); >>>> - } >>>> - >>>> - vdev->start_on_kick = false; >>>> - vdev->started = false; >>>> - vdev->broken = false; >>>> - vdev->guest_features = 0; >>>> - vdev->queue_sel = 0; >>>> - vdev->status = 0; >>>> - vdev->disabled = false; >>>> - qatomic_set(&vdev->isr, 0); >>>> - vdev->config_vector = VIRTIO_NO_VECTOR; >>>> - virtio_notify_vector(vdev, vdev->config_vector); >>>> - >>>> - for(i = 0; i < VIRTIO_QUEUE_MAX; i++) { >>>> - __virtio_queue_reset(vdev, i); >>>> - } >>>> -} >>>> - >>>> void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr) >>>> { >>>> if (!vdev->vq[n].vring.num) { >>>> @@ -3169,6 +3126,49 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val) >>>> return ret; >>>> } >>>> +void virtio_reset(void *opaque) >>>> +{ >>>> + VirtIODevice *vdev = opaque; >>>> + VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); >>>> + int i; >>>> + >>>> + virtio_set_status(vdev, 0); >>>> + if (current_cpu) { >>>> + /* Guest initiated reset */ >>>> + vdev->device_endian = virtio_current_cpu_endian(); >>>> + } else { >>>> + /* System reset */ >>>> + vdev->device_endian = virtio_default_endian(); >>>> + } >>>> + >>>> + if (k->get_vhost) { >>>> + struct vhost_dev *hdev = k->get_vhost(vdev); >>>> + /* Only reset when vhost back-end is connected */ >>>> + if (hdev && hdev->vhost_ops) { >>>> + vhost_reset_device(hdev); >>>> + } >>>> + } >>>> + >>>> + if (k->reset) { >>>> + k->reset(vdev); >>>> + } >>>> + >>>> + vdev->start_on_kick = false; >>>> + vdev->started = false; >>>> + vdev->broken = false; >>>> + virtio_set_features_nocheck(vdev, 0); >>>> + vdev->queue_sel = 0; >>>> + vdev->status = 0; >>>> + vdev->disabled = false; >>>> + qatomic_set(&vdev->isr, 0); >>>> + vdev->config_vector = VIRTIO_NO_VECTOR; >>>> + virtio_notify_vector(vdev, vdev->config_vector); >>>> + >>>> + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { >>>> + __virtio_queue_reset(vdev, i); >>>> + } >>>> +} >>>> + >>>> static void virtio_device_check_notification_compatibility(VirtIODevice *vdev, >>>> Error **errp) >>>> { >>>> >>>> --- >>>> base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32 >>>> change-id: 20250406-reset-5ed5248ee3c1 >>>> >>>> Best regards, >>>> -- >>>> Akihiko Odaki <akihiko.odaki@daynix.com> >>> >
Hi Akihiko, On 10/4/25 09:42, Akihiko Odaki wrote: > virtio-net expects set_features() will be called when the feature set > used by the guest changes to update the number of virtqueues. Call it > during reset as reset clears all features and the queues added for > VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS will need to be removed. > > Fixes: f9d6dbf0bf6e ("virtio-net: remove virtio queues if the guest doesn't support multiqueue") > Buglink: https://issues.redhat.com/browse/RHEL-73842 > Cc: qemu-stable@nongnu.org > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > hw/virtio/virtio.c | 86 +++++++++++++++++++++++++++--------------------------- > 1 file changed, 43 insertions(+), 43 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 85110bce3744..033e87cdd3b9 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2316,49 +2316,6 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index) > } > } > > -void virtio_reset(void *opaque) > -{ > - VirtIODevice *vdev = opaque; > - VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); > - int i; > - > - virtio_set_status(vdev, 0); > - if (current_cpu) { > - /* Guest initiated reset */ > - vdev->device_endian = virtio_current_cpu_endian(); > - } else { > - /* System reset */ > - vdev->device_endian = virtio_default_endian(); > - } > - > - if (k->get_vhost) { > - struct vhost_dev *hdev = k->get_vhost(vdev); > - /* Only reset when vhost back-end is connected */ > - if (hdev && hdev->vhost_ops) { > - vhost_reset_device(hdev); > - } > - } > - > - if (k->reset) { > - k->reset(vdev); > - } > - > - vdev->start_on_kick = false; > - vdev->started = false; > - vdev->broken = false; > - vdev->guest_features = 0; > - vdev->queue_sel = 0; > - vdev->status = 0; > - vdev->disabled = false; > - qatomic_set(&vdev->isr, 0); > - vdev->config_vector = VIRTIO_NO_VECTOR; > - virtio_notify_vector(vdev, vdev->config_vector); > - > - for(i = 0; i < VIRTIO_QUEUE_MAX; i++) { > - __virtio_queue_reset(vdev, i); > - } > -} > - > void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr) > { > if (!vdev->vq[n].vring.num) { > @@ -3169,6 +3126,49 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val) > return ret; > } > > +void virtio_reset(void *opaque) > +{ > + VirtIODevice *vdev = opaque; > + VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); > + int i; > + > + virtio_set_status(vdev, 0); > + if (current_cpu) { > + /* Guest initiated reset */ > + vdev->device_endian = virtio_current_cpu_endian(); > + } else { > + /* System reset */ > + vdev->device_endian = virtio_default_endian(); > + } > + > + if (k->get_vhost) { > + struct vhost_dev *hdev = k->get_vhost(vdev); > + /* Only reset when vhost back-end is connected */ > + if (hdev && hdev->vhost_ops) { > + vhost_reset_device(hdev); > + } > + } > + > + if (k->reset) { > + k->reset(vdev); > + } > + > + vdev->start_on_kick = false; > + vdev->started = false; > + vdev->broken = false; > + virtio_set_features_nocheck(vdev, 0); It would be simpler to review having a first patch doing code movement, then a second one with the addition. For my own education, are feature sets modifiable at runtime? > + vdev->queue_sel = 0; > + vdev->status = 0; > + vdev->disabled = false; > + qatomic_set(&vdev->isr, 0); > + vdev->config_vector = VIRTIO_NO_VECTOR; > + virtio_notify_vector(vdev, vdev->config_vector); > + > + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { > + __virtio_queue_reset(vdev, i); > + } > +} > + > static void virtio_device_check_notification_compatibility(VirtIODevice *vdev, > Error **errp) > { > > --- > base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32 > change-id: 20250406-reset-5ed5248ee3c1 > > Best regards,
On Thu, Apr 10, 2025 at 05:26:47PM +0900, Akihiko Odaki wrote: > On 2025/04/10 17:02, Michael S. Tsirkin wrote: > > On Thu, Apr 10, 2025 at 04:54:41PM +0900, Akihiko Odaki wrote: > > > On 2025/04/10 16:48, 'Michael S. Tsirkin' via devel wrote: > > > > On Thu, Apr 10, 2025 at 04:42:06PM +0900, Akihiko Odaki wrote: > > > > > virtio-net expects set_features() will be called when the feature set > > > > > used by the guest changes to update the number of virtqueues. Call it > > > > > during reset as reset clears all features and the queues added for > > > > > VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS will need to be removed. > > > > > > > > > > Fixes: f9d6dbf0bf6e ("virtio-net: remove virtio queues if the guest doesn't support multiqueue") > > > > > Buglink: https://issues.redhat.com/browse/RHEL-73842 > > > > > Cc: qemu-stable@nongnu.org > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > > > > > > The issue seems specific to virtio net: rset is reset, > > > > it is distict from set features. > > > > Why not just call the necessary functionality from virtio_net_reset? > > > > > > set_features is currently implemented only in virtio-net; virtio-gpu-base > > > also have a function set but it only has code to trace. If another device > > > implements the function in the future, I think the device will also want to > > > have it called during reset for the same reason with virtio-net. > > > > > > virtio_reset() also calls set_status to update the status field so calling > > > set_features() is more aligned with the handling of the status field. > > > > That came to be because writing 0 to status resets the virtio device. > > For a while, this was the only way to reset vhost-user so we just > > went along with it. > > It is possible to have code to send a command to write 0 to status to > vhost-user in reset(), but calling set_status() in virtio_reset() is more > convenient and makes sense as the status is indeed being set to 0. I think > the same reasoning applies to features. I don't know who makes assumptions that features are only set during driver setup, though. This will send an extra VHOST_USER_SET_FEATURES message for vhost-user, for example. I want to have a good reason to add this overhead. > > > > > > > > > > > > > > > > > --- > > > > > hw/virtio/virtio.c | 86 +++++++++++++++++++++++++++--------------------------- > > > > > 1 file changed, 43 insertions(+), 43 deletions(-) > > > > > > > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > > > > index 85110bce3744..033e87cdd3b9 100644 > > > > > --- a/hw/virtio/virtio.c > > > > > +++ b/hw/virtio/virtio.c > > > > > @@ -2316,49 +2316,6 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index) > > > > > } > > > > > } > > > > > -void virtio_reset(void *opaque) > > > > > -{ > > > > > - VirtIODevice *vdev = opaque; > > > > > - VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); > > > > > - int i; > > > > > - > > > > > - virtio_set_status(vdev, 0); > > > > > - if (current_cpu) { > > > > > - /* Guest initiated reset */ > > > > > - vdev->device_endian = virtio_current_cpu_endian(); > > > > > - } else { > > > > > - /* System reset */ > > > > > - vdev->device_endian = virtio_default_endian(); > > > > > - } > > > > > - > > > > > - if (k->get_vhost) { > > > > > - struct vhost_dev *hdev = k->get_vhost(vdev); > > > > > - /* Only reset when vhost back-end is connected */ > > > > > - if (hdev && hdev->vhost_ops) { > > > > > - vhost_reset_device(hdev); > > > > > - } > > > > > - } > > > > > - > > > > > - if (k->reset) { > > > > > - k->reset(vdev); > > > > > - } > > > > > - > > > > > - vdev->start_on_kick = false; > > > > > - vdev->started = false; > > > > > - vdev->broken = false; > > > > > - vdev->guest_features = 0; > > > > > - vdev->queue_sel = 0; > > > > > - vdev->status = 0; > > > > > - vdev->disabled = false; > > > > > - qatomic_set(&vdev->isr, 0); > > > > > - vdev->config_vector = VIRTIO_NO_VECTOR; > > > > > - virtio_notify_vector(vdev, vdev->config_vector); > > > > > - > > > > > - for(i = 0; i < VIRTIO_QUEUE_MAX; i++) { > > > > > - __virtio_queue_reset(vdev, i); > > > > > - } > > > > > -} > > > > > - > > > > > void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr) > > > > > { > > > > > if (!vdev->vq[n].vring.num) { > > > > > @@ -3169,6 +3126,49 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val) > > > > > return ret; > > > > > } > > > > > +void virtio_reset(void *opaque) > > > > > +{ > > > > > + VirtIODevice *vdev = opaque; > > > > > + VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); > > > > > + int i; > > > > > + > > > > > + virtio_set_status(vdev, 0); > > > > > + if (current_cpu) { > > > > > + /* Guest initiated reset */ > > > > > + vdev->device_endian = virtio_current_cpu_endian(); > > > > > + } else { > > > > > + /* System reset */ > > > > > + vdev->device_endian = virtio_default_endian(); > > > > > + } > > > > > + > > > > > + if (k->get_vhost) { > > > > > + struct vhost_dev *hdev = k->get_vhost(vdev); > > > > > + /* Only reset when vhost back-end is connected */ > > > > > + if (hdev && hdev->vhost_ops) { > > > > > + vhost_reset_device(hdev); > > > > > + } > > > > > + } > > > > > + > > > > > + if (k->reset) { > > > > > + k->reset(vdev); > > > > > + } > > > > > + > > > > > + vdev->start_on_kick = false; > > > > > + vdev->started = false; > > > > > + vdev->broken = false; > > > > > + virtio_set_features_nocheck(vdev, 0); > > > > > + vdev->queue_sel = 0; > > > > > + vdev->status = 0; > > > > > + vdev->disabled = false; > > > > > + qatomic_set(&vdev->isr, 0); > > > > > + vdev->config_vector = VIRTIO_NO_VECTOR; > > > > > + virtio_notify_vector(vdev, vdev->config_vector); > > > > > + > > > > > + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { > > > > > + __virtio_queue_reset(vdev, i); > > > > > + } > > > > > +} > > > > > + > > > > > static void virtio_device_check_notification_compatibility(VirtIODevice *vdev, > > > > > Error **errp) > > > > > { > > > > > > > > > > --- > > > > > base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32 > > > > > change-id: 20250406-reset-5ed5248ee3c1 > > > > > > > > > > Best regards, > > > > > -- > > > > > Akihiko Odaki <akihiko.odaki@daynix.com> > > > > > >
On 2025/04/10 22:45, Michael S. Tsirkin wrote: > On Thu, Apr 10, 2025 at 05:26:47PM +0900, Akihiko Odaki wrote: >> On 2025/04/10 17:02, Michael S. Tsirkin wrote: >>> On Thu, Apr 10, 2025 at 04:54:41PM +0900, Akihiko Odaki wrote: >>>> On 2025/04/10 16:48, 'Michael S. Tsirkin' via devel wrote: >>>>> On Thu, Apr 10, 2025 at 04:42:06PM +0900, Akihiko Odaki wrote: >>>>>> virtio-net expects set_features() will be called when the feature set >>>>>> used by the guest changes to update the number of virtqueues. Call it >>>>>> during reset as reset clears all features and the queues added for >>>>>> VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS will need to be removed. >>>>>> >>>>>> Fixes: f9d6dbf0bf6e ("virtio-net: remove virtio queues if the guest doesn't support multiqueue") >>>>>> Buglink: https://issues.redhat.com/browse/RHEL-73842 >>>>>> Cc: qemu-stable@nongnu.org >>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>>> >>>>> The issue seems specific to virtio net: rset is reset, >>>>> it is distict from set features. >>>>> Why not just call the necessary functionality from virtio_net_reset? >>>> >>>> set_features is currently implemented only in virtio-net; virtio-gpu-base >>>> also have a function set but it only has code to trace. If another device >>>> implements the function in the future, I think the device will also want to >>>> have it called during reset for the same reason with virtio-net. >>>> >>>> virtio_reset() also calls set_status to update the status field so calling >>>> set_features() is more aligned with the handling of the status field. >>> >>> That came to be because writing 0 to status resets the virtio device. >>> For a while, this was the only way to reset vhost-user so we just >>> went along with it. >> >> It is possible to have code to send a command to write 0 to status to >> vhost-user in reset(), but calling set_status() in virtio_reset() is more >> convenient and makes sense as the status is indeed being set to 0. I think >> the same reasoning applies to features. > > I don't know who makes assumptions that features are only set during > driver setup, though. > This will send an extra VHOST_USER_SET_FEATURES message for vhost-user, > for example. > I want to have a good reason to add this overhead. This change won't add an extra VHOST_USER_SET_FEATURES message for vhost-user because QEMU sends one when VIRTIO_CONFIG_S_DRIVER_OK is being set for status. The only current implementers of set_features() are virtio-net and virtio-gpu-base. We may have more implementers of set_features() in the future though so it can be worth to think of the semantics of set_features() before that. When thinking of that, I found the current semantics of set_features() is not aligned with set_status(), which is confusing. I hope changing this in virtio_reset() will eliminate this source of confusion and prevents a bug like what virtio-net has in the future. > >>> >>> >>>>> >>>>> >>>>>> --- >>>>>> hw/virtio/virtio.c | 86 +++++++++++++++++++++++++++--------------------------- >>>>>> 1 file changed, 43 insertions(+), 43 deletions(-) >>>>>> >>>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>>>>> index 85110bce3744..033e87cdd3b9 100644 >>>>>> --- a/hw/virtio/virtio.c >>>>>> +++ b/hw/virtio/virtio.c >>>>>> @@ -2316,49 +2316,6 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index) >>>>>> } >>>>>> } >>>>>> -void virtio_reset(void *opaque) >>>>>> -{ >>>>>> - VirtIODevice *vdev = opaque; >>>>>> - VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); >>>>>> - int i; >>>>>> - >>>>>> - virtio_set_status(vdev, 0); >>>>>> - if (current_cpu) { >>>>>> - /* Guest initiated reset */ >>>>>> - vdev->device_endian = virtio_current_cpu_endian(); >>>>>> - } else { >>>>>> - /* System reset */ >>>>>> - vdev->device_endian = virtio_default_endian(); >>>>>> - } >>>>>> - >>>>>> - if (k->get_vhost) { >>>>>> - struct vhost_dev *hdev = k->get_vhost(vdev); >>>>>> - /* Only reset when vhost back-end is connected */ >>>>>> - if (hdev && hdev->vhost_ops) { >>>>>> - vhost_reset_device(hdev); >>>>>> - } >>>>>> - } >>>>>> - >>>>>> - if (k->reset) { >>>>>> - k->reset(vdev); >>>>>> - } >>>>>> - >>>>>> - vdev->start_on_kick = false; >>>>>> - vdev->started = false; >>>>>> - vdev->broken = false; >>>>>> - vdev->guest_features = 0; >>>>>> - vdev->queue_sel = 0; >>>>>> - vdev->status = 0; >>>>>> - vdev->disabled = false; >>>>>> - qatomic_set(&vdev->isr, 0); >>>>>> - vdev->config_vector = VIRTIO_NO_VECTOR; >>>>>> - virtio_notify_vector(vdev, vdev->config_vector); >>>>>> - >>>>>> - for(i = 0; i < VIRTIO_QUEUE_MAX; i++) { >>>>>> - __virtio_queue_reset(vdev, i); >>>>>> - } >>>>>> -} >>>>>> - >>>>>> void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr) >>>>>> { >>>>>> if (!vdev->vq[n].vring.num) { >>>>>> @@ -3169,6 +3126,49 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val) >>>>>> return ret; >>>>>> } >>>>>> +void virtio_reset(void *opaque) >>>>>> +{ >>>>>> + VirtIODevice *vdev = opaque; >>>>>> + VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); >>>>>> + int i; >>>>>> + >>>>>> + virtio_set_status(vdev, 0); >>>>>> + if (current_cpu) { >>>>>> + /* Guest initiated reset */ >>>>>> + vdev->device_endian = virtio_current_cpu_endian(); >>>>>> + } else { >>>>>> + /* System reset */ >>>>>> + vdev->device_endian = virtio_default_endian(); >>>>>> + } >>>>>> + >>>>>> + if (k->get_vhost) { >>>>>> + struct vhost_dev *hdev = k->get_vhost(vdev); >>>>>> + /* Only reset when vhost back-end is connected */ >>>>>> + if (hdev && hdev->vhost_ops) { >>>>>> + vhost_reset_device(hdev); >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + if (k->reset) { >>>>>> + k->reset(vdev); >>>>>> + } >>>>>> + >>>>>> + vdev->start_on_kick = false; >>>>>> + vdev->started = false; >>>>>> + vdev->broken = false; >>>>>> + virtio_set_features_nocheck(vdev, 0); >>>>>> + vdev->queue_sel = 0; >>>>>> + vdev->status = 0; >>>>>> + vdev->disabled = false; >>>>>> + qatomic_set(&vdev->isr, 0); >>>>>> + vdev->config_vector = VIRTIO_NO_VECTOR; >>>>>> + virtio_notify_vector(vdev, vdev->config_vector); >>>>>> + >>>>>> + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { >>>>>> + __virtio_queue_reset(vdev, i); >>>>>> + } >>>>>> +} >>>>>> + >>>>>> static void virtio_device_check_notification_compatibility(VirtIODevice *vdev, >>>>>> Error **errp) >>>>>> { >>>>>> >>>>>> --- >>>>>> base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32 >>>>>> change-id: 20250406-reset-5ed5248ee3c1 >>>>>> >>>>>> Best regards, >>>>>> -- >>>>>> Akihiko Odaki <akihiko.odaki@daynix.com> >>>>> >>> >
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 85110bce3744..033e87cdd3b9 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -2316,49 +2316,6 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index) } } -void virtio_reset(void *opaque) -{ - VirtIODevice *vdev = opaque; - VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); - int i; - - virtio_set_status(vdev, 0); - if (current_cpu) { - /* Guest initiated reset */ - vdev->device_endian = virtio_current_cpu_endian(); - } else { - /* System reset */ - vdev->device_endian = virtio_default_endian(); - } - - if (k->get_vhost) { - struct vhost_dev *hdev = k->get_vhost(vdev); - /* Only reset when vhost back-end is connected */ - if (hdev && hdev->vhost_ops) { - vhost_reset_device(hdev); - } - } - - if (k->reset) { - k->reset(vdev); - } - - vdev->start_on_kick = false; - vdev->started = false; - vdev->broken = false; - vdev->guest_features = 0; - vdev->queue_sel = 0; - vdev->status = 0; - vdev->disabled = false; - qatomic_set(&vdev->isr, 0); - vdev->config_vector = VIRTIO_NO_VECTOR; - virtio_notify_vector(vdev, vdev->config_vector); - - for(i = 0; i < VIRTIO_QUEUE_MAX; i++) { - __virtio_queue_reset(vdev, i); - } -} - void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr) { if (!vdev->vq[n].vring.num) { @@ -3169,6 +3126,49 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val) return ret; } +void virtio_reset(void *opaque) +{ + VirtIODevice *vdev = opaque; + VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); + int i; + + virtio_set_status(vdev, 0); + if (current_cpu) { + /* Guest initiated reset */ + vdev->device_endian = virtio_current_cpu_endian(); + } else { + /* System reset */ + vdev->device_endian = virtio_default_endian(); + } + + if (k->get_vhost) { + struct vhost_dev *hdev = k->get_vhost(vdev); + /* Only reset when vhost back-end is connected */ + if (hdev && hdev->vhost_ops) { + vhost_reset_device(hdev); + } + } + + if (k->reset) { + k->reset(vdev); + } + + vdev->start_on_kick = false; + vdev->started = false; + vdev->broken = false; + virtio_set_features_nocheck(vdev, 0); + vdev->queue_sel = 0; + vdev->status = 0; + vdev->disabled = false; + qatomic_set(&vdev->isr, 0); + vdev->config_vector = VIRTIO_NO_VECTOR; + virtio_notify_vector(vdev, vdev->config_vector); + + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { + __virtio_queue_reset(vdev, i); + } +} + static void virtio_device_check_notification_compatibility(VirtIODevice *vdev, Error **errp) {
virtio-net expects set_features() will be called when the feature set used by the guest changes to update the number of virtqueues. Call it during reset as reset clears all features and the queues added for VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS will need to be removed. Fixes: f9d6dbf0bf6e ("virtio-net: remove virtio queues if the guest doesn't support multiqueue") Buglink: https://issues.redhat.com/browse/RHEL-73842 Cc: qemu-stable@nongnu.org Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- hw/virtio/virtio.c | 86 +++++++++++++++++++++++++++--------------------------- 1 file changed, 43 insertions(+), 43 deletions(-) --- base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32 change-id: 20250406-reset-5ed5248ee3c1 Best regards,