On 12/12/2024 7:02 PM, Stephen Hemminger wrote:
> On Thu, 12 Dec 2024 16:19:03 +0000
> Anatoly Burakov <anatoly.burakov@intel.com> wrote:
>
>> Currently, the architecture of the base driver is such that it uses
>> function pointers internally. These are not guaranteed to be valid in
>> secondary processes, which can lead to crashes. This patch prevents these
>> functions from being executed in e1000 driver.
>>
>> Fixes: 805803445a02 ("e1000: support EM devices (also known as e1000/e1000e)")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>
> Not a fan of this. It creates so many special cases like: "This is ixgbe, and
> it can do X but not Y in secondary process".
>
> Either the driver should get fixed correctly so that all operations work
> in secondary process, yes you would have to fix the base code.
>
> Or the driver should be not support secondary process model at all.
>
> If you have to write lots of documentation about limitations, it is not helping
> the user.
That is the intention. Fixing these issues will take some effort as
there's a lot of code to fix due to how endemic function pointers' usage
are to these drivers, but in the meantime, things "arbitrarily not
working" is better than things crashing outright.
@@ -153,3 +153,21 @@ The following are known limitations:
#. Qemu e1000 only supports one interrupt source, so link and Rx interrupt should be exclusive.
#. Qemu e1000 does not support interrupt auto-clear, application should disable interrupt immediately when woken up.
+
+Secondary Process Support
+-------------------------
+
+The following ethdev API's are currently not supported for use in secondary processes:
+
+* ``rte_eth_dev_start``
+* ``rte_eth_dev_stop``
+* ``rte_eth_dev_rx_intr_enable``
+* ``rte_eth_dev_rx_intr_disable``
+* ``rte_eth_link_get``
+* ``rte_eth_dev_led_on``
+* ``rte_eth_dev_led_off``
+* ``rte_eth_dev_flow_ctrl_set``
+* ``rte_eth_dev_default_mac_addr_set``
+* ``rte_eth_dev_mac_addr_add``
+* ``rte_eth_dev_mac_addr_remove``
+* ``rte_eth_dev_set_mc_addr_list``
@@ -543,6 +543,14 @@ eth_em_start(struct rte_eth_dev *dev)
PMD_INIT_FUNC_TRACE();
+ /*
+ * This function calls into the base driver, which in turn will use
+ * function pointers, which are not guaranteed to be valid in secondary
+ * processes, so avoid using this function in secondary processes.
+ */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return -E_RTE_SECONDARY;
+
ret = eth_em_stop(dev);
if (ret != 0)
return ret;
@@ -727,6 +735,14 @@ eth_em_stop(struct rte_eth_dev *dev)
struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
+ /*
+ * This function calls into the base driver, which in turn will use
+ * function pointers, which are not guaranteed to be valid in secondary
+ * processes, so avoid using this function in secondary processes.
+ */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return -E_RTE_SECONDARY;
+
dev->data->dev_started = 0;
eth_em_rxtx_control(dev, false);
@@ -1016,6 +1032,10 @@ eth_em_rx_queue_intr_enable(struct rte_eth_dev *dev, __rte_unused uint16_t queue
struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
+ /* device interrupts are only subscribed to in primary processes */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return -E_RTE_SECONDARY;
+
em_rxq_intr_enable(hw);
rte_intr_ack(intr_handle);
@@ -1027,6 +1047,10 @@ eth_em_rx_queue_intr_disable(struct rte_eth_dev *dev, __rte_unused uint16_t queu
{
struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ /* device interrupts are only subscribed to in primary processes */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return -E_RTE_SECONDARY;
+
em_rxq_intr_disable(hw);
return 0;
@@ -1654,6 +1678,14 @@ eth_em_led_on(struct rte_eth_dev *dev)
{
struct e1000_hw *hw;
+ /*
+ * This function calls into the base driver, which in turn will use
+ * function pointers, which are not guaranteed to be valid in secondary
+ * processes, so avoid using this function in secondary processes.
+ */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return -E_RTE_SECONDARY;
+
hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
return e1000_led_on(hw) == E1000_SUCCESS ? 0 : -ENOTSUP;
}
@@ -1663,6 +1695,14 @@ eth_em_led_off(struct rte_eth_dev *dev)
{
struct e1000_hw *hw;
+ /*
+ * This function calls into the base driver, which in turn will use
+ * function pointers, which are not guaranteed to be valid in secondary
+ * processes, so avoid using this function in secondary processes.
+ */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return -E_RTE_SECONDARY;
+
hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
return e1000_led_off(hw) == E1000_SUCCESS ? 0 : -ENOTSUP;
}
@@ -1724,6 +1764,14 @@ eth_em_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf)
uint32_t max_high_water;
uint32_t rctl;
+ /*
+ * This function calls into the base driver, which in turn will use
+ * function pointers, which are not guaranteed to be valid in secondary
+ * processes, so avoid using this function in secondary processes.
+ */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return -E_RTE_SECONDARY;
+
hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
if (fc_conf->autoneg != hw->mac.autoneg)
return -ENOTSUP;
@@ -1775,6 +1823,14 @@ eth_em_rar_set(struct rte_eth_dev *dev, struct rte_ether_addr *mac_addr,
{
struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ /*
+ * This function calls into the base driver, which in turn will use
+ * function pointers, which are not guaranteed to be valid in secondary
+ * processes, so avoid using this function in secondary processes.
+ */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return -E_RTE_SECONDARY;
+
return e1000_rar_set(hw, mac_addr->addr_bytes, index);
}
@@ -1784,6 +1840,14 @@ eth_em_rar_clear(struct rte_eth_dev *dev, uint32_t index)
uint8_t addr[RTE_ETHER_ADDR_LEN];
struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ /*
+ * This function calls into the base driver, which in turn will use
+ * function pointers, which are not guaranteed to be valid in secondary
+ * processes, so avoid using this function in secondary processes.
+ */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return;
+
memset(addr, 0, sizeof(addr));
e1000_rar_set(hw, addr, index);
@@ -1793,6 +1857,14 @@ static int
eth_em_default_mac_addr_set(struct rte_eth_dev *dev,
struct rte_ether_addr *addr)
{
+ /*
+ * This function calls into the base driver, which in turn will use
+ * function pointers, which are not guaranteed to be valid in secondary
+ * processes, so avoid using this function in secondary processes.
+ */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return -E_RTE_SECONDARY;
+
eth_em_rar_clear(dev, 0);
return eth_em_rar_set(dev, (void *)addr, 0, 0);
@@ -1837,6 +1909,14 @@ eth_em_set_mc_addr_list(struct rte_eth_dev *dev,
{
struct e1000_hw *hw;
+ /*
+ * This function calls into the base driver, which in turn will use
+ * function pointers, which are not guaranteed to be valid in secondary
+ * processes, so avoid using this function in secondary processes.
+ */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return -E_RTE_SECONDARY;
+
hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
e1000_update_mc_addr_list(hw, (u8 *)mc_addr_set, nb_mc_addr);
return 0;