[PATCH 0/6] Prefixed instruction tests to cover negative cases

[PATCH 0/6] Prefixed instruction tests to cover negative cases

[Balamuruhan S]

This patchset adds support to test negative scenarios and adds testcase
for paddi with few fixes. It is based on powerpc/next and on top of
Jordan's tests for prefixed instructions patchset,

https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-May/211394.html

Balamuruhan S (6):
  powerpc test_emulate_step: update nip with patched instruction address
  powerpc test_emulate_step: fix pr_info() to print 8-byte for prefixed
    instruction
  powerpc test_emulate_step: enhancement to test negative scenarios
  powerpc test_emulate_step: add negative tests for prefixed addi
  powerpc sstep: introduce macros to retrieve Prefix instruction
    operands
  powerpc test_emulate_step: move extern declaration to sstep.h

 arch/powerpc/include/asm/sstep.h     |  6 +++
 arch/powerpc/lib/sstep.c             | 12 ++---
 arch/powerpc/lib/test_emulate_step.c | 78 +++++++++++++++++++++++-----
 3 files changed, 77 insertions(+), 19 deletions(-)

-- 
2.24.1

[PATCH 1/6] powerpc test_emulate_step: update nip with patched instruction address

[Balamuruhan S]

pt_regs are initialized to zero in the test infrastructure, R bit
in prefixed instruction form is used to specify whether the effective
address of the storage operand is computed relative to the address
of the instruction.

If R = 1 and RA = R0|0, the sum of the address of the instruction
and the value SI is placed into register RT. So to assert the emulated
instruction with executed instruction, update nip of emulated pt_regs.

Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 arch/powerpc/lib/test_emulate_step.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
index 33a72b7d2764..d5902b7b4e5c 100644
--- a/arch/powerpc/lib/test_emulate_step.c
+++ b/arch/powerpc/lib/test_emulate_step.c
@@ -1204,13 +1204,24 @@ static struct compute_test compute_tests[] = {
 static int __init emulate_compute_instr(struct pt_regs *regs,
 					struct ppc_inst instr)
 {
+	int prefix_r, ra;
 	extern s32 patch__exec_instr;
 	struct instruction_op op;
 
 	if (!regs || !ppc_inst_val(instr))
 		return -EINVAL;
 
-	regs->nip = patch_site_addr(&patch__exec_instr);
+	/*
+	 * If R=1 and RA=0 in Prefixed instruction form, calculate the address
+	 * of the instruction and update nip to assert with executed
+	 * instruction
+	 */
+	if (ppc_inst_prefixed(instr)) {
+		prefix_r = ppc_inst_val(instr) & (1UL << 20);
+		ra = (ppc_inst_suffix(instr) >> 16) & 0x1f;
+		if (prefix_r && !ra)
+			regs->nip = patch_site_addr(&patch__exec_instr);
+	}
 
 	if (analyse_instr(&op, regs, instr) != 1 ||
 	    GETTYPE(op.type) != COMPUTE) {
-- 
2.24.1

Re: [PATCH 1/6] powerpc test_emulate_step: update nip with patched instruction address

[Jordan Niethe]

On Mon, Jun 22, 2020 at 5:10 PM Balamuruhan S <bala24@linux.ibm.com> wrote:
>
> pt_regs are initialized to zero in the test infrastructure, R bit
> in prefixed instruction form is used to specify whether the effective
> address of the storage operand is computed relative to the address
> of the instruction.
>
> If R = 1 and RA = R0|0, the sum of the address of the instruction
> and the value SI is placed into register RT. So to assert the emulated
> instruction with executed instruction, update nip of emulated pt_regs.
>
> Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> ---
>  arch/powerpc/lib/test_emulate_step.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
> index 33a72b7d2764..d5902b7b4e5c 100644
> --- a/arch/powerpc/lib/test_emulate_step.c
> +++ b/arch/powerpc/lib/test_emulate_step.c
> @@ -1204,13 +1204,24 @@ static struct compute_test compute_tests[] = {
>  static int __init emulate_compute_instr(struct pt_regs *regs,
>                                         struct ppc_inst instr)
>  {
> +       int prefix_r, ra;
>         extern s32 patch__exec_instr;
>         struct instruction_op op;
>
>         if (!regs || !ppc_inst_val(instr))
>                 return -EINVAL;
>
> -       regs->nip = patch_site_addr(&patch__exec_instr);
Is there any harm in just always setting the NIP like this instead of
only setting it for relative prefixed instructions?
> +       /*
> +        * If R=1 and RA=0 in Prefixed instruction form, calculate the address
> +        * of the instruction and update nip to assert with executed
> +        * instruction
> +        */
> +       if (ppc_inst_prefixed(instr)) {
> +               prefix_r = ppc_inst_val(instr) & (1UL << 20);
> +               ra = (ppc_inst_suffix(instr) >> 16) & 0x1f;
> +               if (prefix_r && !ra)
> +                       regs->nip = patch_site_addr(&patch__exec_instr);
> +       }
>
>         if (analyse_instr(&op, regs, instr) != 1 ||
>             GETTYPE(op.type) != COMPUTE) {
> --
> 2.24.1
>

[PATCH 2/6] powerpc test_emulate_step: fix pr_info() to print 8-byte for prefixed instruction

[Balamuruhan S]

On test failure, `pr_log()` prints 4 bytes instruction
irrespective of word/prefix instruction, fix it by printing
them appropriately.

Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 arch/powerpc/lib/test_emulate_step.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
index d5902b7b4e5c..e3b1797adfae 100644
--- a/arch/powerpc/lib/test_emulate_step.c
+++ b/arch/powerpc/lib/test_emulate_step.c
@@ -1225,7 +1225,14 @@ static int __init emulate_compute_instr(struct pt_regs *regs,
 
 	if (analyse_instr(&op, regs, instr) != 1 ||
 	    GETTYPE(op.type) != COMPUTE) {
-		pr_info("emulation failed, instruction = 0x%08x\n", ppc_inst_val(instr));
+		if (!ppc_inst_prefixed(instr)) {
+			pr_info("emulation failed, instruction = 0x%08x\n",
+				ppc_inst_val(instr));
+		} else {
+			pr_info("emulation failed, instruction = 0x%08x 0x%08x\n",
+				ppc_inst_val(instr),
+				ppc_inst_suffix(instr));
+		}
 		return -EFAULT;
 	}
 
-- 
2.24.1

Re: [PATCH 2/6] powerpc test_emulate_step: fix pr_info() to print 8-byte for prefixed instruction

[Jordan Niethe]

On Mon, Jun 22, 2020 at 5:10 PM Balamuruhan S <bala24@linux.ibm.com> wrote:
>
> On test failure, `pr_log()` prints 4 bytes instruction
> irrespective of word/prefix instruction, fix it by printing
> them appropriately.
This patch to add a ppc_inst_as_str() function should help with this,
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20200602052728.18227-1-jniethe5@gmail.com/
>
> Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> ---
>  arch/powerpc/lib/test_emulate_step.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
> index d5902b7b4e5c..e3b1797adfae 100644
> --- a/arch/powerpc/lib/test_emulate_step.c
> +++ b/arch/powerpc/lib/test_emulate_step.c
> @@ -1225,7 +1225,14 @@ static int __init emulate_compute_instr(struct pt_regs *regs,
>
>         if (analyse_instr(&op, regs, instr) != 1 ||
>             GETTYPE(op.type) != COMPUTE) {
> -               pr_info("emulation failed, instruction = 0x%08x\n", ppc_inst_val(instr));
> +               if (!ppc_inst_prefixed(instr)) {
> +                       pr_info("emulation failed, instruction = 0x%08x\n",
> +                               ppc_inst_val(instr));
> +               } else {
> +                       pr_info("emulation failed, instruction = 0x%08x 0x%08x\n",
> +                               ppc_inst_val(instr),
> +                               ppc_inst_suffix(instr));
> +               }
>                 return -EFAULT;
>         }
>
> --
> 2.24.1
>

[PATCH 3/6] powerpc test_emulate_step: enhancement to test negative scenarios

[Balamuruhan S]

add provision to declare test is a negative scenario, verify
whether emulation fails and avoid executing it.

Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 arch/powerpc/lib/test_emulate_step.c | 46 ++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
index e3b1797adfae..79acc899a618 100644
--- a/arch/powerpc/lib/test_emulate_step.c
+++ b/arch/powerpc/lib/test_emulate_step.c
@@ -703,6 +703,7 @@ struct compute_test {
 		unsigned long flags;
 		struct ppc_inst instr;
 		struct pt_regs regs;
+		bool negative;
 	} subtests[MAX_SUBTESTS + 1];
 };
 
@@ -1202,9 +1203,10 @@ static struct compute_test compute_tests[] = {
 };
 
 static int __init emulate_compute_instr(struct pt_regs *regs,
-					struct ppc_inst instr)
+					struct ppc_inst instr,
+					bool negative)
 {
-	int prefix_r, ra;
+	int prefix_r, ra, analysed;
 	extern s32 patch__exec_instr;
 	struct instruction_op op;
 
@@ -1223,8 +1225,10 @@ static int __init emulate_compute_instr(struct pt_regs *regs,
 			regs->nip = patch_site_addr(&patch__exec_instr);
 	}
 
-	if (analyse_instr(&op, regs, instr) != 1 ||
-	    GETTYPE(op.type) != COMPUTE) {
+	analysed = analyse_instr(&op, regs, instr);
+	if (analysed != 1 || GETTYPE(op.type) != COMPUTE) {
+		if (negative)
+			return -EFAULT;
 		if (!ppc_inst_prefixed(instr)) {
 			pr_info("emulation failed, instruction = 0x%08x\n",
 				ppc_inst_val(instr));
@@ -1235,8 +1239,18 @@ static int __init emulate_compute_instr(struct pt_regs *regs,
 		}
 		return -EFAULT;
 	}
-
-	emulate_update_regs(regs, &op);
+	if (analysed == 1 && negative) {
+		if (!ppc_inst_prefixed(instr)) {
+			pr_info("negative test failed, instruction = 0x%08x\n",
+				ppc_inst_val(instr));
+		} else {
+			pr_info("negative test  failed, instruction = 0x%08x 0x%08x\n",
+				ppc_inst_val(instr),
+				ppc_inst_suffix(instr));
+		}
+	}
+	if (!negative)
+		emulate_update_regs(regs, &op);
 	return 0;
 }
 
@@ -1252,7 +1266,14 @@ static int __init execute_compute_instr(struct pt_regs *regs,
 	/* Patch the NOP with the actual instruction */
 	patch_instruction_site(&patch__exec_instr, instr);
 	if (exec_instr(regs)) {
-		pr_info("execution failed, instruction = 0x%08x\n", ppc_inst_val(instr));
+		if (!ppc_inst_prefixed(instr)) {
+			pr_info("execution failed, instruction = 0x%08x\n",
+				ppc_inst_val(instr));
+		} else {
+			pr_info("execution failed, instruction = 0x%08x 0x%08x\n",
+				ppc_inst_val(instr),
+				ppc_inst_suffix(instr));
+		}
 		return -EFAULT;
 	}
 
@@ -1274,7 +1295,7 @@ static void __init run_tests_compute(void)
 	struct pt_regs *regs, exp, got;
 	unsigned int i, j, k;
 	struct ppc_inst instr;
-	bool ignore_gpr, ignore_xer, ignore_ccr, passed;
+	bool ignore_gpr, ignore_xer, ignore_ccr, passed, rc, negative;
 
 	for (i = 0; i < ARRAY_SIZE(compute_tests); i++) {
 		test = &compute_tests[i];
@@ -1288,6 +1309,7 @@ static void __init run_tests_compute(void)
 			instr = test->subtests[j].instr;
 			flags = test->subtests[j].flags;
 			regs = &test->subtests[j].regs;
+			negative = test->subtests[j].negative;
 			ignore_xer = flags & IGNORE_XER;
 			ignore_ccr = flags & IGNORE_CCR;
 			passed = true;
@@ -1302,8 +1324,12 @@ static void __init run_tests_compute(void)
 			exp.msr = MSR_KERNEL;
 			got.msr = MSR_KERNEL;
 
-			if (emulate_compute_instr(&got, instr) ||
-			    execute_compute_instr(&exp, instr)) {
+			rc = emulate_compute_instr(&got, instr, negative) != 0;
+			if (negative) {
+				/* skip executing instruction */
+				passed = rc;
+				goto print;
+			} else if (rc || execute_compute_instr(&exp, instr)) {
 				passed = false;
 				goto print;
 			}
-- 
2.24.1

Re: [PATCH 3/6] powerpc test_emulate_step: enhancement to test negative scenarios

[Sandipan Das]

Hi Bala,

On 22/06/20 12:39 pm, Balamuruhan S wrote:
> add provision to declare test is a negative scenario, verify
> whether emulation fails and avoid executing it.
> 
> Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> ---
>  arch/powerpc/lib/test_emulate_step.c | 46 ++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
> index e3b1797adfae..79acc899a618 100644
> --- a/arch/powerpc/lib/test_emulate_step.c
> +++ b/arch/powerpc/lib/test_emulate_step.c
> @@ -703,6 +703,7 @@ struct compute_test {
>  		unsigned long flags;
>  		struct ppc_inst instr;
>  		struct pt_regs regs;
> +		bool negative;
>  	} subtests[MAX_SUBTESTS + 1];
>  };
>  

Bits of 'flags' are currently used to specify if parts of the resulting pt_regs
are to be ignored. Instead of adding a new member to the struct, can we not do
this using a bit in 'flags'?


- Sandipan

[PATCH 4/6] powerpc test_emulate_step: add negative tests for prefixed addi

[Balamuruhan S]

testcases for `paddi` instruction to cover the negative case,
if R is equal to 1 and RA is not equal to 0, the instruction
form is invalid.

Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 arch/powerpc/lib/test_emulate_step.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
index 79acc899a618..f9825c275c31 100644
--- a/arch/powerpc/lib/test_emulate_step.c
+++ b/arch/powerpc/lib/test_emulate_step.c
@@ -1197,6 +1197,16 @@ static struct compute_test compute_tests[] = {
 				.regs = {
 					.gpr[21] = 0,
 				}
+			},
+			/* Invalid instruction form with R = 1 and RA != 0 */
+			{
+				.descr = "RA = R22(0), SI = 0, R = 1",
+				.instr = TEST_PADDI(21, 22, 0, 1),
+				.negative = true,
+				.regs = {
+					.gpr[21] = 0,
+					.gpr[22] = 0,
+				}
 			}
 		}
 	}
-- 
2.24.1

[PATCH 5/6] powerpc sstep: introduce macros to retrieve Prefix instruction operands

[Balamuruhan S]

retrieve prefix instruction operands RA and pc relative bit R values
using macros and adopt it in sstep.c and test_emulate_step.c.

Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 arch/powerpc/include/asm/sstep.h     |  4 ++++
 arch/powerpc/lib/sstep.c             | 12 ++++++------
 arch/powerpc/lib/test_emulate_step.c |  4 ++--
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
index 3b01c69a44aa..325975b4ef30 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -104,6 +104,10 @@ enum instruction_type {
 
 #define MKOP(t, f, s)	((t) | (f) | SIZE(s))
 
+/* Prefix instruction operands */
+#define GET_PREFIX_RA(i)	(((i) >> 16) & 0x1f)
+#define GET_PREFIX_R(i)		((i) & (1ul << 20))
+
 struct instruction_op {
 	int type;
 	int reg;
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 5abe98216dc2..fb4c5767663d 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -200,8 +200,8 @@ static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned int instr,
 	unsigned int  dd;
 	unsigned long ea, d0, d1, d;
 
-	prefix_r = instr & (1ul << 20);
-	ra = (suffix >> 16) & 0x1f;
+	prefix_r = GET_PREFIX_R(instr);
+	ra = GET_PREFIX_RA(suffix);
 
 	d0 = instr & 0x3ffff;
 	d1 = suffix & 0xffff;
@@ -1339,8 +1339,8 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
 	switch (opcode) {
 #ifdef __powerpc64__
 	case 1:
-		prefix_r = word & (1ul << 20);
-		ra = (suffix >> 16) & 0x1f;
+		prefix_r = GET_PREFIX_R(word);
+		ra = GET_PREFIX_RA(suffix);
 		rd = (suffix >> 21) & 0x1f;
 		op->reg = rd;
 		op->val = regs->gpr[rd];
@@ -2715,8 +2715,8 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
 		}
 		break;
 	case 1: /* Prefixed instructions */
-		prefix_r = word & (1ul << 20);
-		ra = (suffix >> 16) & 0x1f;
+		prefix_r = GET_PREFIX_R(word);
+		ra = GET_PREFIX_RA(suffix);
 		op->update_reg = ra;
 		rd = (suffix >> 21) & 0x1f;
 		op->reg = rd;
diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
index f9825c275c31..f1a447026b6e 100644
--- a/arch/powerpc/lib/test_emulate_step.c
+++ b/arch/powerpc/lib/test_emulate_step.c
@@ -1229,8 +1229,8 @@ static int __init emulate_compute_instr(struct pt_regs *regs,
 	 * instruction
 	 */
 	if (ppc_inst_prefixed(instr)) {
-		prefix_r = ppc_inst_val(instr) & (1UL << 20);
-		ra = (ppc_inst_suffix(instr) >> 16) & 0x1f;
+		prefix_r = GET_PREFIX_R(ppc_inst_val(instr));
+		ra = GET_PREFIX_RA(ppc_inst_suffix(instr));
 		if (prefix_r && !ra)
 			regs->nip = patch_site_addr(&patch__exec_instr);
 	}
-- 
2.24.1

[PATCH 6/6] powerpc test_emulate_step: move extern declaration to sstep.h

[Balamuruhan S]

fix checkpatch.pl warnings by moving extern declaration from source
file to headerfile.

Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 arch/powerpc/include/asm/sstep.h     | 2 ++
 arch/powerpc/lib/test_emulate_step.c | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
index 325975b4ef30..c8e37ef060c1 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -108,6 +108,8 @@ enum instruction_type {
 #define GET_PREFIX_RA(i)	(((i) >> 16) & 0x1f)
 #define GET_PREFIX_R(i)		((i) & (1ul << 20))
 
+extern s32 patch__exec_instr;
+
 struct instruction_op {
 	int type;
 	int reg;
diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
index f1a447026b6e..386245607568 100644
--- a/arch/powerpc/lib/test_emulate_step.c
+++ b/arch/powerpc/lib/test_emulate_step.c
@@ -1217,7 +1217,6 @@ static int __init emulate_compute_instr(struct pt_regs *regs,
 					bool negative)
 {
 	int prefix_r, ra, analysed;
-	extern s32 patch__exec_instr;
 	struct instruction_op op;
 
 	if (!regs || !ppc_inst_val(instr))
@@ -1268,7 +1267,6 @@ static int __init execute_compute_instr(struct pt_regs *regs,
 					struct ppc_inst instr)
 {
 	extern int exec_instr(struct pt_regs *regs);
-	extern s32 patch__exec_instr;
 
 	if (!regs || !ppc_inst_val(instr))
 		return -EINVAL;
-- 
2.24.1