Message ID | 1439072952-14302-1-git-send-email-andrew.smirnov@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | Albert ARIBAUD |
Headers | show |
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@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot Amicalement,
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.
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.
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(-) -- 2.1.4