| Message ID | 1469785882-9892-1-git-send-email-ldufour@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
Hi Laurent, On Fri, Jul 29, 2016 at 11:51:22AM +0200, Laurent Dufour wrote: > static int set_user_msr(struct task_struct *task, unsigned long msr) > { > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > + if (!(task->thread.regs->msr & MSR_TM)) { > + /* If TM is not available, discard TM bits changes */ > + msr &= ~(MSR_TM | MSR_TS_MASK); > + } > +#endif I am not sure whether following is an issue: Per PowerISA, any exception/interrupt will disable MSR[TM] bit automatically and mark MSR_TS to be suspended when it is transactional. It is possible that MSR[TM] = 0 and MSR[MSR_TS] != 0 (suspended). Will set_user_msr() be able to escape from the above? For example, one user space application encountered page fault during transaction, its task->thread.regs->msr & MSR_TM == 0 and MSR[MSR_TS] == suspended. Then it is being traced and set_user_msr() is invoked on it. I think it will be incorrect to clear its MSR_TS_MASK bits..... Please correct me if I am wrong. Thanks, - Simon
On 02/08/2016 07:43, Simon Guo wrote: > Hi Laurent, > On Fri, Jul 29, 2016 at 11:51:22AM +0200, Laurent Dufour wrote: >> static int set_user_msr(struct task_struct *task, unsigned long msr) >> { >> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM >> + if (!(task->thread.regs->msr & MSR_TM)) { >> + /* If TM is not available, discard TM bits changes */ >> + msr &= ~(MSR_TM | MSR_TS_MASK); >> + } >> +#endif > > I am not sure whether following is an issue: > Per PowerISA, any exception/interrupt will disable MSR[TM] bit > automatically and mark MSR_TS to be suspended when it is > transactional. It is possible that MSR[TM] = 0 and MSR[MSR_TS] != 0 > (suspended). > > Will set_user_msr() be able to escape from the above? > For example, one user space application encountered > page fault during transaction, its task->thread.regs->msr & MSR_TM == 0 > and MSR[MSR_TS] == suspended. Then it is being traced and > set_user_msr() is invoked on it. I think it will be incorrect to > clear its MSR_TS_MASK bits..... You raised a good point, but I'm not sure this case is valid. The interrupt case you described is happening in the kernel, not in user space, and set_user_msr() is dealing with the user space register's state, not the kernel's one. So it can't apply and I can't see how in user space MSR[TM]=0 and MSR[TS]=1 could happen. Am I missing something here ? Thanks, Laurent.
On Tue, 2016-08-02 at 13:43 +0800, Simon Guo wrote: > Hi Laurent, > On Fri, Jul 29, 2016 at 11:51:22AM +0200, Laurent Dufour wrote: > > > > static int set_user_msr(struct task_struct *task, unsigned long > > msr) > > { > > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > + if (!(task->thread.regs->msr & MSR_TM)) { > > + /* If TM is not available, discard TM bits changes > > */ > > + msr &= ~(MSR_TM | MSR_TS_MASK); > > + } > > +#endif > > I am not sure whether following is an issue: > Per PowerISA, any exception/interrupt will disable MSR[TM] bit > automatically and mark MSR_TS to be suspended when it is > transactional. It is possible that MSR[TM] = 0 and MSR[MSR_TS] != 0 > (suspended). > > Will set_user_msr() be able to escape from the above? > For example, one user space application encountered > page fault during transaction, its task->thread.regs->msr & MSR_TM == > 0 > and MSR[MSR_TS] == suspended. Then it is being traced and > set_user_msr() is invoked on it. I think it will be incorrect to > clear its MSR_TS_MASK bits..... > > (suspended).ible that MSRTM] = 0 and MSR[MSR_TS] != 0> (suspended). > Please correct me if I am wrong. I'm not very familiar with ptracing and exactly what can happen but I agree with Simon. Trying to change an MSR with that possible condition stated ("It is possible that MSR[TM] = 0 and MSR[MSR_TS] != 0> (suspended)") to MSR_TS and MSR_TS_MASK bits all 0 will cause a Bad Thing. Cyril > > Thanks, > - Simon > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev
On 22/08/2016 03:01, Cyril Bur wrote: > On Tue, 2016-08-02 at 13:43 +0800, Simon Guo wrote: >> Hi Laurent, >> On Fri, Jul 29, 2016 at 11:51:22AM +0200, Laurent Dufour wrote: >>> >>> static int set_user_msr(struct task_struct *task, unsigned long >>> msr) >>> { >>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM >>> + if (!(task->thread.regs->msr & MSR_TM)) { >>> + /* If TM is not available, discard TM bits changes >>> */ >>> + msr &= ~(MSR_TM | MSR_TS_MASK); >>> + } >>> +#endif >> >> I am not sure whether following is an issue: >> Per PowerISA, any exception/interrupt will disable MSR[TM] bit >> automatically and mark MSR_TS to be suspended when it is >> transactional. It is possible that MSR[TM] = 0 and MSR[MSR_TS] != 0 >> (suspended). >> >> Will set_user_msr() be able to escape from the above? >> For example, one user space application encountered >> page fault during transaction, its task->thread.regs->msr & MSR_TM == >> 0 >> and MSR[MSR_TS] == suspended. Then it is being traced and >> set_user_msr() is invoked on it. I think it will be incorrect to >> clear its MSR_TS_MASK bits..... >> >> (suspended).ible that MSRTM] = 0 and MSR[MSR_TS] != 0> (suspended). > >> Please correct me if I am wrong. > > I'm not very familiar with ptracing and exactly what can happen but I > agree with Simon. Trying to change an MSR with that possible condition > stated ("It is possible that MSR[TM] = 0 and MSR[MSR_TS] != > 0> (suspended)") to MSR_TS and MSR_TS_MASK bits all 0 will cause a Bad > Thing. I don't think this may happen since from the user space point of view, we can't have MSR[TM]=0 & MSR[MSR_TS]=1, because MSR[TM] will be set once the transaction is initiated. Anyway, this patch is no more required since the recent patch http://patchwork.ozlabs.org/patch/661358/ is now dropping the TM state form the signal return path, which is enough fof CRIU to work fine. Cheers, Laurent.
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c index 1d8998bd6321..e2c16eb0cabe 100644 --- a/arch/powerpc/kernel/ptrace.c +++ b/arch/powerpc/kernel/ptrace.c @@ -161,8 +161,12 @@ const char *regs_query_register_name(unsigned int offset) #ifdef CONFIG_PPC_ADV_DEBUG_REGS #define MSR_DEBUGCHANGE 0 #else +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM +#define MSR_DEBUGCHANGE (MSR_TS_MASK | MSR_SE | MSR_BE) +#else #define MSR_DEBUGCHANGE (MSR_SE | MSR_BE) #endif +#endif /* * Max register writeable via put_reg @@ -180,6 +184,12 @@ static unsigned long get_user_msr(struct task_struct *task) static int set_user_msr(struct task_struct *task, unsigned long msr) { +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM + if (!(task->thread.regs->msr & MSR_TM)) { + /* If TM is not available, discard TM bits changes */ + msr &= ~(MSR_TM | MSR_TS_MASK); + } +#endif task->thread.regs->msr &= ~MSR_DEBUGCHANGE; task->thread.regs->msr |= msr & MSR_DEBUGCHANGE; return 0; @@ -193,6 +203,7 @@ static unsigned long get_user_ckpt_msr(struct task_struct *task) static int set_user_ckpt_msr(struct task_struct *task, unsigned long msr) { + msr &= ~MSR_TS_MASK; /* Checkpoint state can't be in transaction */ task->thread.ckpt_regs.msr &= ~MSR_DEBUGCHANGE; task->thread.ckpt_regs.msr |= msr & MSR_DEBUGCHANGE; return 0;
This patch allows the MSR bits relative to the Transactional memory state to be manipulated through the ptrace API. However, in the case the TM available bit is not set in the manipulated MSR, the changes are ignored. When dealing with the checkpointed MSR, we must be sure that the TM state bits will not be set since the checkpointed state can't be a transactional one. This patch is a follow up of the Anshuman's series pushed by Simon Guo recently, titled "Add new powerpc specific ELF core notes" : https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-July/146711.html Cc: Simon Guo <wei.guo.simon@gmail.com> Cc: Anshuman Khandual <khandual@linux.vnet.ibm.com> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com> --- arch/powerpc/kernel/ptrace.c | 11 +++++++++++ 1 file changed, 11 insertions(+)