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

[Thomas Huth]

On 01/02/2024 15.17, Peter Maydell wrote:
> 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.

The only user of this function are the arm_v7m_tcg_ops that are defined 
later in the cpu-v7m.c file, so I think it makes sense to keep it here.

> > 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?

cpu32.c gets added to the arm_ss source set which is linked into all 
possible arm-related binaries (qemu-system-aarch64, qemu-system-arm, 
qemu-aarch64 and qemu-arm).

The goal of this rework is to have the v7m code only linked into the 
binaries that need it, i.e. qemu-system-arm/aarch64 if CONFIG_ARM_V7M is 
set, or the 32-bit qemu-arm linux-user binary.

> What goes wrong if we add it to arm_ss() the way we do cpu32.c?

The problem is that CONFIG_ARM_V7M is not set for the linux-user builds, so 
the code does not get included in the "qemu-arm" binary anymore.
Now, the obvious answer to that statement is of course: Let's add 
CONFIG_ARM_V7M to configs/targets/arm-linux-user.mak, too! ... but I tried 
that already, and it also does not work, since then we'll suddenly try to 
include the hw/intc/armv7m_nvic.c stuff into the qemu-arm binary, which of 
course also does not work. It might be possible to rework that by moving 
armv7m_nvic.c from specific_ss to system_ss, but looks like that will 
require a *lot* of other reworks (e.g. arm_feature() is not available for 
common code). Another solution might be to move armv7m_nvic.c into the 
hw/arm/ directory and add it there to arm_ss instead ... it's then a little 
bit weird that this is the only interrupt controller there, but at least the 
changes would be quite trivial. What do you think?

 Thomas