| Message ID | 20230306021307.1879483-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | accel/tcg: Fix page_set_flags and related [#1528] | expand |
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
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~
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
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
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~
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
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
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)
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(+)