Hi Jeremy, others,
> Kernel patches - http://server2.fdos.org/tests/kernel-win3-patch.diff > rest of sources (kernel, FreeCOM, format, fdisk, sys, share, edit) - > https://github.com/fdos I have been browsing those patches a bit. So I can share some insights about them, also implying many questions. Maybe Jeremy can explain the areas which looked complicated or confusing to me. Maybe the confusing areas even include some which actually could use code improvements :-) Sorry about the length of the "review" below. > hdr/portab.h | 7 + > kernel/entry.asm | 186 ++++++++++++++++++++++++- > kernel/init-mod.h | 4 +- > kernel/int2f.asm | 51 ++++++- > kernel/inthndlr.c | 340 +++++++++++++++++++++++++++++++++++++++++++++- > kernel/kernel.asm | 37 ++++- > kernel/main.c | 19 ++- > 7 files changed, 623 insertions(+), 21 deletions(-) portab.h just introduces loword and hiword access macros entry.asm: there is a new int 13 handler, but the style worries me. It does CLI STC, then calls the original handler while forcing DS to be the kernel one, then increments SP twice, then returns with STI via RETF 2. Why all those things? Changing the interrupt flag, carry flag and DS all seem to be a bad idea and SP should probably not be incremented like that either, it gets unaligned for a moment. The actual action "if carry set and AH is 6 on return" even is commented out right now and the 8086-compatible method of pushing the value 13h for that looks convoluted. There also is some intWrapCall helper which uses a lot of stack. The next new thing in entry.asm is a new step for the int 19 handler which uses intWrapCall to call int 19 instead of just doing "int 19", but I do not see what the advantage of that is supposed to be. Interestingly, int21_user only calls dos_crit_sect if NOT compiled with Windows 3.1 386enh support? If compiled WITH that, there will instead be an int 2a.8001 call (preserving AX) before byte InDOS is incremented. Also, dos_crit_sect is only called at int21_3 if NOT compiled with Windows support, while it is called after decrementing InDOS at int21_exit instead if compiled WITH Windows support? Note that there is NO call to int 21.8101 (leave critical section) at that point (it is commented out via "if 0") and that dos_crit_sect is what is called (copies AX to a local variable, calls int 2a.82 with preserved AX) which leaves all critical sections 0-7, instead of just a specific one as int 2a.810x would do after int 2a.800x has been called to enter it? What is the "skip to int21_3 if ah is 6" thing about, which has a comment "testing, this function call crashes" next to it? init-mod.h: Adds the int13_handler and makes the lol FAR *const LoL, adding const to the properties of the list of lists pointer. int2f.asm: Adds IntDosCal style functions 13h and 46h and clears BX for function 4a33h for some "undocumented" reason? If Win 3.1 support is compiled in, some non-reentrant switch to a local mux2F_stk_top-30 will be triggered, copying 13 words to it, while preparing for IntDosCal style int 2f functions, of course copying everything back to the user stack later. Why the effort? inthndlr.c: Includes a whole new win.h (as well as debug.h) but win.h itself does not seem to be part of the Win 3.1 compatibility patch? I seem to be missing something here? It also accesses BIOS and user int 13 and int 19 vectors. The updated int2F_12_handler() now has ah=13h "disk interrupt hook" functionality as described in RBIL, which is used by Windows to mess with disk I/O in WDCTRL.386 which insists on this to at least return a changed DS when invoked. It also implements int 2f.1600 / 2f.160a (both NOP), int 2f.1603 (get instance data, dummy because 1607.bx=15 is preferred), int 2f.1605 (Win startup broadcast, returns some data structure, of course does not return optional vm86 on/off callback), 2f.1606 (Win exit broadcast, just logged at the moment) int 2f.1607 is the large VxD API, BX selects which VxD to work with: Only bx 15h supported, DOSMGR: CX 0 returns a flag, the DOS driver data segment (segment of nul_dev) and a patch table, CX 1 activates and CX 2 optionally deactivates patches, CX 3 returns DOS sizes of DOS data structures, CX 4 tells about instance data structures etc.: FreeDOS just reports it would have patched everything as requested for critical section support, to keep Win3 from trying to patch it. This involves features such as int 2a.8x0x calls, machine ID tracking, limiting atomic I/O size (split up to multitask better) and notifying Windows if "halt on stack error" or for "floppy drive DJ messages". For function 3, only the CDS size is counted: FreeDOS pretends that SFT, device list and SDA would all be zero size. Sounds dangerous! Why is the implementation like that? Would Windows only care about the CDS size? What if multiple DOS boxes are opened in Windows 3? Our function 4 suggests that FreeDOS tries to avoid function 3? In function 4, DOS could tell which data structures should be kept separately for each instance: FreeDOS returns none, but comments say that this would mean "all"? Also, RBIL says MS DOS 5/6 would return DX=0 here, meaning "no instanced data structures supported", which would be safer if it would not mean that int 2f.1603 is used instead? Function 5 returns the size of the device driver at ES, which checks for sub-MCB type "D" and might actually provide functionality which some advanced MEM style tools are happy about as well? :-) Int 2f.1607 can also be used to extend a20 and mouse handling etc. Even non-BIOS int 13 handlers are supposed to provide services here to tell whether they are WDCTRL / FastDisk compatible. The next few int 2f.16xx functions are mostly no-ops with logging: 08 init complete broadcast, 09 begin exit broadcast, 0B identify TSR (0A is where Win reports its own version, 0B is for Win-aware TSR) 80 release time slice (just calls DosIdle_hlt() and returns AX=0) (0C would be detect ROM-Windows, 0E would be about Win95 boot logo, 0F would be about some MS DOS 7 Win9x handlers, 10 is a deprecated XMS thing, 11 does something with SHELL=..., 12 seems VTD and clock related, 13/14 are Win9x registry related, 15 is for Win9x SAVE32), 81 (begin critical section, CALLED by DOS, not implemented by DOS!), 82 (end critical section, similar) (function 2f.1683 would return the current VM ID not implemented, 84 provides MANY Win VxD API, 85 is about invoking other VM, 86 and 87 are for DPMI, 88 for 386MAX, 89 is Win kernel idle, 8a are DPMI extensions, 8b VM focus jump, 8c restart, 8e app/VM title...) 8f "close awareness" (Win9x/Win4) returns success for most things APART from "query close", does that mean FreeDOS windows tell Win9x that they do not want to be closed? Would they exist there at all? There is no "win old app" (clipboard) in the kernel, in general, but for some reason, int 2f.4601 and 2f.4602 ARE PLANNED to exist when Win3 support is compiled in: Not sure where Jeremy got the description of what those would do, RBIL is rather vague here? Not sure why the int 2f handler assumes AH to be 12 (DOS 3 internal file and similar helpers) at the end. Maybe those should come FIRST to speed up handling? Windows related things are called less often. As mentioned above, the intXXregs and intXX_filter wrappers and local stack things confuse me. Those seem to be used for int 19 and int 13, with the former just wiping the first 512 bytes of the HMA and the latter doing something if r.a.b.h. is 6 and r.param.b.l is 0-7fh, but claiming to be related to floppy disk change detection. Confusing, too, but ultimately triggers pddt->ddt_descflags |= DF_DISKCHANGE if the flags of that drive have both CHANGELINE and CURBPBLOCK set? kernel.asm: Our intvec_table is supposed to track int 10h, 13h, 15h, 19h and 1bh, the patch gives the int 13 and 19 pointers an actual use and names: BIOSInt13 and BIOSInt19. Confusingly, it also adds another "int 13" item to the end of the list, called UserInt13. Why that? For some unknown reason, this also changes the padding between the NetBios and the NetRetry offsets. Why that? -times (26h - 0ch - ($ - DATASTART)) db 0 +times (26h - 12h - ($ - DATASTART)) db 0 Apparently, the patch fixes a bug in the instance table? instance_table: ; should include stacks, Win may auto determine SDA region ; we simply include whole DOS data segment -dw 0, seg _DATASTART ; [SEG:OFF] address of region's base +dw seg _DATASTART, 0 ; [SEG:OFF] address of region's base After the firsttsftt, there now is some zero padding for 5 x 59 bytes of SFT data plus one padding byte. Why that? Older comments here were: "The first 5 sft entries appear to have to be at DS:00cc" which refers to where the first 5 SFT entries "started", but there were not actual SFT contents here. Apparently something Windows cares about? Is having all-zero SFT entries a good idea and is 59 defined somewhere? If yes, it could make the code easier to read to use that constant. If Win 3 support is compiled in, a 256 byte stack before mux2F_stk_top is added after the one for int2f_stk_top. Nitpick: Inconsistent upper and lower case policy between the two names. And why the separate stack? Note that the error_tos, disk_api_tos and char_api_tos stacks each use "times STACK_SIZE dw 0x9090" for their stack spaces, which gives a nice way to see how much of them got used. Maybe the TWO int2f stacks could be changed to do the same? Similar for blk_stk_top and clk_stk_top, even when those are respectively 192 and 64 words: Could pre-fill with 0x9090 instead of pre-filling with "dw 0"? Our STACK_SIZE is 384/2 (192 words). The int13_handler, like all other existing int??_handler, jumps to the reloc_call_int??_handler and is FOLLOWED by call forceEnableA20, which looks weird. But all HMA trampolines seem to be like that? main.c: Simply adds const to struct lol FAR * const LoL = &DATASTART; and adds a few comments. The new int13_handler installation step is actually commented out (using C++ comment style). The intvec_table is 6 instead of 5 elements here, related to the kernel.asm change which puts TWO different int 13 vector stores there. Which still confuses me. I hope my review is shorter than the 870 lines of the patch itself :-) And I hope Jeremy will find the time to explain some of the odd parts. Thanks! Regards, Eric _______________________________________________ Freedos-devel mailing list Freedos-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/freedos-devel
