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

[Max Filippov <jcmvbkbc@gmail.com>]

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