DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v7 0/4] Add virtio support for arm/arm64
@ 2016-02-07 13:51 Santosh Shukla
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 1/4] eal/linux: never check iopl for arm Santosh Shukla
                   ` (4 more replies)
  0 siblings, 5 replies; 25+ messages in thread
From: Santosh Shukla @ 2016-02-07 13:51 UTC (permalink / raw)
  To: dev

Hi,

Patch series to allow access to virtio using vfio interface.
Tested for vfio-noiommu mode for x86_64/arm64{thunderX} platform.
patch series builds successfully for armv7/v8/x86_64/i686.

Note: Rebased on David(s) recent patch series titled "rework virtio for ioport".

Step to enable vfio-noiommu mode:
- modprobe vfio-pci
echo 1 > /sys/module/vfio/parameters/enable_unsafe_*
- then bind
./tools/dpdk_nic_bind.py -b vfio-pci 0000:00:03.0

- Testpmd application to try out for:
./app/testpmd -c 0x3 -n 4 -- -i --portmask=0x0  --nb-cores=1 --port-topology=chained

On host side ping to tapX interface and observe pkt_cnt on guest side.

v6-->v7:
- added support for vfio map/umap/rd/wr method for linuxapp.
- Tested for x86 / arm64{thunderX} vfio mode both.

v5-->v6:
- Removed KDRV_NOIOMMU mode
- patchseries aligned in topic-wise ordered way
- Introduced virtio_io.h; has in/out api, and header file sys/io.h 
- Renamed virtio_vfio_rw.h to virtio_vfio_io.h, renamed ioport_in/out{b,w,l} to
  vfio_in/out{b,w,l}

v4 --> v5:
- Introducing RTE_KDRV_VFIO_NOIOMMU driver mode
- Incorporated v4 review comments, Pl. refer each patchset for review change.

For older version(v4.. v1) patch history, refer [2].

Thanks.
[1] https://github.com/sshukla82/dpdk.git branch virtio-vfio-v6-review
[2] http://comments.gmane.org/gmane.comp.networking.dpdk.devel/31402


Santosh Shukla (4):
  eal/linux: never check iopl for arm
  virtio: Introduce config RTE_VIRTIO_INC_VECTOR
  eal/linux: vfio: ignore mapping for ioport region
  eal/linux: vfio: add pci ioport support

 config/common_linuxapp                     |    1 +
 config/defconfig_arm-armv7a-linuxapp-gcc   |    4 +-
 config/defconfig_arm64-armv8a-linuxapp-gcc |    4 +-
 config/defconfig_i686-native-linuxapp-gcc  |    1 +
 config/defconfig_i686-native-linuxapp-icc  |    1 +
 drivers/net/virtio/Makefile                |    2 +-
 drivers/net/virtio/virtio_rxtx.c           |   16 ++++++-
 drivers/net/virtio/virtio_rxtx.h           |    2 +
 lib/librte_eal/linuxapp/eal/eal.c          |    2 +
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   68 ++++++++++++++++++++++------
 10 files changed, 83 insertions(+), 18 deletions(-)

-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH v7 1/4] eal/linux: never check iopl for arm
  2016-02-07 13:51 [dpdk-dev] [PATCH v7 0/4] Add virtio support for arm/arm64 Santosh Shukla
@ 2016-02-07 13:51 ` Santosh Shukla
  2016-02-18  5:26   ` Santosh Shukla
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR Santosh Shukla
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 25+ messages in thread
From: Santosh Shukla @ 2016-02-07 13:51 UTC (permalink / raw)
  To: dev

iopl() syscall not supported in linux-arm/arm64 so always return 0 value.

Signed-off-by: Santosh Shukla <sshukla@mvista.com>
Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Jan Viktorin <viktorin@rehivetech.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 635ec36..a2a3485 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -715,6 +715,8 @@ rte_eal_iopl_init(void)
 	if (iopl(3) != 0)
 		return -1;
 	return 0;
+#elif defined(RTE_ARCH_ARM) || defined(RTE_ARCH_ARM64)
+	return 0; /* iopl syscall not supported for ARM/ARM64 */
 #else
 	return -1;
 #endif
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR
  2016-02-07 13:51 [dpdk-dev] [PATCH v7 0/4] Add virtio support for arm/arm64 Santosh Shukla
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 1/4] eal/linux: never check iopl for arm Santosh Shukla
@ 2016-02-07 13:51 ` Santosh Shukla
  2016-02-07 21:25   ` Thomas Monjalon
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 3/4] eal/linux: vfio: ignore mapping for ioport region Santosh Shukla
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 25+ messages in thread
From: Santosh Shukla @ 2016-02-07 13:51 UTC (permalink / raw)
  To: dev

- virtio_recv_pkts_vec and other virtio vector friend apis are written for
  sse/avx instructions. For arm64 in particular, virtio vector implementation
  does not exist(todo).

So virtio pmd driver wont build for targets like i686, arm64.  By making
RTE_VIRTIO_INC_VECTOR=n, Driver can build for non-sse/avx targets and will work
in non-vectored virtio mode.

Disabling RTE_VIRTIO_INC_VECTOR config for :

- i686 arch as i686 target config says:
  config/defconfig_i686-native-linuxapp-gcc says "Vectorized PMD is not
  supported on 32-bit".

- armv7/v8 arch.

Signed-off-by: Santosh Shukla <sshukla@mvista.com>
---
 config/common_linuxapp                     |    1 +
 config/defconfig_arm-armv7a-linuxapp-gcc   |    4 +++-
 config/defconfig_arm64-armv8a-linuxapp-gcc |    4 +++-
 config/defconfig_i686-native-linuxapp-gcc  |    1 +
 config/defconfig_i686-native-linuxapp-icc  |    1 +
 drivers/net/virtio/Makefile                |    2 +-
 drivers/net/virtio/virtio_rxtx.c           |   16 +++++++++++++++-
 drivers/net/virtio/virtio_rxtx.h           |    2 ++
 8 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/config/common_linuxapp b/config/common_linuxapp
index 74bc515..8677697 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -274,6 +274,7 @@ CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
 CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
 CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=n
 CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
+CONFIG_RTE_VIRTIO_INC_VECTOR=y
 
 #
 # Compile burst-oriented VMXNET3 PMD driver
diff --git a/config/defconfig_arm-armv7a-linuxapp-gcc b/config/defconfig_arm-armv7a-linuxapp-gcc
index cbebd64..9f852ce 100644
--- a/config/defconfig_arm-armv7a-linuxapp-gcc
+++ b/config/defconfig_arm-armv7a-linuxapp-gcc
@@ -43,6 +43,9 @@ CONFIG_RTE_FORCE_INTRINSICS=y
 CONFIG_RTE_TOOLCHAIN="gcc"
 CONFIG_RTE_TOOLCHAIN_GCC=y
 
+# Disable VIRTIO VECTOR support
+CONFIG_RTE_VIRTIO_INC_VECTOR=n
+
 # ARM doesn't have support for vmware TSC map
 CONFIG_RTE_LIBRTE_EAL_VMWARE_TSC_MAP_SUPPORT=n
 
@@ -70,7 +73,6 @@ CONFIG_RTE_LIBRTE_I40E_PMD=n
 CONFIG_RTE_LIBRTE_IXGBE_PMD=n
 CONFIG_RTE_LIBRTE_MLX4_PMD=n
 CONFIG_RTE_LIBRTE_MPIPE_PMD=n
-CONFIG_RTE_LIBRTE_VIRTIO_PMD=n
 CONFIG_RTE_LIBRTE_VMXNET3_PMD=n
 CONFIG_RTE_LIBRTE_PMD_XENVIRT=n
 CONFIG_RTE_LIBRTE_PMD_BNX2X=n
diff --git a/config/defconfig_arm64-armv8a-linuxapp-gcc b/config/defconfig_arm64-armv8a-linuxapp-gcc
index 504f3ed..1a638b3 100644
--- a/config/defconfig_arm64-armv8a-linuxapp-gcc
+++ b/config/defconfig_arm64-armv8a-linuxapp-gcc
@@ -45,8 +45,10 @@ CONFIG_RTE_TOOLCHAIN_GCC=y
 
 CONFIG_RTE_CACHE_LINE_SIZE=64
 
+# Disable VIRTIO VECTOR support
+CONFIG_RTE_VIRTIO_INC_VECTOR=n
+
 CONFIG_RTE_IXGBE_INC_VECTOR=n
-CONFIG_RTE_LIBRTE_VIRTIO_PMD=n
 CONFIG_RTE_LIBRTE_IVSHMEM=n
 CONFIG_RTE_LIBRTE_FM10K_PMD=n
 CONFIG_RTE_LIBRTE_I40E_PMD=n
diff --git a/config/defconfig_i686-native-linuxapp-gcc b/config/defconfig_i686-native-linuxapp-gcc
index a90de9b..a4b1c49 100644
--- a/config/defconfig_i686-native-linuxapp-gcc
+++ b/config/defconfig_i686-native-linuxapp-gcc
@@ -49,3 +49,4 @@ CONFIG_RTE_LIBRTE_KNI=n
 # Vectorized PMD is not supported on 32-bit
 #
 CONFIG_RTE_IXGBE_INC_VECTOR=n
+CONFIG_RTE_VIRTIO_INC_VECTOR=n
diff --git a/config/defconfig_i686-native-linuxapp-icc b/config/defconfig_i686-native-linuxapp-icc
index c021321..f8eb6ad 100644
--- a/config/defconfig_i686-native-linuxapp-icc
+++ b/config/defconfig_i686-native-linuxapp-icc
@@ -49,3 +49,4 @@ CONFIG_RTE_LIBRTE_KNI=n
 # Vectorized PMD is not supported on 32-bit
 #
 CONFIG_RTE_IXGBE_INC_VECTOR=n
+CONFIG_RTE_VIRTIO_INC_VECTOR=n
diff --git a/drivers/net/virtio/Makefile b/drivers/net/virtio/Makefile
index 43835ba..25a842d 100644
--- a/drivers/net/virtio/Makefile
+++ b/drivers/net/virtio/Makefile
@@ -50,7 +50,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtqueue.c
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_pci.c
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx.c
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_ethdev.c
-SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
+SRCS-$(CONFIG_RTE_VIRTIO_INC_VECTOR) += virtio_rxtx_simple.c
 
 # this lib depends upon:
 DEPDIRS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += lib/librte_eal lib/librte_ether
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 41a1366..d8169d1 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -67,7 +67,9 @@
 #define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
 	ETH_TXQ_FLAGS_NOOFFLOADS)
 
+#ifdef RTE_VIRTIO_INC_VECTOR
 static int use_simple_rxtx;
+#endif
 
 static void
 vq_ring_free_chain(struct virtqueue *vq, uint16_t desc_idx)
@@ -307,12 +309,13 @@ virtio_dev_vring_start(struct virtqueue *vq, int queue_type)
 		nbufs = 0;
 		error = ENOSPC;
 
+#ifdef RTE_VIRTIO_INC_VECTOR
 		if (use_simple_rxtx)
 			for (i = 0; i < vq->vq_nentries; i++) {
 				vq->vq_ring.avail->ring[i] = i;
 				vq->vq_ring.desc[i].flags = VRING_DESC_F_WRITE;
 			}
-
+#endif
 		memset(&vq->fake_mbuf, 0, sizeof(vq->fake_mbuf));
 		for (i = 0; i < RTE_PMD_VIRTIO_RX_MAX_BURST; i++)
 			vq->sw_ring[vq->vq_nentries + i] = &vq->fake_mbuf;
@@ -325,9 +328,11 @@ virtio_dev_vring_start(struct virtqueue *vq, int queue_type)
 			/******************************************
 			*         Enqueue allocated buffers        *
 			*******************************************/
+#ifdef RTE_VIRTIO_INC_VECTOR
 			if (use_simple_rxtx)
 				error = virtqueue_enqueue_recv_refill_simple(vq, m);
 			else
+#endif
 				error = virtqueue_enqueue_recv_refill(vq, m);
 			if (error) {
 				rte_pktmbuf_free(m);
@@ -340,6 +345,7 @@ virtio_dev_vring_start(struct virtqueue *vq, int queue_type)
 
 		PMD_INIT_LOG(DEBUG, "Allocated %d bufs", nbufs);
 	} else if (queue_type == VTNET_TQ) {
+#ifdef RTE_VIRTIO_INC_VECTOR
 		if (use_simple_rxtx) {
 			int mid_idx  = vq->vq_nentries >> 1;
 			for (i = 0; i < mid_idx; i++) {
@@ -357,6 +363,7 @@ virtio_dev_vring_start(struct virtqueue *vq, int queue_type)
 			for (i = mid_idx; i < vq->vq_nentries; i++)
 				vq->vq_ring.avail->ring[i] = i;
 		}
+#endif
 	}
 }
 
@@ -423,7 +430,9 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
 
 	dev->data->rx_queues[queue_idx] = vq;
 
+#ifdef RTE_VIRTIO_INC_VECTOR
 	virtio_rxq_vec_setup(vq);
+#endif
 
 	return 0;
 }
@@ -449,7 +458,10 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 			const struct rte_eth_txconf *tx_conf)
 {
 	uint8_t vtpci_queue_idx = 2 * queue_idx + VTNET_SQ_TQ_QUEUE_IDX;
+
+#ifdef RTE_VIRTIO_INC_VECTOR
 	struct virtio_hw *hw = dev->data->dev_private;
+#endif
 	struct virtqueue *vq;
 	uint16_t tx_free_thresh;
 	int ret;
@@ -462,6 +474,7 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 		return -EINVAL;
 	}
 
+#ifdef RTE_VIRTIO_INC_VECTOR
 	/* Use simple rx/tx func if single segment and no offloads */
 	if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
 	     !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
@@ -470,6 +483,7 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 		dev->rx_pkt_burst = virtio_recv_pkts_vec;
 		use_simple_rxtx = 1;
 	}
+#endif
 
 	ret = virtio_dev_queue_setup(dev, VTNET_TQ, queue_idx, vtpci_queue_idx,
 			nb_desc, socket_id, &vq);
diff --git a/drivers/net/virtio/virtio_rxtx.h b/drivers/net/virtio/virtio_rxtx.h
index 831e492..9be1378 100644
--- a/drivers/net/virtio/virtio_rxtx.h
+++ b/drivers/net/virtio/virtio_rxtx.h
@@ -33,7 +33,9 @@
 
 #define RTE_PMD_VIRTIO_RX_MAX_BURST 64
 
+#ifdef RTE_VIRTIO_INC_VECTOR
 int virtio_rxq_vec_setup(struct virtqueue *rxq);
 
 int virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
 	struct rte_mbuf *m);
+#endif
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH v7 3/4] eal/linux: vfio: ignore mapping for ioport region
  2016-02-07 13:51 [dpdk-dev] [PATCH v7 0/4] Add virtio support for arm/arm64 Santosh Shukla
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 1/4] eal/linux: never check iopl for arm Santosh Shukla
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR Santosh Shukla
@ 2016-02-07 13:51 ` Santosh Shukla
  2016-02-08  9:15   ` Burakov, Anatoly
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 4/4] eal/linux: vfio: add pci ioport support Santosh Shukla
  2016-02-21 14:17 ` [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64 Santosh Shukla
  4 siblings, 1 reply; 25+ messages in thread
From: Santosh Shukla @ 2016-02-07 13:51 UTC (permalink / raw)
  To: dev

vfio_pci_mmap() try to map all pci bars. ioport region are not mapped in
vfio/kernel so ignore mmaping for ioport.

Signed-off-by: Santosh Shukla <sshukla@mvista.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index ffa2dd0..4832313 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -659,6 +659,7 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
 	struct pci_map *maps;
 	uint32_t msix_table_offset = 0;
 	uint32_t msix_table_size = 0;
+	uint32_t ioport_bar;
 
 	dev->intr_handle.fd = -1;
 	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
@@ -853,6 +854,25 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
 			return -1;
 		}
 
+		/* chk for io port region */
+		ret = pread64(vfio_dev_fd, &ioport_bar, sizeof(ioport_bar),
+			      VFIO_GET_REGION_ADDR(VFIO_PCI_CONFIG_REGION_INDEX)
+			      + PCI_BASE_ADDRESS_0 + i*4);
+
+		if (ret != sizeof(ioport_bar)) {
+			RTE_LOG(ERR, EAL,
+				"Cannot read command (%x) from config space!\n",
+				PCI_BASE_ADDRESS_0 + i*4);
+			return -1;
+		}
+
+		if (ioport_bar & PCI_BASE_ADDRESS_SPACE_IO) {
+			RTE_LOG(INFO, EAL,
+				"Ignore mapping IO port bar(%d) addr: %x\n",
+				 i, ioport_bar);
+			continue;
+		}
+
 		/* skip non-mmapable BARs */
 		if ((reg.flags & VFIO_REGION_INFO_FLAG_MMAP) == 0)
 			continue;
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH v7 4/4] eal/linux: vfio: add pci ioport support
  2016-02-07 13:51 [dpdk-dev] [PATCH v7 0/4] Add virtio support for arm/arm64 Santosh Shukla
                   ` (2 preceding siblings ...)
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 3/4] eal/linux: vfio: ignore mapping for ioport region Santosh Shukla
@ 2016-02-07 13:51 ` Santosh Shukla
  2016-02-08  8:51   ` David Marchand
  2016-02-21 14:17 ` [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64 Santosh Shukla
  4 siblings, 1 reply; 25+ messages in thread
From: Santosh Shukla @ 2016-02-07 13:51 UTC (permalink / raw)
  To: dev

Include vfio map/unmap/rd/wr support for pci ioport.

Signed-off-by: Santosh Shukla <sshukla@mvista.com>
---
v7:

- This is enhancement patch for vfio map/rd/wr, rebased on top of David(s) -
    "Rework ioport for virtio" patchset. For more information about api, refer
    patch [1].
[1]  http://dpdk.org/dev/patchwork/patch/10426/

 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   48 ++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index 4832313..d83ece5 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -74,6 +74,7 @@ EAL_REGISTER_TAILQ(rte_vfio_tailq)
 #define VFIO_GROUP_FMT "/dev/vfio/%u"
 #define VFIO_NOIOMMU_GROUP_FMT "/dev/vfio/noiommu-%u"
 #define VFIO_GET_REGION_ADDR(x) ((uint64_t) x << 40ULL)
+#define VFIO_GET_REGION_IDX(x) (x >> 40)
 
 /* per-process VFIO config */
 static struct vfio_config vfio_cfg;
@@ -999,37 +1000,56 @@ int
 pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
 		    struct rte_pci_ioport *p)
 {
-	RTE_SET_USED(dev);
-	RTE_SET_USED(bar);
-	RTE_SET_USED(p);
-	return -1;
+	if (bar < VFIO_PCI_BAR0_REGION_INDEX ||
+	    bar > VFIO_PCI_BAR5_REGION_INDEX) {
+		RTE_LOG(ERR, EAL, "invalid bar (%d)!\n", bar);
+		return -1;
+	}
+
+	p = rte_zmalloc("VFIO_IOPORT", sizeof(*p), 0);
+	if (p == NULL) {
+		RTE_LOG(ERR, EAL, "cannot alloc vfio ioport mem\n");
+		return -1;
+	}
+
+	p->dev = dev;
+	p->offset = VFIO_GET_REGION_ADDR(bar);
+	return 0;
 }
 
 void
 pci_vfio_ioport_read(struct rte_pci_ioport *p,
 		     void *data, size_t len, off_t offset)
 {
-	RTE_SET_USED(p);
-	RTE_SET_USED(data);
-	RTE_SET_USED(len);
-	RTE_SET_USED(offset);
+	const struct rte_intr_handle *intr_handle = &p->dev->intr_handle;
+	if (pread64(intr_handle->vfio_dev_fd, data,
+		    len, p->offset + offset) <= 0)
+		RTE_LOG(ERR, EAL,
+			"Can't read from PCI bar (%" PRIu64 ") : offset (%x)\n",
+			VFIO_GET_REGION_IDX(p->offset), (int)offset);
 }
 
 void
 pci_vfio_ioport_write(struct rte_pci_ioport *p,
 		      const void *data, size_t len, off_t offset)
 {
-	RTE_SET_USED(p);
-	RTE_SET_USED(data);
-	RTE_SET_USED(len);
-	RTE_SET_USED(offset);
+	const struct rte_intr_handle *intr_handle = &p->dev->intr_handle;
+	if (pwrite64(intr_handle->vfio_dev_fd, data,
+		     len, p->offset + offset) <= 0)
+		RTE_LOG(ERR, EAL,
+			"Can't write to PCI bar (%" PRIu64 ") : offset (%x)\n",
+			VFIO_GET_REGION_IDX(p->offset), (int)offset);
 }
 
 int
 pci_vfio_ioport_unmap(struct rte_pci_ioport *p)
 {
-	RTE_SET_USED(p);
-	return -1;
+	if (p == NULL)
+		return -1;
+	else {
+		rte_free(p);
+		return 0;
+	}
 }
 
 int
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR Santosh Shukla
@ 2016-02-07 21:25   ` Thomas Monjalon
  2016-02-08  5:45     ` Santosh Shukla
  0 siblings, 1 reply; 25+ messages in thread
From: Thomas Monjalon @ 2016-02-07 21:25 UTC (permalink / raw)
  To: Santosh Shukla, huawei.xie, yuanhan.liu; +Cc: dev

2016-02-07 19:21, Santosh Shukla:
> - virtio_recv_pkts_vec and other virtio vector friend apis are written for
>   sse/avx instructions. For arm64 in particular, virtio vector implementation
>   does not exist(todo).
> 
> So virtio pmd driver wont build for targets like i686, arm64.  By making
> RTE_VIRTIO_INC_VECTOR=n, Driver can build for non-sse/avx targets and will work
> in non-vectored virtio mode.
> 
> Disabling RTE_VIRTIO_INC_VECTOR config for :
> 
> - i686 arch as i686 target config says:
>   config/defconfig_i686-native-linuxapp-gcc says "Vectorized PMD is not
>   supported on 32-bit".
> 
> - armv7/v8 arch.

Yes it can be useful to disable vector optimizations, but it should done
at runtime, not a compilation option. I know it is already wrongly configured
at compilation for other drivers, we should fix them.

Here, you want to avoid SSE/AVX code on ARM. So we should just add the
appropriate ifdefs. Adding a compilation option does not prevent from enabling
it on ARM or old x86 which do not support these instructions.

Please virtio maintainers, we need to fix this code. Thanks

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR
  2016-02-07 21:25   ` Thomas Monjalon
@ 2016-02-08  5:45     ` Santosh Shukla
       [not found]       ` <CAAyOgsZO+6+kFZZZM203fPR3AmVYB0v7j3-f+DawZOCuR-AVvQ@mail.gmail.com>
  0 siblings, 1 reply; 25+ messages in thread
From: Santosh Shukla @ 2016-02-08  5:45 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev

On Mon, Feb 8, 2016 at 2:55 AM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> 2016-02-07 19:21, Santosh Shukla:
>> - virtio_recv_pkts_vec and other virtio vector friend apis are written for
>>   sse/avx instructions. For arm64 in particular, virtio vector implementation
>>   does not exist(todo).
>>
>> So virtio pmd driver wont build for targets like i686, arm64.  By making
>> RTE_VIRTIO_INC_VECTOR=n, Driver can build for non-sse/avx targets and will work
>> in non-vectored virtio mode.
>>
>> Disabling RTE_VIRTIO_INC_VECTOR config for :
>>
>> - i686 arch as i686 target config says:
>>   config/defconfig_i686-native-linuxapp-gcc says "Vectorized PMD is not
>>   supported on 32-bit".
>>
>> - armv7/v8 arch.
>
> Yes it can be useful to disable vector optimizations, but it should done
> at runtime, not a compilation option. I know it is already wrongly configured
> at compilation for other drivers, we should fix them.
>

Can't we consider this separate topic. My intent is virtio works for arm.

> Here, you want to avoid SSE/AVX code on ARM. So we should just add the
> appropriate ifdefs. Adding a compilation option does not prevent from enabling
> it on ARM or old x86 which do not support these instructions.
>

By disabling VIRTIO_INC_VEC, compiler wont build
virtio_recv_pkts_vec(), so wont generate SSE/AVX code. Adding ifdef
for other arch example arm, is next step. Vector instruction for arm
are not fully supported, Its a todolist (Pl. refer my early v1/2
cover-letter), We'll add that after virtio functionally works for arm.

> Please virtio maintainers, we need to fix this code. Thanks

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 4/4] eal/linux: vfio: add pci ioport support
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 4/4] eal/linux: vfio: add pci ioport support Santosh Shukla
@ 2016-02-08  8:51   ` David Marchand
  2016-02-08  9:40     ` Santosh Shukla
  0 siblings, 1 reply; 25+ messages in thread
From: David Marchand @ 2016-02-08  8:51 UTC (permalink / raw)
  To: Santosh Shukla; +Cc: dev

On Sun, Feb 7, 2016 at 2:51 PM, Santosh Shukla <sshukla@mvista.com> wrote:
> @@ -999,37 +1000,56 @@ int
>  pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
>                     struct rte_pci_ioport *p)

p is passed as a value, not a reference ...

>  {
> -       RTE_SET_USED(dev);
> -       RTE_SET_USED(bar);
> -       RTE_SET_USED(p);
> -       return -1;
> +       if (bar < VFIO_PCI_BAR0_REGION_INDEX ||
> +           bar > VFIO_PCI_BAR5_REGION_INDEX) {
> +               RTE_LOG(ERR, EAL, "invalid bar (%d)!\n", bar);
> +               return -1;
> +       }
> +
> +       p = rte_zmalloc("VFIO_IOPORT", sizeof(*p), 0);

... so I don't think this allocation does what you expected.

Anyway, you don't need to allocate a rte_pci_ioport object with current api.
You already have a valid object passed by caller.
You only need to initialise it.


> +       if (p == NULL) {
> +               RTE_LOG(ERR, EAL, "cannot alloc vfio ioport mem\n");
> +               return -1;
> +       }
> +
> +       p->dev = dev;

Does not hurt to do this, but p->dev is already set by caller on ret
== 0 (rte_eal_pci_ioport_map).


>
>  void
>  pci_vfio_ioport_read(struct rte_pci_ioport *p,
>                      void *data, size_t len, off_t offset)
>  {
> -       RTE_SET_USED(p);
> -       RTE_SET_USED(data);
> -       RTE_SET_USED(len);
> -       RTE_SET_USED(offset);
> +       const struct rte_intr_handle *intr_handle = &p->dev->intr_handle;

Missing blank line between declaration and code.

> +       if (pread64(intr_handle->vfio_dev_fd, data,
> +                   len, p->offset + offset) <= 0)
> +               RTE_LOG(ERR, EAL,
> +                       "Can't read from PCI bar (%" PRIu64 ") : offset (%x)\n",
> +                       VFIO_GET_REGION_IDX(p->offset), (int)offset);
>  }
>
>  void
>  pci_vfio_ioport_write(struct rte_pci_ioport *p,
>                       const void *data, size_t len, off_t offset)
>  {
> -       RTE_SET_USED(p);
> -       RTE_SET_USED(data);
> -       RTE_SET_USED(len);
> -       RTE_SET_USED(offset);
> +       const struct rte_intr_handle *intr_handle = &p->dev->intr_handle;

Idem.

> +       if (pwrite64(intr_handle->vfio_dev_fd, data,
> +                    len, p->offset + offset) <= 0)
> +               RTE_LOG(ERR, EAL,
> +                       "Can't write to PCI bar (%" PRIu64 ") : offset (%x)\n",
> +                       VFIO_GET_REGION_IDX(p->offset), (int)offset);
>  }
>
>  int
>  pci_vfio_ioport_unmap(struct rte_pci_ioport *p)
>  {
> -       RTE_SET_USED(p);
> -       return -1;
> +       if (p == NULL)
> +               return -1;
> +       else {
> +               rte_free(p);
> +               return 0;
> +       }
>  }

Since you have nothing to allocate, nothing to free here ?


-- 
David Marchand

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 3/4] eal/linux: vfio: ignore mapping for ioport region
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 3/4] eal/linux: vfio: ignore mapping for ioport region Santosh Shukla
@ 2016-02-08  9:15   ` Burakov, Anatoly
  2016-02-18  5:26     ` Santosh Shukla
  0 siblings, 1 reply; 25+ messages in thread
From: Burakov, Anatoly @ 2016-02-08  9:15 UTC (permalink / raw)
  To: Santosh Shukla, dev

> vfio_pci_mmap() try to map all pci bars. ioport region are not mapped in
> vfio/kernel so ignore mmaping for ioport.
> 
> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> index ffa2dd0..4832313 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> @@ -659,6 +659,7 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
>  	struct pci_map *maps;
>  	uint32_t msix_table_offset = 0;
>  	uint32_t msix_table_size = 0;
> +	uint32_t ioport_bar;
> 
>  	dev->intr_handle.fd = -1;
>  	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; @@ -853,6
> +854,25 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
>  			return -1;
>  		}
> 
> +		/* chk for io port region */
> +		ret = pread64(vfio_dev_fd, &ioport_bar, sizeof(ioport_bar),
> +
> VFIO_GET_REGION_ADDR(VFIO_PCI_CONFIG_REGION_INDEX)
> +			      + PCI_BASE_ADDRESS_0 + i*4);
> +
> +		if (ret != sizeof(ioport_bar)) {
> +			RTE_LOG(ERR, EAL,
> +				"Cannot read command (%x) from config
> space!\n",
> +				PCI_BASE_ADDRESS_0 + i*4);
> +			return -1;
> +		}
> +
> +		if (ioport_bar & PCI_BASE_ADDRESS_SPACE_IO) {
> +			RTE_LOG(INFO, EAL,
> +				"Ignore mapping IO port bar(%d) addr:
> %x\n",
> +				 i, ioport_bar);
> +			continue;
> +		}
> +
>  		/* skip non-mmapable BARs */
>  		if ((reg.flags & VFIO_REGION_INFO_FLAG_MMAP) == 0)
>  			continue;
> --
> 1.7.9.5

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 4/4] eal/linux: vfio: add pci ioport support
  2016-02-08  8:51   ` David Marchand
@ 2016-02-08  9:40     ` Santosh Shukla
  0 siblings, 0 replies; 25+ messages in thread
From: Santosh Shukla @ 2016-02-08  9:40 UTC (permalink / raw)
  To: David Marchand; +Cc: dev

On Mon, Feb 8, 2016 at 2:21 PM, David Marchand <david.marchand@6wind.com> wrote:
> On Sun, Feb 7, 2016 at 2:51 PM, Santosh Shukla <sshukla@mvista.com> wrote:
>> @@ -999,37 +1000,56 @@ int
>>  pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
>>                     struct rte_pci_ioport *p)
>
> p is passed as a value, not a reference ...
>
>>  {
>> -       RTE_SET_USED(dev);
>> -       RTE_SET_USED(bar);
>> -       RTE_SET_USED(p);
>> -       return -1;
>> +       if (bar < VFIO_PCI_BAR0_REGION_INDEX ||
>> +           bar > VFIO_PCI_BAR5_REGION_INDEX) {
>> +               RTE_LOG(ERR, EAL, "invalid bar (%d)!\n", bar);
>> +               return -1;
>> +       }
>> +
>> +       p = rte_zmalloc("VFIO_IOPORT", sizeof(*p), 0);
>
> ... so I don't think this allocation does what you expected.
>
> Anyway, you don't need to allocate a rte_pci_ioport object with current api.
> You already have a valid object passed by caller.
> You only need to initialise it.
>
>
>> +       if (p == NULL) {
>> +               RTE_LOG(ERR, EAL, "cannot alloc vfio ioport mem\n");
>> +               return -1;
>> +       }
>> +
>> +       p->dev = dev;
>
> Does not hurt to do this, but p->dev is already set by caller on ret
> == 0 (rte_eal_pci_ioport_map).
>
>

so far I didn't found caller setting p->dev (looked at virtio pmd
driver), In-fact I tried to use w/o setting p->dev=dev and application
crashed, stating segfault :).

So I am keeping this one in next revision.
>>
>>  void
>>  pci_vfio_ioport_read(struct rte_pci_ioport *p,
>>                      void *data, size_t len, off_t offset)
>>  {
>> -       RTE_SET_USED(p);
>> -       RTE_SET_USED(data);
>> -       RTE_SET_USED(len);
>> -       RTE_SET_USED(offset);
>> +       const struct rte_intr_handle *intr_handle = &p->dev->intr_handle;
>
> Missing blank line between declaration and code.

checkpatch.sh didn't ntocied though, which all param you use in
checkpatch, I just do
./checkpatch.sh --no-tree *.patch

>
>> +       if (pread64(intr_handle->vfio_dev_fd, data,
>> +                   len, p->offset + offset) <= 0)
>> +               RTE_LOG(ERR, EAL,
>> +                       "Can't read from PCI bar (%" PRIu64 ") : offset (%x)\n",
>> +                       VFIO_GET_REGION_IDX(p->offset), (int)offset);
>>  }
>>
>>  void
>>  pci_vfio_ioport_write(struct rte_pci_ioport *p,
>>                       const void *data, size_t len, off_t offset)
>>  {
>> -       RTE_SET_USED(p);
>> -       RTE_SET_USED(data);
>> -       RTE_SET_USED(len);
>> -       RTE_SET_USED(offset);
>> +       const struct rte_intr_handle *intr_handle = &p->dev->intr_handle;
>
> Idem.
>
>> +       if (pwrite64(intr_handle->vfio_dev_fd, data,
>> +                    len, p->offset + offset) <= 0)
>> +               RTE_LOG(ERR, EAL,
>> +                       "Can't write to PCI bar (%" PRIu64 ") : offset (%x)\n",
>> +                       VFIO_GET_REGION_IDX(p->offset), (int)offset);
>>  }
>>
>>  int
>>  pci_vfio_ioport_unmap(struct rte_pci_ioport *p)
>>  {
>> -       RTE_SET_USED(p);
>> -       return -1;
>> +       if (p == NULL)
>> +               return -1;
>> +       else {
>> +               rte_free(p);
>> +               return 0;
>> +       }
>>  }
>
> Since you have nothing to allocate, nothing to free here ?
>

Removed, Thanks
>
> --
> David Marchand

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR
       [not found]           ` <CAAyOgsaT5TcsPfum8x6yzAJAz=5N+c5QebEn7KCyJn7oK=VMsw@mail.gmail.com>
@ 2016-02-16  3:05             ` Yuanhan Liu
  2016-02-19  4:46               ` Santosh Shukla
  0 siblings, 1 reply; 25+ messages in thread
From: Yuanhan Liu @ 2016-02-16  3:05 UTC (permalink / raw)
  To: Santosh Shukla, Xie, Huawei; +Cc: dpdk

On Mon, Feb 15, 2016 at 04:48:36PM +0530, Santosh Shukla wrote:
> Hi Yuanhan,
> 
> On Mon, Feb 15, 2016 at 4:27 PM, Yuanhan Liu
> <yuanhan.liu@linux.intel.com> wrote:
> > On Mon, Feb 15, 2016 at 03:22:11PM +0530, Santosh Shukla wrote:
> >> Hi Yuanhan,
> >>
> >> I guess you are back from vacation.
> >>
> >> Can you pl. review this patch, Except this patch, rest of patches
> >> received ack-by:
> >
> > I had a quick glimpse of the comments from Thomas: he made a good point.
> > I will have a deeper thought tomorrow, to see what I can do to fix it.
> >
> 
> I agree to what Thomas pointed out about runtime mode switch (vectored
> vs non-vectored). I have a proposal in my mind and Like to know you
> opinion:
> 
> - need for apis like is_arch_support_vec().
> 
> if (is_arch_support_vec())
>          simpple_xxxx = 1 /* Switch code path to vector mode */
> else
>          simple_xxxx = 0  /* Switch code path to non-vector mode */
> 
> That api should reside to arch file. i.e.. arch like i686/arm{for
> implementation not exist so say no supported} will return 0 and for
> x86_64 = 1

I was thinking that Thomas meant to something like below (like what
we did at rte_memcpy.h):

    #ifdef RTE_MACHINE_CPUFLAG_SSE (or whatever)
    
        /* with vec here */

    #else
    
        /* without vec here */

    #endif

I mean, you have to bypass the build first; otherwise, you can't
go that further to runtime, right?


Huawei, since it's your patch introduced such issue, mind to fix
it?

	--yliu
> 
> Does this make sense?
> 
> Thanks
> >         --yliu
> >>
> >> Thanks
> >>
> >> On Mon, Feb 8, 2016 at 11:15 AM, Santosh Shukla <sshukla@mvista.com> wrote:
> >> > On Mon, Feb 8, 2016 at 2:55 AM, Thomas Monjalon
> >> > <thomas.monjalon@6wind.com> wrote:
> >> >> 2016-02-07 19:21, Santosh Shukla:
> >> >>> - virtio_recv_pkts_vec and other virtio vector friend apis are written for
> >> >>>   sse/avx instructions. For arm64 in particular, virtio vector implementation
> >> >>>   does not exist(todo).
> >> >>>
> >> >>> So virtio pmd driver wont build for targets like i686, arm64.  By making
> >> >>> RTE_VIRTIO_INC_VECTOR=n, Driver can build for non-sse/avx targets and will work
> >> >>> in non-vectored virtio mode.
> >> >>>
> >> >>> Disabling RTE_VIRTIO_INC_VECTOR config for :
> >> >>>
> >> >>> - i686 arch as i686 target config says:
> >> >>>   config/defconfig_i686-native-linuxapp-gcc says "Vectorized PMD is not
> >> >>>   supported on 32-bit".
> >> >>>
> >> >>> - armv7/v8 arch.
> >> >>
> >> >> Yes it can be useful to disable vector optimizations, but it should done
> >> >> at runtime, not a compilation option. I know it is already wrongly configured
> >> >> at compilation for other drivers, we should fix them.
> >> >>
> >> >
> >> > Can't we consider this separate topic. My intent is virtio works for arm.
> >> >
> >> >> Here, you want to avoid SSE/AVX code on ARM. So we should just add the
> >> >> appropriate ifdefs. Adding a compilation option does not prevent from enabling
> >> >> it on ARM or old x86 which do not support these instructions.
> >> >>
> >> >
> >> > By disabling VIRTIO_INC_VEC, compiler wont build
> >> > virtio_recv_pkts_vec(), so wont generate SSE/AVX code. Adding ifdef
> >> > for other arch example arm, is next step. Vector instruction for arm
> >> > are not fully supported, Its a todolist (Pl. refer my early v1/2
> >> > cover-letter), We'll add that after virtio functionally works for arm.
> >> >
> >> >> Please virtio maintainers, we need to fix this code. Thanks

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 3/4] eal/linux: vfio: ignore mapping for ioport region
  2016-02-08  9:15   ` Burakov, Anatoly
@ 2016-02-18  5:26     ` Santosh Shukla
  0 siblings, 0 replies; 25+ messages in thread
From: Santosh Shukla @ 2016-02-18  5:26 UTC (permalink / raw)
  To: Burakov, Anatoly; +Cc: dev

On Mon, Feb 8, 2016 at 2:45 PM, Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>> vfio_pci_mmap() try to map all pci bars. ioport region are not mapped in
>> vfio/kernel so ignore mmaping for ioport.
>>
>> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
>> ---
>>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> index ffa2dd0..4832313 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
>> @@ -659,6 +659,7 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
>>       struct pci_map *maps;
>>       uint32_t msix_table_offset = 0;
>>       uint32_t msix_table_size = 0;
>> +     uint32_t ioport_bar;
>>
>>       dev->intr_handle.fd = -1;
>>       dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; @@ -853,6
>> +854,25 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
>>                       return -1;
>>               }
>>
>> +             /* chk for io port region */
>> +             ret = pread64(vfio_dev_fd, &ioport_bar, sizeof(ioport_bar),
>> +
>> VFIO_GET_REGION_ADDR(VFIO_PCI_CONFIG_REGION_INDEX)
>> +                           + PCI_BASE_ADDRESS_0 + i*4);
>> +
>> +             if (ret != sizeof(ioport_bar)) {
>> +                     RTE_LOG(ERR, EAL,
>> +                             "Cannot read command (%x) from config
>> space!\n",
>> +                             PCI_BASE_ADDRESS_0 + i*4);
>> +                     return -1;
>> +             }
>> +
>> +             if (ioport_bar & PCI_BASE_ADDRESS_SPACE_IO) {
>> +                     RTE_LOG(INFO, EAL,
>> +                             "Ignore mapping IO port bar(%d) addr:
>> %x\n",
>> +                              i, ioport_bar);
>> +                     continue;
>> +             }
>> +
>>               /* skip non-mmapable BARs */
>>               if ((reg.flags & VFIO_REGION_INFO_FLAG_MMAP) == 0)
>>                       continue;
>> --
>> 1.7.9.5
>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Merge request for this one too..

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 1/4] eal/linux: never check iopl for arm
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 1/4] eal/linux: never check iopl for arm Santosh Shukla
@ 2016-02-18  5:26   ` Santosh Shukla
  0 siblings, 0 replies; 25+ messages in thread
From: Santosh Shukla @ 2016-02-18  5:26 UTC (permalink / raw)
  To: dev

On Sun, Feb 7, 2016 at 7:21 PM, Santosh Shukla <sshukla@mvista.com> wrote:
> iopl() syscall not supported in linux-arm/arm64 so always return 0 value.
>
> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
> Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Jan Viktorin <viktorin@rehivetech.com>
> Acked-by: David Marchand <david.marchand@6wind.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 635ec36..a2a3485 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -715,6 +715,8 @@ rte_eal_iopl_init(void)
>         if (iopl(3) != 0)
>                 return -1;
>         return 0;
> +#elif defined(RTE_ARCH_ARM) || defined(RTE_ARCH_ARM64)
> +       return 0; /* iopl syscall not supported for ARM/ARM64 */
>  #else
>         return -1;
>  #endif
> --
> 1.7.9.5
>

Pl. merge this one too. Thanks

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR
  2016-02-16  3:05             ` Yuanhan Liu
@ 2016-02-19  4:46               ` Santosh Shukla
  2016-02-19  6:42                 ` Yuanhan Liu
  0 siblings, 1 reply; 25+ messages in thread
From: Santosh Shukla @ 2016-02-19  4:46 UTC (permalink / raw)
  To: Yuanhan Liu; +Cc: dpdk

On Tue, Feb 16, 2016 at 8:35 AM, Yuanhan Liu
<yuanhan.liu@linux.intel.com> wrote:
> On Mon, Feb 15, 2016 at 04:48:36PM +0530, Santosh Shukla wrote:
>> Hi Yuanhan,
>>
>> On Mon, Feb 15, 2016 at 4:27 PM, Yuanhan Liu
>> <yuanhan.liu@linux.intel.com> wrote:
>> > On Mon, Feb 15, 2016 at 03:22:11PM +0530, Santosh Shukla wrote:
>> >> Hi Yuanhan,
>> >>
>> >> I guess you are back from vacation.
>> >>
>> >> Can you pl. review this patch, Except this patch, rest of patches
>> >> received ack-by:
>> >
>> > I had a quick glimpse of the comments from Thomas: he made a good point.
>> > I will have a deeper thought tomorrow, to see what I can do to fix it.
>> >
>>
>> I agree to what Thomas pointed out about runtime mode switch (vectored
>> vs non-vectored). I have a proposal in my mind and Like to know you
>> opinion:
>>
>> - need for apis like is_arch_support_vec().
>>
>> if (is_arch_support_vec())
>>          simpple_xxxx = 1 /* Switch code path to vector mode */
>> else
>>          simple_xxxx = 0  /* Switch code path to non-vector mode */
>>
>> That api should reside to arch file. i.e.. arch like i686/arm{for
>> implementation not exist so say no supported} will return 0 and for
>> x86_64 = 1
>
> I was thinking that Thomas meant to something like below (like what
> we did at rte_memcpy.h):
>
>     #ifdef RTE_MACHINE_CPUFLAG_SSE (or whatever)
>
>         /* with vec here */
>
>     #else
>
>         /* without vec here */
>
>     #endif
>
> I mean, you have to bypass the build first; otherwise, you can't
> go that further to runtime, right?
>

I meant: move virtio_recv_pkt_vec() implementation in
lib/libeal_rte/xx/include/arch/xx/virtio_vec.h. virtio driver to check
for CPUFLAG supported or not and then use _recv_pkt() call back
function from arch files. This approach will avoid #ifdef ARCH
clutter.

This patch is blocking virtio-for-arm entry which is floating close to
month or so, if no taker for this topic then pl. let me know, I'll
propose a patch. Thanks!

>
> Huawei, since it's your patch introduced such issue, mind to fix
> it?
>
>         --yliu
>>
>> Does this make sense?
>>
>> Thanks
>> >         --yliu
>> >>
>> >> Thanks
>> >>
>> >> On Mon, Feb 8, 2016 at 11:15 AM, Santosh Shukla <sshukla@mvista.com> wrote:
>> >> > On Mon, Feb 8, 2016 at 2:55 AM, Thomas Monjalon
>> >> > <thomas.monjalon@6wind.com> wrote:
>> >> >> 2016-02-07 19:21, Santosh Shukla:
>> >> >>> - virtio_recv_pkts_vec and other virtio vector friend apis are written for
>> >> >>>   sse/avx instructions. For arm64 in particular, virtio vector implementation
>> >> >>>   does not exist(todo).
>> >> >>>
>> >> >>> So virtio pmd driver wont build for targets like i686, arm64.  By making
>> >> >>> RTE_VIRTIO_INC_VECTOR=n, Driver can build for non-sse/avx targets and will work
>> >> >>> in non-vectored virtio mode.
>> >> >>>
>> >> >>> Disabling RTE_VIRTIO_INC_VECTOR config for :
>> >> >>>
>> >> >>> - i686 arch as i686 target config says:
>> >> >>>   config/defconfig_i686-native-linuxapp-gcc says "Vectorized PMD is not
>> >> >>>   supported on 32-bit".
>> >> >>>
>> >> >>> - armv7/v8 arch.
>> >> >>
>> >> >> Yes it can be useful to disable vector optimizations, but it should done
>> >> >> at runtime, not a compilation option. I know it is already wrongly configured
>> >> >> at compilation for other drivers, we should fix them.
>> >> >>
>> >> >
>> >> > Can't we consider this separate topic. My intent is virtio works for arm.
>> >> >
>> >> >> Here, you want to avoid SSE/AVX code on ARM. So we should just add the
>> >> >> appropriate ifdefs. Adding a compilation option does not prevent from enabling
>> >> >> it on ARM or old x86 which do not support these instructions.
>> >> >>
>> >> >
>> >> > By disabling VIRTIO_INC_VEC, compiler wont build
>> >> > virtio_recv_pkts_vec(), so wont generate SSE/AVX code. Adding ifdef
>> >> > for other arch example arm, is next step. Vector instruction for arm
>> >> > are not fully supported, Its a todolist (Pl. refer my early v1/2
>> >> > cover-letter), We'll add that after virtio functionally works for arm.
>> >> >
>> >> >> Please virtio maintainers, we need to fix this code. Thanks

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR
  2016-02-19  4:46               ` Santosh Shukla
@ 2016-02-19  6:42                 ` Yuanhan Liu
  2016-02-22  2:03                   ` Xie, Huawei
  0 siblings, 1 reply; 25+ messages in thread
From: Yuanhan Liu @ 2016-02-19  6:42 UTC (permalink / raw)
  To: Santosh Shukla, Xie, Huawei; +Cc: dpdk

On Fri, Feb 19, 2016 at 10:16:42AM +0530, Santosh Shukla wrote:
> On Tue, Feb 16, 2016 at 8:35 AM, Yuanhan Liu
> <yuanhan.liu@linux.intel.com> wrote:
> > On Mon, Feb 15, 2016 at 04:48:36PM +0530, Santosh Shukla wrote:
> >> Hi Yuanhan,
> >>
> >> On Mon, Feb 15, 2016 at 4:27 PM, Yuanhan Liu
> >> <yuanhan.liu@linux.intel.com> wrote:
> >> > On Mon, Feb 15, 2016 at 03:22:11PM +0530, Santosh Shukla wrote:
> >> >> Hi Yuanhan,
> >> >>
> >> >> I guess you are back from vacation.
> >> >>
> >> >> Can you pl. review this patch, Except this patch, rest of patches
> >> >> received ack-by:
> >> >
> >> > I had a quick glimpse of the comments from Thomas: he made a good point.
> >> > I will have a deeper thought tomorrow, to see what I can do to fix it.
> >> >
> >>
> >> I agree to what Thomas pointed out about runtime mode switch (vectored
> >> vs non-vectored). I have a proposal in my mind and Like to know you
> >> opinion:
> >>
> >> - need for apis like is_arch_support_vec().
> >>
> >> if (is_arch_support_vec())
> >>          simpple_xxxx = 1 /* Switch code path to vector mode */
> >> else
> >>          simple_xxxx = 0  /* Switch code path to non-vector mode */
> >>
> >> That api should reside to arch file. i.e.. arch like i686/arm{for
> >> implementation not exist so say no supported} will return 0 and for
> >> x86_64 = 1
> >
> > I was thinking that Thomas meant to something like below (like what
> > we did at rte_memcpy.h):
> >
> >     #ifdef RTE_MACHINE_CPUFLAG_SSE (or whatever)
> >
> >         /* with vec here */
> >
> >     #else
> >
> >         /* without vec here */
> >
> >     #endif
> >
> > I mean, you have to bypass the build first; otherwise, you can't
> > go that further to runtime, right?
> >
> 
> I meant: move virtio_recv_pkt_vec() implementation in
> lib/libeal_rte/xx/include/arch/xx/virtio_vec.h. virtio driver to check
> for CPUFLAG supported or not and then use _recv_pkt() call back
> function from arch files. This approach will avoid #ifdef ARCH
> clutter.

Moving virtio stuff to eal looks wrong to me.

Huawei, please comment on this.

	--yliu

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64
  2016-02-07 13:51 [dpdk-dev] [PATCH v7 0/4] Add virtio support for arm/arm64 Santosh Shukla
                   ` (3 preceding siblings ...)
  2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 4/4] eal/linux: vfio: add pci ioport support Santosh Shukla
@ 2016-02-21 14:17 ` Santosh Shukla
  2016-02-21 14:17   ` [dpdk-dev] [PATCH v9 1/3] eal/linux: never check iopl for arm Santosh Shukla
                     ` (3 more replies)
  4 siblings, 4 replies; 25+ messages in thread
From: Santosh Shukla @ 2016-02-21 14:17 UTC (permalink / raw)
  To: dev

v9 patchset to support vfio infrasture for ioport, required for archs example
arm64/arm and x86.


For virtio inc_vector patch which is not part of v9..its under review, refer [2].

Follow on patch history summary, refer[1]
[1] http://comments.gmane.org/gmane.comp.networking.dpdk.devel/32821
[2] http://dpdk.org/dev/patchwork/patch/10429/

Santosh Shukla (3):
  eal/linux: never check iopl for arm
  eal/linux: vfio: ignore mapping for ioport region
  eal/linux: vfio: add pci ioport support

 lib/librte_eal/linuxapp/eal/eal.c          |    2 +
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   56 ++++++++++++++++++++++------
 2 files changed, 46 insertions(+), 12 deletions(-)

-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH v9 1/3] eal/linux: never check iopl for arm
  2016-02-21 14:17 ` [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64 Santosh Shukla
@ 2016-02-21 14:17   ` Santosh Shukla
  2016-02-21 14:18   ` [dpdk-dev] [PATCH v9 2/3] eal/linux: vfio: ignore mapping for ioport region Santosh Shukla
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 25+ messages in thread
From: Santosh Shukla @ 2016-02-21 14:17 UTC (permalink / raw)
  To: dev

iopl() syscall not supported in linux-arm/arm64 so always return 0 value.

Signed-off-by: Santosh Shukla <sshukla@mvista.com>
Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Jan Viktorin <viktorin@rehivetech.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 4d3e0de..ceac435 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -716,6 +716,8 @@ rte_eal_iopl_init(void)
 	if (iopl(3) != 0)
 		return -1;
 	return 0;
+#elif defined(RTE_ARCH_ARM) || defined(RTE_ARCH_ARM64)
+	return 0; /* iopl syscall not supported for ARM/ARM64 */
 #else
 	return -1;
 #endif
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH v9 2/3] eal/linux: vfio: ignore mapping for ioport region
  2016-02-21 14:17 ` [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64 Santosh Shukla
  2016-02-21 14:17   ` [dpdk-dev] [PATCH v9 1/3] eal/linux: never check iopl for arm Santosh Shukla
@ 2016-02-21 14:18   ` Santosh Shukla
  2016-02-21 14:18   ` [dpdk-dev] [PATCH v9 3/3] eal/linux: vfio: add pci ioport support Santosh Shukla
  2016-02-22  5:41   ` [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64 Yuanhan Liu
  3 siblings, 0 replies; 25+ messages in thread
From: Santosh Shukla @ 2016-02-21 14:18 UTC (permalink / raw)
  To: dev

vfio_pci_mmap() try to map all pci bars. ioport region are not mapped in
vfio/kernel so ignore mmaping for ioport.

Signed-off-by: Santosh Shukla <sshukla@mvista.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
v9: included anatoly acked-by 

 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index ffa2dd0..4832313 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -659,6 +659,7 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
 	struct pci_map *maps;
 	uint32_t msix_table_offset = 0;
 	uint32_t msix_table_size = 0;
+	uint32_t ioport_bar;
 
 	dev->intr_handle.fd = -1;
 	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
@@ -853,6 +854,25 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
 			return -1;
 		}
 
+		/* chk for io port region */
+		ret = pread64(vfio_dev_fd, &ioport_bar, sizeof(ioport_bar),
+			      VFIO_GET_REGION_ADDR(VFIO_PCI_CONFIG_REGION_INDEX)
+			      + PCI_BASE_ADDRESS_0 + i*4);
+
+		if (ret != sizeof(ioport_bar)) {
+			RTE_LOG(ERR, EAL,
+				"Cannot read command (%x) from config space!\n",
+				PCI_BASE_ADDRESS_0 + i*4);
+			return -1;
+		}
+
+		if (ioport_bar & PCI_BASE_ADDRESS_SPACE_IO) {
+			RTE_LOG(INFO, EAL,
+				"Ignore mapping IO port bar(%d) addr: %x\n",
+				 i, ioport_bar);
+			continue;
+		}
+
 		/* skip non-mmapable BARs */
 		if ((reg.flags & VFIO_REGION_INFO_FLAG_MMAP) == 0)
 			continue;
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH v9 3/3] eal/linux: vfio: add pci ioport support
  2016-02-21 14:17 ` [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64 Santosh Shukla
  2016-02-21 14:17   ` [dpdk-dev] [PATCH v9 1/3] eal/linux: never check iopl for arm Santosh Shukla
  2016-02-21 14:18   ` [dpdk-dev] [PATCH v9 2/3] eal/linux: vfio: ignore mapping for ioport region Santosh Shukla
@ 2016-02-21 14:18   ` Santosh Shukla
  2016-02-22  5:41   ` [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64 Yuanhan Liu
  3 siblings, 0 replies; 25+ messages in thread
From: Santosh Shukla @ 2016-02-21 14:18 UTC (permalink / raw)
  To: dev

Include vfio map/rd/wr support for pci ioport.

Signed-off-by: Santosh Shukla <sshukla@mvista.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
v7:
- This is enhancement patch for vfio map/rd/wr, rebased on top of David(s) -
"Rework ioport for virtio" patchset. For more information about api, refer
patch [1].
[1]  http://dpdk.org/dev/patchwork/patch/10426/

v8:
- Remove rte_pci_ioport malloc and rte_free()/unmap() func from v7.
- removed umap from git header.
v9:
- renamed p->offset to p->base

 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   36 ++++++++++++++++++----------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index 4832313..4d29a0d 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -74,6 +74,7 @@ EAL_REGISTER_TAILQ(rte_vfio_tailq)
 #define VFIO_GROUP_FMT "/dev/vfio/%u"
 #define VFIO_NOIOMMU_GROUP_FMT "/dev/vfio/noiommu-%u"
 #define VFIO_GET_REGION_ADDR(x) ((uint64_t) x << 40ULL)
+#define VFIO_GET_REGION_IDX(x) (x >> 40)
 
 /* per-process VFIO config */
 static struct vfio_config vfio_cfg;
@@ -999,30 +1000,41 @@ int
 pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
 		    struct rte_pci_ioport *p)
 {
-	RTE_SET_USED(dev);
-	RTE_SET_USED(bar);
-	RTE_SET_USED(p);
-	return -1;
+	if (bar < VFIO_PCI_BAR0_REGION_INDEX ||
+	    bar > VFIO_PCI_BAR5_REGION_INDEX) {
+		RTE_LOG(ERR, EAL, "invalid bar (%d)!\n", bar);
+		return -1;
+	}
+
+	p->dev = dev;
+	p->base = VFIO_GET_REGION_ADDR(bar);
+	return 0;
 }
 
 void
 pci_vfio_ioport_read(struct rte_pci_ioport *p,
 		     void *data, size_t len, off_t offset)
 {
-	RTE_SET_USED(p);
-	RTE_SET_USED(data);
-	RTE_SET_USED(len);
-	RTE_SET_USED(offset);
+	const struct rte_intr_handle *intr_handle = &p->dev->intr_handle;
+
+	if (pread64(intr_handle->vfio_dev_fd, data,
+		    len, p->base + offset) <= 0)
+		RTE_LOG(ERR, EAL,
+			"Can't read from PCI bar (%" PRIu64 ") : offset (%x)\n",
+			VFIO_GET_REGION_IDX(p->base), (int)offset);
 }
 
 void
 pci_vfio_ioport_write(struct rte_pci_ioport *p,
 		      const void *data, size_t len, off_t offset)
 {
-	RTE_SET_USED(p);
-	RTE_SET_USED(data);
-	RTE_SET_USED(len);
-	RTE_SET_USED(offset);
+	const struct rte_intr_handle *intr_handle = &p->dev->intr_handle;
+
+	if (pwrite64(intr_handle->vfio_dev_fd, data,
+		     len, p->base + offset) <= 0)
+		RTE_LOG(ERR, EAL,
+			"Can't write to PCI bar (%" PRIu64 ") : offset (%x)\n",
+			VFIO_GET_REGION_IDX(p->base), (int)offset);
 }
 
 int
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR
  2016-02-19  6:42                 ` Yuanhan Liu
@ 2016-02-22  2:03                   ` Xie, Huawei
  2016-02-22  4:14                     ` Santosh Shukla
  0 siblings, 1 reply; 25+ messages in thread
From: Xie, Huawei @ 2016-02-22  2:03 UTC (permalink / raw)
  To: Yuanhan Liu, Santosh Shukla; +Cc: dpdk

On 2/19/2016 2:42 PM, Yuanhan Liu wrote:
> On Fri, Feb 19, 2016 at 10:16:42AM +0530, Santosh Shukla wrote:
>> On Tue, Feb 16, 2016 at 8:35 AM, Yuanhan Liu
>> <yuanhan.liu@linux.intel.com> wrote:
>>> On Mon, Feb 15, 2016 at 04:48:36PM +0530, Santosh Shukla wrote:
>>>> Hi Yuanhan,
>>>>
>>>> On Mon, Feb 15, 2016 at 4:27 PM, Yuanhan Liu
>>>> <yuanhan.liu@linux.intel.com> wrote:
>>>>> On Mon, Feb 15, 2016 at 03:22:11PM +0530, Santosh Shukla wrote:
>>>>>> Hi Yuanhan,
>>>>>>
>>>>>> I guess you are back from vacation.
>>>>>>
>>>>>> Can you pl. review this patch, Except this patch, rest of patches
>>>>>> received ack-by:
>>>>> I had a quick glimpse of the comments from Thomas: he made a good point.
>>>>> I will have a deeper thought tomorrow, to see what I can do to fix it.
>>>>>
>>>> I agree to what Thomas pointed out about runtime mode switch (vectored
>>>> vs non-vectored). I have a proposal in my mind and Like to know you
>>>> opinion:
>>>>
>>>> - need for apis like is_arch_support_vec().
>>>>
>>>> if (is_arch_support_vec())
>>>>          simpple_xxxx = 1 /* Switch code path to vector mode */
>>>> else
>>>>          simple_xxxx = 0  /* Switch code path to non-vector mode */
>>>>
>>>> That api should reside to arch file. i.e.. arch like i686/arm{for
>>>> implementation not exist so say no supported} will return 0 and for
>>>> x86_64 = 1
>>> I was thinking that Thomas meant to something like below (like what
>>> we did at rte_memcpy.h):
>>>
>>>     #ifdef RTE_MACHINE_CPUFLAG_SSE (or whatever)
>>>
>>>         /* with vec here */
>>>
>>>     #else
>>>
>>>         /* without vec here */
>>>
>>>     #endif
>>>
>>> I mean, you have to bypass the build first; otherwise, you can't
>>> go that further to runtime, right?
>>>
>> I meant: move virtio_recv_pkt_vec() implementation in
>> lib/libeal_rte/xx/include/arch/xx/virtio_vec.h. virtio driver to check
>> for CPUFLAG supported or not and then use _recv_pkt() call back
>> function from arch files. This approach will avoid #ifdef ARCH
>> clutter.
> Moving virtio stuff to eal looks wrong to me.
>
> Huawei, please comment on this.
>
> 	--yliu
>

This issue doesn't apply to virtio driver only but to all other PMDs,
unless they are assumed to run on only one arch. As we are close to
release, for the time being, i prefer to use RTE_MACHINE_CPUFLAG_. Later
we look for other elegant solutions, like moving different arch specific
optimizations into the arch directory under driver/virtio/ directory?
Other thoughts?




^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR
  2016-02-22  2:03                   ` Xie, Huawei
@ 2016-02-22  4:14                     ` Santosh Shukla
  2016-02-22 10:22                       ` Thomas Monjalon
  0 siblings, 1 reply; 25+ messages in thread
From: Santosh Shukla @ 2016-02-22  4:14 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dpdk

On Mon, Feb 22, 2016 at 7:33 AM, Xie, Huawei <huawei.xie@intel.com> wrote:
> On 2/19/2016 2:42 PM, Yuanhan Liu wrote:
>> On Fri, Feb 19, 2016 at 10:16:42AM +0530, Santosh Shukla wrote:
>>> On Tue, Feb 16, 2016 at 8:35 AM, Yuanhan Liu
>>> <yuanhan.liu@linux.intel.com> wrote:
>>>> On Mon, Feb 15, 2016 at 04:48:36PM +0530, Santosh Shukla wrote:
>>>>> Hi Yuanhan,
>>>>>
>>>>> On Mon, Feb 15, 2016 at 4:27 PM, Yuanhan Liu
>>>>> <yuanhan.liu@linux.intel.com> wrote:
>>>>>> On Mon, Feb 15, 2016 at 03:22:11PM +0530, Santosh Shukla wrote:
>>>>>>> Hi Yuanhan,
>>>>>>>
>>>>>>> I guess you are back from vacation.
>>>>>>>
>>>>>>> Can you pl. review this patch, Except this patch, rest of patches
>>>>>>> received ack-by:
>>>>>> I had a quick glimpse of the comments from Thomas: he made a good point.
>>>>>> I will have a deeper thought tomorrow, to see what I can do to fix it.
>>>>>>
>>>>> I agree to what Thomas pointed out about runtime mode switch (vectored
>>>>> vs non-vectored). I have a proposal in my mind and Like to know you
>>>>> opinion:
>>>>>
>>>>> - need for apis like is_arch_support_vec().
>>>>>
>>>>> if (is_arch_support_vec())
>>>>>          simpple_xxxx = 1 /* Switch code path to vector mode */
>>>>> else
>>>>>          simple_xxxx = 0  /* Switch code path to non-vector mode */
>>>>>
>>>>> That api should reside to arch file. i.e.. arch like i686/arm{for
>>>>> implementation not exist so say no supported} will return 0 and for
>>>>> x86_64 = 1
>>>> I was thinking that Thomas meant to something like below (like what
>>>> we did at rte_memcpy.h):
>>>>
>>>>     #ifdef RTE_MACHINE_CPUFLAG_SSE (or whatever)
>>>>
>>>>         /* with vec here */
>>>>
>>>>     #else
>>>>
>>>>         /* without vec here */
>>>>
>>>>     #endif
>>>>
>>>> I mean, you have to bypass the build first; otherwise, you can't
>>>> go that further to runtime, right?
>>>>
>>> I meant: move virtio_recv_pkt_vec() implementation in
>>> lib/libeal_rte/xx/include/arch/xx/virtio_vec.h. virtio driver to check
>>> for CPUFLAG supported or not and then use _recv_pkt() call back
>>> function from arch files. This approach will avoid #ifdef ARCH
>>> clutter.
>> Moving virtio stuff to eal looks wrong to me.
>>
>> Huawei, please comment on this.
>>
>>       --yliu
>>
>
> This issue doesn't apply to virtio driver only but to all other PMDs,
> unless they are assumed to run on only one arch. As we are close to
> release, for the time being, i prefer to use RTE_MACHINE_CPUFLAG_. Later
> we look for other elegant solutions, like moving different arch specific
> optimizations into the arch directory under driver/virtio/ directory?
> Other thoughts?
>

Creating arch specifics files in driver/virtio/: approach look okay to
me. It look alike to my proposal except eal. I choose eal so that one
api and its implementation stays in arch files, no ifdef clutter. I
guess - Same doable in virtio directory too, create arch files and
keep arch specific implementation their.

so, +1 to approach.
>
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64
  2016-02-21 14:17 ` [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64 Santosh Shukla
                     ` (2 preceding siblings ...)
  2016-02-21 14:18   ` [dpdk-dev] [PATCH v9 3/3] eal/linux: vfio: add pci ioport support Santosh Shukla
@ 2016-02-22  5:41   ` Yuanhan Liu
  2016-02-23  6:11     ` Santosh Shukla
  2016-02-24 10:45     ` Thomas Monjalon
  3 siblings, 2 replies; 25+ messages in thread
From: Yuanhan Liu @ 2016-02-22  5:41 UTC (permalink / raw)
  To: Santosh Shukla; +Cc: dev

On Sun, Feb 21, 2016 at 07:47:58PM +0530, Santosh Shukla wrote:
> v9 patchset to support vfio infrasture for ioport, required for archs example
> arm64/arm and x86.
> 
> 
> For virtio inc_vector patch which is not part of v9..its under review, refer [2].
> 
> Follow on patch history summary, refer[1]
> [1] http://comments.gmane.org/gmane.comp.networking.dpdk.devel/32821
> [2] http://dpdk.org/dev/patchwork/patch/10429/
> 
> Santosh Shukla (3):
>   eal/linux: never check iopl for arm
>   eal/linux: vfio: ignore mapping for ioport region
>   eal/linux: vfio: add pci ioport support
> 
>  lib/librte_eal/linuxapp/eal/eal.c          |    2 +
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   56 ++++++++++++++++++++++------
>  2 files changed, 46 insertions(+), 12 deletions(-)

Series looks good to me:

Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

	--yliu

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR
  2016-02-22  4:14                     ` Santosh Shukla
@ 2016-02-22 10:22                       ` Thomas Monjalon
  0 siblings, 0 replies; 25+ messages in thread
From: Thomas Monjalon @ 2016-02-22 10:22 UTC (permalink / raw)
  To: Santosh Shukla, Xie, Huawei, Yuanhan Liu; +Cc: dev

2016-02-22 09:44, Santosh Shukla:
> On Mon, Feb 22, 2016 at 7:33 AM, Xie, Huawei <huawei.xie@intel.com> wrote:
> > On 2/19/2016 2:42 PM, Yuanhan Liu wrote:
> >> On Fri, Feb 19, 2016 at 10:16:42AM +0530, Santosh Shukla wrote:
> >>> On Tue, Feb 16, 2016 at 8:35 AM, Yuanhan Liu
> >>> <yuanhan.liu@linux.intel.com> wrote:
> >>>> On Mon, Feb 15, 2016 at 04:48:36PM +0530, Santosh Shukla wrote:
> >>>>> Hi Yuanhan,
> >>>>>
> >>>>> On Mon, Feb 15, 2016 at 4:27 PM, Yuanhan Liu
> >>>>> <yuanhan.liu@linux.intel.com> wrote:
> >>>>>> I had a quick glimpse of the comments from Thomas: he made a good point.
> >>>>>> I will have a deeper thought tomorrow, to see what I can do to fix it.
> >>>>>>
> >>>>> I agree to what Thomas pointed out about runtime mode switch (vectored
> >>>>> vs non-vectored). I have a proposal in my mind and Like to know you
> >>>>> opinion:
> >>>>>
> >>>>> - need for apis like is_arch_support_vec().
> >>>>>
> >>>>> if (is_arch_support_vec())
> >>>>>          simpple_xxxx = 1 /* Switch code path to vector mode */
> >>>>> else
> >>>>>          simple_xxxx = 0  /* Switch code path to non-vector mode */
> >>>>>
> >>>>> That api should reside to arch file. i.e.. arch like i686/arm{for
> >>>>> implementation not exist so say no supported} will return 0 and for
> >>>>> x86_64 = 1
> >>>> I was thinking that Thomas meant to something like below (like what
> >>>> we did at rte_memcpy.h):
> >>>>
> >>>>     #ifdef RTE_MACHINE_CPUFLAG_SSE (or whatever)
> >>>>
> >>>>         /* with vec here */
> >>>>
> >>>>     #else
> >>>>
> >>>>         /* without vec here */
> >>>>
> >>>>     #endif
> >>>>
> >>>> I mean, you have to bypass the build first; otherwise, you can't
> >>>> go that further to runtime, right?
> >>>>
> >>> I meant: move virtio_recv_pkt_vec() implementation in
> >>> lib/libeal_rte/xx/include/arch/xx/virtio_vec.h. virtio driver to check
> >>> for CPUFLAG supported or not and then use _recv_pkt() call back
> >>> function from arch files. This approach will avoid #ifdef ARCH
> >>> clutter.
> >> Moving virtio stuff to eal looks wrong to me.
> >
> > This issue doesn't apply to virtio driver only but to all other PMDs,
> > unless they are assumed to run on only one arch. As we are close to
> > release, for the time being, i prefer to use RTE_MACHINE_CPUFLAG_.

Yes the obvious fix is to use some CPU flags.
In ACL the flags are checked on runtime to allow using some optimizations
after a "default" build. It can be considered later for virtio.

> > Later
> > we look for other elegant solutions, like moving different arch specific
> > optimizations into the arch directory under driver/virtio/ directory?
> > Other thoughts?
> 
> Creating arch specifics files in driver/virtio/: approach look okay to
> me. It look alike to my proposal except eal. I choose eal so that one
> api and its implementation stays in arch files, no ifdef clutter. I
> guess - Same doable in virtio directory too, create arch files and
> keep arch specific implementation their.
> 
> so, +1 to approach.

If there are some basic functions which can be re-used in other libs, there
must be in EAL. For virtio-specific functions, you can have some arch-specific
files in virtio.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64
  2016-02-22  5:41   ` [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64 Yuanhan Liu
@ 2016-02-23  6:11     ` Santosh Shukla
  2016-02-24 10:45     ` Thomas Monjalon
  1 sibling, 0 replies; 25+ messages in thread
From: Santosh Shukla @ 2016-02-23  6:11 UTC (permalink / raw)
  To: Yuanhan Liu; +Cc: dpdk

Hi Thomas,

On Mon, Feb 22, 2016 at 11:11 AM, Yuanhan Liu
<yuanhan.liu@linux.intel.com> wrote:
> On Sun, Feb 21, 2016 at 07:47:58PM +0530, Santosh Shukla wrote:
>> v9 patchset to support vfio infrasture for ioport, required for archs example
>> arm64/arm and x86.
>>
>>
>> For virtio inc_vector patch which is not part of v9..its under review, refer [2].
>>
>> Follow on patch history summary, refer[1]
>> [1] http://comments.gmane.org/gmane.comp.networking.dpdk.devel/32821
>> [2] http://dpdk.org/dev/patchwork/patch/10429/
>>
>> Santosh Shukla (3):
>>   eal/linux: never check iopl for arm
>>   eal/linux: vfio: ignore mapping for ioport region
>>   eal/linux: vfio: add pci ioport support
>>
>>  lib/librte_eal/linuxapp/eal/eal.c          |    2 +
>>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   56 ++++++++++++++++++++++------
>>  2 files changed, 46 insertions(+), 12 deletions(-)
>
> Series looks good to me:
>
> Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>

Request to merge this series, Thanks.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64
  2016-02-22  5:41   ` [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64 Yuanhan Liu
  2016-02-23  6:11     ` Santosh Shukla
@ 2016-02-24 10:45     ` Thomas Monjalon
  1 sibling, 0 replies; 25+ messages in thread
From: Thomas Monjalon @ 2016-02-24 10:45 UTC (permalink / raw)
  To: Santosh Shukla; +Cc: dev

2016-02-22 13:41, Yuanhan Liu:
> On Sun, Feb 21, 2016 at 07:47:58PM +0530, Santosh Shukla wrote:
> > v9 patchset to support vfio infrasture for ioport, required for archs example
> > arm64/arm and x86.
> > 
> > 
> > For virtio inc_vector patch which is not part of v9..its under review, refer [2].
> > 
> > Follow on patch history summary, refer[1]
> > [1] http://comments.gmane.org/gmane.comp.networking.dpdk.devel/32821
> > [2] http://dpdk.org/dev/patchwork/patch/10429/
> > 
> > Santosh Shukla (3):
> >   eal/linux: never check iopl for arm
> >   eal/linux: vfio: ignore mapping for ioport region
> >   eal/linux: vfio: add pci ioport support
> > 
> >  lib/librte_eal/linuxapp/eal/eal.c          |    2 +
> >  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   56 ++++++++++++++++++++++------
> >  2 files changed, 46 insertions(+), 12 deletions(-)
> 
> Series looks good to me:
> 
> Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Applied, thanks

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2016-02-24 10:47 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-07 13:51 [dpdk-dev] [PATCH v7 0/4] Add virtio support for arm/arm64 Santosh Shukla
2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 1/4] eal/linux: never check iopl for arm Santosh Shukla
2016-02-18  5:26   ` Santosh Shukla
2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR Santosh Shukla
2016-02-07 21:25   ` Thomas Monjalon
2016-02-08  5:45     ` Santosh Shukla
     [not found]       ` <CAAyOgsZO+6+kFZZZM203fPR3AmVYB0v7j3-f+DawZOCuR-AVvQ@mail.gmail.com>
     [not found]         ` <20160215105743.GB21426@yliu-dev.sh.intel.com>
     [not found]           ` <CAAyOgsaT5TcsPfum8x6yzAJAz=5N+c5QebEn7KCyJn7oK=VMsw@mail.gmail.com>
2016-02-16  3:05             ` Yuanhan Liu
2016-02-19  4:46               ` Santosh Shukla
2016-02-19  6:42                 ` Yuanhan Liu
2016-02-22  2:03                   ` Xie, Huawei
2016-02-22  4:14                     ` Santosh Shukla
2016-02-22 10:22                       ` Thomas Monjalon
2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 3/4] eal/linux: vfio: ignore mapping for ioport region Santosh Shukla
2016-02-08  9:15   ` Burakov, Anatoly
2016-02-18  5:26     ` Santosh Shukla
2016-02-07 13:51 ` [dpdk-dev] [PATCH v7 4/4] eal/linux: vfio: add pci ioport support Santosh Shukla
2016-02-08  8:51   ` David Marchand
2016-02-08  9:40     ` Santosh Shukla
2016-02-21 14:17 ` [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64 Santosh Shukla
2016-02-21 14:17   ` [dpdk-dev] [PATCH v9 1/3] eal/linux: never check iopl for arm Santosh Shukla
2016-02-21 14:18   ` [dpdk-dev] [PATCH v9 2/3] eal/linux: vfio: ignore mapping for ioport region Santosh Shukla
2016-02-21 14:18   ` [dpdk-dev] [PATCH v9 3/3] eal/linux: vfio: add pci ioport support Santosh Shukla
2016-02-22  5:41   ` [dpdk-dev] [PATCH v9 0/3] Add virtio support for arm/arm64 Yuanhan Liu
2016-02-23  6:11     ` Santosh Shukla
2016-02-24 10:45     ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).