Re: [PATCH] powerpc: Fix check for copy/paste instructions in alignment handler

Re: [PATCH] powerpc: Fix check for copy/paste instructions in alignment handler

[Nicholas Piggin]

On Wed, 25 Oct 2017 18:16:53 +1100
Paul Mackerras <paulus@ozlabs.org> wrote:

> Commit 07d2a628bc00 ("powerpc/64s: Avoid cpabort in context switch
> when possible", 2017-06-09) changed the definition of PPC_INST_COPY
> and in so doing inadvertently broke the check for copy/paste
> instructions in the alignment fault handler.  The check currently
> matches no instructions.
> 
> This fixes it by ANDing both sides of the comparison with the mask.

Thanks for fixing it. Any reason not to change the mask to 0xfc2006fe
to include the 'last' bit that is now mandatory?

Thanks,
Nick

> 
> Fixes: 07d2a628bc00 ("powerpc/64s: Avoid cpabort in context switch
> when possible")
> Reported-by: Markus Trippelsdorf <markus@trippelsdorf.de>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> ---
> Should go to 4.13 stable as well, and will probably apply there
> with fuzz.
> 
>  arch/powerpc/kernel/align.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
> index 43ef251..3e6c074 100644
> --- a/arch/powerpc/kernel/align.c
> +++ b/arch/powerpc/kernel/align.c
> @@ -332,7 +332,7 @@ int fix_alignment(struct pt_regs *regs)
>  	 * when pasting to a co-processor. Furthermore, paste_last is the
>  	 * synchronisation point for preceding copy/paste sequences.
>  	 */
> -	if ((instr & 0xfc0006fe) == PPC_INST_COPY)
> +	if ((instr & 0xfc0006fe) == (PPC_INST_COPY & 0xfc0006fe))
>  		return -EIO;
>  
>  	r = analyse_instr(&op, regs, instr);

Re: [PATCH] powerpc: Fix check for copy/paste instructions in alignment handler

[Paul Mackerras]

On Thu, Oct 26, 2017 at 01:22:37AM +1000, Nicholas Piggin wrote:
> On Wed, 25 Oct 2017 18:16:53 +1100
> Paul Mackerras <paulus@ozlabs.org> wrote:
> 
> > Commit 07d2a628bc00 ("powerpc/64s: Avoid cpabort in context switch
> > when possible", 2017-06-09) changed the definition of PPC_INST_COPY
> > and in so doing inadvertently broke the check for copy/paste
> > instructions in the alignment fault handler.  The check currently
> > matches no instructions.
> > 
> > This fixes it by ANDing both sides of the comparison with the mask.
> 
> Thanks for fixing it. Any reason not to change the mask to 0xfc2006fe
> to include the 'last' bit that is now mandatory?

What does real hardware do if you execute the instruction without that
bit set?  Does it take the alignment interrupt, or does it take an
illegal instruction interrupt (i.e. HEAI)?  We need to catch all the
variants that take an alignment interrupt.  If P9 hardware takes a
HEAI when that bit isn't set, then I'm fine with changing the mask.

Paul.

Re: [PATCH] powerpc: Fix check for copy/paste instructions in alignment handler

[Nicholas Piggin]

On Thu, 26 Oct 2017 09:41:32 +1100
Paul Mackerras <paulus@ozlabs.org> wrote:

> On Thu, Oct 26, 2017 at 01:22:37AM +1000, Nicholas Piggin wrote:
> > On Wed, 25 Oct 2017 18:16:53 +1100
> > Paul Mackerras <paulus@ozlabs.org> wrote:
> >   
> > > Commit 07d2a628bc00 ("powerpc/64s: Avoid cpabort in context switch
> > > when possible", 2017-06-09) changed the definition of PPC_INST_COPY
> > > and in so doing inadvertently broke the check for copy/paste
> > > instructions in the alignment fault handler.  The check currently
> > > matches no instructions.
> > > 
> > > This fixes it by ANDing both sides of the comparison with the mask.  
> > 
> > Thanks for fixing it. Any reason not to change the mask to 0xfc2006fe
> > to include the 'last' bit that is now mandatory?  
> 
> What does real hardware do if you execute the instruction without that
> bit set?  Does it take the alignment interrupt, or does it take an
> illegal instruction interrupt (i.e. HEAI)?  We need to catch all the
> variants that take an alignment interrupt.  If P9 hardware takes a
> HEAI when that bit isn't set, then I'm fine with changing the mask.

Just tested. Copy without the bit results in an illegal instruction.
Even the valid paste variant causes a sigill because it's set to use
HEA to avoid the thread reconfig problem.

Thanks,
Nick

[PATCH] powerpc: Fix check for copy/paste instructions in alignment handler

[Paul Mackerras]

Commit 07d2a628bc00 ("powerpc/64s: Avoid cpabort in context switch
when possible", 2017-06-09) changed the definition of PPC_INST_COPY
and in so doing inadvertently broke the check for copy/paste
instructions in the alignment fault handler.  The check currently
matches no instructions.

This fixes it by ANDing both sides of the comparison with the mask.

Fixes: 07d2a628bc00 ("powerpc/64s: Avoid cpabort in context switch
when possible")
Reported-by: Markus Trippelsdorf <markus@trippelsdorf.de>
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
---
Should go to 4.13 stable as well, and will probably apply there
with fuzz.

 arch/powerpc/kernel/align.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index 43ef251..3e6c074 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -332,7 +332,7 @@ int fix_alignment(struct pt_regs *regs)
 	 * when pasting to a co-processor. Furthermore, paste_last is the
 	 * synchronisation point for preceding copy/paste sequences.
 	 */
-	if ((instr & 0xfc0006fe) == PPC_INST_COPY)
+	if ((instr & 0xfc0006fe) == (PPC_INST_COPY & 0xfc0006fe))
 		return -EIO;
 
 	r = analyse_instr(&op, regs, instr);
-- 
2.7.4