Re: [Intel-wired-lan] [next-queue 02/10] ixgbe: add ipsec register access routines

quoted

On Wed, Dec 6, 2017 at 9:43 PM, Shannon Nelson
[off-list ref] wrote:
Thanks, Alex, for your detailed comments, I do appreciate the time and
thought you put into them.

Responses below...

sln

On 12/5/2017 8:56 AM, Alexander Duyck wrote:
On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
[off-list ref] wrote:
Add a few routines to make access to the ipsec registers just a little
easier, and throw in the beginnings of an initialization.

Signed-off-by: Shannon Nelson <redacted>
---
  drivers/net/ethernet/intel/ixgbe/Makefile      |   1 +
  drivers/net/ethernet/intel/ixgbe/ixgbe.h       |   6 +
  drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 157
+++++++++++++++++++++++++
  drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h |  50 ++++++++
  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |   1 +
  5 files changed, 215 insertions(+)
  create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
  create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h

diff --git a/drivers/net/ethernet/intel/ixgbe/Makefile
b/drivers/net/ethernet/intel/ixgbe/Makefile
index 35e6fa6..8319465 100644
--- a/drivers/net/ethernet/intel/ixgbe/Makefile
+++ b/drivers/net/ethernet/intel/ixgbe/Makefile
@@ -42,3 +42,4 @@ ixgbe-$(CONFIG_IXGBE_DCB) +=  ixgbe_dcb.o
ixgbe_dcb_82598.o \
  ixgbe-$(CONFIG_IXGBE_HWMON) += ixgbe_sysfs.o
  ixgbe-$(CONFIG_DEBUG_FS) += ixgbe_debugfs.o
  ixgbe-$(CONFIG_FCOE:m=y) += ixgbe_fcoe.o
+ixgbe-$(CONFIG_XFRM_OFFLOAD) += ixgbe_ipsec.o
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index dd55787..1e11462 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -52,6 +52,7 @@
  #ifdef CONFIG_IXGBE_DCA
  #include <linux/dca.h>
  #endif
+#include "ixgbe_ipsec.h"

  #include <net/busy_poll.h>

@@ -1001,4 +1002,9 @@ void ixgbe_store_key(struct ixgbe_adapter
*adapter);
  void ixgbe_store_reta(struct ixgbe_adapter *adapter);
  s32 ixgbe_negotiate_fc(struct ixgbe_hw *hw, u32 adv_reg, u32 lp_reg,
                        u32 adv_sym, u32 adv_asm, u32 lp_sym, u32
lp_asm);
+#ifdef CONFIG_XFRM_OFFLOAD
+void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter);
+#else
+static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter
*adapter) { };
+#endif /* CONFIG_XFRM_OFFLOAD */
  #endif /* _IXGBE_H_ */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
new file mode 100644
index 0000000..14dd011
--- /dev/null
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
@@ -0,0 +1,157 @@

+/*******************************************************************************
+ *
+ * Intel 10 Gigabit PCI Express Linux driver
+ * Copyright(c) 2017 Oracle and/or its affiliates. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * The full GNU General Public License is included in this distribution
in
+ * the file called "COPYING".
+ *
+ * Contact Information:
+ * Linux NICS [off-list ref]
+ * e1000-devel Mailing List [off-list ref]
+ * Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR
97124-6497
+ *
+
******************************************************************************/
+
+#include "ixgbe.h"
+
+/**
+ * ixgbe_ipsec_set_tx_sa - set the Tx SA registers
+ * @hw: hw specific details
+ * @idx: register index to write
+ * @key: key byte array
+ * @salt: salt bytes
+ **/
+static void ixgbe_ipsec_set_tx_sa(struct ixgbe_hw *hw, u16 idx,
+                                 u32 key[], u32 salt)
+{
+       u32 reg;
+       int i;
+
+       for (i = 0; i < 4; i++)
+               IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(i),
cpu_to_be32(key[3-i]));
+       IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT, cpu_to_be32(salt));
+       IXGBE_WRITE_FLUSH(hw);
+
+       reg = IXGBE_READ_REG(hw, IXGBE_IPSTXIDX);
+       reg &= IXGBE_RXTXIDX_IPS_EN;
+       reg |= idx << 3 | IXGBE_RXTXIDX_IDX_WRITE;
+       IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, reg);
+       IXGBE_WRITE_FLUSH(hw);
+}
+

So there are a few things here to unpack.

The first is the carry-forward of the IPS bit. I'm not sure that is
the best way to go. Do we really expect to be updating SA values if
IPsec offload is not enabled?

In order to save on energy, we don't enable the engine until we have the
first SA successfully stored in the tables, so the enable bit will be off
for that one.

Also, the datasheet specifically says for the Rx table "Software should not
make changes in the Rx SA tables while changing the IPSEC_EN bit." I figured
I'd use the same method on both tables for consistency.

If so we may just want to carry a bit
flag somewhere in the ixgbe_hw struct indicating if Tx IPsec offload
is enabled and use that to determine the value for this bit.

Also we should probably replace "3" with a value indicating that it is
the SA index shift.

Sure, that would be good.

Also technically the WRITE_FLUSH isn't needed if you are doing a PCIe
read anyway to get IPSTXIDX.

That's from having to be very fastidious about these reads/writes/flushes
before the engine actually worked for me.  I could spend time taking them
out and testing each change again, but they aren't in a fast path, so I'm
really not worried about it.



+/**
+ * ixgbe_ipsec_set_rx_item - set an Rx table item
+ * @hw: hw specific details
+ * @idx: register index to write
+ * @tbl: table selector
+ *
+ * Trigger the device to store into a particular Rx table the
+ * data that has already been loaded into the input register
+ **/
+static void ixgbe_ipsec_set_rx_item(struct ixgbe_hw *hw, u16 idx, u32
tbl)
+{
+       u32 reg;
+
+       reg = IXGBE_READ_REG(hw, IXGBE_IPSRXIDX);
+       reg &= IXGBE_RXTXIDX_IPS_EN;
+       reg |= tbl | idx << 3 | IXGBE_RXTXIDX_IDX_WRITE;
+       IXGBE_WRITE_REG(hw, IXGBE_IPSRXIDX, reg);
+       IXGBE_WRITE_FLUSH(hw);
+}
+

The Rx version of this gets a bit trickier since the datasheet
actually indicates there are a few different types of tables that can
be indexed via this. Also why is the tbl value not being shifted? It
seems like it should be shifted by 1 to avoid overwriting the IPS_EN
bit. Really I would like to see the tbl value converted to an enum and
shifted by 1 in order to generate the table reference.

I would have done this, but we can't use an enum shifted bit because the
field values are 01, 10, and 11.  I used the direct 2, 4, and 6 values
rather than shifting by one, but I can reset them and shift by 1.
I didn't mean 1 << enum I was referring to enum << 1. Right now you
can be given a table value of 3 if somebody incorrectly used the
function and the side effect is that it overwrites the enable bit.

Here the "3" is a table index. It might be nice to call that out with
a name instead of using the magic number.

Yep



+/**
+ * ixgbe_ipsec_set_rx_sa - set up the register bits to save SA info
+ * @hw: hw specific details
+ * @idx: register index to write
+ * @spi: security parameter index
+ * @key: key byte array
+ * @salt: salt bytes
+ * @mode: rx decrypt control bits
+ * @ip_idx: index into IP table for related IP address
+ **/
+static void ixgbe_ipsec_set_rx_sa(struct ixgbe_hw *hw, u16 idx, __be32
spi,
+                                 u32 key[], u32 salt, u32 mode, u32
ip_idx)
+{
+       int i;
+
+       /* store the SPI (in bigendian) and IPidx */
+       IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI, spi);
+       IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPIDX, ip_idx);
+       IXGBE_WRITE_FLUSH(hw);
+
+       ixgbe_ipsec_set_rx_item(hw, idx, IXGBE_RXIDX_TBL_SPI);
+
+       /* store the key, salt, and mode */
+       for (i = 0; i < 4; i++)
+               IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(i),
cpu_to_be32(key[3-i]));
+       IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT, cpu_to_be32(salt));
+       IXGBE_WRITE_REG(hw, IXGBE_IPSRXMOD, mode);
+       IXGBE_WRITE_FLUSH(hw);
+
+       ixgbe_ipsec_set_rx_item(hw, idx, IXGBE_RXIDX_TBL_KEY);
+}

Is there any reason why you could write the SPI, key, salt, and mode,
then flush, and trigger the writes via the IPSRXIDX? Just wondering
since it would likely save you a few cycles avoiding PCIe bus stalls.

See note above about religiously flushing everything to make a persnickety
chip work.
I get the flushing. What I am saying is that as far as I can tell the
SPI, salt, and mode don't overlap so you could update all 3, flush,
and then call set_rx_item twice.


+
+/**
+ * ixgbe_ipsec_set_rx_ip - set up the register bits to save SA IP addr
info
+ * @hw: hw specific details
+ * @idx: register index to write
+ * @addr: IP address byte array
+ **/
+static void ixgbe_ipsec_set_rx_ip(struct ixgbe_hw *hw, u16 idx, u32
addr[])
+{
+       int i;
+
+       /* store the ip address */
+       for (i = 0; i < 4; i++)
+               IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(i), addr[i]);
+       IXGBE_WRITE_FLUSH(hw);
+
+       ixgbe_ipsec_set_rx_item(hw, idx, IXGBE_RXIDX_TBL_IP);
+}
+

This piece is kind of confusing. I would suggest storing the address
as a __be32 pointer instead of a u32 array. That way you start with
either an IPv6 or an IPv4 address at offset 0 instead of the way the
hardware is defined which has you writing it at either 0 or 3
depending on if the address is IPv6 or IPv4.

Using a __be32 rather than u32 is fine here, it doesn't make much
difference.

If I understand your suggestion correctly, we would also need an additional
function parameter to tell us if we were pointing to an ipv6 or ipv4
address.  Since the driver's SW tables are modeling the HW, I think it is
simpler to leave it in the array.
Actually I am not too concerned about needing a flag, but the __be32
usage addresses another problem. If I am not mistaken in order to
store an IPv6 value you will have to write addr[3] to IPADDR(0) and so
forth since the hardware is storing the IPv6 address as little endian.
So if you store the IPv4 address in addr[0] as a __be32 value and
leave the rest as zero you should get the correct ordering in either
setup when you store either IPv6 or IPv4 values.



+/**
+ * ixgbe_ipsec_clear_hw_tables - because some tables don't get cleared
on reset
+ * @adapter: board private structure
+ **/
+void ixgbe_ipsec_clear_hw_tables(struct ixgbe_adapter *adapter)
+{
+       struct ixgbe_hw *hw = &adapter->hw;
+       u32 buf[4] = {0, 0, 0, 0};
+       u16 idx;
+
+       /* disable Rx and Tx SA lookup */
+       IXGBE_WRITE_REG(hw, IXGBE_IPSRXIDX, 0);
+       IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, 0);
+
+       /* scrub the tables */
+       for (idx = 0; idx < IXGBE_IPSEC_MAX_SA_COUNT; idx++)
+               ixgbe_ipsec_set_tx_sa(hw, idx, buf, 0);
+
+       for (idx = 0; idx < IXGBE_IPSEC_MAX_SA_COUNT; idx++)
+               ixgbe_ipsec_set_rx_sa(hw, idx, 0, buf, 0, 0, 0);
+
+       for (idx = 0; idx < IXGBE_IPSEC_MAX_RX_IP_COUNT; idx++)
+               ixgbe_ipsec_set_rx_ip(hw, idx, buf);
+}
+
+/**
+ * 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);
+}
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
new file mode 100644
index 0000000..017b13f
--- /dev/null
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
@@ -0,0 +1,50 @@

+/*******************************************************************************
+
+  Intel 10 Gigabit PCI Express Linux driver
+  Copyright(c) 2017 Oracle and/or its affiliates. All rights reserved.
+
+  This program is free software; you can redistribute it and/or modify
it
+  under the terms and conditions of the GNU General Public License,
+  version 2, as published by the Free Software Foundation.
+
+  This program is distributed in the hope it will be useful, but WITHOUT
+  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
for
+  more details.
+
+  You should have received a copy of the GNU General Public License
along with
+  this program.  If not, see <http://www.gnu.org/licenses/>.
+
+  The full GNU General Public License is included in this distribution
in
+  the file called "COPYING".
+
+  Contact Information:
+  Linux NICS [off-list ref]
+  e1000-devel Mailing List [off-list ref]
+  Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR
97124-6497
+

+*******************************************************************************/
+
+#ifndef _IXGBE_IPSEC_H_
+#define _IXGBE_IPSEC_H_
+
+#define IXGBE_IPSEC_MAX_SA_COUNT       1024
+#define IXGBE_IPSEC_MAX_RX_IP_COUNT    128
+#define IXGBE_IPSEC_BASE_RX_INDEX      IXGBE_IPSEC_MAX_SA_COUNT
+#define IXGBE_IPSEC_BASE_TX_INDEX      (IXGBE_IPSEC_MAX_SA_COUNT * 2)
+
+#define IXGBE_RXTXIDX_IPS_EN           0x00000001
+#define IXGBE_RXIDX_TBL_MASK           0x00000006
+#define IXGBE_RXIDX_TBL_IP             0x00000002
+#define IXGBE_RXIDX_TBL_SPI            0x00000004
+#define IXGBE_RXIDX_TBL_KEY            0x00000006

You might look at converting these table entries into an enum and add
a shift value. It will make things much easier to read.

+#define IXGBE_RXTXIDX_IDX_MASK         0x00001ff8
+#define IXGBE_RXTXIDX_IDX_READ         0x40000000
+#define IXGBE_RXTXIDX_IDX_WRITE                0x80000000
+
+#define IXGBE_RXMOD_VALID              0x00000001
+#define IXGBE_RXMOD_PROTO_ESP          0x00000004
+#define IXGBE_RXMOD_DECRYPT            0x00000008
+#define IXGBE_RXMOD_IPV6               0x00000010
+
+#endif /* _IXGBE_IPSEC_H_ */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 6d5f31e..51fb3cf 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -10327,6 +10327,7 @@ static int ixgbe_probe(struct pci_dev *pdev,
const struct pci_device_id *ent)
                                          NETIF_F_FCOE_MTU;
         }
  #endif /* IXGBE_FCOE */
+       ixgbe_init_ipsec_offload(adapter);

         if (adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE)
                 netdev->hw_features |= NETIF_F_LRO;
--
2.7.4

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help