diff mbox series

[v6,2/2] bus/pci: support MMIO in PCI ioport accessors

Message ID 1611890309-99135-3-git-send-email-huawei.xhw@alibaba-inc.com (mailing list archive)
State Superseded
Delegated to: David Marchand
Headers show
Series support both PIO and MMIO BAR for legacy device in virtio PMD | expand

Checks

Context Check Description
ci/iol-testing fail Testing issues
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/checkpatch warning coding style issues

Commit Message

谢华伟(此时此刻） Jan. 29, 2021, 3:18 a.m. UTC 
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(-)

Comments

Maxime Coquelin Feb. 3, 2021, 9:37 a.m. UTC | #1 
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
Ferruh Yigit Feb. 9, 2021, 2:51 p.m. UTC | #2 
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);
>   		}
>   	}
>   }
>
David Marchand Feb. 17, 2021, 9:06 a.m. UTC | #3 
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
谢华伟(此时此刻） Feb. 17, 2021, 2:15 p.m. UTC | #4 
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
David Marchand Feb. 18, 2021, 9:33 a.m. UTC | #5 
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.
Ferruh Yigit Feb. 19, 2021, 8:52 a.m. UTC | #6 
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);
>>           }
>>       }
>>   }
>>
>
谢华伟(此时此刻） Feb. 21, 2021, 3:45 p.m. UTC | #7 
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);
>>>           }
>>>       }
>>>   }
>>>
>>
diff mbox series

Patch

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