Re: [PATCH v2 2/7] target/arm: add ARMCPUClass->do_interrupt_locked

[Richard Henderson]

On 8/31/20 2:18 PM, Richard Henderson wrote:
> On 8/19/20 11:28 AM, Robert Foley wrote:
>> Adding ->do_interrupt_locked to ARMCPUClass is preparation for
>> pushing the BQL down into the per-arch implementation of ->do_interrupt.
>>
>> This is needed since ARM's *_cpu_exec_interrupt calls to *_do_interrupt.
>> With the push down of the BQL into *_cpu_exec_interrupt and
>> *_do_interrupt, *_cpu_exec_interrupt will call to ->do_interrupt
>> with lock held.  Since ->do_interrupt also has the lock, we need a way
>> to allow cpu_exec_interrupt to call do_interrupt with lock held.
>> This patch solves this issue of *_cpu_exec_interrupt needing
>> to call do_interrupt with lock held.
>>
>> This patch is part of a series of transitions to move the
>> BQL down into the do_interrupt per arch functions.  This set of
>> transitions is needed to maintain bisectability.
>>
>> This approach was suggested by Paolo Bonzini.
>> For reference, here are two key posts in the discussion, explaining
>> the reasoning/benefits of this approach.
>> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00784.html
>> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg01517.html
>> https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
>> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html
>>
>> Signed-off-by: Robert Foley <robert.foley@linaro.org>
>> ---
>>  target/arm/cpu-qom.h | 3 +++
>>  target/arm/cpu.c     | 5 +++--
>>  target/arm/cpu_tcg.c | 5 +++--
>>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

I take it back.  These two cc->do_interrupt calls can be replaced with direct
calls.

> #ifndef CONFIG_USER_ONLY
>     cc->do_interrupt = arm_v7m_cpu_do_interrupt;
> #endif
> 
>     cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;

If we are in arm_v7m_cpu_exec_interrupt we will always call
arm_v7m_cpu_do_interrupt.

I think the mismatch of #ifdef, which implies a different destination is
possible, is a bug -- cc->do_interrupt is not otherwise assigned and in fact
would be NULL.

I suspect that some of these slots themselves should be ifdefed, so that we
cannot assign to them when they are unused.  That would help keep the ifdefs in
the cpu init functions in sync.

This same condition is *not* true for cris -- there is no
crisv10_cpu_exec_interrupt -- so you do need the new do_interrupt_locked field
there.


r~