DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
@ 2021-01-29  3:18 谢华伟(此时此刻)
  2021-01-29  3:18 ` [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
                   ` (3 more replies)
  0 siblings, 4 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-01-29  3:18 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin
  Cc: dev, david.marchand, anatoly.burakov, xuemingl, grive,
	chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>


virtio PMD assumes legacy device only supports PIO BAR resource. This is wrong.
As we need to create lots of devices, as PIO resource on x86 is very limited, 
we expose MMIO(memory IO) BAR.

Kernel supports both PIO and MMIO BAR for legacy virtio-pci device, and for all
other pci devices. This patchset handles different type of BAR in the similar way.

In previous implementation, under igb_uio driver we get PIO address from igb_uio
sysfs entry; with uio_pci_generic, we get PIO address from /proc/ioports for x86,
and for other ARCHs, we get PIO address from standard PCI sysfs entry.
For PIO/MMIO RW, there is different path for different drivers and arch.


All of the above is too much twisted.
This patchset unifies the way to get both PIO and MMIO address for different driver
and ARCHs, all from standard resource attr under pci sysfs. This is most generic.

We distinguish PIO and MMIO by their address like how kernel does. It is ugly but works.

v2 changes:
    - add more explanation in the commit message

v3 changes:
    - fix patch format issues

v4 changes:
    - fixes for RTE_KDRV_UIO_GENERIC -> RTE_PCI_KDRV_UIO_GENERIC

v5 changes:
    - split into three seperate patches

v6 changes:
    - change to DEBUG level for IO bar detection in pci_uio_ioport_map
    - rework the code in iobar branch
    - fixes commit message format issue
    - temporarily remove the 3rd patch for vfio path, leave it for future discusssion
    - rework against virtio_pmd_rework_v2


huawei.xhw (2):
  bus/pci: use PCI standard sysfs entry to get PIO address
  bus/pci: support MMIO in PCI ioport accessors

 drivers/bus/pci/linux/pci.c     |  81 -------------------
 drivers/bus/pci/linux/pci_uio.c | 170 ++++++++++++++++++++++++++++------------
 2 files changed, 118 insertions(+), 133 deletions(-)

-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-01-29  3:18 [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
@ 2021-01-29  3:18 ` 谢华伟(此时此刻)
  2021-02-03  9:37   ` Maxime Coquelin
  2021-02-18  9:33   ` David Marchand
  2021-01-29  3:18 ` [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-01-29  3:18 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin
  Cc: dev, david.marchand, anatoly.burakov, xuemingl, grive,
	chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>

Currently virtio PMD asssumes legacy device uses PIO bar.
There are three ways to get PIO(PortIO) address for virtio legacy device.
    under igb_uio, get pio address from uio/uio# sysfs attribute
    under uio_pci_generic:
        for X86, get PIO address from /proc/ioport
        for other ARCH, get PIO address from standard PCI sysfs attribute

Actually, igb_uio sysfs attribute exports exactly the same thing as standard PCI sysfs, i.e,
    pci_dev->resource[]

This patch fixes these messy things, and uses standard PCI sysfs attribute.

Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/pci/linux/pci.c     | 77 -----------------------------------------
 drivers/bus/pci/linux/pci_uio.c | 64 ++++++++++++++++++++++++----------
 2 files changed, 46 insertions(+), 95 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 2e1808b..0f38abf 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -677,71 +677,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 	}
 }
 
-#if defined(RTE_ARCH_X86)
-static int
-pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
-		struct rte_pci_ioport *p)
-{
-	uint16_t start, end;
-	FILE *fp;
-	char *line = NULL;
-	char pci_id[16];
-	int found = 0;
-	size_t linesz;
-
-	if (rte_eal_iopl_init() != 0) {
-		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
-			__func__, dev->name);
-		return -1;
-	}
-
-	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
-		 dev->addr.domain, dev->addr.bus,
-		 dev->addr.devid, dev->addr.function);
-
-	fp = fopen("/proc/ioports", "r");
-	if (fp == NULL) {
-		RTE_LOG(ERR, EAL, "%s(): can't open ioports\n", __func__);
-		return -1;
-	}
-
-	while (getdelim(&line, &linesz, '\n', fp) > 0) {
-		char *ptr = line;
-		char *left;
-		int n;
-
-		n = strcspn(ptr, ":");
-		ptr[n] = 0;
-		left = &ptr[n + 1];
-
-		while (*left && isspace(*left))
-			left++;
-
-		if (!strncmp(left, pci_id, strlen(pci_id))) {
-			found = 1;
-
-			while (*ptr && isspace(*ptr))
-				ptr++;
-
-			sscanf(ptr, "%04hx-%04hx", &start, &end);
-
-			break;
-		}
-	}
-
-	free(line);
-	fclose(fp);
-
-	if (!found)
-		return -1;
-
-	p->base = start;
-	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%x\n", start);
-
-	return 0;
-}
-#endif
-
 int
 rte_pci_ioport_map(struct rte_pci_device *dev, int bar,
 		struct rte_pci_ioport *p)
@@ -756,14 +691,8 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		ret = pci_uio_ioport_map(dev, bar, p);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
-#if defined(RTE_ARCH_X86)
-		ret = pci_ioport_map(dev, bar, p);
-#else
 		ret = pci_uio_ioport_map(dev, bar, p);
-#endif
 		break;
 	default:
 		break;
@@ -830,14 +759,8 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		ret = pci_uio_ioport_unmap(p);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
-#if defined(RTE_ARCH_X86)
-		ret = 0;
-#else
 		ret = pci_uio_ioport_unmap(p);
-#endif
 		break;
 	default:
 		break;
diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index f3305a2..01f2a40 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -373,10 +373,13 @@
 pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
 		   struct rte_pci_ioport *p)
 {
+	FILE *f = NULL;
 	char dirname[PATH_MAX];
 	char filename[PATH_MAX];
-	int uio_num;
-	unsigned long start;
+	char buf[BUFSIZ];
+	uint64_t phys_addr, end_addr, flags;
+	unsigned long base;
+	int i;
 
 	if (rte_eal_iopl_init() != 0) {
 		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
@@ -384,41 +387,66 @@
 		return -1;
 	}
 
-	uio_num = pci_get_uio_dev(dev, dirname, sizeof(dirname), 0);
-	if (uio_num < 0)
+	/* open and read addresses of the corresponding resource in sysfs */
+	snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
+		rte_pci_get_sysfs_path(), dev->addr.domain, dev->addr.bus,
+		dev->addr.devid, dev->addr.function);
+	f = fopen(filename, "r");
+	if (f == NULL) {
+		RTE_LOG(ERR, EAL, "%s(): Cannot open sysfs resource: %s\n",
+			__func__, strerror(errno));
 		return -1;
+	}
 
-	/* get portio start */
-	snprintf(filename, sizeof(filename),
-		 "%s/portio/port%d/start", dirname, bar);
-	if (eal_parse_sysfs_value(filename, &start) < 0) {
-		RTE_LOG(ERR, EAL, "%s(): cannot parse portio start\n",
-			__func__);
-		return -1;
+	for (i = 0; i < bar + 1; i++) {
+		if (fgets(buf, sizeof(buf), f) == NULL) {
+			RTE_LOG(ERR, EAL, "%s(): Cannot read sysfs resource\n", __func__);
+			goto error;
+		}
 	}
-	/* ensure we don't get anything funny here, read/write will cast to
-	 * uin16_t */
-	if (start > UINT16_MAX)
-		return -1;
+	if (pci_parse_one_sysfs_resource(buf, sizeof(buf), &phys_addr,
+		&end_addr, &flags) < 0)
+		goto error;
+
+	if (!(flags & IORESOURCE_IO)) {
+		RTE_LOG(ERR, EAL, "%s(): bar resource other than IO is not supported\n", __func__);
+		goto error;
+	}
+	base = (unsigned long)phys_addr;
+	RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+
+	if (base > UINT16_MAX)
+		goto error;
 
 	/* FIXME only for primary process ? */
 	if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) {
+		int uio_num = pci_get_uio_dev(dev, dirname, sizeof(dirname), 0);
+		if (uio_num < 0) {
+			RTE_LOG(ERR, EAL, "cannot open %s: %s\n",
+				dirname, strerror(errno));
+			goto error;
+		}
 
 		snprintf(filename, sizeof(filename), "/dev/uio%u", uio_num);
 		dev->intr_handle.fd = open(filename, O_RDWR);
 		if (dev->intr_handle.fd < 0) {
 			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 				filename, strerror(errno));
-			return -1;
+			goto error;
 		}
 		dev->intr_handle.type = RTE_INTR_HANDLE_UIO;
 	}
 
-	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx\n", start);
+	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx\n", base);
 
-	p->base = start;
+	p->base = base;
 	p->len = 0;
+	fclose(f);
 	return 0;
+error:
+	if (f)
+		fclose(f);
+	return -1;
 }
 #else
 int
-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-01-29  3:18 [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
  2021-01-29  3:18 ` [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
@ 2021-01-29  3:18 ` 谢华伟(此时此刻)
  2021-02-03  9:37   ` Maxime Coquelin
                     ` (2 more replies)
  2021-01-29  3:25 ` [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
  2021-02-22 17:15 ` [dpdk-dev] [PATCH v7 " 谢华伟(此时此刻)
  3 siblings, 3 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-01-29  3:18 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin
  Cc: dev, david.marchand, anatoly.burakov, xuemingl, grive,
	chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>

With IO BAR, we get PIO(programmed IO) address.
With MMIO BAR, we get mapped virtual address.
We distinguish PIO(Programmed IO) and MMIO(memory mapped IO) by their address like how kernel does.
ioread/write8/16/32 is provided to access PIO/MMIO.
By the way, for virtio on arch other than x86, BAR flag indicates PIO but is mapped.

Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/pci/linux/pci.c     |   4 --
 drivers/bus/pci/linux/pci_uio.c | 124 ++++++++++++++++++++++++++--------------
 2 files changed, 81 insertions(+), 47 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 0f38abf..0dc99e9 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -715,8 +715,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		pci_uio_ioport_read(p, data, len, offset);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
 		pci_uio_ioport_read(p, data, len, offset);
 		break;
@@ -736,8 +734,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		pci_uio_ioport_write(p, data, len, offset);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
 		pci_uio_ioport_write(p, data, len, offset);
 		break;
diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 01f2a40..78b2952 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -368,6 +368,8 @@
 	return -1;
 }
 
+#define PIO_MAX 0x10000
+
 #if defined(RTE_ARCH_X86)
 int
 pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
@@ -381,12 +383,6 @@
 	unsigned long base;
 	int i;
 
-	if (rte_eal_iopl_init() != 0) {
-		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
-			__func__, dev->name);
-		return -1;
-	}
-
 	/* open and read addresses of the corresponding resource in sysfs */
 	snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
 		rte_pci_get_sysfs_path(), dev->addr.domain, dev->addr.bus,
@@ -408,15 +404,27 @@
 		&end_addr, &flags) < 0)
 		goto error;
 
-	if (!(flags & IORESOURCE_IO)) {
-		RTE_LOG(ERR, EAL, "%s(): bar resource other than IO is not supported\n", __func__);
-		goto error;
-	}
-	base = (unsigned long)phys_addr;
-	RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+	if (flags & IORESOURCE_IO) {
+		if (rte_eal_iopl_init()) {
+			RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
+				__func__, dev->name);
+			goto error;
+		}
+
+		base = (unsigned long)phys_addr;
+		if (base > PIO_MAX) {
+			RTE_LOG(ERR, EAL, "%s(): %08lx too large PIO resource\n", __func__, base);
+			goto error;
+		}
 
-	if (base > UINT16_MAX)
+		RTE_LOG(DEBUG, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+	} else if (flags & IORESOURCE_MEM) {
+		base = (unsigned long)dev->mem_resource[bar].addr;
+		RTE_LOG(DEBUG, EAL, "%s(): MMIO BAR %08lx detected\n", __func__, base);
+	} else {
+		RTE_LOG(ERR, EAL, "%s(): unknown BAR type\n", __func__);
 		goto error;
+	}
 
 	/* FIXME only for primary process ? */
 	if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) {
@@ -517,6 +525,60 @@
 }
 #endif
 
+static inline uint8_t ioread8(void *addr)
+{
+	uint8_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint8_t *)addr :
+		inb((unsigned long)addr);
+
+	return val;
+}
+
+static inline uint16_t ioread16(void *addr)
+{
+	uint16_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint16_t *)addr :
+		inw((unsigned long)addr);
+
+	return val;
+}
+
+static inline uint32_t ioread32(void *addr)
+{
+	uint32_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint32_t *)addr :
+		inl((unsigned long)addr);
+
+	return val;
+}
+
+static inline void iowrite8(uint8_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint8_t *)addr = val :
+		outb(val, (unsigned long)addr);
+}
+
+static inline void iowrite16(uint16_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint16_t *)addr = val :
+		outw(val, (unsigned long)addr);
+}
+
+static inline void iowrite32(uint32_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint32_t *)addr = val :
+		outl(val, (unsigned long)addr);
+}
+
 void
 pci_uio_ioport_read(struct rte_pci_ioport *p,
 		    void *data, size_t len, off_t offset)
@@ -528,25 +590,13 @@
 	for (d = data; len > 0; d += size, reg += size, len -= size) {
 		if (len >= 4) {
 			size = 4;
-#if defined(RTE_ARCH_X86)
-			*(uint32_t *)d = inl(reg);
-#else
-			*(uint32_t *)d = *(volatile uint32_t *)reg;
-#endif
+			*(uint32_t *)d = ioread32((void *)reg);
 		} else if (len >= 2) {
 			size = 2;
-#if defined(RTE_ARCH_X86)
-			*(uint16_t *)d = inw(reg);
-#else
-			*(uint16_t *)d = *(volatile uint16_t *)reg;
-#endif
+			*(uint16_t *)d = ioread16((void *)reg);
 		} else {
 			size = 1;
-#if defined(RTE_ARCH_X86)
-			*d = inb(reg);
-#else
-			*d = *(volatile uint8_t *)reg;
-#endif
+			*d = ioread8((void *)reg);
 		}
 	}
 }
@@ -562,25 +612,13 @@
 	for (s = data; len > 0; s += size, reg += size, len -= size) {
 		if (len >= 4) {
 			size = 4;
-#if defined(RTE_ARCH_X86)
-			outl_p(*(const uint32_t *)s, reg);
-#else
-			*(volatile uint32_t *)reg = *(const uint32_t *)s;
-#endif
+			iowrite32(*(const uint32_t *)s, (void *)reg);
 		} else if (len >= 2) {
 			size = 2;
-#if defined(RTE_ARCH_X86)
-			outw_p(*(const uint16_t *)s, reg);
-#else
-			*(volatile uint16_t *)reg = *(const uint16_t *)s;
-#endif
+			iowrite16(*(const uint16_t *)s, (void *)reg);
 		} else {
 			size = 1;
-#if defined(RTE_ARCH_X86)
-			outb_p(*s, reg);
-#else
-			*(volatile uint8_t *)reg = *s;
-#endif
+			iowrite8(*s, (void *)reg);
 		}
 	}
 }
-- 
1.8.3.1


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

* Re: [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
  2021-01-29  3:18 [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
  2021-01-29  3:18 ` [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
  2021-01-29  3:18 ` [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
@ 2021-01-29  3:25 ` 谢华伟(此时此刻)
  2021-02-01  7:43   ` 谢华伟(此时此刻)
  2021-02-22 17:15 ` [dpdk-dev] [PATCH v7 " 谢华伟(此时此刻)
  3 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-01-29  3:25 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin
  Cc: dev, david.marchand, anatoly.burakov, xuemingl, grive, chenbo.xia

Hi ferruh and maxime:

v6 changes:

send v6. Let us discuss if merge in this or early next release.

Sorry that forget to reply to previous message id.

>      - change to DEBUG level for IO bar detection in pci_uio_ioport_map
>      - rework the code in iobar branch
>      - fixes commit message format issue
>      - temporarily remove the 3rd patch for vfio path, leave it for future discusssion
>      - rework against virtio_pmd_rework_v2
>
>
> huawei.xhw (2):
>    bus/pci: use PCI standard sysfs entry to get PIO address
>    bus/pci: support MMIO in PCI ioport accessors
>
>   drivers/bus/pci/linux/pci.c     |  81 -------------------
>   drivers/bus/pci/linux/pci_uio.c | 170 ++++++++++++++++++++++++++++------------
>   2 files changed, 118 insertions(+), 133 deletions(-)
>

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

* Re: [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
  2021-01-29  3:25 ` [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
@ 2021-02-01  7:43   ` 谢华伟(此时此刻)
  2021-02-03  9:37     ` Maxime Coquelin
  0 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-02-01  7:43 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin
  Cc: dev, david.marchand, anatoly.burakov, xuemingl, grive, chenbo.xia


On 2021/1/29 11:25, chris wrote:
> Hi ferruh and maxime:
>
> v6 changes:
>
> send v6. Let us discuss if merge in this or early next release.

Ping.


>
> Sorry that forget to reply to previous message id.
>
>>      - change to DEBUG level for IO bar detection in pci_uio_ioport_map
>>      - rework the code in iobar branch
>>      - fixes commit message format issue
>>      - temporarily remove the 3rd patch for vfio path, leave it for 
>> future discusssion
>>      - rework against virtio_pmd_rework_v2
>>
>>
>> huawei.xhw (2):
>>    bus/pci: use PCI standard sysfs entry to get PIO address
>>    bus/pci: support MMIO in PCI ioport accessors
>>
>>   drivers/bus/pci/linux/pci.c     |  81 -------------------
>>   drivers/bus/pci/linux/pci_uio.c | 170 
>> ++++++++++++++++++++++++++++------------
>>   2 files changed, 118 insertions(+), 133 deletions(-)
>>

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

* Re: [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
  2021-02-01  7:43   ` 谢华伟(此时此刻)
@ 2021-02-03  9:37     ` Maxime Coquelin
  2021-02-04  2:50       ` 谢华伟(此时此刻)
  0 siblings, 1 reply; 61+ messages in thread
From: Maxime Coquelin @ 2021-02-03  9:37 UTC (permalink / raw)
  To: 谢华伟(此时此刻),
	ferruh.yigit
  Cc: dev, david.marchand, anatoly.burakov, xuemingl, grive, chenbo.xia

Hi Huawei,

On 2/1/21 8:43 AM, 谢华伟(此时此刻) wrote:
> 
> On 2021/1/29 11:25, chris wrote:
>> Hi ferruh and maxime:
>>
>> v6 changes:
>>
>> send v6. Let us discuss if merge in this or early next release.
> 
> Ping.

The -rc2 was released on the 29th, so I think it is too late for this
release.

As Ferruh suggested, he can pick it early for next release, so doing
that we'll be sure to have a wider testing.

Apart from that, I am fine with the series.

Thanks,
Maxime

> 
>>
>> Sorry that forget to reply to previous message id.
>>
>>>      - change to DEBUG level for IO bar detection in pci_uio_ioport_map
>>>      - rework the code in iobar branch
>>>      - fixes commit message format issue
>>>      - temporarily remove the 3rd patch for vfio path, leave it for
>>> future discusssion
>>>      - rework against virtio_pmd_rework_v2
>>>
>>>
>>> huawei.xhw (2):
>>>    bus/pci: use PCI standard sysfs entry to get PIO address
>>>    bus/pci: support MMIO in PCI ioport accessors
>>>
>>>   drivers/bus/pci/linux/pci.c     |  81 -------------------
>>>   drivers/bus/pci/linux/pci_uio.c | 170
>>> ++++++++++++++++++++++++++++------------
>>>   2 files changed, 118 insertions(+), 133 deletions(-)
>>>
> 


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

* Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-01-29  3:18 ` [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
@ 2021-02-03  9:37   ` Maxime Coquelin
  2021-02-18  9:33   ` David Marchand
  1 sibling, 0 replies; 61+ messages in thread
From: Maxime Coquelin @ 2021-02-03  9:37 UTC (permalink / raw)
  To: 谢华伟(此时此刻),
	ferruh.yigit
  Cc: dev, david.marchand, anatoly.burakov, xuemingl, grive, chenbo.xia



On 1/29/21 4:18 AM, 谢华伟(此时此刻) wrote:
> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>
> 
> Currently virtio PMD asssumes legacy device uses PIO bar.
> There are three ways to get PIO(PortIO) address for virtio legacy device.
>     under igb_uio, get pio address from uio/uio# sysfs attribute
>     under uio_pci_generic:
>         for X86, get PIO address from /proc/ioport
>         for other ARCH, get PIO address from standard PCI sysfs attribute
> 
> Actually, igb_uio sysfs attribute exports exactly the same thing as standard PCI sysfs, i.e,
>     pci_dev->resource[]
> 
> This patch fixes these messy things, and uses standard PCI sysfs attribute.
> 
> Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/bus/pci/linux/pci.c     | 77 -----------------------------------------
>  drivers/bus/pci/linux/pci_uio.c | 64 ++++++++++++++++++++++++----------
>  2 files changed, 46 insertions(+), 95 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>


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

* Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-01-29  3:18 ` [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
@ 2021-02-03  9:37   ` Maxime Coquelin
  2021-02-09 14:51   ` Ferruh Yigit
  2021-02-17  9:06   ` David Marchand
  2 siblings, 0 replies; 61+ messages in thread
From: Maxime Coquelin @ 2021-02-03  9:37 UTC (permalink / raw)
  To: 谢华伟(此时此刻),
	ferruh.yigit
  Cc: dev, david.marchand, anatoly.burakov, xuemingl, grive, chenbo.xia



On 1/29/21 4:18 AM, 谢华伟(此时此刻) wrote:
> |From: "huawei.xhw" <huawei.xhw@alibaba-inc.com> With IO BAR, we get
> PIO(programmed IO) address. With MMIO BAR, we get mapped virtual
> address. We distinguish PIO(Programmed IO) and MMIO(memory mapped IO) by
> their address like how kernel does. ioread/write8/16/32 is provided to
> access PIO/MMIO. By the way, for virtio on arch other than x86, BAR flag
> indicates PIO but is mapped. Signed-off-by: huawei xie
> <huawei.xhw@alibaba-inc.com> Reviewed-by: Maxime Coquelin
> <maxime.coquelin@redhat.com> --- drivers/bus/pci/linux/pci.c | 4 --
> drivers/bus/pci/linux/pci_uio.c | 124
> ++++++++++++++++++++++++++-------------- 2 files changed, 81
> insertions(+), 47 deletions(-)|

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime


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

* Re: [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
  2021-02-03  9:37     ` Maxime Coquelin
@ 2021-02-04  2:50       ` 谢华伟(此时此刻)
  0 siblings, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-02-04  2:50 UTC (permalink / raw)
  To: Maxime Coquelin, ferruh.yigit
  Cc: dev, david.marchand, anatoly.burakov, xuemingl, grive, chenbo.xia


On 2021/2/3 17:37, Maxime Coquelin wrote:
> Hi Huawei,
>
> On 2/1/21 8:43 AM, 谢华伟(此时此刻) wrote:
>> On 2021/1/29 11:25, chris wrote:
>>> Hi ferruh and maxime:
>>>
>>> v6 changes:
>>>
>>> send v6. Let us discuss if merge in this or early next release.
>> Ping.
> The -rc2 was released on the 29th, so I think it is too late for this
> release.
>
> As Ferruh suggested, he can pick it early for next release, so doing
> that we'll be sure to have a wider testing.
>
> Apart from that, I am fine with the series.
OK. Thanks Maxime.
>
> Thanks,
> Maxime
>
>>> Sorry that forget to reply to previous message id.
>>>
>>>>       - change to DEBUG level for IO bar detection in pci_uio_ioport_map
>>>>       - rework the code in iobar branch
>>>>       - fixes commit message format issue
>>>>       - temporarily remove the 3rd patch for vfio path, leave it for
>>>> future discusssion
>>>>       - rework against virtio_pmd_rework_v2
>>>>
>>>>
>>>> huawei.xhw (2):
>>>>     bus/pci: use PCI standard sysfs entry to get PIO address
>>>>     bus/pci: support MMIO in PCI ioport accessors
>>>>
>>>>    drivers/bus/pci/linux/pci.c     |  81 -------------------
>>>>    drivers/bus/pci/linux/pci_uio.c | 170
>>>> ++++++++++++++++++++++++++++------------
>>>>    2 files changed, 118 insertions(+), 133 deletions(-)
>>>>

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

* Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-01-29  3:18 ` [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
  2021-02-03  9:37   ` Maxime Coquelin
@ 2021-02-09 14:51   ` Ferruh Yigit
  2021-02-19  8:52     ` Ferruh Yigit
  2021-02-17  9:06   ` David Marchand
  2 siblings, 1 reply; 61+ messages in thread
From: Ferruh Yigit @ 2021-02-09 14:51 UTC (permalink / raw)
  To: 谢华伟(此时此刻),
	maxime.coquelin
  Cc: dev, david.marchand, anatoly.burakov, xuemingl, grive, chenbo.xia

On 1/29/2021 3:18 AM, 谢华伟(此时此刻) wrote:
> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>
> 
> With IO BAR, we get PIO(programmed IO) address.
> With MMIO BAR, we get mapped virtual address.
> We distinguish PIO(Programmed IO) and MMIO(memory mapped IO) by their address like how kernel does.
> ioread/write8/16/32 is provided to access PIO/MMIO.
> By the way, for virtio on arch other than x86, BAR flag indicates PIO but is mapped.
> 
> Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

<...>

> +static inline void iowrite8(uint8_t val, void *addr)
> +{
> +	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
> +		*(volatile uint8_t *)addr = val :
> +		outb(val, (unsigned long)addr);

Is the 'outb_p' to 'outb' conversion intentional? And if so why?

Same of the all 'outb_p', 'outw_p', 'outl_p'.

<...>

>   			size = 1;
> -#if defined(RTE_ARCH_X86)
> -			outb_p(*s, reg);
> -#else
> -			*(volatile uint8_t *)reg = *s;
> -#endif
> +			iowrite8(*s, (void *)reg);
>   		}
>   	}
>   }
> 


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

* Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-01-29  3:18 ` [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
  2021-02-03  9:37   ` Maxime Coquelin
  2021-02-09 14:51   ` Ferruh Yigit
@ 2021-02-17  9:06   ` David Marchand
  2021-02-17 14:15     ` 谢华伟(此时此刻)
  2 siblings, 1 reply; 61+ messages in thread
From: David Marchand @ 2021-02-17  9:06 UTC (permalink / raw)
  To: 谢华伟(此时此刻)
  Cc: Yigit, Ferruh, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo

On Fri, Jan 29, 2021 at 4:19 AM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
> @@ -517,6 +525,60 @@
>  }
>  #endif
>
> +static inline uint8_t ioread8(void *addr)
> +{
> +       uint8_t val;
> +
> +       val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
> +               *(volatile uint8_t *)addr :
> +               inb((unsigned long)addr);

inb/outb and others are architecture (x86?) specific.

The CI caught this issue, see build failures on ARM64.
https://lab.dpdk.org/results/dashboard/results/results-uploads/test_runs/82432e287bc94831b7a65d7cd6f05783/log_upload_file/2021/2/dpdk_b49c677a0d24_15433_2021-02-01_00-15-59_NA.zip

I can see the same issue with ppc64le.


--
David Marchand


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

* Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-02-17  9:06   ` David Marchand
@ 2021-02-17 14:15     ` 谢华伟(此时此刻)
  2021-02-18  9:33       ` David Marchand
  0 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-02-17 14:15 UTC (permalink / raw)
  To: David Marchand
  Cc: Yigit, Ferruh, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo


On 2021/2/17 17:06, David Marchand wrote:
> On Fri, Jan 29, 2021 at 4:19 AM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>> @@ -517,6 +525,60 @@
>>   }
>>   #endif
>>
>> +static inline uint8_t ioread8(void *addr)
>> +{
>> +       uint8_t val;
>> +
>> +       val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
>> +               *(volatile uint8_t *)addr :
>> +               inb((unsigned long)addr);
> inb/outb and others are architecture (x86?) specific.
Yes, only X86 has PIO.
>
> The CI caught this issue, see build failures on ARM64.
> https://lab.dpdk.org/results/dashboard/results/results-uploads/test_runs/82432e287bc94831b7a65d7cd6f05783/log_upload_file/2021/2/dpdk_b49c677a0d24_15433_2021-02-01_00-15-59_NA.zip
>
> I can see the same issue with ppc64le.
>
>
> --
> David Marchand

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

* Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-02-17 14:15     ` 谢华伟(此时此刻)
@ 2021-02-18  9:33       ` David Marchand
  0 siblings, 0 replies; 61+ messages in thread
From: David Marchand @ 2021-02-18  9:33 UTC (permalink / raw)
  To: 谢华伟(此时此刻)
  Cc: Yigit, Ferruh, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo

On Wed, Feb 17, 2021 at 3:15 PM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>
>
> On 2021/2/17 17:06, David Marchand wrote:
> > On Fri, Jan 29, 2021 at 4:19 AM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
> >> @@ -517,6 +525,60 @@
> >>   }
> >>   #endif
> >>
> >> +static inline uint8_t ioread8(void *addr)
> >> +{
> >> +       uint8_t val;
> >> +
> >> +       val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
> >> +               *(volatile uint8_t *)addr :
> >> +               inb((unsigned long)addr);
> > inb/outb and others are architecture (x86?) specific.
> Yes, only X86 has PIO.

Ok, thanks for confirming.

> >
> > The CI caught this issue, see build failures on ARM64.
> > https://lab.dpdk.org/results/dashboard/results/results-uploads/test_runs/82432e287bc94831b7a65d7cd6f05783/log_upload_file/2021/2/dpdk_b49c677a0d24_15433_2021-02-01_00-15-59_NA.zip
> >
> > I can see the same issue with ppc64le.

Just to be clear.
This series can't be merged as it breaks non-x86 architectures.


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-01-29  3:18 ` [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
  2021-02-03  9:37   ` Maxime Coquelin
@ 2021-02-18  9:33   ` David Marchand
  2021-02-21 15:58     ` 谢华伟(此时此刻)
  1 sibling, 1 reply; 61+ messages in thread
From: David Marchand @ 2021-02-18  9:33 UTC (permalink / raw)
  To: 谢华伟(此时此刻)
  Cc: Yigit, Ferruh, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo

On Fri, Jan 29, 2021 at 4:19 AM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>
> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>
>
> Currently virtio PMD asssumes legacy device uses PIO bar.
> There are three ways to get PIO(PortIO) address for virtio legacy device.
>     under igb_uio, get pio address from uio/uio# sysfs attribute
>     under uio_pci_generic:
>         for X86, get PIO address from /proc/ioport
>         for other ARCH, get PIO address from standard PCI sysfs attribute
>
> Actually, igb_uio sysfs attribute exports exactly the same thing as standard PCI sysfs, i.e,
>     pci_dev->resource[]
>
> This patch fixes these messy things, and uses standard PCI sysfs attribute.

I can not find what is being fixed.
Could you elaborate?


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-02-09 14:51   ` Ferruh Yigit
@ 2021-02-19  8:52     ` Ferruh Yigit
  2021-02-21 15:45       ` 谢华伟(此时此刻)
  0 siblings, 1 reply; 61+ messages in thread
From: Ferruh Yigit @ 2021-02-19  8:52 UTC (permalink / raw)
  To: 谢华伟(此时此刻),
	maxime.coquelin
  Cc: dev, david.marchand, anatoly.burakov, xuemingl, grive, chenbo.xia

On 2/9/2021 2:51 PM, Ferruh Yigit wrote:
> On 1/29/2021 3:18 AM, 谢华伟(此时此刻) wrote:
>> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>
>>
>> With IO BAR, we get PIO(programmed IO) address.
>> With MMIO BAR, we get mapped virtual address.
>> We distinguish PIO(Programmed IO) and MMIO(memory mapped IO) by their address 
>> like how kernel does.
>> ioread/write8/16/32 is provided to access PIO/MMIO.
>> By the way, for virtio on arch other than x86, BAR flag indicates PIO but is 
>> mapped.
>>
>> Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> <...>
> 
>> +static inline void iowrite8(uint8_t val, void *addr)
>> +{
>> +    (uint64_t)(uintptr_t)addr >= PIO_MAX ?
>> +        *(volatile uint8_t *)addr = val :
>> +        outb(val, (unsigned long)addr);
> 
> Is the 'outb_p' to 'outb' conversion intentional? And if so why?
> 
> Same of the all 'outb_p', 'outw_p', 'outl_p'.
> 

Reminder of above question.

Let's try to close this patch before release pressure hit again.
And as far as I understand already a new version is required for build errors on 
non x86 architectures.

> <...>
> 
>>               size = 1;
>> -#if defined(RTE_ARCH_X86)
>> -            outb_p(*s, reg);
>> -#else
>> -            *(volatile uint8_t *)reg = *s;
>> -#endif
>> +            iowrite8(*s, (void *)reg);
>>           }
>>       }
>>   }
>>
> 


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

* Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-02-19  8:52     ` Ferruh Yigit
@ 2021-02-21 15:45       ` 谢华伟(此时此刻)
  0 siblings, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-02-21 15:45 UTC (permalink / raw)
  To: Ferruh Yigit, maxime.coquelin, David Marchand
  Cc: dev, david.marchand, anatoly.burakov, xuemingl, grive, chenbo.xia


On 2021/2/19 16:52, Ferruh Yigit wrote:
> On 2/9/2021 2:51 PM, Ferruh Yigit wrote:
>> On 1/29/2021 3:18 AM, 谢华伟(此时此刻) wrote:
>>> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>
>>>
>>> With IO BAR, we get PIO(programmed IO) address.
>>> With MMIO BAR, we get mapped virtual address.
>>> We distinguish PIO(Programmed IO) and MMIO(memory mapped IO) by 
>>> their address like how kernel does.
>>> ioread/write8/16/32 is provided to access PIO/MMIO.
>>> By the way, for virtio on arch other than x86, BAR flag indicates 
>>> PIO but is mapped.
>>>
>>> Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> <...>
>>
>>> +static inline void iowrite8(uint8_t val, void *addr)
>>> +{
>>> +    (uint64_t)(uintptr_t)addr >= PIO_MAX ?
>>> +        *(volatile uint8_t *)addr = val :
>>> +        outb(val, (unsigned long)addr);
>>
>> Is the 'outb_p' to 'outb' conversion intentional? And if so why?
>>
>> Same of the all 'outb_p', 'outw_p', 'outl_p'.
>>
>
> Reminder of above question.
>
> Let's try to close this patch before release pressure hit again.
> And as far as I understand already a new version is required for build 
> errors on non x86 architectures.

I will check how to fix the arch issue.

>
>> <...>
>>
>>>               size = 1;
>>> -#if defined(RTE_ARCH_X86)
>>> -            outb_p(*s, reg);
>>> -#else
>>> -            *(volatile uint8_t *)reg = *s;
>>> -#endif
>>> +            iowrite8(*s, (void *)reg);
>>>           }
>>>       }
>>>   }
>>>
>>

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

* Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-02-18  9:33   ` David Marchand
@ 2021-02-21 15:58     ` 谢华伟(此时此刻)
  2021-02-24 12:49       ` David Marchand
  0 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-02-21 15:58 UTC (permalink / raw)
  To: David Marchand
  Cc: Yigit, Ferruh, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo


On 2021/2/18 17:33, David Marchand wrote:
> On Fri, Jan 29, 2021 at 4:19 AM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>
>>
>> Currently virtio PMD asssumes legacy device uses PIO bar.
>> There are three ways to get PIO(PortIO) address for virtio legacy device.
>>      under igb_uio, get pio address from uio/uio# sysfs attribute
>>      under uio_pci_generic:
>>          for X86, get PIO address from /proc/ioport
>>          for other ARCH, get PIO address from standard PCI sysfs attribute
>>
>> Actually, igb_uio sysfs attribute exports exactly the same thing as standard PCI sysfs, i.e,
>>      pci_dev->resource[]
>>
>> This patch fixes these messy things, and uses standard PCI sysfs attribute.
> I can not find what is being fixed.
> Could you elaborate?

Maybe i wrongly use the word fix?  With this patch, different drivers 
use the same way to get IO address for virtio device.

Previously different driver get IO address with different method.

>

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

* [dpdk-dev] [PATCH v7 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
  2021-01-29  3:18 [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
                   ` (2 preceding siblings ...)
  2021-01-29  3:25 ` [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
@ 2021-02-22 17:15 ` 谢华伟(此时此刻)
  2021-02-22 17:15   ` [dpdk-dev] [PATCH v7 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
                     ` (2 more replies)
  3 siblings, 3 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-02-22 17:15 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>

virtio PMD assumes legacy device only supports PIO BAR resource. This is wrong.
As we need to create lots of devices, as PIO resource on x86 is very limited, 
we expose MMIO(memory IO) BAR.

Kernel supports both PIO and MMIO BAR for legacy virtio-pci device, and for all
other pci devices. This patchset handles different type of BAR in the similar way.

In previous implementation, under igb_uio driver we get PIO address from igb_uio
sysfs entry; with uio_pci_generic, we get PIO address from /proc/ioports for x86,
and for other ARCHs, we get PIO address from standard PCI sysfs entry.
For PIO/MMIO RW, there is different path for different drivers and arch.


All of the above is too much twisted.
This patchset unifies the way to get both PIO and MMIO address for different driver
and ARCHs, all from standard resource attr under pci sysfs. This is most generic.

We distinguish PIO and MMIO by their address like how kernel does. It is ugly but works.

v2 changes:
    - add more explanation in the commit message

v3 changes:
    - fix patch format issues

v4 changes:
    - fixes for RTE_KDRV_UIO_GENERIC -> RTE_PCI_KDRV_UIO_GENERIC

v5 changes:
    - split into three seperate patches

v6 changes:
    - change to DEBUG level for IO bar detection in pci_uio_ioport_map
    - rework the code in iobar branch
    - fixes commit message format issue
    - temporarily remove the 3rd patch for vfio path, leave it for future discusssion
    - rework against virtio_pmd_rework_v2

v7 changes:
    - fix compilation issues of in/out instruction on non X86 archs

huawei.xhw (2):
  bus/pci: use PCI standard sysfs entry to get PIO address
  bus/pci: support MMIO in PCI ioport accessors

 drivers/bus/pci/linux/pci.c     |  81 ----------------
 drivers/bus/pci/linux/pci_uio.c | 202 +++++++++++++++++++++++++++++-----------
 2 files changed, 150 insertions(+), 133 deletions(-)

-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v7 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-02-22 17:15 ` [dpdk-dev] [PATCH v7 " 谢华伟(此时此刻)
@ 2021-02-22 17:15   ` 谢华伟(此时此刻)
  2021-02-22 17:15   ` [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
  2021-03-01 16:01   ` [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
  2 siblings, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-02-22 17:15 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>

Currently virtio PMD asssumes legacy device uses PIO bar.
There are three ways to get PIO(PortIO) address for virtio legacy device.
    under igb_uio, get pio address from uio/uio# sysfs attribute
    under uio_pci_generic:
        for X86, get PIO address from /proc/ioport
        for other ARCH, get PIO address from standard PCI sysfs attribute

Actually, igb_uio sysfs attribute exports exactly the same thing as standard PCI sysfs, i.e,
    pci_dev->resource[]

This patch fixes these messy things, and uses standard PCI sysfs attribute.

Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/pci/linux/pci.c     | 77 -----------------------------------------
 drivers/bus/pci/linux/pci_uio.c | 64 ++++++++++++++++++++++++----------
 2 files changed, 46 insertions(+), 95 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 2e1808b..0f38abf 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -677,71 +677,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 	}
 }
 
-#if defined(RTE_ARCH_X86)
-static int
-pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
-		struct rte_pci_ioport *p)
-{
-	uint16_t start, end;
-	FILE *fp;
-	char *line = NULL;
-	char pci_id[16];
-	int found = 0;
-	size_t linesz;
-
-	if (rte_eal_iopl_init() != 0) {
-		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
-			__func__, dev->name);
-		return -1;
-	}
-
-	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
-		 dev->addr.domain, dev->addr.bus,
-		 dev->addr.devid, dev->addr.function);
-
-	fp = fopen("/proc/ioports", "r");
-	if (fp == NULL) {
-		RTE_LOG(ERR, EAL, "%s(): can't open ioports\n", __func__);
-		return -1;
-	}
-
-	while (getdelim(&line, &linesz, '\n', fp) > 0) {
-		char *ptr = line;
-		char *left;
-		int n;
-
-		n = strcspn(ptr, ":");
-		ptr[n] = 0;
-		left = &ptr[n + 1];
-
-		while (*left && isspace(*left))
-			left++;
-
-		if (!strncmp(left, pci_id, strlen(pci_id))) {
-			found = 1;
-
-			while (*ptr && isspace(*ptr))
-				ptr++;
-
-			sscanf(ptr, "%04hx-%04hx", &start, &end);
-
-			break;
-		}
-	}
-
-	free(line);
-	fclose(fp);
-
-	if (!found)
-		return -1;
-
-	p->base = start;
-	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%x\n", start);
-
-	return 0;
-}
-#endif
-
 int
 rte_pci_ioport_map(struct rte_pci_device *dev, int bar,
 		struct rte_pci_ioport *p)
@@ -756,14 +691,8 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		ret = pci_uio_ioport_map(dev, bar, p);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
-#if defined(RTE_ARCH_X86)
-		ret = pci_ioport_map(dev, bar, p);
-#else
 		ret = pci_uio_ioport_map(dev, bar, p);
-#endif
 		break;
 	default:
 		break;
@@ -830,14 +759,8 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		ret = pci_uio_ioport_unmap(p);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
-#if defined(RTE_ARCH_X86)
-		ret = 0;
-#else
 		ret = pci_uio_ioport_unmap(p);
-#endif
 		break;
 	default:
 		break;
diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index f3305a2..01f2a40 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -373,10 +373,13 @@
 pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
 		   struct rte_pci_ioport *p)
 {
+	FILE *f = NULL;
 	char dirname[PATH_MAX];
 	char filename[PATH_MAX];
-	int uio_num;
-	unsigned long start;
+	char buf[BUFSIZ];
+	uint64_t phys_addr, end_addr, flags;
+	unsigned long base;
+	int i;
 
 	if (rte_eal_iopl_init() != 0) {
 		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
@@ -384,41 +387,66 @@
 		return -1;
 	}
 
-	uio_num = pci_get_uio_dev(dev, dirname, sizeof(dirname), 0);
-	if (uio_num < 0)
+	/* open and read addresses of the corresponding resource in sysfs */
+	snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
+		rte_pci_get_sysfs_path(), dev->addr.domain, dev->addr.bus,
+		dev->addr.devid, dev->addr.function);
+	f = fopen(filename, "r");
+	if (f == NULL) {
+		RTE_LOG(ERR, EAL, "%s(): Cannot open sysfs resource: %s\n",
+			__func__, strerror(errno));
 		return -1;
+	}
 
-	/* get portio start */
-	snprintf(filename, sizeof(filename),
-		 "%s/portio/port%d/start", dirname, bar);
-	if (eal_parse_sysfs_value(filename, &start) < 0) {
-		RTE_LOG(ERR, EAL, "%s(): cannot parse portio start\n",
-			__func__);
-		return -1;
+	for (i = 0; i < bar + 1; i++) {
+		if (fgets(buf, sizeof(buf), f) == NULL) {
+			RTE_LOG(ERR, EAL, "%s(): Cannot read sysfs resource\n", __func__);
+			goto error;
+		}
 	}
-	/* ensure we don't get anything funny here, read/write will cast to
-	 * uin16_t */
-	if (start > UINT16_MAX)
-		return -1;
+	if (pci_parse_one_sysfs_resource(buf, sizeof(buf), &phys_addr,
+		&end_addr, &flags) < 0)
+		goto error;
+
+	if (!(flags & IORESOURCE_IO)) {
+		RTE_LOG(ERR, EAL, "%s(): bar resource other than IO is not supported\n", __func__);
+		goto error;
+	}
+	base = (unsigned long)phys_addr;
+	RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+
+	if (base > UINT16_MAX)
+		goto error;
 
 	/* FIXME only for primary process ? */
 	if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) {
+		int uio_num = pci_get_uio_dev(dev, dirname, sizeof(dirname), 0);
+		if (uio_num < 0) {
+			RTE_LOG(ERR, EAL, "cannot open %s: %s\n",
+				dirname, strerror(errno));
+			goto error;
+		}
 
 		snprintf(filename, sizeof(filename), "/dev/uio%u", uio_num);
 		dev->intr_handle.fd = open(filename, O_RDWR);
 		if (dev->intr_handle.fd < 0) {
 			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 				filename, strerror(errno));
-			return -1;
+			goto error;
 		}
 		dev->intr_handle.type = RTE_INTR_HANDLE_UIO;
 	}
 
-	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx\n", start);
+	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx\n", base);
 
-	p->base = start;
+	p->base = base;
 	p->len = 0;
+	fclose(f);
 	return 0;
+error:
+	if (f)
+		fclose(f);
+	return -1;
 }
 #else
 int
-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-02-22 17:15 ` [dpdk-dev] [PATCH v7 " 谢华伟(此时此刻)
  2021-02-22 17:15   ` [dpdk-dev] [PATCH v7 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
@ 2021-02-22 17:15   ` 谢华伟(此时此刻)
  2021-02-22 17:25     ` Ferruh Yigit
  2021-03-01 16:01   ` [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
  2 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-02-22 17:15 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>

With IO BAR, we get PIO(programmed IO) address.
With MMIO BAR, we get mapped virtual address.
We distinguish PIO(Programmed IO) and MMIO(memory mapped IO) by their address like how kernel does.
ioread/write8/16/32 is provided to access PIO/MMIO.
By the way, for virtio on arch other than x86, BAR flag indicates PIO but is mapped.

Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/pci/linux/pci.c     |   4 --
 drivers/bus/pci/linux/pci_uio.c | 156 +++++++++++++++++++++++++++++-----------
 2 files changed, 113 insertions(+), 47 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 0f38abf..0dc99e9 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -715,8 +715,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		pci_uio_ioport_read(p, data, len, offset);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
 		pci_uio_ioport_read(p, data, len, offset);
 		break;
@@ -736,8 +734,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		pci_uio_ioport_write(p, data, len, offset);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
 		pci_uio_ioport_write(p, data, len, offset);
 		break;
diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 01f2a40..f566953 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -368,6 +368,8 @@
 	return -1;
 }
 
+#define PIO_MAX 0x10000
+
 #if defined(RTE_ARCH_X86)
 int
 pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
@@ -381,12 +383,6 @@
 	unsigned long base;
 	int i;
 
-	if (rte_eal_iopl_init() != 0) {
-		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
-			__func__, dev->name);
-		return -1;
-	}
-
 	/* open and read addresses of the corresponding resource in sysfs */
 	snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
 		rte_pci_get_sysfs_path(), dev->addr.domain, dev->addr.bus,
@@ -408,15 +404,27 @@
 		&end_addr, &flags) < 0)
 		goto error;
 
-	if (!(flags & IORESOURCE_IO)) {
-		RTE_LOG(ERR, EAL, "%s(): bar resource other than IO is not supported\n", __func__);
-		goto error;
-	}
-	base = (unsigned long)phys_addr;
-	RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+	if (flags & IORESOURCE_IO) {
+		if (rte_eal_iopl_init()) {
+			RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
+				__func__, dev->name);
+			goto error;
+		}
 
-	if (base > UINT16_MAX)
+		base = (unsigned long)phys_addr;
+		if (base > PIO_MAX) {
+			RTE_LOG(ERR, EAL, "%s(): %08lx too large PIO resource\n", __func__, base);
+			goto error;
+		}
+
+		RTE_LOG(DEBUG, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+	} else if (flags & IORESOURCE_MEM) {
+		base = (unsigned long)dev->mem_resource[bar].addr;
+		RTE_LOG(DEBUG, EAL, "%s(): MMIO BAR %08lx detected\n", __func__, base);
+	} else {
+		RTE_LOG(ERR, EAL, "%s(): unknown BAR type\n", __func__);
 		goto error;
+	}
 
 	/* FIXME only for primary process ? */
 	if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) {
@@ -517,6 +525,92 @@
 }
 #endif
 
+#if defined(RTE_ARCH_X86)
+static inline uint8_t ioread8(void *addr)
+{
+	uint8_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint8_t *)addr :
+		inb((unsigned long)addr);
+
+	return val;
+}
+
+static inline uint16_t ioread16(void *addr)
+{
+	uint16_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint16_t *)addr :
+		inw((unsigned long)addr);
+
+	return val;
+}
+
+static inline uint32_t ioread32(void *addr)
+{
+	uint32_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint32_t *)addr :
+		inl((unsigned long)addr);
+
+	return val;
+}
+
+static inline void iowrite8(uint8_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint8_t *)addr = val :
+		outb(val, (unsigned long)addr);
+}
+
+static inline void iowrite16(uint16_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint16_t *)addr = val :
+		outw(val, (unsigned long)addr);
+}
+
+static inline void iowrite32(uint32_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint32_t *)addr = val :
+		outl(val, (unsigned long)addr);
+}
+#else
+static inline uint8_t ioread8(void *addr)
+{
+	return *(volatile uint8_t *)addr;
+}
+
+static inline uint16_t ioread16(void *addr)
+{
+	return *(volatile uint16_t *)addr;
+}
+
+static inline uint32_t ioread32(void *addr)
+{
+	return *(volatile uint32_t *)addr;
+}
+
+static inline void iowrite8(uint8_t val, void *addr)
+{
+	*(volatile uint8_t *)addr = val;
+}
+
+static inline void iowrite16(uint16_t val, void *addr)
+{
+	*(volatile uint16_t *)addr = val;
+}
+
+static inline void iowrite32(uint32_t val, void *addr)
+{
+	*(volatile uint32_t *)addr = val;
+}
+#endif
+
 void
 pci_uio_ioport_read(struct rte_pci_ioport *p,
 		    void *data, size_t len, off_t offset)
@@ -528,25 +622,13 @@
 	for (d = data; len > 0; d += size, reg += size, len -= size) {
 		if (len >= 4) {
 			size = 4;
-#if defined(RTE_ARCH_X86)
-			*(uint32_t *)d = inl(reg);
-#else
-			*(uint32_t *)d = *(volatile uint32_t *)reg;
-#endif
+			*(uint32_t *)d = ioread32((void *)reg);
 		} else if (len >= 2) {
 			size = 2;
-#if defined(RTE_ARCH_X86)
-			*(uint16_t *)d = inw(reg);
-#else
-			*(uint16_t *)d = *(volatile uint16_t *)reg;
-#endif
+			*(uint16_t *)d = ioread16((void *)reg);
 		} else {
 			size = 1;
-#if defined(RTE_ARCH_X86)
-			*d = inb(reg);
-#else
-			*d = *(volatile uint8_t *)reg;
-#endif
+			*d = ioread8((void *)reg);
 		}
 	}
 }
@@ -562,25 +644,13 @@
 	for (s = data; len > 0; s += size, reg += size, len -= size) {
 		if (len >= 4) {
 			size = 4;
-#if defined(RTE_ARCH_X86)
-			outl_p(*(const uint32_t *)s, reg);
-#else
-			*(volatile uint32_t *)reg = *(const uint32_t *)s;
-#endif
+			iowrite32(*(const uint32_t *)s, (void *)reg);
 		} else if (len >= 2) {
 			size = 2;
-#if defined(RTE_ARCH_X86)
-			outw_p(*(const uint16_t *)s, reg);
-#else
-			*(volatile uint16_t *)reg = *(const uint16_t *)s;
-#endif
+			iowrite16(*(const uint16_t *)s, (void *)reg);
 		} else {
 			size = 1;
-#if defined(RTE_ARCH_X86)
-			outb_p(*s, reg);
-#else
-			*(volatile uint8_t *)reg = *s;
-#endif
+			iowrite8(*s, (void *)reg);
 		}
 	}
 }
-- 
1.8.3.1


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

* Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-02-22 17:15   ` [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
@ 2021-02-22 17:25     ` Ferruh Yigit
  2021-02-23 14:20       ` 谢华伟(此时此刻)
  0 siblings, 1 reply; 61+ messages in thread
From: Ferruh Yigit @ 2021-02-22 17:25 UTC (permalink / raw)
  To: 谢华伟(此时此刻),
	maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia

On 2/22/2021 5:15 PM, 谢华伟(此时此刻) wrote:
> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>
> 
> With IO BAR, we get PIO(programmed IO) address.
> With MMIO BAR, we get mapped virtual address.
> We distinguish PIO(Programmed IO) and MMIO(memory mapped IO) by their address like how kernel does.
> ioread/write8/16/32 is provided to access PIO/MMIO.
> By the way, for virtio on arch other than x86, BAR flag indicates PIO but is mapped.
> 
> Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

<...>

> +
> +static inline void iowrite8(uint8_t val, void *addr)
> +{
> +	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
> +		*(volatile uint8_t *)addr = val :
> +		outb(val, (unsigned long)addr);

//copying question from previous version:

Is the 'outb_p' to 'outb' conversion intentional? And if so why?

Same of the all 'outb_p', 'outw_p', 'outl_p'.

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

* Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-02-22 17:25     ` Ferruh Yigit
@ 2021-02-23 14:20       ` 谢华伟(此时此刻)
  2021-02-24 15:45         ` Ferruh Yigit
  0 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-02-23 14:20 UTC (permalink / raw)
  To: Ferruh Yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia


On 2021/2/23 1:25, Ferruh Yigit wrote:
> On 2/22/2021 5:15 PM, 谢华伟(此时此刻) wrote:
>> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>
>>
>> With IO BAR, we get PIO(programmed IO) address.
>> With MMIO BAR, we get mapped virtual address.
>> We distinguish PIO(Programmed IO) and MMIO(memory mapped IO) by their 
>> address like how kernel does.
>> ioread/write8/16/32 is provided to access PIO/MMIO.
>> By the way, for virtio on arch other than x86, BAR flag indicates PIO 
>> but is mapped.
>>
>> Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> <...>
>
>> +
>> +static inline void iowrite8(uint8_t val, void *addr)
>> +{
>> +    (uint64_t)(uintptr_t)addr >= PIO_MAX ?
>> +        *(volatile uint8_t *)addr = val :
>> +        outb(val, (unsigned long)addr);
>
> //copying question from previous version:
>
> Is the 'outb_p' to 'outb' conversion intentional? And if so why?
>
> Same of the all 'outb_p', 'outw_p', 'outl_p'.

There is no need to delay for virtio device, as we can see in virtio 
legacy driver.

IMO, the delay is for ugly old device. The device itself should assure 
the previous IO completes when the subsequent IO instruction arrives.



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

* Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-02-21 15:58     ` 谢华伟(此时此刻)
@ 2021-02-24 12:49       ` David Marchand
  2021-02-24 15:29         ` 谢华伟(此时此刻)
  0 siblings, 1 reply; 61+ messages in thread
From: David Marchand @ 2021-02-24 12:49 UTC (permalink / raw)
  To: 谢华伟(此时此刻)
  Cc: Yigit, Ferruh, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo

On Sun, Feb 21, 2021 at 4:58 PM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>
>
> On 2021/2/18 17:33, David Marchand wrote:
> > On Fri, Jan 29, 2021 at 4:19 AM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
> >> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>
> >>
> >> Currently virtio PMD asssumes legacy device uses PIO bar.
> >> There are three ways to get PIO(PortIO) address for virtio legacy device.
> >>      under igb_uio, get pio address from uio/uio# sysfs attribute
> >>      under uio_pci_generic:
> >>          for X86, get PIO address from /proc/ioport
> >>          for other ARCH, get PIO address from standard PCI sysfs attribute
> >>
> >> Actually, igb_uio sysfs attribute exports exactly the same thing as standard PCI sysfs, i.e,
> >>      pci_dev->resource[]
> >>
> >> This patch fixes these messy things, and uses standard PCI sysfs attribute.
> > I can not find what is being fixed.
> > Could you elaborate?
>
> Maybe i wrongly use the word fix?  With this patch, different drivers
> use the same way to get IO address for virtio device.
> Previously different driver get IO address with different method.

The commitlog is confusing, mentioning pci_dev->resource[] (I guess
from the kernel sources?) and talking about a fix while it works fine
so far.
So this is a refactoring.

Did you check that virtio devices bound to uio_pci_generic still works
with legacy mode + PIO?


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-02-24 12:49       ` David Marchand
@ 2021-02-24 15:29         ` 谢华伟(此时此刻)
  2021-02-24 17:52           ` David Marchand
  0 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-02-24 15:29 UTC (permalink / raw)
  To: David Marchand
  Cc: Yigit, Ferruh, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo


On 2021/2/24 20:49, David Marchand wrote:
> On Sun, Feb 21, 2021 at 4:58 PM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>>
>> On 2021/2/18 17:33, David Marchand wrote:
>>> On Fri, Jan 29, 2021 at 4:19 AM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>>>> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>
>>>>
>>>> Currently virtio PMD asssumes legacy device uses PIO bar.
>>>> There are three ways to get PIO(PortIO) address for virtio legacy device.
>>>>       under igb_uio, get pio address from uio/uio# sysfs attribute
>>>>       under uio_pci_generic:
>>>>           for X86, get PIO address from /proc/ioport
>>>>           for other ARCH, get PIO address from standard PCI sysfs attribute
>>>>
>>>> Actually, igb_uio sysfs attribute exports exactly the same thing as standard PCI sysfs, i.e,
>>>>       pci_dev->resource[]
>>>>
>>>> This patch fixes these messy things, and uses standard PCI sysfs attribute.
>>> I can not find what is being fixed.
>>> Could you elaborate?
>> Maybe i wrongly use the word fix?  With this patch, different drivers
>> use the same way to get IO address for virtio device.
>> Previously different driver get IO address with different method.
> The commitlog is confusing, mentioning pci_dev->resource[] (I guess
> from the kernel sources?) and talking about a fix while it works fine
Yes.
> so far.
> So this is a refactoring.
Ok, will change the word fix.
>
> Did you check that virtio devices bound to uio_pci_generic still works
> with legacy mode + PIO?

I had verified PIO, might under igb_uio driver.

Theoretically it works with uio_pci_generic driver. I could do the test 
if needed. Give me some time.  Have to create a legacy VM. Now bars in 
virtio device in the cloud are basically mmio.

>
>

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

* Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-02-23 14:20       ` 谢华伟(此时此刻)
@ 2021-02-24 15:45         ` Ferruh Yigit
  2021-02-25  3:59           ` 谢华伟(此时此刻)
  0 siblings, 1 reply; 61+ messages in thread
From: Ferruh Yigit @ 2021-02-24 15:45 UTC (permalink / raw)
  To: 谢华伟(此时此刻),
	maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia

On 2/23/2021 2:20 PM, 谢华伟(此时此刻) wrote:
> 
> On 2021/2/23 1:25, Ferruh Yigit wrote:
>> On 2/22/2021 5:15 PM, 谢华伟(此时此刻) wrote:
>>> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>
>>>
>>> With IO BAR, we get PIO(programmed IO) address.
>>> With MMIO BAR, we get mapped virtual address.
>>> We distinguish PIO(Programmed IO) and MMIO(memory mapped IO) by their address 
>>> like how kernel does.
>>> ioread/write8/16/32 is provided to access PIO/MMIO.
>>> By the way, for virtio on arch other than x86, BAR flag indicates PIO but is 
>>> mapped.
>>>
>>> Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> <...>
>>
>>> +
>>> +static inline void iowrite8(uint8_t val, void *addr)
>>> +{
>>> +    (uint64_t)(uintptr_t)addr >= PIO_MAX ?
>>> +        *(volatile uint8_t *)addr = val :
>>> +        outb(val, (unsigned long)addr);
>>
>> //copying question from previous version:
>>
>> Is the 'outb_p' to 'outb' conversion intentional? And if so why?
>>
>> Same of the all 'outb_p', 'outw_p', 'outl_p'.
> 
> There is no need to delay for virtio device, as we can see in virtio legacy driver.
> 
> IMO, the delay is for ugly old device. The device itself should assure the 
> previous IO completes when the subsequent IO instruction arrives.
> 

Can there be any virtio legacy device needing this?

What is the downside of using "pause until the I/O completes" versions?

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

* Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-02-24 15:29         ` 谢华伟(此时此刻)
@ 2021-02-24 17:52           ` David Marchand
  2021-03-01 15:47             ` 谢华伟(此时此刻)
  2021-03-02 12:31             ` 谢华伟(此时此刻)
  0 siblings, 2 replies; 61+ messages in thread
From: David Marchand @ 2021-02-24 17:52 UTC (permalink / raw)
  To: 谢华伟(此时此刻)
  Cc: Yigit, Ferruh, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo

On Wed, Feb 24, 2021 at 4:29 PM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
> > Did you check that virtio devices bound to uio_pci_generic still works
> > with legacy mode + PIO?
>
> I had verified PIO, might under igb_uio driver.

Well, if you are unsure, please retest both cases, igb_uio and uio_pci_generic.


>
> Theoretically it works with uio_pci_generic driver. I could do the test
> if needed. Give me some time.  Have to create a legacy VM. Now bars in
> virtio device in the cloud are basically mmio.

Sure, take the time to check.
I'll wait for your confirmation, thanks.


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-02-24 15:45         ` Ferruh Yigit
@ 2021-02-25  3:59           ` 谢华伟(此时此刻)
  2021-02-25  9:52             ` David Marchand
  0 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-02-25  3:59 UTC (permalink / raw)
  To: Ferruh Yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia


On 2021/2/24 23:45, Ferruh Yigit wrote:
> On 2/23/2021 2:20 PM, 谢华伟(此时此刻) wrote:
>>
>> On 2021/2/23 1:25, Ferruh Yigit wrote:
>>> On 2/22/2021 5:15 PM, 谢华伟(此时此刻) wrote:
>>>> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>
>>>>
>>>> With IO BAR, we get PIO(programmed IO) address.
>>>> With MMIO BAR, we get mapped virtual address.
>>>> We distinguish PIO(Programmed IO) and MMIO(memory mapped IO) by 
>>>> their address like how kernel does.
>>>> ioread/write8/16/32 is provided to access PIO/MMIO.
>>>> By the way, for virtio on arch other than x86, BAR flag indicates 
>>>> PIO but is mapped.
>>>>
>>>> Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
>>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>
>>> <...>
>>>
>>>> +
>>>> +static inline void iowrite8(uint8_t val, void *addr)
>>>> +{
>>>> +    (uint64_t)(uintptr_t)addr >= PIO_MAX ?
>>>> +        *(volatile uint8_t *)addr = val :
>>>> +        outb(val, (unsigned long)addr);
>>>
>>> //copying question from previous version:
>>>
>>> Is the 'outb_p' to 'outb' conversion intentional? And if so why?
>>>
>>> Same of the all 'outb_p', 'outw_p', 'outl_p'.
>>
>> There is no need to delay for virtio device, as we can see in virtio 
>> legacy driver.
>>
>> IMO, the delay is for ugly old device. The device itself should 
>> assure the previous IO completes when the subsequent IO instruction 
>> arrives.
>>
>
> Can there be any virtio legacy device needing this?

The pause version delays sometime by writing to 0x80 debug port. virtio 
doesn't need this. virtio legacy PMD driver doens't use this.

Any device relying on this i think is buggy. How could the device rely 
on some uncertain cpu cycles to behave correct?

>
> What is the downside of using "pause until the I/O completes" versions?

The downside in virtio PMD is a small performance penalty when we use it 
to notify backend. CPU executes unnecessary serializing IO instruction.

I check kernel code, io wrapper for in/out doesn't use p version.


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

* Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-02-25  3:59           ` 谢华伟(此时此刻)
@ 2021-02-25  9:52             ` David Marchand
  2021-03-01 15:43               ` 谢华伟(此时此刻)
  0 siblings, 1 reply; 61+ messages in thread
From: David Marchand @ 2021-02-25  9:52 UTC (permalink / raw)
  To: 谢华伟(此时此刻)
  Cc: Ferruh Yigit, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo

On Thu, Feb 25, 2021 at 5:00 AM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
> >>> Is the 'outb_p' to 'outb' conversion intentional? And if so why?
> >>>
> >>> Same of the all 'outb_p', 'outw_p', 'outl_p'.
> >>
> >> There is no need to delay for virtio device, as we can see in virtio
> >> legacy driver.
> >>
> >> IMO, the delay is for ugly old device. The device itself should
> >> assure the previous IO completes when the subsequent IO instruction
> >> arrives.
> >>
> >
> > Can there be any virtio legacy device needing this?
>
> The pause version delays sometime by writing to 0x80 debug port. virtio
> doesn't need this. virtio legacy PMD driver doens't use this.
>
> Any device relying on this i think is buggy. How could the device rely
> on some uncertain cpu cycles to behave correct?
>
> >
> > What is the downside of using "pause until the I/O completes" versions?
>
> The downside in virtio PMD is a small performance penalty when we use it
> to notify backend. CPU executes unnecessary serializing IO instruction.
>
> I check kernel code, io wrapper for in/out doesn't use p version.

This change is a fix/optimisation.
This is a separate topic from adding MMIO support with x86 ioport.
I would split as a separate patch.


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-02-25  9:52             ` David Marchand
@ 2021-03-01 15:43               ` 谢华伟(此时此刻)
  2021-03-02 13:14                 ` David Marchand
  0 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-01 15:43 UTC (permalink / raw)
  To: David Marchand
  Cc: Ferruh Yigit, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo


On 2021/2/25 17:52, David Marchand wrote:
> On Thu, Feb 25, 2021 at 5:00 AM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>>>>> Is the 'outb_p' to 'outb' conversion intentional? And if so why?
>>>>>
>>>>> Same of the all 'outb_p', 'outw_p', 'outl_p'.
>>>> There is no need to delay for virtio device, as we can see in virtio
>>>> legacy driver.
>>>>
>>>> IMO, the delay is for ugly old device. The device itself should
>>>> assure the previous IO completes when the subsequent IO instruction
>>>> arrives.
>>>>
>>> Can there be any virtio legacy device needing this?
>> The pause version delays sometime by writing to 0x80 debug port. virtio
>> doesn't need this. virtio legacy PMD driver doens't use this.
>>
>> Any device relying on this i think is buggy. How could the device rely
>> on some uncertain cpu cycles to behave correct?
>>
>>> What is the downside of using "pause until the I/O completes" versions?
>> The downside in virtio PMD is a small performance penalty when we use it
>> to notify backend. CPU executes unnecessary serializing IO instruction.
>>
>> I check kernel code, io wrapper for in/out doesn't use p version.
> This change is a fix/optimisation.
> This is a separate topic from adding MMIO support with x86 ioport.
> I would split as a separate patch.

Hi David:

Maybe there is confuse? There is no change. The out/in is added. I don't 
remove _p on purpose.

>
>

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

* Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-02-24 17:52           ` David Marchand
@ 2021-03-01 15:47             ` 谢华伟(此时此刻)
  2021-03-02 12:31             ` 谢华伟(此时此刻)
  1 sibling, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-01 15:47 UTC (permalink / raw)
  To: David Marchand
  Cc: Yigit, Ferruh, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo


On 2021/2/25 1:52, David Marchand wrote:
> On Wed, Feb 24, 2021 at 4:29 PM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>>> Did you check that virtio devices bound to uio_pci_generic still works
>>> with legacy mode + PIO?
>> I had verified PIO, might under igb_uio driver.
> Well, if you are unsure, please retest both cases, igb_uio and uio_pci_generic.
>
I still haven't got a VM with PIO, but retested MMIO with both igb_uio 
and uio_pci_generic. They both works.

@Chenbo

Could you help run  with PIO?

>> Theoretically it works with uio_pci_generic driver. I could do the test
>> if needed. Give me some time.  Have to create a legacy VM. Now bars in
>> virtio device in the cloud are basically mmio.
> Sure, take the time to check.
> I'll wait for your confirmation, thanks.
>
>

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

* [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
  2021-02-22 17:15 ` [dpdk-dev] [PATCH v7 " 谢华伟(此时此刻)
  2021-02-22 17:15   ` [dpdk-dev] [PATCH v7 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
  2021-02-22 17:15   ` [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
@ 2021-03-01 16:01   ` 谢华伟(此时此刻)
  2021-03-01 16:01     ` [dpdk-dev] [PATCH v8 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
                       ` (3 more replies)
  2 siblings, 4 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-01 16:01 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>

virtio PMD assumes legacy device only supports PIO BAR resource. This is wrong.
As we need to create lots of devices, as PIO resource on x86 is very limited, 
we expose MMIO(memory IO) BAR.

Kernel supports both PIO and MMIO BAR for legacy virtio-pci device, and for all
other pci devices. This patchset handles different type of BAR in the similar way.

In previous implementation, under igb_uio driver we get PIO address from igb_uio
sysfs entry; with uio_pci_generic, we get PIO address from /proc/ioports for x86,
and for other ARCHs, we get PIO address from standard PCI sysfs entry.
For PIO/MMIO RW, there is different path for different drivers and arch.


All of the above is too much twisted.
This patchset unifies the way to get both PIO and MMIO address for different driver
and ARCHs, all from standard resource attr under pci sysfs. This is most generic.

We distinguish PIO and MMIO by their address like how kernel does. It is ugly but works.

v2 changes:
    - add more explanation in the commit message

v3 changes:
    - fix patch format issues

v4 changes:
    - fixes for RTE_KDRV_UIO_GENERIC -> RTE_PCI_KDRV_UIO_GENERIC

v5 changes:
    - split into three seperate patches

v6 changes:
    - change to DEBUG level for IO bar detection in pci_uio_ioport_map
    - rework the code in iobar branch
    - fixes commit message format issue
    - temporarily remove the 3rd patch for vfio path, leave it for future discusssion
    - rework against virtio_pmd_rework_v2

v7 changes:
    - fix compilation issues of in/out instruction on non X86 archs

v8 changes:
    - change the word fix to refactor in patch 1's commit message

huawei.xhw (2):
  bus/pci: use PCI standard sysfs entry to get PIO address
  bus/pci: support MMIO in PCI ioport accessors

 drivers/bus/pci/linux/pci.c     |  81 ----------------
 drivers/bus/pci/linux/pci_uio.c | 202 +++++++++++++++++++++++++++++-----------
 2 files changed, 150 insertions(+), 133 deletions(-)

-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v8 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-03-01 16:01   ` [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
@ 2021-03-01 16:01     ` 谢华伟(此时此刻)
  2021-03-01 16:01     ` [dpdk-dev] [PATCH v8 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-01 16:01 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>

Currently virtio PMD asssumes legacy device uses PIO bar.
There are three ways to get PIO(PortIO) address for virtio legacy device.
    under igb_uio, get pio address from uio/uio# sysfs attribute
    under uio_pci_generic:
        for X86, get PIO address from /proc/ioport
        for other ARCH, get PIO address from standard PCI sysfs attribute

Actually, igb_uio sysfs attribute exports exactly the same thing as standard PCI sysfs, i.e,
    pci_dev->resource[] in kernel source code

This patch refactors these messy things, and uses standard PCI sysfs attribute.

Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/pci/linux/pci.c     | 77 -----------------------------------------
 drivers/bus/pci/linux/pci_uio.c | 64 ++++++++++++++++++++++++----------
 2 files changed, 46 insertions(+), 95 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 2e1808b..0f38abf 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -677,71 +677,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 	}
 }
 
-#if defined(RTE_ARCH_X86)
-static int
-pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
-		struct rte_pci_ioport *p)
-{
-	uint16_t start, end;
-	FILE *fp;
-	char *line = NULL;
-	char pci_id[16];
-	int found = 0;
-	size_t linesz;
-
-	if (rte_eal_iopl_init() != 0) {
-		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
-			__func__, dev->name);
-		return -1;
-	}
-
-	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
-		 dev->addr.domain, dev->addr.bus,
-		 dev->addr.devid, dev->addr.function);
-
-	fp = fopen("/proc/ioports", "r");
-	if (fp == NULL) {
-		RTE_LOG(ERR, EAL, "%s(): can't open ioports\n", __func__);
-		return -1;
-	}
-
-	while (getdelim(&line, &linesz, '\n', fp) > 0) {
-		char *ptr = line;
-		char *left;
-		int n;
-
-		n = strcspn(ptr, ":");
-		ptr[n] = 0;
-		left = &ptr[n + 1];
-
-		while (*left && isspace(*left))
-			left++;
-
-		if (!strncmp(left, pci_id, strlen(pci_id))) {
-			found = 1;
-
-			while (*ptr && isspace(*ptr))
-				ptr++;
-
-			sscanf(ptr, "%04hx-%04hx", &start, &end);
-
-			break;
-		}
-	}
-
-	free(line);
-	fclose(fp);
-
-	if (!found)
-		return -1;
-
-	p->base = start;
-	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%x\n", start);
-
-	return 0;
-}
-#endif
-
 int
 rte_pci_ioport_map(struct rte_pci_device *dev, int bar,
 		struct rte_pci_ioport *p)
@@ -756,14 +691,8 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		ret = pci_uio_ioport_map(dev, bar, p);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
-#if defined(RTE_ARCH_X86)
-		ret = pci_ioport_map(dev, bar, p);
-#else
 		ret = pci_uio_ioport_map(dev, bar, p);
-#endif
 		break;
 	default:
 		break;
@@ -830,14 +759,8 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		ret = pci_uio_ioport_unmap(p);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
-#if defined(RTE_ARCH_X86)
-		ret = 0;
-#else
 		ret = pci_uio_ioport_unmap(p);
-#endif
 		break;
 	default:
 		break;
diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index f3305a2..01f2a40 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -373,10 +373,13 @@
 pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
 		   struct rte_pci_ioport *p)
 {
+	FILE *f = NULL;
 	char dirname[PATH_MAX];
 	char filename[PATH_MAX];
-	int uio_num;
-	unsigned long start;
+	char buf[BUFSIZ];
+	uint64_t phys_addr, end_addr, flags;
+	unsigned long base;
+	int i;
 
 	if (rte_eal_iopl_init() != 0) {
 		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
@@ -384,41 +387,66 @@
 		return -1;
 	}
 
-	uio_num = pci_get_uio_dev(dev, dirname, sizeof(dirname), 0);
-	if (uio_num < 0)
+	/* open and read addresses of the corresponding resource in sysfs */
+	snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
+		rte_pci_get_sysfs_path(), dev->addr.domain, dev->addr.bus,
+		dev->addr.devid, dev->addr.function);
+	f = fopen(filename, "r");
+	if (f == NULL) {
+		RTE_LOG(ERR, EAL, "%s(): Cannot open sysfs resource: %s\n",
+			__func__, strerror(errno));
 		return -1;
+	}
 
-	/* get portio start */
-	snprintf(filename, sizeof(filename),
-		 "%s/portio/port%d/start", dirname, bar);
-	if (eal_parse_sysfs_value(filename, &start) < 0) {
-		RTE_LOG(ERR, EAL, "%s(): cannot parse portio start\n",
-			__func__);
-		return -1;
+	for (i = 0; i < bar + 1; i++) {
+		if (fgets(buf, sizeof(buf), f) == NULL) {
+			RTE_LOG(ERR, EAL, "%s(): Cannot read sysfs resource\n", __func__);
+			goto error;
+		}
 	}
-	/* ensure we don't get anything funny here, read/write will cast to
-	 * uin16_t */
-	if (start > UINT16_MAX)
-		return -1;
+	if (pci_parse_one_sysfs_resource(buf, sizeof(buf), &phys_addr,
+		&end_addr, &flags) < 0)
+		goto error;
+
+	if (!(flags & IORESOURCE_IO)) {
+		RTE_LOG(ERR, EAL, "%s(): bar resource other than IO is not supported\n", __func__);
+		goto error;
+	}
+	base = (unsigned long)phys_addr;
+	RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+
+	if (base > UINT16_MAX)
+		goto error;
 
 	/* FIXME only for primary process ? */
 	if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) {
+		int uio_num = pci_get_uio_dev(dev, dirname, sizeof(dirname), 0);
+		if (uio_num < 0) {
+			RTE_LOG(ERR, EAL, "cannot open %s: %s\n",
+				dirname, strerror(errno));
+			goto error;
+		}
 
 		snprintf(filename, sizeof(filename), "/dev/uio%u", uio_num);
 		dev->intr_handle.fd = open(filename, O_RDWR);
 		if (dev->intr_handle.fd < 0) {
 			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 				filename, strerror(errno));
-			return -1;
+			goto error;
 		}
 		dev->intr_handle.type = RTE_INTR_HANDLE_UIO;
 	}
 
-	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx\n", start);
+	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx\n", base);
 
-	p->base = start;
+	p->base = base;
 	p->len = 0;
+	fclose(f);
 	return 0;
+error:
+	if (f)
+		fclose(f);
+	return -1;
 }
 #else
 int
-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v8 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-03-01 16:01   ` [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
  2021-03-01 16:01     ` [dpdk-dev] [PATCH v8 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
@ 2021-03-01 16:01     ` 谢华伟(此时此刻)
  2021-03-02 12:48     ` [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
  2021-03-03 17:46     ` [dpdk-dev] [PATCH v9 " 谢华伟(此时此刻)
  3 siblings, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-01 16:01 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>

With IO BAR, we get PIO(programmed IO) address.
With MMIO BAR, we get mapped virtual address.
We distinguish PIO(Programmed IO) and MMIO(memory mapped IO) by their address like how kernel does.
ioread/write8/16/32 is provided to access PIO/MMIO.
By the way, for virtio on arch other than x86, BAR flag indicates PIO but is mapped.

Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/pci/linux/pci.c     |   4 --
 drivers/bus/pci/linux/pci_uio.c | 156 +++++++++++++++++++++++++++++-----------
 2 files changed, 113 insertions(+), 47 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 0f38abf..0dc99e9 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -715,8 +715,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		pci_uio_ioport_read(p, data, len, offset);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
 		pci_uio_ioport_read(p, data, len, offset);
 		break;
@@ -736,8 +734,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		pci_uio_ioport_write(p, data, len, offset);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
 		pci_uio_ioport_write(p, data, len, offset);
 		break;
diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 01f2a40..f566953 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -368,6 +368,8 @@
 	return -1;
 }
 
+#define PIO_MAX 0x10000
+
 #if defined(RTE_ARCH_X86)
 int
 pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
@@ -381,12 +383,6 @@
 	unsigned long base;
 	int i;
 
-	if (rte_eal_iopl_init() != 0) {
-		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
-			__func__, dev->name);
-		return -1;
-	}
-
 	/* open and read addresses of the corresponding resource in sysfs */
 	snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
 		rte_pci_get_sysfs_path(), dev->addr.domain, dev->addr.bus,
@@ -408,15 +404,27 @@
 		&end_addr, &flags) < 0)
 		goto error;
 
-	if (!(flags & IORESOURCE_IO)) {
-		RTE_LOG(ERR, EAL, "%s(): bar resource other than IO is not supported\n", __func__);
-		goto error;
-	}
-	base = (unsigned long)phys_addr;
-	RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+	if (flags & IORESOURCE_IO) {
+		if (rte_eal_iopl_init()) {
+			RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
+				__func__, dev->name);
+			goto error;
+		}
 
-	if (base > UINT16_MAX)
+		base = (unsigned long)phys_addr;
+		if (base > PIO_MAX) {
+			RTE_LOG(ERR, EAL, "%s(): %08lx too large PIO resource\n", __func__, base);
+			goto error;
+		}
+
+		RTE_LOG(DEBUG, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+	} else if (flags & IORESOURCE_MEM) {
+		base = (unsigned long)dev->mem_resource[bar].addr;
+		RTE_LOG(DEBUG, EAL, "%s(): MMIO BAR %08lx detected\n", __func__, base);
+	} else {
+		RTE_LOG(ERR, EAL, "%s(): unknown BAR type\n", __func__);
 		goto error;
+	}
 
 	/* FIXME only for primary process ? */
 	if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) {
@@ -517,6 +525,92 @@
 }
 #endif
 
+#if defined(RTE_ARCH_X86)
+static inline uint8_t ioread8(void *addr)
+{
+	uint8_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint8_t *)addr :
+		inb((unsigned long)addr);
+
+	return val;
+}
+
+static inline uint16_t ioread16(void *addr)
+{
+	uint16_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint16_t *)addr :
+		inw((unsigned long)addr);
+
+	return val;
+}
+
+static inline uint32_t ioread32(void *addr)
+{
+	uint32_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint32_t *)addr :
+		inl((unsigned long)addr);
+
+	return val;
+}
+
+static inline void iowrite8(uint8_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint8_t *)addr = val :
+		outb(val, (unsigned long)addr);
+}
+
+static inline void iowrite16(uint16_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint16_t *)addr = val :
+		outw(val, (unsigned long)addr);
+}
+
+static inline void iowrite32(uint32_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint32_t *)addr = val :
+		outl(val, (unsigned long)addr);
+}
+#else
+static inline uint8_t ioread8(void *addr)
+{
+	return *(volatile uint8_t *)addr;
+}
+
+static inline uint16_t ioread16(void *addr)
+{
+	return *(volatile uint16_t *)addr;
+}
+
+static inline uint32_t ioread32(void *addr)
+{
+	return *(volatile uint32_t *)addr;
+}
+
+static inline void iowrite8(uint8_t val, void *addr)
+{
+	*(volatile uint8_t *)addr = val;
+}
+
+static inline void iowrite16(uint16_t val, void *addr)
+{
+	*(volatile uint16_t *)addr = val;
+}
+
+static inline void iowrite32(uint32_t val, void *addr)
+{
+	*(volatile uint32_t *)addr = val;
+}
+#endif
+
 void
 pci_uio_ioport_read(struct rte_pci_ioport *p,
 		    void *data, size_t len, off_t offset)
@@ -528,25 +622,13 @@
 	for (d = data; len > 0; d += size, reg += size, len -= size) {
 		if (len >= 4) {
 			size = 4;
-#if defined(RTE_ARCH_X86)
-			*(uint32_t *)d = inl(reg);
-#else
-			*(uint32_t *)d = *(volatile uint32_t *)reg;
-#endif
+			*(uint32_t *)d = ioread32((void *)reg);
 		} else if (len >= 2) {
 			size = 2;
-#if defined(RTE_ARCH_X86)
-			*(uint16_t *)d = inw(reg);
-#else
-			*(uint16_t *)d = *(volatile uint16_t *)reg;
-#endif
+			*(uint16_t *)d = ioread16((void *)reg);
 		} else {
 			size = 1;
-#if defined(RTE_ARCH_X86)
-			*d = inb(reg);
-#else
-			*d = *(volatile uint8_t *)reg;
-#endif
+			*d = ioread8((void *)reg);
 		}
 	}
 }
@@ -562,25 +644,13 @@
 	for (s = data; len > 0; s += size, reg += size, len -= size) {
 		if (len >= 4) {
 			size = 4;
-#if defined(RTE_ARCH_X86)
-			outl_p(*(const uint32_t *)s, reg);
-#else
-			*(volatile uint32_t *)reg = *(const uint32_t *)s;
-#endif
+			iowrite32(*(const uint32_t *)s, (void *)reg);
 		} else if (len >= 2) {
 			size = 2;
-#if defined(RTE_ARCH_X86)
-			outw_p(*(const uint16_t *)s, reg);
-#else
-			*(volatile uint16_t *)reg = *(const uint16_t *)s;
-#endif
+			iowrite16(*(const uint16_t *)s, (void *)reg);
 		} else {
 			size = 1;
-#if defined(RTE_ARCH_X86)
-			outb_p(*s, reg);
-#else
-			*(volatile uint8_t *)reg = *s;
-#endif
+			iowrite8(*s, (void *)reg);
 		}
 	}
 }
-- 
1.8.3.1


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

* Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-02-24 17:52           ` David Marchand
  2021-03-01 15:47             ` 谢华伟(此时此刻)
@ 2021-03-02 12:31             ` 谢华伟(此时此刻)
  1 sibling, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-02 12:31 UTC (permalink / raw)
  To: David Marchand
  Cc: Yigit, Ferruh, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo


On 2021/2/25 1:52, David Marchand wrote:
> On Wed, Feb 24, 2021 at 4:29 PM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>>> Did you check that virtio devices bound to uio_pci_generic still works
>>> with legacy mode + PIO?
>> I had verified PIO, might under igb_uio driver.
> Well, if you are unsure, please retest both cases, igb_uio and uio_pci_generic.
>
>
>> Theoretically it works with uio_pci_generic driver. I could do the test
>> if needed. Give me some time.  Have to create a legacy VM. Now bars in
>> virtio device in the cloud are basically mmio.
> Sure, take the time to check.
> I'll wait for your confirmation, thanks.

Hi David:

Tested PIO under igb_uio and uio_pci_generic driver. Both works.

Also tested MMIO under both drivers.

>
>

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

* Re: [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
  2021-03-01 16:01   ` [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
  2021-03-01 16:01     ` [dpdk-dev] [PATCH v8 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
  2021-03-01 16:01     ` [dpdk-dev] [PATCH v8 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
@ 2021-03-02 12:48     ` 谢华伟(此时此刻)
  2021-03-02 13:01       ` Ferruh Yigit
  2021-03-02 13:17       ` David Marchand
  2021-03-03 17:46     ` [dpdk-dev] [PATCH v9 " 谢华伟(此时此刻)
  3 siblings, 2 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-02 12:48 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia

Hi David and ferru:

Any other issue integrating this patch?

On 2021/3/2 0:01, 谢华伟(此时此刻) wrote:
> From: "huawei.xhw"<huawei.xhw@alibaba-inc.com>
>
> virtio PMD assumes legacy device only supports PIO BAR resource. This is wrong.
> As we need to create lots of devices, as PIO resource on x86 is very limited,
> we expose MMIO(memory IO) BAR.
>
> Kernel supports both PIO and MMIO BAR for legacy virtio-pci device, and for all
> other pci devices. This patchset handles different type of BAR in the similar way.
>
> In previous implementation, under igb_uio driver we get PIO address from igb_uio
> sysfs entry; with uio_pci_generic, we get PIO address from /proc/ioports for x86,
> and for other ARCHs, we get PIO address from standard PCI sysfs entry.
> For PIO/MMIO RW, there is different path for different drivers and arch.

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

* Re: [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
  2021-03-02 12:48     ` [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
@ 2021-03-02 13:01       ` Ferruh Yigit
  2021-03-02 13:17       ` David Marchand
  1 sibling, 0 replies; 61+ messages in thread
From: Ferruh Yigit @ 2021-03-02 13:01 UTC (permalink / raw)
  To: 谢华伟(此时此刻),
	maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia

On 3/2/2021 12:48 PM, 谢华伟(此时此刻) wrote:

Please don't top post, message moved down.

> On 2021/3/2 0:01, 谢华伟(此时此刻) wrote:
>> From: "huawei.xhw"<huawei.xhw@alibaba-inc.com>
>>
>> virtio PMD assumes legacy device only supports PIO BAR resource. This is wrong.
>> As we need to create lots of devices, as PIO resource on x86 is very limited,
>> we expose MMIO(memory IO) BAR.
>>
>> Kernel supports both PIO and MMIO BAR for legacy virtio-pci device, and for all
>> other pci devices. This patchset handles different type of BAR in the similar 
>> way.
>>
>> In previous implementation, under igb_uio driver we get PIO address from igb_uio
>> sysfs entry; with uio_pci_generic, we get PIO address from /proc/ioports for x86,
>> and for other ARCHs, we get PIO address from standard PCI sysfs entry.
>> For PIO/MMIO RW, there is different path for different drivers and arch.
 >
 > Hi David and ferru:
 >
 > Any other issue integrating this patch?
 >

As far as I can see the 'outw_p' to 'outw' conversion is not clarified.

Before this patch, 'outw_p' was used, now 'outw' is used, right?
And this seem to optimize the performance, so the suggestion was to separate 
this change into another patch, what do you think about this?

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

* Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-03-01 15:43               ` 谢华伟(此时此刻)
@ 2021-03-02 13:14                 ` David Marchand
  2021-03-03  7:56                   ` 谢华伟(此时此刻)
  0 siblings, 1 reply; 61+ messages in thread
From: David Marchand @ 2021-03-02 13:14 UTC (permalink / raw)
  To: 谢华伟(此时此刻)
  Cc: Ferruh Yigit, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo

On Mon, Mar 1, 2021 at 4:44 PM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
> >>> What is the downside of using "pause until the I/O completes" versions?
> >> The downside in virtio PMD is a small performance penalty when we use it
> >> to notify backend. CPU executes unnecessary serializing IO instruction.
> >>
> >> I check kernel code, io wrapper for in/out doesn't use p version.
> > This change is a fix/optimisation.
> > This is a separate topic from adding MMIO support with x86 ioport.
> > I would split as a separate patch.
>
> Hi David:
>
> Maybe there is confuse? There is no change. The out/in is added. I don't
> remove _p on purpose.

Looking at v8 and repeating previous mails:

+#if defined(RTE_ARCH_X86)
...
+static inline void iowrite8(uint8_t val, void *addr)
+{
+    (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+        *(volatile uint8_t *)addr = val :
+        outb(val, (unsigned long)addr); <======
+}

[...]


-#if defined(RTE_ARCH_X86)
-            outb_p(*s, reg); <======
-#else
-            *(volatile uint8_t *)reg = *s;
-#endif
+            iowrite8(*s, (void *)reg);


This almost went unnoticed (thanks Ferruh for spotting).

Do we _need_ this change on outX_p -> outX?

I am not comfortable at touching such low level internal routines that
have been in dpdk since v1.5.0.

If there is a good reason, it has nothing to do with adding MMIO
support and must be split in a separate patch.
If there is no reason, please restore outX_p, since the safest is not to touch.


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
  2021-03-02 12:48     ` [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
  2021-03-02 13:01       ` Ferruh Yigit
@ 2021-03-02 13:17       ` David Marchand
  1 sibling, 0 replies; 61+ messages in thread
From: David Marchand @ 2021-03-02 13:17 UTC (permalink / raw)
  To: 谢华伟(此时此刻)
  Cc: Yigit, Ferruh, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo

On Tue, Mar 2, 2021 at 1:48 PM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>
> Hi David and ferru:
>
> Any other issue integrating this patch?

I just replied on patch 2.

Besides, checkpatch complains about trivial issues.
http://mails.dpdk.org/archives/test-report/2021-March/180454.html
http://mails.dpdk.org/archives/test-report/2021-March/180455.html

Please fix this in v9.

Thanks.

-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-03-02 13:14                 ` David Marchand
@ 2021-03-03  7:56                   ` 谢华伟(此时此刻)
  0 siblings, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-03  7:56 UTC (permalink / raw)
  To: David Marchand
  Cc: Ferruh Yigit, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo


On 2021/3/2 21:14, David Marchand wrote:
>>> This change is a fix/optimisation.
>>> This is a separate topic from adding MMIO support with x86 ioport.
>>> I would split as a separate patch.
>> Hi David:
>>
>> Maybe there is confuse? There is no change. The out/in is added. I don't
>> remove _p on purpose.
> Looking at v8 and repeating previous mails:
>
> +#if defined(RTE_ARCH_X86)
> ...
> +static inline void iowrite8(uint8_t val, void *addr)
> +{
> +    (uint64_t)(uintptr_t)addr >= PIO_MAX ?
> +        *(volatile uint8_t *)addr = val :
> +        outb(val, (unsigned long)addr); <======
> +}
>
> [...]
>
>
> -#if defined(RTE_ARCH_X86)
> -            outb_p(*s, reg); <======
> -#else
> -            *(volatile uint8_t *)reg = *s;
> -#endif
> +            iowrite8(*s, (void *)reg);
>
>
> This almost went unnoticed (thanks Ferruh for spotting).
>
> Do we_need_  this change on outX_p -> outX?

I understand where the confuse comes from. In the previous 
implementation, it is _p version, however i think _p is not needed.

In the initial DPDK virtio PMD, there is no _p, if my memory is correct. 
Don't know who added it, if any reason.

Anyway, i will send v9 with _p.

> I am not comfortable at touching such low level internal routines that
> have been in dpdk since v1.5.0.
>
> If there is a good reason, it has nothing to do with adding MMIO
> support and must be split in a separate patch.
> If there is no reason, please restore outX_p, since the safest is not to touch.

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

* [dpdk-dev] [PATCH v9 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
  2021-03-01 16:01   ` [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
                       ` (2 preceding siblings ...)
  2021-03-02 12:48     ` [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
@ 2021-03-03 17:46     ` 谢华伟(此时此刻)
  2021-03-03 17:46       ` [dpdk-dev] [PATCH v9 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
                         ` (3 more replies)
  3 siblings, 4 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-03 17:46 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia,
	谢华伟(此时此刻)


virtio PMD assumes legacy device only supports PIO BAR resource. This is wrong.
As we need to create lots of devices, as PIO resource on x86 is very limited, 
we expose MMIO(memory IO) BAR.

Kernel supports both PIO and MMIO BAR for legacy virtio-pci device, and for all
other pci devices. This patchset handles different type of BAR in the similar way.

In previous implementation, under igb_uio driver we get PIO address from igb_uio
sysfs entry; with uio_pci_generic, we get PIO address from /proc/ioports for x86,
and for other ARCHs, we get PIO address from standard PCI sysfs entry.
For PIO/MMIO RW, there is different path for different drivers and arch.


All of the above is too much twisted.
This patchset unifies the way to get both PIO and MMIO address for different driver
and ARCHs, all from standard resource attr under pci sysfs. This is most generic.

We distinguish PIO and MMIO by their address like how kernel does. It is ugly but works.

v2 changes:
    - add more explanation in the commit message

v3 changes:
    - fix patch format issues

v4 changes:
    - fixes for RTE_KDRV_UIO_GENERIC -> RTE_PCI_KDRV_UIO_GENERIC

v5 changes:
    - split into three seperate patches

v6 changes:
    - change to DEBUG level for IO bar detection in pci_uio_ioport_map
    - rework the code in iobar branch
    - fixes commit message format issue
    - temporarily remove the 3rd patch for vfio path, leave it for future discusssion
    - rework against virtio_pmd_rework_v2

v7 changes:
    - fix compilation issues of in/out instruction on non X86 archs

v8 changes:
    - change the word fix to refactor in patch 1's commit message

v9 changes:
    - keep pause version in in/out instructions

huawei.xhw (2):
  bus/pci: use PCI standard sysfs entry to get PIO address
  bus/pci: support MMIO in PCI ioport accessors

 drivers/bus/pci/linux/pci.c     |  81 ----------------
 drivers/bus/pci/linux/pci_uio.c | 202 +++++++++++++++++++++++++++++-----------
 2 files changed, 150 insertions(+), 133 deletions(-)

-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v9 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-03-03 17:46     ` [dpdk-dev] [PATCH v9 " 谢华伟(此时此刻)
@ 2021-03-03 17:46       ` 谢华伟(此时此刻)
  2021-03-03 17:46       ` [dpdk-dev] [PATCH v9 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-03 17:46 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>

Currently virtio PMD asssumes legacy device uses PIO bar.
There are three ways to get PIO(PortIO) address for virtio legacy device.
    under igb_uio, get pio address from uio/uio# sysfs attribute
    under uio_pci_generic:
        for X86, get PIO address from /proc/ioport
        for other ARCH, get PIO address from standard PCI sysfs attribute

Actually, igb_uio sysfs attribute exports exactly the same thing as standard PCI sysfs, i.e,
    pci_dev->resource[] in kernel source code

This patch refactors these messy things, and uses standard PCI sysfs attribute.

Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/pci/linux/pci.c     | 77 -----------------------------------------
 drivers/bus/pci/linux/pci_uio.c | 64 ++++++++++++++++++++++++----------
 2 files changed, 46 insertions(+), 95 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 2e1808b..0f38abf 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -677,71 +677,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 	}
 }
 
-#if defined(RTE_ARCH_X86)
-static int
-pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
-		struct rte_pci_ioport *p)
-{
-	uint16_t start, end;
-	FILE *fp;
-	char *line = NULL;
-	char pci_id[16];
-	int found = 0;
-	size_t linesz;
-
-	if (rte_eal_iopl_init() != 0) {
-		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
-			__func__, dev->name);
-		return -1;
-	}
-
-	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
-		 dev->addr.domain, dev->addr.bus,
-		 dev->addr.devid, dev->addr.function);
-
-	fp = fopen("/proc/ioports", "r");
-	if (fp == NULL) {
-		RTE_LOG(ERR, EAL, "%s(): can't open ioports\n", __func__);
-		return -1;
-	}
-
-	while (getdelim(&line, &linesz, '\n', fp) > 0) {
-		char *ptr = line;
-		char *left;
-		int n;
-
-		n = strcspn(ptr, ":");
-		ptr[n] = 0;
-		left = &ptr[n + 1];
-
-		while (*left && isspace(*left))
-			left++;
-
-		if (!strncmp(left, pci_id, strlen(pci_id))) {
-			found = 1;
-
-			while (*ptr && isspace(*ptr))
-				ptr++;
-
-			sscanf(ptr, "%04hx-%04hx", &start, &end);
-
-			break;
-		}
-	}
-
-	free(line);
-	fclose(fp);
-
-	if (!found)
-		return -1;
-
-	p->base = start;
-	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%x\n", start);
-
-	return 0;
-}
-#endif
-
 int
 rte_pci_ioport_map(struct rte_pci_device *dev, int bar,
 		struct rte_pci_ioport *p)
@@ -756,14 +691,8 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		ret = pci_uio_ioport_map(dev, bar, p);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
-#if defined(RTE_ARCH_X86)
-		ret = pci_ioport_map(dev, bar, p);
-#else
 		ret = pci_uio_ioport_map(dev, bar, p);
-#endif
 		break;
 	default:
 		break;
@@ -830,14 +759,8 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		ret = pci_uio_ioport_unmap(p);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
-#if defined(RTE_ARCH_X86)
-		ret = 0;
-#else
 		ret = pci_uio_ioport_unmap(p);
-#endif
 		break;
 	default:
 		break;
diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index f3305a2..01f2a40 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -373,10 +373,13 @@
 pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
 		   struct rte_pci_ioport *p)
 {
+	FILE *f = NULL;
 	char dirname[PATH_MAX];
 	char filename[PATH_MAX];
-	int uio_num;
-	unsigned long start;
+	char buf[BUFSIZ];
+	uint64_t phys_addr, end_addr, flags;
+	unsigned long base;
+	int i;
 
 	if (rte_eal_iopl_init() != 0) {
 		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
@@ -384,41 +387,66 @@
 		return -1;
 	}
 
-	uio_num = pci_get_uio_dev(dev, dirname, sizeof(dirname), 0);
-	if (uio_num < 0)
+	/* open and read addresses of the corresponding resource in sysfs */
+	snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
+		rte_pci_get_sysfs_path(), dev->addr.domain, dev->addr.bus,
+		dev->addr.devid, dev->addr.function);
+	f = fopen(filename, "r");
+	if (f == NULL) {
+		RTE_LOG(ERR, EAL, "%s(): Cannot open sysfs resource: %s\n",
+			__func__, strerror(errno));
 		return -1;
+	}
 
-	/* get portio start */
-	snprintf(filename, sizeof(filename),
-		 "%s/portio/port%d/start", dirname, bar);
-	if (eal_parse_sysfs_value(filename, &start) < 0) {
-		RTE_LOG(ERR, EAL, "%s(): cannot parse portio start\n",
-			__func__);
-		return -1;
+	for (i = 0; i < bar + 1; i++) {
+		if (fgets(buf, sizeof(buf), f) == NULL) {
+			RTE_LOG(ERR, EAL, "%s(): Cannot read sysfs resource\n", __func__);
+			goto error;
+		}
 	}
-	/* ensure we don't get anything funny here, read/write will cast to
-	 * uin16_t */
-	if (start > UINT16_MAX)
-		return -1;
+	if (pci_parse_one_sysfs_resource(buf, sizeof(buf), &phys_addr,
+		&end_addr, &flags) < 0)
+		goto error;
+
+	if (!(flags & IORESOURCE_IO)) {
+		RTE_LOG(ERR, EAL, "%s(): bar resource other than IO is not supported\n", __func__);
+		goto error;
+	}
+	base = (unsigned long)phys_addr;
+	RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+
+	if (base > UINT16_MAX)
+		goto error;
 
 	/* FIXME only for primary process ? */
 	if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) {
+		int uio_num = pci_get_uio_dev(dev, dirname, sizeof(dirname), 0);
+		if (uio_num < 0) {
+			RTE_LOG(ERR, EAL, "cannot open %s: %s\n",
+				dirname, strerror(errno));
+			goto error;
+		}
 
 		snprintf(filename, sizeof(filename), "/dev/uio%u", uio_num);
 		dev->intr_handle.fd = open(filename, O_RDWR);
 		if (dev->intr_handle.fd < 0) {
 			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 				filename, strerror(errno));
-			return -1;
+			goto error;
 		}
 		dev->intr_handle.type = RTE_INTR_HANDLE_UIO;
 	}
 
-	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx\n", start);
+	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx\n", base);
 
-	p->base = start;
+	p->base = base;
 	p->len = 0;
+	fclose(f);
 	return 0;
+error:
+	if (f)
+		fclose(f);
+	return -1;
 }
 #else
 int
-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v9 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-03-03 17:46     ` [dpdk-dev] [PATCH v9 " 谢华伟(此时此刻)
  2021-03-03 17:46       ` [dpdk-dev] [PATCH v9 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
@ 2021-03-03 17:46       ` 谢华伟(此时此刻)
  2021-03-03 18:24       ` [dpdk-dev] [PATCH v9 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD Stephen Hemminger
  2021-03-03 18:47       ` [dpdk-dev] [PATCH v10 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
  3 siblings, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-03 17:46 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>

With IO BAR, we get PIO(programmed IO) address.
With MMIO BAR, we get mapped virtual address.
We distinguish PIO(Programmed IO) and MMIO(memory mapped IO) by their address like how kernel does.
ioread/write8/16/32 is provided to access PIO/MMIO.
By the way, for virtio on arch other than x86, BAR flag indicates PIO but is mapped.

Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/pci/linux/pci.c     |   4 --
 drivers/bus/pci/linux/pci_uio.c | 156 +++++++++++++++++++++++++++++-----------
 2 files changed, 113 insertions(+), 47 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 0f38abf..0dc99e9 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -715,8 +715,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		pci_uio_ioport_read(p, data, len, offset);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
 		pci_uio_ioport_read(p, data, len, offset);
 		break;
@@ -736,8 +734,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		pci_uio_ioport_write(p, data, len, offset);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
 		pci_uio_ioport_write(p, data, len, offset);
 		break;
diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 01f2a40..0907051 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -368,6 +368,8 @@
 	return -1;
 }
 
+#define PIO_MAX 0x10000
+
 #if defined(RTE_ARCH_X86)
 int
 pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
@@ -381,12 +383,6 @@
 	unsigned long base;
 	int i;
 
-	if (rte_eal_iopl_init() != 0) {
-		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
-			__func__, dev->name);
-		return -1;
-	}
-
 	/* open and read addresses of the corresponding resource in sysfs */
 	snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
 		rte_pci_get_sysfs_path(), dev->addr.domain, dev->addr.bus,
@@ -408,15 +404,27 @@
 		&end_addr, &flags) < 0)
 		goto error;
 
-	if (!(flags & IORESOURCE_IO)) {
-		RTE_LOG(ERR, EAL, "%s(): bar resource other than IO is not supported\n", __func__);
-		goto error;
-	}
-	base = (unsigned long)phys_addr;
-	RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+	if (flags & IORESOURCE_IO) {
+		if (rte_eal_iopl_init()) {
+			RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
+				__func__, dev->name);
+			goto error;
+		}
 
-	if (base > UINT16_MAX)
+		base = (unsigned long)phys_addr;
+		if (base > PIO_MAX) {
+			RTE_LOG(ERR, EAL, "%s(): %08lx too large PIO resource\n", __func__, base);
+			goto error;
+		}
+
+		RTE_LOG(DEBUG, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+	} else if (flags & IORESOURCE_MEM) {
+		base = (unsigned long)dev->mem_resource[bar].addr;
+		RTE_LOG(DEBUG, EAL, "%s(): MMIO BAR %08lx detected\n", __func__, base);
+	} else {
+		RTE_LOG(ERR, EAL, "%s(): unknown BAR type\n", __func__);
 		goto error;
+	}
 
 	/* FIXME only for primary process ? */
 	if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) {
@@ -517,6 +525,92 @@
 }
 #endif
 
+#if defined(RTE_ARCH_X86)
+static inline uint8_t ioread8(void *addr)
+{
+	uint8_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint8_t *)addr :
+		inb_p((unsigned long)addr);
+
+	return val;
+}
+
+static inline uint16_t ioread16(void *addr)
+{
+	uint16_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint16_t *)addr :
+		inw_p((unsigned long)addr);
+
+	return val;
+}
+
+static inline uint32_t ioread32(void *addr)
+{
+	uint32_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint32_t *)addr :
+		inl_p((unsigned long)addr);
+
+	return val;
+}
+
+static inline void iowrite8(uint8_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint8_t *)addr = val :
+		outb_p(val, (unsigned long)addr);
+}
+
+static inline void iowrite16(uint16_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint16_t *)addr = val :
+		outw_p(val, (unsigned long)addr);
+}
+
+static inline void iowrite32(uint32_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint32_t *)addr = val :
+		outl_p(val, (unsigned long)addr);
+}
+#else
+static inline uint8_t ioread8(void *addr)
+{
+	return *(volatile uint8_t *)addr;
+}
+
+static inline uint16_t ioread16(void *addr)
+{
+	return *(volatile uint16_t *)addr;
+}
+
+static inline uint32_t ioread32(void *addr)
+{
+	return *(volatile uint32_t *)addr;
+}
+
+static inline void iowrite8(uint8_t val, void *addr)
+{
+	*(volatile uint8_t *)addr = val;
+}
+
+static inline void iowrite16(uint16_t val, void *addr)
+{
+	*(volatile uint16_t *)addr = val;
+}
+
+static inline void iowrite32(uint32_t val, void *addr)
+{
+	*(volatile uint32_t *)addr = val;
+}
+#endif
+
 void
 pci_uio_ioport_read(struct rte_pci_ioport *p,
 		    void *data, size_t len, off_t offset)
@@ -528,25 +622,13 @@
 	for (d = data; len > 0; d += size, reg += size, len -= size) {
 		if (len >= 4) {
 			size = 4;
-#if defined(RTE_ARCH_X86)
-			*(uint32_t *)d = inl(reg);
-#else
-			*(uint32_t *)d = *(volatile uint32_t *)reg;
-#endif
+			*(uint32_t *)d = ioread32((void *)reg);
 		} else if (len >= 2) {
 			size = 2;
-#if defined(RTE_ARCH_X86)
-			*(uint16_t *)d = inw(reg);
-#else
-			*(uint16_t *)d = *(volatile uint16_t *)reg;
-#endif
+			*(uint16_t *)d = ioread16((void *)reg);
 		} else {
 			size = 1;
-#if defined(RTE_ARCH_X86)
-			*d = inb(reg);
-#else
-			*d = *(volatile uint8_t *)reg;
-#endif
+			*d = ioread8((void *)reg);
 		}
 	}
 }
@@ -562,25 +644,13 @@
 	for (s = data; len > 0; s += size, reg += size, len -= size) {
 		if (len >= 4) {
 			size = 4;
-#if defined(RTE_ARCH_X86)
-			outl_p(*(const uint32_t *)s, reg);
-#else
-			*(volatile uint32_t *)reg = *(const uint32_t *)s;
-#endif
+			iowrite32(*(const uint32_t *)s, (void *)reg);
 		} else if (len >= 2) {
 			size = 2;
-#if defined(RTE_ARCH_X86)
-			outw_p(*(const uint16_t *)s, reg);
-#else
-			*(volatile uint16_t *)reg = *(const uint16_t *)s;
-#endif
+			iowrite16(*(const uint16_t *)s, (void *)reg);
 		} else {
 			size = 1;
-#if defined(RTE_ARCH_X86)
-			outb_p(*s, reg);
-#else
-			*(volatile uint8_t *)reg = *s;
-#endif
+			iowrite8(*s, (void *)reg);
 		}
 	}
 }
-- 
1.8.3.1


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

* Re: [dpdk-dev] [PATCH v9 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
  2021-03-03 17:46     ` [dpdk-dev] [PATCH v9 " 谢华伟(此时此刻)
  2021-03-03 17:46       ` [dpdk-dev] [PATCH v9 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
  2021-03-03 17:46       ` [dpdk-dev] [PATCH v9 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
@ 2021-03-03 18:24       ` Stephen Hemminger
  2021-03-04 13:45         ` 谢华伟(此时此刻)
  2021-03-03 18:47       ` [dpdk-dev] [PATCH v10 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
  3 siblings, 1 reply; 61+ messages in thread
From: Stephen Hemminger @ 2021-03-03 18:24 UTC (permalink / raw)
  To: 谢华伟(此时此刻)
  Cc: ferruh.yigit, maxime.coquelin, david.marchand, dev,
	anatoly.burakov, xuemingl, grive, chenbo.xia

On Thu, 04 Mar 2021 01:46:50 +0800
"谢华伟(此时此刻)" <huawei.xhw@alibaba-inc.com> wrote:

> virtio PMD assumes legacy device only supports PIO BAR resource. This is wrong.
> As we need to create lots of devices, as PIO resource on x86 is very limited, 
> we expose MMIO(memory IO) BAR.
> 
> Kernel supports both PIO and MMIO BAR for legacy virtio-pci device, and for all
> other pci devices. This patchset handles different type of BAR in the similar way.
> 
> In previous implementation, under igb_uio driver we get PIO address from igb_uio
> sysfs entry; with uio_pci_generic, we get PIO address from /proc/ioports for x86,
> and for other ARCHs, we get PIO address from standard PCI sysfs entry.
> For PIO/MMIO RW, there is different path for different drivers and arch.

Just to add some background. At the time virtio for DPDK was developed,
the kernel only supported legacy mode, and it required I/O ports on x86.

One concern is that, you should make sure these patches still work on
the oldest releases of Linux kernel that DPDK supports. For upstream
kernel that should be 4.4 kernel (oldest currently maintained LTS).
The Linux system requirements doc file needs update!

For distributions, the oldest version would be probably be RHEL 7.

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

* [dpdk-dev] [PATCH v10 0/2] support both PIO and MMIO BAR for legacy virito device
  2021-03-03 17:46     ` [dpdk-dev] [PATCH v9 " 谢华伟(此时此刻)
                         ` (2 preceding siblings ...)
  2021-03-03 18:24       ` [dpdk-dev] [PATCH v9 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD Stephen Hemminger
@ 2021-03-03 18:47       ` 谢华伟(此时此刻)
  2021-03-03 18:47         ` [dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
                           ` (2 more replies)
  3 siblings, 3 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-03 18:47 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia,
	谢华伟(此时此刻)

virtio PMD assumes legacy device only supports PIO(port-mapped) BAR resource.
This is wrong.  As we need to create lots of devices, as PIO resource on x86 is
very limited, we expose MMIO(memory-mapped I/O) BAR.

Kernel supports both PIO and MMIO BAR for legacy virtio-pci device, and for all
other pci devices. This patchset handles different type of BAR in the similar way.

In previous implementation, under igb_uio driver we get PIO address from igb_uio
sysfs entry; with uio_pci_generic, we get PIO address from /proc/ioports for x86,
and for other ARCHs, we get PIO address from standard PCI sysfs entry.
For PIO/MMIO RW, there is different path for different drivers and arch.

All of the above is too much twisted.
This patchset unifies the way to get both PIO and MMIO address for different driver
and ARCHs, all from standard resource attr under pci sysfs. This is most generic.

We distinguish PIO and MMIO by their address range like how kernel does.
It is ugly but works.

v2 changes:
    - add more explanation in the commit message

v3 changes:
    - fix patch format issues

v4 changes:
    - fixes for RTE_KDRV_UIO_GENERIC -> RTE_PCI_KDRV_UIO_GENERIC

v5 changes:
    - split into three seperate patches

v6 changes:
    - change to DEBUG level for IO bar detection in pci_uio_ioport_map
    - rework the code in iobar branch
    - fixes commit message format issue
    - temporarily remove the 3rd patch for vfio path, leave it for future discusssion
    - rework against virtio_pmd_rework_v2

v7 changes:
    - fix compilation issues of in/out instruction on non X86 archs

v8 changes:
    - change the word fix to refactor in patch 1's commit message

v9 changes:
    - keep pause version in in/out instructions

v10 changes:
    - trival fixes in commit message, like > 75 chars

huawei.xhw (2):
  bus/pci: use PCI standard sysfs entry to get PIO address
  bus/pci: support MMIO in PCI ioport accessors

 drivers/bus/pci/linux/pci.c     |  81 ----------------
 drivers/bus/pci/linux/pci_uio.c | 202 +++++++++++++++++++++++++++++-----------
 2 files changed, 150 insertions(+), 133 deletions(-)

-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-03-03 18:47       ` [dpdk-dev] [PATCH v10 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
@ 2021-03-03 18:47         ` 谢华伟(此时此刻)
  2021-03-05 16:17           ` 谢华伟(此时此刻)
  2021-03-03 18:47         ` [dpdk-dev] [PATCH v10 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
  2021-03-10 17:36         ` [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
  2 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-03 18:47 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>

Currently virtio PMD asssumes legacy device uses PIO bar.
There are three ways to get PIO(PortIO) address for virtio legacy device.
    under igb_uio, get pio address from uio/uio# sysfs attribute
    under uio_pci_generic:
        for X86, get PIO address from /proc/ioport
        for other ARCH, get PIO address from standard PCI sysfs attribute

Actually, igb_uio sysfs attribute exports exactly the same thing as standard
PCI sysfs, i.e, pci_dev->resource[] in kernel source code

This patch refactors these messy things, and uses standard PCI sysfs attribute.

Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/pci/linux/pci.c     | 77 -----------------------------------------
 drivers/bus/pci/linux/pci_uio.c | 64 ++++++++++++++++++++++++----------
 2 files changed, 46 insertions(+), 95 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 2e1808b..0f38abf 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -677,71 +677,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 	}
 }
 
-#if defined(RTE_ARCH_X86)
-static int
-pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
-		struct rte_pci_ioport *p)
-{
-	uint16_t start, end;
-	FILE *fp;
-	char *line = NULL;
-	char pci_id[16];
-	int found = 0;
-	size_t linesz;
-
-	if (rte_eal_iopl_init() != 0) {
-		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
-			__func__, dev->name);
-		return -1;
-	}
-
-	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
-		 dev->addr.domain, dev->addr.bus,
-		 dev->addr.devid, dev->addr.function);
-
-	fp = fopen("/proc/ioports", "r");
-	if (fp == NULL) {
-		RTE_LOG(ERR, EAL, "%s(): can't open ioports\n", __func__);
-		return -1;
-	}
-
-	while (getdelim(&line, &linesz, '\n', fp) > 0) {
-		char *ptr = line;
-		char *left;
-		int n;
-
-		n = strcspn(ptr, ":");
-		ptr[n] = 0;
-		left = &ptr[n + 1];
-
-		while (*left && isspace(*left))
-			left++;
-
-		if (!strncmp(left, pci_id, strlen(pci_id))) {
-			found = 1;
-
-			while (*ptr && isspace(*ptr))
-				ptr++;
-
-			sscanf(ptr, "%04hx-%04hx", &start, &end);
-
-			break;
-		}
-	}
-
-	free(line);
-	fclose(fp);
-
-	if (!found)
-		return -1;
-
-	p->base = start;
-	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%x\n", start);
-
-	return 0;
-}
-#endif
-
 int
 rte_pci_ioport_map(struct rte_pci_device *dev, int bar,
 		struct rte_pci_ioport *p)
@@ -756,14 +691,8 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		ret = pci_uio_ioport_map(dev, bar, p);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
-#if defined(RTE_ARCH_X86)
-		ret = pci_ioport_map(dev, bar, p);
-#else
 		ret = pci_uio_ioport_map(dev, bar, p);
-#endif
 		break;
 	default:
 		break;
@@ -830,14 +759,8 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		ret = pci_uio_ioport_unmap(p);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
-#if defined(RTE_ARCH_X86)
-		ret = 0;
-#else
 		ret = pci_uio_ioport_unmap(p);
-#endif
 		break;
 	default:
 		break;
diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index f3305a2..01f2a40 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -373,10 +373,13 @@
 pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
 		   struct rte_pci_ioport *p)
 {
+	FILE *f = NULL;
 	char dirname[PATH_MAX];
 	char filename[PATH_MAX];
-	int uio_num;
-	unsigned long start;
+	char buf[BUFSIZ];
+	uint64_t phys_addr, end_addr, flags;
+	unsigned long base;
+	int i;
 
 	if (rte_eal_iopl_init() != 0) {
 		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
@@ -384,41 +387,66 @@
 		return -1;
 	}
 
-	uio_num = pci_get_uio_dev(dev, dirname, sizeof(dirname), 0);
-	if (uio_num < 0)
+	/* open and read addresses of the corresponding resource in sysfs */
+	snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
+		rte_pci_get_sysfs_path(), dev->addr.domain, dev->addr.bus,
+		dev->addr.devid, dev->addr.function);
+	f = fopen(filename, "r");
+	if (f == NULL) {
+		RTE_LOG(ERR, EAL, "%s(): Cannot open sysfs resource: %s\n",
+			__func__, strerror(errno));
 		return -1;
+	}
 
-	/* get portio start */
-	snprintf(filename, sizeof(filename),
-		 "%s/portio/port%d/start", dirname, bar);
-	if (eal_parse_sysfs_value(filename, &start) < 0) {
-		RTE_LOG(ERR, EAL, "%s(): cannot parse portio start\n",
-			__func__);
-		return -1;
+	for (i = 0; i < bar + 1; i++) {
+		if (fgets(buf, sizeof(buf), f) == NULL) {
+			RTE_LOG(ERR, EAL, "%s(): Cannot read sysfs resource\n", __func__);
+			goto error;
+		}
 	}
-	/* ensure we don't get anything funny here, read/write will cast to
-	 * uin16_t */
-	if (start > UINT16_MAX)
-		return -1;
+	if (pci_parse_one_sysfs_resource(buf, sizeof(buf), &phys_addr,
+		&end_addr, &flags) < 0)
+		goto error;
+
+	if (!(flags & IORESOURCE_IO)) {
+		RTE_LOG(ERR, EAL, "%s(): bar resource other than IO is not supported\n", __func__);
+		goto error;
+	}
+	base = (unsigned long)phys_addr;
+	RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+
+	if (base > UINT16_MAX)
+		goto error;
 
 	/* FIXME only for primary process ? */
 	if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) {
+		int uio_num = pci_get_uio_dev(dev, dirname, sizeof(dirname), 0);
+		if (uio_num < 0) {
+			RTE_LOG(ERR, EAL, "cannot open %s: %s\n",
+				dirname, strerror(errno));
+			goto error;
+		}
 
 		snprintf(filename, sizeof(filename), "/dev/uio%u", uio_num);
 		dev->intr_handle.fd = open(filename, O_RDWR);
 		if (dev->intr_handle.fd < 0) {
 			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 				filename, strerror(errno));
-			return -1;
+			goto error;
 		}
 		dev->intr_handle.type = RTE_INTR_HANDLE_UIO;
 	}
 
-	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx\n", start);
+	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx\n", base);
 
-	p->base = start;
+	p->base = base;
 	p->len = 0;
+	fclose(f);
 	return 0;
+error:
+	if (f)
+		fclose(f);
+	return -1;
 }
 #else
 int
-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v10 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-03-03 18:47       ` [dpdk-dev] [PATCH v10 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
  2021-03-03 18:47         ` [dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
@ 2021-03-03 18:47         ` 谢华伟(此时此刻)
  2021-03-10 17:36         ` [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
  2 siblings, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-03 18:47 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia,
	谢华伟(此时此刻)

From: "huawei.xhw" <huawei.xhw@alibaba-inc.com>

With I/O BAR, we get PIO(port-mapped I/O) address.
With MMIO(memory-mapped I/O) BAR, we get mapped virtual address.
We distinguish PIO and MMIO by their address range
like how kernel does, i.e, address below 64K is PIO..
ioread/write8/16/32 is provided to access PIO/MMIO.
By the way, for virtio on arch other than x86, BAR flag indicates PIO but is mapped.

Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/pci/linux/pci.c     |   4 --
 drivers/bus/pci/linux/pci_uio.c | 156 +++++++++++++++++++++++++++++-----------
 2 files changed, 113 insertions(+), 47 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 0f38abf..0dc99e9 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -715,8 +715,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		pci_uio_ioport_read(p, data, len, offset);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
 		pci_uio_ioport_read(p, data, len, offset);
 		break;
@@ -736,8 +734,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		pci_uio_ioport_write(p, data, len, offset);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
 		pci_uio_ioport_write(p, data, len, offset);
 		break;
diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 01f2a40..0907051 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -368,6 +368,8 @@
 	return -1;
 }
 
+#define PIO_MAX 0x10000
+
 #if defined(RTE_ARCH_X86)
 int
 pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
@@ -381,12 +383,6 @@
 	unsigned long base;
 	int i;
 
-	if (rte_eal_iopl_init() != 0) {
-		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
-			__func__, dev->name);
-		return -1;
-	}
-
 	/* open and read addresses of the corresponding resource in sysfs */
 	snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
 		rte_pci_get_sysfs_path(), dev->addr.domain, dev->addr.bus,
@@ -408,15 +404,27 @@
 		&end_addr, &flags) < 0)
 		goto error;
 
-	if (!(flags & IORESOURCE_IO)) {
-		RTE_LOG(ERR, EAL, "%s(): bar resource other than IO is not supported\n", __func__);
-		goto error;
-	}
-	base = (unsigned long)phys_addr;
-	RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+	if (flags & IORESOURCE_IO) {
+		if (rte_eal_iopl_init()) {
+			RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
+				__func__, dev->name);
+			goto error;
+		}
 
-	if (base > UINT16_MAX)
+		base = (unsigned long)phys_addr;
+		if (base > PIO_MAX) {
+			RTE_LOG(ERR, EAL, "%s(): %08lx too large PIO resource\n", __func__, base);
+			goto error;
+		}
+
+		RTE_LOG(DEBUG, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+	} else if (flags & IORESOURCE_MEM) {
+		base = (unsigned long)dev->mem_resource[bar].addr;
+		RTE_LOG(DEBUG, EAL, "%s(): MMIO BAR %08lx detected\n", __func__, base);
+	} else {
+		RTE_LOG(ERR, EAL, "%s(): unknown BAR type\n", __func__);
 		goto error;
+	}
 
 	/* FIXME only for primary process ? */
 	if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) {
@@ -517,6 +525,92 @@
 }
 #endif
 
+#if defined(RTE_ARCH_X86)
+static inline uint8_t ioread8(void *addr)
+{
+	uint8_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint8_t *)addr :
+		inb_p((unsigned long)addr);
+
+	return val;
+}
+
+static inline uint16_t ioread16(void *addr)
+{
+	uint16_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint16_t *)addr :
+		inw_p((unsigned long)addr);
+
+	return val;
+}
+
+static inline uint32_t ioread32(void *addr)
+{
+	uint32_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint32_t *)addr :
+		inl_p((unsigned long)addr);
+
+	return val;
+}
+
+static inline void iowrite8(uint8_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint8_t *)addr = val :
+		outb_p(val, (unsigned long)addr);
+}
+
+static inline void iowrite16(uint16_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint16_t *)addr = val :
+		outw_p(val, (unsigned long)addr);
+}
+
+static inline void iowrite32(uint32_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint32_t *)addr = val :
+		outl_p(val, (unsigned long)addr);
+}
+#else
+static inline uint8_t ioread8(void *addr)
+{
+	return *(volatile uint8_t *)addr;
+}
+
+static inline uint16_t ioread16(void *addr)
+{
+	return *(volatile uint16_t *)addr;
+}
+
+static inline uint32_t ioread32(void *addr)
+{
+	return *(volatile uint32_t *)addr;
+}
+
+static inline void iowrite8(uint8_t val, void *addr)
+{
+	*(volatile uint8_t *)addr = val;
+}
+
+static inline void iowrite16(uint16_t val, void *addr)
+{
+	*(volatile uint16_t *)addr = val;
+}
+
+static inline void iowrite32(uint32_t val, void *addr)
+{
+	*(volatile uint32_t *)addr = val;
+}
+#endif
+
 void
 pci_uio_ioport_read(struct rte_pci_ioport *p,
 		    void *data, size_t len, off_t offset)
@@ -528,25 +622,13 @@
 	for (d = data; len > 0; d += size, reg += size, len -= size) {
 		if (len >= 4) {
 			size = 4;
-#if defined(RTE_ARCH_X86)
-			*(uint32_t *)d = inl(reg);
-#else
-			*(uint32_t *)d = *(volatile uint32_t *)reg;
-#endif
+			*(uint32_t *)d = ioread32((void *)reg);
 		} else if (len >= 2) {
 			size = 2;
-#if defined(RTE_ARCH_X86)
-			*(uint16_t *)d = inw(reg);
-#else
-			*(uint16_t *)d = *(volatile uint16_t *)reg;
-#endif
+			*(uint16_t *)d = ioread16((void *)reg);
 		} else {
 			size = 1;
-#if defined(RTE_ARCH_X86)
-			*d = inb(reg);
-#else
-			*d = *(volatile uint8_t *)reg;
-#endif
+			*d = ioread8((void *)reg);
 		}
 	}
 }
@@ -562,25 +644,13 @@
 	for (s = data; len > 0; s += size, reg += size, len -= size) {
 		if (len >= 4) {
 			size = 4;
-#if defined(RTE_ARCH_X86)
-			outl_p(*(const uint32_t *)s, reg);
-#else
-			*(volatile uint32_t *)reg = *(const uint32_t *)s;
-#endif
+			iowrite32(*(const uint32_t *)s, (void *)reg);
 		} else if (len >= 2) {
 			size = 2;
-#if defined(RTE_ARCH_X86)
-			outw_p(*(const uint16_t *)s, reg);
-#else
-			*(volatile uint16_t *)reg = *(const uint16_t *)s;
-#endif
+			iowrite16(*(const uint16_t *)s, (void *)reg);
 		} else {
 			size = 1;
-#if defined(RTE_ARCH_X86)
-			outb_p(*s, reg);
-#else
-			*(volatile uint8_t *)reg = *s;
-#endif
+			iowrite8(*s, (void *)reg);
 		}
 	}
 }
-- 
1.8.3.1


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

* Re: [dpdk-dev] [PATCH v9 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD
  2021-03-03 18:24       ` [dpdk-dev] [PATCH v9 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD Stephen Hemminger
@ 2021-03-04 13:45         ` 谢华伟(此时此刻)
  0 siblings, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-04 13:45 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: ferruh.yigit, maxime.coquelin, david.marchand, dev,
	anatoly.burakov, xuemingl, grive, chenbo.xia


On 2021/3/4 2:24, Stephen Hemminger wrote:
> On Thu, 04 Mar 2021 01:46:50 +0800
> "谢华伟(此时此刻)"<huawei.xhw@alibaba-inc.com>  wrote:
>
>> virtio PMD assumes legacy device only supports PIO BAR resource. This is wrong.
>> As we need to create lots of devices, as PIO resource on x86 is very limited,
>> we expose MMIO(memory IO) BAR.
>>
>> Kernel supports both PIO and MMIO BAR for legacy virtio-pci device, and for all
>> other pci devices. This patchset handles different type of BAR in the similar way.
>>
>> In previous implementation, under igb_uio driver we get PIO address from igb_uio
>> sysfs entry; with uio_pci_generic, we get PIO address from /proc/ioports for x86,
>> and for other ARCHs, we get PIO address from standard PCI sysfs entry.
>> For PIO/MMIO RW, there is different path for different drivers and arch.
> Just to add some background. At the time virtio for DPDK was developed,
> the kernel only supported legacy mode, and it required I/O ports on x86.

Hi Stephen:

Do you mean QEMU?  I think QEMU exports PIO by default.

For kernel, I checked 3.10 code at hand, virtio legacy driver doesn't 
assume I/O port. It uses generic API pci_iomap and ioread/write.

>
> One concern is that, you should make sure these patches still work on
> the oldest releases of Linux kernel that DPDK supports. For upstream
> kernel that should be 4.4 kernel (oldest currently maintained LTS).
> The Linux system requirements doc file needs update!
>
> For distributions, the oldest version would be probably be RHEL 7.

PIO is verified on Redhat 7.9 (3.10.0-1160.11.1.el7.x86_64).

As this patch uses the generic kernel exported interface, which all 
other PMDs have been using, i think it doesn't break compatibility.




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

* Re: [dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-03-03 18:47         ` [dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
@ 2021-03-05 16:17           ` 谢华伟(此时此刻)
  2021-03-09  6:22             ` 谢华伟(此时此刻)
  0 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-05 16:17 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia


On 2021/3/4 2:47, 谢华伟(此时此刻) wrote:
> Actually, igb_uio sysfs attribute exports exactly the same thing as standard
> PCI sysfs, i.e, pci_dev->resource[] in kernel source code
>
> This patch refactors these messy things, and uses standard PCI sysfs attribute.

Hi David:

My fault. I set vim cc=80, so there is still warnings about "unwrapped 
commit description".

WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email name mismatch: 'From: "huawei.xhw"<huawei.xhw@alibaba-inc.com>' != 'Signed-off-by: huawei xie<huawei.xhw@alibaba-inc.com>'

Does this matter? Don't know how to fix it. my git settting is already "user.name=huawei xie". Do i need to send v11 version?


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

* Re: [dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-03-05 16:17           ` 谢华伟(此时此刻)
@ 2021-03-09  6:22             ` 谢华伟(此时此刻)
  2021-03-09  7:44               ` David Marchand
  0 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-09  6:22 UTC (permalink / raw)
  To: ferruh.yigit, maxime.coquelin, david.marchand
  Cc: dev, anatoly.burakov, xuemingl, grive, chenbo.xia


On 2021/3/6 0:17, chris wrote:
>
> On 2021/3/4 2:47, 谢华伟(此时此刻) wrote:
>> Actually, igb_uio sysfs attribute exports exactly the same thing as 
>> standard
>> PCI sysfs, i.e, pci_dev->resource[] in kernel source code
>>
>> This patch refactors these messy things, and uses standard PCI sysfs 
>> attribute.
>
> Hi David:
>
> My fault. I set vim cc=80, so there is still warnings about "unwrapped 
> commit description".
>
> WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email name 
> mismatch: 'From: "huawei.xhw"<huawei.xhw@alibaba-inc.com>' != 
> 'Signed-off-by: huawei xie<huawei.xhw@alibaba-inc.com>'
>
> Does this matter? Don't know how to fix it. my git settting is already 
> "user.name=huawei xie". Do i need to send v11 version?
>
ping.

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

* Re: [dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-03-09  6:22             ` 谢华伟(此时此刻)
@ 2021-03-09  7:44               ` David Marchand
  0 siblings, 0 replies; 61+ messages in thread
From: David Marchand @ 2021-03-09  7:44 UTC (permalink / raw)
  To: 谢华伟(此时此刻)
  Cc: Yigit, Ferruh, Maxime Coquelin, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Xia, Chenbo

On Tue, Mar 9, 2021 at 7:23 AM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>
>
> On 2021/3/6 0:17, chris wrote:
> >
> > On 2021/3/4 2:47, 谢华伟(此时此刻) wrote:
> >> Actually, igb_uio sysfs attribute exports exactly the same thing as
> >> standard
> >> PCI sysfs, i.e, pci_dev->resource[] in kernel source code
> >>
> >> This patch refactors these messy things, and uses standard PCI sysfs
> >> attribute.
> >
> > Hi David:
> >
> > My fault. I set vim cc=80, so there is still warnings about "unwrapped
> > commit description".
> >
> > WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email name
> > mismatch: 'From: "huawei.xhw"<huawei.xhw@alibaba-inc.com>' !=
> > 'Signed-off-by: huawei xie<huawei.xhw@alibaba-inc.com>'
> >
> > Does this matter? Don't know how to fix it. my git settting is already
> > "user.name=huawei xie". Do i need to send v11 version?
> >
> ping.
>

Sorry, I also have other stuff to work on.
I did not forget about this series, I am waiting for more tests (Intel
QE intends to do some).

As for the trivial issues on formatting the patches, since you have
the time to send a ping, then I guess you can find the time to
configure your environment.
Thanks.


-- 
David Marchand


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

* [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device
  2021-03-03 18:47       ` [dpdk-dev] [PATCH v10 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
  2021-03-03 18:47         ` [dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
  2021-03-03 18:47         ` [dpdk-dev] [PATCH v10 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
@ 2021-03-10 17:36         ` 谢华伟(此时此刻)
  2021-03-10 17:36           ` [dpdk-dev] [PATCH v11 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
                             ` (3 more replies)
  2 siblings, 4 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-10 17:36 UTC (permalink / raw)
  To: david.marchand, maxime.coquelin, ferruh.yigit
  Cc: dev, anatoly.burakov, xuemingl, grive,
	谢华伟(此时此刻)

virtio PMD assumes legacy device only supports PIO(port-mapped) BAR
resource. This is wrong. As we need to create lots of devices, adn PIO
resource on x86 is very limited, we expose MMIO(memory-mapped I/O) BAR.

Kernel supports both PIO and MMIO BAR for legacy virtio-pci device, and
for all other pci devices. This patchset handles different type of BAR in
the similar way.

In previous implementation, under igb_uio driver we get PIO address from
igb_uio sysfs entry; with uio_pci_generic, we get PIO address from
/proc/ioports for x86, and for other ARCHs, we get PIO address from
standard PCI sysfs entry. For PIO/MMIO RW, there is different path for
different drivers and arch.

All of the above is too much twisted. This patchset unifies the way to get
both PIO and MMIO address for different driver and ARCHs, all from standard
resource attr under pci sysfs. This is most generic.

We distinguish PIO and MMIO by their address range like how kernel does.
It is ugly but works.

v2 changes:
    - add more explanation in the commit message

v3 changes:
    - fix patch format issues

v4 changes:
    - fixes for RTE_KDRV_UIO_GENERIC -> RTE_PCI_KDRV_UIO_GENERIC

v5 changes:
    - split into three seperate patches

v6 changes:
    - change to DEBUG level for IO bar detection in pci_uio_ioport_map
    - rework the code in iobar branch
    - fixes commit message format issue
    - temporarily remove the 3rd patch for vfio path, leave it for future discusssion
    - rework against virtio_pmd_rework_v2

v7 changes:
    - fix compilation issues of in/out instruction on non X86 archs

v8 changes:
    - change the word fix to refactor in patch 1's commit message

v9 changes:
    - keep pause version in in/out instructions

v10 changes:
    - trival fixes in commit message, like > 75 chars

v11 changes:
    - commit message fix and change

huawei xie (2):
  bus/pci: use PCI standard sysfs entry to get PIO address
  bus/pci: support MMIO in PCI ioport accessors

 drivers/bus/pci/linux/pci.c     |  81 ----------------
 drivers/bus/pci/linux/pci_uio.c | 202 +++++++++++++++++++++++++++++-----------
 2 files changed, 150 insertions(+), 133 deletions(-)

-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v11 1/2] bus/pci: use PCI standard sysfs entry to get PIO address
  2021-03-10 17:36         ` [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
@ 2021-03-10 17:36           ` 谢华伟(此时此刻)
  2021-03-10 17:36           ` [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
                             ` (2 subsequent siblings)
  3 siblings, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-10 17:36 UTC (permalink / raw)
  To: david.marchand, maxime.coquelin, ferruh.yigit
  Cc: dev, anatoly.burakov, xuemingl, grive,
	谢华伟(此时此刻)

Currently virtio PMD assumes legacy device uses PIO bar.
There are three ways to get PIO(PortIO) address for virtio legacy device.
    1) under igb_uio, get PIO address from uio/uio# sysfs attribute, for
instance: /sys/bus/pci/devices/0000:00:09.0/uio/uio0/portio/port0/start
    2) under uio_pci_generic
        for X86, get PIO address from /proc/ioport.
        for other ARCH, get PIO address from standard PCI sysfs attribute,
for instance: /sys/bus/pci/devices/0000:00:09.0/resource

Actually, "port0/start" in igb_uio and "resource" point to exactly the
same thing, i.e, pci_dev->resource[0] in kernel source code.

This patch refactors these messy things, and uses standard PCI sysfs
attribute "resource".

Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/pci/linux/pci.c     | 77 -----------------------------------------
 drivers/bus/pci/linux/pci_uio.c | 64 ++++++++++++++++++++++++----------
 2 files changed, 46 insertions(+), 95 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 2e1808b..0f38abf 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -677,71 +677,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 	}
 }
 
-#if defined(RTE_ARCH_X86)
-static int
-pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
-		struct rte_pci_ioport *p)
-{
-	uint16_t start, end;
-	FILE *fp;
-	char *line = NULL;
-	char pci_id[16];
-	int found = 0;
-	size_t linesz;
-
-	if (rte_eal_iopl_init() != 0) {
-		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
-			__func__, dev->name);
-		return -1;
-	}
-
-	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
-		 dev->addr.domain, dev->addr.bus,
-		 dev->addr.devid, dev->addr.function);
-
-	fp = fopen("/proc/ioports", "r");
-	if (fp == NULL) {
-		RTE_LOG(ERR, EAL, "%s(): can't open ioports\n", __func__);
-		return -1;
-	}
-
-	while (getdelim(&line, &linesz, '\n', fp) > 0) {
-		char *ptr = line;
-		char *left;
-		int n;
-
-		n = strcspn(ptr, ":");
-		ptr[n] = 0;
-		left = &ptr[n + 1];
-
-		while (*left && isspace(*left))
-			left++;
-
-		if (!strncmp(left, pci_id, strlen(pci_id))) {
-			found = 1;
-
-			while (*ptr && isspace(*ptr))
-				ptr++;
-
-			sscanf(ptr, "%04hx-%04hx", &start, &end);
-
-			break;
-		}
-	}
-
-	free(line);
-	fclose(fp);
-
-	if (!found)
-		return -1;
-
-	p->base = start;
-	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%x\n", start);
-
-	return 0;
-}
-#endif
-
 int
 rte_pci_ioport_map(struct rte_pci_device *dev, int bar,
 		struct rte_pci_ioport *p)
@@ -756,14 +691,8 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		ret = pci_uio_ioport_map(dev, bar, p);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
-#if defined(RTE_ARCH_X86)
-		ret = pci_ioport_map(dev, bar, p);
-#else
 		ret = pci_uio_ioport_map(dev, bar, p);
-#endif
 		break;
 	default:
 		break;
@@ -830,14 +759,8 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		ret = pci_uio_ioport_unmap(p);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
-#if defined(RTE_ARCH_X86)
-		ret = 0;
-#else
 		ret = pci_uio_ioport_unmap(p);
-#endif
 		break;
 	default:
 		break;
diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index f3305a2..01f2a40 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -373,10 +373,13 @@
 pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
 		   struct rte_pci_ioport *p)
 {
+	FILE *f = NULL;
 	char dirname[PATH_MAX];
 	char filename[PATH_MAX];
-	int uio_num;
-	unsigned long start;
+	char buf[BUFSIZ];
+	uint64_t phys_addr, end_addr, flags;
+	unsigned long base;
+	int i;
 
 	if (rte_eal_iopl_init() != 0) {
 		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
@@ -384,41 +387,66 @@
 		return -1;
 	}
 
-	uio_num = pci_get_uio_dev(dev, dirname, sizeof(dirname), 0);
-	if (uio_num < 0)
+	/* open and read addresses of the corresponding resource in sysfs */
+	snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
+		rte_pci_get_sysfs_path(), dev->addr.domain, dev->addr.bus,
+		dev->addr.devid, dev->addr.function);
+	f = fopen(filename, "r");
+	if (f == NULL) {
+		RTE_LOG(ERR, EAL, "%s(): Cannot open sysfs resource: %s\n",
+			__func__, strerror(errno));
 		return -1;
+	}
 
-	/* get portio start */
-	snprintf(filename, sizeof(filename),
-		 "%s/portio/port%d/start", dirname, bar);
-	if (eal_parse_sysfs_value(filename, &start) < 0) {
-		RTE_LOG(ERR, EAL, "%s(): cannot parse portio start\n",
-			__func__);
-		return -1;
+	for (i = 0; i < bar + 1; i++) {
+		if (fgets(buf, sizeof(buf), f) == NULL) {
+			RTE_LOG(ERR, EAL, "%s(): Cannot read sysfs resource\n", __func__);
+			goto error;
+		}
 	}
-	/* ensure we don't get anything funny here, read/write will cast to
-	 * uin16_t */
-	if (start > UINT16_MAX)
-		return -1;
+	if (pci_parse_one_sysfs_resource(buf, sizeof(buf), &phys_addr,
+		&end_addr, &flags) < 0)
+		goto error;
+
+	if (!(flags & IORESOURCE_IO)) {
+		RTE_LOG(ERR, EAL, "%s(): bar resource other than IO is not supported\n", __func__);
+		goto error;
+	}
+	base = (unsigned long)phys_addr;
+	RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+
+	if (base > UINT16_MAX)
+		goto error;
 
 	/* FIXME only for primary process ? */
 	if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) {
+		int uio_num = pci_get_uio_dev(dev, dirname, sizeof(dirname), 0);
+		if (uio_num < 0) {
+			RTE_LOG(ERR, EAL, "cannot open %s: %s\n",
+				dirname, strerror(errno));
+			goto error;
+		}
 
 		snprintf(filename, sizeof(filename), "/dev/uio%u", uio_num);
 		dev->intr_handle.fd = open(filename, O_RDWR);
 		if (dev->intr_handle.fd < 0) {
 			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 				filename, strerror(errno));
-			return -1;
+			goto error;
 		}
 		dev->intr_handle.type = RTE_INTR_HANDLE_UIO;
 	}
 
-	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx\n", start);
+	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx\n", base);
 
-	p->base = start;
+	p->base = base;
 	p->len = 0;
+	fclose(f);
 	return 0;
+error:
+	if (f)
+		fclose(f);
+	return -1;
 }
 #else
 int
-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-03-10 17:36         ` [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
  2021-03-10 17:36           ` [dpdk-dev] [PATCH v11 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
@ 2021-03-10 17:36           ` 谢华伟(此时此刻)
  2021-03-11  6:42             ` Wang, Haiyue
  2021-03-11 11:54           ` [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device Wang, Yinan
  2021-03-15 14:16           ` David Marchand
  3 siblings, 1 reply; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-10 17:36 UTC (permalink / raw)
  To: david.marchand, maxime.coquelin, ferruh.yigit
  Cc: dev, anatoly.burakov, xuemingl, grive,
	谢华伟(此时此刻)

With I/O BAR, we get PIO(port-mapped I/O) address.
With MMIO(memory-mapped I/O) BAR, we get mapped virtual address.
We distinguish PIO and MMIO by their address range like how kernel does,
i.e, address below 64K is PIO.
ioread/write8/16/32 is provided to access PIO/MMIO.
By the way, for virtio on arch other than x86, BAR flag indicates PIO
but is mapped.

Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/pci/linux/pci.c     |   4 --
 drivers/bus/pci/linux/pci_uio.c | 156 +++++++++++++++++++++++++++++-----------
 2 files changed, 113 insertions(+), 47 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 0f38abf..0dc99e9 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -715,8 +715,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		pci_uio_ioport_read(p, data, len, offset);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
 		pci_uio_ioport_read(p, data, len, offset);
 		break;
@@ -736,8 +734,6 @@ int rte_pci_write_config(const struct rte_pci_device *device,
 		break;
 #endif
 	case RTE_PCI_KDRV_IGB_UIO:
-		pci_uio_ioport_write(p, data, len, offset);
-		break;
 	case RTE_PCI_KDRV_UIO_GENERIC:
 		pci_uio_ioport_write(p, data, len, offset);
 		break;
diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 01f2a40..0907051 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -368,6 +368,8 @@
 	return -1;
 }
 
+#define PIO_MAX 0x10000
+
 #if defined(RTE_ARCH_X86)
 int
 pci_uio_ioport_map(struct rte_pci_device *dev, int bar,
@@ -381,12 +383,6 @@
 	unsigned long base;
 	int i;
 
-	if (rte_eal_iopl_init() != 0) {
-		RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
-			__func__, dev->name);
-		return -1;
-	}
-
 	/* open and read addresses of the corresponding resource in sysfs */
 	snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource",
 		rte_pci_get_sysfs_path(), dev->addr.domain, dev->addr.bus,
@@ -408,15 +404,27 @@
 		&end_addr, &flags) < 0)
 		goto error;
 
-	if (!(flags & IORESOURCE_IO)) {
-		RTE_LOG(ERR, EAL, "%s(): bar resource other than IO is not supported\n", __func__);
-		goto error;
-	}
-	base = (unsigned long)phys_addr;
-	RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+	if (flags & IORESOURCE_IO) {
+		if (rte_eal_iopl_init()) {
+			RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for PCI device %s\n",
+				__func__, dev->name);
+			goto error;
+		}
 
-	if (base > UINT16_MAX)
+		base = (unsigned long)phys_addr;
+		if (base > PIO_MAX) {
+			RTE_LOG(ERR, EAL, "%s(): %08lx too large PIO resource\n", __func__, base);
+			goto error;
+		}
+
+		RTE_LOG(DEBUG, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base);
+	} else if (flags & IORESOURCE_MEM) {
+		base = (unsigned long)dev->mem_resource[bar].addr;
+		RTE_LOG(DEBUG, EAL, "%s(): MMIO BAR %08lx detected\n", __func__, base);
+	} else {
+		RTE_LOG(ERR, EAL, "%s(): unknown BAR type\n", __func__);
 		goto error;
+	}
 
 	/* FIXME only for primary process ? */
 	if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) {
@@ -517,6 +525,92 @@
 }
 #endif
 
+#if defined(RTE_ARCH_X86)
+static inline uint8_t ioread8(void *addr)
+{
+	uint8_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint8_t *)addr :
+		inb_p((unsigned long)addr);
+
+	return val;
+}
+
+static inline uint16_t ioread16(void *addr)
+{
+	uint16_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint16_t *)addr :
+		inw_p((unsigned long)addr);
+
+	return val;
+}
+
+static inline uint32_t ioread32(void *addr)
+{
+	uint32_t val;
+
+	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint32_t *)addr :
+		inl_p((unsigned long)addr);
+
+	return val;
+}
+
+static inline void iowrite8(uint8_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint8_t *)addr = val :
+		outb_p(val, (unsigned long)addr);
+}
+
+static inline void iowrite16(uint16_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint16_t *)addr = val :
+		outw_p(val, (unsigned long)addr);
+}
+
+static inline void iowrite32(uint32_t val, void *addr)
+{
+	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
+		*(volatile uint32_t *)addr = val :
+		outl_p(val, (unsigned long)addr);
+}
+#else
+static inline uint8_t ioread8(void *addr)
+{
+	return *(volatile uint8_t *)addr;
+}
+
+static inline uint16_t ioread16(void *addr)
+{
+	return *(volatile uint16_t *)addr;
+}
+
+static inline uint32_t ioread32(void *addr)
+{
+	return *(volatile uint32_t *)addr;
+}
+
+static inline void iowrite8(uint8_t val, void *addr)
+{
+	*(volatile uint8_t *)addr = val;
+}
+
+static inline void iowrite16(uint16_t val, void *addr)
+{
+	*(volatile uint16_t *)addr = val;
+}
+
+static inline void iowrite32(uint32_t val, void *addr)
+{
+	*(volatile uint32_t *)addr = val;
+}
+#endif
+
 void
 pci_uio_ioport_read(struct rte_pci_ioport *p,
 		    void *data, size_t len, off_t offset)
@@ -528,25 +622,13 @@
 	for (d = data; len > 0; d += size, reg += size, len -= size) {
 		if (len >= 4) {
 			size = 4;
-#if defined(RTE_ARCH_X86)
-			*(uint32_t *)d = inl(reg);
-#else
-			*(uint32_t *)d = *(volatile uint32_t *)reg;
-#endif
+			*(uint32_t *)d = ioread32((void *)reg);
 		} else if (len >= 2) {
 			size = 2;
-#if defined(RTE_ARCH_X86)
-			*(uint16_t *)d = inw(reg);
-#else
-			*(uint16_t *)d = *(volatile uint16_t *)reg;
-#endif
+			*(uint16_t *)d = ioread16((void *)reg);
 		} else {
 			size = 1;
-#if defined(RTE_ARCH_X86)
-			*d = inb(reg);
-#else
-			*d = *(volatile uint8_t *)reg;
-#endif
+			*d = ioread8((void *)reg);
 		}
 	}
 }
@@ -562,25 +644,13 @@
 	for (s = data; len > 0; s += size, reg += size, len -= size) {
 		if (len >= 4) {
 			size = 4;
-#if defined(RTE_ARCH_X86)
-			outl_p(*(const uint32_t *)s, reg);
-#else
-			*(volatile uint32_t *)reg = *(const uint32_t *)s;
-#endif
+			iowrite32(*(const uint32_t *)s, (void *)reg);
 		} else if (len >= 2) {
 			size = 2;
-#if defined(RTE_ARCH_X86)
-			outw_p(*(const uint16_t *)s, reg);
-#else
-			*(volatile uint16_t *)reg = *(const uint16_t *)s;
-#endif
+			iowrite16(*(const uint16_t *)s, (void *)reg);
 		} else {
 			size = 1;
-#if defined(RTE_ARCH_X86)
-			outb_p(*s, reg);
-#else
-			*(volatile uint8_t *)reg = *s;
-#endif
+			iowrite8(*s, (void *)reg);
 		}
 	}
 }
-- 
1.8.3.1


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

* Re: [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-03-10 17:36           ` [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
@ 2021-03-11  6:42             ` Wang, Haiyue
  2021-03-15 10:19               ` David Marchand
  0 siblings, 1 reply; 61+ messages in thread
From: Wang, Haiyue @ 2021-03-11  6:42 UTC (permalink / raw)
  To: 谢华伟(此时此刻),
	david.marchand, maxime.coquelin, Yigit, Ferruh
  Cc: dev, Burakov, Anatoly, xuemingl, grive

Hi Huawei,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of ???(????)
> Sent: Thursday, March 11, 2021 01:37
> To: david.marchand@redhat.com; maxime.coquelin@redhat.com; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Burakov, Anatoly <anatoly.burakov@intel.com>; xuemingl@nvidia.com; grive@u256.net;
> 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com>
> Subject: [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors
> 
> With I/O BAR, we get PIO(port-mapped I/O) address.
> With MMIO(memory-mapped I/O) BAR, we get mapped virtual address.
> We distinguish PIO and MMIO by their address range like how kernel does,
> i.e, address below 64K is PIO.
> ioread/write8/16/32 is provided to access PIO/MMIO.
> By the way, for virtio on arch other than x86, BAR flag indicates PIO
> but is mapped.
> 
> Signed-off-by: huawei xie <huawei.xhw@alibaba-inc.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/bus/pci/linux/pci.c     |   4 --
>  drivers/bus/pci/linux/pci_uio.c | 156 +++++++++++++++++++++++++++++-----------
>  2 files changed, 113 insertions(+), 47 deletions(-)
> 


> 
> +#if defined(RTE_ARCH_X86)
> +static inline uint8_t ioread8(void *addr)
> +{
> +	uint8_t val;
> +
> +	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
> +		*(volatile uint8_t *)addr :
> +		inb_p((unsigned long)addr);
> +
> +	return val;
> +}
> +
> +static inline uint16_t ioread16(void *addr)
> +{
> +	uint16_t val;
> +
> +	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
> +		*(volatile uint16_t *)addr :
> +		inw_p((unsigned long)addr);
> +
> +	return val;
> +}
> +
> +static inline uint32_t ioread32(void *addr)
> +{
> +	uint32_t val;
> +
> +	val = (uint64_t)(uintptr_t)addr >= PIO_MAX ?
> +		*(volatile uint32_t *)addr :
> +		inl_p((unsigned long)addr);
> +
> +	return val;
> +}
> +
> +static inline void iowrite8(uint8_t val, void *addr)
> +{
> +	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
> +		*(volatile uint8_t *)addr = val :
> +		outb_p(val, (unsigned long)addr);
> +}
> +
> +static inline void iowrite16(uint16_t val, void *addr)
> +{
> +	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
> +		*(volatile uint16_t *)addr = val :
> +		outw_p(val, (unsigned long)addr);
> +}
> +
> +static inline void iowrite32(uint32_t val, void *addr)
> +{
> +	(uint64_t)(uintptr_t)addr >= PIO_MAX ?
> +		*(volatile uint32_t *)addr = val :
> +		outl_p(val, (unsigned long)addr);
> +}
> +#else
> +static inline uint8_t ioread8(void *addr)
> +{
> +	return *(volatile uint8_t *)addr;
> +}
> +
> +static inline uint16_t ioread16(void *addr)
> +{
> +	return *(volatile uint16_t *)addr;
> +}
> +
> +static inline uint32_t ioread32(void *addr)
> +{
> +	return *(volatile uint32_t *)addr;
> +}
> +
> +static inline void iowrite8(uint8_t val, void *addr)
> +{
> +	*(volatile uint8_t *)addr = val;
> +}
> +
> +static inline void iowrite16(uint16_t val, void *addr)
> +{
> +	*(volatile uint16_t *)addr = val;
> +}
> +
> +static inline void iowrite32(uint32_t val, void *addr)
> +{
> +	*(volatile uint32_t *)addr = val;
> +}
> +#endif
> +

Like kernel use macro to do pio and mmio, maybe we can also to do so for
making code clean:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/iomap.c

#define IO_COND(addr, is_pio, is_mmio) do {			\
	unsigned long port = (unsigned long __force)addr;	\
	if (port >= PIO_RESERVED) {				\
		is_mmio;					\
	} else if (port > PIO_OFFSET) {				\
		port &= PIO_MASK;				\
		is_pio;						\
	} else							\
		bad_io_access(port, #is_pio );			\
} while (0)


Like:

#if defined(RTE_ARCH_X86)
#define IO_COND(addr, is_pio, is_mmio) do {           \
	if ((uint64_t)(uintptr_t)addr >= PIO_MAX) {   \
		is_mmio;                              \
	} else {                                      \
		is_pio;                               \
	}                                             \
} while (0)
#else
#define IO_COND(addr, is_pio, is_mmio) do {           \
		is_mmio;                              \
} while (0)
#endif

static inline uint8_t ioread8(void *addr)
{
	uint8_t val;

	IO_COND(addr,
		val = inb_p((unsigned long)addr),
		val = *(volatile uint8_t *)addr);

	return val;
}

static inline uint16_t ioread16(void *addr)
{
	uint16_t val;

	IO_COND(addr,
		val = inw_p((unsigned long)addr),
		val = *(volatile uint16_t *)addr);

	return val;
}

static inline uint32_t ioread32(void *addr)
{
	uint32_t val;

	IO_COND(addr,
		val = inl_p((unsigned long)addr),
		val = *(volatile uint32_t *)addr);

	return val;
}

static inline void iowrite8(uint8_t val, void *addr)
{
	IO_COND(addr,
		outb_p(val, (unsigned long)addr),
		*(volatile uint8_t *)addr = val);
}

static inline void iowrite16(uint16_t val, void *addr)
{
	IO_COND(addr,
		outw_p(val, (unsigned long)addr),
		*(volatile uint16_t *)addr = val);
}

static inline void iowrite32(uint32_t val, void *addr)
{
	IO_COND(addr,
		outl_p(val, (unsigned long)addr),
		*(volatile uint32_t *)addr = val);
}

>  void
>  pci_uio_ioport_read(struct rte_pci_ioport *p,
>  		    void *data, size_t len, off_t offset)
> @@ -528,25 +622,13 @@
>  	for (d = data; len > 0; d += size, reg += size, len -= size) {
>  		if (len >= 4) {
>  			size = 4;
> -#if defined(RTE_ARCH_X86)
> -			*(uint32_t *)d = inl(reg);
> -#else
> -			*(uint32_t *)d = *(volatile uint32_t *)reg;
> -#endif
> +			*(uint32_t *)d = ioread32((void *)reg);
>  		} else if (len >= 2) {
>  			size = 2;
> -#if defined(RTE_ARCH_X86)
> -			*(uint16_t *)d = inw(reg);
> -#else
> -			*(uint16_t *)d = *(volatile uint16_t *)reg;
> -#endif
> +			*(uint16_t *)d = ioread16((void *)reg);
>  		} else {
>  			size = 1;
> -#if defined(RTE_ARCH_X86)
> -			*d = inb(reg);
> -#else
> -			*d = *(volatile uint8_t *)reg;
> -#endif
> +			*d = ioread8((void *)reg);
>  		}
>  	}
>  }
> @@ -562,25 +644,13 @@
>  	for (s = data; len > 0; s += size, reg += size, len -= size) {
>  		if (len >= 4) {
>  			size = 4;
> -#if defined(RTE_ARCH_X86)
> -			outl_p(*(const uint32_t *)s, reg);
> -#else
> -			*(volatile uint32_t *)reg = *(const uint32_t *)s;
> -#endif
> +			iowrite32(*(const uint32_t *)s, (void *)reg);
>  		} else if (len >= 2) {
>  			size = 2;
> -#if defined(RTE_ARCH_X86)
> -			outw_p(*(const uint16_t *)s, reg);
> -#else
> -			*(volatile uint16_t *)reg = *(const uint16_t *)s;
> -#endif
> +			iowrite16(*(const uint16_t *)s, (void *)reg);
>  		} else {
>  			size = 1;
> -#if defined(RTE_ARCH_X86)
> -			outb_p(*s, reg);
> -#else
> -			*(volatile uint8_t *)reg = *s;
> -#endif
> +			iowrite8(*s, (void *)reg);
>  		}
>  	}
>  }
> --
> 1.8.3.1


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

* Re: [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device
  2021-03-10 17:36         ` [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
  2021-03-10 17:36           ` [dpdk-dev] [PATCH v11 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
  2021-03-10 17:36           ` [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
@ 2021-03-11 11:54           ` Wang, Yinan
  2021-03-12 14:32             ` David Marchand
  2021-03-15 14:16           ` David Marchand
  3 siblings, 1 reply; 61+ messages in thread
From: Wang, Yinan @ 2021-03-11 11:54 UTC (permalink / raw)
  To: 谢华伟(此时此刻),
	david.marchand, maxime.coquelin, Yigit, Ferruh
  Cc: dev, Burakov, Anatoly, xuemingl, grive

Tested-by: Wang, Yinan <yinan.wang@intel.com>

Tested PVP case with virtio PMD assumes legacy device, VM with below kernel LTS versions, all pass except vfio-pci test blocked with kernel v4.4.
5.10.0-051000-generic                     virtio-pmd test with  vfio-pci/ igb_uio/uio_pci_generic  all pass
5.4.0-050400-generic                       virtio-pmd test with  vfio-pci/ igb_uio/uio_pci_generic  all pass
4.19.179-0419179-generic             virtio-pmd test with  vfio-pci/ igb_uio/uio_pci_generic  all pass
4.9.260-0409260-generic               virtio-pmd test with  vfio-pci/ igb_uio/uio_pci_generic  all pass
4.4.260-0404260-generic               virtio-pmd test with  igb_uio/uio_pci_generic can pass ; vfio-pci blocked as fail to bind vfio-pci to virtio-pmd

Error info:		
root@vmubuntu2004:~/dpdk/usertools# ./dpdk-devbind.py -b vfio-pci 00:04.0
lspci: Unable to load libkmod resources: error -12

root@vmubuntu2004:~/dpdk/usertools# dmesg
[  161.553493] VFIO - User Level meta-driver version: 0.3
[  179.430529] vfio-pci: probe of 0000:00:04.0 failed with error -22

BR,
Yinan
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of ???(????)
> Sent: 2021年3月11日 1:36
> To: david.marchand@redhat.com; maxime.coquelin@redhat.com; Yigit,
> Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Burakov, Anatoly <anatoly.burakov@intel.com>;
> xuemingl@nvidia.com; grive@u256.net; 谢华伟(此时此刻)
> <huawei.xhw@alibaba-inc.com>
> Subject: [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for
> legacy virito device
> 
> virtio PMD assumes legacy device only supports PIO(port-mapped) BAR
> resource. This is wrong. As we need to create lots of devices, adn PIO
> resource on x86 is very limited, we expose MMIO(memory-mapped I/O) BAR.
> 
> Kernel supports both PIO and MMIO BAR for legacy virtio-pci device, and
> for all other pci devices. This patchset handles different type of BAR in
> the similar way.
> 
> In previous implementation, under igb_uio driver we get PIO address from
> igb_uio sysfs entry; with uio_pci_generic, we get PIO address from
> /proc/ioports for x86, and for other ARCHs, we get PIO address from
> standard PCI sysfs entry. For PIO/MMIO RW, there is different path for
> different drivers and arch.
> 
> All of the above is too much twisted. This patchset unifies the way to get
> both PIO and MMIO address for different driver and ARCHs, all from standard
> resource attr under pci sysfs. This is most generic.
> 
> We distinguish PIO and MMIO by their address range like how kernel does.
> It is ugly but works.
> 
> v2 changes:
>     - add more explanation in the commit message
> 
> v3 changes:
>     - fix patch format issues
> 
> v4 changes:
>     - fixes for RTE_KDRV_UIO_GENERIC -> RTE_PCI_KDRV_UIO_GENERIC
> 
> v5 changes:
>     - split into three seperate patches
> 
> v6 changes:
>     - change to DEBUG level for IO bar detection in pci_uio_ioport_map
>     - rework the code in iobar branch
>     - fixes commit message format issue
>     - temporarily remove the 3rd patch for vfio path, leave it for future
> discusssion
>     - rework against virtio_pmd_rework_v2
> 
> v7 changes:
>     - fix compilation issues of in/out instruction on non X86 archs
> 
> v8 changes:
>     - change the word fix to refactor in patch 1's commit message
> 
> v9 changes:
>     - keep pause version in in/out instructions
> 
> v10 changes:
>     - trival fixes in commit message, like > 75 chars
> 
> v11 changes:
>     - commit message fix and change
> 
> huawei xie (2):
>   bus/pci: use PCI standard sysfs entry to get PIO address
>   bus/pci: support MMIO in PCI ioport accessors
> 
>  drivers/bus/pci/linux/pci.c     |  81 ----------------
>  drivers/bus/pci/linux/pci_uio.c | 202 +++++++++++++++++++++++++++++--
> ---------
>  2 files changed, 150 insertions(+), 133 deletions(-)
> 
> --
> 1.8.3.1


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

* Re: [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device
  2021-03-11 11:54           ` [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device Wang, Yinan
@ 2021-03-12 14:32             ` David Marchand
  0 siblings, 0 replies; 61+ messages in thread
From: David Marchand @ 2021-03-12 14:32 UTC (permalink / raw)
  To: Wang, Yinan
  Cc: 谢华伟(此时此刻),
	maxime.coquelin, Yigit, Ferruh, dev, Burakov, Anatoly, xuemingl,
	grive

Hello,

On Thu, Mar 11, 2021 at 12:55 PM Wang, Yinan <yinan.wang@intel.com> wrote:
>
> Tested-by: Wang, Yinan <yinan.wang@intel.com>
>
> Tested PVP case with virtio PMD assumes legacy device, VM with below kernel LTS versions, all pass except vfio-pci test blocked with kernel v4.4.
> 5.10.0-051000-generic                     virtio-pmd test with  vfio-pci/ igb_uio/uio_pci_generic  all pass
> 5.4.0-050400-generic                       virtio-pmd test with  vfio-pci/ igb_uio/uio_pci_generic  all pass
> 4.19.179-0419179-generic             virtio-pmd test with  vfio-pci/ igb_uio/uio_pci_generic  all pass
> 4.9.260-0409260-generic               virtio-pmd test with  vfio-pci/ igb_uio/uio_pci_generic  all pass
> 4.4.260-0404260-generic               virtio-pmd test with  igb_uio/uio_pci_generic can pass ; vfio-pci blocked as fail to bind vfio-pci to virtio-pmd
>
> Error info:
> root@vmubuntu2004:~/dpdk/usertools# ./dpdk-devbind.py -b vfio-pci 00:04.0
> lspci: Unable to load libkmod resources: error -12
>
> root@vmubuntu2004:~/dpdk/usertools# dmesg
> [  161.553493] VFIO - User Level meta-driver version: 0.3
> [  179.430529] vfio-pci: probe of 0000:00:04.0 failed with error -22

The vanilla stable 4.4 kernel does not support "No-IOMMU mode" for vfio.
ae5515d66362 - Revert: "vfio: Include No-IOMMU mode" (5 years ago)
<Alex Williamson>

So this behavior is expected if you were testing with no iommu in your
virtual machine.


Thanks for the tests!

-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-03-11  6:42             ` Wang, Haiyue
@ 2021-03-15 10:19               ` David Marchand
  2021-03-15 11:25                 ` 谢华伟(此时此刻)
  2021-03-15 13:11                 ` Wang, Haiyue
  0 siblings, 2 replies; 61+ messages in thread
From: David Marchand @ 2021-03-15 10:19 UTC (permalink / raw)
  To: Wang, Haiyue,
	谢华伟(此时此刻)
  Cc: maxime.coquelin, Yigit, Ferruh, dev, Burakov, Anatoly, xuemingl, grive

On Thu, Mar 11, 2021 at 7:43 AM Wang, Haiyue <haiyue.wang@intel.com> wrote:
> Like kernel use macro to do pio and mmio, maybe we can also to do so for
> making code clean:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/iomap.c
>
> #define IO_COND(addr, is_pio, is_mmio) do {                     \
>         unsigned long port = (unsigned long __force)addr;       \
>         if (port >= PIO_RESERVED) {                             \
>                 is_mmio;                                        \
>         } else if (port > PIO_OFFSET) {                         \
>                 port &= PIO_MASK;                               \
>                 is_pio;                                         \
>         } else                                                  \
>                 bad_io_access(port, #is_pio );                  \
> } while (0)
>
>
> Like:
>
> #if defined(RTE_ARCH_X86)
> #define IO_COND(addr, is_pio, is_mmio) do {           \
>         if ((uint64_t)(uintptr_t)addr >= PIO_MAX) {   \
>                 is_mmio;                              \
>         } else {                                      \
>                 is_pio;                               \
>         }                                             \
> } while (0)
> #else
> #define IO_COND(addr, is_pio, is_mmio) do {           \
>                 is_mmio;                              \
> } while (0)
> #endif

We should not just copy/paste kernel code.

Plus here, this seems a bit overkill.
And there are other parts in this code that could use some polishing.

What do you think of merging this series as is (now that we got non
regression reports) and doing such cleanups in followup patches?


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-03-15 10:19               ` David Marchand
@ 2021-03-15 11:25                 ` 谢华伟(此时此刻)
  2021-03-15 13:11                 ` Wang, Haiyue
  1 sibling, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-15 11:25 UTC (permalink / raw)
  To: David Marchand, Wang, Haiyue
  Cc: maxime.coquelin, Yigit, Ferruh, dev, Burakov, Anatoly, xuemingl, grive


On 2021/3/15 18:19, David Marchand wrote:
>> #else
>> #define IO_COND(addr, is_pio, is_mmio) do {           \
>>                  is_mmio;                              \
>> } while (0)
>> #endif
> We should not just copy/paste kernel code.
>
> Plus here, this seems a bit overkill.
> And there are other parts in this code that could use some polishing.
>
> What do you think of merging this series as is (now that we got non
> regression reports) and doing such cleanups in followup patches?

I am OK. Yes, we could do some cleanup after it is merged, for example 
against vfio, if it is really necessary for virtio PMD only to use vfio 
to access IO port.


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

* Re: [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors
  2021-03-15 10:19               ` David Marchand
  2021-03-15 11:25                 ` 谢华伟(此时此刻)
@ 2021-03-15 13:11                 ` Wang, Haiyue
  1 sibling, 0 replies; 61+ messages in thread
From: Wang, Haiyue @ 2021-03-15 13:11 UTC (permalink / raw)
  To: David Marchand,
	谢华伟(此时此刻)
  Cc: maxime.coquelin, Yigit, Ferruh, dev, Burakov, Anatoly, xuemingl, grive

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, March 15, 2021 18:20
> To: Wang, Haiyue <haiyue.wang@intel.com>; 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com>
> Cc: maxime.coquelin@redhat.com; Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org; Burakov, Anatoly
> <anatoly.burakov@intel.com>; xuemingl@nvidia.com; grive@u256.net
> Subject: Re: [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors
> 
> On Thu, Mar 11, 2021 at 7:43 AM Wang, Haiyue <haiyue.wang@intel.com> wrote:
> > Like kernel use macro to do pio and mmio, maybe we can also to do so for
> > making code clean:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/iomap.c
> >
> > #define IO_COND(addr, is_pio, is_mmio) do {                     \
> >         unsigned long port = (unsigned long __force)addr;       \
> >         if (port >= PIO_RESERVED) {                             \
> >                 is_mmio;                                        \
> >         } else if (port > PIO_OFFSET) {                         \
> >                 port &= PIO_MASK;                               \
> >                 is_pio;                                         \
> >         } else                                                  \
> >                 bad_io_access(port, #is_pio );                  \
> > } while (0)
> >
> >
> > Like:
> >
> > #if defined(RTE_ARCH_X86)
> > #define IO_COND(addr, is_pio, is_mmio) do {           \
> >         if ((uint64_t)(uintptr_t)addr >= PIO_MAX) {   \
> >                 is_mmio;                              \
> >         } else {                                      \
> >                 is_pio;                               \
> >         }                                             \
> > } while (0)
> > #else
> > #define IO_COND(addr, is_pio, is_mmio) do {           \
> >                 is_mmio;                              \
> > } while (0)
> > #endif
> 
> We should not just copy/paste kernel code.
> 

Got it ;-)

> 
> 
> --
> David Marchand


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

* Re: [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device
  2021-03-10 17:36         ` [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
                             ` (2 preceding siblings ...)
  2021-03-11 11:54           ` [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device Wang, Yinan
@ 2021-03-15 14:16           ` David Marchand
  2021-03-17  8:12             ` 谢华伟(此时此刻)
  3 siblings, 1 reply; 61+ messages in thread
From: David Marchand @ 2021-03-15 14:16 UTC (permalink / raw)
  To: 谢华伟(此时此刻)
  Cc: Maxime Coquelin, Yigit, Ferruh, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Wang, Yinan, Wang, Haiyue

On Wed, Mar 10, 2021 at 6:37 PM 谢华伟(此时此刻) <huawei.xhw@alibaba-inc.com> wrote:
>
> virtio PMD assumes legacy device only supports PIO(port-mapped) BAR
> resource. This is wrong. As we need to create lots of devices, adn PIO
> resource on x86 is very limited, we expose MMIO(memory-mapped I/O) BAR.
>
> Kernel supports both PIO and MMIO BAR for legacy virtio-pci device, and
> for all other pci devices. This patchset handles different type of BAR in
> the similar way.
>
> In previous implementation, under igb_uio driver we get PIO address from
> igb_uio sysfs entry; with uio_pci_generic, we get PIO address from
> /proc/ioports for x86, and for other ARCHs, we get PIO address from
> standard PCI sysfs entry. For PIO/MMIO RW, there is different path for
> different drivers and arch.
>
> All of the above is too much twisted. This patchset unifies the way to get
> both PIO and MMIO address for different driver and ARCHs, all from standard
> resource attr under pci sysfs. This is most generic.
>
> We distinguish PIO and MMIO by their address range like how kernel does.
> It is ugly but works.
>
> v2 changes:
>     - add more explanation in the commit message
>
> v3 changes:
>     - fix patch format issues
>
> v4 changes:
>     - fixes for RTE_KDRV_UIO_GENERIC -> RTE_PCI_KDRV_UIO_GENERIC
>
> v5 changes:
>     - split into three seperate patches
>
> v6 changes:
>     - change to DEBUG level for IO bar detection in pci_uio_ioport_map
>     - rework the code in iobar branch
>     - fixes commit message format issue
>     - temporarily remove the 3rd patch for vfio path, leave it for future discusssion
>     - rework against virtio_pmd_rework_v2
>
> v7 changes:
>     - fix compilation issues of in/out instruction on non X86 archs
>
> v8 changes:
>     - change the word fix to refactor in patch 1's commit message
>
> v9 changes:
>     - keep pause version in in/out instructions
>
> v10 changes:
>     - trival fixes in commit message, like > 75 chars
>
> v11 changes:
>     - commit message fix and change
>

Aligned Sob and Author to fix the last checkpatch warning.

Series applied to the main branch.
Thanks Huawei and thanks too to reviewers/testers.


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device
  2021-03-15 14:16           ` David Marchand
@ 2021-03-17  8:12             ` 谢华伟(此时此刻)
  0 siblings, 0 replies; 61+ messages in thread
From: 谢华伟(此时此刻) @ 2021-03-17  8:12 UTC (permalink / raw)
  To: David Marchand
  Cc: Maxime Coquelin, Yigit, Ferruh, dev, Burakov, Anatoly, xuemingl,
	Gaetan Rivet, Wang, Yinan, Wang, Haiyue


On 2021/3/15 22:16, David Marchand wrote:
>> v10 changes:
>>      - trival fixes in commit message, like > 75 chars
>>
>> v11 changes:
>>      - commit message fix and change
>>
> Aligned Sob and Author to fix the last checkpatch warning.
>
> Series applied to the main branch.
> Thanks Huawei and thanks too to reviewers/testers.

Thanks!


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

end of thread, other threads:[~2021-03-17  8:12 UTC | newest]

Thread overview: 61+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-29  3:18 [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
2021-01-29  3:18 ` [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
2021-02-03  9:37   ` Maxime Coquelin
2021-02-18  9:33   ` David Marchand
2021-02-21 15:58     ` 谢华伟(此时此刻)
2021-02-24 12:49       ` David Marchand
2021-02-24 15:29         ` 谢华伟(此时此刻)
2021-02-24 17:52           ` David Marchand
2021-03-01 15:47             ` 谢华伟(此时此刻)
2021-03-02 12:31             ` 谢华伟(此时此刻)
2021-01-29  3:18 ` [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
2021-02-03  9:37   ` Maxime Coquelin
2021-02-09 14:51   ` Ferruh Yigit
2021-02-19  8:52     ` Ferruh Yigit
2021-02-21 15:45       ` 谢华伟(此时此刻)
2021-02-17  9:06   ` David Marchand
2021-02-17 14:15     ` 谢华伟(此时此刻)
2021-02-18  9:33       ` David Marchand
2021-01-29  3:25 ` [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
2021-02-01  7:43   ` 谢华伟(此时此刻)
2021-02-03  9:37     ` Maxime Coquelin
2021-02-04  2:50       ` 谢华伟(此时此刻)
2021-02-22 17:15 ` [dpdk-dev] [PATCH v7 " 谢华伟(此时此刻)
2021-02-22 17:15   ` [dpdk-dev] [PATCH v7 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
2021-02-22 17:15   ` [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
2021-02-22 17:25     ` Ferruh Yigit
2021-02-23 14:20       ` 谢华伟(此时此刻)
2021-02-24 15:45         ` Ferruh Yigit
2021-02-25  3:59           ` 谢华伟(此时此刻)
2021-02-25  9:52             ` David Marchand
2021-03-01 15:43               ` 谢华伟(此时此刻)
2021-03-02 13:14                 ` David Marchand
2021-03-03  7:56                   ` 谢华伟(此时此刻)
2021-03-01 16:01   ` [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
2021-03-01 16:01     ` [dpdk-dev] [PATCH v8 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
2021-03-01 16:01     ` [dpdk-dev] [PATCH v8 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
2021-03-02 12:48     ` [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD 谢华伟(此时此刻)
2021-03-02 13:01       ` Ferruh Yigit
2021-03-02 13:17       ` David Marchand
2021-03-03 17:46     ` [dpdk-dev] [PATCH v9 " 谢华伟(此时此刻)
2021-03-03 17:46       ` [dpdk-dev] [PATCH v9 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
2021-03-03 17:46       ` [dpdk-dev] [PATCH v9 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
2021-03-03 18:24       ` [dpdk-dev] [PATCH v9 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD Stephen Hemminger
2021-03-04 13:45         ` 谢华伟(此时此刻)
2021-03-03 18:47       ` [dpdk-dev] [PATCH v10 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
2021-03-03 18:47         ` [dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
2021-03-05 16:17           ` 谢华伟(此时此刻)
2021-03-09  6:22             ` 谢华伟(此时此刻)
2021-03-09  7:44               ` David Marchand
2021-03-03 18:47         ` [dpdk-dev] [PATCH v10 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
2021-03-10 17:36         ` [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device 谢华伟(此时此刻)
2021-03-10 17:36           ` [dpdk-dev] [PATCH v11 1/2] bus/pci: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
2021-03-10 17:36           ` [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors 谢华伟(此时此刻)
2021-03-11  6:42             ` Wang, Haiyue
2021-03-15 10:19               ` David Marchand
2021-03-15 11:25                 ` 谢华伟(此时此刻)
2021-03-15 13:11                 ` Wang, Haiyue
2021-03-11 11:54           ` [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device Wang, Yinan
2021-03-12 14:32             ` David Marchand
2021-03-15 14:16           ` David Marchand
2021-03-17  8:12             ` 谢华伟(此时此刻)

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).