diff mbox series

[v4,3/4] arm64/ptrace: Support access to TPIDR2_EL0

Message ID 20220829154921.837871-4-broonie@kernel.org (mailing list archive)
State Accepted
Commit 0027d9c6c7b57b61b82f7275633ba89d0de2d2fd
Headers show
Series arm64/sme: ptrace support for TPIDR2_EL0 | expand

Commit Message

Mark Brown Aug. 29, 2022, 3:49 p.m. UTC
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(-)

Comments

Catalin Marinas Sept. 16, 2022, 12:01 p.m. UTC | #1
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?
Mark Brown Sept. 19, 2022, 12:43 p.m. UTC | #2
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).
Luis Machado Sept. 22, 2022, 11:15 a.m. UTC | #3
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.
Catalin Marinas Sept. 22, 2022, 1:57 p.m. UTC | #4
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 mbox series

Patch

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,