[{"id":1798044,"web_url":"http://patchwork.ozlabs.org/comment/1798044/","msgid":"<87wp38hdrd.fsf@linaro.org>","list_archive_url":null,"date":"2017-11-02T16:00:22","subject":"Re: [Qemu-devel] [PATCH] cpu-exec: Exit exclusive region on longjmp\n\tfrom step_atomic","submitter":{"id":39532,"url":"http://patchwork.ozlabs.org/api/people/39532/","name":"Alex Bennée","email":"alex.bennee@linaro.org"},"content":"Peter Maydell writes:\n\n> Commit ac03ee5331612e44be narrowed the scope of the exclusive\n> region so it only covers when we're executing the TB, not when\n> we're generating it. However it missed that there is more than\n> one execution path out of cpu_tb_exec -- if the atomic insn\n> causes an exception then the code will longjmp out, skipping\n> the code to end the exclusive region. This causes QEMU to hang\n> the next time the CPU calls start_exclusive(), waiting for\n> itself to exit the region.\n>\n> Move the \"end the region\" code out to the end of the\n> function so that it is run for both normal exit and also\n> for exit-via-longjmp.\n>\n> (For some reason this only reproduces for me with a clang\n> optimized build, not a gcc debug build.)\n>\n> Fixes: ac03ee5331612e44beb393df2b578c951d27dc0d\n> Signed-off-by: Peter Maydell \n> ---\n> accel/tcg/cpu-exec.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c\n> index 4318441..ac316bb 100644\n> --- a/accel/tcg/cpu-exec.c\n> +++ b/accel/tcg/cpu-exec.c\n> @@ -256,9 +256,6 @@ void cpu_exec_step_atomic(CPUState *cpu)\n> trace_exec_tb(tb, pc);\n> cpu_tb_exec(cpu, tb);\n> cc->cpu_exec_exit(cpu);\n> - parallel_cpus = true;\n> -\n> - end_exclusive();\n> } else {\n> /* We may have exited due to another problem here, so we need\n> * to reset any tb_locks we may have taken but didn't release.\n> @@ -270,6 +267,9 @@ void cpu_exec_step_atomic(CPUState *cpu)\n> #endif\n> tb_lock_reset();\n> }\n> +\n> + parallel_cpus = true;\n> + end_exclusive();\n\nWe assume sigsetjmp can never fail - we either set the jump or are\nreturning from a longjmp back. So we can never be in the position of\nhaving not been through start_exclusive?\n\nWhat happens for example if we fault during translation?\n\n--\nAlex Bennée","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"I9B2lGF0\"; dkim-atps=neutral"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3ySVLV4xdNz9t2r\n\tfor ;\n\tFri, 3 Nov 2017 03:05:54 +1100 (AEDT)","from localhost ([::1]:32887 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1eAI08-0006tp-PT\n\tfor incoming@patchwork.ozlabs.org; Thu, 02 Nov 2017 12:05:52 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:34770)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1eAHuv-0003Fi-As\n\tfor qemu-devel@nongnu.org; Thu, 02 Nov 2017 12:00:35 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1eAHur-0000RO-CK\n\tfor qemu-devel@nongnu.org; Thu, 02 Nov 2017 12:00:29 -0400","from mail-wr0-x244.google.com ([2a00:1450:400c:c0c::244]:45008)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16)\n\t(Exim 4.71) (envelope-from )\n\tid 1eAHur-0000Qy-6Y\n\tfor qemu-devel@nongnu.org; Thu, 02 Nov 2017 12:00:25 -0400","by mail-wr0-x244.google.com with SMTP id z55so22319wrz.1\n\tfor ; Thu, 02 Nov 2017 09:00:24 -0700 (PDT)","from zen.linaro.local ([81.128.185.34])\n\tby smtp.gmail.com with ESMTPSA id\n\tt189sm1446049wmf.43.2017.11.02.09.00.22\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tThu, 02 Nov 2017 09:00:22 -0700 (PDT)","from zen (localhost [127.0.0.1])\n\tby zen.linaro.local (Postfix) with ESMTPS id 713E73E0078;\n\tThu, 2 Nov 2017 16:00:22 +0000 (GMT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=references:user-agent:from:to:cc:subject:in-reply-to:date\n\t:message-id:mime-version:content-transfer-encoding;\n\tbh=w/y5KRRbjaB6b8UGFS7U6yNjM0OGEQ3lEOXAzdq0IEs=;\n\tb=I9B2lGF0BTabpcYY1ggXYcVke/K8JoVL25gruqNEq5dndAJndbKPVe7VZqfxSMSBwq\n\t14GSfK1JuqywfScQJ1oBelWmzcwfx9T/beZWOpLgmwu7FCNWf11+f3oq0W8x7NcqrtC7\n\tvUbwRvR7+tDv774D2Stz4gg2PqfhXFhIXL1dU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:references:user-agent:from:to:cc:subject\n\t:in-reply-to:date:message-id:mime-version:content-transfer-encoding; \n\tbh=w/y5KRRbjaB6b8UGFS7U6yNjM0OGEQ3lEOXAzdq0IEs=;\n\tb=KUng3EP49dtM2GlwE1HoyQmfKxP/OKjajMNOtwdpXo+cRmPjxBYs32kcn/hZilvXhj\n\tt+Z8uLGU0N76IXU2MnVPS8lsgskaqh546TvHFfBNWgVJHUYy0PCh9Nksv7a0lJRl4UCE\n\tpeSMWvsV4874uqSgn7iQIsoNcgRahPlvDkl0IayOkcKzp5nhptQNkwyCBz7n4XmfaNwZ\n\tiAdLeRrtO297KhBonPnpJAmVfD4sGJP7htBluVsk1IAJsA8w3ZmuPKiWiU97T3+TAf8G\n\tenC+CcsAySGBBKIpTVnlG5am3FziWS1ciDGKr4wFBd0hcrB3/nByiU95MLRduItb/tXV\n\tYGpg==","X-Gm-Message-State":"AMCzsaU29ZUSnNBLyJQq1Rav1UeH7bFHiRubrVOYsNPf3yRdQQREJXM0\n\t02S5puFUNlfCRHMHXtfzo3tdWQ==","X-Google-Smtp-Source":"ABhQp+SJqWyxIPEIeBHMBAwqZyWmpdKle3FDoOUHvDaTQufTd7eN6LfhDfPY0rJ33N4sBt5U7Q3O+w==","X-Received":"by 10.223.155.208 with SMTP id e16mr3500776wrc.161.1509638423777;\n\tThu, 02 Nov 2017 09:00:23 -0700 (PDT)","References":"<1509634273-29111-1-git-send-email-peter.maydell@linaro.org>","User-agent":"mu4e 1.0-alpha0; emacs 26.0.90","From":"Alex =?utf-8?q?Benn=C3=A9e?= ","To":"Peter Maydell ","In-reply-to":"<1509634273-29111-1-git-send-email-peter.maydell@linaro.org>","Date":"Thu, 02 Nov 2017 16:00:22 +0000","Message-ID":"<87wp38hdrd.fsf@linaro.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: Genre and OS details not\n\trecognized.","X-Received-From":"2a00:1450:400c:c0c::244","Subject":"Re: [Qemu-devel] [PATCH] cpu-exec: Exit exclusive region on longjmp\n\tfrom step_atomic","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"\"Emilio G . Cota\" , qemu-devel@nongnu.org,\n\tRichard Henderson ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1798060,"web_url":"http://patchwork.ozlabs.org/comment/1798060/","msgid":"","list_archive_url":null,"date":"2017-11-02T16:17:29","subject":"Re: [Qemu-devel] [PATCH] cpu-exec: Exit exclusive region on longjmp\n\tfrom step_atomic","submitter":{"id":5111,"url":"http://patchwork.ozlabs.org/api/people/5111/","name":"Peter Maydell","email":"peter.maydell@linaro.org"},"content":"On 2 November 2017 at 16:00, Alex Bennée wrote:\n>\n> Peter Maydell writes:\n>\n>> Commit ac03ee5331612e44be narrowed the scope of the exclusive\n>> region so it only covers when we're executing the TB, not when\n>> we're generating it. However it missed that there is more than\n>> one execution path out of cpu_tb_exec -- if the atomic insn\n>> causes an exception then the code will longjmp out, skipping\n>> the code to end the exclusive region. This causes QEMU to hang\n>> the next time the CPU calls start_exclusive(), waiting for\n>> itself to exit the region.\n>>\n>> Move the \"end the region\" code out to the end of the\n>> function so that it is run for both normal exit and also\n>> for exit-via-longjmp.\n>>\n>> (For some reason this only reproduces for me with a clang\n>> optimized build, not a gcc debug build.)\n>>\n>> Fixes: ac03ee5331612e44beb393df2b578c951d27dc0d\n>> Signed-off-by: Peter Maydell \n>> ---\n>> accel/tcg/cpu-exec.c | 6 +++---\n>> 1 file changed, 3 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c\n>> index 4318441..ac316bb 100644\n>> --- a/accel/tcg/cpu-exec.c\n>> +++ b/accel/tcg/cpu-exec.c\n>> @@ -256,9 +256,6 @@ void cpu_exec_step_atomic(CPUState *cpu)\n>> trace_exec_tb(tb, pc);\n>> cpu_tb_exec(cpu, tb);\n>> cc->cpu_exec_exit(cpu);\n>> - parallel_cpus = true;\n>> -\n>> - end_exclusive();\n>> } else {\n>> /* We may have exited due to another problem here, so we need\n>> * to reset any tb_locks we may have taken but didn't release.\n>> @@ -270,6 +267,9 @@ void cpu_exec_step_atomic(CPUState *cpu)\n>> #endif\n>> tb_lock_reset();\n>> }\n>> +\n>> + parallel_cpus = true;\n>> + end_exclusive();\n>\n> We assume sigsetjmp can never fail - we either set the jump or are\n> returning from a longjmp back.\n\nCorrect, it can't fail.\n\n> So we can never be in the position of\n> having not been through start_exclusive?\n>\n> What happens for example if we fault during translation?\n\nHmm, yes, we can longjump out of tb_gen_code() too. Any suggestions\nfor how to handle that? The simple approach would be to\nhave a 'volatile bool in_exclusive_block;' that we set before\nexecuting the tb and then can check to determine whether to call\nend_exclusive() etc.\n\nCan we get away with\n if (!parallel_cpus) {\n /* We must have been inside the exclusive block, and got here\n * either by the TB longjmping out or by execution finishing\n */\n parallel_cpus = true;\n end_exclusive();\n }\n\nor is that unsafe?\n\nI guess the volatile flag is easier to analyze without having to\nthink very hard, which is a strong argument in its favour...\n\nthanks\n-- PMM","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"W8TkqY3O\"; dkim-atps=neutral"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3ySVdH5890z9t2r\n\tfor ;\n\tFri, 3 Nov 2017 03:18:41 +1100 (AEDT)","from localhost ([::1]:33041 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1eAICT-0006JV-9n\n\tfor incoming@patchwork.ozlabs.org; Thu, 02 Nov 2017 12:18:37 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:40189)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1eAIBk-00063C-LR\n\tfor qemu-devel@nongnu.org; Thu, 02 Nov 2017 12:17:53 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1eAIBj-00056i-Hr\n\tfor qemu-devel@nongnu.org; Thu, 02 Nov 2017 12:17:52 -0400","from mail-wr0-x244.google.com ([2a00:1450:400c:c0c::244]:44088)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16)\n\t(Exim 4.71) (envelope-from )\n\tid 1eAIBj-00055M-AM\n\tfor qemu-devel@nongnu.org; Thu, 02 Nov 2017 12:17:51 -0400","by mail-wr0-x244.google.com with SMTP id z55so71184wrz.1\n\tfor ; Thu, 02 Nov 2017 09:17:51 -0700 (PDT)","by 10.223.161.5 with HTTP; Thu, 2 Nov 2017 09:17:29 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=8H2+oXBduqTcRtZ4GPRMnRP1QzahCuVg6lYsf9hpINo=;\n\tb=W8TkqY3ONDN8/k4Gpg5vA7GkrydK8XZyGNPkslmLkaWgMJTOo6kGoWHyilxOm42X/4\n\tKqmGlcyuMvS+3NBtIdAxQnRDqNHk40YUV+JxyYN0t4gl/dVN6vst7NM6xSZTSGQAk0gO\n\tlayv4v9JSBXwEU1GPhyDbHNIzj1h/nyAXr2os=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=8H2+oXBduqTcRtZ4GPRMnRP1QzahCuVg6lYsf9hpINo=;\n\tb=OMYWENGHPFuf6L/hO/BBqYatQKafwxQPumIjs5ibNE/LeEF/SP2/dBHdaf/sTS/Jlf\n\t9+uGAnAvRaNkws7dhOGOpuElwU5Zc/vhhtYeUc8loqc42U2doV9SByZ8CAqDt83nP8iA\n\tBtD5PBtlziUr/3lnuLTGwUvDWEQPaLgPG+4Jdc/iP9D3B+VxlE91h6KXtRRIFOIKpibr\n\tRZCu1nh/xDgqOH1mJmhtZHvc5lX3CzouhyicSS3t7NY4uPVgauev/iXfu4O74nCTZcJ7\n\tlwca83G8c73k3H1pk1l0t+sBz4bUUIrlWpl89sd6f7X3ORYLQNi1PZv6+quyZoUCGrdr\n\t0u6A==","X-Gm-Message-State":"AMCzsaViGGXtY1gCvEyQz0kOTLUxzeiT9fcLIP31cpr2r7jXfcB3hTjD\n\t6PcCWiwBOzucsxgPkHyjiYRWKKsN2l/joxOP1RInaQ==","X-Google-Smtp-Source":"ABhQp+THYBNtIVnW0WYEVZcfH/00vVfHU0bJcMVO07nLweO6e6//mj+bTmxUDxFme+GwpGwJXz9NY0remDHDiSg44QA=","X-Received":"by 10.223.151.198 with SMTP id t6mr3577894wrb.2.1509639470280;\n\tThu, 02 Nov 2017 09:17:50 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<87wp38hdrd.fsf@linaro.org>","References":"<1509634273-29111-1-git-send-email-peter.maydell@linaro.org>\n\t<87wp38hdrd.fsf@linaro.org>","From":"Peter Maydell ","Date":"Thu, 2 Nov 2017 16:17:29 +0000","Message-ID":"","To":"=?utf-8?q?Alex_Benn=C3=A9e?= ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: Genre and OS details not\n\trecognized.","X-Received-From":"2a00:1450:400c:c0c::244","Subject":"Re: [Qemu-devel] [PATCH] cpu-exec: Exit exclusive region on longjmp\n\tfrom step_atomic","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"\"Emilio G . Cota\" ,\n\tQEMU Developers , \n\tRichard Henderson ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]