diff mbox series

[PULL,v2,05/13] accel/tcg: Relax va restrictions on 64-bit guests

Message ID 20200515144405.20580-6-alex.bennee@linaro.org
State New
Headers show
Series testing, tcg and plugin updates | expand

Commit Message

Alex Bennée May 15, 2020, 2:43 p.m. UTC 
From: Richard Henderson <richard.henderson@linaro.org>


We cannot at present limit a 64-bit guest to a virtual address
space smaller than the host.  It will mostly work to ignore this
limitation, except if the guest uses high bits of the address
space for tags.  But it will certainly work better, as presently
we can wind up failing to allocate the guest stack.

Widen our user-only page tree to the host or abi pointer width.
Remove the workaround for this problem from target/alpha.
Always validate guest addresses vs reserved_va, as there we
control allocation ourselves.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>


Message-Id: <20200513175134.19619-7-alex.bennee@linaro.org>

-- 
2.20.1

Comments

Laurent Vivier June 4, 2020, 2:15 p.m. UTC | #1 
On 15/05/2020 16:43, Alex Bennée wrote:
> From: Richard Henderson <richard.henderson@linaro.org>

> 

> We cannot at present limit a 64-bit guest to a virtual address

> space smaller than the host.  It will mostly work to ignore this

> limitation, except if the guest uses high bits of the address

> space for tags.  But it will certainly work better, as presently

> we can wind up failing to allocate the guest stack.

> 

> Widen our user-only page tree to the host or abi pointer width.

> Remove the workaround for this problem from target/alpha.

> Always validate guest addresses vs reserved_va, as there we

> control allocation ourselves.

> 

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> 

> Message-Id: <20200513175134.19619-7-alex.bennee@linaro.org>

> 


This patch breaks a test case in LTP with 64bit targets on x86_64 host:

sudo linux-user/mips64el-linux-user/qemu-mips64el \
-L chroot/mips64el/stretch/ \
chroot/mips64el/stretch/opt/ltp/testcases/bin/mmap15

qemu-mips64el: accel/tcg/translate-all.c:2533: page_set_flags: Assertion
`start < end' failed.
qemu:handle_cpu_signal received signal outside vCPU context @
pc=0x7f0015f6e7cb

Could you have a look?

Thanks,
Laurent
Alex Bennée June 4, 2020, 5:31 p.m. UTC | #2 
Laurent Vivier <laurent@vivier.eu> writes:

> On 15/05/2020 16:43, Alex Bennée wrote:

>> From: Richard Henderson <richard.henderson@linaro.org>

>> 

>> We cannot at present limit a 64-bit guest to a virtual address

>> space smaller than the host.  It will mostly work to ignore this

>> limitation, except if the guest uses high bits of the address

>> space for tags.  But it will certainly work better, as presently

>> we can wind up failing to allocate the guest stack.

>> 

>> Widen our user-only page tree to the host or abi pointer width.

>> Remove the workaround for this problem from target/alpha.

>> Always validate guest addresses vs reserved_va, as there we

>> control allocation ourselves.

>> 

>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> 

>> Message-Id: <20200513175134.19619-7-alex.bennee@linaro.org>

>> 

>

> This patch breaks a test case in LTP with 64bit targets on x86_64 host:

>

> sudo linux-user/mips64el-linux-user/qemu-mips64el \

> -L chroot/mips64el/stretch/ \

> chroot/mips64el/stretch/opt/ltp/testcases/bin/mmap15

>

> qemu-mips64el: accel/tcg/translate-all.c:2533: page_set_flags: Assertion

> `start < end' failed.

> qemu:handle_cpu_signal received signal outside vCPU context @

> pc=0x7f0015f6e7cb

>

> Could you have a look?


Can confirm I've replicated:

  18:30:20 [alex.bennee@hackbox2:~/l/q/b/user.static] next/various-fixes|✔ 32 +
  sudo ./mips64el-linux-user/qemu-mips64el -L ~/lsrc/buildroot.git/builds/mips64el/target/ ~/lsrc/buildroot.git/builds/mips64el/target/usr/lib/ltp-testsuite/testcases/bin/mmap
  15
  [sudo] password for alex.bennee:
  qemu-mips64el: /home/alex.bennee/lsrc/qemu.git/accel/tcg/translate-all.c:2533: page_set_flags: Assertion `start < end' failed.
  qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x6c28c2

Also TIL you can use buildroot to build ltp ;-)

>

> Thanks,

> Laurent



-- 
Alex Bennée
Alex Bennée June 5, 2020, 2:11 p.m. UTC | #3 
Alex Bennée <alex.bennee@linaro.org> writes:

> Laurent Vivier <laurent@vivier.eu> writes:

>

>> On 15/05/2020 16:43, Alex Bennée wrote:

>>> From: Richard Henderson <richard.henderson@linaro.org>

>>> 

>>> We cannot at present limit a 64-bit guest to a virtual address

>>> space smaller than the host.  It will mostly work to ignore this

>>> limitation, except if the guest uses high bits of the address

>>> space for tags.  But it will certainly work better, as presently

>>> we can wind up failing to allocate the guest stack.

>>> 

>>> Widen our user-only page tree to the host or abi pointer width.

>>> Remove the workaround for this problem from target/alpha.

>>> Always validate guest addresses vs reserved_va, as there we

>>> control allocation ourselves.

>>> 

>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>>> 

>>> Message-Id: <20200513175134.19619-7-alex.bennee@linaro.org>

>>> 

>>

>> This patch breaks a test case in LTP with 64bit targets on x86_64 host:

>>

>> sudo linux-user/mips64el-linux-user/qemu-mips64el \

>> -L chroot/mips64el/stretch/ \

>> chroot/mips64el/stretch/opt/ltp/testcases/bin/mmap15

>>

>> qemu-mips64el: accel/tcg/translate-all.c:2533: page_set_flags: Assertion

>> `start < end' failed.

>> qemu:handle_cpu_signal received signal outside vCPU context @

>> pc=0x7f0015f6e7cb

>>

>> Could you have a look?

>

> Can confirm I've replicated:

>

>   18:30:20 [alex.bennee@hackbox2:~/l/q/b/user.static] next/various-fixes|✔ 32 +

>   sudo ./mips64el-linux-user/qemu-mips64el -L ~/lsrc/buildroot.git/builds/mips64el/target/ ~/lsrc/buildroot.git/builds/mips64el/target/usr/lib/ltp-testsuite/testcases/bin/mmap

>   15

>   [sudo] password for alex.bennee:

>   qemu-mips64el: /home/alex.bennee/lsrc/qemu.git/accel/tcg/translate-all.c:2533: page_set_flags: Assertion `start < end' failed.

>   qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x6c28c2

>

> Also TIL you can use buildroot to build ltp ;-)


I'm not sure how the change has tripped this failure but it seems to be
a simple overflow. The following fixes it but I'm curious about the
formulation of guest_addr_valid and why it's written that way:

modified   linux-user/mmap.c
@@ -467,7 +467,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
          * It can fail only on 64-bit host with 32-bit target.
          * On any other target/host host mmap() handles this error correctly.
          */
-        if (!guest_range_valid(start, len)) {
+        if (end < start || !guest_range_valid(start, len)) {
             errno = ENOMEM;
             goto fail;
         }


>

>>

>> Thanks,

>> Laurent



-- 
Alex Bennée
Richard Henderson June 5, 2020, 5:38 p.m. UTC | #4 
On 6/5/20 7:11 AM, Alex Bennée wrote:
> @@ -467,7 +467,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,

>           * It can fail only on 64-bit host with 32-bit target.

>           * On any other target/host host mmap() handles this error correctly.

>           */

> -        if (!guest_range_valid(start, len)) {

> +        if (end < start || !guest_range_valid(start, len)) {

>              errno = ENOMEM;

>              goto fail;

>          }


Interesting.  I was adjusting guest_range_valid tagged pointers yesterday, and
thought that it looked buggy.


r~
Alex Bennée June 5, 2020, 5:46 p.m. UTC | #5 
Richard Henderson <rth@twiddle.net> writes:

> On 6/5/20 7:11 AM, Alex Bennée wrote:

>> @@ -467,7 +467,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,

>>           * It can fail only on 64-bit host with 32-bit target.

>>           * On any other target/host host mmap() handles this error correctly.

>>           */

>> -        if (!guest_range_valid(start, len)) {

>> +        if (end < start || !guest_range_valid(start, len)) {

>>              errno = ENOMEM;

>>              goto fail;

>>          }

>

> Interesting.  I was adjusting guest_range_valid tagged pointers yesterday, and

> thought that it looked buggy.


Should be picking this up in guest_range_valid?

-- 
Alex Bennée
Richard Henderson June 5, 2020, 6:26 p.m. UTC | #6 
On 6/5/20 10:46 AM, Alex Bennée wrote:
> 

> Richard Henderson <rth@twiddle.net> writes:

> 

>> On 6/5/20 7:11 AM, Alex Bennée wrote:

>>> @@ -467,7 +467,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,

>>>           * It can fail only on 64-bit host with 32-bit target.

>>>           * On any other target/host host mmap() handles this error correctly.

>>>           */

>>> -        if (!guest_range_valid(start, len)) {

>>> +        if (end < start || !guest_range_valid(start, len)) {

>>>              errno = ENOMEM;

>>>              goto fail;

>>>          }

>>

>> Interesting.  I was adjusting guest_range_valid tagged pointers yesterday, and

>> thought that it looked buggy.

> 

> Should be picking this up in guest_range_valid?


I think so.  How can a range really be considered valid if it wraps?


r~
diff mbox series

Patch

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 0895a57881a..d14374bdd49 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -162,12 +162,27 @@  extern unsigned long guest_base;
 extern bool have_guest_base;
 extern unsigned long reserved_va;
 
-#if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
-#define GUEST_ADDR_MAX (~0ul)
+/*
+ * Limit the guest addresses as best we can.
+ *
+ * When not using -R reserved_va, we cannot really limit the guest
+ * to less address space than the host.  For 32-bit guests, this
+ * acts as a sanity check that we're not giving the guest an address
+ * that it cannot even represent.  For 64-bit guests... the address
+ * might not be what the real kernel would give, but it is at least
+ * representable in the guest.
+ *
+ * TODO: Improve address allocation to avoid this problem, and to
+ * avoid setting bits at the top of guest addresses that might need
+ * to be used for tags.
+ */
+#if MIN(TARGET_VIRT_ADDR_SPACE_BITS, TARGET_ABI_BITS) <= 32
+# define GUEST_ADDR_MAX_  UINT32_MAX
 #else
-#define GUEST_ADDR_MAX (reserved_va ? reserved_va - 1 : \
-                                    (1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
+# define GUEST_ADDR_MAX_  (~0ul)
 #endif
+#define GUEST_ADDR_MAX    (reserved_va ? reserved_va - 1 : GUEST_ADDR_MAX_)
+
 #else
 
 #include "exec/hwaddr.h"
diff --git a/target/alpha/cpu-param.h b/target/alpha/cpu-param.h
index 692aee27ca9..1153992e42a 100644
--- a/target/alpha/cpu-param.h
+++ b/target/alpha/cpu-param.h
@@ -10,22 +10,11 @@ 
 
 #define TARGET_LONG_BITS 64
 #define TARGET_PAGE_BITS 13
-#ifdef CONFIG_USER_ONLY
-/*
- * ??? The kernel likes to give addresses in high memory.  If the host has
- * more virtual address space than the guest, this can lead to impossible
- * allocations.  Honor the long-standing assumption that only kernel addrs
- * are negative, but otherwise allow allocations anywhere.  This could lead
- * to tricky emulation problems for programs doing tagged addressing, but
- * that's far fewer than encounter the impossible allocation problem.
- */
-#define TARGET_PHYS_ADDR_SPACE_BITS  63
-#define TARGET_VIRT_ADDR_SPACE_BITS  63
-#else
+
 /* ??? EV4 has 34 phys addr bits, EV5 has 40, EV6 has 44.  */
 #define TARGET_PHYS_ADDR_SPACE_BITS  44
 #define TARGET_VIRT_ADDR_SPACE_BITS  (30 + TARGET_PAGE_BITS)
-#endif
+
 #define NB_MMU_MODES 3
 
 #endif
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 9924e66d1f7..e4f703a7e6d 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -173,8 +173,13 @@  struct page_collection {
 #define TB_FOR_EACH_JMP(head_tb, tb, n)                                 \
     TB_FOR_EACH_TAGGED((head_tb)->jmp_list_head, tb, n, jmp_list_next)
 
-/* In system mode we want L1_MAP to be based on ram offsets,
-   while in user mode we want it to be based on virtual addresses.  */
+/*
+ * In system mode we want L1_MAP to be based on ram offsets,
+ * while in user mode we want it to be based on virtual addresses.
+ *
+ * TODO: For user mode, see the caveat re host vs guest virtual
+ * address spaces near GUEST_ADDR_MAX.
+ */
 #if !defined(CONFIG_USER_ONLY)
 #if HOST_LONG_BITS < TARGET_PHYS_ADDR_SPACE_BITS
 # define L1_MAP_ADDR_SPACE_BITS  HOST_LONG_BITS
@@ -182,7 +187,7 @@  struct page_collection {
 # define L1_MAP_ADDR_SPACE_BITS  TARGET_PHYS_ADDR_SPACE_BITS
 #endif
 #else
-# define L1_MAP_ADDR_SPACE_BITS  TARGET_VIRT_ADDR_SPACE_BITS
+# define L1_MAP_ADDR_SPACE_BITS  MIN(HOST_LONG_BITS, TARGET_ABI_BITS)
 #endif
 
 /* Size of the L2 (and L3, etc) page tables.  */
@@ -2497,9 +2502,7 @@  void page_set_flags(target_ulong start, target_ulong end, int flags)
     /* This function should never be called with addresses outside the
        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));
-#endif
+    assert(end - 1 <= GUEST_ADDR_MAX);
     assert(start < end);
     assert_memory_lock();