[Qemu-devel] [PATCH v2] cpu-exec: Exit exclusive region on longjmp from step_atomic

[Qemu-devel] [PATCH v2] cpu-exec: Exit exclusive region on longjmp from step_atomic

[Peter Maydell]

Commit ac03ee5331612e44be narrowed the scope of the exclusive
region so it only covers when we're executing the TB, not when
we're generating it. However it missed that there is more than
one execution path out of cpu_tb_exec -- if the atomic insn
causes an exception then the code will longjmp out, skipping
the code to end the exclusive region. This causes QEMU to hang
the next time the CPU calls start_exclusive(), waiting for
itself to exit the region.

Move the "end the region" code out to the end of the
function so that it is run for both normal exit and also
for exit-via-longjmp. We have to use a volatile bool flag
to decide whether we need to end the region, because we
can longjump out of the codegen as well as the execution.

(For some reason this only reproduces for me with a clang
optimized build, not a gcc debug build.)

Fixes: ac03ee5331612e44beb393df2b578c951d27dc0d
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Changes v1->v2: use a flag so we get the "longjump out of the
codegen" case right.

 accel/tcg/cpu-exec.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 4318441..61297f8 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -233,6 +233,8 @@ void cpu_exec_step_atomic(CPUState *cpu)
     uint32_t flags;
     uint32_t cflags = 1;
     uint32_t cf_mask = cflags & CF_HASH_MASK;
+    /* volatile because we modify it between setjmp and longjmp */
+    volatile bool in_exclusive_region = false;
 
     if (sigsetjmp(cpu->jmp_env, 0) == 0) {
         tb = tb_lookup__cpu_state(cpu, &pc, &cs_base, &flags, cf_mask);
@@ -251,14 +253,12 @@ void cpu_exec_step_atomic(CPUState *cpu)
 
         /* Since we got here, we know that parallel_cpus must be true.  */
         parallel_cpus = false;
+        in_exclusive_region = true;
         cc->cpu_exec_enter(cpu);
         /* execute the generated code */
         trace_exec_tb(tb, pc);
         cpu_tb_exec(cpu, tb);
         cc->cpu_exec_exit(cpu);
-        parallel_cpus = true;
-
-        end_exclusive();
     } else {
         /* We may have exited due to another problem here, so we need
          * to reset any tb_locks we may have taken but didn't release.
@@ -270,6 +270,15 @@ void cpu_exec_step_atomic(CPUState *cpu)
 #endif
         tb_lock_reset();
     }
+
+    if (in_exclusive_region) {
+        /* We might longjump out of either the codegen or the
+         * execution, so must make sure we only end the exclusive
+         * region if we started it.
+         */
+        parallel_cpus = true;
+        end_exclusive();
+    }
 }
 
 struct tb_desc {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2] cpu-exec: Exit exclusive region on longjmp from step_atomic

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH v2] cpu-exec: Exit exclusive region on longjmp from step_atomic
Type: series
Message-id: 1509640536-32160-1-git-send-email-peter.maydell@linaro.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/1509640536-32160-1-git-send-email-peter.maydell@linaro.org -> patchew/1509640536-32160-1-git-send-email-peter.maydell@linaro.org
Switched to a new branch 'test'
bf245cb3db cpu-exec: Exit exclusive region on longjmp from step_atomic

=== OUTPUT BEGIN ===
Checking PATCH 1/1: cpu-exec: Exit exclusive region on longjmp from step_atomic...
ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#37: FILE: accel/tcg/cpu-exec.c:237:
+    volatile bool in_exclusive_region = false;

total: 1 errors, 0 warnings, 38 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v2] cpu-exec: Exit exclusive region on longjmp from step_atomic

[Peter Maydell]

On 2 November 2017 at 16:38,  <no-reply@patchew.org> wrote:
> Checking PATCH 1/1: cpu-exec: Exit exclusive region on longjmp from step_atomic...
> ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
> #37: FILE: accel/tcg/cpu-exec.c:237:
> +    volatile bool in_exclusive_region = false;

Usually, but not in this case :-)

-- PMM

Re: [Qemu-devel] [PATCH v2] cpu-exec: Exit exclusive region on longjmp from step_atomic

[Emilio G. Cota]

On Thu, Nov 02, 2017 at 16:35:36 +0000, Peter Maydell wrote:
> Commit ac03ee5331612e44be narrowed the scope of the exclusive
> region so it only covers when we're executing the TB, not when
> we're generating it. However it missed that there is more than
> one execution path out of cpu_tb_exec -- if the atomic insn
> causes an exception then the code will longjmp out, skipping
> the code to end the exclusive region. This causes QEMU to hang
> the next time the CPU calls start_exclusive(), waiting for
> itself to exit the region.
> 
> Move the "end the region" code out to the end of the
> function so that it is run for both normal exit and also
> for exit-via-longjmp. We have to use a volatile bool flag
> to decide whether we need to end the region, because we
> can longjump out of the codegen as well as the execution.
> 
> (For some reason this only reproduces for me with a clang
> optimized build, not a gcc debug build.)
> 
> Fixes: ac03ee5331612e44beb393df2b578c951d27dc0d
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Emilio G. Cota <cota@braap.org>

Good catch, thanks for the fix!

		E.

Re: [Qemu-devel] [PATCH v2] cpu-exec: Exit exclusive region on longjmp from step_atomic

[Richard Henderson]

On 11/02/2017 05:35 PM, Peter Maydell wrote:
> Commit ac03ee5331612e44be narrowed the scope of the exclusive
> region so it only covers when we're executing the TB, not when
> we're generating it. However it missed that there is more than
> one execution path out of cpu_tb_exec -- if the atomic insn
> causes an exception then the code will longjmp out, skipping
> the code to end the exclusive region. This causes QEMU to hang
> the next time the CPU calls start_exclusive(), waiting for
> itself to exit the region.
> 
> Move the "end the region" code out to the end of the
> function so that it is run for both normal exit and also
> for exit-via-longjmp. We have to use a volatile bool flag
> to decide whether we need to end the region, because we
> can longjump out of the codegen as well as the execution.
> 
> (For some reason this only reproduces for me with a clang
> optimized build, not a gcc debug build.)
> 
> Fixes: ac03ee5331612e44beb393df2b578c951d27dc0d
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Changes v1->v2: use a flag so we get the "longjump out of the
> codegen" case right.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

I've got one other tcg patch queued for 2.11.
I could take this via tcg-next if you like.


r~

Re: [Qemu-devel] [PATCH v2] cpu-exec: Exit exclusive region on longjmp from step_atomic

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Commit ac03ee5331612e44be narrowed the scope of the exclusive
> region so it only covers when we're executing the TB, not when
> we're generating it. However it missed that there is more than
> one execution path out of cpu_tb_exec -- if the atomic insn
> causes an exception then the code will longjmp out, skipping
> the code to end the exclusive region. This causes QEMU to hang
> the next time the CPU calls start_exclusive(), waiting for
> itself to exit the region.
>
> Move the "end the region" code out to the end of the
> function so that it is run for both normal exit and also
> for exit-via-longjmp. We have to use a volatile bool flag
> to decide whether we need to end the region, because we
> can longjump out of the codegen as well as the execution.
>
> (For some reason this only reproduces for me with a clang
> optimized build, not a gcc debug build.)
>
> Fixes: ac03ee5331612e44beb393df2b578c951d27dc0d
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée