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 <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"
+
+/**
+ * arm_set_mte_tcf0 - Set TCF0 field in SCTLR_EL1 register
+ * @env: The CPU environment
+ * @value: The value to be set for the Tag Check Fault in EL0 field.
+ *
+ * Only SYNC and ASYNC modes can be selected. If ASYMM mode is given, the SYNC
+ * mode is selected instead. So, there is no way to set the ASYMM mode.
+ */
+void arm_set_mte_tcf0(CPUArchState *env, abi_long value);
+
+#endif /* AARCH64_MTE_USER_HELPER_H */
diff --git a/linux-user/aarch64/target_prctl.h b/linux-user/aarch64/target_prctl.h
index aa8e203c15..ed75b9e4b5 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 "mte_user_helper.h"
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);
+ arm_set_mte_tcf0(env, arg2);
/*
* Write PR_MTE_TAG to GCR_EL1[Exclude].
--
2.34.1Gustavo 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/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 <sys/prctl.h>
Aside from missing the osdep Phillipe pointed out including prctl.h here
is very suspect as its a system header. I assume if we need
PR_MTE_TCF_SYNC we should hoist the definition that linux-user uses into
a common header.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
Hi Alex,
On 6/28/24 9:14 AM, Alex Bennée wrote:
> 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/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 <sys/prctl.h>
>
> Aside from missing the osdep Phillipe pointed out including prctl.h here
> is very suspect as its a system header. I assume if we need
> PR_MTE_TCF_SYNC we should hoist the definition that linux-user uses into
> a common header.
Other .c files include <sys/prctl.h> for other PR_ definitions. For example,
syscall.c and elfload.c. Is this really a problem? I see that would be a
problem when trying to build, for instance, aarch64-linux-user target on a
BSD host, but we don't support it. Building *-linux-user target is only
supported on Linux host, no?
Cheers,
Gustavo
Gustavo Romero <gustavo.romero@linaro.org> writes:
> Hi Alex,
>
> On 6/28/24 9:14 AM, Alex Bennée wrote:
>> 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/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 <sys/prctl.h>
>> Aside from missing the osdep Phillipe pointed out including prctl.h
>> here
>> is very suspect as its a system header. I assume if we need
>> PR_MTE_TCF_SYNC we should hoist the definition that linux-user uses into
>> a common header.
> Other .c files include <sys/prctl.h> for other PR_ definitions. For example,
> syscall.c and elfload.c. Is this really a problem?
If your building on an arch that doesn't natively have MTE but you'll
run an aarch64 linux-user guest.
> I see that would be a
> problem when trying to build, for instance, aarch64-linux-user target on a
> BSD host, but we don't support it. Building *-linux-user target is only
> supported on Linux host, no?
True, but multiple libcs. Anyway I've fixed up and posted the maintainer
tree.
>
>
> Cheers,
> Gustavo
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
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.
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
On 6/28/24 08:49, Gustavo Romero wrote: > 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.". Not "when necessary", always, and always first. See the "Include directives" section of docs/devel/style.rst, which does explicitly say 'Do not include "qemu/osdep.h" from header files'. > 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. CPUArchState will come from qemu/typedefs.h via osdep.h. For this particular function, 'int' would have been enough, since we only care about the low two bits. r~
Hi Richard, On 6/28/24 2:00 PM, Richard Henderson wrote: > On 6/28/24 08:49, Gustavo Romero wrote: >> 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.". > > Not "when necessary", always, and always first. Got it! > See the "Include directives" section of docs/devel/style.rst, which does explicitly say 'Do not include "qemu/osdep.h" from header files'. Yep, Phil pointed out this doc when we were discussing it in v5. I was actually referring to it about the wording. Maybe then it should be more explicitly that osdep.h _always_ has to be present. Re-reading it after your clarifications makes it clear, but the first time Phil pointed it out the phrases: "[...] since the .c file will have already included it." and "Headers should normally include everything they need beyond osdep.h." weren't enough to me to make it clear that osdep.h must always be included (present) in the .c files. "will have already included" sounded ambiguous to me, more like, if necessary it would have already be included in .c (but not always). But, well, that can be a falt in my interpretation.. Thanks a lot for the clarification. > >> 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. > > CPUArchState will come from qemu/typedefs.h via osdep.h. > > For this particular function, 'int' would have been enough, > since we only care about the low two bits. hmm, right. I'll send a follow up patch to improve it since Alex already picked up the series to gdbstub/next. Thanks! Cheers, Gustavo
On Fri, 28 Jun 2024 at 19:13, Gustavo Romero <gustavo.romero@linaro.org> wrote: > Re-reading it after your clarifications makes it clear, but the first time > Phil pointed it out the phrases: > > "[...] since the .c file will have already included it." and > "Headers should normally include everything they need beyond osdep.h." > > weren't enough to me to make it clear that osdep.h must always be included > (present) in the .c files. "will have already included" sounded ambiguous to > me, more like, if necessary it would have already be included in .c (but not > always). But, well, that can be a falt in my interpretation. Yeah, the rule is simple: * .c files always include osdep.h, and it comes first * .h files never include osdep.h but the wording in the developer docs doesn't manage to state that exactly. -- PMM
On 28/6/24 17:49, Gustavo Romero wrote: > 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 > 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 Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
© 2016 - 2024 Red Hat, Inc.