From: Jan Klemkow <jan@openbsd.org>
Subject: Re: ixl(4): add ICAP_VLAN_MTU and NVLAN checks
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Wed, 30 Oct 2024 16:52:33 +0100

On Tue, Oct 29, 2024 at 08:50:40PM GMT, Alexander Bluhm wrote:
> On Fri, Oct 25, 2024 at 10:30:04AM +0200, Jan Klemkow wrote:
> > Hi,
> > 
> > ixl(4) interfaces can handle extra vlan tag bytes in hardware.
> > While here also add missing NVLAN checks for vlan code.
> > 
> > ok?
> 
> I have tested it with and without vlan.  Works.
> 
> Please change the diff as below, IFCAP_VLAN_MTU must also be set
> if NVLAN is not configured.
> 
> -     ifp->if_capabilities = IFCAP_VLAN_HWTAGGING;
> +     ifp->if_capabilities = IFCAP_VLAN_MTU; 
> +#if NVLAN > 0
> +     ifp->if_capabilities |= IFCAP_VLAN_HWTAGGING;
> +#endif
> 
> I encountered the path MTU problem on a production machine where
> etherip(4) was bridged to ixl(4).  No matter whether we have vlan(4)
> support, bridge(4) must handle packets with vlan header correctly.

Yes, but in bridge(4) and all other uses of IFCAP_VLAN_MTU are inside of
#if NVLAN > 0 macros.  So, I think it would be more consistent to put it
inside here as well like in vio(4) and hvn(4).

> with that OK bluhm@
> 
> > Index: dev/pci/if_ixl.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/pci/if_ixl.c,v
> > diff -u -p -r1.101 if_ixl.c
> > --- dev/pci/if_ixl.c	24 May 2024 06:02:53 -0000	1.101
> > +++ dev/pci/if_ixl.c	25 Oct 2024 00:02:28 -0000
> > @@ -49,6 +49,7 @@
> >  
> >  #include "bpfilter.h"
> >  #include "kstat.h"
> > +#include "vlan.h"
> >  
> >  #include <sys/param.h>
> >  #include <sys/systm.h>
> > @@ -1966,7 +1967,11 @@ ixl_attach(struct device *parent, struct
> >  	strlcpy(ifp->if_xname, DEVNAME(sc), IFNAMSIZ);
> >  	ifq_init_maxlen(&ifp->if_snd, sc->sc_tx_ring_ndescs);
> >  
> > -	ifp->if_capabilities = IFCAP_VLAN_HWTAGGING;
> > +	ifp->if_capabilities = 0;
> > +#if NVLAN > 0
> > +	ifp->if_capabilities |= IFCAP_VLAN_MTU;
> > +	ifp->if_capabilities |= IFCAP_VLAN_HWTAGGING;
> > +#endif
> >  	ifp->if_capabilities |= IFCAP_CSUM_IPv4 |
> >  	    IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4 |
> >  	    IFCAP_CSUM_TCPv6 | IFCAP_CSUM_UDPv6;
> > @@ -2806,11 +2811,13 @@ ixl_tx_setup_offload(struct mbuf *m0, st
> >  	uint64_t hlen;
> >  	uint64_t offload = 0;
> >  
> > +#if NVLAN > 0
> >  	if (ISSET(m0->m_flags, M_VLANTAG)) {
> >  		uint64_t vtag = m0->m_pkthdr.ether_vtag;
> >  		offload |= IXL_TX_DESC_CMD_IL2TAG1;
> >  		offload |= vtag << IXL_TX_DESC_L2TAG1_SHIFT;
> >  	}
> > +#endif
> >  
> >  	if (!ISSET(m0->m_pkthdr.csum_flags,
> >  	    M_IPV4_CSUM_OUT|M_TCP_CSUM_OUT|M_UDP_CSUM_OUT|M_TCP_TSO))
> > @@ -3309,11 +3316,13 @@ ixl_rxeof(struct ixl_softc *sc, struct i
> >  					m->m_pkthdr.csum_flags |= M_FLOWID;
> >  				}
> >  
> > +#if NVLAN > 0
> >  				if (ISSET(word, IXL_RX_DESC_L2TAG1P)) {
> >  					m->m_pkthdr.ether_vtag =
> >  					    lemtoh16(&rxd->l2tag1);
> >  					SET(m->m_flags, M_VLANTAG);
> >  				}
> > +#endif
> >  
> >  				ixl_rx_checksum(m, word);
> >  				ml_enqueue(&ml, m);
>