| Message ID | 20200527100546.29297-3-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | some linux-user guest_base fixes | expand |
сре, 27. мај 2020. у 12:07 Alex Bennée <alex.bennee@linaro.org> је написао/ла: > > We rely on the pointer to wrap when accessing the high address of the > COMMPAGE so it lands somewhere reasonable. However on 32 bit hosts we > cannot afford just to map the entire 4gb address range. The old mmap > trial and error code handled this by just checking we could map both > the guest_base and the computed COMMPAGE address. > > We can't just manipulate loadaddr to get what we want so we introduce > an offset which pgb_find_hole can apply when looking for a gap for > guest_base that ensures there is space left to map the COMMPAGE > afterwards. > > This is arguably a little inefficient for the one 32 bit > value (kuser_helper_version) we need to keep there given all the > actual code entries are picked up during the translation phase. > > Fixes: ee94743034b > Bug: https://bugs.launchpad.net/qemu/+bug/1880225 For the scenario in this bug report, for today's master, before and after applying this patch: BEFORE: $ ~/Build/qemu-master/build-gcc/arm-linux-user/qemu-arm ./toupper_string-arm qemu-arm: /home/rtrk/Build/qemu-master/linux-user/elfload.c:2294: probe_guest_base: Assertion `have_guest_base' failed. Aborted AFTER: $ ~/Build/qemu-master/build-gcc/arm-linux-user/qemu-arm ./toupper_string-arm CONTROL RESULT: (toupper_string) nwlrbbmqbhcdarz owkkyhiddqscdxr jmowfrxsjybldbe fsarcbynecdyggx xpklorellnmpapq NWLRBBMQBHCDARZ OWKKYHIDDQSCDXR JMOWFRXSJYBLDBE FSARCBYNECDYGGX XPKLORELLNMPAPQ So, it works in my test bed. Tested-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> > Cc: Bug 1880225 <1880225@bugs.launchpad.net> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Richard Henderson <richard.henderson@linaro.org> > Cc: Peter Maydell <peter.maydell@linaro.org> > --- > linux-user/elfload.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/linux-user/elfload.c b/linux-user/elfload.c > index d6027867a1a..31defce95b5 100644 > --- a/linux-user/elfload.c > +++ b/linux-user/elfload.c > @@ -2145,7 +2145,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon > > /* Return value for guest_base, or -1 if no hole found. */ > static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size, > - long align) > + long align, uintptr_t offset) > { > GSList *maps, *iter; > uintptr_t this_start, this_end, next_start, brk; > @@ -2171,7 +2171,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size, > > this_end = ((MapInfo *)iter->data)->start; > next_start = ((MapInfo *)iter->data)->end; > - align_start = ROUND_UP(this_start, align); > + align_start = ROUND_UP(this_start + offset, align); > > /* Skip holes that are too small. */ > if (align_start >= this_end) { > @@ -2221,6 +2221,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, > { > uintptr_t loaddr = orig_loaddr; > uintptr_t hiaddr = orig_hiaddr; > + uintptr_t offset = 0; > uintptr_t addr; > > if (hiaddr != orig_hiaddr) { > @@ -2234,18 +2235,19 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, > if (ARM_COMMPAGE) { > /* > * Extend the allocation to include the commpage. > - * For a 64-bit host, this is just 4GiB; for a 32-bit host, > - * the address arithmetic will wrap around, but the difference > - * will produce the correct allocation size. > + * For a 64-bit host, this is just 4GiB; for a 32-bit host we > + * need to ensure there is space bellow the guest_base so we > + * can map the commpage in the place needed when the address > + * arithmetic wraps around. > */ > if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) { > hiaddr = (uintptr_t)4 << 30; > } else { > - loaddr = ARM_COMMPAGE & -align; > + offset = (128 * KiB); > } > } > > - addr = pgb_find_hole(loaddr, hiaddr - loaddr, align); > + addr = pgb_find_hole(loaddr, hiaddr - loaddr, align, offset); > if (addr == -1) { > /* > * If ARM_COMMPAGE, there *might* be a non-consecutive allocation > @@ -2280,7 +2282,7 @@ static void pgb_dynamic(const char *image_name, long align) > * just above that, and maximises the positive guest addresses. > */ > commpage = ARM_COMMPAGE & -align; > - addr = pgb_find_hole(commpage, -commpage, align); > + addr = pgb_find_hole(commpage, -commpage, align, 0); > assert(addr != -1); > guest_base = addr; > } > -- > 2.20.1 > >
сре, 27. мај 2020. у 14:05 Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> је написао/ла: > > сре, 27. мај 2020. у 12:07 Alex Bennée <alex.bennee@linaro.org> је написао/ла: > > > > We rely on the pointer to wrap when accessing the high address of the > > COMMPAGE so it lands somewhere reasonable. However on 32 bit hosts we > > cannot afford just to map the entire 4gb address range. The old mmap > > trial and error code handled this by just checking we could map both > > the guest_base and the computed COMMPAGE address. > > > > We can't just manipulate loadaddr to get what we want so we introduce > > an offset which pgb_find_hole can apply when looking for a gap for > > guest_base that ensures there is space left to map the COMMPAGE > > afterwards. > > > > This is arguably a little inefficient for the one 32 bit > > value (kuser_helper_version) we need to keep there given all the > > actual code entries are picked up during the translation phase. > > > > Fixes: ee94743034b > > Bug: https://bugs.launchpad.net/qemu/+bug/1880225 > > For the scenario in this bug report, for today's master, before and after > applying this patch: > I am not sure how significant is this info, but I executed the test without applying patch 1/3, so only 2/3 was applied in the case "AFTER". Aleksandar > BEFORE: > > $ ~/Build/qemu-master/build-gcc/arm-linux-user/qemu-arm ./toupper_string-arm > qemu-arm: /home/rtrk/Build/qemu-master/linux-user/elfload.c:2294: > probe_guest_base: Assertion `have_guest_base' failed. > Aborted > > AFTER: > > $ ~/Build/qemu-master/build-gcc/arm-linux-user/qemu-arm ./toupper_string-arm > CONTROL RESULT: (toupper_string) > nwlrbbmqbhcdarz owkkyhiddqscdxr jmowfrxsjybldbe fsarcbynecdyggx xpklorellnmpapq > NWLRBBMQBHCDARZ OWKKYHIDDQSCDXR JMOWFRXSJYBLDBE FSARCBYNECDYGGX XPKLORELLNMPAPQ > > So, it works in my test bed. > > Tested-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> > > > Cc: Bug 1880225 <1880225@bugs.launchpad.net> > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > Cc: Richard Henderson <richard.henderson@linaro.org> > > Cc: Peter Maydell <peter.maydell@linaro.org> > > --- > > linux-user/elfload.c | 18 ++++++++++-------- > > 1 file changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/linux-user/elfload.c b/linux-user/elfload.c > > index d6027867a1a..31defce95b5 100644 > > --- a/linux-user/elfload.c > > +++ b/linux-user/elfload.c > > @@ -2145,7 +2145,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon > > > > /* Return value for guest_base, or -1 if no hole found. */ > > static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size, > > - long align) > > + long align, uintptr_t offset) > > { > > GSList *maps, *iter; > > uintptr_t this_start, this_end, next_start, brk; > > @@ -2171,7 +2171,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size, > > > > this_end = ((MapInfo *)iter->data)->start; > > next_start = ((MapInfo *)iter->data)->end; > > - align_start = ROUND_UP(this_start, align); > > + align_start = ROUND_UP(this_start + offset, align); > > > > /* Skip holes that are too small. */ > > if (align_start >= this_end) { > > @@ -2221,6 +2221,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, > > { > > uintptr_t loaddr = orig_loaddr; > > uintptr_t hiaddr = orig_hiaddr; > > + uintptr_t offset = 0; > > uintptr_t addr; > > > > if (hiaddr != orig_hiaddr) { > > @@ -2234,18 +2235,19 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, > > if (ARM_COMMPAGE) { > > /* > > * Extend the allocation to include the commpage. > > - * For a 64-bit host, this is just 4GiB; for a 32-bit host, > > - * the address arithmetic will wrap around, but the difference > > - * will produce the correct allocation size. > > + * For a 64-bit host, this is just 4GiB; for a 32-bit host we > > + * need to ensure there is space bellow the guest_base so we > > + * can map the commpage in the place needed when the address > > + * arithmetic wraps around. > > */ > > if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) { > > hiaddr = (uintptr_t)4 << 30; > > } else { > > - loaddr = ARM_COMMPAGE & -align; > > + offset = (128 * KiB); > > } > > } > > > > - addr = pgb_find_hole(loaddr, hiaddr - loaddr, align); > > + addr = pgb_find_hole(loaddr, hiaddr - loaddr, align, offset); > > if (addr == -1) { > > /* > > * If ARM_COMMPAGE, there *might* be a non-consecutive allocation > > @@ -2280,7 +2282,7 @@ static void pgb_dynamic(const char *image_name, long align) > > * just above that, and maximises the positive guest addresses. > > */ > > commpage = ARM_COMMPAGE & -align; > > - addr = pgb_find_hole(commpage, -commpage, align); > > + addr = pgb_find_hole(commpage, -commpage, align, 0); > > assert(addr != -1); > > guest_base = addr; > > } > > -- > > 2.20.1 > > > >
Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> writes: > сре, 27. мај 2020. у 14:05 Aleksandar Markovic > <aleksandar.qemu.devel@gmail.com> је написао/ла: >> >> сре, 27. мај 2020. у 12:07 Alex Bennée <alex.bennee@linaro.org> је написао/ла: >> > >> > We rely on the pointer to wrap when accessing the high address of the >> > COMMPAGE so it lands somewhere reasonable. However on 32 bit hosts we >> > cannot afford just to map the entire 4gb address range. The old mmap >> > trial and error code handled this by just checking we could map both >> > the guest_base and the computed COMMPAGE address. >> > >> > We can't just manipulate loadaddr to get what we want so we introduce >> > an offset which pgb_find_hole can apply when looking for a gap for >> > guest_base that ensures there is space left to map the COMMPAGE >> > afterwards. >> > >> > This is arguably a little inefficient for the one 32 bit >> > value (kuser_helper_version) we need to keep there given all the >> > actual code entries are picked up during the translation phase. >> > >> > Fixes: ee94743034b >> > Bug: https://bugs.launchpad.net/qemu/+bug/1880225 >> >> For the scenario in this bug report, for today's master, before and after >> applying this patch: >> > > I am not sure how significant is this info, but I executed the test > without applying patch 1/3, so only 2/3 was applied in the case > "AFTER". That is expected. The other fix only affects binaries run inside a /proc-less chroot and the final patch is a test case for COMMPAGE support. -- Alex Bennée
On 5/27/20 3:05 AM, Alex Bennée wrote: > @@ -2145,7 +2145,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon > > /* Return value for guest_base, or -1 if no hole found. */ > static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size, > - long align) > + long align, uintptr_t offset) > { > GSList *maps, *iter; > uintptr_t this_start, this_end, next_start, brk; > @@ -2171,7 +2171,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size, > > this_end = ((MapInfo *)iter->data)->start; > next_start = ((MapInfo *)iter->data)->end; > - align_start = ROUND_UP(this_start, align); > + align_start = ROUND_UP(this_start + offset, align); > > /* Skip holes that are too small. */ I suppose offset is supposed to mean we start from -offset? You didn't update pgb_find_hole_fallback. > - loaddr = ARM_COMMPAGE & -align; > + offset = (128 * KiB); Why 128K? Surely this should be an expression against ARM_COMMPAGE. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 5/27/20 3:05 AM, Alex Bennée wrote: >> @@ -2145,7 +2145,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon >> >> /* Return value for guest_base, or -1 if no hole found. */ >> static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size, >> - long align) >> + long align, uintptr_t offset) >> { >> GSList *maps, *iter; >> uintptr_t this_start, this_end, next_start, brk; >> @@ -2171,7 +2171,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size, >> >> this_end = ((MapInfo *)iter->data)->start; >> next_start = ((MapInfo *)iter->data)->end; >> - align_start = ROUND_UP(this_start, align); >> + align_start = ROUND_UP(this_start + offset, align); >> >> /* Skip holes that are too small. */ > > I suppose offset is supposed to mean we start from -offset? Well guest_base will start higher meaning we have space for the commpage beneath it. > You didn't update > pgb_find_hole_fallback. Fixed. > >> - loaddr = ARM_COMMPAGE & -align; >> + offset = (128 * KiB); > > Why 128K? Surely this should be an expression against ARM_COMMPAGE. In theory: offset = -(ARM_COMMPAGE & -align); should do the trick but I found it failed every now and again. Frustratingly putting printfs in made it go away so in frustration I just upped the offset until it stopped happening. I do kinda wish rr worked on i386 :-/ > > > r~ -- Alex Bennée
Alex Bennée <alex.bennee@linaro.org> writes: > Richard Henderson <richard.henderson@linaro.org> writes: > >> On 5/27/20 3:05 AM, Alex Bennée wrote: >>> @@ -2145,7 +2145,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon >>> >>> /* Return value for guest_base, or -1 if no hole found. */ >>> static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size, >>> - long align) >>> + long align, uintptr_t offset) >>> { >>> GSList *maps, *iter; >>> uintptr_t this_start, this_end, next_start, brk; >>> @@ -2171,7 +2171,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size, >>> >>> this_end = ((MapInfo *)iter->data)->start; >>> next_start = ((MapInfo *)iter->data)->end; >>> - align_start = ROUND_UP(this_start, align); >>> + align_start = ROUND_UP(this_start + offset, align); >>> >>> /* Skip holes that are too small. */ >> >> I suppose offset is supposed to mean we start from -offset? > > Well guest_base will start higher meaning we have space for the > commpage beneath it. > >> You didn't update >> pgb_find_hole_fallback. > > Fixed. > >> >>> - loaddr = ARM_COMMPAGE & -align; >>> + offset = (128 * KiB); >> >> Why 128K? Surely this should be an expression against ARM_COMMPAGE. > > In theory: > > offset = -(ARM_COMMPAGE & -align); > > should do the trick but I found it failed every now and again. > Frustratingly putting printfs in made it go away so in frustration I > just upped the offset until it stopped happening. > > I do kinda wish rr worked on i386 :-/ Ahh all I needed was a MAP_FIXED for init_commpage -- Alex Bennée
diff --git a/linux-user/elfload.c b/linux-user/elfload.c index d6027867a1a..31defce95b5 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -2145,7 +2145,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon /* Return value for guest_base, or -1 if no hole found. */ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size, - long align) + long align, uintptr_t offset) { GSList *maps, *iter; uintptr_t this_start, this_end, next_start, brk; @@ -2171,7 +2171,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size, this_end = ((MapInfo *)iter->data)->start; next_start = ((MapInfo *)iter->data)->end; - align_start = ROUND_UP(this_start, align); + align_start = ROUND_UP(this_start + offset, align); /* Skip holes that are too small. */ if (align_start >= this_end) { @@ -2221,6 +2221,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, { uintptr_t loaddr = orig_loaddr; uintptr_t hiaddr = orig_hiaddr; + uintptr_t offset = 0; uintptr_t addr; if (hiaddr != orig_hiaddr) { @@ -2234,18 +2235,19 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, if (ARM_COMMPAGE) { /* * Extend the allocation to include the commpage. - * For a 64-bit host, this is just 4GiB; for a 32-bit host, - * the address arithmetic will wrap around, but the difference - * will produce the correct allocation size. + * For a 64-bit host, this is just 4GiB; for a 32-bit host we + * need to ensure there is space bellow the guest_base so we + * can map the commpage in the place needed when the address + * arithmetic wraps around. */ if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) { hiaddr = (uintptr_t)4 << 30; } else { - loaddr = ARM_COMMPAGE & -align; + offset = (128 * KiB); } } - addr = pgb_find_hole(loaddr, hiaddr - loaddr, align); + addr = pgb_find_hole(loaddr, hiaddr - loaddr, align, offset); if (addr == -1) { /* * If ARM_COMMPAGE, there *might* be a non-consecutive allocation @@ -2280,7 +2282,7 @@ static void pgb_dynamic(const char *image_name, long align) * just above that, and maximises the positive guest addresses. */ commpage = ARM_COMMPAGE & -align; - addr = pgb_find_hole(commpage, -commpage, align); + addr = pgb_find_hole(commpage, -commpage, align, 0); assert(addr != -1); guest_base = addr; }
We rely on the pointer to wrap when accessing the high address of the COMMPAGE so it lands somewhere reasonable. However on 32 bit hosts we cannot afford just to map the entire 4gb address range. The old mmap trial and error code handled this by just checking we could map both the guest_base and the computed COMMPAGE address. We can't just manipulate loadaddr to get what we want so we introduce an offset which pgb_find_hole can apply when looking for a gap for guest_base that ensures there is space left to map the COMMPAGE afterwards. This is arguably a little inefficient for the one 32 bit value (kuser_helper_version) we need to keep there given all the actual code entries are picked up during the translation phase. Fixes: ee94743034b Bug: https://bugs.launchpad.net/qemu/+bug/1880225 Cc: Bug 1880225 <1880225@bugs.launchpad.net> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Cc: Richard Henderson <richard.henderson@linaro.org> Cc: Peter Maydell <peter.maydell@linaro.org> --- linux-user/elfload.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) -- 2.20.1