[PATCH for-8.0 11/11] linux-user/arm: Take more care allocating commpage

Richard Henderson posted 11 patches 2 years ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Riku Voipio <riku.voipio@iki.fi>, Warner Losh <imp@bsdimp.com>, Kyle Evans <kevans@freebsd.org>, "Alex Bennée" <alex.bennee@linaro.org>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <laurent@vivier.eu>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>
There is a newer version of this series
[PATCH for-8.0 11/11] linux-user/arm: Take more care allocating commpage
Posted by Richard Henderson 2 years ago
User setting of -R reserved_va can lead to an assertion
failure in page_set_flags.  Sanity check the value of
reserved_va and print an error message instead.  Do not
allocate a commpage at all for m-profile cpus.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/elfload.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index b068676340..0529430b1d 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -422,12 +422,32 @@ enum {
 
 static bool init_guest_commpage(void)
 {
-    abi_ptr commpage = HI_COMMPAGE & -qemu_host_page_size;
-    void *want = g2h_untagged(commpage);
-    void *addr = mmap(want, qemu_host_page_size, PROT_READ | PROT_WRITE,
-                      MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
+    ARMCPU *cpu = ARM_CPU(thread_cpu);
+    abi_ptr want = HI_COMMPAGE & TARGET_PAGE_MASK;
+    abi_ptr addr;
 
-    if (addr == MAP_FAILED) {
+    /*
+     * M-profile allocates maximum of 2GB address space, so can never
+     * allocate the commpage.  Skip it.
+     */
+    if (arm_feature(&cpu->env, ARM_FEATURE_M)) {
+        return true;
+    }
+
+    /*
+     * If reserved_va does not cover the commpage, we get an assert
+     * in page_set_flags.  Produce an intelligent error instead.
+     */
+    if (reserved_va != 0 && want + TARGET_PAGE_SIZE - 1 > reserved_va) {
+        error_report("Allocating guest commpage: -R 0x%" PRIx64 " too small",
+                     (uint64_t)reserved_va + 1);
+        exit(EXIT_FAILURE);
+    }
+
+    addr = target_mmap(want, TARGET_PAGE_SIZE, PROT_READ | PROT_WRITE,
+                       MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
+
+    if (addr == -1) {
         perror("Allocating guest commpage");
         exit(EXIT_FAILURE);
     }
@@ -436,15 +456,12 @@ static bool init_guest_commpage(void)
     }
 
     /* Set kernel helper versions; rest of page is 0.  */
-    __put_user(5, (uint32_t *)g2h_untagged(0xffff0ffcu));
+    put_user_u32(5, 0xffff0ffcu);
 
-    if (mprotect(addr, qemu_host_page_size, PROT_READ)) {
+    if (target_mprotect(addr, qemu_host_page_size, PROT_READ | PROT_EXEC)) {
         perror("Protecting guest commpage");
         exit(EXIT_FAILURE);
     }
-
-    page_set_flags(commpage, commpage | ~qemu_host_page_mask,
-                   PAGE_READ | PAGE_EXEC | PAGE_VALID);
     return true;
 }
 
-- 
2.34.1
Re: [PATCH for-8.0 11/11] linux-user/arm: Take more care allocating commpage
Posted by Alex Bennée 2 years ago
Richard Henderson <richard.henderson@linaro.org> writes:

> User setting of -R reserved_va can lead to an assertion
> failure in page_set_flags.  Sanity check the value of
> reserved_va and print an error message instead.  Do not
> allocate a commpage at all for m-profile cpus.

I see this:

  TEST    convd on i386
qemu-i386: Unable to reserve 0x100000000 bytes of virtual address space
at 0x8000 (File exists) for use as guest address space (check your
virtual memory ulimit setting, min_mmap_addr or reserve less using -R
option)

on the ubuntu aarch64 static build:

  https://gitlab.com/stsquad/qemu/-/jobs/4003523064

>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/elfload.c | 37 +++++++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index b068676340..0529430b1d 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -422,12 +422,32 @@ enum {
>  
>  static bool init_guest_commpage(void)
>  {
> -    abi_ptr commpage = HI_COMMPAGE & -qemu_host_page_size;
> -    void *want = g2h_untagged(commpage);
> -    void *addr = mmap(want, qemu_host_page_size, PROT_READ | PROT_WRITE,
> -                      MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> +    ARMCPU *cpu = ARM_CPU(thread_cpu);
> +    abi_ptr want = HI_COMMPAGE & TARGET_PAGE_MASK;
> +    abi_ptr addr;
>  
> -    if (addr == MAP_FAILED) {
> +    /*
> +     * M-profile allocates maximum of 2GB address space, so can never
> +     * allocate the commpage.  Skip it.
> +     */
> +    if (arm_feature(&cpu->env, ARM_FEATURE_M)) {
> +        return true;
> +    }
> +
> +    /*
> +     * If reserved_va does not cover the commpage, we get an assert
> +     * in page_set_flags.  Produce an intelligent error instead.
> +     */
> +    if (reserved_va != 0 && want + TARGET_PAGE_SIZE - 1 > reserved_va) {
> +        error_report("Allocating guest commpage: -R 0x%" PRIx64 " too small",
> +                     (uint64_t)reserved_va + 1);
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    addr = target_mmap(want, TARGET_PAGE_SIZE, PROT_READ | PROT_WRITE,
> +                       MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> +
> +    if (addr == -1) {
>          perror("Allocating guest commpage");
>          exit(EXIT_FAILURE);
>      }
> @@ -436,15 +456,12 @@ static bool init_guest_commpage(void)
>      }
>  
>      /* Set kernel helper versions; rest of page is 0.  */
> -    __put_user(5, (uint32_t *)g2h_untagged(0xffff0ffcu));
> +    put_user_u32(5, 0xffff0ffcu);
>  
> -    if (mprotect(addr, qemu_host_page_size, PROT_READ)) {
> +    if (target_mprotect(addr, qemu_host_page_size, PROT_READ | PROT_EXEC)) {
>          perror("Protecting guest commpage");
>          exit(EXIT_FAILURE);
>      }
> -
> -    page_set_flags(commpage, commpage | ~qemu_host_page_mask,
> -                   PAGE_READ | PAGE_EXEC | PAGE_VALID);
>      return true;
>  }


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH for-8.0 11/11] linux-user/arm: Take more care allocating commpage
Posted by Richard Henderson 2 years ago
On 3/27/23 01:38, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> User setting of -R reserved_va can lead to an assertion
>> failure in page_set_flags.  Sanity check the value of
>> reserved_va and print an error message instead.  Do not
>> allocate a commpage at all for m-profile cpus.
> 
> I see this:
> 
>    TEST    convd on i386
> qemu-i386: Unable to reserve 0x100000000 bytes of virtual address space
> at 0x8000 (File exists) for use as guest address space (check your
> virtual memory ulimit setting, min_mmap_addr or reserve less using -R
> option)
> 
> on the ubuntu aarch64 static build:
> 
>    https://gitlab.com/stsquad/qemu/-/jobs/4003523064

Odd.  Works on aarch64.ci.qemu.org outside of the gitlab environment.


r~

Re: [PATCH for-8.0 11/11] linux-user/arm: Take more care allocating commpage
Posted by Alex Bennée 2 years ago
Richard Henderson <richard.henderson@linaro.org> writes:

> On 3/27/23 01:38, Alex Bennée wrote:
>> Richard Henderson <richard.henderson@linaro.org> writes:
>> 
>>> User setting of -R reserved_va can lead to an assertion
>>> failure in page_set_flags.  Sanity check the value of
>>> reserved_va and print an error message instead.  Do not
>>> allocate a commpage at all for m-profile cpus.
>> I see this:
>>    TEST    convd on i386
>> qemu-i386: Unable to reserve 0x100000000 bytes of virtual address space
>> at 0x8000 (File exists) for use as guest address space (check your
>> virtual memory ulimit setting, min_mmap_addr or reserve less using -R
>> option)
>> on the ubuntu aarch64 static build:
>>    https://gitlab.com/stsquad/qemu/-/jobs/4003523064
>
> Odd.  Works on aarch64.ci.qemu.org outside of the gitlab environment.

15:50:17 [alex@aarch64:~/l/q/b/ci.all.linux.static] review/tcg-queue-for-8.0↓1|… + head config.log
# QEMU configure log Mon 27 Mar 10:20:07 UTC 2023
# Configured with: '../../configure' '--enable-debug' '--static' '--disable-system' '--disable-pie' '--gdb=' '--skip-meson'

>
>
> r~


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH for-8.0 11/11] linux-user/arm: Take more care allocating commpage
Posted by Richard Henderson 2 years ago
On 3/27/23 10:36, Richard Henderson wrote:
> On 3/27/23 01:38, Alex Bennée wrote:
>>
>> Richard Henderson <richard.henderson@linaro.org> writes:
>>
>>> User setting of -R reserved_va can lead to an assertion
>>> failure in page_set_flags.  Sanity check the value of
>>> reserved_va and print an error message instead.  Do not
>>> allocate a commpage at all for m-profile cpus.
>>
>> I see this:
>>
>>    TEST    convd on i386
>> qemu-i386: Unable to reserve 0x100000000 bytes of virtual address space
>> at 0x8000 (File exists) for use as guest address space (check your
>> virtual memory ulimit setting, min_mmap_addr or reserve less using -R
>> option)
>>
>> on the ubuntu aarch64 static build:
>>
>>    https://gitlab.com/stsquad/qemu/-/jobs/4003523064
> 
> Odd.  Works on aarch64.ci.qemu.org outside of the gitlab environment.

Bah.  I forgot --disable-pie.


r~


Re: [PATCH for-8.0 11/11] linux-user/arm: Take more care allocating commpage
Posted by Philippe Mathieu-Daudé 2 years ago
On 27/3/23 10:38, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> User setting of -R reserved_va can lead to an assertion
>> failure in page_set_flags.  Sanity check the value of
>> reserved_va and print an error message instead.  Do not
>> allocate a commpage at all for m-profile cpus.
> 
> I see this:
> 
>    TEST    convd on i386
> qemu-i386: Unable to reserve 0x100000000 bytes of virtual address space
> at 0x8000 (File exists) for use as guest address space (check your
> virtual memory ulimit setting, min_mmap_addr or reserve less using -R
> option)

Maybe revealing some pre-existing issue?
https://gitlab.com/qemu-project/qemu/-/issues/447

> 
> on the ubuntu aarch64 static build:
> 
>    https://gitlab.com/stsquad/qemu/-/jobs/4003523064
> 
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   linux-user/elfload.c | 37 +++++++++++++++++++++++++++----------
>>   1 file changed, 27 insertions(+), 10 deletions(-)