[Qemu-devel] [PATCH 0/3] linux-user fixes for va mapping

[Qemu-devel] [PATCH 0/3] linux-user fixes for va mapping

[Richard Henderson]

At first I was simply going to add TARGET_SH to the existing
set of defines that trigger a 31-bit address space.  But then
I realized that one could create non-working va configurations
from the command-line.


r~


Richard Henderson (3):
  tcg: Fix off-by-one in assert in page_set_flags
  linux-user: Tidy and enforce reserved_va initialization
  linux-user/sh4: Reduce TARGET_VIRT_ADDR_SPACE_BITS to 31

 linux-user/arm/target_cpu.h |  4 ++++
 target/mips/mips-defs.h     |  6 +++++-
 target/nios2/cpu.h          |  6 +++++-
 target/sh4/cpu.h            |  6 +++++-
 accel/tcg/translate-all.c   |  2 +-
 linux-user/main.c           | 38 +++++++++++++++++++++++++-------------
 6 files changed, 45 insertions(+), 17 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 1/3] tcg: Fix off-by-one in assert in page_set_flags

[Richard Henderson]

Most of the users of page_set_flags offset (page, page + len) as
the end points.  One might consider this an error, since the other
users do supply an endpoint as the last byte of the region.

However, the first thing that page_set_flags does is round end UP
to the start of the next page.  Which means computing page + len - 1
is in the end pointless.  Therefore, accept this usage and do not
assert when given the exact size of the vm as the endpoint.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 accel/tcg/translate-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index dfb9f0d..57578a4 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2068,7 +2068,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags)
        guest address space.  If this assert fires, it probably indicates
        a missing call to h2g_valid.  */
 #if TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS
-    assert(end < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
+    assert(end <= ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
 #endif
     assert(start < end);
     assert_memory_lock();
-- 
2.9.4

[Qemu-devel] [PATCH 2/3] linux-user: Tidy and enforce reserved_va initialization

[Richard Henderson]

We had a check using TARGET_VIRT_ADDR_SPACE_BITS to make sure
that the allocation coming in from the command-line option was
not too large, but that didn't include target-specific knowledge
about other restrictions on user-space.

Remove several target-specific hacks in linux-user/main.c.

For MIPS and Nios, we can replace them with proper adjustments
to the respective target's TARGET_VIRT_ADDR_SPACE_BITS definition.

For ARM, we had no existing ifdef but I suspect that the current
default value of 0xf7000000 was chosen with this in mind.  Define
a workable value in linux-user/arm/, and also document why the
special case is required.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/arm/target_cpu.h |  4 ++++
 target/mips/mips-defs.h     |  6 +++++-
 target/nios2/cpu.h          |  6 +++++-
 linux-user/main.c           | 38 +++++++++++++++++++++++++-------------
 4 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/linux-user/arm/target_cpu.h b/linux-user/arm/target_cpu.h
index d888219..c4f79eb 100644
--- a/linux-user/arm/target_cpu.h
+++ b/linux-user/arm/target_cpu.h
@@ -19,6 +19,10 @@
 #ifndef ARM_TARGET_CPU_H
 #define ARM_TARGET_CPU_H
 
+/* We need to be able to map the commpage.
+   See validate_guest_space in linux-user/elfload.c.  */
+#define MAX_RESERVED_VA  0xfff00000ul
+
 static inline void cpu_clone_regs(CPUARMState *env, target_ulong newsp)
 {
     if (newsp) {
diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h
index 047554e..d239069 100644
--- a/target/mips/mips-defs.h
+++ b/target/mips/mips-defs.h
@@ -15,7 +15,11 @@
 #else
 #define TARGET_LONG_BITS 32
 #define TARGET_PHYS_ADDR_SPACE_BITS 40
-#define TARGET_VIRT_ADDR_SPACE_BITS 32
+# ifdef CONFIG_USER_ONLY
+#  define TARGET_VIRT_ADDR_SPACE_BITS 31
+# else
+#  define TARGET_VIRT_ADDR_SPACE_BITS 32
+#endif
 #endif
 
 /* Masks used to mark instructions to indicate which ISA level they
diff --git a/target/nios2/cpu.h b/target/nios2/cpu.h
index 13931f3..da3f637 100644
--- a/target/nios2/cpu.h
+++ b/target/nios2/cpu.h
@@ -227,7 +227,11 @@ qemu_irq *nios2_cpu_pic_init(Nios2CPU *cpu);
 void nios2_check_interrupts(CPUNios2State *env);
 
 #define TARGET_PHYS_ADDR_SPACE_BITS 32
-#define TARGET_VIRT_ADDR_SPACE_BITS 32
+#ifdef CONFIG_USER_ONLY
+# define TARGET_VIRT_ADDR_SPACE_BITS 31
+#else
+# define TARGET_VIRT_ADDR_SPACE_BITS 32
+#endif
 
 #define cpu_init(cpu_model) CPU(cpu_nios2_init(cpu_model))
 
diff --git a/linux-user/main.c b/linux-user/main.c
index ad03c9e..e000533 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -60,23 +60,38 @@ do {                                                                    \
     }                                                                   \
 } while (0)
 
-#if (TARGET_LONG_BITS == 32) && (HOST_LONG_BITS == 64)
 /*
  * When running 32-on-64 we should make sure we can fit all of the possible
  * guest address space into a contiguous chunk of virtual host memory.
  *
  * This way we will never overlap with our own libraries or binaries or stack
  * or anything else that QEMU maps.
+ *
+ * Many cpus reserve the high bit (or more than one for some 64-bit cpus)
+ * of the address for the kernel.  Some cpus rely on this and user space
+ * uses the high bit(s) for pointer tagging and the like.  For them, we
+ * must preserve the expected address space.
  */
-# if defined(TARGET_MIPS) || defined(TARGET_NIOS2)
-/*
- * MIPS only supports 31 bits of virtual address space for user space.
- * Nios2 also only supports 31 bits.
- */
-unsigned long reserved_va = 0x77000000;
+#ifndef MAX_RESERVED_VA
+# if HOST_LONG_BITS > TARGET_VIRT_ADDR_SPACE_BITS
+#  if TARGET_VIRT_ADDR_SPACE_BITS == 32 && \
+      (TARGET_LONG_BITS == 32 || defined(TARGET_ABI32))
+/* There are a number of places where we assign reserved_va to a variable
+   of type abi_ulong and expect it to fit.  Avoid the last page.  */
+#   define MAX_RESERVED_VA  (0xfffffffful & TARGET_PAGE_MASK)
+#  else
+#   define MAX_RESERVED_VA  (1ul << TARGET_VIRT_ADDR_SPACE_BITS)
+#  endif
 # else
-unsigned long reserved_va = 0xf7000000;
+#  define MAX_RESERVED_VA  0
 # endif
+#endif
+
+/* That said, reserving *too* much vm space via mmap can run into problems
+   with rlimits, oom due to page table creation, etc.  We will still try it,
+   if directed by the command-line option, but not by default.  */
+#if HOST_LONG_BITS == 64 && TARGET_VIRT_ADDR_SPACE_BITS <= 32
+unsigned long reserved_va = MAX_RESERVED_VA;
 #else
 unsigned long reserved_va;
 #endif
@@ -3975,11 +3990,8 @@ static void handle_arg_reserved_va(const char *arg)
         unsigned long unshifted = reserved_va;
         p++;
         reserved_va <<= shift;
-        if (((reserved_va >> shift) != unshifted)
-#if HOST_LONG_BITS > TARGET_VIRT_ADDR_SPACE_BITS
-            || (reserved_va > (1ul << TARGET_VIRT_ADDR_SPACE_BITS))
-#endif
-            ) {
+        if (reserved_va >> shift != unshifted
+            || (MAX_RESERVED_VA && reserved_va > MAX_RESERVED_VA)) {
             fprintf(stderr, "Reserved virtual address too big\n");
             exit(EXIT_FAILURE);
         }
-- 
2.9.4

[Qemu-devel] [PATCH 3/3] linux-user/sh4: Reduce TARGET_VIRT_ADDR_SPACE_BITS to 31

[Richard Henderson]

The real kernel has TASK_SIZE as 0x7c000000, due to quirks with
a couple of SH parts.  But nominally user-space is limited to 2GB.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target/sh4/cpu.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h
index e3abb6a..3121d1e 100644
--- a/target/sh4/cpu.h
+++ b/target/sh4/cpu.h
@@ -45,7 +45,11 @@
 #define TARGET_PAGE_BITS 12	/* 4k XXXXX */
 
 #define TARGET_PHYS_ADDR_SPACE_BITS 32
-#define TARGET_VIRT_ADDR_SPACE_BITS 32
+#ifdef CONFIG_USER_ONLY
+# define TARGET_VIRT_ADDR_SPACE_BITS 31
+#else
+# define TARGET_VIRT_ADDR_SPACE_BITS 32
+#endif
 
 #define SR_MD 30
 #define SR_RB 29
-- 
2.9.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/3] tcg: Fix off-by-one in assert in page_set_flags

[Philippe Mathieu-Daudé]

On 07/07/2017 11:50 PM, Richard Henderson wrote:
> Most of the users of page_set_flags offset (page, page + len) as
> the end points.  One might consider this an error, since the other
> users do supply an endpoint as the last byte of the region.
> 
> However, the first thing that page_set_flags does is round end UP
> to the start of the next page.  Which means computing page + len - 1
> is in the end pointless.  Therefore, accept this usage and do not
> assert when given the exact size of the vm as the endpoint.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>   accel/tcg/translate-all.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index dfb9f0d..57578a4 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -2068,7 +2068,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags)
>          guest address space.  If this assert fires, it probably indicates
>          a missing call to h2g_valid.  */
>   #if TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS
> -    assert(end < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
> +    assert(end <= ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));

worth adding a comment /* end rounded up */ ?

anyway for this tricky catch:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>   #endif
>       assert(start < end);
>       assert_memory_lock();
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] linux-user/sh4: Reduce TARGET_VIRT_ADDR_SPACE_BITS to 31

[Philippe Mathieu-Daudé]

On 07/07/2017 11:50 PM, Richard Henderson wrote:
> The real kernel has TASK_SIZE as 0x7c000000, due to quirks with
> a couple of SH parts.  But nominally user-space is limited to 2GB.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   target/sh4/cpu.h | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h
> index e3abb6a..3121d1e 100644
> --- a/target/sh4/cpu.h
> +++ b/target/sh4/cpu.h
> @@ -45,7 +45,11 @@
>   #define TARGET_PAGE_BITS 12	/* 4k XXXXX */
>   
>   #define TARGET_PHYS_ADDR_SPACE_BITS 32
> -#define TARGET_VIRT_ADDR_SPACE_BITS 32
> +#ifdef CONFIG_USER_ONLY
> +# define TARGET_VIRT_ADDR_SPACE_BITS 31
> +#else
> +# define TARGET_VIRT_ADDR_SPACE_BITS 32
> +#endif
>   
>   #define SR_MD 30
>   #define SR_RB 29
> 

Re: [Qemu-devel] [PATCH 2/3] linux-user: Tidy and enforce reserved_va initialization

[Peter Maydell]

On 8 July 2017 at 03:50, Richard Henderson <rth@twiddle.net> wrote:
> We had a check using TARGET_VIRT_ADDR_SPACE_BITS to make sure
> that the allocation coming in from the command-line option was
> not too large, but that didn't include target-specific knowledge
> about other restrictions on user-space.
>
> Remove several target-specific hacks in linux-user/main.c.
>
> For MIPS and Nios, we can replace them with proper adjustments
> to the respective target's TARGET_VIRT_ADDR_SPACE_BITS definition.
>
> For ARM, we had no existing ifdef but I suspect that the current
> default value of 0xf7000000 was chosen with this in mind.  Define
> a workable value in linux-user/arm/, and also document why the
> special case is required.
>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  linux-user/arm/target_cpu.h |  4 ++++
>  target/mips/mips-defs.h     |  6 +++++-
>  target/nios2/cpu.h          |  6 +++++-
>  linux-user/main.c           | 38 +++++++++++++++++++++++++-------------
>  4 files changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/linux-user/arm/target_cpu.h b/linux-user/arm/target_cpu.h
> index d888219..c4f79eb 100644
> --- a/linux-user/arm/target_cpu.h
> +++ b/linux-user/arm/target_cpu.h
> @@ -19,6 +19,10 @@
>  #ifndef ARM_TARGET_CPU_H
>  #define ARM_TARGET_CPU_H
>
> +/* We need to be able to map the commpage.
> +   See validate_guest_space in linux-user/elfload.c.  */
> +#define MAX_RESERVED_VA  0xfff00000ul
> +

This should be 0xffff0000, but you'll need the bugfix patch I just sent
out first.

(Why "UL" ? That's usually a wrong choice compared to either U or ULL.)

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] linux-user: Tidy and enforce reserved_va initialization

[Richard Henderson]

On 10/03/2017 12:24 PM, Peter Maydell wrote:
> On 8 July 2017 at 03:50, Richard Henderson <rth@twiddle.net> wrote:
>> We had a check using TARGET_VIRT_ADDR_SPACE_BITS to make sure
>> that the allocation coming in from the command-line option was
>> not too large, but that didn't include target-specific knowledge
>> about other restrictions on user-space.
>>
>> Remove several target-specific hacks in linux-user/main.c.
>>
>> For MIPS and Nios, we can replace them with proper adjustments
>> to the respective target's TARGET_VIRT_ADDR_SPACE_BITS definition.
>>
>> For ARM, we had no existing ifdef but I suspect that the current
>> default value of 0xf7000000 was chosen with this in mind.  Define
>> a workable value in linux-user/arm/, and also document why the
>> special case is required.
>>
>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>> ---
>>  linux-user/arm/target_cpu.h |  4 ++++
>>  target/mips/mips-defs.h     |  6 +++++-
>>  target/nios2/cpu.h          |  6 +++++-
>>  linux-user/main.c           | 38 +++++++++++++++++++++++++-------------
>>  4 files changed, 39 insertions(+), 15 deletions(-)
>>
>> diff --git a/linux-user/arm/target_cpu.h b/linux-user/arm/target_cpu.h
>> index d888219..c4f79eb 100644
>> --- a/linux-user/arm/target_cpu.h
>> +++ b/linux-user/arm/target_cpu.h
>> @@ -19,6 +19,10 @@
>>  #ifndef ARM_TARGET_CPU_H
>>  #define ARM_TARGET_CPU_H
>>
>> +/* We need to be able to map the commpage.
>> +   See validate_guest_space in linux-user/elfload.c.  */
>> +#define MAX_RESERVED_VA  0xfff00000ul
>> +
> 
> This should be 0xffff0000, but you'll need the bugfix patch I just sent
> out first.
> 
> (Why "UL" ? That's usually a wrong choice compared to either U or ULL.)

Because that matches the type of

+unsigned long reserved_va = MAX_RESERVED_VA;

Which, arguably, should be uintptr_t or size_t something instead, but that
would certainly be for a different patch.

If you prefer, since this is a 32-bit value, I could trim the define to U and
still be correct.


r~