Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M
On 08/03/2024 15.00, Peter Maydell wrote: On Fri, 8 Mar 2024 at 12:54, Thomas Huth 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
Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M
On Fri, 8 Mar 2024 at 12:54, Thomas Huth 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
Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M
On 04/03/2024 16.22, Peter Maydell wrote: On Thu, 1 Feb 2024 at 19:12, Thomas Huth wrote: On 01/02/2024 15.19, Peter Maydell wrote: On Mon, 29 Jan 2024 at 08:18, Thomas Huth 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 --- 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
Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M
On Thu, 1 Feb 2024 at 19:12, Thomas Huth wrote: > > On 01/02/2024 15.19, Peter Maydell wrote: > > On Mon, 29 Jan 2024 at 08:18, Thomas Huth 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 > >> --- > >> 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
Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M
On 01/02/2024 15.19, Peter Maydell wrote: On Mon, 29 Jan 2024 at 08:18, Thomas Huth 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 --- 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
Re: [PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M
On Mon, 29 Jan 2024 at 08:18, Thomas Huth 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
> ---
> 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
[PATCH v2 2/3] target/arm/tcg/m_helper.c: Include the full helpers only with CONFIG_ARM_V7M
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
---
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
