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

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

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.

> +{
> +    /*
> +     * Write PR_MTE_TCF to SCTLR_EL1[TCF0].
> +     *
> +     * The kernel has a per-cpu configuration for the sysadmin,
> +     * /sys/devices/system/cpu/cpu<N>/mte_tcf_preferred,
> +     * which qemu does not implement.
> +     *
> +     * Because there is no performance difference between the modes, and
> +     * because SYNC is most useful for debugging MTE errors, choose SYNC
> +     * as the preferred mode.  With this preference, and the way the API
> +     * uses only two bits, there is no way for the program to select
> +     * ASYMM mode.
> +     */
> +    unsigned tcf = 0;
> +    if (value & PR_MTE_TCF_SYNC) {
> +        tcf = 1;
> +    } else if (value & PR_MTE_TCF_ASYNC) {
> +        tcf = 2;
> +    }
> +    env->cp15.sctlr_el[1] = deposit64(env->cp15.sctlr_el[1], 38, 2, tcf);
> +}
> +#endif /* CONFIG_USER_ONLY */
> +#endif /* CONFIG_TCG */
> +
> +#endif /* MTE_H */

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.


>> +{
>> +    /*
>> +     * Write PR_MTE_TCF to SCTLR_EL1[TCF0].
>> +     *
>> +     * The kernel has a per-cpu configuration for the sysadmin,
>> +     * /sys/devices/system/cpu/cpu<N>/mte_tcf_preferred,
>> +     * which qemu does not implement.
>> +     *
>> +     * Because there is no performance difference between the modes, and
>> +     * because SYNC is most useful for debugging MTE errors, choose SYNC
>> +     * as the preferred mode.  With this preference, and the way the API
>> +     * uses only two bits, there is no way for the program to select
>> +     * ASYMM mode.
>> +     */
>> +    unsigned tcf = 0;
>> +    if (value & PR_MTE_TCF_SYNC) {
>> +        tcf = 1;
>> +    } else if (value & PR_MTE_TCF_ASYNC) {
>> +        tcf = 2;
>> +    }
>> +    env->cp15.sctlr_el[1] = deposit64(env->cp15.sctlr_el[1], 38, 2, tcf);
>> +}
>> +#endif /* CONFIG_USER_ONLY */
>> +#endif /* CONFIG_TCG */
>> +
>> +#endif /* MTE_H */
>

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.

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

>>> +{
>>> +    /*
>>> +     * Write PR_MTE_TCF to SCTLR_EL1[TCF0].
>>> +     *
>>> +     * The kernel has a per-cpu configuration for the sysadmin,
>>> +     * /sys/devices/system/cpu/cpu<N>/mte_tcf_preferred,
>>> +     * which qemu does not implement.
>>> +     *
>>> +     * Because there is no performance difference between the modes, 
>>> and
>>> +     * because SYNC is most useful for debugging MTE errors, choose 
>>> SYNC
>>> +     * as the preferred mode.  With this preference, and the way the 
>>> API
>>> +     * uses only two bits, there is no way for the program to select
>>> +     * ASYMM mode.
>>> +     */
>>> +    unsigned tcf = 0;
>>> +    if (value & PR_MTE_TCF_SYNC) {
>>> +        tcf = 1;
>>> +    } else if (value & PR_MTE_TCF_ASYNC) {
>>> +        tcf = 2;
>>> +    }
>>> +    env->cp15.sctlr_el[1] = deposit64(env->cp15.sctlr_el[1], 38, 2, 
>>> tcf);
>>> +}
>>> +#endif /* CONFIG_USER_ONLY */
>>> +#endif /* CONFIG_TCG */
>>> +
>>> +#endif /* MTE_H */
>>

Gustavo Romero <gustavo.romero@linaro.org> writes:

> 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/linux-user/aarch64/target_prctl.h b/linux-user/aarch64/target_prctl.h
> index aa8e203c15..8a11404222 100644
> --- a/linux-user/aarch64/target_prctl.h
> +++ b/linux-user/aarch64/target_prctl.h
> @@ -7,6 +7,7 @@
>  #define AARCH64_TARGET_PRCTL_H
>  
>  #include "target/arm/cpu-features.h"
> +#include "target/arm/mte.h"

We are moving a helper that is TCG only but using the common code path.
I suspect the prototype better belongs in target/arm/tcg/mte_helper.h or
possibly target/arm/tcg/mte_user_helper.h if we are just using it for
user-mode.

>  
>  static abi_long do_prctl_sve_get_vl(CPUArchState *env)
>  {
> @@ -173,26 +174,7 @@ static abi_long do_prctl_set_tagged_addr_ctrl(CPUArchState *env, abi_long arg2)
>      env->tagged_addr_enable = arg2 & PR_TAGGED_ADDR_ENABLE;
>  
>      if (cpu_isar_feature(aa64_mte, cpu)) {
> -        /*
> -         * Write PR_MTE_TCF to SCTLR_EL1[TCF0].
> -         *
> -         * The kernel has a per-cpu configuration for the sysadmin,
> -         * /sys/devices/system/cpu/cpu<N>/mte_tcf_preferred,
> -         * which qemu does not implement.
> -         *
> -         * Because there is no performance difference between the modes, and
> -         * because SYNC is most useful for debugging MTE errors, choose SYNC
> -         * as the preferred mode.  With this preference, and the way the API
> -         * uses only two bits, there is no way for the program to select
> -         * ASYMM mode.
> -         */
> -        unsigned tcf = 0;
> -        if (arg2 & PR_MTE_TCF_SYNC) {
> -            tcf = 1;
> -        } else if (arg2 & PR_MTE_TCF_ASYNC) {
> -            tcf = 2;
> -        }
> -        env->cp15.sctlr_el[1] = deposit64(env->cp15.sctlr_el[1], 38, 2, tcf);
> +        set_mte_tcf0(env, arg2);
>  
>          /*
>           * Write PR_MTE_TAG to GCR_EL1[Exclude].
> 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)

Same comments as Phil regarding should it be inline, add a prefix etc...

> +{
> +    /*
> +     * Write PR_MTE_TCF to SCTLR_EL1[TCF0].
> +     *
> +     * The kernel has a per-cpu configuration for the sysadmin,
> +     * /sys/devices/system/cpu/cpu<N>/mte_tcf_preferred,
> +     * which qemu does not implement.
> +     *
> +     * Because there is no performance difference between the modes, and
> +     * because SYNC is most useful for debugging MTE errors, choose SYNC
> +     * as the preferred mode.  With this preference, and the way the API
> +     * uses only two bits, there is no way for the program to select
> +     * ASYMM mode.
> +     */
> +    unsigned tcf = 0;
> +    if (value & PR_MTE_TCF_SYNC) {
> +        tcf = 1;
> +    } else if (value & PR_MTE_TCF_ASYNC) {
> +        tcf = 2;
> +    }
> +    env->cp15.sctlr_el[1] = deposit64(env->cp15.sctlr_el[1], 38, 2, tcf);
> +}
> +#endif /* CONFIG_USER_ONLY */
> +#endif /* CONFIG_TCG */
> +
> +#endif /* MTE_H */

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