| Message ID | 20221116150556.1294049-11-eperezma@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ASID support in vhost-vdpa net | expand |
在 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? > + 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. > + } > } 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"? 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);
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); >
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); > >
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)) { + 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; + } + 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; + } + } + + 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: "); + } } + } + + 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);
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(-)