[swan] Add 4x8 font, improve memory usage

[asiekierka]

This PR is the next step of the WonderSwan ELKS port, and adds some quality-of-life improvements and cleanup:

4x8 font support

This adds support for an alternate 4x8 font and sets it as the new default. This gives the console's screen a larger (if arguably less readable) 56x18 character resolution.

For various reasons, this mode requires more memory than the 8x8 font mode. However, this is not too important, as I had to increase the amount of memory reserved for video with graphics mode support in mind regardless - which brings us to...

Rearranged memory layout

Given that, for supporting a 224x144 4bpp framebuffer, the area between 0x03800 and 0x07EFF has to be reserved, this left me with only 32512 bytes of free contiguous space - requiring a further reduction in kernel heap size. However, there is also a fair amount of contiguous free space from 0x00600 onwards. As such, I made a simple patch to place the kernel's near data/BSS sections before the area reserved for video/graphics data, but the kernel's near heap section after.

I've also documented the memory layout as it stands today, as it is getting a little unusual at this point.

Implemented hard reset / power off hooks

This is a simple bonus patch that allows the system to both reset itself (by jumping to the reset vector of the ELKS kernel, which might not be ideal in isolation?) and power itself off (by using a feature of the console itself).

[ghaerr]

Thank you @asiekierka for your next Swan installment! I'm going to make a few comments, mostly to do with our ongoing ifdef problem in ELKS, but also possibly smaller issues. I'll make those comments beside the associated code.

Regarding ifdef CONFIG_ARCH_* in many places, as well as in general C ifdef's in general, I view the entire lot as things that should be removed entirely, if at all possible. Looking at a codebase with no ifdefs versus one with many gives one a huge difference in feeling. A new and better world can be constructed with tons of C code with no ifdefs, and the opposite with them lol.

I spent lots of time removing ifdefs in the early ELKS codebase, but heaven knows I've also added too many back, and the time always come to try to swat them out again. I'll do a code review on this PR, and likely will recommend we move code to specific architecture files, (e.g. bell.c) like we've already done for timer-*.c and irq-*.c. I'm open to your ideas on how to keep the cruft smaller as well, without having to add too many extra function calls, etc.

[asiekierka]

The problem with bell.c for WonderSwan specifically is that the wavetable used by the bell is actually initialized by the console code, because it's the console code which manages allocation of the video area (to maximize the amount of space available for kernel data/BSS, and make it possible to reduce the space used by f.e. enabling fewer consoles). As such, the two are non-obviously linked together.

However, I do see that for example the hard reset/power off code could be moved from #ifdefs to architecture-specific files.

[ghaerr]

Regarding all these ifdefs - it's your port and your specific console-*swan.c file, so I am OK with all of them. IMO, getting rid of ifdefs is better, even at the cost of "slightly larger code" and using an if (setting) instead.

Your call. Same for HALFWIDTH, etc. Someday someone may want to have all this dynamically and it gets to be rewritten.

[asiekierka]

I do eventually want to get to the point where this is configured dynamically. However, there's an unsolved question I had before I approached that. I haven't had time to write the issue yet, but the gist of it is: "given some programs can support graphics mode and directly write to the graphics data space, which will corrupt the font and console data present therein, how - and when - should that be handled by the console driver?" It's probably a novel problem, as IBM-based platforms, as well as the PC-98, tend to separate text and graphics data...

[ghaerr]

Ok. Since this PR is getting a bit complicated and I'm asking a lot to remove ifdefs, lets discuss the graphics/text mode issue separately. There are upcoming issues in that area for IBM PC vs PC-98 and other issues, now that we're starting to see graphics programs written for ELKS. I'm also interested in solving the problem of writing text to the "console" when graphics mode is ON.

[ghaerr]

Adding a dynamic heap offset start is a nice enhancement, but things are getting ifdef messy. Lets leave this in as you have it for now, but I think it's time to split this whole routine outside of system.c into something more readable.

I've made a note of it, and will improve later.

[ghaerr]

I've been aware for some time that hard_reset_now and apm_shutdown_now need to get pulled out into specific architecture files. Lets leave this in for now, I've made a note of it.

[asiekierka]

Okay, I think that covers the feedback? (Well, excluding #ifdef HALFWIDTH_FONT, but as I said, I'd like to not work on the console code too much before it's inevitably broken by adding text/graphics mode switching.)

[ghaerr]

Thanks, give me a bit to review, I'm running out of time this morning. Thank you!

[asiekierka]

One concern I have with further upstreaming of this port:

For any hope of real-world production use on a console, there's a need to implement support for banking (that is, giving each user process its own 64 KiB bank of memory, then switching them whenever DS is also switched or the user process's DS is accessed). I'm worried this, itself, may generate a number of #ifdefs substantial enough that you might want to consider instead letting the WonderSwan port live as its own fork of ELKS, if long-term maintenance cost is your concern - as I feel that specific feature will require a lot of annoying little patches, and I can't think of another 8086-80286 target that would require it (with perhaps the sole exception of being able to take advantage of this for EMS support that's not limited solely to buffers, but I'm not sure that's a good enough reason).

[ghaerr]

@asiekierka: I've completed a code review, there's still a few items to address. I'll post those next to the code.

concern I have with further upstreaming of this port
I'm worried this, itself, may generate a number of #ifdefs substantial enough
that you might want to consider instead letting the WonderSwan port live as its own fork of ELKS

Thank you for bringing this up; you may be right that bank switching specific to only Swan might prove to be very messy to too many kernel files, or other reasons. I don't really know, but what we might want to do also depends on other things, such as your preference of maintaining a specific fork that allows you easier access and doing whatever you want, and/or whether you might want to pull new kernel or other features from upstream or not.

There are benefits to ELKS and myself though, by continuing Swan updates here, for some time period at least. Ports that are written well help identify areas where the kernel, tools or applications may want to be refactored, and this helps everyone, especially future porters. Having a talented programmer such as yourself around helps ELKS by identifying areas that can be improved, that otherwise might not be noticed. I myself enjoy seeing others finding ELKS useful and easy to work with, and continue to learn with the variety of requests/issues/fixes being discussed or submitted.

I would say it's probably a bit early to stop upstream submissions, as we're both just learning about the peculiarities of what and how a full-fledged Swan port would best be engineered, and that kind of thing might be best initially handled here, where I have to be involved in decisions about mainline code changes. That said, I have no problem if or when you'd like to take over and maintain your own port, and will be glad to continue help you as your project. You're welcome to fork at any time you like, or fork now and start working full speed in a separate sandbox, even if you later decide you'd like to push changes upstream.

All of this really depends on what eventual outcome is desired by both parties, so lets keep discussing it.

there's a need to implement support for banking (that is, giving each user process its own 64 KiB bank of memory, then switching them whenever DS is also switched or the user process's DS is accessed)

The first part, bank switching RAM and associated DS reloads on task switches would be interesting to talk about. Possibly start with a complete replacement of irqtab.S to allow experimentation without affecting other ports. Even the upper level (re)scheduling ends up returning through the bottom half of irqtab. The kernel either runs on the "kernel stack" portion of the task structure, or runs in a special kernel stack when kernel code, rather than user code, is interrupted. If the task structure and all of the kernel data segment stays resident, then bank switching applications at a known address will be lots easier.

A much harder part might be hunting through the entire kernel looking for interrupt or other code that expects the process is resident/mapped. In general, the kernel only writes into the stored register set or other parts of the task structure, not the task itself, except for signals, where are handled slightly differently, and involve writing to the user stack outside kernel DS.

Let me know what you think.

Thank you!

[ghaerr]

If I'm not mistaken, the reference to _heapofs, which I think is non-existent, will break the build. I think you mean heapofs. Are you building the IBM PC version also after testing, after Swan?

However, there's a slight problem with using heapofs here, instead of _endbss. The problem is that the old endbss above on Line 42, along with heapofs, are set to an even number. Here, _endbss was used, which is not always the value of endbss (or heapofs). Specifically, it seems that if _endbss happens to end on a paragraph boundary + 1, then it will be converted to a paragraph boundary + 2 in Line 46 above. Then, here, we take that value and add 15 to it, which pushes us over to yet the next paragraph alignment. In doing so, we end up discarding a full paragraph from the kernel heap needlessly.

I realize this is quite minor, but wanted to point it out, since it is incorrect. I haven't yet figured and easy way to solve this without more ifdefs. [EDIT: or just replacing setup_arch() with a version specific for Swan and removing the new config.h SETUP_ options as unneeded].

There is yet a bigger issue with using the new SETUP_HEAPOFS macro: the problem is that in init/main.c::kernel_banner(), _endbss is used instead of SETUP_HEAPOFS to display the size of the kernel heap, which would be incorrect. I'm trying to think of a way to specify a kernel heap offset without rewriting this function and kernel_banner. This might be a case where it's easier to replace setup_arch and/or kernel_banner completely for Swan, or not.

See my additional comment below regarding config.h that might help determine the best way for a non-PC architecture to specify the initial kernel and application heap start/end addresses or segments. When things get complicated like this could, I don't mind starting with a separate file for the functionality, rather than inventing a lot of new config.h defines that then have to be ifdefed into IBM PC code, and neither side uses the other's stuff.

Thinking while I'm writing here, having setup_arch be a separate routine makes some sense seeing that setup_arch is only called once by main.c::early_kernel_init() and then immediately just calls heap_add(endbss, heapsize):

    endbss = setup_arch();          /* sets membase and memend globals */
    heap_add((void *)endbss, heapsize);

If endbss were renamed and made a global in main.c, kernel_banner() could use that instead of _endbss and things would probably work OK. If that route is taken, we might as well just write a specific version of setup_arch for Swan.

[asiekierka]

I think a separate setup_arch makes the most sense.

[asiekierka]

The problem is that in init/main.c::kernel_banner(), _endbss is used instead of SETUP_HEAPOFS to display the size of the kernel heap,

It's not, though? _endbss is only used to display the size of kernel BSS, which is correct. heapsize is used to display the size of the kernel heap.

[ghaerr]

Shouldn't SETUP_HEAPOFS be written using arithmetic with CONFIG_ROM_KERNEL_DATA? When that value changes, this computation will also have to, or things won't work. In addition, should 0x8000 be hardcoded, or is this dependent and connected with SETUP_HEAPSIZE?

Regardless, as a result of the arch_setup() issue above, perhaps a good solution is to code a straightforward arch_setup for Swan that either contains all the definitions locally, or uses upper level constants defined here, such as 32K heap, and then let arch_setup handle the nitty gritty details of how internal kernel variables need to be set as a result. This might be a good reason to revert to just specifying SETUP_HEAPSIZE 32768 and SETUP_USERHEAPSEG 0x1000 and using them directly in a custom setup_arch() without ifdefs.

We are getting closer and I am happy to be having this discussion! ELKS can be improved here, and more split-outs, or perhaps just a single ifdef CONFIG_ARCH_SWAN around a specific arch_setup() remaining in system.c is easier for now; I can clean that up later after keeping both systems working.

[asiekierka]

Shouldn't SETUP_HEAPOFS be written using arithmetic with CONFIG_ROM_KERNEL_DATA?

I wanted to do that, however there's a small problem: the .config files shipped with ELKS seem to set such values to 0x8000 (with the 0x prefix present), while editing them in make menuconfig seems to return them as 8000 (with the 0x prefix absent). For these values to work in macros, one has to be chosen.

[ghaerr]

Oh geez. I remember trying to fix that in the config stuff. It ended up becoming very messy and I couldn't get it totally fixed. You are welcome to take a pass at it, but our config machine is quite old and can't be easily updated.
Perhaps with the new move to a separate architectural file, this won't be an issue anyways.

[asiekierka]

Actually, I can just use kernel_ds in the meantime... so that's fine.

[asiekierka]

There's one other problem: with the current approach of disabling sys_utils/clock, the rootfs build script complains that the file is not present, as it is required by the :128k tag.

[asiekierka]

... so to fix this problem, because I don't want to make the Makefiles even worse than they already are, I have pushed my untested RTC code. I'll be able to test it extensively, but not right now.

[asiekierka]

Let me know what you think.

I want to keep contributing to upstream, if possible; when I say "fork", I'm thinking "friendly fork", not "hostile fork". It's just a matter of convenience for the broader project - there might be a point in time where the changes become too numerous to justify having them mainline for the "cool" factor (given that running ELKS on a game console is cool, but ultimately not really a practical thing people [EDIT] okay, using an 8088 in 2025 is impractical for many reasons, but I think there's levels of impracticaly involved here...).

I have no preference myself, it was just a matter of trying to set expectations straight on my behalf so neither you nor I are caught by surprise.

[asiekierka]

Also, another issue I'd like to hint at while I'm here - what about prectimer.c? It's also used in libc, so I'm wary of adding a prectimer-swan.c option, but it may have to be taken care of too...

[ghaerr]

Lets revert this and continue to use ifndef CONFIG_ARCH_SWAN. The reason being is we want to always build nano-X when possible, even when its not selected in the image config file.

Compiling all the applications all the time helps determine when an application is using features unavailable by the kernel (or system). This is good to know, as you've found out with clock.

[asiekierka]

Then I need to also add ifndef CONFIG_ARCH_8018X, though :-)

[ghaerr]

You do? Did you just find that Nano-X doesn't build on the CONFIG_ARCH_8018X case? I thought it always has in the past, of course it won't run. I have never encountered a build problem with 8018X before.

[asiekierka]

ia16-elf-gcc -mcmodel=small -melks-libc -mtune=i8086 -Wall -Os -mno-segment-relocation-stuff -fno-inline -fno-builtin-printf -fno-builtin-fprintf -o bin/nano-X nanox/srvmain.o nanox/srvfunc.o nanox/srvutil.o nanox/srvevent.o engine/devdraw.o engine/devmouse.o engine/devkbd.o engine/devclip1.o engine/devpal1.o engine/devpal2.o engine/devpal4.o nanox/srvnet.o 
nanox/srvmain.o:function GsInitialize: error: undefined reference to 'scrdev'
nanox/srvmain.o:function GsInitialize: error: undefined reference to 'scrdev!'
nanox/srvmain.o:function GsInitialize: error: undefined reference to 'scrdev'
nanox/srvmain.o:function GsInitialize: error: undefined reference to 'scrdev!'
nanox/srvmain.o:function GsInitialize: error: undefined reference to 'scrdev'
nanox/srvmain.o:function GsInitialize: error: undefined reference to 'scrdev!'
nanox/srvmain.o:function GsInitialize: error: undefined reference to 'scrdev'
nanox/srvmain.o:function GsInitialize: error: undefined reference to 'scrdev!'
engine/devmouse.o:function GdCloseMouse: error: undefined reference to 'mousedev'
engine/devmouse.o:function GdCloseMouse: error: undefined reference to 'mousedev!'
engine/devmouse.o:function GdGetButtonInfo: error: undefined reference to 'mousedev'
engine/devmouse.o:function GdGetButtonInfo: error: undefined reference to 'mousedev!'
engine/devmouse.o:function GdReadMouse: error: undefined reference to 'mousedev'
engine/devmouse.o:function GdReadMouse: error: undefined reference to 'mousedev!'
engine/devmouse.o:function GdOpenMouse: error: undefined reference to 'mousedev'
engine/devmouse.o:function GdOpenMouse: error: undefined reference to 'mousedev!'
engine/devkbd.o:function GdOpenKeyboard: error: undefined reference to 'kbddev'
engine/devkbd.o:function GdOpenKeyboard: error: undefined reference to 'kbddev!'
engine/devkbd.o:function GdCloseKeyboard: error: undefined reference to 'kbddev'
engine/devkbd.o:function GdCloseKeyboard: error: undefined reference to 'kbddev!'
engine/devkbd.o:function GdGetModifierInfo: error: undefined reference to 'kbddev'
engine/devkbd.o:function GdGetModifierInfo: error: undefined reference to 'kbddev!'
engine/devkbd.o:function GdReadKeyboard: error: undefined reference to 'kbddev'
engine/devkbd.o:function GdReadKeyboard: error: undefined reference to 'kbddev!'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:73: bin/nano-X] Error 1
make[2]: Leaving directory '[snip]/elks/elkscmd/nano-X'
make[1]: *** [Makefile:56: all] Error 1
make[1]: Leaving directory '[snip]/elks/elkscmd'
make: *** [Makefile:16: all] Error 2

Yes, it must have regressed.

[ghaerr]

Oh my goodness. It appears that what's really happened is that the buildimage.sh never actually built the 8018X version since the shell function build_rom_8018x was miscalled as build_8018x, and never actually built?!!

We're running set -e so that shouldn't have happened, but I guess it could have just terminated at the build_8018x with an error and gone unnoticed.

Ok. I've made a note to look into the nano-X build, thank you!

[asiekierka]

That's because, from what I can see, CI doesn't use buildimages.sh; it uses ./build.sh auto allimages, which runs make -j1 images, which only builds IBM images (not 8018x, not PC-98, not Swan)?

[ghaerr]

Yes, you're right about the CI. We'll want to deal with the difference between build.sh and buildimage.sh at some point in the future, by all rights there should only be one script. It is nice to keep the CI push build times down though. It would be nice to have a quick script run for pushes and then a longer script on release to build all the images. I have to do that manually now and upload the images on releases.
,
It appears I never noticed the bug in buildimages.sh before for 8018X, and build.sh worked, because CONFIG_APP_NANOX wasn't set.

[ghaerr]

Hello @asiekierka, I've completed my review of your most recent changes, thank you! We're almost there, thanks for bearing with me on this. :)

[asiekierka]

Done. Don't worry about it - any code review is a learning opportunity.