| Message ID | 39d40efd808c6346b97e5bfe4be47546919cb91b.1498732172.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 29 Jun 2017 16:11:10 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > We can't take traps with relocation off, so blacklist enter_rtas() and > rtas_return_loc(). However, instead of blacklisting all of enter_rtas(), > introduce a new symbol __enter_rtas from where on we can't take a trap > and blacklist that. > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > --- > arch/powerpc/kernel/entry_64.S | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S > index 0c27084800b6..16f4c4a1a294 100644 > --- a/arch/powerpc/kernel/entry_64.S > +++ b/arch/powerpc/kernel/entry_64.S > @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas) > sync /* disable interrupts so SRR0/1 */ > mtmsrd r0 /* don't get trashed */ > > +__enter_rtas: Hmm, am I missing something, or is there a reason to put these labels after the mtmsr? Even if kprobes does the right thing, I think it's easier to read the code if you cover the mtmsr as well. Thanks, Nick
On 2017/06/29 09:01PM, Nicholas Piggin wrote: > On Thu, 29 Jun 2017 16:11:10 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > > We can't take traps with relocation off, so blacklist enter_rtas() and > > rtas_return_loc(). However, instead of blacklisting all of enter_rtas(), > > introduce a new symbol __enter_rtas from where on we can't take a trap > > and blacklist that. > > > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > > --- > > arch/powerpc/kernel/entry_64.S | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S > > index 0c27084800b6..16f4c4a1a294 100644 > > --- a/arch/powerpc/kernel/entry_64.S > > +++ b/arch/powerpc/kernel/entry_64.S > > @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas) > > sync /* disable interrupts so SRR0/1 */ > > mtmsrd r0 /* don't get trashed */ > > > > +__enter_rtas: > > Hmm, am I missing something, or is there a reason to put these labels > after the mtmsr? Even if kprobes does the right thing, I think it's > easier to read the code if you cover the mtmsr as well. I thought you asked for this, per your previous review comment: https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg119667.html Or, did I get that wrong? Thanks, Naveen
On Thu, 29 Jun 2017 17:24:14 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > On 2017/06/29 09:01PM, Nicholas Piggin wrote: > > On Thu, 29 Jun 2017 16:11:10 +0530 > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > > > > We can't take traps with relocation off, so blacklist enter_rtas() and > > > rtas_return_loc(). However, instead of blacklisting all of enter_rtas(), > > > introduce a new symbol __enter_rtas from where on we can't take a trap > > > and blacklist that. > > > > > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > > > --- > > > arch/powerpc/kernel/entry_64.S | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S > > > index 0c27084800b6..16f4c4a1a294 100644 > > > --- a/arch/powerpc/kernel/entry_64.S > > > +++ b/arch/powerpc/kernel/entry_64.S > > > @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas) > > > sync /* disable interrupts so SRR0/1 */ > > > mtmsrd r0 /* don't get trashed */ > > > > > > +__enter_rtas: > > > > Hmm, am I missing something, or is there a reason to put these labels > > after the mtmsr? Even if kprobes does the right thing, I think it's > > easier to read the code if you cover the mtmsr as well. > > I thought you asked for this, per your previous review comment: > https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg119667.html > > Or, did I get that wrong? No you're right, I'm contradicting myself. Let me start again. I think we'd like to put the label before the mtmsrd if possible. So in that case, should we adjust the system call code instead (then you wouldn't have to add a comment for it). And then you could move this label back above the mtmsrd. Sorry for the confusion. Thanks, Nick
On 2017/06/29 10:13PM, Nicholas Piggin wrote: > On Thu, 29 Jun 2017 17:24:14 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > > On 2017/06/29 09:01PM, Nicholas Piggin wrote: > > > On Thu, 29 Jun 2017 16:11:10 +0530 > > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > > > > > > We can't take traps with relocation off, so blacklist enter_rtas() and > > > > rtas_return_loc(). However, instead of blacklisting all of enter_rtas(), > > > > introduce a new symbol __enter_rtas from where on we can't take a trap > > > > and blacklist that. > > > > > > > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > > > > --- > > > > arch/powerpc/kernel/entry_64.S | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S > > > > index 0c27084800b6..16f4c4a1a294 100644 > > > > --- a/arch/powerpc/kernel/entry_64.S > > > > +++ b/arch/powerpc/kernel/entry_64.S > > > > @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas) > > > > sync /* disable interrupts so SRR0/1 */ > > > > mtmsrd r0 /* don't get trashed */ > > > > > > > > +__enter_rtas: > > > > > > Hmm, am I missing something, or is there a reason to put these labels > > > after the mtmsr? Even if kprobes does the right thing, I think it's > > > easier to read the code if you cover the mtmsr as well. > > > > I thought you asked for this, per your previous review comment: > > https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg119667.html > > > > Or, did I get that wrong? > > No you're right, I'm contradicting myself. Let me start again. > > I think we'd like to put the label before the mtmsrd if possible. So > in that case, should we adjust the system call code instead (then you > wouldn't have to add a comment for it). > > And then you could move this label back above the mtmsrd. Sorry for > the confusion. Sure - I now get why you were insisting on a comment with the system_call_exit symbol. v5 enroute... Thanks, Naveen
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S index 0c27084800b6..16f4c4a1a294 100644 --- a/arch/powerpc/kernel/entry_64.S +++ b/arch/powerpc/kernel/entry_64.S @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas) sync /* disable interrupts so SRR0/1 */ mtmsrd r0 /* don't get trashed */ +__enter_rtas: LOAD_REG_ADDR(r4, rtas) ld r5,RTASENTRY(r4) /* get the rtas->entry value */ ld r4,RTASBASE(r4) /* get the rtas->base value */ @@ -1115,6 +1116,8 @@ rtas_return_loc: mtspr SPRN_SRR1,r4 rfid b . /* prevent speculative execution */ +_ASM_NOKPROBE_SYMBOL(__enter_rtas) +_ASM_NOKPROBE_SYMBOL(rtas_return_loc) .align 3 1: .llong rtas_restore_regs
We can't take traps with relocation off, so blacklist enter_rtas() and rtas_return_loc(). However, instead of blacklisting all of enter_rtas(), introduce a new symbol __enter_rtas from where on we can't take a trap and blacklist that. Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- arch/powerpc/kernel/entry_64.S | 3 +++ 1 file changed, 3 insertions(+)