[PATCH 1/3] powerpc: Split user/kernel definitions of struct pt_regs

[PATCH 1/3] powerpc: Split user/kernel definitions of struct pt_regs

[Michael Ellerman]

We use a shared definition for struct pt_regs in uapi/asm/ptrace.h.
That means the layout of the structure is ABI, ie. we can't change it.

That would be fine if it was only used to describe the user-visible
register state of a process, but it's also the struct we use in the
kernel to describe the registers saved in an interrupt frame.

We'd like more flexibility in the content (and possibly layout) of the
kernel version of the struct, but currently that's not possible.

So split the definition into a user-visible definition which remains
unchanged, and a kernel internal one.

At the moment they're still identical, and we check that at build
time. That's because we have code (in ptrace etc.) that assumes that
they are the same. We will fix that code in future patches, and then
we can break the strict symmetry between the two structs.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/ptrace.h      | 27 ++++++++++++++++++
 arch/powerpc/include/uapi/asm/ptrace.h |  7 ++++-
 arch/powerpc/kernel/ptrace.c           | 39 ++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 447cbd1bee99..3dd15024db93 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -26,6 +26,33 @@
 #include <uapi/asm/ptrace.h>
 #include <asm/asm-const.h>
 
+#ifndef __ASSEMBLY__
+struct pt_regs
+{
+	union {
+		struct user_pt_regs user_regs;
+		struct {
+			unsigned long gpr[32];
+			unsigned long nip;
+			unsigned long msr;
+			unsigned long orig_gpr3;
+			unsigned long ctr;
+			unsigned long link;
+			unsigned long xer;
+			unsigned long ccr;
+#ifdef CONFIG_PPC64
+			unsigned long softe;
+#else
+			unsigned long mq;
+#endif
+			unsigned long trap;
+			unsigned long dar;
+			unsigned long dsisr;
+			unsigned long result;
+		};
+	};
+};
+#endif
 
 #ifdef __powerpc64__
 
diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
index 55c7a131d2ab..f5f1ccc740fc 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -29,7 +29,12 @@
 
 #ifndef __ASSEMBLY__
 
-struct pt_regs {
+#ifdef __KERNEL__
+struct user_pt_regs
+#else
+struct pt_regs
+#endif
+{
 	unsigned long gpr[32];
 	unsigned long nip;
 	unsigned long msr;
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 4e372f54088f..939d7f81bbbe 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -3335,3 +3335,42 @@ void do_syscall_trace_leave(struct pt_regs *regs)
 
 	user_enter();
 }
+
+void __init pt_regs_check(void)
+{
+	BUILD_BUG_ON(offsetof(struct pt_regs, gpr) !=
+		     offsetof(struct user_pt_regs, gpr));
+	BUILD_BUG_ON(offsetof(struct pt_regs, nip) !=
+		     offsetof(struct user_pt_regs, nip));
+	BUILD_BUG_ON(offsetof(struct pt_regs, msr) !=
+		     offsetof(struct user_pt_regs, msr));
+	BUILD_BUG_ON(offsetof(struct pt_regs, msr) !=
+		     offsetof(struct user_pt_regs, msr));
+	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
+		     offsetof(struct user_pt_regs, orig_gpr3));
+	BUILD_BUG_ON(offsetof(struct pt_regs, ctr) !=
+		     offsetof(struct user_pt_regs, ctr));
+	BUILD_BUG_ON(offsetof(struct pt_regs, link) !=
+		     offsetof(struct user_pt_regs, link));
+	BUILD_BUG_ON(offsetof(struct pt_regs, xer) !=
+		     offsetof(struct user_pt_regs, xer));
+	BUILD_BUG_ON(offsetof(struct pt_regs, ccr) !=
+		     offsetof(struct user_pt_regs, ccr));
+#ifdef __powerpc64__
+	BUILD_BUG_ON(offsetof(struct pt_regs, softe) !=
+		     offsetof(struct user_pt_regs, softe));
+#else
+	BUILD_BUG_ON(offsetof(struct pt_regs, mq) !=
+		     offsetof(struct user_pt_regs, mq));
+#endif
+	BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
+		     offsetof(struct user_pt_regs, trap));
+	BUILD_BUG_ON(offsetof(struct pt_regs, dar) !=
+		     offsetof(struct user_pt_regs, dar));
+	BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
+		     offsetof(struct user_pt_regs, dsisr));
+	BUILD_BUG_ON(offsetof(struct pt_regs, result) !=
+		     offsetof(struct user_pt_regs, result));
+
+	BUILD_BUG_ON(sizeof(struct user_pt_regs) > sizeof(struct pt_regs));
+}
-- 
2.17.1

[PATCH 2/3] powerpc/ptrace: Don't use sizeof(struct pt_regs) in ptrace code

[Michael Ellerman]

Now that we've split the user & kernel versions of pt_regs we need to
be more careful in the ptrace code.

For now we've ensured the location of the fields in both structs is
the same, so most of the ptrace code doesn't need updating.

But there are a few places where we use sizeof(pt_regs), and these
will be wrong as soon as we increase the size of the kernel structure.

So flip them all to use sizeof(user_pt_regs).

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/ptrace.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 939d7f81bbbe..c7d0d0c1e34d 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -297,7 +297,7 @@ int ptrace_get_reg(struct task_struct *task, int regno, unsigned long *data)
 	}
 #endif
 
-	if (regno < (sizeof(struct pt_regs) / sizeof(unsigned long))) {
+	if (regno < (sizeof(struct user_pt_regs) / sizeof(unsigned long))) {
 		*data = ((unsigned long *)task->thread.regs)[regno];
 		return 0;
 	}
@@ -360,10 +360,10 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
 		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
 					  &target->thread.regs->orig_gpr3,
 					  offsetof(struct pt_regs, orig_gpr3),
-					  sizeof(struct pt_regs));
+					  sizeof(struct user_pt_regs));
 	if (!ret)
 		ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
-					       sizeof(struct pt_regs), -1);
+					       sizeof(struct user_pt_regs), -1);
 
 	return ret;
 }
@@ -853,10 +853,10 @@ static int tm_cgpr_get(struct task_struct *target,
 		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
 					  &target->thread.ckpt_regs.orig_gpr3,
 					  offsetof(struct pt_regs, orig_gpr3),
-					  sizeof(struct pt_regs));
+					  sizeof(struct user_pt_regs));
 	if (!ret)
 		ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
-					       sizeof(struct pt_regs), -1);
+					       sizeof(struct user_pt_regs), -1);
 
 	return ret;
 }
@@ -3131,7 +3131,7 @@ long arch_ptrace(struct task_struct *child, long request,
 	case PTRACE_GETREGS:	/* Get all pt_regs from the child. */
 		return copy_regset_to_user(child, &user_ppc_native_view,
 					   REGSET_GPR,
-					   0, sizeof(struct pt_regs),
+					   0, sizeof(struct user_pt_regs),
 					   datavp);
 
 #ifdef CONFIG_PPC64
@@ -3140,7 +3140,7 @@ long arch_ptrace(struct task_struct *child, long request,
 	case PTRACE_SETREGS:	/* Set all gp regs in the child. */
 		return copy_regset_from_user(child, &user_ppc_native_view,
 					     REGSET_GPR,
-					     0, sizeof(struct pt_regs),
+					     0, sizeof(struct user_pt_regs),
 					     datavp);
 
 	case PTRACE_GETFPREGS: /* Get the child FPU state (FPR0...31 + FPSCR) */
-- 
2.17.1

[PATCH 3/3] powerpc/64: Interrupts save PPR on stack rather than thread_struct

[Michael Ellerman]

From: Nicholas Piggin <npiggin@gmail.com>

PPR is the odd register out when it comes to interrupt handling, it is
saved in current->thread.ppr while all others are saved on the stack.

The difficulty with this is that accessing thread.ppr can cause a SLB
fault, but the SLB fault handler implementation in C change had
assumed the normal exception entry handlers would not cause an SLB
fault.

Fix this by allocating room in the interrupt stack to save PPR.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/exception-64s.h |  9 ++++-----
 arch/powerpc/include/asm/processor.h     |  6 ++----
 arch/powerpc/include/asm/ptrace.h        |  4 ++++
 arch/powerpc/kernel/asm-offsets.c        |  2 +-
 arch/powerpc/kernel/entry_64.S           | 15 +++++----------
 arch/powerpc/kernel/process.c            |  2 +-
 arch/powerpc/kernel/ptrace.c             |  4 ++--
 7 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
index a86feddddad0..403d73898a9a 100644
--- a/arch/powerpc/include/asm/exception-64s.h
+++ b/arch/powerpc/include/asm/exception-64s.h
@@ -236,11 +236,10 @@
  * PPR save/restore macros used in exceptions_64s.S  
  * Used for P7 or later processors
  */
-#define SAVE_PPR(area, ra, rb)						\
+#define SAVE_PPR(area, ra)						\
 BEGIN_FTR_SECTION_NESTED(940)						\
-	ld	ra,PACACURRENT(r13);					\
-	ld	rb,area+EX_PPR(r13);	/* Read PPR from paca */	\
-	std	rb,TASKTHREADPPR(ra);					\
+	ld	ra,area+EX_PPR(r13);	/* Read PPR from paca */	\
+	std	ra,_PPR(r1);						\
 END_FTR_SECTION_NESTED(CPU_FTR_HAS_PPR,CPU_FTR_HAS_PPR,940)
 
 #define RESTORE_PPR_PACA(area, ra)					\
@@ -508,7 +507,7 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
 3:	EXCEPTION_PROLOG_COMMON_1();					   \
 	beq	4f;			/* if from kernel mode		*/ \
 	ACCOUNT_CPU_USER_ENTRY(r13, r9, r10);				   \
-	SAVE_PPR(area, r9, r10);					   \
+	SAVE_PPR(area, r9);						   \
 4:	EXCEPTION_PROLOG_COMMON_2(area)					   \
 	EXCEPTION_PROLOG_COMMON_3(n)					   \
 	ACCOUNT_STOLEN_TIME
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 52fadded5c1e..3fefb8a65b17 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -32,9 +32,9 @@
 /* Default SMT priority is set to 3. Use 11- 13bits to save priority. */
 #define PPR_PRIORITY 3
 #ifdef __ASSEMBLY__
-#define INIT_PPR (PPR_PRIORITY << 50)
+#define DEFAULT_PPR (PPR_PRIORITY << 50)
 #else
-#define INIT_PPR ((u64)PPR_PRIORITY << 50)
+#define DEFAULT_PPR ((u64)PPR_PRIORITY << 50)
 #endif /* __ASSEMBLY__ */
 #endif /* CONFIG_PPC64 */
 
@@ -341,7 +341,6 @@ struct thread_struct {
 	 * onwards.
 	 */
 	int		dscr_inherit;
-	unsigned long	ppr;	/* used to save/restore SMT priority */
 	unsigned long	tidr;
 #endif
 #ifdef CONFIG_PPC_BOOK3S_64
@@ -389,7 +388,6 @@ struct thread_struct {
 	.regs = (struct pt_regs *)INIT_SP - 1, /* XXX bogus, I think */ \
 	.addr_limit = KERNEL_DS, \
 	.fpexc_mode = 0, \
-	.ppr = INIT_PPR, \
 	.fscr = FSCR_TAR | FSCR_EBB \
 }
 #endif
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 3dd15024db93..2ba2a1e52291 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -51,6 +51,10 @@ struct pt_regs
 			unsigned long result;
 		};
 	};
+
+#ifdef CONFIG_PPC64
+	unsigned long ppr;
+#endif
 };
 #endif
 
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 2eb4923f8468..92156c61d21c 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -89,7 +89,6 @@ int main(void)
 #ifdef CONFIG_PPC64
 	DEFINE(SIGSEGV, SIGSEGV);
 	DEFINE(NMI_MASK, NMI_MASK);
-	OFFSET(TASKTHREADPPR, task_struct, thread.ppr);
 #else
 	OFFSET(THREAD_INFO, task_struct, stack);
 	DEFINE(THREAD_INFO_GAP, _ALIGN_UP(sizeof(struct thread_info), 16));
@@ -323,6 +322,7 @@ int main(void)
 	STACK_PT_REGS_OFFSET(_ESR, dsisr);
 #else /* CONFIG_PPC64 */
 	STACK_PT_REGS_OFFSET(SOFTE, softe);
+	STACK_PT_REGS_OFFSET(_PPR, ppr);
 #endif /* CONFIG_PPC64 */
 
 #if defined(CONFIG_PPC32)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 7db00ee6be48..7b1693adff2a 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -386,10 +386,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 
 4:	/* Anything else left to do? */
 BEGIN_FTR_SECTION
-	lis	r3,INIT_PPR@highest	/* Set thread.ppr = 3 */
-	ld	r10,PACACURRENT(r13)
+	lis	r3,DEFAULT_PPR@highest	/* Set default PPR */
 	sldi	r3,r3,32	/* bits 11-13 are used for ppr */
-	std	r3,TASKTHREADPPR(r10)
+	std	r3,_PPR(r1)
 END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 
 	andi.	r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP)
@@ -942,12 +941,6 @@ fast_exception_return:
 	andi.	r0,r3,MSR_RI
 	beq-	.Lunrecov_restore
 
-	/* Load PPR from thread struct before we clear MSR:RI */
-BEGIN_FTR_SECTION
-	ld	r2,PACACURRENT(r13)
-	ld	r2,TASKTHREADPPR(r2)
-END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
-
 	/*
 	 * Clear RI before restoring r13.  If we are returning to
 	 * userspace and we take an exception after restoring r13,
@@ -968,7 +961,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	andi.	r0,r3,MSR_PR
 	beq	1f
 BEGIN_FTR_SECTION
-	mtspr	SPRN_PPR,r2	/* Restore PPR */
+	/* Restore PPR */
+	ld	r2,_PPR(r1)
+	mtspr	SPRN_PPR,r2
 END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	ACCOUNT_CPU_USER_EXIT(r13, r2, r4)
 	REST_GPR(13, r1)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 0ed8d0968515..f9d1cca28cce 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1710,7 +1710,7 @@ int copy_thread(unsigned long clone_flags, unsigned long usp,
 		p->thread.dscr = mfspr(SPRN_DSCR);
 	}
 	if (cpu_has_feature(CPU_FTR_HAS_PPR))
-		p->thread.ppr = INIT_PPR;
+		childregs->ppr = DEFAULT_PPR;
 
 	p->thread.tidr = 0;
 #endif
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index c7d0d0c1e34d..afb819f4ca68 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1609,7 +1609,7 @@ static int ppr_get(struct task_struct *target,
 		      void *kbuf, void __user *ubuf)
 {
 	return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
-				   &target->thread.ppr, 0, sizeof(u64));
+				   &target->thread.regs->ppr, 0, sizeof(u64));
 }
 
 static int ppr_set(struct task_struct *target,
@@ -1618,7 +1618,7 @@ static int ppr_set(struct task_struct *target,
 		      const void *kbuf, const void __user *ubuf)
 {
 	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
-				  &target->thread.ppr, 0, sizeof(u64));
+				  &target->thread.regs->ppr, 0, sizeof(u64));
 }
 
 static int dscr_get(struct task_struct *target,
-- 
2.17.1

Re: [PATCH 1/3] powerpc: Split user/kernel definitions of struct pt_regs

[Nicholas Piggin]

On Sat, 13 Oct 2018 21:56:44 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:

> We use a shared definition for struct pt_regs in uapi/asm/ptrace.h.
> That means the layout of the structure is ABI, ie. we can't change it.
> 
> That would be fine if it was only used to describe the user-visible
> register state of a process, but it's also the struct we use in the
> kernel to describe the registers saved in an interrupt frame.
> 
> We'd like more flexibility in the content (and possibly layout) of the
> kernel version of the struct, but currently that's not possible.
> 
> So split the definition into a user-visible definition which remains
> unchanged, and a kernel internal one.
> 
> At the moment they're still identical, and we check that at build
> time. That's because we have code (in ptrace etc.) that assumes that
> they are the same. We will fix that code in future patches, and then
> we can break the strict symmetry between the two structs.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Yeah this looks much better than my int_frame thing, thanks.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks,
Nick

Re: [PATCH 1/3] powerpc: Split user/kernel definitions of struct pt_regs

[Madhavan Srinivasan]

On Saturday 13 October 2018 04:26 PM, Michael Ellerman wrote:
> We use a shared definition for struct pt_regs in uapi/asm/ptrace.h.
> That means the layout of the structure is ABI, ie. we can't change it.
>
> That would be fine if it was only used to describe the user-visible
> register state of a process, but it's also the struct we use in the
> kernel to describe the registers saved in an interrupt frame.
>
> We'd like more flexibility in the content (and possibly layout) of the
> kernel version of the struct, but currently that's not possible.
>
> So split the definition into a user-visible definition which remains
> unchanged, and a kernel internal one.
>
> At the moment they're still identical, and we check that at build
> time. That's because we have code (in ptrace etc.) that assumes that
> they are the same. We will fix that code in future patches, and then
> we can break the strict symmetry between the two structs.

Nice and awesome. But just trying to understand. What will
*regs will point to in the "struct sigcontext".


>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   arch/powerpc/include/asm/ptrace.h      | 27 ++++++++++++++++++
>   arch/powerpc/include/uapi/asm/ptrace.h |  7 ++++-
>   arch/powerpc/kernel/ptrace.c           | 39 ++++++++++++++++++++++++++
>   3 files changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index 447cbd1bee99..3dd15024db93 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -26,6 +26,33 @@
>   #include <uapi/asm/ptrace.h>
>   #include <asm/asm-const.h>
>
> +#ifndef __ASSEMBLY__
> +struct pt_regs
> +{
> +	union {
> +		struct user_pt_regs user_regs;
> +		struct {
> +			unsigned long gpr[32];
> +			unsigned long nip;
> +			unsigned long msr;
> +			unsigned long orig_gpr3;
> +			unsigned long ctr;
> +			unsigned long link;
> +			unsigned long xer;
> +			unsigned long ccr;
> +#ifdef CONFIG_PPC64
> +			unsigned long softe;
> +#else
> +			unsigned long mq;
> +#endif
> +			unsigned long trap;
> +			unsigned long dar;
> +			unsigned long dsisr;
> +			unsigned long result;
> +		};
> +	};
> +};
> +#endif
>
>   #ifdef __powerpc64__
>
> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> index 55c7a131d2ab..f5f1ccc740fc 100644
> --- a/arch/powerpc/include/uapi/asm/ptrace.h
> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> @@ -29,7 +29,12 @@
>
>   #ifndef __ASSEMBLY__
>
> -struct pt_regs {
> +#ifdef __KERNEL__
> +struct user_pt_regs
> +#else
> +struct pt_regs
> +#endif
> +{
>   	unsigned long gpr[32];
>   	unsigned long nip;
>   	unsigned long msr;
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 4e372f54088f..939d7f81bbbe 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -3335,3 +3335,42 @@ void do_syscall_trace_leave(struct pt_regs *regs)
>
>   	user_enter();
>   }
> +
> +void __init pt_regs_check(void)
> +{
> +	BUILD_BUG_ON(offsetof(struct pt_regs, gpr) !=
> +		     offsetof(struct user_pt_regs, gpr));
> +	BUILD_BUG_ON(offsetof(struct pt_regs, nip) !=
> +		     offsetof(struct user_pt_regs, nip));
> +	BUILD_BUG_ON(offsetof(struct pt_regs, msr) !=
> +		     offsetof(struct user_pt_regs, msr));
> +	BUILD_BUG_ON(offsetof(struct pt_regs, msr) !=
> +		     offsetof(struct user_pt_regs, msr));
> +	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> +		     offsetof(struct user_pt_regs, orig_gpr3));
> +	BUILD_BUG_ON(offsetof(struct pt_regs, ctr) !=
> +		     offsetof(struct user_pt_regs, ctr));
> +	BUILD_BUG_ON(offsetof(struct pt_regs, link) !=
> +		     offsetof(struct user_pt_regs, link));
> +	BUILD_BUG_ON(offsetof(struct pt_regs, xer) !=
> +		     offsetof(struct user_pt_regs, xer));
> +	BUILD_BUG_ON(offsetof(struct pt_regs, ccr) !=
> +		     offsetof(struct user_pt_regs, ccr));
> +#ifdef __powerpc64__
> +	BUILD_BUG_ON(offsetof(struct pt_regs, softe) !=
> +		     offsetof(struct user_pt_regs, softe));
> +#else
> +	BUILD_BUG_ON(offsetof(struct pt_regs, mq) !=
> +		     offsetof(struct user_pt_regs, mq));
> +#endif
> +	BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
> +		     offsetof(struct user_pt_regs, trap));
> +	BUILD_BUG_ON(offsetof(struct pt_regs, dar) !=
> +		     offsetof(struct user_pt_regs, dar));
> +	BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> +		     offsetof(struct user_pt_regs, dsisr));
> +	BUILD_BUG_ON(offsetof(struct pt_regs, result) !=
> +		     offsetof(struct user_pt_regs, result));
> +
> +	BUILD_BUG_ON(sizeof(struct user_pt_regs) > sizeof(struct pt_regs));
> +}

Re: [1/3] powerpc: Split user/kernel definitions of struct pt_regs

[Michael Ellerman]

On Sat, 2018-10-13 at 10:56:44 UTC, Michael Ellerman wrote:
> We use a shared definition for struct pt_regs in uapi/asm/ptrace.h.
> That means the layout of the structure is ABI, ie. we can't change it.
> 
> That would be fine if it was only used to describe the user-visible
> register state of a process, but it's also the struct we use in the
> kernel to describe the registers saved in an interrupt frame.
> 
> We'd like more flexibility in the content (and possibly layout) of the
> kernel version of the struct, but currently that's not possible.
> 
> So split the definition into a user-visible definition which remains
> unchanged, and a kernel internal one.
> 
> At the moment they're still identical, and we check that at build
> time. That's because we have code (in ptrace etc.) that assumes that
> they are the same. We will fix that code in future patches, and then
> we can break the strict symmetry between the two structs.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Series applied to powerpc next.

https://git.kernel.org/powerpc/c/002af9391bfbe84f8e491bb10bd9c6

cheers

Re: [PATCH 1/3] powerpc: Split user/kernel definitions of struct pt_regs

[Michael Ellerman]

Madhavan Srinivasan <maddy@linux.vnet.ibm.com> writes:

> On Saturday 13 October 2018 04:26 PM, Michael Ellerman wrote:
>> We use a shared definition for struct pt_regs in uapi/asm/ptrace.h.
>> That means the layout of the structure is ABI, ie. we can't change it.
>>
>> That would be fine if it was only used to describe the user-visible
>> register state of a process, but it's also the struct we use in the
>> kernel to describe the registers saved in an interrupt frame.
>>
>> We'd like more flexibility in the content (and possibly layout) of the
>> kernel version of the struct, but currently that's not possible.
>>
>> So split the definition into a user-visible definition which remains
>> unchanged, and a kernel internal one.
>>
>> At the moment they're still identical, and we check that at build
>> time. That's because we have code (in ptrace etc.) that assumes that
>> they are the same. We will fix that code in future patches, and then
>> we can break the strict symmetry between the two structs.
>
> Nice and awesome. But just trying to understand. What will
> *regs will point to in the "struct sigcontext".

Yeah that's a bit fishy.

It should always point to a user_pt_regs.

So in the kernel we want:

  struct sigcontext {
  	...
  	struct user_pt_regs	__user *regs;

And in userspace we want:

  struct sigcontext {
  	...
  	struct pt_regs	__user *regs;


I think it's not actually broken at the moment, because it's just a
pointer, and we don't do anything based on the sizeof() the type.

But still we should fix it.

I guess I'll do this:

diff --git a/arch/powerpc/include/uapi/asm/sigcontext.h b/arch/powerpc/include/uapi/asm/sigcontext.h
index 2fbe485acdb4..630aeda56d59 100644
--- a/arch/powerpc/include/uapi/asm/sigcontext.h
+++ b/arch/powerpc/include/uapi/asm/sigcontext.h
@@ -22,7 +22,11 @@ struct sigcontext {
 #endif
 	unsigned long	handler;
 	unsigned long	oldmask;
-	struct pt_regs	__user *regs;
+#ifdef __KERNEL__
+	struct user_pt_regs __user *regs;
+#else
+	struct pt_regs	*regs;
+#endif
 #ifdef __powerpc64__
 	elf_gregset_t	gp_regs;
 	elf_fpregset_t	fp_regs;


Thanks for the review.

cheers

Re: [PATCH 1/3] powerpc: Split user/kernel definitions of struct pt_regs

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:
> On Sat, 13 Oct 2018 21:56:44 +1100
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
>> We use a shared definition for struct pt_regs in uapi/asm/ptrace.h.
>> That means the layout of the structure is ABI, ie. we can't change it.
>> 
>> That would be fine if it was only used to describe the user-visible
>> register state of a process, but it's also the struct we use in the
>> kernel to describe the registers saved in an interrupt frame.
>> 
>> We'd like more flexibility in the content (and possibly layout) of the
>> kernel version of the struct, but currently that's not possible.
>> 
>> So split the definition into a user-visible definition which remains
>> unchanged, and a kernel internal one.
>> 
>> At the moment they're still identical, and we check that at build
>> time. That's because we have code (in ptrace etc.) that assumes that
>> they are the same. We will fix that code in future patches, and then
>> we can break the strict symmetry between the two structs.
>> 
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>
> Yeah this looks much better than my int_frame thing, thanks.

Thanks. More bug prone to :)

But hopefully it will pay off in the long run.

cheers

Re: [PATCH 1/3] powerpc: Split user/kernel definitions of struct pt_regs

[Madhavan Srinivasan]

On Monday 15 October 2018 04:38 PM, Michael Ellerman wrote:
> Madhavan Srinivasan <maddy@linux.vnet.ibm.com> writes:
>
>> On Saturday 13 October 2018 04:26 PM, Michael Ellerman wrote:
>>> We use a shared definition for struct pt_regs in uapi/asm/ptrace.h.
>>> That means the layout of the structure is ABI, ie. we can't change it.
>>>
>>> That would be fine if it was only used to describe the user-visible
>>> register state of a process, but it's also the struct we use in the
>>> kernel to describe the registers saved in an interrupt frame.
>>>
>>> We'd like more flexibility in the content (and possibly layout) of the
>>> kernel version of the struct, but currently that's not possible.
>>>
>>> So split the definition into a user-visible definition which remains
>>> unchanged, and a kernel internal one.
>>>
>>> At the moment they're still identical, and we check that at build
>>> time. That's because we have code (in ptrace etc.) that assumes that
>>> they are the same. We will fix that code in future patches, and then
>>> we can break the strict symmetry between the two structs.
>> Nice and awesome. But just trying to understand. What will
>> *regs will point to in the "struct sigcontext".
> Yeah that's a bit fishy.
>
> It should always point to a user_pt_regs.
>
> So in the kernel we want:
>
>    struct sigcontext {
>    	...
>    	struct user_pt_regs	__user *regs;
>
> And in userspace we want:
>
>    struct sigcontext {
>    	...
>    	struct pt_regs	__user *regs;
>
>
> I think it's not actually broken at the moment, because it's just a
> pointer, and we don't do anything based on the sizeof() the type.

yes. This clarifies. But still perf/perf_regs.c needs changes.
Because perf support dumping user_space regs and interrupt regs.
Once again, we dont use any sizeof(), but need to handle the
user_pt_regs changes.

I will have a look at that in the morning.

Thanks for clarification.
Maddy

>
> But still we should fix it.
>
> I guess I'll do this:
>
> diff --git a/arch/powerpc/include/uapi/asm/sigcontext.h b/arch/powerpc/include/uapi/asm/sigcontext.h
> index 2fbe485acdb4..630aeda56d59 100644
> --- a/arch/powerpc/include/uapi/asm/sigcontext.h
> +++ b/arch/powerpc/include/uapi/asm/sigcontext.h
> @@ -22,7 +22,11 @@ struct sigcontext {
>   #endif
>   	unsigned long	handler;
>   	unsigned long	oldmask;
> -	struct pt_regs	__user *regs;
> +#ifdef __KERNEL__
> +	struct user_pt_regs __user *regs;
> +#else
> +	struct pt_regs	*regs;
> +#endif
>   #ifdef __powerpc64__
>   	elf_gregset_t	gp_regs;
>   	elf_fpregset_t	fp_regs;
>
>
> Thanks for the review.
>
> cheers
>

Re: [PATCH 1/3] powerpc: Split user/kernel definitions of struct pt_regs

[Michael Ellerman]

Madhavan Srinivasan <maddy@linux.vnet.ibm.com> writes:
> On Monday 15 October 2018 04:38 PM, Michael Ellerman wrote:
>> Madhavan Srinivasan <maddy@linux.vnet.ibm.com> writes:
>>
>>> On Saturday 13 October 2018 04:26 PM, Michael Ellerman wrote:
...
>>>> At the moment they're still identical, and we check that at build
>>>> time. That's because we have code (in ptrace etc.) that assumes that
>>>> they are the same. We will fix that code in future patches, and then
>>>> we can break the strict symmetry between the two structs.
>>> Nice and awesome. But just trying to understand. What will
>>> *regs will point to in the "struct sigcontext".
>>
>> It should always point to a user_pt_regs.
...
>>
>> I think it's not actually broken at the moment, because it's just a
>> pointer, and we don't do anything based on the sizeof() the type.
>
> yes. This clarifies. But still perf/perf_regs.c needs changes.
> Because perf support dumping user_space regs and interrupt regs.
> Once again, we dont use any sizeof(), but need to handle the
> user_pt_regs changes.
>
> I will have a look at that in the morning.

I did look at that and convinced myself that it was OK, but maybe I'm
wrong :D

My reasoning was that the regs we're using there are always the
in-kernel regs for the process at the point it took the PMU interrupt.
And the regs values aren't exported directly as a struct but rather via
regs_get_register().

But we may still want to change it to make things clearer.

cheers