From 001a1c0f98f4e3ac19c26515892e2448a7547c38 Mon Sep 17 00:00:00 2001 From: Zyta Szpak Date: Mon, 4 Jul 2016 13:36:46 +0200 Subject: [PATCH] ethdev: get registers width The ethtool app was allocating too little space for 64-bit registers which resulted in memory corruption. Removes hard-coded assumption that device registers are always 32 bits wide. The rte_eth_dev_get_reg_length and rte_eth_dev_get_reg_info callbacks did not provide register size to the app in any way while is needed to allocate correct number of bytes before retrieving registers using rte_eth_dev_get_reg. This commit changes rte_eth_dev_get_reg_info so that it can be used to retrieve both the number of registers and their width, and removes the now-redundant rte_eth_dev_get_reg_length. Signed-off-by: Zyta Szpak Acked-by: Remy Horton --- drivers/net/cxgbe/cxgbe_ethdev.c | 14 ++++++++++---- drivers/net/e1000/igb_ethdev.c | 14 ++++++++++++-- drivers/net/i40e/i40e_ethdev.c | 15 ++++++--------- drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++-- drivers/net/thunderx/nicvf_ethdev.c | 14 +++++--------- drivers/net/thunderx/nicvf_ethdev.h | 1 + examples/ethtool/lib/rte_ethtool.c | 19 +++++++++++++------ lib/librte_ether/rte_dev_info.h | 1 + lib/librte_ether/rte_ethdev.c | 12 ------------ lib/librte_ether/rte_ethdev.h | 25 +++++-------------------- lib/librte_ether/rte_ether_version.map | 1 - 11 files changed, 65 insertions(+), 65 deletions(-) diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c index 0c12bec45e..9208a61533 100644 --- a/drivers/net/cxgbe/cxgbe_ethdev.c +++ b/drivers/net/cxgbe/cxgbe_ethdev.c @@ -934,10 +934,17 @@ static int cxgbe_get_regs(struct rte_eth_dev *eth_dev, struct port_info *pi = (struct port_info *)(eth_dev->data->dev_private); struct adapter *adapter = pi->adapter; - regs->length = cxgbe_get_regs_len(eth_dev); regs->version = CHELSIO_CHIP_VERSION(adapter->params.chip) | - (CHELSIO_CHIP_RELEASE(adapter->params.chip) << 10) | - (1 << 16); + (CHELSIO_CHIP_RELEASE(adapter->params.chip) << 10) | + (1 << 16); + + if (regs->data == NULL) { + regs->length = cxgbe_get_regs_len(eth_dev); + regs->width = sizeof(uint32_t); + + return 0; + } + t4_get_regs(adapter, regs->data, (regs->length * sizeof(uint32_t))); return 0; @@ -971,7 +978,6 @@ static const struct eth_dev_ops cxgbe_eth_dev_ops = { .get_eeprom_length = cxgbe_get_eeprom_length, .get_eeprom = cxgbe_get_eeprom, .set_eeprom = cxgbe_set_eeprom, - .get_reg_length = cxgbe_get_regs_len, .get_reg = cxgbe_get_regs, }; diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c index 76d9f5307a..fbf4d09098 100644 --- a/drivers/net/e1000/igb_ethdev.c +++ b/drivers/net/e1000/igb_ethdev.c @@ -386,7 +386,6 @@ static const struct eth_dev_ops eth_igb_ops = { .timesync_disable = igb_timesync_disable, .timesync_read_rx_timestamp = igb_timesync_read_rx_timestamp, .timesync_read_tx_timestamp = igb_timesync_read_tx_timestamp, - .get_reg_length = eth_igb_get_reg_length, .get_reg = eth_igb_get_regs, .get_eeprom_length = eth_igb_get_eeprom_length, .get_eeprom = eth_igb_get_eeprom, @@ -426,7 +425,6 @@ static const struct eth_dev_ops igbvf_eth_dev_ops = { .rxq_info_get = igb_rxq_info_get, .txq_info_get = igb_txq_info_get, .mac_addr_set = igbvf_default_mac_addr_set, - .get_reg_length = igbvf_get_reg_length, .get_reg = igbvf_get_regs, }; @@ -4945,6 +4943,12 @@ eth_igb_get_regs(struct rte_eth_dev *dev, int count = 0; const struct reg_info *reg_group; + if (data == NULL) { + regs->length = eth_igb_get_reg_length(dev); + regs->width = sizeof(uint32_t); + return 0; + } + /* Support only full register dump */ if ((regs->length == 0) || (regs->length == (uint32_t)eth_igb_get_reg_length(dev))) { @@ -4969,6 +4973,12 @@ igbvf_get_regs(struct rte_eth_dev *dev, int count = 0; const struct reg_info *reg_group; + if (data == NULL) { + regs->length = igbvf_get_reg_length(dev); + regs->width = sizeof(uint32_t); + return 0; + } + /* Support only full register dump */ if ((regs->length == 0) || (regs->length == (uint32_t)igbvf_get_reg_length(dev))) { diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index 47b9b6b2e3..06bc0b6a7d 100644 --- a/drivers/net/i40e/i40e_ethdev.c +++ b/drivers/net/i40e/i40e_ethdev.c @@ -440,8 +440,6 @@ static int i40e_dev_rx_queue_intr_enable(struct rte_eth_dev *dev, static int i40e_dev_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t queue_id); -static int i40e_get_reg_length(struct rte_eth_dev *dev); - static int i40e_get_regs(struct rte_eth_dev *dev, struct rte_dev_reg_info *regs); @@ -524,7 +522,6 @@ static const struct eth_dev_ops i40e_eth_dev_ops = { .timesync_adjust_time = i40e_timesync_adjust_time, .timesync_read_time = i40e_timesync_read_time, .timesync_write_time = i40e_timesync_write_time, - .get_reg_length = i40e_get_reg_length, .get_reg = i40e_get_regs, .get_eeprom_length = i40e_get_eeprom_length, .get_eeprom = i40e_get_eeprom, @@ -9343,12 +9340,6 @@ i40e_dev_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t queue_id) return 0; } -static int i40e_get_reg_length(__rte_unused struct rte_eth_dev *dev) -{ - /* Highest base addr + 32-bit word */ - return I40E_GLGEN_STAT_CLEAR + 4; -} - static int i40e_get_regs(struct rte_eth_dev *dev, struct rte_dev_reg_info *regs) { @@ -9357,6 +9348,12 @@ static int i40e_get_regs(struct rte_eth_dev *dev, uint32_t reg_idx, arr_idx, arr_idx2, reg_offset; const struct i40e_reg_info *reg_info; + if (ptr_data == NULL) { + regs->length = I40E_GLGEN_STAT_CLEAR + 4; + regs->width = sizeof(uint32_t); + return 0; + } + /* The first few registers have to be read using AQ operations */ reg_idx = 0; while (i40e_regs_adminq[reg_idx].name) { diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c index 3daa070d0e..d478a159b1 100644 --- a/drivers/net/ixgbe/ixgbe_ethdev.c +++ b/drivers/net/ixgbe/ixgbe_ethdev.c @@ -538,7 +538,6 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = { .timesync_disable = ixgbe_timesync_disable, .timesync_read_rx_timestamp = ixgbe_timesync_read_rx_timestamp, .timesync_read_tx_timestamp = ixgbe_timesync_read_tx_timestamp, - .get_reg_length = ixgbe_get_reg_length, .get_reg = ixgbe_get_regs, .get_eeprom_length = ixgbe_get_eeprom_length, .get_eeprom = ixgbe_get_eeprom, @@ -589,7 +588,6 @@ static const struct eth_dev_ops ixgbevf_eth_dev_ops = { .rxq_info_get = ixgbe_rxq_info_get, .txq_info_get = ixgbe_txq_info_get, .mac_addr_set = ixgbevf_set_default_mac_addr, - .get_reg_length = ixgbevf_get_reg_length, .get_reg = ixgbevf_get_regs, .reta_update = ixgbe_dev_rss_reta_update, .reta_query = ixgbe_dev_rss_reta_query, @@ -6316,6 +6314,12 @@ ixgbe_get_regs(struct rte_eth_dev *dev, const struct reg_info **reg_set = (hw->mac.type == ixgbe_mac_82598EB) ? ixgbe_regs_mac_82598EB : ixgbe_regs_others; + if (data == NULL) { + regs->length = ixgbe_get_reg_length(dev); + regs->width = sizeof(uint32_t); + return 0; + } + /* Support only full register dump */ if ((regs->length == 0) || (regs->length == (uint32_t)ixgbe_get_reg_length(dev))) { @@ -6340,6 +6344,12 @@ ixgbevf_get_regs(struct rte_eth_dev *dev, int count = 0; const struct reg_info *reg_group; + if (data == NULL) { + regs->length = ixgbevf_get_reg_length(dev); + regs->width = sizeof(uint32_t); + return 0; + } + /* Support only full register dump */ if ((regs->length == 0) || (regs->length == (uint32_t)ixgbevf_get_reg_length(dev))) { diff --git a/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c index 6e58d824f6..68a72e42f7 100644 --- a/drivers/net/thunderx/nicvf_ethdev.c +++ b/drivers/net/thunderx/nicvf_ethdev.c @@ -188,20 +188,17 @@ nicvf_dev_set_mtu(struct rte_eth_dev *dev, uint16_t mtu) return 0; } -static int -nicvf_dev_get_reg_length(struct rte_eth_dev *dev __rte_unused) -{ - return nicvf_reg_get_count(); -} - static int nicvf_dev_get_regs(struct rte_eth_dev *dev, struct rte_dev_reg_info *regs) { uint64_t *data = regs->data; struct nicvf *nic = nicvf_pmd_priv(dev); - if (data == NULL) - return -EINVAL; + if (data == NULL) { + regs->length = nicvf_reg_get_count(); + regs->width = THUNDERX_REG_BYTES; + return 0; + } /* Support only full register dump */ if ((regs->length == 0) || @@ -1623,7 +1620,6 @@ static const struct eth_dev_ops nicvf_eth_dev_ops = { .rx_queue_count = nicvf_dev_rx_queue_count, .tx_queue_setup = nicvf_dev_tx_queue_setup, .tx_queue_release = nicvf_dev_tx_queue_release, - .get_reg_length = nicvf_dev_get_reg_length, .get_reg = nicvf_dev_get_regs, }; diff --git a/drivers/net/thunderx/nicvf_ethdev.h b/drivers/net/thunderx/nicvf_ethdev.h index 59fa19cf2d..34447e05f0 100644 --- a/drivers/net/thunderx/nicvf_ethdev.h +++ b/drivers/net/thunderx/nicvf_ethdev.h @@ -36,6 +36,7 @@ #include #define THUNDERX_NICVF_PMD_VERSION "1.0" +#define THUNDERX_REG_BYTES 8 #define NICVF_INTR_POLL_INTERVAL_MS 50 #define NICVF_HALF_DUPLEX 0x00 diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c index 54391f213c..a1f91d45d3 100644 --- a/examples/ethtool/lib/rte_ethtool.c +++ b/examples/ethtool/lib/rte_ethtool.c @@ -46,6 +46,7 @@ int rte_ethtool_get_drvinfo(uint8_t port_id, struct ethtool_drvinfo *drvinfo) { struct rte_eth_dev_info dev_info; + struct rte_dev_reg_info reg_info; int n; if (drvinfo == NULL) @@ -65,7 +66,9 @@ rte_ethtool_get_drvinfo(uint8_t port_id, struct ethtool_drvinfo *drvinfo) dev_info.pci_dev->addr.domain, dev_info.pci_dev->addr.bus, dev_info.pci_dev->addr.devid, dev_info.pci_dev->addr.function); - n = rte_eth_dev_get_reg_length(port_id); + memset(®_info, 0, sizeof(reg_info)); + rte_eth_dev_get_reg_info(port_id, ®_info); + n = reg_info.length; if (n > 0) drvinfo->regdump_len = n; else @@ -86,12 +89,16 @@ rte_ethtool_get_drvinfo(uint8_t port_id, struct ethtool_drvinfo *drvinfo) int rte_ethtool_get_regs_len(uint8_t port_id) { - int count_regs; + struct rte_dev_reg_info reg_info; + int ret; + + memset(®_info, 0, sizeof(reg_info)); + + ret = rte_eth_dev_get_reg_info(port_id, ®_info); + if (ret) + return ret; - count_regs = rte_eth_dev_get_reg_length(port_id); - if (count_regs > 0) - return count_regs * sizeof(uint32_t); - return count_regs; + return reg_info.length * reg_info.width; } int diff --git a/lib/librte_ether/rte_dev_info.h b/lib/librte_ether/rte_dev_info.h index 291bd4d73b..574683d315 100644 --- a/lib/librte_ether/rte_dev_info.h +++ b/lib/librte_ether/rte_dev_info.h @@ -41,6 +41,7 @@ struct rte_dev_reg_info { void *data; /**< Buffer for return registers */ uint32_t offset; /**< Start register table location for access */ uint32_t length; /**< Number of registers to fetch */ + uint32_t width; /**< Size of device register */ uint32_t version; /**< Device version */ }; diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index eac260f15f..3cc667af9c 100644 --- a/lib/librte_ether/rte_ethdev.c +++ b/lib/librte_ether/rte_ethdev.c @@ -3304,18 +3304,6 @@ rte_eth_timesync_write_time(uint8_t port_id, const struct timespec *timestamp) return (*dev->dev_ops->timesync_write_time)(dev, timestamp); } -int -rte_eth_dev_get_reg_length(uint8_t port_id) -{ - struct rte_eth_dev *dev; - - RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); - - dev = &rte_eth_devices[port_id]; - RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_reg_length, -ENOTSUP); - return (*dev->dev_ops->get_reg_length)(dev); -} - int rte_eth_dev_get_reg_info(uint8_t port_id, struct rte_dev_reg_info *info) { diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index 0f1732310c..4dac364a26 100644 --- a/lib/librte_ether/rte_ethdev.h +++ b/lib/librte_ether/rte_ethdev.h @@ -1321,9 +1321,6 @@ typedef int (*eth_timesync_write_time)(struct rte_eth_dev *dev, const struct timespec *timestamp); /**< @internal Function used to get time from the device clock */ -typedef int (*eth_get_reg_length_t)(struct rte_eth_dev *dev); -/**< @internal Retrieve device register count */ - typedef int (*eth_get_reg_t)(struct rte_eth_dev *dev, struct rte_dev_reg_info *info); /**< @internal Retrieve registers */ @@ -1487,8 +1484,6 @@ struct eth_dev_ops { /** Query redirection table. */ reta_query_t reta_query; - eth_get_reg_length_t get_reg_length; - /**< Get # of registers */ eth_get_reg_t get_reg; /**< Get registers */ eth_get_eeprom_length_t get_eeprom_length; @@ -4061,25 +4056,15 @@ int rte_eth_tx_queue_info_get(uint8_t port_id, uint16_t queue_id, struct rte_eth_txq_info *qinfo); /** - * Retrieve number of available registers for access - * - * @param port_id - * The port identifier of the Ethernet device. - * @return - * - (>=0) number of registers if successful. - * - (-ENOTSUP) if hardware doesn't support. - * - (-ENODEV) if *port_id* invalid. - * - others depends on the specific operations implementation. - */ -int rte_eth_dev_get_reg_length(uint8_t port_id); - -/** - * Retrieve device registers and register attributes + * Retrieve device registers and register attributes (number of registers and + * register size) * * @param port_id * The port identifier of the Ethernet device. * @param info - * The template includes buffer for register data and attribute to be filled. + * Pointer to rte_dev_reg_info structure to fill in. If info->data is + * NULL the function fills in the width and length fields. If non-NULL + * the registers are put into the buffer pointed at by the data field. * @return * - (0) if successful. * - (-ENOTSUP) if hardware doesn't support. diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map index e1ccebe0be..45ddf44cc9 100644 --- a/lib/librte_ether/rte_ether_version.map +++ b/lib/librte_ether/rte_ether_version.map @@ -36,7 +36,6 @@ DPDK_2.2 { rte_eth_dev_get_eeprom_length; rte_eth_dev_get_mtu; rte_eth_dev_get_reg_info; - rte_eth_dev_get_reg_length; rte_eth_dev_get_vlan_offload; rte_eth_devices; rte_eth_dev_info_get; -- 2.20.1