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-sta...@nongnu.org
> > > Signed-off-by: Akihiko Odaki <akihiko.od...@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.od...@daynix.com>
> >
