Re: [dpdk-dev] [PATCH v5 2/3] PCI: support MMIO in rte_pci_ioport_map/unap/read/write
From: Xueming(Steven) Li <hidden>
Date: 2021-01-24 15:22:19
Hi Huawei, Nice work, just some small comments.
quoted hunk ↗ jump to hunk
-----Original Message----- From: dev <redacted> On Behalf Of 谢华伟(此时此刻) Sent: Thursday, October 22, 2020 11:51 PM To: ferruh.yigit@intel.com Cc: dev@dpdk.org; maxime.coquelin@redhat.com; anatoly.burakov@intel.com; david.marchand@redhat.com; zhihong.wang@intel.com; chenbo.xia@intel.com; grive@u256.net; 谢华伟(此 时此刻) [off-list ref] Subject: [dpdk-dev] [PATCH v5 2/3] PCI: support MMIO in rte_pci_ioport_map/unap/read/write From: "huawei.xhw" <redacted> If IO BAR, we get PIO address. If MMIO BAR, we get mapped virtual address. We distinguish PIO and MMIO by their address like how kernel does. ioread/write8/16/32 is provided to access PIO/MMIO. BTW, for virtio on arch other than x86, BAR flag indicates PIO but is mapped. Signed-off-by: huawei.xhw <redacted> --- drivers/bus/pci/linux/pci.c | 4 -- drivers/bus/pci/linux/pci_uio.c | 123 ++++++++++++++++++++++++++----------- --- 2 files changed, 82 insertions(+), 45 deletions(-)diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c index0f38abf..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..c19382f 100644 --- a/drivers/bus/pci/linux/pci_uio.c +++ b/drivers/bus/pci/linux/pci_uio.c@@ -379,14 +379,9 @@char buf[BUFSIZ]; uint64_t phys_addr, end_addr, flags; unsigned long base; + bool iobar; 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 +403,30 @@&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__); + if (flags & IORESOURCE_IO) { + iobar = 1; + base = (unsigned long)phys_addr; + RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__, base); + } else if (flags & IORESOURCE_MEM) { + iobar = 0; + base = (unsigned long)dev->mem_resource[bar].addr; + RTE_LOG(INFO, EAL, "%s(): MMIO BAR %08lx detected\n", __func__, base);
Same here, INFO level seems chatty.
+ } else {
+ RTE_LOG(ERR, EAL, "%s(): unknown BAR type\n", __func__);
+ goto error;
+ }
+
+
+ if (iobar && rte_eal_iopl_init() != 0) {
+ RTE_LOG(ERR, EAL, "%s(): insufficient ioport permissions for
PCI device %s\n",
+ __func__, dev->name);
goto error;
}Same as Maxime's suggestion, please move this block as well.
- base = (unsigned long)phys_addr;
- RTE_LOG(INFO, EAL, "%s(): PIO BAR %08lx detected\n", __func__,
base);
- if (base > UINT16_MAX)
+ if (iobar && (base > UINT16_MAX)) {PIO_MAX defined below, please use it here. UNI16_MAX used in patch 1/3 as well.
quoted hunk ↗ jump to hunk
+ RTE_LOG(ERR, EAL, "%s(): %08lx too large PIO resource\n", __func__, +base); goto error; + } /* FIXME only for primary process ? */ if (dev->intr_handle.type == RTE_INTR_HANDLE_UNKNOWN) { @@ - 517,6 +527,61 @@ } #endif +#define PIO_MAX 0x10000 +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 +593,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 +615,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