On 6/25/20 9:20 AM, Peter Maydell wrote:
> On Fri, 5 Jun 2020 at 05:17, Richard Henderson
> <[email protected]> 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 <[email protected]>
>> ---
>
>> @@ -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~
