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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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).

> But I can remove it for this function and leave it just for 
> {load,store}_tag1.