From: Hans-Jörg Höxer <Hans-Joerg_Hoexer@genua.de>
Subject: Re: bus dma bounce buffer pages used
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: <tech@openbsd.org>, <Hans-Joerg_Hoexer@genua.de>
Date: Tue, 20 Aug 2024 13:11:11 +0200

Hi,

On Mon, Aug 19, 2024 at 06:35:26PM +0200, Alexander Bluhm wrote:
> Hi,
> 
> There is an off-by-one bug when comparing the used pages for bounce
> buffers with the available pages.  So _bus_dmamap_load_buffer()
> returns ENOMEM although there is one buffer left.
> 
> Also the _dm_nused field is updated and never reset in case of an
> error.  Use a local variable to count the used pages and update
> global map->_dm_nused only if there was no error.
> 
> This fixes a hanging network if bounce buffers are enforced for
> vio(4).
> 
> ok?

both fixes are ok.

> bluhm
> 
> Index: arch/amd64/amd64/bus_dma.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/arch/amd64/amd64/bus_dma.c,v
> diff -u -p -r1.53 bus_dma.c
> --- arch/amd64/amd64/bus_dma.c	18 Aug 2024 21:04:29 -0000	1.53
> +++ arch/amd64/amd64/bus_dma.c	19 Aug 2024 16:10:56 -0000
> @@ -102,7 +102,7 @@
>  #endif
>  
>  int _bus_dmamap_load_buffer(bus_dma_tag_t, bus_dmamap_t, void *, bus_size_t,
> -    struct proc *, int, paddr_t *, int *, int);
> +    struct proc *, int, paddr_t *, int *, int *, int);
>  
>  /*
>   * Common function for DMA map creation.  May be called by bus-specific
> @@ -251,7 +251,7 @@ _bus_dmamap_load(bus_dma_tag_t t, bus_dm
>      bus_size_t buflen, struct proc *p, int flags)
>  {
>  	bus_addr_t lastaddr = 0;
> -	int seg, error;
> +	int seg, used, error;
>  
>  	/*
>  	 * Make sure that on error condition we return "no valid mappings".
> @@ -263,11 +263,13 @@ _bus_dmamap_load(bus_dma_tag_t t, bus_dm
>  		return (EINVAL);
>  
>  	seg = 0;
> +	used = 0;
>  	error = _bus_dmamap_load_buffer(t, map, buf, buflen, p, flags,
> -	    &lastaddr, &seg, 1);
> +	    &lastaddr, &seg, &used, 1);
>  	if (error == 0) {
>  		map->dm_mapsize = buflen;
>  		map->dm_nsegs = seg + 1;
> +		map->_dm_nused = used;
>  	}
>  	return (error);
>  }
> @@ -280,7 +282,7 @@ _bus_dmamap_load_mbuf(bus_dma_tag_t t, b
>      int flags)
>  {
>  	paddr_t lastaddr = 0;
> -	int seg, error, first;
> +	int seg, used, error, first;
>  	struct mbuf *m;
>  
>  	/*
> @@ -299,17 +301,19 @@ _bus_dmamap_load_mbuf(bus_dma_tag_t t, b
>  
>  	first = 1;
>  	seg = 0;
> +	used = 0;
>  	error = 0;
>  	for (m = m0; m != NULL && error == 0; m = m->m_next) {
>  		if (m->m_len == 0)
>  			continue;
>  		error = _bus_dmamap_load_buffer(t, map, m->m_data, m->m_len,
> -		    NULL, flags, &lastaddr, &seg, first);
> +		    NULL, flags, &lastaddr, &seg, &used, first);
>  		first = 0;
>  	}
>  	if (error == 0) {
>  		map->dm_mapsize = m0->m_pkthdr.len;
>  		map->dm_nsegs = seg + 1;
> +		map->_dm_nused = used;
>  	}
>  	return (error);
>  }
> @@ -322,7 +326,7 @@ _bus_dmamap_load_uio(bus_dma_tag_t t, bu
>      int flags)
>  {
>  	paddr_t lastaddr = 0;
> -	int seg, i, error, first;
> +	int seg, used, i, error, first;
>  	bus_size_t minlen, resid;
>  	struct proc *p = NULL;
>  	struct iovec *iov;
> @@ -347,6 +351,7 @@ _bus_dmamap_load_uio(bus_dma_tag_t t, bu
>  
>  	first = 1;
>  	seg = 0;
> +	used = 0;
>  	error = 0;
>  	for (i = 0; i < uio->uio_iovcnt && resid != 0 && error == 0; i++) {
>  		/*
> @@ -357,7 +362,7 @@ _bus_dmamap_load_uio(bus_dma_tag_t t, bu
>  		addr = (caddr_t)iov[i].iov_base;
>  
>  		error = _bus_dmamap_load_buffer(t, map, addr, minlen,
> -		    p, flags, &lastaddr, &seg, first);
> +		    p, flags, &lastaddr, &seg, &used, first);
>  		first = 0;
>  
>  		resid -= minlen;
> @@ -365,6 +370,7 @@ _bus_dmamap_load_uio(bus_dma_tag_t t, bu
>  	if (error == 0) {
>  		map->dm_mapsize = uio->uio_resid;
>  		map->dm_nsegs = seg + 1;
> +		map->_dm_nused = used;
>  	}
>  	return (error);
>  }
> @@ -683,8 +689,8 @@ _bus_dmamem_mmap(bus_dma_tag_t t, bus_dm
>   */
>  int
>  _bus_dmamap_load_buffer(bus_dma_tag_t t, bus_dmamap_t map, void *buf,
> -    bus_size_t buflen, struct proc *p, int flags, paddr_t *lastaddrp, int *segp,
> -    int first)
> +    bus_size_t buflen, struct proc *p, int flags, paddr_t *lastaddrp,
> +    int *segp, int *usedp, int first)
>  {
>  	bus_size_t sgsize;
>  	bus_addr_t curaddr, lastaddr, baddr, bmask;
> @@ -699,6 +705,7 @@ _bus_dmamap_load_buffer(bus_dma_tag_t t,
>  	else
>  		pmap = pmap_kernel();
>  
> +	page = *usedp;
>  	lastaddr = *lastaddrp;
>  	bmask  = ~(map->_dm_boundary - 1);
>  
> @@ -714,14 +721,14 @@ _bus_dmamap_load_buffer(bus_dma_tag_t t,
>  			    curaddr);
>  
>  		if (use_bounce_buffer) {
> -			if (map->_dm_nused + 1 >= map->_dm_npages)
> +			if (page >= map->_dm_npages)
>  				return (ENOMEM);
>  
>  			off = vaddr & PAGE_MASK;
> -			pg = map->_dm_pages[page = map->_dm_nused++];
> +			pg = map->_dm_pages[page];
>  			curaddr = VM_PAGE_TO_PHYS(pg) + off;
> -
>  			pgva = map->_dm_pgva + (page << PGSHIFT) + off;
> +			page++;
>  		}
>  
>  		/*
> @@ -774,6 +781,7 @@ _bus_dmamap_load_buffer(bus_dma_tag_t t,
>  	}
>  
>  	*segp = seg;
> +	*usedp = page;
>  	*lastaddrp = lastaddr;
>  
>  	/*
>