mbox series

[0/9] accel/tcg: Fix page_set_flags and related [#1528]

Message ID 20230306021307.1879483-1-richard.henderson@linaro.org
Headers show
Series accel/tcg: Fix page_set_flags and related [#1528] | expand

Message

Richard Henderson March 6, 2023, 2:12 a.m. UTC 
The primary issue is that of overflow, where "end" for the last
page of the 32-bit address space overflows to 0.  The fix is to
use "last" instead, which can always be represented.

This requires that we adjust reserved_va as well, because of

-/*
- * 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)

and the related

-        /*
-         * reserved_va must be aligned with the host page size
-         * as it is used with mmap()
-         */
-        reserved_va = local_max_va & qemu_host_page_mask;

whereby we avoided the final (host | guest) page of the address space
because of said overflow.  With the change in representation, we can
always use UINT32_MAX as the end of the 32-bit address space.

This was observable on ppc64le (or any other 64k page host) not being
able to load any arm32 binary, because the COMMPAGE goes at 0xffff0000,
which violated that last host page problem above.

The issue is resolved in patch 4, but the rest clean up other interfaces
with the same issue.  I'm not touching any interfaces that use start+len
instead of start+end.


r~


Richard Henderson (9):
  linux-user: Diagnose incorrect -R size
  linux-user: Rename max_reserved_va in main
  include/exec: Replace reserved_va with max_reserved_va
  accel/tcg: Pass last not end to page_set_flags
  accel/tcg: Pass last not end to page_reset_target_data
  accel/tcg: Pass last not end to PAGE_FOR_EACH_TB
  accel/tcg: Pass last not end to page_collection_lock
  accel/tcg: Pass last not end to tb_invalidate_phys_page_range__locked
  accel/tcg: Pass last not end to tb_invalidate_phys_range

 include/exec/cpu-all.h      | 19 ++++++--
 include/exec/exec-all.h     |  2 +-
 linux-user/arm/target_cpu.h |  2 +-
 accel/tcg/tb-maint.c        | 95 +++++++++++++++++++------------------
 accel/tcg/translate-all.c   |  2 +-
 accel/tcg/user-exec.c       | 25 +++++-----
 bsd-user/main.c             | 18 +++----
 bsd-user/mmap.c             | 18 +++----
 bsd-user/signal.c           |  4 +-
 linux-user/elfload.c        | 47 +++++++++---------
 linux-user/main.c           | 44 +++++++++--------
 linux-user/mmap.c           | 38 +++++++--------
 linux-user/signal.c         |  4 +-
 linux-user/syscall.c        |  4 +-
 softmmu/physmem.c           |  2 +-
 target/arm/cpu.c            |  2 +-
 16 files changed, 169 insertions(+), 157 deletions(-)

Comments

Joel Stanley March 7, 2023, 3:19 a.m. UTC | #1 
On Mon, 6 Mar 2023 at 02:14, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The primary issue is that of overflow, where "end" for the last
> page of the 32-bit address space overflows to 0.  The fix is to
> use "last" instead, which can always be represented.
>
> This requires that we adjust reserved_va as well, because of
>
> -/*
> - * 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)
>
> and the related
>
> -        /*
> -         * reserved_va must be aligned with the host page size
> -         * as it is used with mmap()
> -         */
> -        reserved_va = local_max_va & qemu_host_page_mask;
>
> whereby we avoided the final (host | guest) page of the address space
> because of said overflow.  With the change in representation, we can
> always use UINT32_MAX as the end of the 32-bit address space.
>
> This was observable on ppc64le (or any other 64k page host) not being
> able to load any arm32 binary, because the COMMPAGE goes at 0xffff0000,
> which violated that last host page problem above.
>
> The issue is resolved in patch 4, but the rest clean up other interfaces
> with the same issue.  I'm not touching any interfaces that use start+len
> instead of start+end.

Thanks for looking at this Richard. I gave it a spin on a ppc64le host
and it resolved the assert.

Tested-by: Joel Stanley <joel@jms.id.au>

Cheers,

Joel
Ninad Palsule March 7, 2023, 1:55 p.m. UTC | #2 
On 3/6/23 9:19 PM, Joel Stanley wrote:
> On Mon, 6 Mar 2023 at 02:14, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> The primary issue is that of overflow, where "end" for the last
>> page of the 32-bit address space overflows to 0.  The fix is to
>> use "last" instead, which can always be represented.
>>
>> This requires that we adjust reserved_va as well, because of
>>
>> -/*
>> - * 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)
>>
>> and the related
>>
>> -        /*
>> -         * reserved_va must be aligned with the host page size
>> -         * as it is used with mmap()
>> -         */
>> -        reserved_va = local_max_va & qemu_host_page_mask;
>>
>> whereby we avoided the final (host | guest) page of the address space
>> because of said overflow.  With the change in representation, we can
>> always use UINT32_MAX as the end of the 32-bit address space.
>>
>> This was observable on ppc64le (or any other 64k page host) not being
>> able to load any arm32 binary, because the COMMPAGE goes at 0xffff0000,
>> which violated that last host page problem above.
>>
>> The issue is resolved in patch 4, but the rest clean up other interfaces
>> with the same issue.  I'm not touching any interfaces that use start+len
>> instead of start+end.

Richard, I tested it on ppc64le host and it fix is working.

Tested-by:NinadPalsule <ninad@linux.ibm.com<mailto:ninad@linux.ibm.com>>

Thx,

Ninad Palsule