Date:   Thu, 7 Dec 2017 08:02:39 -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 02/10] ixgbe: add ipsec register
 access routines

On Wed, Dec 6, 2017 at 9:43 PM, Shannon Nelson
<shannon.nelson@...cle.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.nelson@...cle.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.nelson@...cle.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.nics@...el.com>
>>> + * e1000-devel Mailing List <e1000-devel@...ts.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.nics@...el.com>
>>> +  e1000-devel Mailing List <e1000-devel@...ts.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-lan@...osl.org
>>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Powered by blists - more mailing lists