* [PATCH] lib/sstep: Fix incorrect return from analyze_instr()
@ 2021-01-21 16:48 Ananth N Mavinakayanahalli
2021-01-22 6:27 ` Naveen N. Rao
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Ananth N Mavinakayanahalli @ 2021-01-21 16:48 UTC (permalink / raw
To: linuxppc-dev; +Cc: naveen.n.rao, paulus, sandipan, ravi.bangoria
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 */
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] lib/sstep: Fix incorrect return from analyze_instr()
2021-01-21 16:48 [PATCH] lib/sstep: Fix incorrect return from analyze_instr() Ananth N Mavinakayanahalli
@ 2021-01-22 6:27 ` Naveen N. Rao
2021-01-22 7:09 ` Sandipan Das
2021-01-23 0:33 ` Michael Ellerman
2 siblings, 0 replies; 5+ messages in thread
From: Naveen N. Rao @ 2021-01-22 6:27 UTC (permalink / raw
To: Ananth N Mavinakayanahalli
Cc: ravi.bangoria, paulus, sandipan, naveen.n.rao, linuxppc-dev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lib/sstep: Fix incorrect return from analyze_instr()
2021-01-21 16:48 [PATCH] lib/sstep: Fix incorrect return from analyze_instr() Ananth N Mavinakayanahalli
2021-01-22 6:27 ` Naveen N. Rao
@ 2021-01-22 7:09 ` Sandipan Das
2021-01-23 0:33 ` Michael Ellerman
2 siblings, 0 replies; 5+ messages in thread
From: Sandipan Das @ 2021-01-22 7:09 UTC (permalink / raw
To: Ananth N Mavinakayanahalli
Cc: naveen.n.rao, linuxppc-dev, paulus, ravi.bangoria
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lib/sstep: Fix incorrect return from analyze_instr()
2021-01-21 16:48 [PATCH] lib/sstep: Fix incorrect return from analyze_instr() Ananth N Mavinakayanahalli
2021-01-22 6:27 ` Naveen N. Rao
2021-01-22 7:09 ` Sandipan Das
@ 2021-01-23 0:33 ` Michael Ellerman
2021-01-25 4:52 ` Ananth N Mavinakayanahalli
2 siblings, 1 reply; 5+ messages in thread
From: Michael Ellerman @ 2021-01-23 0:33 UTC (permalink / raw
To: Ananth N Mavinakayanahalli, linuxppc-dev
Cc: naveen.n.rao, ravi.bangoria, paulus, sandipan
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] lib/sstep: Fix incorrect return from analyze_instr()
2021-01-23 0:33 ` Michael Ellerman
@ 2021-01-25 4:52 ` Ananth N Mavinakayanahalli
0 siblings, 0 replies; 5+ messages in thread
From: Ananth N Mavinakayanahalli @ 2021-01-25 4:52 UTC (permalink / raw
To: Michael Ellerman, linuxppc-dev
Cc: naveen.n.rao, ravi.bangoria, paulus, sandipan
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-25 4:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-21 16:48 [PATCH] lib/sstep: Fix incorrect return from analyze_instr() Ananth N Mavinakayanahalli
2021-01-22 6:27 ` Naveen N. Rao
2021-01-22 7:09 ` Sandipan Das
2021-01-23 0:33 ` Michael Ellerman
2021-01-25 4:52 ` Ananth N Mavinakayanahalli
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.