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)
 
 void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
 {
-- 
2.43.0

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?

>
>  void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>  {

thanks
-- PMM

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.

And then other files are depending on these helpers, too, e.g. 
target/arm/tcg/translate-vfp.c calls gen_helper_v7m_preserve_fp_state() etc. 
... I guess it might be possible to disable all those spots with some 
#ifdefs, too, but it's getting quite ugly that way. It's way nicer to use 
the stub functions that we have in m_helper already anyway.

  Thomas

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?

thanks
-- PMM

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

On Fri, 8 Mar 2024 at 12:54, Thomas Huth <thuth@redhat.com> wrote:
> 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...

I don't object to taking the first patch, but... it doesn't apply
so it needs rebasing :-/ If you can rebase and send it out this
afternoon I can put it in a pullreq I'm working on.

-- PMM

On 08/03/2024 15.00, Peter Maydell wrote:
> On Fri, 8 Mar 2024 at 12:54, Thomas Huth <thuth@redhat.com> wrote:
>> 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...
> 
> I don't object to taking the first patch, but... it doesn't apply
> so it needs rebasing :-/ If you can rebase and send it out this
> afternoon I can put it in a pullreq I'm working on.

That's what I meant with it's "ugly to rebase it" ;-) ... fortunately, it 
was only a rather simple conflict this time.

I sent out a v3 now, also including another try for the second patch (which 
now includes the stubs that assert() in the m_helper.c file itself instead 
of using a separate stub file). Not sure whether that looks much nicer, but 
at least it seems to work from a compilation/linking perspective. Anyway, if 
you could at least include patch 1 from that v3 in your next pull request, 
that would be great!

  Thanks,
   Thomas