Re: [PATCH 04/31] vdpa: Add vhost_svq_set_svq_kick_fd

2022-02-08 Thread Jason Wang


在 2022/1/31 下午6:18, Eugenio Perez Martin 写道:

On Fri, Jan 28, 2022 at 7:29 AM Jason Wang  wrote:


在 2022/1/22 上午4:27, Eugenio Pérez 写道:

This function allows the vhost-vdpa backend to override kick_fd.

Signed-off-by: Eugenio Pérez 
---
   hw/virtio/vhost-shadow-virtqueue.h |  1 +
   hw/virtio/vhost-shadow-virtqueue.c | 45 ++
   2 files changed, 46 insertions(+)

diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
b/hw/virtio/vhost-shadow-virtqueue.h
index 400effd9f2..a56ecfc09d 100644
--- a/hw/virtio/vhost-shadow-virtqueue.h
+++ b/hw/virtio/vhost-shadow-virtqueue.h
@@ -15,6 +15,7 @@

   typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;

+void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd);
   const EventNotifier *vhost_svq_get_dev_kick_notifier(
 const VhostShadowVirtqueue 
*svq);

diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
b/hw/virtio/vhost-shadow-virtqueue.c
index bd87110073..21534bc94d 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -11,6 +11,7 @@
   #include "hw/virtio/vhost-shadow-virtqueue.h"

   #include "qemu/error-report.h"
+#include "qemu/main-loop.h"

   /* Shadow virtqueue to relay notifications */
   typedef struct VhostShadowVirtqueue {
@@ -18,8 +19,20 @@ typedef struct VhostShadowVirtqueue {
   EventNotifier hdev_kick;
   /* Shadow call notifier, sent to vhost */
   EventNotifier hdev_call;
+
+/*
+ * Borrowed virtqueue's guest to host notifier.
+ * To borrow it in this event notifier allows to register on the event
+ * loop and access the associated shadow virtqueue easily. If we use the
+ * VirtQueue, we don't have an easy way to retrieve it.
+ *
+ * So shadow virtqueue must not clean it, or we would lose VirtQueue one.
+ */
+EventNotifier svq_kick;
   } VhostShadowVirtqueue;

+#define INVALID_SVQ_KICK_FD -1
+
   /**
* The notifier that SVQ will use to notify the device.
*/
@@ -29,6 +42,35 @@ const EventNotifier *vhost_svq_get_dev_kick_notifier(
   return &svq->hdev_kick;
   }

+/**
+ * Set a new file descriptor for the guest to kick SVQ and notify for avail
+ *
+ * @svq  The svq
+ * @svq_kick_fd  The new svq kick fd
+ */
+void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd)
+{
+EventNotifier tmp;
+bool check_old = INVALID_SVQ_KICK_FD !=
+ event_notifier_get_fd(&svq->svq_kick);
+
+if (check_old) {
+event_notifier_set_handler(&svq->svq_kick, NULL);
+event_notifier_init_fd(&tmp, event_notifier_get_fd(&svq->svq_kick));
+}


It looks to me we don't do similar things in vhost-net. Any reason for
caring about the old svq_kick?


Do you mean to check for old kick_fd in case we miss notifications,
and explicitly omit the INVALID_SVQ_KICK_FD?



Yes.




If you mean qemu's vhost-net, I guess it's because the device's kick
fd is never changed in all the vhost device lifecycle, it's only set
at the beginning. Previous RFC also depended on that, but you
suggested better vhost and SVQ in v4 feedback if I understood
correctly [1]. Or am I missing something?



No, I forgot that. But in this case we should have a better dealing with 
the the conversion from valid fd to -1 by disabling the handler.





Qemu's vhost-net does not need to use this because it is not polling
it. For kernel's vhost, I guess the closest is the use of pollstop and
pollstart at vhost_vring_ioctl.

In my opinion, I think that SVQ code size can benefit from now
allowing to override kick_fd from the start of the operation. Not from
initialization, but start. But I can see the benefits of having the
change into account from this moment so it's more resilient to the
future.


+
+/*
+ * event_notifier_set_handler already checks for guest's notifications if
+ * they arrive to the new file descriptor in the switch, so there is no
+ * need to explicitely check for them.
+ */
+event_notifier_init_fd(&svq->svq_kick, svq_kick_fd);
+
+if (!check_old || event_notifier_test_and_clear(&tmp)) {
+event_notifier_set(&svq->hdev_kick);


Any reason we need to kick the device directly here?


At this point of the series only notifications are forwarded, not
buffers. If kick_fd is set, we need to check the old one, the same way
as vhost checks the masked notifier in case of change.



I meant we need to kick the svq instead of vhost-vdpa in this case?

Thanks




Thanks!

[1] https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg03152.html
, from "I'd suggest to not depend on this since it:"



Thanks



+}
+}
+
   /**
* Creates vhost shadow virtqueue, and instruct vhost device to use the 
shadow
* methods and file descriptors.
@@ -52,6 +94,9 @@ VhostShadowVirtqueue *vhost_svq_new(void)
   goto err_init_hdev_call;
   }

+/* Placeholder descriptor, it should be deleted at set_kick_fd */
+ev

Re: [PATCH 04/31] vdpa: Add vhost_svq_set_svq_kick_fd

2022-01-27 Thread Jason Wang


在 2022/1/22 上午4:27, Eugenio Pérez 写道:

This function allows the vhost-vdpa backend to override kick_fd.

Signed-off-by: Eugenio Pérez 
---
  hw/virtio/vhost-shadow-virtqueue.h |  1 +
  hw/virtio/vhost-shadow-virtqueue.c | 45 ++
  2 files changed, 46 insertions(+)

diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
b/hw/virtio/vhost-shadow-virtqueue.h
index 400effd9f2..a56ecfc09d 100644
--- a/hw/virtio/vhost-shadow-virtqueue.h
+++ b/hw/virtio/vhost-shadow-virtqueue.h
@@ -15,6 +15,7 @@
  
  typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
  
+void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd);

  const EventNotifier *vhost_svq_get_dev_kick_notifier(
const VhostShadowVirtqueue 
*svq);
  
diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c

index bd87110073..21534bc94d 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -11,6 +11,7 @@
  #include "hw/virtio/vhost-shadow-virtqueue.h"
  
  #include "qemu/error-report.h"

+#include "qemu/main-loop.h"
  
  /* Shadow virtqueue to relay notifications */

  typedef struct VhostShadowVirtqueue {
@@ -18,8 +19,20 @@ typedef struct VhostShadowVirtqueue {
  EventNotifier hdev_kick;
  /* Shadow call notifier, sent to vhost */
  EventNotifier hdev_call;
+
+/*
+ * Borrowed virtqueue's guest to host notifier.
+ * To borrow it in this event notifier allows to register on the event
+ * loop and access the associated shadow virtqueue easily. If we use the
+ * VirtQueue, we don't have an easy way to retrieve it.
+ *
+ * So shadow virtqueue must not clean it, or we would lose VirtQueue one.
+ */
+EventNotifier svq_kick;
  } VhostShadowVirtqueue;
  
+#define INVALID_SVQ_KICK_FD -1

+
  /**
   * The notifier that SVQ will use to notify the device.
   */
@@ -29,6 +42,35 @@ const EventNotifier *vhost_svq_get_dev_kick_notifier(
  return &svq->hdev_kick;
  }
  
+/**

+ * Set a new file descriptor for the guest to kick SVQ and notify for avail
+ *
+ * @svq  The svq
+ * @svq_kick_fd  The new svq kick fd
+ */
+void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd)
+{
+EventNotifier tmp;
+bool check_old = INVALID_SVQ_KICK_FD !=
+ event_notifier_get_fd(&svq->svq_kick);
+
+if (check_old) {
+event_notifier_set_handler(&svq->svq_kick, NULL);
+event_notifier_init_fd(&tmp, event_notifier_get_fd(&svq->svq_kick));
+}



It looks to me we don't do similar things in vhost-net. Any reason for 
caring about the old svq_kick?




+
+/*
+ * event_notifier_set_handler already checks for guest's notifications if
+ * they arrive to the new file descriptor in the switch, so there is no
+ * need to explicitely check for them.
+ */
+event_notifier_init_fd(&svq->svq_kick, svq_kick_fd);
+
+if (!check_old || event_notifier_test_and_clear(&tmp)) {
+event_notifier_set(&svq->hdev_kick);



Any reason we need to kick the device directly here?

Thanks



+}
+}
+
  /**
   * Creates vhost shadow virtqueue, and instruct vhost device to use the shadow
   * methods and file descriptors.
@@ -52,6 +94,9 @@ VhostShadowVirtqueue *vhost_svq_new(void)
  goto err_init_hdev_call;
  }
  
+/* Placeholder descriptor, it should be deleted at set_kick_fd */

+event_notifier_init_fd(&svq->svq_kick, INVALID_SVQ_KICK_FD);
+
  return g_steal_pointer(&svq);
  
  err_init_hdev_call:


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization