Re: [PATCH net-next V2 1/2] net/mlx5e: Support routed networks during IPsec MACs initialization

[Leon Romanovsky]

On Wed, Jul 23, 2025 at 08:52:09AM +0200, Michal Swiatkowski wrote:
> On Tue, Jul 22, 2025 at 05:23:47PM +0300, Tariq Toukan wrote:
> > From: Alexandre Cassen <acassen@xxxxxxxxxxxx>
> > 
> > Remote IPsec tunnel endpoint may refer to a network segment that is
> > not directly connected to the host. In such a case, IPsec tunnel
> > endpoints are connected to a router and reachable via a routing path.
> > In IPsec packet offload mode, HW is initialized with the MAC address
> > of both IPsec tunnel endpoints.
> > 
> > Extend the current IPsec init MACs procedure to resolve nexthop for
> > routed networks. Direct neighbour lookup and probe is still used
> > for directly connected networks and as a fallback mechanism if fib
> > lookup fails.
> > 
> > Signed-off-by: Alexandre Cassen <acassen@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxx>
> > Reviewed-by: Cosmin Ratiu <cratiu@xxxxxxxxxx>
> > Signed-off-by: Tariq Toukan <tariqt@xxxxxxxxxx>
> > ---
> >  .../mellanox/mlx5/core/en_accel/ipsec.c       | 82 ++++++++++++++++++-
> >  1 file changed, 80 insertions(+), 2 deletions(-)

<...>

> > @@ -274,18 +281,89 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
> >  	case XFRM_DEV_OFFLOAD_IN:
> >  		src = attrs->dmac;
> >  		dst = attrs->smac;
> > -		pkey = &attrs->addrs.saddr.a4;
> > +
> > +		switch (addrs->family) {
> > +		case AF_INET:
> > +			fl4.flowi4_proto = x->sel.proto;
> > +			fl4.daddr = addrs->saddr.a4;
> > +			fl4.saddr = addrs->daddr.a4;
> > +			pkey = &addrs->saddr.a4;
> > +			break;
> > +		case AF_INET6:
> > +			fl6.flowi6_proto = x->sel.proto;
> > +			memcpy(fl6.daddr.s6_addr32, addrs->saddr.a6, 16);
> > +			memcpy(fl6.saddr.s6_addr32, addrs->daddr.a6, 16);
> > +			pkey = &addrs->saddr.a6;
> > +			break;
> > +		default:
> > +			return;
> > +		}
> >  		break;
> >  	case XFRM_DEV_OFFLOAD_OUT:
> >  		src = attrs->smac;
> >  		dst = attrs->dmac;
> > -		pkey = &attrs->addrs.daddr.a4;
> 
> Isn't it worth to move getting pkey to separate function? The switch is
> the same with OFFLOAD_IN and OFFLOAD_OUT.

The content of the switch is completely opposite, as an example for pkey:
In OFFLOAD_IN:
pkey = &addrs->...S...addr.a4;
pkey = &addrs->...S...addr.a6;

In OFFLOAD_OUT:
pkey = &addrs->...D...addr.a4;
pkey = &addrs->...D...addr.a6;

There is only one line which is common: "... = x->sel.proto;"

> 
> > +		switch (addrs->family) {
> > +		case AF_INET:
> > +			fl4.flowi4_proto = x->sel.proto;
> > +			fl4.daddr = addrs->daddr.a4;
> > +			fl4.saddr = addrs->saddr.a4;
> > +			pkey = &addrs->daddr.a4;
> > +			break;
> > +		case AF_INET6:
> > +			fl6.flowi6_proto = x->sel.proto;
> > +			memcpy(fl6.daddr.s6_addr32, addrs->daddr.a6, 16);
> > +			memcpy(fl6.saddr.s6_addr32, addrs->saddr.a6, 16);
> > +			pkey = &addrs->daddr.a6;
> > +			break;
> > +		default:
> > +			return;

<...>

> >  	n = neigh_lookup(&arp_tbl, pkey, netdev);
> >  	if (!n) {
> >  		n = neigh_create(&arp_tbl, pkey, netdev);
> 
> Code looks fine,
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@xxxxxxxxxxxxxxx>

Thanks

> 
> > -- 
> > 2.31.1
>