* [PATCH net-next v2 0/5] Introduce NETIF_F_GRO_HW
From: Michael Chan @ 2017-12-07 8:03 UTC (permalink / raw)
To: davem; +Cc: netdev, andrew.gospodarek
Introduce NETIF_F_GRO_HW feature flag and convert drivers that support
hardware GRO to use the new flag.
v2:
- NETIF_F_GRO_HW flag propagation between upper and lower devices not
required (see patch 1).
- NETIF_F_GRO_HW depends on NETIF_F_GRO and NETIF_F_RXCSUM.
- Add dev_disable_gro_hw() to disable GRO_HW for generic XDP.
- Use ndo_fix_features() on all 3 drivers to drop GRO_HW when it is not
supported
Michael Chan (5):
net: Introduce NETIF_F_GRO_HW.
net: Disable GRO_HW when generic XDP is installed on a device.
bnxt_en: Use NETIF_F_GRO_HW.
bnx2x: Use NETIF_F_GRO_HW.
qede: Use NETIF_F_GRO_HW.
Documentation/networking/netdev-features.txt | 8 +++++
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 17 ++++++----
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 4 ++-
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 21 ++++++++----
drivers/net/ethernet/qlogic/qede/qede.h | 2 ++
drivers/net/ethernet/qlogic/qede/qede_ethtool.c | 3 ++
drivers/net/ethernet/qlogic/qede/qede_filter.c | 20 ++++++++----
drivers/net/ethernet/qlogic/qede/qede_main.c | 17 ++++------
include/linux/netdev_features.h | 3 ++
net/core/dev.c | 41 ++++++++++++++++++++++++
net/core/ethtool.c | 1 +
11 files changed, 104 insertions(+), 33 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: achiad shochat @ 2017-12-07 7:28 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexander Duyck, Stephen Hemminger, Jakub Kicinski,
Hannes Frederic Sowa, Sridhar Samudrala, netdev, virtualization,
Achiad, Peter Waskiewicz Jr, Singhai, Anjali, Shannon Nelson,
Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171205211603-mutt-send-email-mst@kernel.org>
On 5 December 2017 at 21:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Dec 05, 2017 at 11:59:17AM +0200, achiad shochat wrote:
>> Then we'll have a single solution for both netvsc and virtio (and any
>> other PV device).
>> And we could handle the VF DMA dirt issue agnostically.
>
> For the record, I won't block patches adding this kist to virtio
> on the basis that they must be generic. It's not a lot
> of code, implementation can come first, prettify later.
It's not a lot of code either way.
So I fail to understand why not to do it right from the beginning.
For the record...
>
> But we do need to have a discussion about how devices are paired.
> I am not sure using just MAC works. E.g. some passthrough
> devices don't give host ability to set the MAC.
> Are these worth worrying about?
>
> --
> MST
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: Disable interrupts if napi_complete_done rescheduled napi
From: Jason Wang @ 2017-12-07 7:08 UTC (permalink / raw)
To: Michael S. Tsirkin, Toshiaki Makita
Cc: David S . Miller, netdev, virtualization, Eric Dumazet
In-Reply-To: <20171207070618-mutt-send-email-mst@kernel.org>
On 2017年12月07日 13:09, Michael S. Tsirkin wrote:
> On Thu, Dec 07, 2017 at 01:15:15PM +0900, Toshiaki Makita wrote:
>> Since commit 39e6c8208d7b ("net: solve a NAPI race") napi has been able
>> to be rescheduled within napi_complete_done() even in non-busypoll case,
>> but virtnet_poll() always enabled interrupts before complete, and when
>> napi was rescheduled within napi_complete_done() it did not disable
>> interrupts.
>> This caused more interrupts when event idx is disabled.
>>
>> According to commit cbdadbbf0c79 ("virtio_net: fix race in RX VQ
>> processing") we cannot place virtqueue_enable_cb_prepare() after
>> NAPI_STATE_SCHED is cleared, so disable interrupts again if
>> napi_complete_done() returned false.
>>
>> Tested with vhost-user of OVS 2.7 on host, which does not have the event
>> idx feature.
>>
>> * Before patch:
>>
>> $ netperf -t UDP_STREAM -H 192.168.150.253 -l 60 -- -m 1472
>> MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.150.253 () port 0 AF_INET
>> Socket Message Elapsed Messages
>> Size Size Time Okay Errors Throughput
>> bytes bytes secs # # 10^6bits/sec
>>
>> 212992 1472 60.00 32763206 0 6430.32
>> 212992 60.00 23384299 4589.56
>>
>> Interrupts on guest: 9872369
>> Packets/interrupt: 2.37
>>
>> * After patch
>>
>> $ netperf -t UDP_STREAM -H 192.168.150.253 -l 60 -- -m 1472
>> MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.150.253 () port 0 AF_INET
>> Socket Message Elapsed Messages
>> Size Size Time Okay Errors Throughput
>> bytes bytes secs # # 10^6bits/sec
>>
>> 212992 1472 60.00 32794646 0 6436.49
>> 212992 60.00 32793501 6436.27
>>
>> Interrupts on guest: 4941299
>> Packets/interrupt: 6.64
>>
>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> it might make sense in net and not -next since tx napi regressed performance
> in some configs, this might bring it back at least partially.
> Jason - what do you think?
No sure, the regression I saw was tested with event idx on. And
virtqueue_disable_cb() does almost nothing for event idx (or even a
little bit slower).
The patch looks good.
Acked-by: Jason Wang <jasowang@redhat.com>
>
>> ---
>> drivers/net/virtio_net.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 19a985e..c0db48d 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -261,9 +261,12 @@ static void virtqueue_napi_complete(struct napi_struct *napi,
>> int opaque;
>>
>> opaque = virtqueue_enable_cb_prepare(vq);
>> - if (napi_complete_done(napi, processed) &&
>> - unlikely(virtqueue_poll(vq, opaque)))
>> - virtqueue_napi_schedule(napi, vq);
>> + if (napi_complete_done(napi, processed)) {
>> + if (unlikely(virtqueue_poll(vq, opaque)))
>> + virtqueue_napi_schedule(napi, vq);
>> + } else {
>> + virtqueue_disable_cb(vq);
>> + }
>> }
>>
>> static void skb_xmit_done(struct virtqueue *vq)
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* [PATCH net-next 3/3] net: dsa: mediatek: update MAINTAINERS entry with MediaTek switch driver
From: sean.wang-NuS5LvNUpcJWk0Htik3J/w @ 2017-12-07 6:06 UTC (permalink / raw)
To: davem-fT/PcQaiUtIeIZ0/mPfg9Q, andrew-g2DYL2Zd6BY,
f.fainelli-Re5JQEeQqe8AvxtiuMwx3w,
vivien.didelot-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, Sean Wang,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1512625814.git.sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
I work for MediaTek and maintain SoC targeting to home gateway and
also will keep extending and testing the function from MediaTek
switch.
Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index c0edf30..070fd91 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8723,6 +8723,13 @@ L: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
S: Maintained
F: drivers/net/ethernet/mediatek/
+MEDIATEK SWITCH DRIVER
+M: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
+L: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+S: Maintained
+F: drivers/net/dsa/mt7530.*
+F: net/dsa/tag_mtk.c
+
MEDIATEK JPEG DRIVER
M: Rick Chang <rick.chang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
M: Bin Liu <bin.liu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 2/3] net: dsa: mediatek: combine MediaTek tag with VLAN tag
From: sean.wang @ 2017-12-07 6:06 UTC (permalink / raw)
To: davem, andrew, f.fainelli, vivien.didelot
Cc: netdev, linux-kernel, linux-mediatek, Sean Wang
In-Reply-To: <cover.1512625814.git.sean.wang@mediatek.com>
From: Sean Wang <sean.wang@mediatek.com>
In order to let MT7530 switch can recognize well those packets
having both special tag and VLAN tag, the information about
the special tag should be carried on the existing VLAN tag.
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
net/dsa/tag_mtk.c | 38 +++++++++++++++++++++++++++++---------
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index 8475434..11535bc 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -13,10 +13,13 @@
*/
#include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
#include "dsa_priv.h"
#define MTK_HDR_LEN 4
+#define MTK_HDR_XMIT_UNTAGGED 0
+#define MTK_HDR_XMIT_TAGGED_TPID_8100 1
#define MTK_HDR_RECV_SOURCE_PORT_MASK GENMASK(2, 0)
#define MTK_HDR_XMIT_DP_BIT_MASK GENMASK(5, 0)
@@ -25,20 +28,37 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
{
struct dsa_port *dp = dsa_slave_to_port(dev);
u8 *mtk_tag;
+ bool is_vlan_skb = true;
- if (skb_cow_head(skb, MTK_HDR_LEN) < 0)
- return NULL;
-
- skb_push(skb, MTK_HDR_LEN);
+ /* Build the special tag after the MAC Source Address. If VLAN header
+ * is present, it's required that VLAN header and special tag is
+ * being combined. Only in this way we can allow the switch can parse
+ * the both special and VLAN tag at the same time and then look up VLAN
+ * table with VID.
+ */
+ if (!skb_vlan_tagged(skb)) {
+ if (skb_cow_head(skb, MTK_HDR_LEN) < 0)
+ return NULL;
- memmove(skb->data, skb->data + MTK_HDR_LEN, 2 * ETH_ALEN);
+ skb_push(skb, MTK_HDR_LEN);
+ memmove(skb->data, skb->data + MTK_HDR_LEN, 2 * ETH_ALEN);
+ is_vlan_skb = false;
+ }
- /* Build the tag after the MAC Source Address */
mtk_tag = skb->data + 2 * ETH_ALEN;
- mtk_tag[0] = 0;
+
+ /* Mark tag attribute on special tag insertion to notify hardware
+ * whether that's a combined special tag with 802.1Q header.
+ */
+ mtk_tag[0] = is_vlan_skb ? MTK_HDR_XMIT_TAGGED_TPID_8100 :
+ MTK_HDR_XMIT_UNTAGGED;
mtk_tag[1] = (1 << dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;
- mtk_tag[2] = 0;
- mtk_tag[3] = 0;
+
+ /* Tag control information is kept for 802.1Q */
+ if (!is_vlan_skb) {
+ mtk_tag[2] = 0;
+ mtk_tag[3] = 0;
+ }
return skb;
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530
From: sean.wang @ 2017-12-07 6:06 UTC (permalink / raw)
To: davem, andrew, f.fainelli, vivien.didelot
Cc: netdev, linux-kernel, linux-mediatek, Sean Wang
In-Reply-To: <cover.1512625814.git.sean.wang@mediatek.com>
From: Sean Wang <sean.wang@mediatek.com>
MT7530 can treat each port as either VLAN-unware port or VLAN-ware port
through the implementation of port matrix mode or port security mode on
the ingress port, respectively. On one hand, Each port has been acting as
the VLAN-unware one whenever the device is created in the initial or
certain port joins or leaves into/from the bridge at the runtime. On the
other hand, the patch just filling the required callbacks for VLAN
operations is achieved via extending the port to be into port security
mode when the port is configured as VLAN-ware port. Which mode can make
the port be able to recognize VID from incoming packets and look up VLAN
table to validate and judge which port it should be going to. And the
range for VID from 1 to 4094 is valid for the hardware.
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
drivers/net/dsa/mt7530.c | 292 ++++++++++++++++++++++++++++++++++++++++++++++-
drivers/net/dsa/mt7530.h | 83 +++++++++++++-
2 files changed, 368 insertions(+), 7 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 2820d69..a7c5370 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -805,6 +805,69 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
}
static void
+mt7530_port_set_vlan_unware(struct dsa_switch *ds, int port)
+{
+ struct mt7530_priv *priv = ds->priv;
+ int i;
+ bool all_user_ports_removed = true;
+
+ /* When a port is removed from the bridge, the port would be set up
+ * back to the default as is at initial boot which is a VLAN-unware
+ * port.
+ */
+ mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK,
+ MT7530_PORT_MATRIX_MODE);
+ mt7530_rmw(priv, MT7530_PVC_P(port), VLAN_ATTR_MASK,
+ VLAN_ATTR(MT7530_VLAN_TRANSPARENT));
+
+ priv->ports[port].vlan_filtering = false;
+
+ for (i = 0; i < MT7530_NUM_PORTS; i++) {
+ if (dsa_is_user_port(ds, i) &&
+ priv->ports[i].vlan_filtering) {
+ all_user_ports_removed = false;
+ break;
+ }
+ }
+
+ /* CPU port also does the same thing until all user ports belonging to
+ * the CPU port get out of VLAN filtering mode.
+ */
+ if (all_user_ports_removed) {
+ mt7530_write(priv, MT7530_PCR_P(MT7530_CPU_PORT),
+ PCR_MATRIX(dsa_user_ports(priv->ds)));
+ mt7530_write(priv, MT7530_PVC_P(MT7530_CPU_PORT),
+ PORT_SPEC_TAG);
+ }
+}
+
+static void
+mt7530_port_set_vlan_ware(struct dsa_switch *ds, int port)
+{
+ struct mt7530_priv *priv = ds->priv;
+
+ /* The real fabric path would be decided on the membership in the
+ * entry of VLAN table. PCR_MATRIX set up here with ALL_MEMBERS
+ * means potential VLAN can be consisting of certain subset of all
+ * ports.
+ */
+ mt7530_rmw(priv, MT7530_PCR_P(port),
+ PCR_MATRIX_MASK, PCR_MATRIX(MT7530_ALL_MEMBERS));
+
+ /* Trapped into security mode allows packet forwarding through VLAN
+ * table lookup.
+ */
+ mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK,
+ MT7530_PORT_SECURITY_MODE);
+
+ /* Set the port as a user port which is to be able to recognize VID
+ * from incoming packets before fetching entry within the VLAN table.
+ */
+ mt7530_rmw(priv, MT7530_PVC_P(port), VLAN_ATTR_MASK,
+ VLAN_ATTR(MT7530_VLAN_USER));
+}
+
+static void
mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
struct net_device *bridge)
{
@@ -817,8 +880,11 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
/* Remove this port from the port matrix of the other ports
* in the same bridge. If the port is disabled, port matrix
* is kept and not being setup until the port becomes enabled.
+ * And the other port's port matrix cannot be broken when the
+ * other port is still a VLAN-ware port.
*/
- if (dsa_is_user_port(ds, i) && i != port) {
+ if (!priv->ports[i].vlan_filtering &&
+ dsa_is_user_port(ds, i) && i != port) {
if (dsa_to_port(ds, i)->bridge_dev != bridge)
continue;
if (priv->ports[i].enable)
@@ -836,6 +902,8 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
PCR_MATRIX(BIT(MT7530_CPU_PORT)));
priv->ports[port].pm = PCR_MATRIX(BIT(MT7530_CPU_PORT));
+ mt7530_port_set_vlan_unware(ds, port);
+
mutex_unlock(&priv->reg_mutex);
}
@@ -906,6 +974,224 @@ mt7530_port_fdb_dump(struct dsa_switch *ds, int port,
return 0;
}
+static int
+mt7530_vlan_cmd(struct mt7530_priv *priv, enum mt7530_vlan_cmd cmd, u16 vid)
+{
+ u32 val;
+ int ret;
+ struct mt7530_dummy_poll p;
+
+ val = VTCR_BUSY | VTCR_FUNC(cmd) | vid;
+ mt7530_write(priv, MT7530_VTCR, val);
+
+ INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_VTCR);
+ ret = readx_poll_timeout(_mt7530_read, &p, val,
+ !(val & VTCR_BUSY), 20, 20000);
+ if (ret < 0) {
+ dev_err(priv->dev, "poll timeout\n");
+ return ret;
+ }
+
+ val = mt7530_read(priv, MT7530_VTCR);
+ if (val & VTCR_INVALID) {
+ dev_err(priv->dev, "read VTCR invalid\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int
+mt7530_port_vlan_filtering(struct dsa_switch *ds, int port,
+ bool vlan_filtering)
+{
+ struct mt7530_priv *priv = ds->priv;
+
+ priv->ports[port].vlan_filtering = vlan_filtering;
+
+ return 0;
+}
+
+static int
+mt7530_port_vlan_prepare(struct dsa_switch *ds, int port,
+ const struct switchdev_obj_port_vlan *vlan,
+ struct switchdev_trans *trans)
+{
+ struct mt7530_priv *priv = ds->priv;
+
+ /* The port is being kept as VLAN-unware port when bridge is set up
+ * with vlan_filtering not being set, Otherwise, the port and the
+ * corresponding CPU port is required the setup for becoming a
+ * VLAN-ware port.
+ */
+ if (!priv->ports[port].vlan_filtering)
+ return 0;
+
+ mt7530_port_set_vlan_ware(ds, port);
+ mt7530_port_set_vlan_ware(ds, MT7530_CPU_PORT);
+
+ return 0;
+}
+
+static void
+mt7530_hw_vlan_add(struct mt7530_priv *priv,
+ struct mt7530_hw_vlan_entry *entry)
+{
+ u32 val;
+ u8 new_members;
+
+ new_members = entry->old_members | BIT(entry->port) |
+ BIT(MT7530_CPU_PORT);
+
+ /* Validate the entry with independent learning, create egress tag per
+ * VLAN and joining the port as one of the port members.
+ */
+ val = IVL_MAC | VTAG_EN | PORT_MEM(new_members) | VLAN_VALID;
+ mt7530_write(priv, MT7530_VAWD1, val);
+
+ /* Decide whether adding tag or not for those outgoing packets from the
+ * port inside the VLAN.
+ */
+ val = entry->untagged ? MT7530_VLAN_EGRESS_UNTAG :
+ MT7530_VLAN_EGRESS_TAG;
+ mt7530_rmw(priv, MT7530_VAWD2,
+ ETAG_CTRL_P_MASK(entry->port),
+ ETAG_CTRL_P(entry->port, val));
+
+ /* CPU port is always taken as a tagged port for serving more than one
+ * VLANs across and also being applied with egress type stack mode for
+ * that VLAN tags would be appended after hardware special tag used as
+ * DSA tag.
+ */
+ mt7530_rmw(priv, MT7530_VAWD2,
+ ETAG_CTRL_P_MASK(MT7530_CPU_PORT),
+ ETAG_CTRL_P(MT7530_CPU_PORT,
+ MT7530_VLAN_EGRESS_STACK));
+}
+
+static void
+mt7530_hw_vlan_del(struct mt7530_priv *priv,
+ struct mt7530_hw_vlan_entry *entry)
+{
+ u32 val;
+ u8 new_members;
+
+ new_members = entry->old_members & ~BIT(entry->port);
+
+ val = mt7530_read(priv, MT7530_VAWD1);
+ if (!(val & VLAN_VALID)) {
+ dev_err(priv->dev,
+ "Cannot be deleted due to invalid entry\n");
+ return;
+ }
+
+ /* If certain member apart from CPU port is still alive in the VLAN,
+ * the entry would be kept valid. Otherwise, the entry is got to be
+ * disabled.
+ */
+ if (new_members && new_members != BIT(MT7530_CPU_PORT)) {
+ val = IVL_MAC | VTAG_EN | PORT_MEM(new_members) |
+ VLAN_VALID;
+ mt7530_write(priv, MT7530_VAWD1, val);
+ } else {
+ mt7530_write(priv, MT7530_VAWD1, 0);
+ mt7530_write(priv, MT7530_VAWD2, 0);
+ }
+}
+
+static void
+mt7530_hw_vlan_update(struct mt7530_priv *priv, u16 vid,
+ struct mt7530_hw_vlan_entry *entry,
+ mt7530_vlan_op vlan_op)
+{
+ u32 val;
+
+ /* Fetch entry */
+ mt7530_vlan_cmd(priv, MT7530_VTCR_RD_VID, vid);
+
+ val = mt7530_read(priv, MT7530_VAWD1);
+
+ entry->old_members = (val >> PORT_MEM_SHFT) & PORT_MEM_MASK;
+
+ /* Manipulate entry */
+ vlan_op(priv, entry);
+
+ /* Flush result to hardware */
+ mt7530_vlan_cmd(priv, MT7530_VTCR_WR_VID, vid);
+}
+
+static void
+mt7530_port_vlan_add(struct dsa_switch *ds, int port,
+ const struct switchdev_obj_port_vlan *vlan,
+ struct switchdev_trans *trans)
+{
+ struct mt7530_priv *priv = ds->priv;
+ struct mt7530_hw_vlan_entry new_entry;
+ u16 vid;
+ bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+ bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+ int ret;
+
+ /* The port is kept as VLAN-unware if bridge with vlan_filtering not
+ * being set.
+ */
+ if (!priv->ports[port].vlan_filtering)
+ return;
+
+ mutex_lock(&priv->reg_mutex);
+
+ for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid) {
+ INIT_MT7530_HW_ENTRY(&new_entry, port, untagged);
+ mt7530_hw_vlan_update(priv, vid, &new_entry,
+ mt7530_hw_vlan_add);
+ }
+
+ if (pvid) {
+ mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK,
+ G0_PORT_VID(vlan->vid_end));
+ priv->ports[port].pvid = vlan->vid_end;
+ }
+
+ mutex_unlock(&priv->reg_mutex);
+}
+
+static int
+mt7530_port_vlan_del(struct dsa_switch *ds, int port,
+ const struct switchdev_obj_port_vlan *vlan)
+{
+ struct mt7530_priv *priv = ds->priv;
+ struct mt7530_hw_vlan_entry target_entry;
+ u16 vid, pvid;
+
+ /* The port is kept as VLAN-unware if bridge with vlan_filtering not
+ * being set.
+ */
+ if (!priv->ports[port].vlan_filtering)
+ return 0;
+
+ mutex_lock(&priv->reg_mutex);
+
+ pvid = priv->ports[port].pvid;
+ for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid) {
+ INIT_MT7530_HW_ENTRY(&target_entry, port, 0);
+ mt7530_hw_vlan_update(priv, vid, &target_entry,
+ mt7530_hw_vlan_del);
+
+ /* PVID is being restored to the default whenever the PVID port
+ * is being removed from the VLAN.
+ */
+ if (pvid == vid)
+ pvid = G0_PORT_VID_DEF;
+ }
+
+ mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK, pvid);
+ priv->ports[port].pvid = pvid;
+
+ mutex_unlock(&priv->reg_mutex);
+
+ return 0;
+}
+
static enum dsa_tag_protocol
mtk_get_tag_protocol(struct dsa_switch *ds, int port)
{
@@ -1035,6 +1321,10 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
.port_fdb_add = mt7530_port_fdb_add,
.port_fdb_del = mt7530_port_fdb_del,
.port_fdb_dump = mt7530_port_fdb_dump,
+ .port_vlan_filtering = mt7530_port_vlan_filtering,
+ .port_vlan_prepare = mt7530_port_vlan_prepare,
+ .port_vlan_add = mt7530_port_vlan_add,
+ .port_vlan_del = mt7530_port_vlan_del,
};
static int
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 74db982..2984812 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -17,6 +17,7 @@
#define MT7530_NUM_PORTS 7
#define MT7530_CPU_PORT 6
#define MT7530_NUM_FDB_RECORDS 2048
+#define MT7530_ALL_MEMBERS 0xff
#define NUM_TRGMII_CTRL 5
@@ -88,21 +89,42 @@ enum mt7530_fdb_cmd {
/* Register for vlan table control */
#define MT7530_VTCR 0x90
#define VTCR_BUSY BIT(31)
-#define VTCR_FUNC (((x) & 0xf) << 12)
-#define VTCR_FUNC_RD_VID 0x1
-#define VTCR_FUNC_WR_VID 0x2
-#define VTCR_FUNC_INV_VID 0x3
-#define VTCR_FUNC_VAL_VID 0x4
+#define VTCR_INVALID BIT(16)
+#define VTCR_FUNC(x) (((x) & 0xf) << 12)
#define VTCR_VID ((x) & 0xfff)
+enum mt7530_vlan_cmd {
+ /* Read/Write the specified VID Entry from VAWD register based
+ * on VID.
+ */
+ MT7530_VTCR_RD_VID = 0,
+ MT7530_VTCR_WR_VID = 1,
+};
+
/* Register for setup vlan and acl write data */
#define MT7530_VAWD1 0x94
#define PORT_STAG BIT(31)
+/* Independent VLAN Learning */
#define IVL_MAC BIT(30)
+/* Per VLAN Egress Tag Control */
+#define VTAG_EN BIT(28)
+/* VLAN Member Control */
#define PORT_MEM(x) (((x) & 0xff) << 16)
-#define VALID BIT(1)
+/* VLAN Entry Valid */
+#define VLAN_VALID BIT(0)
+#define PORT_MEM_SHFT 16
+#define PORT_MEM_MASK 0xff
#define MT7530_VAWD2 0x98
+/* Egress Tag Control */
+#define ETAG_CTRL_P(p, x) (((x) & 0x3) << ((p) << 1))
+#define ETAG_CTRL_P_MASK(p) ETAG_CTRL_P(p, 3)
+
+enum mt7530_vlan_egress_attr {
+ MT7530_VLAN_EGRESS_UNTAG = 0,
+ MT7530_VLAN_EGRESS_TAG = 2,
+ MT7530_VLAN_EGRESS_STACK = 3,
+};
/* Register for port STP state control */
#define MT7530_SSP_P(x) (0x2000 + ((x) * 0x100))
@@ -120,11 +142,23 @@ enum mt7530_stp_state {
/* Register for port control */
#define MT7530_PCR_P(x) (0x2004 + ((x) * 0x100))
#define PORT_VLAN(x) ((x) & 0x3)
+
+enum mt7530_port_mode {
+ /* Port Matrix Mode: frames are forwarded by the PCR_MATRIX members. */
+ MT7530_PORT_MATRIX_MODE = PORT_VLAN(0),
+
+ /* Security Mode: discard any frame due to ingress membership
+ * violation or VID missed on the VLAN table.
+ */
+ MT7530_PORT_SECURITY_MODE = PORT_VLAN(3),
+};
+
#define PCR_MATRIX(x) (((x) & 0xff) << 16)
#define PORT_PRI(x) (((x) & 0x7) << 24)
#define EG_TAG(x) (((x) & 0x3) << 28)
#define PCR_MATRIX_MASK PCR_MATRIX(0xff)
#define PCR_MATRIX_CLR PCR_MATRIX(0)
+#define PCR_PORT_VLAN_MASK PORT_VLAN(3)
/* Register for port security control */
#define MT7530_PSC_P(x) (0x200c + ((x) * 0x100))
@@ -134,10 +168,20 @@ enum mt7530_stp_state {
#define MT7530_PVC_P(x) (0x2010 + ((x) * 0x100))
#define PORT_SPEC_TAG BIT(5)
#define VLAN_ATTR(x) (((x) & 0x3) << 6)
+#define VLAN_ATTR_MASK VLAN_ATTR(3)
+
+enum mt7530_vlan_port_attr {
+ MT7530_VLAN_USER = 0,
+ MT7530_VLAN_TRANSPARENT = 3,
+};
+
#define STAG_VPID (((x) & 0xffff) << 16)
/* Register for port port-and-protocol based vlan 1 control */
#define MT7530_PPBV1_P(x) (0x2014 + ((x) * 0x100))
+#define G0_PORT_VID(x) (((x) & 0xfff) << 0)
+#define G0_PORT_VID_MASK G0_PORT_VID(0xfff)
+#define G0_PORT_VID_DEF G0_PORT_VID(1)
/* Register for port MAC control register */
#define MT7530_PMCR_P(x) (0x3000 + ((x) * 0x100))
@@ -345,9 +389,20 @@ struct mt7530_fdb {
bool noarp;
};
+/* struct mt7530_port - This is the main data structure for holding the state
+ * of the port.
+ * @enable: The status used for show port is enabled or not.
+ * @pm: The matrix used to show all connections with the port.
+ * @pvid: The VLAN specified is to be considered a PVID at ingress. Any
+ * untagged frames will be assigned to the related VLAN.
+ * @vlan_filtering: The flags indicating whether the port that can recognize
+ * VLAN-tagged frames.
+ */
struct mt7530_port {
bool enable;
u32 pm;
+ u16 pvid;
+ bool vlan_filtering;
};
/* struct mt7530_priv - This is the main data structure for holding the state
@@ -382,6 +437,22 @@ struct mt7530_priv {
struct mutex reg_mutex;
};
+struct mt7530_hw_vlan_entry {
+ int port;
+ u8 old_members;
+ bool untagged;
+};
+
+static inline void INIT_MT7530_HW_ENTRY(struct mt7530_hw_vlan_entry *e,
+ int port, bool untagged)
+{
+ e->port = port;
+ e->untagged = untagged;
+}
+
+typedef void (*mt7530_vlan_op)(struct mt7530_priv *,
+ struct mt7530_hw_vlan_entry *);
+
struct mt7530_hw_stats {
const char *string;
u16 reg;
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 0/3] add VLAN support to DSA MT7530
From: sean.wang @ 2017-12-07 6:06 UTC (permalink / raw)
To: davem, andrew, f.fainelli, vivien.didelot
Cc: netdev, linux-kernel, linux-mediatek, Sean Wang
From: Sean Wang <sean.wang@mediatek.com>
The patchset extends DSA MT7530 to VLAN support through filling required
callbacks in patch 1 and merging the special tag with VLAN tag in patch 2
for allowing that the hardware can handle these packets with VID from the
CPU port.
Sean Wang (3):
net: dsa: mediatek: add VLAN support for MT7530
net: dsa: mediatek: combine MediaTek tag with VLAN tag
net: dsa: mediatek: update MAINTAINERS entry with MediaTek switch
driver
MAINTAINERS | 7 ++
drivers/net/dsa/mt7530.c | 292 ++++++++++++++++++++++++++++++++++++++++++++++-
drivers/net/dsa/mt7530.h | 83 +++++++++++++-
net/dsa/tag_mtk.c | 38 ++++--
4 files changed, 404 insertions(+), 16 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH net-next] netdevsim: check return value of debugfs_create_dir
From: Jakub Kicinski @ 2017-12-07 6:05 UTC (permalink / raw)
To: Prashant Bhole; +Cc: 'David S . Miller', netdev
In-Reply-To: <000401d36f11$577e2b90$067a82b0$@lab.ntt.co.jp>
On Thu, 7 Dec 2017 13:10:39 +0900, Prashant Bhole wrote:
> > From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> >
> > On Thu, 7 Dec 2017 10:02:13 +0900, Prashant Bhole wrote:
> > > - Handled debugfs_create_dir failure in nsim_init()
> > > - Fixed return value of nsim_module_init() when debugfs_create_dir
> > > fails
> > >
> > > Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> >
> > Why? Failing to expose the state via DebugFS is not fatal to the driver.
>
> Ok, my intention was to handle the return code properly, which is not needed
> as per your comment.
> Shall I remove the existing handling in nsim_module_init() in separate
> patch?
I was going back and forth on the error handling quite a bit writing
that code. In the end I decided to leave the module_init check and
check for bpf prog directory. Former one is mostly useful to make sure
the is no duplicate directory with the same name, the latter to limit
possible false positive in the selftest..
> Because it will prevent netdevsim from loading when debugfs is disabled.
Note that netdevsim depends on DEBUG_FS:
config NETDEVSIM
tristate "Simulated networking device"
depends on DEBUG_FS
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 10/10] ixgbe: register ipsec offload with the xfrm subsystem
From: Shannon Nelson @ 2017-12-07 5:43 UTC (permalink / raw)
To: Alexander Duyck
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <CAKgT0Ue_PxNbqJL-7PprJ5V9M6gCd1ObJk+EXtJpwAJz-N+eew@mail.gmail.com>
On 12/5/2017 12:11 PM, Alexander Duyck wrote:
> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
> <shannon.nelson@oracle.com> wrote:
>> With all the support code in place we can now link in the ipsec
>> offload operations and set the ESP feature flag for the XFRM
>> subsystem to see.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 4 ++++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++++
>> 2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> index d1220bf..0d5497b 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> @@ -884,6 +884,10 @@ void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter)
>> ixgbe_ipsec_clear_hw_tables(adapter);
>> ixgbe_ipsec_stop_engine(adapter);
>>
>> + adapter->netdev->xfrmdev_ops = &ixgbe_xfrmdev_ops;
>> + adapter->netdev->features |= NETIF_F_HW_ESP;
>> + adapter->netdev->hw_enc_features |= NETIF_F_HW_ESP;
>> +
>> return;
>> err:
>> if (ipsec) {
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index c857594..9231351 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -9799,6 +9799,10 @@ ixgbe_features_check(struct sk_buff *skb, struct net_device *dev,
>> if (skb->encapsulation && !(features & NETIF_F_TSO_MANGLEID))
>> features &= ~NETIF_F_TSO;
>>
>> + /* IPsec offload doesn't get along well with others *yet* */
>> + if (skb->sp)
>> + features &= ~(NETIF_F_TSO | NETIF_F_HW_CSUM_BIT);
>
> I'm pretty sure the feature flag stripping here isn't correct. The
Well, first of all that NETIF_F_HW_CSUM_BIT should be NETIF_F_HW_CSUM.
> feature bits you want to strip would probably be consistent with the
> network_hdr_len check bits included before the MANGLEID check.
>
> We should do some digging into this as it may be a kernel issue. I'm
> just wondering if ipsec updates any headers such as the transport
> offset or skb checksum start. If either of those are updated that
> would explain the issues with getting the offloads to work.
Doing this got the TSO and such out of my way so I didn't have to turn
tx csum off with ethtool, but you're right, this can be tweaked a little.
There will be more digging later when I work on getting TSO and CSUM
working with ipsec offload, but I want to get these patches out first
now that they're working, and then tweak for more performance.
Again, thanks for your time and thoughts.
sln
>
>> +
>> return features;
>> }
>>
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 09/10] ixgbe: ipsec offload stats
From: Shannon Nelson @ 2017-12-07 5:43 UTC (permalink / raw)
To: Alexander Duyck
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <CAKgT0Uc2TEE4HdzJ8jPSX13LbHSpydCH+Csw8q_6+773ML17RQ@mail.gmail.com>
On 12/5/2017 11:53 AM, Alexander Duyck wrote:
> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
> <shannon.nelson@oracle.com> wrote:
>> Add a simple statistic to count the ipsec offloads.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 28 ++++++++++++++----------
>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 3 +++
>> 3 files changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 68097fe..bb66c85 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -265,6 +265,7 @@ struct ixgbe_rx_buffer {
>> struct ixgbe_queue_stats {
>> u64 packets;
>> u64 bytes;
>> + u64 ipsec_offloads;
>> };
>>
>> struct ixgbe_tx_queue_stats {
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> index c3e7a81..dddbc74 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> @@ -1233,34 +1233,34 @@ static void ixgbe_get_ethtool_stats(struct net_device *netdev,
>> for (j = 0; j < netdev->num_tx_queues; j++) {
>> ring = adapter->tx_ring[j];
>> if (!ring) {
>> - data[i] = 0;
>> - data[i+1] = 0;
>> - i += 2;
>> + data[i++] = 0;
>> + data[i++] = 0;
>> + data[i++] = 0;
>> continue;
>> }
>>
>> do {
>> start = u64_stats_fetch_begin_irq(&ring->syncp);
>> - data[i] = ring->stats.packets;
>> - data[i+1] = ring->stats.bytes;
>> + data[i++] = ring->stats.packets;
>> + data[i++] = ring->stats.bytes;
>> + data[i++] = ring->stats.ipsec_offloads;
>> } while (u64_stats_fetch_retry_irq(&ring->syncp, start));
>> - i += 2;
>> }
>> for (j = 0; j < IXGBE_NUM_RX_QUEUES; j++) {
>> ring = adapter->rx_ring[j];
>> if (!ring) {
>> - data[i] = 0;
>> - data[i+1] = 0;
>> - i += 2;
>> + data[i++] = 0;
>> + data[i++] = 0;
>> + data[i++] = 0;
>> continue;
>> }
>>
>> do {
>> start = u64_stats_fetch_begin_irq(&ring->syncp);
>> - data[i] = ring->stats.packets;
>> - data[i+1] = ring->stats.bytes;
>> + data[i++] = ring->stats.packets;
>> + data[i++] = ring->stats.bytes;
>> + data[i++] = ring->stats.ipsec_offloads;
>> } while (u64_stats_fetch_retry_irq(&ring->syncp, start));
>> - i += 2;
>> }
>>
>> for (j = 0; j < IXGBE_MAX_PACKET_BUFFERS; j++) {
>> @@ -1297,12 +1297,16 @@ static void ixgbe_get_strings(struct net_device *netdev, u32 stringset,
>> p += ETH_GSTRING_LEN;
>> sprintf(p, "tx_queue_%u_bytes", i);
>> p += ETH_GSTRING_LEN;
>> + sprintf(p, "tx_queue_%u_ipsec_offloads", i);
>> + p += ETH_GSTRING_LEN;
>> }
>> for (i = 0; i < IXGBE_NUM_RX_QUEUES; i++) {
>> sprintf(p, "rx_queue_%u_packets", i);
>> p += ETH_GSTRING_LEN;
>> sprintf(p, "rx_queue_%u_bytes", i);
>> p += ETH_GSTRING_LEN;
>> + sprintf(p, "rx_queue_%u_ipsec_offloads", i);
>> + p += ETH_GSTRING_LEN;
>> }
>> for (i = 0; i < IXGBE_MAX_PACKET_BUFFERS; i++) {
>> sprintf(p, "tx_pb_%u_pxon", i);
>
> I probably wouldn't bother reporting this per ring. It might make more
> sense to handle this as an adapter statistic.
I agree, it really messes up the output. However, I like seeing it per
ring while I'm testing. I'll move it.
>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> index 2a0dd7a..d1220bf 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> @@ -782,6 +782,7 @@ int ixgbe_ipsec_tx(struct ixgbe_ring *tx_ring, struct sk_buff *skb,
>> if (tsa->encrypt)
>> itd->flags |= IXGBE_ADVTXD_TUCMD_IPSEC_ENCRYPT_EN;
>>
>> + tx_ring->stats.ipsec_offloads++;
>> return 1;
>
> Instead of doing this here you may want to make it a part of the Tx
> clean-up path. You should still have the flag bit set so you could
> test a test for the IPSEC flag bit and if it is set on the tx_buffer
> following the transmit you could then increment it there.
Is there a benefit to doing it elsewhere? I'm assuming the answer has
to do with fastpath cycles...
sln
>
>> }
>>
>> @@ -843,6 +844,8 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>> xo = xfrm_offload(skb);
>> xo->flags = CRYPTO_DONE;
>> xo->status = CRYPTO_SUCCESS;
>> +
>> + rx_ring->stats.ipsec_offloads++;
>> }
>>
>> /**
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 08/10] ixgbe: process the Tx ipsec offload
From: Shannon Nelson @ 2017-12-07 5:43 UTC (permalink / raw)
To: Alexander Duyck
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <CAKgT0UdajKqTk+FCaQivtV=i7tZ1RC2cA8ifqzFMhLqQMspN7A@mail.gmail.com>
On 12/5/2017 10:13 AM, Alexander Duyck wrote:
> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
> <shannon.nelson@oracle.com> wrote:
>> If the skb has a security association referenced in the skb, then
>> set up the Tx descriptor with the ipsec offload bits. While we're
>> here, we fix an oddly named field in the context descriptor struct.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 10 +++-
>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 77 ++++++++++++++++++++++++++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c | 4 +-
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 38 ++++++++++---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h | 2 +-
>> 5 files changed, 118 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 77f07dc..68097fe 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -171,10 +171,11 @@ enum ixgbe_tx_flags {
>> IXGBE_TX_FLAGS_CC = 0x08,
>> IXGBE_TX_FLAGS_IPV4 = 0x10,
>> IXGBE_TX_FLAGS_CSUM = 0x20,
>> + IXGBE_TX_FLAGS_IPSEC = 0x40,
>>
>> /* software defined flags */
>> - IXGBE_TX_FLAGS_SW_VLAN = 0x40,
>> - IXGBE_TX_FLAGS_FCOE = 0x80,
>> + IXGBE_TX_FLAGS_SW_VLAN = 0x80,
>> + IXGBE_TX_FLAGS_FCOE = 0x100,
>> };
>>
>> /* VLAN info */
>> @@ -1012,12 +1013,17 @@ void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter);
>> void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>> union ixgbe_adv_rx_desc *rx_desc,
>> struct sk_buff *skb);
>> +int ixgbe_ipsec_tx(struct ixgbe_ring *tx_ring, struct sk_buff *skb,
>> + __be16 protocol, struct ixgbe_ipsec_tx_data *itd);
>> void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter);
>> #else
>> static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) { };
>> static inline void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>> union ixgbe_adv_rx_desc *rx_desc,
>> struct sk_buff *skb) { };
>> +static inline int ixgbe_ipsec_tx(struct ixgbe_ring *tx_ring,
>> + struct sk_buff *skb, __be16 protocol,
>> + struct ixgbe_ipsec_tx_data *itd) { return 0; };
>> static inline void ixgbe_ipsec_restore(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
>> index fd06d9b..2a0dd7a 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> @@ -703,12 +703,89 @@ static void ixgbe_ipsec_del_sa(struct xfrm_state *xs)
>> }
>> }
>>
>> +/**
>> + * ixgbe_ipsec_offload_ok - can this packet use the xfrm hw offload
>> + * @skb: current data packet
>> + * @xs: pointer to transformer state struct
>> + **/
>> +static bool ixgbe_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
>> +{
>> + if (xs->props.family == AF_INET) {
>> + /* Offload with IPv4 options is not supported yet */
>> + if (ip_hdr(skb)->ihl > 5)
>
> I would make this ihl != 5 instead of "> 5" since smaller values would
> be invalid as well.
Sure
>
>> + return false;
>> + } else {
>> + /* Offload with IPv6 extension headers is not support yet */
>> + if (ipv6_ext_hdr(ipv6_hdr(skb)->nexthdr))
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> static const struct xfrmdev_ops ixgbe_xfrmdev_ops = {
>> .xdo_dev_state_add = ixgbe_ipsec_add_sa,
>> .xdo_dev_state_delete = ixgbe_ipsec_del_sa,
>> + .xdo_dev_offload_ok = ixgbe_ipsec_offload_ok,
>> };
>>
>> /**
>> + * ixgbe_ipsec_tx - setup Tx flags for ipsec offload
>> + * @tx_ring: outgoing context
>> + * @skb: current data packet
>> + * @protocol: network protocol
>> + * @itd: ipsec Tx data for later use in building context descriptor
>> + **/
>> +int ixgbe_ipsec_tx(struct ixgbe_ring *tx_ring, struct sk_buff *skb,
>> + __be16 protocol, struct ixgbe_ipsec_tx_data *itd)
>> +{
>> + struct ixgbe_adapter *adapter = netdev_priv(tx_ring->netdev);
>> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
>> + struct xfrm_state *xs;
>> + struct tx_sa *tsa;
>> +
>> + if (!skb->sp->len) {
>> + netdev_err(tx_ring->netdev, "%s: no xfrm state len = %d\n",
>> + __func__, skb->sp->len);
>> + return 0;
>> + }
>> +
>> + xs = xfrm_input_state(skb);
>> + if (!xs) {
>> + netdev_err(tx_ring->netdev, "%s: no xfrm_input_state() xs = %p\n",
>> + __func__, xs);
>> + return 0;
>> + }
>> +
>> + itd->sa_idx = xs->xso.offload_handle - IXGBE_IPSEC_BASE_TX_INDEX;
>> + if (itd->sa_idx > IXGBE_IPSEC_MAX_SA_COUNT) {
>> + netdev_err(tx_ring->netdev, "%s: bad sa_idx=%d handle=%lu\n",
>> + __func__, itd->sa_idx, xs->xso.offload_handle);
>> + return 0;
>> + }
>> +
>> + tsa = &ipsec->tx_tbl[itd->sa_idx];
>> + if (!tsa->used) {
>> + netdev_err(tx_ring->netdev, "%s: unused sa_idx=%d\n",
>> + __func__, itd->sa_idx);
>> + return 0;
>> + }
>> +
>> + itd->flags = 0;
>> + if (xs->id.proto == IPPROTO_ESP) {
>> + itd->flags |= IXGBE_ADVTXD_TUCMD_IPSEC_TYPE_ESP |
>> + IXGBE_ADVTXD_TUCMD_L4T_TCP;
>
> Why is the TCP value being set here? This doesn't seem correct either.
> This implies TCP a TCP offload. It seems like this should only be
> setting ESP.
Honestly? Because when I was testing that, it didn't work without it.
This was one of the things I was going to come back to when I started
working on the csum and tso support.
>
>> + if (protocol == htons(ETH_P_IP))
>> + itd->flags |= IXGBE_ADVTXD_TUCMD_IPV4;
>
> Does the IPsec offload need to know if the frame is v4 or v6? I'm just
> wondering if it does or not.
Yes, I believe this is how it knows how much header to skip to find the
ESP header. However, I'll test that and see if it can come out.
> If not then this probably isn't needed.
> One thought on this line is you might look at moving it into
> ixgbe_tx_csum. If setting the bit is harmless without setting IXSM we
> might look at moving it into the end of ixgbe_tx_csum and just make it
> compare against first->protocol there.
>
>> + itd->trailer_len = xs->props.trailer_len;
>> + }
>> + if (tsa->encrypt)
>> + itd->flags |= IXGBE_ADVTXD_TUCMD_IPSEC_ENCRYPT_EN;
>> +
>> + return 1;
>> +}
>> +
>> +/**
>> * ixgbe_ipsec_rx - decode ipsec bits from Rx descriptor
>> * @rx_ring: receiving ring
>> * @rx_desc: receive data descriptor
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>> index f1bfae0..d7875b3 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>> @@ -1261,7 +1261,7 @@ void ixgbe_clear_interrupt_scheme(struct ixgbe_adapter *adapter)
>> }
>>
>> void ixgbe_tx_ctxtdesc(struct ixgbe_ring *tx_ring, u32 vlan_macip_lens,
>> - u32 fcoe_sof_eof, u32 type_tucmd, u32 mss_l4len_idx)
>> + u32 fceof_saidx, u32 type_tucmd, u32 mss_l4len_idx)
>> {
>> struct ixgbe_adv_tx_context_desc *context_desc;
>> u16 i = tx_ring->next_to_use;
>> @@ -1275,7 +1275,7 @@ void ixgbe_tx_ctxtdesc(struct ixgbe_ring *tx_ring, u32 vlan_macip_lens,
>> type_tucmd |= IXGBE_TXD_CMD_DEXT | IXGBE_ADVTXD_DTYP_CTXT;
>>
>> context_desc->vlan_macip_lens = cpu_to_le32(vlan_macip_lens);
>> - context_desc->seqnum_seed = cpu_to_le32(fcoe_sof_eof);
>> + context_desc->fceof_saidx = cpu_to_le32(fceof_saidx);
>> context_desc->type_tucmd_mlhl = cpu_to_le32(type_tucmd);
>> context_desc->mss_l4len_idx = cpu_to_le32(mss_l4len_idx);
>> }
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 60f9f2d..c857594 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -7659,9 +7659,10 @@ static void ixgbe_service_task(struct work_struct *work)
>>
>> static int ixgbe_tso(struct ixgbe_ring *tx_ring,
>> struct ixgbe_tx_buffer *first,
>> - u8 *hdr_len)
>> + u8 *hdr_len,
>> + struct ixgbe_ipsec_tx_data *itd)
>> {
>> - u32 vlan_macip_lens, type_tucmd, mss_l4len_idx;
>> + u32 vlan_macip_lens, type_tucmd, mss_l4len_idx, fceof_saidx = 0;
>> struct sk_buff *skb = first->skb;
>> union {
>> struct iphdr *v4;
>> @@ -7740,7 +7741,12 @@ static int ixgbe_tso(struct ixgbe_ring *tx_ring,
>> vlan_macip_lens |= (ip.hdr - skb->data) << IXGBE_ADVTXD_MACLEN_SHIFT;
>> vlan_macip_lens |= first->tx_flags & IXGBE_TX_FLAGS_VLAN_MASK;
>>
>> - ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, 0, type_tucmd,
>> + if (first->tx_flags & IXGBE_TX_FLAGS_IPSEC) {
>> + fceof_saidx |= itd->sa_idx;
>> + type_tucmd |= itd->flags | itd->trailer_len;
>> + }
>> +
>> + ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, fceof_saidx, type_tucmd,
>> mss_l4len_idx);
>>
>> return 1;
>> @@ -7756,10 +7762,12 @@ static inline bool ixgbe_ipv6_csum_is_sctp(struct sk_buff *skb)
>> }
>>
>> static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
>> - struct ixgbe_tx_buffer *first)
>> + struct ixgbe_tx_buffer *first,
>> + struct ixgbe_ipsec_tx_data *itd)
>> {
>> struct sk_buff *skb = first->skb;
>> u32 vlan_macip_lens = 0;
>> + u32 fceof_saidx = 0;
>> u32 type_tucmd = 0;
>>
>> if (skb->ip_summed != CHECKSUM_PARTIAL) {
>> @@ -7800,7 +7808,12 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
>> vlan_macip_lens |= skb_network_offset(skb) << IXGBE_ADVTXD_MACLEN_SHIFT;
>> vlan_macip_lens |= first->tx_flags & IXGBE_TX_FLAGS_VLAN_MASK;
>>
>> - ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, 0, type_tucmd, 0);
>> + if (first->tx_flags & IXGBE_TX_FLAGS_IPSEC) {
>> + fceof_saidx |= itd->sa_idx;
>> + type_tucmd |= itd->flags | itd->trailer_len;
>> + }
>> +
>> + ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, fceof_saidx, type_tucmd, 0);
>> }
>>
>> #define IXGBE_SET_FLAG(_input, _flag, _result) \
>> @@ -7843,11 +7856,16 @@ static void ixgbe_tx_olinfo_status(union ixgbe_adv_tx_desc *tx_desc,
>> IXGBE_TX_FLAGS_CSUM,
>> IXGBE_ADVTXD_POPTS_TXSM);
>>
>> - /* enble IPv4 checksum for TSO */
>> + /* enable IPv4 checksum for TSO */
>> olinfo_status |= IXGBE_SET_FLAG(tx_flags,
>> IXGBE_TX_FLAGS_IPV4,
>> IXGBE_ADVTXD_POPTS_IXSM);
>>
>> + /* enable IPsec */
>> + olinfo_status |= IXGBE_SET_FLAG(tx_flags,
>> + IXGBE_TX_FLAGS_IPSEC,
>> + IXGBE_ADVTXD_POPTS_IPSEC);
>> +
>> /*
>> * Check Context must be set if Tx switch is enabled, which it
>> * always is for case where virtual functions are running
>> @@ -8306,6 +8324,7 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
>> u32 tx_flags = 0;
>> unsigned short f;
>> u16 count = TXD_USE_COUNT(skb_headlen(skb));
>> + struct ixgbe_ipsec_tx_data ipsec_tx = { 0 };
>> __be16 protocol = skb->protocol;
>> u8 hdr_len = 0;
>>
>> @@ -8394,6 +8413,9 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
>> }
>> }
>>
>> + if (skb->sp && ixgbe_ipsec_tx(tx_ring, skb, protocol, &ipsec_tx))
>> + tx_flags |= IXGBE_TX_FLAGS_IPSEC | IXGBE_TX_FLAGS_CC;
>
> You might just want to pull the skb->sp check into ixgbe_ipsec_tx and
> could pass tx_flags as a part of the first buffer. It doesn't really
> matter anyway as most of this will just be inlined so it will all end
> up a part of the same function anyway.
Since the function is defined in a different .o file, are you sure it
will get inlined? I put the skb->sp check here to make sure we don't do
an unnecessary jump.
>
> Also I would move this down so that it is handled after the fields in
> the first buffer_info structure are set. Then this can ll just fall
> inline with the TSO block and get handled there.
>
>> +
>> /* record initial flags and protocol */
>> first->tx_flags = tx_flags;
>> first->protocol = protocol;
>> @@ -8410,11 +8432,11 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
>> }
>>
>> #endif /* IXGBE_FCOE */
>
> So if you move the function down here it will help to avoid any other
> complication. In addition you could follow the same logic that we do
> for ixgbe_tso/fso so you could drop the frame instead of transmitting
> it if it is requesting a bad offload.
Sure
sln
>
>> - tso = ixgbe_tso(tx_ring, first, &hdr_len);
>> + tso = ixgbe_tso(tx_ring, first, &hdr_len, &ipsec_tx);
>> if (tso < 0)
>> goto out_drop;
>> else if (!tso)
>> - ixgbe_tx_csum(tx_ring, first);
>> + ixgbe_tx_csum(tx_ring, first, &ipsec_tx);
>>
>> /* add the ATR filter if ATR is on */
>> if (test_bit(__IXGBE_TX_FDIR_INIT_DONE, &tx_ring->state))
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>> index 3df0763..0ac725fa 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>> @@ -2856,7 +2856,7 @@ union ixgbe_adv_rx_desc {
>> /* Context descriptors */
>> struct ixgbe_adv_tx_context_desc {
>> __le32 vlan_macip_lens;
>> - __le32 seqnum_seed;
>> + __le32 fceof_saidx;
>> __le32 type_tucmd_mlhl;
>> __le32 mss_l4len_idx;
>> };
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 07/10] ixgbe: process the Rx ipsec offload
From: Shannon Nelson @ 2017-12-07 5:43 UTC (permalink / raw)
To: Alexander Duyck
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <CAKgT0UdJxfXZoHifkUFtnwwhuV9WLgEFO3ccqfTnS-nX5a7wHQ@mail.gmail.com>
On 12/5/2017 9:40 AM, Alexander Duyck wrote:
> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
> <shannon.nelson@oracle.com> wrote:
>> If the chip sees and decrypts an ipsec offload, set up the skb
>> sp pointer with the ralated SA info. Since the chip is rude
>> enough to keep to itself the table index it used for the
>> decryption, we have to do our own table lookup, using the
>> hash for speed.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 6 ++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 89 ++++++++++++++++++++++++++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +
>> 3 files changed, 98 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 7e8bca7..77f07dc 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -1009,9 +1009,15 @@ 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);
>> +void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>> + union ixgbe_adv_rx_desc *rx_desc,
>> + struct sk_buff *skb);
>> void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter);
>> #else
>> static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) { };
>> +static inline void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>> + union ixgbe_adv_rx_desc *rx_desc,
>> + struct sk_buff *skb) { };
>> static inline void ixgbe_ipsec_restore(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
>> index b93ee7f..fd06d9b 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> @@ -379,6 +379,35 @@ static int ixgbe_ipsec_find_empty_idx(struct ixgbe_ipsec *ipsec, bool rxtable)
>> }
>>
>> /**
>> + * ixgbe_ipsec_find_rx_state - find the state that matches
>> + * @ipsec: pointer to ipsec struct
>> + * @daddr: inbound address to match
>> + * @proto: protocol to match
>> + * @spi: SPI to match
>> + *
>> + * Returns a pointer to the matching SA state information
>> + **/
>> +static struct xfrm_state *ixgbe_ipsec_find_rx_state(struct ixgbe_ipsec *ipsec,
>> + __be32 daddr, u8 proto,
>> + __be32 spi)
>> +{
>> + struct rx_sa *rsa;
>> + struct xfrm_state *ret = NULL;
>> +
>> + rcu_read_lock();
>> + hash_for_each_possible_rcu(ipsec->rx_sa_list, rsa, hlist, spi)
>> + if (spi == rsa->xs->id.spi &&
>> + daddr == rsa->xs->id.daddr.a4 &&
>> + proto == rsa->xs->id.proto) {
>> + ret = rsa->xs;
>> + xfrm_state_hold(ret);
>> + break;
>> + }
>> + rcu_read_unlock();
>> + return ret;
>> +}
>> +
>
> You need to choose a bucket, not just walk through all buckets.
I may be wrong, but I believe that is what is happening here, where the
spi is the hash key. As the function description says "iterate over all
possible objects hashing to the same bucket". Besides, I basically
cribbed this directly from our Mellanox friends (thanks!).
> Otherwise you might as well have just used a linked list. You might
> look at using something like jhash_3words to generate a hash which you
> then use to choose the bucket.
>
>> +/**
>> * ixgbe_ipsec_parse_proto_keys - find the key and salt based on the protocol
>> * @xs: pointer to xfrm_state struct
>> * @mykey: pointer to key array to populate
>> @@ -680,6 +709,66 @@ static const struct xfrmdev_ops ixgbe_xfrmdev_ops = {
>> };
>>
>> /**
>> + * ixgbe_ipsec_rx - decode ipsec bits from Rx descriptor
>> + * @rx_ring: receiving ring
>> + * @rx_desc: receive data descriptor
>> + * @skb: current data packet
>> + *
>> + * Determine if there was an ipsec encapsulation noticed, and if so set up
>> + * the resulting status for later in the receive stack.
>> + **/
>> +void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>> + union ixgbe_adv_rx_desc *rx_desc,
>> + struct sk_buff *skb)
>> +{
>> + struct ixgbe_adapter *adapter = netdev_priv(rx_ring->netdev);
>> + u16 pkt_info = le16_to_cpu(rx_desc->wb.lower.lo_dword.hs_rss.pkt_info);
>> + u16 ipsec_pkt_types = IXGBE_RXDADV_PKTTYPE_IPSEC_AH |
>> + IXGBE_RXDADV_PKTTYPE_IPSEC_ESP;
>> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
>> + struct xfrm_offload *xo = NULL;
>> + struct xfrm_state *xs = NULL;
>> + struct iphdr *iph;
>> + u8 *c_hdr;
>> + __be32 spi;
>> + u8 proto;
>> +
>> + /* we can assume no vlan header in the way, b/c the
>> + * hw won't recognize the IPsec packet and anyway the
>> + * currently vlan device doesn't support xfrm offload.
>> + */
>> + /* TODO: not supporting IPv6 yet */
>> + iph = (struct iphdr *)(skb->data + ETH_HLEN);
>> + c_hdr = (u8 *)iph + iph->ihl * 4;
>> + switch (pkt_info & ipsec_pkt_types) {
>> + case IXGBE_RXDADV_PKTTYPE_IPSEC_AH:
>> + spi = ((struct ip_auth_hdr *)c_hdr)->spi;
>> + proto = IPPROTO_AH;
>> + break;
>> + case IXGBE_RXDADV_PKTTYPE_IPSEC_ESP:
>> + spi = ((struct ip_esp_hdr *)c_hdr)->spi;
>> + proto = IPPROTO_ESP;
>> + break;
>> + default:
>> + return;
>> + }
>> +
>> + xs = ixgbe_ipsec_find_rx_state(ipsec, iph->daddr, proto, spi);
>> + if (unlikely(!xs))
>> + return;
>> +
>> + skb->sp = secpath_dup(skb->sp);
>> + if (unlikely(!skb->sp))
>> + return;
>> +
>> + skb->sp->xvec[skb->sp->len++] = xs;
>> + skb->sp->olen++;
>> + xo = xfrm_offload(skb);
>> + xo->flags = CRYPTO_DONE;
>> + xo->status = CRYPTO_SUCCESS;
>> +}
>> +
>> +/**
>> * ixgbe_init_ipsec_offload - initialize security registers for IPSec operation
>> * @adapter: board private structure
>> **/
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 6eabf92..60f9f2d 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -1755,6 +1755,9 @@ static void ixgbe_process_skb_fields(struct ixgbe_ring *rx_ring,
>>
>> skb_record_rx_queue(skb, rx_ring->queue_index);
>>
>> + if (ixgbe_test_staterr(rx_desc, IXGBE_RXDADV_STAT_SECP))
>> + ixgbe_ipsec_rx(rx_ring, rx_desc, skb);
>> +
>> skb->protocol = eth_type_trans(skb, dev);
>> }
>>
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 06/10] ixgbe: restore offloaded SAs after a reset
From: Shannon Nelson @ 2017-12-07 5:43 UTC (permalink / raw)
To: Alexander Duyck
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <CAKgT0UdOG5H02Tqin4OhEvp=6ZDn365g04JvVOVAiTyCnncPew@mail.gmail.com>
On 12/5/2017 9:30 AM, Alexander Duyck wrote:
> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
> <shannon.nelson@oracle.com> wrote:
>> On a chip reset most of the table contents are lost, so must be
>> restored. This scans the driver's ipsec tables and restores both
>> the filled and empty table slots to their pre-reset values.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 2 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 53 ++++++++++++++++++++++++++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 1 +
>> 3 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 9487750..7e8bca7 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -1009,7 +1009,9 @@ 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);
>> +void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter);
>> #else
>> static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) { };
>> +static inline void ixgbe_ipsec_restore(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
>> index 7b01d92..b93ee7f 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> @@ -292,6 +292,59 @@ static void ixgbe_ipsec_start_engine(struct ixgbe_adapter *adapter)
>> }
>>
>> /**
>> + * ixgbe_ipsec_restore - restore the ipsec HW settings after a reset
>> + * @adapter: board private structure
>> + **/
>> +void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter)
>> +{
>> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
>> + struct ixgbe_hw *hw = &adapter->hw;
>> + u32 zbuf[4] = {0, 0, 0, 0};
>
> zbuf should be a static const.
Yep
>
>> + int i;
>> +
>> + if (!(adapter->flags2 & IXGBE_FLAG2_IPSEC_ENABLED))
>> + return;
>> +
>> + /* clean up the engine settings */
>> + ixgbe_ipsec_stop_engine(adapter);
>> +
>> + /* start the engine */
>> + ixgbe_ipsec_start_engine(adapter);
>> +
>> + /* reload the IP addrs */
>> + for (i = 0; i < IXGBE_IPSEC_MAX_RX_IP_COUNT; i++) {
>> + struct rx_ip_sa *ipsa = &ipsec->ip_tbl[i];
>> +
>> + if (ipsa->used)
>> + ixgbe_ipsec_set_rx_ip(hw, i, ipsa->ipaddr);
>> + else
>> + ixgbe_ipsec_set_rx_ip(hw, i, zbuf);
>
> If we are doing a restore do we actually need to write the zero
> values? If we did a reset I thought you had a function that was going
> through and zeroing everything out. If so this now becomes redundant.
Currently ixgbe_ipsec_clear_hw_tables() only gets run at at probe. It
should probably get run at remove as well. Doing this is a bit of
safety paranoia, and making sure the CAM memory structures that don't
get cleared on reset have exactly what I expect in them.
>
>> + }
>> +
>> + /* reload the Rx keys */
>> + for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
>> + struct rx_sa *rsa = &ipsec->rx_tbl[i];
>> +
>> + if (rsa->used)
>> + ixgbe_ipsec_set_rx_sa(hw, i, rsa->xs->id.spi,
>> + rsa->key, rsa->salt,
>> + rsa->mode, rsa->iptbl_ind);
>> + else
>> + ixgbe_ipsec_set_rx_sa(hw, i, 0, zbuf, 0, 0, 0);
>
> same here
>
>> + }
>> +
>> + /* reload the Tx keys */
>> + for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
>> + struct tx_sa *tsa = &ipsec->tx_tbl[i];
>> +
>> + if (tsa->used)
>> + ixgbe_ipsec_set_tx_sa(hw, i, tsa->key, tsa->salt);
>> + else
>> + ixgbe_ipsec_set_tx_sa(hw, i, zbuf, 0);
>
> and here
>
>> + }
>> +}
>> +
>> +/**
>> * ixgbe_ipsec_find_empty_idx - find the first unused security parameter index
>> * @ipsec: pointer to ipsec struct
>> * @rxtable: true if we need to look in the Rx table
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 01fd89b..6eabf92 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -5347,6 +5347,7 @@ static void ixgbe_configure(struct ixgbe_adapter *adapter)
>>
>> ixgbe_set_rx_mode(adapter->netdev);
>> ixgbe_restore_vlan(adapter);
>> + ixgbe_ipsec_restore(adapter);
>>
>> switch (hw->mac.type) {
>> case ixgbe_mac_82599EB:
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 05/10] ixgbe: implement ipsec add and remove of offloaded SA
From: Shannon Nelson @ 2017-12-07 5:43 UTC (permalink / raw)
To: Alexander Duyck
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <CAKgT0UcCgeOfebWdVB3Z5NNaVXXd917JUMRbignojMf8TZw2ow@mail.gmail.com>
On 12/5/2017 9:26 AM, Alexander Duyck wrote:
> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
> <shannon.nelson@oracle.com> wrote:
>> Add the functions for setting up and removing offloaded SAs (Security
>> Associations) with the x540 hardware. We set up the callback structure
>> but we don't yet set the hardware feature bit to be sure the XFRM service
>> won't actually try to use us for an offload yet.
>>
>> The software tables are made up to mimic the hardware tables to make it
>> easier to track what's in the hardware, and the SA table index is used
>> for the XFRM offload handle. However, there is a hashing field in the
>> Rx SA tracking that will be used to facilitate faster table searches in
>> the Rx fast path.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 377 +++++++++++++++++++++++++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 +
>> 2 files changed, 383 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> index 38a1a16..7b01d92 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> @@ -26,6 +26,8 @@
>> ******************************************************************************/
>>
>> #include "ixgbe.h"
>> +#include <net/xfrm.h>
>> +#include <crypto/aead.h>
>>
>> /**
>> * ixgbe_ipsec_set_tx_sa - set the Tx SA registers
>> @@ -128,6 +130,7 @@ static void ixgbe_ipsec_set_rx_ip(struct ixgbe_hw *hw, u16 idx, u32 addr[])
>> **/
>> void ixgbe_ipsec_clear_hw_tables(struct ixgbe_adapter *adapter)
>> {
>> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
>> struct ixgbe_hw *hw = &adapter->hw;
>> u32 buf[4] = {0, 0, 0, 0};
>> u16 idx;
>> @@ -139,9 +142,11 @@ void ixgbe_ipsec_clear_hw_tables(struct ixgbe_adapter *adapter)
>> /* scrub the tables */
>> for (idx = 0; idx < IXGBE_IPSEC_MAX_SA_COUNT; idx++)
>> ixgbe_ipsec_set_tx_sa(hw, idx, buf, 0);
>> + ipsec->num_tx_sa = 0;
>>
>> for (idx = 0; idx < IXGBE_IPSEC_MAX_SA_COUNT; idx++)
>> ixgbe_ipsec_set_rx_sa(hw, idx, 0, buf, 0, 0, 0);
>> + ipsec->num_rx_sa = 0;
>>
>> for (idx = 0; idx < IXGBE_IPSEC_MAX_RX_IP_COUNT; idx++)
>> ixgbe_ipsec_set_rx_ip(hw, idx, buf);
>> @@ -287,11 +292,383 @@ static void ixgbe_ipsec_start_engine(struct ixgbe_adapter *adapter)
>> }
>>
>> /**
>> + * ixgbe_ipsec_find_empty_idx - find the first unused security parameter index
>> + * @ipsec: pointer to ipsec struct
>> + * @rxtable: true if we need to look in the Rx table
>> + *
>> + * Returns the first unused index in either the Rx or Tx SA table
>> + **/
>> +static int ixgbe_ipsec_find_empty_idx(struct ixgbe_ipsec *ipsec, bool rxtable)
>> +{
>> + u32 i;
>> +
>> + if (rxtable) {
>> + if (ipsec->num_rx_sa == IXGBE_IPSEC_MAX_SA_COUNT)
>> + return -ENOSPC;
>> +
>> + /* search rx sa table */
>> + for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
>> + if (!ipsec->rx_tbl[i].used)
>> + return i;
>> + }
>> + } else {
>> + if (ipsec->num_rx_sa == IXGBE_IPSEC_MAX_SA_COUNT)
>> + return -ENOSPC;
>
> Should this bi num_tx_sa?
Hmm - can you say cut-and-paste? Will fix.
>
>> +
>> + /* search tx sa table */
>> + for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
>> + if (!ipsec->tx_tbl[i].used)
>> + return i;
>> + }
>> + }
>> +
>> + return -ENOSPC;
>> +}
>> +
>> +/**
>> + * ixgbe_ipsec_parse_proto_keys - find the key and salt based on the protocol
>> + * @xs: pointer to xfrm_state struct
>> + * @mykey: pointer to key array to populate
>> + * @mysalt: pointer to salt value to populate
>> + *
>> + * This copies the protocol keys and salt to our own data tables. The
>> + * 82599 family only supports the one algorithm.
>> + **/
>> +static int ixgbe_ipsec_parse_proto_keys(struct xfrm_state *xs,
>> + u32 *mykey, u32 *mysalt)
>> +{
>> + struct net_device *dev = xs->xso.dev;
>> + unsigned char *key_data;
>> + char *alg_name = NULL;
>> + char *aes_gcm_name = "rfc4106(gcm(aes))";
>
> aes_gcm_name should probably be a static const char array instead of a pointer.
Sure.
>
>> + int key_len;
>> +
>> + if (xs->aead) {
>> + key_data = &xs->aead->alg_key[0];
>> + key_len = xs->aead->alg_key_len;
>> + alg_name = xs->aead->alg_name;
>> + } else {
>> + netdev_err(dev, "Unsupported IPsec algorithm\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (strcmp(alg_name, aes_gcm_name)) {
>> + netdev_err(dev, "Unsupported IPsec algorithm - please use %s\n",
>> + aes_gcm_name);
>> + return -EINVAL;
>> + }
>> +
>> + /* 160 accounts for 16 byte key and 4 byte salt */
>> + if (key_len == 128) {
>> + netdev_info(dev, "IPsec hw offload parameters missing 32 bit salt value\n");
>> + } else if (key_len != 160) {
>> + netdev_err(dev, "IPsec hw offload only supports keys up to 128 bits with a 32 bit salt\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* The key bytes come down in a bigendian array of bytes, and
>> + * salt is always the last 4 bytes of the key array.
>> + * We don't need to do any byteswapping.
>> + */
>> + memcpy(mykey, key_data, 16);
>> + if (key_len == 160)
>> + *mysalt = ((u32 *)key_data)[4];
>> + else
>> + *mysalt = 0;
>
> You could combine these key_len checks into a single if/else set.
> Basically just do something like the following:
Alex, ever the reductionist :-)
Yep, makes sense.
>
> /* 160 accounts for 16 byte key and 4 byte salt */
> if (key_len == 160) {
> *mysalt = ((u32 *)key_data)[4];
> } else if (key_len != 128) {
> netdev_err(dev, "IPsec hw offload only supports keys up to 128
> bits with a 32 bit salt\n");
> return -EINVAL;
> } else {
> netdev_info(dev, "IPsec hw offload parameters missing 32 bit
> salt value\n");
> *mysalt = 0;
> }
>
> /* The key bytes come down in a bigendian array of bytes, and
> * salt is always the last 4 bytes of the key array.
> * We don't need to do any byteswapping.
> */
> memcpy(mykey, key_data, 16);
>
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * ixgbe_ipsec_add_sa - program device with a security association
>> + * @xs: pointer to transformer state struct
>> + **/
>> +static int ixgbe_ipsec_add_sa(struct xfrm_state *xs)
>> +{
>> + struct net_device *dev = xs->xso.dev;
>> + struct ixgbe_adapter *adapter = netdev_priv(dev);
>> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
>> + struct ixgbe_hw *hw = &adapter->hw;
>> + int checked, match, first;
>> + u16 sa_idx;
>> + int ret;
>> + int i;
>> +
>> + if (xs->id.proto != IPPROTO_ESP && xs->id.proto != IPPROTO_AH) {
>> + netdev_err(dev, "Unsupported protocol 0x%04x for ipsec offload\n",
>> + xs->id.proto);
>> + return -EINVAL;
>> + }
>> +
>> + if (xs->xso.flags & XFRM_OFFLOAD_INBOUND) {
>> + struct rx_sa rsa;
>> +
>> + if (xs->calg) {
>> + netdev_err(dev, "Compression offload not supported\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* find the first unused index */
>> + ret = ixgbe_ipsec_find_empty_idx(ipsec, true);
>> + if (ret < 0) {
>> + netdev_err(dev, "No space for SA in Rx table!\n");
>> + return ret;
>> + }
>> + sa_idx = (u16)ret;
>> +
>> + memset(&rsa, 0, sizeof(rsa));
>> + rsa.used = true;
>> + rsa.xs = xs;
>> +
>> + if (rsa.xs->id.proto & IPPROTO_ESP)
>> + rsa.decrypt = xs->ealg || xs->aead;
>> +
>> + /* get the key and salt */
>> + ret = ixgbe_ipsec_parse_proto_keys(xs, rsa.key, &rsa.salt);
>> + if (ret) {
>> + netdev_err(dev, "Failed to get key data for Rx SA table\n");
>> + return ret;
>> + }
>> +
>> + /* get ip for rx sa table */
>> + if (xs->xso.flags & XFRM_OFFLOAD_IPV6)
>> + memcpy(rsa.ipaddr, &xs->id.daddr.a6, 16);
>> + else
>> + memcpy(&rsa.ipaddr[3], &xs->id.daddr.a4, 4);
>> +
>> + /* The HW does not have a 1:1 mapping from keys to IP addrs, so
>> + * check for a matching IP addr entry in the table. If the addr
>> + * already exists, use it; else find an unused slot and add the
>> + * addr. If one does not exist and there are no unused table
>> + * entries, fail the request.
>> + */
>> +
>> + /* Find an existing match or first not used, and stop looking
>> + * after we've checked all we know we have.
>> + */
>> + checked = 0;
>> + match = -1;
>> + first = -1;
>> + for (i = 0;
>> + i < IXGBE_IPSEC_MAX_RX_IP_COUNT &&
>> + (checked < ipsec->num_rx_sa || first < 0);
>> + i++) {
>> + if (ipsec->ip_tbl[i].used) {
>> + if (!memcmp(ipsec->ip_tbl[i].ipaddr,
>> + rsa.ipaddr, sizeof(rsa.ipaddr))) {
>> + match = i;
>> + break;
>> + }
>> + checked++;
>> + } else if (first < 0) {
>> + first = i; /* track the first empty seen */
>> + }
>> + }
>> +
>> + if (ipsec->num_rx_sa == 0)
>> + first = 0;
>> +
>> + if (match >= 0) {
>> + /* addrs are the same, we should use this one */
>> + rsa.iptbl_ind = match;
>> + ipsec->ip_tbl[match].ref_cnt++;
>> +
>> + } else if (first >= 0) {
>> + /* no matches, but here's an empty slot */
>> + rsa.iptbl_ind = first;
>> +
>> + memcpy(ipsec->ip_tbl[first].ipaddr,
>> + rsa.ipaddr, sizeof(rsa.ipaddr));
>> + ipsec->ip_tbl[first].ref_cnt = 1;
>> + ipsec->ip_tbl[first].used = true;
>> +
>> + ixgbe_ipsec_set_rx_ip(hw, rsa.iptbl_ind, rsa.ipaddr);
>> +
>> + } else {
>> + /* no match and no empty slot */
>> + netdev_err(dev, "No space for SA in Rx IP SA table\n");
>> + memset(&rsa, 0, sizeof(rsa));
>> + return -ENOSPC;
>> + }
>> +
>> + rsa.mode = IXGBE_RXMOD_VALID;
>> + if (rsa.xs->id.proto & IPPROTO_ESP)
>> + rsa.mode |= IXGBE_RXMOD_PROTO_ESP;
>> + if (rsa.decrypt)
>> + rsa.mode |= IXGBE_RXMOD_DECRYPT;
>> + if (rsa.xs->xso.flags & XFRM_OFFLOAD_IPV6)
>> + rsa.mode |= IXGBE_RXMOD_IPV6;
>> +
>> + /* the preparations worked, so save the info */
>> + memcpy(&ipsec->rx_tbl[sa_idx], &rsa, sizeof(rsa));
>> +
>> + ixgbe_ipsec_set_rx_sa(hw, sa_idx, rsa.xs->id.spi, rsa.key,
>> + rsa.salt, rsa.mode, rsa.iptbl_ind);
>> + xs->xso.offload_handle = sa_idx + IXGBE_IPSEC_BASE_RX_INDEX;
>> +
>> + ipsec->num_rx_sa++;
>> +
>> + /* hash the new entry for faster search in Rx path */
>> + hash_add_rcu(ipsec->rx_sa_list, &ipsec->rx_tbl[sa_idx].hlist,
>> + rsa.xs->id.spi);
>> + } else {
>> + struct tx_sa tsa;
>> +
>> + /* find the first unused index */
>> + ret = ixgbe_ipsec_find_empty_idx(ipsec, false);
>> + if (ret < 0) {
>> + netdev_err(dev, "No space for SA in Tx table\n");
>> + return ret;
>> + }
>> + sa_idx = (u16)ret;
>> +
>> + memset(&tsa, 0, sizeof(tsa));
>> + tsa.used = true;
>> + tsa.xs = xs;
>> +
>> + if (xs->id.proto & IPPROTO_ESP)
>> + tsa.encrypt = xs->ealg || xs->aead;
>> +
>> + ret = ixgbe_ipsec_parse_proto_keys(xs, tsa.key, &tsa.salt);
>> + if (ret) {
>> + netdev_err(dev, "Failed to get key data for Tx SA table\n");
>> + memset(&tsa, 0, sizeof(tsa));
>> + return ret;
>> + }
>> +
>> + /* the preparations worked, so save the info */
>> + memcpy(&ipsec->tx_tbl[sa_idx], &tsa, sizeof(tsa));
>> +
>> + ixgbe_ipsec_set_tx_sa(hw, sa_idx, tsa.key, tsa.salt);
>> +
>> + xs->xso.offload_handle = sa_idx + IXGBE_IPSEC_BASE_TX_INDEX;
>> +
>> + ipsec->num_tx_sa++;
>> + }
>> +
>> + /* enable the engine if not already warmed up */
>> + if (!(adapter->flags2 & IXGBE_FLAG2_IPSEC_ENABLED)) {
>> + ixgbe_ipsec_start_engine(adapter);
>> + adapter->flags2 |= IXGBE_FLAG2_IPSEC_ENABLED;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * ixgbe_ipsec_del_sa - clear out this specific SA
>> + * @xs: pointer to transformer state struct
>> + **/
>> +static void ixgbe_ipsec_del_sa(struct xfrm_state *xs)
>> +{
>> + struct net_device *dev = xs->xso.dev;
>> + struct ixgbe_adapter *adapter = netdev_priv(dev);
>> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
>> + struct ixgbe_hw *hw = &adapter->hw;
>> + u32 zerobuf[4] = {0, 0, 0, 0};
>> + u16 sa_idx;
>> +
>> + if (xs->xso.flags & XFRM_OFFLOAD_INBOUND) {
>> + struct rx_sa *rsa;
>> + u8 ipi;
>> +
>> + sa_idx = xs->xso.offload_handle - IXGBE_IPSEC_BASE_RX_INDEX;
>> + rsa = &ipsec->rx_tbl[sa_idx];
>> +
>> + if (!rsa->used) {
>> + netdev_err(dev, "Invalid Rx SA selected sa_idx=%d offload_handle=%lu\n",
>> + sa_idx, xs->xso.offload_handle);
>> + return;
>> + }
>> +
>> + ixgbe_ipsec_set_rx_sa(hw, sa_idx, 0, zerobuf, 0, 0, 0);
>> + hash_del_rcu(&rsa->hlist);
>> +
>> + /* if the IP table entry is referenced by only this SA,
>> + * i.e. ref_cnt is only 1, clear the IP table entry as well
>> + */
>> + ipi = rsa->iptbl_ind;
>> + if (ipsec->ip_tbl[ipi].ref_cnt > 0) {
>> + ipsec->ip_tbl[ipi].ref_cnt--;
>> +
>> + if (!ipsec->ip_tbl[ipi].ref_cnt) {
>> + memset(&ipsec->ip_tbl[ipi], 0,
>> + sizeof(struct rx_ip_sa));
>> + ixgbe_ipsec_set_rx_ip(hw, ipi, zerobuf);
>> + }
>> + }
>> +
>> + memset(rsa, 0, sizeof(struct rx_sa));
>> + ipsec->num_rx_sa--;
>> + } else {
>> + sa_idx = xs->xso.offload_handle - IXGBE_IPSEC_BASE_TX_INDEX;
>> +
>> + if (!ipsec->tx_tbl[sa_idx].used) {
>> + netdev_err(dev, "Invalid Tx SA selected sa_idx=%d offload_handle=%lu\n",
>> + sa_idx, xs->xso.offload_handle);
>> + return;
>> + }
>> +
>> + ixgbe_ipsec_set_tx_sa(hw, sa_idx, zerobuf, 0);
>> + memset(&ipsec->tx_tbl[sa_idx], 0, sizeof(struct tx_sa));
>> + ipsec->num_tx_sa--;
>> + }
>> +
>> + /* if there are no SAs left, stop the engine to save energy */
>> + if (ipsec->num_rx_sa == 0 && ipsec->num_tx_sa == 0) {
>> + adapter->flags2 &= ~IXGBE_FLAG2_IPSEC_ENABLED;
>> + ixgbe_ipsec_stop_engine(adapter);
>> + }
>> +}
>> +
>> +static const struct xfrmdev_ops ixgbe_xfrmdev_ops = {
>> + .xdo_dev_state_add = ixgbe_ipsec_add_sa,
>> + .xdo_dev_state_delete = ixgbe_ipsec_del_sa,
>> +};
>> +
>> +/**
>> * ixgbe_init_ipsec_offload - initialize security registers for IPSec operation
>> * @adapter: board private structure
>> **/
>> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter)
>> {
>> + struct ixgbe_ipsec *ipsec;
>> + size_t size;
>> +
>> + ipsec = kzalloc(sizeof(*ipsec), GFP_KERNEL);
>> + if (!ipsec)
>> + goto err;
>
> I would say just add another label to skip over the if statement you
> added below.
Yep.
>
>> + hash_init(ipsec->rx_sa_list);
>> +
>> + size = sizeof(struct rx_sa) * IXGBE_IPSEC_MAX_SA_COUNT;
>> + ipsec->rx_tbl = kzalloc(size, GFP_KERNEL);
>> + if (!ipsec->rx_tbl)
>> + goto err;
>> +
>> + size = sizeof(struct tx_sa) * IXGBE_IPSEC_MAX_SA_COUNT;
>> + ipsec->tx_tbl = kzalloc(size, GFP_KERNEL);
>> + if (!ipsec->tx_tbl)
>> + goto err;
>> +
>> + size = sizeof(struct rx_ip_sa) * IXGBE_IPSEC_MAX_RX_IP_COUNT;
>> + ipsec->ip_tbl = kzalloc(size, GFP_KERNEL);
>> + if (!ipsec->ip_tbl)
>> + goto err;
>
> Do all these tables need to be allocated separately? I'm just
> wondering if we can get away with doing something like what we did
> with the ixgbe_q_vector structure where you just allocate this as one
> physical block of memory and just split it up into multiple chunks
> with a separate pointer to each chunk. Doing that would cut down on
> the exception handling needed since it would be a single allocation
> failure you would have to deal with.
This may really just come down to style, and my thoughts around this are
relatively trivial:
- Is it nicer to the memory system to do one large alloc or a couple
of smaller ones?
- If any bounds-checking scans are done, this method would allow for
checking, while I think the single large alloc wouldn't be as good for
bounds checking between these tables.
>
>> + ipsec->num_rx_sa = 0;
>> + ipsec->num_tx_sa = 0;
>> +
>> + adapter->ipsec = ipsec;
>> ixgbe_ipsec_clear_hw_tables(adapter);
>> ixgbe_ipsec_stop_engine(adapter);
By the way, I hink I need to turn these two around and make sure the
engine is stopped first. It just seems right.
>> +
>> + return;
>> +err:
>> + if (ipsec) {
>> + kfree(ipsec->ip_tbl);
>> + kfree(ipsec->rx_tbl);
>> + kfree(ipsec->tx_tbl);
>> + kfree(adapter->ipsec);
>> + }
>> + netdev_err(adapter->netdev, "Unable to allocate memory for SA tables");
>> }
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 51fb3cf..01fd89b 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -10542,6 +10542,12 @@ static void ixgbe_remove(struct pci_dev *pdev)
>> set_bit(__IXGBE_REMOVING, &adapter->state);
>> cancel_work_sync(&adapter->service_task);
>>
>> +#ifdef CONFIG_XFRM
>> + kfree(adapter->ipsec->ip_tbl);
>> + kfree(adapter->ipsec->rx_tbl);
>> + kfree(adapter->ipsec->tx_tbl);
>> + kfree(adapter->ipsec);
>> +#endif /* CONFIG_XFRM */
>
> It might be useful if you were to move this into a function of its
> own. Also you should probably check for adapter->ipsec first,
> otherwise you are going to cause NULL pointer dereference any time
> adapter->ipsec isn't defined. because you are dereferencing it when
> you go to free each of those tables.
Yep
Thanks,
sln
>
>>
>> #ifdef CONFIG_IXGBE_DCA
>> if (adapter->flags & IXGBE_FLAG_DCA_ENABLED) {
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 04/10] ixgbe: add ipsec data structures
From: Shannon Nelson @ 2017-12-07 5:43 UTC (permalink / raw)
To: Alexander Duyck
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <CAKgT0UfR5V9c6VW_b2WzjW0-2XQiw=89pmO0OpXEon71kzPtUw@mail.gmail.com>
On 12/5/2017 9:03 AM, Alexander Duyck wrote:
> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
> <shannon.nelson@oracle.com> wrote:
>> Set up the data structures to be used by the ipsec offload.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 5 ++++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h | 40 ++++++++++++++++++++++++++
>> 2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 1e11462..9487750 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -622,6 +622,7 @@ struct ixgbe_adapter {
>> #define IXGBE_FLAG2_EEE_CAPABLE BIT(14)
>> #define IXGBE_FLAG2_EEE_ENABLED BIT(15)
>> #define IXGBE_FLAG2_RX_LEGACY BIT(16)
>> +#define IXGBE_FLAG2_IPSEC_ENABLED BIT(17)
>>
>> /* Tx fast path data */
>> int num_tx_queues;
>> @@ -772,6 +773,10 @@ struct ixgbe_adapter {
>>
>> #define IXGBE_RSS_KEY_SIZE 40 /* size of RSS Hash Key in bytes */
>> u32 *rss_key;
>> +
>> +#ifdef CONFIG_XFRM
>> + struct ixgbe_ipsec *ipsec;
>> +#endif /* CONFIG_XFRM */
>> };
>>
>> static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
>> index 017b13f..cb9a4be 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
>> @@ -47,4 +47,44 @@
>> #define IXGBE_RXMOD_DECRYPT 0x00000008
>> #define IXGBE_RXMOD_IPV6 0x00000010
>>
>> +struct rx_sa {
>> + struct hlist_node hlist;
>> + struct xfrm_state *xs;
>> + u32 ipaddr[4];
>
> ipaddr should be stored as a __be32, not a u32.
Yep
>
>> + u32 key[4];
>> + u32 salt;
>> + u32 mode;
>> + u8 iptbl_ind;
>> + bool used;
>> + bool decrypt;
>> +};
>> +
>> +struct rx_ip_sa {
>> + u32 ipaddr[4];
>
> Same thing here.
Yep
>
>> + u32 ref_cnt;
>> + bool used;
>> +};
>> +
>> +struct tx_sa {
>> + struct xfrm_state *xs;
>> + u32 key[4];
>> + u32 salt;
>> + bool encrypt;
>> + bool used;
>> +};
>> +
>> +struct ixgbe_ipsec_tx_data {
>> + u32 flags;
>> + u16 trailer_len;
>> + u16 sa_idx;
>> +};
>> +
>> +struct ixgbe_ipsec {
>> + u16 num_rx_sa;
>> + u16 num_tx_sa;
>> + struct rx_ip_sa *ip_tbl;
>> + struct rx_sa *rx_tbl;
>> + struct tx_sa *tx_tbl;
>> + DECLARE_HASHTABLE(rx_sa_list, 8);
>
> The hash table seems a bit on the small side. You might look at
> increasing this to something like 32 in order to try and cut down on
> the load in each bucket since the upper limit is 1K or so isn't it?
I probably can - didn't want to use too much memory real estate if not
really needed. I did a little perf timing with a mostly full table and
didn't see much degradation. Maybe boost it to 16?
sln
>
>> +};
>> #endif /* _IXGBE_IPSEC_H_ */
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 02/10] ixgbe: add ipsec register access routines
From: Shannon Nelson @ 2017-12-07 5:43 UTC (permalink / raw)
To: Alexander Duyck
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <CAKgT0Ufx7m2eTZ8hQOX9dpEaNKozb85axCvS_e7RPAxTsm33MA@mail.gmail.com>
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@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.nelson@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.nics@intel.com>
>> + * e1000-devel Mailing List <e1000-devel@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.
>
> 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.
>
>> +
>> +/**
>> + * 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.
>
>> +/**
>> + * 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@intel.com>
>> + e1000-devel Mailing List <e1000-devel@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-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 03/10] ixgbe: add ipsec engine start and stop routines
From: Shannon Nelson @ 2017-12-07 5:43 UTC (permalink / raw)
To: Alexander Duyck
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <CAKgT0UfJDtGBJEij3TBKAaS_QFukMSiG9TieA49kKXdPwLi9PA@mail.gmail.com>
On 12/5/2017 8:22 AM, Alexander Duyck wrote:
> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
> <shannon.nelson@oracle.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@oracle.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.
Yep - I had meant to limit that. Thanks.
>
>> +
>> + /* 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.
As I responded earlier, the datasheet says to be sure to not change the
EN bit while writing the Rx tables. I'm going to use the same logic on
both Tx and Rx tables.
sln
>
>> +/**
>> * 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@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
From: Sergey Senozhatsky @ 2017-12-07 5:37 UTC (permalink / raw)
To: Tobin C. Harding
Cc: Sergey Senozhatsky, Geert Uytterhoeven, Linus Torvalds,
Randy Dunlap, David Laight, Kees Cook,
kernel-hardening@lists.openwall.com, Jason A. Donenfeld,
Theodore Ts'o, Paolo Bonzini, Tycho Andersen,
Roberts, William C, Tejun Heo, Jordan Glover, Greg KH,
Petr Mladek, Joe Perches, Ian Campbell, Sergey
In-Reply-To: <20171207051711.GB2191@eros>
On (12/07/17 16:17), Tobin C. Harding wrote:
[..]
> > hm, indeed. and !CONFIG_KALLSYMS config turns %pS/%ps
> > into special_hex_number().
>
> But totally misses this :(
>
> "<no-sym>" would be better returned when !CONFIG_KALLSYMS, right?
I guess I'll take back my comment.
I assume there are tons of embedded devices that have !CONFIG_KALLSYMS
in 'release' builds, yet those devices still warn/oops sometimes; having
pointers/hex numbers is really the only way to make any sense out of
backtraces... yet it, basically, means that we are leaking kernel pointers.
-ss
^ permalink raw reply
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
From: Linus Torvalds @ 2017-12-07 5:29 UTC (permalink / raw)
To: tcharding
Cc: Sergey Senozhatsky, Geert Uytterhoeven, Randy Dunlap,
David Laight, Kees Cook, kernel-hardening@lists.openwall.com,
Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini,
Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover,
Greg KH, Petr Mladek, Joe Perches, Ian Campbell,
Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt,
Chris Fries <c
In-Reply-To: <20171207051711.GB2191@eros>
[-- Attachment #1: Type: text/plain, Size: 477 bytes --]
On Dec 6, 2017 21:17, "Tobin C. Harding" <me@tobin.cc> wrote:
> hm, indeed. and !CONFIG_KALLSYMS config turns %pS/%ps
> into special_hex_number().
But totally misses this :(
"<no-sym>" would be better returned when !CONFIG_KALLSYMS, right?
No, we still want those configurations to be debuggable.
If !CONFIG_KALLSYMS, we'll just continue print the hex number. It's not a
normal configuration, leaking an address there is fine, and preferable to
"cannot debug"
Linus
[-- Attachment #2: Type: text/html, Size: 1084 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 1/6] net: sched: sch_api: handle generic qdisc errors
From: David Ahern @ 2017-12-07 5:28 UTC (permalink / raw)
To: Alexander Aring, davem; +Cc: jhs, xiyou.wangcong, jiri, netdev, kernel
In-Reply-To: <20171206160845.6646-2-aring@mojatatu.com>
On 12/6/17 9:08 AM, Alexander Aring wrote:
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index a48ca41b7ecf..7b52a16d2fea 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -449,7 +449,8 @@ static const struct nla_policy stab_policy[TCA_STAB_MAX + 1] = {
> [TCA_STAB_DATA] = { .type = NLA_BINARY },
> };
>
> -static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt)
> +static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt,
> + struct netlink_ext_ack *extack)
> {
> struct nlattr *tb[TCA_STAB_MAX + 1];
> struct qdisc_size_table *stab;
> @@ -458,23 +459,31 @@ static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt)
> u16 *tab = NULL;
> int err;
>
> - err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, NULL);
> - if (err < 0)
> + err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, extack);
> + if (err < 0) {
> + NL_SET_ERR_MSG(extack, "Failed to parse stab netlink msg");
you don't want to set extack here; it should be set in nla_parse_nested
since it is passed as an arg.
> return ERR_PTR(err);
> - if (!tb[TCA_STAB_BASE])
> + }
> + if (!tb[TCA_STAB_BASE]) {
> + NL_SET_ERR_MSG(extack, "Stab base is missing");
"stab base" is the name of the netlink attribute, but it does not
describe *what* is missing. The key here is returning a message that
tells an average user what they did wrong.
> return ERR_PTR(-EINVAL);
> + }
>
> s = nla_data(tb[TCA_STAB_BASE]);
>
> if (s->tsize > 0) {
> - if (!tb[TCA_STAB_DATA])
> + if (!tb[TCA_STAB_DATA]) {
> + NL_SET_ERR_MSG(extack, "Stab data is missing");
ditto here.
Looking at the code for the tc command, stab appears to be table size;
perhaps a tc author can elaborate on what STAB really means.
> return ERR_PTR(-EINVAL);
> + }
> tab = nla_data(tb[TCA_STAB_DATA]);
> tsize = nla_len(tb[TCA_STAB_DATA]) / sizeof(u16);
> }
>
> - if (tsize != s->tsize || (!tab && tsize > 0))
> + if (tsize != s->tsize || (!tab && tsize > 0)) {
> + NL_SET_ERR_MSG(extack, "Invalid data length of stab data");
> return ERR_PTR(-EINVAL);
> + }
>
> list_for_each_entry(stab, &qdisc_stab_list, list) {
> if (memcmp(&stab->szopts, s, sizeof(*s)))
> @@ -486,8 +495,10 @@ static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt)
> }
>
> stab = kmalloc(sizeof(*stab) + tsize * sizeof(u16), GFP_KERNEL);
> - if (!stab)
> + if (!stab) {
> + NL_SET_ERR_MSG(extack, "Failed to allocate memory for stab data");
ENOMEM does not need a text message. Which memory allocation failed is
not really relevant.
> return ERR_PTR(-ENOMEM);
> + }
>
> stab->refcnt = 1;
> stab->szopts = *s;
> @@ -896,7 +907,8 @@ static void notify_and_destroy(struct net *net, struct sk_buff *skb,
>
> static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
> struct sk_buff *skb, struct nlmsghdr *n, u32 classid,
> - struct Qdisc *new, struct Qdisc *old)
> + struct Qdisc *new, struct Qdisc *old,
> + struct netlink_ext_ack *extack)
> {
> struct Qdisc *q = old;
> struct net *net = dev_net(dev);
> @@ -911,8 +923,10 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
> (new && new->flags & TCQ_F_INGRESS)) {
> num_q = 1;
> ingress = 1;
> - if (!dev_ingress_queue(dev))
> + if (!dev_ingress_queue(dev)) {
> + NL_SET_ERR_MSG(extack, "Cannot find ingress queue for specified netdev");
netdev is a kernel term. 'device' is in the tc man page so a relevant
term for the message.
I think the above gives you an idea. Apply those comments to the rest of
this patch and the next ones.
^ permalink raw reply
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
From: Tobin C. Harding @ 2017-12-07 5:17 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Geert Uytterhoeven, Linus Torvalds, Randy Dunlap, David Laight,
Kees Cook, kernel-hardening@lists.openwall.com,
Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini,
Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover,
Greg KH, Petr Mladek, Joe Perches, Ian Campbell,
Sergey Senozhatsky, Catalin
In-Reply-To: <20171206084537.GA462@jagdpanzerIV>
On Wed, Dec 06, 2017 at 05:45:37PM +0900, Sergey Senozhatsky wrote:
> On (12/06/17 09:32), Geert Uytterhoeven wrote:
> [..]
> > >> show_fault_oops(struct pt_regs *regs, unsigned long error_code,
> > >> unsigned long address)
> > >> ...
> > >> printk(KERN_CONT " at %p\n", (void *) address);
> > >> printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip);
> > >
> > > So %pS isn't %p, and shows the symbolic name.
> >
> > If the symbolic name is available.
> > Else it prints the non-hashed pointer value (FTR).
Well, this
[RFC 0/3] kallsyms: don't leak address when printing symbol
_trys_ to fix that
> hm, indeed. and !CONFIG_KALLSYMS config turns %pS/%ps
> into special_hex_number().
But totally misses this :(
"<no-sym>" would be better returned when !CONFIG_KALLSYMS, right?
thanks,
Tobin.
^ permalink raw reply
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
From: Tobin C. Harding @ 2017-12-07 5:12 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Linus Torvalds, Sergey Senozhatsky, Randy Dunlap, David Laight,
Kees Cook, kernel-hardening@lists.openwall.com,
Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini,
Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover,
Greg KH, Petr Mladek, Joe Perches, Ian Campbell,
Sergey Senozhatsky, Cat
In-Reply-To: <CAMuHMdUebiN16BA6qLO+Wv2LetdYgbmd8E_e74hQj52yEUwPOQ@mail.gmail.com>
On Wed, Dec 06, 2017 at 09:32:14AM +0100, Geert Uytterhoeven wrote:
> Hi Linus,
>
> On Wed, Dec 6, 2017 at 2:59 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky
> > <sergey.senozhatsky.work@gmail.com> wrote:
> >> I see some %p-s being used in _supposedly_ important output,
> >> like arch/x86/mm/fault.c
> >>
> >> show_fault_oops(struct pt_regs *regs, unsigned long error_code,
> >> unsigned long address)
> >> ...
> >> printk(KERN_CONT " at %p\n", (void *) address);
> >> printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip);
> >
> > So %pS isn't %p, and shows the symbolic name.
>
> If the symbolic name is available.
> Else it prints the non-hashed pointer value (FTR).
I'm trying to fix this :)
[RFC 0/3] kallsyms: don't leak address when printing symbol
thanks,
Tobin.
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: Disable interrupts if napi_complete_done rescheduled napi
From: Michael S. Tsirkin @ 2017-12-07 5:09 UTC (permalink / raw)
To: Toshiaki Makita; +Cc: netdev, David S . Miller, Eric Dumazet, virtualization
In-Reply-To: <1512620115-7569-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>
On Thu, Dec 07, 2017 at 01:15:15PM +0900, Toshiaki Makita wrote:
> Since commit 39e6c8208d7b ("net: solve a NAPI race") napi has been able
> to be rescheduled within napi_complete_done() even in non-busypoll case,
> but virtnet_poll() always enabled interrupts before complete, and when
> napi was rescheduled within napi_complete_done() it did not disable
> interrupts.
> This caused more interrupts when event idx is disabled.
>
> According to commit cbdadbbf0c79 ("virtio_net: fix race in RX VQ
> processing") we cannot place virtqueue_enable_cb_prepare() after
> NAPI_STATE_SCHED is cleared, so disable interrupts again if
> napi_complete_done() returned false.
>
> Tested with vhost-user of OVS 2.7 on host, which does not have the event
> idx feature.
>
> * Before patch:
>
> $ netperf -t UDP_STREAM -H 192.168.150.253 -l 60 -- -m 1472
> MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.150.253 () port 0 AF_INET
> Socket Message Elapsed Messages
> Size Size Time Okay Errors Throughput
> bytes bytes secs # # 10^6bits/sec
>
> 212992 1472 60.00 32763206 0 6430.32
> 212992 60.00 23384299 4589.56
>
> Interrupts on guest: 9872369
> Packets/interrupt: 2.37
>
> * After patch
>
> $ netperf -t UDP_STREAM -H 192.168.150.253 -l 60 -- -m 1472
> MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.150.253 () port 0 AF_INET
> Socket Message Elapsed Messages
> Size Size Time Okay Errors Throughput
> bytes bytes secs # # 10^6bits/sec
>
> 212992 1472 60.00 32794646 0 6436.49
> 212992 60.00 32793501 6436.27
>
> Interrupts on guest: 4941299
> Packets/interrupt: 6.64
>
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
it might make sense in net and not -next since tx napi regressed performance
in some configs, this might bring it back at least partially.
Jason - what do you think?
> ---
> drivers/net/virtio_net.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 19a985e..c0db48d 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -261,9 +261,12 @@ static void virtqueue_napi_complete(struct napi_struct *napi,
> int opaque;
>
> opaque = virtqueue_enable_cb_prepare(vq);
> - if (napi_complete_done(napi, processed) &&
> - unlikely(virtqueue_poll(vq, opaque)))
> - virtqueue_napi_schedule(napi, vq);
> + if (napi_complete_done(napi, processed)) {
> + if (unlikely(virtqueue_poll(vq, opaque)))
> + virtqueue_napi_schedule(napi, vq);
> + } else {
> + virtqueue_disable_cb(vq);
> + }
> }
>
> static void skb_xmit_done(struct virtqueue *vq)
> --
> 1.8.3.1
>
^ permalink raw reply
* [PATCH net-next] virtio_net: Disable interrupts if napi_complete_done rescheduled napi
From: Toshiaki Makita @ 2017-12-07 4:15 UTC (permalink / raw)
To: David S . Miller, Michael S . Tsirkin, Jason Wang
Cc: Toshiaki Makita, netdev, virtualization, Eric Dumazet
Since commit 39e6c8208d7b ("net: solve a NAPI race") napi has been able
to be rescheduled within napi_complete_done() even in non-busypoll case,
but virtnet_poll() always enabled interrupts before complete, and when
napi was rescheduled within napi_complete_done() it did not disable
interrupts.
This caused more interrupts when event idx is disabled.
According to commit cbdadbbf0c79 ("virtio_net: fix race in RX VQ
processing") we cannot place virtqueue_enable_cb_prepare() after
NAPI_STATE_SCHED is cleared, so disable interrupts again if
napi_complete_done() returned false.
Tested with vhost-user of OVS 2.7 on host, which does not have the event
idx feature.
* Before patch:
$ netperf -t UDP_STREAM -H 192.168.150.253 -l 60 -- -m 1472
MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.150.253 () port 0 AF_INET
Socket Message Elapsed Messages
Size Size Time Okay Errors Throughput
bytes bytes secs # # 10^6bits/sec
212992 1472 60.00 32763206 0 6430.32
212992 60.00 23384299 4589.56
Interrupts on guest: 9872369
Packets/interrupt: 2.37
* After patch
$ netperf -t UDP_STREAM -H 192.168.150.253 -l 60 -- -m 1472
MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.150.253 () port 0 AF_INET
Socket Message Elapsed Messages
Size Size Time Okay Errors Throughput
bytes bytes secs # # 10^6bits/sec
212992 1472 60.00 32794646 0 6436.49
212992 60.00 32793501 6436.27
Interrupts on guest: 4941299
Packets/interrupt: 6.64
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
drivers/net/virtio_net.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 19a985e..c0db48d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -261,9 +261,12 @@ static void virtqueue_napi_complete(struct napi_struct *napi,
int opaque;
opaque = virtqueue_enable_cb_prepare(vq);
- if (napi_complete_done(napi, processed) &&
- unlikely(virtqueue_poll(vq, opaque)))
- virtqueue_napi_schedule(napi, vq);
+ if (napi_complete_done(napi, processed)) {
+ if (unlikely(virtqueue_poll(vq, opaque)))
+ virtqueue_napi_schedule(napi, vq);
+ } else {
+ virtqueue_disable_cb(vq);
+ }
}
static void skb_xmit_done(struct virtqueue *vq)
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 1/1] forcedeth: remove unnecessary variable
From: Zhu Yanjun @ 2017-12-07 4:15 UTC (permalink / raw)
To: netdev, keescook
Since both tx_ring and first_tx are the head of tx ring, it not
necessary to use two variables. So first_tx is removed.
CC: Srinivas Eeda <srinivas.eeda@oracle.com>
CC: Joe Jin <joe.jin@oracle.com>
CC: Junxiao Bi <junxiao.bi@oracle.com>
Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
---
drivers/net/ethernet/nvidia/forcedeth.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index 53614ed..cadea67 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -822,7 +822,7 @@ struct fe_priv {
/*
* tx specific fields.
*/
- union ring_type get_tx, put_tx, first_tx, last_tx;
+ union ring_type get_tx, put_tx, last_tx;
struct nv_skb_map *get_tx_ctx, *put_tx_ctx;
struct nv_skb_map *first_tx_ctx, *last_tx_ctx;
struct nv_skb_map *tx_skb;
@@ -1932,7 +1932,8 @@ static void nv_init_tx(struct net_device *dev)
struct fe_priv *np = netdev_priv(dev);
int i;
- np->get_tx = np->put_tx = np->first_tx = np->tx_ring;
+ np->get_tx = np->tx_ring;
+ np->put_tx = np->tx_ring;
if (!nv_optimized(np))
np->last_tx.orig = &np->tx_ring.orig[np->tx_ring_size-1];
@@ -2248,7 +2249,7 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
offset += bcnt;
size -= bcnt;
if (unlikely(put_tx++ == np->last_tx.orig))
- put_tx = np->first_tx.orig;
+ put_tx = np->tx_ring.orig;
if (unlikely(np->put_tx_ctx++ == np->last_tx_ctx))
np->put_tx_ctx = np->first_tx_ctx;
} while (size);
@@ -2294,13 +2295,13 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
offset += bcnt;
frag_size -= bcnt;
if (unlikely(put_tx++ == np->last_tx.orig))
- put_tx = np->first_tx.orig;
+ put_tx = np->tx_ring.orig;
if (unlikely(np->put_tx_ctx++ == np->last_tx_ctx))
np->put_tx_ctx = np->first_tx_ctx;
} while (frag_size);
}
- if (unlikely(put_tx == np->first_tx.orig))
+ if (unlikely(put_tx == np->tx_ring.orig))
prev_tx = np->last_tx.orig;
else
prev_tx = put_tx - 1;
@@ -2406,7 +2407,7 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff *skb,
offset += bcnt;
size -= bcnt;
if (unlikely(put_tx++ == np->last_tx.ex))
- put_tx = np->first_tx.ex;
+ put_tx = np->tx_ring.ex;
if (unlikely(np->put_tx_ctx++ == np->last_tx_ctx))
np->put_tx_ctx = np->first_tx_ctx;
} while (size);
@@ -2452,13 +2453,13 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff *skb,
offset += bcnt;
frag_size -= bcnt;
if (unlikely(put_tx++ == np->last_tx.ex))
- put_tx = np->first_tx.ex;
+ put_tx = np->tx_ring.ex;
if (unlikely(np->put_tx_ctx++ == np->last_tx_ctx))
np->put_tx_ctx = np->first_tx_ctx;
} while (frag_size);
}
- if (unlikely(put_tx == np->first_tx.ex))
+ if (unlikely(put_tx == np->tx_ring.ex))
prev_tx = np->last_tx.ex;
else
prev_tx = put_tx - 1;
@@ -2597,7 +2598,7 @@ static int nv_tx_done(struct net_device *dev, int limit)
}
}
if (unlikely(np->get_tx.orig++ == np->last_tx.orig))
- np->get_tx.orig = np->first_tx.orig;
+ np->get_tx.orig = np->tx_ring.orig;
if (unlikely(np->get_tx_ctx++ == np->last_tx_ctx))
np->get_tx_ctx = np->first_tx_ctx;
}
@@ -2651,7 +2652,7 @@ static int nv_tx_done_optimized(struct net_device *dev, int limit)
}
if (unlikely(np->get_tx.ex++ == np->last_tx.ex))
- np->get_tx.ex = np->first_tx.ex;
+ np->get_tx.ex = np->tx_ring.ex;
if (unlikely(np->get_tx_ctx++ == np->last_tx_ctx))
np->get_tx_ctx = np->first_tx_ctx;
}
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox