Hi, Xiaolong
On 10/29, Sun GuinanX wrote:
> >macsec setting is not valid when port is stopped.
> >In order to make it valid, the patch changes the setting to where port
> >is started.
> >
> >Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
> >Cc: sta...@dpdk.org
> >
> >Signed-off-by: Sun GuinanX <guinanx....@intel.com>
> >---
> >v2:
> >* Modified function name
> >* Refactored the rte_pmd_ixgbe_macsec_enable/disable function
> >* Modified structure name
> >* Modified the data type of a structure variable
> >---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 160 ++++++++++++++++++++++++++++++
> >drivers/net/ixgbe/ixgbe_ethdev.h | 15 +++
> >drivers/net/ixgbe/rte_pmd_ixgbe.c | 127 ++----------------------
> > 3 files changed, 182 insertions(+), 120 deletions(-)
> >
> >diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >b/drivers/net/ixgbe/ixgbe_ethdev.c
> >index dbce7a80e..7ae7bc9ca 100644
> >--- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >@@ -379,6 +379,11 @@ static int ixgbe_dev_udp_tunnel_port_del(struct
> >rte_eth_dev *dev, static int ixgbe_filter_restore(struct rte_eth_dev
> >*dev); static void ixgbe_l2_tunnel_conf(struct rte_eth_dev *dev);
> >
> >+static void ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
> >+ struct ixgbe_macsec_setting *macsec_contrl);
> >+
> >+static void ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev
> >+*dev);
> >+
> > /*
> > * Define VF Stats MACRO for Non "cleared on read" register
> > */
> >@@ -2542,6 +2547,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> > uint32_t *link_speeds;
> > struct ixgbe_tm_conf *tm_conf =
> > IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
> >+ struct ixgbe_macsec_setting *macsec_ctrl =
> >+ IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data-
> >dev_private);
> >
> > PMD_INIT_FUNC_TRACE();
> >
> >@@ -2794,6 +2801,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> > */
> > ixgbe_dev_link_update(dev, 0);
> >
> >+ /* setup the macsec ctrl register */
> >+ ixgbe_dev_macsec_ctrl_register_enable(dev, macsec_ctrl);
> >+
> > return 0;
> >
> > error:
> >@@ -2825,6 +2835,9 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
> >
> > PMD_INIT_FUNC_TRACE();
> >
> >+ /* disable mecsec register */
> >+ ixgbe_dev_macsec_ctrl_register_disable(dev);
> >+
> > rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
> >
> > /* disable interrupts */
> >@@ -8816,6 +8829,153 @@ ixgbe_clear_all_l2_tn_filter(struct rte_eth_dev
> *dev)
> > return 0;
> > }
> >
> >+/* macsec ctrl setup enable */
>
> This comment is not aligned with the function name, actually since this
> function
> is quite straightforward, the comment is unnecessary.
Similar useless comments had been removed
>
> >+void
> >+ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
> >+ struct ixgbe_macsec_setting *macsec_ctrl) {
> >+ struct ixgbe_macsec_setting *macsec =
> >+ IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data-
> >dev_private);
> >+
> >+ macsec->encrypt_en = macsec_ctrl->encrypt_en;
> >+ macsec->replayprotect_en = macsec_ctrl->replayprotect_en; }
> >+
> >+/* macsec ctrl setup disable */
>
> ditto
Similar useless comments had been removed
>
> >+void
> >+ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev) {
> >+ struct ixgbe_macsec_setting *macsec =
> >+ IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data-
> >dev_private);
> >+
> >+ macsec->encrypt_en = 0;
> >+ macsec->replayprotect_en = 0;
> >+}
> >+
> >+/* macsec ctrl register set */
> >+static void
> >+ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
> >+ struct ixgbe_macsec_setting *macsec_contrl) {
> >+ struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> >+ uint32_t ctrl;
> >+ uint8_t en = (uint8_t)macsec_contrl->encrypt_en;
> >+ uint8_t rp = (uint8_t)macsec_contrl->replayprotect_en;
> >+
> >+ /**
> >+ * Workaround:
> >+ * As no ixgbe_disable_sec_rx_path equivalent is
> >+ * implemented for tx in the base code, and we are
> >+ * not allowed to modify the base code in DPDK, so
> >+ * just call the hand-written one directly for now.
> >+ * The hardware support has been checked by
> >+ * ixgbe_disable_sec_rx_path().
> >+ */
> >+ ixgbe_disable_sec_tx_path_generic(hw);
> >+
> >+ /* Enable Ethernet CRC (required by MACsec offload) */
> >+ ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
> >+ ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
> >+ IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
> >+
> >+ /* Enable the TX and RX crypto engines */
> >+ ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
> >+ ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
> >+ IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
> >+
> >+ ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
> >+ ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
> >+ IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
> >+
> >+ ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
> >+ ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
> >+ ctrl |= 0x3;
> >+ IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
> >+
> >+ /* Enable SA lookup */
> >+ ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
> >+ ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
> >+ ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
> >+ IXGBE_LSECTXCTRL_AUTH;
> >+ ctrl |= IXGBE_LSECTXCTRL_AISCI;
> >+ ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
> >+ ctrl |= IXGBE_MACSEC_PNTHRSH &
> IXGBE_LSECTXCTRL_PNTHRSH_MASK;
> >+ IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
> >+
> >+ ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
> >+ ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
> >+ ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
> >+ ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
> >+ if (rp)
> >+ ctrl |= IXGBE_LSECRXCTRL_RP;
> >+ else
> >+ ctrl &= ~IXGBE_LSECRXCTRL_RP;
> >+ IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
> >+
> >+ /* Start the data paths */
> >+ ixgbe_enable_sec_rx_path(hw);
> >+ /**
> >+ * Workaround:
> >+ * As no ixgbe_enable_sec_rx_path equivalent is
> >+ * implemented for tx in the base code, and we are
> >+ * not allowed to modify the base code in DPDK, so
> >+ * just call the hand-written one directly for now.
> >+ */
> >+ ixgbe_enable_sec_tx_path_generic(hw);
> >+}
> >+
> >+/* macsec ctrl register set */
> >+static void
> >+ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev *dev) {
> >+ struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> >+ uint32_t ctrl;
> >+
> >+ /**
> >+ * Workaround:
> >+ * As no ixgbe_disable_sec_rx_path equivalent is
> >+ * implemented for tx in the base code, and we are
> >+ * not allowed to modify the base code in DPDK, so
> >+ * just call the hand-written one directly for now.
> >+ * The hardware support has been checked by
> >+ * ixgbe_disable_sec_rx_path().
> >+ */
> >+ ixgbe_disable_sec_tx_path_generic(hw);
> >+
> >+ /* Disable the TX and RX crypto engines */
> >+ ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
> >+ ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
> >+ IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
> >+
> >+ ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
> >+ ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
> >+ IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
> >+
> >+ /* Disable SA lookup */
> >+ ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
> >+ ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
> >+ ctrl |= IXGBE_LSECTXCTRL_DISABLE;
> >+ IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
> >+
> >+ ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
> >+ ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
> >+ ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
> >+ IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
> >+
> >+ /* Start the data paths */
> >+ ixgbe_enable_sec_rx_path(hw);
> >+ /**
> >+ * Workaround:
> >+ * As no ixgbe_enable_sec_rx_path equivalent is
> >+ * implemented for tx in the base code, and we are
> >+ * not allowed to modify the base code in DPDK, so
> >+ * just call the hand-written one directly for now.
> >+ */
> >+ ixgbe_enable_sec_tx_path_generic(hw);
> >+}
> >+
> >+
> >+
>
> One empty line is enough here.
had been removed
>
> > RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
> >RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
> >RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic |
> >vfio-pci"); diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h
> >b/drivers/net/ixgbe/ixgbe_ethdev.h
> >index 6e9ed2e10..6aa2cdbbc 100644
> >--- a/drivers/net/ixgbe/ixgbe_ethdev.h
> >+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> >@@ -365,6 +365,12 @@ struct rte_flow {
> > void *rule;
> > };
> >
> >+/* MACsec Control structure */
>
> Comment is unaligned and unnecessary.
had been removed
>
> >+struct ixgbe_macsec_setting {
> >+ uint8_t encrypt_en; /* encrypt enabled */
> >+ uint8_t replayprotect_en; /* replayprotect enabled */
> >+};
> >+
> > /*
> > * Statistics counters collected by the MACsec
> > */
> >@@ -471,6 +477,7 @@ struct ixgbe_adapter {
> > struct ixgbe_hw hw;
> > struct ixgbe_hw_stats stats;
> > struct ixgbe_macsec_stats macsec_stats;
> >+ struct ixgbe_macsec_setting macsec_ctrl;
> > struct ixgbe_hw_fdir_info fdir;
> > struct ixgbe_interrupt intr;
> > struct ixgbe_stat_mapping_registers stat_mappings; @@ -523,6 +530,9
> >@@ int ixgbe_vf_representor_uninit(struct rte_eth_dev *ethdev);
> >#define IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(adapter) \
> > (&((struct ixgbe_adapter *)adapter)->macsec_stats)
> >
> >+#define IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(adapter) \
> >+ (&((struct ixgbe_adapter *)adapter)->macsec_ctrl)
> >+
> > #define IXGBE_DEV_PRIVATE_TO_INTR(adapter) \
> > (&((struct ixgbe_adapter *)adapter)->intr)
> >
> >@@ -741,6 +751,11 @@ int ixgbe_action_rss_same(const struct
> >rte_flow_action_rss *comp, int ixgbe_config_rss_filter(struct rte_eth_dev
> *dev,
> > struct ixgbe_rte_flow_rss_conf *conf, bool add);
> >
> >+void ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
> >+ struct ixgbe_macsec_setting *macsec_ctrl);
> >+
> >+void ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev);
> >+
> > static inline int
> > ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info,
> > uint16_t ethertype)
> >diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.c
> >b/drivers/net/ixgbe/rte_pmd_ixgbe.c
> >index 9514f2cf5..3a1474876 100644
> >--- a/drivers/net/ixgbe/rte_pmd_ixgbe.c
> >+++ b/drivers/net/ixgbe/rte_pmd_ixgbe.c
> >@@ -515,82 +515,18 @@ rte_pmd_ixgbe_set_vf_rate_limit(uint16_t port,
> >uint16_t vf, int rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t
> >en, uint8_t rp) {
>
> I think there is usercase when dev is started, and user calls
> rte_pmd_ixgbe_macsec_enable and he expects it will take effect, so we still
> need to configure register (call ixgbe_dev_macsec_ctrl_register_enable) here
> other than just caching the macsec setting.
>
This kind of usercase processing will be added in the patch of V3.
> >- struct ixgbe_hw *hw;
> > struct rte_eth_dev *dev;
> >- uint32_t ctrl;
> >+ struct ixgbe_macsec_setting macsec_setting;
> >
> > RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> >
> > dev = &rte_eth_devices[port];
> >
> >- if (!is_ixgbe_supported(dev))
> >- return -ENOTSUP;
> >-
> >- hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >+ macsec_setting.encrypt_en = en;
> >+ macsec_setting.replayprotect_en = rp;
> >
> >- /* Stop the data paths */
> >- if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
> >- return -ENOTSUP;
> >- /**
> >- * Workaround:
> >- * As no ixgbe_disable_sec_rx_path equivalent is
> >- * implemented for tx in the base code, and we are
> >- * not allowed to modify the base code in DPDK, so
> >- * just call the hand-written one directly for now.
> >- * The hardware support has been checked by
> >- * ixgbe_disable_sec_rx_path().
> >- */
> >- ixgbe_disable_sec_tx_path_generic(hw);
> >-
> >- /* Enable Ethernet CRC (required by MACsec offload) */
> >- ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
> >- ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
> >- IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
> >-
> >- /* Enable the TX and RX crypto engines */
> >- ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
> >- ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
> >- IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
> >-
> >- ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
> >- ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
> >- IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
> >-
> >- ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
> >- ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
> >- ctrl |= 0x3;
> >- IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
> >-
> >- /* Enable SA lookup */
> >- ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
> >- ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
> >- ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
> >- IXGBE_LSECTXCTRL_AUTH;
> >- ctrl |= IXGBE_LSECTXCTRL_AISCI;
> >- ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
> >- ctrl |= IXGBE_MACSEC_PNTHRSH &
> IXGBE_LSECTXCTRL_PNTHRSH_MASK;
> >- IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
> >-
> >- ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
> >- ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
> >- ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
> >- ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
> >- if (rp)
> >- ctrl |= IXGBE_LSECRXCTRL_RP;
> >- else
> >- ctrl &= ~IXGBE_LSECRXCTRL_RP;
> >- IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
> >-
> >- /* Start the data paths */
> >- ixgbe_enable_sec_rx_path(hw);
> >- /**
> >- * Workaround:
> >- * As no ixgbe_enable_sec_rx_path equivalent is
> >- * implemented for tx in the base code, and we are
> >- * not allowed to modify the base code in DPDK, so
> >- * just call the hand-written one directly for now.
> >- */
> >- ixgbe_enable_sec_tx_path_generic(hw);
> >+ /* Enable macsec setup */
> >+ ixgbe_dev_macsec_setting_save(dev, &macsec_setting);
> >
> > return 0;
> > }
> >@@ -598,63 +534,14 @@ rte_pmd_ixgbe_macsec_enable(uint16_t port,
> >uint8_t en, uint8_t rp) int rte_pmd_ixgbe_macsec_disable(uint16_t
> >port) {
>
> Ditto.
>
This kind of usercase processing will be added in the patch of V3.
> Thanks,
> Xiaolong
