| Message ID | 1469593067-20872-1-git-send-email-nikunj@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jul 27, 2016 at 09:47:47AM +0530, Nikunj A Dadhania wrote: > With the addition of cpu hotplug in QEMU, cpu@0 can be removed as well. > SLOF should not depend on it. Find the first child in the "/cpus" node > and get the timer base frequency and set it as the chosen cpu as well > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> There are problems within qemu for removing cpu 0 at the moment, and they probably won't be fixed for a while, but we might as well fix SLOF in the meantime. > --- > board-qemu/slof/tree.fs | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/board-qemu/slof/tree.fs b/board-qemu/slof/tree.fs > index 78dafab..5912c1c 100644 > --- a/board-qemu/slof/tree.fs > +++ b/board-qemu/slof/tree.fs > @@ -45,7 +45,9 @@ device-end > > \ Fixup timebase frequency from device-tree > : fixup-tbfreq > - " /cpus/@0" find-device > + " /cpus" find-device > + get-node child ?dup 0= IF ABORT" start-cpu not found" THEN > + set-node > " timebase-frequency" get-node get-package-property IF > 2drop > ELSE > @@ -167,7 +169,14 @@ populate-pci-busses > > 6c0 cp > > -s" /cpus/@0" open-dev encode-int s" cpu" set-chosen > +\ Do not assume that cpu0 is available > +: set-chosen-cpu > + " /cpus" find-device > + get-node child ?dup 0= IF ABORT" start-cpu not found" THEN > + node>path open-dev encode-int s" cpu" set-chosen > +; > +set-chosen-cpu > + > s" /memory@0" open-dev encode-int s" memory" set-chosen > > 6e0 cp
David Gibson <david@gibson.dropbear.id.au> writes: > [ Unknown signature status ] > On Wed, Jul 27, 2016 at 09:47:47AM +0530, Nikunj A Dadhania wrote: >> With the addition of cpu hotplug in QEMU, cpu@0 can be removed as well. >> SLOF should not depend on it. Find the first child in the "/cpus" node >> and get the timer base frequency and set it as the chosen cpu as well >> >> Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > > There are problems within qemu for removing cpu 0 at the moment, and > they probably won't be fixed for a while, but we might as well fix > SLOF in the meantime. Right, and was a simple fix, so thought of pushing it. Regards Nikunj
On Wed, Jul 27, 2016 at 09:47:47AM +0530, Nikunj A Dadhania wrote: > \ Fixup timebase frequency from device-tree > : fixup-tbfreq > - " /cpus/@0" find-device > + " /cpus" find-device > + get-node child ?dup 0= IF ABORT" start-cpu not found" THEN > + set-node ABORT" has an IF built in. It also empties the stack, so this is just get-node child dup 0= ABORT" start-cpu not found" What guarantees this finds the "start-cpu"? Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Wed, Jul 27, 2016 at 09:47:47AM +0530, Nikunj A Dadhania wrote: >> \ Fixup timebase frequency from device-tree >> : fixup-tbfreq >> - " /cpus/@0" find-device >> + " /cpus" find-device >> + get-node child ?dup 0= IF ABORT" start-cpu not found" THEN >> + set-node > > ABORT" has an IF built in. It also empties the stack, so this is just > > get-node child dup 0= ABORT" start-cpu not found" Sure. > What guarantees this finds the "start-cpu"? QEMU does not allow to remove last CPU. Regards Nikunj
On Wed, Jul 27, 2016 at 12:06:06PM +0530, Nikunj A Dadhania wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > > On Wed, Jul 27, 2016 at 09:47:47AM +0530, Nikunj A Dadhania wrote: > >> \ Fixup timebase frequency from device-tree > >> : fixup-tbfreq > >> - " /cpus/@0" find-device > >> + " /cpus" find-device > >> + get-node child ?dup 0= IF ABORT" start-cpu not found" THEN > >> + set-node > > > > ABORT" has an IF built in. It also empties the stack, so this is just > > > > get-node child dup 0= ABORT" start-cpu not found" > > Sure. > > > What guarantees this finds the "start-cpu"? > > QEMU does not allow to remove last CPU. More to the point, if there are no CPUs, presumably there's nothing to execute this code in the first place.
On Wed, Jul 27, 2016 at 12:06:06PM +0530, Nikunj A Dadhania wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > > On Wed, Jul 27, 2016 at 09:47:47AM +0530, Nikunj A Dadhania wrote: > >> \ Fixup timebase frequency from device-tree > >> : fixup-tbfreq > >> - " /cpus/@0" find-device > >> + " /cpus" find-device > >> + get-node child ?dup 0= IF ABORT" start-cpu not found" THEN > >> + set-node > > > > ABORT" has an IF built in. It also empties the stack, so this is just > > > > get-node child dup 0= ABORT" start-cpu not found" > > Sure. Well, the original code is incorrect: if there happens to be a zero next on stack, it will not abort, and your stack is unbalanced. Whoops. > > What guarantees this finds the "start-cpu"? > > QEMU does not allow to remove last CPU. But that is not the same thing? Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Wed, Jul 27, 2016 at 12:06:06PM +0530, Nikunj A Dadhania wrote: >> Segher Boessenkool <segher@kernel.crashing.org> writes: >> >> > On Wed, Jul 27, 2016 at 09:47:47AM +0530, Nikunj A Dadhania wrote: >> >> \ Fixup timebase frequency from device-tree >> >> : fixup-tbfreq >> >> - " /cpus/@0" find-device >> >> + " /cpus" find-device >> >> + get-node child ?dup 0= IF ABORT" start-cpu not found" THEN >> >> + set-node >> > >> > ABORT" has an IF built in. It also empties the stack, so this is just >> > >> > get-node child dup 0= ABORT" start-cpu not found" >> >> Sure. i understood the IF part, but why not ?dup ? > Well, the original code is incorrect: if there happens to be a zero next > on stack, it will not abort, and your stack is unbalanced. Whoops. " /cpus" find-device ( ) get-node ( node ) child ( 0 | child-node ) ?dup ( 0 | child-node child-node ) 0= ABORT" start-cpu not found " ( child-node ) set-node > >> > What guarantees this finds the "start-cpu"? >> >> QEMU does not allow to remove last CPU. > > But that is not the same thing? Dont get you, can you please elaborate? Regards Nikunj
On Wed, Jul 27, 2016 at 12:42:23PM +0530, Nikunj A Dadhania wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > > On Wed, Jul 27, 2016 at 12:06:06PM +0530, Nikunj A Dadhania wrote: > >> Segher Boessenkool <segher@kernel.crashing.org> writes: > >> > >> > On Wed, Jul 27, 2016 at 09:47:47AM +0530, Nikunj A Dadhania wrote: > >> >> \ Fixup timebase frequency from device-tree > >> >> : fixup-tbfreq > >> >> - " /cpus/@0" find-device > >> >> + " /cpus" find-device > >> >> + get-node child ?dup 0= IF ABORT" start-cpu not found" THEN > >> >> + set-node > >> > > >> > ABORT" has an IF built in. It also empties the stack, so this is just > >> > > >> > get-node child dup 0= ABORT" start-cpu not found" > >> > >> Sure. > > i understood the IF part, but why not ?dup ? It's a tiny bit slower, but much more importantly, it is harder to read. Well, in my opinion, anyway. ?DUP works fine here of course, but why bother, the ABORT will clean the whole stack no matter what. > >> > What guarantees this finds the "start-cpu"? > >> > >> QEMU does not allow to remove last CPU. > > > > But that is not the same thing? > > Dont get you, can you please elaborate? Oh, sorry. The code tries to find *a* cpu. But the error message says "start-cpu not found". The code is not looking for a "start-cpu", a user getting that error message will waste time scratching his head before looking at the code (if he is lucky enough to do look at the code). Just say what is *actually* wrong, what did not work ;-) Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Wed, Jul 27, 2016 at 12:42:23PM +0530, Nikunj A Dadhania wrote: >> Segher Boessenkool <segher@kernel.crashing.org> writes: >> >> > On Wed, Jul 27, 2016 at 12:06:06PM +0530, Nikunj A Dadhania wrote: >> >> Segher Boessenkool <segher@kernel.crashing.org> writes: >> >> >> >> > On Wed, Jul 27, 2016 at 09:47:47AM +0530, Nikunj A Dadhania wrote: >> >> >> \ Fixup timebase frequency from device-tree >> >> >> : fixup-tbfreq >> >> >> - " /cpus/@0" find-device >> >> >> + " /cpus" find-device >> >> >> + get-node child ?dup 0= IF ABORT" start-cpu not found" THEN >> >> >> + set-node >> >> > >> >> > ABORT" has an IF built in. It also empties the stack, so this is just >> >> > >> >> > get-node child dup 0= ABORT" start-cpu not found" >> >> >> >> Sure. >> >> i understood the IF part, but why not ?dup ? > > It's a tiny bit slower, but much more importantly, it is harder to read. > Well, in my opinion, anyway. ?DUP works fine here of course, but why > bother, the ABORT will clean the whole stack no matter what. Thanks for clarification. >> >> > What guarantees this finds the "start-cpu"? >> >> >> >> QEMU does not allow to remove last CPU. >> > >> > But that is not the same thing? >> >> Dont get you, can you please elaborate? > > Oh, sorry. > > The code tries to find *a* cpu. But the error message says "start-cpu > not found". The code is not looking for a "start-cpu", a user getting > that error message will waste time scratching his head before looking > at the code (if he is lucky enough to do look at the code). > > Just say what is *actually* wrong, what did not work ;-) Crystal clear now :-) " CPU not found" Regards, Nikunj
diff --git a/board-qemu/slof/tree.fs b/board-qemu/slof/tree.fs index 78dafab..5912c1c 100644 --- a/board-qemu/slof/tree.fs +++ b/board-qemu/slof/tree.fs @@ -45,7 +45,9 @@ device-end \ Fixup timebase frequency from device-tree : fixup-tbfreq - " /cpus/@0" find-device + " /cpus" find-device + get-node child ?dup 0= IF ABORT" start-cpu not found" THEN + set-node " timebase-frequency" get-node get-package-property IF 2drop ELSE @@ -167,7 +169,14 @@ populate-pci-busses 6c0 cp -s" /cpus/@0" open-dev encode-int s" cpu" set-chosen +\ Do not assume that cpu0 is available +: set-chosen-cpu + " /cpus" find-device + get-node child ?dup 0= IF ABORT" start-cpu not found" THEN + node>path open-dev encode-int s" cpu" set-chosen +; +set-chosen-cpu + s" /memory@0" open-dev encode-int s" memory" set-chosen 6e0 cp
With the addition of cpu hotplug in QEMU, cpu@0 can be removed as well. SLOF should not depend on it. Find the first child in the "/cpus" node and get the timer base frequency and set it as the chosen cpu as well Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> --- board-qemu/slof/tree.fs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)