| Message ID | 20200622153756.19189-9-lulu@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vDPA support in qemu | expand |
On 2020/6/22 下午11:37, Cindy Lu wrote: > use the vhost_dev_start callback to send the status to backend I suggest to squash this into previous patch. > > 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); I think we need a better name for this function. > + 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); > + } > + return 0; > +} > 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); Any reason for exporting this? It looks to me there's no real user out this file. Thanks > #endif
On Tue, Jun 23, 2020 at 3:21 PM Jason Wang <jasowang@redhat.com> wrote: > > > On 2020/6/22 下午11:37, Cindy Lu wrote: > > use the vhost_dev_start callback to send the status to backend > > > I suggest to squash this into previous patch. > Sure will do > > > > > 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); > > > I think we need a better name for this function. > Shall we change it to vhost_set_device_start ? Since the vhost_dev_start was occupied by other function > > > + 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); > > + } > > + return 0; > > +} > > 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); > > > Any reason for exporting this? It looks to me there's no real user out > this file. > > Thanks > Sure will fix this > > > #endif >
On 2020/6/23 下午5:34, Cindy Lu wrote: > On Tue, Jun 23, 2020 at 3:21 PM Jason Wang<jasowang@redhat.com> wrote: >> On 2020/6/22 下午11:37, Cindy Lu wrote: >>> use the vhost_dev_start callback to send the status to backend >> I suggest to squash this into previous patch. >> > Sure will do >>> 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); >> I think we need a better name for this function. >> > Shall we change it to vhost_set_device_start ? Since the > vhost_dev_start was occupied by other function Or maybe just open code the vhost_set_start here since there's no other users. Thanks
On Tue, Jun 23, 2020 at 5:38 PM Jason Wang <jasowang@redhat.com> wrote: > > > On 2020/6/23 下午5:34, Cindy Lu wrote: > > On Tue, Jun 23, 2020 at 3:21 PM Jason Wang<jasowang@redhat.com> wrote: > >> On 2020/6/22 下午11:37, Cindy Lu wrote: > >>> use the vhost_dev_start callback to send the status to backend > >> I suggest to squash this into previous patch. > >> > > Sure will do > >>> 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); > >> I think we need a better name for this function. > >> > > Shall we change it to vhost_set_device_start ? Since the > > vhost_dev_start was occupied by other function > > > Or maybe just open code the vhost_set_start here since there's no other > users. > > Thanks > Sure will do >
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. > 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()? > + 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; } > 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, Laurent
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 >
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); + 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); + } + return 0; +} 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); #endif
use the vhost_dev_start callback to send the status to backend Signed-off-by: Cindy Lu <lulu@redhat.com> --- hw/virtio/vhost.c | 17 +++++++++++++++++ include/hw/virtio/vhost.h | 2 ++ 2 files changed, 19 insertions(+)