Date: Sat, 10 May 2008 00:09:22 +0200 From: Juergen Lock <nox@jelal.kn-bremen.de> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-emulation@freebsd.org Subject: Re: seems I finally found what upset kqemu on amd64 SMP... shared gdt! (please test patch :) Message-ID: <20080509220922.GA13480@saturn.kn-bremen.de> In-Reply-To: <20080508195843.G17500@delplex.bde.org> References: <20080507162713.73A3A5B47@mail.bitblocks.com> <20080508195843.G17500@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, May 08, 2008 at 09:59:57PM +1000, Bruce Evans wrote: > On Wed, 7 May 2008, Bakul Shah wrote: > >> On Wed, 07 May 2008 15:54:56 +1000 Bruce Evans <brde@optusnet.com.au> >> wrote: >>> On Tue, 6 May 2008, Bakul Shah wrote: >>>> See for instance >>>> >>>> http://docs.freebsd.org/cgi/getmsg.cgi?fetch=100953+0+archive/2007/freebsd- >>> emulation/20070415.freebsd-emulation >>>> >>>> This seems to have not caused any problem in practice. And >>>> any way taking out the message doesn't change the essential >>>> behavior (the invariant is still broken) but it can speed up >>>> your emulation considerably. >>> >>> I should have changed it to a panic long ago. That would give the >>> correct >>> number of messages (1) :-). >> >> Too late now for you to go fundamentalist :-) >> >>> i386 still doesn't even print a message (perhaps it never did). The >>> bug would probably never have existed in any FreeBSD version of kqemu if >>> i386 had had enough invariant checking. >> >> It does (in isa/npx.c) and I've disabled it! > > Not too late to go fundamentalist for that :-). I wrote it correctly. > It paniced, but it was broken to a printf in rev.1.131 when I wasn't > watching closely enough (though the log claims that I reviewed the > patch, ISTR looking mainly at the parts in machdep.c). > > The message in npx.c is actually about violation of an even more > fundamental invariant -- the invariant that owning the FPU includes > having the TS flag clear so that DNA traps cannot occur. The bug in > kqemu seems to be mismanagement of the TS flag related to this. I > forget if it is the host or the target TS flag that seems to be mismanaged. > For the target, it would take a bug in the virtualization of the TS flag > to break this invariant (assuming no related bugs in the target kernel). > Well the `fpcurthread == curthread' bug has been fixed quite a while ago already, or do you mean another one? > The message in amd64/machdep.c is about violation of the invariant > that the kernel cannot cause DNA traps. Spurious DNA traps in the > target kernel might be caused either by violation of the previous > invariant (then we might get a spurious DNA trap as part of non-broken > target kernel FPU handling, after we have properly acquired ownership > of the FPU). Non-spurious but wrong DNA traps in the host kernel might > be caused by not properly acquiring ownership of the FPU before using > it. This bug would be easier to implement, but I now remember more > of the previous discussion and doubt that it is the bug in kqemu. It > was said that kqemu never uses the FPU directly (not even for > fxsave/fxrstor?) > Okay I _think_ I know a little more about this now... kqemu itself doesn't use the fpu, but the guest code it runs can, and in that case the DNA trap is just used for (host) lazy fpu context switching like as if the code was running in userland regularly. And I just tested the following patch that should get rid of the message by calling fpudna/npxdna directly (files/patch-fpucontext is the interesting part:) Index: Makefile =================================================================== RCS file: /home/pcvs/ports/emulators/kqemu-kmod/Makefile,v retrieving revision 1.23 diff -u -p -r1.23 Makefile --- Makefile 1 May 2008 13:29:16 -0000 1.23 +++ Makefile 9 May 2008 19:53:08 -0000 @@ -7,7 +7,7 @@ PORTNAME= kqemu PORTVERSION= 1.3.0.p11 -PORTREVISION= 4 +PORTREVISION= 5 CATEGORIES= emulators kld MASTER_SITES= http://fabrice.bellard.free.fr/qemu/ \ http://qemu.org/ \ Index: files/patch-tssworkaround =================================================================== RCS file: /home/pcvs/ports/emulators/kqemu-kmod/files/patch-tssworkaround,v retrieving revision 1.1 diff -u -p -r1.1 patch-tssworkaround --- files/patch-tssworkaround 1 May 2008 13:29:16 -0000 1.1 +++ files/patch-tssworkaround 9 May 2008 19:34:12 -0000 @@ -1,8 +1,8 @@ Index: kqemu-freebsd.c -@@ -33,6 +33,11 @@ - - #include <machine/vmparam.h> - #include <machine/stdarg.h> +@@ -38,6 +38,11 @@ + #else + #include <machine/npx.h> + #endif +#ifdef __x86_64__ +#include <sys/pcpu.h> +#include <machine/segments.h> @@ -11,13 +11,13 @@ Index: kqemu-freebsd.c #include "kqemu-kernel.h" -@@ -234,6 +239,19 @@ +@@ -248,6 +253,19 @@ va_end(ap); } +#ifdef __x86_64__ +/* called with interrupts disabled */ -+void CDECL kqemu_tss_workaround(void) ++void CDECL kqemu_tss_fixup(void) +{ + int gsel_tss = GSEL(GPROC0_SEL, SEL_KPL); + @@ -49,7 +49,7 @@ Index: common/kernel.c +#ifdef __FreeBSD__ +#ifdef __x86_64__ + spin_lock(&g->lock); -+ kqemu_tss_workaround(); ++ kqemu_tss_fixup(); + spin_unlock(&g->lock); +#endif +#endif @@ -63,7 +63,7 @@ Index: kqemu-kernel.h +#ifdef __FreeBSD__ +#ifdef __x86_64__ -+void CDECL kqemu_tss_workaround(void); ++void CDECL kqemu_tss_fixup(void); +#endif +#endif + Index: files/patch-fpucontext @@ -0,0 +1,76 @@ +Index: common/kernel.c +@@ -1240,6 +1240,9 @@ + case MON_REQ_EXCEPTION: + exec_exception(s->arg0); + break; ++ case MON_REQ_LOADFPUCONTEXT: ++ kqemu_loadfpucontext(s->arg0); ++ break; + default: + kqemu_log("invalid mon request: %d\n", s->mon_req); + break; +Index: common/kqemu_int.h +@@ -523,6 +523,7 @@ + MON_REQ_LOCK_USER_PAGE, + MON_REQ_UNLOCK_USER_PAGE, + MON_REQ_EXCEPTION, ++ MON_REQ_LOADFPUCONTEXT, + } MonitorRequest; + + #define INTERRUPT_ENTRY_SIZE 16 +Index: common/monitor.c +@@ -1995,8 +1995,13 @@ + raise_exception_err(s, EXCP07_PREX, 0); + } else { + /* the host needs to restore the FPU state for us */ ++#ifndef __FreeBSD__ + s->mon_req = MON_REQ_EXCEPTION; + s->arg0 = 0x07; ++#else ++ s->mon_req = MON_REQ_LOADFPUCONTEXT; ++ s->arg0 = (unsigned long)s->cpu_state.cpl; ++#endif + monitor2kernel1(s); + } + } +Index: kqemu-freebsd.c +@@ -33,6 +33,11 @@ + + #include <machine/vmparam.h> + #include <machine/stdarg.h> ++#ifdef __x86_64__ ++#include <machine/fpu.h> ++#else ++#include <machine/npx.h> ++#endif + + #include "kqemu-kernel.h" + +@@ -172,6 +177,15 @@ + { + } + ++void CDECL kqemu_loadfpucontext(unsigned long cpl) ++{ ++#ifdef __x86_64__ ++ fpudna(); ++#else ++ npxdna(); ++#endif ++} ++ + #if __FreeBSD_version < 500000 + static int + curpriority_cmp(struct proc *p) +Index: kqemu-kernel.h +@@ -40,6 +40,10 @@ + void * CDECL kqemu_io_map(unsigned long page_index, unsigned int size); + void CDECL kqemu_io_unmap(void *ptr, unsigned int size); + ++#ifdef __FreeBSD__ ++void CDECL kqemu_loadfpucontext(unsigned long cpl); ++#endif ++ + int CDECL kqemu_schedule(void); + + void CDECL kqemu_log(const char *fmt, ...);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080509220922.GA13480>