Re: [PATCH v2 1/3] target/arm: Move v7m-related code from cpu32.c into a separate file

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 29 Jan 2024 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
>
> Move the code to a separate file so that we do not have to compile
> it anymore if CONFIG_ARM_V7M is not set.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/arm/tcg/cpu-v7m.c   | 290 +++++++++++++++++++++++++++++++++++++
>  target/arm/tcg/cpu32.c     | 261 ---------------------------------
>  target/arm/meson.build     |   3 +
>  target/arm/tcg/meson.build |   3 +
>  4 files changed, 296 insertions(+), 261 deletions(-)
>  create mode 100644 target/arm/tcg/cpu-v7m.c
>
> diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c
> new file mode 100644
> index 0000000000..89a25444a2
> --- /dev/null
> +++ b/target/arm/tcg/cpu-v7m.c
> @@ -0,0 +1,290 @@
> +/*
> + * QEMU ARMv7-M TCG-only CPUs.
> + *
> + * Copyright (c) 2012 SUSE LINUX Products GmbH
> + *
> + * This code is licensed under the GNU GPL v2 or later.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "hw/core/tcg-cpu-ops.h"
> +#include "internals.h"
> +
> +#if !defined(CONFIG_USER_ONLY)
> +
> +#include "hw/intc/armv7m_nvic.h"
> +
> +static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    bool ret = false;
> +
> +    /*
> +     * ARMv7-M interrupt masking works differently than -A or -R.
> +     * There is no FIQ/IRQ distinction. Instead of I and F bits
> +     * masking FIQ and IRQ interrupts, an exception is taken only
> +     * if it is higher priority than the current execution priority
> +     * (which depends on state like BASEPRI, FAULTMASK and the
> +     * currently active exception).
> +     */
> +    if (interrupt_request & CPU_INTERRUPT_HARD
> +        && (armv7m_nvic_can_take_pending_exception(env->nvic))) {
> +        cs->exception_index = EXCP_IRQ;
> +        cc->tcg_ops->do_interrupt(cs);
> +        ret = true;
> +    }
> +    return ret;
> +}
> +
> +#endif /* !CONFIG_USER_ONLY */

I wonder if this function could go in target/arm/tcg/m_helper.c:
it looks a bit odd in this file, which is mostly initfns for
specific CPU types. But it was in cpu32.c so I'm happy that
we just move it to cpu-v7m.c for now.

> diff --git a/target/arm/meson.build b/target/arm/meson.build
> index 46b5a21eb3..2e10464dbb 100644
> --- a/target/arm/meson.build
> +++ b/target/arm/meson.build
> @@ -26,6 +26,8 @@ arm_system_ss.add(files(
>    'ptw.c',
>  ))
>
> +arm_user_ss = ss.source_set()
> +
>  subdir('hvf')
>
>  if 'CONFIG_TCG' in config_all_accel
> @@ -36,3 +38,4 @@ endif
>
>  target_arch += {'arm': arm_ss}
>  target_system_arch += {'arm': arm_system_ss}
> +target_user_arch += {'arm': arm_user_ss}
> diff --git a/target/arm/tcg/meson.build b/target/arm/tcg/meson.build
> index 6fca38f2cc..3b1a9f0fc5 100644
> --- a/target/arm/tcg/meson.build
> +++ b/target/arm/tcg/meson.build
> @@ -55,3 +55,6 @@ arm_ss.add(when: 'TARGET_AARCH64', if_true: files(
>  arm_system_ss.add(files(
>    'psci.c',
>  ))
> +
> +arm_system_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('cpu-v7m.c'))
> +arm_user_ss.add(when: 'TARGET_AARCH64', if_false: files('cpu-v7m.c'))

Why do we need to add this new arm_user_ss() sourceset,
when we didn't need it for the A/R profile CPUs?
What goes wrong if we add it to arm_ss() the way we do cpu32.c?

-- PMM