[PATCH] lib/sstep: Fix incorrect return from analyze_instr()

[PATCH] lib/sstep: Fix incorrect return from analyze_instr()

[Ananth N Mavinakayanahalli]

We currently just percolate the return value from analyze_instr()
to the caller of emulate_step(), especially if it is a -1.

For one particular case (opcode = 4) for instructions that
aren't currently emulated, we are returning 'should not be
single-stepped' while we should have returned 0 which says
'did not emulate, may have to single-step'.

Signed-off-by: Ananth N Mavinakayanahalli <ananth@linux.ibm.com>
Tested-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/lib/sstep.c |   49 +++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 5a425a4a1d88..a3a0373843cd 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1445,34 +1445,39 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
 
 #ifdef __powerpc64__
 	case 4:
-		if (!cpu_has_feature(CPU_FTR_ARCH_300))
-			return -1;
-
-		switch (word & 0x3f) {
-		case 48:	/* maddhd */
-			asm volatile(PPC_MADDHD(%0, %1, %2, %3) :
-				     "=r" (op->val) : "r" (regs->gpr[ra]),
-				     "r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
-			goto compute_done;
+		/*
+		 * There are very many instructions with this primary opcode
+		 * introduced in the ISA as early as v2.03. However, the ones
+		 * we currently emulate were all introduced with ISA 3.0
+		 */
+		if (cpu_has_feature(CPU_FTR_ARCH_300)) {
+			switch (word & 0x3f) {
+			case 48:	/* maddhd */
+				asm volatile(PPC_MADDHD(%0, %1, %2, %3) :
+					     "=r" (op->val) : "r" (regs->gpr[ra]),
+					     "r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
+				goto compute_done;
 
-		case 49:	/* maddhdu */
-			asm volatile(PPC_MADDHDU(%0, %1, %2, %3) :
-				     "=r" (op->val) : "r" (regs->gpr[ra]),
-				     "r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
-			goto compute_done;
+			case 49:	/* maddhdu */
+				asm volatile(PPC_MADDHDU(%0, %1, %2, %3) :
+					     "=r" (op->val) : "r" (regs->gpr[ra]),
+					     "r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
+				goto compute_done;
 
-		case 51:	/* maddld */
-			asm volatile(PPC_MADDLD(%0, %1, %2, %3) :
-				     "=r" (op->val) : "r" (regs->gpr[ra]),
-				     "r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
-			goto compute_done;
+			case 51:	/* maddld */
+				asm volatile(PPC_MADDLD(%0, %1, %2, %3) :
+					     "=r" (op->val) : "r" (regs->gpr[ra]),
+					     "r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
+				goto compute_done;
+			}
 		}
 
 		/*
-		 * There are other instructions from ISA 3.0 with the same
-		 * primary opcode which do not have emulation support yet.
+		 * Rest of the instructions with this primary opcode do not
+		 * have emulation support yet.
 		 */
-		return -1;
+		op->type = UNKNOWN;
+		return 0;
 #endif
 
 	case 7:		/* mulli */

Re: [PATCH] lib/sstep: Fix incorrect return from analyze_instr()

[Naveen N. Rao]

On 2021/01/21 10:18PM, Ananth N Mavinakayanahalli wrote:
> We currently just percolate the return value from analyze_instr()
> to the caller of emulate_step(), especially if it is a -1.
> 
> For one particular case (opcode = 4) for instructions that
> aren't currently emulated, we are returning 'should not be
> single-stepped' while we should have returned 0 which says
> 'did not emulate, may have to single-step'.
> 
> Signed-off-by: Ananth N Mavinakayanahalli <ananth@linux.ibm.com>
> Tested-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/lib/sstep.c |   49 +++++++++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 22 deletions(-)

Fixes: 930d6288a26787 ("powerpc: sstep: Add support for maddhd, maddhdu, maddld instructions")
Reviewed-by: Naveen N. Rao < naveen.n.rao@linux.vnet.ibm.com>

- Naveen

Re: [PATCH] lib/sstep: Fix incorrect return from analyze_instr()

[Sandipan Das]

On 21/01/21 10:18 pm, Ananth N Mavinakayanahalli wrote:
> We currently just percolate the return value from analyze_instr()
> to the caller of emulate_step(), especially if it is a -1.
> 
> For one particular case (opcode = 4) for instructions that
> aren't currently emulated, we are returning 'should not be
> single-stepped' while we should have returned 0 which says
> 'did not emulate, may have to single-step'.
> 
> Signed-off-by: Ananth N Mavinakayanahalli <ananth@linux.ibm.com>
> Tested-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/lib/sstep.c |   49 +++++++++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 22 deletions(-)
> 

Thanks for fixing this.

Reviewed-by: Sandipan Das <sandipan@linux.ibm.com>

Re: [PATCH] lib/sstep: Fix incorrect return from analyze_instr()

[Michael Ellerman]

Ananth N Mavinakayanahalli <ananth@linux.ibm.com> writes:
> We currently just percolate the return value from analyze_instr()
> to the caller of emulate_step(), especially if it is a -1.
>
> For one particular case (opcode = 4) for instructions that
> aren't currently emulated, we are returning 'should not be
> single-stepped' while we should have returned 0 which says
> 'did not emulate, may have to single-step'.
>
> Signed-off-by: Ananth N Mavinakayanahalli <ananth@linux.ibm.com>
> Tested-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/lib/sstep.c |   49 +++++++++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index 5a425a4a1d88..a3a0373843cd 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1445,34 +1445,39 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
>  
>  #ifdef __powerpc64__
>  	case 4:
> -		if (!cpu_has_feature(CPU_FTR_ARCH_300))
> -			return -1;
> -
> -		switch (word & 0x3f) {
> -		case 48:	/* maddhd */
> -			asm volatile(PPC_MADDHD(%0, %1, %2, %3) :
> -				     "=r" (op->val) : "r" (regs->gpr[ra]),
> -				     "r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
> -			goto compute_done;
> +		/*
> +		 * There are very many instructions with this primary opcode
> +		 * introduced in the ISA as early as v2.03. However, the ones
> +		 * we currently emulate were all introduced with ISA 3.0
> +		 */
> +		if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> +			switch (word & 0x3f) {
> +			case 48:	/* maddhd */
> +				asm volatile(PPC_MADDHD(%0, %1, %2, %3) :
> +					     "=r" (op->val) : "r" (regs->gpr[ra]),
> +					     "r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
> +				goto compute_done;

Indenting everything makes this patch harder to read, and I think makes
the resulting code harder to read too. We already have two levels of
switch here, and we're inside a ~1700 line function, so keeping things
simple is important I think.

Doesn't this achieve the same result?

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index bf7a7d62ae8b..d631baaf1da2 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1443,8 +1443,10 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
 
 #ifdef __powerpc64__
 	case 4:
-		if (!cpu_has_feature(CPU_FTR_ARCH_300))
-			return -1;
+		if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
+			op->type = UNKNOWN;
+			return 0;
+		}
 
 		switch (word & 0x3f) {
 		case 48:	/* maddhd */
@@ -1470,7 +1472,8 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
 		 * There are other instructions from ISA 3.0 with the same
 		 * primary opcode which do not have emulation support yet.
 		 */
-		return -1;
+		op->type = UNKNOWN;
+		return 0;
 #endif
 
 	case 7:		/* mulli */


cheers

Re: [PATCH] lib/sstep: Fix incorrect return from analyze_instr()

[Ananth N Mavinakayanahalli]

On 1/23/21 6:03 AM, Michael Ellerman wrote:
> Ananth N Mavinakayanahalli <ananth@linux.ibm.com> writes:
>> We currently just percolate the return value from analyze_instr()
>> to the caller of emulate_step(), especially if it is a -1.
>>
>> For one particular case (opcode = 4) for instructions that
>> aren't currently emulated, we are returning 'should not be
>> single-stepped' while we should have returned 0 which says
>> 'did not emulate, may have to single-step'.
>>
>> Signed-off-by: Ananth N Mavinakayanahalli <ananth@linux.ibm.com>
>> Tested-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/lib/sstep.c |   49 +++++++++++++++++++++++++---------------------
>>   1 file changed, 27 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
>> index 5a425a4a1d88..a3a0373843cd 100644
>> --- a/arch/powerpc/lib/sstep.c
>> +++ b/arch/powerpc/lib/sstep.c
>> @@ -1445,34 +1445,39 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
>>   
>>   #ifdef __powerpc64__
>>   	case 4:
>> -		if (!cpu_has_feature(CPU_FTR_ARCH_300))
>> -			return -1;
>> -
>> -		switch (word & 0x3f) {
>> -		case 48:	/* maddhd */
>> -			asm volatile(PPC_MADDHD(%0, %1, %2, %3) :
>> -				     "=r" (op->val) : "r" (regs->gpr[ra]),
>> -				     "r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
>> -			goto compute_done;
>> +		/*
>> +		 * There are very many instructions with this primary opcode
>> +		 * introduced in the ISA as early as v2.03. However, the ones
>> +		 * we currently emulate were all introduced with ISA 3.0
>> +		 */
>> +		if (cpu_has_feature(CPU_FTR_ARCH_300)) {
>> +			switch (word & 0x3f) {
>> +			case 48:	/* maddhd */
>> +				asm volatile(PPC_MADDHD(%0, %1, %2, %3) :
>> +					     "=r" (op->val) : "r" (regs->gpr[ra]),
>> +					     "r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
>> +				goto compute_done;
> 
> Indenting everything makes this patch harder to read, and I think makes
> the resulting code harder to read too. We already have two levels of
> switch here, and we're inside a ~1700 line function, so keeping things
> simple is important I think.
> 
> Doesn't this achieve the same result?
> 
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index bf7a7d62ae8b..d631baaf1da2 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1443,8 +1443,10 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
>   
>   #ifdef __powerpc64__
>   	case 4:
> -		if (!cpu_has_feature(CPU_FTR_ARCH_300))
> -			return -1;
> +		if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
> +			op->type = UNKNOWN;
> +			return 0;
> +		}
>   
>   		switch (word & 0x3f) {
>   		case 48:	/* maddhd */
> @@ -1470,7 +1472,8 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
>   		 * There are other instructions from ISA 3.0 with the same
>   		 * primary opcode which do not have emulation support yet.
>   		 */
> -		return -1;
> +		op->type = UNKNOWN;
> +		return 0;
>   #endif
>   
>   	case 7:		/* mulli */
> 

Looks good to me.

Acked-by: Ananth N Mavinakayanahalli <ananth@linux.ibm.com>


-- 
Ananth