Re: [PATCH v2 01/17] tcg: Introduce target-specific page data for user-only
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~
Re: [PATCH v2 01/17] tcg: Introduce target-specific page data for user-only
On 6/25/20 9:20 AM, Peter Maydell wrote:
> On Fri, 5 Jun 2020 at 05:17, Richard Henderson
> 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
>> ---
>
>> @@ -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~
Re: [PATCH v2 01/17] tcg: Introduce target-specific page data for user-only
On Fri, 5 Jun 2020 at 05:17, Richard Henderson
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
> ---
> @@ -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
[PATCH v2 01/17] tcg: Introduce target-specific page data for user-only
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
---
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(-)
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,
