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

[Richard Henderson <richard.henderson@linaro.org>]

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~