All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: dev@dpdk.org
Cc: Eelco Chaudron <echaudro@redhat.com>,
	Maxime Coquelin <maxime.coquelin@redhat.com>,
	Chenbo Xia <chenbox@nvidia.com>
Subject: [PATCH v2 5/5] vhost: enhance virtqueue access lock asserts
Date: Tue,  5 Dec 2023 10:45:35 +0100	[thread overview]
Message-ID: <20231205094536.2816720-5-david.marchand@redhat.com> (raw)
In-Reply-To: <20231205094536.2816720-1-david.marchand@redhat.com>

A simple comment in vhost_user_msg_handler() is not that robust.

Add a lock_all_qps property to message handlers so that their
implementation can add a build check and assert a vq is locked.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
Changes since v1:
- moved this patch as the last of the series,

---
 lib/vhost/vhost_user.c | 114 +++++++++++++++++++----------------------
 1 file changed, 53 insertions(+), 61 deletions(-)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 651ea5854b..e8020ccfd3 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -56,14 +56,24 @@
 #define INFLIGHT_ALIGNMENT	64
 #define INFLIGHT_VERSION	0x1
 
-typedef struct vhost_message_handler {
+typedef const struct vhost_message_handler {
 	const char *description;
 	int (*callback)(struct virtio_net **pdev, struct vhu_msg_context *ctx,
 		int main_fd);
 	bool accepts_fd;
+	bool lock_all_qps;
 } vhost_message_handler_t;
 static vhost_message_handler_t vhost_message_handlers[];
 
+/* vhost_user_msg_handler() locks all qps based on a handler's lock_all_qps.
+ * Later, a handler may need to ensure the vq has been locked (for example,
+ * when calling lock annotated helpers).
+ */
+#define VHOST_USER_ASSERT_LOCK(dev, vq, id) do { \
+	RTE_BUILD_BUG_ON(!vhost_message_handlers[id].lock_all_qps); \
+	vq_assert_lock(dev, vq); \
+} while (0)
+
 static int send_vhost_reply(struct virtio_net *dev, int sockfd, struct vhu_msg_context *ctx);
 static int read_vhost_message(struct virtio_net *dev, int sockfd, struct vhu_msg_context *ctx);
 
@@ -400,7 +410,7 @@ vhost_user_set_features(struct virtio_net **pdev,
 			cleanup_vq(vq, 1);
 			cleanup_vq_inflight(dev, vq);
 			/* vhost_user_lock_all_queue_pairs locked all qps */
-			vq_assert_lock(dev, vq);
+			VHOST_USER_ASSERT_LOCK(dev, vq, VHOST_USER_SET_FEATURES);
 			rte_rwlock_write_unlock(&vq->access_lock);
 			free_vq(dev, vq);
 		}
@@ -927,7 +937,7 @@ vhost_user_set_vring_addr(struct virtio_net **pdev,
 	vq = dev->virtqueue[ctx->msg.payload.addr.index];
 
 	/* vhost_user_lock_all_queue_pairs locked all qps */
-	vq_assert_lock(dev, vq);
+	VHOST_USER_ASSERT_LOCK(dev, vq, VHOST_USER_SET_VRING_ADDR);
 
 	access_ok = vq->access_ok;
 
@@ -1442,7 +1452,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev,
 
 		if (vq->desc || vq->avail || vq->used) {
 			/* vhost_user_lock_all_queue_pairs locked all qps */
-			vq_assert_lock(dev, vq);
+			VHOST_USER_ASSERT_LOCK(dev, vq, VHOST_USER_SET_MEM_TABLE);
 
 			/*
 			 * If the memory table got updated, the ring addresses
@@ -2234,7 +2244,7 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
 	vq = dev->virtqueue[index];
 	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
 		/* vhost_user_lock_all_queue_pairs locked all qps */
-		vq_assert_lock(dev, vq);
+		VHOST_USER_ASSERT_LOCK(dev, vq, VHOST_USER_SET_VRING_ENABLE);
 		if (enable && vq->async && vq->async->pkts_inflight_n) {
 			VHOST_LOG_CONFIG(dev->ifname, ERR,
 				"failed to enable vring. Inflight packets must be completed first\n");
@@ -2839,41 +2849,43 @@ vhost_user_set_status(struct virtio_net **pdev,
 }
 
 #define VHOST_MESSAGE_HANDLERS \
-VHOST_MESSAGE_HANDLER(VHOST_USER_NONE, NULL, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_FEATURES, vhost_user_get_features, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_FEATURES, vhost_user_set_features, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_OWNER, vhost_user_set_owner, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_RESET_OWNER, vhost_user_reset_owner, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_MEM_TABLE, vhost_user_set_mem_table, true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_LOG_BASE, vhost_user_set_log_base, true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_LOG_FD, vhost_user_set_log_fd, true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_NUM, vhost_user_set_vring_num, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ADDR, vhost_user_set_vring_addr, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_BASE, vhost_user_set_vring_base, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_VRING_BASE, vhost_user_get_vring_base, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_KICK, vhost_user_set_vring_kick, true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_CALL, vhost_user_set_vring_call, true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ERR, vhost_user_set_vring_err, true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_PROTOCOL_FEATURES, vhost_user_get_protocol_features, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_PROTOCOL_FEATURES, vhost_user_set_protocol_features, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_QUEUE_NUM, vhost_user_get_queue_num, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ENABLE, vhost_user_set_vring_enable, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SEND_RARP, vhost_user_send_rarp, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_NET_SET_MTU, vhost_user_net_set_mtu, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_BACKEND_REQ_FD, vhost_user_set_req_fd, true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_IOTLB_MSG, vhost_user_iotlb_msg, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_CONFIG, vhost_user_get_config, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_CONFIG, vhost_user_set_config, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_ADVISE, vhost_user_set_postcopy_advise, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_LISTEN, vhost_user_set_postcopy_listen, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_END, vhost_user_postcopy_end, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_INFLIGHT_FD, vhost_user_get_inflight_fd, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_INFLIGHT_FD, vhost_user_set_inflight_fd, true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_STATUS, vhost_user_set_status, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_STATUS, vhost_user_get_status, false)
-
-#define VHOST_MESSAGE_HANDLER(id, handler, accepts_fd) \
-	[id] = { #id, handler, accepts_fd },
+VHOST_MESSAGE_HANDLER(VHOST_USER_NONE, NULL, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_FEATURES, vhost_user_get_features, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_FEATURES, vhost_user_set_features, false, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_OWNER, vhost_user_set_owner, false, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_RESET_OWNER, vhost_user_reset_owner, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_MEM_TABLE, vhost_user_set_mem_table, true, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_LOG_BASE, vhost_user_set_log_base, true, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_LOG_FD, vhost_user_set_log_fd, true, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_NUM, vhost_user_set_vring_num, false, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ADDR, vhost_user_set_vring_addr, false, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_BASE, vhost_user_set_vring_base, false, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_VRING_BASE, vhost_user_get_vring_base, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_KICK, vhost_user_set_vring_kick, true, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_CALL, vhost_user_set_vring_call, true, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ERR, vhost_user_set_vring_err, true, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_PROTOCOL_FEATURES, vhost_user_get_protocol_features, \
+	false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_PROTOCOL_FEATURES, vhost_user_set_protocol_features, \
+	false, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_QUEUE_NUM, vhost_user_get_queue_num, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ENABLE, vhost_user_set_vring_enable, false, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SEND_RARP, vhost_user_send_rarp, false, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_NET_SET_MTU, vhost_user_net_set_mtu, false, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_BACKEND_REQ_FD, vhost_user_set_req_fd, true, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_IOTLB_MSG, vhost_user_iotlb_msg, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_CONFIG, vhost_user_get_config, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_CONFIG, vhost_user_set_config, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_ADVISE, vhost_user_set_postcopy_advise, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_LISTEN, vhost_user_set_postcopy_listen, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_END, vhost_user_postcopy_end, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_INFLIGHT_FD, vhost_user_get_inflight_fd, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_INFLIGHT_FD, vhost_user_set_inflight_fd, true, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_STATUS, vhost_user_set_status, false, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_STATUS, vhost_user_get_status, false, false)
+
+#define VHOST_MESSAGE_HANDLER(id, handler, accepts_fd, lock_all_qps) \
+	[id] = { #id, handler, accepts_fd, lock_all_qps },
 static vhost_message_handler_t vhost_message_handlers[] = {
 	VHOST_MESSAGE_HANDLERS
 };
@@ -3141,31 +3153,11 @@ vhost_user_msg_handler(int vid, int fd)
 	 * inactive, so it is safe. Otherwise taking the access_lock
 	 * would cause a dead lock.
 	 */
-	switch (request) {
-	case VHOST_USER_SET_FEATURES:
-	case VHOST_USER_SET_PROTOCOL_FEATURES:
-	case VHOST_USER_SET_OWNER:
-	case VHOST_USER_SET_MEM_TABLE:
-	case VHOST_USER_SET_LOG_BASE:
-	case VHOST_USER_SET_LOG_FD:
-	case VHOST_USER_SET_VRING_NUM:
-	case VHOST_USER_SET_VRING_ADDR:
-	case VHOST_USER_SET_VRING_BASE:
-	case VHOST_USER_SET_VRING_KICK:
-	case VHOST_USER_SET_VRING_CALL:
-	case VHOST_USER_SET_VRING_ERR:
-	case VHOST_USER_SET_VRING_ENABLE:
-	case VHOST_USER_SEND_RARP:
-	case VHOST_USER_NET_SET_MTU:
-	case VHOST_USER_SET_BACKEND_REQ_FD:
+	if (msg_handler->lock_all_qps) {
 		if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
 			vhost_user_lock_all_queue_pairs(dev);
 			unlock_required = 1;
 		}
-		break;
-	default:
-		break;
-
 	}
 
 	handled = false;
-- 
2.42.0


  parent reply	other threads:[~2023-12-05  9:46 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-23  9:55 [PATCH 1/3] vhost: robustify virtqueue access lock asserts David Marchand
2023-10-23  9:55 ` [PATCH 2/3] vhost: fix virtqueue access lock in datapath David Marchand
2023-10-27  9:03   ` Eelco Chaudron
2023-10-27  9:22     ` David Marchand
2023-10-27 10:11       ` Eelco Chaudron
2023-12-05  9:10   ` Maxime Coquelin
2023-10-23  9:55 ` [PATCH 3/3] vhost: annotate virtqueue access checks David Marchand
2023-10-27  9:04   ` Eelco Chaudron
2023-10-27  8:59 ` [PATCH 1/3] vhost: robustify virtqueue access lock asserts Eelco Chaudron
2023-12-05  9:02 ` Maxime Coquelin
2023-12-05  9:45 ` [PATCH v2 1/5] vhost: fix virtqueue access check in datapath David Marchand
2023-12-05  9:45   ` [PATCH v2 2/5] vhost: fix virtqueue access check in VDUSE setup David Marchand
2023-12-05  9:57     ` Maxime Coquelin
2023-12-05  9:45   ` [PATCH v2 3/5] vhost: fix virtqueue access check in vhost-user setup David Marchand
2023-12-05  9:59     ` Maxime Coquelin
2023-12-05  9:45   ` [PATCH v2 4/5] vhost: annotate virtqueue access checks David Marchand
2023-12-05 11:04     ` Maxime Coquelin
2023-12-05  9:45   ` David Marchand [this message]
2024-02-19 10:52     ` [PATCH v2 5/5] vhost: enhance virtqueue access lock asserts Thomas Monjalon
2024-02-26 15:46       ` David Marchand
2024-02-26 17:05     ` [PATCH v3] " David Marchand
2024-02-26 19:40       ` Maxime Coquelin
2024-02-26 20:56         ` Stephen Hemminger
2024-02-27 10:39     ` [PATCH v4] " David Marchand
2024-03-01 15:22       ` Maxime Coquelin
2024-03-05  9:04       ` David Marchand
2023-12-12 11:37   ` [PATCH v2 1/5] vhost: fix virtqueue access check in datapath Maxime Coquelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231205094536.2816720-5-david.marchand@redhat.com \
    --to=david.marchand@redhat.com \
    --cc=chenbox@nvidia.com \
    --cc=dev@dpdk.org \
    --cc=echaudro@redhat.com \
    --cc=maxime.coquelin@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.