On Wed, Dec 6, 2017 at 9:43 PM, Shannon Nelson
<shannon.nel...@oracle.com> 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
>> <shannon.nel...@oracle.com> 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 <shannon.nel...@oracle.com>
>>> ---
>>> 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 <linux.n...@intel.com>
>>> + * e1000-devel Mailing List <e1000-de...@lists.sourceforge.net>
>>> + * 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 <linux.n...@intel.com>
>>> + e1000-devel Mailing List <e1000-de...@lists.sourceforge.net>
>>> + 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-...@osuosl.org
>>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
