| Message ID | 20221006031113.1139454-15-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/tcg: Rewrite user-only vma tracking | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > When PAGE_RESET is set, we are replacing pages with new > content, which means that we need to invalidate existing > cached data, such as TranslationBlocks. Perform the > reset invalidate while we're doing other invalidates, > which allows us to remove the separate invalidates from > the user-only mmap/munmap/mprotect routines. > > In addition, restrict invalidation to PAGE_EXEC pages. > Since cdf713085131, we have validated PAGE_EXEC is present > before translation, which means we can assume that if the > bit is not present, there are no translations to invalidate. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > accel/tcg/translate-all.c | 19 +++++++++++-------- > bsd-user/mmap.c | 2 -- > linux-user/mmap.c | 4 ---- > 3 files changed, 11 insertions(+), 14 deletions(-) > > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c > index 8d5233fa9e..478301f227 100644 > --- a/accel/tcg/translate-all.c > +++ b/accel/tcg/translate-all.c > @@ -1352,7 +1352,7 @@ int page_get_flags(target_ulong address) > void page_set_flags(target_ulong start, target_ulong end, int flags) > { > target_ulong addr, len; > - bool reset_target_data; > + bool reset; > > /* This function should never be called with addresses outside the > guest address space. If this assert fires, it probably indicates > @@ -1369,7 +1369,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags) > if (flags & PAGE_WRITE) { > flags |= PAGE_WRITE_ORG; > } > - reset_target_data = !(flags & PAGE_VALID) || (flags & PAGE_RESET); > + reset = !(flags & PAGE_VALID) || (flags & PAGE_RESET); > flags &= ~PAGE_RESET; Not a problem with this patch but I was a little confused by PAGE_VALID because its the one "special" flag not documented in cpu-all.h: /* same as PROT_xxx */ #define PAGE_READ 0x0001 #define PAGE_WRITE 0x0002 #define PAGE_EXEC 0x0004 #define PAGE_BITS (PAGE_READ | PAGE_WRITE | PAGE_EXEC) The above are self explanatory as they mirror the mmap flags. But what does PAGE_VALID really mean. We set it in the elfloader and whenever we mmap which begs the question when is a page not VALID? The only place that ever seems to clear the flag is the PPC mmu_helper code in a response to a particular TLB operations. Should that code instead be doing page_set_flags(PAGE_RESET)? #define PAGE_VALID 0x0008 /* * Original state of the write flag (used when tracking self-modifying code) */ #define PAGE_WRITE_ORG 0x0010 /* * Invalidate the TLB entry immediately, helpful for s390x * Low-Address-Protection. Used with PAGE_WRITE in tlb_set_page_with_attrs() */ #define PAGE_WRITE_INV 0x0020 /* For use with page_set_flags: page is being replaced; target_data cleared. */ #define PAGE_RESET 0x0040 /* For linux-user, indicates that the page is MAP_ANON. */ #define PAGE_ANON 0x0080 #if defined(CONFIG_BSD) && defined(CONFIG_USER_ONLY) /* FIXME: Code that sets/uses this is broken and needs to go away. */ #define PAGE_RESERVED 0x0100 #endif /* Target-specific bits that will be used via page_get_flags(). */ #define PAGE_TARGET_1 0x0200 #define PAGE_TARGET_2 0x0400 /* * For linux-user, indicates that the page is mapped with the same semantics * in both guest and host. */ #define PAGE_PASSTHROUGH 0x0800 Anyway that confusion aside: Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
On 10/26/22 01:42, Alex Bennée wrote: > Not a problem with this patch but I was a little confused by PAGE_VALID > because its the one "special" flag not documented in cpu-all.h: > > /* same as PROT_xxx */ > #define PAGE_READ 0x0001 > #define PAGE_WRITE 0x0002 > #define PAGE_EXEC 0x0004 > #define PAGE_BITS (PAGE_READ | PAGE_WRITE | PAGE_EXEC) > > The above are self explanatory as they mirror the mmap flags. But what > does PAGE_VALID really mean. It exists so that unmapped pages have flags == 0 and mmap(..., PROT_NONE, ...) pages have flags != 0. Perhaps a better name would have been "allocated". You are perhaps 25 years to late to bikeshed that name. :-) > The only place > that ever seems to clear the flag is the PPC mmu_helper code in a > response to a particular TLB operations. Should that code instead be > doing page_set_flags(PAGE_RESET)? Heh. ppc seems to have re-used the symbol for its own internal data structure. Well, considering that it's got to pass on "prot" to tlb_set_page at some point, re-using those names isn't a horrible idea. Anyway, you can see target_munmap clear PAGE_VALID here: page_set_flags(start, start + len, 0); PAGE_RESET exists to distinguish mmap replacing an exiting mapping (old backing storage replaced) from mprotect (old backing storage retained, new permissions applied). r~
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index 8d5233fa9e..478301f227 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1352,7 +1352,7 @@ int page_get_flags(target_ulong address) void page_set_flags(target_ulong start, target_ulong end, int flags) { target_ulong addr, len; - bool reset_target_data; + bool reset; /* This function should never be called with addresses outside the guest address space. If this assert fires, it probably indicates @@ -1369,7 +1369,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags) if (flags & PAGE_WRITE) { flags |= PAGE_WRITE_ORG; } - reset_target_data = !(flags & PAGE_VALID) || (flags & PAGE_RESET); + reset = !(flags & PAGE_VALID) || (flags & PAGE_RESET); flags &= ~PAGE_RESET; for (addr = start, len = end - start; @@ -1377,14 +1377,17 @@ void page_set_flags(target_ulong start, target_ulong end, int flags) len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) { PageDesc *p = page_find_alloc(addr >> TARGET_PAGE_BITS, true); - /* If the write protection bit is set, then we invalidate - the code inside. */ - if (!(p->flags & PAGE_WRITE) && - (flags & PAGE_WRITE) && - p->first_tb) { + /* + * If the page was executable, but is reset, or is no longer + * executable, or has become writable, then invalidate any code. + */ + if ((p->flags & PAGE_EXEC) + && (reset || + !(flags & PAGE_EXEC) || + (flags & ~p->flags & PAGE_WRITE))) { tb_invalidate_phys_page(addr); } - if (reset_target_data) { + if (reset) { g_free(p->target_data); p->target_data = NULL; p->flags = flags; diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c index e54e26de17..d6c5a344c9 100644 --- a/bsd-user/mmap.c +++ b/bsd-user/mmap.c @@ -663,7 +663,6 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot, page_dump(stdout); printf("\n"); #endif - tb_invalidate_phys_range(start, start + len); mmap_unlock(); return start; fail: @@ -769,7 +768,6 @@ int target_munmap(abi_ulong start, abi_ulong len) if (ret == 0) { page_set_flags(start, start + len, 0); - tb_invalidate_phys_range(start, start + len); } mmap_unlock(); return ret; diff --git a/linux-user/mmap.c b/linux-user/mmap.c index 28f3bc85ed..10f5079331 100644 --- a/linux-user/mmap.c +++ b/linux-user/mmap.c @@ -182,7 +182,6 @@ int target_mprotect(abi_ulong start, abi_ulong len, int target_prot) } page_set_flags(start, start + len, page_flags); - tb_invalidate_phys_range(start, start + len); ret = 0; error: @@ -662,7 +661,6 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot, qemu_log_unlock(f); } } - tb_invalidate_phys_range(start, start + len); mmap_unlock(); return start; fail: @@ -766,7 +764,6 @@ int target_munmap(abi_ulong start, abi_ulong len) if (ret == 0) { page_set_flags(start, start + len, 0); - tb_invalidate_phys_range(start, start + len); } mmap_unlock(); return ret; @@ -856,7 +853,6 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size, page_set_flags(new_addr, new_addr + new_size, prot | PAGE_VALID | PAGE_RESET); } - tb_invalidate_phys_range(new_addr, new_addr + new_size); mmap_unlock(); return new_addr; }
When PAGE_RESET is set, we are replacing pages with new content, which means that we need to invalidate existing cached data, such as TranslationBlocks. Perform the reset invalidate while we're doing other invalidates, which allows us to remove the separate invalidates from the user-only mmap/munmap/mprotect routines. In addition, restrict invalidation to PAGE_EXEC pages. Since cdf713085131, we have validated PAGE_EXEC is present before translation, which means we can assume that if the bit is not present, there are no translations to invalidate. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- accel/tcg/translate-all.c | 19 +++++++++++-------- bsd-user/mmap.c | 2 -- linux-user/mmap.c | 4 ---- 3 files changed, 11 insertions(+), 14 deletions(-)