On Thu, Nov 17, 2022 at 7:52 AM Jason Wang <jasowang@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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);
>
