Improved memory handling for 68K co-pro

[rjpontefract]

I've been playing around with the 68K co-pro which is commented out by default. The CiscOS ROM determines the amount of installed memory as follows:

checkramsize:
	MOVE.L	A0, -(SP)                          ; Push A0 to the stack
	MOVE.L	0x00000000, -(SP)                  ; Save original contents of 0x00000000 on the stack

	MOVE.L	#0x00000000, A0                    ; Reset hex counter
	MOVE.L	#0xDEADBEEF, 0x00000000            ; Set 0x00000000 to the magic marker (Yes, this is where the magic happens *^@@**)

checkramsize_loop:
	ADDA.L	#0x00000400, A0                    ; Increase hex counter with 1Kbyte
	CMPI.L	#0xDEADBEEF, (A0)                  ; Is it the same as location 0x00000000?
	BEQ	checkramsize_exit                  ; Yes, then exit
	CMP.L	#max_ram_size, A0                  ; Prevent indefenitely looping
	BLO	checkramsize_loop

checkramsize_exit:
	MOVE.L	A0, D0
	MOVE.L	(SP)+, 0x00000000                  ; Pull original contents of 0x00000000 from the stack
	MOVE.L	(SP)+, A0                          ; Pull A0 from the stack
	RTS

So it depends on the address decoding wrapping around, which b-em doesn't currently do.

I made some simple changes to b-em involving:
uint8_t data = mc68000_ram[addr % MC68000_RAM_SIZE];

And changing MC68000_RAM_SIZE to 0x800000 gives 8MB which is nice.

Going extreme and setting it to 0xFFFE0000 (the maximum value) gives:

I can create a pull request for my changes, but you may have a preferable way of doing it.

[SteveFosdick]

A pull request is a fine way seeing the changes. I did just have a quick look and there is a need to deal with the ROM at the top of the address space - I don't know how that is decoded on real hardware. I am also not sure there is any one authoritative version of the hardware but having b-em support CiscOS seems like a useful goal.

We could make the memory size a config file parameter - it is allocated with malloc rather than static so allocation would probably be done after the config file has been written but don't wait for that one. I can always do that if you want.

[rjpontefract]

I've submitted a pull request.

I don't know if the original hardware that CiscOS ran on was ever made public. The images of it show that it used a 68008 and had a different memory map (according to the CiscOS source code). So, having it run under b-em without those hardware limitations seems like a good thing to me. The code currently emulates a 68020 which gives access to the full 4GB address space.

I'm OK with the memory size being a parameter. Maybe that change goes along with having the available co-pro models and ROM versions in the config file as mentioned in the Z80 discussion? Would any of the other co-processors benefit from having a configurable memory size? I think 8192K is probably a good default for the 68K co-pro RAM size.

The code in CiscOS to determine the CPU type doesn't appear to work with the b-em 680x0 emulation, AFAIK it's only purpose is to set the CPU string in the startup banner. If I get time, I'll see if I can work out why it's not working.

Update on the last point. The address/bus error stack frame is only implemented in Musashi for the 68000 and I'm not sure that the code in CiscOS gets that correct. The 68000 documentation says that the CPU saves 7 words (14 bytes) of information to the stack, which ties up with the code in Musashi. However, CiscOS is looking for 18 bytes, so it fails. As Musashi doesn't have any code for anything other than the 680000, the other checks are destined to fail.

[SteveFosdick]

Are you running a more recent version of CiscOS than the ROM bundled with b-em?

Also see: https://github.com/stardot/b-em/tree/sf/tubecfg which has the changes to define tube models in the config file.

[rjpontefract]

I'm running version 2.03 of CiscOS which is the from the most recent source code I can find for it. I couldn't assemble the 2.03 source code though, so I converted it to vasm format and used that to assemble it. I also changed the version string to 2.04 so I could tell the difference. b-em has version 2.02 bundled with it. If you want a more up to date version, I can send my build of it your way.

I'll take a look at the tubecfg branch, thanks.

[SteveFosdick]

It would probably be useful to have a copy of what you're running. I did a bit of searching and found a forum post https://stardot.org.uk/forums/viewtopic.php?p=248915#p248915 where I had discovered I could assemble some version of CiscOS with vasm. I also found a copy on my PC that claims to be 2.03, in the sense that the filename is CiscoOs203.asm, but which still has the version string in the file set to 2.02.

The reason for querying this is that anything I have tried to run reports the memory size as 3072K regardless of what I configure the size to be, so I am trying to get to the bottom of whether I have broken something or just that the code to test the amount of memory is new.

[SteveFosdick]

I have found the issue: there is a constant for the maximum amount of memory the checkramsize routine will scan and it was set too low to check to the end of the memory sizes I was trying.

[rjpontefract]

Ah yes, I changed that when I was playing around getting it to build:

.set max_ram_size, 0xFFFE0000 ; Address at which checkramsize will stop

There are two small fixes in the 2.03 version that aren't in the 2.02 version:

680c680
<       MOVEA.L nmi_handler_table, A0             ; Pointer to NMI routine in A0
---
>       LEA     nmi_handler_table, A0             ; Pointer to NMI routine in A0
1591c1591
<       SUBQ.L  #1, A0
---
>       SUBQ    #1, A0

I'd recommend updating the version string to 2.03 and using that version with b-em. Maybe use the current date for the build date string?

I've been trying to find out what the CiscOS license allows and doesn't allow. Unfortunately, the text is contradictory. There are a number of bugs in 2.03 that it would be good to fix.

[SteveFosdick]

Thanks. I am getting some warnings from vasm:

warning 2028 in line 984 of "CiscOs203.asm": using signed operand as unsigned: 255 (valid: -128..127), -1 to fix
>	MOVEQ	#$FF, D0                          ; set unknown processor

warning 2028 in line 1030 of "CiscOs203.asm": using signed operand as unsigned: 255 (valid: -128..127), -1 to fix
>	MOVEQ	#$FF, D0                          ; Set FPU flag

warning 2028 in line 2295 of "CiscOs203.asm": using signed operand as unsigned: 255 (valid: -128..127), -1 to fix
>	MOVEQ	#$FF, D4                          ; Point is off screen so D4(R4) = -1

warning 2028 in line 2536 of "CiscOs203.asm": using signed operand as unsigned: 255 (valid: -128..127), -1 to fix
>	MOVEQ	#$FF, D4                          ; Point is off screen so D4(R4) = -1

warning 2028 in line 2744 of "CiscOs203.asm": using signed operand as unsigned: 138 (valid: -128..127), -118 to fix
>	MOVEQ	#$8A, D0                          ; OSWORD &8A (138): Insert value into buffer

warning 2028 in line 3726 of "CiscOs203.asm": using signed operand as unsigned: 255 (valid: -128..127), -1 to fix
>	MOVEQ	#$FF, D2                          ; Size of buffer

warning 51 in line 6510 of "CiscOs203.asm": instruction has been auto-aligned
>	MOVE.L	#$00000000, D6

As far as I can see, the first warning is that MOVEQ will sign-extend the 8-bit value loaded so that will leave the relevant register set to 0xffffffff not 0x000000ff. What I have not established is whether this is a bug or harmless in the context.

[rjpontefract]

I suspect that in the first two cases, it doesn't matter as the code following just moves a byte anyway:

BSR	check_cpu                         ; Find out which CPU is present
MOVE.B	D0, cputype                       ; ...and store it
BSR	check_fpu                         ; Find out which FPU is present
MOVE.B	D0, fputype                       ; ...and store it

The comment on the next two seems to imply that it is intended:

MOVEQ #$FF, D4 ; Point is off screen so D4(R4) = -1

The call to OSBYTE seems wrong to me. It should be using MOVE.B not MOVEQ as it will be sign extended.

MOVEQ	#$8A, D0                          ; OSWORD &8A (138): Insert value into buffer
MOVEQ	#$03, D1                          ; Buffer 3: printer
BSR	SWI_OS_Byte                       ; Call OS_Byte

This does seem to be done correctly in other cases where the D0 value is > 127, so it was probably an oversight:

MOVE.B	#$81, D0
MOVE.B	#$F6, D1
MOVE.B	#$FF, D2
BSR	SWI_OS_Byte                       ; Mouse left button

The last one seems wrong, but the call following is commented out:

	MOVEQ	#$FF, D2                          ; Size of buffer
;	BSR	SWI_OS_ConvertHex2                ; Print device id
;	BSR	SWI_OS_Write0

Later, where it is used, the code is correct:

	MOVE.L	#$000000FF, D2                    ; Size of buffer (not quite $FF but this will do)
	BSR	SWI_OS_ConvertHex2                ; Convert to string

The auto align warning at the end could be mitigated by adding this before it:

ALIGN 4

[SteveFosdick]

Thanks.

On why the CPU detection doesn't work, that is because the 68K emulation doesn't emulate the correct stack frame for the address exception. It does push something onto the stack and says it only works for 68000 but what is pushed doesn't match any of the stack sizes expected by the CPU detection code. There are comments in the relevant code to the effect that this bit of the emulation is not complete.

[rjpontefract]

It's odd, because as far as I can tell, the 68K emulation does it correctly for the 68000. The 68000 documentation says:

and the emulation code code does:

INLINE void m68ki_stack_frame_buserr(uint sr)
{
    m68ki_push_32(REG_PC);
    m68ki_push_16(sr);
    m68ki_push_16(REG_IR);
    m68ki_push_32(m68ki_aerr_address);  /* access address */
    /* 0 0 0 0 0 0 0 0 0 0 0 R/W I/N FC
     * R/W  0 = write, 1 = read
     * I/N  0 = instruction, 1 = not
     * FC   3-bit function code
     */
    m68ki_push_16(m68ki_aerr_write_mode | CPU_INSTR_MODE | m68ki_aerr_fc);
}

So both agree that it's 14 bytes, but the CiscOS code checks for 18 bytes.

	MOVEQ	#0x01, D0                         ; default to 68000 processor
	CMPI.B	#0x12, D7                         ; compare frame size with that of 68000
	BEQ.S	proc_found                        ; if 68000 size then exit

Maybe I'm missing something. But, as you say, the other CPU types aren't emulated correctly, so it doesn't really matter until they are done.

[SteveFosdick]

Somewhere, I think, you asked about the new tube config sections. Running the tubecfg version should add some to .config/b-em/b-em.cfg but here is the extra one for the 68K:

[tube_13]
name=68000
cpu=68000
romsize=0x8000
bootrom=CiscOs203.rom
speed=4
ramsize=0x2000000

For the boot ROM, you can also include a full path if you have built it out of tree,

[hoglet67]

I don't know if the original hardware that CiscOS ran on was ever made public. T

FYI, there is a photo here:
https://acorn.huininga.nl/pub/projects/CiscOS/Pics/IMG_0366.JPG

[rjpontefract]

Thanks for the configuration example.

It seems that path is freed twice and causes an abend on the second one:

                           fclose(romf);
                            if (path)
                                al_destroy_path(path);
                            if (tube->cpu->init(tuberom)) {
                                tube_updatespeed();
                                tube_reset();
                                if (path)
                                    al_destroy_path(path);
                                return;

Here's the output from an LLDB session:

Process 49030 stopped
* thread #5, stop reason = EXC_BAD_ACCESS (code=1, address=0x6e1921846704)
    frame #0: 0x0000000101e13008 liballegro.5.2.dylib`_al_bdestroy(b=0x00006e1921846700) at bstrlib.c:1002:22 [opt]
   999   *  been bdestroyed is undefined.
   1000  */
   1001 int _al_bdestroy (_al_bstring b) {
-> 1002         if (b == NULL || b->slen < 0 || b->mlen <= 0 || b->mlen < b->slen ||
   1003             b->data == NULL)
   1004                 return _AL_BSTR_ERR;
   1005
Target 0: (b-em) stopped.
warning: liballegro.5.2.dylib was compiled with optimization - stepping may behave oddly; variables may not be available.
(lldb) p b
(_al_bstring) 0x00006e1921846700
(lldb) p b->slen
error: Couldn't apply expression side effects : Couldn't dematerialize a result variable: couldn't read its memory
(lldb) bt
* thread #5, stop reason = EXC_BAD_ACCESS (code=1, address=0x6e1921846704)
  * frame #0: 0x0000000101e13008 liballegro.5.2.dylib`_al_bdestroy(b=0x00006e1921846700) at bstrlib.c:1002:22 [opt]
    frame #1: 0x0000000101de8b70 liballegro.5.2.dylib`al_destroy_path(path=0x00006000015c6700) at path.c:438:7 [opt]
    frame #2: 0x0000000100135f48 b-em`model_init at model.c:326:37 [opt]
    frame #3: 0x0000000100135dec b-em`model_init at model.c:371:5 [opt]
    frame #4: 0x0000000100132e70 b-em`main_restart at main.c:363:5 [opt]
    frame #5: 0x000000010009eb50 b-em`change_tube(event=<unavailable>) at gui-allegro.c:1178:5 [opt]
    frame #6: 0x00000001001332d4 b-em`main_run at main.c:552:17 [opt]
    frame #7: 0x00000001001336fc b-em`_al_mangled_main(argc=<unavailable>, argv=<unavailable>) at main.c:658:5 [opt]
    frame #8: 0x0000000101e23c34 liballegro.5.2.dylib`+[AllegroAppDelegate app_main:] [inlined] call_user_main at osx_app_delegate.m:217:10 [opt]
    frame #9: 0x0000000101e23c18 liballegro.5.2.dylib`+[AllegroAppDelegate app_main:](self=<unavailable>, _cmd=<unavailable>, arg=<unavailable>) at osx_app_delegate.m:228:5 [opt]
    frame #10: 0x0000000182685ff4 Foundation`__NSThread__start__ + 716
    frame #11: 0x00000001814a9034 libsystem_pthread.dylib`_pthread_start + 136
(lldb) exit

[SteveFosdick]

Oops, I will look at that - that may have been a merge mistake.

Meanwhile, see https://stardot.org.uk/forums/viewtopic.php?t=27849 - it looks like checking for a stack frame of 18 bytes for the 68000 is a bug in CiscOS.

[SteveFosdick]

Fix for the double-free pushed.