| Message ID | 20171016051653.31014-4-aik@ozlabs.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fdt: Pass the resulting device tree to QEMU + related fixes | expand |
Hi Alexey, On Mon, Oct 16, 2017 at 04:16:50PM +1100, Alexey Kardashevskiy wrote: > +: fdt-skip-string ( cur -- cur ) > + BEGIN > + dup c@ > + WHILE > + 1+ > + REPEAT > + 4 + -4 and > +; You could use ZCOUNT, like: : fdt-skip-string ( addr -- addr ) zcount + char+ 4 #aligned ; > +: fdt-begin-node ( name namelen -- ) > + OF_DT_BEGIN_NODE fdt-l, > + 2dup 1 = swap c@ [char] / = and IF 2drop s" " THEN \ is it '/'? 2dup s" /" str= IF 2drop s" " THEN But you probably shouldn't test for name "/" at all: the problem is that the FDT doesn't allow anything in "name" for the root node? So pass the phandle (instead of the name string), test for *that*, and do the node>qname in here. Like: : fdt-begin-node ( phandle -- ) OF_DT_BEGIN_NODE fdt-l, dup device-tree @ = IF drop s" " ELSE node>qname THEN fdt-ztr, fdt-align ; Here, have a tag :-) Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org> Segher
On 16/10/17 18:18, Segher Boessenkool wrote: > Hi Alexey, > > On Mon, Oct 16, 2017 at 04:16:50PM +1100, Alexey Kardashevskiy wrote: >> +: fdt-skip-string ( cur -- cur ) >> + BEGIN >> + dup c@ >> + WHILE >> + 1+ >> + REPEAT >> + 4 + -4 and >> +; > > You could use ZCOUNT, like: > > : fdt-skip-string ( addr -- addr ) zcount + char+ 4 #aligned ; > > >> +: fdt-begin-node ( name namelen -- ) >> + OF_DT_BEGIN_NODE fdt-l, >> + 2dup 1 = swap c@ [char] / = and IF 2drop s" " THEN \ is it '/'? > > 2dup s" /" str= IF 2drop s" " THEN > > > But you probably shouldn't test for name "/" at all: the problem is that > the FDT doesn't allow anything in "name" for the root node? So pass > the phandle (instead of the name string), test for *that*, and do the > node>qname in here. Like: > > : fdt-begin-node ( phandle -- ) > OF_DT_BEGIN_NODE fdt-l, > dup device-tree @ = IF drop s" " ELSE node>qname THEN > fdt-ztr, > fdt-align > ; > > > Here, have a tag :-) > > Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org> Yay, thanks for the help with this. v6 is still coming though for this (so I'll add your suggestions too): diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs index 90e4f1c..d151031 100644 --- a/board-qemu/slof/fdt.fs +++ b/board-qemu/slof/fdt.fs @@ -631,6 +631,15 @@ VARIABLE fdt-ms \ debug only drop + ; +: fdt-boot-cpu ( -- bootcpu ) + s" cpu" s" /chosen" find-node get-property 0<> IF 0 EXIT THEN + decode-int + nip nip ihandle>phandle + s" reg" rot get-property 0<> IF 0 EXIT THEN + decode-int + nip nip +; + : fdt-flatten-tree ( -- tree ) 1 to fdt-debug @@ -676,7 +685,7 @@ VARIABLE fdt-ms \ debug only /fdth r@ >fdth_rsvmap_off l! 11 r@ >fdth_version l! 10 r@ >fdth_compat_vers l! - 0 r@ >fdth_boot_cpu l! + fdt-boot-cpu r@ >fdth_boot_cpu l! over r@ >fdth_string_size l! 2 pick r@ >fdth_struct_size l! ( struct-len strings-len total-len r: fdt ) @@ -693,8 +702,8 @@ VARIABLE fdt-ms \ debug only drop \ Free temporary blobs - fdtfl-struct @ 200000 free-mem - fdtfl-strings @ 200000 free-mem + fdtfl-struct @ 100000 free-mem + fdtfl-strings @ 100000 free-mem
On 16/10/17 18:32, Alexey Kardashevskiy wrote: > On 16/10/17 18:18, Segher Boessenkool wrote: >> Hi Alexey, >> >> On Mon, Oct 16, 2017 at 04:16:50PM +1100, Alexey Kardashevskiy wrote: >>> +: fdt-skip-string ( cur -- cur ) >>> + BEGIN >>> + dup c@ >>> + WHILE >>> + 1+ >>> + REPEAT >>> + 4 + -4 and >>> +; >> >> You could use ZCOUNT, like: >> >> : fdt-skip-string ( addr -- addr ) zcount + char+ 4 #aligned ; btw using zcount (which is C's strlen()) here reduced time from 335ms to 243ms. Nice!
On Mon, Oct 16, 2017 at 06:32:17PM +1100, Alexey Kardashevskiy wrote: > +: fdt-boot-cpu ( -- bootcpu ) > + s" cpu" s" /chosen" find-node get-property 0<> IF 0 EXIT THEN > + decode-int > + nip nip ihandle>phandle > + s" reg" rot get-property 0<> IF 0 EXIT THEN > + decode-int > + nip nip > +; 0<> IF is exactly the same as just IF :-) There is get-chosen to make this easier. The first line then becomes: s" cpu" get-chosen 0= IF 0 EXIT THEN Is there no variable you can get the boot CPU from directly though? Segher
On 16/10/17 19:02, Segher Boessenkool wrote: > On Mon, Oct 16, 2017 at 06:32:17PM +1100, Alexey Kardashevskiy wrote: >> +: fdt-boot-cpu ( -- bootcpu ) >> + s" cpu" s" /chosen" find-node get-property 0<> IF 0 EXIT THEN >> + decode-int >> + nip nip ihandle>phandle >> + s" reg" rot get-property 0<> IF 0 EXIT THEN >> + decode-int >> + nip nip >> +; > > 0<> IF is exactly the same as just IF :-) Right, it was 0= before I ended up with this variant :) > > There is get-chosen to make this easier. The first line then > becomes: > > s" cpu" get-chosen 0= IF 0 EXIT THEN > > Is there no variable you can get the boot CPU from directly though? It does not seem so: board-qemu/slof/tree.fs \ Do not assume that cpu0 is available : set-chosen-cpu " /cpus" find-device get-node child dup 0= ABORT" CPU not found" node>path open-dev encode-int s" cpu" set-chosen ; set-chosen-cpu
On Mon, Oct 16, 2017 at 07:35:11PM +1100, Alexey Kardashevskiy wrote: > On 16/10/17 19:02, Segher Boessenkool wrote: > > Is there no variable you can get the boot CPU from directly though? > > It does not seem so: > > board-qemu/slof/tree.fs > > \ Do not assume that cpu0 is available > : set-chosen-cpu > " /cpus" find-device > get-node child dup 0= ABORT" CPU not found" > node>path open-dev encode-int s" cpu" set-chosen > ; > set-chosen-cpu Oh lovely nasty code. Maybe something like VARIABLE chosen-cpu-phandle VARIABLE chosen-cpu-ihandle : set-chosen-cpu ( -- ) s" /cpus" find-node dup 0= ABORT" /cpus not found" child dup 0= ABORT" /cpus/cpu not found" dup chosen-cpu-phandle ! 0 0 rot open-node dup chosen-cpu-ihandle ! encode-int s" cpu" set-chosen ; and then you can use chosen-cpu-ihandle @ etc.? Segher
On 16/10/17 20:07, Segher Boessenkool wrote: > On Mon, Oct 16, 2017 at 07:35:11PM +1100, Alexey Kardashevskiy wrote: >> On 16/10/17 19:02, Segher Boessenkool wrote: >>> Is there no variable you can get the boot CPU from directly though? >> >> It does not seem so: >> >> board-qemu/slof/tree.fs >> >> \ Do not assume that cpu0 is available >> : set-chosen-cpu >> " /cpus" find-device >> get-node child dup 0= ABORT" CPU not found" >> node>path open-dev encode-int s" cpu" set-chosen >> ; >> set-chosen-cpu > > Oh lovely nasty code. > > Maybe something like > > > VARIABLE chosen-cpu-phandle > VARIABLE chosen-cpu-ihandle > : set-chosen-cpu ( -- ) > s" /cpus" find-node dup 0= ABORT" /cpus not found" > child dup 0= ABORT" /cpus/cpu not found" > dup chosen-cpu-phandle ! 0 0 rot open-node > dup chosen-cpu-ihandle ! encode-int s" cpu" set-chosen ; > > > and then you can use chosen-cpu-ihandle @ etc.? I can do that but what is the exact benefit of doing it this way? It is going to be used once really, and why would we need to cache both ihandle and phandle but not reg (which we really care about here), for example?
On Mon, Oct 16, 2017 at 11:06:15PM +1100, Alexey Kardashevskiy wrote: > On 16/10/17 20:07, Segher Boessenkool wrote: > > Maybe something like > > > > > > VARIABLE chosen-cpu-phandle > > VARIABLE chosen-cpu-ihandle > > : set-chosen-cpu ( -- ) > > s" /cpus" find-node dup 0= ABORT" /cpus not found" > > child dup 0= ABORT" /cpus/cpu not found" > > dup chosen-cpu-phandle ! 0 0 rot open-node > > dup chosen-cpu-ihandle ! encode-int s" cpu" set-chosen ; > > > > > > and then you can use chosen-cpu-ihandle @ etc.? > > I can do that but what is the exact benefit of doing it this way? It is > going to be used once really, and why would we need to cache both ihandle > and phandle but not reg (which we really care about here), for example? It's not caching: this is the "real" data, what is in the device tree is just some external representation. Reading things back from the device tree is extremely slow at best, and very harmful in worse cases. Just say no! You can of course choose to not keep the phandle or ihandle around, instead just the unit address, for example. The point is to use find-node etc. instead of find-device etc. -- easier to use and it does not clobber the current packages, etc. And, of course, don't read /chosen data back from the device tree. Segher
On 16/10/17 23:27, Segher Boessenkool wrote: > On Mon, Oct 16, 2017 at 11:06:15PM +1100, Alexey Kardashevskiy wrote: >> On 16/10/17 20:07, Segher Boessenkool wrote: >>> Maybe something like >>> >>> >>> VARIABLE chosen-cpu-phandle >>> VARIABLE chosen-cpu-ihandle >>> : set-chosen-cpu ( -- ) >>> s" /cpus" find-node dup 0= ABORT" /cpus not found" >>> child dup 0= ABORT" /cpus/cpu not found" >>> dup chosen-cpu-phandle ! 0 0 rot open-node >>> dup chosen-cpu-ihandle ! encode-int s" cpu" set-chosen ; >>> >>> >>> and then you can use chosen-cpu-ihandle @ etc.? >> >> I can do that but what is the exact benefit of doing it this way? It is >> going to be used once really, and why would we need to cache both ihandle >> and phandle but not reg (which we really care about here), for example? > > It's not caching: this is the "real" data, what is in the device tree > is just some external representation. Reading things back from the > device tree is extremely slow at best, and very harmful in worse cases. > Just say no! > > You can of course choose to not keep the phandle or ihandle around, > instead just the unit address, for example. > > The point is to use find-node etc. instead of find-device etc. -- easier > to use and it does not clobber the current packages, etc. And, of course, > don't read /chosen data back from the device tree. Oh, ok. So I assume there is a better way of getting "reg" as well rather than reading the property, like "chosen-cpu-phandle @ >unit"? This is my draft, need "defer" as fdt.fs is included before tree.fs: diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs index f27cd1b..9ae2e4b 100644 --- a/board-qemu/slof/fdt.fs +++ b/board-qemu/slof/fdt.fs @@ -624,14 +624,7 @@ VARIABLE fdt-ms \ debug only drop + ; -: fdt-boot-cpu ( -- bootcpu ) - s" cpu" s" /chosen" find-node get-property 0<> IF 0 EXIT THEN - decode-int - nip nip ihandle>phandle - s" reg" rot get-property 0<> IF 0 EXIT THEN - decode-int - nip nip -; +DEFER chosen-cpu : fdt-flatten-tree ( -- tree ) 1 to fdt-debug @@ -678,7 +671,7 @@ VARIABLE fdt-ms \ debug only /fdth r@ >fdth_rsvmap_off l! 11 r@ >fdth_version l! 10 r@ >fdth_compat_vers l! - fdt-boot-cpu r@ >fdth_boot_cpu l! + chosen-cpu r@ >fdth_boot_cpu l! over r@ >fdth_string_size l! 2 pick r@ >fdth_struct_size l! ( struct-len strings-len total-len r: fdt ) diff --git a/board-qemu/slof/tree.fs b/board-qemu/slof/tree.fs index cc35fa3..2655500 100644 --- a/board-qemu/slof/tree.fs +++ b/board-qemu/slof/tree.fs @@ -156,11 +156,18 @@ populate-pci-busses 6c0 cp \ Do not assume that cpu0 is available -: set-chosen-cpu - " /cpus" find-device - get-node child dup 0= ABORT" CPU not found" - node>path open-dev encode-int s" cpu" set-chosen +VARIABLE chosen-cpu-phandle +VARIABLE chosen-cpu-ihandle +: set-chosen-cpu ( -- ) + s" /cpus" find-node dup 0= ABORT" /cpus not found" + child dup 0= ABORT" /cpus/cpu not found" + dup chosen-cpu-phandle ! 0 0 rot open-node + dup chosen-cpu-ihandle ! encode-int s" cpu" set-chosen ; + +: chosen-cpu-func ( -- reg ) chosen-cpu-phandle @ >unit ; +' chosen-cpu-func to chosen-cpu +
On Tue, Oct 17, 2017 at 02:12:39PM +1100, Alexey Kardashevskiy wrote: > On 16/10/17 23:27, Segher Boessenkool wrote: > > You can of course choose to not keep the phandle or ihandle around, > > instead just the unit address, for example. > > > > The point is to use find-node etc. instead of find-device etc. -- easier > > to use and it does not clobber the current packages, etc. And, of course, > > don't read /chosen data back from the device tree. > > Oh, ok. > So I assume there is a better way of getting "reg" as well rather than > reading the property, like "chosen-cpu-phandle @ >unit"? That should work fine here. > This is my draft, need "defer" as fdt.fs is included before tree.fs: > > > diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs > index f27cd1b..9ae2e4b 100644 > --- a/board-qemu/slof/fdt.fs > +++ b/board-qemu/slof/fdt.fs > @@ -624,14 +624,7 @@ VARIABLE fdt-ms \ debug only > drop + > ; > > -: fdt-boot-cpu ( -- bootcpu ) > - s" cpu" s" /chosen" find-node get-property 0<> IF 0 EXIT THEN > - decode-int > - nip nip ihandle>phandle > - s" reg" rot get-property 0<> IF 0 EXIT THEN > - decode-int > - nip nip > -; > +DEFER chosen-cpu Chosen cpu what? chosen-cpu-address maybe? > +VARIABLE chosen-cpu-phandle > +VARIABLE chosen-cpu-ihandle > +: set-chosen-cpu ( -- ) > + s" /cpus" find-node dup 0= ABORT" /cpus not found" > + child dup 0= ABORT" /cpus/cpu not found" > + dup chosen-cpu-phandle ! 0 0 rot open-node > + dup chosen-cpu-ihandle ! encode-int s" cpu" set-chosen > ; You can also calculate and store the address right here (and you of course do not need the phandle and ihandle if you do not want them for anything else). There's no need for DEFER -- just define the variable before you first use it :-) Segher
On 17/10/17 20:48, Segher Boessenkool wrote: > On Tue, Oct 17, 2017 at 02:12:39PM +1100, Alexey Kardashevskiy wrote: >> On 16/10/17 23:27, Segher Boessenkool wrote: >>> You can of course choose to not keep the phandle or ihandle around, >>> instead just the unit address, for example. >>> >>> The point is to use find-node etc. instead of find-device etc. -- easier >>> to use and it does not clobber the current packages, etc. And, of course, >>> don't read /chosen data back from the device tree. >> >> Oh, ok. >> So I assume there is a better way of getting "reg" as well rather than >> reading the property, like "chosen-cpu-phandle @ >unit"? > > That should work fine here. > >> This is my draft, need "defer" as fdt.fs is included before tree.fs: >> >> >> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs >> index f27cd1b..9ae2e4b 100644 >> --- a/board-qemu/slof/fdt.fs >> +++ b/board-qemu/slof/fdt.fs >> @@ -624,14 +624,7 @@ VARIABLE fdt-ms \ debug only >> drop + >> ; >> >> -: fdt-boot-cpu ( -- bootcpu ) >> - s" cpu" s" /chosen" find-node get-property 0<> IF 0 EXIT THEN >> - decode-int >> - nip nip ihandle>phandle >> - s" reg" rot get-property 0<> IF 0 EXIT THEN >> - decode-int >> - nip nip >> -; >> +DEFER chosen-cpu > > Chosen cpu what? chosen-cpu-address maybe? May be chosen-cpu>unit or chosen-cpu-unit ? > >> +VARIABLE chosen-cpu-phandle >> +VARIABLE chosen-cpu-ihandle >> +: set-chosen-cpu ( -- ) >> + s" /cpus" find-node dup 0= ABORT" /cpus not found" >> + child dup 0= ABORT" /cpus/cpu not found" >> + dup chosen-cpu-phandle ! 0 0 rot open-node >> + dup chosen-cpu-ihandle ! encode-int s" cpu" set-chosen >> ; > > You can also calculate and store the address right here (and you of > course do not need the phandle and ihandle if you do not want them > for anything else). I can, I just have no good taste :) Ok, I'll store chosen-cpu-ihandle only and use "ihandle>phandle >unit" in "chosen-cpu>unit". > There's no need for DEFER -- just define the > variable before you first use it :-) Pfff. Where would I put it? Won't it confuse future readers? :) But sure I can do that and define chosen-cpu-ihandle in fdt.fs, and if we decide to use chosen-cpu-ihandle for something else - move it, and so on.
On Wed, Oct 18, 2017 at 01:45:43PM +1100, Alexey Kardashevskiy wrote: > On 17/10/17 20:48, Segher Boessenkool wrote: > >> -: fdt-boot-cpu ( -- bootcpu ) > >> - s" cpu" s" /chosen" find-node get-property 0<> IF 0 EXIT THEN > >> - decode-int > >> - nip nip ihandle>phandle > >> - s" reg" rot get-property 0<> IF 0 EXIT THEN > >> - decode-int > >> - nip nip > >> -; > >> +DEFER chosen-cpu > > > > Chosen cpu what? chosen-cpu-address maybe? > > May be chosen-cpu>unit or chosen-cpu-unit ? ">" means "to" so that's a bit misleading (there is no chosen-cpu as input to this word). "chosen-cpu-unit" is fine with me. > >> +VARIABLE chosen-cpu-phandle > >> +VARIABLE chosen-cpu-ihandle > >> +: set-chosen-cpu ( -- ) > >> + s" /cpus" find-node dup 0= ABORT" /cpus not found" > >> + child dup 0= ABORT" /cpus/cpu not found" > >> + dup chosen-cpu-phandle ! 0 0 rot open-node > >> + dup chosen-cpu-ihandle ! encode-int s" cpu" set-chosen > >> ; > > > > You can also calculate and store the address right here (and you of > > course do not need the phandle and ihandle if you do not want them > > for anything else). > > I can, I just have no good taste :) Ok, I'll store chosen-cpu-ihandle only > and use "ihandle>phandle >unit" in "chosen-cpu>unit". > > > There's no need for DEFER -- just define the > > variable before you first use it :-) > > Pfff. Where would I put it? The same place as you now have the DEFER? DEFER and VARIABLE are very much alike: : VARIABLE CREATE 0 , ; : DEFER CREATE POSTPONE ABORT DOES> @ EXECUTE ; or they looks even more alike if you write this as : VARIABLE CREATE 0 , DOES> ; : DEFER CREATE ['] ABORT , DOES> @ EXECUTE ; (this isn't how those words are actually implemented, it is optimised a little, but it works essentially the same way). > Won't it confuse future readers? :) But sure I > can do that and define chosen-cpu-ihandle in fdt.fs, and if we decide to > use chosen-cpu-ihandle for something else - move it, and so on. Or even not define words for the phandle and ihandle if you don't use them. Your choice :-) Segher
On 18/10/17 20:24, Segher Boessenkool wrote: > On Wed, Oct 18, 2017 at 01:45:43PM +1100, Alexey Kardashevskiy wrote: >> On 17/10/17 20:48, Segher Boessenkool wrote: >>>> -: fdt-boot-cpu ( -- bootcpu ) >>>> - s" cpu" s" /chosen" find-node get-property 0<> IF 0 EXIT THEN >>>> - decode-int >>>> - nip nip ihandle>phandle >>>> - s" reg" rot get-property 0<> IF 0 EXIT THEN >>>> - decode-int >>>> - nip nip >>>> -; >>>> +DEFER chosen-cpu >>> >>> Chosen cpu what? chosen-cpu-address maybe? >> >> May be chosen-cpu>unit or chosen-cpu-unit ? > > ">" means "to" so that's a bit misleading (there is no chosen-cpu as > input to this word). "chosen-cpu-unit" is fine with me. > >>>> +VARIABLE chosen-cpu-phandle >>>> +VARIABLE chosen-cpu-ihandle >>>> +: set-chosen-cpu ( -- ) >>>> + s" /cpus" find-node dup 0= ABORT" /cpus not found" >>>> + child dup 0= ABORT" /cpus/cpu not found" >>>> + dup chosen-cpu-phandle ! 0 0 rot open-node >>>> + dup chosen-cpu-ihandle ! encode-int s" cpu" set-chosen >>>> ; >>> >>> You can also calculate and store the address right here (and you of >>> course do not need the phandle and ihandle if you do not want them >>> for anything else). >> >> I can, I just have no good taste :) Ok, I'll store chosen-cpu-ihandle only >> and use "ihandle>phandle >unit" in "chosen-cpu>unit". >> >>> There's no need for DEFER -- just define the >>> variable before you first use it :-) >> >> Pfff. Where would I put it? > > The same place as you now have the DEFER? DEFER and VARIABLE are very > much alike: > > : VARIABLE CREATE 0 , ; > : DEFER CREATE POSTPONE ABORT DOES> @ EXECUTE ; > > or they looks even more alike if you write this as > > : VARIABLE CREATE 0 , DOES> ; > : DEFER CREATE ['] ABORT , DOES> @ EXECUTE ; > > (this isn't how those words are actually implemented, it is optimised > a little, but it works essentially the same way). > >> Won't it confuse future readers? :) But sure I >> can do that and define chosen-cpu-ihandle in fdt.fs, and if we decide to >> use chosen-cpu-ihandle for something else - move it, and so on. > > Or even not define words for the phandle and ihandle if you don't use > them. Your choice :-) I need either: - a variable for the unit address or - a word to get it ("chosen-cpu-unit"). They both are better be defined nearby set-chosen-cpu. But fdt.fs is parsed before that. If I do not use "defer", then it cannot be "chosen-cpu-unit" as it is going to look weird - "set" is defined in one place and "get" - in another. If I define chosen-cpu-ihandle in fdt.fs - this is better but still inconsistent and I won't be able to use "chosen-cpu-unit" in fdt.fs anyway. My choice was to do "defer chosen-cpu-unit" but you are opposing it which makes me think I am missing something, again :)
On Wed, Oct 18, 2017 at 08:34:57PM +1100, Alexey Kardashevskiy wrote: > > Or even not define words for the phandle and ihandle if you don't use > > them. Your choice :-) > > I need either: > - a variable for the unit address or > - a word to get it ("chosen-cpu-unit"). > > They both are better be defined nearby set-chosen-cpu. But fdt.fs is parsed > before that. Well, set-chosen-cpu shouldn't really decide what the boot cpu is anyway -- it should just set the link in /chosen! The current method of just picking whatever is the first device in /cpus isn't so great anyway. > My choice was to do "defer chosen-cpu-unit" but you are opposing it which > makes me think I am missing something, again :) I'm not opposing it, just suggesting ways to make it better. In general having DEFERs for random stuff means you should do some restructuring, or you'll end up with spaghetti (and things build with spaghetti aren't typically very sturdy). Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Wed, Oct 18, 2017 at 08:34:57PM +1100, Alexey Kardashevskiy wrote: >> > Or even not define words for the phandle and ihandle if you don't use >> > them. Your choice :-) >> >> I need either: >> - a variable for the unit address or >> - a word to get it ("chosen-cpu-unit"). >> >> They both are better be defined nearby set-chosen-cpu. But fdt.fs is parsed >> before that. > > Well, set-chosen-cpu shouldn't really decide what the boot cpu is > anyway -- it should just set the link in /chosen! > > The current method of just picking whatever is the first device in /cpus > isn't so great anyway. Earlier, there was an assumption of /cpus/@0, with addition of cpu hotplug in QEMU, I had added code to remove this dependency. That was the reason to find the first device in /cpus Regards Nikunj
On Wed, Oct 18, 2017 at 04:15:05PM +0530, Nikunj A Dadhania wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > On Wed, Oct 18, 2017 at 08:34:57PM +1100, Alexey Kardashevskiy wrote: > >> > Or even not define words for the phandle and ihandle if you don't use > >> > them. Your choice :-) > >> > >> I need either: > >> - a variable for the unit address or > >> - a word to get it ("chosen-cpu-unit"). > >> > >> They both are better be defined nearby set-chosen-cpu. But fdt.fs is parsed > >> before that. > > > > Well, set-chosen-cpu shouldn't really decide what the boot cpu is > > anyway -- it should just set the link in /chosen! > > > > The current method of just picking whatever is the first device in /cpus > > isn't so great anyway. > > Earlier, there was an assumption of /cpus/@0, with addition of cpu > hotplug in QEMU, I had added code to remove this dependency. That was > the reason to find the first device in /cpus Right, but is there any guarantee that will actually be the boot cpu; maybe on qemu it is, but certainly not elsewhere! (In practice it will usually work of course). Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Wed, Oct 18, 2017 at 04:15:05PM +0530, Nikunj A Dadhania wrote: >> Segher Boessenkool <segher@kernel.crashing.org> writes: >> > On Wed, Oct 18, 2017 at 08:34:57PM +1100, Alexey Kardashevskiy wrote: >> >> > Or even not define words for the phandle and ihandle if you don't use >> >> > them. Your choice :-) >> >> >> >> I need either: >> >> - a variable for the unit address or >> >> - a word to get it ("chosen-cpu-unit"). >> >> >> >> They both are better be defined nearby set-chosen-cpu. But fdt.fs is parsed >> >> before that. >> > >> > Well, set-chosen-cpu shouldn't really decide what the boot cpu is >> > anyway -- it should just set the link in /chosen! >> > >> > The current method of just picking whatever is the first device in /cpus >> > isn't so great anyway. >> >> Earlier, there was an assumption of /cpus/@0, with addition of cpu >> hotplug in QEMU, I had added code to remove this dependency. That was >> the reason to find the first device in /cpus > > Right, but is there any guarantee that will actually be the boot cpu; > maybe on qemu it is, but certainly not elsewhere! (In practice it will > usually work of course). Right Regards Nikunj
On 18/10/17 20:46, Segher Boessenkool wrote: > On Wed, Oct 18, 2017 at 08:34:57PM +1100, Alexey Kardashevskiy wrote: >>> Or even not define words for the phandle and ihandle if you don't use >>> them. Your choice :-) >> >> I need either: >> - a variable for the unit address or >> - a word to get it ("chosen-cpu-unit"). >> >> They both are better be defined nearby set-chosen-cpu. But fdt.fs is parsed >> before that. > > Well, set-chosen-cpu shouldn't really decide what the boot cpu is > anyway -- it should just set the link in /chosen! > > The current method of just picking whatever is the first device in /cpus > isn't so great anyway. > >> My choice was to do "defer chosen-cpu-unit" but you are opposing it which >> makes me think I am missing something, again :) > > I'm not opposing it, just suggesting ways to make it better. Let's assume I moved new FDT stuff to fdt-fl.fs. Now we have in OF.fs: #include "board-qemu/slof/fdt.fs" <- parses the initial fdt #include <slof/fs/root.fs> <- defines set-chosen #include "board-qemu/slof/fdt-fl.fs" <- uses chosen-cpu-unit and defines fdt-flatten-tree #include "board-qemu/slof/rtas.fs" <- it needs fdt and it also needs fdt-flatten-tree #include "board-qemu/slof/tree.fs" <- defines set-chosen-cpu right now (which uses set-chosen) and defines chosen-cpu-unit (as it uses shared variable - chosen-cpu-ihandle) #include "slof/fs/client.fs" <- uses fdt-flatten-tree So, "set-chosen-cpu" is not found by running SLOF. How do I fix this properly? I can move set-chosen-cpu/chosen-cpu-unit from QEMU's tree.fs to the common root.fs, next to "set-chosen". Will this be less spaghetti or it is bad in some other way? Or move the set-chosen-cpu/chosen-cpu-unit definitions to board-qemu/slof/OF.fs right after #include <slof/fs/root.fs> (but will definitely look spaghetti)? I pushed the tree to github if anyone is interested: https://github.com/aik/SLOF/commits/fdt > In general > having DEFERs for random stuff means you should do some restructuring, or > you'll end up with spaghetti (and things build with spaghetti aren't > typically very sturdy).
Hi Alexey, On Thu, Oct 19, 2017 at 03:10:16PM +1100, Alexey Kardashevskiy wrote: > On 18/10/17 20:46, Segher Boessenkool wrote: > > On Wed, Oct 18, 2017 at 08:34:57PM +1100, Alexey Kardashevskiy wrote: > >> I need either: > >> - a variable for the unit address or I'd choose this one, and make it a global variable, defined quite early (before the tree is constructed). It should probably be set early, too. Then later the tree is constructed, and even later /chosen/cpu is set. Similar for some other things in /chosen... We want to have a stdout before we have the corresponding device in the device tree, etc. Clients of SLOF will find the device via the device tree, but SLOF itself doesn't have to! Segher
diff --git a/lib/libhvcall/libhvcall.h b/lib/libhvcall/libhvcall.h index 6356a62..3fa4398 100644 --- a/lib/libhvcall/libhvcall.h +++ b/lib/libhvcall/libhvcall.h @@ -24,7 +24,8 @@ #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1) /* Client Architecture support */ #define KVMPPC_H_CAS (KVMPPC_HCALL_BASE + 0x2) -#define KVMPPC_HCALL_MAX KVMPPC_H_CAS +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3) +#define KVMPPC_HCALL_MAX KVMPPC_H_UPDATE_DT #ifndef __ASSEMBLY__ diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs index 851645e..2d0df64 100644 --- a/board-qemu/slof/fdt.fs +++ b/board-qemu/slof/fdt.fs @@ -27,7 +27,7 @@ struct 4 field >fdth_boot_cpu 4 field >fdth_string_size 4 field >fdth_struct_size -drop +constant /fdth h# d00dfeed constant OF_DT_HEADER h# 1 constant OF_DT_BEGIN_NODE @@ -69,7 +69,7 @@ fdt-start fdt-init dup >fdth_version l@ 3 >= IF ." strings size : 0x" dup >fdth_string_size l@ . cr THEN - dup >fdth_version l@ 17 >= IF + dup >fdth_version l@ 11 >= IF ." struct size : 0x" dup >fdth_struct_size l@ . cr THEN THEN @@ -439,4 +439,267 @@ r> drop fdt-cas-fix? ; +VARIABLE fdtfl-struct +VARIABLE fdtfl-struct-here +VARIABLE fdtfl-strings +VARIABLE fdtfl-strings-cache +VARIABLE fdtfl-strings-here +VARIABLE fdtfl-strings-reused \ debug only +VARIABLE fdt-ms \ debug only + +: fdt-skip-string ( cur -- cur ) + BEGIN + dup c@ + WHILE + 1+ + REPEAT + 4 + -4 and +; + +: zstring= ( str len zstr -- flag ) + 2dup + c@ 0<> IF + 3drop false + EXIT + THEN + swap comp 0= +; + +: fdt-find-string ( name namelen -- nameoff true | false ) + fdtfl-strings @ + BEGIN + dup fdtfl-strings-cache @ < + WHILE + 3dup zstring= IF + nip nip ( curstr ) + fdtfl-strings @ - + true + EXIT + THEN + fdt-skip-string + REPEAT + 3drop + false +; + +: fdt-str-allot ( len -- ) fdtfl-strings-here @ + to fdtfl-strings-here ; +: fdt-str-c, ( char -- ) fdtfl-strings-here @ 1 fdt-str-allot c! ; +: fdt-str-align ( -- ) + fdtfl-strings-here @ + dup dup 4 #aligned swap - ( here bytes-to-erase ) + dup -rot + erase + fdt-str-allot +; +: fdt-str-bytes, ( data len -- ) fdtfl-strings-here @ over fdt-str-allot swap move ; +: fdt-str-ztr, ( str len -- ) fdt-str-bytes, 0 fdt-str-c, ; + +: fdt-add-string ( name namelen -- nameoff ) + fdtfl-strings-here @ -rot + fdt-str-ztr, + fdt-str-align + fdtfl-strings @ - +; + +: fdt-get-string ( name namelen -- nameoff ) + 2dup fdt-find-string IF + -rot 2drop + fdt-debug IF + 1 fdtfl-strings-reused +! + THEN + EXIT + THEN + fdt-add-string +; + +: fdt-allot ( len -- ) fdtfl-struct-here @ + to fdtfl-struct-here ; +: fdt-c, ( char -- ) fdtfl-struct-here @ 1 fdt-allot c! ; +: fdt-align ( -- ) + fdtfl-struct-here @ + dup dup 4 #aligned swap - ( here bytes-to-erase ) + dup -rot + erase + fdt-allot +; +: fdt-bytes, ( data len -- ) fdtfl-struct-here @ over fdt-allot swap move ; +: fdt-ztr, ( str len -- ) fdt-bytes, 0 fdt-c, ; +: fdt-l, ( token -- ) fdtfl-struct-here @ l! /l fdt-allot ; + +: fdt-begin-node ( name namelen -- ) + OF_DT_BEGIN_NODE fdt-l, + 2dup 1 = swap c@ [char] / = and IF 2drop s" " THEN \ is it '/'? + fdt-ztr, + fdt-align +; + +: fdt-end-node ( -- ) OF_DT_END_NODE fdt-l, ; + +: fdt-prop ( prop len name namelen -- ) + OF_DT_PROP fdt-l, + + \ get string offset + fdt-get-string ( prop len nameoff ) + + \ store len and nameoff + over fdt-l, + fdt-l, ( prop len ) + + \ now store the bytes + fdt-bytes, + fdt-align +; + +: fdt-end ( -- ) OF_DT_END fdt-l, ; + +: fdt-copy-property ( link -- ) + dup link> execute + rot + link>name name>string + 2dup s" name" str= IF 4drop EXIT THEN \ skipping useless "name" + fdt-prop +; + +: for-all-words ( wid xt -- ) \ xt has sig ( lfa -- ) + >r + cell+ @ BEGIN dup WHILE dup r@ execute @ REPEAT + r> 2drop +; + +: fdt-copy-properties ( phandle -- ) + dup encode-int s" phandle" fdt-prop + node>properties @ + ['] fdt-copy-property for-all-words +; + +: fdt-copy-node ( node -- ) + fdt-debug 1 > IF dup node>path type cr THEN + dup node>qname fdt-begin-node + dup fdt-copy-properties + child BEGIN dup WHILE dup recurse peer REPEAT + drop + fdt-end-node +; + +: fdtfl-strings-preload ( -- ) + s" reg" fdt-add-string drop + s" status" fdt-add-string drop + s" 64-bit" fdt-add-string drop + s" phandle" fdt-add-string drop + s" ibm,vmx" fdt-add-string drop + s" ibm,dfp" fdt-add-string drop + s" slb-size" fdt-add-string drop + s" ibm,purr" fdt-add-string drop + s" vendor-id" fdt-add-string drop + s" device-id" fdt-add-string drop + s" min-grant" fdt-add-string drop + s" class-code" fdt-add-string drop + s" compatible" fdt-add-string drop + s" interrupts" fdt-add-string drop + s" cpu-version" fdt-add-string drop + s" #size-cells" fdt-add-string drop + s" ibm,req#msi" fdt-add-string drop + s" revision-id" fdt-add-string drop + s" device_type" fdt-add-string drop + s" max-latency" fdt-add-string drop + s" ibm,chip-id" fdt-add-string drop + s" ibm,pft-size" fdt-add-string drop + s" ibm,slb-size" fdt-add-string drop + s" devsel-speed" fdt-add-string drop + s" ibm,loc-code" fdt-add-string drop + s" subsystem-id" fdt-add-string drop + s" d-cache-size" fdt-add-string drop + s" i-cache-size" fdt-add-string drop + s" #address-cells" fdt-add-string drop + s" clock-frequency" fdt-add-string drop + s" cache-line-size" fdt-add-string drop + s" ibm,pa-features" fdt-add-string drop + s" ibm,my-drc-index" fdt-add-string drop + s" d-cache-line-size" fdt-add-string drop + s" i-cache-line-size" fdt-add-string drop + s" assigned-addresses" fdt-add-string drop + s" d-cache-block-size" fdt-add-string drop + s" i-cache-block-size" fdt-add-string drop + s" timebase-frequency" fdt-add-string drop + s" subsystem-vendor-id" fdt-add-string drop + s" ibm,segment-page-sizes" fdt-add-string drop + s" ibm,ppc-interrupt-server#s" fdt-add-string drop + s" ibm,processor-segment-sizes" fdt-add-string drop + s" ibm,ppc-interrupt-gserver#s" fdt-add-string drop +; + +: fdt-append-blob ( bytes cur blob -- cur ) + 3dup -rot swap move + drop + +; + +: fdt-flatten-tree ( -- tree ) + 100000 alloc-mem dup fdtfl-struct-here ! fdtfl-struct ! + 100000 alloc-mem dup fdtfl-strings-here ! fdtfl-strings ! + + fdt-debug IF + 0 fdtfl-strings-reused ! + milliseconds fdt-ms ! + THEN + + \ Preload strings cache + fdtfl-strings-preload + fdtfl-strings-here @ fdtfl-strings-cache ! + \ Render the blobs + device-tree @ fdt-copy-node + fdt-end + + \ Calculate strings and struct sizes + fdtfl-struct-here @ fdtfl-struct @ - + fdtfl-strings-here @ fdtfl-strings @ - ( struct-len strings-len ) + + 2dup + /fdth + + 10 + \ Reserve 16 bytes for an empty reserved block + + fdt-debug IF + 3dup + ." FDTsize=" .d ." Strings=" .d ." Struct=" .d + ." Reused str=" fdtfl-strings-reused @ .d + milliseconds fdt-ms @ - .d ." ms" + cr + THEN + + \ Allocate flatten DT blob + dup alloc-mem ( struct-len strings-len total-len fdt ) + >r ( struct-len strings-len total-len r: fdt ) + + \ Write header + OF_DT_HEADER r@ >fdth_magic l! + dup r@ >fdth_tsize l! + /fdth 10 + 2 pick + r@ >fdth_struct_off l! + /fdth 10 + r@ >fdth_string_off l! + /fdth r@ >fdth_rsvmap_off l! + 11 r@ >fdth_version l! + 10 r@ >fdth_compat_vers l! + 0 r@ >fdth_boot_cpu l! + over r@ >fdth_string_size l! + 2 pick r@ >fdth_struct_size l! + ( struct-len strings-len total-len r: fdt ) + + drop ( struct-len strings-len r: fdt ) + r@ /fdth + ( struct-len strings-len cur r: fdt ) + + \ Write the reserved entry + 0 over ! cell+ 0 over ! cell+ + + \ Write strings and struct blobs + fdtfl-strings @ fdt-append-blob + fdtfl-struct @ fdt-append-blob + drop + + \ Free temporary blobs + fdtfl-struct @ 200000 free-mem + fdtfl-strings @ 200000 free-mem + + \ Return fdt + r> +; + +: fdt-flatten-tree-free ( tree ) + dup >fdth_tsize l@ free-mem +; + s" /" find-node fdt-fix-phandles diff --git a/board-qemu/slof/rtas.fs b/board-qemu/slof/rtas.fs index b17157e..f1a4abe 100644 --- a/board-qemu/slof/rtas.fs +++ b/board-qemu/slof/rtas.fs @@ -98,6 +98,12 @@ find-qemu-rtas ; : rtas-quiesce ( -- ) + fdt-flatten-tree + dup hv-update-dt ?dup IF + \ Ignore hcall not implemented error, print error otherwise + dup -2 <> IF ." HV-UPDATE-DT error: " . cr ELSE drop THEN + THEN + fdt-flatten-tree-free " quiesce" rtas-get-token rtas-cb rtas>token l! 0 rtas-cb rtas>nargs l! 0 rtas-cb rtas>nret l! diff --git a/lib/libhvcall/hvcall.code b/lib/libhvcall/hvcall.code index 744469f..5918c90 100644 --- a/lib/libhvcall/hvcall.code +++ b/lib/libhvcall/hvcall.code @@ -123,3 +123,8 @@ PRIM(check_X2d_and_X2d_patch_X2d_sc1) patch_broken_sc1((void*)start, (void*)end, (void*)patch_ins); MIRP + +PRIM(hv_X2d_update_X2d_dt) + unsigned long dt = TOS.u; + TOS.u = hv_generic(KVMPPC_H_UPDATE_DT, dt); +MIRP diff --git a/lib/libhvcall/hvcall.in b/lib/libhvcall/hvcall.in index e99d6d1..9193162 100644 --- a/lib/libhvcall/hvcall.in +++ b/lib/libhvcall/hvcall.in @@ -30,4 +30,5 @@ cod(RX!) cod(hv-logical-memop) cod(hv-cas) +cod(hv-update-dt) cod(get-print-version)
This creates flatten device tree and passes it to QEMU via a custom hypercall right before jumping to RTAS. This preloads strings with 40 property names from CPU and PCI device nodes and the strings lookup only searches within these. Test results on a guest with 256 CPUs and 256 virtual Intel E1000 devices running on a POWER8 box: - the patch as it is: FDTsize=366024 Strings=15888 Struct=350080 Reused str=12457 338 ms - no strings search, simply add them all to the strings blob: FDTsize=548720 Strings=198584 Struct=350080 Reused str=0 154 ms A simple guest (one CPU, no PCI) with this patch as is: FDTsize=15940 Strings=3148 Struct=12736 Reused str=84 10 ms While we are here, fix the version handling in fdt-init. It only matters a little for the fdt-debug==1 case though. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- Changes: v5: * applied latest comments from Segher * s/fdt-property/fdt-copy-property/, s/fdt-properties/fdt-copy-properties/ * reduced the temporary buffers to 1MB each as the guest uses 1MB in total anyway * do not pass root phandle to fdt-flatten-tree, it fetches it from device-tree itself * reworked fdt-copy-properties to use for-all-words proposed by Segher v4: * reworked fdt-properties, works lot faster * do not store "name" properties as nodes have names already v3: * fixed stack handling after hcall returned * fixed format versions in both rendering and parsing paths * rebased on top of removed unused hvcalls * renamed used variables to have fdtfl- prefixes as there are already some for parsing the initial dt v2: * fixed comments from review * added strings cache * changed last_compat_vers from 0x17 to 0x16 as suggested by dwg --- I tested the blob by storing it from QEMU to a file and decompiling it. --- lib/libhvcall/libhvcall.h | 3 +- board-qemu/slof/fdt.fs | 267 +++++++++++++++++++++++++++++++++++++++++++++- board-qemu/slof/rtas.fs | 6 ++ lib/libhvcall/hvcall.code | 5 + lib/libhvcall/hvcall.in | 1 + 5 files changed, 279 insertions(+), 3 deletions(-)