[U-Boot] [PATCH] semihosting: Improve ARMv7A support

[U-Boot] [PATCH] semihosting: Improve ARMv7A support

[Andrey Smirnov]

Previous implementation of semihosting didn't account for cases where
doing an SVC call would clobber various data(most notable case would
be 'lr' and 'spsr' when doing SVC call in Supervisor mode). This patch
is an adaptation of the code from Newlib's Angel_SWI feature (can be
found in libc/sys/arm/swi.h) which hopefuly fixes the problem.

Tested with modified OpenOCD and custom Vybrid VF610 based board

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/lib/semihosting.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/arch/arm/lib/semihosting.c b/arch/arm/lib/semihosting.c
index c3e964e..5f22c2d 100644
--- a/arch/arm/lib/semihosting.c
+++ b/arch/arm/lib/semihosting.c
@@ -26,18 +26,40 @@
 /*
  * Call the handler
  */
+#if defined(CONFIG_ARM64)
 static noinline long smh_trap(unsigned int sysnum, void *addr)
 {
 	register long result asm("r0");
-#if defined(CONFIG_ARM64)
 	asm volatile ("hlt #0xf000" : "=r" (result) : "0"(sysnum), "r"(addr));
+	return result;
+}
 #else
-	/* Note - untested placeholder */
-	asm volatile ("svc #0x123456" : "=r" (result) : "0"(sysnum), "r"(addr));
+#if defined (CONFIG_SYS_THUMB_BUILD)
+#error "Support for semihosting in THUMB mode is not implemented"
 #endif
+/*
+ * Call the handler
+ */
+static noinline long smh_trap(unsigned int sysnum, void *addr)
+{
+	int result;
+
+	asm volatile ("mov r0, %1"	"\n\t"
+		      "mov r1, %2"	"\n\t"
+		      "svc #0x123456"	"\n\t"
+		      "mov %0, r0"	"\n\t"
+		      : "=r" (result)
+		      : "r" (sysnum), "r" (addr)
+		      : "r0", "r1", "r2", "r3", "ip", "lr", "memory", "cc");
+		      /* Clobbers r0 and r1, and lr if in supervisor
+			 mode Accordingly to page 13-77 of ARM DUI
+			 0040D other registers can also be clobbered.
+			 Some memory positions may also be changed by
+			 a system call, so they should not be kept in
+			 registers.  */
 	return result;
 }
-
+#endif
 /*
  * Open a file on the host. Mode is "r" or "rb" currently. Returns a file
  * descriptor or -1 on error.
--
2.1.4

[U-Boot] [PATCH] semihosting: Improve ARMv7A support

[Albert ARIBAUD]

Hello Andrey,

Two nitpicks:

On Sat,  8 Aug 2015 15:29:12 -0700, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> Previous implementation of semihosting didn't account for cases where
> doing an SVC call would clobber various data(most notable case would
> be 'lr' and 'spsr' when doing SVC call in Supervisor mode). This patch
> is an adaptation of the code from Newlib's Angel_SWI feature (can be
> found in libc/sys/arm/swi.h) which hopefuly fixes the problem.
> 
> Tested with modified OpenOCD and custom Vybrid VF610 based board
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  arch/arm/lib/semihosting.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/lib/semihosting.c b/arch/arm/lib/semihosting.c
> index c3e964e..5f22c2d 100644
> --- a/arch/arm/lib/semihosting.c
> +++ b/arch/arm/lib/semihosting.c
> @@ -26,18 +26,40 @@
>  /*
>   * Call the handler
>   */
> +#if defined(CONFIG_ARM64)
>  static noinline long smh_trap(unsigned int sysnum, void *addr)
>  {
>  	register long result asm("r0");
> -#if defined(CONFIG_ARM64)

What's the point of moving the #ifdef up? It just forces you to
duplicate the function start with no discernable benefit since the
function name and arguments are the same in the 64 and 32 bit case.

>  	asm volatile ("hlt #0xf000" : "=r" (result) : "0"(sysnum), "r"(addr));
> +	return result;
> +}
>  #else
> -	/* Note - untested placeholder */
> -	asm volatile ("svc #0x123456" : "=r" (result) : "0"(sysnum), "r"(addr));
> +#if defined (CONFIG_SYS_THUMB_BUILD)
> +#error "Support for semihosting in THUMB mode is not implemented"
>  #endif
> +/*
> + * Call the handler
> + */

No point in duplicating this comment.

> +static noinline long smh_trap(unsigned int sysnum, void *addr)
> +{
> +	int result;
> +
> +	asm volatile ("mov r0, %1"	"\n\t"
> +		      "mov r1, %2"	"\n\t"
> +		      "svc #0x123456"	"\n\t"
> +		      "mov %0, r0"	"\n\t"
> +		      : "=r" (result)
> +		      : "r" (sysnum), "r" (addr)
> +		      : "r0", "r1", "r2", "r3", "ip", "lr", "memory", "cc");
> +		      /* Clobbers r0 and r1, and lr if in supervisor
> +			 mode Accordingly to page 13-77 of ARM DUI
> +			 0040D other registers can also be clobbered.
> +			 Some memory positions may also be changed by
> +			 a system call, so they should not be kept in
> +			 registers.  */
>  	return result;
>  }
> -
> +#endif
>  /*
>   * Open a file on the host. Mode is "r" or "rb" currently. Returns a file
>   * descriptor or -1 on error.
> --
> 2.1.4
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



Amicalement,
-- 
Albert.

[U-Boot] [PATCH] semihosting: Improve ARMv7A support

[Andrey Smirnov]

On Sat, Sep 12, 2015 at 12:12 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hello Andrey,
>
> Two nitpicks:
>
>> +#if defined(CONFIG_ARM64)
>>  static noinline long smh_trap(unsigned int sysnum, void *addr)
>>  {
>>       register long result asm("r0");
>> -#if defined(CONFIG_ARM64)
>
> What's the point of moving the #ifdef up? It just forces you to
> duplicate the function start with no discernable benefit since the
> function name and arguments are the same in the 64 and 32 bit case.
>

The point is to improve readability of the code. With #ifdef moved up
you have to figure out the version of the code you care about first
and then you can read the entirety of the function and reason about
it. With #ifdefs inside of the body of the function, every time you
read it you have to filter out all of the "irrelevant junk" that
compiler would not even see on your platform.