Re: [Qemu-devel] [PATCH v4 6/9] target-avr: adding helpers for IN, OUT, SLEEP, WBR & unsupported instructions

[Richard Henderson]

On 06/06/2016 03:37 AM, Michael Rolnik wrote:
> Signed-off-by: Michael Rolnik <address@hidden>
> ---
>  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~