[SLOF] [PATCH slof v3 1/4] pci-phb: Reimplement dma-map-in/out
Michael Roth
mdroth at linux.vnet.ibm.com
Wed Dec 4 08:47:04 AEDT 2019
Quoting Alexey Kardashevskiy (2019-11-20 17:57:58)
> The immediate problem with the code is that it relies on memory allocator
> aligning addresses to the size. This is true for SLOF but not for GRUB
> and in unaligned situations we end up mapping more pages than bm-alloc
> allocated.
>
> This fixes the problem by calculating aligned DMA size before calling
> bm-alloc.
>
> While at this, simplify the code by removing global variables. Also
> replace 1000/fff (the default 4K IOMMU page size) with tce-ps/mask.
>
> Signed-off-by: Alexey Kardashevskiy <aik at ozlabs.ru>
> ---
> board-qemu/slof/pci-phb.fs | 95 ++++++++++++++++----------------------
> 1 file changed, 39 insertions(+), 56 deletions(-)
>
> diff --git a/board-qemu/slof/pci-phb.fs b/board-qemu/slof/pci-phb.fs
> index 06729bcf77a0..be529c9fea0c 100644
> --- a/board-qemu/slof/pci-phb.fs
> +++ b/board-qemu/slof/pci-phb.fs
> @@ -14,6 +14,8 @@
>
> 0 VALUE phb-debug?
>
> +1000 CONSTANT tce-ps \ Default TCE page size is 4K
> +tce-ps 1- CONSTANT tce-mask
>
> ." Populating " pwd cr
>
> @@ -86,17 +88,17 @@ setup-puid
>
> : dma-alloc ( size -- virt )
> phb-debug? IF cr ." dma-alloc called: " .s cr THEN
> - fff + fff not and \ Align size to next 4k boundary
> + tce-ps #aligned
> alloc-mem
> \ alloc-mem always returns aligned memory - double check just to be sure
> - dup fff and IF
> + dup tce-mask and IF
> ." Warning: dma-alloc got unaligned memory!" cr
> THEN
> ;
>
> : dma-free ( virt size -- )
> phb-debug? IF cr ." dma-free called: " .s cr THEN
> - fff + fff not and \ Align size to next 4k boundary
> + tce-ps #aligned
> free-mem
> ;
>
> @@ -107,10 +109,6 @@ setup-puid
> 0 VALUE dma-window-size \ Size of the window
>
> 0 VALUE bm-handle \ Bitmap allocator handle
> -0 VALUE my-virt
> -0 VALUE my-size
> -0 VALUE dev-addr
> -0 VALUE tmp-dev-addr
>
> \ Read helper variables (LIOBN, DMA window base and size) from the
> \ "ibm,dma-window" property. This property can be either located
> @@ -130,11 +128,11 @@ setup-puid
> decode-64 TO dma-window-size
> 2drop
> bm-handle 0= IF
> - dma-window-base dma-window-size 1000 bm-allocator-init to bm-handle
> + dma-window-base dma-window-size tce-ps bm-allocator-init to bm-handle
> \ Sometimes the window-base appears as zero, that does not
> \ go well with NULL pointers. So block this address
> dma-window-base 0= IF
> - bm-handle 1000 bm-alloc drop
> + bm-handle tce-ps bm-alloc drop
> THEN
> THEN
> ;
> @@ -145,69 +143,54 @@ setup-puid
> 0 TO dma-window-size
> ;
>
> -\ We assume that firmware never maps more than the whole dma-window-size
> -\ so we cheat by calculating the remainder of addr/windowsize instead
> -\ of taking care to maintain a list of assigned device addresses
> -: dma-virt2dev ( virt -- devaddr )
> - dma-window-size mod dma-window-base +
> -;
> +\ grub does not align allocated addresses to the size so when mapping,
> +\ we might ask bm-alloc for an extra IOMMU page
"we might need to ask" might be clearer, otherwise it might read as
though the extra IOMMU page is undesirable rather than the correct
behavior.
> +: dma-align ( size virt -- aligned-size ) tce-mask and + tce-ps #aligned ;
> +: dma-trunc ( addr -- addr&~fff) tce-mask not and ;
>
> : dma-map-in ( virt size cachable? -- devaddr )
> phb-debug? IF cr ." dma-map-in called: " .s cr THEN
> (init-dma-window-vars)
> - drop ( virt size )
> -
> - to my-size
> - to my-virt
> - bm-handle my-size bm-alloc
> - to dev-addr
> - dev-addr 0 < IF
> - ." Bitmap allocation Failed " dev-addr .
> - FALSE EXIT
> + drop
> + over dma-align ( virt size ) \ size is aligned now
> + tuck ( size virt size )
there's a mix of tabs and spaces here ^ for alignment and it gets messy
with anything other than tab-width=8. The other comments in this file just
use spaces for alignment, maybe we should do the same?
> + bm-handle swap bm-alloc ( size virt dev-addr ) \ dev-addr is aligned
> + dup 0 < IF
> + ." Bitmap allocation Failed "
> + 2drop
Wouldn't this leave 'size' on the stack?
> + 0 EXIT
> THEN
> - dev-addr to tmp-dev-addr
>
> - my-virt my-size
> - bounds dup >r ( v+s virt R: virt )
> - swap fff + fff not and \ Align end to next 4k boundary
> - swap fff not and ( v+s' virt' R: virt )
> + swap ( size dev-addr virt )
> + 2dup tce-mask and or >r \ add page offset to the return value
> +
> + dma-trunc 3 OR \ Truncate and add read and write perm
> + rot ( virt dev-addr size r: dev-addr )
Isn't the stack now (dev-addr virt size r: dev-addr) ? The following
loop seems to assume that as well.
> + 0
> ?DO
> - \ ." mapping " i . cr
> - dma-window-liobn \ liobn
> - tmp-dev-addr \ ioba
> - i 3 OR \ Make a read- & writeable TCE
> - ( liobn ioba tce R: virt )
> + 2dup dma-window-liobn -rot
Maybe not needed but the following comments might be helpful:
( dev-addr virt liobn dev-addr virt r: dev-addr )
> hv-put-tce ABORT" H_PUT_TCE failed"
> - tmp-dev-addr 1000 + to tmp-dev-addr
> - 1000 +LOOP
> - r> drop
> - my-virt FFF and dev-addr or
> + tce-ps + swap tce-ps + swap
( dev-addr' virt' r: dev-addr )
Looks good otherwise.
> + tce-ps +LOOP
> (clear-dma-window-vars)
> + 2drop
> + r>
> ;
>
> : dma-map-out ( virt devaddr size -- )
> phb-debug? IF cr ." dma-map-out called: " .s cr THEN
> (init-dma-window-vars)
> - to my-size
> - to dev-addr
> - to my-virt
> - dev-addr fff not and to dev-addr
> - dev-addr to tmp-dev-addr
> -
> - my-virt my-size ( virt size )
> - bounds ( v+s virt )
> - swap fff + fff not and \ Align end to next 4k boundary
> - swap fff not and ( v+s' virt' )
> + rot drop ( devaddr size )
> + over dma-align
> + swap dma-trunc swap ( devaddr-trunc size-extended )
> + 2dup bm-handle -rot bm-free
> + 0
> ?DO
> - \ ." unmapping " i . cr
> - dma-window-liobn \ liobn
> - tmp-dev-addr \ ioba
> - i \ Lowest bits not set => invalid TCE
> - ( liobn ioba tce )
> + dup 0 dma-window-liobn -rot
> hv-put-tce ABORT" H_PUT_TCE failed"
> - tmp-dev-addr 1000 + to tmp-dev-addr
> - 1000 +LOOP
> - bm-handle dev-addr my-size bm-free
> + tce-ps +
> + tce-ps +LOOP
> + drop
> (clear-dma-window-vars)
> ;
>
> --
> 2.17.1
>
> _______________________________________________
> SLOF mailing list
> SLOF at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/slof
More information about the SLOF
mailing list