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

[Peter Maydell]

On Thu, 1 Feb 2024 at 18:52, Thomas Huth <thuth@redhat.com> wrote:
>
> On 01/02/2024 15.17, Peter Maydell wrote:
> > On Mon, 29 Jan 2024 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
> >> 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?

The NVIC is in some sense part of the CPU, but I think I'd rather
not move it. (You'll also find that setting CONFIG_ARM_V7M pulls
in code in hw/misc (RAS handling) and hw/timer (systick timer), so
it's not just hw/intc code that's affected here.)

I guess this is kind of happening because we have one symbol
CONFIG_ARM_V7M and we're using it both for "we want the v7M
system components like the NVIC and timer" and for "we want
the v7M core CPU support". So maybe another approach here would
be to have CONFIG_ARM_V7M and CONFIG_ARM_V7M_CPU or something,
where linux-user selects just the latter. But unless you think
that approach would work out more cleanly, I'm OK with how this
patch does things.

(Sorry for not getting back to this series for so long.)

thanks
-- PMM