[dpdk-dev] [PATCH 1/4] vhost: introduce new API to get the active vring number

[dpdk-dev] [PATCH 1/4] vhost: introduce new API to get the active vring number

[Andy Pei]

It's useful for hardware vhost backend (like vDPA devices) to set
multiqueue configuration accordingly.

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
Signed-off-by: Andy Pei <andy.pei@intel.com>
---
 lib/librte_vhost/rte_vhost.h | 12 ++++++++++++
 lib/librte_vhost/vhost.c     | 19 +++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
index 7fb1729..28811b0 100644
--- a/lib/librte_vhost/rte_vhost.h
+++ b/lib/librte_vhost/rte_vhost.h
@@ -525,6 +525,18 @@ int rte_vhost_driver_callback_register(const char *path,
 uint16_t rte_vhost_get_vring_num(int vid);
 
 /**
+ * Get the number of active vrings of the device.
+ *
+ * @param vid
+ *  vhost device ID
+ *
+ * @return
+ *  The number of active vrings, 0 on failure
+ */
+uint16_t
+rte_vhost_get_active_vring_num(int vid);
+
+/**
  * Get the virtio net device's ifname, which is the vhost-user socket
  * file path.
  *
diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 981837b..c714818 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -674,6 +674,25 @@
 	return dev->nr_vring;
 }
 
+uint16_t
+rte_vhost_get_active_vring_num(int vid)
+{
+	struct virtio_net *dev = get_device(vid);
+	struct vhost_virtqueue *vq;
+	uint16_t qid;
+
+	if (dev == NULL)
+		return 0;
+
+	for (qid = 0; qid < dev->nr_vring; qid++) {
+		vq = dev->virtqueue[qid];
+		if (!vq->enabled)
+			break;
+	}
+
+	return qid;
+}
+
 int
 rte_vhost_get_ifname(int vid, char *buf, size_t len)
 {
-- 
1.8.3.1

[dpdk-dev] [PATCH 2/4] net/ifcvf: add multiqueue configuration

[Andy Pei]

This is in preparation for multiqueue enabling for vDPA devices.

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
Signed-off-by: Andy Pei <andy.pei@intel.com>
---
 drivers/net/ifc/base/ifcvf.c | 1 +
 drivers/net/ifc/base/ifcvf.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/net/ifc/base/ifcvf.c b/drivers/net/ifc/base/ifcvf.c
index 3c0b2df..6462281 100644
--- a/drivers/net/ifc/base/ifcvf.c
+++ b/drivers/net/ifc/base/ifcvf.c
@@ -82,6 +82,7 @@
 	}
 
 	hw->lm_cfg = hw->mem_resource[4].addr;
+	hw->mq_cfg = hw->mem_resource[4].addr + IFCVF_MQ_OFFSET;
 
 	if (hw->common_cfg == NULL || hw->notify_base == NULL ||
 			hw->isr == NULL || hw->dev_cfg == NULL) {
diff --git a/drivers/net/ifc/base/ifcvf.h b/drivers/net/ifc/base/ifcvf.h
index 9be2770..a4cb1a4 100644
--- a/drivers/net/ifc/base/ifcvf.h
+++ b/drivers/net/ifc/base/ifcvf.h
@@ -38,6 +38,7 @@
 
 #define IFCVF_LM_CFG_SIZE		0x40
 #define IFCVF_LM_RING_STATE_OFFSET	0x20
+#define IFCVF_MQ_OFFSET			0x28
 
 #define IFCVF_LM_LOGGING_CTRL		0x0
 
@@ -127,6 +128,7 @@ struct ifcvf_hw {
 	u16    *notify_base;
 	u16    *notify_addr[IFCVF_MAX_QUEUES * 2];
 	u8     *lm_cfg;
+	u8     *mq_cfg;
 	struct vring_info vring[IFCVF_MAX_QUEUES * 2];
 	u8 nr_vring;
 	struct ifcvf_pci_mem_resource mem_resource[IFCVF_PCI_MAX_RESOURCE];
-- 
1.8.3.1

[dpdk-dev] [PATCH 3/4] vhost: call vDPA callback at the end of vring enable handler

[Andy Pei]

vDPA's set_vring_state callback would need to know the virtqueues'
enable status to configure the hardware.

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
Signed-off-by: Andy Pei <andy.pei@intel.com>
---
 lib/librte_vhost/rte_vdpa.h   | 4 ++--
 lib/librte_vhost/vhost_user.c | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
index 9a3deb3..19cf897 100644
--- a/lib/librte_vhost/rte_vdpa.h
+++ b/lib/librte_vhost/rte_vdpa.h
@@ -54,8 +54,8 @@ struct rte_vdpa_dev_ops {
 	int (*dev_conf)(int vid);
 	int (*dev_close)(int vid);
 
-	/** Enable/disable this vring */
-	int (*set_vring_state)(int vid, int vring, int state);
+	/** Enable/disable vring queue pairs */
+	int (*set_vring_state)(int vid);
 
 	/** Set features when changed */
 	int (*set_features)(int vid);
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 0b72648..21028cc 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1346,8 +1346,6 @@ static int vhost_user_set_vring_err(struct virtio_net **pdev __rte_unused,
 
 	did = dev->vdpa_dev_id;
 	vdpa_dev = rte_vdpa_get_device(did);
-	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
-		vdpa_dev->ops->set_vring_state(dev->vid, index, enable);
 
 	if (dev->notify_ops->vring_state_changed)
 		dev->notify_ops->vring_state_changed(dev->vid,
@@ -1359,6 +1357,9 @@ static int vhost_user_set_vring_err(struct virtio_net **pdev __rte_unused,
 
 	dev->virtqueue[index]->enabled = enable;
 
+	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
+		vdpa_dev->ops->set_vring_state(dev->vid);
+
 	return RTE_VHOST_MSG_RESULT_OK;
 }
 
-- 
1.8.3.1

[dpdk-dev] [PATCH 4/4] net/ifcvf: enable mutliqueue support

[Andy Pei]

Enable mutliqueue support for ifcvf devices by setting the mutliqueue
configuration space.

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
Signed-off-by: Andy Pei <andy.pei@intel.com>
---
 drivers/net/ifc/base/ifcvf.c |  8 ++++++++
 drivers/net/ifc/base/ifcvf.h |  5 ++++-
 drivers/net/ifc/ifcvf_vdpa.c | 31 +++++++++++++++++++++++++++++--
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ifc/base/ifcvf.c b/drivers/net/ifc/base/ifcvf.c
index 6462281..ac09537 100644
--- a/drivers/net/ifc/base/ifcvf.c
+++ b/drivers/net/ifc/base/ifcvf.c
@@ -310,6 +310,14 @@
 	*(u32 *)(lm_cfg + IFCVF_LM_LOGGING_CTRL) = IFCVF_LM_DISABLE;
 }
 
+void ifcvf_enable_multiqueue(struct ifcvf_hw *hw, u16 nr_queue_pair)
+{
+	u8 *mq_cfg;
+
+	mq_cfg = hw->mq_cfg;
+	*(u32 *)mq_cfg = nr_queue_pair;
+}
+
 void
 ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
 {
diff --git a/drivers/net/ifc/base/ifcvf.h b/drivers/net/ifc/base/ifcvf.h
index a4cb1a4..3eb0bf3 100644
--- a/drivers/net/ifc/base/ifcvf.h
+++ b/drivers/net/ifc/base/ifcvf.h
@@ -12,7 +12,7 @@
 #define IFCVF_SUBSYS_VENDOR_ID	0x8086
 #define IFCVF_SUBSYS_DEVICE_ID	0x001A
 
-#define IFCVF_MAX_QUEUES		1
+#define IFCVF_MAX_QUEUES		32
 #define VIRTIO_F_IOMMU_PLATFORM		33
 
 /* Common configuration */
@@ -153,6 +153,9 @@ struct ifcvf_hw {
 ifcvf_disable_logging(struct ifcvf_hw *hw);
 
 void
+ifcvf_enable_multiqueue(struct ifcvf_hw *hw, u16 nr_queue_pair);
+
+void
 ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid);
 
 u8
diff --git a/drivers/net/ifc/ifcvf_vdpa.c b/drivers/net/ifc/ifcvf_vdpa.c
index 8de9ef1..be71f92 100644
--- a/drivers/net/ifc/ifcvf_vdpa.c
+++ b/drivers/net/ifc/ifcvf_vdpa.c
@@ -994,6 +994,31 @@ struct internal_list {
 }
 
 static int
+ifcvf_set_vring_state(int vid)
+{
+	int did, nr_active_vring, nr_queue_pair;
+	struct internal_list *list;
+
+	nr_active_vring = rte_vhost_get_active_vring_num(vid);
+	if (nr_active_vring == 0) {
+		DRV_LOG(ERR, "No enabled vring");
+		return -1;
+	}
+	nr_queue_pair = (nr_active_vring + 1) / 2;
+
+	did = rte_vhost_get_vdpa_device_id(vid);
+	list = find_internal_resource_by_did(did);
+	if (list == NULL) {
+		DRV_LOG(ERR, "Invalid device id: %d", did);
+		return -1;
+	}
+
+	ifcvf_enable_multiqueue(&list->internal->hw, nr_queue_pair);
+
+	return 0;
+}
+
+static int
 ifcvf_get_notify_area(int vid, int qid, uint64_t *offset, uint64_t *size)
 {
 	int did;
@@ -1062,7 +1087,9 @@ struct internal_list {
 		 1ULL << VHOST_USER_PROTOCOL_F_SLAVE_REQ | \
 		 1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD | \
 		 1ULL << VHOST_USER_PROTOCOL_F_HOST_NOTIFIER | \
-		 1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD)
+		 1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD | \
+		 1ULL << VHOST_USER_PROTOCOL_F_MQ)
+
 static int
 ifcvf_get_protocol_features(int did __rte_unused, uint64_t *features)
 {
@@ -1076,7 +1103,7 @@ struct internal_list {
 	.get_protocol_features = ifcvf_get_protocol_features,
 	.dev_conf = ifcvf_dev_config,
 	.dev_close = ifcvf_dev_close,
-	.set_vring_state = NULL,
+	.set_vring_state = ifcvf_set_vring_state,
 	.set_features = ifcvf_set_features,
 	.migration_done = NULL,
 	.get_vfio_group_fd = ifcvf_get_vfio_group_fd,
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 1/4] vhost: introduce new API to get the active vring number

[Aaron Conole]

Andy Pei <andy.pei@intel.com> writes:

> It's useful for hardware vhost backend (like vDPA devices) to set
> multiqueue configuration accordingly.
>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> Signed-off-by: Andy Pei <andy.pei@intel.com>
> ---

I think there's something wrong with this patch - I see the following
error after all the patches are applied:

   drivers/a715181@@tmp_rte_pmd_ifc@sta/net_ifc_ifcvf_vdpa.c.o: In function `ifcvf_set_vring_state':
   ifcvf_vdpa.c:(.text+0x1157): undefined reference to `rte_vhost_get_active_vring_num'

Looking at the linker line, I see that librte_vhost is being included.

Is it possible that you need to make an export for this?

Travis build:

   https://travis-ci.com/ovsrobot/dpdk/jobs/231687745

Possibly, this is because the robot needs to call the right script to
apply to a -next tree.  I'd be a little surprised, though.

>  lib/librte_vhost/rte_vhost.h | 12 ++++++++++++
>  lib/librte_vhost/vhost.c     | 19 +++++++++++++++++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> index 7fb1729..28811b0 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -525,6 +525,18 @@ int rte_vhost_driver_callback_register(const char *path,
>  uint16_t rte_vhost_get_vring_num(int vid);
>  
>  /**
> + * Get the number of active vrings of the device.
> + *
> + * @param vid
> + *  vhost device ID
> + *
> + * @return
> + *  The number of active vrings, 0 on failure
> + */
> +uint16_t
> +rte_vhost_get_active_vring_num(int vid);
> +
> +/**
>   * Get the virtio net device's ifname, which is the vhost-user socket
>   * file path.
>   *
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index 981837b..c714818 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -674,6 +674,25 @@
>  	return dev->nr_vring;
>  }
>  
> +uint16_t
> +rte_vhost_get_active_vring_num(int vid)
> +{
> +	struct virtio_net *dev = get_device(vid);
> +	struct vhost_virtqueue *vq;
> +	uint16_t qid;
> +
> +	if (dev == NULL)
> +		return 0;
> +
> +	for (qid = 0; qid < dev->nr_vring; qid++) {
> +		vq = dev->virtqueue[qid];
> +		if (!vq->enabled)
> +			break;
> +	}
> +
> +	return qid;
> +}
> +
>  int
>  rte_vhost_get_ifname(int vid, char *buf, size_t len)
>  {

Re: [dpdk-dev] [PATCH 4/4] net/ifcvf: enable mutliqueue support

[Wang, Xiao W]

Hi Andy,

> -----Original Message-----
> From: Pei, Andy
> Sent: Friday, September 6, 2019 11:21 AM
> To: dev@dpdk.org
> Cc: Xu, Rosen <rosen.xu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Bie, Tiwei <tiwei.bie@intel.com>; Wang, Xiao W <xiao.w.wang@intel.com>
> Subject: [PATCH 4/4] net/ifcvf: enable mutliqueue support
> 
> Enable mutliqueue support for ifcvf devices by setting the mutliqueue
> configuration space.
> 
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> Signed-off-by: Andy Pei <andy.pei@intel.com>
> ---
>  drivers/net/ifc/base/ifcvf.c |  8 ++++++++
>  drivers/net/ifc/base/ifcvf.h |  5 ++++-
>  drivers/net/ifc/ifcvf_vdpa.c | 31 +++++++++++++++++++++++++++++--
>  3 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ifc/base/ifcvf.c b/drivers/net/ifc/base/ifcvf.c
> index 6462281..ac09537 100644
> --- a/drivers/net/ifc/base/ifcvf.c
> +++ b/drivers/net/ifc/base/ifcvf.c
> @@ -310,6 +310,14 @@
>  	*(u32 *)(lm_cfg + IFCVF_LM_LOGGING_CTRL) = IFCVF_LM_DISABLE;
>  }
> 
> +void ifcvf_enable_multiqueue(struct ifcvf_hw *hw, u16 nr_queue_pair)
> +{
> +	u8 *mq_cfg;
> +
> +	mq_cfg = hw->mq_cfg;
> +	*(u32 *)mq_cfg = nr_queue_pair;
> +}
> +
>  void
>  ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
>  {
> diff --git a/drivers/net/ifc/base/ifcvf.h b/drivers/net/ifc/base/ifcvf.h
> index a4cb1a4..3eb0bf3 100644
> --- a/drivers/net/ifc/base/ifcvf.h
> +++ b/drivers/net/ifc/base/ifcvf.h
> @@ -12,7 +12,7 @@
>  #define IFCVF_SUBSYS_VENDOR_ID	0x8086
>  #define IFCVF_SUBSYS_DEVICE_ID	0x001A
> 
> -#define IFCVF_MAX_QUEUES		1
> +#define IFCVF_MAX_QUEUES		32

The IFC supports flexible queue resource allocation among VFs, a VF may have
at most 32 queue pair allocated, but the actual number depends on user's
allocation policy. You can just get this number from a register in virtio device
config capability.

BRs,
Xiao

>  #define VIRTIO_F_IOMMU_PLATFORM		33
> 
>  /* Common configuration */
> @@ -153,6 +153,9 @@ struct ifcvf_hw {
>  ifcvf_disable_logging(struct ifcvf_hw *hw);
> 
>  void
> +ifcvf_enable_multiqueue(struct ifcvf_hw *hw, u16 nr_queue_pair);
> +
> +void
>  ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid);
> 
>  u8
> diff --git a/drivers/net/ifc/ifcvf_vdpa.c b/drivers/net/ifc/ifcvf_vdpa.c
> index 8de9ef1..be71f92 100644
> --- a/drivers/net/ifc/ifcvf_vdpa.c
> +++ b/drivers/net/ifc/ifcvf_vdpa.c
> @@ -994,6 +994,31 @@ struct internal_list {
>  }
> 
>  static int
> +ifcvf_set_vring_state(int vid)
> +{
> +	int did, nr_active_vring, nr_queue_pair;
> +	struct internal_list *list;
> +
> +	nr_active_vring = rte_vhost_get_active_vring_num(vid);
> +	if (nr_active_vring == 0) {
> +		DRV_LOG(ERR, "No enabled vring");
> +		return -1;
> +	}
> +	nr_queue_pair = (nr_active_vring + 1) / 2;
> +
> +	did = rte_vhost_get_vdpa_device_id(vid);
> +	list = find_internal_resource_by_did(did);
> +	if (list == NULL) {
> +		DRV_LOG(ERR, "Invalid device id: %d", did);
> +		return -1;
> +	}
> +
> +	ifcvf_enable_multiqueue(&list->internal->hw, nr_queue_pair);
> +
> +	return 0;
> +}
> +
> +static int
>  ifcvf_get_notify_area(int vid, int qid, uint64_t *offset, uint64_t *size)
>  {
>  	int did;
> @@ -1062,7 +1087,9 @@ struct internal_list {
>  		 1ULL << VHOST_USER_PROTOCOL_F_SLAVE_REQ | \
>  		 1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD | \
>  		 1ULL << VHOST_USER_PROTOCOL_F_HOST_NOTIFIER | \
> -		 1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD)
> +		 1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD | \
> +		 1ULL << VHOST_USER_PROTOCOL_F_MQ)
> +
>  static int
>  ifcvf_get_protocol_features(int did __rte_unused, uint64_t *features)
>  {
> @@ -1076,7 +1103,7 @@ struct internal_list {
>  	.get_protocol_features = ifcvf_get_protocol_features,
>  	.dev_conf = ifcvf_dev_config,
>  	.dev_close = ifcvf_dev_close,
> -	.set_vring_state = NULL,
> +	.set_vring_state = ifcvf_set_vring_state,
>  	.set_features = ifcvf_set_features,
>  	.migration_done = NULL,
>  	.get_vfio_group_fd = ifcvf_get_vfio_group_fd,
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH 3/4] vhost: call vDPA callback at the end of vring enable handler

[Wang, Xiao W]

Hi Andy,

> -----Original Message-----
> From: Pei, Andy
> Sent: Friday, September 6, 2019 11:21 AM
> To: dev@dpdk.org
> Cc: Xu, Rosen <rosen.xu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Bie, Tiwei <tiwei.bie@intel.com>; Wang, Xiao W <xiao.w.wang@intel.com>
> Subject: [PATCH 3/4] vhost: call vDPA callback at the end of vring enable
> handler
> 
> vDPA's set_vring_state callback would need to know the virtqueues'
> enable status to configure the hardware.
> 
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> Signed-off-by: Andy Pei <andy.pei@intel.com>
> ---
>  lib/librte_vhost/rte_vdpa.h   | 4 ++--
>  lib/librte_vhost/vhost_user.c | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
> index 9a3deb3..19cf897 100644
> --- a/lib/librte_vhost/rte_vdpa.h
> +++ b/lib/librte_vhost/rte_vdpa.h
> @@ -54,8 +54,8 @@ struct rte_vdpa_dev_ops {
>  	int (*dev_conf)(int vid);
>  	int (*dev_close)(int vid);
> 
> -	/** Enable/disable this vring */
> -	int (*set_vring_state)(int vid, int vring, int state);
> +	/** Enable/disable vring queue pairs */
> +	int (*set_vring_state)(int vid);

If rte_vhost_get_active_vring_num() always needs be called inside set_vring_state callback,
inorder to get the active queue pair number, then why not pass the active queue pair number
directly as a parameter to set_vring_state() ? This could help to avoid exposing new API.

BRs,
Xiao

> 
>  	/** Set features when changed */
>  	int (*set_features)(int vid);
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 0b72648..21028cc 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1346,8 +1346,6 @@ static int vhost_user_set_vring_err(struct virtio_net
> **pdev __rte_unused,
> 
>  	did = dev->vdpa_dev_id;
>  	vdpa_dev = rte_vdpa_get_device(did);
> -	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
> -		vdpa_dev->ops->set_vring_state(dev->vid, index, enable);
> 
>  	if (dev->notify_ops->vring_state_changed)
>  		dev->notify_ops->vring_state_changed(dev->vid,
> @@ -1359,6 +1357,9 @@ static int vhost_user_set_vring_err(struct virtio_net
> **pdev __rte_unused,
> 
>  	dev->virtqueue[index]->enabled = enable;
> 
> +	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
> +		vdpa_dev->ops->set_vring_state(dev->vid);
> +
>  	return RTE_VHOST_MSG_RESULT_OK;
>  }
> 
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH 3/4] vhost: call vDPA callback at the end of vring enable handler

[Pei, Andy]

Yes, I will do this way in v2.

-----Original Message-----
From: Wang, Xiao W 
Sent: Thursday, September 12, 2019 3:42 PM
To: Pei, Andy <andy.pei@intel.com>; dev@dpdk.org
Cc: Xu, Rosen <rosen.xu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>; Liang, Cunming <cunming.liang@intel.com>
Subject: RE: [PATCH 3/4] vhost: call vDPA callback at the end of vring enable handler

Hi Andy,

> -----Original Message-----
> From: Pei, Andy
> Sent: Friday, September 6, 2019 11:21 AM
> To: dev@dpdk.org
> Cc: Xu, Rosen <rosen.xu@intel.com>; Ye, Xiaolong 
> <xiaolong.ye@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Xiao 
> W <xiao.w.wang@intel.com>
> Subject: [PATCH 3/4] vhost: call vDPA callback at the end of vring 
> enable handler
> 
> vDPA's set_vring_state callback would need to know the virtqueues'
> enable status to configure the hardware.
> 
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> Signed-off-by: Andy Pei <andy.pei@intel.com>
> ---
>  lib/librte_vhost/rte_vdpa.h   | 4 ++--
>  lib/librte_vhost/vhost_user.c | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h 
> index 9a3deb3..19cf897 100644
> --- a/lib/librte_vhost/rte_vdpa.h
> +++ b/lib/librte_vhost/rte_vdpa.h
> @@ -54,8 +54,8 @@ struct rte_vdpa_dev_ops {
>  	int (*dev_conf)(int vid);
>  	int (*dev_close)(int vid);
> 
> -	/** Enable/disable this vring */
> -	int (*set_vring_state)(int vid, int vring, int state);
> +	/** Enable/disable vring queue pairs */
> +	int (*set_vring_state)(int vid);

If rte_vhost_get_active_vring_num() always needs be called inside set_vring_state callback, inorder to get the active queue pair number, then why not pass the active queue pair number directly as a parameter to set_vring_state() ? This could help to avoid exposing new API.

BRs,
Xiao

> 
>  	/** Set features when changed */
>  	int (*set_features)(int vid);
> diff --git a/lib/librte_vhost/vhost_user.c 
> b/lib/librte_vhost/vhost_user.c index 0b72648..21028cc 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1346,8 +1346,6 @@ static int vhost_user_set_vring_err(struct 
> virtio_net **pdev __rte_unused,
> 
>  	did = dev->vdpa_dev_id;
>  	vdpa_dev = rte_vdpa_get_device(did);
> -	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
> -		vdpa_dev->ops->set_vring_state(dev->vid, index, enable);
> 
>  	if (dev->notify_ops->vring_state_changed)
>  		dev->notify_ops->vring_state_changed(dev->vid,
> @@ -1359,6 +1357,9 @@ static int vhost_user_set_vring_err(struct 
> virtio_net **pdev __rte_unused,
> 
>  	dev->virtqueue[index]->enabled = enable;
> 
> +	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
> +		vdpa_dev->ops->set_vring_state(dev->vid);
> +
>  	return RTE_VHOST_MSG_RESULT_OK;
>  }
> 
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH 1/4] vhost: introduce new API to get the active vring number

[Pei, Andy]

Hi Aaron,

I try to solve this by avoiding exposing this new API.
Hope this works.

BRs,

-----Original Message-----
From: Aaron Conole [mailto:aconole@redhat.com] 
Sent: Friday, September 6, 2019 9:26 PM
To: Pei, Andy <andy.pei@intel.com>
Cc: dev@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Xiao W <xiao.w.wang@intel.com>
Subject: Re: [dpdk-dev] [PATCH 1/4] vhost: introduce new API to get the active vring number

Andy Pei <andy.pei@intel.com> writes:

> It's useful for hardware vhost backend (like vDPA devices) to set 
> multiqueue configuration accordingly.
>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> Signed-off-by: Andy Pei <andy.pei@intel.com>
> ---

I think there's something wrong with this patch - I see the following error after all the patches are applied:

   drivers/a715181@@tmp_rte_pmd_ifc@sta/net_ifc_ifcvf_vdpa.c.o: In function `ifcvf_set_vring_state':
   ifcvf_vdpa.c:(.text+0x1157): undefined reference to `rte_vhost_get_active_vring_num'

Looking at the linker line, I see that librte_vhost is being included.

Is it possible that you need to make an export for this?

Travis build:

   https://travis-ci.com/ovsrobot/dpdk/jobs/231687745

Possibly, this is because the robot needs to call the right script to apply to a -next tree.  I'd be a little surprised, though.

>  lib/librte_vhost/rte_vhost.h | 12 ++++++++++++
>  lib/librte_vhost/vhost.c     | 19 +++++++++++++++++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/lib/librte_vhost/rte_vhost.h 
> b/lib/librte_vhost/rte_vhost.h index 7fb1729..28811b0 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -525,6 +525,18 @@ int rte_vhost_driver_callback_register(const char 
> *path,  uint16_t rte_vhost_get_vring_num(int vid);
>  
>  /**
> + * Get the number of active vrings of the device.
> + *
> + * @param vid
> + *  vhost device ID
> + *
> + * @return
> + *  The number of active vrings, 0 on failure  */ uint16_t 
> +rte_vhost_get_active_vring_num(int vid);
> +
> +/**
>   * Get the virtio net device's ifname, which is the vhost-user socket
>   * file path.
>   *
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c index 
> 981837b..c714818 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -674,6 +674,25 @@
>  	return dev->nr_vring;
>  }
>  
> +uint16_t
> +rte_vhost_get_active_vring_num(int vid) {
> +	struct virtio_net *dev = get_device(vid);
> +	struct vhost_virtqueue *vq;
> +	uint16_t qid;
> +
> +	if (dev == NULL)
> +		return 0;
> +
> +	for (qid = 0; qid < dev->nr_vring; qid++) {
> +		vq = dev->virtqueue[qid];
> +		if (!vq->enabled)
> +			break;
> +	}
> +
> +	return qid;
> +}
> +
>  int
>  rte_vhost_get_ifname(int vid, char *buf, size_t len)  {