[PATCH] virtio: Call set_features during reset

[PATCH] virtio: Call set_features during reset

[Akihiko Odaki]

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);
+    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>

Re: [PATCH] virtio: Call set_features during reset

[Michael S. Tsirkin]

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>

Re: [PATCH] virtio: Call set_features during reset

[Akihiko Odaki]

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

Re: [PATCH] virtio: Call set_features during reset

[Michael S. Tsirkin]

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

Re: [PATCH] virtio: Call set_features during reset

[Akihiko Odaki]

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

Re: [PATCH] virtio: Call set_features during reset

[Michael S. Tsirkin]

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

Re: [PATCH] virtio: Call set_features during reset

[Akihiko Odaki]

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

Re: [PATCH] virtio: Call set_features during reset

[Philippe Mathieu-Daudé]

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,

Re: [PATCH] virtio: Call set_features during reset

[Akihiko Odaki]

On 2025/04/10 18:32, Philippe Mathieu-Daudé wrote:
> 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.

I'm thinking of splitting in the reversed order: add this function call 
with a forward declaration in a first patch and move virtio_reset() in a 
second one. Hopefully it will also make backporting easier.

> 
> For my own education, are feature sets modifiable at runtime?

Looking at the code and spec, yes, I think so. The feature set cannot be 
modified if VIRTIO_CONFIG_S_FEATURES_OK is set as part of the "status", 
but the flag can be cleared anytime.

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

Re: [PATCH] virtio: Call set_features during reset

[Jason Wang]

On Thu, Apr 10, 2025 at 3:42 PM Akihiko Odaki <akihiko.odaki@daynix.com> 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.

It's not clear to me what kind of problem we want to fix here. For
example, what happens if we don't do this.

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

I would just add a forward declaration instead for a smaller changset
to ease the review and backport.

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

Thanks

>
>

Re: [PATCH] virtio: Call set_features during reset

[Akihiko Odaki]

On 2025/04/16 14:46, Jason Wang wrote:
> On Thu, Apr 10, 2025 at 3:42 PM Akihiko Odaki <akihiko.odaki@daynix.com> 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.
> 
> It's not clear to me what kind of problem we want to fix here. For
> example, what happens if we don't do this.

Without this change, if the device gets reset after VIRTIO_NET_F_MQ or 
VIRTIO_NET_F_RSS is configured:
- the guest will see too many virtqueues
- migration results in segfault due to the mismatch with the number of 
virtqueues and feature set

I'll add the description to the patch message with the next version.

> 
>>
>> 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);
> 
> I would just add a forward declaration instead for a smaller changset
> to ease the review and backport.

Moving virtio_reset() indeed requires a bigger change, but I still want 
to move it to the latter part of this file. I occasionally saw that 
functions that affect the whole device state like this calls a number of 
static functions. Moving virtio_reset() requires more lines to change 
now, but I hope it will minimize the possibility to require forward 
declarations or code movement in the future.

Regards,
Akihiko Odaki

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

Re: [PATCH] migration: fix SEEK_CUR offset calculation in qio_channel_block_seek

[Fabiano Rosas]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 26.03.2025 19:22, Marco Cavenati wrote:
>> The SEEK_CUR case in qio_channel_block_seek was incorrectly using the
>> 'whence' parameter instead of the 'offset' parameter when calculating the
>> new position.
>> 
>> Fixes: 65cf200a51ddc6d0a28ecceac30dc892233cddd7 ("migration: introduce a QIOChannel impl for BlockDriverState VMState")
>> 
>> Signed-off-by: Marco Cavenati <Marco.Cavenati@eurecom.fr>
>> ---
>>   migration/channel-block.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/migration/channel-block.c b/migration/channel-block.c
>> index fff8d87094..b0477f5b6d 100644
>> --- a/migration/channel-block.c
>> +++ b/migration/channel-block.c
>> @@ -123,7 +123,7 @@ qio_channel_block_seek(QIOChannel *ioc,
>>           bioc->offset = offset;
>>           break;
>>       case SEEK_CUR:
>> -        bioc->offset += whence;
>> +        bioc->offset += offset;
>>           break;
>>       case SEEK_END:
>>           error_setg(errp, "Size of VMstate region is unknown");
>
> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
>
> This is a (trivial) bugfix, I'd say it should be in 10.0.
> Will you guys send a pullreq for the block layer, or should
> I make a single-patch pullreq from the trivial tree?

I'll take it. It's not entirely trivial as it shifts a value by 1 in
mapped-ram migration. Fortunately, it's a value that doesn't need to be
the same between migration source and destination.

Thanks

>
> Thanks,
>
> /mjt

Re: [PATCH] migration: fix SEEK_CUR offset calculation in qio_channel_block_seek

[Michael Tokarev]

Hi!

This is

commit c0b32426ce56182c1ce2a12904f3a702c2ecc460
Author: Marco Cavenati <Marco.Cavenati@eurecom.fr>
Date:   Wed Mar 26 17:22:30 2025 +0100

     migration: fix SEEK_CUR offset calculation in qio_channel_block_seek

which went to 10.0.0-rc2, and has been cherry-picked to
7.2 and 9.2 stable series.

Reportedly it breaks migration in 7.2.18 and up.  Which is
kinda strange, as it shouldn't do any harm?

https://bugs.debian.org/1112044

any guess what's going on?

Thanks,

/mjt

Re: [PATCH] migration: fix SEEK_CUR offset calculation in qio_channel_block_seek

[Fabiano Rosas]

Michael Tokarev <mjt@tls.msk.ru> writes:

+CC Akihiko

> Hi!
>
> This is
>
> commit c0b32426ce56182c1ce2a12904f3a702c2ecc460
> Author: Marco Cavenati <Marco.Cavenati@eurecom.fr>
> Date:   Wed Mar 26 17:22:30 2025 +0100
>
>      migration: fix SEEK_CUR offset calculation in qio_channel_block_seek
>
> which went to 10.0.0-rc2, and has been cherry-picked to
> 7.2 and 9.2 stable series.
>
> Reportedly it breaks migration in 7.2.18 and up.  Which is
> kinda strange, as it shouldn't do any harm?
>

Yeah, this is not it. Unless you're using colo or mapped-ram.

> https://bugs.debian.org/1112044
>
> any guess what's going on?
>

The virtio changes are probably the issue. One of them touches
mhdr.num_buffers, under mergeable_rx_bufs, which is migrated state. The
flag in turn depends on VIRTIO_NET_F_MRG_RXBUF, which is set on the
cmdline with -device virtio-net-pci,mrg_rxbuf= but also reset by
virtio_set_features_nocheck, if I'm reading this right.

Let's ask Akihiko.

> Thanks,
>
> /mjt

Re: [PATCH] migration: fix SEEK_CUR offset calculation in qio_channel_block_seek

[Akihiko Odaki]

On 2025/08/27 6:52, Fabiano Rosas wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
> +CC Akihiko
> 
>> Hi!
>>
>> This is
>>
>> commit c0b32426ce56182c1ce2a12904f3a702c2ecc460
>> Author: Marco Cavenati <Marco.Cavenati@eurecom.fr>
>> Date:   Wed Mar 26 17:22:30 2025 +0100
>>
>>       migration: fix SEEK_CUR offset calculation in qio_channel_block_seek
>>
>> which went to 10.0.0-rc2, and has been cherry-picked to
>> 7.2 and 9.2 stable series.
>>
>> Reportedly it breaks migration in 7.2.18 and up.  Which is
>> kinda strange, as it shouldn't do any harm?
>>
> 
> Yeah, this is not it. Unless you're using colo or mapped-ram.
> 
>> https://bugs.debian.org/1112044
>>
>> any guess what's going on?
>>
> 
> The virtio changes are probably the issue. One of them touches
> mhdr.num_buffers, under mergeable_rx_bufs, which is migrated state. The
> flag in turn depends on VIRTIO_NET_F_MRG_RXBUF, which is set on the
> cmdline with -device virtio-net-pci,mrg_rxbuf= but also reset by
> virtio_set_features_nocheck, if I'm reading this right.

I don't think commit cefd67f25430 ("virtio-net: Fix num_buffers for 
version 1") is related to the issue. Commit ce1431615292 ("virtio: Call 
set_features during reset") is more likely.

virtio_set_features_nocheck() shouldn't reset VIRTIO_NET_F_MRG_RXBUF. It 
calls virtio_net_set_features(), which does not clear features. 
virtio_net_get_features() clears features, but it is called before 
migration.

The posted call trace indicates a lockup happens in the control path, 
but commit cefd67f25430 ("virtio-net: Fix num_buffers for version 1") 
changes the data path.

On the other hand, I can come up with a possible failure scenario with 
commit ce1431615292 ("virtio: Call set_features during reset"). Perhaps 
it changed the machine state before loading the migrated state, and 
caused a mismatch between them.

I need more information to understand the issue. A command line to 
reproduce the issue is especially helpful because options like 
mrg_rxbuf=, which you mentioned, tell enabled features, which is 
valuable information.

Regards,
Akihiko Odaki

Re: [PATCH] virtio: Call set_features during reset

[Michael Tokarev]

On 28.08.2025 03:57, Akihiko Odaki wrote:

> The posted call trace indicates a lockup happens in the control path, 
> but commit cefd67f25430 ("virtio-net: Fix num_buffers for version 1") 
> changes the data path.
> 
> On the other hand, I can come up with a possible failure scenario with 
> commit ce1431615292 ("virtio: Call set_features during reset"). Perhaps 
> it changed the machine state before loading the migrated state, and 
> caused a mismatch between them.

Yes, the problem commit is 0caed25cd171c6 "virtio: Call set_features
during reset", - the OP corrected himself in the next message (subject
line updated).

> I need more information to understand the issue. A command line to 
> reproduce the issue is especially helpful because options like 
> mrg_rxbuf=, which you mentioned, tell enabled features, which is 
> valuable information.

See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1112044#69 for
the command line.  The guest is started by libvirtd.

Please note: this is stable-7.2 series, it is *not* master (I picked
up this commit to 7.2.x).  It'd be interesting to check if master is
affected too.  Unfortunately I never tried migration, and now I only
have my notebook, so can only migrate between two qemu instances on
the same box - which is probably okay too.

Thanks,

/mjt

Re: [PATCH] virtio: Call set_features during reset

[Akihiko Odaki]

On 2025/08/29 17:34, Michael Tokarev wrote:
> On 28.08.2025 03:57, Akihiko Odaki wrote:
> 
>> The posted call trace indicates a lockup happens in the control path, 
>> but commit cefd67f25430 ("virtio-net: Fix num_buffers for version 1") 
>> changes the data path.
>>
>> On the other hand, I can come up with a possible failure scenario with 
>> commit ce1431615292 ("virtio: Call set_features during reset"). 
>> Perhaps it changed the machine state before loading the migrated 
>> state, and caused a mismatch between them.
> 
> Yes, the problem commit is 0caed25cd171c6 "virtio: Call set_features
> during reset", - the OP corrected himself in the next message (subject
> line updated).
> 
>> I need more information to understand the issue. A command line to 
>> reproduce the issue is especially helpful because options like 
>> mrg_rxbuf=, which you mentioned, tell enabled features, which is 
>> valuable information.
> 
> See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1112044#69 for
> the command line.  The guest is started by libvirtd.

Thank you, now I think I understand the problem.

> 
> Please note: this is stable-7.2 series, it is *not* master (I picked
> up this commit to 7.2.x).  It'd be interesting to check if master is
> affected too.  Unfortunately I never tried migration, and now I only
> have my notebook, so can only migrate between two qemu instances on
> the same box - which is probably okay too.

I think you need to backport commit 9379ea9db3c0 ("virtio-net: Add 
queues before loading them") and adda0ad56bd2 ("virtio-net: Add queues 
for RSS during migration"). Here is an explanation:

First, let me define two variables for conciseness:
N: the number of queue pairs
M: the maximum number of queue pairs, which is determined with
    n->max_queue_pairs

The problem is that QEMU inconsistently chose N for virtio-net in the 
past. Before commit 8c49756825da ("virtio-net: Add only one queue pair 
when realizing"):
1) realize() chose M.
2) set_features() chose: 1 (when RSS and MQ are disabled)
                          M (otherwise)

This itself was a problem; both RSS and MQ were disabled when realize() 
but N was M, which is inconsistent with 2) and this inconsistency was 
guest-visible.

I wrote commit 8c49756825da ("virtio-net: Add only one queue pair when 
realizing") to make QEMU implement the behavior in 2) also during 
realization and fix the inconsistency, but it broke migration when the 
migrated VM had enabled VIRTIO_NET_F_RSS and VIRTIO_NET_F_MQ because it 
expected that N == M.

This is also why the backported commit also broke migration; it 
accidentally fixed the inconsistency between the first reset state and 
the state after set_features() and caused the same problem.

I wrote commit 9379ea9db3c0 ("virtio-net: Add queues before loading 
them") to fix the issue and later complemented it with commit 
adda0ad56bd2 ("virtio-net: Add queues for RSS during migration").

There are several relevant commits because I could not fix the 
underlying problem at once, but hopefully this email clarifies how the 
two commits fixed it in the end.

Regards,
Akihiko Odaki

Re: [PATCH] virtio: Call set_features during reset

[Michael Tokarev]

On 29.08.2025 17:40, Akihiko Odaki wrote:
> On 2025/08/29 17:34, Michael Tokarev wrote:
>> On 28.08.2025 03:57, Akihiko Odaki wrote:
...
>> Please note: this is stable-7.2 series, it is *not* master (I picked
>> up this commit to 7.2.x).  It'd be interesting to check if master is
>> affected too.  Unfortunately I never tried migration, and now I only
>> have my notebook, so can only migrate between two qemu instances on
>> the same box - which is probably okay too.
> 
> I think you need to backport commit 9379ea9db3c0 ("virtio-net: Add 
> queues before loading them") and adda0ad56bd2 ("virtio-net: Add queues 
> for RSS during migration"). Here is an explanation:

"Add queues before loading them" is marked as fixing

Both the mentioned commits were tagged with Cc: qemu-stable, but both
has Fixes: the same commit 8c49756825da ("virtio-net: Add only one queue
pair when realizing"), which is in 9.1, but not in 7.2.  I guess, in
this case, I also need 8c49756825da - let's first break things, and
next fix them :)

> First, let me define two variables for conciseness:
> N: the number of queue pairs
> M: the maximum number of queue pairs, which is determined with
>     n->max_queue_pairs
> 
> The problem is that QEMU inconsistently chose N for virtio-net in the 
> past. Before commit 8c49756825da ("virtio-net: Add only one queue pair 
> when realizing"):
> 1) realize() chose M.
> 2) set_features() chose: 1 (when RSS and MQ are disabled)
>                           M (otherwise)
> 
> This itself was a problem; both RSS and MQ were disabled when realize() 
> but N was M, which is inconsistent with 2) and this inconsistency was 
> guest-visible.
> 
> I wrote commit 8c49756825da ("virtio-net: Add only one queue pair when 
> realizing") to make QEMU implement the behavior in 2) also during 
> realization and fix the inconsistency, but it broke migration when the 
> migrated VM had enabled VIRTIO_NET_F_RSS and VIRTIO_NET_F_MQ because it 
> expected that N == M.

Yup, 8c49756825da it is, which is missing in 7.2, which broke things :))

> This is also why the backported commit also broke migration; it 
> accidentally fixed the inconsistency between the first reset state and 
> the state after set_features() and caused the same problem.
> 
> I wrote commit 9379ea9db3c0 ("virtio-net: Add queues before loading 
> them") to fix the issue and later complemented it with commit 
> adda0ad56bd2 ("virtio-net: Add queues for RSS during migration").
> 
> There are several relevant commits because I could not fix the 
> underlying problem at once, but hopefully this email clarifies how the 
> two commits fixed it in the end.

Now, with 3 commits picked up to 7.2, things are at least compiles :))

Let's test the result..  hopefully I didn't break something else in the
process of breaking+fixing stuff :))

Thank you very much for the detailed explanation and attention to
details.  It makes sense.

An for the 7.2.x series which is getting old(ish) - I plan to end
support for it in the near future, so there will be less surprises
like this one.. if not only for 10.0.x :)

/mjt