From: Hans-Jörg Höxer <Hans-Joerg_Hoexer@genua.de>
Subject: Re: bus dma bounce buffer contiguous virtual address
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: <Hans-Joerg_Hoexer@genua.de>
Date: Thu, 22 Aug 2024 12:35:34 +0200

Hi,

On Wed, Aug 21, 2024 at 07:21:31PM +0200, Alexander Bluhm wrote:
> Hi,
> 
> Together with hshoexer@ we found another bug in the bus dma bounce
> buffer implementation.  If the physical pages are contiguous,
> _bus_dmamap_load_buffer() tries to merge the segments.  In case of
> mbuf chains, it can happen that the physical bounce buffers are
> contiguous, but the virtual addresses the mbuf m_data are not.  Then
> during transmit _bus_dmamap_sync() tries to copy segments where it
> cannot access the virtual source address which is mapped in a
> different mbuf.  So if bounce buffers are used, physical and virtual
> buffer must be contigous, to merge a segment.
> 
> While there split the check and decrement of variable i in the for
> loop to make the code readable.
> 
> ok?

ok hshoexer

>
> 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.56 bus_dma.c
> --- arch/amd64/amd64/bus_dma.c	20 Aug 2024 15:30:29 -0000	1.56
> +++ arch/amd64/amd64/bus_dma.c	21 Aug 2024 16:37:31 -0000
> @@ -499,7 +499,7 @@ _bus_dmamap_sync(bus_dma_tag_t t, bus_dm
>  	if (!use_bounce_buffer)
>  		return;
>  
> -	for (i = map->_dm_segcnt, sg = map->dm_segs; size && i--; sg++) {
> +	for (i = map->_dm_segcnt, sg = map->dm_segs; size && i; i--, sg++) {
>  		if (off >= sg->ds_len) {
>  			off -= sg->ds_len;
>  			continue;
> @@ -767,9 +767,11 @@ _bus_dmamap_load_buffer(bus_dma_tag_t t,
>  			     map->_dm_maxsegsz &&
>  			    (map->_dm_boundary == 0 ||
>  			     (map->dm_segs[seg].ds_addr & bmask) ==
> -			     (curaddr & bmask)))
> +			     (curaddr & bmask)) &&
> +			    (!use_bounce_buffer || (map->dm_segs[seg]._ds_va +
> +			     map->dm_segs[seg].ds_len) == vaddr)) {
>  				map->dm_segs[seg].ds_len += sgsize;
> -			else {
> +			} else {
>  				if (++seg >= map->_dm_segcnt)
>  					break;
>  				map->dm_segs[seg].ds_addr = curaddr;
>