Hi Chenbo,

-----Original Message-----
From: Xia, Chenbo <redacted>
Sent: Wednesday, September 15, 2021 4:52 PM
To: Li, Miao <redacted>; dev@dpdk.org
Cc: maxime.coquelin@redhat.com
Subject: RE: [PATCH 2/5] lib/vhost: implement rte_power_monitor API

Hi Miao,

quoted

-----Original Message-----
From: Li, Miao <redacted>
Sent: Friday, September 10, 2021 9:06 PM
To: dev@dpdk.org
Cc: Xia, Chenbo <redacted>; maxime.coquelin@redhat.com;
Li, Miao [off-list ref]
Subject: [PATCH 2/5] lib/vhost: implement rte_power_monitor API

Should be 'vhost: implement rte_power_monitor API'

I will modify it in the next version.

quoted

This patch defines rte_vhost_power_monitor_cond which is used to pass
some information to vhost driver. The information is including the
address to monitor, the expected value, the mask to extract value read
from 'addr', the flag used to distinguish packed ring or split ring.
Vhost driver can use these information to fill rte_power_monitor_cond.

Signed-off-by: Miao Li <redacted>
---
 lib/vhost/rte_vhost.h | 33 +++++++++++++++++++++++++++++++++
lib/vhost/version.map |  3 +++
 lib/vhost/vhost.c     | 30 ++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+)

diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h index

8d875e9322..f58643b0a3 100644

--- a/lib/vhost/rte_vhost.h
+++ b/lib/vhost/rte_vhost.h

@@ -38,6 +38,8 @@ extern "C" {
 #define RTE_VHOST_USER_ASYNC_COPY	(1ULL << 7)
 #define RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS	(1ULL << 8)

+#define VHOST_POWER_MONITOR_RING_PACKED (1ULL << 0)

I'd say I don't quite like introducing this flag so that vhost lib app needs to know
the vring is split or packed. I have another suggestion to do the same thing,
please check below comment.

Yes, I agree with you. I will remove it in the next version.

quoted

+
 /* Features. */
 #ifndef VIRTIO_NET_F_GUEST_ANNOUNCE
  #define VIRTIO_NET_F_GUEST_ANNOUNCE 21 @@ -292,6 +294,20 @@

struct

quoted

vhost_device_ops {
 	void *reserved[1]; /**< Reserved for future extension */  };

+/**
+ * Power monitor condition.
+ */
+struct rte_vhost_power_monitor_cond {
+	volatile void *addr;  /**< Address to monitor for changes */
+	/**< If the `mask` is non-zero, location pointed
+	 *   to by `addr` will be read and compared
+	 *   against this value.
+	 */
+	uint64_t val;
+	uint64_t mask; /**< 64-bit mask to extract value read from `addr` */
+	uint8_t flag;  /**< if 1, vhost packed ring, otherwise split ring */

What about define two values instead of the flag. One value for '(value & m) ==
v ?' is True, another for False.

I think one value to represent true or false is enough. I will fix it in the next version.

quoted

+};
+
 /**
  * Convert guest physical address to host virtual address
  *

@@ -914,6 +930,23 @@ int rte_vhost_vring_call(int vid, uint16_t vring_idx);
  */
 uint32_t rte_vhost_rx_queue_count(int vid, uint16_t qid);

+/**
+ * Get power monitor address of the vhost device
+ *
+ * @param vid
+ *  vhost device ID
+ * @param queue_id
+ *  vhost queue ID
+ * @param pmc
+ *  power monitor condition
+ * @return
+ *  0 on success, -1 on failure
+ */
+__rte_experimental
+int
+rte_vhost_get_monitor_addr(int vid, uint16_t queue_id,
+		struct rte_vhost_power_monitor_cond *pmc);
+
 /**
  * Get log base and log size of the vhost device
  *

diff --git a/lib/vhost/version.map b/lib/vhost/version.map index

c92a9d4962..0a9667ef1e 100644

--- a/lib/vhost/version.map
+++ b/lib/vhost/version.map

@@ -85,4 +85,7 @@ EXPERIMENTAL {
 	rte_vhost_async_channel_register_thread_unsafe;
 	rte_vhost_async_channel_unregister_thread_unsafe;
 	rte_vhost_clear_queue_thread_unsafe;
+
+	# added in 21.11
+	rte_vhost_get_monitor_addr;
 };

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index

355ff37651..f7374d3f94 100644

--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c

@@ -1886,5 +1886,35 @@ int rte_vhost_async_get_inflight(int vid,

uint16_t
queue_id)
 	return ret;
 }

+int
+rte_vhost_get_monitor_addr(int vid, uint16_t queue_id,
+		struct rte_vhost_power_monitor_cond *pmc) {
+	struct virtio_net *dev = get_device(vid);

Check dev is not NULL before accessing its member.

I will add dev and queue_id check here in the next version.

quoted

+	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
+	if (vq == NULL)
+		return -1;
+	if (vq_is_packed(dev)) {
+		struct vring_packed_desc *desc;
+		desc = vq->desc_packed;
+		pmc->addr = &desc[vq->last_avail_idx].flags;
+		if (vq->avail_wrap_counter)
+			pmc->val = VRING_DESC_F_AVAIL;
+		else
+			pmc->val = VRING_DESC_F_USED;
+		pmc->mask = VRING_DESC_F_AVAIL | VRING_DESC_F_USED;
+		pmc->flag = VHOST_POWER_MONITOR_RING_PACKED;
+	} else {
+		pmc->addr = &vq->avail->idx;
+		pmc->val = vq->last_avail_idx & (vq->size - 1);
+		pmc->mask = vq->size - 1;
+		pmc->flag = 0;
+	}
+	if (pmc->addr == NULL)
+		return -1;

Is it possible that addr == NULL?

Yes, it is unnecessary. I will delete it in the next version.

Thanks,
Miao

Thanks,
Chenbo

quoted

+
+	return 0;
+}
+
 RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO);
RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING);
--
2.25.1