From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <stable-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id B6D3FA00BE
for <public@inbox.dpdk.org>; Tue, 29 Oct 2019 04:27:53 +0100 (CET)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id 838161BEE7;
Tue, 29 Oct 2019 04:27:53 +0100 (CET)
Received: from mga02.intel.com (mga02.intel.com [134.134.136.20])
by dpdk.org (Postfix) with ESMTP id 7AD6C1BEC3;
Tue, 29 Oct 2019 04:27:50 +0100 (CET)
X-Amp-Result: UNKNOWN
X-Amp-Original-Verdict: FILE UNKNOWN
X-Amp-File-Uploaded: False
Received: from fmsmga003.fm.intel.com ([10.253.24.29])
by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
28 Oct 2019 20:27:49 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.68,242,1569308400"; d="scan'208";a="205352984"
Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.117.17])
by FMSMGA003.fm.intel.com with ESMTP; 28 Oct 2019 20:27:47 -0700
Date: Tue, 29 Oct 2019 11:24:18 +0800
From: Ye Xiaolong <xiaolong.ye@intel.com>
To: Sun GuinanX <guinanx.sun@intel.com>
Cc: dev@dpdk.org, Wenzhuo Lu <wenzhuo.lu@intel.com>,
Qiming Yang <qiming.yang@intel.com>, stable@dpdk.org
Message-ID: <20191029032418.GD12923@intel.com>
References: <20191025092140.37288-1-guinanx.sun@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191025092140.37288-1-guinanx.sun@intel.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] net/ixgbe: fix macsec setting
X-BeenThere: stable@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches for DPDK stable branches <stable.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/stable>,
<mailto:stable-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/stable/>
List-Post: <mailto:stable@dpdk.org>
List-Help: <mailto:stable-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/stable>,
<mailto:stable-request@dpdk.org?subject=subscribe>
Errors-To: stable-bounces@dpdk.org
Sender: "stable" <stable-bounces@dpdk.org>
Hi, Guinan
Overall the fix looks good to me, a few comments inline.
On 10/25, 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 <guinanx.sun@intel.com>
>---
> drivers/net/ixgbe/ixgbe_ethdev.c | 160 ++++++++++++++++++++++++++++++
> drivers/net/ixgbe/ixgbe_ethdev.h | 15 +++
> drivers/net/ixgbe/rte_pmd_ixgbe.c | 9 ++
> 3 files changed, 184 insertions(+)
>
>diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>index dbce7a80e..29455f7ee 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_ctrl *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_ctrl *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 */
>+void
>+ixgbe_dev_macsec_ctrl_setup_enable(struct rte_eth_dev *dev,
>+ struct ixgbe_macsec_ctrl *macsec_ctrl)
what about ixgbe_dev_macsec_setting_save?
>+{
>+ struct ixgbe_macsec_ctrl *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 */
>+void
>+ixgbe_dev_macsec_ctrl_setup_disable(struct rte_eth_dev *dev)
what about ixgbe_dev_macsec_setting_reset?
>+{
>+ struct ixgbe_macsec_ctrl *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_ctrl *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);
>+}
Above functions share a lot of code with rte_pmd_ixgbe_macsec_enable/disable,
try to refactor to reduce the duplication.
>+
>+
>+
> 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..e6cff8b3a 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 */
>+struct ixgbe_macsec_ctrl {
what about ixgbe_macsec_setting, it seems more like setting of macsec other than
the control structure.
>+ bool encrypt_en; /* encrypt enabled */
>+ bool replayprotect_en; /* replayprotect enabled */
what about using uint8_t to avoid further cast?
Thanks,
Xiaolong