On 1/23/21 6:03 AM, Michael Ellerman wrote:
Ananth N Mavinakayanahalli <ana...@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 <ana...@linux.ibm.com>
Tested-by: Naveen N. Rao <naveen.n....@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 <ana...@linux.ibm.com>
--
Ananth
