Re: [PATCH for 8.0 v7 10/10] vdpa: Always start CVQ in SVQ mode if possible

[Eugenio Perez Martin]

On Thu, Nov 17, 2022 at 7:52 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/11/16 23:05, Eugenio Pérez 写道:
> > Isolate control virtqueue in its own group, allowing to intercept control
> > commands but letting dataplane run totally passthrough to the guest.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> > v7:
> > * Never ask for number of address spaces, just react if isolation is not
> >    possible.
> > * Return ASID ioctl errors instead of masking them as if the device has
> >    no asid.
> > * Simplify net_init_vhost_vdpa logic
> > * Add "if possible" suffix
> >
> > v6:
> > * Disable control SVQ if the device does not support it because of
> > features.
> >
> > v5:
> > * Fixing the not adding cvq buffers when x-svq=on is specified.
> > * Move vring state in vhost_vdpa_get_vring_group instead of using a
> >    parameter.
> > * Rename VHOST_VDPA_NET_CVQ_PASSTHROUGH to VHOST_VDPA_NET_DATA_ASID
> >
> > v4:
> > * Squash vhost_vdpa_cvq_group_is_independent.
> > * Rebased on last CVQ start series, that allocated CVQ cmd bufs at load
> > * Do not check for cvq index on vhost_vdpa_net_prepare, we only have one
> >    that callback registered in that NetClientInfo.
> >
> > v3:
> > * Make asid related queries print a warning instead of returning an
> >    error and stop the start of qemu.
> > ---
> >   hw/virtio/vhost-vdpa.c |   3 +-
> >   net/vhost-vdpa.c       | 117 +++++++++++++++++++++++++++++++++++++++--
> >   2 files changed, 114 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 852baf8b2c..a29a18a6a9 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -653,7 +653,8 @@ static int vhost_vdpa_set_backend_cap(struct vhost_dev 
> > *dev)
> >   {
> >       uint64_t features;
> >       uint64_t f = 0x1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2 |
> > -        0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH;
> > +        0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH |
> > +        0x1ULL << VHOST_BACKEND_F_IOTLB_ASID;
> >       int r;
> >
> >       if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, &features)) {
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index a9c864741a..dc13a49311 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -101,6 +101,8 @@ static const uint64_t vdpa_svq_device_features =
> >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> >
> > +#define VHOST_VDPA_NET_CVQ_ASID 1
> > +
> >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> >   {
> >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > @@ -242,6 +244,40 @@ static NetClientInfo net_vhost_vdpa_info = {
> >           .check_peer_type = vhost_vdpa_check_peer_type,
> >   };
> >
> > +static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index)
> > +{
> > +    struct vhost_vring_state state = {
> > +        .index = vq_index,
> > +    };
> > +    int r = ioctl(device_fd, VHOST_VDPA_GET_VRING_GROUP, &state);
> > +
> > +    if (unlikely(r < 0)) {
> > +        error_report("Cannot get VQ %u group: %s", vq_index,
> > +                     g_strerror(errno));
> > +        return r;
> > +    }
> > +
> > +    return state.num;
> > +}
> > +
> > +static int vhost_vdpa_set_address_space_id(struct vhost_vdpa *v,
> > +                                           unsigned vq_group,
> > +                                           unsigned asid_num)
> > +{
> > +    struct vhost_vring_state asid = {
> > +        .index = vq_group,
> > +        .num = asid_num,
> > +    };
> > +    int r;
> > +
> > +    r = ioctl(v->device_fd, VHOST_VDPA_SET_GROUP_ASID, &asid);
> > +    if (unlikely(r < 0)) {
> > +        error_report("Can't set vq group %u asid %u, errno=%d (%s)",
> > +                     asid.index, asid.num, errno, g_strerror(errno));
> > +    }
> > +    return r;
> > +}
> > +
> >   static void vhost_vdpa_cvq_unmap_buf(struct vhost_vdpa *v, void *addr)
> >   {
> >       VhostIOVATree *tree = v->iova_tree;
> > @@ -316,11 +352,69 @@ dma_map_err:
> >   static int vhost_vdpa_net_cvq_start(NetClientState *nc)
> >   {
> >       VhostVDPAState *s;
> > -    int r;
> > +    struct vhost_vdpa *v;
> > +    uint64_t backend_features;
> > +    int64_t cvq_group;
> > +    int cvq_index, r;
> >
> >       assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> >
> >       s = DO_UPCAST(VhostVDPAState, nc, nc);
> > +    v = &s->vhost_vdpa;
> > +
> > +    v->shadow_data = s->always_svq;
> > +    v->shadow_vqs_enabled = s->always_svq;
> > +    s->vhost_vdpa.address_space_id = VHOST_VDPA_GUEST_PA_ASID;
> > +
> > +    if (s->always_svq) {
> > +        goto out;
> > +    }
> > +
> > +    /* Backend features are not available in v->dev yet. */
> > +    r = ioctl(v->device_fd, VHOST_GET_BACKEND_FEATURES, &backend_features);
> > +    if (unlikely(r < 0)) {
> > +        error_report("Cannot get vdpa backend_features: %s(%d)",
> > +            g_strerror(errno), errno);
> > +        return -1;
> > +    }
> > +    if (!(backend_features & VHOST_BACKEND_F_IOTLB_ASID) ||
> > +        !vhost_vdpa_net_valid_svq_features(v->dev->features, NULL)) {
>
>
> I think there should be some logic to block migration in this case?
>

Since vhost_vdpa are not shadowed they don't expose _F_LOG, so
migration is blocked that way.

We can override it to make its message a little bit clearer.

>
> > +        return 0;
> > +    }
> > +
> > +    /**
> > +     * Check if all the virtqueues of the virtio device are in a different 
> > vq
> > +     * than the last vq. VQ group of last group passed in cvq_group.
> > +     */
> > +    cvq_index = v->dev->vq_index_end - 1;
> > +    cvq_group = vhost_vdpa_get_vring_group(v->device_fd, cvq_index);
> > +    if (unlikely(cvq_group < 0)) {
> > +        return cvq_group;x
> > +    }
> > +    for (int i = 0; i < cvq_index; ++i) {
> > +        int64_t group = vhost_vdpa_get_vring_group(v->device_fd, i);
> > +
> > +        if (unlikely(group < 0)) {
> > +            return group;
> > +        }
> > +
> > +        if (unlikely(group == cvq_group)) {
> > +            warn_report(
> > +                "CVQ %"PRId64" group is the same as VQ %d one (%"PRId64")",
> > +                cvq_group, i, group);
> > +            return 0;
>
>
> Ditto.
>
>
> > +        }
> > +    }
> > +
> > +    r = vhost_vdpa_set_address_space_id(v, cvq_group, 
> > VHOST_VDPA_NET_CVQ_ASID);
> > +    if (unlikely(r < 0)) {
> > +        return r;
> > +    } else {
> > +        v->shadow_vqs_enabled = true;
> > +        s->vhost_vdpa.address_space_id = VHOST_VDPA_NET_CVQ_ASID;
> > +    }
> > +
> > +out:
> >       if (!s->vhost_vdpa.shadow_vqs_enabled) {
> >           return 0;
> >       }
> > @@ -652,6 +746,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const 
> > char *name,
> >       g_autoptr(VhostIOVATree) iova_tree = NULL;
> >       NetClientState *nc;
> >       int queue_pairs, r, i = 0, has_cvq = 0;
> > +    bool svq_cvq;
> >
> >       assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> >       opts = &netdev->u.vhost_vdpa;
> > @@ -693,12 +788,24 @@ int net_init_vhost_vdpa(const Netdev *netdev, const 
> > char *name,
> >           return queue_pairs;
> >       }
> >
> > -    if (opts->x_svq) {
> > -        struct vhost_vdpa_iova_range iova_range;
> > +    svq_cvq = opts->x_svq || has_cvq;
> > +    if (svq_cvq) {
> > +        Error *warn = NULL;
> >
> > -        if (!vhost_vdpa_net_valid_svq_features(features, errp)) {
> > -            goto err_svq;
> > +        svq_cvq = vhost_vdpa_net_valid_svq_features(features,
> > +                                                   opts->x_svq ? errp : 
> > &warn);
> > +        if (!svq_cvq) {
> > +            if (opts->x_svq) {
> > +                goto err_svq;
> > +            } else {
> > +                warn_reportf_err(warn, "Cannot shadow CVQ: ");
>
>
> This seems suspicious, we reach here we we can't just use svq for cvq.
>

Right, but what is the suspicious part?

>
>
> > +            }
> >           }
>
>
> The above logic is not easy to follow. I guess the root cause is the
> variable name. It looks to me svq_cvq is better to be renamed as "svq"?
>

Yes, we can rename it. I'll send a newer version with the rename.

Thanks!

> Thanks
>
>
> > +    }
> > +
> > +    if (svq_cvq) {
> > +        /* Allocate a common iova tree if there is a possibility of SVQ */
> > +        struct vhost_vdpa_iova_range iova_range;
> >
> >           vhost_vdpa_get_iova_range(vdpa_device_fd, &iova_range);
> >           iova_tree = vhost_iova_tree_new(iova_range.first, 
> > iova_range.last);
>