It was returned as error before. Instead of it, simply update the
corresponding field so qemu can send it in the migration data.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/net/virtio-net.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index dd0d056fde..63a8332cd0 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;
     }
 
-    /* Avoid changing the number of queue_pairs for vdpa device in
-     * userspace handler. A future fix is needed to handle the mq
-     * change in userspace handler with vhost-vdpa. Let's disable
-     * the mq handling from userspace for now and only allow get
-     * done through the kernel. Ripples may be seen when falling
-     * back to userspace, but without doing it qemu process would
-     * crash on a recursive entry to virtio_net_set_status().
-     */
+    n->curr_queue_pairs = queue_pairs;
     if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
-        return VIRTIO_NET_ERR;
+        /*
+         * Avoid updating the backend for a vdpa device: We're only interested
+         * in updating the device model queues.
+         */
+        return VIRTIO_NET_OK;
     }
-
-    n->curr_queue_pairs = queue_pairs;
     /* stop the backend before changing the number of queue_pairs to avoid handling a
      * disabled queue */
     virtio_net_set_status(vdev, vdev->status);
-- 
2.31.1

在 2022/8/20 01:13, Eugenio Pérez 写道:
> It was returned as error before. Instead of it, simply update the
> corresponding field so qemu can send it in the migration data.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---


Looks correct.

Adding Si Wei for double check.

Thanks


>   hw/net/virtio-net.c | 17 ++++++-----------
>   1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index dd0d056fde..63a8332cd0 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
>           return VIRTIO_NET_ERR;
>       }
>   
> -    /* Avoid changing the number of queue_pairs for vdpa device in
> -     * userspace handler. A future fix is needed to handle the mq
> -     * change in userspace handler with vhost-vdpa. Let's disable
> -     * the mq handling from userspace for now and only allow get
> -     * done through the kernel. Ripples may be seen when falling
> -     * back to userspace, but without doing it qemu process would
> -     * crash on a recursive entry to virtio_net_set_status().
> -     */
> +    n->curr_queue_pairs = queue_pairs;
>       if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> -        return VIRTIO_NET_ERR;
> +        /*
> +         * Avoid updating the backend for a vdpa device: We're only interested
> +         * in updating the device model queues.
> +         */
> +        return VIRTIO_NET_OK;
>       }
> -
> -    n->curr_queue_pairs = queue_pairs;
>       /* stop the backend before changing the number of queue_pairs to avoid handling a
>        * disabled queue */
>       virtio_net_set_status(vdev, vdev->status);

On 8/23/2022 9:27 PM, Jason Wang wrote:
>
> 在 2022/8/20 01:13, Eugenio Pérez 写道:
>> It was returned as error before. Instead of it, simply update the
>> corresponding field so qemu can send it in the migration data.
>>
>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> ---
>
>
> Looks correct.
>
> Adding Si Wei for double check.
Hmmm, I understand why this change is needed for live migration, but 
this would easily cause userspace out of sync with the kernel for other 
use cases, such as link down or userspace fallback due to vdpa ioctl 
error. Yes, these are edge cases. Not completely against it, but I 
wonder if there's a way we can limit the change scope to live migration 
case only?

-Siwei

>
> Thanks
>
>
>>   hw/net/virtio-net.c | 17 ++++++-----------
>>   1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index dd0d056fde..63a8332cd0 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n, 
>> uint8_t cmd,
>>           return VIRTIO_NET_ERR;
>>       }
>>   -    /* Avoid changing the number of queue_pairs for vdpa device in
>> -     * userspace handler. A future fix is needed to handle the mq
>> -     * change in userspace handler with vhost-vdpa. Let's disable
>> -     * the mq handling from userspace for now and only allow get
>> -     * done through the kernel. Ripples may be seen when falling
>> -     * back to userspace, but without doing it qemu process would
>> -     * crash on a recursive entry to virtio_net_set_status().
>> -     */
>> +    n->curr_queue_pairs = queue_pairs;
>>       if (nc->peer && nc->peer->info->type == 
>> NET_CLIENT_DRIVER_VHOST_VDPA) {
>> -        return VIRTIO_NET_ERR;
>> +        /*
>> +         * Avoid updating the backend for a vdpa device: We're only 
>> interested
>> +         * in updating the device model queues.
>> +         */
>> +        return VIRTIO_NET_OK;
>>       }
>> -
>> -    n->curr_queue_pairs = queue_pairs;
>>       /* stop the backend before changing the number of queue_pairs 
>> to avoid handling a
>>        * disabled queue */
>>       virtio_net_set_status(vdev, vdev->status);
>

On Thu, Aug 25, 2022 at 2:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 8/23/2022 9:27 PM, Jason Wang wrote:
> >
> > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> >> It was returned as error before. Instead of it, simply update the
> >> corresponding field so qemu can send it in the migration data.
> >>
> >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >> ---
> >
> >
> > Looks correct.
> >
> > Adding Si Wei for double check.
> Hmmm, I understand why this change is needed for live migration, but
> this would easily cause userspace out of sync with the kernel for other
> use cases, such as link down or userspace fallback due to vdpa ioctl
> error. Yes, these are edge cases.

The link down case is not possible at this moment because that cvq
command does not call virtio_net_handle_ctrl_iov. A similar treatment
than mq would be needed when supported, and the call to
virtio_net_set_status will be avoided.

I'll double check device initialization ioctl failure with
n->curr_queue_pairs > 1 in the destination, but I think we should be
safe.

> Not completely against it, but I
> wonder if there's a way we can limit the change scope to live migration
> case only?
>

The reason to update the device model is to send the curr_queue_pairs
to the destination in a backend agnostic way. To send it otherwise
would limit the live migration possibilities, but sure we can explore
another way.

Thanks!

On 8/24/2022 11:19 PM, Eugenio Perez Martin wrote:
> On Thu, Aug 25, 2022 at 2:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 8/23/2022 9:27 PM, Jason Wang wrote:
>>> 在 2022/8/20 01:13, Eugenio Pérez 写道:
>>>> It was returned as error before. Instead of it, simply update the
>>>> corresponding field so qemu can send it in the migration data.
>>>>
>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> ---
>>>
>>> Looks correct.
>>>
>>> Adding Si Wei for double check.
>> Hmmm, I understand why this change is needed for live migration, but
>> this would easily cause userspace out of sync with the kernel for other
>> use cases, such as link down or userspace fallback due to vdpa ioctl
>> error. Yes, these are edge cases.
> The link down case is not possible at this moment because that cvq
> command does not call virtio_net_handle_ctrl_iov.
Right. Though shadow cvq would need to rely on extra ASID support from 
kernel. For the case without shadow cvq we still need to look for an 
alternative mechanism.

> A similar treatment
> than mq would be needed when supported, and the call to
> virtio_net_set_status will be avoided.
So, maybe the seemingly "right" fix for the moment is to prohibit manual 
set_link at all (for vDPA only)? In longer term we'd need to come up 
with appropriate support for applying mq config regardless of asid or 
shadow cvq support.

>
> I'll double check device initialization ioctl failure with
> n->curr_queue_pairs > 1 in the destination, but I think we should be
> safe.
>
>> Not completely against it, but I
>> wonder if there's a way we can limit the change scope to live migration
>> case only?
>>
> The reason to update the device model is to send the curr_queue_pairs
> to the destination in a backend agnostic way. To send it otherwise
> would limit the live migration possibilities, but sure we can explore
> another way.
A hacky workaround that came off the top of my head was to allow sending 
curr_queue_pairs for the !vm_running case for vdpa. It doesn't look it 
would affect other backend I think. But I agree with Jason, this doesn't 
look decent so I give up on this idea. Hence for this patch,

Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>

>
> Thanks!
>

On Fri, Aug 26, 2022 at 6:29 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 8/24/2022 11:19 PM, Eugenio Perez Martin wrote:
> > On Thu, Aug 25, 2022 at 2:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >>
> >>
> >> On 8/23/2022 9:27 PM, Jason Wang wrote:
> >>> 在 2022/8/20 01:13, Eugenio Pérez 写道:
> >>>> It was returned as error before. Instead of it, simply update the
> >>>> corresponding field so qemu can send it in the migration data.
> >>>>
> >>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>> ---
> >>>
> >>> Looks correct.
> >>>
> >>> Adding Si Wei for double check.
> >> Hmmm, I understand why this change is needed for live migration, but
> >> this would easily cause userspace out of sync with the kernel for other
> >> use cases, such as link down or userspace fallback due to vdpa ioctl
> >> error. Yes, these are edge cases.
> > The link down case is not possible at this moment because that cvq
> > command does not call virtio_net_handle_ctrl_iov.
> Right. Though shadow cvq would need to rely on extra ASID support from
> kernel. For the case without shadow cvq we still need to look for an
> alternative mechanism.
>
> > A similar treatment
> > than mq would be needed when supported, and the call to
> > virtio_net_set_status will be avoided.
> So, maybe the seemingly "right" fix for the moment is to prohibit manual
> set_link at all (for vDPA only)?

We can apply a similar solution and just save the link status, without
stopping any vqp backend. The code can be more elegant than checking
if the backend is vhost-vdpa of course, but what is the problem with
doing it that way?

> In longer term we'd need to come up
> with appropriate support for applying mq config regardless of asid or
> shadow cvq support.
>

What do you mean by applying "mq config"? To the virtio-net device
model in qemu? Is there any use case to apply it to the model outside
of live migration?

On the other hand, the current approach is not using ASID at all, it
will be added on top. Do you mean that it is needed for data
passthrough & CVQ shadow, isn't it?

> >
> > I'll double check device initialization ioctl failure with
> > n->curr_queue_pairs > 1 in the destination, but I think we should be
> > safe.
> >
> >> Not completely against it, but I
> >> wonder if there's a way we can limit the change scope to live migration
> >> case only?
> >>
> > The reason to update the device model is to send the curr_queue_pairs
> > to the destination in a backend agnostic way. To send it otherwise
> > would limit the live migration possibilities, but sure we can explore
> > another way.
> A hacky workaround that came off the top of my head was to allow sending
> curr_queue_pairs for the !vm_running case for vdpa. It doesn't look it
> would affect other backend I think. But I agree with Jason, this doesn't
> look decent so I give up on this idea. Hence for this patch,
>

I still don't get the problem. Also, the guest would need to reset the
device anyway, so that information will be lost, isn't it?

Thanks!

> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>
> >
> > Thanks!
> >
>

On Thu, Aug 25, 2022 at 8:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 8/23/2022 9:27 PM, Jason Wang wrote:
> >
> > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> >> It was returned as error before. Instead of it, simply update the
> >> corresponding field so qemu can send it in the migration data.
> >>
> >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >> ---
> >
> >
> > Looks correct.
> >
> > Adding Si Wei for double check.
> Hmmm, I understand why this change is needed for live migration, but
> this would easily cause userspace out of sync with the kernel for other
> use cases, such as link down or userspace fallback due to vdpa ioctl
> error. Yes, these are edge cases.

Considering 7.2 will start, maybe it's time to fix the root cause
instead of having a workaround like this?

THanks

> Not completely against it, but I
> wonder if there's a way we can limit the change scope to live migration
> case only?
>
> -Siwei
>
> >
> > Thanks
> >
> >
> >>   hw/net/virtio-net.c | 17 ++++++-----------
> >>   1 file changed, 6 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index dd0d056fde..63a8332cd0 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n,
> >> uint8_t cmd,
> >>           return VIRTIO_NET_ERR;
> >>       }
> >>   -    /* Avoid changing the number of queue_pairs for vdpa device in
> >> -     * userspace handler. A future fix is needed to handle the mq
> >> -     * change in userspace handler with vhost-vdpa. Let's disable
> >> -     * the mq handling from userspace for now and only allow get
> >> -     * done through the kernel. Ripples may be seen when falling
> >> -     * back to userspace, but without doing it qemu process would
> >> -     * crash on a recursive entry to virtio_net_set_status().
> >> -     */
> >> +    n->curr_queue_pairs = queue_pairs;
> >>       if (nc->peer && nc->peer->info->type ==
> >> NET_CLIENT_DRIVER_VHOST_VDPA) {
> >> -        return VIRTIO_NET_ERR;
> >> +        /*
> >> +         * Avoid updating the backend for a vdpa device: We're only
> >> interested
> >> +         * in updating the device model queues.
> >> +         */
> >> +        return VIRTIO_NET_OK;
> >>       }
> >> -
> >> -    n->curr_queue_pairs = queue_pairs;
> >>       /* stop the backend before changing the number of queue_pairs
> >> to avoid handling a
> >>        * disabled queue */
> >>       virtio_net_set_status(vdev, vdev->status);
> >
>

Hi Jason,

On 8/24/2022 7:53 PM, Jason Wang wrote:
> On Thu, Aug 25, 2022 at 8:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 8/23/2022 9:27 PM, Jason Wang wrote:
>>> 在 2022/8/20 01:13, Eugenio Pérez 写道:
>>>> It was returned as error before. Instead of it, simply update the
>>>> corresponding field so qemu can send it in the migration data.
>>>>
>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> ---
>>>
>>> Looks correct.
>>>
>>> Adding Si Wei for double check.
>> Hmmm, I understand why this change is needed for live migration, but
>> this would easily cause userspace out of sync with the kernel for other
>> use cases, such as link down or userspace fallback due to vdpa ioctl
>> error. Yes, these are edge cases.
> Considering 7.2 will start, maybe it's time to fix the root cause
> instead of having a workaround like this?
The fix for the immediate cause is not hard, though what is missing from 
my WIP series for a full blown fix is something similar to Shadow CVQ 
for all general cases than just live migration: QEMU will have to apply 
the curr_queue_pairs to the kernel once switched back from the userspace 
virtqueues. I think Shadow CVQ won't work if ASID support is missing 
from kernel. Do you think if it bother to build another similar 
facility, or we reuse Shadow CVQ code to make it work without ASID support?

I have been a bit busy with internal project for the moment, but I hope 
I can post my series next week. Here's what I have for the relevant 
patches from the WIP series.

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index dd0d056..16edfa3 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -361,16 +361,13 @@ static void 
virtio_net_drop_tx_queue_data(VirtIODevice *vdev, VirtQueue *vq)
      }
  }

-static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t 
status)
+static void virtio_net_queue_status(struct VirtIONet *n, uint8_t status)
  {
-    VirtIONet *n = VIRTIO_NET(vdev);
+    VirtIODevice *vdev = VIRTIO_DEVICE(n);
      VirtIONetQueue *q;
      int i;
      uint8_t queue_status;

-    virtio_net_vnet_endian_status(n, status);
-    virtio_net_vhost_status(n, status);
-
      for (i = 0; i < n->max_queue_pairs; i++) {
          NetClientState *ncs = qemu_get_subqueue(n->nic, i);
          bool queue_started;
@@ -418,6 +415,15 @@ static void virtio_net_set_status(struct 
VirtIODevice *vdev, uint8_t status)
      }
  }

+static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t 
status)
+{
+    VirtIONet *n = VIRTIO_NET(vdev);
+
+    virtio_net_vnet_endian_status(n, status);
+    virtio_net_vhost_status(n, status);
+    virtio_net_queue_status(n, status);
+}
+
  static void virtio_net_set_link_status(NetClientState *nc)
  {
      VirtIONet *n = qemu_get_nic_opaque(nc);
@@ -1380,7 +1386,6 @@ static int virtio_net_handle_mq(VirtIONet *n, 
uint8_t cmd,
  {
      VirtIODevice *vdev = VIRTIO_DEVICE(n);
      uint16_t queue_pairs;
-    NetClientState *nc = qemu_get_queue(n->nic);

      virtio_net_disable_rss(n);
      if (cmd == VIRTIO_NET_CTRL_MQ_HASH_CONFIG) {
@@ -1412,22 +1417,10 @@ static int virtio_net_handle_mq(VirtIONet *n, 
uint8_t cmd,
          return VIRTIO_NET_ERR;
      }

-    /* Avoid changing the number of queue_pairs for vdpa device in
-     * userspace handler. A future fix is needed to handle the mq
-     * change in userspace handler with vhost-vdpa. Let's disable
-     * the mq handling from userspace for now and only allow get
-     * done through the kernel. Ripples may be seen when falling
-     * back to userspace, but without doing it qemu process would
-     * crash on a recursive entry to virtio_net_set_status().
-     */
-    if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
-        return VIRTIO_NET_ERR;
-    }
-
      n->curr_queue_pairs = queue_pairs;
      /* stop the backend before changing the number of queue_pairs to 
avoid handling a
       * disabled queue */
-    virtio_net_set_status(vdev, vdev->status);
+    virtio_net_queue_status(n, vdev->status);
      virtio_net_set_queue_pairs(n);

      return VIRTIO_NET_OK;

----
Regards,
-Siwei
>
> THanks
>
>> Not completely against it, but I
>> wonder if there's a way we can limit the change scope to live migration
>> case only?
>>
>> -Siwei
>>
>>> Thanks
>>>
>>>
>>>>    hw/net/virtio-net.c | 17 ++++++-----------
>>>>    1 file changed, 6 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index dd0d056fde..63a8332cd0 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n,
>>>> uint8_t cmd,
>>>>            return VIRTIO_NET_ERR;
>>>>        }
>>>>    -    /* Avoid changing the number of queue_pairs for vdpa device in
>>>> -     * userspace handler. A future fix is needed to handle the mq
>>>> -     * change in userspace handler with vhost-vdpa. Let's disable
>>>> -     * the mq handling from userspace for now and only allow get
>>>> -     * done through the kernel. Ripples may be seen when falling
>>>> -     * back to userspace, but without doing it qemu process would
>>>> -     * crash on a recursive entry to virtio_net_set_status().
>>>> -     */
>>>> +    n->curr_queue_pairs = queue_pairs;
>>>>        if (nc->peer && nc->peer->info->type ==
>>>> NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>> -        return VIRTIO_NET_ERR;
>>>> +        /*
>>>> +         * Avoid updating the backend for a vdpa device: We're only
>>>> interested
>>>> +         * in updating the device model queues.
>>>> +         */
>>>> +        return VIRTIO_NET_OK;
>>>>        }
>>>> -
>>>> -    n->curr_queue_pairs = queue_pairs;
>>>>        /* stop the backend before changing the number of queue_pairs
>>>> to avoid handling a
>>>>         * disabled queue */
>>>>        virtio_net_set_status(vdev, vdev->status);

On Thu, Aug 25, 2022 at 10:53 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Aug 25, 2022 at 8:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >
> >
> >
> > On 8/23/2022 9:27 PM, Jason Wang wrote:
> > >
> > > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > >> It was returned as error before. Instead of it, simply update the
> > >> corresponding field so qemu can send it in the migration data.
> > >>
> > >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > >> ---
> > >
> > >
> > > Looks correct.
> > >
> > > Adding Si Wei for double check.
> > Hmmm, I understand why this change is needed for live migration, but
> > this would easily cause userspace out of sync with the kernel for other
> > use cases, such as link down or userspace fallback due to vdpa ioctl
> > error. Yes, these are edge cases.
>
> Considering 7.2 will start, maybe it's time to fix the root cause
> instead of having a workaround like this?

Btw, the patch actually tries its best to limit the behaviour, e.g it
doesn't do the following set_status() stuff. So I think it won't
trigger the issue you mentioned here?

Thanks

>
> THanks
>
> > Not completely against it, but I
> > wonder if there's a way we can limit the change scope to live migration
> > case only?
> >
> > -Siwei
> >
> > >
> > > Thanks
> > >
> > >
> > >>   hw/net/virtio-net.c | 17 ++++++-----------
> > >>   1 file changed, 6 insertions(+), 11 deletions(-)
> > >>
> > >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > >> index dd0d056fde..63a8332cd0 100644
> > >> --- a/hw/net/virtio-net.c
> > >> +++ b/hw/net/virtio-net.c
> > >> @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n,
> > >> uint8_t cmd,
> > >>           return VIRTIO_NET_ERR;
> > >>       }
> > >>   -    /* Avoid changing the number of queue_pairs for vdpa device in
> > >> -     * userspace handler. A future fix is needed to handle the mq
> > >> -     * change in userspace handler with vhost-vdpa. Let's disable
> > >> -     * the mq handling from userspace for now and only allow get
> > >> -     * done through the kernel. Ripples may be seen when falling
> > >> -     * back to userspace, but without doing it qemu process would
> > >> -     * crash on a recursive entry to virtio_net_set_status().
> > >> -     */
> > >> +    n->curr_queue_pairs = queue_pairs;
> > >>       if (nc->peer && nc->peer->info->type ==
> > >> NET_CLIENT_DRIVER_VHOST_VDPA) {
> > >> -        return VIRTIO_NET_ERR;
> > >> +        /*
> > >> +         * Avoid updating the backend for a vdpa device: We're only
> > >> interested
> > >> +         * in updating the device model queues.
> > >> +         */
> > >> +        return VIRTIO_NET_OK;
> > >>       }
> > >> -
> > >> -    n->curr_queue_pairs = queue_pairs;
> > >>       /* stop the backend before changing the number of queue_pairs
> > >> to avoid handling a
> > >>        * disabled queue */
> > >>       virtio_net_set_status(vdev, vdev->status);
> > >
> >

On 8/24/2022 8:05 PM, Jason Wang wrote:
> On Thu, Aug 25, 2022 at 10:53 AM Jason Wang <jasowang@redhat.com> wrote:
>> On Thu, Aug 25, 2022 at 8:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>>
>>>
>>> On 8/23/2022 9:27 PM, Jason Wang wrote:
>>>> 在 2022/8/20 01:13, Eugenio Pérez 写道:
>>>>> It was returned as error before. Instead of it, simply update the
>>>>> corresponding field so qemu can send it in the migration data.
>>>>>
>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>> ---
>>>>
>>>> Looks correct.
>>>>
>>>> Adding Si Wei for double check.
>>> Hmmm, I understand why this change is needed for live migration, but
>>> this would easily cause userspace out of sync with the kernel for other
>>> use cases, such as link down or userspace fallback due to vdpa ioctl
>>> error. Yes, these are edge cases.
>> Considering 7.2 will start, maybe it's time to fix the root cause
>> instead of having a workaround like this?
> Btw, the patch actually tries its best to limit the behaviour, e.g it
> doesn't do the following set_status() stuff. So I think it won't
> trigger the issue you mentioned here?
Well, we can claim we don't support the link down+up case while changing 
queue numbers in between. On the other hand, the error recovery from 
fallback userspace is another story, which would need more attention and 
care on the error path. Yes, if see it from that perspective the change 
is fine. For completeness, please refer to the patch in the other email.

-Siwei

>
> Thanks
>
>> THanks
>>
>>> Not completely against it, but I
>>> wonder if there's a way we can limit the change scope to live migration
>>> case only?
>>>
>>> -Siwei
>>>
>>>> Thanks
>>>>
>>>>
>>>>>    hw/net/virtio-net.c | 17 ++++++-----------
>>>>>    1 file changed, 6 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>> index dd0d056fde..63a8332cd0 100644
>>>>> --- a/hw/net/virtio-net.c
>>>>> +++ b/hw/net/virtio-net.c
>>>>> @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n,
>>>>> uint8_t cmd,
>>>>>            return VIRTIO_NET_ERR;
>>>>>        }
>>>>>    -    /* Avoid changing the number of queue_pairs for vdpa device in
>>>>> -     * userspace handler. A future fix is needed to handle the mq
>>>>> -     * change in userspace handler with vhost-vdpa. Let's disable
>>>>> -     * the mq handling from userspace for now and only allow get
>>>>> -     * done through the kernel. Ripples may be seen when falling
>>>>> -     * back to userspace, but without doing it qemu process would
>>>>> -     * crash on a recursive entry to virtio_net_set_status().
>>>>> -     */
>>>>> +    n->curr_queue_pairs = queue_pairs;
>>>>>        if (nc->peer && nc->peer->info->type ==
>>>>> NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>>> -        return VIRTIO_NET_ERR;
>>>>> +        /*
>>>>> +         * Avoid updating the backend for a vdpa device: We're only
>>>>> interested
>>>>> +         * in updating the device model queues.
>>>>> +         */
>>>>> +        return VIRTIO_NET_OK;
>>>>>        }
>>>>> -
>>>>> -    n->curr_queue_pairs = queue_pairs;
>>>>>        /* stop the backend before changing the number of queue_pairs
>>>>> to avoid handling a
>>>>>         * disabled queue */
>>>>>        virtio_net_set_status(vdev, vdev->status);