[dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[谢华伟(此时此刻）]

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

[dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[谢华伟(此时此刻）]

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

[dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors

[谢华伟(此时此刻）]

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

Re: [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[谢华伟(此时此刻）]

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

Re: [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[谢华伟(此时此刻）]

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

Re: [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[Maxime Coquelin]

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>

Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH v6 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[谢华伟(此时此刻）]

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

Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors

[Ferruh Yigit]

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);
>   		}
>   	}
>   }
> 

Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors

[David Marchand]

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

Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors

[谢华伟(此时此刻）]

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

Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors

[David Marchand]

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

Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[David Marchand]

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

Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors

[Ferruh Yigit]

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);
>>           }
>>       }
>>   }
>>
> 

Re: [dpdk-dev] [PATCH v6 2/2] bus/pci: support MMIO in PCI ioport accessors

[谢华伟(此时此刻）]

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);
>>>           }
>>>       }
>>>   }
>>>
>>

Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[谢华伟(此时此刻）]

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.

>

[dpdk-dev] [PATCH v7 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[谢华伟(此时此刻）]

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

[dpdk-dev] [PATCH v7 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[谢华伟(此时此刻）]

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

[dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors

[谢华伟(此时此刻）]

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

Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors

[Ferruh Yigit]

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

Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors

[谢华伟(此时此刻）]

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.

Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[David Marchand]

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

Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[谢华伟(此时此刻）]

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.

>
>

Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors

[Ferruh Yigit]

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?

Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[David Marchand]

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

Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors

[谢华伟(此时此刻）]

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.

Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors

[David Marchand]

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

Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors

[谢华伟(此时此刻）]

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.

>
>

Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[谢华伟(此时此刻）]

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

[dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[谢华伟(此时此刻）]

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

[dpdk-dev] [PATCH v8 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[谢华伟(此时此刻）]

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

[dpdk-dev] [PATCH v8 2/2] bus/pci: support MMIO in PCI ioport accessors

[谢华伟(此时此刻）]

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

Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[谢华伟(此时此刻）]

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.

>
>

Re: [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[谢华伟(此时此刻）]

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.

Re: [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[Ferruh Yigit]

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?

Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors

[David Marchand]

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

Re: [dpdk-dev] [PATCH v8 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[David Marchand]

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

Re: [dpdk-dev] [PATCH v7 2/2] bus/pci: support MMIO in PCI ioport accessors

[谢华伟(此时此刻）]

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.

[dpdk-dev] [PATCH v9 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[谢华伟(此时此刻）]

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

[dpdk-dev] [PATCH v9 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[谢华伟(此时此刻）]

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

[dpdk-dev] [PATCH v9 2/2] bus/pci: support MMIO in PCI ioport accessors

[谢华伟(此时此刻）]

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

Re: [dpdk-dev] [PATCH v9 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[Stephen Hemminger]

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.

[dpdk-dev] [PATCH v10 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, 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

[dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[谢华伟(此时此刻）]

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

[dpdk-dev] [PATCH v10 2/2] bus/pci: support MMIO in PCI ioport accessors

[谢华伟(此时此刻）]

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

Re: [dpdk-dev] [PATCH v9 0/2] support both PIO and MMIO BAR for legacy device in virtio PMD

[谢华伟(此时此刻）]

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.

Re: [dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[谢华伟(此时此刻）]

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?

Re: [dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[谢华伟(此时此刻）]

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.

Re: [dpdk-dev] [PATCH v10 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[David Marchand]

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

[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

[dpdk-dev] [PATCH v11 1/2] bus/pci: use PCI standard sysfs entry to get PIO address

[谢华伟(此时此刻）]

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

[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(-)

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

Re: [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors

[Wang, Haiyue]

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

Re: [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device

[Wang, Yinan]

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

Re: [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device

[David Marchand]

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

Re: [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors

[David Marchand]

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

Re: [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors

[谢华伟(此时此刻）]

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.

Re: [dpdk-dev] [PATCH v11 2/2] bus/pci: support MMIO in PCI ioport accessors

[Wang, Haiyue]

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

Re: [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device

[David Marchand]

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

Re: [dpdk-dev] [PATCH v11 0/2] support both PIO and MMIO BAR for legacy virito device

[谢华伟(此时此刻）]

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!