Add call back function for configure interrupt.
Set the notifier's fd to the kernel driver when vdpa start.
also set -1 while vdpa stop. then the kernel will release
the related resource

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 hw/virtio/trace-events            |  2 ++
 hw/virtio/vhost-vdpa.c            | 37 ++++++++++++++++++++++++++++++-
 include/hw/virtio/vhost-backend.h |  4 ++++
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 2060a144a2..6710835b46 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -52,6 +52,8 @@ vhost_vdpa_set_vring_call(void *dev, unsigned int index, int fd) "dev: %p index:
 vhost_vdpa_get_features(void *dev, uint64_t features) "dev: %p features: 0x%"PRIx64
 vhost_vdpa_set_owner(void *dev) "dev: %p"
 vhost_vdpa_vq_get_addr(void *dev, void *vq, uint64_t desc_user_addr, uint64_t avail_user_addr, uint64_t used_user_addr) "dev: %p vq: %p desc_user_addr: 0x%"PRIx64" avail_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64
+vhost_vdpa_set_config_call(void *dev, int *fd)"dev: %p fd: %p"
+
 
 # virtio.c
 virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u"
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 01d2101d09..cc1d39d663 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -467,20 +467,47 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
     }
     return ret;
  }
+static void vhost_vdpa_config_notify_start(struct vhost_dev *dev,
+                                struct VirtIODevice *vdev, bool start)
+{
+    int fd, r;
+    if (start) {
+        fd = event_notifier_get_fd(&vdev->config_notifier);
+        vdev->use_config_notifier = true;
+     } else {
+        fd = -1;
+        vdev->use_config_notifier = false;
+     }
+     /*set the fd call back to vdpa driver*/
+    r = dev->vhost_ops->vhost_set_config_call(dev, &fd);
+    if (r) {
+        vdev->use_config_notifier = false;
+        info_report("vhost_vdpa_config_notify not started!");
+    }
+    /*active the config_notifier when vdev->use_config_notifier is true*/
+    if ((vdev->use_config_notifier) && (start)) {
+        event_notifier_set(&vdev->config_notifier);
+    }
+    return;
 
+}
 static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
 {
     struct vhost_vdpa *v = dev->opaque;
     trace_vhost_vdpa_dev_start(dev, started);
+    VirtIODevice *vdev = dev->vdev;
+
     if (started) {
         uint8_t status = 0;
         memory_listener_register(&v->listener, &address_space_memory);
         vhost_vdpa_set_vring_ready(dev);
         vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
         vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &status);
-
+        /*set the configure interrupt call back*/
+        vhost_vdpa_config_notify_start(dev, vdev, true);
         return !(status & VIRTIO_CONFIG_S_DRIVER_OK);
     } else {
+        vhost_vdpa_config_notify_start(dev, vdev, false);
         vhost_vdpa_reset_device(dev);
         vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
                                    VIRTIO_CONFIG_S_DRIVER);
@@ -546,6 +573,13 @@ static int vhost_vdpa_set_vring_call(struct vhost_dev *dev,
     return vhost_vdpa_call(dev, VHOST_SET_VRING_CALL, file);
 }
 
+static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
+                                       int *fd)
+{
+    trace_vhost_vdpa_set_config_call(dev, fd);
+    return vhost_vdpa_call(dev, VHOST_VDPA_SET_CONFIG_CALL, fd);
+}
+
 static int vhost_vdpa_get_features(struct vhost_dev *dev,
                                      uint64_t *features)
 {
@@ -611,4 +645,5 @@ const VhostOps vdpa_ops = {
         .vhost_get_device_id = vhost_vdpa_get_device_id,
         .vhost_vq_get_addr = vhost_vdpa_vq_get_addr,
         .vhost_force_iommu = vhost_vdpa_force_iommu,
+        .vhost_set_config_call = vhost_vdpa_set_config_call,
 };
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index 8a6f8e2a7a..1a2fee8994 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -125,6 +125,9 @@ typedef int (*vhost_get_device_id_op)(struct vhost_dev *dev, uint32_t *dev_id);
 
 typedef bool (*vhost_force_iommu_op)(struct vhost_dev *dev);
 
+typedef int (*vhost_set_config_call_op)(struct vhost_dev *dev,
+                                       int *fd);
+
 typedef struct VhostOps {
     VhostBackendType backend_type;
     vhost_backend_init vhost_backend_init;
@@ -170,6 +173,7 @@ typedef struct VhostOps {
     vhost_vq_get_addr_op  vhost_vq_get_addr;
     vhost_get_device_id_op vhost_get_device_id;
     vhost_force_iommu_op vhost_force_iommu;
+    vhost_set_config_call_op vhost_set_config_call;
 } VhostOps;
 
 extern const VhostOps user_ops;
-- 
2.21.3

On Tue, Jan 26, 2021 at 03:42:54PM +0800, Cindy Lu wrote:
>Add call back function for configure interrupt.
>Set the notifier's fd to the kernel driver when vdpa start.
>also set -1 while vdpa stop. then the kernel will release
>the related resource
>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> hw/virtio/trace-events            |  2 ++
> hw/virtio/vhost-vdpa.c            | 37 ++++++++++++++++++++++++++++++-
> include/hw/virtio/vhost-backend.h |  4 ++++
> 3 files changed, 42 insertions(+), 1 deletion(-)
>
>diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
>index 2060a144a2..6710835b46 100644
>--- a/hw/virtio/trace-events
>+++ b/hw/virtio/trace-events
>@@ -52,6 +52,8 @@ vhost_vdpa_set_vring_call(void *dev, unsigned int index, int fd) "dev: %p index:
> vhost_vdpa_get_features(void *dev, uint64_t features) "dev: %p features: 0x%"PRIx64
> vhost_vdpa_set_owner(void *dev) "dev: %p"
> vhost_vdpa_vq_get_addr(void *dev, void *vq, uint64_t desc_user_addr, uint64_t avail_user_addr, uint64_t used_user_addr) "dev: %p vq: %p desc_user_addr: 0x%"PRIx64" avail_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64
>+vhost_vdpa_set_config_call(void *dev, int *fd)"dev: %p fd: %p"
>+
>
> # virtio.c
> virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u"
>diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>index 01d2101d09..cc1d39d663 100644
>--- a/hw/virtio/vhost-vdpa.c
>+++ b/hw/virtio/vhost-vdpa.c
>@@ -467,20 +467,47 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
>     }
>     return ret;
>  }
>+static void vhost_vdpa_config_notify_start(struct vhost_dev *dev,
>+                                struct VirtIODevice *vdev, bool start)
>+{
>+    int fd, r;
>+    if (start) {
>+        fd = event_notifier_get_fd(&vdev->config_notifier);
>+        vdev->use_config_notifier = true;
>+     } else {
       ^
>+        fd = -1;
>+        vdev->use_config_notifier = false;
>+     }
       ^
>+     /*set the fd call back to vdpa driver*/
       ^
       It seems to me that there is an extra space in these places.

>+    r = dev->vhost_ops->vhost_set_config_call(dev, &fd);
>+    if (r) {
>+        vdev->use_config_notifier = false;
>+        info_report("vhost_vdpa_config_notify not started!");
>+    }
>+    /*active the config_notifier when vdev->use_config_notifier is true*/
>+    if ((vdev->use_config_notifier) && (start)) {
>+        event_notifier_set(&vdev->config_notifier);
>+    }
>+    return;
>
>+}

Here and also in other patches I would leave a new line between two 
functions.

> static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> {
>     struct vhost_vdpa *v = dev->opaque;
>     trace_vhost_vdpa_dev_start(dev, started);
>+    VirtIODevice *vdev = dev->vdev;
>+
>     if (started) {
>         uint8_t status = 0;
>         memory_listener_register(&v->listener, &address_space_memory);
>         vhost_vdpa_set_vring_ready(dev);
>         vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>         vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &status);
>-
>+        /*set the configure interrupt call back*/
>+        vhost_vdpa_config_notify_start(dev, vdev, true);
>         return !(status & VIRTIO_CONFIG_S_DRIVER_OK);
>     } else {
>+        vhost_vdpa_config_notify_start(dev, vdev, false);
>         vhost_vdpa_reset_device(dev);
>         vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>                                    VIRTIO_CONFIG_S_DRIVER);
>@@ -546,6 +573,13 @@ static int vhost_vdpa_set_vring_call(struct vhost_dev *dev,
>     return vhost_vdpa_call(dev, VHOST_SET_VRING_CALL, file);
> }
>
>+static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
>+                                       int *fd)
>+{
>+    trace_vhost_vdpa_set_config_call(dev, fd);
>+    return vhost_vdpa_call(dev, VHOST_VDPA_SET_CONFIG_CALL, fd);
>+}
>+
> static int vhost_vdpa_get_features(struct vhost_dev *dev,
>                                      uint64_t *features)
> {
>@@ -611,4 +645,5 @@ const VhostOps vdpa_ops = {
>         .vhost_get_device_id = vhost_vdpa_get_device_id,
>         .vhost_vq_get_addr = vhost_vdpa_vq_get_addr,
>         .vhost_force_iommu = vhost_vdpa_force_iommu,
>+        .vhost_set_config_call = vhost_vdpa_set_config_call,
> };
>diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
>index 8a6f8e2a7a..1a2fee8994 100644
>--- a/include/hw/virtio/vhost-backend.h
>+++ b/include/hw/virtio/vhost-backend.h
>@@ -125,6 +125,9 @@ typedef int (*vhost_get_device_id_op)(struct vhost_dev *dev, uint32_t *dev_id);
>
> typedef bool (*vhost_force_iommu_op)(struct vhost_dev *dev);
>
>+typedef int (*vhost_set_config_call_op)(struct vhost_dev *dev,
>+                                       int *fd);
                                          ^
                                          One more space :-)

Thanks,
Stefano

On Tue, Jan 26, 2021 at 10:53 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Tue, Jan 26, 2021 at 03:42:54PM +0800, Cindy Lu wrote:
> >Add call back function for configure interrupt.
> >Set the notifier's fd to the kernel driver when vdpa start.
> >also set -1 while vdpa stop. then the kernel will release
> >the related resource
> >
> >Signed-off-by: Cindy Lu <lulu@redhat.com>
> >---
> > hw/virtio/trace-events            |  2 ++
> > hw/virtio/vhost-vdpa.c            | 37 ++++++++++++++++++++++++++++++-
> > include/hw/virtio/vhost-backend.h |  4 ++++
> > 3 files changed, 42 insertions(+), 1 deletion(-)
> >
> >diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> >index 2060a144a2..6710835b46 100644
> >--- a/hw/virtio/trace-events
> >+++ b/hw/virtio/trace-events
> >@@ -52,6 +52,8 @@ vhost_vdpa_set_vring_call(void *dev, unsigned int index, int fd) "dev: %p index:
> > vhost_vdpa_get_features(void *dev, uint64_t features) "dev: %p features: 0x%"PRIx64
> > vhost_vdpa_set_owner(void *dev) "dev: %p"
> > vhost_vdpa_vq_get_addr(void *dev, void *vq, uint64_t desc_user_addr, uint64_t avail_user_addr, uint64_t used_user_addr) "dev: %p vq: %p desc_user_addr: 0x%"PRIx64" avail_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64
> >+vhost_vdpa_set_config_call(void *dev, int *fd)"dev: %p fd: %p"
> >+
> >
> > # virtio.c
> > virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u"
> >diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >index 01d2101d09..cc1d39d663 100644
> >--- a/hw/virtio/vhost-vdpa.c
> >+++ b/hw/virtio/vhost-vdpa.c
> >@@ -467,20 +467,47 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
> >     }
> >     return ret;
> >  }
> >+static void vhost_vdpa_config_notify_start(struct vhost_dev *dev,
> >+                                struct VirtIODevice *vdev, bool start)
> >+{
> >+    int fd, r;
> >+    if (start) {
> >+        fd = event_notifier_get_fd(&vdev->config_notifier);
> >+        vdev->use_config_notifier = true;
> >+     } else {
>        ^
> >+        fd = -1;
> >+        vdev->use_config_notifier = false;
> >+     }
>        ^
> >+     /*set the fd call back to vdpa driver*/
>        ^
>        It seems to me that there is an extra space in these places.
>
sure, I will fix this
> >+    r = dev->vhost_ops->vhost_set_config_call(dev, &fd);
> >+    if (r) {
> >+        vdev->use_config_notifier = false;
> >+        info_report("vhost_vdpa_config_notify not started!");
> >+    }
> >+    /*active the config_notifier when vdev->use_config_notifier is true*/
> >+    if ((vdev->use_config_notifier) && (start)) {
> >+        event_notifier_set(&vdev->config_notifier);
> >+    }
> >+    return;
> >
> >+}
>
> Here and also in other patches I would leave a new line between two
> functions.
>
sure. will fix this
> > static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> > {
> >     struct vhost_vdpa *v = dev->opaque;
> >     trace_vhost_vdpa_dev_start(dev, started);
> >+    VirtIODevice *vdev = dev->vdev;
> >+
> >     if (started) {
> >         uint8_t status = 0;
> >         memory_listener_register(&v->listener, &address_space_memory);
> >         vhost_vdpa_set_vring_ready(dev);
> >         vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> >         vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &status);
> >-
> >+        /*set the configure interrupt call back*/
> >+        vhost_vdpa_config_notify_start(dev, vdev, true);
> >         return !(status & VIRTIO_CONFIG_S_DRIVER_OK);
> >     } else {
> >+        vhost_vdpa_config_notify_start(dev, vdev, false);
> >         vhost_vdpa_reset_device(dev);
> >         vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >                                    VIRTIO_CONFIG_S_DRIVER);
> >@@ -546,6 +573,13 @@ static int vhost_vdpa_set_vring_call(struct vhost_dev *dev,
> >     return vhost_vdpa_call(dev, VHOST_SET_VRING_CALL, file);
> > }
> >
> >+static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
> >+                                       int *fd)
> >+{
> >+    trace_vhost_vdpa_set_config_call(dev, fd);
> >+    return vhost_vdpa_call(dev, VHOST_VDPA_SET_CONFIG_CALL, fd);
> >+}
> >+
> > static int vhost_vdpa_get_features(struct vhost_dev *dev,
> >                                      uint64_t *features)
> > {
> >@@ -611,4 +645,5 @@ const VhostOps vdpa_ops = {
> >         .vhost_get_device_id = vhost_vdpa_get_device_id,
> >         .vhost_vq_get_addr = vhost_vdpa_vq_get_addr,
> >         .vhost_force_iommu = vhost_vdpa_force_iommu,
> >+        .vhost_set_config_call = vhost_vdpa_set_config_call,
> > };
> >diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> >index 8a6f8e2a7a..1a2fee8994 100644
> >--- a/include/hw/virtio/vhost-backend.h
> >+++ b/include/hw/virtio/vhost-backend.h
> >@@ -125,6 +125,9 @@ typedef int (*vhost_get_device_id_op)(struct vhost_dev *dev, uint32_t *dev_id);
> >
> > typedef bool (*vhost_force_iommu_op)(struct vhost_dev *dev);
> >
> >+typedef int (*vhost_set_config_call_op)(struct vhost_dev *dev,
> >+                                       int *fd);
>                                           ^
>                                           One more space :-)
>
Thanks  Stefano , will fix this
> Thanks,
> Stefano
>

On 2021/1/26 下午3:42, Cindy Lu wrote:
> Add call back function for configure interrupt.
> Set the notifier's fd to the kernel driver when vdpa start.
> also set -1 while vdpa stop. then the kernel will release
> the related resource
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>   hw/virtio/trace-events            |  2 ++
>   hw/virtio/vhost-vdpa.c            | 37 ++++++++++++++++++++++++++++++-
>   include/hw/virtio/vhost-backend.h |  4 ++++
>   3 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 2060a144a2..6710835b46 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -52,6 +52,8 @@ vhost_vdpa_set_vring_call(void *dev, unsigned int index, int fd) "dev: %p index:
>   vhost_vdpa_get_features(void *dev, uint64_t features) "dev: %p features: 0x%"PRIx64
>   vhost_vdpa_set_owner(void *dev) "dev: %p"
>   vhost_vdpa_vq_get_addr(void *dev, void *vq, uint64_t desc_user_addr, uint64_t avail_user_addr, uint64_t used_user_addr) "dev: %p vq: %p desc_user_addr: 0x%"PRIx64" avail_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64
> +vhost_vdpa_set_config_call(void *dev, int *fd)"dev: %p fd: %p"
> +
>   
>   # virtio.c
>   virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u"
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 01d2101d09..cc1d39d663 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -467,20 +467,47 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
>       }
>       return ret;
>    }
> +static void vhost_vdpa_config_notify_start(struct vhost_dev *dev,
> +                                struct VirtIODevice *vdev, bool start)
> +{
> +    int fd, r;
> +    if (start) {
> +        fd = event_notifier_get_fd(&vdev->config_notifier);
> +        vdev->use_config_notifier = true;
> +     } else {
> +        fd = -1;
> +        vdev->use_config_notifier = false;
> +     }
> +     /*set the fd call back to vdpa driver*/


I guess checkpatch.pl might warn here. Please try to silent checkpath.pl 
before submitting patches.


> +    r = dev->vhost_ops->vhost_set_config_call(dev, &fd);
> +    if (r) {
> +        vdev->use_config_notifier = false;
> +        info_report("vhost_vdpa_config_notify not started!");


This looks kind of fragile. Do we need some workaround here like:

1) filter out the features that depends on config interrupt

or

2) A timer to watch the change of config space

Thanks


> +    }
> +    /*active the config_notifier when vdev->use_config_notifier is true*/
> +    if ((vdev->use_config_notifier) && (start)) {
> +        event_notifier_set(&vdev->config_notifier);
> +    }
> +    return;
>   
> +}
>   static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>   {
>       struct vhost_vdpa *v = dev->opaque;
>       trace_vhost_vdpa_dev_start(dev, started);
> +    VirtIODevice *vdev = dev->vdev;
> +
>       if (started) {
>           uint8_t status = 0;
>           memory_listener_register(&v->listener, &address_space_memory);
>           vhost_vdpa_set_vring_ready(dev);
>           vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>           vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &status);
> -
> +        /*set the configure interrupt call back*/
> +        vhost_vdpa_config_notify_start(dev, vdev, true);
>           return !(status & VIRTIO_CONFIG_S_DRIVER_OK);
>       } else {
> +        vhost_vdpa_config_notify_start(dev, vdev, false);
>           vhost_vdpa_reset_device(dev);
>           vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>                                      VIRTIO_CONFIG_S_DRIVER);
> @@ -546,6 +573,13 @@ static int vhost_vdpa_set_vring_call(struct vhost_dev *dev,
>       return vhost_vdpa_call(dev, VHOST_SET_VRING_CALL, file);
>   }
>   
> +static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
> +                                       int *fd)
> +{
> +    trace_vhost_vdpa_set_config_call(dev, fd);
> +    return vhost_vdpa_call(dev, VHOST_VDPA_SET_CONFIG_CALL, fd);
> +}
> +
>   static int vhost_vdpa_get_features(struct vhost_dev *dev,
>                                        uint64_t *features)
>   {
> @@ -611,4 +645,5 @@ const VhostOps vdpa_ops = {
>           .vhost_get_device_id = vhost_vdpa_get_device_id,
>           .vhost_vq_get_addr = vhost_vdpa_vq_get_addr,
>           .vhost_force_iommu = vhost_vdpa_force_iommu,
> +        .vhost_set_config_call = vhost_vdpa_set_config_call,
>   };
> diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> index 8a6f8e2a7a..1a2fee8994 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -125,6 +125,9 @@ typedef int (*vhost_get_device_id_op)(struct vhost_dev *dev, uint32_t *dev_id);
>   
>   typedef bool (*vhost_force_iommu_op)(struct vhost_dev *dev);
>   
> +typedef int (*vhost_set_config_call_op)(struct vhost_dev *dev,
> +                                       int *fd);
> +
>   typedef struct VhostOps {
>       VhostBackendType backend_type;
>       vhost_backend_init vhost_backend_init;
> @@ -170,6 +173,7 @@ typedef struct VhostOps {
>       vhost_vq_get_addr_op  vhost_vq_get_addr;
>       vhost_get_device_id_op vhost_get_device_id;
>       vhost_force_iommu_op vhost_force_iommu;
> +    vhost_set_config_call_op vhost_set_config_call;
>   } VhostOps;
>   
>   extern const VhostOps user_ops;

On Wed, Jan 27, 2021 at 1:47 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/1/26 下午3:42, Cindy Lu wrote:
> > Add call back function for configure interrupt.
> > Set the notifier's fd to the kernel driver when vdpa start.
> > also set -1 while vdpa stop. then the kernel will release
> > the related resource
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >   hw/virtio/trace-events            |  2 ++
> >   hw/virtio/vhost-vdpa.c            | 37 ++++++++++++++++++++++++++++++-
> >   include/hw/virtio/vhost-backend.h |  4 ++++
> >   3 files changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> > index 2060a144a2..6710835b46 100644
> > --- a/hw/virtio/trace-events
> > +++ b/hw/virtio/trace-events
> > @@ -52,6 +52,8 @@ vhost_vdpa_set_vring_call(void *dev, unsigned int index, int fd) "dev: %p index:
> >   vhost_vdpa_get_features(void *dev, uint64_t features) "dev: %p features: 0x%"PRIx64
> >   vhost_vdpa_set_owner(void *dev) "dev: %p"
> >   vhost_vdpa_vq_get_addr(void *dev, void *vq, uint64_t desc_user_addr, uint64_t avail_user_addr, uint64_t used_user_addr) "dev: %p vq: %p desc_user_addr: 0x%"PRIx64" avail_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64
> > +vhost_vdpa_set_config_call(void *dev, int *fd)"dev: %p fd: %p"
> > +
> >
> >   # virtio.c
> >   virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u"
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 01d2101d09..cc1d39d663 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -467,20 +467,47 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
> >       }
> >       return ret;
> >    }
> > +static void vhost_vdpa_config_notify_start(struct vhost_dev *dev,
> > +                                struct VirtIODevice *vdev, bool start)
> > +{
> > +    int fd, r;
> > +    if (start) {
> > +        fd = event_notifier_get_fd(&vdev->config_notifier);
> > +        vdev->use_config_notifier = true;
> > +     } else {
> > +        fd = -1;
> > +        vdev->use_config_notifier = false;
> > +     }
> > +     /*set the fd call back to vdpa driver*/
>
>
> I guess checkpatch.pl might warn here. Please try to silent checkpath.pl
> before submitting patches.
>
Actually I do have run this script, but seems not warned here. I will
pay attention next time
>
> > +    r = dev->vhost_ops->vhost_set_config_call(dev, &fd);
> > +    if (r) {
> > +        vdev->use_config_notifier = false;
> > +        info_report("vhost_vdpa_config_notify not started!");
>
>
> This looks kind of fragile. Do we need some workaround here like:
>
> 1) filter out the features that depends on config interrupt
>
> or
>
> 2) A timer to watch the change of config space
>
> Thanks
>
sure I will add this check
>
> > +    }
> > +    /*active the config_notifier when vdev->use_config_notifier is true*/
> > +    if ((vdev->use_config_notifier) && (start)) {
> > +        event_notifier_set(&vdev->config_notifier);
> > +    }
> > +    return;
> >
> > +}
> >   static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> >   {
> >       struct vhost_vdpa *v = dev->opaque;
> >       trace_vhost_vdpa_dev_start(dev, started);
> > +    VirtIODevice *vdev = dev->vdev;
> > +
> >       if (started) {
> >           uint8_t status = 0;
> >           memory_listener_register(&v->listener, &address_space_memory);
> >           vhost_vdpa_set_vring_ready(dev);
> >           vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> >           vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &status);
> > -
> > +        /*set the configure interrupt call back*/
> > +        vhost_vdpa_config_notify_start(dev, vdev, true);
> >           return !(status & VIRTIO_CONFIG_S_DRIVER_OK);
> >       } else {
> > +        vhost_vdpa_config_notify_start(dev, vdev, false);
> >           vhost_vdpa_reset_device(dev);
> >           vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >                                      VIRTIO_CONFIG_S_DRIVER);
> > @@ -546,6 +573,13 @@ static int vhost_vdpa_set_vring_call(struct vhost_dev *dev,
> >       return vhost_vdpa_call(dev, VHOST_SET_VRING_CALL, file);
> >   }
> >
> > +static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
> > +                                       int *fd)
> > +{
> > +    trace_vhost_vdpa_set_config_call(dev, fd);
> > +    return vhost_vdpa_call(dev, VHOST_VDPA_SET_CONFIG_CALL, fd);
> > +}
> > +
> >   static int vhost_vdpa_get_features(struct vhost_dev *dev,
> >                                        uint64_t *features)
> >   {
> > @@ -611,4 +645,5 @@ const VhostOps vdpa_ops = {
> >           .vhost_get_device_id = vhost_vdpa_get_device_id,
> >           .vhost_vq_get_addr = vhost_vdpa_vq_get_addr,
> >           .vhost_force_iommu = vhost_vdpa_force_iommu,
> > +        .vhost_set_config_call = vhost_vdpa_set_config_call,
> >   };
> > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> > index 8a6f8e2a7a..1a2fee8994 100644
> > --- a/include/hw/virtio/vhost-backend.h
> > +++ b/include/hw/virtio/vhost-backend.h
> > @@ -125,6 +125,9 @@ typedef int (*vhost_get_device_id_op)(struct vhost_dev *dev, uint32_t *dev_id);
> >
> >   typedef bool (*vhost_force_iommu_op)(struct vhost_dev *dev);
> >
> > +typedef int (*vhost_set_config_call_op)(struct vhost_dev *dev,
> > +                                       int *fd);
> > +
> >   typedef struct VhostOps {
> >       VhostBackendType backend_type;
> >       vhost_backend_init vhost_backend_init;
> > @@ -170,6 +173,7 @@ typedef struct VhostOps {
> >       vhost_vq_get_addr_op  vhost_vq_get_addr;
> >       vhost_get_device_id_op vhost_get_device_id;
> >       vhost_force_iommu_op vhost_force_iommu;
> > +    vhost_set_config_call_op vhost_set_config_call;
> >   } VhostOps;
> >
> >   extern const VhostOps user_ops;
>

On 2021/1/27 下午3:51, Cindy Lu wrote:
>>> +     /*set the fd call back to vdpa driver*/
>> I guess checkpatch.pl might warn here. Please try to silent checkpath.pl
>> before submitting patches.
>>
> Actually I do have run this script, but seems not warned here. I will
> pay attention next time


Oh right, I remember kernel checkpath did the check but I don't know if 
the qemu one check the comment sytle.

Thanks