| Message ID | 20220829154921.837871-4-broonie@kernel.org |
|---|---|
| State | Accepted |
| Commit | 0027d9c6c7b57b61b82f7275633ba89d0de2d2fd |
| Headers | show |
| Series | [v4,1/4] kselftest/arm64: Add test coverage for NT_ARM_TLS | expand |
On Mon, Aug 29, 2022 at 04:49:20PM +0100, Mark Brown wrote: > @@ -1392,7 +1407,7 @@ static const struct user_regset aarch64_regsets[] = { > }, > [REGSET_TLS] = { > .core_note_type = NT_ARM_TLS, > - .n = 1, > + .n = 2, > .size = sizeof(void *), > .align = sizeof(void *), > .regset_get = tls_get, Does this change confuse user-space? I presume an updated gdb would check the iov.len to figure out whether a new register is available but would existing debuggers complain of the new size of this regset?
On Fri, Sep 16, 2022 at 01:01:31PM +0100, Catalin Marinas wrote: > On Mon, Aug 29, 2022 at 04:49:20PM +0100, Mark Brown wrote: > > @@ -1392,7 +1407,7 @@ static const struct user_regset aarch64_regsets[] = { > > }, > > [REGSET_TLS] = { > > .core_note_type = NT_ARM_TLS, > > - .n = 1, > > + .n = 2, > > .size = sizeof(void *), > > .align = sizeof(void *), > > .regset_get = tls_get, > Does this change confuse user-space? I presume an updated gdb would > check the iov.len to figure out whether a new register is available but > would existing debuggers complain of the new size of this regset? gdb seems happy as far as I can see, it is possible something would be reusing the read_iov for repeated TLS read calls in a context where it was only pointing at a single u64 but I'm not sure how realistic that is given the idiom. I did do a search on sources.debian.net and didn't turn up anything that'd have problems. If using this as an extensiblility mechanism is a concern we need to bear that in mind elsewhere, and for this it's either a case of providing another single register regset or trying to do a generic sysreg read/get (though that'd be another regset that's not idiomatic for the regset API).
On 9/19/22 13:43, Mark Brown wrote: > On Fri, Sep 16, 2022 at 01:01:31PM +0100, Catalin Marinas wrote: >> On Mon, Aug 29, 2022 at 04:49:20PM +0100, Mark Brown wrote: >>> @@ -1392,7 +1407,7 @@ static const struct user_regset aarch64_regsets[] = { >>> }, >>> [REGSET_TLS] = { >>> .core_note_type = NT_ARM_TLS, >>> - .n = 1, >>> + .n = 2, >>> .size = sizeof(void *), >>> .align = sizeof(void *), >>> .regset_get = tls_get, > >> Does this change confuse user-space? I presume an updated gdb would >> check the iov.len to figure out whether a new register is available but >> would existing debuggers complain of the new size of this regset? > > gdb seems happy as far as I can see, it is possible something would be > reusing the read_iov for repeated TLS read calls in a context where it > was only pointing at a single u64 but I'm not sure how realistic that > is given the idiom. I did do a search on sources.debian.net and didn't > turn up anything that'd have problems. > > If using this as an extensiblility mechanism is a concern we need to > bear that in mind elsewhere, and for this it's either a case of > providing another single register regset or trying to do a generic > sysreg read/get (though that'd be another regset that's not idiomatic > for the regset API). Older GDB's assume a single register for NT_ARM_TLS, so they will always fetch TPIDR. Newer GDB's will check the size and act accordingly.
On Thu, Sep 22, 2022 at 12:15:13PM +0100, Luis Machado wrote: > On 9/19/22 13:43, Mark Brown wrote: > > On Fri, Sep 16, 2022 at 01:01:31PM +0100, Catalin Marinas wrote: > > > On Mon, Aug 29, 2022 at 04:49:20PM +0100, Mark Brown wrote: > > > > @@ -1392,7 +1407,7 @@ static const struct user_regset aarch64_regsets[] = { > > > > }, > > > > [REGSET_TLS] = { > > > > .core_note_type = NT_ARM_TLS, > > > > - .n = 1, > > > > + .n = 2, > > > > .size = sizeof(void *), > > > > .align = sizeof(void *), > > > > .regset_get = tls_get, > > > > > Does this change confuse user-space? I presume an updated gdb would > > > check the iov.len to figure out whether a new register is available but > > > would existing debuggers complain of the new size of this regset? > > > > gdb seems happy as far as I can see, it is possible something would be > > reusing the read_iov for repeated TLS read calls in a context where it > > was only pointing at a single u64 but I'm not sure how realistic that > > is given the idiom. I did do a search on sources.debian.net and didn't > > turn up anything that'd have problems. > > > > If using this as an extensiblility mechanism is a concern we need to > > bear that in mind elsewhere, and for this it's either a case of > > providing another single register regset or trying to do a generic > > sysreg read/get (though that'd be another regset that's not idiomatic > > for the regset API). > > Older GDB's assume a single register for NT_ARM_TLS, so they will always > fetch TPIDR. Newer GDB's will check the size and act accordingly. That's fine. Looking at the ptrace_regset() implementation in Linux, it updates the user iov_len to what was actually copied. In this case it would be 1 (the minimum of the user iov_len and the regset n * size). So from the old gdb ABI perspective, it shouldn't notice a difference. An old gdb setting the TLS reg will also leave tpidr2_el0 unchanged. An updated gdb running on an older kernel should be aware that even if it asked for two u64, it may only get one back and the user iov_len updated accordingly.
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index eb7c08dfb834..0e9764f73d61 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -666,10 +666,18 @@ static int fpr_set(struct task_struct *target, const struct user_regset *regset, static int tls_get(struct task_struct *target, const struct user_regset *regset, struct membuf to) { + int ret; + if (target == current) tls_preserve_current_state(); - return membuf_store(&to, target->thread.uw.tp_value); + ret = membuf_store(&to, target->thread.uw.tp_value); + if (system_supports_tpidr2()) + ret = membuf_store(&to, target->thread.tpidr2_el0); + else + ret = membuf_zero(&to, sizeof(u64)); + + return ret; } static int tls_set(struct task_struct *target, const struct user_regset *regset, @@ -677,13 +685,20 @@ static int tls_set(struct task_struct *target, const struct user_regset *regset, const void *kbuf, const void __user *ubuf) { int ret; - unsigned long tls = target->thread.uw.tp_value; + unsigned long tls[2]; - ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &tls, 0, -1); + tls[0] = target->thread.uw.tp_value; + if (system_supports_sme()) + tls[1] = target->thread.tpidr2_el0; + + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, tls, 0, count); if (ret) return ret; - target->thread.uw.tp_value = tls; + target->thread.uw.tp_value = tls[0]; + if (system_supports_sme()) + target->thread.tpidr2_el0 = tls[1]; + return ret; } @@ -1392,7 +1407,7 @@ static const struct user_regset aarch64_regsets[] = { }, [REGSET_TLS] = { .core_note_type = NT_ARM_TLS, - .n = 1, + .n = 2, .size = sizeof(void *), .align = sizeof(void *), .regset_get = tls_get,
SME introduces an additional EL0 register, TPIDR2_EL0, intended for use by userspace as part of the SME. Provide ptrace access to it through the existing NT_ARM_TLS regset used for TPIDR_EL0 by expanding it to two registers with TPIDR2_EL0 being the second one. Existing programs that query the size of the register set will be able to observe the increased size of the register set. Programs that assume the register set is single register will see no change. On systems that do not support SME TPIDR2_EL0 will read as 0 and writes will be ignored, support for SME should be queried via hwcaps as normal. Signed-off-by: Mark Brown <broonie@kernel.org> --- arch/arm64/kernel/ptrace.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)