Re: [PATCH for-8.1 v10 08/14] linux-user: Do not adjust zero_bss for host page size

["Alex Bennée" <alex.bennee@linaro.org>]

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

> Rely on target_mmap to handle guest vs host page size mismatch.
>
> Tested-by: Helge Deller <deller@gmx.de>
> Reviewed-by: Helge Deller <deller@gmx.de>
> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/elfload.c | 54 +++++++++++++++++++-------------------------
>  1 file changed, 23 insertions(+), 31 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 964b21f997..6c28cb70ef 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2213,44 +2213,36 @@ static abi_ulong setup_arg_pages(struct linux_binprm *bprm,
>  
>  /* Map and zero the bss.  We need to explicitly zero any fractional pages
>     after the data section (i.e. bss).  */
> -static void zero_bss(abi_ulong elf_bss, abi_ulong last_bss, int prot)
> +static void zero_bss(abi_ulong start_bss, abi_ulong end_bss, int prot)
>  {
> -    uintptr_t host_start, host_map_start, host_end;
> +    abi_ulong align_bss;
>  
> -    last_bss = TARGET_PAGE_ALIGN(last_bss);
> +    align_bss = TARGET_PAGE_ALIGN(start_bss);
> +    end_bss = TARGET_PAGE_ALIGN(end_bss);
>  
> -    /* ??? There is confusion between qemu_real_host_page_size and
> -       qemu_host_page_size here and elsewhere in target_mmap, which
> -       may lead to the end of the data section mapping from the file
> -       not being mapped.  At least there was an explicit test and
> -       comment for that here, suggesting that "the file size must
> -       be known".  The comment probably pre-dates the introduction
> -       of the fstat system call in target_mmap which does in fact
> -       find out the size.  What isn't clear is if the workaround
> -       here is still actually needed.  For now, continue with it,
> -       but merge it with the "normal" mmap that would allocate the bss.  */
> +    if (start_bss < align_bss) {
> +        int flags = page_get_flags(start_bss);
>  
> -    host_start = (uintptr_t) g2h_untagged(elf_bss);
> -    host_end = (uintptr_t) g2h_untagged(last_bss);
> -    host_map_start = REAL_HOST_PAGE_ALIGN(host_start);
> -
> -    if (host_map_start < host_end) {
> -        void *p = mmap((void *)host_map_start, host_end - host_map_start,
> -                       prot, MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> -        if (p == MAP_FAILED) {
> -            perror("cannot mmap brk");
> -            exit(-1);
> +        if (!(flags & PAGE_VALID)) {
> +            /* Map the start of the bss. */
> +            align_bss -= TARGET_PAGE_SIZE;
> +        } else if (flags & PAGE_WRITE) {
> +            /* The page is already mapped writable. */
> +            memset(g2h_untagged(start_bss), 0, align_bss - start_bss);
> +        } else {
> +            /* Read-only zeros? */
> +            g_assert_not_reached();
>          }
>      }
>  
> -    /* Ensure that the bss page(s) are valid */
> -    if ((page_get_flags(last_bss-1) & prot) != prot) {
> -        page_set_flags(elf_bss & TARGET_PAGE_MASK, last_bss - 1,
> -                       prot | PAGE_VALID);
> -    }
> -
> -    if (host_start < host_map_start) {
> -        memset((void *)host_start, 0, host_map_start - host_start);
> +    if (align_bss < end_bss) {
> +        abi_long err = target_mmap(align_bss, end_bss - align_bss, prot,
> +                                   MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS,
> +                                   -1, 0);
> +        if (err == -1) {
> +            perror("cannot mmap brk");
> +            exit(-1);

brk != bss even if brk generally comes after the bss section.

> +        }
>      }
>  }


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro