Re: [Qemu-devel] [PATCH 23/26] target-xtensa: implement interrupt option

[Richard Henderson <rth@twiddle.net>]

On 05/17/2011 03:32 PM, Max Filippov wrote:
> +    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.


> +            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?

> +        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?

>  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?

> @@ -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.

> +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.


r~