From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42996) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UiLGS-0002RL-Bd for qemu-devel@nongnu.org; Fri, 31 May 2013 05:04:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UiLGQ-0002oM-RS for qemu-devel@nongnu.org; Fri, 31 May 2013 05:04:48 -0400 Received: from mel.act-europe.fr ([194.98.77.210]:41124 helo=smtp.eu.adacore.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UiLGQ-0002oI-LM for qemu-devel@nongnu.org; Fri, 31 May 2013 05:04:46 -0400 Message-ID: <51A867A5.4050101@adacore.com> Date: Fri, 31 May 2013 11:04:37 +0200 From: Fabien Chouteau MIME-Version: 1.0 References: <1369930950-18564-1-git-send-email-chouteau@adacore.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] ARM: Fix disable interrupt for M profile List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: blauwirbel@gmail.com, pbonzini@redhat.com, rth@twiddle.net, qemu-devel@nongnu.org, afaerber@suse.de On 05/30/2013 07:26 PM, Peter Maydell wrote: > On 30 May 2013 17:22, Fabien Chouteau wrote: >> I'm not sure this was expected or not, but it looks like the "||" should >> be a "&&". Otherwise it's not possible to disable interrupt. >> >> Signed-off-by: Fabien Chouteau > > We've had people trying to fiddle with this bit of code > before. I'd like to see a clear description of: > * the bug (ideally with a test case) The bug: It's impossible to disable interrupt on M-profile. Test case: I can try to build a binary, the source will be in Ada. > * why this patch fixes it (probably with reference to the > architecture manual and to what QEMU means when it sets > "CPSR_I" on M profile [hint: look at how we handle PRIMASK]) I do not intend to do a full review of interrupt handling in the M-profile. I just noticed that disabling interrupt was not effective, and found this condition that looked faulty to me. I want to fix the condition to match requirements of the comment above. > * a demonstration that it doesn't break non-M-profile > Much more difficult for me, but that's what peer-review is for, right?. >> @@ -462,8 +462,8 @@ int cpu_exec(CPUArchState *env) >> We avoid this by disabling interrupts when >> pc contains a magic address. */ >> if (interrupt_request & CPU_INTERRUPT_HARD >> - && ((IS_M(env) & env->regs[15] < 0xfffffff0) >> - || !(env->uncached_cpsr & CPSR_I))) { >> + && (IS_M(env) && env->regs[15] < 0xfffffff0) >> + && !(env->uncached_cpsr & CPSR_I)) { >> env->exception_index = EXCP_IRQ; >> cc->do_interrupt(cpu); >> next_tb = 0; > > ...for instance this change means that the if() condition > will now never be satisfied for a non-M-profile core, > which is definitely wrong. > You're right I did this a little bit too quickly. The expression should be: if (interrupt_request & CPU_INTERRUPT_HARD && (!IS_M(env) || env->regs[15] < 0xfffffff0) && !(env->uncached_cpsr & CPSR_I)) { I'll resend a patch, just in case. Thanks, -- Fabien Chouteau