| Message ID | 1603381885-88819-3-git-send-email-huawei.xhw@alibaba-inc.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Maxime Coquelin |
| Headers | show |
| Series | support both PIO and MMIO BAR for virtio PMD | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | warning | coding style issues |
Title should be something like: "bus/pci: support MMIO in PCI ioport accessors On 10/22/20 5:51 PM, 谢华伟(此时此刻） wrote: > From: "huawei.xhw" <huawei.xhw@alibaba-inc.com> > > 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. No acronym in the commit message. Also, I am not sure to understand this comment. Does it means in the case of ARM for example, the IORESOURCE_IO flag is set but the base address is above PIO_MAX? > > Signed-off-by: huawei.xhw <huawei.xhw@alibaba-inc.com> As in previous patch, we need your full name for the sign-off. > --- > 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 > 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; I think this part should be in patch 1. > 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; Same here. > 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); > + } 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; > } > - 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)) { > + RTE_LOG(ERR, EAL, "%s(): %08lx too large PIO resource\n", __func__, base); > goto error; > + } It looks like above check could be moved directly to (flags & IORESOURCE_IO) case, so iobar boolean is not needed. > > /* 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); > } > } > } >
On 2021/1/12 16:23, Maxime Coquelin wrote: > Title should be something like: > > "bus/pci: support MMIO in PCI ioport accessors > > On 10/22/20 5:51 PM, 谢华伟(此时此刻） wrote: >> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com> >> >> 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. > No acronym in the commit message. BTW? fixed. PIO(programmed IO) and MMIO(memory mapped IO) explained. > Also, I am not sure to understand this comment. > Does it means in the case of ARM for example, the IORESOURCE_IO flag is > set but the base address is above PIO_MAX? ARM doesn't have PIO but only MMIO. The device sets IORESOURCE_IO flag anyway. Should i remove this message as it causes confuse? > >> Signed-off-by: huawei.xhw <huawei.xhw@alibaba-inc.com> > As in previous patch, we need your full name for the sign-off. fixed. > >> --- >> 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 >> 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; > I think this part should be in patch 1. Patch 1 handles IO port map. Patch 2 unifies IO/MMIO. Patch 3 handles vfio. I feel current split is more clear. > >> 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; > Same here. > >> 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); >> + } 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; >> } >> - 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)) { >> + RTE_LOG(ERR, EAL, "%s(): %08lx too large PIO resource\n", __func__, base); >> goto error; >> + } > It looks like above check could be moved directly to (flags & > IORESOURCE_IO) case, so iobar boolean is not needed. yes, code is more clear with your suggestion.
Hi Huawei, Nice work, just some small comments. >-----Original Message----- >From: dev <dev-bounces@dpdk.org> 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; 谢华伟(此 >时此刻） <huawei.xhw@alibaba-inc.com> >Subject: [dpdk-dev] [PATCH v5 2/3] PCI: support MMIO in >rte_pci_ioport_map/unap/read/write > >From: "huawei.xhw" <huawei.xhw@alibaba-inc.com> > >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 <huawei.xhw@alibaba-inc.com> >--- > 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 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..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. >+ 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
On 2021/1/24 23:22, Xueming(Steven) Li wrote: >> + } 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. makes sense. would remove it. > >> + } 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. Thanks. It is already moved in v6 patch. >> - 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. ok.
On 10/22/2020 4:51 PM, 谢华伟(此时此刻） wrote: > From: "huawei.xhw" <huawei.xhw@alibaba-inc.com> > > 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 <huawei.xhw@alibaba-inc.com> <...> > @@ -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; Hi Huawei, At this stage, to have a valid 'addr' it should be already mmap'ed, can you please provide the call stack when it is set/mmaped, to confirm it will be always valid at this point? Thanks, ferruh
On 2021/1/27 18:40, Ferruh Yigit wrote: > On 10/22/2020 4:51 PM, 谢华伟(此时此刻） wrote: >> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com> >> >> 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 <huawei.xhw@alibaba-inc.com> > > <...> > >> @@ -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; > > Hi Huawei, > > At this stage, to have a valid 'addr' it should be already mmap'ed, > can you please provide the call stack when it is set/mmaped, to > confirm it will be always valid at this point? > > Thanks, > ferruh #0 pci_uio_map_resource_by_index (dev=0x420c700, res_idx=0, uio_res=0x1003b19c0, map_idx=0) at ../drivers/bus/pci/linux/pci_uio.c:286 #1 0x000000000095f047 in pci_uio_map_resource (dev=0x420c700) at ../drivers/bus/pci/pci_common_uio.c:112 #2 0x000000000095f645 in rte_pci_map_device (dev=0x420c700) at ../drivers/bus/pci/linux/pci.c:81 #3 0x000000000174b5b9 in virtio_read_caps (dev=0x420c700, hw=0x1003b2d80) at ../drivers/net/virtio/virtio_pci.c:574 #4 0x000000000174baf9 in vtpci_init (dev=0x420c700, hw=0x1003b2d80) at ../drivers/net/virtio/virtio_pci.c:697 #5 0x0000000001743c84 in eth_virtio_dev_init (eth_dev=0x3461e40 <rte_eth_devices>) at ../drivers/net/virtio/virtio_ethdev.c:1954
On 1/27/2021 3:34 PM, 谢华伟(此时此刻） wrote: > > On 2021/1/27 18:40, Ferruh Yigit wrote: >> On 10/22/2020 4:51 PM, 谢华伟(此时此刻） wrote: >>> From: "huawei.xhw" <huawei.xhw@alibaba-inc.com> >>> >>> 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 <huawei.xhw@alibaba-inc.com> >> >> <...> >> >>> @@ -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; >> >> Hi Huawei, >> >> At this stage, to have a valid 'addr' it should be already mmap'ed, can you >> please provide the call stack when it is set/mmaped, to confirm it will be >> always valid at this point? >> >> Thanks, >> ferruh > > #0 pci_uio_map_resource_by_index (dev=0x420c700, res_idx=0, > uio_res=0x1003b19c0, map_idx=0) at ../drivers/bus/pci/linux/pci_uio.c:286 > #1 0x000000000095f047 in pci_uio_map_resource (dev=0x420c700) at > ../drivers/bus/pci/pci_common_uio.c:112 > #2 0x000000000095f645 in rte_pci_map_device (dev=0x420c700) at > ../drivers/bus/pci/linux/pci.c:81 > #3 0x000000000174b5b9 in virtio_read_caps (dev=0x420c700, hw=0x1003b2d80) at > ../drivers/net/virtio/virtio_pci.c:574 > #4 0x000000000174baf9 in vtpci_init (dev=0x420c700, hw=0x1003b2d80) at > ../drivers/net/virtio/virtio_pci.c:697 > #5 0x0000000001743c84 in eth_virtio_dev_init (eth_dev=0x3461e40 > <rte_eth_devices>) at ../drivers/net/virtio/virtio_ethdev.c:1954 > > Thanks. This looks good.
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..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); + } 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; } - 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)) { + 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); } } }