March 2013 - SeaBIOS - mail.coreboot.org

Re: [SeaBIOS] SeaVGABIOS crash on FC13 X11
by Kevin O'Connor March 10, 2013

March 10, 2013

On Sun, Mar 10, 2013 at 02:42:09AM +0000, Julian Pidancet wrote: > Pleasure :) Tell me if you find anything. Sigh. It's another x86emu bug. It was fixed in Xorg server with commit bb18f277 (x86emu: Fix more mis-decoding of the data prefix). Basically, "calll" isn't supported. The patch below (which is not fully correct, but demonstrates the problem) fixes SeaVGABIOS bochsvga on fc13 and fc14. fc11 and fc12 are still crashing - not sure if it's something different though. Ughh. -Kevin diff --git a/src/entryfuncs.S b/src/entryfuncs.S index ea6f990..c37fec1 100644 --- a/src/entryfuncs.S +++ b/src/entryfuncs.S @@ -93,7 +93,8 @@ movl %esp, %ebx // Backup %esp, then zero high bits movzwl %sp, %esp movl %esp, %eax // First arg is pointer to struct bregs - calll \cfunc + pushw %ax + callw \cfunc movl %ebx, %esp // Restore %esp (including high bits) POPBREGS .endm diff --git a/tools/vgafixup.py b/tools/vgafixup.py index 52fb934..2493f35 100644 --- a/tools/vgafixup.py +++ b/tools/vgafixup.py @@ -28,6 +28,8 @@ def main(): out.append('retw $2\n') elif sline == 'leave': out.append('movl %ebp, %esp ; popl %ebp\n') + elif sline.startswith('call'): + out.append('pushw %ax ; callw' + sline[4:] + '\n') else: out.append(line) infile.close()

1 1

Re: [SeaBIOS] SeaVGABIOS crash on FC13 X11
by Kevin O'Connor March 10, 2013

March 10, 2013

On Sun, Mar 10, 2013 at 02:04:02AM +0000, Julian Pidancet wrote: > On Sun, Mar 10, 2013 at 1:15 AM, Kevin O'Connor <kevin(a)koconnor.net> wrote: > > On Sun, Mar 10, 2013 at 01:09:35AM +0000, Julian Pidancet wrote: > >> On Sun, Mar 10, 2013 at 12:09 AM, Kevin O'Connor <kevin(a)koconnor.net> wrote: > > > > Yeah - I thought the same thing and looked into that. The assembler > > translation is still being performed and I don't see anything that > > looks suspicious. > > > > I managed to get set up with qemu and SeaBIOS and I'm seeing > interesting results: > First of all, I'm not able to access the Xorg logs, because it looks > like the VM crashes completely. (Maybe I need to ask Fedora to boot in > text mode ?) I've been able to get to the logs by pressing control-alt-2, then typing "sendkey ctrl-alt-f2" in the qemu console, and then hitting control-alt-1 to go back to the session (now on the second console). > It is printing the same sequence of bytes over and over again: > > 0002d160 30 30 33 0d 0a ff 55 aa 48 e9 ed 4e 93 55 aa 48 |003...U.H..N.U.H| > 0002d170 ec ed 4e 93 55 aa 48 e9 ed 4e 93 55 aa 48 e9 ed |..N.U.H..N.U.H..| > 0002d180 4e 93 55 aa 48 e9 ed 4e 93 55 aa 48 e9 ed 4e 93 |N.U.H..N.U.H..N.| > 0002d190 55 aa 48 e9 ed 4e 93 55 aa 48 e9 ed 4e 93 55 aa |U.H..N.U.H..N.U.| > 0002d1a0 48 e9 ed 4e 93 55 aa 48 e9 ed 4e 93 55 aa 48 e9 |H..N.U.H..N.U.H.| I'm not seeing this right now, but I think I have seen it in previous tests. I think subtle changes in the binary layout may make this junk come out/not come out. Thanks again for looking at this. -Kevin

2 1

SeaVGABIOS crash on FC13 X11
by Kevin O'Connor March 10, 2013

March 10, 2013

Hi, I'm seeing an X11 crash when using the "bochs" style vga interface of SeaVGABIOS on a Fedora 13 guest. The log (see attached) shows the X server segfaulting. The crash is fully reproducible. I don't see the problem with the "lgpl vgabios". I tried various versions of qemu and SeaVGABIOS, and they all show the same crash. So, this doesn't look like a regression within SeaVGABIOS (though obviously it's a regression from the "lgpl vgabios"). It's easy to reproduce the crash - I use an installer image I have handy (Fedora-13-x86_64-DVD.iso) and run: qemu-system-x86_64 -cdrom Fedora-13-x86_64-DVD.iso -vga std -m 512 --enable-kvm The installer will fail to start the X server and attempt the rest of the install in text mode. Julian - I know you played with SeaVGABIOS and X11 a bit a year or so ago. Any thoughts on what is happening? -Kevin

2 2

Re: [SeaBIOS] SeaVGABIOS crash on FC13 X11
by Kevin O'Connor March 10, 2013

March 10, 2013

On Sun, Mar 10, 2013 at 01:09:35AM +0000, Julian Pidancet wrote: > On Sun, Mar 10, 2013 at 12:09 AM, Kevin O'Connor <kevin(a)koconnor.net> wrote: > > > > Julian - I know you played with SeaVGABIOS and X11 a bit a year or so > > ago. Any thoughts on what is happening? > > > > Hi Kevin, > > I have not really followed the latest developments on SeaBIOS, so I'm > not sure I'll be very useful. But I can try to take a look at it. > I seem to remember that qemu uses SeaVGABIOS now. If I try to compile > a recent qemu and launch the fedora liveCD, will it exhibit the issue > ? Not much has changed in the SeaVGABIOS area. I believe QEMU can build SeaVGABIOS, but it is not the default vgabios. > The last time I investigated on an issue with SeaVGABIOS and X11, it > was because the 16bit code emulator of X11 wasn't handling properly > certain prefixed instructions. I think we worked around the issue by > post-processing the assembly output of the compilation to replace the > problematic instructions with non-prefixed instructions. > > I also tried to send several times a patch on the Xorg mailing list to > address that issue, but never managed to attract anyone's attention. > It could be useful if someone volunteered to try sending them again. > > According to the backtrace you sent, the crash seems to be located in > the libint10 module. The issue I worked on was in "x86emu". I'm not > sure how these two parts relate to each other, but we could well be > facing something very similar. > > The first think I would try, is to check in the vga bios assembly and > make sure we're correctly replacing all of the "sensitive" prefixed > x86 instructions. Some new form of one of these instruction may have > made it's way in the VGA rom code. Yeah - I thought the same thing and looked into that. The assembler translation is still being performed and I don't see anything that looks suspicious. Thanks, -Kevin

2 1

[PATCH] Fix bug in NUMA node setup - don't create SRAT if NUMA not present.
by Kevin O'Connor March 10, 2013

March 10, 2013

Make sure to check for the case where there are no NUMA nodes passed in from QEMU. Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net> --- src/acpi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/acpi.c b/src/acpi.c index 98a5d40..119d1c1 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -655,6 +655,8 @@ build_srat(void) return NULL; int max_cpu = romfile_loadint("etc/max-cpus", 0); int nb_numa_nodes = (filesize / sizeof(u64)) - max_cpu; + if (!nb_numa_nodes) + return NULL; struct system_resource_affinity_table *srat; int srat_size = sizeof(*srat) + -- 1.7.11.7

1 0

[PATCH] vgabios: Fix cirrus memory clear on mode switch.
by Kevin O'Connor March 9, 2013

March 9, 2013

The cirrus_clear_vram() code wasn't actually doing anything because of a u8 overflow. Fix that. Fill with 0xff when performing a legacy cirrus mode switch (WinXP has been observed to incorrectly render dialog boxes if the memory is filled to 0). This was the behavior of the original LGPL vgabios code. To support this, add mechanism (MF_LEGACY) to allow vga drivers to detect if the mode switch is from vesa or int10. Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net> --- vgasrc/clext.c | 11 ++++++----- vgasrc/vgabios.c | 2 +- vgasrc/vgabios.h | 1 + 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/vgasrc/clext.c b/vgasrc/clext.c index d02b880..8eb226a 100644 --- a/vgasrc/clext.c +++ b/vgasrc/clext.c @@ -443,14 +443,14 @@ cirrus_enable_16k_granularity(void) } static void -cirrus_clear_vram(void) +cirrus_clear_vram(u16 fill) { cirrus_enable_16k_granularity(); - u8 count = GET_GLOBAL(VBE_total_memory) / (16 * 1024); - u8 i; + int count = GET_GLOBAL(VBE_total_memory) / (16 * 1024); + int i; for (i=0; i<count; i++) { stdvga_grdc_write(0x09, i); - memset16_far(SEG_GRAPH, 0, 0, 16 * 1024); + memset16_far(SEG_GRAPH, 0, fill, 16 * 1024); } stdvga_grdc_write(0x09, 0x00); } @@ -469,7 +469,8 @@ clext_set_mode(struct vgamode_s *vmode_g, int flags) if (!(flags & MF_LINEARFB)) cirrus_enable_16k_granularity(); if (!(flags & MF_NOCLEARMEM)) - cirrus_clear_vram(); + // fill with 0xff to keep win 2K happy + cirrus_clear_vram(flags & MF_LEGACY ? 0xffff : 0x0000); return 0; } diff --git a/vgasrc/vgabios.c b/vgasrc/vgabios.c index 6abe639..987a259 100644 --- a/vgasrc/vgabios.c +++ b/vgasrc/vgabios.c @@ -420,7 +420,7 @@ handle_1000(struct bregs *regs) else regs->al = 0x30; - int flags = GET_BDA(modeset_ctl) & (MF_NOPALETTE|MF_GRAYSUM); + int flags = MF_LEGACY | (GET_BDA(modeset_ctl) & (MF_NOPALETTE|MF_GRAYSUM)); if (regs->al & 0x80) flags |= MF_NOCLEARMEM; diff --git a/vgasrc/vgabios.h b/vgasrc/vgabios.h index ff5ec45..5adbf4b 100644 --- a/vgasrc/vgabios.h +++ b/vgasrc/vgabios.h @@ -39,6 +39,7 @@ struct saveBDAstate { }; // Mode flags +#define MF_LEGACY 0x0001 #define MF_GRAYSUM 0x0002 #define MF_NOPALETTE 0x0008 #define MF_CUSTOMCRTC 0x0800 -- 1.7.11.7

1 0

[PATCH] vgabios: Fix stdvga_perform_gray_scale_summing().
by Kevin O'Connor March 9, 2013

March 9, 2013

Fix conversion error that resulted in stdvga_perform_gray_scale_summing not actually writing the new results back. Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net> --- vgasrc/stdvga.c | 1 + 1 file changed, 1 insertion(+) diff --git a/vgasrc/stdvga.c b/vgasrc/stdvga.c index a29d6d4..1dd947e 100644 --- a/vgasrc/stdvga.c +++ b/vgasrc/stdvga.c @@ -123,6 +123,7 @@ stdvga_perform_gray_scale_summing(u16 start, u16 count) u16 intensity = ((77 * rgb[0] + 151 * rgb[1] + 28 * rgb[2]) + 0x80) >> 8; if (intensity > 0x3f) intensity = 0x3f; + rgb[0] = rgb[1] = rgb[2] = intensity; stdvga_dac_write(GET_SEG(SS), rgb, i, 1); } -- 1.7.11.7

1 0

[PATCH 0/4] Build checks for "tmp" memory access after POST; resume fixes.
by Kevin O'Connor March 9, 2013

March 9, 2013

Memory allocated with malloc_tmp() (and its variants) is only valid during the POST phase. It can be tricky to find all users of "tmp" memory and failures from getting this wrong can be subtle. So, this series adds a mechanism to try and catch these cases. The idea is to flag global variables that point to "tmp" memory as only being reachable from "init" code. The build can then verify it. No change of this nature would be complete without uncovering existing errors. The S3 resume code was accessing invalid memory when it called the shadow ram functions and the smm init code. The first two patches fix this up. The remaining two patches add the build time check. -Kevin Kevin O'Connor (4): shadow: Don't use PCIDevices list in make_bios_readonly(). smm: Don't use PCIDevices list in smm_setup(). Add VARVERIFY32INIT attribute for variables only available during "init". Use VARVERIFY32INIT on global variables that point to "tmp" memory. src/boot.c | 4 ++-- src/paravirt.c | 1 + src/pci.c | 2 +- src/pmm.c | 6 +++-- src/romfile.c | 2 +- src/shadow.c | 34 +++++++++++----------------- src/smm.c | 66 +++++++++++++++++++++++++++++------------------------- src/types.h | 4 ++++ src/util.h | 1 + tools/layoutrom.py | 18 +++++++-------- 10 files changed, 71 insertions(+), 67 deletions(-) -- 1.7.11.7

1 4

[PATCH] Don't use __FILE__ in virtio-ring.c.
by Kevin O'Connor March 9, 2013

March 9, 2013

Avoid referencing __FILE__ - that value changes depending on the user's build setting of OUT. The function and line number are sufficient. Also, use panic() instead of looping forever. Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net> --- src/virtio-ring.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/virtio-ring.c b/src/virtio-ring.c index 0883330..9d46c41 100644 --- a/src/virtio-ring.c +++ b/src/virtio-ring.c @@ -21,10 +21,8 @@ #include "biosvar.h" // GET_GLOBAL #include "util.h" // dprintf -#define BUG() do { \ - dprintf(1, "BUG: failure at %s:%d/%s()!\n", \ - __FILE__, __LINE__, __func__); \ - while(1); \ +#define BUG() do { \ + panic("BUG: failure at %d/%s()!\n", __LINE__, __func__); \ } while (0) #define BUG_ON(condition) do { if (condition) BUG(); } while (0) -- 1.7.11.7

1 0

[PATCH] Add additional dependency checks to Makefile.
by Kevin O'Connor March 9, 2013

March 9, 2013

Generate dependencies on pre-processed ASL files. This ensures that a change to an imported dsdt file will cause iasl to be called. Make .config depend on Kconfig files. Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net> --- Makefile | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index ada8fa5..7ca8036 100644 --- a/Makefile +++ b/Makefile @@ -25,6 +25,8 @@ SRC32SEG=util.c output.c pci.c pcibios.c apm.c stacks.c cc-option=$(shell if test -z "`$(1) $(2) -S -o /dev/null -xc /dev/null 2>&1`" \ ; then echo "$(2)"; else echo "$(3)"; fi ;) +CPPFLAGS = -P -MD -MT $@ + COMMONCFLAGS := -I$(OUT) -Os -MD -g \ -Wall -Wno-strict-aliasing -Wold-style-definition \ $(call cc-option,$(CC),-Wtype-limits,) \ @@ -63,6 +65,7 @@ OBJCOPY=objcopy OBJDUMP=objdump STRIP=strip PYTHON=python +CPP=cpp IASL:=iasl # Default targets @@ -115,7 +118,7 @@ $(OUT)%.o: %.c $(OUT)autoconf.h $(OUT)%.lds: %.lds.S @echo " Precompiling $@" - $(Q)$(CPP) -P -D__ASSEMBLY__ $< -o $@ + $(Q)$(CPP) $(CPPFLAGS) -D__ASSEMBLY__ $< -o $@ ################ Main BIOS build rules @@ -214,7 +217,7 @@ iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \ $(OUT)%.hex: src/%.dsl ./tools/acpi_extract_preprocess.py ./tools/acpi_extract.py @echo " Compiling IASL $@" - $(Q)cpp -P $< > $(OUT)$*.dsl.i.orig + $(Q)$(CPP) $(CPPFLAGS) $< -o $(OUT)$*.dsl.i.orig $(Q)$(PYTHON) ./tools/acpi_extract_preprocess.py $(OUT)$*.dsl.i.orig > $(OUT)$*.dsl.i $(Q)$(IASL) $(call iasl-option,$(IASL),-Pn,) -vs -l -tc -p $(OUT)$* $(OUT)$*.dsl.i $(Q)$(PYTHON) ./tools/acpi_extract.py $(OUT)$*.lst > $(OUT)$*.off @@ -231,7 +234,7 @@ $(Q)$(MAKE) -C $(OUT) -f $(CURDIR)/tools/kconfig/Makefile srctree=$(CURDIR) src= endef $(OUT)autoconf.h : $(KCONFIG_CONFIG) ; $(call do-kconfig, silentoldconfig) -$(KCONFIG_CONFIG): ; $(call do-kconfig, defconfig) +$(KCONFIG_CONFIG): src/Kconfig vgasrc/Kconfig ; $(call do-kconfig, defconfig) %onfig: ; $(call do-kconfig, $@) help: ; $(call do-kconfig, $@) -- 1.7.11.7

1 0