Re: [PATCH v1 08/10] vhost: implement vhost_dev_start method

[Cindy Lu]

On Thu, Jun 25, 2020 at 10:35 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> On 22/06/2020 17:37, Cindy Lu wrote:
> > use the vhost_dev_start callback to send the status to backend
>
> I agree with Jason, squash this patch with the previous one.
>
will fix this
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  hw/virtio/vhost.c         | 17 +++++++++++++++++
> >  include/hw/virtio/vhost.h |  2 ++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 01ebe12f28..bfd7f9ce1f 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -744,6 +744,7 @@ static void vhost_iommu_region_del(MemoryListener 
> > *listener,
> >      }
> >  }
> >
> > +
> >  static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
> >                                      struct vhost_virtqueue *vq,
> >                                      unsigned idx, bool enable_log)
> > @@ -1661,6 +1662,11 @@ int vhost_dev_start(struct vhost_dev *hdev, 
> > VirtIODevice *vdev)
> >          }
> >      }
> >
> > +    r = vhost_set_start(hdev, true);
>
> Perhaps you can use the same kind of name we have for the queue
> (queue_set_started()) and use something like vhost_dev_set_started()?
>
sure， will fix this
> > +    if (r) {
> > +        goto fail_log;
> > +    }
> > +
> >      if (vhost_dev_has_iommu(hdev)) {
> >          hdev->vhost_ops->vhost_set_iotlb_callback(hdev, true);
> >
> > @@ -1697,6 +1703,8 @@ void vhost_dev_stop(struct vhost_dev *hdev, 
> > VirtIODevice *vdev)
> >      /* should only be called after backend is connected */
> >      assert(hdev->vhost_ops);
> >
> > +    vhost_set_start(hdev, false);
> > +
> >      for (i = 0; i < hdev->nvqs; ++i) {
> >          vhost_virtqueue_stop(hdev,
> >                               vdev,
> > @@ -1722,3 +1730,12 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
> >
> >      return -1;
> >  }
> > +
> > +int vhost_set_start(struct vhost_dev *hdev, bool started)
> > +{
> > +
> > +    if (hdev->vhost_ops->vhost_dev_start) {
> > +        hdev->vhost_ops->vhost_dev_start(hdev, started);
>
> The "return" is missing.
>
> And generally a function that only embeds a call to a hook has the same
> as the hook.
>
> > +    }
> > +    return 0;
> > +}
>
> so something like:
>
>     int vhost_dev_set_started(struct vhost_dev *hdev, bool started)
>     {
>         if (hdev->vhost_ops->dev_set_started) {
>             return hdev->vhost_ops->dev_set_started(hdev, started);
>         }
>         return 0;
>     }
>
>
thanks will fix this
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index 085450c6f8..59ea53f8c2 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -92,6 +92,7 @@ struct vhost_dev {
> >      const VhostDevConfigOps *config_ops;
> >  };
> >
> > +
> >  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> >                     VhostBackendType backend_type,
> >                     uint32_t busyloop_timeout);
> > @@ -137,4 +138,5 @@ int vhost_dev_set_inflight(struct vhost_dev *dev,
> >                             struct vhost_inflight *inflight);
> >  int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
> >                             struct vhost_inflight *inflight);
> > +int vhost_set_start(struct vhost_dev *dev, bool started);
>
> There is no need to export it, so set it "static" in hw/virtio/vhost.c
> and move the definition before the use.
>
thanks will fix this
> Thanks,
> Laurent
>