Re: kern/58049: vioif(4) panics if the virtq size is too small

[Tetsuya Isaki <isaki%pastel-flower.jp@localhost>]

The following reply was made to PR kern/58049; it has been noted by GNATS.

From: Tetsuya Isaki <isaki%pastel-flower.jp@localhost>
To: Jason Thorpe <thorpej%me.com@localhost>
Cc: kern-bug-people%netbsd.org@localhost,
	gnats-admin%netbsd.org@localhost,
	netbsd-bugs%netbsd.org@localhost,
	gnats-bugs%netbsd.org@localhost
Subject: Re: kern/58049: vioif(4) panics if the virtq size is too small
Date: Wed, 20 Mar 2024 12:44:25 +0900

 At Mon, 18 Mar 2024 08:51:45 -0700,
 Jason Thorpe wrote:
 > When the DMA map is created, do we not yet know the max number of
 > segments?  Or is the problem that different queues might have
 > different segment counts and the DMA maps are shared between them?
 > In a perfect world, this extra test isn't needed because the bus_dma
 > back-end honors the device's maximum segment count.
 
 Thank you for comment.
 I have found the cause of this problem.  This is the revised patch.
 
 --- src/sys/dev/pci/if_vioif.c
 +++ src/sys/dev/pci/if_vioif.c
 @@ -1280,12 +1280,14 @@ vioif_alloc_mems(struct vioif_softc *sc)
  
  		struct virtio_net_hdr *hdrs;
  		int dir;
 +		int nsegs;
  
  		dir = VIOIF_NETQ_DIR(qid);
  		netq = &sc->sc_netqs[qid];
  		vq_num = netq->netq_vq->vq_num;
  		maps = netq->netq_maps;
  		hdrs = netq->netq_maps_kva;
 +		nsegs = uimin(dmaparams[dir].dma_nsegs, vq_num - 1/*hdr*/);
  
  		for (i = 0; i < vq_num; i++) {
  			maps[i].vnm_hdr = &hdrs[i];
 @@ -1297,7 +1299,7 @@ vioif_alloc_mems(struct vioif_softc *sc)
  				goto err_reqs;
  
  			r = vioif_dmamap_create(sc, &maps[i].vnm_mbuf_map,
 -			    dmaparams[dir].dma_size, dmaparams[dir].dma_nsegs,
 +			    dmaparams[dir].dma_size, nsegs,
  			    dmaparams[dir].msg_payload);
  			if (r != 0)
  				goto err_reqs;
 
 
 vioif_net_load_mbuf() (in vioif_send_common_locked()) loaded more
 than 8 segments(fragments) despite (my) virtio queue size was 8.
 So I tried to defragment in the previous patch.  But it was not
 correct as you pointed out.
 
 This is because vioif_alloc_mems() told bus_dma that it was able to
 handle up to hardcoded VIRTIO_NET_TX_MAXNSEGS (=16) segments during
 initialization, despite my virtio queue size was 8(!).
 If the virtq size is less than or equal to VIRTIO_NET_TX_MAXNSEGS,
 the max number of dmamap segments for payload must be virtq size - 1.
 
 Thanks,
 ---
 Tetsuya Isaki <isaki%pastel-flower.jp@localhost / isaki%NetBSD.org@localhost>