Re: [PATCH v2 04/14] vhost: Add vhost_svq_valid_features to shadow vq

[Jason Wang]

在 2022/2/27 下午9:41, Eugenio Pérez 写道:
> This allows SVQ to negotiate features with the guest and the device. For
> the device, SVQ is a driver. While this function bypasses all
> non-transport features, it needs to disable the features that SVQ does
> not support when forwarding buffers. This includes packed vq layout,
> indirect descriptors or event idx.
>
> Future changes can add support to offer more features to the guest,
> since the use of VirtQueue gives this for free. This is left out at the
> moment for simplicity.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-shadow-virtqueue.h |  2 ++
>   hw/virtio/vhost-shadow-virtqueue.c | 39 ++++++++++++++++++++++++++++++
>   hw/virtio/vhost-vdpa.c             | 18 ++++++++++++++
>   3 files changed, 59 insertions(+)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
> b/hw/virtio/vhost-shadow-virtqueue.h
> index 1d4c160d0a..84747655ad 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.h
> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> @@ -33,6 +33,8 @@ typedef struct VhostShadowVirtqueue {
>       EventNotifier svq_call;
>   } VhostShadowVirtqueue;
>   
> +bool vhost_svq_valid_features(uint64_t *features);
> +
>   void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd);
>   void vhost_svq_set_guest_call_notifier(VhostShadowVirtqueue *svq, int 
> call_fd);
>   
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index 54c701a196..34354aea2c 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -14,6 +14,45 @@
>   #include "qemu/main-loop.h"
>   #include "linux-headers/linux/vhost.h"
>   
> +/**
> + * Validate the transport device features that both guests can use with the SVQ
> + * and SVQs can use with the device.
> + *
> + * @dev_features  The features. If success, the acknowledged features. If
> + *                failure, the minimal set from it.
> + *
> + * Returns true if SVQ can go with a subset of these, false otherwise.
> + */
> +bool vhost_svq_valid_features(uint64_t *features)
> +{
> +    bool r = true;
> +
> +    for (uint64_t b = VIRTIO_TRANSPORT_F_START; b <= VIRTIO_TRANSPORT_F_END;
> +         ++b) {
> +        switch (b) {
> +        case VIRTIO_F_ANY_LAYOUT:
> +            continue;
> +
> +        case VIRTIO_F_ACCESS_PLATFORM:
> +            /* SVQ trust in the host's IOMMU to translate addresses */
> +        case VIRTIO_F_VERSION_1:
> +            /* SVQ trust that the guest vring is little endian */
> +            if (!(*features & BIT_ULL(b))) {
> +                set_bit(b, features);
> +                r = false;
> +            }
> +            continue;


It looks to me the *features is only used for logging errors, if this is 
the truth. I suggest to do error_setg in this function instead of 
letting the caller to pass a pointer.


> +
> +        default:
> +            if (*features & BIT_ULL(b)) {
> +                clear_bit(b, features);
> +            }


Do we need to check indirect and event idx here?

Thanks


> +        }
> +    }
> +
> +    return r;
> +}
> +
>   /** Forward guest notifications */
>   static void vhost_handle_guest_kick(EventNotifier *n)
>   {
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index c73215751d..d614c435f3 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -348,11 +348,29 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, 
> struct vhost_vdpa *v,
>                                  Error **errp)
>   {
>       g_autoptr(GPtrArray) shadow_vqs = NULL;
> +    uint64_t dev_features, svq_features;
> +    int r;
> +    bool ok;
>   
>       if (!v->shadow_vqs_enabled) {
>           return 0;
>       }
>   
> +    r = hdev->vhost_ops->vhost_get_features(hdev, &dev_features);
> +    if (r != 0) {
> +        error_setg_errno(errp, -r, "Can't get vdpa device features");
> +        return r;
> +    }
> +
> +    svq_features = dev_features;
> +    ok = vhost_svq_valid_features(&svq_features);
> +    if (unlikely(!ok)) {
> +        error_setg(errp,
> +            "SVQ Invalid device feature flags, offer: 0x%"PRIx64", ok: 
> 0x%"PRIx64,
> +            dev_features, svq_features);
> +        return -1;
> +    }
> +
>       shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
>       for (unsigned n = 0; n < hdev->nvqs; ++n) {
>           g_autoptr(VhostShadowVirtqueue) svq = vhost_svq_new();