Message ID | 152128d646973ed298d41dafd7a5bccff43336c8.1687782442.git.mst@redhat.com |
---|---|
State | New |
Headers | show |
Series | [PULL,01/53] bswap: Add the ability to store to an unaligned 24 bit field | expand |
On Mon, 26 Jun 2023 at 13:29, Michael S. Tsirkin <mst@redhat.com> wrote: > > From: Eugenio Pérez <eperezma@redhat.com> > > Evaluating it at start time instead of initialization time may make the > guest capable of dynamically adding or removing migration blockers. > > Also, moving to initialization reduces the number of ioctls in the > migration, reducing failure possibilities. > > As a drawback we need to check for CVQ isolation twice: one time with no > MQ negotiated and another one acking it, as long as the device supports > it. This is because Vring ASID / group management is based on vq > indexes, but we don't know the index of CVQ before negotiating MQ. I was looking at this code because of a Coverity report. That turned out to be a false positive, but I did notice something here that looks like it might be wrong: > > +/** > + * Probe if CVQ is isolated > + * > + * @device_fd The vdpa device fd > + * @features Features offered by the device. > + * @cvq_index The control vq pair index > + * > + * Returns <0 in case of failure, 0 if false and 1 if true. > + */ > +static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features, > + int cvq_index, Error **errp) > +{ > + uint64_t backend_features; > + int64_t cvq_group; > + uint8_t status = VIRTIO_CONFIG_S_ACKNOWLEDGE | > + VIRTIO_CONFIG_S_DRIVER | > + VIRTIO_CONFIG_S_FEATURES_OK; > + int r; > + > + ERRP_GUARD(); > + > + r = ioctl(device_fd, VHOST_GET_BACKEND_FEATURES, &backend_features); > + if (unlikely(r < 0)) { > + error_setg_errno(errp, errno, "Cannot get vdpa backend_features"); > + return r; > + } > + > + if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_IOTLB_ASID))) { > + return 0; > + } > + > + r = ioctl(device_fd, VHOST_SET_FEATURES, &features); > + if (unlikely(r)) { > + error_setg_errno(errp, errno, "Cannot set features"); Shouldn't we have a 'return r' (or maybe a 'goto out') here ? Otherwise we'll just plough onward and attempt to continue execution... > + } > + > + r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status); > + if (unlikely(r)) { > + error_setg_errno(errp, -r, "Cannot set device features"); Isn't this trying to set device status, not features ? > + goto out; > + } thanks -- PMM
On Tue, 27 Jun 2023 at 12:30, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 26 Jun 2023 at 13:29, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > From: Eugenio Pérez <eperezma@redhat.com> > > > > Evaluating it at start time instead of initialization time may make the > > guest capable of dynamically adding or removing migration blockers. > > > > Also, moving to initialization reduces the number of ioctls in the > > migration, reducing failure possibilities. > > > > As a drawback we need to check for CVQ isolation twice: one time with no > > MQ negotiated and another one acking it, as long as the device supports > > it. This is because Vring ASID / group management is based on vq > > indexes, but we don't know the index of CVQ before negotiating MQ. > > I was looking at this code because of a Coverity report. > That turned out to be a false positive, but I did notice > something here that looks like it might be wrong: Ping? Would somebody like to have a look at whether there's a missing return statement here? > > > > +/** > > + * Probe if CVQ is isolated > > + * > > + * @device_fd The vdpa device fd > > + * @features Features offered by the device. > > + * @cvq_index The control vq pair index > > + * > > + * Returns <0 in case of failure, 0 if false and 1 if true. > > + */ > > +static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features, > > + int cvq_index, Error **errp) > > +{ > > + uint64_t backend_features; > > + int64_t cvq_group; > > + uint8_t status = VIRTIO_CONFIG_S_ACKNOWLEDGE | > > + VIRTIO_CONFIG_S_DRIVER | > > + VIRTIO_CONFIG_S_FEATURES_OK; > > + int r; > > + > > + ERRP_GUARD(); > > + > > + r = ioctl(device_fd, VHOST_GET_BACKEND_FEATURES, &backend_features); > > + if (unlikely(r < 0)) { > > + error_setg_errno(errp, errno, "Cannot get vdpa backend_features"); > > + return r; > > + } > > + > > + if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_IOTLB_ASID))) { > > + return 0; > > + } > > + > > + r = ioctl(device_fd, VHOST_SET_FEATURES, &features); > > + if (unlikely(r)) { > > + error_setg_errno(errp, errno, "Cannot set features"); > > Shouldn't we have a 'return r' (or maybe a 'goto out') here ? > Otherwise we'll just plough onward and attempt to continue > execution... > > > + } > > + > > + r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status); > > + if (unlikely(r)) { > > + error_setg_errno(errp, -r, "Cannot set device features"); > > Isn't this trying to set device status, not features ? > > > + goto out; > > + } > > thanks > -- PMM thanks -- PMM
On Fri, Sep 15, 2023 at 4:53 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 27 Jun 2023 at 12:30, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Mon, 26 Jun 2023 at 13:29, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > From: Eugenio Pérez <eperezma@redhat.com> > > > > > > Evaluating it at start time instead of initialization time may make the > > > guest capable of dynamically adding or removing migration blockers. > > > > > > Also, moving to initialization reduces the number of ioctls in the > > > migration, reducing failure possibilities. > > > > > > As a drawback we need to check for CVQ isolation twice: one time with no > > > MQ negotiated and another one acking it, as long as the device supports > > > it. This is because Vring ASID / group management is based on vq > > > indexes, but we don't know the index of CVQ before negotiating MQ. > > > > I was looking at this code because of a Coverity report. > > That turned out to be a false positive, but I did notice > > something here that looks like it might be wrong: > > Ping? Would somebody like to have a look at whether there's > a missing return statement here? > Hi Peter, I'm sorry, it fell through the cracks. I'll send two patches to fix it right now. Thanks! > > > > > > +/** > > > + * Probe if CVQ is isolated > > > + * > > > + * @device_fd The vdpa device fd > > > + * @features Features offered by the device. > > > + * @cvq_index The control vq pair index > > > + * > > > + * Returns <0 in case of failure, 0 if false and 1 if true. > > > + */ > > > +static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features, > > > + int cvq_index, Error **errp) > > > +{ > > > + uint64_t backend_features; > > > + int64_t cvq_group; > > > + uint8_t status = VIRTIO_CONFIG_S_ACKNOWLEDGE | > > > + VIRTIO_CONFIG_S_DRIVER | > > > + VIRTIO_CONFIG_S_FEATURES_OK; > > > + int r; > > > + > > > + ERRP_GUARD(); > > > + > > > + r = ioctl(device_fd, VHOST_GET_BACKEND_FEATURES, &backend_features); > > > + if (unlikely(r < 0)) { > > > + error_setg_errno(errp, errno, "Cannot get vdpa backend_features"); > > > + return r; > > > + } > > > + > > > + if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_IOTLB_ASID))) { > > > + return 0; > > > + } > > > + > > > + r = ioctl(device_fd, VHOST_SET_FEATURES, &features); > > > + if (unlikely(r)) { > > > + error_setg_errno(errp, errno, "Cannot set features"); > > > > Shouldn't we have a 'return r' (or maybe a 'goto out') here ? > > Otherwise we'll just plough onward and attempt to continue > > execution... > > > > > + } > > > + > > > + r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status); > > > + if (unlikely(r)) { > > > + error_setg_errno(errp, -r, "Cannot set device features"); > > > > Isn't this trying to set device status, not features ? > > > > > + goto out; > > > + } > > > > thanks > > -- PMM > > thanks > -- PMM >
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 3fb833fe76..46778d5313 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -43,6 +43,10 @@ typedef struct VhostVDPAState { /* The device always have SVQ enabled */ bool always_svq; + + /* The device can isolate CVQ in its own ASID */ + bool cvq_isolated; + bool started; } VhostVDPAState; @@ -362,15 +366,8 @@ static NetClientInfo net_vhost_vdpa_info = { .check_peer_type = vhost_vdpa_check_peer_type, }; -/** - * Get vring virtqueue group - * - * @device_fd vdpa device fd - * @vq_index Virtqueue index - * - * Return -errno in case of error, or vq group if success. - */ -static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index) +static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index, + Error **errp) { struct vhost_vring_state state = { .index = vq_index, @@ -379,8 +376,7 @@ static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index) if (unlikely(r < 0)) { r = -errno; - error_report("Cannot get VQ %u group: %s", vq_index, - g_strerror(errno)); + error_setg_errno(errp, errno, "Cannot get VQ %u group", vq_index); return r; } @@ -480,9 +476,9 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc) { VhostVDPAState *s, *s0; struct vhost_vdpa *v; - uint64_t backend_features; int64_t cvq_group; - int cvq_index, r; + int r; + Error *err = NULL; assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA); @@ -502,40 +498,21 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc) /* * If we early return in these cases SVQ will not be enabled. The migration * will be blocked as long as vhost-vdpa backends will not offer _F_LOG. - * - * Calling VHOST_GET_BACKEND_FEATURES as they 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 & BIT_ULL(VHOST_BACKEND_F_IOTLB_ASID)) || - !vhost_vdpa_net_valid_svq_features(v->dev->features, NULL)) { + if (!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; + if (!s->cvq_isolated) { + return 0; } - 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 (group == cvq_group) { - return 0; - } + cvq_group = vhost_vdpa_get_vring_group(v->device_fd, + v->dev->vq_index_end - 1, + &err); + if (unlikely(cvq_group < 0)) { + error_report_err(err); + return cvq_group; } r = vhost_vdpa_set_address_space_id(v, cvq_group, VHOST_VDPA_NET_CVQ_ASID); @@ -799,6 +776,87 @@ static const VhostShadowVirtqueueOps vhost_vdpa_net_svq_ops = { .avail_handler = vhost_vdpa_net_handle_ctrl_avail, }; +/** + * Probe if CVQ is isolated + * + * @device_fd The vdpa device fd + * @features Features offered by the device. + * @cvq_index The control vq pair index + * + * Returns <0 in case of failure, 0 if false and 1 if true. + */ +static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features, + int cvq_index, Error **errp) +{ + uint64_t backend_features; + int64_t cvq_group; + uint8_t status = VIRTIO_CONFIG_S_ACKNOWLEDGE | + VIRTIO_CONFIG_S_DRIVER | + VIRTIO_CONFIG_S_FEATURES_OK; + int r; + + ERRP_GUARD(); + + r = ioctl(device_fd, VHOST_GET_BACKEND_FEATURES, &backend_features); + if (unlikely(r < 0)) { + error_setg_errno(errp, errno, "Cannot get vdpa backend_features"); + return r; + } + + if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_IOTLB_ASID))) { + return 0; + } + + r = ioctl(device_fd, VHOST_SET_FEATURES, &features); + if (unlikely(r)) { + error_setg_errno(errp, errno, "Cannot set features"); + } + + r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status); + if (unlikely(r)) { + error_setg_errno(errp, -r, "Cannot set device features"); + goto out; + } + + cvq_group = vhost_vdpa_get_vring_group(device_fd, cvq_index, errp); + if (unlikely(cvq_group < 0)) { + if (cvq_group != -ENOTSUP) { + r = cvq_group; + goto out; + } + + /* + * The kernel report VHOST_BACKEND_F_IOTLB_ASID if the vdpa frontend + * support ASID even if the parent driver does not. The CVQ cannot be + * isolated in this case. + */ + error_free(*errp); + *errp = NULL; + r = 0; + goto out; + } + + for (int i = 0; i < cvq_index; ++i) { + int64_t group = vhost_vdpa_get_vring_group(device_fd, i, errp); + if (unlikely(group < 0)) { + r = group; + goto out; + } + + if (group == (int64_t)cvq_group) { + r = 0; + goto out; + } + } + + r = 1; + +out: + status = 0; + ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status); + return r; +} + static NetClientState *net_vhost_vdpa_init(NetClientState *peer, const char *device, const char *name, @@ -808,16 +866,26 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, bool is_datapath, bool svq, struct vhost_vdpa_iova_range iova_range, - uint64_t features) + uint64_t features, + Error **errp) { NetClientState *nc = NULL; VhostVDPAState *s; int ret = 0; assert(name); + int cvq_isolated; + if (is_datapath) { nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name); } else { + cvq_isolated = vhost_vdpa_probe_cvq_isolation(vdpa_device_fd, features, + queue_pair_index * 2, + errp); + if (unlikely(cvq_isolated < 0)) { + return NULL; + } + nc = qemu_new_net_control_client(&net_vhost_vdpa_cvq_info, peer, device, name); } @@ -844,6 +912,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, s->vhost_vdpa.shadow_vq_ops = &vhost_vdpa_net_svq_ops; s->vhost_vdpa.shadow_vq_ops_opaque = s; + s->cvq_isolated = cvq_isolated; /* * TODO: We cannot migrate devices with CVQ as there is no way to set @@ -972,7 +1041,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name, for (i = 0; i < queue_pairs; i++) { ncs[i] = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, vdpa_device_fd, i, 2, true, opts->x_svq, - iova_range, features); + iova_range, features, errp); if (!ncs[i]) goto err; } @@ -980,7 +1049,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name, if (has_cvq) { nc = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, vdpa_device_fd, i, 1, false, - opts->x_svq, iova_range, features); + opts->x_svq, iova_range, features, errp); if (!nc) goto err; }