Re: [Qemu-devel] [RFC PATCH v4 01/23] This patch adds a condition before overwriting exception_index fields.

[Paolo Bonzini]

On 19/01/2018 09:42, Pavel Dovgalyuk wrote:
> It is needed when exception_index is already set to some meaningful value.
> 

Pavel, very frankly, this commit message is awful, and for two reasons.

First, it should include the high level overview of the bug ("XYZ does
not work") and the description of why XYZ does not work (what is the
meaningful value?  who set it?)

Second, it also resets cpu->exception_index to -1 in after calling
cc->cpu_exec_interrupt.  Why is this correct?  Who was clearing it
before?  Is that clearing still needed?  What does
cc->cpu_exec_interrupt even have to do with cpu->exception_index, since
exceptions are handled by cc->do_interrupt?

And so on.

Paolo

> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  accel/tcg/cpu-exec.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 280200f..9cc6972 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -585,6 +585,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>          else {
>              if (cc->cpu_exec_interrupt(cpu, interrupt_request)) {
>                  replay_interrupt();
> +                cpu->exception_index = -1;
>                  *last_tb = NULL;
>              }
>              /* The target hook may have updated the 'cpu->interrupt_request';
> @@ -606,7 +607,9 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>      if (unlikely(atomic_read(&cpu->exit_request)
>          || (use_icount && cpu->icount_decr.u16.low + cpu->icount_extra == 
> 0))) {
>          atomic_set(&cpu->exit_request, 0);
> -        cpu->exception_index = EXCP_INTERRUPT;
> +        if (cpu->exception_index == -1) {
> +            cpu->exception_index = EXCP_INTERRUPT;
> +        }
>          return true;
>      }
>  
>