[Qemu-devel] [PATCH 0/2] target-arm: Fix SYS_HEAPINFO for 64-bit guests

[Qemu-devel] [PATCH 0/2] target-arm: Fix SYS_HEAPINFO for 64-bit guests

[Peter Maydell]

These patches fix bugs in the ARM semihosting SYS_HEAPINFO
syscall for 64-bit guests:
 * the fields in linux-user's TaskState should be abi_ulong,
   not uint32_t, since they're guest addresses
 * the SYS_HEAPINFO implementation needs to write its return
   data struct using fields of the right width

The recent patch from Tsung-Han Lin ("target-arm: fix semihosting ram
base issue") addressed this issue in passing, but these patches
take a slightly different approach:
 * factor out the "write fields back" code to reduce duplication
   between the various (32,64) x (user,system) cases
 * use put_user*() rather than tswap and direct write, to
   avoid potential issues with the guest handing us a
   misaligned pointer

thanks
-- PMM


Peter Maydell (2):
  linux-user: Make semihosting heap/stack fields abi_ulongs
  target-arm/arm-semi.c: Fix SYS_HEAPINFO for 64-bit guests

 linux-user/qemu.h     |  6 +++---
 target-arm/arm-semi.c | 47 ++++++++++++++++++++++++++---------------------
 2 files changed, 29 insertions(+), 24 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] linux-user: Make semihosting heap/stack fields abi_ulongs

[Peter Maydell]

The fields in the TaskState heap_base, heap_limit and stack_base
are all guest addresses (representing the locations of the heap
and stack for the guest binary), so they should be abi_ulong
rather than uint32_t. (This only in practice affects ARM AArch64
since all the other semihosting implementations are 32-bit.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/qemu.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 56f29c3..1b3b03b 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -111,10 +111,10 @@ typedef struct TaskState {
 #endif
 #if defined(TARGET_ARM) || defined(TARGET_M68K) || defined(TARGET_UNICORE32)
     /* Extra fields for semihosted binaries.  */
-    uint32_t heap_base;
-    uint32_t heap_limit;
+    abi_ulong heap_base;
+    abi_ulong heap_limit;
 #endif
-    uint32_t stack_base;
+    abi_ulong stack_base;
     int used; /* non zero if used */
     struct image_info *info;
     struct linux_binprm *bprm;
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] target-arm/arm-semi.c: Fix SYS_HEAPINFO for 64-bit guests

[Peter Maydell]

SYS_HEAPINFO is one of the few semihosting calls which has to write
values back into a parameter block in memory.  When we added
support for 64-bit semihosting we updated the code which reads from
the parameter block to read 64-bit words but forgot to change the
code that writes back into the block. Update it to treat the
block as a set of words of the appropriate width for the guest.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/arm-semi.c | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
index 8be0645..d50726f 100644
--- a/target-arm/arm-semi.c
+++ b/target-arm/arm-semi.c
@@ -564,8 +564,10 @@ target_ulong do_arm_semihosting(CPUARMState *env)
         }
     case TARGET_SYS_HEAPINFO:
         {
-            uint32_t *ptr;
+            target_ulong retvals[4];
             uint32_t limit;
+            int i;
+
             GET_ARG(0);
 
 #ifdef CONFIG_USER_ONLY
@@ -587,30 +589,33 @@ target_ulong do_arm_semihosting(CPUARMState *env)
                 ts->heap_limit = limit;
             }
 
-            ptr = lock_user(VERIFY_WRITE, arg0, 16, 0);
-            if (!ptr) {
-                /* FIXME - should this error code be -TARGET_EFAULT ? */
-                return (uint32_t)-1;
-            }
-            ptr[0] = tswap32(ts->heap_base);
-            ptr[1] = tswap32(ts->heap_limit);
-            ptr[2] = tswap32(ts->stack_base);
-            ptr[3] = tswap32(0); /* Stack limit.  */
-            unlock_user(ptr, arg0, 16);
+            retvals[0] = ts->heap_base;
+            retvals[1] = ts->heap_limit;
+            retvals[2] = ts->stack_base;
+            retvals[3] = 0; /* Stack limit.  */
 #else
             limit = ram_size;
-            ptr = lock_user(VERIFY_WRITE, arg0, 16, 0);
-            if (!ptr) {
-                /* FIXME - should this error code be -TARGET_EFAULT ? */
-                return (uint32_t)-1;
-            }
             /* TODO: Make this use the limit of the loaded application.  */
-            ptr[0] = tswap32(limit / 2);
-            ptr[1] = tswap32(limit);
-            ptr[2] = tswap32(limit); /* Stack base */
-            ptr[3] = tswap32(0); /* Stack limit.  */
-            unlock_user(ptr, arg0, 16);
+            retvals[0] = limit / 2;
+            retvals[1] = limit;
+            retvals[2] = limit; /* Stack base */
+            retvals[3] = 0; /* Stack limit.  */
 #endif
+
+            for (i = 0; i < ARRAY_SIZE(retvals); i++) {
+                bool fail;
+
+                if (is_a64(env)) {
+                    fail = put_user_u64(retvals[i], arg0 + i * 8);
+                } else {
+                    fail = put_user_u32(retvals[i], arg0 + i * 4);
+                }
+
+                if (fail) {
+                    /* Couldn't write back to argument block */
+                    return -1;
+                }
+            }
             return 0;
         }
     case TARGET_SYS_EXIT:
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/2] linux-user: Make semihosting heap/stack fields abi_ulongs

[Laurent Desnogues]

On Fri, Jun 24, 2016 at 5:49 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The fields in the TaskState heap_base, heap_limit and stack_base
> are all guest addresses (representing the locations of the heap
> and stack for the guest binary), so they should be abi_ulong
> rather than uint32_t. (This only in practice affects ARM AArch64
> since all the other semihosting implementations are 32-bit.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks,

Laurent

> ---
>  linux-user/qemu.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 56f29c3..1b3b03b 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -111,10 +111,10 @@ typedef struct TaskState {
>  #endif
>  #if defined(TARGET_ARM) || defined(TARGET_M68K) || defined(TARGET_UNICORE32)
>      /* Extra fields for semihosted binaries.  */
> -    uint32_t heap_base;
> -    uint32_t heap_limit;
> +    abi_ulong heap_base;
> +    abi_ulong heap_limit;
>  #endif
> -    uint32_t stack_base;
> +    abi_ulong stack_base;
>      int used; /* non zero if used */
>      struct image_info *info;
>      struct linux_binprm *bprm;
> --
> 1.9.1
>

Re: [Qemu-devel] [PATCH 2/2] target-arm/arm-semi.c: Fix SYS_HEAPINFO for 64-bit guests

[Laurent Desnogues]

On Fri, Jun 24, 2016 at 5:49 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> SYS_HEAPINFO is one of the few semihosting calls which has to write
> values back into a parameter block in memory.  When we added
> support for 64-bit semihosting we updated the code which reads from
> the parameter block to read 64-bit words but forgot to change the
> code that writes back into the block. Update it to treat the
> block as a set of words of the appropriate width for the guest.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/arm-semi.c | 47 ++++++++++++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
>
> diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
> index 8be0645..d50726f 100644
> --- a/target-arm/arm-semi.c
> +++ b/target-arm/arm-semi.c
> @@ -564,8 +564,10 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>          }
>      case TARGET_SYS_HEAPINFO:
>          {
> -            uint32_t *ptr;
> +            target_ulong retvals[4];
>              uint32_t limit;

I think limit should also be converted to target_ulong.

Thanks,

Laurent

> +            int i;
> +
>              GET_ARG(0);
>
>  #ifdef CONFIG_USER_ONLY
> @@ -587,30 +589,33 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>                  ts->heap_limit = limit;
>              }
>
> -            ptr = lock_user(VERIFY_WRITE, arg0, 16, 0);
> -            if (!ptr) {
> -                /* FIXME - should this error code be -TARGET_EFAULT ? */
> -                return (uint32_t)-1;
> -            }
> -            ptr[0] = tswap32(ts->heap_base);
> -            ptr[1] = tswap32(ts->heap_limit);
> -            ptr[2] = tswap32(ts->stack_base);
> -            ptr[3] = tswap32(0); /* Stack limit.  */
> -            unlock_user(ptr, arg0, 16);
> +            retvals[0] = ts->heap_base;
> +            retvals[1] = ts->heap_limit;
> +            retvals[2] = ts->stack_base;
> +            retvals[3] = 0; /* Stack limit.  */
>  #else
>              limit = ram_size;
> -            ptr = lock_user(VERIFY_WRITE, arg0, 16, 0);
> -            if (!ptr) {
> -                /* FIXME - should this error code be -TARGET_EFAULT ? */
> -                return (uint32_t)-1;
> -            }
>              /* TODO: Make this use the limit of the loaded application.  */
> -            ptr[0] = tswap32(limit / 2);
> -            ptr[1] = tswap32(limit);
> -            ptr[2] = tswap32(limit); /* Stack base */
> -            ptr[3] = tswap32(0); /* Stack limit.  */
> -            unlock_user(ptr, arg0, 16);
> +            retvals[0] = limit / 2;
> +            retvals[1] = limit;
> +            retvals[2] = limit; /* Stack base */
> +            retvals[3] = 0; /* Stack limit.  */
>  #endif
> +
> +            for (i = 0; i < ARRAY_SIZE(retvals); i++) {
> +                bool fail;
> +
> +                if (is_a64(env)) {
> +                    fail = put_user_u64(retvals[i], arg0 + i * 8);
> +                } else {
> +                    fail = put_user_u32(retvals[i], arg0 + i * 4);
> +                }
> +
> +                if (fail) {
> +                    /* Couldn't write back to argument block */
> +                    return -1;
> +                }
> +            }
>              return 0;
>          }
>      case TARGET_SYS_EXIT:
> --
> 1.9.1
>

Re: [Qemu-devel] [PATCH 2/2] target-arm/arm-semi.c: Fix SYS_HEAPINFO for 64-bit guests

[Peter Maydell]

On 24 June 2016 at 17:09, Laurent Desnogues <laurent.desnogues@gmail.com> wrote:
> On Fri, Jun 24, 2016 at 5:49 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> SYS_HEAPINFO is one of the few semihosting calls which has to write
>> values back into a parameter block in memory.  When we added
>> support for 64-bit semihosting we updated the code which reads from
>> the parameter block to read 64-bit words but forgot to change the
>> code that writes back into the block. Update it to treat the
>> block as a set of words of the appropriate width for the guest.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target-arm/arm-semi.c | 47 ++++++++++++++++++++++++++---------------------
>>  1 file changed, 26 insertions(+), 21 deletions(-)
>>
>> diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
>> index 8be0645..d50726f 100644
>> --- a/target-arm/arm-semi.c
>> +++ b/target-arm/arm-semi.c
>> @@ -564,8 +564,10 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>>          }
>>      case TARGET_SYS_HEAPINFO:
>>          {
>> -            uint32_t *ptr;
>> +            target_ulong retvals[4];
>>              uint32_t limit;
>
> I think limit should also be converted to target_ulong.

Yes, it should.

thanks
-- PMM