| Message ID | 20191114033341.17111-1-aik@ozlabs.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pci-phb: Reimplement dma-map-in/out | expand |
Hi! On Thu, Nov 14, 2019 at 02:33:41PM +1100, Alexey Kardashevskiy wrote: > +: align-4k > + fff not and > +; This is aligning down. "align" means "align upwards" normally. The word "align" aligns the data space, while "aligned" aligns a pointer. SLOF also has a utility word "#aligned" to align to some certain boundary. So you would do : align-4k ( a - a ) 1000 #aligned ; but that sligns *upwards*. : trunc-4k ( a - a ) -1000 and ; if you want a lower address, instead (less often useful, btw). (OF always is two's complement, you can write -1000 instead of fff invert). > +\ grub does not align allocated addresses to the size > +\ so when mapping in dma-map-in, sometime we call H_PUT_TCE more than > +\ bm-alloc allocated > +: dma-align ( virt size -- virt aligned-size ) > + over fff and + fff + fff not and > +; : dma-align ( virt size -- virt a-size ) over fff and + 1000 @aligned ; fff + fff not and is just 1000 #aligned . I don't understand why you are adding the low twelve bits of the address to the size as well? Segher
On 14/11/2019 22:47, Segher Boessenkool wrote: > Hi! > > On Thu, Nov 14, 2019 at 02:33:41PM +1100, Alexey Kardashevskiy wrote: >> +: align-4k >> + fff not and >> +; > > This is aligning down. "align" means "align upwards" normally. The word > "align" aligns the data space, while "aligned" aligns a pointer. SLOF also > has a utility word "#aligned" to align to some certain boundary. So you > would do > > : align-4k ( a - a ) 1000 #aligned ; > > but that sligns *upwards*. > > : trunc-4k ( a - a ) -1000 and ; > > if you want a lower address, instead (less often useful, btw). > > (OF always is two's complement, you can write -1000 instead of fff invert). I am really not used to "-1000" :-/ > >> +\ grub does not align allocated addresses to the size >> +\ so when mapping in dma-map-in, sometime we call H_PUT_TCE more than >> +\ bm-alloc allocated >> +: dma-align ( virt size -- virt aligned-size ) >> + over fff and + fff + fff not and >> +; > > : dma-align ( virt size -- virt a-size ) over fff and + 1000 @aligned ; > > fff + fff not and is just 1000 #aligned . > > I don't understand why you are adding the low twelve bits of the address > to the size as well? Because everything from the beginning of 4K chunk need to be included into the DMA mapping - the IOMMU pages are 4K aligned.
On Fri, Nov 15, 2019 at 10:33:58AM +1100, Alexey Kardashevskiy wrote: > On 14/11/2019 22:47, Segher Boessenkool wrote: > > : trunc-4k ( a - a ) -1000 and ; > > > > if you want a lower address, instead (less often useful, btw). > > > > (OF always is two's complement, you can write -1000 instead of fff invert). > > I am really not used to "-1000" :-/ -N reads a bit nicer than ~(N-1), but it is the same thing. I find it easier to read -1000 and instead of fff invert and but that is for a big part just what you are used to. > >> +\ grub does not align allocated addresses to the size > >> +\ so when mapping in dma-map-in, sometime we call H_PUT_TCE more than > >> +\ bm-alloc allocated > >> +: dma-align ( virt size -- virt aligned-size ) > >> + over fff and + fff + fff not and > >> +; > > > > : dma-align ( virt size -- virt a-size ) over fff and + 1000 @aligned ; > > > > fff + fff not and is just 1000 #aligned . > > > > I don't understand why you are adding the low twelve bits of the address > > to the size as well? > > Because everything from the beginning of 4K chunk need to be included > into the DMA mapping - the IOMMU pages are 4K aligned. Ah, tricky, so virt has to be rounded down to 4k as well? Segher
On 20/11/2019 05:10, Segher Boessenkool wrote: > On Fri, Nov 15, 2019 at 10:33:58AM +1100, Alexey Kardashevskiy wrote: >> On 14/11/2019 22:47, Segher Boessenkool wrote: >>> : trunc-4k ( a - a ) -1000 and ; >>> >>> if you want a lower address, instead (less often useful, btw). >>> >>> (OF always is two's complement, you can write -1000 instead of fff invert). >> >> I am really not used to "-1000" :-/ > > -N reads a bit nicer than ~(N-1), but it is the same thing. I find it > easier to read -1000 and instead of fff invert and but that is for > a big part just what you are used to. Well, I did not see this being used anywhere else, like, in linux. >>>> +\ grub does not align allocated addresses to the size >>>> +\ so when mapping in dma-map-in, sometime we call H_PUT_TCE more than >>>> +\ bm-alloc allocated >>>> +: dma-align ( virt size -- virt aligned-size ) >>>> + over fff and + fff + fff not and >>>> +; >>> >>> : dma-align ( virt size -- virt a-size ) over fff and + 1000 @aligned ; >>> >>> fff + fff not and is just 1000 #aligned . >>> >>> I don't understand why you are adding the low twelve bits of the address >>> to the size as well? >> >> Because everything from the beginning of 4K chunk need to be included >> into the DMA mapping - the IOMMU pages are 4K aligned. > > Ah, tricky, so virt has to be rounded down to 4k as well? The virt address passed to H_PUT_TCE - yes it has but the dma-map-in helper still should return nonaligned address so I keep the page offset around until return. I'll try to make this clearer. Thanks,
Hi! On Wed, Nov 20, 2019 at 11:25:20AM +1100, Alexey Kardashevskiy wrote: > On 20/11/2019 05:10, Segher Boessenkool wrote: > > On Fri, Nov 15, 2019 at 10:33:58AM +1100, Alexey Kardashevskiy wrote: > >> I am really not used to "-1000" :-/ > > > > -N reads a bit nicer than ~(N-1), but it is the same thing. I find it > > easier to read -1000 and instead of fff invert and but that is for > > a big part just what you are used to. > > Well, I did not see this being used anywhere else, like, in linux. It is used a lot in binutils and in GCC, at least. Linux doesn't use it much, indeed. It uses the "x & -x" idiom to isolate the lowest set bit quite a bit, but the "& -align" idiom not often; mostly a bit in fs/ and in random drivers. Segher
diff --git a/board-qemu/slof/pci-phb.fs b/board-qemu/slof/pci-phb.fs index 06729bcf77a0..a1ea8f5e476f 100644 --- a/board-qemu/slof/pci-phb.fs +++ b/board-qemu/slof/pci-phb.fs @@ -107,10 +107,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 @@ -152,62 +148,57 @@ setup-puid dma-window-size mod dma-window-base + ; +: align-4k + fff not and +; + +\ grub does not align allocated addresses to the size +\ so when mapping in dma-map-in, sometime we call H_PUT_TCE more than +\ bm-alloc allocated +: dma-align ( virt size -- virt aligned-size ) + over fff and + fff + fff 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 dma-align tuck ( size virt size ) + bm-handle swap bm-alloc ( size virt dev-addr ) + dup 0 < IF + ." Bitmap allocation Failed " + 2drop + 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 ) + over fff and over or >r \ add page offset to return value + align-4k swap align-4k ( size dev-addr virt r: dev-addr ) + 3 OR \ Allow read and write + rot ( virt dev-addr size r: dev-addr ) + 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 hv-put-tce ABORT" H_PUT_TCE failed" - tmp-dev-addr 1000 + to tmp-dev-addr + 1000 + swap 1000 + swap 1000 +LOOP - r> drop - my-virt FFF and dev-addr or (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 ) + dma-align + 2dup swap fff not and swap 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 + 1000 +LOOP - bm-handle dev-addr my-size bm-free + drop (clear-dma-window-vars) ;
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. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- board-qemu/slof/pci-phb.fs | 77 +++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 43 deletions(-)