Re: [PATCH] virtio: Call set_features during reset

["Michael S. Tsirkin" <mst@redhat.com>]

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>
> > > > 
> >