| Message ID | 20171208053251.9958-1-nikunj@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] boot: do not use catpad to concatenate strings | expand |
On 08/12/17 16:32, Nikunj A Dadhania wrote: > The catpad size is 1K size, which can be hit easily hit with around 20 devices > with bootindex. > > Open code EVALUATE such that concatenation is not required. Replace usage of > $cat with a dynamically allocated buffer(16K) here. > > Reported here: https://github.com/qemu/SLOF/issues/3 > > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> I've read the v1..v3 of this patch and (to my embarrassment) I do not understand how this works at all :) Above you said "concatenation is not required" but there is bootdev-string-cat, how is that not concat? We did cat to bootdevice, now - to bootdev-string-cat, but this is not just it? For example: 0 > $bootdev type /pci@800000020000000/ethernet@1 /pci@800000020000000/ethernet@2 disk cdrom net net1 ok Thanks. > > --- > > qemu-system-ppc64 -nographic -nodefaults -serial stdio -monitor pty -m 2G \ > -device virtio-scsi-pci \ > `for ((i=2;i<=50;i++)) ; \ > do echo -n " -drive file=/tmp/storage$i.qcow2,if=none,id=drive$i,format=qcow2 \ > -device > scsi-hd,drive=drive$i,id=disk$i,channel=0,scsi-id=0,lun=$i,bootindex=$i"; \ > done;` \ > -drive file=guest.disk,if=none,id=drv1,format=qcow2,cache=none \ > -device scsi-hd,drive=drv1,bootindex=1 -boot menu=on,splash-time=3000,strict=on > --- > slof/fs/boot.fs | 53 ++++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 48 insertions(+), 5 deletions(-) > > diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs > index 1fd7439..6d16c54 100644 > --- a/slof/fs/boot.fs > +++ b/slof/fs/boot.fs > @@ -15,8 +15,27 @@ > VARIABLE state-valid false state-valid ! > CREATE go-args 2 cells allot go-args 2 cells erase > > +4000 CONSTANT bootdev-size > +0 VALUE bootdev-buf > + > \ \\\\\\\\\\\\\\ Structure/Implementation Dependent Methods > > +: alloc-bootdev-buf ( -- ) > + bootdev-size alloc-mem ?dup 0= ABORT" Unable to allocate bootdev buffer!" > + dup bootdev-size erase > + to bootdev-buf > +; > + > +: free-bootdev-buf ( -- ) > + bootdev-buf bootdev-size free-mem > + 0 to bootdev-buf > +; > + > +: bootdev-string-cat ( addr1 len1 addr2 len2 -- addr1 len1+len2 ) > + dup 3 pick + bootdev-size > ABORT" bootdev size too big!" > + string-cat > +; > + > : $bootargs > bootargs 2@ ?dup IF > ELSE s" diagnostic-mode?" evaluate and IF s" diag-file" evaluate > @@ -24,14 +43,23 @@ CREATE go-args 2 cells allot go-args 2 cells erase > ; > > : $bootdev ( -- device-name len ) > - bootdevice 2@ dup IF s" " $cat THEN > + alloc-bootdev-buf > + bootdevice 2@ ?dup IF > + swap bootdev-buf 2 pick move > + bootdev-buf swap s" " bootdev-string-cat > + ELSE > + \ use bootdev-buf for concatenating diag mode/boot-device if any > + drop bootdev-buf 0 > + THEN > s" diagnostic-mode?" evaluate IF > s" diag-device" evaluate > ELSE > s" boot-device" evaluate > THEN > - $cat \ prepend bootdevice setting from vpd-bootlist > + ( bootdev len str len1 ) > + bootdev-string-cat \ concatenate both > strdup > + free-bootdev-buf > ?dup 0= IF > disable-watchdog > drop true ABORT" No boot device!" > @@ -51,7 +79,14 @@ CREATE go-args 2 cells allot go-args 2 cells erase > ' (set-boot-device) to set-boot-device > > : (add-boot-device) ( str len -- ) \ Concatenate " str" to "bootdevice" > - bootdevice 2@ ?dup IF $cat-space ELSE drop THEN set-boot-device > + bootdevice 2@ ?dup IF > + alloc-bootdev-buf > + swap bootdev-buf 2 pick move > + bootdev-buf swap s" " bootdev-string-cat > + 2swap bootdev-string-cat > + ELSE drop THEN > + set-boot-device > + bootdev-buf 0 <> IF free-bootdev-buf THEN > ; > > ' (add-boot-device) to add-boot-device > @@ -221,11 +256,19 @@ defer go ( -- ) > ELSE > drop > THEN > - set-boot-args s" parse-load " $bootdev $cat strdup evaluate > + set-boot-args > + save-source -1 to source-id > + $bootdev dup #ib ! span ! to ib > + 0 >in ! > + ['] parse-load catch restore-source throw > ; > > : load-next ( -- success ) \ Continue after go failed > - load-list 2@ ?dup IF s" parse-load " 2swap $cat strdup evaluate > + load-list 2@ ?dup IF > + save-source -1 to source-id > + dup #ib ! span ! to ib > + 0 >in ! > + ['] parse-load catch restore-source throw > ELSE drop false THEN > ; > >
Hi Alexey, Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 08/12/17 16:32, Nikunj A Dadhania wrote: >> The catpad size is 1K size, which can be hit easily hit with around 20 devices >> with bootindex. >> >> Open code EVALUATE such that concatenation is not required. Replace usage of >> $cat with a dynamically allocated buffer(16K) here. >> >> Reported here: https://github.com/qemu/SLOF/issues/3 >> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > > I've read the v1..v3 of this patch and (to my embarrassment) I do not > understand how this works at all :) > > Above you said "concatenation is not required" but there is > bootdev-string-cat, how is that not concat? We did cat to bootdevice, now - > to bootdev-string-cat, but this is not just it? Earlier the concatenation was done using a common buffer $catpad slof/fs/base.fs:CREATE $catpad 400 allot Which had a limitation of 1K, my first patch was to increase the size of $catpad and Segher said that $catpad was kept small on purpose for concatenating quickly without much overhead. Version-2 patch did following things: 1) introduced a similar static buffer for bootdev concatenation without using $cat. 2) Replaced concatenation in LOAD and LOAD-NEXT by open coding EVALUATE word. Version-3 made the static buffer allocate from heap instead of a static buffer. > For example: > > 0 > $bootdev type /pci@800000020000000/ethernet@1 > /pci@800000020000000/ethernet@2 disk cdrom net net1 ok HTH Regards Nikunj
On 11/12/17 17:30, Nikunj A Dadhania wrote: > Hi Alexey, > > Alexey Kardashevskiy <aik@ozlabs.ru> writes: > >> On 08/12/17 16:32, Nikunj A Dadhania wrote: >>> The catpad size is 1K size, which can be hit easily hit with around 20 devices >>> with bootindex. >>> >>> Open code EVALUATE such that concatenation is not required. Replace usage of >>> $cat with a dynamically allocated buffer(16K) here. >>> >>> Reported here: https://github.com/qemu/SLOF/issues/3 >>> >>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >> >> I've read the v1..v3 of this patch and (to my embarrassment) I do not >> understand how this works at all :) >> >> Above you said "concatenation is not required" but there is >> bootdev-string-cat, how is that not concat? We did cat to bootdevice, now - >> to bootdev-string-cat, but this is not just it? > > Earlier the concatenation was done using a common buffer $catpad > > slof/fs/base.fs:CREATE $catpad 400 allot > > Which had a limitation of 1K, my first patch was to increase the size of > $catpad and Segher said that $catpad was kept small on purpose for > concatenating quickly without much overhead. > > Version-2 patch did following things: > > 1) introduced a similar static buffer for bootdev concatenation without > using $cat. This part I understood :) > 2) Replaced concatenation in LOAD and LOAD-NEXT by open coding EVALUATE word. This part I do not understand - why is this change needed or how does it make it better? There is a global list of boot devices - bootdevice; and for some reason now there is another one - bootdev-buf, both are strings... And there is also a load-list list which is what for? :) > > Version-3 made the static buffer allocate from heap instead of a static > buffer. > >> For example: >> >> 0 > $bootdev type /pci@800000020000000/ethernet@1 >> /pci@800000020000000/ethernet@2 disk cdrom net net1 ok > > HTH > > Regards > Nikunj >
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 11/12/17 17:30, Nikunj A Dadhania wrote: >> Hi Alexey, >> >> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >> >>> On 08/12/17 16:32, Nikunj A Dadhania wrote: >>>> The catpad size is 1K size, which can be hit easily hit with around 20 devices >>>> with bootindex. >>>> >>>> Open code EVALUATE such that concatenation is not required. Replace usage of >>>> $cat with a dynamically allocated buffer(16K) here. >>>> >>>> Reported here: https://github.com/qemu/SLOF/issues/3 >>>> >>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>> >>> I've read the v1..v3 of this patch and (to my embarrassment) I do not >>> understand how this works at all :) >>> >>> Above you said "concatenation is not required" but there is >>> bootdev-string-cat, how is that not concat? We did cat to bootdevice, now - >>> to bootdev-string-cat, but this is not just it? >> >> Earlier the concatenation was done using a common buffer $catpad >> >> slof/fs/base.fs:CREATE $catpad 400 allot >> >> Which had a limitation of 1K, my first patch was to increase the size of >> $catpad and Segher said that $catpad was kept small on purpose for >> concatenating quickly without much overhead. >> >> Version-2 patch did following things: >> >> 1) introduced a similar static buffer for bootdev concatenation without >> using $cat. > > This part I understood :) > > >> 2) Replaced concatenation in LOAD and LOAD-NEXT by open coding EVALUATE word. > > This part I do not understand - why is this change needed or how does it > make it better? Because we were concatenating and then evaluating during runtime, newer code plays directly with args and populates the stack and calls parse-load, so concatenation is not required anymore. > There is a global list of boot devices - bootdevice; and for some reason > now there is another one - bootdev-buf, both are strings... No, bootdev-buf is a temporary buffer, its not replacing bootdevice. > And there is also a load-list list which is what for? :) Not sure what you are referring to. Regards Nikunj
Hi! On Mon, Dec 11, 2017 at 12:00:25PM +0530, Nikunj A Dadhania wrote: > Alexey Kardashevskiy <aik@ozlabs.ru> writes: > > I've read the v1..v3 of this patch and (to my embarrassment) I do not > > understand how this works at all :) > > > > Above you said "concatenation is not required" but there is > > bootdev-string-cat, how is that not concat? We did cat to bootdevice, now - > > to bootdev-string-cat, but this is not just it? > > Earlier the concatenation was done using a common buffer $catpad > > slof/fs/base.fs:CREATE $catpad 400 allot > > Which had a limitation of 1K, my first patch was to increase the size of > $catpad and Segher said that $catpad was kept small on purpose for > concatenating quickly without much overhead. No, I said you do not concatenate much, on purpose. This is to avoid quadratic time. Since the way the code used $cat is bad, it doesn't matter if the buffer was too small for it; it is big enough for all legitimate users. I don't know if your patch achieves proper scaling. Segher
On 11/12/17 19:57, Nikunj A Dadhania wrote: > Alexey Kardashevskiy <aik@ozlabs.ru> writes: > >> On 11/12/17 17:30, Nikunj A Dadhania wrote: >>> Hi Alexey, >>> >>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>> >>>> On 08/12/17 16:32, Nikunj A Dadhania wrote: >>>>> The catpad size is 1K size, which can be hit easily hit with around 20 devices >>>>> with bootindex. >>>>> >>>>> Open code EVALUATE such that concatenation is not required. Replace usage of >>>>> $cat with a dynamically allocated buffer(16K) here. >>>>> >>>>> Reported here: https://github.com/qemu/SLOF/issues/3 >>>>> >>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>> >>>> I've read the v1..v3 of this patch and (to my embarrassment) I do not >>>> understand how this works at all :) >>>> >>>> Above you said "concatenation is not required" but there is >>>> bootdev-string-cat, how is that not concat? We did cat to bootdevice, now - >>>> to bootdev-string-cat, but this is not just it? >>> >>> Earlier the concatenation was done using a common buffer $catpad >>> >>> slof/fs/base.fs:CREATE $catpad 400 allot >>> >>> Which had a limitation of 1K, my first patch was to increase the size of >>> $catpad and Segher said that $catpad was kept small on purpose for >>> concatenating quickly without much overhead. >>> >>> Version-2 patch did following things: >>> >>> 1) introduced a similar static buffer for bootdev concatenation without >>> using $cat. >> >> This part I understood :) >> >> >>> 2) Replaced concatenation in LOAD and LOAD-NEXT by open coding EVALUATE word. >> >> This part I do not understand - why is this change needed or how does it >> make it better? > > Because we were concatenating and then evaluating during runtime, newer > code plays directly with args and populates the stack and calls > parse-load, so concatenation is not required anymore. This must be some different concatenation then as the patch uses new bootdev-buf for concatenation. >> There is a global list of boot devices - bootdevice; and for some reason >> now there is another one - bootdev-buf, both are strings... > > No, bootdev-buf is a temporary buffer, its not replacing bootdevice. Ufff. I am lost now. $bootdev adds something to the dictionary (via strdup), and so does (set-boot-device), and "bootdevice" points to what? Since you understand this stuff, can you please outline what is called what and where it is all stored? (add-boot-device) is called when devices are discovered, the result is in the dictionary, "bootdevice" points to a concatenated string. What happens then? > > >> And there is also a load-list list which is what for? :) > > Not sure what you are referring to. slof/fs/boot.fs : : load-next ( -- success ). \ Continue after go failed load-list 2@ ?dup IF save-source -1 to source-id dup #ib ! span ! to ib 0 >in ! ['] parse-load catch restore-source throw ELSE drop false THEN ; This uses "load-list" - what is that for? I would have pushed this out already if they were 2 patches - one replacing $cat with bootdev-string-cat and the other doing this open coded thing but I do not know if these parts are independent and where is the line :)
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 11/12/17 19:57, Nikunj A Dadhania wrote: >> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >> >>> On 11/12/17 17:30, Nikunj A Dadhania wrote: >>>> Hi Alexey, >>>> >>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>>> >>>>> On 08/12/17 16:32, Nikunj A Dadhania wrote: >>>>>> The catpad size is 1K size, which can be hit easily hit with around 20 devices >>>>>> with bootindex. >>>>>> >>>>>> Open code EVALUATE such that concatenation is not required. Replace usage of >>>>>> $cat with a dynamically allocated buffer(16K) here. >>>>>> >>>>>> Reported here: https://github.com/qemu/SLOF/issues/3 >>>>>> >>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>> >>>>> I've read the v1..v3 of this patch and (to my embarrassment) I do not >>>>> understand how this works at all :) >>>>> >>>>> Above you said "concatenation is not required" but there is >>>>> bootdev-string-cat, how is that not concat? We did cat to bootdevice, now - >>>>> to bootdev-string-cat, but this is not just it? >>>> >>>> Earlier the concatenation was done using a common buffer $catpad >>>> >>>> slof/fs/base.fs:CREATE $catpad 400 allot >>>> >>>> Which had a limitation of 1K, my first patch was to increase the size of >>>> $catpad and Segher said that $catpad was kept small on purpose for >>>> concatenating quickly without much overhead. >>>> >>>> Version-2 patch did following things: >>>> >>>> 1) introduced a similar static buffer for bootdev concatenation without >>>> using $cat. >>> >>> This part I understood :) >>> >>> >>>> 2) Replaced concatenation in LOAD and LOAD-NEXT by open coding EVALUATE word. >>> >>> This part I do not understand - why is this change needed or how does it >>> make it better? >> >> Because we were concatenating and then evaluating during runtime, newer >> code plays directly with args and populates the stack and calls >> parse-load, so concatenation is not required anymore. > > > This must be some different concatenation then as the patch uses new > bootdev-buf for concatenation. Let us take the example: set-boot-args s" parse-load " $bootdev $cat strdup evaluate ^^^^^^^^^^^ We were concatenating the word " parse-load" and our bootdevice list that was input to evaluate. In the discussion with Segher, he suggested to open code evaluate, that eliminates the requirement for concatenation. Newer "load" does not use $cat anymore. set-boot-args save-source -1 to source-id $bootdev dup #ib ! span ! to ib 0 >in ! ['] parse-load catch restore-source throw > >>> There is a global list of boot devices - bootdevice; and for some reason >>> now there is another one - bootdev-buf, both are strings... >> >> No, bootdev-buf is a temporary buffer, its not replacing bootdevice. > > Ufff. I am lost now. $bootdev adds something to the dictionary (via > strdup), and so does (set-boot-device), and "bootdevice" points to > what? slof/fs/loaders.fs:CREATE bootdevice 2 cells allot bootdevice 2 cells erase Its where you store the boot device string and its length. Whenever you read/update the list of boot devices, this is where finally the list is updated. Something sort of a pointer + length > Since you understand this stuff, can you please outline what is called what > and where it is all stored? > > (add-boot-device) is called when devices are discovered, the result is in > the dictionary, "bootdevice" points to a concatenated string. What happens > then? At every discovery of boot device, old list of boot device and new one is concatenated and stored back in bootdevice. During the loading stage, this list is passed as argument to parse-load. It goes one by one through this list trying to boot: There are three conditions: 1) Fails loading (not bootable device) 2) Execption during loading (bootable device but something went wrong) 3) Successfully loaded (boots to the guest) For first case we just go to next device. Second case is caught in "boot" word and we need to resume back from the next eligible boot device. That is where load-next comes into picture, but till what point did we complete in the list of boot devices? That info is there in load-list. And finally if nothing works we drop to slof prompt. Thomas/Segher, please feel free to add/comment >>> And there is also a load-list list which is what for? :) >> >> Not sure what you are referring to. > > slof/fs/boot.fs : > > : load-next ( -- success ). \ Continue after go failed > load-list 2@ ?dup IF > save-source -1 to source-id > dup #ib ! span ! to ib > 0 >in ! > ['] parse-load catch restore-source throw > ELSE drop false THEN > ; > This uses "load-list" - what is that for? Explained above. > I would have pushed this out already if they were 2 patches - one replacing > $cat with bootdev-string-cat and the other doing this open coded thing but > I do not know if these parts are independent and where is the line :) Yes, i was thinking on that line as well. Maybe easier to read them. The will be solved with both the patches though. Regards Nikunj
On 12/12/17 14:47, Nikunj A Dadhania wrote: > Alexey Kardashevskiy <aik@ozlabs.ru> writes: > >> On 11/12/17 19:57, Nikunj A Dadhania wrote: >>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>> >>>> On 11/12/17 17:30, Nikunj A Dadhania wrote: >>>>> Hi Alexey, >>>>> >>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>>>> >>>>>> On 08/12/17 16:32, Nikunj A Dadhania wrote: >>>>>>> The catpad size is 1K size, which can be hit easily hit with around 20 devices >>>>>>> with bootindex. >>>>>>> >>>>>>> Open code EVALUATE such that concatenation is not required. Replace usage of >>>>>>> $cat with a dynamically allocated buffer(16K) here. >>>>>>> >>>>>>> Reported here: https://github.com/qemu/SLOF/issues/3 >>>>>>> >>>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>>> >>>>>> I've read the v1..v3 of this patch and (to my embarrassment) I do not >>>>>> understand how this works at all :) >>>>>> >>>>>> Above you said "concatenation is not required" but there is >>>>>> bootdev-string-cat, how is that not concat? We did cat to bootdevice, now - >>>>>> to bootdev-string-cat, but this is not just it? >>>>> >>>>> Earlier the concatenation was done using a common buffer $catpad >>>>> >>>>> slof/fs/base.fs:CREATE $catpad 400 allot >>>>> >>>>> Which had a limitation of 1K, my first patch was to increase the size of >>>>> $catpad and Segher said that $catpad was kept small on purpose for >>>>> concatenating quickly without much overhead. >>>>> >>>>> Version-2 patch did following things: >>>>> >>>>> 1) introduced a similar static buffer for bootdev concatenation without >>>>> using $cat. >>>> >>>> This part I understood :) >>>> >>>> >>>>> 2) Replaced concatenation in LOAD and LOAD-NEXT by open coding EVALUATE word. >>>> >>>> This part I do not understand - why is this change needed or how does it >>>> make it better? >>> >>> Because we were concatenating and then evaluating during runtime, newer >>> code plays directly with args and populates the stack and calls >>> parse-load, so concatenation is not required anymore. >> >> >> This must be some different concatenation then as the patch uses new >> bootdev-buf for concatenation. > > Let us take the example: > > set-boot-args s" parse-load " $bootdev $cat strdup evaluate > ^^^^^^^^^^^ > > We were concatenating the word " parse-load" and our bootdevice list > that was input to evaluate. In the discussion with Segher, he suggested > to open code evaluate, that eliminates the requirement for > concatenation. Newer "load" does not use $cat anymore. > > set-boot-args > save-source -1 to source-id > $bootdev dup #ib ! span ! to ib > 0 >in ! > ['] parse-load catch restore-source throw Ah, I see... Hm. But why do not we just call parse-load directly, without evaluate or this really not obvious open coded version of evaluate? :) It all looks unnecessary complicated :( > > >> >>>> There is a global list of boot devices - bootdevice; and for some reason >>>> now there is another one - bootdev-buf, both are strings... >>> >>> No, bootdev-buf is a temporary buffer, its not replacing bootdevice. >> >> Ufff. I am lost now. $bootdev adds something to the dictionary (via >> strdup), and so does (set-boot-device), and "bootdevice" points to >> what? > > slof/fs/loaders.fs:CREATE bootdevice 2 cells allot bootdevice 2 cells erase > > Its where you store the boot device string and its length. Whenever you > read/update the list of boot devices, this is where finally the list is > updated. Something sort of a pointer + length > > >> Since you understand this stuff, can you please outline what is called what >> and where it is all stored? >> >> (add-boot-device) is called when devices are discovered, the result is in >> the dictionary, "bootdevice" points to a concatenated string. What happens >> then? > > At every discovery of boot device, old list of boot device and new one > is concatenated and stored back in bootdevice. > > During the loading stage, this list is passed as argument to parse-load. > It goes one by one through this list trying to boot: > > There are three conditions: > 1) Fails loading (not bootable device) > 2) Execption during loading (bootable device but something went wrong) > 3) Successfully loaded (boots to the guest) > > > For first case we just go to next device. Second case is caught in > "boot" word and we need to resume back from the next eligible boot > device. That is where load-next comes into picture, but till what point > did we complete in the list of boot devices? That info is there in > load-list. I have $bootdev = "/pci@800000020000000/ethernet@1 /pci@800000020000000/ethernet@2 disk cdrom net net1" and I see slof trying them all but I do not see load-next called at all, hence my question... > > And finally if nothing works we drop to slof prompt. > > Thomas/Segher, please feel free to add/comment > >>>> And there is also a load-list list which is what for? :) >>> >>> Not sure what you are referring to. >> >> slof/fs/boot.fs : >> >> : load-next ( -- success ). \ Continue after go failed >> load-list 2@ ?dup IF >> save-source -1 to source-id >> dup #ib ! span ! to ib >> 0 >in ! >> ['] parse-load catch restore-source throw >> ELSE drop false THEN >> ; > > >> This uses "load-list" - what is that for? > > Explained above. > >> I would have pushed this out already if they were 2 patches - one replacing >> $cat with bootdev-string-cat and the other doing this open coded thing but >> I do not know if these parts are independent and where is the line :) > > Yes, i was thinking on that line as well. Maybe easier to read them. The > will be solved with both the patches though. > > Regards > Nikunj >
On Tue, Dec 12, 2017 at 09:17:25AM +0530, Nikunj A Dadhania wrote:
> slof/fs/loaders.fs:CREATE bootdevice 2 cells allot bootdevice 2 cells erase
That is just
CREATE bootdevice 0 , 0 ,
which is a bit easier to read ;-)
Segher
On Tue, Dec 12, 2017 at 05:44:04PM +1100, Alexey Kardashevskiy wrote: > > We were concatenating the word " parse-load" and our bootdevice list > > that was input to evaluate. In the discussion with Segher, he suggested > > to open code evaluate, that eliminates the requirement for > > concatenation. Newer "load" does not use $cat anymore. > > > > set-boot-args > > save-source -1 to source-id > > $bootdev dup #ib ! span ! to ib > > 0 >in ! > > ['] parse-load catch restore-source throw > > > Ah, I see... Hm. But why do not we just call parse-load directly, without > evaluate or this really not obvious open coded version of evaluate? :) > > It all looks unnecessary complicated :( Since parse-load reads from the input device, via the parse area, you need to set up your own, and save / restore it around it. The CATCH is to ensure the restore happens even if something down in parse-load calls THROW (or ABORT etc.). I did suggest to have this factored out so you could do something like $bootdev ['] parse-load execute-with-input Segher
On 13/12/17 04:29, Segher Boessenkool wrote: > On Tue, Dec 12, 2017 at 05:44:04PM +1100, Alexey Kardashevskiy wrote: >>> We were concatenating the word " parse-load" and our bootdevice list >>> that was input to evaluate. In the discussion with Segher, he suggested >>> to open code evaluate, that eliminates the requirement for >>> concatenation. Newer "load" does not use $cat anymore. >>> >>> set-boot-args >>> save-source -1 to source-id >>> $bootdev dup #ib ! span ! to ib >>> 0 >in ! >>> ['] parse-load catch restore-source throw >> >> >> Ah, I see... Hm. But why do not we just call parse-load directly, without >> evaluate or this really not obvious open coded version of evaluate? :) >> >> It all looks unnecessary complicated :( > > Since parse-load reads from the input device, via the parse area, you > need to set up your own, and save / restore it around it. > > The CATCH is to ensure the restore happens even if something down in > parse-load calls THROW (or ABORT etc.). > > I did suggest to have this factored out so you could do something like > > $bootdev ['] parse-load execute-with-input Factor out "de-alias do-load" bits from parse-load and call it directly, may be?
On Wed, Dec 13, 2017 at 12:51:24PM +1100, Alexey Kardashevskiy wrote: > >> Ah, I see... Hm. But why do not we just call parse-load directly, without > >> evaluate or this really not obvious open coded version of evaluate? :) > >> > >> It all looks unnecessary complicated :( > > > > Since parse-load reads from the input device, via the parse area, you > > need to set up your own, and save / restore it around it. > > > > The CATCH is to ensure the restore happens even if something down in > > parse-load calls THROW (or ABORT etc.). > > > > I did suggest to have this factored out so you could do something like > > > > $bootdev ['] parse-load execute-with-input > > > Factor out "de-alias do-load" bits from parse-load and call it directly, > may be? I think that's a lot of refactoring... But if you feel up to it, sure, that should improve the code :-) Segher
diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs index 1fd7439..6d16c54 100644 --- a/slof/fs/boot.fs +++ b/slof/fs/boot.fs @@ -15,8 +15,27 @@ VARIABLE state-valid false state-valid ! CREATE go-args 2 cells allot go-args 2 cells erase +4000 CONSTANT bootdev-size +0 VALUE bootdev-buf + \ \\\\\\\\\\\\\\ Structure/Implementation Dependent Methods +: alloc-bootdev-buf ( -- ) + bootdev-size alloc-mem ?dup 0= ABORT" Unable to allocate bootdev buffer!" + dup bootdev-size erase + to bootdev-buf +; + +: free-bootdev-buf ( -- ) + bootdev-buf bootdev-size free-mem + 0 to bootdev-buf +; + +: bootdev-string-cat ( addr1 len1 addr2 len2 -- addr1 len1+len2 ) + dup 3 pick + bootdev-size > ABORT" bootdev size too big!" + string-cat +; + : $bootargs bootargs 2@ ?dup IF ELSE s" diagnostic-mode?" evaluate and IF s" diag-file" evaluate @@ -24,14 +43,23 @@ CREATE go-args 2 cells allot go-args 2 cells erase ; : $bootdev ( -- device-name len ) - bootdevice 2@ dup IF s" " $cat THEN + alloc-bootdev-buf + bootdevice 2@ ?dup IF + swap bootdev-buf 2 pick move + bootdev-buf swap s" " bootdev-string-cat + ELSE + \ use bootdev-buf for concatenating diag mode/boot-device if any + drop bootdev-buf 0 + THEN s" diagnostic-mode?" evaluate IF s" diag-device" evaluate ELSE s" boot-device" evaluate THEN - $cat \ prepend bootdevice setting from vpd-bootlist + ( bootdev len str len1 ) + bootdev-string-cat \ concatenate both strdup + free-bootdev-buf ?dup 0= IF disable-watchdog drop true ABORT" No boot device!" @@ -51,7 +79,14 @@ CREATE go-args 2 cells allot go-args 2 cells erase ' (set-boot-device) to set-boot-device : (add-boot-device) ( str len -- ) \ Concatenate " str" to "bootdevice" - bootdevice 2@ ?dup IF $cat-space ELSE drop THEN set-boot-device + bootdevice 2@ ?dup IF + alloc-bootdev-buf + swap bootdev-buf 2 pick move + bootdev-buf swap s" " bootdev-string-cat + 2swap bootdev-string-cat + ELSE drop THEN + set-boot-device + bootdev-buf 0 <> IF free-bootdev-buf THEN ; ' (add-boot-device) to add-boot-device @@ -221,11 +256,19 @@ defer go ( -- ) ELSE drop THEN - set-boot-args s" parse-load " $bootdev $cat strdup evaluate + set-boot-args + save-source -1 to source-id + $bootdev dup #ib ! span ! to ib + 0 >in ! + ['] parse-load catch restore-source throw ; : load-next ( -- success ) \ Continue after go failed - load-list 2@ ?dup IF s" parse-load " 2swap $cat strdup evaluate + load-list 2@ ?dup IF + save-source -1 to source-id + dup #ib ! span ! to ib + 0 >in ! + ['] parse-load catch restore-source throw ELSE drop false THEN ;
The catpad size is 1K size, which can be hit easily hit with around 20 devices with bootindex. Open code EVALUATE such that concatenation is not required. Replace usage of $cat with a dynamically allocated buffer(16K) here. Reported here: https://github.com/qemu/SLOF/issues/3 Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> --- qemu-system-ppc64 -nographic -nodefaults -serial stdio -monitor pty -m 2G \ -device virtio-scsi-pci \ `for ((i=2;i<=50;i++)) ; \ do echo -n " -drive file=/tmp/storage$i.qcow2,if=none,id=drive$i,format=qcow2 \ -device scsi-hd,drive=drive$i,id=disk$i,channel=0,scsi-id=0,lun=$i,bootindex=$i"; \ done;` \ -drive file=guest.disk,if=none,id=drv1,format=qcow2,cache=none \ -device scsi-hd,drive=drv1,bootindex=1 -boot menu=on,splash-time=3000,strict=on --- slof/fs/boot.fs | 53 ++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 48 insertions(+), 5 deletions(-)