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

[Christoph H. <hc...@in...>]

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.