On 08/08/20 00:18, Robert Foley wrote:
> 2) Another perhaps cleaner option is to add a new cpu class function
> ->do_interrupt_locked.
> This lets callers like *_cpu_exec_interrupt call to ->do_interrupt_locked
> with lock held and solves the issue without resorting to conditional
> locking.
>
> Another benefit we could gain from this approach is to simplify our
> solution
> overall by adding a common do_interrupt function.
>
> void cpu_common_do_interrupt(CPUState *cs)
> {
> CPUClass *cc = CPU_GET_CLASS(cpu);
> qemu_mutex_lock_iothread();
> cc->do_interrupt_locked(cpu);
> qemu_mutex_unlock_iothread();
> }
> cc->do_interrupt would be set to cpu_common_do_interrupt by default
> in cpu_class_init.
> In other words, the base cpu class would handle holding the BQL for us,
> and we would not need to implement a new *_do_interrupt function
> for each arch.
>
> We are thinking that 2) would be a good option.
Yes, it is. The only slight complication is that you'd have both
->do_interrupt and ->do_interrupt_locked so you probably should add some
consistency check, for example
/*
* cc->do_interrupt_locked should only be needed if
* the class uses cpu_common_do_interrupt.
*/
assert(cc->do_interrupt == cpu_common_do_interrupt ||
!cc->do_interrupt_locked);
Therefore, a variant is to add ->do_interrupt_locked to ARMCPUClass and
CRISCPUClass (target/avr/helper.c can just call
avr_cpu_do_interrupt_locked, because that's the only value that
cc->do_interrupt can have). Then ARM and CRIS can have a do_interrupt
like you wrote above:
void arm_do_interrupt(CPUState *cs)
{
ARMCPUClass *acc = ARM_CPU_GET_CLASS(cs);
qemu_mutex_lock_iothread();
acc->do_interrupt_locked(cpu);
qemu_mutex_unlock_iothread();
}
with a small duplication between ARM and CRIS but on the other hand a
simpler definition of the common CPUClass.
Paolo
