| Message ID | 20200605041733.415188-2-richard.henderson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target-arm: Implement ARMv8.5-MemTag, user mode | expand |
On Fri, 5 Jun 2020 at 05:17, Richard Henderson <richard.henderson@linaro.org> wrote: > > This data can be allocated by page_alloc_target_data() and > released by page_set_flags(start, end, prot | PAGE_RESET). > > This data will be used to hold tag memory for AArch64 MTE. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > @@ -289,6 +295,8 @@ int walk_memory_regions(void *, walk_memory_regions_fn); > int page_get_flags(target_ulong address); > void page_set_flags(target_ulong start, target_ulong end, int flags); > int page_check_range(target_ulong start, target_ulong len, int flags); > +void *page_get_target_data(target_ulong address); > +void *page_alloc_target_data(target_ulong address, size_t size); Could we have a doc comment for any new function that's got global scope, please? > #endif > > CPUArchState *cpu_copy(CPUArchState *env); > +void *page_alloc_target_data(target_ulong address, size_t size) > +{ > + PageDesc *p = page_find(address >> TARGET_PAGE_BITS); > + void *ret = NULL; > + > + if (p) { > + ret = p->target_data; > + if (!ret && (p->flags & PAGE_VALID)) { > + p->target_data = ret = g_malloc0(size); > + } > + } > + return ret; Can a PageDesc validly have p->target_data != NULL but p->flags with PAGE_VALID not set ? It's not clear to me why for a !PAGE_VALID page which has target_data already we return that pointer but if it doesn't have any we don't allocate: either "always allocate" or "always return NULL for non-valid pages" would seem more self-consistent. > @@ -787,9 +788,11 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size, > new_addr = -1; > } else { > new_addr = h2g(host_addr); > + /* FIXME: Move page flags and target_data for each page. */ Is this something we're going to address later in the patchset? > prot = page_get_flags(old_addr); > page_set_flags(old_addr, old_addr + old_size, 0); > - page_set_flags(new_addr, new_addr + new_size, prot | PAGE_VALID); > + 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(); thanks -- PMM
On 6/25/20 9:20 AM, Peter Maydell wrote: > On Fri, 5 Jun 2020 at 05:17, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> This data can be allocated by page_alloc_target_data() and >> released by page_set_flags(start, end, prot | PAGE_RESET). >> >> This data will be used to hold tag memory for AArch64 MTE. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- > >> @@ -289,6 +295,8 @@ int walk_memory_regions(void *, walk_memory_regions_fn); >> int page_get_flags(target_ulong address); >> void page_set_flags(target_ulong start, target_ulong end, int flags); >> int page_check_range(target_ulong start, target_ulong len, int flags); >> +void *page_get_target_data(target_ulong address); >> +void *page_alloc_target_data(target_ulong address, size_t size); > > Could we have a doc comment for any new function that's got > global scope, please? > >> #endif >> >> CPUArchState *cpu_copy(CPUArchState *env); > >> +void *page_alloc_target_data(target_ulong address, size_t size) >> +{ >> + PageDesc *p = page_find(address >> TARGET_PAGE_BITS); >> + void *ret = NULL; >> + >> + if (p) { >> + ret = p->target_data; >> + if (!ret && (p->flags & PAGE_VALID)) { >> + p->target_data = ret = g_malloc0(size); >> + } >> + } >> + return ret; > > Can a PageDesc validly have p->target_data != NULL but > p->flags with PAGE_VALID not set ? No. But we can be called for a page that is not mapped (returning NULL) and can be called for a page that already has associated data (returning the old value). > It's not clear to me why for a !PAGE_VALID page which > has target_data already we return that pointer but > if it doesn't have any we don't allocate: either > "always allocate" or "always return NULL for non-valid pages" > would seem more self-consistent. I was expecting a non-valid page to have no data. I will rearrange this to ret = NULL; if (p->flags & PAGE_VALID) { ret = p->target_data; if (!ret) { p->target_data = ret = g_malloc0(size); } } which is probably clearer. >> + /* FIXME: Move page flags and target_data for each page. */ > > Is this something we're going to address later in the patchset? I had not, but I should. Will fix. r~
On 6/25/20 9:20 AM, Peter Maydell wrote: > On Fri, 5 Jun 2020 at 05:17, Richard Henderson >> @@ -787,9 +788,11 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size, >> new_addr = -1; >> } else { >> new_addr = h2g(host_addr); >> + /* FIXME: Move page flags and target_data for each page. */ > > Is this something we're going to address later in the patchset? I've removed the comment. The mremap system call is not as general as I think it should be. It only applies to MAP_SHARED vmas and returns EINVAL on MAP_PRIVATE. Therefore, at least for the MTE usage of target_data, there cannot be any. r~
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index 2bd023d692..e5f564fa1f 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -266,15 +266,21 @@ extern intptr_t qemu_host_page_mask; #define PAGE_EXEC 0x0004 #define PAGE_BITS (PAGE_READ | PAGE_WRITE | PAGE_EXEC) #define PAGE_VALID 0x0008 -/* original state of the write flag (used when tracking self-modifying - code */ +/* + * 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 0x0040 +/* + * 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 + #if defined(CONFIG_BSD) && defined(CONFIG_USER_ONLY) /* FIXME: Code that sets/uses this is broken and needs to go away. */ -#define PAGE_RESERVED 0x0020 +#define PAGE_RESERVED 0x0100 #endif /* Target-specific bits that will be used via page_get_flags(). */ #define PAGE_TARGET_1 0x0080 @@ -289,6 +295,8 @@ int walk_memory_regions(void *, walk_memory_regions_fn); int page_get_flags(target_ulong address); void page_set_flags(target_ulong start, target_ulong end, int flags); int page_check_range(target_ulong start, target_ulong len, int flags); +void *page_get_target_data(target_ulong address); +void *page_alloc_target_data(target_ulong address, size_t size); #endif CPUArchState *cpu_copy(CPUArchState *env); diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index 42ce1dfcff..a65bc1d4c2 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -111,6 +111,7 @@ typedef struct PageDesc { unsigned int code_write_count; #else unsigned long flags; + void *target_data; #endif #ifndef CONFIG_USER_ONLY QemuSpin lock; @@ -2525,6 +2526,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; /* This function should never be called with addresses outside the guest address space. If this assert fires, it probably indicates @@ -2539,6 +2541,8 @@ 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); + flags &= ~PAGE_RESET; for (addr = start, len = end - start; len != 0; @@ -2552,10 +2556,34 @@ void page_set_flags(target_ulong start, target_ulong end, int flags) p->first_tb) { tb_invalidate_phys_page(addr, 0); } + if (reset_target_data && p->target_data) { + g_free(p->target_data); + p->target_data = NULL; + } p->flags = flags; } } +void *page_get_target_data(target_ulong address) +{ + PageDesc *p = page_find(address >> TARGET_PAGE_BITS); + return p ? p->target_data : NULL; +} + +void *page_alloc_target_data(target_ulong address, size_t size) +{ + PageDesc *p = page_find(address >> TARGET_PAGE_BITS); + void *ret = NULL; + + if (p) { + ret = p->target_data; + if (!ret && (p->flags & PAGE_VALID)) { + p->target_data = ret = g_malloc0(size); + } + } + return ret; +} + int page_check_range(target_ulong start, target_ulong len, int flags) { PageDesc *p; diff --git a/linux-user/mmap.c b/linux-user/mmap.c index 40f03e3174..f9832609b0 100644 --- a/linux-user/mmap.c +++ b/linux-user/mmap.c @@ -599,6 +599,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot, } } the_end1: + page_flags |= PAGE_RESET; page_set_flags(start, start + len, page_flags); the_end: trace_target_mmap_complete(start); @@ -787,9 +788,11 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size, new_addr = -1; } else { new_addr = h2g(host_addr); + /* FIXME: Move page flags and target_data for each page. */ prot = page_get_flags(old_addr); page_set_flags(old_addr, old_addr + old_size, 0); - page_set_flags(new_addr, new_addr + new_size, prot | PAGE_VALID); + 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(); diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 7f6700c54e..d190fb1122 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -4315,8 +4315,8 @@ static inline abi_ulong do_shmat(CPUArchState *cpu_env, raddr=h2g((unsigned long)host_raddr); page_set_flags(raddr, raddr + shm_info.shm_segsz, - PAGE_VALID | PAGE_READ | - ((shmflg & SHM_RDONLY)? 0 : PAGE_WRITE)); + PAGE_VALID | PAGE_RESET | PAGE_READ | + (shmflg & SHM_RDONLY ? 0 : PAGE_WRITE)); for (i = 0; i < N_SHM_REGIONS; i++) { if (!shm_regions[i].in_use) {
This data can be allocated by page_alloc_target_data() and released by page_set_flags(start, end, prot | PAGE_RESET). This data will be used to hold tag memory for AArch64 MTE. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- include/exec/cpu-all.h | 20 ++++++++++++++------ accel/tcg/translate-all.c | 28 ++++++++++++++++++++++++++++ linux-user/mmap.c | 5 ++++- linux-user/syscall.c | 4 ++-- 4 files changed, 48 insertions(+), 9 deletions(-)