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

[Paolo Bonzini <pbonzini@redhat.com>]

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