Re: [Qemu-devel] [PATCH v2 08/23] target/arm: Move all v7m insn helpers into their own file

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 6/17/19 1:42 PM, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> From: Samuel Ortiz <sameo@linux.intel.com>
>>
>> In preparation for supporting TCG disablement on ARM, we move most
>> of TCG related v7m helpers and APIs into their own file.
>>
>> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
>> [PMD: Patch rewritten]
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Is there a way to not use $CONFIG_USER_ONLY?
> 
> Is this because the CONFIG_ARM_V7M symbol only appears for softmmu
> targets but we still want vXm -cpu's for user mode?

No :(

If I use this diff:

-- >8 --
diff --git a/target/arm/helper.h b/target/arm/helper.h
@@ -58,24 +58,26 @@ DEF_HELPER_2(pre_smc, void, env, i32)
 DEF_HELPER_1(check_breakpoints, void, env)

 DEF_HELPER_3(cpsr_write, void, env, i32, i32)
 DEF_HELPER_2(cpsr_write_eret, void, env, i32)
 DEF_HELPER_1(cpsr_read, i32, env)

+#ifndef CONFIG_USER_ONLY
 DEF_HELPER_3(v7m_msr, void, env, i32, i32)
 DEF_HELPER_2(v7m_mrs, i32, env, i32)

 DEF_HELPER_2(v7m_bxns, void, env, i32)
 DEF_HELPER_2(v7m_blxns, void, env, i32)

 DEF_HELPER_3(v7m_tt, i32, env, i32, i32)

 DEF_HELPER_1(v7m_preserve_fp_state, void, env)

 DEF_HELPER_2(v7m_vlstm, void, env, i32)
 DEF_HELPER_2(v7m_vlldm, void, env, i32)
+#endif /* CONFIG_USER_ONLY */

 DEF_HELPER_2(v8m_stackcheck, void, env, i32)
---

I get:

target/arm/translate.c:10607:29: error: nested extern declaration of
‘gen_helper_v7m_mrs’ [-Werror=nested-externs]
target/arm/translate.c: In function ‘disas_thumb_insn’:
target/arm/translate.c:11224:25: error: implicit declaration of function
‘gen_blxns’; did you mean ‘gen_bx’? [-Werror=implicit-function-declaration]
                         gen_blxns(s, rm);
                         ^~~~~~~~~

Because:

static void disas_thumb_insn(DisasContext *s, uint32_t insn)
{
    ...
    switch (insn >> 12) {
    ...
    case 4:
        ...
        if (insn & (1 << 10)) {
            ...
            case 3:
            {
                /* 0b0100_0111_xxxx_xxxx
                 * - branch [and link] exchange thumb register
                 */
                bool link = insn & (1 << 7);

                if (insn & 3) {
                    goto undef;
                }
                if (link) {
                    ARCH(5);
                }
                if ((insn & 4)) {
                    /* BXNS/BLXNS: only exists for v8M with the
                     * security extensions, and always UNDEF if NonSecure.
                     * We don't implement these in the user-only mode
                     * either (in theory you can use them from Secure User
                     * mode but they are too tied in to system emulation.)
                     */
                    if (!s->v8m_secure || IS_USER_ONLY) {
                        goto undef;
                    }
                    if (link) {
                        gen_blxns(s, rm);
                    } else {
                        gen_bxns(s, rm);
                    }
                    break;
                }

Should we add "#ifndef CONFIG_USER_ONLY" all around? I believe we rather
not...

For 'cps' and 'mrs' we have:

                        /* Implemented as NOP in user mode.  */
                        if (IS_USER(s))
                            break;

but not for 'msr'.