| Message ID | 20240722214313.89503-14-imp@bsdimp.com |
|---|---|
| State | New |
| Headers | show |
| Series | bsd-user: Misc changes for 9.1 (I hope) | expand |
Hi Warner, On 22/7/24 23:43, Warner Losh wrote: > We include the files that define PR_MTE_TCF_SHIFT only on Linux, but use > them unconditionally. Restrict its use to Linux-only. We should check that in meson, i.e.: config_host_data.set('CONFIG_PRCTL_PR_MTE_TCF_SHIFT', cc.has_header_symbol('sys/prctl.h', 'PR_MTE_TCF_SHIFT')) (like we do for CONFIG_PRCTL_PR_SET_TIMERSLACK), then rework linux-user/aarch64/mte_user_helper.h (and possibly tests/tcg/aarch64/mte.h), moving in a common directory. That said, your patch matches this file header: #if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX) #include <sys/prctl.h> #include "mte_user_helper.h" #endif Even if this #ifdef'ry is ugly, it isn't wrong, so: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> ¯\_(ツ)_/¯ > Signed-off-by: Warner Losh <imp@bsdimp.com> > --- > target/arm/gdbstub64.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c > index 2e2bc2700b8..6dc81aecb2a 100644 > --- a/target/arm/gdbstub64.c > +++ b/target/arm/gdbstub64.c > @@ -404,6 +404,7 @@ int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg) > > int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg) > { > +#if defined(CONFIG_LINUX) > ARMCPU *cpu = ARM_CPU(cs); > CPUARMState *env = &cpu->env; > > @@ -425,6 +426,9 @@ int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg) > arm_set_mte_tcf0(env, tcf); > > return 1; > +#else > + return 0; > +#endif > } > > static void handle_q_memtag(GArray *params, void *user_ctx)
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > Hi Warner, > > On 22/7/24 23:43, Warner Losh wrote: >> We include the files that define PR_MTE_TCF_SHIFT only on Linux, but use >> them unconditionally. Restrict its use to Linux-only. > > We should check that in meson, i.e.: > > config_host_data.set('CONFIG_PRCTL_PR_MTE_TCF_SHIFT', > cc.has_header_symbol('sys/prctl.h', > 'PR_MTE_TCF_SHIFT')) Surely this fails for non-aarch64 hosts? > (like we do for CONFIG_PRCTL_PR_SET_TIMERSLACK), then rework > linux-user/aarch64/mte_user_helper.h (and possibly > tests/tcg/aarch64/mte.h), moving in a common directory. > > That said, your patch matches this file header: > > #if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX) > #include <sys/prctl.h> > #include "mte_user_helper.h" > #endif > > Even if this #ifdef'ry is ugly, it isn't wrong, so: > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > ¯\_(ツ)_/¯ > >> Signed-off-by: Warner Losh <imp@bsdimp.com> >> --- >> target/arm/gdbstub64.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c >> index 2e2bc2700b8..6dc81aecb2a 100644 >> --- a/target/arm/gdbstub64.c >> +++ b/target/arm/gdbstub64.c >> @@ -404,6 +404,7 @@ int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg) >> int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int >> reg) >> { >> +#if defined(CONFIG_LINUX) >> ARMCPU *cpu = ARM_CPU(cs); >> CPUARMState *env = &cpu->env; >> @@ -425,6 +426,9 @@ int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, >> uint8_t *buf, int reg) >> arm_set_mte_tcf0(env, tcf); >> return 1; >> +#else >> + return 0; >> +#endif >> } >> static void handle_q_memtag(GArray *params, void *user_ctx)
On 7/23/24 07:43, Warner Losh wrote: > We include the files that define PR_MTE_TCF_SHIFT only on Linux, but use > them unconditionally. Restrict its use to Linux-only. > > Signed-off-by: Warner Losh<imp@bsdimp.com> > --- > target/arm/gdbstub64.c | 4 ++++ > 1 file changed, 4 insertions(+) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Mon, Jul 22, 2024 at 4:01 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > Hi Warner, > > On 22/7/24 23:43, Warner Losh wrote: > > We include the files that define PR_MTE_TCF_SHIFT only on Linux, but use > > them unconditionally. Restrict its use to Linux-only. > > We should check that in meson, i.e.: > > config_host_data.set('CONFIG_PRCTL_PR_MTE_TCF_SHIFT', > cc.has_header_symbol('sys/prctl.h', > 'PR_MTE_TCF_SHIFT')) > > (like we do for CONFIG_PRCTL_PR_SET_TIMERSLACK), then rework > linux-user/aarch64/mte_user_helper.h (and possibly > tests/tcg/aarch64/mte.h), moving in a common directory. > Hmmm... I'll have to check, but since I didn't add the incompatibility, it will likely be on my backlog for a while... > That said, your patch matches this file header: > > #if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX) > #include <sys/prctl.h> > #include "mte_user_helper.h" > #endif > > Even if this #ifdef'ry is ugly, it isn't wrong, so: > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > ¯\_(ツ)_/¯ > Thanks! Warner > > Signed-off-by: Warner Losh <imp@bsdimp.com> > > --- > > target/arm/gdbstub64.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c > > index 2e2bc2700b8..6dc81aecb2a 100644 > > --- a/target/arm/gdbstub64.c > > +++ b/target/arm/gdbstub64.c > > @@ -404,6 +404,7 @@ int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, > GByteArray *buf, int reg) > > > > int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg) > > { > > +#if defined(CONFIG_LINUX) > > ARMCPU *cpu = ARM_CPU(cs); > > CPUARMState *env = &cpu->env; > > > > @@ -425,6 +426,9 @@ int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, > uint8_t *buf, int reg) > > arm_set_mte_tcf0(env, tcf); > > > > return 1; > > +#else > > + return 0; > > +#endif > > } > > > > static void handle_q_memtag(GArray *params, void *user_ctx) > >
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c index 2e2bc2700b8..6dc81aecb2a 100644 --- a/target/arm/gdbstub64.c +++ b/target/arm/gdbstub64.c @@ -404,6 +404,7 @@ int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg) int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg) { +#if defined(CONFIG_LINUX) ARMCPU *cpu = ARM_CPU(cs); CPUARMState *env = &cpu->env; @@ -425,6 +426,9 @@ int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg) arm_set_mte_tcf0(env, tcf); return 1; +#else + return 0; +#endif } static void handle_q_memtag(GArray *params, void *user_ctx)
We include the files that define PR_MTE_TCF_SHIFT only on Linux, but use them unconditionally. Restrict its use to Linux-only. Signed-off-by: Warner Losh <imp@bsdimp.com> --- target/arm/gdbstub64.c | 4 ++++ 1 file changed, 4 insertions(+)