* [PATCH] ppc64: allow ptrace to set TM bits
@ 2016-07-29 9:51 Laurent Dufour
2016-08-02 5:43 ` Simon Guo
0 siblings, 1 reply; 5+ messages in thread
From: Laurent Dufour @ 2016-07-29 9:51 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mpe, Simon Guo, Anshuman Khandual
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(+)
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;
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] ppc64: allow ptrace to set TM bits
2016-07-29 9:51 [PATCH] ppc64: allow ptrace to set TM bits Laurent Dufour
@ 2016-08-02 5:43 ` Simon Guo
2016-08-17 14:40 ` Laurent Dufour
2016-08-22 1:01 ` Cyril Bur
0 siblings, 2 replies; 5+ messages in thread
From: Simon Guo @ 2016-08-02 5:43 UTC (permalink / raw)
To: Laurent Dufour; +Cc: linuxppc-dev, mpe, Anshuman Khandual
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ppc64: allow ptrace to set TM bits
2016-08-02 5:43 ` Simon Guo
@ 2016-08-17 14:40 ` Laurent Dufour
2016-08-22 1:01 ` Cyril Bur
1 sibling, 0 replies; 5+ messages in thread
From: Laurent Dufour @ 2016-08-17 14:40 UTC (permalink / raw)
To: Simon Guo; +Cc: linuxppc-dev, mpe, Anshuman Khandual
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ppc64: allow ptrace to set TM bits
2016-08-02 5:43 ` Simon Guo
2016-08-17 14:40 ` Laurent Dufour
@ 2016-08-22 1:01 ` Cyril Bur
2016-08-22 9:53 ` Laurent Dufour
1 sibling, 1 reply; 5+ messages in thread
From: Cyril Bur @ 2016-08-22 1:01 UTC (permalink / raw)
To: Simon Guo, Laurent Dufour; +Cc: linuxppc-dev, Anshuman Khandual
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ppc64: allow ptrace to set TM bits
2016-08-22 1:01 ` Cyril Bur
@ 2016-08-22 9:53 ` Laurent Dufour
0 siblings, 0 replies; 5+ messages in thread
From: Laurent Dufour @ 2016-08-22 9:53 UTC (permalink / raw)
To: Cyril Bur, Simon Guo; +Cc: linuxppc-dev, Anshuman Khandual
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-08-22 9:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-29 9:51 [PATCH] ppc64: allow ptrace to set TM bits Laurent Dufour
2016-08-02 5:43 ` Simon Guo
2016-08-17 14:40 ` Laurent Dufour
2016-08-22 1:01 ` Cyril Bur
2016-08-22 9:53 ` Laurent Dufour
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.