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

[Thomas Huth]

On 01/02/2024 19.52, Thomas Huth wrote:
> 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?

 Hi Peter!

Any hints how to continue here? Respin the series with changes? Or is the 
current shape OK? Or are these changes rather unwanted and I should rather 
forget about these patches?

 Thomas