Re: [PATCH v5 01/31] target/arm: Implement arm_v7m_cpu_has_work()

[Philippe Mathieu-Daudé]

On 9/21/21 11:34, Peter Maydell wrote:
> On Mon, 20 Sept 2021 at 22:44, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > Implement SysemuCPUOps::has_work() handler for the ARM v7M CPU.
> >
> > See the comments added in commit 7ecdaa4a963 ("armv7m: Fix
> > condition check for taking exceptions") which eventually
> > forgot to implement this has_work() handler:
>
> Huh? M-profile and A-profile share the same arm_cpu_has_work()
> function. Some of the checks the code there does are perhaps
> unnecessary for M-profile, but they're harmless.
>
> >    * ARMv7-M interrupt masking works differently than -A or -R.
> >    * There is no FIQ/IRQ distinction.
> >
> > The NVIC signal any pending interrupt by raising ARM_CPU_IRQ
> > (see commit 56b7c66f498: "armv7m: QOMify the armv7m container")
> > which ends setting the CPU_INTERRUPT_HARD bit in interrupt_request.
> >
> > Thus arm_v7m_cpu_has_work() implementation is thus quite trivial,
> > we simply need to check for this bit.
> >
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: Michael Davidsaver <mdavidsaver@gmail.com>
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >   target/arm/cpu_tcg.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
> > index 0d5adccf1a7..da348938407 100644
> > --- a/target/arm/cpu_tcg.c
> > +++ b/target/arm/cpu_tcg.c
> > @@ -23,6 +23,11 @@
> >   #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
> >
> >   #if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG)
> > +static bool arm_v7m_cpu_has_work(CPUState *cs)
> > +{
> > +    return cs->interrupt_request & CPU_INTERRUPT_HARD;
> > +}
>
> This seems to be missing at least the check on
> cpu->power_state and the CPU_INTERRUPT_EXITTB test.
>
> Is there any reason why we shouldn't just continue to
> share the same function between A and M profile, and avoid
> the extra function and the ifdefs ?

The only reason I can think of is I should have been resting
instead of posting this patch :/ I'll re-use arm_cpu_has_work()
which is, as you said, harmless and safer.