Re: [PATCH v2 5/9] target/arm: Make some MTE helpers widely available

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Phil,

On 6/14/24 9:34 AM, Philippe Mathieu-Daudé wrote:
> On 13/6/24 20:13, Gustavo Romero wrote:
>> Hi Phil!
>>
>> On 6/13/24 2:32 PM, Philippe Mathieu-Daudé wrote:
>>> Hi Gustavo,
>>>
>>> On 13/6/24 19:20, Gustavo Romero wrote:
>>>> Make the MTE helpers allocation_tag_mem_probe, load_tag1, and store_tag1
>>>> available to other subsystems by moving them from mte_helper.c to a new
>>>> header file, mte_helper.h.
>>>>
>>>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>>>> ---
>>>>   target/arm/tcg/mte_helper.c | 184 +------------------------------
>>>>   target/arm/tcg/mte_helper.h | 211 ++++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 212 insertions(+), 183 deletions(-)
>>>>   create mode 100644 target/arm/tcg/mte_helper.h
> 
> 
>>>> + */
>>>> +
>>>> +#ifndef TARGET_ARM_MTE_H
>>>> +#define TARGET_ARM_MTE_H
>>>> +
>>>> +#include "exec/exec-all.h"
>>>
>>> Why do you need "exec/exec-all.h"?
>>
>> Otherwise one gets:
>>
>> In file included from ../target/arm/gdbstub.c:30:
>> ../target/arm/tcg/mte_helper.h: In function ‘allocation_tag_mem_probe’:
>> ../target/arm/tcg/mte_helper.h:77:9: error: implicit declaration of function ‘cpu_loop_exit_sigsegv’; did you mean ‘cpu_loop_exit_noexc’? [-Werror=implicit-function-declaration]
>>     77 |         cpu_loop_exit_sigsegv(env_cpu(env), ptr, ptr_access,
>>        |         ^~~~~~~~~~~~~~~~~~~~~
>>        |         cpu_loop_exit_noexc
>> ../target/arm/tcg/mte_helper.h:77:9: error: nested extern declaration of ‘cpu_loop_exit_sigsegv’ [-Werror=nested-externs]
>>
>> Any other idea on how to satisfy it?
> 
> OK, I'll fix once I get my include/exec/ rework merged.
> 
>>>> +#include "exec/ram_addr.h"
>>>> +#include "hw/core/tcg-cpu-ops.h"
>>>> +#include "qemu/log.h"
>>>> +
>>>> +/**
>>>> + * allocation_tag_mem_probe:
>>>> + * @env: the cpu environment
>>>> + * @ptr_mmu_idx: the addressing regime to use for the virtual address
>>>> + * @ptr: the virtual address for which to look up tag memory
>>>> + * @ptr_access: the access to use for the virtual address
>>>> + * @ptr_size: the number of bytes in the normal memory access
>>>> + * @tag_access: the access to use for the tag memory
>>>> + * @probe: true to merely probe, never taking an exception
>>>> + * @ra: the return address for exception handling
>>>> + *
>>>> + * Our tag memory is formatted as a sequence of little-endian nibbles.
>>>> + * That is, the byte at (addr >> (LOG2_TAG_GRANULE + 1)) contains two
>>>> + * tags, with the tag at [3:0] for the lower addr and the tag at [7:4]
>>>> + * for the higher addr.
>>>> + *
>>>> + * Here, resolve the physical address from the virtual address, and return
>>>> + * a pointer to the corresponding tag byte.
>>>> + *
>>>> + * If there is no tag storage corresponding to @ptr, return NULL.
>>>> + *
>>>> + * If the page is inaccessible for @ptr_access, or has a watchpoint, there are
>>>> + * three options:
>>>> + * (1) probe = true, ra = 0 : pure probe -- we return NULL if the page is not
>>>> + *     accessible, and do not take watchpoint traps. The calling code must
>>>> + *     handle those cases in the right priority compared to MTE traps.
>>>> + * (2) probe = false, ra = 0 : probe, no fault expected -- the caller guarantees
>>>> + *     that the page is going to be accessible. We will take watchpoint traps.
>>>> + * (3) probe = false, ra != 0 : non-probe -- we will take both memory access
>>>> + *     traps and watchpoint traps.
>>>> + * (probe = true, ra != 0 is invalid and will assert.)
>>>> + */
>>>> +static inline uint8_t *allocation_tag_mem_probe(CPUARMState *env, int ptr_mmu_idx,
>>>> +                                         uint64_t ptr, MMUAccessType ptr_access,
>>>> +                                         int ptr_size, MMUAccessType tag_access,
>>>> +                                         bool probe, uintptr_t ra)
>>>
>>> Do we really need an inlined function? Since it calls non-inlined
>>> methods, I don't really see the point.
>>
>> inline is just a hint and I think that in general at least the overhead
>> for calling this function is reduced, but it's hard to say what the
>> compile heuristics will do exactly without looking at the compiled code.
> 
> My question is about having the function definition in an header,
> instead of its prototype (and the definition in a .c source file).

Got it. I've moved the definition back to .c file and used only the
protypes in .h. I removed the inline for this function but kept the
inline for {load,store}_tag1 because I understand they can be easily
inline by the compiler. Please see v3.


Cheers,
Gustavo