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

[Philippe Mathieu-Daudé]

Hi Peter,

On 9/21/21 11:45, Philippe Mathieu-Daudé wrote:
> 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.

A v7M core is registered as (example for Cortex-M0):

static const ARMCPUInfo arm_tcg_cpus[] = {
    ...
    { .name = "cortex-m0",   .initfn = cortex_m0_initfn,
                             .class_init = arm_v7m_class_init },


static void arm_tcg_cpu_register_types(void)
{
    ...
    for (i = 0; i < ARRAY_SIZE(arm_tcg_cpus); ++i) {
        arm_cpu_register(&arm_tcg_cpus[i]);


void arm_cpu_register(const ARMCPUInfo *info)
{
    TypeInfo type_info = {
        .parent = TYPE_ARM_CPU,
        .instance_size = sizeof(ARMCPU),
        .instance_align = __alignof__(ARMCPU),
        .instance_init = arm_cpu_instance_init,
        .class_size = sizeof(ARMCPUClass),
        .class_init = info->class_init ?: cpu_register_class_init,
        .class_data = (void *)info,
    };

Since we provide info->class_init as arm_v7m_class_init(), only
tcg_ops and gdb_core_xml_file from CPUClass are set:

static void arm_v7m_class_init(ObjectClass *oc, void *data)
{
    ARMCPUClass *acc = ARM_CPU_CLASS(oc);
    CPUClass *cc = CPU_CLASS(oc);

    acc->info = data;
#ifdef CONFIG_TCG
    cc->tcg_ops = &arm_v7m_tcg_ops;
#endif /* CONFIG_TCG */

    cc->gdb_core_xml_file = "arm-m-profile.xml";
}

Thus v7M cores end calling cpu_common_has_work() registered by
cpu_class_init(), which is:

static bool cpu_common_has_work(CPUState *cs)
{
    return false;
}

What am I missing?

> > >    * 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.