From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34028) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bA2XQ-0003k4-83 for qemu-devel@nongnu.org; Mon, 06 Jun 2016 17:58:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bA2XM-0008Ch-3v for qemu-devel@nongnu.org; Mon, 06 Jun 2016 17:58:23 -0400 Received: from mail-qg0-x241.google.com ([2607:f8b0:400d:c04::241]:33321) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bA2XL-0008CX-Ua for qemu-devel@nongnu.org; Mon, 06 Jun 2016 17:58:20 -0400 Received: by mail-qg0-x241.google.com with SMTP id p34so7788603qgp.0 for ; Mon, 06 Jun 2016 14:58:19 -0700 (PDT) Sender: Richard Henderson References: <1465209480-71364-1-git-send-email-rolnik@amazon.com> <1465209480-71364-7-git-send-email-rolnik@amazon.com> From: Richard Henderson Message-ID: Date: Mon, 6 Jun 2016 14:58:16 -0700 MIME-Version: 1.0 In-Reply-To: <1465209480-71364-7-git-send-email-rolnik@amazon.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 6/9] target-avr: adding helpers for IN, OUT, SLEEP, WBR & unsupported instructions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Rolnik , qemu-devel@nongnu.org Cc: Michael Rolnik On 06/06/2016 03:37 AM, Michael Rolnik wrote: > Signed-off-by: Michael Rolnik > --- > target-avr/helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++---- > target-avr/helper.h | 5 ++ > 2 files changed, 147 insertions(+), 11 deletions(-) > > diff --git a/target-avr/helper.c b/target-avr/helper.c > index e798dd9..f514ed6 100644 > --- a/target-avr/helper.c > +++ b/target-avr/helper.c > @@ -31,7 +31,7 @@ > > bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request) > { > - CPUClass *cc = CPU_GET_CLASS(cs); > + CPUClass *cc = CPU_GET_CLASS(cs); > AVRCPU *cpu = AVR_CPU(cs); > CPUAVRState *env = &cpu->env; > > @@ -49,7 +49,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request) > } > if (interrupt_request & CPU_INTERRUPT_HARD) { > if (cpu_interrupts_enabled(env) && env->intsrc != 0) { > - int index = ctz32(env->intsrc); > + int index = ctz32(env->intsrc); > cs->exception_index = EXCP_INT(index); > cc->do_interrupt(cs); > > @@ -64,18 +64,18 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request) > > void avr_cpu_do_interrupt(CPUState *cs) > { > - AVRCPU *cpu = AVR_CPU(cs); > - CPUAVRState *env = &cpu->env; > + AVRCPU *cpu = AVR_CPU(cs); > + CPUAVRState *env = &cpu->env; > > - uint32_t ret = env->pc; > - int vector; > - int size = avr_feature(env, AVR_FEATURE_JMP_CALL) ? 2 : 1; > - int base = 0; /* TODO: where to get it */ > + uint32_t ret = env->pc; > + int vector = 0; > + int size = avr_feature(env, AVR_FEATURE_JMP_CALL) ? 2 : 1; > + int base = 0; /* TODO: where to get it */ > > if (cs->exception_index == EXCP_RESET) { > - vector = 0; > + vector = 0; > } else if (env->intsrc != 0) { > - vector = ctz32(env->intsrc) + 1; > + vector = ctz32(env->intsrc) + 1; > } > > if (avr_feature(env, AVR_FEATURE_3_BYTE_PC)) { > @@ -90,7 +90,7 @@ void avr_cpu_do_interrupt(CPUState *cs) > } > > env->pc = base + vector * size; > - env->sregI = 0; /* clear Global Interrupt Flag */ > + env->sregI = 0; /* clear Global Interrupt Flag */ > > cs->exception_index = -1; > } Fold all of these whitespace changes into the original patch. > } > +void helper_sleep(CPUAVRState *env) Spacing. > +} > +void helper_unsupported(CPUAVRState *env) Spacing. > +{ > + CPUState *cs = CPU(avr_env_get_cpu(env)); > + > + cs->exception_index = EXCP_DEBUG; > + cpu_dump_state(cs, stderr, fprintf, 0); if (qemu_loglevel_mask(LOG_UNIMP)) { qemu_log("UNSUPPORTED\n"); cpu_dump_state(cs, qemu_logfile, fprintf, 0); } You also should be using something other than EXCP_DEBUG. > +void helper_wdr(CPUAVRState *env) > +{ > + CPUState *cs = CPU(avr_env_get_cpu(env)); > + > + cs->exception_index = EXCP_DEBUG; Likewise. > + sreg = (env->sregC & 0x01) << 0 > + | (env->sregZ & 0x01) << 1 > + | (env->sregN & 0x01) << 2 > + | (env->sregV & 0x01) << 3 > + | (env->sregS & 0x01) << 4 > + | (env->sregH & 0x01) << 5 > + | (env->sregT & 0x01) << 6 > + | (env->sregI & 0x01) << 7; Same bug with sregZ. > + case 0x3d: { /* SPL */ > + if (avr_feature(env, AVR_FEATURE_2_BYTE_SP)) { > + env->sp = (env->sp & 0xff00) | (data); > + } > + break; Surely you should write to the low byte of SP all the time. How else do you change SPL when you have a one-byte stack pointer? > + case 0x3f: { /* SREG */ > + env->sregC = (data >> 0) & 0x01; > + env->sregZ = (data >> 1) & 0x01; > + env->sregN = (data >> 2) & 0x01; > + env->sregV = (data >> 3) & 0x01; > + env->sregS = (data >> 4) & 0x01; > + env->sregH = (data >> 5) & 0x01; > + env->sregT = (data >> 6) & 0x01; > + env->sregI = (data >> 7) & 0x01; Same bug with sregZ. r~