From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 87E93A00BE; Wed, 30 Oct 2019 04:19:23 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EE8F51BEA1; Wed, 30 Oct 2019 04:19:21 +0100 (CET) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id A17ED1BE96; Wed, 30 Oct 2019 04:19:19 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Oct 2019 20:19:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,245,1569308400"; d="scan'208";a="211966403" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga002.jf.intel.com with ESMTP; 29 Oct 2019 20:19:17 -0700 Received: from fmsmsx607.amr.corp.intel.com (10.18.126.87) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 29 Oct 2019 20:19:17 -0700 Received: from fmsmsx607.amr.corp.intel.com (10.18.126.87) by fmsmsx607.amr.corp.intel.com (10.18.126.87) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Tue, 29 Oct 2019 20:19:17 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx607.amr.corp.intel.com (10.18.126.87) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Tue, 29 Oct 2019 20:19:16 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.213]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.60]) with mapi id 14.03.0439.000; Wed, 30 Oct 2019 11:19:15 +0800 From: "Sun, GuinanX" To: "Ye, Xiaolong" CC: "dev@dpdk.org" , "Lu, Wenzhuo" , "Yang, Qiming" , "stable@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2] net/ixgbe: fix macsec setting Thread-Index: AQHVjjTyPr6+qMtuwEqAwJaU3ajOeKdwzNQAgAGvlUA= Date: Wed, 30 Oct 2019 03:19:14 +0000 Message-ID: <05758BDAD7FC8E4BAED63D0390A8A9557DC27D@SHSMSX101.ccr.corp.intel.com> References: <20191025092140.37288-1-guinanx.sun@intel.com> <20191029163211.3254-1-guinanx.sun@intel.com> <20191029090306.GA106706@intel.com> In-Reply-To: <20191029090306.GA106706@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix macsec setting X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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: stable@dpdk.org > > > >Signed-off-by: Sun GuinanX > >--- > >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 *d= ev, > >+ 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 =3D > > IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private); > >+ struct ixgbe_macsec_setting *macsec_ctrl =3D > >+ 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 */ >=20 > This comment is not aligned with the function name, actually since this f= unction > is quite straightforward, the comment is unnecessary. Similar useless comments had been removed >=20 > >+void > >+ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev, > >+ struct ixgbe_macsec_setting *macsec_ctrl) { > >+ struct ixgbe_macsec_setting *macsec =3D > >+ IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data- > >dev_private); > >+ > >+ macsec->encrypt_en =3D macsec_ctrl->encrypt_en; > >+ macsec->replayprotect_en =3D macsec_ctrl->replayprotect_en; } > >+ > >+/* macsec ctrl setup disable */ >=20 > ditto Similar useless comments had been removed >=20 > >+void > >+ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev) { > >+ struct ixgbe_macsec_setting *macsec =3D > >+ IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data- > >dev_private); > >+ > >+ macsec->encrypt_en =3D 0; > >+ macsec->replayprotect_en =3D 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 =3D IXGBE_DEV_PRIVATE_TO_HW(dev->data- > >dev_private); > >+ uint32_t ctrl; > >+ uint8_t en =3D (uint8_t)macsec_contrl->encrypt_en; > >+ uint8_t rp =3D (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 =3D IXGBE_READ_REG(hw, IXGBE_HLREG0); > >+ ctrl |=3D IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP; > >+ IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl); > >+ > >+ /* Enable the TX and RX crypto engines */ > >+ ctrl =3D IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); > >+ ctrl &=3D ~IXGBE_SECTXCTRL_SECTX_DIS; > >+ IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl); > >+ > >+ ctrl =3D IXGBE_READ_REG(hw, IXGBE_SECRXCTRL); > >+ ctrl &=3D ~IXGBE_SECRXCTRL_SECRX_DIS; > >+ IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl); > >+ > >+ ctrl =3D IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG); > >+ ctrl &=3D ~IXGBE_SECTX_MINSECIFG_MASK; > >+ ctrl |=3D 0x3; > >+ IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl); > >+ > >+ /* Enable SA lookup */ > >+ ctrl =3D IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL); > >+ ctrl &=3D ~IXGBE_LSECTXCTRL_EN_MASK; > >+ ctrl |=3D en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT : > >+ IXGBE_LSECTXCTRL_AUTH; > >+ ctrl |=3D IXGBE_LSECTXCTRL_AISCI; > >+ ctrl &=3D ~IXGBE_LSECTXCTRL_PNTHRSH_MASK; > >+ ctrl |=3D IXGBE_MACSEC_PNTHRSH & > IXGBE_LSECTXCTRL_PNTHRSH_MASK; > >+ IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl); > >+ > >+ ctrl =3D IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL); > >+ ctrl &=3D ~IXGBE_LSECRXCTRL_EN_MASK; > >+ ctrl |=3D IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT; > >+ ctrl &=3D ~IXGBE_LSECRXCTRL_PLSH; > >+ if (rp) > >+ ctrl |=3D IXGBE_LSECRXCTRL_RP; > >+ else > >+ ctrl &=3D ~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 =3D 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 =3D IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); > >+ ctrl |=3D IXGBE_SECTXCTRL_SECTX_DIS; > >+ IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl); > >+ > >+ ctrl =3D IXGBE_READ_REG(hw, IXGBE_SECRXCTRL); > >+ ctrl |=3D IXGBE_SECRXCTRL_SECRX_DIS; > >+ IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl); > >+ > >+ /* Disable SA lookup */ > >+ ctrl =3D IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL); > >+ ctrl &=3D ~IXGBE_LSECTXCTRL_EN_MASK; > >+ ctrl |=3D IXGBE_LSECTXCTRL_DISABLE; > >+ IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl); > >+ > >+ ctrl =3D IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL); > >+ ctrl &=3D ~IXGBE_LSECRXCTRL_EN_MASK; > >+ ctrl |=3D 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); > >+} > >+ > >+ > >+ >=20 > One empty line is enough here. had been removed >=20 > > 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 */ >=20 > Comment is unaligned and unnecessary. had been removed >=20 > >+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_d= ev > *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) { >=20 > 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 sti= ll > need to configure register (call ixgbe_dev_macsec_ctrl_register_enable) h= ere > other than just caching the macsec setting. > This kind of usercase processing will be added in the patch of V3. =20 > >- 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 =3D &rte_eth_devices[port]; > > > >- if (!is_ixgbe_supported(dev)) > >- return -ENOTSUP; > >- > >- hw =3D IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > >+ macsec_setting.encrypt_en =3D en; > >+ macsec_setting.replayprotect_en =3D rp; > > > >- /* Stop the data paths */ > >- if (ixgbe_disable_sec_rx_path(hw) !=3D 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 =3D IXGBE_READ_REG(hw, IXGBE_HLREG0); > >- ctrl |=3D IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP; > >- IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl); > >- > >- /* Enable the TX and RX crypto engines */ > >- ctrl =3D IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); > >- ctrl &=3D ~IXGBE_SECTXCTRL_SECTX_DIS; > >- IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl); > >- > >- ctrl =3D IXGBE_READ_REG(hw, IXGBE_SECRXCTRL); > >- ctrl &=3D ~IXGBE_SECRXCTRL_SECRX_DIS; > >- IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl); > >- > >- ctrl =3D IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG); > >- ctrl &=3D ~IXGBE_SECTX_MINSECIFG_MASK; > >- ctrl |=3D 0x3; > >- IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl); > >- > >- /* Enable SA lookup */ > >- ctrl =3D IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL); > >- ctrl &=3D ~IXGBE_LSECTXCTRL_EN_MASK; > >- ctrl |=3D en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT : > >- IXGBE_LSECTXCTRL_AUTH; > >- ctrl |=3D IXGBE_LSECTXCTRL_AISCI; > >- ctrl &=3D ~IXGBE_LSECTXCTRL_PNTHRSH_MASK; > >- ctrl |=3D IXGBE_MACSEC_PNTHRSH & > IXGBE_LSECTXCTRL_PNTHRSH_MASK; > >- IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl); > >- > >- ctrl =3D IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL); > >- ctrl &=3D ~IXGBE_LSECRXCTRL_EN_MASK; > >- ctrl |=3D IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT; > >- ctrl &=3D ~IXGBE_LSECRXCTRL_PLSH; > >- if (rp) > >- ctrl |=3D IXGBE_LSECRXCTRL_RP; > >- else > >- ctrl &=3D ~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) { >=20 > Ditto. > This kind of usercase processing will be added in the patch of V3. =20 > Thanks, > Xiaolong