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-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.
>
> 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.od...@daynix.com>
> > > >
> >
