Re: [PATCH v3 04/11] accel/tcg: Document cpu_exec_interrupt() callback contract

["Philippe Mathieu-Daudé" <philmd@oss.qualcomm.com>]

On 2026-09-08 18:25, Peter Maydell wrote:
> On Wed, 2 Sept 2026 at 16:21, Philippe Mathieu-Daudé
> <philmd@oss.qualcomm.com> wrote:
>>
>> Expand docstring to clarify that @interrupt_request is a bitmask,
>> the handler is called with BQL held, and document the return value
>> semantics. Fix typo in arm_cpu_exec_interrupt() documentation
>> (s/inrerrupt/interrupt).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
>> ---
>>   include/accel/tcg/cpu-ops.h | 15 ++++++++++++++-
>>   target/arm/internals.h      |  2 +-
>>   2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/accel/tcg/cpu-ops.h b/include/accel/tcg/cpu-ops.h
>> index ded7cc700df..87850402203 100644
>> --- a/include/accel/tcg/cpu-ops.h
>> +++ b/include/accel/tcg/cpu-ops.h
>> @@ -171,8 +171,21 @@ struct TCGCPUOps {
>>   #else
>>       /** @do_interrupt: Callback for interrupt handling.  */
>>       void (*do_interrupt)(CPUState *cpu);
>> -    /** @cpu_exec_interrupt: Callback for processing interrupts in cpu_exec */
>> +
>> +    /**
>> +     * @cpu_exec_interrupt: Callback for processing target-specific interrupts
>> +     * @cpu: cpu context
>> +     * @interrupt_request: Bitmask of pending interrupts
>> +     *
>> +     * Called from cpu_handle_interrupt() with the BQL held. Process the
>> +     * pending interrupt according to the target specific @interrupt_request
>> +     * masking rules.
> 
> I think we could clarify this a little to explain what to do with
> multiple pending interrupts. How about:
> 
> Identify which, if any, of the possibly multiple pending interrupts
> specified by @interrupt_request should be taken, using the target
> architecture's rules for interrupt prioritization and masking.
> If an interrupt can validly be taken, take it (by updating the
> emulated CPU state for an interrupt entry).

Does that update sound good to you?

* Called from cpu_handle_interrupt() with the BQL held. Identify which,
* if any, of the possibly multiple pending interrupts specified by
* @interrupt_request should be taken, using the target architecture's
* prioritization and masking rules. If an interrupt can be taken, update
* the emulated CPU state for interrupt entry.

With the update desc I wonder if we should rename the
helper as cpu_take_interrupt()...

> (I'm trying to get across that the function sets things up
> for execution of the interrupt handler as the next thing, it
> doesn't cause the whole interrupt handler to run. But maybe
> that's obvious enough we don't need to say so.)
> 
>> +     *
>> +     * Returns: %true if an interrupt was processed and the next TB should
>> +     * be restarted, %false to continue normal execution.
>> +     */
>>       bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request);
>> +
>>       /** @cpu_exec_reset: Callback for reset in cpu_exec.  */
>>       void (*cpu_exec_reset)(CPUState *cpu);
>>       /**
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index 1775835ad50..551a6b4a872 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -1248,7 +1248,7 @@ static inline const char *aarch32_mode_name(uint32_t psr)
>>   }
>>
>>   /**
>> - * arm_cpu_exec_interrupt(): Implementation of the cpu_exec_inrerrupt hook.
>> + * arm_cpu_exec_interrupt(): Implementation of the cpu_exec_interrupt hook.
>>    */
>>   bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request);
>>
> 
> -- PMM