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 8:43 AM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> 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.
>

Another possibility is to get rid of the variable and allocate the
iova_tree unconditionally. We could send the log in
vhost_vdpa_net_cvq_start, or make that message the vhost migration
blocker.

Thanks!

> 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);
> >