[Qemu-devel] [PATCH 0/2] ARM: fix commandline handling for semihosted executables

[Qemu-devel] [PATCH 0/2] ARM: fix commandline handling for semihosted executables

[Peter Maydell]

This patchset fixes the commandline handling for ARM semihosted executables
run under linux-user mode, and cleans up the resulting unused variable in
the linux-user image_info struct. The code is all by Wolfgang but he is
having difficulty sending properly formatted patches to the list so I am
retransmitting them (with my Reviewed-by: signoff).

Wolfgang Schildbach (2):
  Fix commandline handling for ARM semihosted executables
  Remove dead code for ARM semihosting commandline handling

 arm-semi.c             |   79 +++++++++++++++++++++++++++++------------------
 bsd-user/bsdload.c     |    2 -
 bsd-user/qemu.h        |    1 -
 linux-user/linuxload.c |    2 -
 linux-user/qemu.h      |    1 -
 5 files changed, 49 insertions(+), 36 deletions(-)

[Qemu-devel] [PATCH 1/2] Fix commandline handling for ARM semihosted executables

[Peter Maydell]

From: Wolfgang Schildbach <wschi@dolby.com>

Use the copy of the command line that loader_build_argptr() sets up in guest
memory as the command line to return from the ARM SYS_GET_CMDLINE semihosting
call. Previously we were using a pointer to memory which had already been
freed before the guest program started.

This fixes https://bugs.launchpad.net/qemu/+bug/673613 .

Signed-off-by: Wolfgang Schildbach <wschi@dolby.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 arm-semi.c |   79 +++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/arm-semi.c b/arm-semi.c
index 0687b03..1d5179b 100644
--- a/arm-semi.c
+++ b/arm-semi.c
@@ -373,45 +373,64 @@ uint32_t do_arm_semihosting(CPUState *env)
 #ifdef CONFIG_USER_ONLY
         /* Build a commandline from the original argv.  */
         {
-            char **arg = ts->info->host_argv;
-            int len = ARG(1);
-            /* lock the buffer on the ARM side */
-            char *cmdline_buffer = (char*)lock_user(VERIFY_WRITE, ARG(0), len, 0);
+            char *arm_cmdline_buffer;
+            const char *host_cmdline_buffer;
 
-            if (!cmdline_buffer)
-                /* FIXME - should this error code be -TARGET_EFAULT ? */
-                return (uint32_t)-1;
+            unsigned int i;
+            unsigned int arm_cmdline_len = ARG(1);
+            unsigned int host_cmdline_len =
+                ts->info->arg_end-ts->info->arg_start;
+
+            if (!arm_cmdline_len || host_cmdline_len > arm_cmdline_len) {
+                return -1; /* not enough space to store command line */
+            }
 
-            s = cmdline_buffer;
-            while (*arg && len > 2) {
-                int n = strlen(*arg);
+            if (!host_cmdline_len) {
+                /* We special-case the "empty command line" case (argc==0).
+                   Just provide the terminating 0. */
+                arm_cmdline_buffer = lock_user(VERIFY_WRITE, ARG(0), 1, 0);
+                arm_cmdline_buffer[0] = 0;
+                unlock_user(arm_cmdline_buffer, ARG(0), 1);
 
-                if (s != cmdline_buffer) {
-                    *(s++) = ' ';
-                    len--;
-                }
-                if (n >= len)
-                    n = len - 1;
-                memcpy(s, *arg, n);
-                s += n;
-                len -= n;
-                arg++;
+                /* Adjust the commandline length argument. */
+                SET_ARG(1, 0);
+                return 0;
             }
-            /* Null terminate the string.  */
-            *s = 0;
-            len = s - cmdline_buffer;
 
-            /* Unlock the buffer on the ARM side.  */
-            unlock_user(cmdline_buffer, ARG(0), len);
+            /* lock the buffers on the ARM side */
+            arm_cmdline_buffer =
+                lock_user(VERIFY_WRITE, ARG(0), host_cmdline_len, 0);
+            host_cmdline_buffer =
+                lock_user(VERIFY_READ, ts->info->arg_start,
+                                       host_cmdline_len, 1);
 
-            /* Adjust the commandline length argument.  */
-            SET_ARG(1, len);
+            if (arm_cmdline_buffer && host_cmdline_buffer)
+            {
+                /* the last argument is zero-terminated;
+                   no need for additional termination */
+                memcpy(arm_cmdline_buffer, host_cmdline_buffer,
+                       host_cmdline_len);
 
-            /* Return success if commandline fit into buffer.  */
-            return *arg ? -1 : 0;
+                /* separate arguments by white spaces */
+                for (i = 0; i < host_cmdline_len-1; i++) {
+                    if (arm_cmdline_buffer[i] == 0) {
+                        arm_cmdline_buffer[i] = ' ';
+                    }
+                }
+
+                /* Adjust the commandline length argument. */
+                SET_ARG(1, host_cmdline_len-1);
+            }
+
+            /* Unlock the buffers on the ARM side.  */
+            unlock_user(arm_cmdline_buffer, ARG(0), host_cmdline_len);
+            unlock_user((void*)host_cmdline_buffer, ts->info->arg_start, 0);
+
+            /* Return success if we could return a commandline.  */
+            return (arm_cmdline_buffer && host_cmdline_buffer) ? 0 : -1;
         }
 #else
-      return -1;
+        return -1;
 #endif
     case SYS_HEAPINFO:
         {
-- 
1.6.3.3

[Qemu-devel] [PATCH 2/2] Remove dead code for ARM semihosting commandline handling

[Peter Maydell]

From: Wolfgang Schildbach <wschi@dolby.com>

There are some bits in the code which were used to store the commandline for
the semihosting call. These bits are now write-only and can be removed.

Signed-off-by: Wolfgang Schildbach <wschi@dolby.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 bsd-user/bsdload.c     |    2 --
 bsd-user/qemu.h        |    1 -
 linux-user/linuxload.c |    2 --
 linux-user/qemu.h      |    1 -
 4 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/bsd-user/bsdload.c b/bsd-user/bsdload.c
index 14a93bf..6d9bb6f 100644
--- a/bsd-user/bsdload.c
+++ b/bsd-user/bsdload.c
@@ -176,8 +176,6 @@ int loader_exec(const char * filename, char ** argv, char ** envp,
 
     retval = prepare_binprm(&bprm);
 
-    infop->host_argv = argv;
-
     if(retval>=0) {
         if (bprm.buf[0] == 0x7f
                 && bprm.buf[1] == 'E'
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 9763616..e343894 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -50,7 +50,6 @@ struct image_info {
     abi_ulong entry;
     abi_ulong code_offset;
     abi_ulong data_offset;
-    char      **host_argv;
     int       personality;
 };
 
diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c
index 9ee27c3..ac8c486 100644
--- a/linux-user/linuxload.c
+++ b/linux-user/linuxload.c
@@ -174,8 +174,6 @@ int loader_exec(const char * filename, char ** argv, char ** envp,
 
     retval = prepare_binprm(bprm);
 
-    infop->host_argv = argv;
-
     if(retval>=0) {
         if (bprm->buf[0] == 0x7f
                 && bprm->buf[1] == 'E'
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index e66a02b..32de241 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -50,7 +50,6 @@ struct image_info {
         abi_ulong       saved_auxv;
         abi_ulong       arg_start;
         abi_ulong       arg_end;
-        char            **host_argv;
 	int		personality;
 };
 
-- 
1.6.3.3

RE: [Qemu-devel] [PATCH 0/2] ARM: fix commandline handling forsemihosted executables

[Schildbach, Wolfgang]

Ping -- I have not seen any corresponding commit in the qemu master
repository. Anthony, could you please consider this for inclusion into
0.14?

Thanks,
- Wolfgang


-----Original Message-----
From: qemu-devel-bounces+wschi=dolby.com@nongnu.org
[mailto:qemu-devel-bounces+wschi=dolby.com@nongnu.org] On Behalf Of
Peter Maydell
Sent: Monday, December 06, 2010 4:06 PM
To: qemu-devel@nongnu.org
Cc: Riku Voipio; Schildbach, Wolfgang; Nathan Froyd
Subject: [Qemu-devel] [PATCH 0/2] ARM: fix commandline handling
forsemihosted executables

This patchset fixes the commandline handling for ARM semihosted
executables run under linux-user mode, and cleans up the resulting
unused variable in the linux-user image_info struct. The code is all by
Wolfgang but he is having difficulty sending properly formatted patches
to the list so I am retransmitting them (with my Reviewed-by: signoff).

Wolfgang Schildbach (2):
  Fix commandline handling for ARM semihosted executables
  Remove dead code for ARM semihosting commandline handling

 arm-semi.c             |   79
+++++++++++++++++++++++++++++------------------
 bsd-user/bsdload.c     |    2 -
 bsd-user/qemu.h        |    1 -
 linux-user/linuxload.c |    2 -
 linux-user/qemu.h      |    1 -
 5 files changed, 49 insertions(+), 36 deletions(-)

Re: [Qemu-devel] [PATCH 0/2] ARM: fix commandline handling forsemihosted executables

[Peter Maydell]

Riku, did you want to pick this one up as a linux-user patch? Otherwise
it's on my list of "patches which got no comments and will go into the
next arm pull request" -- although that won't be until early next year
and I expect 0.14 will have branched by then.

-- PMM

On 23 December 2010 12:58, Schildbach, Wolfgang <WSCHI@dolby.com> wrote:
> Ping -- I have not seen any corresponding commit in the qemu master
> repository. Anthony, could you please consider this for inclusion into
> 0.14?
>
> Thanks,
> - Wolfgang
>
>
> -----Original Message-----
> From: qemu-devel-bounces+wschi=dolby.com@nongnu.org
> [mailto:qemu-devel-bounces+wschi=dolby.com@nongnu.org] On Behalf Of
> Peter Maydell
> Sent: Monday, December 06, 2010 4:06 PM
> To: qemu-devel@nongnu.org
> Cc: Riku Voipio; Schildbach, Wolfgang; Nathan Froyd
> Subject: [Qemu-devel] [PATCH 0/2] ARM: fix commandline handling
> forsemihosted executables
>
> This patchset fixes the commandline handling for ARM semihosted
> executables run under linux-user mode, and cleans up the resulting
> unused variable in the linux-user image_info struct. The code is all by
> Wolfgang but he is having difficulty sending properly formatted patches
> to the list so I am retransmitting them (with my Reviewed-by: signoff).
>
> Wolfgang Schildbach (2):
>  Fix commandline handling for ARM semihosted executables
>  Remove dead code for ARM semihosting commandline handling
>
>  arm-semi.c             |   79
> +++++++++++++++++++++++++++++------------------
>  bsd-user/bsdload.c     |    2 -
>  bsd-user/qemu.h        |    1 -
>  linux-user/linuxload.c |    2 -
>  linux-user/qemu.h      |    1 -
>  5 files changed, 49 insertions(+), 36 deletions(-)
>
>
>

Re: [Qemu-devel] [PATCH 0/2] ARM: fix commandline handling forsemihosted executables

[riku voipio]

Hi,

Honestly, I have never tried ARM semihosting. Do you have some 
recommended test instructions?

Riku

On 12/23/2010 04:09 PM, Peter Maydell wrote:
> Riku, did you want to pick this one up as a linux-user patch? Otherwise
> it's on my list of "patches which got no comments and will go into the
> next arm pull request" -- although that won't be until early next year
> and I expect 0.14 will have branched by then.
>
> -- PMM
>
> On 23 December 2010 12:58, Schildbach, Wolfgang<WSCHI@dolby.com>  wrote:
>> Ping -- I have not seen any corresponding commit in the qemu master
>> repository. Anthony, could you please consider this for inclusion into
>> 0.14?
>>
>> Thanks,
>> - Wolfgang
>>
>>
>> -----Original Message-----
>> From: qemu-devel-bounces+wschi=dolby.com@nongnu.org
>> [mailto:qemu-devel-bounces+wschi=dolby.com@nongnu.org] On Behalf Of
>> Peter Maydell
>> Sent: Monday, December 06, 2010 4:06 PM
>> To: qemu-devel@nongnu.org
>> Cc: Riku Voipio; Schildbach, Wolfgang; Nathan Froyd
>> Subject: [Qemu-devel] [PATCH 0/2] ARM: fix commandline handling
>> forsemihosted executables
>>
>> This patchset fixes the commandline handling for ARM semihosted
>> executables run under linux-user mode, and cleans up the resulting
>> unused variable in the linux-user image_info struct. The code is all by
>> Wolfgang but he is having difficulty sending properly formatted patches
>> to the list so I am retransmitting them (with my Reviewed-by: signoff).
>>
>> Wolfgang Schildbach (2):
>>   Fix commandline handling for ARM semihosted executables
>>   Remove dead code for ARM semihosting commandline handling
>>
>>   arm-semi.c             |   79
>> +++++++++++++++++++++++++++++------------------
>>   bsd-user/bsdload.c     |    2 -
>>   bsd-user/qemu.h        |    1 -
>>   linux-user/linuxload.c |    2 -
>>   linux-user/qemu.h      |    1 -
>>   5 files changed, 49 insertions(+), 36 deletions(-)
>>
>>
>>

RE: [Qemu-devel] [PATCH 0/2] ARM: fix commandline handling forsemihosted executables

[Schildbach, Wolfgang]

Hi Riku, 

I usually compile a hello world (echoing its arguments) with rvds2.2 or
above:

#include <stdio.h>

int main(int ac, char *av[])
{
  int i;
  printf("Hello world\n");
  for (i=0; i<ac; i++)
    printf("'%s'\n", av[i]);
  return 0;
}

> armcc -o hello -L--sysv hello.c

I configure qemu like this:

> ./configure '--prefix=/data/project/qemu/latest'
'--target-list=arm-linux-user'
> gmake install

...and execute it, checking whether the execution echoes back the
arguments:

> /data/project/qemu/latest/bin/qemu-arm hello a
"hello"
"a"

If you don't have an ARM compiler available, I could send you the
compiled binary to test. Peter, is there another way to generate
binaries using semihosting?

Best regards,
- Wolfgang
 

-----Original Message-----
From: riku voipio [mailto:riku.voipio@iki.fi] 
Sent: Friday, January 07, 2011 4:34 PM
To: Peter Maydell
Cc: Schildbach, Wolfgang; qemu-devel@nongnu.org; Nathan Froyd
Subject: Re: [Qemu-devel] [PATCH 0/2] ARM: fix commandline handling
forsemihosted executables

Hi,

Honestly, I have never tried ARM semihosting. Do you have some
recommended test instructions?

Riku

On 12/23/2010 04:09 PM, Peter Maydell wrote:
> Riku, did you want to pick this one up as a linux-user patch? 
> Otherwise it's on my list of "patches which got no comments and will 
> go into the next arm pull request" -- although that won't be until 
> early next year and I expect 0.14 will have branched by then.
>
> -- PMM
>
> On 23 December 2010 12:58, Schildbach, Wolfgang<WSCHI@dolby.com>
wrote:
>> Ping -- I have not seen any corresponding commit in the qemu master 
>> repository. Anthony, could you please consider this for inclusion 
>> into 0.14?
>>
>> Thanks,
>> - Wolfgang
>>
>>
>> -----Original Message-----
>> From: qemu-devel-bounces+wschi=dolby.com@nongnu.org
>> [mailto:qemu-devel-bounces+wschi=dolby.com@nongnu.org] On Behalf Of 
>> Peter Maydell
>> Sent: Monday, December 06, 2010 4:06 PM
>> To: qemu-devel@nongnu.org
>> Cc: Riku Voipio; Schildbach, Wolfgang; Nathan Froyd
>> Subject: [Qemu-devel] [PATCH 0/2] ARM: fix commandline handling 
>> forsemihosted executables
>>
>> This patchset fixes the commandline handling for ARM semihosted 
>> executables run under linux-user mode, and cleans up the resulting 
>> unused variable in the linux-user image_info struct. The code is all 
>> by Wolfgang but he is having difficulty sending properly formatted 
>> patches to the list so I am retransmitting them (with my Reviewed-by:
signoff).
>>
>> Wolfgang Schildbach (2):
>>   Fix commandline handling for ARM semihosted executables
>>   Remove dead code for ARM semihosting commandline handling
>>
>>   arm-semi.c             |   79
>> +++++++++++++++++++++++++++++------------------
>>   bsd-user/bsdload.c     |    2 -
>>   bsd-user/qemu.h        |    1 -
>>   linux-user/linuxload.c |    2 -
>>   linux-user/qemu.h      |    1 -
>>   5 files changed, 49 insertions(+), 36 deletions(-)
>>
>>
>>

Re: [Qemu-devel] [PATCH 0/2] ARM: fix commandline handling forsemihosted executables

[Peter Maydell]

On 7 January 2011 15:46, Schildbach, Wolfgang <WSCHI@dolby.com> wrote:
> I usually compile a hello world (echoing its arguments) with rvds2.2 or
> above:

> If you don't have an ARM compiler available, I could send you the
> compiled binary to test. Peter, is there another way to generate
> binaries using semihosting?

You can also do this with a gcc compiled for arm-none-eabi (eg as found
in the codesourcery "ARM EABI" toolchain:
http://www.codesourcery.com/sgpp/lite/arm/portal/release1592 ):

/opt/arm-2010.09/bin/arm-none-eabi-gcc -g -Wall -o hello hello.c -T
generic-hosted.ld

Or you can just test "by hand":
#include <stdio.h>

struct datablock {
   char *string;
   int buflen;
};

char buf[20];

int main(int argc, char **argv) {
   int r, i;
   struct datablock datablock;
   printf("semihosting-cmd test: argc %d\n", argc);
   for (i = 0; i < argc; i++) {
      printf("argv[%d]: %s\n", i, argv[i]);
   }
   datablock.string = buf;
   datablock.buflen = sizeof(buf);
   {
      register int r0 asm ("r0") = 0x15;
      register void * r1 asm ("r1") = &datablock;
      register int result asm ("r0");
      __asm__ __volatile__ (
      "svc 0x123456"
      : "=r" (result)
      : "r" (r0), "r" (r1)
      : "cc", "memory");
      r = result;
   }
   printf("semihosting returned status %d\n", r);
   if (r == 0) {
       printf("and string '%s'\n", datablock.string);
   }
   return 0;
}

-- PMM

Re: [Qemu-devel] [PATCH 0/2] ARM: fix commandline handling forsemihosted executables

[riku voipio]

On 01/07/2011 05:56 PM, Peter Maydell wrote:
> Or you can just test "by hand":

Got it. For anyone wondering into this thread, if you use ubuntu/linaro 
toolchains to compile the example below, you need a -marm flag for the 
compiler or you'll get a weird error on swi...

Looks good, I'll add these the next batch of patches.

> #include<stdio.h>
>
> struct datablock {
>     char *string;
>     int buflen;
> };
>
> char buf[20];
>
> int main(int argc, char **argv) {
>     int r, i;
>     struct datablock datablock;
>     printf("semihosting-cmd test: argc %d\n", argc);
>     for (i = 0; i<  argc; i++) {
>        printf("argv[%d]: %s\n", i, argv[i]);
>     }
>     datablock.string = buf;
>     datablock.buflen = sizeof(buf);
>     {
>        register int r0 asm ("r0") = 0x15;
>        register void * r1 asm ("r1") =&datablock;
>        register int result asm ("r0");
>        __asm__ __volatile__ (
>        "svc 0x123456"
>        : "=r" (result)
>        : "r" (r0), "r" (r1)
>        : "cc", "memory");
>        r = result;
>     }
>     printf("semihosting returned status %d\n", r);
>     if (r == 0) {
>         printf("and string '%s'\n", datablock.string);
>     }
>     return 0;
> }
>
> -- PMM