Re: [PATCH -tip v8 5/7] x86: add pt_regs register and stack access APIs

Re: [PATCH -tip v8 5/7] x86: add pt_regs register and stack access APIs

[Christoph Hellwig]

You might want to run this past linux-arch to make sure this is suitable
for other architectures.

> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -7,6 +7,7 @@
>  
>  #ifdef __KERNEL__
>  #include <asm/segment.h>
> +#include <asm/page_types.h>
>  #endif

I really wonder if we should split asm/ptrace.h into one file
just defining pt_regs both for userspace and the kernel, and one with
all kinds of register access helpers and maybe another one for the
ptrace architecture interface.

Unforuntately we would have to keep the ptrace.h name for the one
carrying pt_regs as it's exposed to userland.

> +static inline unsigned long get_register(struct pt_regs *regs, unsigned offset)
> +{

I woner if all these names aren't a bit generic.  Shoud we maybe add a
regs_ prefix to make it clear it operates on a pt_regs register set?

Also some kerneldoc documentation for all these helpers would be nice.

> +/* Get Nth argument at function call */
> +static inline unsigned long get_argument_nth(struct pt_regs *regs, unsigned n)
> +{
> +#ifdef CONFIG_X86_32
> +#define NR_REGPARMS 3

I think completely separate version for 32 vs 64 bit for this one would
be cleaner.

> +	if (n < NR_REGPARMS) {
> +		switch (n) {
> +		case 0: return regs->ax;
> +		case 1: return regs->dx;
> +		case 2: return regs->cx;
> +		}

Normal kernel style would be

		switch (n) {
		case 0:
			return regs->ax;
		case 1:
			return regs->dx;
		case 2:
			return regs->cx;
		}

> +#define REG_OFFSET(r) offsetof(struct pt_regs, r)
> +#define REG_OFFSET_NAME(r) {.name = #r, .offset = REG_OFFSET(r)}
> +#define REG_OFFSET_END {.name = NULL, .offset = 0}

At least the REG_OFFSET macro seems superflous to me.

Re: [PATCH -tip v8 5/7] x86: add pt_regs register and stack access APIs

[Masami Hiramatsu]

Hi Christoph,

Thank you for review.

Christoph Hellwig wrote:
> You might want to run this past linux-arch to make sure this is suitable
> for other architectures.

Frankly, I'm not sure about linux-arch, could you explain it?
Anyway, I'm interested in that idea.

>> --- a/arch/x86/include/asm/ptrace.h
>> +++ b/arch/x86/include/asm/ptrace.h
>> @@ -7,6 +7,7 @@
>>  
>>  #ifdef __KERNEL__
>>  #include <asm/segment.h>
>> +#include <asm/page_types.h>
>>  #endif
> 
> I really wonder if we should split asm/ptrace.h into one file
> just defining pt_regs both for userspace and the kernel, and one with
> all kinds of register access helpers and maybe another one for the
> ptrace architecture interface.

Agreed, pt_regs is used more broadly than ptrace itself in kernel.

> Unforuntately we would have to keep the ptrace.h name for the one
> carrying pt_regs as it's exposed to userland.

Perhaps, we should split pt_regs from ptrace.h, like as ptrace-regs.h.

>> +static inline unsigned long get_register(struct pt_regs *regs, unsigned offset)
>> +{
> 
> I woner if all these names aren't a bit generic.  Shoud we maybe add a
> regs_ prefix to make it clear it operates on a pt_regs register set?

Indeed.

> Also some kerneldoc documentation for all these helpers would be nice.

Sure.

>> +/* Get Nth argument at function call */
>> +static inline unsigned long get_argument_nth(struct pt_regs *regs, unsigned n)
>> +{
>> +#ifdef CONFIG_X86_32
>> +#define NR_REGPARMS 3
> 
> I think completely separate version for 32 vs 64 bit for this one would
> be cleaner.

Agreed,

> 
>> +	if (n < NR_REGPARMS) {
>> +		switch (n) {
>> +		case 0: return regs->ax;
>> +		case 1: return regs->dx;
>> +		case 2: return regs->cx;
>> +		}
> 
> Normal kernel style would be
> 
> 		switch (n) {
> 		case 0:
> 			return regs->ax;
> 		case 1:
> 			return regs->dx;
> 		case 2:
> 			return regs->cx;
> 		}

Oops, thanks,

> 
>> +#define REG_OFFSET(r) offsetof(struct pt_regs, r)
>> +#define REG_OFFSET_NAME(r) {.name = #r, .offset = REG_OFFSET(r)}
>> +#define REG_OFFSET_END {.name = NULL, .offset = 0}
> 
> At least the REG_OFFSET macro seems superflous to me.
> 

Exactly.

Thank you again!

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH -tip v8 5/7] x86: add pt_regs register and stack access APIs

[Ingo Molnar]

* Christoph Hellwig <hch@infradead.org> wrote:

> > +	if (n < NR_REGPARMS) {
> > +		switch (n) {
> > +		case 0: return regs->ax;
> > +		case 1: return regs->dx;
> > +		case 2: return regs->cx;
> > +		}
> 
> Normal kernel style would be
> 
> 		switch (n) {
> 		case 0:
> 			return regs->ax;
> 		case 1:
> 			return regs->dx;
> 		case 2:
> 			return regs->cx;
> 		}

the original single-line shortcut is acceptable too.

	Ingo

Re: [PATCH -tip v8 5/7] x86: add pt_regs register and stack access APIs

[Ingo Molnar]

* Christoph Hellwig <hch@infradead.org> wrote:

> > +	if (n < NR_REGPARMS) {
> > +		switch (n) {
> > +		case 0: return regs->ax;
> > +		case 1: return regs->dx;
> > +		case 2: return regs->cx;
> > +		}
> 
> Normal kernel style would be
> 
> 		switch (n) {
> 		case 0:
> 			return regs->ax;
> 		case 1:
> 			return regs->dx;
> 		case 2:
> 			return regs->cx;
> 		}

the original single-line shortcut is acceptable too.

	Ingo