Re: [PATCH 2/3] powerpc sstep: add support for divde[.] and divdeu[.] instructions

[Balamuruhan S]

On Thu, Jan 09, 2020 at 09:23:14AM +1100, Paul Mackerras wrote:
> On Tue, Dec 10, 2019 at 12:49:03PM +0530, Balamuruhan S wrote:
> > This patch adds emulation support for divde, divdeu instructions,
> > * Divide Doubleword Extended (divde[.])
> > * Divide Doubleword Extended Unsigned (divdeu[.])
> > 
> > Signed-off-by: Balamuruhan S 
> > ---
> >  arch/powerpc/lib/sstep.c | 27 ++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> > index c077acb983a1..4b4119729e59 100644
> > --- a/arch/powerpc/lib/sstep.c
> > +++ b/arch/powerpc/lib/sstep.c
> > @@ -1736,7 +1736,32 @@ int analyse_instr(struct instruction_op *op, const 
> > struct pt_regs *regs,
> > op->val = (int) regs->gpr[ra] /
> > (int) regs->gpr[rb];
> > goto arith_done;
> > -
> > +#ifdef __powerpc64__
> > +   case 425:   /* divde[.] */
> > +   if (instr & 1) {
> > +   asm volatile(PPC_DIVDE_DOT(%0, %1, %2) :
> > +   "=r" (op->val) : "r" (regs->gpr[ra]),
> > +   "r" (regs->gpr[rb]));
> > +   set_cr0(regs, op);
> 
> This seems unneccesarily complicated.  You take the trouble to do a
> "divde." instruction rather than a "divde" instruction but then don't
> use the CR0 setting that the instruction did, but instead go and work
> out what happens to CR0 manually in set_cr0().  Also you don't tell
> the compiler that CR0 has been modified, which could lead to problems.
> 
> This case could be done much more simply like this:
> 
> 
> 
>   case 425:   /* divde[.] */
>   asm volatile(PPC_DIVDE(%0, %1, %2) :
>   "=r" (op->val) : "r" (regs->gpr[ra]),
>   "r" (regs->gpr[rb]));
>   goto arith_done;
> 
> (note, goto arith_done rather than compute_done) and similarly for the
> divdeu case.

Thanks Paul for review, I will fix it as suggested and post the v2 version.

-- Bala
> 
> Paul.

Re: [PATCH 2/3] powerpc sstep: add support for divde[.] and divdeu[.] instructions

[Paul Mackerras]

On Tue, Dec 10, 2019 at 12:49:03PM +0530, Balamuruhan S wrote:
> This patch adds emulation support for divde, divdeu instructions,
>   * Divide Doubleword Extended (divde[.])
>   * Divide Doubleword Extended Unsigned (divdeu[.])
> 
> Signed-off-by: Balamuruhan S 
> ---
>  arch/powerpc/lib/sstep.c | 27 ++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index c077acb983a1..4b4119729e59 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1736,7 +1736,32 @@ int analyse_instr(struct instruction_op *op, const 
> struct pt_regs *regs,
>   op->val = (int) regs->gpr[ra] /
>   (int) regs->gpr[rb];
>   goto arith_done;
> -
> +#ifdef __powerpc64__
> + case 425:   /* divde[.] */
> + if (instr & 1) {
> + asm volatile(PPC_DIVDE_DOT(%0, %1, %2) :
> + "=r" (op->val) : "r" (regs->gpr[ra]),
> + "r" (regs->gpr[rb]));
> + set_cr0(regs, op);

This seems unneccesarily complicated.  You take the trouble to do a
"divde." instruction rather than a "divde" instruction but then don't
use the CR0 setting that the instruction did, but instead go and work
out what happens to CR0 manually in set_cr0().  Also you don't tell
the compiler that CR0 has been modified, which could lead to problems.

This case could be done much more simply like this:



case 425:   /* divde[.] */
asm volatile(PPC_DIVDE(%0, %1, %2) :
"=r" (op->val) : "r" (regs->gpr[ra]),
"r" (regs->gpr[rb]));
goto arith_done;

(note, goto arith_done rather than compute_done) and similarly for the
divdeu case.

Paul.

[PATCH 2/3] powerpc sstep: add support for divde[.] and divdeu[.] instructions

[Balamuruhan S]

This patch adds emulation support for divde, divdeu instructions,
* Divide Doubleword Extended (divde[.])
* Divide Doubleword Extended Unsigned (divdeu[.])

Signed-off-by: Balamuruhan S 
---
 arch/powerpc/lib/sstep.c | 27 ++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index c077acb983a1..4b4119729e59 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1736,7 +1736,32 @@ int analyse_instr(struct instruction_op *op, const 
struct pt_regs *regs,
op->val = (int) regs->gpr[ra] /
(int) regs->gpr[rb];
goto arith_done;
-
+#ifdef __powerpc64__
+   case 425:   /* divde[.] */
+   if (instr & 1) {
+   asm volatile(PPC_DIVDE_DOT(%0, %1, %2) :
+   "=r" (op->val) : "r" (regs->gpr[ra]),
+   "r" (regs->gpr[rb]));
+   set_cr0(regs, op);
+   } else {
+   asm volatile(PPC_DIVDE(%0, %1, %2) :
+   "=r" (op->val) : "r" (regs->gpr[ra]),
+   "r" (regs->gpr[rb]));
+   }
+   goto compute_done;
+   case 393:   /* divdeu[.] */
+   if (instr & 1) {
+   asm volatile(PPC_DIVDEU_DOT(%0, %1, %2) :
+   "=r" (op->val) : "r" (regs->gpr[ra]),
+   "r" (regs->gpr[rb]));
+   set_cr0(regs, op);
+   } else {
+   asm volatile(PPC_DIVDEU(%0, %1, %2) :
+   "=r" (op->val) : "r" (regs->gpr[ra]),
+   "r" (regs->gpr[rb]));
+   }
+   goto compute_done;
+#endif
case 755:   /* darn */
if (!cpu_has_feature(CPU_FTR_ARCH_300))
return -1;
-- 
2.14.5