On Fri, Sep 15, 2023 at 4:53 PM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Tue, 27 Jun 2023 at 12:30, Peter Maydell <peter.mayd...@linaro.org> wrote:
> >
> > On Mon, 26 Jun 2023 at 13:29, Michael S. Tsirkin <m...@redhat.com> wrote:
> > >
> > > From: Eugenio Pérez <epere...@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
>
