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

[Gustavo Romero]

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