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

[Paul Mackerras <paulus@ozlabs.org>]

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 <bala24@linux.ibm.com>
> ---
>  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.