Re: [Qemu-devel] [RFC PATCH v7 08/21] replay: interrupts and exceptions

2015-01-14 Thread Pavel Dovgaluk 
 From: Paolo Bonzini [mailto:pbonz...@redhat.com]
 On 12/01/2015 13:40, Pavel Dovgaluk wrote:
 
   Perhaps check the replay_interrupt() outside, in an  with if
   (unlikely(interrupt_request))?
  You mean that I should wrap whole condition into unlikely?
 
 
 No, I wanted to have a single check of replay_interrupt() and/or
 replay_has_interrupt().
 
 BTW, I think this is incorrect:
 
  +if ((replay_mode != REPLAY_MODE_PLAY
  +|| replay_has_interrupt())
  + cc-cpu_exec_interrupt(cpu, interrupt_request)) 
  {
  +replay_interrupt();
 
 because cc-cpu_exec_interrupt() can exit with cpu_loop_exit(cpu).

Haven't found any. Do you have an example?

 
 I guess it could be something like this:
 
 if (replay_mode == REPLAY_MODE_PLAY  !replay_has_interrupt()) {
 /* do nothing */
 } else if (interrupt_request  CPU_INTERRUPT_HALT) {
 replay_interrupt();
 ...
 cpu_loop_exit(cpu);
 } else if (interrupt_request  CPU_INTERRUPT_INIT) {
 replay_interrupt();
 ...
 cpu_loop_exit(cpu);
 } else {
 replay_interrupt();
 if (cc-cpu_exec_interrupt(cpu, interrupt_request)) {
 next_tb = 0;
 }
 }

Is it normal that processing of the reset request does not execute 
cpu_loop_exit(cpu)?

 #else
-if (interrupt_request  CPU_INTERRUPT_RESET) {
+if ((interrupt_request  CPU_INTERRUPT_RESET)
+ replay_interrupt()) {
 cpu_reset(cpu);
 }
 #endif


Pavel Dovgalyuk

Re: [Qemu-devel] [RFC PATCH v7 08/21] replay: interrupts and exceptions

2015-01-14 Thread Paolo Bonzini 


On 14/01/2015 10:07, Pavel Dovgaluk wrote:
 From: Paolo Bonzini [mailto:pbonz...@redhat.com]
 On 12/01/2015 13:40, Pavel Dovgaluk wrote:

 Perhaps check the replay_interrupt() outside, in an  with if
 (unlikely(interrupt_request))?
 You mean that I should wrap whole condition into unlikely?


 No, I wanted to have a single check of replay_interrupt() and/or
 replay_has_interrupt().

 BTW, I think this is incorrect:

 +if ((replay_mode != REPLAY_MODE_PLAY
 +|| replay_has_interrupt())
 + cc-cpu_exec_interrupt(cpu, interrupt_request)) 
 {
 +replay_interrupt();

 because cc-cpu_exec_interrupt() can exit with cpu_loop_exit(cpu).
 
 Haven't found any. Do you have an example?

Yes:

cpu_svm_check_intercept_param -
helper_svm_check_intercept_param -
helper_vmexit - cpu_loop_exit

 if (replay_mode == REPLAY_MODE_PLAY  !replay_has_interrupt()) {
 /* do nothing */
 } else if (interrupt_request  CPU_INTERRUPT_HALT) {
 replay_interrupt();
 ...
 cpu_loop_exit(cpu);
 } else if (interrupt_request  CPU_INTERRUPT_INIT) {
 replay_interrupt();
 ...
 cpu_loop_exit(cpu);
 } else {
 replay_interrupt();
 if (cc-cpu_exec_interrupt(cpu, interrupt_request)) {
 next_tb = 0;
 }
 }
 
 Is it normal that processing of the reset request does not execute 
 cpu_loop_exit(cpu)?

I think it is okay.  INIT executes cpu_loop_exit() on x86 because
processors other than the boot processor are halted after they receive INIT.

Paolo

Re: [Qemu-devel] [RFC PATCH v7 08/21] replay: interrupts and exceptions

2015-01-14 Thread Pavel Dovgaluk 
 From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo 
 Bonzini
 On 14/01/2015 10:07, Pavel Dovgaluk wrote:
  From: Paolo Bonzini [mailto:pbonz...@redhat.com]
  On 12/01/2015 13:40, Pavel Dovgaluk wrote:
 
  Perhaps check the replay_interrupt() outside, in an  with if
  (unlikely(interrupt_request))?
  You mean that I should wrap whole condition into unlikely?
 
 
  No, I wanted to have a single check of replay_interrupt() and/or
  replay_has_interrupt().
 
  BTW, I think this is incorrect:
 
  +if ((replay_mode != REPLAY_MODE_PLAY
  +|| replay_has_interrupt())
  + cc-cpu_exec_interrupt(cpu, 
  interrupt_request)) {
  +replay_interrupt();
 
  because cc-cpu_exec_interrupt() can exit with cpu_loop_exit(cpu).
 
  Haven't found any. Do you have an example?
 
 Yes:
 
 cpu_svm_check_intercept_param -
 helper_svm_check_intercept_param -
 helper_vmexit - cpu_loop_exit

Thanks.

  if (replay_mode == REPLAY_MODE_PLAY  !replay_has_interrupt()) {
  /* do nothing */
  } else if (interrupt_request  CPU_INTERRUPT_HALT) {
  replay_interrupt();
  ...
  cpu_loop_exit(cpu);
  } else if (interrupt_request  CPU_INTERRUPT_INIT) {
  replay_interrupt();
  ...
  cpu_loop_exit(cpu);
  } else {
  replay_interrupt();
  if (cc-cpu_exec_interrupt(cpu, interrupt_request)) {
  next_tb = 0;
  }
  }
 
  Is it normal that processing of the reset request does not execute 
  cpu_loop_exit(cpu)?
 
 I think it is okay.  INIT executes cpu_loop_exit() on x86 because
 processors other than the boot processor are halted after they receive INIT.

Then I cannot put everything in one if-else chain because it will change the 
behavior of the code.
After processing RESET branch we can also process hardware interrupts (in 
unmodified code).

Pavel Dovgalyuk

Re: [Qemu-devel] [RFC PATCH v7 08/21] replay: interrupts and exceptions

2015-01-14 Thread Paolo Bonzini 


On 14/01/2015 11:06, Pavel Dovgaluk wrote:
  I think it is okay.  INIT executes cpu_loop_exit() on x86 because
  processors other than the boot processor are halted after they receive 
  INIT.
 Then I cannot put everything in one if-else chain because it will change the 
 behavior of the code.
 After processing RESET branch we can also process hardware interrupts (in 
 unmodified code).

Then if it's simpler you can add cpu_loop_exit().  It's not required,
but it's also definitely not a fast path.

Paolo

Re: [Qemu-devel] [RFC PATCH v7 08/21] replay: interrupts and exceptions

2015-01-12 Thread Paolo Bonzini 


On 12/01/2015 13:00, Pavel Dovgalyuk wrote:
 
 +if (replay_exception()) {
 +cc-do_interrupt(cpu);
 +cpu-exception_index = -1;

I cannot see replay_exception() in the series?

 @@ -419,21 +434,24 @@ int cpu_exec(CPUArchState *env)
  cpu-exception_index = EXCP_DEBUG;
  cpu_loop_exit(cpu);

Why not for EXCP_DEBUG?

  }
 -if (interrupt_request  CPU_INTERRUPT_HALT) {
 +if ((interrupt_request  CPU_INTERRUPT_HALT)
 + replay_interrupt()) {
  cpu-interrupt_request = ~CPU_INTERRUPT_HALT;
  cpu-halted = 1;
  cpu-exception_index = EXCP_HLT;
  cpu_loop_exit(cpu);
  }
  #if defined(TARGET_I386)
 -if (interrupt_request  CPU_INTERRUPT_INIT) {
 +if ((interrupt_request  CPU_INTERRUPT_INIT)
 + replay_interrupt()) {
  cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0);
  do_cpu_init(x86_cpu);
  cpu-exception_index = EXCP_HALTED;
  cpu_loop_exit(cpu);
  }
  #else
 -if (interrupt_request  CPU_INTERRUPT_RESET) {
 +if ((interrupt_request  CPU_INTERRUPT_RESET)
 + replay_interrupt()) {
  cpu_reset(cpu);
  }
  #endif

Perhaps check the replay_interrupt() outside, in an  with if
(unlikely(interrupt_request))?

 @@ -441,7 +459,10 @@ int cpu_exec(CPUArchState *env)
 False when the interrupt isn't processed,
 True when it is, and we should restart on a new TB,
 and via longjmp via cpu_loop_exit.  */
 -if (cc-cpu_exec_interrupt(cpu, interrupt_request)) {
 +if ((replay_mode != REPLAY_MODE_PLAY
 +|| replay_has_interrupt())
 + cc-cpu_exec_interrupt(cpu, interrupt_request)) {
 +replay_interrupt();

Please put this in a separate function like:

if (replay_mode == REPLAY_MODE_PLAY  !replay_has_interrupt()) {
return false;
}
ret = cc-cpu_exec_interrupt(cpu, interrupt_request);
if (ret) {
replay_interrupt();
}
return ret;

Paolo

  next_tb = 0;
  }
  /* Don't use the cached interrupt_request value,
 @@ -453,7 +474,8 @@ int cpu_exec(CPUArchState *env)

Re: [Qemu-devel] [RFC PATCH v7 08/21] replay: interrupts and exceptions

2015-01-12 Thread Pavel Dovgaluk 
 From: Paolo Bonzini [mailto:pbonz...@redhat.com]
 On 12/01/2015 13:00, Pavel Dovgalyuk wrote:
 
  +if (replay_exception()) {
  +cc-do_interrupt(cpu);
  +cpu-exception_index = -1;
 
 I cannot see replay_exception() in the series?

It is in the patch number 8.

 
  @@ -419,21 +434,24 @@ int cpu_exec(CPUArchState *env)
   cpu-exception_index = EXCP_DEBUG;
   cpu_loop_exit(cpu);
 
 Why not for EXCP_DEBUG?

As far as I understand, EXCP_DEBUG is used for external debugger (like gdb).
Debugger is usually connected to VM during replay.
That is why we do not need to replay EXCP_DEBUG - it may appear in different
places, and it does not affect on behavior of the virtual machine.

  -if (interrupt_request  CPU_INTERRUPT_HALT) {
  +if ((interrupt_request  CPU_INTERRUPT_HALT)
  + replay_interrupt()) {
   cpu-interrupt_request = ~CPU_INTERRUPT_HALT;
   cpu-halted = 1;
   cpu-exception_index = EXCP_HLT;
   cpu_loop_exit(cpu);
   }
   #if defined(TARGET_I386)
  -if (interrupt_request  CPU_INTERRUPT_INIT) {
  +if ((interrupt_request  CPU_INTERRUPT_INIT)
  + replay_interrupt()) {
   cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 
  0);
   do_cpu_init(x86_cpu);
   cpu-exception_index = EXCP_HALTED;
   cpu_loop_exit(cpu);
   }
   #else
  -if (interrupt_request  CPU_INTERRUPT_RESET) {
  +if ((interrupt_request  CPU_INTERRUPT_RESET)
  + replay_interrupt()) {
   cpu_reset(cpu);
   }
   #endif
 
 Perhaps check the replay_interrupt() outside, in an  with if
 (unlikely(interrupt_request))?

You mean that I should wrap whole condition into unlikely?

  @@ -441,7 +459,10 @@ int cpu_exec(CPUArchState *env)
  False when the interrupt isn't processed,
  True when it is, and we should restart on a new TB,
  and via longjmp via cpu_loop_exit.  */
  -if (cc-cpu_exec_interrupt(cpu, interrupt_request)) {
  +if ((replay_mode != REPLAY_MODE_PLAY
  +|| replay_has_interrupt())
  + cc-cpu_exec_interrupt(cpu, interrupt_request)) 
  {
  +replay_interrupt();
 
 Please put this in a separate function like:
 
 if (replay_mode == REPLAY_MODE_PLAY  !replay_has_interrupt()) {
 return false;
 }
 ret = cc-cpu_exec_interrupt(cpu, interrupt_request);
 if (ret) {
 replay_interrupt();
 }
 return ret;

Ok.

Pavel Dovgalyuk

Re: [Qemu-devel] [RFC PATCH v7 08/21] replay: interrupts and exceptions

2015-01-12 Thread Paolo Bonzini 


On 12/01/2015 13:40, Pavel Dovgaluk wrote:

 +if (replay_exception()) {
 +cc-do_interrupt(cpu);
 +cpu-exception_index = -1;

 I cannot see replay_exception() in the series?
 
 It is in the patch number 8.

Oops, it is in this patch in fact.

   @@ -419,21 +434,24 @@ int cpu_exec(CPUArchState *env)
cpu-exception_index = EXCP_DEBUG;
cpu_loop_exit(cpu);
  
  Why not for EXCP_DEBUG?
 As far as I understand, EXCP_DEBUG is used for external debugger (like gdb).
 Debugger is usually connected to VM during replay.
 That is why we do not need to replay EXCP_DEBUG - it may appear in different
 places, and it does not affect on behavior of the virtual machine.

Ok.

   -if (interrupt_request  CPU_INTERRUPT_HALT) {
   +if ((interrupt_request  CPU_INTERRUPT_HALT)
   + replay_interrupt()) {
cpu-interrupt_request = ~CPU_INTERRUPT_HALT;
cpu-halted = 1;
cpu-exception_index = EXCP_HLT;
cpu_loop_exit(cpu);
}
#if defined(TARGET_I386)
   -if (interrupt_request  CPU_INTERRUPT_INIT) {
   +if ((interrupt_request  CPU_INTERRUPT_INIT)
   + replay_interrupt()) {
cpu_svm_check_intercept_param(env, 
   SVM_EXIT_INIT, 0);
do_cpu_init(x86_cpu);
cpu-exception_index = EXCP_HALTED;
cpu_loop_exit(cpu);
}
#else
   -if (interrupt_request  CPU_INTERRUPT_RESET) {
   +if ((interrupt_request  CPU_INTERRUPT_RESET)
   + replay_interrupt()) {
cpu_reset(cpu);
}
#endif
  
  Perhaps check the replay_interrupt() outside, in an  with if
  (unlikely(interrupt_request))?
 You mean that I should wrap whole condition into unlikely?
 

No, I wanted to have a single check of replay_interrupt() and/or
replay_has_interrupt().

BTW, I think this is incorrect:

 +if ((replay_mode != REPLAY_MODE_PLAY
 +|| replay_has_interrupt())
 + cc-cpu_exec_interrupt(cpu, interrupt_request)) {
 +replay_interrupt();

because cc-cpu_exec_interrupt() can exit with cpu_loop_exit(cpu).

I guess it could be something like this:

if (replay_mode == REPLAY_MODE_PLAY  !replay_has_interrupt()) {
/* do nothing */
} else if (interrupt_request  CPU_INTERRUPT_HALT) {
replay_interrupt();
...
cpu_loop_exit(cpu);
} else if (interrupt_request  CPU_INTERRUPT_INIT) {
replay_interrupt();
...
cpu_loop_exit(cpu);
} else {
replay_interrupt();
if (cc-cpu_exec_interrupt(cpu, interrupt_request)) {
next_tb = 0;
}
}

Paolo