Re: [PATCH v6 06/11] target/arm: Factor out code for setting MTE TCF0 field

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

Hi Phil,

On 6/28/24 4:08 AM, Philippe Mathieu-Daudé wrote:
> On 28/6/24 07:08, 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/meson.build       |  2 ++
>>   linux-user/aarch64/mte_user_helper.c | 34 ++++++++++++++++++++++++++++
>>   linux-user/aarch64/mte_user_helper.h | 25 ++++++++++++++++++++
>>   linux-user/aarch64/target_prctl.h    | 22 ++----------------
>>   4 files changed, 63 insertions(+), 20 deletions(-)
>>   create mode 100644 linux-user/aarch64/mte_user_helper.c
>>   create mode 100644 linux-user/aarch64/mte_user_helper.h
>>
>> diff --git a/linux-user/aarch64/meson.build b/linux-user/aarch64/meson.build
>> index 248c578d15..f75bb3cd75 100644
>> --- a/linux-user/aarch64/meson.build
>> +++ b/linux-user/aarch64/meson.build
>> @@ -9,3 +9,5 @@ vdso_le_inc = gen_vdso.process('vdso-le.so',
>>                                  extra_args: ['-r', '__kernel_rt_sigreturn'])
>>   linux_user_ss.add(when: 'TARGET_AARCH64', if_true: [vdso_be_inc, vdso_le_inc])
>> +
>> +linux_user_ss.add(when: 'TARGET_AARCH64', if_true: [files('mte_user_helper.c')])
>> diff --git a/linux-user/aarch64/mte_user_helper.c b/linux-user/aarch64/mte_user_helper.c
>> new file mode 100644
>> index 0000000000..8be6deaf03
>> --- /dev/null
>> +++ b/linux-user/aarch64/mte_user_helper.c
>> @@ -0,0 +1,34 @@
>> +/*
>> + * ARM MemTag convenience functions.
>> + *
>> + * This code is licensed under the GNU GPL v2 or later.
>> + *
>> + * SPDX-License-Identifier: LGPL-2.1-or-later
>> + */
>> +
> 
>    #include "qemu/osdep.h"
>    #include "qemu.h"
> 
>> +#include <sys/prctl.h>
>> +#include "mte_user_helper.h"
>> +
>> +void arm_set_mte_tcf0(CPUArchState *env, abi_long value)
>> +{
>> +    /*
>> +     * 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);
>> +}
>> diff --git a/linux-user/aarch64/mte_user_helper.h b/linux-user/aarch64/mte_user_helper.h
>> new file mode 100644
>> index 0000000000..ee3f6b190a
>> --- /dev/null
>> +++ b/linux-user/aarch64/mte_user_helper.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * ARM MemTag convenience functions.
>> + *
>> + * This code is licensed under the GNU GPL v2 or later.
>> + *
>> + * SPDX-License-Identifier: LGPL-2.1-or-later
>> + */
>> +
>> +#ifndef AARCH64_MTE_USER_HELPER_H
>> +#define AARCH64_MTE USER_HELPER_H
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu.h"
> 
> NACK. See my comment on v5.

Yes, I saw your comment in v5 about it, I haven't ignored it, I just wanted to
publish v6 updating the parts we reached out a consensus.

So,

>>>> diff --git a/linux-user/aarch64/mte_user_helper.h b/linux-user/aarch64/mte_user_helper.h
>>>> new file mode 100644
>>>> index 0000000000..ee3f6b190a
>>>> --- /dev/null
>>>> +++ b/linux-user/aarch64/mte_user_helper.h
>>>> @@ -0,0 +1,25 @@
>>>> +/*
>>>> + * ARM MemTag convenience functions.
>>>> + *
>>>> + * This code is licensed under the GNU GPL v2 or later.
>>>> + *
>>>> + * SPDX-License-Identifier: LGPL-2.1-or-later
>>>> + */
>>>> +
>>>> +#ifndef AARCH64_MTE_USER_HELPER_H
>>>> +#define AARCH64_MTE USER_HELPER_H
>>>> +
>>>> +#include "qemu/osdep.h"
>>>
>>> https://www.qemu.org/docs/master/devel/style.html#include-directives
>>>
>>>    Do not include “qemu/osdep.h” from header files since the .c file
>>>    will have already included it.
>>>

I thought you meant osdep.h should not be included _at all_ in my case, either
in mte_user_helper.h or in mte_user_helper.c. Maybe the wording in the docs
should be "Do not include "qemu/osdep.h" from header files. Include it from .c
files, when necessary.".

I think we agree osdep.h is necessary and must be put in mte_user_helper.c. But
that left me wondering how it would work for sources including mte_user_helper.h,
because it can be the case they don't have the declarations for the types used in
the function prototypes, in this case, for CPUArchState and abi_long types in
arm_set_mte_tcf0. It just happens that gdbstub64.c, that includes this header file,
actually includes osdep.h at the top of includes, so all good, but how about other
types not provided by the osdep.h, they would have to be included in the .c that
defines the function (in this case mte_user_helper.c) and also in the .h that
includes the function prototype (in this case gdbstub64.c) anyways, which is the
case of abi_long type, which is not provided by osdep.h, for instance. Luckily,
gdbstub64.c also includes cpu.h, so for abi_long it's fine also, but is a tad odd
to me. Anyways, see my code suggestion below.


>>>> +#include "qemu.h"
>>>
>>> "qemu.h" shouldn't be required neither.
>>
>> If I remove qemu/osdep.h CPUArchState can't resolved. If I remove qemu.h
>> then abi_long can't be resolved. I'm in a tight corner here.
>
> Does it work with "exec/cpu-all.h"?

It resolves the abi_long declaration, yes, but then it fails to resolve MMU_USER_IDX.
I think one level up of include works, so how about cpu.h? cpu.h works.

So, how about:

diff --git a/linux-user/aarch64/mte_user_helper.c b/linux-user/aarch64/mte_user_helper.c
index 8be6deaf03..a0e8abd551 100644
--- a/linux-user/aarch64/mte_user_helper.c
+++ b/linux-user/aarch64/mte_user_helper.c
@@ -6,7 +6,9 @@
   * SPDX-License-Identifier: LGPL-2.1-or-later
   */
  
+#include "qemu/osdep.h"
  #include <sys/prctl.h>
+#include "cpu.h"
  #include "mte_user_helper.h"
  
  void arm_set_mte_tcf0(CPUArchState *env, abi_long value)
diff --git a/linux-user/aarch64/mte_user_helper.h b/linux-user/aarch64/mte_user_helper.h
index ee3f6b190a..07fc0bcebf 100644
--- a/linux-user/aarch64/mte_user_helper.h
+++ b/linux-user/aarch64/mte_user_helper.h
@@ -9,9 +9,6 @@
  #ifndef AARCH64_MTE_USER_HELPER_H
  #define AARCH64_MTE USER_HELPER_H
  
-#include "qemu/osdep.h"
-#include "qemu.h"
-
  /**
   * arm_set_mte_tcf0 - Set TCF0 field in SCTLR_EL1 register
   * @env: The CPU environment


Cheers,
Gustavo