DPDK patches and discussions
 help / color / mirror / Atom feed
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
Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix macsec setting
Date: Tue, 29 Oct 2019 11:24:18 +0800	[thread overview]
Message-ID: <20191029032418.GD12923@intel.com> (raw)
In-Reply-To: <20191025092140.37288-1-guinanx.sun@intel.com>

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

  reply	other threads:[~2019-10-29  3:27 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-25  9:21 Sun GuinanX
2019-10-29  3:24 ` Ye Xiaolong [this message]
2019-10-29 16:32 ` [dpdk-dev] [PATCH v2] " Sun GuinanX
2019-10-29  9:03   ` Ye Xiaolong
2019-10-30  3:19     ` Sun, GuinanX
2019-10-30 10:43   ` [dpdk-dev] [PATCH v3] " Sun GuinanX
2019-10-30 11:31     ` [dpdk-dev] [PATCH v4] " Sun GuinanX
2019-10-30  5:34       ` Ye Xiaolong
2019-10-30 14:27       ` [dpdk-dev] [PATCH v5] " Sun GuinanX
2019-10-31  2:12         ` Ye Xiaolong
2019-10-31 11:31         ` [dpdk-dev] [PATCH v6] " Sun GuinanX
2019-11-01  2:25           ` Ye Xiaolong
2020-05-18  2:56           ` Zhao1, Wei

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191029032418.GD12923@intel.com \
    --to=xiaolong.ye@intel.com \
    --cc=dev@dpdk.org \
    --cc=guinanx.sun@intel.com \
    --cc=qiming.yang@intel.com \
    --cc=stable@dpdk.org \
    --cc=wenzhuo.lu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).