Re: [PATCH V10 03/28] powerpc, ptrace: Enable in transaction NT_PRFPREG ptrace requests

On Tue, 2016-02-16 at 12:09 +0300, Denis Kirjanov wrote:

On 2/16/16, Anshuman Khandual [off-list ref] wrote:

quoted

This patch enables in transaction NT_PRFPREG ptrace requests.
The function fpr_get which gets the running value of all FPR
registers and the function fpr_set which sets the running
value of of all FPR registers work on the running set of FPR
registers whose location will be different if transaction is
active. This patch makes these functions adapt to situations
when the transaction is active.

Signed-off-by: Anshuman Khandual <redacted>
---
 arch/powerpc/kernel/ptrace.c | 93
++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 89 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 30a03c0..547a979 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c

@@ -358,6 +358,29 @@ static int gpr_set(struct task_struct *target, const

struct user_regset *regset,
 	return ret;
 }

+/*
+ * When the transaction is active, 'transact_fp' holds the current running
+ * value of all FPR registers and 'fp_state' holds the last checkpointed
+ * value of all FPR registers for the current transaction. When transaction
+ * is not active 'fp_state' holds the current running state of all the FPR
+ * registers. So this function which returns the current running values of
+ * all the FPR registers, needs to know whether any transaction is active
+ * or not.
+ *
+ * Userspace interface buffer layout:
+ *
+ * struct data {
+ *	u64	fpr[32];
+ *	u64	fpscr;
+ * };
+ *
+ * There are two config options CONFIG_VSX and CONFIG_PPC_TRANSACTIONAL_MEM
+ * which determines the final code in this function. All the combinations
of
+ * these two config options are possible except the one below as
transactional
+ * memory config pulls in CONFIG_VSX automatically.
+ *
+ *	!defined(CONFIG_VSX) && defined(CONFIG_PPC_TRANSACTIONAL_MEM)
+ */
 static int fpr_get(struct task_struct *target, const struct user_regset
*regset,
 		   unsigned int pos, unsigned int count,
 		   void *kbuf, void __user *ubuf)

@@ -368,14 +391,31 @@ static int fpr_get(struct task_struct *target, const

struct user_regset *regset,
 #endif
 	flush_fp_to_thread(target);

-#ifdef CONFIG_VSX
+#if defined(CONFIG_VSX) && defined(CONFIG_PPC_TRANSACTIONAL_MEM)
+	/* copy to local buffer then write that out */
+	if (MSR_TM_ACTIVE(target->thread.regs->msr)) {
+		flush_altivec_to_thread(target);
+		flush_tmregs_to_thread(target);
+		for (i = 0; i < 32 ; i++)

use ELF_NFPREG

quoted

+			buf[i] = target->thread.TS_TRANS_FPR(i);
+		buf[32] = target->thread.transact_fp.fpscr;

#define ELF_NFPREG	33	/* includes fpscr */

So it's not what he wants here. Unless you mean that he should use i < ELF_NFPREG - 1; ?

We already have several loops over the 32 fprs, I don't think hiding the "32"
behind a macro really buys us anything.

cheers