[PATCH 1/9] linux-user: Diagnose incorrect -R size

Richard Henderson posted 9 patches 2 years, 11 months 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>, Laurent Vivier <laurent@vivier.eu>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Peter Maydell <peter.maydell@linaro.org>
There is a newer version of this series
[PATCH 1/9] linux-user: Diagnose incorrect -R size
Posted by Richard Henderson 2 years, 11 months ago
Zero is the value for 'off', and should not be used with -R.
We have been enforcing host page alignment for the non-R
fallback of MAX_RESERVED_VA, but failing to enforce for -R.

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

diff --git a/linux-user/main.c b/linux-user/main.c
index 4ff30ff980..f4dea25242 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -395,6 +395,16 @@ static void handle_arg_reserved_va(const char *arg)
         fprintf(stderr, "Unrecognised -R size suffix '%s'\n", p);
         exit(EXIT_FAILURE);
     }
+    if (reserved_va == 0) {
+        fprintf(stderr, "Invalid -R size value 0\n");
+        exit(EXIT_FAILURE);
+    }
+    /* Must be aligned with the host page size as it is used with mmap. */
+    if (reserved_va & qemu_host_page_mask) {
+        fprintf(stderr, "Invalid -R size value %lu: must be aligned mod %lu\n",
+		reserved_va, qemu_host_page_size);
+        exit(EXIT_FAILURE);
+    }
 }
 
 static void handle_arg_singlestep(const char *arg)
-- 
2.34.1
Re: [PATCH 1/9] linux-user: Diagnose incorrect -R size
Posted by Peter Maydell 2 years, 11 months ago
On Mon, 6 Mar 2023 at 02:14, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Zero is the value for 'off', and should not be used with -R.
> We have been enforcing host page alignment for the non-R
> fallback of MAX_RESERVED_VA, but failing to enforce for -R.

I'm pretty sure we have users who specifically use "-R 0" to
ask for "definitely turn off any reserved VA".
Here's a random example from an old gcc bug report:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60681
and somebody using it via the environment variable:
https://www.openembedded.org/pipermail/openembedded-core/2015-January/100572.html

thanks
-- PMM
Re: [PATCH 1/9] linux-user: Diagnose incorrect -R size
Posted by Richard Henderson 2 years, 11 months ago
On 3/6/23 04:56, Peter Maydell wrote:
> On Mon, 6 Mar 2023 at 02:14, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Zero is the value for 'off', and should not be used with -R.
>> We have been enforcing host page alignment for the non-R
>> fallback of MAX_RESERVED_VA, but failing to enforce for -R.
> 
> I'm pretty sure we have users who specifically use "-R 0" to
> ask for "definitely turn off any reserved VA".
> Here's a random example from an old gcc bug report:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60681
> and somebody using it via the environment variable:
> https://www.openembedded.org/pipermail/openembedded-core/2015-January/100572.html

Odd.

Well, it won't actually have the effect of "definitely turn off", it will merely leave 
things as per the default, which *will* enable reserved va for 32-bit guests on 64-bit hosts.

The only remaining question is whether we diagnose this oddness or silently accept it.  It 
feels like someone playing with options they don't actually understand and an error is 
warranted.


r~
Re: [PATCH 1/9] linux-user: Diagnose incorrect -R size
Posted by Peter Maydell 2 years, 11 months ago
On Mon, 6 Mar 2023 at 21:24, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/6/23 04:56, Peter Maydell wrote:
> > On Mon, 6 Mar 2023 at 02:14, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> Zero is the value for 'off', and should not be used with -R.
> >> We have been enforcing host page alignment for the non-R
> >> fallback of MAX_RESERVED_VA, but failing to enforce for -R.
> >
> > I'm pretty sure we have users who specifically use "-R 0" to
> > ask for "definitely turn off any reserved VA".
> > Here's a random example from an old gcc bug report:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60681
> > and somebody using it via the environment variable:
> > https://www.openembedded.org/pipermail/openembedded-core/2015-January/100572.html
>
> Odd.
>
> Well, it won't actually have the effect of "definitely turn off", it will merely leave
> things as per the default, which *will* enable reserved va for 32-bit guests on 64-bit hosts.
>
> The only remaining question is whether we diagnose this oddness or silently accept it.  It
> feels like someone playing with options they don't actually understand and an error is
> warranted.

I'm pretty sure I've issued the advice "turn off the reserved
area stuff with -R 0" in the past, for working around various
QEMU bugs where it wasn't able to allocate the whole reserved
area it wanted to but the guest program didn't actually care.

thanks
-- PMM
Re: [PATCH 1/9] linux-user: Diagnose incorrect -R size
Posted by Peter Maydell 2 years, 11 months ago
On Tue, 7 Mar 2023 at 10:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 6 Mar 2023 at 21:24, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > On 3/6/23 04:56, Peter Maydell wrote:
> > > On Mon, 6 Mar 2023 at 02:14, Richard Henderson
> > > <richard.henderson@linaro.org> wrote:
> > >>
> > >> Zero is the value for 'off', and should not be used with -R.
> > >> We have been enforcing host page alignment for the non-R
> > >> fallback of MAX_RESERVED_VA, but failing to enforce for -R.
> > >
> > > I'm pretty sure we have users who specifically use "-R 0" to
> > > ask for "definitely turn off any reserved VA".
> > > Here's a random example from an old gcc bug report:
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60681
> > > and somebody using it via the environment variable:
> > > https://www.openembedded.org/pipermail/openembedded-core/2015-January/100572.html
> >
> > Odd.
> >
> > Well, it won't actually have the effect of "definitely turn off", it will merely leave
> > things as per the default, which *will* enable reserved va for 32-bit guests on 64-bit hosts.
> >
> > The only remaining question is whether we diagnose this oddness or silently accept it.  It
> > feels like someone playing with options they don't actually understand and an error is
> > warranted.
>
> I'm pretty sure I've issued the advice "turn off the reserved
> area stuff with -R 0" in the past, for working around various
> QEMU bugs where it wasn't able to allocate the whole reserved
> area it wanted to but the guest program didn't actually care.

It looks like we (inadvertently) broke "-R 0 means turn off"
in 2019 with commit dc18baaef36d95e5; prior to that the
64-on-32 default was set by the initial value of the global
variable and could be overridden on the command line. After
that we ended up doing the default-value stuff after the
command line was parsed instead.

thanks
-- PMM
Re: [PATCH 1/9] linux-user: Diagnose incorrect -R size
Posted by Richard Henderson 2 years, 10 months ago
On 3/7/23 02:17, Peter Maydell wrote:
> It looks like we (inadvertently) broke "-R 0 means turn off"
> in 2019 with commit dc18baaef36d95e5; prior to that the
> 64-on-32 default was set by the initial value of the global
> variable and could be overridden on the command line. After
> that we ended up doing the default-value stuff after the
> command line was parsed instead.

(Not 64-on-32, but 32-on-64.)

I don't understand how 32-on-64 would ever work without reserved_va.  The host kernel 
would otherwise place mmap blocks anywhere it chooses, which may not be within 4GB of any 
given guest_base.

I would not characterize that patch as "inadvertently broke" but "fixed bug but didn't 
record that fact in the commit message".


r~
Re: [PATCH 1/9] linux-user: Diagnose incorrect -R size
Posted by Peter Maydell 2 years, 10 months ago
On Fri, 17 Mar 2023 at 14:46, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/7/23 02:17, Peter Maydell wrote:
> > It looks like we (inadvertently) broke "-R 0 means turn off"
> > in 2019 with commit dc18baaef36d95e5; prior to that the
> > 64-on-32 default was set by the initial value of the global
> > variable and could be overridden on the command line. After
> > that we ended up doing the default-value stuff after the
> > command line was parsed instead.
>
> (Not 64-on-32, but 32-on-64.)
>
> I don't understand how 32-on-64 would ever work without reserved_va.  The host kernel
> would otherwise place mmap blocks anywhere it chooses, which may not be within 4GB of any
> given guest_base.

I think most of the use cases weren't doing mmap of any
kind. The gcc test suite is one example of that.

-- PMM
Re: [PATCH 1/9] linux-user: Diagnose incorrect -R size
Posted by Peter Maydell 2 years, 10 months ago
On Fri, 17 Mar 2023 at 14:57, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 17 Mar 2023 at 14:46, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > On 3/7/23 02:17, Peter Maydell wrote:
> > > It looks like we (inadvertently) broke "-R 0 means turn off"
> > > in 2019 with commit dc18baaef36d95e5; prior to that the
> > > 64-on-32 default was set by the initial value of the global
> > > variable and could be overridden on the command line. After
> > > that we ended up doing the default-value stuff after the
> > > command line was parsed instead.
> >
> > (Not 64-on-32, but 32-on-64.)
> >
> > I don't understand how 32-on-64 would ever work without reserved_va.  The host kernel
> > would otherwise place mmap blocks anywhere it chooses, which may not be within 4GB of any
> > given guest_base.
>
> I think most of the use cases weren't doing mmap of any
> kind. The gcc test suite is one example of that.

...but in any case, looking at the linux-user/mmap.c
code it doesn't let the kernel give it any old host
address, even in the no-reserved_va code path:
mmap_find_vma() calls mmap() with a hint address it wants
the kernel to try, and it refuses to use addresses which
aren't reachable by the guest (as defined by h2g_valid()).
So as long as the guest program isn't a really heavy
mmap user it will be fine even with a 0 reserved_va.

thanks
-- PMM