| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAKgT0UfJDtGBJEij3TBKAaS_QFukMSiG9TieA49kKXdPwLi9PA@mail.gmail.com>
Date: Tue, 5 Dec 2017 08:22:44 -0800
From: Alexander Duyck <alexander.duyck@...il.com>
To: Shannon Nelson <shannon.nelson@...cle.com>
Cc: intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
Steffen Klassert <steffen.klassert@...unet.com>,
Sowmini Varadhan <sowmini.varadhan@...cle.com>,
Netdev <netdev@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [next-queue 03/10] ixgbe: add ipsec engine
start and stop routines
On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
<shannon.nelson@...cle.com> wrote:
> Add in the code for running and stopping the hardware ipsec
> encryption/decryption engine. It is good to keep the engine
> off when not in use in order to save on the power draw.
>
> Signed-off-by: Shannon Nelson <shannon.nelson@...cle.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 140 +++++++++++++++++++++++++
> 1 file changed, 140 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> index 14dd011..38a1a16 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> @@ -148,10 +148,150 @@ void ixgbe_ipsec_clear_hw_tables(struct ixgbe_adapter *adapter)
> }
>
> /**
> + * ixgbe_ipsec_stop_data
> + * @adapter: board private structure
> + **/
> +static void ixgbe_ipsec_stop_data(struct ixgbe_adapter *adapter)
> +{
> + struct ixgbe_hw *hw = &adapter->hw;
> + bool link = adapter->link_up;
> + u32 t_rdy, r_rdy;
> + u32 reg;
> +
> + /* halt data paths */
> + reg = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
> + reg |= IXGBE_SECTXCTRL_TX_DIS;
> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg);
> +
> + reg = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
> + reg |= IXGBE_SECRXCTRL_RX_DIS;
> + IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, reg);
> +
> + IXGBE_WRITE_FLUSH(hw);
> +
> + /* If the tx fifo doesn't have link, but still has data,
> + * we can't clear the tx sec block. Set the MAC loopback
> + * before block clear
> + */
> + if (!link) {
> + reg = IXGBE_READ_REG(hw, IXGBE_MACC);
> + reg |= IXGBE_MACC_FLU;
> + IXGBE_WRITE_REG(hw, IXGBE_MACC, reg);
> +
> + reg = IXGBE_READ_REG(hw, IXGBE_HLREG0);
> + reg |= IXGBE_HLREG0_LPBK;
> + IXGBE_WRITE_REG(hw, IXGBE_HLREG0, reg);
> +
> + IXGBE_WRITE_FLUSH(hw);
> + mdelay(3);
> + }
> +
> + /* wait for the paths to empty */
> + do {
> + mdelay(10);
> + t_rdy = IXGBE_READ_REG(hw, IXGBE_SECTXSTAT) &
> + IXGBE_SECTXSTAT_SECTX_RDY;
> + r_rdy = IXGBE_READ_REG(hw, IXGBE_SECRXSTAT) &
> + IXGBE_SECRXSTAT_SECRX_RDY;
> + } while (!t_rdy && !r_rdy);
This piece seems buggy to me. There should be some sort of limit on
how long you are willing to delay. Otherwise a surprise remove can
cause this to spin forever when the register reads return all 1's.
> +
> + /* undo loopback if we played with it earlier */
> + if (!link) {
> + reg = IXGBE_READ_REG(hw, IXGBE_MACC);
> + reg &= ~IXGBE_MACC_FLU;
> + IXGBE_WRITE_REG(hw, IXGBE_MACC, reg);
> +
> + reg = IXGBE_READ_REG(hw, IXGBE_HLREG0);
> + reg &= ~IXGBE_HLREG0_LPBK;
> + IXGBE_WRITE_REG(hw, IXGBE_HLREG0, reg);
> +
> + IXGBE_WRITE_FLUSH(hw);
> + }
> +}
> +
> +/**
> + * ixgbe_ipsec_stop_engine
> + * @adapter: board private structure
> + **/
> +static void ixgbe_ipsec_stop_engine(struct ixgbe_adapter *adapter)
> +{
> + struct ixgbe_hw *hw = &adapter->hw;
> + u32 reg;
> +
> + ixgbe_ipsec_stop_data(adapter);
> +
> + /* disable Rx and Tx SA lookup */
> + IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, 0);
> + IXGBE_WRITE_REG(hw, IXGBE_IPSRXIDX, 0);
> +
> + /* disable the Rx and Tx engines and full packet store-n-forward */
> + reg = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
> + reg |= IXGBE_SECTXCTRL_SECTX_DIS;
> + reg &= ~IXGBE_SECTXCTRL_STORE_FORWARD;
> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg);
> +
> + reg = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
> + reg |= IXGBE_SECRXCTRL_SECRX_DIS;
> + IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, reg);
> +
> + /* restore the "tx security buffer almost full threshold" to 0x250 */
> + IXGBE_WRITE_REG(hw, IXGBE_SECTXBUFFAF, 0x250);
> +
> + /* Set minimum IFG between packets back to the default 0x1 */
> + reg = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
> + reg = (reg & 0xfffffff0) | 0x1;
> + IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, reg);
> +
> + /* final set for normal (no ipsec offload) processing */
> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, IXGBE_SECTXCTRL_SECTX_DIS);
> + IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, IXGBE_SECRXCTRL_SECRX_DIS);
> +
> + IXGBE_WRITE_FLUSH(hw);
> +}
> +
> +/**
> + * ixgbe_ipsec_start_engine
> + * @adapter: board private structure
> + *
> + * NOTE: this increases power consumption whether being used or not
> + **/
> +static void ixgbe_ipsec_start_engine(struct ixgbe_adapter *adapter)
> +{
> + struct ixgbe_hw *hw = &adapter->hw;
> + u32 reg;
> +
> + ixgbe_ipsec_stop_data(adapter);
> +
> + /* Set minimum IFG between packets to 3 */
> + reg = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
> + reg = (reg & 0xfffffff0) | 0x3;
> + IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, reg);
> +
> + /* Set "tx security buffer almost full threshold" to 0x15 so that the
> + * almost full indication is generated only after buffer contains at
> + * least an entire jumbo packet.
> + */
> + reg = IXGBE_READ_REG(hw, IXGBE_SECTXBUFFAF);
> + reg = (reg & 0xfffffc00) | 0x15;
> + IXGBE_WRITE_REG(hw, IXGBE_SECTXBUFFAF, reg);
> +
> + /* restart the data paths by clearing the DISABLE bits */
> + IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, 0);
> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, IXGBE_SECTXCTRL_STORE_FORWARD);
> +
> + /* enable Rx and Tx SA lookup */
> + IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, IXGBE_RXTXIDX_IPS_EN);
> + IXGBE_WRITE_REG(hw, IXGBE_IPSRXIDX, IXGBE_RXTXIDX_IPS_EN);
> +
> + IXGBE_WRITE_FLUSH(hw);
> +}
> +
It would probably make sense to add a data member to the hardware
structure that tracks if you have IPsec enabled or not. Then you don't
have to track the IPS_EN bits in patch 2 like you currently are and
could instead either not do IPsec SA updates if IPsec is not enabled,
or use the enable value to determine what you write for IPS_EN instead
of having to read registers.
> +/**
> * ixgbe_init_ipsec_offload - initialize security registers for IPSec operation
> * @adapter: board private structure
> **/
> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter)
> {
> ixgbe_ipsec_clear_hw_tables(adapter);
> + ixgbe_ipsec_stop_engine(adapter);
> }
> --
> 2.7.4
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@...osl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Powered by blists - more mailing lists