From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:51877) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QNVwg-0003Wm-LJ for qemu-devel@nongnu.org; Fri, 20 May 2011 16:05:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QNVwf-0006xy-Hg for qemu-devel@nongnu.org; Fri, 20 May 2011 16:05:14 -0400 Received: from mail-bw0-f45.google.com ([209.85.214.45]:48970) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QNVwf-0006xu-Ch for qemu-devel@nongnu.org; Fri, 20 May 2011 16:05:13 -0400 Received: by bwz16 with SMTP id 16so3551628bwz.4 for ; Fri, 20 May 2011 13:05:12 -0700 (PDT) From: Max Filippov Date: Sat, 21 May 2011 00:05:00 +0400 References: <1305671572-5899-1-git-send-email-jcmvbkbc@gmail.com> <1305671572-5899-24-git-send-email-jcmvbkbc@gmail.com> <4DD68C41.8080201@twiddle.net> In-Reply-To: <4DD68C41.8080201@twiddle.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201105210005.00438.jcmvbkbc@gmail.com> Subject: Re: [Qemu-devel] [PATCH 23/26] target-xtensa: implement interrupt option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: qemu-devel@nongnu.org > > + if (xtensa_option_enabled(env->config, XTENSA_OPTION_TIMER_INTERRUPT)) { > > + int i; > > + for (i = 0; i < env->config->nccompare; ++i) { > > + if (env->sregs[CCOMPARE + i] - old_ccount <= d) { > > + env->halted = 0; > > + xtensa_timer_irq(env, i, 1); > > I don't think you should be writing to halted here; this is done by > the code in cpu-exec.c, when noticing when cpu_has_work. Which will > be true as a function of env->interrupt_request and the interrupt mask. I do it here to distinguish interrupt caused by CCOMPARE match, for which I want exact CCOUNT value, from other interrupt sources, when CCOUNT may be advanced approximately. > > + if (env->halted) { > > + xtensa_advance_ccount(env, > > + muldiv64(qemu_get_clock_ns(vm_clock) - env->halt_clock, > > + env->config->clock_freq_khz, 1000000)); > > + } > > Why are you polling the vm_clock rather than setting up a timer? I'm not polling, I'm adjusting ccount according to vm_clock time passed since we were halted. > > + env->ccompare_timer = > > + qemu_new_timer_ns(vm_clock, &xtensa_ccompare_cb, env); > > ... er, actually you are setting up a timer. So why aren't you using it? I'm using it: it is wound up by HELPER(waiti) when there's no currently pending interrupts and xtensa_ccompare_cb calls xtensa_advance_ccount, which in turn calls xtensa_timer_irq. > > void do_interrupt(CPUState *env) > > { > > switch (env->exception_index) { > > + case EXC_IRQ: > > + if (handle_interrupt(env)) { > > + break; > > + } > > + /* not handled interrupt falls through, > > + * env->exception_index is updated > > + */ > > Do you really want to fall through, rather than restart the switch? Handle_interrupt will handle high-priority interrupt requests, leaving only level-1 interrupts, which it converts into EXC_USER or EXC_KERNEL. If only for the sake of readability... > > @@ -124,12 +198,16 @@ void do_interrupt(CPUState *env) > > if (env->config->exception_vector[env->exception_index]) { > > env->pc = env->config->exception_vector[env->exception_index]; > > env->exception_taken = 1; > > + env->interrupt_request |= CPU_INTERRUPT_EXITTB; > > Huh? What are you trying to accomplish here? > EXITTB is supposed to be used when a device external to the cpu > changes the memory mapping of the system. E.g. the x86 a20 line. I used it to have next_tb = 0 in cpu_exec, after return from do_interrupt, but now it is done unconditionally, so there's no need in CPU_INTERRUPT_EXITTB. By the way, do I understand it right that if I chain TBs than I need to periodically check for pending interrupts myself, otherwise e.g. "j $" will create uninterruptible infinite loop? > > +DEF_HELPER_0(check_interrupts, void) > > +DEF_HELPER_2(waiti, void, i32, i32) > > +DEF_HELPER_2(timer_irq, void, i32, i32) > > +DEF_HELPER_1(advance_ccount, void, i32) > > You shouldn't have to manage any of this from within the translator. Please explain. Thanks. -- Max