On Wed, Oct 30, 2024 at 04:52:33PM +0100, Jan Klemkow wrote:
> 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).

Maybe that is suboptimal in bridge(4).  The hardware capability is
independent of the kernel config.  Look at em(4) and ix(4), there
it IFCAP_VLAN_MTU is outside of #if NVLAN > 0.  I think this is
better.  And it was that way in if_ixl.c rev 1.79.

bluhm

> > 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);
> >