diff mbox series

[2/8] app/testpmd: use PCI memory resource access APIs

Message ID 20210910022402.26620-3-chenbo.xia@intel.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series Removal of PCI bus ABIs |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Chenbo Xia Sept. 10, 2021, 2:23 a.m. UTC 
  Currently testpmd uses struct rte_pci_device to access PCI memory
resource. Since this structure will be internal later, this patch
replaces use of rte_pci_device with new PCI memory resource access
APIs to read/write BAR 0.

Signed-off-by: Chenbo Xia <chenbo.xia@intel.com>
---
 app/test-pmd/config.c  | 38 +++++++++++-----------------------
 app/test-pmd/testpmd.h | 46 +++++++++++++++++++++---------------------
 2 files changed, 35 insertions(+), 49 deletions(-)

Comments

Li, Xiaoyun Sept. 16, 2021, 6:10 a.m. UTC | #1 
Hi

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Friday, September 10, 2021 10:24
> To: dev@dpdk.org
> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>
> Subject: [PATCH 2/8] app/testpmd: use PCI memory resource access APIs
> 
> Currently testpmd uses struct rte_pci_device to access PCI memory resource.
> Since this structure will be internal later, this patch replaces use of
> rte_pci_device with new PCI memory resource access APIs to read/write BAR 0.
> 
> Signed-off-by: Chenbo Xia <chenbo.xia@intel.com>
> ---
>  app/test-pmd/config.c  | 38 +++++++++++-----------------------
>  app/test-pmd/testpmd.h | 46 +++++++++++++++++++++---------------------
>  2 files changed, 35 insertions(+), 49 deletions(-)
> 
<snip>
> 
> @@ -1081,7 +1063,9 @@ port_reg_bit_set(portid_t port_id, uint32_t reg_off,
> uint8_t bit_pos,
>  			(int) bit_v);
>  		return;
>  	}
> -	reg_v = port_id_pci_reg_read(port_id, reg_off);
> +	if (port_id_pci_reg_read(port_id, reg_off, &reg_v))
> +		return;
> +
>  	if (bit_v == 0)
>  		reg_v &= ~(1 << bit_pos);
>  	else
> @@ -1123,7 +1107,9 @@ port_reg_bit_field_set(portid_t port_id, uint32_t
> reg_off,
>  				(unsigned)max_v, (unsigned)max_v);
>  		return;
>  	}
> -	reg_v = port_id_pci_reg_read(port_id, reg_off);
> +	if (port_id_pci_reg_read(port_id, reg_off, &reg_v))
> +		return;
> +
>  	reg_v &= ~(max_v << l_bit); /* Keep unchanged bits */
>  	reg_v |= (value << l_bit); /* Set changed bits */
>  	port_id_pci_reg_write(port_id, reg_off, reg_v); diff --git a/app/test-

In your implementation, port_id_pci_reg_write() can fail. Then the following display shouldn't be called.
So change this function to have return value and check it before use it will be better.

> pmd/testpmd.h b/app/test-pmd/testpmd.h index 16a3598e48..7922807a6e
> 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
<snip>
>  static inline void
> -port_pci_reg_write(struct rte_port *port, uint32_t reg_off, uint32_t reg_v)
> +port_id_pci_reg_write(portid_t pt_id, uint32_t reg_off, uint32_t reg_v)
>  {
> -	const struct rte_pci_device *pci_dev;
> +	struct rte_port *port = &ports[(pt_id)];
> +	char name[RTE_ETH_NAME_MAX_LEN];
>  	const struct rte_bus *bus;
> -	void *reg_addr;
> 
>  	if (!port->dev_info.device) {
>  		fprintf(stderr, "Invalid device\n");
> @@ -721,19 +721,19 @@ port_pci_reg_write(struct rte_port *port, uint32_t
> reg_off, uint32_t reg_v)
> 
>  	bus = rte_bus_find_by_device(port->dev_info.device);
>  	if (bus && !strcmp(bus->name, "pci")) {
> -		pci_dev = RTE_DEV_TO_PCI(port->dev_info.device);
> +		rte_eth_dev_get_name_by_port(pt_id, name);
>  	} else {
>  		fprintf(stderr, "Not a PCI device\n");
>  		return;
>  	}
> 
> -	reg_addr = ((char *)pci_dev->mem_resource[0].addr + reg_off);
> -	*((volatile uint32_t *)reg_addr) = rte_cpu_to_le_32(reg_v);
> +	reg_v = rte_cpu_to_le_32(reg_v);
> +	if (rte_pci_mem_wr32(name, 0, &reg_v, reg_off)) {
> +		fprintf(stderr, "Failed to write register\n");
> +		return;
> +	}
>  }
> 
> -#define port_id_pci_reg_write(pt_id, reg_off, reg_value) \
> -	port_pci_reg_write(&ports[(pt_id)], (reg_off), (reg_value))
> -
>  static inline void
>  get_start_cycles(uint64_t *start_tsc)
>  {
> --
> 2.17.1
Chenbo Xia Sept. 16, 2021, 6:38 a.m. UTC | #2 
Hi,

> -----Original Message-----
> From: Li, Xiaoyun <xiaoyun.li@intel.com>
> Sent: Thursday, September 16, 2021 2:10 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Subject: RE: [PATCH 2/8] app/testpmd: use PCI memory resource access APIs
> 
> Hi
> 
> > -----Original Message-----
> > From: Xia, Chenbo <chenbo.xia@intel.com>
> > Sent: Friday, September 10, 2021 10:24
> > To: dev@dpdk.org
> > Cc: Li, Xiaoyun <xiaoyun.li@intel.com>
> > Subject: [PATCH 2/8] app/testpmd: use PCI memory resource access APIs
> >
> > Currently testpmd uses struct rte_pci_device to access PCI memory resource.
> > Since this structure will be internal later, this patch replaces use of
> > rte_pci_device with new PCI memory resource access APIs to read/write BAR 0.
> >
> > Signed-off-by: Chenbo Xia <chenbo.xia@intel.com>
> > ---
> >  app/test-pmd/config.c  | 38 +++++++++++-----------------------
> >  app/test-pmd/testpmd.h | 46 +++++++++++++++++++++---------------------
> >  2 files changed, 35 insertions(+), 49 deletions(-)
> >
> <snip>
> >
> > @@ -1081,7 +1063,9 @@ port_reg_bit_set(portid_t port_id, uint32_t reg_off,
> > uint8_t bit_pos,
> >  			(int) bit_v);
> >  		return;
> >  	}
> > -	reg_v = port_id_pci_reg_read(port_id, reg_off);
> > +	if (port_id_pci_reg_read(port_id, reg_off, &reg_v))
> > +		return;
> > +
> >  	if (bit_v == 0)
> >  		reg_v &= ~(1 << bit_pos);
> >  	else
> > @@ -1123,7 +1107,9 @@ port_reg_bit_field_set(portid_t port_id, uint32_t
> > reg_off,
> >  				(unsigned)max_v, (unsigned)max_v);
> >  		return;
> >  	}
> > -	reg_v = port_id_pci_reg_read(port_id, reg_off);
> > +	if (port_id_pci_reg_read(port_id, reg_off, &reg_v))
> > +		return;
> > +
> >  	reg_v &= ~(max_v << l_bit); /* Keep unchanged bits */
> >  	reg_v |= (value << l_bit); /* Set changed bits */
> >  	port_id_pci_reg_write(port_id, reg_off, reg_v); diff --git a/app/test-
> 
> In your implementation, port_id_pci_reg_write() can fail. Then the following
> display shouldn't be called.
> So change this function to have return value and check it before use it will
> be better.

Make sense. Will fix in v2.

Thanks,
Chenbo

> 
> > pmd/testpmd.h b/app/test-pmd/testpmd.h index 16a3598e48..7922807a6e
> > 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> <snip>
> >  static inline void
> > -port_pci_reg_write(struct rte_port *port, uint32_t reg_off, uint32_t reg_v)
> > +port_id_pci_reg_write(portid_t pt_id, uint32_t reg_off, uint32_t reg_v)
> >  {
> > -	const struct rte_pci_device *pci_dev;
> > +	struct rte_port *port = &ports[(pt_id)];
> > +	char name[RTE_ETH_NAME_MAX_LEN];
> >  	const struct rte_bus *bus;
> > -	void *reg_addr;
> >
> >  	if (!port->dev_info.device) {
> >  		fprintf(stderr, "Invalid device\n");
> > @@ -721,19 +721,19 @@ port_pci_reg_write(struct rte_port *port, uint32_t
> > reg_off, uint32_t reg_v)
> >
> >  	bus = rte_bus_find_by_device(port->dev_info.device);
> >  	if (bus && !strcmp(bus->name, "pci")) {
> > -		pci_dev = RTE_DEV_TO_PCI(port->dev_info.device);
> > +		rte_eth_dev_get_name_by_port(pt_id, name);
> >  	} else {
> >  		fprintf(stderr, "Not a PCI device\n");
> >  		return;
> >  	}
> >
> > -	reg_addr = ((char *)pci_dev->mem_resource[0].addr + reg_off);
> > -	*((volatile uint32_t *)reg_addr) = rte_cpu_to_le_32(reg_v);
> > +	reg_v = rte_cpu_to_le_32(reg_v);
> > +	if (rte_pci_mem_wr32(name, 0, &reg_v, reg_off)) {
> > +		fprintf(stderr, "Failed to write register\n");
> > +		return;
> > +	}
> >  }
> >
> > -#define port_id_pci_reg_write(pt_id, reg_off, reg_value) \
> > -	port_pci_reg_write(&ports[(pt_id)], (reg_off), (reg_value))
> > -
> >  static inline void
> >  get_start_cycles(uint64_t *start_tsc)
> >  {
> > --
> > 2.17.1
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 31d8ba1b91..a0ecda3a8e 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -950,10 +950,6 @@  vlan_id_is_invalid(uint16_t vlan_id)
 static int
 port_reg_off_is_invalid(portid_t port_id, uint32_t reg_off)
 {
-	const struct rte_pci_device *pci_dev;
-	const struct rte_bus *bus;
-	uint64_t pci_len;
-
 	if (reg_off & 0x3) {
 		fprintf(stderr,
 			"Port register offset 0x%X not aligned on a 4-byte boundary\n",
@@ -966,22 +962,6 @@  port_reg_off_is_invalid(portid_t port_id, uint32_t reg_off)
 		return 0;
 	}
 
-	bus = rte_bus_find_by_device(ports[port_id].dev_info.device);
-	if (bus && !strcmp(bus->name, "pci")) {
-		pci_dev = RTE_DEV_TO_PCI(ports[port_id].dev_info.device);
-	} else {
-		fprintf(stderr, "Not a PCI device\n");
-		return 1;
-	}
-
-	pci_len = pci_dev->mem_resource[0].len;
-	if (reg_off >= pci_len) {
-		fprintf(stderr,
-			"Port %d: register offset %u (0x%X) out of port PCI resource (length=%"PRIu64")\n",
-			port_id, (unsigned int)reg_off, (unsigned int)reg_off,
-			pci_len);
-		return 1;
-	}
 	return 0;
 }
 
@@ -1009,14 +989,14 @@  port_reg_bit_display(portid_t port_id, uint32_t reg_off, uint8_t bit_x)
 {
 	uint32_t reg_v;
 
-
 	if (port_id_is_invalid(port_id, ENABLED_WARN))
 		return;
 	if (port_reg_off_is_invalid(port_id, reg_off))
 		return;
 	if (reg_bit_pos_is_invalid(bit_x))
 		return;
-	reg_v = port_id_pci_reg_read(port_id, reg_off);
+	if (port_id_pci_reg_read(port_id, reg_off, &reg_v))
+		return;
 	display_port_and_reg_off(port_id, (unsigned)reg_off);
 	printf("bit %d=%d\n", bit_x, (int) ((reg_v & (1 << bit_x)) >> bit_x));
 }
@@ -1042,7 +1022,8 @@  port_reg_bit_field_display(portid_t port_id, uint32_t reg_off,
 	else
 		l_bit = bit1_pos, h_bit = bit2_pos;
 
-	reg_v = port_id_pci_reg_read(port_id, reg_off);
+	if (port_id_pci_reg_read(port_id, reg_off, &reg_v))
+		return;
 	reg_v >>= l_bit;
 	if (h_bit < 31)
 		reg_v &= ((1 << (h_bit - l_bit + 1)) - 1);
@@ -1060,7 +1041,8 @@  port_reg_display(portid_t port_id, uint32_t reg_off)
 		return;
 	if (port_reg_off_is_invalid(port_id, reg_off))
 		return;
-	reg_v = port_id_pci_reg_read(port_id, reg_off);
+	if (port_id_pci_reg_read(port_id, reg_off, &reg_v))
+		return;
 	display_port_reg_value(port_id, reg_off, reg_v);
 }
 
@@ -1081,7 +1063,9 @@  port_reg_bit_set(portid_t port_id, uint32_t reg_off, uint8_t bit_pos,
 			(int) bit_v);
 		return;
 	}
-	reg_v = port_id_pci_reg_read(port_id, reg_off);
+	if (port_id_pci_reg_read(port_id, reg_off, &reg_v))
+		return;
+
 	if (bit_v == 0)
 		reg_v &= ~(1 << bit_pos);
 	else
@@ -1123,7 +1107,9 @@  port_reg_bit_field_set(portid_t port_id, uint32_t reg_off,
 				(unsigned)max_v, (unsigned)max_v);
 		return;
 	}
-	reg_v = port_id_pci_reg_read(port_id, reg_off);
+	if (port_id_pci_reg_read(port_id, reg_off, &reg_v))
+		return;
+
 	reg_v &= ~(max_v << l_bit); /* Keep unchanged bits */
 	reg_v |= (value << l_bit); /* Set changed bits */
 	port_id_pci_reg_write(port_id, reg_off, reg_v);
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 16a3598e48..7922807a6e 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -678,41 +678,41 @@  mbuf_pool_find(unsigned int sock_id, uint16_t idx)
 /**
  * Read/Write operations on a PCI register of a port.
  */
-static inline uint32_t
-port_pci_reg_read(struct rte_port *port, uint32_t reg_off)
+static inline int
+port_id_pci_reg_read(portid_t pt_id, uint32_t reg_off, uint32_t *reg_v)
 {
-	const struct rte_pci_device *pci_dev;
+	struct rte_port *port = &ports[(pt_id)];
+	char name[RTE_ETH_NAME_MAX_LEN];
 	const struct rte_bus *bus;
-	void *reg_addr;
-	uint32_t reg_v;
 
 	if (!port->dev_info.device) {
 		fprintf(stderr, "Invalid device\n");
-		return 0;
+		return -1;
 	}
 
 	bus = rte_bus_find_by_device(port->dev_info.device);
 	if (bus && !strcmp(bus->name, "pci")) {
-		pci_dev = RTE_DEV_TO_PCI(port->dev_info.device);
+		rte_eth_dev_get_name_by_port(pt_id, name);
 	} else {
 		fprintf(stderr, "Not a PCI device\n");
-		return 0;
+		return -1;
 	}
 
-	reg_addr = ((char *)pci_dev->mem_resource[0].addr + reg_off);
-	reg_v = *((volatile uint32_t *)reg_addr);
-	return rte_le_to_cpu_32(reg_v);
-}
+	if (rte_pci_mem_rd32(name, 0, reg_v, reg_off)) {
+		fprintf(stderr, "Failed to read register\n");
+		return -1;
+	}
 
-#define port_id_pci_reg_read(pt_id, reg_off) \
-	port_pci_reg_read(&ports[(pt_id)], (reg_off))
+	*reg_v = rte_le_to_cpu_32(*reg_v);
+	return 0;
+}
 
 static inline void
-port_pci_reg_write(struct rte_port *port, uint32_t reg_off, uint32_t reg_v)
+port_id_pci_reg_write(portid_t pt_id, uint32_t reg_off, uint32_t reg_v)
 {
-	const struct rte_pci_device *pci_dev;
+	struct rte_port *port = &ports[(pt_id)];
+	char name[RTE_ETH_NAME_MAX_LEN];
 	const struct rte_bus *bus;
-	void *reg_addr;
 
 	if (!port->dev_info.device) {
 		fprintf(stderr, "Invalid device\n");
@@ -721,19 +721,19 @@  port_pci_reg_write(struct rte_port *port, uint32_t reg_off, uint32_t reg_v)
 
 	bus = rte_bus_find_by_device(port->dev_info.device);
 	if (bus && !strcmp(bus->name, "pci")) {
-		pci_dev = RTE_DEV_TO_PCI(port->dev_info.device);
+		rte_eth_dev_get_name_by_port(pt_id, name);
 	} else {
 		fprintf(stderr, "Not a PCI device\n");
 		return;
 	}
 
-	reg_addr = ((char *)pci_dev->mem_resource[0].addr + reg_off);
-	*((volatile uint32_t *)reg_addr) = rte_cpu_to_le_32(reg_v);
+	reg_v = rte_cpu_to_le_32(reg_v);
+	if (rte_pci_mem_wr32(name, 0, &reg_v, reg_off)) {
+		fprintf(stderr, "Failed to write register\n");
+		return;
+	}
 }
 
-#define port_id_pci_reg_write(pt_id, reg_off, reg_value) \
-	port_pci_reg_write(&ports[(pt_id)], (reg_off), (reg_value))
-
 static inline void
 get_start_cycles(uint64_t *start_tsc)
 {