Re: [PATCH v2 6/9] target/arm: Factor out code for setting MTE TCF0 field

[Gustavo Romero]

Hi Phil,

On 6/14/24 6:02 AM, Philippe Mathieu-Daudé wrote:
> On 13/6/24 20:15, Gustavo Romero wrote:
> > Hi Phil,
> >
> > On 6/13/24 2:35 PM, Philippe Mathieu-Daudé wrote:
> > > On 13/6/24 19:21, Gustavo Romero wrote:
> > > > Factor out the code used for setting the MTE TCF0 field from the prctl
> > > > code into a convenient function. Other subsystems, like gdbstub, need to
> > > > set this field as well, so keep it as a separate function to avoid
> > > > duplication and ensure consistency in how this field is set across the
> > > > board.
> > > >
> > > > Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> > > > ---
> > > >   linux-user/aarch64/target_prctl.h | 22 ++-----------
> > > >   target/arm/mte.h                  | 53 +++++++++++++++++++++++++++++++
> > > >   2 files changed, 55 insertions(+), 20 deletions(-)
> > > >   create mode 100644 target/arm/mte.h
> > >
> > >
> > > > diff --git a/target/arm/mte.h b/target/arm/mte.h
> > > > new file mode 100644
> > > > index 0000000000..89712aad70
> > > > --- /dev/null
> > > > +++ b/target/arm/mte.h
> > > > @@ -0,0 +1,53 @@
> > > > +/*
> > > > + * ARM MemTag convenience functions.
> > > > + *
> > > > + * Copyright (c) 2024 Linaro, Ltd.
> > > > + *
> > > > + * This library is free software; you can redistribute it and/or
> > > > + * modify it under the terms of the GNU Lesser General Public
> > > > + * License as published by the Free Software Foundation; either
> > > > + * version 2.1 of the License, or (at your option) any later version.
> > > > + *
> > > > + * This library is distributed in the hope that it will be useful,
> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > > + * Lesser General Public License for more details.
> > > > + *
> > > > + * You should have received a copy of the GNU Lesser General Public
> > > > + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> > > > + */
> > > > +
> > > > +#ifndef MTE_H
> > > > +#define MTE_H
> > > > +
> > > > +#ifdef CONFIG_TCG
> > > > +#ifdef CONFIG_USER_ONLY
> > > > +#include "sys/prctl.h"
> > > > +
> > > > +static void set_mte_tcf0(CPUArchState *env, abi_long value)
> > >
> > > Either declare it inlined (otherwise we'll get multiple symbols
> > > declared if this header is included multiple times), or
> > > preferably only expose the prototype.
> > >
> > > Also I'd use the 'arm_' prefix.
> >
> > Thanks, I forgot to add the inline hint and was really wondering if
> > I should add the arm_ prefix.
>
> If you expose the prototype, it can be used elsewhere. Here it
> will be used by linux-user code. Althought it will be used by ARM
> specific code, from this other subsystem PoV it will be clearer
> that this method is ARM specific if the prefix is used. But I'm
> being picky and it isn't a requirement.

That's alright :) I think that using the prefix is the right thing
to do. I added it in v3.

 
> However the question about why do we want this method inlined remains
> (usually all inlined functions need a justification).

I can't see how using 'static' would cause multiple symbols declared
if the header is included multiple times. If multiple .c files include
such a header each one would get its own independent copy of the function,
so the function would be local to each translation unit. Just like defining
a function of the same name in different .c but marking them as static, so
keeping them local to the translation unit.

That said, I agree it should be 'inline'. That's a tiny function and it
seems really simple to be inlined by the compiler.

This function can't be just marked as 'inline' because deposit() is 'static',
hence in v3 it's marked as 'static inline', just like many other cases in .h
we have.


Cheers,
Gustavo