Quantcast

amd64 struct reg

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

amd64 struct reg

Mark Kettenis
It turns out that pretty much all relevant aarch64 OSes use the same
layout for transferring registers in their debug interfaces.  Except
for us.  That doesn't make sense and would mean I'd have to do
additional work in my lldb porting efforts.

Diff below revises "struct reg" for amd64 to be compatible with what
NetBSD provides.  That just matches our naming conventions for struct
reg better.  The actual names are largely irrelevant as debuggers
hardcode the layouts anyway to support cross-debugging.

This struct isn't actually used yet, so these changes don't really
have any ABI consequences.  But once this is in, I'm planning on
making core dumps and ptrace actually work on arm64.

ok?


Index: arch/arm64/include/reg.h
===================================================================
RCS file: /cvs/src/sys/arch/arm64/include/reg.h,v
retrieving revision 1.1
diff -u -p -r1.1 reg.h
--- arch/arm64/include/reg.h 17 Dec 2016 23:38:33 -0000 1.1
+++ arch/arm64/include/reg.h 20 Mar 2017 00:27:12 -0000
@@ -19,11 +19,11 @@
 #define _MACHINE_REG_H_
 
 struct reg {
- unsigned long x[30];
- unsigned long x_sp;
- unsigned long x_lr;
- unsigned long x_pc;
- unsigned long x_cpsr;
+ unsigned long r_reg[31];
+ unsigned long r_sp;
+ unsigned long r_pc;
+ unsigned long r_spsr;
+ unsigned long r_tpidr;
 };
 
 struct fpreg {

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: amd64 struct reg

Jonathan Gray-11
On Mon, Mar 20, 2017 at 01:35:04AM +0100, Mark Kettenis wrote:

> It turns out that pretty much all relevant aarch64 OSes use the same
> layout for transferring registers in their debug interfaces.  Except
> for us.  That doesn't make sense and would mean I'd have to do
> additional work in my lldb porting efforts.
>
> Diff below revises "struct reg" for amd64 to be compatible with what
> NetBSD provides.  That just matches our naming conventions for struct
> reg better.  The actual names are largely irrelevant as debuggers
> hardcode the layouts anyway to support cross-debugging.
>
> This struct isn't actually used yet, so these changes don't really
> have any ABI consequences.  But once this is in, I'm planning on
> making core dumps and ptrace actually work on arm64.
>
> ok?

It looks like NetBSD just has a cross compiled toolchain
nothing that runs on actual hardware.

Given FreeBSD does I'd consider that more relevant, and it
has:

struct reg {
        uint64_t x[30];
        uint64_t lr;
        uint64_t sp;
        uint64_t elr;
        uint32_t spsr;
};

struct fpreg {
        __uint128_t fp_q[32];
        uint32_t fp_sr;
        uint32_t fp_cr;
};

>
>
> Index: arch/arm64/include/reg.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/include/reg.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 reg.h
> --- arch/arm64/include/reg.h 17 Dec 2016 23:38:33 -0000 1.1
> +++ arch/arm64/include/reg.h 20 Mar 2017 00:27:12 -0000
> @@ -19,11 +19,11 @@
>  #define _MACHINE_REG_H_
>  
>  struct reg {
> - unsigned long x[30];
> - unsigned long x_sp;
> - unsigned long x_lr;
> - unsigned long x_pc;
> - unsigned long x_cpsr;
> + unsigned long r_reg[31];
> + unsigned long r_sp;
> + unsigned long r_pc;
> + unsigned long r_spsr;
> + unsigned long r_tpidr;
>  };
>  
>  struct fpreg {
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: amd64 struct reg

Mark Kettenis
> Date: Mon, 20 Mar 2017 13:31:32 +1100
> From: Jonathan Gray <[hidden email]>
> Cc: [hidden email]
> Mail-Followup-To: Mark Kettenis <[hidden email]>, [hidden email]
> Content-Disposition: inline
> X-XS4ALL-DNSBL-Checked: mxdrop306.xs4all.net checked 210.15.216.215 against DNS blacklists
> X-CNFS-Analysis: v=2.2 cv=eoad9chX c=1 sm=0 tr=0
> a=0rbIscUo4apI/L6UuJIdkA==:117 a=0rbIscUo4apI/L6UuJIdkA==:17
> a=kj9zAlcOel0A:10 a=6Iz7jQTuP9IA:10 a=d_dndPjGE7KARIjjoGsA:9
> a=CjuIK1q_8ugA:10
> X-Virus-Scanned: by XS4ALL Virus Scanner
> X-XS4ALL-Spam-Score: -0.5 () RP_MATCHES_RCVD, UNPARSEABLE_RELAY
> X-XS4ALL-Spam: NO
> Envelope-To: [hidden email]
>
> On Mon, Mar 20, 2017 at 01:35:04AM +0100, Mark Kettenis wrote:
> > It turns out that pretty much all relevant aarch64 OSes use the same
> > layout for transferring registers in their debug interfaces.  Except
> > for us.  That doesn't make sense and would mean I'd have to do
> > additional work in my lldb porting efforts.
> >
> > Diff below revises "struct reg" for amd64 to be compatible with what
> > NetBSD provides.  That just matches our naming conventions for struct
> > reg better.  The actual names are largely irrelevant as debuggers
> > hardcode the layouts anyway to support cross-debugging.
> >
> > This struct isn't actually used yet, so these changes don't really
> > have any ABI consequences.  But once this is in, I'm planning on
> > making core dumps and ptrace actually work on arm64.
> >
> > ok?
>
> It looks like NetBSD just has a cross compiled toolchain
> nothing that runs on actual hardware.
>
> Given FreeBSD does I'd consider that more relevant, and it
> has:
>
> struct reg {
> uint64_t x[30];
> uint64_t lr;
> uint64_t sp;
> uint64_t elr;
> uint32_t spsr;
> };
>
> struct fpreg {
> __uint128_t fp_q[32];
> uint32_t fp_sr;
> uint32_t fp_cr;
> };

The FreeBSD definition is equivalent to the NetBSD one:

lr  == x30
elr == pc

It's just that the names assigned by NetBSD make a little bit more
sense and follow the pattern we use on many other architectures.

NetBSD also includes the userland per-thread-register which I think is
a good idea.

> > Index: arch/arm64/include/reg.h
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/arm64/include/reg.h,v
> > retrieving revision 1.1
> > diff -u -p -r1.1 reg.h
> > --- arch/arm64/include/reg.h 17 Dec 2016 23:38:33 -0000 1.1
> > +++ arch/arm64/include/reg.h 20 Mar 2017 00:27:12 -0000
> > @@ -19,11 +19,11 @@
> >  #define _MACHINE_REG_H_
> >  
> >  struct reg {
> > - unsigned long x[30];
> > - unsigned long x_sp;
> > - unsigned long x_lr;
> > - unsigned long x_pc;
> > - unsigned long x_cpsr;
> > + unsigned long r_reg[31];
> > + unsigned long r_sp;
> > + unsigned long r_pc;
> > + unsigned long r_spsr;
> > + unsigned long r_tpidr;
> >  };
> >  
> >  struct fpreg {
> >
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: amd64 struct reg

Dale Rahn
On Mon, Mar 20, 2017 at 11:54:03AM +0100, Mark Kettenis wrote:

> > Date: Mon, 20 Mar 2017 13:31:32 +1100
> > From: Jonathan Gray <[hidden email]>
> > Cc: [hidden email]
> > Mail-Followup-To: Mark Kettenis <[hidden email]>, [hidden email]
> > Content-Disposition: inline
> > X-XS4ALL-DNSBL-Checked: mxdrop306.xs4all.net checked 210.15.216.215 against DNS blacklists
> > X-CNFS-Analysis: v=2.2 cv=eoad9chX c=1 sm=0 tr=0
> > a=0rbIscUo4apI/L6UuJIdkA==:117 a=0rbIscUo4apI/L6UuJIdkA==:17
> > a=kj9zAlcOel0A:10 a=6Iz7jQTuP9IA:10 a=d_dndPjGE7KARIjjoGsA:9
> > a=CjuIK1q_8ugA:10
> > X-Virus-Scanned: by XS4ALL Virus Scanner
> > X-XS4ALL-Spam-Score: -0.5 () RP_MATCHES_RCVD, UNPARSEABLE_RELAY
> > X-XS4ALL-Spam: NO
> > Envelope-To: [hidden email]
> >
> > On Mon, Mar 20, 2017 at 01:35:04AM +0100, Mark Kettenis wrote:
> > > It turns out that pretty much all relevant aarch64 OSes use the same
> > > layout for transferring registers in their debug interfaces.  Except
> > > for us.  That doesn't make sense and would mean I'd have to do
> > > additional work in my lldb porting efforts.
> > >
> > > Diff below revises "struct reg" for amd64 to be compatible with what
> > > NetBSD provides.  That just matches our naming conventions for struct
> > > reg better.  The actual names are largely irrelevant as debuggers
> > > hardcode the layouts anyway to support cross-debugging.
> > >
> > > This struct isn't actually used yet, so these changes don't really
> > > have any ABI consequences.  But once this is in, I'm planning on
> > > making core dumps and ptrace actually work on arm64.
> > >
> > > ok?
> >
> > It looks like NetBSD just has a cross compiled toolchain
> > nothing that runs on actual hardware.
> >
> > Given FreeBSD does I'd consider that more relevant, and it
> > has:
> >
> > struct reg {
> > uint64_t x[30];
> > uint64_t lr;
> > uint64_t sp;
> > uint64_t elr;
> > uint32_t spsr;
> > };
> >
> > struct fpreg {
> > __uint128_t fp_q[32];
> > uint32_t fp_sr;
> > uint32_t fp_cr;
> > };
>
> The FreeBSD definition is equivalent to the NetBSD one:
>
> lr  == x30
> elr == pc
>
> It's just that the names assigned by NetBSD make a little bit more
> sense and follow the pattern we use on many other architectures.
>
> NetBSD also includes the userland per-thread-register which I think is
> a good idea.
>

Naming and order realistically doesn't matter that much, If it is easier to
move registers around to match a userland structure in process_*regs(),
that is reasonable. That said: the more I have followed NetBSD's lead in the
past, the more I have regretted it later.

Including the thread pointer would seem to make sense, but there is there
a proc vs process issue there (thread vs p

using __uint128_t for FPU is a lot better than the uint64 [64] that is there
now.

Dale Rahn [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: amd64 struct reg

Philip Guenther-2
On Mon, 20 Mar 2017, Dale Rahn wrote:
...
> Including the thread pointer would seem to make sense, but there is there
> a proc vs process issue there (thread vs p

Uh, the registers are _all_ per-thread!


Philip

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: amd64 struct reg

Dale Rahn
On Mon, Mar 20, 2017 at 09:15:09AM -0700, Philip Guenther wrote:

> On Mon, 20 Mar 2017, Dale Rahn wrote:
> ...
> > Including the thread pointer would seem to make sense, but there is there
> > a proc vs process issue there (thread vs p
>
> Uh, the registers are _all_ per-thread!
>
>
> Philip
>

Oops? I thought I had deleted that line of comment in the email. I had
looked it up and realized my mistake already.

Dale Rahn [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: amd64 struct reg

Mark Kettenis
In reply to this post by Dale Rahn
> Date: Mon, 20 Mar 2017 12:26:28 -0400
> From: Dale Rahn <[hidden email]>
>
> On Mon, Mar 20, 2017 at 11:54:03AM +0100, Mark Kettenis wrote:
> > > Date: Mon, 20 Mar 2017 13:31:32 +1100
> > > From: Jonathan Gray <[hidden email]>
> > > Cc: [hidden email]
> > > Mail-Followup-To: Mark Kettenis <[hidden email]>, [hidden email]
> > > Content-Disposition: inline
> > > X-XS4ALL-DNSBL-Checked: mxdrop306.xs4all.net checked 210.15.216.215 against DNS blacklists
> > > X-CNFS-Analysis: v=2.2 cv=eoad9chX c=1 sm=0 tr=0
> > > a=0rbIscUo4apI/L6UuJIdkA==:117 a=0rbIscUo4apI/L6UuJIdkA==:17
> > > a=kj9zAlcOel0A:10 a=6Iz7jQTuP9IA:10 a=d_dndPjGE7KARIjjoGsA:9
> > > a=CjuIK1q_8ugA:10
> > > X-Virus-Scanned: by XS4ALL Virus Scanner
> > > X-XS4ALL-Spam-Score: -0.5 () RP_MATCHES_RCVD, UNPARSEABLE_RELAY
> > > X-XS4ALL-Spam: NO
> > > Envelope-To: [hidden email]
> > >
> > > On Mon, Mar 20, 2017 at 01:35:04AM +0100, Mark Kettenis wrote:
> > > > It turns out that pretty much all relevant aarch64 OSes use the same
> > > > layout for transferring registers in their debug interfaces.  Except
> > > > for us.  That doesn't make sense and would mean I'd have to do
> > > > additional work in my lldb porting efforts.
> > > >
> > > > Diff below revises "struct reg" for amd64 to be compatible with what
> > > > NetBSD provides.  That just matches our naming conventions for struct
> > > > reg better.  The actual names are largely irrelevant as debuggers
> > > > hardcode the layouts anyway to support cross-debugging.
> > > >
> > > > This struct isn't actually used yet, so these changes don't really
> > > > have any ABI consequences.  But once this is in, I'm planning on
> > > > making core dumps and ptrace actually work on arm64.
> > > >
> > > > ok?
> > >
> > > It looks like NetBSD just has a cross compiled toolchain
> > > nothing that runs on actual hardware.
> > >
> > > Given FreeBSD does I'd consider that more relevant, and it
> > > has:
> > >
> > > struct reg {
> > > uint64_t x[30];
> > > uint64_t lr;
> > > uint64_t sp;
> > > uint64_t elr;
> > > uint32_t spsr;
> > > };
> > >
> > > struct fpreg {
> > > __uint128_t fp_q[32];
> > > uint32_t fp_sr;
> > > uint32_t fp_cr;
> > > };
> >
> > The FreeBSD definition is equivalent to the NetBSD one:
> >
> > lr  == x30
> > elr == pc
> >
> > It's just that the names assigned by NetBSD make a little bit more
> > sense and follow the pattern we use on many other architectures.
> >
> > NetBSD also includes the userland per-thread-register which I think is
> > a good idea.
> >
>
> Naming and order realistically doesn't matter that much, If it is easier to
> move registers around to match a userland structure in process_*regs(),
> that is reasonable. That said: the more I have followed NetBSD's lead in the
> past, the more I have regretted it later.

Here is an update diff that actually implementes process_read_regs().
Changed the struct reg definition slightly to make r_rl explicit to
match our struct trapframe a bit closer.  That makes the
process_read_regs() implementation more obvious.

This is enough to make core dumps useful, so I'd like to commit this
partial implementation.  Did explicitly zero out the struct fpreg in
process_read_fpregs() to avoid leaking kernel stack contents.

> using __uint128_t for FPU is a lot better than the uint64 [64] that is there
> now.

I'd like to address that in a future diff.

ok?

Index: include/reg.h
===================================================================
RCS file: /cvs/src/sys/arch/arm64/include/reg.h,v
retrieving revision 1.1
diff -u -p -r1.1 reg.h
--- include/reg.h 17 Dec 2016 23:38:33 -0000 1.1
+++ include/reg.h 21 Mar 2017 14:52:35 -0000
@@ -19,11 +19,12 @@
 #define _MACHINE_REG_H_
 
 struct reg {
- unsigned long x[30];
- unsigned long x_sp;
- unsigned long x_lr;
- unsigned long x_pc;
- unsigned long x_cpsr;
+ uint64_t r_reg[30];
+ uint64_t r_lr;
+ uint64_t r_sp;
+ uint64_t r_pc;
+ uint64_t r_spsr;
+ uint64_t r_tpidr;
 };
 
 struct fpreg {
Index: arm64/process_machdep.c
===================================================================
RCS file: /cvs/src/sys/arch/arm64/arm64/process_machdep.c,v
retrieving revision 1.1
diff -u -p -r1.1 process_machdep.c
--- arm64/process_machdep.c 17 Dec 2016 23:38:33 -0000 1.1
+++ arm64/process_machdep.c 21 Mar 2017 14:52:35 -0000
@@ -53,12 +53,22 @@
 int
 process_read_regs(struct proc *p, struct reg *regs)
 {
+ struct trapframe *tf = p->p_addr->u_pcb.pcb_tf;
+
+ memcpy(&regs->r_reg[0], &tf->tf_x[0], sizeof(regs->r_reg));
+ regs->r_lr = tf->tf_lr;
+ regs->r_sp = tf->tf_sp;
+ regs->r_pc = tf->tf_elr;
+ regs->r_spsr = tf->tf_spsr;
+ regs->r_tpidr = (uint64_t)p->p_addr->u_pcb.pcb_tcb;
+
  return(0);
 }
 
 int
 process_read_fpregs(struct proc *p, struct fpreg *regs)
 {
+ memset(&regs, 0, sizeof(*regs));
  return(0);
 }
 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: amd64 struct reg

Mark Kettenis
> Date: Tue, 21 Mar 2017 15:57:46 +0100 (CET)
> From: Mark Kettenis <[hidden email]>
>
> > Date: Mon, 20 Mar 2017 12:26:28 -0400
> > From: Dale Rahn <[hidden email]>
> >
> > On Mon, Mar 20, 2017 at 11:54:03AM +0100, Mark Kettenis wrote:
> > > > Date: Mon, 20 Mar 2017 13:31:32 +1100
> > > > From: Jonathan Gray <[hidden email]>
> > > > Cc: [hidden email]
> > > > Mail-Followup-To: Mark Kettenis <[hidden email]>, [hidden email]
> > > > Content-Disposition: inline
> > > > X-XS4ALL-DNSBL-Checked: mxdrop306.xs4all.net checked 210.15.216.215 against DNS blacklists
> > > > X-CNFS-Analysis: v=2.2 cv=eoad9chX c=1 sm=0 tr=0
> > > > a=0rbIscUo4apI/L6UuJIdkA==:117 a=0rbIscUo4apI/L6UuJIdkA==:17
> > > > a=kj9zAlcOel0A:10 a=6Iz7jQTuP9IA:10 a=d_dndPjGE7KARIjjoGsA:9
> > > > a=CjuIK1q_8ugA:10
> > > > X-Virus-Scanned: by XS4ALL Virus Scanner
> > > > X-XS4ALL-Spam-Score: -0.5 () RP_MATCHES_RCVD, UNPARSEABLE_RELAY
> > > > X-XS4ALL-Spam: NO
> > > > Envelope-To: [hidden email]
> > > >
> > > > On Mon, Mar 20, 2017 at 01:35:04AM +0100, Mark Kettenis wrote:
> > > > > It turns out that pretty much all relevant aarch64 OSes use the same
> > > > > layout for transferring registers in their debug interfaces.  Except
> > > > > for us.  That doesn't make sense and would mean I'd have to do
> > > > > additional work in my lldb porting efforts.
> > > > >
> > > > > Diff below revises "struct reg" for amd64 to be compatible with what
> > > > > NetBSD provides.  That just matches our naming conventions for struct
> > > > > reg better.  The actual names are largely irrelevant as debuggers
> > > > > hardcode the layouts anyway to support cross-debugging.
> > > > >
> > > > > This struct isn't actually used yet, so these changes don't really
> > > > > have any ABI consequences.  But once this is in, I'm planning on
> > > > > making core dumps and ptrace actually work on arm64.
> > > > >
> > > > > ok?
> > > >
> > > > It looks like NetBSD just has a cross compiled toolchain
> > > > nothing that runs on actual hardware.
> > > >
> > > > Given FreeBSD does I'd consider that more relevant, and it
> > > > has:
> > > >
> > > > struct reg {
> > > > uint64_t x[30];
> > > > uint64_t lr;
> > > > uint64_t sp;
> > > > uint64_t elr;
> > > > uint32_t spsr;
> > > > };
> > > >
> > > > struct fpreg {
> > > > __uint128_t fp_q[32];
> > > > uint32_t fp_sr;
> > > > uint32_t fp_cr;
> > > > };
> > >
> > > The FreeBSD definition is equivalent to the NetBSD one:
> > >
> > > lr  == x30
> > > elr == pc
> > >
> > > It's just that the names assigned by NetBSD make a little bit more
> > > sense and follow the pattern we use on many other architectures.
> > >
> > > NetBSD also includes the userland per-thread-register which I think is
> > > a good idea.
> > >
> >
> > Naming and order realistically doesn't matter that much, If it is easier to
> > move registers around to match a userland structure in process_*regs(),
> > that is reasonable. That said: the more I have followed NetBSD's lead in the
> > past, the more I have regretted it later.
>
> Here is an update diff that actually implementes process_read_regs().
> Changed the struct reg definition slightly to make r_rl explicit to
> match our struct trapframe a bit closer.  That makes the
> process_read_regs() implementation more obvious.
>
> This is enough to make core dumps useful, so I'd like to commit this
> partial implementation.  Did explicitly zero out the struct fpreg in
> process_read_fpregs() to avoid leaking kernel stack contents.
>
> > using __uint128_t for FPU is a lot better than the uint64 [64] that is there
> > now.
>
> I'd like to address that in a future diff.
>
> ok?
>
> Index: arm64/process_machdep.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/arm64/process_machdep.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 process_machdep.c
> --- arm64/process_machdep.c 17 Dec 2016 23:38:33 -0000 1.1
> +++ arm64/process_machdep.c 21 Mar 2017 14:52:35 -0000
> @@ -53,12 +53,22 @@
>  int
>  process_read_regs(struct proc *p, struct reg *regs)
>  {
> + struct trapframe *tf = p->p_addr->u_pcb.pcb_tf;
> +
> + memcpy(&regs->r_reg[0], &tf->tf_x[0], sizeof(regs->r_reg));
> + regs->r_lr = tf->tf_lr;
> + regs->r_sp = tf->tf_sp;
> + regs->r_pc = tf->tf_elr;
> + regs->r_spsr = tf->tf_spsr;
> + regs->r_tpidr = (uint64_t)p->p_addr->u_pcb.pcb_tcb;
> +
>   return(0);
>  }
>  
>  int
>  process_read_fpregs(struct proc *p, struct fpreg *regs)
>  {
> + memset(&regs, 0, sizeof(*regs));

That should be:

  memset(regs, 0, sizeof(*regs));

Thanks to Peter J. Philipp for pointing that out.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: amd64 struct reg

Dale Rahn
On Tue, Mar 21, 2017 at 04:45:01PM +0100, Mark Kettenis wrote:

> > Date: Tue, 21 Mar 2017 15:57:46 +0100 (CET)
> > From: Mark Kettenis <[hidden email]>
> >
> > > Date: Mon, 20 Mar 2017 12:26:28 -0400
> > > From: Dale Rahn <[hidden email]>
> > >
> > > On Mon, Mar 20, 2017 at 11:54:03AM +0100, Mark Kettenis wrote:
> > > > > Date: Mon, 20 Mar 2017 13:31:32 +1100
> > > > > From: Jonathan Gray <[hidden email]>
> > > > > Cc: [hidden email]
> > > > > Mail-Followup-To: Mark Kettenis <[hidden email]>, [hidden email]
> > > > > Content-Disposition: inline
> > > > > X-XS4ALL-DNSBL-Checked: mxdrop306.xs4all.net checked 210.15.216.215 against DNS blacklists
> > > > > X-CNFS-Analysis: v=2.2 cv=eoad9chX c=1 sm=0 tr=0
> > > > > a=0rbIscUo4apI/L6UuJIdkA==:117 a=0rbIscUo4apI/L6UuJIdkA==:17
> > > > > a=kj9zAlcOel0A:10 a=6Iz7jQTuP9IA:10 a=d_dndPjGE7KARIjjoGsA:9
> > > > > a=CjuIK1q_8ugA:10
> > > > > X-Virus-Scanned: by XS4ALL Virus Scanner
> > > > > X-XS4ALL-Spam-Score: -0.5 () RP_MATCHES_RCVD, UNPARSEABLE_RELAY
> > > > > X-XS4ALL-Spam: NO
> > > > > Envelope-To: [hidden email]
> > > > >
> > > > > On Mon, Mar 20, 2017 at 01:35:04AM +0100, Mark Kettenis wrote:
> > > > > > It turns out that pretty much all relevant aarch64 OSes use the same
> > > > > > layout for transferring registers in their debug interfaces.  Except
> > > > > > for us.  That doesn't make sense and would mean I'd have to do
> > > > > > additional work in my lldb porting efforts.
> > > > > >
> > > > > > Diff below revises "struct reg" for amd64 to be compatible with what
> > > > > > NetBSD provides.  That just matches our naming conventions for struct
> > > > > > reg better.  The actual names are largely irrelevant as debuggers
> > > > > > hardcode the layouts anyway to support cross-debugging.
> > > > > >
> > > > > > This struct isn't actually used yet, so these changes don't really
> > > > > > have any ABI consequences.  But once this is in, I'm planning on
> > > > > > making core dumps and ptrace actually work on arm64.
> > > > > >
> > > > > > ok?
> > > > >
> > > > > It looks like NetBSD just has a cross compiled toolchain
> > > > > nothing that runs on actual hardware.
> > > > >
> > > > > Given FreeBSD does I'd consider that more relevant, and it
> > > > > has:
> > > > >
> > > > > struct reg {
> > > > > uint64_t x[30];
> > > > > uint64_t lr;
> > > > > uint64_t sp;
> > > > > uint64_t elr;
> > > > > uint32_t spsr;
> > > > > };
> > > > >
> > > > > struct fpreg {
> > > > > __uint128_t fp_q[32];
> > > > > uint32_t fp_sr;
> > > > > uint32_t fp_cr;
> > > > > };
> > > >
> > > > The FreeBSD definition is equivalent to the NetBSD one:
> > > >
> > > > lr  == x30
> > > > elr == pc
> > > >
> > > > It's just that the names assigned by NetBSD make a little bit more
> > > > sense and follow the pattern we use on many other architectures.
> > > >
> > > > NetBSD also includes the userland per-thread-register which I think is
> > > > a good idea.
> > > >
> > >
> > > Naming and order realistically doesn't matter that much, If it is easier to
> > > move registers around to match a userland structure in process_*regs(),
> > > that is reasonable. That said: the more I have followed NetBSD's lead in the
> > > past, the more I have regretted it later.
> >
> > Here is an update diff that actually implementes process_read_regs().
> > Changed the struct reg definition slightly to make r_rl explicit to
> > match our struct trapframe a bit closer.  That makes the
> > process_read_regs() implementation more obvious.
> >
> > This is enough to make core dumps useful, so I'd like to commit this
> > partial implementation.  Did explicitly zero out the struct fpreg in
> > process_read_fpregs() to avoid leaking kernel stack contents.
> >
> > > using __uint128_t for FPU is a lot better than the uint64 [64] that is there
> > > now.
> >
> > I'd like to address that in a future diff.
> >
> > ok?
> >
> > Index: arm64/process_machdep.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/arm64/arm64/process_machdep.c,v
> > retrieving revision 1.1
> > diff -u -p -r1.1 process_machdep.c
> > --- arm64/process_machdep.c 17 Dec 2016 23:38:33 -0000 1.1
> > +++ arm64/process_machdep.c 21 Mar 2017 14:52:35 -0000
> > @@ -53,12 +53,22 @@
> >  int
> >  process_read_regs(struct proc *p, struct reg *regs)
> >  {
> > + struct trapframe *tf = p->p_addr->u_pcb.pcb_tf;
> > +
> > + memcpy(&regs->r_reg[0], &tf->tf_x[0], sizeof(regs->r_reg));
> > + regs->r_lr = tf->tf_lr;
> > + regs->r_sp = tf->tf_sp;
> > + regs->r_pc = tf->tf_elr;
> > + regs->r_spsr = tf->tf_spsr;
> > + regs->r_tpidr = (uint64_t)p->p_addr->u_pcb.pcb_tcb;
> > +
> >   return(0);
> >  }
> >  
> >  int
> >  process_read_fpregs(struct proc *p, struct fpreg *regs)
> >  {
> > + memset(&regs, 0, sizeof(*regs));
>
> That should be:
>
>   memset(regs, 0, sizeof(*regs));
>
> Thanks to Peter J. Philipp for pointing that out.
>

With this fix, the diff is ok drahn@

Dale Rahn [hidden email]

Loading...