Hi!
This series do clean up several issues with openbios implementation trying to access zero virtual memory page.
These changes are needed to prevent corrupting low memory space, albeit it is hard to verify without corresponding change to qemu which moves pci memory space above low 32bit space. Qemu part is not completed yet, so I use workaround for testing purposes. Change to package.fs dealing with NULL instance or package handles should be visible even witout changes to qemu (meaning more issues are probably there, to be fixed).
I think timer handler currently fails into this problematic category as well, so related change postponing timer initialization is here as well. Since timer event handling requires trap table support, I added corresponding entry which does nothing but rearms timer at the moment.
Signed-off-by: igor.v.kovalenko@gmail.com
Index: openbios-devel/arch/sparc64/entry.S =================================================================== --- openbios-devel.orig/arch/sparc64/entry.S +++ openbios-devel/arch/sparc64/entry.S @@ -131,7 +131,7 @@ entry: ! %g1 contains end of memory
setx _end, %g7, %g3 - set 0xffff, %g2 + set 0x7ffff, %g2 add %g3, %g2, %g3 andn %g3, %g2, %g3 setx _data, %g7, %g2 @@ -142,17 +142,17 @@ entry: ! setup .data & .bss setx _data, %g7, %g4 sub %g3, %g4, %g5 - srlx %g5, 16, %g6 ! %g6 = # of 64k .bss pages - set 0xa0000000, %g3 + srlx %g5, 19, %g6 ! %g6 = # of 512k .bss pages + set 0xc0000000, %g3 sllx %g3, 32, %g3 or %g3, 0x76, %g3 - ! valid, 64k, locked, cacheable(I/E/C), priv, writable + ! valid, 512k, locked, cacheable(I/E/C), priv, writable set 48, %g7 -1: stxa %g4, [%g7] ASI_DMMU ! vaddr = _data + N * 0x10000, ctx=0 +1: stxa %g4, [%g7] ASI_DMMU ! vaddr = _data + N * 0x80000, ctx=0 or %g2, %g3, %g5 - ! paddr = start_mem + N * 0x10000 + ! paddr = start_mem + N * 0x80000 stxa %g5, [%g0] ASI_DTLB_DATA_IN - set 0x10000, %g5 + set 0x80000, %g5 add %g2, %g5, %g2 add %g4, %g5, %g4 deccc %g6 @@ -163,16 +163,16 @@ entry: setx _data, %g7, %g5 setx _start, %g7, %g4 sub %g5, %g4, %g5 - srlx %g5, 16, %g6 ! %g6 = # of 64k .rodata pages + srlx %g5, 19, %g6 ! %g6 = # of 512k .rodata pages set 48, %g7 - set 0x10000, %g5 + set 0x80000, %g5 setx PROM_ADDR, %l1, %l2 1: stxa %g4, [%g7] ASI_DMMU ! vaddr = _rodata, ctx=0 - set 0xa0000000, %g3 + set 0xc0000000, %g3 sllx %g3, 32, %g3 or %g3, 0x74, %g3 or %l2, %g3, %g3 - ! valid, 64k, locked, cacheable(I/E/C), priv + ! valid, 512k, locked, cacheable(I/E/C), priv ! paddr = _rodata + N * 0x10000 stxa %g3, [%g0] ASI_DTLB_DATA_IN add %g4, %g5, %g4 @@ -223,17 +223,19 @@ entry: setx _start, %g7, %g4 setx _rodata, %g7, %g5 sub %g5, %g4, %g5 - srlx %g5, 16, %g6 ! %g6 = # of 64k .text pages - set 0x10000, %g5 + set 0x7ffff, %g7 + add %g5, %g7, %g5 ! round to 512k + srlx %g5, 19, %g6 ! %g6 = # of 512k .text pages + set 0x80000, %g5 set 48, %g7 setx PROM_ADDR, %l1, %l2 1: stxa %g4, [%g7] ASI_IMMU ! vaddr = _start, ctx=0 - set 0xa0000000, %g3 + set 0xc0000000, %g3 sllx %g3, 32, %g3 or %g3, 0x74, %g3 or %l2, %g3, %g3 - ! valid, 64k, locked, cacheable(I/E/C), priv - ! paddr = _start + N * 0x10000 + ! valid, 512k, locked, cacheable(I/E/C), priv + ! paddr = _start + N * 0x80000 stxa %g3, [%g0] ASI_ITLB_DATA_IN add %g4, %g5, %g4 deccc %g6 Index: openbios-devel/arch/sparc64/ldscript =================================================================== --- openbios-devel.orig/arch/sparc64/ldscript +++ openbios-devel/arch/sparc64/ldscript @@ -25,12 +25,12 @@ SECTIONS _start = .;
/* Normal sections */ - .text ALIGN(65536): { + .text ALIGN(524288): { *(.text.vectors) *(.text) *(.text.*) } - .rodata ALIGN(65536): { + .rodata ALIGN(524288): { _rodata = .; sound_drivers_start = .; *(.rodata.sound_drivers) @@ -39,7 +39,7 @@ SECTIONS *(.rodata.*) *(.note.ELFBoot) } - .data ALIGN(65536): { + .data ALIGN(524288): { _data = .; *(.data) *(.data.*)
Index: openbios-devel/modules/console_common.c =================================================================== --- openbios-devel.orig/modules/console_common.c +++ openbios-devel/modules/console_common.c @@ -386,6 +386,10 @@ console_draw_str( const char *str ) unsigned int y, x; unsigned char ch;
+ if (!str) { + return 0; + } + if( !cons.inited && console_init() ) return -1;
Sounds like a bug in the caller side, couldn't we fix those instead?
On Fri, Aug 21, 2009 at 11:09 PM, Blue Swirlblauwirbel@gmail.com wrote:
Sounds like a bug in the caller side, couldn't we fix those instead?
Rationale is to not seek for buggy callers.
Since pop_fstr_copy() returns NULL for empty strings it is not possible to tell "" from NULL anyway.
On Fri, Aug 21, 2009 at 11:20 PM, Igor Kovalenkoigor.v.kovalenko@gmail.com wrote:
On Fri, Aug 21, 2009 at 11:09 PM, Blue Swirlblauwirbel@gmail.com wrote:
Sounds like a bug in the caller side, couldn't we fix those instead?
Rationale is to not seek for buggy callers.
Since pop_fstr_copy() returns NULL for empty strings it is not possible to tell "" from NULL anyway.
I see. Applied, thanks.
Index: openbios-devel/modules/disk-label.c =================================================================== --- openbios-devel.orig/modules/disk-label.c +++ openbios-devel/modules/disk-label.c @@ -57,7 +57,7 @@ dlabel_close( dlabel_info_t *di ) static void dlabel_open( dlabel_info_t *di ) { - char *s, *filename; + const char *s, *filename; char *path; char block0[512]; phandle_t ph; @@ -65,6 +65,9 @@ dlabel_open( dlabel_info_t *di ) xt_t xt;
path = my_args_copy(); + if (!path) { + path = strdup(""); + } DPRINTF("dlabel-open '%s'\n", path );
/* open disk interface */
Index: openbios-devel/arch/sparc64/entry.S =================================================================== --- openbios-devel.orig/arch/sparc64/entry.S +++ openbios-devel/arch/sparc64/entry.S @@ -47,8 +47,8 @@ entry: wrpr %g0, 0, %canrestore wrpr %g0, 0, %otherwin wrpr %g0, 0, %wstate - ! 100 Hz timer - set 10 * 1000 * 1000, %g1 + ! disable timer now + setx 0x8000000000000000, %g2, %g1 wr %g1, 0, %tick_cmpr
! Disable I/D MMUs and caches @@ -319,6 +319,12 @@ lowmem: /* Finally, turn on traps so that we can call c-code. */ wrpr %g0, (PSTATE_PRIV|PSTATE_PEF|PSTATE_IE), %pstate
+ ! 100 Hz timer + rd %tick, %g2 + set 10 * 1000 * 1000, %g1 + add %g1, %g2, %g1 + wr %g1, 0, %tick_cmpr + /* Switch to our main context. * Main context is statically defined in C. */
Index: openbios-devel/arch/sparc64/vectors.S =================================================================== --- openbios-devel.orig/arch/sparc64/vectors.S +++ openbios-devel/arch/sparc64/vectors.S @@ -29,11 +29,13 @@ #define ASI_BP ASI_PHYS_BYPASS_EC_E #define PROM_ADDR 0x1fff0000000 #define SER_ADDR 0x1fe020003f8 +#define TICK_INT_DIS 0x8000000000000000 +#define TICK_INTERVAL 10*1000*1000
.section ".text.vectors", "ax" .align 16384 /* Sparc64 trap table */ - .globl trap_table, __divide_error + .globl trap_table, __divide_error, softint_irq, softint_irq_tl1 .register %g2, #scratch .register %g3, #scratch .register %g6, #scratch @@ -96,7 +98,7 @@ trap_table: nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;
#define TRAP_IRQ(routine, level) \ - ba routine; nop; nop; nop; nop; nop; nop; nop; + ba routine; mov level, %g1; nop; nop; nop; nop; nop; nop; #define BTRAP(lvl) \ ba bug; mov lvl, %g1; nop; nop; nop; nop; nop; nop; #define BTRAPTL1(lvl) BTRAP(lvl) @@ -119,7 +121,6 @@ sparc64_ttable_tl0: CLEAN_WINDOW ! 24-27 BTRAPS(0x28) BTRAPS(0x30) BTRAPS(0x38) -#if 0 BTRAP(0x40) BTRAP(0x41) BTRAP(0x42) BTRAP(0x43) tl0_irq4: TRAP_IRQ(handler_irq, 4) tl0_irq5: TRAP_IRQ(handler_irq, 5) TRAP_IRQ(handler_irq, 6) @@ -127,11 +128,8 @@ tl0_irq7: TRAP_IRQ(handler_irq, 7) TRAP tl0_irq9: TRAP_IRQ(handler_irq, 9) TRAP_IRQ(handler_irq, 10) tl0_irq11: TRAP_IRQ(handler_irq, 11) TRAP_IRQ(handler_irq, 12) tl0_irq13: TRAP_IRQ(handler_irq, 13) -tl0_irq14: TRAP_IRQ(handler_irq, 14) +tl0_irq14: TRAP_IRQ(softint_irq, 14) tl0_irq15: TRAP_IRQ(handler_irq, 15) -#else - BTRAPS(0x40) BTRAPS(0x48) -#endif BTRAPS(0x50) BTRAPS(0x58) BTRAPS4(0x60) TRAP_HANDLER(reload_IMMU_tlb) ! 0x64 : instruction_access_MMU_miss @@ -196,6 +194,9 @@ tl0_resv1f0: BTRAPS(0x1f0) BTRAPS(0x1f8) #undef BTRAPS #define BTRAPS(x) BTRAPTL1(x) BTRAPTL1(x+1) BTRAPTL1(x+2) BTRAPTL1(x+3) BTRAPTL1(x+4) BTRAPTL1(x+5) BTRAPTL1(x+6) BTRAPTL1(x+7)
+#define SKIP_IRQ(routine, level) \ + retry; nop; nop; nop; nop; nop; nop; nop; + sparc64_ttable_tl1: BTRAPS(0x00) BTRAPS(0x08) BTRAPS(0x10) BTRAPS(0x18) @@ -203,7 +204,6 @@ sparc64_ttable_tl1: CLEAN_WINDOW ! 24-27 BTRAPS(0x28) BTRAPS(0x30) BTRAPS(0x38) -#if 0 BTRAPTL1(0x40) BTRAPTL1(0x41) BTRAPTL1(0x42) BTRAPTL1(0x43) tl1_irq4: TRAP_IRQ(handler_irq, 4) tl1_irq5: TRAP_IRQ(handler_irq, 5) TRAP_IRQ(handler_irq, 6) @@ -211,11 +211,8 @@ tl1_irq7: TRAP_IRQ(handler_irq, 7) TRAP tl1_irq9: TRAP_IRQ(handler_irq, 9) TRAP_IRQ(handler_irq, 10) tl1_irq11: TRAP_IRQ(handler_irq, 11) TRAP_IRQ(handler_irq, 12) tl1_irq13: TRAP_IRQ(handler_irq, 13) -tl1_irq14: TRAP_IRQ(handler_irq, 14) +tl1_irq14: SKIP_IRQ(softint_irq, 14) tl1_irq15: TRAP_IRQ(handler_irq, 15) -#else - BTRAPS(0x40) BTRAPS(0x48) -#endif BTRAPS(0x50) BTRAPS(0x58) BTRAPS4(0x60) TRAP_HANDLER(reload_IMMU_tlb) ! 0x64 : instruction_access_MMU_miss @@ -400,6 +397,32 @@ immu_next_trans:
retry
+softint_irq_tl1: + ba softint_irq + nop + +softint_irq: + mov 1, %g2 + /* clear tick interrupt */ + wr %g2, 0x0, %clear_softint + sll %g2, %g1, %g2 + sra %g2, 0, %g2 + /* clear softint interrupt */ + wr %g2, 0x0, %clear_softint + + setx TICK_INT_DIS, %g2, %g1 + rd %tick, %g2 + and %g1, %g2, %g1 + brnz,pn %g1, tick_compare_disabled + nop + set TICK_INTERVAL, %g1 + add %g1, %g2, %g1 + wr %g1, 0, %tick_cmpr +tick_compare_disabled: + retry + nop + +handler_irq: __divide_error: bug: /* Dump the exception and its context */
On Fri, Aug 21, 2009 at 12:16 AM, Igor Kovalenkoigor.v.kovalenko@gmail.com wrote:
Index: openbios-devel/arch/sparc64/vectors.S
+softint_irq_tl1:
- ba softint_irq
- nop
These instructions are not needed...
+tick_compare_disabled:
- retry
- nop
There is no delay slot after 'retry', so 'nop' is not needed.
Index: openbios-devel/forth/device/package.fs =================================================================== --- openbios-devel.orig/forth/device/package.fs +++ openbios-devel/forth/device/package.fs @@ -59,10 +59,15 @@
: find-method ( method-str method-len phandle -- false | xt true ) \ should we search the private wordlist too? I don't think so... - >dn.methods @ find-wordlist if - true + ?dup if + >dn.methods @ find-wordlist if + true + else + 2drop false + then else - 2drop false + cr ." find-method: " type ." : NULL phandle" cr + false then ;
@@ -75,10 +80,18 @@
: $call-method ( ... method-str method-len ihandle -- ??? ) - dup >r >in.device-node @ find-method if - r> call-package + \ check if my-self exists; if not, there is nothing to call from + + ?dup if + dup >r >in.device-node @ find-method if + r> call-package + else + cr ." $call-method: instance method not found" cr + -821 throw + then else - -21 throw + cr ." $call-method: " type ." : NULL ihandle" cr + -f21 throw then ;
On Fri, Aug 21, 2009 at 12:16 AM, Igor Kovalenkoigor.v.kovalenko@gmail.com wrote:
Index: openbios-devel/forth/device/package.fs
--- openbios-devel.orig/forth/device/package.fs +++ openbios-devel/forth/device/package.fs @@ -59,10 +59,15 @@
: find-method ( method-str method-len phandle -- false | xt true ) \ should we search the private wordlist too? I don't think so...
- >dn.methods @ find-wordlist if
- true
- ?dup if
- >dn.methods @ find-wordlist if
- true
- else
- 2drop false
- then
else
- 2drop false
- cr ." find-method: " type ." : NULL phandle" cr
- false
then ;
@@ -75,10 +80,18 @@
: $call-method ( ... method-str method-len ihandle -- ??? )
- dup >r >in.device-node @ find-method if
- r> call-package
- \ check if my-self exists; if not, there is nothing to call from
- ?dup if
- dup >r >in.device-node @ find-method if
- r> call-package
- else
- cr ." $call-method: instance method not found" cr
- -821 throw
- then
else
- -21 throw
- cr ." $call-method: " type ." : NULL ihandle" cr
- -f21 throw
then ;
Can we find out the caller in these cases? The error messages are not very useful to the user. They also happen on Sparc32.
On Fri, Aug 21, 2009 at 11:12 PM, Blue Swirlblauwirbel@gmail.com wrote:
On Fri, Aug 21, 2009 at 12:16 AM, Igor Kovalenkoigor.v.kovalenko@gmail.com wrote:
Index: openbios-devel/forth/device/package.fs
--- openbios-devel.orig/forth/device/package.fs +++ openbios-devel/forth/device/package.fs @@ -59,10 +59,15 @@
: find-method ( method-str method-len phandle -- false | xt true ) \ should we search the private wordlist too? I don't think so...
- >dn.methods @ find-wordlist if
- true
- ?dup if
- >dn.methods @ find-wordlist if
- true
- else
- 2drop false
- then
else
- 2drop false
- cr ." find-method: " type ." : NULL phandle" cr
- false
then ;
@@ -75,10 +80,18 @@
: $call-method ( ... method-str method-len ihandle -- ??? )
- dup >r >in.device-node @ find-method if
- r> call-package
- \ check if my-self exists; if not, there is nothing to call from
- ?dup if
- dup >r >in.device-node @ find-method if
- r> call-package
- else
- cr ." $call-method: instance method not found" cr
- -821 throw
- then
else
- -21 throw
- cr ." $call-method: " type ." : NULL ihandle" cr
- -f21 throw
then ;
Can we find out the caller in these cases? The error messages are not very useful to the user. They also happen on Sparc32.
Eventually all callers would be covered, it takes time to trace forth code calls to origins. I think it is better to have extra messages in place while doing so.
In fact for me it takes so much extra time that I'm inclined to reimplement some forth code as C equivalents to keep debugging manageable.
On Fri, Aug 21, 2009 at 11:23 PM, Igor Kovalenkoigor.v.kovalenko@gmail.com wrote:
On Fri, Aug 21, 2009 at 11:12 PM, Blue Swirlblauwirbel@gmail.com wrote:
On Fri, Aug 21, 2009 at 12:16 AM, Igor Kovalenkoigor.v.kovalenko@gmail.com wrote:
Index: openbios-devel/forth/device/package.fs
--- openbios-devel.orig/forth/device/package.fs +++ openbios-devel/forth/device/package.fs @@ -59,10 +59,15 @@
: find-method ( method-str method-len phandle -- false | xt true ) \ should we search the private wordlist too? I don't think so...
- >dn.methods @ find-wordlist if
- true
- ?dup if
- >dn.methods @ find-wordlist if
- true
- else
- 2drop false
- then
else
- 2drop false
- cr ." find-method: " type ." : NULL phandle" cr
- false
then ;
@@ -75,10 +80,18 @@
: $call-method ( ... method-str method-len ihandle -- ??? )
- dup >r >in.device-node @ find-method if
- r> call-package
- \ check if my-self exists; if not, there is nothing to call from
- ?dup if
- dup >r >in.device-node @ find-method if
- r> call-package
- else
- cr ." $call-method: instance method not found" cr
- -821 throw
- then
else
- -21 throw
- cr ." $call-method: " type ." : NULL ihandle" cr
- -f21 throw
then ;
Can we find out the caller in these cases? The error messages are not very useful to the user. They also happen on Sparc32.
Eventually all callers would be covered, it takes time to trace forth code calls to origins. I think it is better to have extra messages in place while doing so.
Maybe the messages should be conditional to DEBUG_METHOD or something?
In fact for me it takes so much extra time that I'm inclined to reimplement some forth code as C equivalents to keep debugging manageable.
Right, I wish there were equivalents of gdb breakpoints and commands 'where', 'up', 'down'. Doesn't rstack contain the return addresses? Some code pushes other stuff to rstack, though.
Blue Swirl wrote:
In fact for me it takes so much extra time that I'm inclined to reimplement some forth code as C equivalents to keep debugging manageable.
I totally feel your pain, having worked on various parts of the FCode interpreter. However, I don't think re-engineering in C is the right solution for lack of OpenBIOS debugging - you're just pushing the problem elsewhere :(
Right, I wish there were equivalents of gdb breakpoints and commands 'where', 'up', 'down'. Doesn't rstack contain the return addresses? Some code pushes other stuff to rstack, though.
Yeah - please see my previous post to the list about this. Storing additional debug information in the current rstack frame isn't possible, since any Forth word that pops/re-pushes onto the rstack loses this information.
My current implementation attempts to maintain a separate colstack and use that instead - however this doesn't work as the number DOCOL and SEMIS never match. I suspect this is to do with the trampoline, although since it is so badly documented, I'm not exactly sure how this works with, say nested execute words for example :(
ATB,
Mark.
On Fri, Aug 21, 2009 at 12:16 AM, Igor Kovalenkoigor.v.kovalenko@gmail.com wrote:
Hi!
This series do clean up several issues with openbios implementation trying to access zero virtual memory page.
These changes are needed to prevent corrupting low memory space, albeit it is hard to verify without corresponding change to qemu which moves pci memory space above low 32bit space. Qemu part is not completed yet, so I use workaround for testing purposes. Change to package.fs dealing with NULL instance or package handles should be visible even witout changes to qemu (meaning more issues are probably there, to be fixed).
I think timer handler currently fails into this problematic category as well, so related change postponing timer initialization is here as well. Since timer event handling requires trap table support, I added corresponding entry which does nothing but rearms timer at the moment.
Signed-off-by: igor.v.kovalenko@gmail.com
Thanks, applied 1/6 and 4/6.
-
Blue Swirl -
Igor Kovalenko -
Mark Cave-Ayland