Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M

[Thomas Huth]

On 04/03/2024 16.22, Peter Maydell wrote:
> On Thu, 1 Feb 2024 at 19:12, Thomas Huth <thuth@redhat.com> wrote:
> > On 01/02/2024 15.19, Peter Maydell wrote:
> > > On Mon, 29 Jan 2024 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
> > > > If CONFIG_ARM_V7M is not set, we don't want to include the full-fledged
> > > > helper functions that require additional functions for linking. The
> > > > reduced set of the linux-user functions works fine as stubs in this
> > > > case, so change the #ifdef statement accordingly.
> > > >
> > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > ---
> > > >    target/arm/tcg/m_helper.c | 3 ++-
> > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c
> > > > index d1f1e02acc..a5a6e96fc3 100644
> > > > --- a/target/arm/tcg/m_helper.c
> > > > +++ b/target/arm/tcg/m_helper.c
> > > > @@ -22,6 +22,7 @@
> > > >    #endif
> > > >    #if !defined(CONFIG_USER_ONLY)
> > > >    #include "hw/intc/armv7m_nvic.h"
> > > > +#include CONFIG_DEVICES
> > > >    #endif
> > > >
> > > >    static void v7m_msr_xpsr(CPUARMState *env, uint32_t mask,
> > > > @@ -69,7 +70,7 @@ uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t 
> > > > secure)
> > > >        return value;
> > > >    }
> > > >
> > > > -#ifdef CONFIG_USER_ONLY
> > > > +#if defined(CONFIG_USER_ONLY) || !defined(CONFIG_ARM_V7M)
> > >
> > > This looks a bit odd. If we don't have CONFIG_ARM_V7M
> > > why are we compiling this file at all?
> >
> > We'll get failures during linking otherwise. target/arm/helper.h still
> > defines code that requires the v7m_* helper functions:
> >
> > /usr/bin/ld: libqemu-arm-softmmu.fa.p/target_arm_tcg_translate.c.o:
> > qemu/target/arm/helper.h:76: undefined reference to `helper_v7m_vlldm'
> > /usr/bin/ld: libqemu-arm-softmmu.fa.p/target_arm_tcg_translate.c.o:
> > qemu/target/arm/helper.h:75: undefined reference to `helper_v7m_vlstm'
> > /usr/bin/ld: libqemu-arm-softmmu.fa.p/target_arm_tcg_translate.c.o:
> > qemu/target/arm/helper.h:73: undefined reference to
> > `helper_v7m_preserve_fp_state'
> >
> > etc.
>
> OK, but what we want in that case is either (a) avoid referring
> to the functions if we're not building for V7M (as you say,
> may be awkward) or else (b) stub versions of the functions that
> abort() if called. We don't really want the linux-user versions
> of the functions.
>
> Does having an m-helper_stub.c which we build if not CONFIG_ARM_V7M
> and having m_helper.c only built with CONFIG_ARM_V7M look
> feasible?

I gave it a try, but then we end up again with the problem that I already 
mentioned in the discussion about patch 1: CONFIG_ARM_V7M is not set for the 
linux-user binaries, so m_helper.c would not get included there anymore and 
we end up with lots of link failures.

So if you don't like the current shape, I guess this needs a little bit more 
pondering 'til it gets acceptable.

But could you maybe at least pick up the first patch already? ... since it's 
a patch with lots of code movement in it, this is quite ugly to rebase it 
each time someone touches some lines of code in that area...

 Thomas