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

[Thomas Huth <thuth@redhat.com>]

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