On Mon, Dec 19, 2016 at 10:59 PM, Stephen Hemminger
<step...@networkplumber.org> wrote:
> The result from rte_eth_dev_info_get should have pointer to
> device not PCI device. This breaks ABI but is necessary.
>
> Signed-off-by: Stephen Hemminger <sthem...@microsoft.com>
> ---
> app/test-pmd/config.c | 32 ++++++++++++++++++++++++++--
> app/test-pmd/testpmd.c | 11 ++++++++--
> app/test-pmd/testpmd.h | 32 ++++++++++++++++------------
> app/test/test_kni.c | 39
> ++++++++++++++++++++++++++++------
> doc/guides/rel_notes/release_17_02.rst | 10 +++------
> lib/librte_ether/rte_ethdev.c | 3 ++-
> lib/librte_ether/rte_ethdev.h | 2 +-
> 7 files changed, 96 insertions(+), 33 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 8cf537d5..1d0974ad 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -553,6 +553,16 @@ port_id_is_invalid(portid_t port_id, enum print_warning
> warning)
> return 1;
> }
>
> +int
> +port_is_not_pci(portid_t port_id)
> +{
> + if (ports[port_id].pci_dev)
> + return 0;
> +
> + printf("Port %u is not a PCI device\n", port_id);
> + return 1;
> +}
> +
> static int
> vlan_id_is_invalid(uint16_t vlan_id)
> {
> @@ -565,15 +575,22 @@ vlan_id_is_invalid(uint16_t vlan_id)
> static int
> port_reg_off_is_invalid(portid_t port_id, uint32_t reg_off)
> {
> + struct rte_pci_device *pci_dev = ports[port_id].pci_dev;
> uint64_t pci_len;
>
> + if (pci_dev == NULL) {
> + printf("Port %u is not a PCI device\n", port_id);
> + return 1;
> + }
> +
> if (reg_off & 0x3) {
> printf("Port register offset 0x%X not aligned on a 4-byte "
> "boundary\n",
> (unsigned)reg_off);
> return 1;
> }
> - pci_len = ports[port_id].dev_info.pci_dev->mem_resource[0].len;
> +
> + pci_len = pci_dev->mem_resource[0].len;
> if (reg_off >= pci_len) {
> printf("Port %d: register offset %u (0x%X) out of port PCI "
> "resource (length=%"PRIu64")\n",
> @@ -607,9 +624,10 @@ 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_is_not_pci(port_id))
> + return;
> if (port_reg_off_is_invalid(port_id, reg_off))
> return;
> if (reg_bit_pos_is_invalid(bit_x))
> @@ -629,6 +647,8 @@ port_reg_bit_field_display(portid_t port_id, uint32_t
> reg_off,
>
> if (port_id_is_invalid(port_id, ENABLED_WARN))
> return;
> + if (port_is_not_pci(port_id))
> + return;
> if (port_reg_off_is_invalid(port_id, reg_off))
> return;
> if (reg_bit_pos_is_invalid(bit1_pos))
> @@ -658,6 +678,8 @@ port_reg_display(portid_t port_id, uint32_t reg_off)
> return;
> if (port_reg_off_is_invalid(port_id, reg_off))
> return;
> + if (port_is_not_pci(port_id))
> + return;
> reg_v = port_id_pci_reg_read(port_id, reg_off);
> display_port_reg_value(port_id, reg_off, reg_v);
> }
> @@ -670,6 +692,8 @@ port_reg_bit_set(portid_t port_id, uint32_t reg_off,
> uint8_t bit_pos,
>
> if (port_id_is_invalid(port_id, ENABLED_WARN))
> return;
> + if (port_is_not_pci(port_id))
> + return;
> if (port_reg_off_is_invalid(port_id, reg_off))
> return;
> if (reg_bit_pos_is_invalid(bit_pos))
> @@ -698,6 +722,8 @@ port_reg_bit_field_set(portid_t port_id, uint32_t reg_off,
>
> if (port_id_is_invalid(port_id, ENABLED_WARN))
> return;
> + if (port_is_not_pci(port_id))
> + return;
> if (port_reg_off_is_invalid(port_id, reg_off))
> return;
> if (reg_bit_pos_is_invalid(bit1_pos))
> @@ -732,6 +758,8 @@ port_reg_set(portid_t port_id, uint32_t reg_off, uint32_t
> reg_v)
> {
> if (port_id_is_invalid(port_id, ENABLED_WARN))
> return;
> + if (port_is_not_pci(port_id))
> + return;
> if (port_reg_off_is_invalid(port_id, reg_off))
> return;
> port_id_pci_reg_write(port_id, reg_off, reg_v);
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index a0332c26..faf1e16d 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -492,7 +492,6 @@ static void
> init_config(void)
> {
> portid_t pid;
> - struct rte_port *port;
> struct rte_mempool *mbp;
> unsigned int nb_mbuf_per_pool;
> lcoreid_t lc_id;
> @@ -547,9 +546,17 @@ init_config(void)
> }
>
> FOREACH_PORT(pid, ports) {
> - port = &ports[pid];
> + struct rte_port *port = &ports[pid];
> + struct rte_device *dev;
> +
> rte_eth_dev_info_get(pid, &port->dev_info);
>
> + dev = port->dev_info.device;
> + if (dev->driver->type == PMD_PCI)
> + port->pci_dev = container_of(dev, struct
> rte_pci_device, device);
> + else
> + port->pci_dev = NULL;
> +
> if (numa_support) {
> if (port_numa[pid] != NUMA_NO_CONFIG)
> port_per_socket[port_numa[pid]]++;
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 9c1e7039..e8aca32a 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -149,6 +149,7 @@ struct fwd_stream {
> */
> struct rte_port {
> uint8_t enabled; /**< Port enabled or not */
> + struct rte_pci_device *pci_dev;
> struct rte_eth_dev_info dev_info; /**< PCI info + driver name */
> struct rte_eth_conf dev_conf; /**< Port configuration. */
> struct ether_addr eth_addr; /**< Port ethernet address */
> @@ -442,34 +443,36 @@ mbuf_pool_find(unsigned int sock_id)
> * 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)
> +pci_reg_read(struct rte_pci_device *pci_dev, uint32_t reg_off)
> {
> - void *reg_addr;
> + void *reg_addr
> + = (char *)pci_dev->mem_resource[0].addr + reg_off;
> uint32_t reg_v;
>
> - reg_addr = (void *)
> - ((char *)port->dev_info.pci_dev->mem_resource[0].addr +
> - reg_off);
> reg_v = *((volatile uint32_t *)reg_addr);
> return rte_le_to_cpu_32(reg_v);
> }
>
> -#define port_id_pci_reg_read(pt_id, reg_off) \
> - port_pci_reg_read(&ports[(pt_id)], (reg_off))
> +static inline uint32_t
> +port_id_pci_reg_read(portid_t pt_id, uint32_t reg_off)
> +{
> + return pci_reg_read(ports[pt_id].pci_dev, reg_off);
> +}
>
> static inline void
> -port_pci_reg_write(struct rte_port *port, uint32_t reg_off, uint32_t reg_v)
> +pci_reg_write(struct rte_pci_device *pci_dev, uint32_t reg_off, uint32_t
> reg_v)
> {
> - void *reg_addr;
> + void *reg_addr
> + = (char *)pci_dev->mem_resource[0].addr + reg_off;
>
> - reg_addr = (void *)
> - ((char *)port->dev_info.pci_dev->mem_resource[0].addr +
> - reg_off);
> *((volatile uint32_t *)reg_addr) = rte_cpu_to_le_32(reg_v);
> }
>
> -#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
> +port_id_pci_reg_write(portid_t pt_id, uint32_t reg_off, uint32_t reg_v)
> +{
> + return pci_reg_write(ports[pt_id].pci_dev, reg_off, reg_v);
> +}
>
> /* Prototypes */
> unsigned int parse_item_list(char* str, const char* item_name,
> @@ -598,6 +601,7 @@ enum print_warning {
> ENABLED_WARN = 0,
> DISABLED_WARN
> };
> +int port_is_not_pci(portid_t port_id);
> int port_id_is_invalid(portid_t port_id, enum print_warning warning);
>
> /*
> diff --git a/app/test/test_kni.c b/app/test/test_kni.c
> index 309741cb..6b2ebbed 100644
> --- a/app/test/test_kni.c
> +++ b/app/test/test_kni.c
> @@ -370,6 +370,8 @@ test_kni_processing(uint8_t port_id, struct rte_mempool
> *mp)
> struct rte_kni_conf conf;
> struct rte_eth_dev_info info;
> struct rte_kni_ops ops;
> + struct rte_device *dev;
> + struct rte_pci_device *pci_dev;
>
> if (!mp)
> return -1;
> @@ -379,8 +381,16 @@ test_kni_processing(uint8_t port_id, struct rte_mempool
> *mp)
> memset(&ops, 0, sizeof(ops));
>
> rte_eth_dev_info_get(port_id, &info);
> - conf.addr = info.pci_dev->addr;
> - conf.id = info.pci_dev->id;
> +
> + dev = info.device;
> + if (dev->driver->type != PMD_PCI) {
> + printf("device is not PCI\n");
> + return -1;
> + }
> +
> + pci_dev = container_of(dev, struct rte_pci_device, device);
> + conf.addr = pci_dev->addr;
> + conf.id = pci_dev->id;
> snprintf(conf.name, sizeof(conf.name), TEST_KNI_PORT);
>
> /* core id 1 configured for kernel thread */
> @@ -478,6 +488,8 @@ test_kni(void)
> struct rte_kni_conf conf;
> struct rte_eth_dev_info info;
> struct rte_kni_ops ops;
> + struct rte_device *dev;
> + struct rte_pci_device *pci_dev;
>
> /* Initialize KNI subsytem */
> rte_kni_init(KNI_TEST_MAX_PORTS);
> @@ -536,8 +548,16 @@ test_kni(void)
> memset(&conf, 0, sizeof(conf));
> memset(&ops, 0, sizeof(ops));
> rte_eth_dev_info_get(port_id, &info);
> - conf.addr = info.pci_dev->addr;
> - conf.id = info.pci_dev->id;
> +
> + dev = info.device;
> + if (dev->driver->type != PMD_PCI) {
> + printf("device is not PCI\n");
> + return -1;
> + }
> +
> + pci_dev = container_of(dev, struct rte_pci_device, device);
> + conf.addr = pci_dev->addr;
> + conf.id = pci_dev->id;
> conf.group_id = (uint16_t)port_id;
> conf.mbuf_size = MAX_PACKET_SZ;
>
> @@ -565,8 +585,15 @@ test_kni(void)
> memset(&info, 0, sizeof(info));
> memset(&ops, 0, sizeof(ops));
> rte_eth_dev_info_get(port_id, &info);
> - conf.addr = info.pci_dev->addr;
> - conf.id = info.pci_dev->id;
> + dev = info.device;
> + if (dev->driver->type != PMD_PCI) {
> + printf("device is not PCI\n");
> + return -1;
> + }
> +
> + pci_dev = container_of(dev, struct rte_pci_device, device);
> + conf.addr = pci_dev->addr;
> + conf.id = pci_dev->id;
> conf.group_id = (uint16_t)port_id;
> conf.mbuf_size = MAX_PACKET_SZ;
>
> diff --git a/doc/guides/rel_notes/release_17_02.rst
> b/doc/guides/rel_notes/release_17_02.rst
> index 3b650388..30b23703 100644
> --- a/doc/guides/rel_notes/release_17_02.rst
> +++ b/doc/guides/rel_notes/release_17_02.rst
> @@ -106,16 +106,12 @@ API Changes
> ABI Changes
> -----------
>
> -.. This section should contain ABI changes. Sample format:
> -
> - * Add a short 1-2 sentence description of the ABI change that was
> announced in
> +.. * Add a short 1-2 sentence description of the ABI change that was
> announced in
> the previous releases and made in this release. Use fixed width quotes
> for
> ``rte_function_names`` or ``rte_struct_names``. Use the past tense.
>
> - This section is a comment. do not overwrite or remove it.
> - Also, make sure to start the actual text at the margin.
> - =========================================================
> -
> +* The ``rte_eth_dev_info`` structure no longer has pointer to PCI device, but
> + instead has new_field ``device`` which is a pointer to a generic ethernet
> device.
>
>
> Shared Library Versions
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 1e0f2061..71a8e9b9 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1568,7 +1568,8 @@ rte_eth_dev_info_get(uint8_t port_id, struct
> rte_eth_dev_info *dev_info)
>
> RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
> (*dev->dev_ops->dev_infos_get)(dev, dev_info);
> - dev_info->pci_dev = dev->pci_dev;
> +
> + dev_info->device = &dev->pci_dev->device;
> dev_info->driver_name = dev->data->drv_name;
I don't think that exposing the device through dev_info makes this a
future proof. If we want to model some kind of extensions to dev_info
we should instead model this explicitly.
So from my point of view the pci_dev should get removed instead.
> dev_info->nb_rx_queues = dev->data->nb_rx_queues;
> dev_info->nb_tx_queues = dev->data->nb_tx_queues;
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 3c85e331..2b3b4014 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -879,7 +879,7 @@ struct rte_eth_conf {
> * Ethernet device information
> */
> struct rte_eth_dev_info {
> - struct rte_pci_device *pci_dev; /**< Device PCI information. */
> + struct rte_device *device; /**< Device information. */
> const char *driver_name; /**< Device Driver name. */
> unsigned int if_index; /**< Index to bound host interface, or 0 if
> none.
> Use if_indextoname() to translate into an interface name. */
> --
> 2.11.0
>
