Subject: Re: NetBSD/xen network problems (need help)
To: Mike M. Volokhov <mishka@intostroy.com>
From: Manuel Bouyer <bouyer@antioche.eu.org>
List: port-xen
Date: 01/26/2006 20:00:11
--7JfCtLOvnd9MIVvH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Jan 26, 2006 at 05:24:54PM +0200, Mike M. Volokhov wrote:
> > Here's a new patch which adds more debug printf.
> 
> Kernel panic message:
> 
> pa 0x5fd5f38 len 200 m_flags 0x2
> new_m 0xc06bbf00 m_paddr 5fd5f00 M_BUFOFFSET 56

Hey, of course ! 200 + 56 = 256 = 0x100. The KASSERT is wrong, it should
use m_pkthdr.len - 1.

Attached is a new patch for if_xennet.c, with this fixed (but still the
additionnal debug printfs, just in case).

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

--7JfCtLOvnd9MIVvH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=diff

Index: if_xennet.c
===================================================================
RCS file: /cvsroot/src/sys/arch/xen/xen/if_xennet.c,v
retrieving revision 1.13.2.19
diff -u -r1.13.2.19 if_xennet.c
--- if_xennet.c	22 Jan 2006 10:41:59 -0000	1.13.2.19
+++ if_xennet.c	26 Jan 2006 18:58:31 -0000
@@ -638,6 +638,7 @@
 	mmu_update_t *mmu = rx_mmu;
 	multicall_entry_t *mcl = rx_mcl;
 	struct mbuf *m;
+	void *pktp;
 
 	network_tx_buf_gc(sc);
 	ifp->if_flags &= ~IFF_OACTIVE;
@@ -660,7 +661,7 @@
 		/* XXXcl check rx->status for error */
 
                 MGETHDR(m, M_DONTWAIT, MT_DATA);
-                if (m == NULL) {
+                if (__predict_false(m == NULL)) {
 			printf("xennet: rx no mbuf\n");
 			break;
 		}
@@ -687,14 +688,12 @@
 
 		/* Do all the remapping work, and M->P updates, in one
 		 * big hypercall. */
-		if ((mcl - rx_mcl) != 0) {
-			mcl->op = __HYPERVISOR_mmu_update;
-			mcl->args[0] = (unsigned long)rx_mmu;
-			mcl->args[1] = mmu - rx_mmu;
-			mcl->args[2] = 0;
-			mcl++;
-			(void)HYPERVISOR_multicall(rx_mcl, mcl - rx_mcl);
-		}
+		mcl->op = __HYPERVISOR_mmu_update;
+		mcl->args[0] = (unsigned long)rx_mmu;
+		mcl->args[1] = mmu - rx_mmu;
+		mcl->args[2] = 0;
+		mcl++;
+		(void)HYPERVISOR_multicall(rx_mcl, mcl - rx_mcl);
 		if (0)
 		printf("page mapped at va %08lx -> %08x/%08lx\n",
 		    sc->sc_rx_bufa[rx->id].xb_rx.xbrx_va,
@@ -711,11 +710,23 @@
 		    (void *)(PTE_BASE[x86_btop
 			(sc->sc_rx_bufa[rx->id].xb_rx.xbrx_va)] & PG_FRAME)));
 
+		pktp = (void *)(sc->sc_rx_bufa[rx->id].xb_rx.xbrx_va +
+		    (rx->addr & PAGE_MASK));
+		if ((ifp->if_flags & IFF_PROMISC) == 0) {
+			struct ether_header *eh = pktp;
+			if (ETHER_IS_MULTICAST(eh->ether_dhost) == 0 &&
+			    memcmp(LLADDR(ifp->if_sadl), eh->ether_dhost,
+			    ETHER_ADDR_LEN) != 0) {
+				m_freem(m);
+				xennet_rx_push_buffer(sc, rx->id);
+				continue; /* packet not for us */
+			}
+		}
 		m->m_pkthdr.rcvif = ifp;
-		if (sc->sc_rx->req_prod != sc->sc_rx->resp_prod) {
+		if (__predict_true(
+		    sc->sc_rx->req_prod != sc->sc_rx->resp_prod)) {
 			m->m_len = m->m_pkthdr.len = rx->status;
-			MEXTADD(m, (void *)(sc->sc_rx_bufa[rx->id].xb_rx.
-			    xbrx_va + (rx->addr & PAGE_MASK)), rx->status,
+			MEXTADD(m, pktp, rx->status,
 			    M_DEVBUF, xennet_rx_mbuf_free,
 			    &sc->sc_rx_bufa[rx->id]);
 		} else {
@@ -726,14 +737,13 @@
 			 */
 			m->m_len = MHLEN;
 			m->m_pkthdr.len = 0;
-			m_copyback(m, 0, rx->status, 
-			    (caddr_t)(sc->sc_rx_bufa[rx->id].xb_rx.xbrx_va +
-			    (rx->addr & PAGE_MASK)));
+			m_copyback(m, 0, rx->status, pktp);
 			xennet_rx_push_buffer(sc, rx->id);
 			if (m->m_pkthdr.len < rx->status) {
+				/* out of memory, just drop packets */
 				ifp->if_ierrors++;
 				m_freem(m);
-				break;
+				continue;
 			}
 		}
 
@@ -989,7 +999,7 @@
 		}
 
 		if (m->m_pkthdr.len != m->m_len ||
-		    (pa ^ (pa + m->m_pkthdr.len)) & PG_FRAME) {
+		    (pa ^ (pa + m->m_pkthdr.len - 1)) & PG_FRAME) {
 
 			MGETHDR(new_m, M_DONTWAIT, MT_DATA);
 			if (new_m == NULL) {
@@ -1009,22 +1019,43 @@
 			m_copydata(m, 0, m->m_pkthdr.len, mtod(new_m, caddr_t));
 			new_m->m_len = new_m->m_pkthdr.len = m->m_pkthdr.len;
 
-			m_freem(m);
-			m = new_m;
-			if ((m->m_flags & M_EXT) != 0) {
-				pa = m->m_ext.ext_paddr;
-				KASSERT(m->m_data == m->m_ext.ext_buf);
+			if ((new_m->m_flags & M_EXT) != 0) {
+				pa = new_m->m_ext.ext_paddr;
+				KASSERT(new_m->m_data == new_m->m_ext.ext_buf);
 				KASSERT(pa != M_PADDR_INVALID);
 			} else {
-				pa = m->m_paddr;
+				pa = new_m->m_paddr;
 				KASSERT(pa != M_PADDR_INVALID);
-				KASSERT(m->m_data == M_BUFADDR(m));
-				pa += M_BUFOFFSET(m);
+				KASSERT(new_m->m_data == M_BUFADDR(new_m));
+				pa += M_BUFOFFSET(new_m);
+			}
+			if (((pa ^ (pa + new_m->m_pkthdr.len - 1)) & PG_FRAME) != 0) {
+				printf("pa %p len %d m_flags 0x%x\n",
+				    (void *)pa, new_m->m_pkthdr.len, new_m->m_flags);
+				printf("new_m %p m_paddr %lx M_BUFOFFSET %d\n",
+				   new_m,
+				   ((new_m->m_flags & M_EXT) != 0) ?
+				       new_m->m_ext.ext_paddr : new_m->m_paddr,
+				   ((new_m->m_flags & M_EXT) != 0) ?
+				   0 : M_BUFOFFSET(new_m));
+				panic("xennet_start1: buffer crosses page");
 			}
+			m_freem(m);
+			m = new_m;
 		} else
 			IFQ_DEQUEUE(&ifp->if_snd, m);
 
-		KASSERT(((pa ^ (pa + m->m_pkthdr.len)) & PG_FRAME) == 0);
+		if (((pa ^ (pa + m->m_pkthdr.len - 1)) & PG_FRAME) != 0) {
+			printf("pa %p len %d m_flags 0x%x\n",
+			    (void *)pa, m->m_pkthdr.len, m->m_flags);
+			printf("m %p m_paddr %lx M_BUFOFFSET %d\n",
+			   m,
+			   ((m->m_flags & M_EXT) != 0) ?
+			       m->m_ext.ext_paddr : m->m_paddr,
+			   ((m->m_flags & M_EXT) != 0) ?
+			   0 : M_BUFOFFSET(m));
+			panic("xennet_start2: buffer crosses page");
+		}
 
 		bufid = get_bufarray_entry(sc->sc_tx_bufa);
 		KASSERT(bufid < NETIF_TX_RING_SIZE);

--7JfCtLOvnd9MIVvH--