Re: [PATCH v2 01/17] tcg: Introduce target-specific page data for user-only

[Peter Maydell]

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