| Message ID | 20221217184823.3606676-1-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/tcg: Drop PAGE_RESERVED for CONFIG_BSD | expand |
On 12/17/22 10:48, Richard Henderson wrote: > Make bsd-user match linux-user in not marking host pages > as reserved. This isn't especially effective anyway, as > it doesn't take into account any heap memory that qemu > may allocate after startup. > > Cc: Warner Losh <imp@bsdimp.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > > I started to simply fix up this code to match my user-only interval-tree > patch set, as L1_MAP_ADDR_SPACE_BITS gets removed from translate-all.c, > but then I decided to remove it all. A further justification for this: The actual PAGE_RESERVED bit is write-only; there is no other reference to this bit elsewhere. r~ > > > r~ > > --- > accel/tcg/translate-all.c | 65 --------------------------------------- > 1 file changed, 65 deletions(-) > > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c > index b964ea44d7..48e9d70b4e 100644 > --- a/accel/tcg/translate-all.c > +++ b/accel/tcg/translate-all.c > @@ -354,71 +354,6 @@ void page_init(void) > { > page_size_init(); > page_table_config_init(); > - > -#if defined(CONFIG_BSD) && defined(CONFIG_USER_ONLY) > - { > -#ifdef HAVE_KINFO_GETVMMAP > - struct kinfo_vmentry *freep; > - int i, cnt; > - > - freep = kinfo_getvmmap(getpid(), &cnt); > - if (freep) { > - mmap_lock(); > - for (i = 0; i < cnt; i++) { > - unsigned long startaddr, endaddr; > - > - startaddr = freep[i].kve_start; > - endaddr = freep[i].kve_end; > - if (h2g_valid(startaddr)) { > - startaddr = h2g(startaddr) & TARGET_PAGE_MASK; > - > - if (h2g_valid(endaddr)) { > - endaddr = h2g(endaddr); > - page_set_flags(startaddr, endaddr, PAGE_RESERVED); > - } else { > -#if TARGET_ABI_BITS <= L1_MAP_ADDR_SPACE_BITS > - endaddr = ~0ul; > - page_set_flags(startaddr, endaddr, PAGE_RESERVED); > -#endif > - } > - } > - } > - free(freep); > - mmap_unlock(); > - } > -#else > - FILE *f; > - > - last_brk = (unsigned long)sbrk(0); > - > - f = fopen("/compat/linux/proc/self/maps", "r"); > - if (f) { > - mmap_lock(); > - > - do { > - unsigned long startaddr, endaddr; > - int n; > - > - n = fscanf(f, "%lx-%lx %*[^\n]\n", &startaddr, &endaddr); > - > - if (n == 2 && h2g_valid(startaddr)) { > - startaddr = h2g(startaddr) & TARGET_PAGE_MASK; > - > - if (h2g_valid(endaddr)) { > - endaddr = h2g(endaddr); > - } else { > - endaddr = ~0ul; > - } > - page_set_flags(startaddr, endaddr, PAGE_RESERVED); > - } > - } while (!feof(f)); > - > - fclose(f); > - mmap_unlock(); > - } > -#endif > - } > -#endif > } > > PageDesc *page_find_alloc(tb_page_addr_t index, bool alloc)
On Sat, Dec 17, 2022 at 11:48 AM Richard Henderson < richard.henderson@linaro.org> wrote: > Make bsd-user match linux-user in not marking host pages > as reserved. This isn't especially effective anyway, as > it doesn't take into account any heap memory that qemu > may allocate after startup. > > Cc: Warner Losh <imp@bsdimp.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > > I started to simply fix up this code to match my user-only interval-tree > patch set, as L1_MAP_ADDR_SPACE_BITS gets removed from translate-all.c, > but then I decided to remove it all. > I think this is fine. We already do a translation for addresses so marking this as 'reserved' doesn't help that much. We need to map memory into a contiguous guess-address-space, but the underlying host memory needn't be contiguous at all. I've not yet tested this, but would like to. What's your timeline on getting this done? Warner > r~ > > --- > accel/tcg/translate-all.c | 65 --------------------------------------- > 1 file changed, 65 deletions(-) > > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c > index b964ea44d7..48e9d70b4e 100644 > --- a/accel/tcg/translate-all.c > +++ b/accel/tcg/translate-all.c > @@ -354,71 +354,6 @@ void page_init(void) > { > page_size_init(); > page_table_config_init(); > - > -#if defined(CONFIG_BSD) && defined(CONFIG_USER_ONLY) > - { > -#ifdef HAVE_KINFO_GETVMMAP > - struct kinfo_vmentry *freep; > - int i, cnt; > - > - freep = kinfo_getvmmap(getpid(), &cnt); > - if (freep) { > - mmap_lock(); > - for (i = 0; i < cnt; i++) { > - unsigned long startaddr, endaddr; > - > - startaddr = freep[i].kve_start; > - endaddr = freep[i].kve_end; > - if (h2g_valid(startaddr)) { > - startaddr = h2g(startaddr) & TARGET_PAGE_MASK; > - > - if (h2g_valid(endaddr)) { > - endaddr = h2g(endaddr); > - page_set_flags(startaddr, endaddr, PAGE_RESERVED); > - } else { > -#if TARGET_ABI_BITS <= L1_MAP_ADDR_SPACE_BITS > - endaddr = ~0ul; > - page_set_flags(startaddr, endaddr, PAGE_RESERVED); > -#endif > - } > - } > - } > - free(freep); > - mmap_unlock(); > - } > -#else > - FILE *f; > - > - last_brk = (unsigned long)sbrk(0); > - > - f = fopen("/compat/linux/proc/self/maps", "r"); > - if (f) { > - mmap_lock(); > - > - do { > - unsigned long startaddr, endaddr; > - int n; > - > - n = fscanf(f, "%lx-%lx %*[^\n]\n", &startaddr, &endaddr); > - > - if (n == 2 && h2g_valid(startaddr)) { > - startaddr = h2g(startaddr) & TARGET_PAGE_MASK; > - > - if (h2g_valid(endaddr)) { > - endaddr = h2g(endaddr); > - } else { > - endaddr = ~0ul; > - } > - page_set_flags(startaddr, endaddr, PAGE_RESERVED); > - } > - } while (!feof(f)); > - > - fclose(f); > - mmap_unlock(); > - } > -#endif > - } > -#endif > } > > PageDesc *page_find_alloc(tb_page_addr_t index, bool alloc) > -- > 2.34.1 > >
On 20/12/22 20:31, Richard Henderson wrote: > On 12/17/22 10:48, Richard Henderson wrote: >> Make bsd-user match linux-user in not marking host pages >> as reserved. This isn't especially effective anyway, as >> it doesn't take into account any heap memory that qemu >> may allocate after startup. >> >> Cc: Warner Losh <imp@bsdimp.com> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> >> I started to simply fix up this code to match my user-only interval-tree >> patch set, as L1_MAP_ADDR_SPACE_BITS gets removed from translate-all.c, >> but then I decided to remove it all. > > A further justification for this: The actual PAGE_RESERVED bit is > write-only; there is no other reference to this bit elsewhere. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Richard Henderson <richard.henderson@linaro.org> writes: > Make bsd-user match linux-user in not marking host pages > as reserved. This isn't especially effective anyway, as > it doesn't take into account any heap memory that qemu > may allocate after startup. > > Cc: Warner Losh <imp@bsdimp.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Tested-by: Alex Bennée <alex.bennee@linaro.org>
On 12/20/22 14:09, Warner Losh wrote: > > > On Sat, Dec 17, 2022 at 11:48 AM Richard Henderson <richard.henderson@linaro.org > <mailto:richard.henderson@linaro.org>> wrote: > > Make bsd-user match linux-user in not marking host pages > as reserved. This isn't especially effective anyway, as > it doesn't take into account any heap memory that qemu > may allocate after startup. > > Cc: Warner Losh <imp@bsdimp.com <mailto:imp@bsdimp.com>> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org > <mailto:richard.henderson@linaro.org>> > --- > > I started to simply fix up this code to match my user-only interval-tree > patch set, as L1_MAP_ADDR_SPACE_BITS gets removed from translate-all.c, > but then I decided to remove it all. > > > I think this is fine. We already do a translation for addresses so marking this as 'reserved' > doesn't help that much. We need to map memory into a contiguous guess-address-space, > but the underlying host memory needn't be contiguous at all. > > I've not yet tested this, but would like to. What's your timeline on getting this done? ASAP. I want to remove... > - if (h2g_valid(endaddr)) { > - endaddr = h2g(endaddr); > - page_set_flags(startaddr, endaddr, PAGE_RESERVED); > - } else { > -#if TARGET_ABI_BITS <= L1_MAP_ADDR_SPACE_BITS L1_MAP_ADDR_SPACE_BITS. r~
On Tue, Dec 20, 2022 at 4:22 PM Richard Henderson < richard.henderson@linaro.org> wrote: > On 12/20/22 14:09, Warner Losh wrote: > > > > > > On Sat, Dec 17, 2022 at 11:48 AM Richard Henderson < > richard.henderson@linaro.org > > <mailto:richard.henderson@linaro.org>> wrote: > > > > Make bsd-user match linux-user in not marking host pages > > as reserved. This isn't especially effective anyway, as > > it doesn't take into account any heap memory that qemu > > may allocate after startup. > > > > Cc: Warner Losh <imp@bsdimp.com <mailto:imp@bsdimp.com>> > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org > > <mailto:richard.henderson@linaro.org>> > > --- > > > > I started to simply fix up this code to match my user-only > interval-tree > > patch set, as L1_MAP_ADDR_SPACE_BITS gets removed from > translate-all.c, > > but then I decided to remove it all. > > > > > > I think this is fine. We already do a translation for addresses so > marking this as 'reserved' > > doesn't help that much. We need to map memory into a contiguous > guess-address-space, > > but the underlying host memory needn't be contiguous at all. > > > > I've not yet tested this, but would like to. What's your timeline on > getting this done? > > ASAP. I want to remove... > > > - if (h2g_valid(endaddr)) { > > - endaddr = h2g(endaddr); > > - page_set_flags(startaddr, endaddr, > PAGE_RESERVED); > > - } else { > > -#if TARGET_ABI_BITS <= L1_MAP_ADDR_SPACE_BITS > > L1_MAP_ADDR_SPACE_BITS. > OK. I've tested this with both 32-bit and 64-bit binaries on a 64-bit host. It works both with the incomplete upstream as well as our 'blitz' branch which is basically complete. I've not run our full regression tests, though, but I suspect they will produce similar results before/after. My test machine is missing a few things due to an incomplete package upgrade that I don't have the time to sort out this evening. And looking at things, I agree with the analysis: It's a pesky nop. At worst, if it does change something, it's likely to change it for the better. And if not, I'll deal with that when I do my next round of upstreaming after the first of the year. So: Reviewed-by: Warner Losh <imp@bsdimp.com> Tested-by: Warner Losh <imp@bsdimp.com> Warner
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index b964ea44d7..48e9d70b4e 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -354,71 +354,6 @@ void page_init(void) { page_size_init(); page_table_config_init(); - -#if defined(CONFIG_BSD) && defined(CONFIG_USER_ONLY) - { -#ifdef HAVE_KINFO_GETVMMAP - struct kinfo_vmentry *freep; - int i, cnt; - - freep = kinfo_getvmmap(getpid(), &cnt); - if (freep) { - mmap_lock(); - for (i = 0; i < cnt; i++) { - unsigned long startaddr, endaddr; - - startaddr = freep[i].kve_start; - endaddr = freep[i].kve_end; - if (h2g_valid(startaddr)) { - startaddr = h2g(startaddr) & TARGET_PAGE_MASK; - - if (h2g_valid(endaddr)) { - endaddr = h2g(endaddr); - page_set_flags(startaddr, endaddr, PAGE_RESERVED); - } else { -#if TARGET_ABI_BITS <= L1_MAP_ADDR_SPACE_BITS - endaddr = ~0ul; - page_set_flags(startaddr, endaddr, PAGE_RESERVED); -#endif - } - } - } - free(freep); - mmap_unlock(); - } -#else - FILE *f; - - last_brk = (unsigned long)sbrk(0); - - f = fopen("/compat/linux/proc/self/maps", "r"); - if (f) { - mmap_lock(); - - do { - unsigned long startaddr, endaddr; - int n; - - n = fscanf(f, "%lx-%lx %*[^\n]\n", &startaddr, &endaddr); - - if (n == 2 && h2g_valid(startaddr)) { - startaddr = h2g(startaddr) & TARGET_PAGE_MASK; - - if (h2g_valid(endaddr)) { - endaddr = h2g(endaddr); - } else { - endaddr = ~0ul; - } - page_set_flags(startaddr, endaddr, PAGE_RESERVED); - } - } while (!feof(f)); - - fclose(f); - mmap_unlock(); - } -#endif - } -#endif } PageDesc *page_find_alloc(tb_page_addr_t index, bool alloc)
Make bsd-user match linux-user in not marking host pages as reserved. This isn't especially effective anyway, as it doesn't take into account any heap memory that qemu may allocate after startup. Cc: Warner Losh <imp@bsdimp.com> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- I started to simply fix up this code to match my user-only interval-tree patch set, as L1_MAP_ADDR_SPACE_BITS gets removed from translate-all.c, but then I decided to remove it all. r~ --- accel/tcg/translate-all.c | 65 --------------------------------------- 1 file changed, 65 deletions(-)