Since GUEST_ANNOUNCE is emulated the feature bit could be set without
backend support.  This happens in the vDPA case.

However, backend vDPA parent may not have CVQ support.  This causes an
incoherent feature set, and the driver may refuse to start.  This
happens in virtio-net Linux driver.

This may be solved differently in the future.  Qemu is able to emulate a
CVQ just for guest_announce purposes, helping guest to notify the new
location with vDPA devices that does not support it.  However, this is
left as a TODO as it is way more complex to backport.

Tested with vdpa_net_sim, toggling manually VIRTIO_NET_F_CTRL_VQ in the
driver and migrating it with x-svq=on.

Fixes: 980003debddd ("vdpa: do not handle VIRTIO_NET_F_GUEST_ANNOUNCE in vhost-vdpa")
Reported-by: Dawar, Gautam <gautam.dawar@amd.com>
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/net/virtio-net.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3ae909041a..09d5c7a664 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -820,6 +820,21 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
         features |= (1ULL << VIRTIO_NET_F_MTU);
     }
 
+    /*
+     * Since GUEST_ANNOUNCE is emulated the feature bit could be set without
+     * enabled. This happens in the vDPA case.
+     *
+     * Make sure the feature set is not incoherent, as the driver could refuse
+     * to start.
+     *
+     * TODO: QEMU is able to emulate a CVQ just for guest_announce purposes,
+     * helping guest to notify the new location with vDPA devices that does not
+     * support it.
+     */
+    if (!virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_CTRL_VQ)) {
+        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ANNOUNCE);
+    }
+
     return features;
 }
 
-- 
2.31.1

On Tue, Jan 24, 2023 at 5:32 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> Since GUEST_ANNOUNCE is emulated the feature bit could be set without
> backend support.  This happens in the vDPA case.
>
> However, backend vDPA parent may not have CVQ support.  This causes an
> incoherent feature set, and the driver may refuse to start.  This
> happens in virtio-net Linux driver.
>
> This may be solved differently in the future.  Qemu is able to emulate a
> CVQ just for guest_announce purposes, helping guest to notify the new
> location with vDPA devices that does not support it.  However, this is
> left as a TODO as it is way more complex to backport.
>
> Tested with vdpa_net_sim, toggling manually VIRTIO_NET_F_CTRL_VQ in the
> driver and migrating it with x-svq=on.
>

Friendly ping about this patch, as it fell through the cracks if I'm not wrong.

Thanks!

> Fixes: 980003debddd ("vdpa: do not handle VIRTIO_NET_F_GUEST_ANNOUNCE in vhost-vdpa")
> Reported-by: Dawar, Gautam <gautam.dawar@amd.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/net/virtio-net.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 3ae909041a..09d5c7a664 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -820,6 +820,21 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>          features |= (1ULL << VIRTIO_NET_F_MTU);
>      }
>
> +    /*
> +     * Since GUEST_ANNOUNCE is emulated the feature bit could be set without
> +     * enabled. This happens in the vDPA case.
> +     *
> +     * Make sure the feature set is not incoherent, as the driver could refuse
> +     * to start.
> +     *
> +     * TODO: QEMU is able to emulate a CVQ just for guest_announce purposes,
> +     * helping guest to notify the new location with vDPA devices that does not
> +     * support it.
> +     */
> +    if (!virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_CTRL_VQ)) {
> +        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ANNOUNCE);
> +    }
> +
>      return features;
>  }
>
> --
> 2.31.1
>
>

QE uses the sim_vdpa device to test this patch and add "ctrl_vq=off"
in the qemu command line. Guest can find this device, there are no any
error messages in guest dmesg, and can migrate successfully.

Tested-by: Lei Yang <leiyang@redhat.com>

Eugenio Perez Martin <eperezma@redhat.com> 于2023年2月14日周二 14:53写道：
>
> On Tue, Jan 24, 2023 at 5:32 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > Since GUEST_ANNOUNCE is emulated the feature bit could be set without
> > backend support.  This happens in the vDPA case.
> >
> > However, backend vDPA parent may not have CVQ support.  This causes an
> > incoherent feature set, and the driver may refuse to start.  This
> > happens in virtio-net Linux driver.
> >
> > This may be solved differently in the future.  Qemu is able to emulate a
> > CVQ just for guest_announce purposes, helping guest to notify the new
> > location with vDPA devices that does not support it.  However, this is
> > left as a TODO as it is way more complex to backport.
> >
> > Tested with vdpa_net_sim, toggling manually VIRTIO_NET_F_CTRL_VQ in the
> > driver and migrating it with x-svq=on.
> >
>
> Friendly ping about this patch, as it fell through the cracks if I'm not wrong.
>
> Thanks!
>
> > Fixes: 980003debddd ("vdpa: do not handle VIRTIO_NET_F_GUEST_ANNOUNCE in vhost-vdpa")
> > Reported-by: Dawar, Gautam <gautam.dawar@amd.com>
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/net/virtio-net.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 3ae909041a..09d5c7a664 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -820,6 +820,21 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> >          features |= (1ULL << VIRTIO_NET_F_MTU);
> >      }
> >
> > +    /*
> > +     * Since GUEST_ANNOUNCE is emulated the feature bit could be set without
> > +     * enabled. This happens in the vDPA case.
> > +     *
> > +     * Make sure the feature set is not incoherent, as the driver could refuse
> > +     * to start.
> > +     *
> > +     * TODO: QEMU is able to emulate a CVQ just for guest_announce purposes,
> > +     * helping guest to notify the new location with vDPA devices that does not
> > +     * support it.
> > +     */
> > +    if (!virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_CTRL_VQ)) {
> > +        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ANNOUNCE);
> > +    }
> > +
> >      return features;
> >  }
> >
> > --
> > 2.31.1
> >
> >
>

On Tuesday, 2023-01-24 at 17:11:59 +01, Eugenio Pérez wrote:
> Since GUEST_ANNOUNCE is emulated the feature bit could be set without
> backend support.  This happens in the vDPA case.
>
> However, backend vDPA parent may not have CVQ support.  This causes an
> incoherent feature set, and the driver may refuse to start.  This
> happens in virtio-net Linux driver.

Could you now simplify the tests in virtio_net_announce() and
virtio_net_post_load_device() to look only for the presence of
GUEST_ANNOUNCE, given that you can now presume that it implies CTRL_VQ?

But anyway:

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> This may be solved differently in the future.  Qemu is able to emulate a
> CVQ just for guest_announce purposes, helping guest to notify the new
> location with vDPA devices that does not support it.  However, this is
> left as a TODO as it is way more complex to backport.
>
> Tested with vdpa_net_sim, toggling manually VIRTIO_NET_F_CTRL_VQ in the
> driver and migrating it with x-svq=on.
>
> Fixes: 980003debddd ("vdpa: do not handle VIRTIO_NET_F_GUEST_ANNOUNCE in vhost-vdpa")
> Reported-by: Dawar, Gautam <gautam.dawar@amd.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/net/virtio-net.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 3ae909041a..09d5c7a664 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -820,6 +820,21 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>          features |= (1ULL << VIRTIO_NET_F_MTU);
>      }
>
> +    /*
> +     * Since GUEST_ANNOUNCE is emulated the feature bit could be set without
> +     * enabled. This happens in the vDPA case.
> +     *
> +     * Make sure the feature set is not incoherent, as the driver could refuse
> +     * to start.
> +     *
> +     * TODO: QEMU is able to emulate a CVQ just for guest_announce purposes,
> +     * helping guest to notify the new location with vDPA devices that does not
> +     * support it.
> +     */
> +    if (!virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_CTRL_VQ)) {
> +        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ANNOUNCE);
> +    }
> +
>      return features;
>  }
-- 
Why stay in college? Why go to night school?

On Wed, Jan 25, 2023 at 4:20 PM David Edmondson
<david.edmondson@oracle.com> wrote:
>
> On Tuesday, 2023-01-24 at 17:11:59 +01, Eugenio Pérez wrote:
> > Since GUEST_ANNOUNCE is emulated the feature bit could be set without
> > backend support.  This happens in the vDPA case.
> >
> > However, backend vDPA parent may not have CVQ support.  This causes an
> > incoherent feature set, and the driver may refuse to start.  This
> > happens in virtio-net Linux driver.
>
> Could you now simplify the tests in virtio_net_announce() and
> virtio_net_post_load_device() to look only for the presence of
> GUEST_ANNOUNCE, given that you can now presume that it implies CTRL_VQ?
>

That's a good question. As far as I know qemu emits an error if only
GUEST_ANNOUNCE is given in a purely emulated device.

At this moment vhost-kernel and vhost-vdpa do not handle it, but
vhost-user do. Would it be beneficial to preserve previous behavior
and passthrough the features? I guess not, so I think we could
simplify those functions on top of this series.

> But anyway:
>
> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
>

Thanks for the review!

> > This may be solved differently in the future.  Qemu is able to emulate a
> > CVQ just for guest_announce purposes, helping guest to notify the new
> > location with vDPA devices that does not support it.  However, this is
> > left as a TODO as it is way more complex to backport.
> >
> > Tested with vdpa_net_sim, toggling manually VIRTIO_NET_F_CTRL_VQ in the
> > driver and migrating it with x-svq=on.
> >
> > Fixes: 980003debddd ("vdpa: do not handle VIRTIO_NET_F_GUEST_ANNOUNCE in vhost-vdpa")
> > Reported-by: Dawar, Gautam <gautam.dawar@amd.com>
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/net/virtio-net.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 3ae909041a..09d5c7a664 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -820,6 +820,21 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> >          features |= (1ULL << VIRTIO_NET_F_MTU);
> >      }
> >
> > +    /*
> > +     * Since GUEST_ANNOUNCE is emulated the feature bit could be set without
> > +     * enabled. This happens in the vDPA case.
> > +     *
> > +     * Make sure the feature set is not incoherent, as the driver could refuse
> > +     * to start.
> > +     *
> > +     * TODO: QEMU is able to emulate a CVQ just for guest_announce purposes,
> > +     * helping guest to notify the new location with vDPA devices that does not
> > +     * support it.
> > +     */
> > +    if (!virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_CTRL_VQ)) {
> > +        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ANNOUNCE);
> > +    }
> > +
> >      return features;
> >  }
> --
> Why stay in college? Why go to night school?
>

On 1/25/23 21:22, Eugenio Perez Martin wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Wed, Jan 25, 2023 at 4:20 PM David Edmondson
> <david.edmondson@oracle.com> wrote:
>> On Tuesday, 2023-01-24 at 17:11:59 +01, Eugenio Pérez wrote:
>>> Since GUEST_ANNOUNCE is emulated the feature bit could be set without
>>> backend support.  This happens in the vDPA case.
>>>
>>> However, backend vDPA parent may not have CVQ support.  This causes an
>>> incoherent feature set, and the driver may refuse to start.  This
>>> happens in virtio-net Linux driver.
>> Could you now simplify the tests in virtio_net_announce() and
>> virtio_net_post_load_device() to look only for the presence of
>> GUEST_ANNOUNCE, given that you can now presume that it implies CTRL_VQ?
>>
> That's a good question. As far as I know qemu emits an error if only
> GUEST_ANNOUNCE is given in a purely emulated device.
>
> At this moment vhost-kernel and vhost-vdpa do not handle it, but
> vhost-user do. Would it be beneficial to preserve previous behavior
> and passthrough the features? I guess not, so I think we could
> simplify those functions on top of this series.
>
>> But anyway:
>>
>> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
>>
> Thanks for the review!
>
>>> This may be solved differently in the future.  Qemu is able to emulate a
>>> CVQ just for guest_announce purposes, helping guest to notify the new
>>> location with vDPA devices that does not support it.  However, this is
>>> left as a TODO as it is way more complex to backport.
>>>
>>> Tested with vdpa_net_sim, toggling manually VIRTIO_NET_F_CTRL_VQ in the
>>> driver and migrating it with x-svq=on.
>>>
>>> Fixes: 980003debddd ("vdpa: do not handle VIRTIO_NET_F_GUEST_ANNOUNCE in vhost-vdpa")
>>> Reported-by: Dawar, Gautam <gautam.dawar@amd.com>
>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>> ---
>>>   hw/net/virtio-net.c | 15 +++++++++++++++
>>>   1 file changed, 15 insertions(+)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 3ae909041a..09d5c7a664 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -820,6 +820,21 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>>>           features |= (1ULL << VIRTIO_NET_F_MTU);
>>>       }
>>>
>>> +    /*
>>> +     * Since GUEST_ANNOUNCE is emulated the feature bit could be set without
>>> +     * enabled. This happens in the vDPA case.
>>> +     *
>>> +     * Make sure the feature set is not incoherent, as the driver could refuse
>>> +     * to start.
>>> +     *
>>> +     * TODO: QEMU is able to emulate a CVQ just for guest_announce purposes,
>>> +     * helping guest to notify the new location with vDPA devices that does not
>>> +     * support it.
>>> +     */
>>> +    if (!virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_CTRL_VQ)) {
>>> +        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ANNOUNCE);
>>> +    }
>>> +
>>>       return features;
>>>   }
>> --
>> Why stay in college? Why go to night school?
Reviewed-by: Gautam Dawar <gautam.dawar@amd.com>
Tested-by: Gautam Dawar <gautam.dawar@amd.com>