| Message ID | 1468486382-21609-6-git-send-email-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 14, 2016 at 10:53:02AM +0200, Gerd Hoffmann wrote: > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > src/optionroms.c | 2 ++ > src/romlayout.S | 39 ++++++++++++++++++++++ > src/sercon.c | 99 +++++++++++++++++++++++++++++++++++++++----------------- > 3 files changed, 111 insertions(+), 29 deletions(-) > > diff --git a/src/optionroms.c b/src/optionroms.c > index f9e9593..f08fcb1 100644 > --- a/src/optionroms.c > +++ b/src/optionroms.c > @@ -442,6 +442,8 @@ vgarom_setup(void) > } > > VgaROM = (void*)BUILD_ROM_START; > + if (romfile_loadint("etc/sercon-enable", 0)) > + sercon_enable(); Minor nit, but why not unconditionally call sercon_enable() and let sercon_enable() check romfile_loadint(). [...] > --- a/src/romlayout.S > +++ b/src/romlayout.S > @@ -522,6 +522,45 @@ irqentry_arg: > DECL_IRQ_ENTRY hwpic1 > DECL_IRQ_ENTRY hwpic2 > > + // hooked int10, for sercon > + DECLFUNC entry_10_hooked > +entry_10_hooked: > + pushl $ handle_10 > +#if CONFIG_ENTRY_EXTRASTACK > + // FIXME: too much cut+paste I'm okay with the cut-and-paste. But, another option would be to use the iretw at the end of the existing irqentry_extrastack to implement the ljmpw into the main vgabios. Something like (totally untested): entry_10_hooked: pushfw // Setup for iretw in irqentry_arg pushl %cs:sercon_int10_hook_resume pushl $handle_10 #if CONFIG_ENTRY_EXTRASTACK jmp irqentry_arg_extrastack #else jmp irqentry_arg #endif Separately, have you considered choosing a separate entry point for entry_10_hooked. That is, changing the above pushl to $handle_sercon_hooked and introducing that function in sercon.c. It seems it would reduce a number of "if (!sercon_splitmode())" checks in the main code, as handle_sercon_hooked() could just use a smaller switch statement and ignore requests it doesn't need to support. Finally, one high level observation is that we know there are a number of quirks in various vgabios emulators. For example, we know some emulators don't handle certain 32bit instructions when in 16bit mode (hence scripts/vgafixup.py), we know some versions of Windows use an emulator that doesn't like some stack relative instructions (hence the vgabios is compiled without -fomit-frame-pointer), and we know Windows Vista doesn't like the extra stack in high ram (the skifree bug). Any thoughts on what happens with these quirks if the main seabios code hooks int10? -Kevin
Hi, > I'm okay with the cut-and-paste. But, another option would be to use > the iretw at the end of the existing irqentry_extrastack to implement > the ljmpw into the main vgabios. Something like (totally untested): > > entry_10_hooked: > pushfw // Setup for iretw in irqentry_arg > pushl %cs:sercon_int10_hook_resume > > pushl $handle_10 > #if CONFIG_ENTRY_EXTRASTACK > jmp irqentry_arg_extrastack > #else > jmp irqentry_arg > #endif Good idea, I'll try it. > Separately, have you considered choosing a separate entry point for > entry_10_hooked. That is, changing the above pushl to > $handle_sercon_hooked and introducing that function in sercon.c. It > seems it would reduce a number of "if (!sercon_splitmode())" checks in > the main code, as handle_sercon_hooked() could just use a smaller > switch statement and ignore requests it doesn't need to support. Makes sense indeed. All functions which only return information are not needed in the splitmode case. > Finally, one high level observation is that we know there are a number > of quirks in various vgabios emulators. For example, we know some > emulators don't handle certain 32bit instructions when in 16bit mode > (hence scripts/vgafixup.py), we know some versions of Windows use an > emulator that doesn't like some stack relative instructions (hence the > vgabios is compiled without -fomit-frame-pointer), and we know Windows > Vista doesn't like the extra stack in high ram (the skifree bug). Any > thoughts on what happens with these quirks if the main seabios code > hooks int10? Good question. Do the emulators (both win, xorg) use the int10 vector set by seabios in the first place? Or go they load the vgabios and run it, including the initialization, and use whatever entry point the init code sets up? I suspect it is the latter. But needs investigation and testing. /me places the item on the todo list. Also a serial console for windows guests isn't that useful, so I wouldn't worry too much about windows emulator issues. cheers, Gerd
On Fri, Jul 15, 2016 at 01:49:49PM +0200, Gerd Hoffmann wrote: > > Finally, one high level observation is that we know there are a number > > of quirks in various vgabios emulators. For example, we know some > > emulators don't handle certain 32bit instructions when in 16bit mode > > (hence scripts/vgafixup.py), we know some versions of Windows use an > > emulator that doesn't like some stack relative instructions (hence the > > vgabios is compiled without -fomit-frame-pointer), and we know Windows > > Vista doesn't like the extra stack in high ram (the skifree bug). Any > > thoughts on what happens with these quirks if the main seabios code > > hooks int10? > > Good question. Do the emulators (both win, xorg) use the int10 vector > set by seabios in the first place? Or go they load the vgabios and run > it, including the initialization, and use whatever entry point the init > code sets up? I suspect it is the latter. But needs investigation and > testing. I think they just call the existing int10 handler. In general, it's not safe to rerun the vga init code. Also, if they did run the init it would lead to extra copies of the SeaVGABIOS version banners in the debug logs, which I don't recall seeing. > Also a serial console for windows guests isn't that useful, so I > wouldn't worry too much about windows emulator issues. It's not uncommon (at least on real hardware) to add sgabios to a system for the boot menus, but then use a regular OS at runtime. The problem with the vga emulation quirks is that they often result in mysterious system failures. Have you considered implementing the serial support as a kind of "serial seavgabios" instead of directly in seabios? That would have the advantage of pulling in all the existing vgabios quirk handling. -Kevin
Hi, Short status update: Project isn't dead. But I'm busy with other stuff and that will not change in August due to holiday season and kvm forum, so don't expect new patches from me before September. For anyone who wants play with this: Pushed my current devel branch to https://www.kraxel.org/cgit/seabios/log/?h=serial (basically the most recent patch series with some not-yet squashed incremental fixes on top). > Have you considered implementing the serial support as a kind of > "serial seavgabios" instead of directly in seabios? That would have > the advantage of pulling in all the existing vgabios quirk handling. Indeed, didn't consider that yet. Briefly looked at this a while back, figured doing this as "serial seavgabios" would allow the reuse of some vgabios code. On the other hand the keyboard handling is easier to do directly in seabios: can queue key events using internal seabios interfaces, no need to hook timer irq (or better serial irq?). I've noticed you've cleaned up vgabios a bit, possibly with the intention to make implementing a serial seavgabios easier? In case you want give it a try: Feel free to grab my branch and run with it. cheers, Gerd
On Fr, 2016-07-15 at 10:35 -0400, Kevin O'Connor wrote: > On Fri, Jul 15, 2016 at 01:49:49PM +0200, Gerd Hoffmann wrote: > > > Finally, one high level observation is that we know there are a number > > > of quirks in various vgabios emulators. For example, we know some > > > emulators don't handle certain 32bit instructions when in 16bit mode > > > (hence scripts/vgafixup.py), we know some versions of Windows use an > > > emulator that doesn't like some stack relative instructions (hence the > > > vgabios is compiled without -fomit-frame-pointer), and we know Windows > > > Vista doesn't like the extra stack in high ram (the skifree bug). Any > > > thoughts on what happens with these quirks if the main seabios code > > > hooks int10? > > > > Good question. Do the emulators (both win, xorg) use the int10 vector > > set by seabios in the first place? Or go they load the vgabios and run > > it, including the initialization, and use whatever entry point the init > > code sets up? I suspect it is the latter. But needs investigation and > > testing. > > I think they just call the existing int10 handler. In general, it's > not safe to rerun the vga init code. Also, if they did run the init > it would lead to extra copies of the SeaVGABIOS version banners in the > debug logs, which I don't recall seeing. Finally found the time to continue with this and ran a bunch of tests. RHEL-5 (known to be affected by x86emu issue) continues to work fine. Xorg server log looks like it goes scan memory for the vgabios instead of depending on the int10 vector: (II) VESA(0): initializing int10 (II) VESA(0): Primary V_BIOS segment is: 0xc000 (II) VESA(0): VESA BIOS detected (II) VESA(0): VESA VBE Version 3.0 (II) VESA(0): VESA VBE Total Mem: 16384 kB (II) VESA(0): VESA VBE OEM: SeaBIOS VBE(C) 2011 (II) VESA(0): VESA VBE OEM Software Rev: 0.0 (II) VESA(0): VESA VBE OEM Vendor: SeaBIOS Developers (II) VESA(0): VESA VBE OEM Product: SeaBIOS VBE Adapter (II) VESA(0): VESA VBE OEM Product Rev: Rev. 1 Running tests with win7 doesn't show any problems too, so I suspect they are basically doing the same. Given these results I think I'll stick to the current approach of integrating this directly into seabios (instead of creating a sgabios-like rom using the seavgabios sources). cheers, Gerd
On Tue, Sep 27, 2016 at 02:00:08PM +0200, Gerd Hoffmann wrote: > On Fr, 2016-07-15 at 10:35 -0400, Kevin O'Connor wrote: > > On Fri, Jul 15, 2016 at 01:49:49PM +0200, Gerd Hoffmann wrote: > > > > Finally, one high level observation is that we know there are a number > > > > of quirks in various vgabios emulators. For example, we know some > > > > emulators don't handle certain 32bit instructions when in 16bit mode > > > > (hence scripts/vgafixup.py), we know some versions of Windows use an > > > > emulator that doesn't like some stack relative instructions (hence the > > > > vgabios is compiled without -fomit-frame-pointer), and we know Windows > > > > Vista doesn't like the extra stack in high ram (the skifree bug). Any > > > > thoughts on what happens with these quirks if the main seabios code > > > > hooks int10? > > > > > > Good question. Do the emulators (both win, xorg) use the int10 vector > > > set by seabios in the first place? Or go they load the vgabios and run > > > it, including the initialization, and use whatever entry point the init > > > code sets up? I suspect it is the latter. But needs investigation and > > > testing. > > > > I think they just call the existing int10 handler. In general, it's > > not safe to rerun the vga init code. Also, if they did run the init > > it would lead to extra copies of the SeaVGABIOS version banners in the > > debug logs, which I don't recall seeing. > > Finally found the time to continue with this and ran a bunch of tests. > RHEL-5 (known to be affected by x86emu issue) continues to work fine. > Xorg server log looks like it goes scan memory for the vgabios instead > of depending on the int10 vector: Interesting. I'm curious how the memory scan works, because I didn't think there was any way to find the vga entry point except from the int10 vector. Were you looking to include this series in SeaBIOS v1.10? We can either delay the release or push the changes into the next release. Also not sure where Igor's patches stand wrt inclusion into QEMU. -Kevin
Hi, > Interesting. I'm curious how the memory scan works, because I didn't > think there was any way to find the vga entry point except from the > int10 vector. Run the init code in emulator? > Were you looking to include this series in SeaBIOS v1.10? We can > either delay the release or push the changes into the next release. 1.10 release in october would be great for qemu as freeze for the 2.8 release is november 1st. That doesn't leave much room to delay things ... I think that means either merge the current sercon version (posted to the list last Wednesday, have you looked at it?) for 1.10 or move it into the next release. > Also not sure where Igor's patches stand wrt inclusion into QEMU. Good question. It isn't yet in the qemu master branch. Igor? Do you have a status update? cheers, Gerd
On Tue, 04 Oct 2016 10:49:41 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > > > Interesting. I'm curious how the memory scan works, because I > > didn't think there was any way to find the vga entry point except > > from the int10 vector. > > Run the init code in emulator? > > > Were you looking to include this series in SeaBIOS v1.10? We can > > either delay the release or push the changes into the next release. > > 1.10 release in october would be great for qemu as freeze for the 2.8 > release is november 1st. That doesn't leave much room to delay > things ... > > I think that means either merge the current sercon version (posted to > the list last Wednesday, have you looked at it?) for 1.10 or move it > into the next release. > > > Also not sure where Igor's patches stand wrt inclusion into QEMU. > > Good question. It isn't yet in the qemu master branch. > Igor? Do you have a status update? So far plan is merge it into QEMU 2.8. I've amended QEMU counterpart according to Radim's and Paolo's reviews and plan to respin it soon. It will depend on Radim's https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08292.html though, so my series will be merged after that > > cheers, > Gerd >
Hi, > So far plan is merge it into QEMU 2.8. > I've amended QEMU counterpart according to Radim's and Paolo's reviews > and plan to respin it soon. No respin yet on the list it seems. What is the status? Do you wait for Radim's patches land upstream first? Can you cc me if you post it? > It will depend on Radim's https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08292.html > though, so my series will be merged after that That series is at v5 now and seems to be ready for merge, even though it didn't land in master yet. cheers, Gerd
On Thu, 13 Oct 2016 09:17:51 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > > > So far plan is merge it into QEMU 2.8. > > I've amended QEMU counterpart according to Radim's and Paolo's reviews > > and plan to respin it soon. > > No respin yet on the list it seems. What is the status? Do you wait > for Radim's patches land upstream first? yep, I'm was waiting on Radim's patches to be ready first. By now they all have been reviewed and I don't expect to any changes in them. So I'll respin v3 todayish as well as v6 of this series. > Can you cc me if you post it? sure > > > It will depend on Radim's https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08292.html > > though, so my series will be merged after that > > That series is at v5 now and seems to be ready for merge, even though it > didn't land in master yet. > > cheers, > Gerd >
diff --git a/src/optionroms.c b/src/optionroms.c index f9e9593..f08fcb1 100644 --- a/src/optionroms.c +++ b/src/optionroms.c @@ -442,6 +442,8 @@ vgarom_setup(void) } VgaROM = (void*)BUILD_ROM_START; + if (romfile_loadint("etc/sercon-enable", 0)) + sercon_enable(); enable_vga_console(); } diff --git a/src/romlayout.S b/src/romlayout.S index 53cc0f5..2a645eb 100644 --- a/src/romlayout.S +++ b/src/romlayout.S @@ -522,6 +522,45 @@ irqentry_arg: DECL_IRQ_ENTRY hwpic1 DECL_IRQ_ENTRY hwpic2 + // hooked int10, for sercon + DECLFUNC entry_10_hooked +entry_10_hooked: + pushl $ handle_10 +#if CONFIG_ENTRY_EXTRASTACK + // FIXME: too much cut+paste + cli + cld + pushw %ds // Set %ds:%eax to space on ExtraStack + pushl %eax + movl $_zonelow_seg, %eax + movl %eax, %ds + movl StackPos, %eax + subl $PUSHBREGS_size+16, %eax + SAVEBREGS_POP_DSEAX // Save registers on extra stack + popl %ecx + movl %esp, PUSHBREGS_size+8(%eax) + movw %ss, PUSHBREGS_size+12(%eax) + popl BREGS_code(%eax) + popw BREGS_flags(%eax) + + movw %ds, %dx // Setup %ss/%esp and call function + movw %dx, %ss + movl %eax, %esp + calll *%ecx + + movl %esp, %eax // Restore registers and return + movw PUSHBREGS_size+12(%eax), %ss + movl PUSHBREGS_size+8(%eax), %esp + popl %edx + popw %dx + pushw BREGS_flags(%eax) + pushl BREGS_code(%eax) + RESTOREBREGS_DSEAX + ljmpw *%cs:sercon_int10_hook_resume +#else +#error FIXME: CONFIG_ENTRY_EXTRASTACK=n not supported yet +#endif + // int 18/19 are special - they reset stack and call into 32bit mode. DECLFUNC entry_19 entry_19: diff --git a/src/sercon.c b/src/sercon.c index c1cc738..4be7441 100644 --- a/src/sercon.c +++ b/src/sercon.c @@ -9,6 +9,7 @@ #include "stacks.h" // yield #include "output.h" // dprintf #include "util.h" // irqtimer_calc_ticks +#include "string.h" // memcpy #include "hw/serialio.h" // SEROFF_IER #include "std/cp437.h" @@ -45,6 +46,8 @@ static void cursor_pos_set(u8 row, u8 col) ****************************************************************/ VARLOW u16 sercon_port; +VARLOW u8 sercon_split; +VARFSEG struct segoff_s sercon_int10_hook_resume; /* * We have a small output buffer here, for lazy output. That allows @@ -64,6 +67,11 @@ VARLOW u8 sercon_attr = 0x07; static VAR16 u8 sercon_cmap[8] = { '0', '4', '2', '6', '1', '5', '3', '7' }; +static int sercon_splitmode(void) +{ + return GET_LOW(sercon_split); +} + static void sercon_putchar(u8 chr) { u16 addr = GET_LOW(sercon_port); @@ -174,6 +182,15 @@ static void sercon_print_utf8(u8 chr) } } +static void sercon_cursor_pos_set(u8 row, u8 col) +{ + if (!sercon_splitmode()) { + cursor_pos_set(row, col); + } else { + /* let vgabios update cursor */ + } +} + static void sercon_lazy_cursor_sync(void) { u8 row = cursor_pos_row(); @@ -222,7 +239,7 @@ static void sercon_lazy_flush(void) static void sercon_lazy_cursor_update(u8 row, u8 col) { - cursor_pos_set(row, col); + sercon_cursor_pos_set(row, col); SET_LOW(sercon_row_last, row); SET_LOW(sercon_col_last, col); } @@ -241,7 +258,7 @@ static void sercon_lazy_backspace(void) static void sercon_lazy_cr(void) { - cursor_pos_set(cursor_pos_row(), 0); + sercon_cursor_pos_set(cursor_pos_row(), 0); } static void sercon_lazy_lf(void) @@ -256,7 +273,7 @@ static void sercon_lazy_lf(void) SET_LOW(sercon_row_last, GET_LOW(sercon_row_last) - 1); } } - cursor_pos_set(row, cursor_pos_col()); + sercon_cursor_pos_set(row, cursor_pos_col()); } static void sercon_lazy_move_cursor(void) @@ -268,7 +285,7 @@ static void sercon_lazy_move_cursor(void) sercon_lazy_cr(); sercon_lazy_lf(); } else { - cursor_pos_set(cursor_pos_row(), col); + sercon_cursor_pos_set(cursor_pos_row(), col); } } @@ -293,23 +310,27 @@ static void sercon_1000(struct bregs *regs) u8 mode = regs->al & 0x7f; u8 rows, cols; - switch (mode) { - case 0x03: - default: - cols = 80; - rows = 25; - regs->al = 0x30; + if (!sercon_splitmode()) { + switch (mode) { + case 0x03: + default: + cols = 80; + rows = 25; + regs->al = 0x30; + } + cursor_pos_set(0, 0); + SET_BDA(video_mode, mode); + SET_BDA(video_cols, cols); + SET_BDA(video_rows, rows-1); + SET_BDA(cursor_type, 0x0007); + } else { + /* let vgabios handle mode init */; } + SET_LOW(sercon_col_last, 0); SET_LOW(sercon_row_last, 0); SET_LOW(sercon_attr_last, 0); - cursor_pos_set(0, 0); - SET_BDA(video_mode, mode); - SET_BDA(video_cols, cols); - SET_BDA(video_rows, rows-1); - SET_BDA(cursor_type, 0x0007); - sercon_term_reset(); sercon_term_no_linewrap(); if (clearscreen) @@ -320,24 +341,24 @@ static void sercon_1000(struct bregs *regs) static void sercon_1001(struct bregs *regs) { /* show/hide cursor? */ - SET_BDA(cursor_type, regs->cx); + if (!sercon_splitmode()) + SET_BDA(cursor_type, regs->cx); } /* Set cursor position */ static void sercon_1002(struct bregs *regs) { - u8 row = regs->dh; - u8 col = regs->dl; - - cursor_pos_set(row, col); + sercon_cursor_pos_set(regs->dh, regs->dl); } /* Get cursor position */ static void sercon_1003(struct bregs *regs) { - regs->cx = GET_BDA(cursor_type); - regs->dh = cursor_pos_row(); - regs->dl = cursor_pos_col(); + if (!sercon_splitmode()) { + regs->cx = GET_BDA(cursor_type); + regs->dh = cursor_pos_row(); + regs->dl = cursor_pos_col(); + } } /* Scroll up window */ @@ -362,8 +383,10 @@ static void sercon_1006(struct bregs *regs) /* Read character and attribute at cursor position */ static void sercon_1008(struct bregs *regs) { - regs->ah = 0x07; - regs->bh = ' '; + if (!sercon_splitmode()) { + regs->ah = 0x07; + regs->bh = ' '; + } } /* Write character and attribute at cursor position */ @@ -420,14 +443,18 @@ static void sercon_100e(struct bregs *regs) /* Get current video mode */ static void sercon_100f(struct bregs *regs) { - regs->al = GET_BDA(video_mode); - regs->ah = GET_BDA(video_cols); + if (!sercon_splitmode()) { + regs->al = GET_BDA(video_mode); + regs->ah = GET_BDA(video_cols); + } } /* VBE 2.0 */ static void sercon_104f(struct bregs *regs) { - regs->ax = 0x0100; + if (!sercon_splitmode()) { + regs->ax = 0x0100; + } } static void sercon_10XX(struct bregs *regs) @@ -458,8 +485,22 @@ sercon_10(struct bregs *regs) void sercon_enable(void) { + struct segoff_s seabios, vgabios; u16 addr = PORT_SERIAL1; + vgabios = GET_IVT(0x10); + seabios = FUNC16(entry_10); + if (vgabios.seg != seabios.seg || + vgabios.offset != seabios.offset) { + dprintf(1, "%s:%d: using splitmode (vgabios %04x:%04x, hook %04x:%04x)\n", + __func__, __LINE__, + vgabios.seg, vgabios.offset, + seabios.seg, seabios.offset); + sercon_int10_hook_resume = vgabios; + SET_IVT(0x10, FUNC16(entry_10_hooked)); + SET_LOW(sercon_split, 1); + } + SET_LOW(sercon_port, addr); outb(0x03, addr + SEROFF_LCR); // 8N1 outb(0x01, addr + 0x02); // enable fifo
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/optionroms.c | 2 ++ src/romlayout.S | 39 ++++++++++++++++++++++ src/sercon.c | 99 +++++++++++++++++++++++++++++++++++++++----------------- 3 files changed, 111 insertions(+), 29 deletions(-)