Re: [Qemu-devel] Re: [PATCH] Fix freezing bug in curses console
2009/2/27 Anthony Liguori : > Matthew Bloch wrote: >> >> Hi there, >> >> We are running lots of kvm processes in screen and found that about 1 in >> 5 froze shortly after startup startup with a backtrace like this one: >> >> #0 0xf7c7fcd9 in pthread_exit () from /lib/tls/libc.so.6 >> #1 0xf7cfbe62 in wresize () from /lib/libncurses.so.5 >> #2 0xf7cfb7ab in is_term_resized () from /lib/libncurses.so.5 >> #3 0xf7cfb877 in is_term_resized () from /lib/libncurses.so.5 >> #4 0xf7cfba31 in resize_term () from /lib/libncurses.so.5 >> #5 0x080d3dd9 in vga_init () >> #6 >> #7 0xf7c0da5b in free () from /lib/tls/libc.so.6 >> #8 0xf7c0effe in calloc () from /lib/tls/libc.so.6 >> #9 0xf7cf222e in newpad () from /lib/libncurses.so.5 >> #10 0x080d3549 in vga_init () >> >> We're just using the lenny version of kvm from 2008-12-16. >> >> On casual inspection, the SIGWINCH signal handling looked ropey to me - >> grandpa always told me not to do any real work in a signal handler, and >> the backtrace suggested re-entrancy problems in curses, so I changed the >> behaviour to set a flag and do the work in the main loop instead. Maybe >> I'm reading the backtrace wrong. >> >> So far that means that when you resize the window, the display is >> corrupt until the VM outputs some text, or the user hits a key. But I >> think it has solved the freezing / crashing bug too - would appreciate >> any comments on my analysis or proposed solution. >> > > It's racy with select(). A better fix would be to create a pipe and write > to that pipe in the SIGWINCH handler. You should then register an io > callback using qemu_set_fd_handler2() that does the actions for SIGWINCH. Maybe a bottom half would work? The scheduling of a bh shouldn't constitute "real work". Cheers -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] Re: [PATCH 1/5] re-register whole area upon lfb unmap.
2008/12/17 Anthony Liguori : > Glauber Costa wrote: >> >> set phys_offset correctly for the whole vga area when unmapping linear >> vram >> (for vga optimization). We first register the old pieces as unassigned >> memory, to make things easier for kvm (and possibly other slot based >> implementations in the future). Replacing the region directly would >> make the slot management significantly more complex. >> > > This change worries me because it involves explicitly unassigning slots and > then assigning a new, bigger slot. This is not necessary for TCG. It > suggests to me that there's a bug in the kvm slot code and that we're > changing QEMU to work around it. > > That will means there may be other places in the code that are completely > valid, but exercise this bug. > > Or is this purely an optimization? It also changes the semantics because IO callbacks are now passed offsets from regions starts instead of absolute addresses. I'm not able to tell if the change is for good or for bad though. Cheers -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH][RFC] Split non-TCG bits out of exec.c
2008/11/14 Anthony Liguori <[EMAIL PROTECTED]>: > The issue is not disabling TCG at runtime. That's easy enough. The issue > is that TCG doesn't exist (and probably won't ever exist) for certain > architectures like ia64 and s390. Being forced to build with TCG support > makes having QEMU + KVM not possible on these platforms even though they > both support KVM. I mean either compile-time or run-time: assuming that each QEMUAccel implementation is a bunch of files + a struct with pointers in the common code, it should make turning on/off each emulator easy. Cheers -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH][RFC] Split non-TCG bits out of exec.c
2008/11/13 Anthony Liguori <[EMAIL PROTECTED]>: > andrzej zaborowski wrote: >> Is this going a bit in the opposite direction to where QEMUAccel is >> going? What Fabrice suggests seems to be like QEMUAccel, with TCG >> treated as another accelerator. >> > > QEMUAccel is a bit orthogonal to what I'm talking about. There is already > KVM support in QEMU today and I'm merely looking to restructure existing > code so that I can build a version of QEMU that has no TCG support, only KVM > support. TCG is too intimately woven into QEMU right now. You could think > of this perhaps as a precursor to making TCG more of an "accelerator" than > it is today. Ah, I agree with your patch, I was only commenting on the idea of *-kvm/ targets. I see something like QEMUAccel as a way to turn on and off the cpu emulators (TCG, kvm, kqemu). Ofcourse, kqemu depends on a fallback emulator - currently TCG - I guess it could be possible to run kqemu with kvm as the fall back and not compile in TCG (even if not a very useful configuration). > > But wrt QEMUAccel and KVM, there are 5 places in QEMU where there is KVM > specific code. > > One is cpu-exec.c to invoke the kvm exec routine instead of TCG. kqemu has > something similar. Unfortunately, kqemu relies on some state that's only > available in cpu-exec.c so we can't make this a single function pointer > invocation without major surgery on cpu-exec. > > One is vl.c to initialize KVM support. kqemu doesn't need this. > > One is exec.c, to hook cpu_register_physical_memory. kqemu does this too so > it could conceivably be a hook. > > Another one is monitor.c to implement 'info kvm'. Not really a place for a > hook. Ideally we could register the monitor callback from kvm-all.c when we > initialize KVM. > > Finally, there is a hook in hw/acpi.c to disable SMM support when using KVM. > This is KVM specific because KVM doesn't support SMM. kqemu uses TCG to > run SMM code. > > Since there is only one shared hook ATM, I don't think something like > QEMUAccel is all that useful for KVM. On the other hand, there are 42 > places that are kqemu specific. I think kqemu could be refactored to > eliminate most of these. > > kqemu relies on TCG so you can't really decouple them from each other. > >> BTW It would be great if before merging a change like this you >> review/merge the patches submitted to the list that might touch the >> same area so as not to break them (such as Jan Kiszka's >> single-stepping/watchpoint fixes). >> > > Yeah, I will make sure to. Many thanks for that. Regards -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH][RFC] Split non-TCG bits out of exec.c
2008/11/12 Anthony Liguori <[EMAIL PROTECTED]>: > Unlike kqemu, KVM does not use TCG at all when accelerating QEMU. Having TCG > present is not a problem when using KVM on x86. x86 already has TCG host and > target support and it's quite convenient to be able to disable/enable KVM and > compare it to TCG when debugging. > > KVM also supports architectures that do not have TCG host and target support > such as ia64, s390, and PPC[1]. For these architectures, TCG is an inhibitor > for upstream inclusion. > > TCG is pretty well isolated in QEMU so building these targets without TCG > should be easy enough. This breaks down in exec.c though. There is a lot of > TCG specific code in exec.c, but also a lot of code that KVM needs. > > This patch moves the non-TCG specific bits of exec.c into a separate file, > exec-all.c. This makes it relatively easy to build QEMU without TCG support. > More patches will come to complete this work but the exec.c bits are probably > 95% of what is needed. > > The remaining bits are some general cleanups where layering has been violated > and the introduction of a new -kvm subtarget, similar to -softmmu or > -linux-user. Is this going a bit in the opposite direction to where QEMUAccel is going? What Fabrice suggests seems to be like QEMUAccel, with TCG treated as another accelerator. BTW It would be great if before merging a change like this you review/merge the patches submitted to the list that might touch the same area so as not to break them (such as Jan Kiszka's single-stepping/watchpoint fixes). Cheers -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Wrapper around dpy_copy to fix segfault with -vnc option
2008/9/23 Jan Niehusmann <[EMAIL PROTECTED]>: > On Mon, Sep 22, 2008 at 11:15:04PM +0200, andrzej zaborowski wrote: >> Yes, I don't think hw/ code should be concerned with what console is >> active. Logically the dpy_ functions should take the pointer returned >> from graphic_console_init() as first parameter. >> >> Please also check the code is formatted consistently with qemu. >> >> I didn't receive Jan's message but the check seems to not be enough >> because there can be multiple graphical consoles with different sizes >> - if I'm guessing correctly what this patch tries to fix. > > Based on these comments I had another look at the code. If there can > be multiple graphical consoles, the only sensible test is 'console > == active_console' where console must be provided by the caller. So, > indeed, a pointer to the console must be provided instead of a pointer > to the DisplayState. > > To make function names consistent, I called the function qemu_console_copy > in analogy to qemu_console_resize (which is a similar wrapper around > dpy_resize). I committed the patch slightly modified because I found this still doesn't account for all the cases. Imagine ds->dpy_copy is not set, the call does nothing and the screen is not updated until fully invalidated. We need to either implement a generic dpy_copy in console.c or have a fallback in hw/cirrus_vga.c depending on which is faster. Also note that till now hw/ files could only call into the ds->dpy_ functions from inside their own vga_hw_update callback, this guaranteed some consistency. The use of dpy_copy inside cirrus_vga.c changed this which is the source of these bugs. I hadn't noticed when this happened. Regards -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] Re: [Patch] Segfault with -vnc option
2008/9/22 Anthony Liguori <[EMAIL PROTECTED]>: > Jan Niehusmann wrote: >> >> Hi! >> > > Hi Jan, > > Very good catch. My only suggestion would be to move this check into > cirrus_vga.c and vmware_vga.c. Even better would be to introduce a wrapper > around callers of dpy_copy. Yes, I don't think hw/ code should be concerned with what console is active. Logically the dpy_ functions should take the pointer returned from graphic_console_init() as first parameter. Please also check the code is formatted consistently with qemu. I didn't receive Jan's message but the check seems to not be enough because there can be multiple graphical consoles with different sizes - if I'm guessing correctly what this patch tries to fix. Cheers -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 1/1][RESEND] v2: Fix text console size/resize when using curses
2008/9/15 Ryan Harper <[EMAIL PROTECTED]>: > * andrzej zaborowski <[EMAIL PROTECTED]> [2008-09-15 11:51]: >> Hi, > > Hello, > >> >> 2008/9/4 Ryan Harper <[EMAIL PROTECTED]>: >> > v2: drop initial size adjustment, not needed. >> > >> > Resize events fail to ensure that both the text console and curses display >> > areas >> > are the same size; this causes broken output like: >> >> It seems that this was broken by r4812. Your patch looks correct but > > Thanks for taking a look, > >> I think it will prevent things like -serial vc:80Cx24C from working as >> documented. There's a conflict here: either virtual consoles have >> commandline-set fixed size or they adjust to the window size like >> before r4812 and don't have parts that don't fit on the screen. The >> SDL window is not resizable but the curses window is always resizable. >> I propose to add TEXT_CONSOLE_FIXED_SIZE back, it was removed by >> r4812. I will do this unless there are better ideas. > > I'll test the -serial to see if that breaks with the current patch.. > I'm also looking at an alternative. I think that in curses.c if on > window change event we can send a qemu_console_resize() to the text > consoles with the new width,height, that does the work of getting the > width/height in sync. I'll send an update if I get that working. The TEXT_CONSOLE_FIXED_SIZE approach worked ok, too, and it's generic (i.e. if in sdl.c you make the SDL window resizable then it'll start working for SDL automatically). > >> >> Another idea would be to set the default console size to 640x400 and >> agree to not support terminals < 80x24. > > I'm not sure that would fix it completely, I'm pretty sure the output > still looks broken even if your terminal is bigger than the min size > required. Right, at the moment it is broken because curses.c assumes that text consoles automatically resized to ds->width,ds->height and they didn't, they keep the size given through vc:XxY or 800x600 if none was given. Regards -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 1/1][RESEND] v2: Fix text console size/resize when using curses
Hi, 2008/9/4 Ryan Harper <[EMAIL PROTECTED]>: > v2: drop initial size adjustment, not needed. > > Resize events fail to ensure that both the text console and curses display > areas > are the same size; this causes broken output like: It seems that this was broken by r4812. Your patch looks correct but I think it will prevent things like -serial vc:80Cx24C from working as documented. There's a conflict here: either virtual consoles have commandline-set fixed size or they adjust to the window size like before r4812 and don't have parts that don't fit on the screen. The SDL window is not resizable but the curses window is always resizable. I propose to add TEXT_CONSOLE_FIXED_SIZE back, it was removed by r4812. I will do this unless there are better ideas. Another idea would be to set the default console size to 640x400 and agree to not support terminals < 80x24. Regards -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 1/2] Fix text console size/resize when using curses
2008/8/22 Ryan Harper <[EMAIL PROTECTED]>: > The current console output is broken in curses mode size the default starting > size might not match the launching terminal size. Change initial size to be > based on the terminal. Also, resize events fail to ensure that both the text > console and curses display areas are the same size; this causes broken output > like: > > QEMU 0.9.1 monitor - type 'help' for more information >(qemu) >(qemu) >(qemu) > > To fix this, ensure that the display width and the text area width are sync > for > text consoles on refresh; also force a resize event whenever we invalidate the > text console. > > Signed-off-by: Ryan Harper <[EMAIL PROTECTED]> > > diff --git a/console.c b/console.c > index 1c94980..89bdc52 100644 > --- a/console.c > +++ b/console.c > @@ -608,6 +608,9 @@ static void console_refresh(TextConsole *s) > s->text_y[0] = 0; > s->text_x[1] = s->width - 1; > s->text_y[1] = s->height - 1; > +/* ensure that textconsole area is the same size as the display */ > +s->g_width = s->ds->width; > +s->g_height = s->ds->height; > s->cursor_invalidate = 1; > return; > } > @@ -1158,6 +1161,8 @@ static void text_console_invalidate(void *opaque) > TextConsole *s = (TextConsole *) opaque; > > console_refresh(s); > +/* resize if needed */ > +text_console_resize(s); > } > > static void text_console_update(void *opaque, console_ch_t *chardata) > diff --git a/curses.c b/curses.c > index d09eff2..a8972f4 100644 > --- a/curses.c > +++ b/curses.c > @@ -362,8 +362,8 @@ void curses_display_init(DisplayState *ds, int > full_screen) > ds->data = (void *) screen; > ds->linesize = 0; > ds->depth = 0; > -ds->width = 640; > -ds->height = 400; > +ds->width = COLS * FONT_WIDTH; > +ds->height = LINES * FONT_HEIGHT; > ds->dpy_update = curses_update; > ds->dpy_resize = curses_resize; > ds->dpy_refresh = curses_refresh; > @@ -371,6 +371,6 @@ void curses_display_init(DisplayState *ds, int > full_screen) > > invalidate = 1; > > -/* Standard VGA initial text mode dimensions */ > -curses_resize(ds, 80, 25); > +/* setup up initial display size based on terminal settings */ > +curses_resize(ds, COLS, LINES); These two changes in curses.c don't look ok. ds->width/height and the curses_resize() parameters are not supposed to be the host terminal size, they're just the default settings for the virtual display size (same way as sdl.c sets 640x400 at start regardless of the host, and likely the bios will force the resize anyway). The host terminal dimensions are later taken into account in curses_calc_pad to ensure correct display. I can't comment on the console.c changes at the moment. Cheers -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH]: Fix i2c_bus_save, which fixes KVM live migration
2008/7/29 Paul Brook <[EMAIL PROTECTED]>: > On Tuesday 29 July 2008, andrzej zaborowski wrote: >> 2008/7/29 Chris Lalancette <[EMAIL PROTECTED]>: >> > Attached is a simple patch to make i2c_bus_save() put a 32-bit quantity >> > in the save file, which matches what i2c_bus_load() expects to pull out >> > of the save file later. Without this fix in place, KVM live migration >> > fails since the sender is only sending 1 byte while the receiver is >> > waiting to receive 4 bytes. >> >> Thanks, I committed this modified to use byte width afterall since the >> addresses are 7-bit quantities. > > No they aren't. I2C also supports 10-bit addressing. Addresses in qemu are still <= 8-bit, for example i2c_slave_load() assumes this and current i2c api users do. Regards -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH]: Fix i2c_bus_save, which fixes KVM live migration
2008/7/29 Chris Lalancette <[EMAIL PROTECTED]>: > Attached is a simple patch to make i2c_bus_save() put a 32-bit quantity in the > save file, which matches what i2c_bus_load() expects to pull out of the save > file later. Without this fix in place, KVM live migration fails since the > sender is only sending 1 byte while the receiver is waiting to receive 4 > bytes. Thanks, I committed this modified to use byte width afterall since the addresses are 7-bit quantities, as noted by Anthony. Means that existing snapshots are not invalidated. Cheers -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
