Re: [OpenBIOS] [PATCH] Introduce forth_init() for trampoline initialization
Am 09.11.2010 um 19:07 schrieb Blue Swirl:
On Mon, Nov 8, 2010 at 11:20 PM, Andreas Färber <andreas.faerber@web.de
wrote: Move trampoline variable initialization to init_trampoline(), like in kernel/bootstrap.c.
Add calls to a new forth_init() function for each architecture to invoke it. Idea courtesy of Blue.
Looks OK. I tried a slightly different approach (attached). I added Mark's excellent analysis as a comment.
General setup_trampoline() idea is fine with me. Please avoid indentation changes though, you're replacing single tabs with 8 spaces. Are you sure it doesn't need to be initialized from anywhere else, like load_dictionary() or forth_init(), for the initialization to take effect before any of these?
However, ppc64 does not build for me: GEN bootstrap.dict panic: segmentation violation at 0xccbd062c dict=0x2aecccbd0010 here=0x2aecccbd0638(dict+0x628) pc=0x0(dict +0x3342fff0) dstackcnt=0 rstackcnt=0 instruction=deadbeef dstack: 0x0 rstack: 0x0 Writing dictionary core file <trampoline.diff>
Will check. You are aware that there is no dependency rule for internal.c since it is #included from primitives.c? You either need to do `make clean` first or reconfigure for changes to take effect. Andreas
On Tue, Nov 9, 2010 at 6:38 PM, Andreas Färber <andreas.faerber@web.de> wrote:
Am 09.11.2010 um 19:07 schrieb Blue Swirl:
On Mon, Nov 8, 2010 at 11:20 PM, Andreas Färber <andreas.faerber@web.de> wrote:
Move trampoline variable initialization to init_trampoline(), like in kernel/bootstrap.c.
Add calls to a new forth_init() function for each architecture to invoke it. Idea courtesy of Blue.
Looks OK. I tried a slightly different approach (attached). I added Mark's excellent analysis as a comment.
General setup_trampoline() idea is fine with me. Please avoid indentation changes though, you're replacing single tabs with 8 spaces.
I've always used spaces. Actually I'd like to switch to QEMU coding style with 4 spaces, this is how Sparc files are formatted.
Are you sure it doesn't need to be initialized from anywhere else, like load_dictionary() or forth_init(), for the initialization to take effect before any of these?
Yes, since all uses of trampoline call setup_trampoline() before use. I also renamed 't' to trampoline2 to catch all uses of it.
However, ppc64 does not build for me: GEN bootstrap.dict panic: segmentation violation at 0xccbd062c dict=0x2aecccbd0010 here=0x2aecccbd0638(dict+0x628) pc=0x0(dict+0x3342fff0) dstackcnt=0 rstackcnt=0 instruction=deadbeef dstack: 0x0 rstack: 0x0 Writing dictionary core file <trampoline.diff>
Will check.
You are aware that there is no dependency rule for internal.c since it is #included from primitives.c? You either need to do `make clean` first or reconfigure for changes to take effect.
Yes, the dependencies are far from complete. The problem is that rules.mak is autogenerated, so the dependencies should be added to build.xml files or some magic added to use gcc -MMD flags. Including a .c file from another is not so great idea, this happens in a few places.
Am 09.11.2010 um 19:57 schrieb Blue Swirl:
On Tue, Nov 9, 2010 at 6:38 PM, Andreas Färber <andreas.faerber@web.de> wrote:
Am 09.11.2010 um 19:07 schrieb Blue Swirl:
On Mon, Nov 8, 2010 at 11:20 PM, Andreas Färber <andreas.faerber@web.de
wrote:
Move trampoline variable initialization to init_trampoline(), like in kernel/bootstrap.c.
Add calls to a new forth_init() function for each architecture to invoke it. Idea courtesy of Blue.
Looks OK. I tried a slightly different approach (attached). I added Mark's excellent analysis as a comment.
General setup_trampoline() idea is fine with me. Please avoid indentation changes though, you're replacing single tabs with 8 spaces.
I've always used spaces. Actually I'd like to switch to QEMU coding style with 4 spaces, this is how Sparc files are formatted.
I personally prefer tabs but would be fine with 4 spaces as well. Just, please, don't convert single lines within a differently formatted function. That decreases readability. Andreas
Blue Swirl wrote:
Are you sure it doesn't need to be initialized from anywhere else, like load_dictionary() or forth_init(), for the initialization to take effect before any of these?
Yes, since all uses of trampoline call setup_trampoline() before use. I also renamed 't' to trampoline2 to catch all uses of it.
I think I prefer Andreas' approach but with a slight modification - change the signature of init_trampoline() from: static void init_trampoline(void) to: static void init_trampoline(ucell *t) This enables the caller to pass in a pointer to the base of the trampoline ucell array, and so should work correctly when compiled as part of both bootstrap.c (host target) and the OpenBIOS output target. ATB, Mark. -- Mark Cave-Ayland - Senior Technical Architect PostgreSQL - PostGIS Sirius Corporation plc - control through freedom http://www.siriusit.co.uk t: +44 870 608 0063 Sirius Labs: http://www.siriusit.co.uk/labs
On Wed, Nov 10, 2010 at 10:52 AM, Mark Cave-Ayland <mark.cave-ayland@siriusit.co.uk> wrote:
Blue Swirl wrote:
Are you sure it doesn't need to be initialized from anywhere else, like load_dictionary() or forth_init(), for the initialization to take effect before any of these?
Yes, since all uses of trampoline call setup_trampoline() before use. I also renamed 't' to trampoline2 to catch all uses of it.
I think I prefer Andreas' approach but with a slight modification - change the signature of init_trampoline() from:
static void init_trampoline(void)
to:
static void init_trampoline(ucell *t)
This enables the caller to pass in a pointer to the base of the trampoline ucell array, and so should work correctly when compiled as part of both bootstrap.c (host target) and the OpenBIOS output target.
Yes, good idea. I still think that adding setup_trampoline() which just does the assignment to trampoline[1] would still be a small cleanup, but it can be added later.
Am 10.11.2010 um 18:27 schrieb Blue Swirl:
On Wed, Nov 10, 2010 at 10:52 AM, Mark Cave-Ayland <mark.cave-ayland@siriusit.co.uk> wrote:
Blue Swirl wrote:
Are you sure it doesn't need to be initialized from anywhere else, like load_dictionary() or forth_init(), for the initialization to take effect before any of these?
Yes, since all uses of trampoline call setup_trampoline() before use. I also renamed 't' to trampoline2 to catch all uses of it.
I think I prefer Andreas' approach but with a slight modification - change the signature of init_trampoline() from:
static void init_trampoline(void)
to:
static void init_trampoline(ucell *t)
This enables the caller to pass in a pointer to the base of the trampoline ucell array, and so should work correctly when compiled as part of both bootstrap.c (host target) and the OpenBIOS output target.
Yes, good idea. I still think that adding setup_trampoline() which just does the assignment to trampoline[1] would still be a small cleanup, but it can be added later.
Is anyone working on this or are you waiting for me to? I'm not sure I understand what exactly you want now... Mark, do you mean we should go with Blue's initial forth_init() suggestion? Or do you just mean splitting init_trampoline(ucell* t) from Blue's setup_trampoline()? Either way, where would we put a unified init_trampoline()? Into kernel/kernel.h as inline function? Andreas
On Sat, Nov 13, 2010 at 9:44 AM, Andreas Färber <andreas.faerber@web.de> wrote:
Am 10.11.2010 um 18:27 schrieb Blue Swirl:
On Wed, Nov 10, 2010 at 10:52 AM, Mark Cave-Ayland <mark.cave-ayland@siriusit.co.uk> wrote:
Blue Swirl wrote:
Are you sure it doesn't need to be initialized from anywhere else, like load_dictionary() or forth_init(), for the initialization to take effect before any of these?
Yes, since all uses of trampoline call setup_trampoline() before use. I also renamed 't' to trampoline2 to catch all uses of it.
I think I prefer Andreas' approach but with a slight modification - change the signature of init_trampoline() from:
static void init_trampoline(void)
to:
static void init_trampoline(ucell *t)
This enables the caller to pass in a pointer to the base of the trampoline ucell array, and so should work correctly when compiled as part of both bootstrap.c (host target) and the OpenBIOS output target.
Yes, good idea. I still think that adding setup_trampoline() which just does the assignment to trampoline[1] would still be a small cleanup, but it can be added later.
Is anyone working on this or are you waiting for me to? I'm not sure I understand what exactly you want now...
I'd be fine with your patch with Mark's modification included.
Mark, do you mean we should go with Blue's initial forth_init() suggestion? Or do you just mean splitting init_trampoline(ucell* t) from Blue's setup_trampoline()?
That can be done later, or if you prefer, included into your patch.
Either way, where would we put a unified init_trampoline()? Into kernel/kernel.h as inline function?
That should work.
Use init_trampoline() for trampoline variable initialization. Add calls to a new forth_init() function for each architecture to invoke it. Idea courtesy of Blue. This fixes ppc64 compilation by avoiding a casted self-reference. v2: * Share init_trampoline() with kernel/bootstrap.c, suggested by Mark. * Adopt QEMU coding style for new functions. Cc: Blue Swirl <blauwirbel@gmail.com> Cc: Mark Cave-Ayland <mark.cave-ayland@siriusit.co.uk> Signed-off-by: Andreas Färber <andreas.faerber@web.de> --- arch/amd64/openbios.c | 1 + arch/ppc/kernel.c | 1 + arch/ppc/qemu/kernel.c | 1 + arch/sparc32/openbios.c | 1 + arch/sparc64/openbios.c | 1 + arch/unix/unix.c | 1 + arch/x86/openbios.c | 1 + include/kernel/kernel.h | 2 ++ kernel/bootstrap.c | 21 ++++++--------------- kernel/internal.c | 16 +++++++++++++++- 10 files changed, 30 insertions(+), 16 deletions(-) diff --git a/arch/amd64/openbios.c b/arch/amd64/openbios.c index df43943..db138ad 100644 --- a/arch/amd64/openbios.c +++ b/arch/amd64/openbios.c @@ -67,6 +67,7 @@ int openbios(void) load_dictionary((char *)sys_info.dict_start, sys_info.dict_end-sys_info.dict_start); + forth_init(); relocate(&sys_info); diff --git a/arch/ppc/kernel.c b/arch/ppc/kernel.c index 57efde7..28f2965 100644 --- a/arch/ppc/kernel.c +++ b/arch/ppc/kernel.c @@ -83,6 +83,7 @@ initialize_forth( void ) { dict = malloc(DICTIONARY_SIZE); load_dictionary( forth_dictionary, sizeof(forth_dictionary) ); + forth_init(); PUSH_xt( bind_noname_func(arch_of_init) ); fword("PREPOST-initializer"); diff --git a/arch/ppc/qemu/kernel.c b/arch/ppc/qemu/kernel.c index dbfca57..4cae525 100644 --- a/arch/ppc/qemu/kernel.c +++ b/arch/ppc/qemu/kernel.c @@ -86,6 +86,7 @@ initialize_forth( void ) dictlimit = DICTIONARY_SIZE; load_dictionary( forth_dictionary, sizeof(forth_dictionary) ); + forth_init(); PUSH_xt( bind_noname_func(arch_of_init) ); fword("PREPOST-initializer"); diff --git a/arch/sparc32/openbios.c b/arch/sparc32/openbios.c index e368d6c..1c86752 100644 --- a/arch/sparc32/openbios.c +++ b/arch/sparc32/openbios.c @@ -970,6 +970,7 @@ int openbios(void) load_dictionary((char *)sys_info.dict_start, (unsigned long)sys_info.dict_end - (unsigned long)sys_info.dict_start); + forth_init(); #ifdef CONFIG_DEBUG_BOOT printk("forth started.\n"); diff --git a/arch/sparc64/openbios.c b/arch/sparc64/openbios.c index 49bbe27..8c08814 100644 --- a/arch/sparc64/openbios.c +++ b/arch/sparc64/openbios.c @@ -623,6 +623,7 @@ int openbios(void) load_dictionary((char *)sys_info.dict_start, (unsigned long)sys_info.dict_end - (unsigned long)sys_info.dict_start); + forth_init(); #ifdef CONFIG_DEBUG_BOOT printk("forth started.\n"); diff --git a/arch/unix/unix.c b/arch/unix/unix.c index a8b9f78..e5c4590 100644 --- a/arch/unix/unix.c +++ b/arch/unix/unix.c @@ -514,6 +514,7 @@ int main(int argc, char *argv[]) printk("done.\n"); read_dictionary(argv[optind]); + forth_init(); PUSH_xt( bind_noname_func(arch_init) ); fword("PREPOST-initializer"); diff --git a/arch/x86/openbios.c b/arch/x86/openbios.c index 62ef587..24b886d 100644 --- a/arch/x86/openbios.c +++ b/arch/x86/openbios.c @@ -90,6 +90,7 @@ int openbios(void) load_dictionary((char *)sys_info.dict_start, (unsigned long)sys_info.dict_end - (unsigned long)sys_info.dict_start); + forth_init(); relocate(&sys_info); diff --git a/include/kernel/kernel.h b/include/kernel/kernel.h index 15605b5..c887e24 100644 --- a/include/kernel/kernel.h +++ b/include/kernel/kernel.h @@ -32,6 +32,8 @@ extern void panic(const char *error) __attribute__ ((noreturn)); extern xt_t findword(const char *s1); extern void modules_init( void ); +extern void init_trampoline(ucell *t); +extern void forth_init(void); /* arch kernel hooks */ extern void exception(cell no); diff --git a/kernel/bootstrap.c b/kernel/bootstrap.c index 90c93c1..c029c02 100644 --- a/kernel/bootstrap.c +++ b/kernel/bootstrap.c @@ -91,20 +91,6 @@ static const char *wordnames[] = { "$include", "$encode-file", "(debug", "(debug-off)" }; -static void init_trampoline(void) -{ - if (!trampoline) { - /* We're using side effects which is to some extent nasty */ - printf("WARNING: no trampoline!\n"); - return; - } - - trampoline[0]=DOCOL; - trampoline[1]=0; - trampoline[2]=target_ucell(pointer2cell(trampoline)+3*sizeof(ucell)); - trampoline[3]=0; -} - /* * dictionary related functions. */ @@ -1181,7 +1167,12 @@ int main(int argc, char *argv[]) TRAMPOLINE_SIZE); #endif - init_trampoline(); + if (trampoline == NULL) { + /* We're using side effects which is to some extent nasty */ + printf("WARNING: no trampoline!\n"); + } else { + init_trampoline(trampoline); + } if (!segfault) { if (verbose) diff --git a/kernel/internal.c b/kernel/internal.c index 91ab440..0bf9c64 100644 --- a/kernel/internal.c +++ b/kernel/internal.c @@ -50,8 +50,13 @@ char xtname[MAXNFALEN]; /* instead of pointing to an explicit 0 variable we * point behind the pointer. */ -static ucell t[] = { DOCOL, 0, (ucell)(t+3), 0 }; +static ucell t[] = { 0, 0, 0, 0 }; static ucell *trampoline = t; + +void forth_init(void) +{ + init_trampoline(trampoline); +} #endif #ifndef CONFIG_DEBUG_INTERPRETER @@ -67,6 +72,15 @@ static ucell *trampoline = t; #endif +void init_trampoline(ucell *tramp) +{ + tramp[0] = DOCOL; + tramp[1] = 0; + tramp[2] = target_ucell(pointer2cell(tramp) + 3 * sizeof(ucell)); + tramp[3] = 0; +} + + static inline void processxt(ucell xt) { void (*tokenp) (void); -- 1.7.3
On Sat, Nov 13, 2010 at 11:09 AM, Andreas Färber <andreas.faerber@web.de> wrote:
Use init_trampoline() for trampoline variable initialization.
Add calls to a new forth_init() function for each architecture to invoke it. Idea courtesy of Blue.
This fixes ppc64 compilation by avoiding a casted self-reference.
v2: * Share init_trampoline() with kernel/bootstrap.c, suggested by Mark. * Adopt QEMU coding style for new functions.
Cc: Blue Swirl <blauwirbel@gmail.com> Cc: Mark Cave-Ayland <mark.cave-ayland@siriusit.co.uk> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
Acked-by: Blue Swirl <blauwirbel@gmail.com>
--- arch/amd64/openbios.c | 1 + arch/ppc/kernel.c | 1 + arch/ppc/qemu/kernel.c | 1 + arch/sparc32/openbios.c | 1 + arch/sparc64/openbios.c | 1 + arch/unix/unix.c | 1 + arch/x86/openbios.c | 1 + include/kernel/kernel.h | 2 ++ kernel/bootstrap.c | 21 ++++++--------------- kernel/internal.c | 16 +++++++++++++++- 10 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/arch/amd64/openbios.c b/arch/amd64/openbios.c index df43943..db138ad 100644 --- a/arch/amd64/openbios.c +++ b/arch/amd64/openbios.c @@ -67,6 +67,7 @@ int openbios(void)
load_dictionary((char *)sys_info.dict_start, sys_info.dict_end-sys_info.dict_start); + forth_init();
relocate(&sys_info);
diff --git a/arch/ppc/kernel.c b/arch/ppc/kernel.c index 57efde7..28f2965 100644 --- a/arch/ppc/kernel.c +++ b/arch/ppc/kernel.c @@ -83,6 +83,7 @@ initialize_forth( void ) { dict = malloc(DICTIONARY_SIZE); load_dictionary( forth_dictionary, sizeof(forth_dictionary) ); + forth_init();
PUSH_xt( bind_noname_func(arch_of_init) ); fword("PREPOST-initializer"); diff --git a/arch/ppc/qemu/kernel.c b/arch/ppc/qemu/kernel.c index dbfca57..4cae525 100644 --- a/arch/ppc/qemu/kernel.c +++ b/arch/ppc/qemu/kernel.c @@ -86,6 +86,7 @@ initialize_forth( void ) dictlimit = DICTIONARY_SIZE;
load_dictionary( forth_dictionary, sizeof(forth_dictionary) ); + forth_init();
PUSH_xt( bind_noname_func(arch_of_init) ); fword("PREPOST-initializer"); diff --git a/arch/sparc32/openbios.c b/arch/sparc32/openbios.c index e368d6c..1c86752 100644 --- a/arch/sparc32/openbios.c +++ b/arch/sparc32/openbios.c @@ -970,6 +970,7 @@ int openbios(void) load_dictionary((char *)sys_info.dict_start, (unsigned long)sys_info.dict_end - (unsigned long)sys_info.dict_start); + forth_init();
#ifdef CONFIG_DEBUG_BOOT printk("forth started.\n"); diff --git a/arch/sparc64/openbios.c b/arch/sparc64/openbios.c index 49bbe27..8c08814 100644 --- a/arch/sparc64/openbios.c +++ b/arch/sparc64/openbios.c @@ -623,6 +623,7 @@ int openbios(void) load_dictionary((char *)sys_info.dict_start, (unsigned long)sys_info.dict_end - (unsigned long)sys_info.dict_start); + forth_init();
#ifdef CONFIG_DEBUG_BOOT printk("forth started.\n"); diff --git a/arch/unix/unix.c b/arch/unix/unix.c index a8b9f78..e5c4590 100644 --- a/arch/unix/unix.c +++ b/arch/unix/unix.c @@ -514,6 +514,7 @@ int main(int argc, char *argv[]) printk("done.\n");
read_dictionary(argv[optind]); + forth_init();
PUSH_xt( bind_noname_func(arch_init) ); fword("PREPOST-initializer"); diff --git a/arch/x86/openbios.c b/arch/x86/openbios.c index 62ef587..24b886d 100644 --- a/arch/x86/openbios.c +++ b/arch/x86/openbios.c @@ -90,6 +90,7 @@ int openbios(void) load_dictionary((char *)sys_info.dict_start, (unsigned long)sys_info.dict_end - (unsigned long)sys_info.dict_start); + forth_init();
relocate(&sys_info);
diff --git a/include/kernel/kernel.h b/include/kernel/kernel.h index 15605b5..c887e24 100644 --- a/include/kernel/kernel.h +++ b/include/kernel/kernel.h @@ -32,6 +32,8 @@ extern void panic(const char *error) __attribute__ ((noreturn));
extern xt_t findword(const char *s1); extern void modules_init( void ); +extern void init_trampoline(ucell *t); +extern void forth_init(void);
'extern' is useless qualifier for function declarations. It's used a lot here, though.
Am 13.11.2010 um 12:35 schrieb Blue Swirl:
On Sat, Nov 13, 2010 at 11:09 AM, Andreas Färber <andreas.faerber@web.de
wrote: diff --git a/include/kernel/kernel.h b/include/kernel/kernel.h index 15605b5..c887e24 100644 --- a/include/kernel/kernel.h +++ b/include/kernel/kernel.h @@ -32,6 +32,8 @@ extern void panic(const char *error) __attribute__ ((noreturn));
extern xt_t findword(const char *s1); extern void modules_init( void ); +extern void init_trampoline(ucell *t); +extern void forth_init(void);
'extern' is useless qualifier for function declarations. It's used a lot here, though.
I thought it used to make a difference on Windows (Microsoft VS6?) and possibly BeOS (Metrowerks toolchain), which used funky macros for shared headers... Given that it's used for the host as well I'd prefer to leave it for now, to get ppc64 compilable for testing the other series. Andreas
Am 13.11.2010 um 12:35 schrieb Blue Swirl:
On Sat, Nov 13, 2010 at 11:09 AM, Andreas Färber <andreas.faerber@web.de
wrote: Use init_trampoline() for trampoline variable initialization.
Add calls to a new forth_init() function for each architecture to invoke it. Idea courtesy of Blue.
This fixes ppc64 compilation by avoiding a casted self-reference.
v2: * Share init_trampoline() with kernel/bootstrap.c, suggested by Mark. * Adopt QEMU coding style for new functions.
Cc: Blue Swirl <blauwirbel@gmail.com> Cc: Mark Cave-Ayland <mark.cave-ayland@siriusit.co.uk> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
Acked-by: Blue Swirl <blauwirbel@gmail.com>
Thanks, applied as r954. Andreas
Andreas Färber wrote:
Thanks, applied as r954.
Andreas
Looks good to me, although I wonder if adding the comment from Blue's patch would save some confusion further down the line? ATB, Mark. -- Mark Cave-Ayland - Senior Technical Architect PostgreSQL - PostGIS Sirius Corporation plc - control through freedom http://www.siriusit.co.uk t: +44 870 608 0063 Sirius Labs: http://www.siriusit.co.uk/labs
Am 15.11.2010 um 18:50 schrieb Mark Cave-Ayland:
Andreas Färber wrote:
Thanks, applied as r954.
Looks good to me, although I wonder if adding the comment from Blue's patch would save some confusion further down the line?
Yes, I would appreciate an explanatory comment, too. But I cannot judge it, so I must leave that to you or Blue. I thought Blue wanted to rebase his setup_trampoline() patch against mine and re-test it and add the comment as part of that. But his large patch moving the code around is not yet applied, so we should avoid fiddling around there for now. Andreas
participants (3)
-
Andreas Färber -
Blue Swirl -
Mark Cave-Ayland