diff mbox series

[v5,2/3] PCI: support MMIO in rte_pci_ioport_map/unap/read/write

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

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

谢华伟(此时此刻) Oct. 22, 2020, 3:51 p.m. UTC
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(-)

Comments

Maxime Coquelin Jan. 12, 2021, 8:23 a.m. UTC | #1
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);
>  		}
>  	}
>  }
>
谢华伟(此时此刻) Jan. 21, 2021, 6:30 a.m. UTC | #2
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.
Xueming(Steven) Li Jan. 24, 2021, 3:22 p.m. UTC | #3
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
谢华伟(此时此刻) Jan. 25, 2021, 3:08 a.m. UTC | #4
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.
Ferruh Yigit Jan. 27, 2021, 10:40 a.m. UTC | #5
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
谢华伟(此时此刻) Jan. 27, 2021, 3:34 p.m. UTC | #6
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
Ferruh Yigit Jan. 27, 2021, 4:45 p.m. UTC | #7
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 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..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);
 		}
 	}
 }