[v6,2/2] bus/pci: support MMIO in PCI ioport accessors
Checks
Commit Message
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
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
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);
> }
> }
> }
>
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
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
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.
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);
>> }
>> }
>> }
>>
>
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);
>>> }
>>> }
>>> }
>>>
>>
@@ -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;
@@ -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);
}
}
}