Re: [PATCH] virtio-net: clear guest_announce feature if no cvq backend

[David Edmondson <david.edmondson@oracle.com>]

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?