[Qemu-devel] [PATCH] Reboot CPU on triple fault

[Qemu-devel] [PATCH] Reboot CPU on triple fault

[Hervé Poussineau]

[-- Attachment #1: Type: text/plain, Size: 125 bytes --]

Hi,

On i386/x86-64, CPU must reboot when a triple fault is detected.
Attached patch implements this behaviour.

Hervé

[-- Attachment #2: triple_fault.patch --]
[-- Type: text/plain, Size: 1064 bytes --]

Index: target-i386/helper.c
===================================================================
RCS file: /sources/qemu/qemu/target-i386/helper.c,v
retrieving revision 1.103
diff -u -r1.103 target-i386/helper.c
--- target-i386/helper.c	28 Mar 2008 22:30:30 -0000	1.103
+++ target-i386/helper.c	28 Mar 2008 22:55:52 -0000
@@ -1276,6 +1276,11 @@
 {
     if (!is_int) {
         svm_check_intercept_param(SVM_EXIT_EXCP_BASE + intno, error_code);
+        if (env->old_exception == EXCP08_DBLE) {
+            cpu_loop_exit();
+            cpu_reset(env);
+            return;
+        }
         intno = check_exception(intno, &error_code);
     }
 
@@ -1289,6 +1294,11 @@
 /* same as raise_exception_err, but do not restore global registers */
 static void raise_exception_err_norestore(int exception_index, int error_code)
 {
+    if (env->old_exception == EXCP08_DBLE) {
+        cpu_loop_exit();
+        cpu_reset(env);
+        return;
+    }
     exception_index = check_exception(exception_index, &error_code);
 
     env->exception_index = exception_index;

Re: [Qemu-devel] [PATCH] Reboot CPU on triple fault

[Alexander Graf]

On Mar 29, 2008, at 7:13 PM, Hervé Poussineau wrote:

> Hi,
>
> On i386/x86-64, CPU must reboot when a triple fault is detected.
> Attached patch implements this behaviour.

So what exactly does check_exception() in target-i386/helper.c do  
then? Should the cpu_reset call go in there?

Re: [Qemu-devel] [PATCH] Reboot CPU on triple fault

[Kevin Wolf]

Alexander Graf schrieb:
> 
> On Mar 29, 2008, at 7:13 PM, Hervé Poussineau wrote:
> 
>> Hi,
>>
>> On i386/x86-64, CPU must reboot when a triple fault is detected.
>> Attached patch implements this behaviour.
> 
> So what exactly does check_exception() in target-i386/helper.c do then?
> Should the cpu_reset call go in there?

It definitely should. Besides, I'd really like to have the CPU dump on
triple faults retained. In most cases, this will be a lot more useful
than a silent reboot.

Kevin

[Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Jan Kiszka]

Kevin Wolf wrote:
> Alexander Graf schrieb:
>> On Mar 29, 2008, at 7:13 PM, Hervé Poussineau wrote:
>>
>>> Hi,
>>>
>>> On i386/x86-64, CPU must reboot when a triple fault is detected.
>>> Attached patch implements this behaviour.
>> So what exactly does check_exception() in target-i386/helper.c do then?
>> Should the cpu_reset call go in there?
> 
> It definitely should. Besides, I'd really like to have the CPU dump on
> triple faults retained. In most cases, this will be a lot more useful
> than a silent reboot.

As the same requirement came up here, I worked out the following patch.
I feel a bit uneasy about it because

 o I'm unsure if breaking out of the exception loop is OK this way.

 o including necessary headers fails, mostly due to stdio redefinitions
   in dyngen-exec.h.

This patch behaves as it should, but only in one specific test case.
Feedback is welcome.

(BTW, the original patch didn't work at all.)

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 dyngen-exec.h        |    1 +
 target-i386/helper.c |   19 +++++++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

Index: b/dyngen-exec.h
===================================================================
--- a/dyngen-exec.h
+++ b/dyngen-exec.h
@@ -86,6 +86,7 @@ typedef struct FILE FILE;
 extern int fprintf(FILE *, const char *, ...);
 extern int fputs(const char *, FILE *);
 extern int printf(const char *, ...);
+extern FILE *stderr;
 #undef NULL
 #define NULL 0
 
Index: b/target-i386/helper.c
===================================================================
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1231,6 +1231,8 @@ void do_interrupt(int intno, int is_int,
     }
 }
 
+void qemu_system_reset_request(void);
+
 /*
  * Check nested exceptions and change to double or triple fault if
  * needed. It should only be called, if this is not an interrupt.
@@ -1248,8 +1250,21 @@ static int check_exception(int intno, in
         fprintf(logfile, "check_exception old: %x new %x\n",
                 env->old_exception, intno);
 
-    if (env->old_exception == EXCP08_DBLE)
-        cpu_abort(env, "triple fault");
+    if (env->old_exception == EXCP08_DBLE) {
+        fprintf(stderr, "qemu: warning: triple fault\n");
+        if(env->intercept & INTERCEPT_SVM_MASK) {
+            /* most probably the virtual machine should not
+               be shut down but rather caught by the VMM */
+            vmexit(SVM_EXIT_SHUTDOWN, 0);
+        }
+        cpu_dump_state(env, stderr, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+        if (logfile) {
+            fprintf(logfile, "qemu: warning: triple fault\n");
+            cpu_dump_state(env, logfile, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+        }
+        qemu_system_reset_request();
+        return 0;
+    }
 
     if ((first_contributory && second_contributory)
         || (env->old_exception == EXCP0E_PAGE &&

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Paul Brook]

> +    if (env->old_exception == EXCP08_DBLE) {
> +        fprintf(stderr, "qemu: warning: triple fault\n");

IMHO There's no reason to print a message to stderr. This is all well defined 
behavior, and the accepted way of exiting from 286 protected mode.

Paul

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 823 bytes --]

Jan Kiszka schrieb:
> As the same requirement came up here, I worked out the following patch.
> I feel a bit uneasy about it because
> 
>  o I'm unsure if breaking out of the exception loop is OK this way.
> 
>  o including necessary headers fails, mostly due to stdio redefinitions
>    in dyngen-exec.h.
> 
> This patch behaves as it should, but only in one specific test case.
> Feedback is welcome.

This is exactly the behaviour I'd like to have. I still needed a small
change to your patch, though: You shouldn't return 0 from
check_exception, after all it's not a divide error. I'm not sure if
EXCP_HLT is the right thing to return here, but the attached patch works
for me. Without this change it kept hanging in a loop forever writing
more dumps than I ever wanted. ;-)

Signed-off-by: Kevin Wolf <kwolf@suse.de>


[-- Attachment #2: qemu-triple-fault.patch --]
[-- Type: text/x-patch, Size: 1729 bytes --]

Index: dyngen-exec.h
===================================================================
--- dyngen-exec.h	(Revision 4215)
+++ dyngen-exec.h	(Arbeitskopie)
@@ -86,6 +86,7 @@
 extern int fprintf(FILE *, const char *, ...);
 extern int fputs(const char *, FILE *);
 extern int printf(const char *, ...);
+extern FILE *stderr;
 #undef NULL
 #define NULL 0
 
Index: target-i386/helper.c
===================================================================
--- target-i386/helper.c	(Revision 4215)
+++ target-i386/helper.c	(Arbeitskopie)
@@ -1231,6 +1231,8 @@
     }
 }
 
+void qemu_system_reset_request(void);
+
 /*
  * Check nested exceptions and change to double or triple fault if
  * needed. It should only be called, if this is not an interrupt.
@@ -1248,8 +1250,21 @@
         fprintf(logfile, "check_exception old: %x new %x\n",
                 env->old_exception, intno);
 
-    if (env->old_exception == EXCP08_DBLE)
-        cpu_abort(env, "triple fault");
+    if (env->old_exception == EXCP08_DBLE) {
+        fprintf(stderr, "qemu: warning: triple fault\n");
+        if(env->intercept & INTERCEPT_SVM_MASK) {
+            /* most probably the virtual machine should not
+               be shut down but rather caught by the VMM */
+            vmexit(SVM_EXIT_SHUTDOWN, 0);
+        }
+        cpu_dump_state(env, stderr, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+        if (logfile) {
+            fprintf(logfile, "qemu: warning: triple fault\n");
+            cpu_dump_state(env, logfile, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+        }
+        qemu_system_reset_request();
+        return EXCP_HLT;
+    }
 
     if ((first_contributory && second_contributory)
         || (env->old_exception == EXCP0E_PAGE &&

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Kevin Wolf]

Paul Brook schrieb:
>> +    if (env->old_exception == EXCP08_DBLE) {
>> +        fprintf(stderr, "qemu: warning: triple fault\n");
> 
> IMHO There's no reason to print a message to stderr. This is all well defined 
> behavior, and the accepted way of exiting from 286 protected mode.

How many users does qemu have who need triple faults to exit from 286
Protected Mode? And how many users does it have who don't use triple
faults (yes, it's called a fault, not a PM exiting feature)
intentionally but might want the message and dumps for debugging?
Personally I suspect that a great majority belongs to the latter case.

And honestly, a message on stderr really shouldn't hurt those 286 folks.
They are even getting the reset they want from this patch.

Kevin

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Jamie Lokier]

Kevin Wolf wrote:
> > IMHO There's no reason to print a message to stderr. This is all
> > well defined behavior, and the accepted way of exiting from 286
> > protected mode.
> 
> How many users does qemu have who need triple faults to exit from 286
> Protected Mode?

Not many, only those users running old MS-DOS apps / OSes.

> And how many users does it have who don't use triple
> faults (yes, it's called a fault, not a PM exiting feature)

Faults don't mean errors.  Think about page faults.

> And honestly, a message on stderr really shouldn't hurt those 286 folks.

For old MS-DOS apps / OSes, I have the impression this can happen
hundreds of times per second.  It's part of task context switching and
BIOS calls.

-- Jamie

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Antoine Kaufmann]

Am Mittwoch, 16. April 2008 11.23:40 schrieb Jamie Lokier:
> Kevin Wolf wrote:
> > > IMHO There's no reason to print a message to stderr. This is all
> > > well defined behavior, and the accepted way of exiting from 286
> > > protected mode.
> >
> > How many users does qemu have who need triple faults to exit from 286
> > Protected Mode?
>
> Not many, only those users running old MS-DOS apps / OSes.
>
> > And how many users does it have who don't use triple
> > faults (yes, it's called a fault, not a PM exiting feature)
>
> Faults don't mean errors.  Think about page faults.

I agree with you, but in that specific case, its definitively an Error if it 
isn't catched earlier.

> > And honestly, a message on stderr really shouldn't hurt those 286 folks.
>
> For old MS-DOS apps / OSes, I have the impression this can happen
> hundreds of times per second.  It's part of task context switching and
> BIOS calls.
If this is really a problem of performance, I would suggest to add a 
commandline switch to disable the error message.
This error message should not be removed imho. Because for those people who 
use it for debugging (I'm one of them), it's really nice. This message makes 
it possible for beginners to see, what went wrong, without further debugging.

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Kevin Wolf]

Jamie Lokier schrieb:
> Kevin Wolf wrote:
>>> IMHO There's no reason to print a message to stderr. This is all
>>> well defined behavior, and the accepted way of exiting from 286
>>> protected mode.
>> How many users does qemu have who need triple faults to exit from 286
>> Protected Mode?
> 
> Not many, only those users running old MS-DOS apps / OSes.

MS-DOS is running in Real Mode, you'd additionally need an application
which uses 286 PM. Apparently there aren't too many users relying on
triple faults as in the past qemu aborted (or in earlier versions even
hung) on triple faults and nobody complained.

>> And how many users does it have who don't use triple
>> faults (yes, it's called a fault, not a PM exiting feature)
> 
> Faults don't mean errors.  Think about page faults.

Ok, you're right on my wording. But double faults (which always occur
before a triple fault) are in fact despite their name not faults, but
aborts. Just to reword it using the correct terminology. ;-)

>> And honestly, a message on stderr really shouldn't hurt those 286 folks.
> 
> For old MS-DOS apps / OSes, I have the impression this can happen
> hundreds of times per second.  It's part of task context switching and
> BIOS calls.

I have to admit that I'm all but an 286 PM expert. What exactly is a
triple fault doing there? If it leads to a reset as implemented here, I
can hardly imagine that it is of much use in regular context switching.

Kevin

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Michal Schulz]

On Wednesday 16 April 2008, Kevin Wolf wrote:

> > For old MS-DOS apps / OSes, I have the impression this can happen
> > hundreds of times per second.  It's part of task context switching and
> > BIOS calls.
>
> I have to admit that I'm all but an 286 PM expert. What exactly is a
> triple fault doing there? If it leads to a reset as implemented here, I
> can hardly imagine that it is of much use in regular context switching.

It is required due to the "feature" of 286 CPU. Once 286 enters protected 
mode, it cannot be switched back to the real mode. The only way to perform it 
is resetting the CPU. A software which tries to go back from protected to 
real mode sets the warm-reset vector in BIOS memory area, sets the reboot 
reason variable in CMOS memory and issues a CPU reset.

CPU may be resetted due to a triple fault :)

-- 
Michal Schulz

[Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Jan Kiszka]

Kevin Wolf wrote:
> Jamie Lokier schrieb:
>> Kevin Wolf wrote:
>>> And honestly, a message on stderr really shouldn't hurt those 286 folks.
>> For old MS-DOS apps / OSes, I have the impression this can happen
>> hundreds of times per second.  It's part of task context switching and
>> BIOS calls.
> 
> I have to admit that I'm all but an 286 PM expert. What exactly is a
> triple fault doing there? If it leads to a reset as implemented here, I
> can hardly imagine that it is of much use in regular context switching.

According to my current information, the triple fault will make the
processor issue a shutdown cycle on its external bus. This is obviously
detected by many PC boards, which then issue a hard reset.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Michal Schulz]

On Wednesday 16 April 2008, Michal Schulz wrote:

> > I have to admit that I'm all but an 286 PM expert. What exactly is a
> > triple fault doing there? If it leads to a reset as implemented here, I
> > can hardly imagine that it is of much use in regular context switching.
>
> It is required due to the "feature" of 286 CPU. Once 286 enters protected
> mode, it cannot be switched back to the real mode. The only way to perform
> it is resetting the CPU. A software which tries to go back from protected
> to real mode sets the warm-reset vector in BIOS memory area, sets the
> reboot reason variable in CMOS memory and issues a CPU reset.
>
> CPU may be resetted due to a triple fault :)

and some example code using it (with comments):

http://www.x86.org/ftp/source/3fault/reset.asm

-- 
Michal Schulz

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Jamie Lokier]

Kevin Wolf wrote:
> > Not many, only those users running old MS-DOS apps / OSes.
> 
> MS-DOS is running in Real Mode, you'd additionally need an application
> which uses 286 PM. Apparently there aren't too many users relying on
> triple faults as in the past qemu aborted (or in earlier versions even
> hung) on triple faults and nobody complained.

That's true. I think they use Bochs instead.

> > For old MS-DOS apps / OSes, I have the impression this can happen
> > hundreds of times per second.  It's part of task context switching and
> > BIOS calls.
> 
> I have to admit that I'm all but an 286 PM expert. What exactly is a
> triple fault doing there? If it leads to a reset as implemented here, I
> can hardly imagine that it is of much use in regular context switching.

CPU reset is the only way to exit 286 protected mode, and that is
needed when context switching between real and protected mode tasks.

So apps set a byte in the CMOS NVRAM, trigger a processor reset, then
the BIOS reset code immediately jumps back to the app in real mode,
avoiding the normal reboot process.

It's slow and silly, but faster than the other method, asking the
keyboard controller to do it.

-- Jamie

[Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Jan Kiszka]

Kevin Wolf wrote:
> Jan Kiszka schrieb:
>> As the same requirement came up here, I worked out the following patch.
>> I feel a bit uneasy about it because
>>
>>  o I'm unsure if breaking out of the exception loop is OK this way.
>>
>>  o including necessary headers fails, mostly due to stdio redefinitions
>>    in dyngen-exec.h.
>>
>> This patch behaves as it should, but only in one specific test case.
>> Feedback is welcome.
> 
> This is exactly the behaviour I'd like to have. I still needed a small
> change to your patch, though: You shouldn't return 0 from
> check_exception, after all it's not a divide error. I'm not sure if
> EXCP_HLT is the right thing to return here, but the attached patch works
> for me. Without this change it kept hanging in a loop forever writing
> more dumps than I ever wanted. ;-)

Your version still works for me (and makes more sense than return 0,
indeed). So we just need someone to confirm that this is actually a
clean way to leave the fault handling.

And we need a consensus regarding fault reporting... What about
confining the dump to the logfile, leaving the console alone?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Kevin Wolf]

Michal Schulz schrieb:
> A software which tries to go back from protected to 
> real mode sets the warm-reset vector in BIOS memory area, sets the reboot 
> reason variable in CMOS memory and issues a CPU reset.

Oh, I see. The modification of the BIOS memory area is the piece I
missed. Now this makes much more sense.

Still the questions remains what to do with triple faults wrt logging. I
understand that in 286 PM there might be masses of faults which nobody
is interested in. On the other hand, I'd really hate to have no output
when my code crashes with a triple fault and I haven't enabled -d int.

Maybe we can check if this vector has been modified? If not, the triple
fault certainly wouldn't be an intended usage of a "feature".

Kevin

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Avi Kivity]

Jan Kiszka wrote:
> Kevin Wolf wrote:
>   
>> Jan Kiszka schrieb:
>>     
>>> As the same requirement came up here, I worked out the following patch.
>>> I feel a bit uneasy about it because
>>>
>>>  o I'm unsure if breaking out of the exception loop is OK this way.
>>>
>>>  o including necessary headers fails, mostly due to stdio redefinitions
>>>    in dyngen-exec.h.
>>>
>>> This patch behaves as it should, but only in one specific test case.
>>> Feedback is welcome.
>>>       
>> This is exactly the behaviour I'd like to have. I still needed a small
>> change to your patch, though: You shouldn't return 0 from
>> check_exception, after all it's not a divide error. I'm not sure if
>> EXCP_HLT is the right thing to return here, but the attached patch works
>> for me. Without this change it kept hanging in a loop forever writing
>> more dumps than I ever wanted. ;-)
>>     
>
> Your version still works for me (and makes more sense than return 0,
> indeed). So we just need someone to confirm that this is actually a
> clean way to leave the fault handling.
>
> And we need a consensus regarding fault reporting... What about
> confining the dump to the logfile, leaving the console alone?
>   

If logging is really necessary, have it disabled by default and add a 
switch to enable it.  This way ordinary (non-developers) don't need to 
worry about it, and developers can conveniently see it on stdout.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Kevin Wolf]

Jan Kiszka schrieb:
> And we need a consensus regarding fault reporting... What about
> confining the dump to the logfile, leaving the console alone?

Seems that the message is a big problem now, even if intentional
triple-faulting didn't work in the past and caused qemu to abort. So
yes, maybe the logfile must be enough.

But we definitely need a new -d option then so that a developer can
always run qemu with this option. The existing ones (most notably -d int
where this one would belong logically) are producing way too much output
to have them always enabled.

Kevin

[Qemu-devel] [PATCH] x86: Reboot CPU on triple fault - Version 3

[Jan Kiszka]

Here comes a version that may hopefully make everyone happy: :)

Reset on triple fault, but only dump the CPU state to stderr and logfile
if -triple-fault was given as command line option.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 dyngen-exec.h        |    1 +
 target-i386/helper.c |   24 ++++++++++++++++++++++--
 vl.c                 |    9 +++++++++
 3 files changed, 32 insertions(+), 2 deletions(-)

Index: b/dyngen-exec.h
===================================================================
--- a/dyngen-exec.h
+++ b/dyngen-exec.h
@@ -86,6 +86,7 @@ typedef struct FILE FILE;
 extern int fprintf(FILE *, const char *, ...);
 extern int fputs(const char *, FILE *);
 extern int printf(const char *, ...);
+extern FILE *stderr;
 #undef NULL
 #define NULL 0
 
Index: b/target-i386/helper.c
===================================================================
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1231,6 +1231,10 @@ void do_interrupt(int intno, int is_int,
     }
 }
 
+/* This should come from sysemu.h - if we could include it here... */
+void qemu_system_reset_request(void);
+extern int warn_on_triple_fault;
+
 /*
  * Check nested exceptions and change to double or triple fault if
  * needed. It should only be called, if this is not an interrupt.
@@ -1248,8 +1252,24 @@ static int check_exception(int intno, in
         fprintf(logfile, "check_exception old: %x new %x\n",
                 env->old_exception, intno);
 
-    if (env->old_exception == EXCP08_DBLE)
-        cpu_abort(env, "triple fault");
+    if (env->old_exception == EXCP08_DBLE) {
+        if(env->intercept & INTERCEPT_SVM_MASK) {
+            /* most probably the virtual machine should not
+               be shut down but rather caught by the VMM */
+            vmexit(SVM_EXIT_SHUTDOWN, 0);
+        }
+        if (warn_on_triple_fault) {
+            fprintf(stderr, "qemu: warning: triple fault\n");
+            cpu_dump_state(env, stderr, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+            if (logfile) {
+                fprintf(logfile, "qemu: warning: triple fault\n");
+                cpu_dump_state(env, logfile, fprintf,
+                               X86_DUMP_FPU | X86_DUMP_CCOP);
+            }
+        }
+        qemu_system_reset_request();
+        return EXCP_HLT;
+    }
 
     if ((first_contributory && second_contributory)
         || (env->old_exception == EXCP0E_PAGE &&
Index: b/vl.c
===================================================================
--- a/vl.c
+++ b/vl.c
@@ -200,6 +200,7 @@ CharDriverState *serial_hds[MAX_SERIAL_P
 CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
 #ifdef TARGET_I386
 int win2k_install_hack = 0;
+int warn_on_triple_fault = 0;
 #endif
 int usb_enabled = 0;
 static VLANState *first_vlan;
@@ -7730,6 +7731,7 @@ static void help(int exitcode)
            "-std-vga        simulate a standard VGA card with VESA Bochs Extensions\n"
            "                (default is CL-GD5446 PCI VGA)\n"
            "-no-acpi        disable ACPI\n"
+           "-triple-fault   enable CPU state dump on triple fault\n"
 #endif
 #ifdef CONFIG_CURSES
            "-curses         use a curses/ncurses interface instead of SDL\n"
@@ -7852,6 +7854,7 @@ enum {
     QEMU_OPTION_old_param,
     QEMU_OPTION_clock,
     QEMU_OPTION_startdate,
+    QEMU_OPTION_triple_fault,
 };
 
 typedef struct QEMUOption {
@@ -7964,6 +7967,9 @@ const QEMUOption qemu_options[] = {
 #endif
     { "clock", HAS_ARG, QEMU_OPTION_clock },
     { "startdate", HAS_ARG, QEMU_OPTION_startdate },
+#if defined(TARGET_I386)
+    { "triple-fault", 0, QEMU_OPTION_triple_fault },
+#endif
     { NULL },
 };
 
@@ -8702,6 +8708,9 @@ int main(int argc, char **argv)
             case QEMU_OPTION_win2k_hack:
                 win2k_install_hack = 1;
                 break;
+            case QEMU_OPTION_triple_fault:
+                warn_on_triple_fault = 1;
+                break;
 #endif
 #ifdef USE_KQEMU
             case QEMU_OPTION_no_kqemu:

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Jamie Lokier]

Kevin Wolf wrote:
> But we definitely need a new -d option then so that a developer can
> always run qemu with this option. The existing ones (most notably -d int
> where this one would belong logically) are producing way too much output
> to have them always enabled.

How about -d cpu_reset?  (As opposed to whole system reset).

These don't happen too often, normally.

-- Jamie

[Qemu-devel] [PATCH] x86: Reboot CPU on triple fault - Version 4

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 191 bytes --]

And another version which implements the -d cpu_reset suggested by
Jamie. Extending -d is probably better than introducing a completely new
option.

Signed-off-by: Kevin Wolf <kwolf@suse.de>

[-- Attachment #2: qemu-triple-fault-v4.patch --]
[-- Type: text/x-patch, Size: 2918 bytes --]

Index: dyngen-exec.h
===================================================================
--- dyngen-exec.h	(Revision 4215)
+++ dyngen-exec.h	(Arbeitskopie)
@@ -86,6 +86,7 @@
 extern int fprintf(FILE *, const char *, ...);
 extern int fputs(const char *, FILE *);
 extern int printf(const char *, ...);
+extern FILE *stderr;
 #undef NULL
 #define NULL 0
 
Index: exec.c
===================================================================
--- exec.c	(Revision 4215)
+++ exec.c	(Arbeitskopie)
@@ -1259,6 +1259,8 @@
 #ifdef TARGET_I386
     { CPU_LOG_PCALL, "pcall",
       "show protected mode far calls/returns/exceptions" },
+    { CPU_LOG_RESET, "cpu_reset",
+      "show CPU state before CPU resets" },
 #endif
 #ifdef DEBUG_IOPORT
     { CPU_LOG_IOPORT, "ioport",
Index: target-i386/helper.c
===================================================================
--- target-i386/helper.c	(Revision 4215)
+++ target-i386/helper.c	(Arbeitskopie)
@@ -1231,6 +1231,10 @@
     }
 }
 
+/* This should come from sysemu.h - if we could include it here... */
+void qemu_system_reset_request(void);
+extern int warn_on_triple_fault;
+
 /*
  * Check nested exceptions and change to double or triple fault if
  * needed. It should only be called, if this is not an interrupt.
@@ -1248,8 +1252,19 @@
         fprintf(logfile, "check_exception old: %x new %x\n",
                 env->old_exception, intno);
 
-    if (env->old_exception == EXCP08_DBLE)
-        cpu_abort(env, "triple fault");
+    if (env->old_exception == EXCP08_DBLE) {
+        if(env->intercept & INTERCEPT_SVM_MASK) {
+            /* most probably the virtual machine should not
+               be shut down but rather caught by the VMM */
+            vmexit(SVM_EXIT_SHUTDOWN, 0);
+        }
+        if (loglevel & CPU_LOG_RESET) {
+            fprintf(stderr, "qemu: warning: triple fault\n");
+            fprintf(logfile, "qemu: warning: triple fault\n");
+        }
+        qemu_system_reset_request();
+        return EXCP_HLT;
+    }
 
     if ((first_contributory && second_contributory)
         || (env->old_exception == EXCP0E_PAGE &&
Index: target-i386/helper2.c
===================================================================
--- target-i386/helper2.c	(Revision 4215)
+++ target-i386/helper2.c	(Arbeitskopie)
@@ -362,6 +362,10 @@
 void cpu_reset(CPUX86State *env)
 {
     int i;
+        
+    if (loglevel & CPU_LOG_RESET) {
+        cpu_dump_state(env, logfile, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+    }
 
     memset(env, 0, offsetof(CPUX86State, breakpoints));
 
Index: cpu-all.h
===================================================================
--- cpu-all.h	(Revision 4215)
+++ cpu-all.h	(Arbeitskopie)
@@ -779,6 +779,7 @@
 #define CPU_LOG_PCALL      (1 << 6)
 #define CPU_LOG_IOPORT     (1 << 7)
 #define CPU_LOG_TB_CPU     (1 << 8)
+#define CPU_LOG_RESET      (1 << 9)
 
 /* define log items */
 typedef struct CPULogItem {

[Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 4

[Jan Kiszka]

Kevin Wolf wrote:
> And another version which implements the -d cpu_reset suggested by
> Jamie. Extending -d is probably better than introducing a completely new
> option.

Sounds good.

> 
> Signed-off-by: Kevin Wolf <kwolf@suse.de>
> 
> Index: dyngen-exec.h
> ===================================================================
> --- dyngen-exec.h	(Revision 4215)
> +++ dyngen-exec.h	(Arbeitskopie)
> @@ -86,6 +86,7 @@
>  extern int fprintf(FILE *, const char *, ...);
>  extern int fputs(const char *, FILE *);
>  extern int printf(const char *, ...);
> +extern FILE *stderr;
>  #undef NULL
>  #define NULL 0
>  
> Index: exec.c
> ===================================================================
> --- exec.c	(Revision 4215)
> +++ exec.c	(Arbeitskopie)
> @@ -1259,6 +1259,8 @@
>  #ifdef TARGET_I386
>      { CPU_LOG_PCALL, "pcall",
>        "show protected mode far calls/returns/exceptions" },
> +    { CPU_LOG_RESET, "cpu_reset",
> +      "show CPU state before CPU resets" },
>  #endif
>  #ifdef DEBUG_IOPORT
>      { CPU_LOG_IOPORT, "ioport",
> Index: target-i386/helper.c
> ===================================================================
> --- target-i386/helper.c	(Revision 4215)
> +++ target-i386/helper.c	(Arbeitskopie)
> @@ -1231,6 +1231,10 @@
>      }
>  }
>  
> +/* This should come from sysemu.h - if we could include it here... */
> +void qemu_system_reset_request(void);
> +extern int warn_on_triple_fault;

The latter is now obsolete.

> +
>  /*
>   * Check nested exceptions and change to double or triple fault if
>   * needed. It should only be called, if this is not an interrupt.
> @@ -1248,8 +1252,19 @@
>          fprintf(logfile, "check_exception old: %x new %x\n",
>                  env->old_exception, intno);
>  
> -    if (env->old_exception == EXCP08_DBLE)
> -        cpu_abort(env, "triple fault");
> +    if (env->old_exception == EXCP08_DBLE) {
> +        if(env->intercept & INTERCEPT_SVM_MASK) {
> +            /* most probably the virtual machine should not
> +               be shut down but rather caught by the VMM */
> +            vmexit(SVM_EXIT_SHUTDOWN, 0);
> +        }
> +        if (loglevel & CPU_LOG_RESET) {
> +            fprintf(stderr, "qemu: warning: triple fault\n");

Does 'loglevel' manage dumping to stderr as well? I don't think so. To
remain consistent, the line above should probably be removed.

> +            fprintf(logfile, "qemu: warning: triple fault\n");
> +        }
> +        qemu_system_reset_request();
> +        return EXCP_HLT;
> +    }
>  
>      if ((first_contributory && second_contributory)
>          || (env->old_exception == EXCP0E_PAGE &&
> Index: target-i386/helper2.c
> ===================================================================
> --- target-i386/helper2.c	(Revision 4215)
> +++ target-i386/helper2.c	(Arbeitskopie)
> @@ -362,6 +362,10 @@
>  void cpu_reset(CPUX86State *env)
>  {
>      int i;
> +        
> +    if (loglevel & CPU_LOG_RESET) {
> +        cpu_dump_state(env, logfile, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
> +    }
>  
>      memset(env, 0, offsetof(CPUX86State, breakpoints));
>  
> Index: cpu-all.h
> ===================================================================
> --- cpu-all.h	(Revision 4215)
> +++ cpu-all.h	(Arbeitskopie)
> @@ -779,6 +779,7 @@
>  #define CPU_LOG_PCALL      (1 << 6)
>  #define CPU_LOG_IOPORT     (1 << 7)
>  #define CPU_LOG_TB_CPU     (1 << 8)
> +#define CPU_LOG_RESET      (1 << 9)
>  
>  /* define log items */
>  typedef struct CPULogItem {

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH] x86: Reboot CPU on triple fault - Version 5

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 196 bytes --]

Didn't you dare posting another patch yourself? ;-)

But you're right with those two points, I've fixed them now. The
hopefully final patch is attached.

Signed-off-by: Kevin Wolf <kwolf@suse.de>

[-- Attachment #2: qemu-triple-fault-v5.patch --]
[-- Type: text/x-patch, Size: 2861 bytes --]

Index: dyngen-exec.h
===================================================================
--- dyngen-exec.h	(Revision 4215)
+++ dyngen-exec.h	(Arbeitskopie)
@@ -86,6 +86,7 @@
 extern int fprintf(FILE *, const char *, ...);
 extern int fputs(const char *, FILE *);
 extern int printf(const char *, ...);
+extern FILE *stderr;
 #undef NULL
 #define NULL 0
 
Index: exec.c
===================================================================
--- exec.c	(Revision 4215)
+++ exec.c	(Arbeitskopie)
@@ -1259,6 +1259,8 @@
 #ifdef TARGET_I386
     { CPU_LOG_PCALL, "pcall",
       "show protected mode far calls/returns/exceptions" },
+    { CPU_LOG_RESET, "cpu_reset",
+      "show CPU state before CPU resets" },
 #endif
 #ifdef DEBUG_IOPORT
     { CPU_LOG_IOPORT, "ioport",
Index: target-i386/helper.c
===================================================================
--- target-i386/helper.c	(Revision 4215)
+++ target-i386/helper.c	(Arbeitskopie)
@@ -1231,6 +1231,9 @@
     }
 }
 
+/* This should come from sysemu.h - if we could include it here... */
+void qemu_system_reset_request(void);
+
 /*
  * Check nested exceptions and change to double or triple fault if
  * needed. It should only be called, if this is not an interrupt.
@@ -1248,9 +1251,20 @@
         fprintf(logfile, "check_exception old: %x new %x\n",
                 env->old_exception, intno);
 
-    if (env->old_exception == EXCP08_DBLE)
-        cpu_abort(env, "triple fault");
+    if (env->old_exception == EXCP08_DBLE) {
+        if(env->intercept & INTERCEPT_SVM_MASK) {
+            /* most probably the virtual machine should not
+               be shut down but rather caught by the VMM */
+            vmexit(SVM_EXIT_SHUTDOWN, 0);
+        }
 
+        if (loglevel & CPU_LOG_RESET)
+            fprintf(logfile, "Triple fault\n");
+
+        qemu_system_reset_request();
+        return EXCP_HLT;
+    }
+
     if ((first_contributory && second_contributory)
         || (env->old_exception == EXCP0E_PAGE &&
             (second_contributory || (intno == EXCP0E_PAGE)))) {
Index: target-i386/helper2.c
===================================================================
--- target-i386/helper2.c	(Revision 4215)
+++ target-i386/helper2.c	(Arbeitskopie)
@@ -362,6 +362,10 @@
 void cpu_reset(CPUX86State *env)
 {
     int i;
+        
+    if (loglevel & CPU_LOG_RESET) {
+        cpu_dump_state(env, logfile, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+    }
 
     memset(env, 0, offsetof(CPUX86State, breakpoints));
 
Index: cpu-all.h
===================================================================
--- cpu-all.h	(Revision 4215)
+++ cpu-all.h	(Arbeitskopie)
@@ -779,6 +779,7 @@
 #define CPU_LOG_PCALL      (1 << 6)
 #define CPU_LOG_IOPORT     (1 << 7)
 #define CPU_LOG_TB_CPU     (1 << 8)
+#define CPU_LOG_RESET      (1 << 9)
 
 /* define log items */
 typedef struct CPULogItem {

[Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 4

[Anthony Liguori]

Kevin Wolf wrote:
> And another version which implements the -d cpu_reset suggested by
> Jamie. Extending -d is probably better than introducing a completely new
> option.
> 
> Signed-off-by: Kevin Wolf <kwolf@suse.de>
>

Why is it so important to log triple faults?  Printing something to 
stderr instead of logfile is just wrong.

What is your use-case?  I think you've got the wrong solution to the 
problem and I can't find an adequate description of the problem your 
trying to solve in this thread.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH] Reboot CPU on triple fault

[Anthony Liguori]

Paul Brook wrote:
>> +    if (env->old_exception == EXCP08_DBLE) {
>> +        fprintf(stderr, "qemu: warning: triple fault\n");
>>     
>
> IMHO There's no reason to print a message to stderr. This is all well defined 
> behavior, and the accepted way of exiting from 286 protected mode.
>   

Moreover, it's a standard means of rebooting in the absence of APM or 
ACPI.  See native_machine_emergency_restart() in 
arch/x86/kernel/reboot.c in Linux.

Regards,

Anthony Liguori

> Paul
>
>
>   

[Qemu-devel] [PATCH] x86: Reboot CPU on triple fault - Version 6

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 208 bytes --]

Quite a few rounds for such a short patch. But surely a CPU dump without
any explanation isn't optimal. We should put a "CPU Reset" line in the
log before the dump.

Signed-off-by: Kevin Wolf <kwolf@suse.de>

[-- Attachment #2: qemu-triple-fault-v6.patch --]
[-- Type: text/x-patch, Size: 2928 bytes --]

Index: dyngen-exec.h
===================================================================
--- dyngen-exec.h	(Revision 4215)
+++ dyngen-exec.h	(Arbeitskopie)
@@ -86,6 +86,7 @@
 extern int fprintf(FILE *, const char *, ...);
 extern int fputs(const char *, FILE *);
 extern int printf(const char *, ...);
+extern FILE *stderr;
 #undef NULL
 #define NULL 0
 
Index: exec.c
===================================================================
--- exec.c	(Revision 4215)
+++ exec.c	(Arbeitskopie)
@@ -1259,6 +1259,8 @@
 #ifdef TARGET_I386
     { CPU_LOG_PCALL, "pcall",
       "show protected mode far calls/returns/exceptions" },
+    { CPU_LOG_RESET, "cpu_reset",
+      "show CPU state before CPU resets" },
 #endif
 #ifdef DEBUG_IOPORT
     { CPU_LOG_IOPORT, "ioport",
Index: target-i386/helper.c
===================================================================
--- target-i386/helper.c	(Revision 4215)
+++ target-i386/helper.c	(Arbeitskopie)
@@ -1231,6 +1231,9 @@
     }
 }
 
+/* This should come from sysemu.h - if we could include it here... */
+void qemu_system_reset_request(void);
+
 /*
  * Check nested exceptions and change to double or triple fault if
  * needed. It should only be called, if this is not an interrupt.
@@ -1248,9 +1251,20 @@
         fprintf(logfile, "check_exception old: %x new %x\n",
                 env->old_exception, intno);
 
-    if (env->old_exception == EXCP08_DBLE)
-        cpu_abort(env, "triple fault");
+    if (env->old_exception == EXCP08_DBLE) {
+        if(env->intercept & INTERCEPT_SVM_MASK) {
+            /* most probably the virtual machine should not
+               be shut down but rather caught by the VMM */
+            vmexit(SVM_EXIT_SHUTDOWN, 0);
+        }
 
+        if (loglevel & CPU_LOG_RESET)
+            fprintf(logfile, "Triple fault\n");
+
+        qemu_system_reset_request();
+        return EXCP_HLT;
+    }
+
     if ((first_contributory && second_contributory)
         || (env->old_exception == EXCP0E_PAGE &&
             (second_contributory || (intno == EXCP0E_PAGE)))) {
Index: target-i386/helper2.c
===================================================================
--- target-i386/helper2.c	(Revision 4215)
+++ target-i386/helper2.c	(Arbeitskopie)
@@ -362,6 +362,11 @@
 void cpu_reset(CPUX86State *env)
 {
     int i;
+        
+    if (loglevel & CPU_LOG_RESET) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+    }
 
     memset(env, 0, offsetof(CPUX86State, breakpoints));
 
Index: cpu-all.h
===================================================================
--- cpu-all.h	(Revision 4215)
+++ cpu-all.h	(Arbeitskopie)
@@ -779,6 +779,7 @@
 #define CPU_LOG_PCALL      (1 << 6)
 #define CPU_LOG_IOPORT     (1 << 7)
 #define CPU_LOG_TB_CPU     (1 << 8)
+#define CPU_LOG_RESET      (1 << 9)
 
 /* define log items */
 typedef struct CPULogItem {

[Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 4

[Jan Kiszka]

Anthony Liguori wrote:
> Kevin Wolf wrote:
>> And another version which implements the -d cpu_reset suggested by
>> Jamie. Extending -d is probably better than introducing a completely new
>> option.
>>
>> Signed-off-by: Kevin Wolf <kwolf@suse.de>
>>
> 
> Why is it so important to log triple faults?  Printing something to
> stderr instead of logfile is just wrong.
> 
> What is your use-case?  I think you've got the wrong solution to the
> problem and I can't find an adequate description of the problem your
> trying to solve in this thread.

You may not always sit next to your VM when it decides to reboot due to
a _real_ problem that caused a triple fault.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 6

[Jan Kiszka]

Kevin Wolf wrote:
> Quite a few rounds for such a short patch. But surely a CPU dump without
> any explanation isn't optimal. We should put a "CPU Reset" line in the
> log before the dump.
> 
> Signed-off-by: Kevin Wolf <kwolf@suse.de>
> 

Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

(But you should kill the trailing whitespace in helper2.c - quilt can
helpful for managing patches and keeping such things clean.)

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 4

[Anthony Liguori]

Jan Kiszka wrote:
> Anthony Liguori wrote:
>   
>> Why is it so important to log triple faults?  Printing something to
>> stderr instead of logfile is just wrong.
>>
>> What is your use-case?  I think you've got the wrong solution to the
>> problem and I can't find an adequate description of the problem your
>> trying to solve in this thread.
>>     
>
> You may not always sit next to your VM when it decides to reboot due to
> a _real_ problem that caused a triple fault.
>   

You can always check within the guest to see if it's rebooted (via 
uptime for instance).

It's extremely unlikely you'll ever see an OS triple fault in the wild 
unless you're doing kernel development.  Triple faulting requires a bad 
IDT or a really bad page table both of which are not something an OS is 
likely to do by accident.  If your OS is triple faulting, I highly doubt 
it's just going to reboot and everything's going to be okay.

Besides, the -no-reboot flag should already satisfy your needs if you're 
really concerned about it.

Regards,

Anthony Liguori

> Jan
>
>   

[Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 4

[Jan Kiszka]

Anthony Liguori wrote:
> Jan Kiszka wrote:
>> Anthony Liguori wrote:
>>  
>>> Why is it so important to log triple faults?  Printing something to
>>> stderr instead of logfile is just wrong.
>>>
>>> What is your use-case?  I think you've got the wrong solution to the
>>> problem and I can't find an adequate description of the problem your
>>> trying to solve in this thread.
>>>     
>>
>> You may not always sit next to your VM when it decides to reboot due to
>> a _real_ problem that caused a triple fault.
>>   
> 
> You can always check within the guest to see if it's rebooted (via
> uptime for instance).

But you won't find the CPU state on triple fault there.

> 
> It's extremely unlikely you'll ever see an OS triple fault in the wild
> unless you're doing kernel development.  Triple faulting requires a bad
> IDT or a really bad page table both of which are not something an OS is
> likely to do by accident.  If your OS is triple faulting, I highly doubt
> it's just going to reboot and everything's going to be okay.

There are various OSes out there in the wild. Not all of them conform to
common assumptions about how OSes typically look like. And once you
start moving things under a different roof (like QEMU), you are better
off logging such /potentially/ critical events (specifically if that
roof is a bit smaller due to missing segment limit and type checks).
That's at least our situation ATM.

> 
> Besides, the -no-reboot flag should already satisfy your needs if you're
> really concerned about it.

Nope, because we also have the case where a triple fault is
intentionally triggered to perform a reboot.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 4

[Anthony Liguori]

Jan Kiszka wrote:
> Anthony Liguori wrote:
>   
>> Jan Kiszka wrote:
>>     
>> You can always check within the guest to see if it's rebooted (via
>> uptime for instance).
>>     
>
> But you won't find the CPU state on triple fault there.
>   

Nor will you with your patch.

>> It's extremely unlikely you'll ever see an OS triple fault in the wild
>> unless you're doing kernel development.  Triple faulting requires a bad
>> IDT or a really bad page table both of which are not something an OS is
>> likely to do by accident.  If your OS is triple faulting, I highly doubt
>> it's just going to reboot and everything's going to be okay.
>>     
>
> There are various OSes out there in the wild. Not all of them conform to
> common assumptions about how OSes typically look like. And once you
> start moving things under a different roof (like QEMU), you are better
> off logging such /potentially/ critical events (specifically if that
> roof is a bit smaller due to missing segment limit and type checks).
> That's at least our situation ATM.
>   

It's not a question of whether it's logged, it's a question of whether 
it gets logged *specially*.  That's the crux of the discussion here.  My 
argument is that triple faults are not sufficiently special that they 
warrant *special* logging.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 4

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]

Anthony Liguori wrote:
> Jan Kiszka wrote:
>> Anthony Liguori wrote:
>>  
>>> Jan Kiszka wrote:
>>>     You can always check within the guest to see if it's rebooted (via
>>> uptime for instance).
>>>     
>>
>> But you won't find the CPU state on triple fault there.
>>   
> 
> Nor will you with your patch.

Provided "-d cpu_reset" with Kevin's last patch, they are. I tried it,
it worked, that's why I acked it.

> 
>>> It's extremely unlikely you'll ever see an OS triple fault in the wild
>>> unless you're doing kernel development.  Triple faulting requires a bad
>>> IDT or a really bad page table both of which are not something an OS is
>>> likely to do by accident.  If your OS is triple faulting, I highly doubt
>>> it's just going to reboot and everything's going to be okay.
>>>     
>>
>> There are various OSes out there in the wild. Not all of them conform to
>> common assumptions about how OSes typically look like. And once you
>> start moving things under a different roof (like QEMU), you are better
>> off logging such /potentially/ critical events (specifically if that
>> roof is a bit smaller due to missing segment limit and type checks).
>> That's at least our situation ATM.
>>   
> 
> It's not a question of whether it's logged, it's a question of whether
> it gets logged *specially*.  That's the crux of the discussion here.  My
> argument is that triple faults are not sufficiently special that they
> warrant *special* logging.

Depends on the POV. But I really think we loose nothing with the latest
patch, rather gain a useful feature for certain (granted) corner cases.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 4

[Kevin Wolf]

Anthony Liguori schrieb:
> It's not a question of whether it's logged, it's a question of whether
> it gets logged *specially*.  That's the crux of the discussion here.  My
> argument is that triple faults are not sufficiently special that they
> warrant *special* logging.

I think CPU resets are special enough indeed. Obviously there _are_ use
cases for this option and nobody who doesn't turn it on intentionally
could ever get hurt by it. So what is your problem with it?

Honestly, it is ridiculous that we keep arguing about log messages while
the main thing changed by the patch is the introduction of the correct
CPU reset. I suspect we would never have discussed all these loglevel
things if the latest patch wouldn't have been submitted as answer on a
(not even working) patch which tried to remove the log...

Kevin

Re: [Qemu-devel] [PATCH] x86: Reboot CPU on triple fault - Version 6

[Anthony Liguori]

Kevin Wolf wrote:
> Quite a few rounds for such a short patch. But surely a CPU dump without
> any explanation isn't optimal. We should put a "CPU Reset" line in the
> log before the dump.
>
> Signed-off-by: Kevin Wolf <kwolf@suse.de>
>   
> Index: dyngen-exec.h
> ===================================================================
> --- dyngen-exec.h	(Revision 4215)
> +++ dyngen-exec.h	(Arbeitskopie)
> @@ -86,6 +86,7 @@
>  extern int fprintf(FILE *, const char *, ...);
>  extern int fputs(const char *, FILE *);
>  extern int printf(const char *, ...);
> +extern FILE *stderr;
>   

This is unnecessary.

>  #undef NULL
>  #define NULL 0
>  
> Index: exec.c
> ===================================================================
> --- exec.c	(Revision 4215)
> +++ exec.c	(Arbeitskopie)
> @@ -1259,6 +1259,8 @@
>  #ifdef TARGET_I386
>      { CPU_LOG_PCALL, "pcall",
>        "show protected mode far calls/returns/exceptions" },
> +    { CPU_LOG_RESET, "cpu_reset",
> +      "show CPU state before CPU resets" },
>  #endif
>  #ifdef DEBUG_IOPORT
>      { CPU_LOG_IOPORT, "ioport",
> Index: target-i386/helper.c
> ===================================================================
> --- target-i386/helper.c	(Revision 4215)
> +++ target-i386/helper.c	(Arbeitskopie)
> @@ -1231,6 +1231,9 @@
>      }
>  }
>  
> +/* This should come from sysemu.h - if we could include it here... */
> +void qemu_system_reset_request(void);
> +
>  /*
>   * Check nested exceptions and change to double or triple fault if
>   * needed. It should only be called, if this is not an interrupt.
> @@ -1248,9 +1251,20 @@
>          fprintf(logfile, "check_exception old: %x new %x\n",
>                  env->old_exception, intno);
>  
> -    if (env->old_exception == EXCP08_DBLE)
> -        cpu_abort(env, "triple fault");
> +    if (env->old_exception == EXCP08_DBLE) {
> +        if(env->intercept & INTERCEPT_SVM_MASK) {
> +            /* most probably the virtual machine should not
> +               be shut down but rather caught by the VMM */
> +            vmexit(SVM_EXIT_SHUTDOWN, 0);
> +        }
>  
> +        if (loglevel & CPU_LOG_RESET)
> +            fprintf(logfile, "Triple fault\n");
> +
> +        qemu_system_reset_request();
>   

This isn't the right function to use here.  If we supported ACPI 
shutdown, this would generate an ACPI shutdown request.  You probably 
want to just do:

cpu_interrupt(env, CPU_INTERRUPT_EXIT);

Regards,

Anthony Liguori

> +        return EXCP_HLT;
> +    }
> +
>      if ((first_contributory && second_contributory)
>          || (env->old_exception == EXCP0E_PAGE &&
>              (second_contributory || (intno == EXCP0E_PAGE)))) {
> Index: target-i386/helper2.c
> ===================================================================
> --- target-i386/helper2.c	(Revision 4215)
> +++ target-i386/helper2.c	(Arbeitskopie)
> @@ -362,6 +362,11 @@
>  void cpu_reset(CPUX86State *env)
>  {
>      int i;
> +        
> +    if (loglevel & CPU_LOG_RESET) {
> +        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
> +        cpu_dump_state(env, logfile, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
> +    }
>  
>      memset(env, 0, offsetof(CPUX86State, breakpoints));
>  
> Index: cpu-all.h
> ===================================================================
> --- cpu-all.h	(Revision 4215)
> +++ cpu-all.h	(Arbeitskopie)
> @@ -779,6 +779,7 @@
>  #define CPU_LOG_PCALL      (1 << 6)
>  #define CPU_LOG_IOPORT     (1 << 7)
>  #define CPU_LOG_TB_CPU     (1 << 8)
> +#define CPU_LOG_RESET      (1 << 9)
>  
>  /* define log items */
>  typedef struct CPULogItem {
>   

Re: [Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 4

[Anthony Liguori]

Kevin Wolf wrote:
> Anthony Liguori schrieb:
>   
>> It's not a question of whether it's logged, it's a question of whether
>> it gets logged *specially*.  That's the crux of the discussion here.  My
>> argument is that triple faults are not sufficiently special that they
>> warrant *special* logging.
>>     
>
> I think CPU resets are special enough indeed. Obviously there _are_ use
> cases for this option and nobody who doesn't turn it on intentionally
> could ever get hurt by it. So what is your problem with it?
>   

You're missing my whole point.  v4 spewed stuff to stderr.  That's the 
problem I had.

> Honestly, it is ridiculous that we keep arguing about log messages while
> the main thing changed by the patch is the introduction of the correct
> CPU reset. I suspect we would never have discussed all these loglevel
> things if the latest patch wouldn't have been submitted as answer on a
> (not even working) patch which tried to remove the log...
>   

The patch still isn't right but it's close.  Logging events to logfile 
is fine.  It's logging to stderr that was problematic.

Regards,

Anthony Liguori

> Kevin
>
>
>   

Re: [Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 4

[Kevin Wolf]

Anthony Liguori schrieb:
> You're missing my whole point.  v4 spewed stuff to stderr.  That's the
> problem I had.

Yes, I was missing that you still were talking about v4, not v6. Sorry.

Kevin

Re: [Qemu-devel] [PATCH] x86: Reboot CPU on triple fault - Version 6

[Kevin Wolf]

Anthony Liguori schrieb:
>>   Index: dyngen-exec.h
>> ===================================================================
>> --- dyngen-exec.h    (Revision 4215)
>> +++ dyngen-exec.h    (Arbeitskopie)
>> @@ -86,6 +86,7 @@
>>  extern int fprintf(FILE *, const char *, ...);
>>  extern int fputs(const char *, FILE *);
>>  extern int printf(const char *, ...);
>> +extern FILE *stderr;
>>   
> 
> This is unnecessary.

Right. I'll fix this with the next version.

>> +
>> +        qemu_system_reset_request();
>>   
> 
> This isn't the right function to use here.  If we supported ACPI
> shutdown, this would generate an ACPI shutdown request.  You probably
> want to just do:
> 
> cpu_interrupt(env, CPU_INTERRUPT_EXIT);

Your suggestion doesn't work for me either. qemu keeps hanging producing
lots of triple faults, but there is no CPU reset. Is there even a
function to correctly request a CPU reset only? Directly calling
cpu_reset doesn't work here.

Kevin

[Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 6

[Jan Kiszka]

To pick this up again, as the problem is still unfixed:

Kevin Wolf wrote:
> Anthony Liguori schrieb:
>>>   Index: dyngen-exec.h
>>> ===================================================================
>>> --- dyngen-exec.h    (Revision 4215)
>>> +++ dyngen-exec.h    (Arbeitskopie)
>>> @@ -86,6 +86,7 @@
>>>  extern int fprintf(FILE *, const char *, ...);
>>>  extern int fputs(const char *, FILE *);
>>>  extern int printf(const char *, ...);
>>> +extern FILE *stderr;
>>>   
>> This is unnecessary.
> 
> Right. I'll fix this with the next version.

To accelerate this "a bit" ;), I attached a refreshed version of your
patch.

> 
>>> +
>>> +        qemu_system_reset_request();
>>>   
>> This isn't the right function to use here.  If we supported ACPI
>> shutdown, this would generate an ACPI shutdown request.  You probably
>> want to just do:
>>
>> cpu_interrupt(env, CPU_INTERRUPT_EXIT);
> 
> Your suggestion doesn't work for me either. qemu keeps hanging producing
> lots of triple faults, but there is no CPU reset. Is there even a
> function to correctly request a CPU reset only? Directly calling
> cpu_reset doesn't work here.

IMHO, qemu_system_reset_request is the right way to raise a system-wide
hard reset. I actually don't see any relation to ACPI at this lowest
level.

Jan

---
 cpu-all.h             |    1 +
 exec.c                |    2 ++
 target-i386/helper.c  |   18 ++++++++++++++++--
 target-i386/helper2.c |    5 +++++
 4 files changed, 24 insertions(+), 2 deletions(-)

Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1302,6 +1302,8 @@ CPULogItem cpu_log_items[] = {
 #ifdef TARGET_I386
     { CPU_LOG_PCALL, "pcall",
       "show protected mode far calls/returns/exceptions" },
+    { CPU_LOG_RESET, "cpu_reset",
+      "show CPU state before CPU resets" },
 #endif
 #ifdef DEBUG_IOPORT
     { CPU_LOG_IOPORT, "ioport",
Index: b/target-i386/helper.c
===================================================================
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1261,6 +1261,9 @@ void do_interrupt(int intno, int is_int,
     }
 }
 
+/* This should come from sysemu.h - if we could include it here... */
+void qemu_system_reset_request(void);
+
 /*
  * Check nested exceptions and change to double or triple fault if
  * needed. It should only be called, if this is not an interrupt.
@@ -1278,8 +1281,19 @@ static int check_exception(int intno, in
         fprintf(logfile, "check_exception old: 0x%x new 0x%x\n",
                 env->old_exception, intno);
 
-    if (env->old_exception == EXCP08_DBLE)
-        cpu_abort(env, "triple fault");
+    if (env->old_exception == EXCP08_DBLE) {
+        if(env->intercept & INTERCEPT_SVM_MASK) {
+            /* most probably the virtual machine should not
+               be shut down but rather caught by the VMM */
+            vmexit(SVM_EXIT_SHUTDOWN, 0);
+        }
+
+        if (loglevel & CPU_LOG_RESET)
+            fprintf(logfile, "Triple fault\n");
+
+        qemu_system_reset_request();
+        return EXCP_HLT;
+    }
 
     if ((first_contributory && second_contributory)
         || (env->old_exception == EXCP0E_PAGE &&
Index: b/target-i386/helper2.c
===================================================================
--- a/target-i386/helper2.c
+++ b/target-i386/helper2.c
@@ -363,6 +363,11 @@ void cpu_reset(CPUX86State *env)
 {
     int i;
 
+    if (loglevel & CPU_LOG_RESET) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+    }
+
     memset(env, 0, offsetof(CPUX86State, breakpoints));
 
     tlb_flush(env, 1);
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -824,6 +824,7 @@ target_phys_addr_t cpu_get_phys_page_deb
 #define CPU_LOG_PCALL      (1 << 6)
 #define CPU_LOG_IOPORT     (1 << 7)
 #define CPU_LOG_TB_CPU     (1 << 8)
+#define CPU_LOG_RESET      (1 << 9)
 
 /* define log items */
 typedef struct CPULogItem {

[Qemu-devel] [PATCH] x86: Reboot CPU on triple fault - Version 8

[Jan Kiszka]

Jan Kiszka wrote:
> To pick this up again, as the problem is still unfixed:
> 
> Kevin Wolf wrote:
>> Anthony Liguori schrieb:
>>>>   Index: dyngen-exec.h
>>>> ===================================================================
>>>> --- dyngen-exec.h    (Revision 4215)
>>>> +++ dyngen-exec.h    (Arbeitskopie)
>>>> @@ -86,6 +86,7 @@
>>>>  extern int fprintf(FILE *, const char *, ...);
>>>>  extern int fputs(const char *, FILE *);
>>>>  extern int printf(const char *, ...);
>>>> +extern FILE *stderr;
>>>>   
>>> This is unnecessary.
>> Right. I'll fix this with the next version.
> 
> To accelerate this "a bit" ;), I attached a refreshed version of your
> patch.

And here is a version that even compiles (sorry).

I assume that we do not want to let a triple fault in SVM context
escalate to the guest's host context, right? So I left the check
in (while it was removed meanwhile from cpu_abort).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpu-all.h             |    1 +
 exec.c                |    2 ++
 target-i386/helper.c  |   15 +++++++++++++--
 target-i386/helper2.c |    5 +++++
 4 files changed, 21 insertions(+), 2 deletions(-)

Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1302,6 +1302,8 @@ CPULogItem cpu_log_items[] = {
 #ifdef TARGET_I386
     { CPU_LOG_PCALL, "pcall",
       "show protected mode far calls/returns/exceptions" },
+    { CPU_LOG_RESET, "cpu_reset",
+      "show CPU state before CPU resets" },
 #endif
 #ifdef DEBUG_IOPORT
     { CPU_LOG_IOPORT, "ioport",
Index: b/target-i386/helper.c
===================================================================
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1261,6 +1261,9 @@ void do_interrupt(int intno, int is_int,
     }
 }
 
+/* This should come from sysemu.h - if we could include it here... */
+void qemu_system_reset_request(void);
+
 /*
  * Check nested exceptions and change to double or triple fault if
  * needed. It should only be called, if this is not an interrupt.
@@ -1278,8 +1281,16 @@ static int check_exception(int intno, in
         fprintf(logfile, "check_exception old: 0x%x new 0x%x\n",
                 env->old_exception, intno);
 
-    if (env->old_exception == EXCP08_DBLE)
-        cpu_abort(env, "triple fault");
+    if (env->old_exception == EXCP08_DBLE) {
+        if (env->intercept & INTERCEPT_SVM_MASK)
+            helper_vmexit(SVM_EXIT_SHUTDOWN, 0);
+
+        if (loglevel & CPU_LOG_RESET)
+            fprintf(logfile, "Triple fault\n");
+
+        qemu_system_reset_request();
+        return EXCP_HLT;
+    }
 
     if ((first_contributory && second_contributory)
         || (env->old_exception == EXCP0E_PAGE &&
Index: b/target-i386/helper2.c
===================================================================
--- a/target-i386/helper2.c
+++ b/target-i386/helper2.c
@@ -363,6 +363,11 @@ void cpu_reset(CPUX86State *env)
 {
     int i;
 
+    if (loglevel & CPU_LOG_RESET) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+    }
+
     memset(env, 0, offsetof(CPUX86State, breakpoints));
 
     tlb_flush(env, 1);
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -824,6 +824,7 @@ target_phys_addr_t cpu_get_phys_page_deb
 #define CPU_LOG_PCALL      (1 << 6)
 #define CPU_LOG_IOPORT     (1 << 7)
 #define CPU_LOG_TB_CPU     (1 << 8)
+#define CPU_LOG_RESET      (1 << 9)
 
 /* define log items */
 typedef struct CPULogItem {

[Qemu-devel] Re: [PATCH] x86: Reboot CPU on triple fault - Version 8

[Jan Kiszka]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> To pick this up again, as the problem is still unfixed:
>>
>> Kevin Wolf wrote:
>>> Anthony Liguori schrieb:
>>>>>   Index: dyngen-exec.h
>>>>> ===================================================================
>>>>> --- dyngen-exec.h    (Revision 4215)
>>>>> +++ dyngen-exec.h    (Arbeitskopie)
>>>>> @@ -86,6 +86,7 @@
>>>>>  extern int fprintf(FILE *, const char *, ...);
>>>>>  extern int fputs(const char *, FILE *);
>>>>>  extern int printf(const char *, ...);
>>>>> +extern FILE *stderr;
>>>>>   
>>>> This is unnecessary.
>>> Right. I'll fix this with the next version.
>> To accelerate this "a bit" ;), I attached a refreshed version of your
>> patch.
> 
> And here is a version that even compiles (sorry).

Sigh, almost. linux-user become broken, and I posted faster than it took
the compiler to reach that target.

No point in testing for double faults in user space, I guess...

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpu-all.h             |    1 +
 exec.c                |    2 ++
 target-i386/helper.c  |   17 +++++++++++++++--
 target-i386/helper2.c |    5 +++++
 4 files changed, 23 insertions(+), 2 deletions(-)

Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1302,6 +1302,8 @@ CPULogItem cpu_log_items[] = {
 #ifdef TARGET_I386
     { CPU_LOG_PCALL, "pcall",
       "show protected mode far calls/returns/exceptions" },
+    { CPU_LOG_RESET, "cpu_reset",
+      "show CPU state before CPU resets" },
 #endif
 #ifdef DEBUG_IOPORT
     { CPU_LOG_IOPORT, "ioport",
Index: b/target-i386/helper.c
===================================================================
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1261,6 +1261,9 @@ void do_interrupt(int intno, int is_int,
     }
 }
 
+/* This should come from sysemu.h - if we could include it here... */
+void qemu_system_reset_request(void);
+
 /*
  * Check nested exceptions and change to double or triple fault if
  * needed. It should only be called, if this is not an interrupt.
@@ -1278,8 +1281,18 @@ static int check_exception(int intno, in
         fprintf(logfile, "check_exception old: 0x%x new 0x%x\n",
                 env->old_exception, intno);
 
-    if (env->old_exception == EXCP08_DBLE)
-        cpu_abort(env, "triple fault");
+#if !defined(CONFIG_USER_ONLY)
+    if (env->old_exception == EXCP08_DBLE) {
+        if (env->intercept & INTERCEPT_SVM_MASK)
+            helper_vmexit(SVM_EXIT_SHUTDOWN, 0);
+
+        if (loglevel & CPU_LOG_RESET)
+            fprintf(logfile, "Triple fault\n");
+
+        qemu_system_reset_request();
+        return EXCP_HLT;
+    }
+#endif
 
     if ((first_contributory && second_contributory)
         || (env->old_exception == EXCP0E_PAGE &&
Index: b/target-i386/helper2.c
===================================================================
--- a/target-i386/helper2.c
+++ b/target-i386/helper2.c
@@ -363,6 +363,11 @@ void cpu_reset(CPUX86State *env)
 {
     int i;
 
+    if (loglevel & CPU_LOG_RESET) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+    }
+
     memset(env, 0, offsetof(CPUX86State, breakpoints));
 
     tlb_flush(env, 1);
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -824,6 +824,7 @@ target_phys_addr_t cpu_get_phys_page_deb
 #define CPU_LOG_PCALL      (1 << 6)
 #define CPU_LOG_IOPORT     (1 << 7)
 #define CPU_LOG_TB_CPU     (1 << 8)
+#define CPU_LOG_RESET      (1 << 9)
 
 /* define log items */
 typedef struct CPULogItem {