[Qemu-devel] [PATCH Risu 0/2] PPC64 improvements

[Qemu-devel] [PATCH Risu 0/2] PPC64 improvements

[Jose Ricardo Ziviani]

This patchset contains a fix necessary to run Risu under linux-user, that is due to the fact that linux-user doesn't fully implement ucontext struct. It also contains an small improvement to validate FPSRC flag for floating-point operations.

Jose Ricardo Ziviani (2):
  risu_ppc64: Fix Risu to run under qemu linux user
  risu_ppc64: Compare FPSCR flags

 risu_ppc64le.c         |  2 +-
 risu_reginfo_ppc64le.c | 16 +++++++++++-----
 test_ppc64le.s         | 20 +++++++++-----------
 3 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH Risu 1/2] risu_ppc64: Fix Risu to run under qemu linux user

[Jose Ricardo Ziviani]

Qemu linux-user doesn't fill uc_mcontext completely like full emul. does.
For instance, uc->uc_mcontext.regs->nip is an invalid so this
commit replaces it by uc->uc_mcontext.gp_regs[PT_NIP]

Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
---
 risu_ppc64le.c         |  2 +-
 risu_reginfo_ppc64le.c | 11 ++++++-----
 test_ppc64le.s         | 20 +++++++++-----------
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/risu_ppc64le.c b/risu_ppc64le.c
index 9c1fafd..773d14c 100644
--- a/risu_ppc64le.c
+++ b/risu_ppc64le.c
@@ -27,7 +27,7 @@ uint8_t apprentice_memblock[MEMBLOCKLEN];
 void advance_pc(void *vuc)
 {
     ucontext_t *uc = (ucontext_t*)vuc;
-    uc->uc_mcontext.regs->nip += 4;
+    uc->uc_mcontext.gp_regs[PT_NIP] += 4;
 }
 
 void set_x0(void *vuc, uint64_t x0)
diff --git a/risu_reginfo_ppc64le.c b/risu_reginfo_ppc64le.c
index 7a54eab..4dc509c 100644
--- a/risu_reginfo_ppc64le.c
+++ b/risu_reginfo_ppc64le.c
@@ -28,8 +28,9 @@ void reginfo_init(struct reginfo *ri, ucontext_t *uc)
     int i;
     memset(ri, 0, sizeof(*ri));
 
-    ri->faulting_insn = *((uint32_t *)uc->uc_mcontext.regs->nip);
-    ri->nip = uc->uc_mcontext.regs->nip - image_start_address;
+    ri->faulting_insn = *((uint32_t *)uc->uc_mcontext.gp_regs[PT_NIP]);
+    ri->prev_insn = *((uint32_t *)(uc->uc_mcontext.gp_regs[PT_NIP] - 4));
+    ri->nip = uc->uc_mcontext.gp_regs[PT_NIP] - image_start_address;
 
     for (i = 0; i < NGREG; i++) {
         ri->gregs[i] = uc->uc_mcontext.gp_regs[i];
@@ -105,9 +106,9 @@ void reginfo_dump(struct reginfo *ri, int is_master)
 {
     int i;
     if (is_master) {
-        fprintf(stderr, "  faulting insn \e[1;101;37m0x%x\e[0m\n", ri->faulting_insn);
-        fprintf(stderr, "  prev insn     \e[1;101;37m0x%x\e[0m\n", ri->prev_insn);
-        fprintf(stderr, "  prev addr     \e[1;101;37m0x%" PRIx64 "\e[0m\n\n", ri->prev_addr);
+        fprintf(stderr, "  faulting insn 0x%x\n", ri->faulting_insn);
+        fprintf(stderr, "  prev insn     0x%x\n", ri->prev_insn);
+        fprintf(stderr, "  prev addr    0x%" PRIx64 "\n\n", ri->nip);
     }
 
     for (i = 0; i < 16; i++) {
diff --git a/test_ppc64le.s b/test_ppc64le.s
index 4321751..4af770c 100644
--- a/test_ppc64le.s
+++ b/test_ppc64le.s
@@ -12,20 +12,18 @@
  *****************************************************************************/
 
 /* Initialise the gp regs */
-li 0,0
-li 1,1
-li 2,2
-li 3,3
-li 4,4
-li 5,5
-li 6,6
-li 7,7
-li 8,8
-li 9,9
+li 0, 0
+li 2, 2
+li 3, 3
+li 4, 4
+li 5, 5
+li 6, 6
+li 7, 7
+li 8, 8
+li 9, 9
 li 10, 10
 li 11, 11
 li 12, 12
-li 13, 13
 li 14, 14
 li 15, 15
 li 16, 16
-- 
2.7.4

Re: [Qemu-devel] [PATCH Risu 1/2] risu_ppc64: Fix Risu to run under qemu linux user

[Alex Bennée]

Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> writes:

> Qemu linux-user doesn't fill uc_mcontext completely like full emul.
> does.

Are you going to submit a fix for QEMU for this? Is there a reason it
doesn't do it correctly?

> For instance, uc->uc_mcontext.regs->nip is an invalid so this
> commit replaces it by uc->uc_mcontext.gp_regs[PT_NIP]
>
> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
> ---
>  risu_ppc64le.c         |  2 +-
>  risu_reginfo_ppc64le.c | 11 ++++++-----
>  test_ppc64le.s         | 20 +++++++++-----------
>  3 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/risu_ppc64le.c b/risu_ppc64le.c
> index 9c1fafd..773d14c 100644
> --- a/risu_ppc64le.c
> +++ b/risu_ppc64le.c
> @@ -27,7 +27,7 @@ uint8_t apprentice_memblock[MEMBLOCKLEN];
>  void advance_pc(void *vuc)
>  {
>      ucontext_t *uc = (ucontext_t*)vuc;
> -    uc->uc_mcontext.regs->nip += 4;
> +    uc->uc_mcontext.gp_regs[PT_NIP] += 4;
>  }
>
>  void set_x0(void *vuc, uint64_t x0)
> diff --git a/risu_reginfo_ppc64le.c b/risu_reginfo_ppc64le.c
> index 7a54eab..4dc509c 100644
> --- a/risu_reginfo_ppc64le.c
> +++ b/risu_reginfo_ppc64le.c
> @@ -28,8 +28,9 @@ void reginfo_init(struct reginfo *ri, ucontext_t *uc)
>      int i;
>      memset(ri, 0, sizeof(*ri));
>
> -    ri->faulting_insn = *((uint32_t *)uc->uc_mcontext.regs->nip);
> -    ri->nip = uc->uc_mcontext.regs->nip - image_start_address;
> +    ri->faulting_insn = *((uint32_t *)uc->uc_mcontext.gp_regs[PT_NIP]);
> +    ri->prev_insn = *((uint32_t *)(uc->uc_mcontext.gp_regs[PT_NIP] - 4));
> +    ri->nip = uc->uc_mcontext.gp_regs[PT_NIP] - image_start_address;
>
>      for (i = 0; i < NGREG; i++) {
>          ri->gregs[i] = uc->uc_mcontext.gp_regs[i];
> @@ -105,9 +106,9 @@ void reginfo_dump(struct reginfo *ri, int is_master)
>  {
>      int i;
>      if (is_master) {
> -        fprintf(stderr, "  faulting insn \e[1;101;37m0x%x\e[0m\n", ri->faulting_insn);
> -        fprintf(stderr, "  prev insn     \e[1;101;37m0x%x\e[0m\n", ri->prev_insn);
> -        fprintf(stderr, "  prev addr     \e[1;101;37m0x%" PRIx64 "\e[0m\n\n", ri->prev_addr);
> +        fprintf(stderr, "  faulting insn 0x%x\n", ri->faulting_insn);
> +        fprintf(stderr, "  prev insn     0x%x\n", ri->prev_insn);
> +        fprintf(stderr, "  prev addr    0x%" PRIx64 "\n\n", ri->nip);
>      }
>
>      for (i = 0; i < 16; i++) {
> diff --git a/test_ppc64le.s b/test_ppc64le.s
> index 4321751..4af770c 100644
> --- a/test_ppc64le.s
> +++ b/test_ppc64le.s
> @@ -12,20 +12,18 @@
>   *****************************************************************************/
>
>  /* Initialise the gp regs */
> -li 0,0
> -li 1,1
> -li 2,2
> -li 3,3
> -li 4,4
> -li 5,5
> -li 6,6
> -li 7,7
> -li 8,8
> -li 9,9
> +li 0, 0
> +li 2, 2
> +li 3, 3
> +li 4, 4
> +li 5, 5
> +li 6, 6
> +li 7, 7
> +li 8, 8
> +li 9, 9
>  li 10, 10
>  li 11, 11
>  li 12, 12
> -li 13, 13
>  li 14, 14
>  li 15, 15
>  li 16, 16


--
Alex Bennée

Re: [Qemu-devel] [PATCH Risu 1/2] risu_ppc64: Fix Risu to run under qemu linux user

[Peter Maydell]

On 30 January 2017 at 02:47, Jose Ricardo Ziviani
<joserz@linux.vnet.ibm.com> wrote:
> Qemu linux-user doesn't fill uc_mcontext completely like full emul. does.
> For instance, uc->uc_mcontext.regs->nip is an invalid so this
> commit replaces it by uc->uc_mcontext.gp_regs[PT_NIP]

It's not clear to me from this commit message whether this is
a bug in QEMU's userspace emulation which this is trying to work
around (in which case we should just fix it in QEMU), or a
bug in risu where we were incorrectly relying on something the
kernel doesn't actually guarantee. Which is it?

Also, looking at the kernel source and headers as far
as I can see uc_context.regs is a pointer set up such that
uc->uc_mcontext.regs->nip is pointing at the same bit of
memory where uc->uc_mcontext.gp_regs[PT_NIP] is,
and the QEMU code does similar, so I don't see how you can
get two different values from the two things.

(It is certainly the case that risu is quite good at exercising
odd corner cases of the signal handling code in QEMU which most
normal programs don't care about...)

> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
> ---
>  risu_ppc64le.c         |  2 +-
>  risu_reginfo_ppc64le.c | 11 ++++++-----
>  test_ppc64le.s         | 20 +++++++++-----------
>  3 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/risu_ppc64le.c b/risu_ppc64le.c
> index 9c1fafd..773d14c 100644
> --- a/risu_ppc64le.c
> +++ b/risu_ppc64le.c
> @@ -27,7 +27,7 @@ uint8_t apprentice_memblock[MEMBLOCKLEN];
>  void advance_pc(void *vuc)
>  {
>      ucontext_t *uc = (ucontext_t*)vuc;
> -    uc->uc_mcontext.regs->nip += 4;
> +    uc->uc_mcontext.gp_regs[PT_NIP] += 4;
>  }
>
>  void set_x0(void *vuc, uint64_t x0)
> diff --git a/risu_reginfo_ppc64le.c b/risu_reginfo_ppc64le.c
> index 7a54eab..4dc509c 100644
> --- a/risu_reginfo_ppc64le.c
> +++ b/risu_reginfo_ppc64le.c
> @@ -28,8 +28,9 @@ void reginfo_init(struct reginfo *ri, ucontext_t *uc)
>      int i;
>      memset(ri, 0, sizeof(*ri));
>
> -    ri->faulting_insn = *((uint32_t *)uc->uc_mcontext.regs->nip);
> -    ri->nip = uc->uc_mcontext.regs->nip - image_start_address;
> +    ri->faulting_insn = *((uint32_t *)uc->uc_mcontext.gp_regs[PT_NIP]);
> +    ri->prev_insn = *((uint32_t *)(uc->uc_mcontext.gp_regs[PT_NIP] - 4));
> +    ri->nip = uc->uc_mcontext.gp_regs[PT_NIP] - image_start_address;
>
>      for (i = 0; i < NGREG; i++) {
>          ri->gregs[i] = uc->uc_mcontext.gp_regs[i];
> @@ -105,9 +106,9 @@ void reginfo_dump(struct reginfo *ri, int is_master)
>  {
>      int i;
>      if (is_master) {
> -        fprintf(stderr, "  faulting insn \e[1;101;37m0x%x\e[0m\n", ri->faulting_insn);
> -        fprintf(stderr, "  prev insn     \e[1;101;37m0x%x\e[0m\n", ri->prev_insn);
> -        fprintf(stderr, "  prev addr     \e[1;101;37m0x%" PRIx64 "\e[0m\n\n", ri->prev_addr);
> +        fprintf(stderr, "  faulting insn 0x%x\n", ri->faulting_insn);
> +        fprintf(stderr, "  prev insn     0x%x\n", ri->prev_insn);
> +        fprintf(stderr, "  prev addr    0x%" PRIx64 "\n\n", ri->nip);

This seems to be fixing two unrelated things:
 * escape sequences in the output (which we should indeed get rid of)
 * printing an uninitialized and unused prev_addr field
   (which we should fix and also drop the unneeded field from reginfo)

>      }
>
>      for (i = 0; i < 16; i++) {
> diff --git a/test_ppc64le.s b/test_ppc64le.s
> index 4321751..4af770c 100644
> --- a/test_ppc64le.s
> +++ b/test_ppc64le.s
> @@ -12,20 +12,18 @@
>   *****************************************************************************/
>
>  /* Initialise the gp regs */
> -li 0,0
> -li 1,1
> -li 2,2
> -li 3,3
> -li 4,4
> -li 5,5
> -li 6,6
> -li 7,7
> -li 8,8
> -li 9,9
> +li 0, 0
> +li 2, 2
> +li 3, 3
> +li 4, 4
> +li 5, 5
> +li 6, 6
> +li 7, 7
> +li 8, 8
> +li 9, 9

This appears to be unrelated changes to whitespace...

>  li 10, 10
>  li 11, 11
>  li 12, 12
> -li 13, 13
>  li 14, 14
>  li 15, 15
>  li 16, 16

thanks
-- PMM

Re: [Qemu-devel] [PATCH Risu 1/2] risu_ppc64: Fix Risu to run under qemu linux user

[joserz]

On Mon, Jan 30, 2017 at 11:49:34AM +0000, Peter Maydell wrote:
> On 30 January 2017 at 02:47, Jose Ricardo Ziviani
> <joserz@linux.vnet.ibm.com> wrote:
> > Qemu linux-user doesn't fill uc_mcontext completely like full emul. does.
> > For instance, uc->uc_mcontext.regs->nip is an invalid so this
> > commit replaces it by uc->uc_mcontext.gp_regs[PT_NIP]
> 
> It's not clear to me from this commit message whether this is
> a bug in QEMU's userspace emulation which this is trying to work
> around (in which case we should just fix it in QEMU), or a
> bug in risu where we were incorrectly relying on something the
> kernel doesn't actually guarantee. Which is it?
> 
> Also, looking at the kernel source and headers as far
> as I can see uc_context.regs is a pointer set up such that
> uc->uc_mcontext.regs->nip is pointing at the same bit of
> memory where uc->uc_mcontext.gp_regs[PT_NIP] is,
> and the QEMU code does similar, so I don't see how you can
> get two different values from the two things.
> 
> (It is certainly the case that risu is quite good at exercising
> odd corner cases of the signal handling code in QEMU which most
> normal programs don't care about...)

Peter

I just sent a patch "linux-user: fill target sigcontext struct accordingly" to fix it in QEMU. Please, forget this patchset, I'll reorganize it and send it later.

Thank you

Ziviani

[Qemu-devel] [PATCH Risu 2/2] risu_ppc64: Compare FPSCR flags

[Jose Ricardo Ziviani]

When running FP operations, FPSCR flag must be compared to make sure
that any exception will behave consistently.

Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
---
 risu_reginfo_ppc64le.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/risu_reginfo_ppc64le.c b/risu_reginfo_ppc64le.c
index 4dc509c..ee0e55e 100644
--- a/risu_reginfo_ppc64le.c
+++ b/risu_reginfo_ppc64le.c
@@ -21,6 +21,7 @@
 
 #define XER 37
 #define CCR 38
+#define FPREG 32
 
 /* reginfo_init: initialize with a ucontext */
 void reginfo_init(struct reginfo *ri, ucontext_t *uc)
@@ -82,6 +83,10 @@ int reginfo_is_eq(struct reginfo *m, struct reginfo *a, ucontext_t *uc)
         }
     }
 
+    if (m->fpregs[FPREG] != a->fpregs[FPREG]) {
+        return 0;
+    }
+
     for (i = 0; i < 32; i++) {
         if (m->vrregs.vrregs[i][0] != a->vrregs.vrregs[i][0] ||
                 m->vrregs.vrregs[i][1] != a->vrregs.vrregs[i][1] ||
-- 
2.7.4

Re: [Qemu-devel] [PATCH Risu 2/2] risu_ppc64: Compare FPSCR flags

[Peter Maydell]

On 30 January 2017 at 02:47, Jose Ricardo Ziviani
<joserz@linux.vnet.ibm.com> wrote:
> When running FP operations, FPSCR flag must be compared to make sure
> that any exception will behave consistently.
>
> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
> ---
>  risu_reginfo_ppc64le.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/risu_reginfo_ppc64le.c b/risu_reginfo_ppc64le.c
> index 4dc509c..ee0e55e 100644
> --- a/risu_reginfo_ppc64le.c
> +++ b/risu_reginfo_ppc64le.c
> @@ -21,6 +21,7 @@
>
>  #define XER 37
>  #define CCR 38
> +#define FPREG 32
>
>  /* reginfo_init: initialize with a ucontext */
>  void reginfo_init(struct reginfo *ri, ucontext_t *uc)
> @@ -82,6 +83,10 @@ int reginfo_is_eq(struct reginfo *m, struct reginfo *a, ucontext_t *uc)
>          }
>      }
>
> +    if (m->fpregs[FPREG] != a->fpregs[FPREG]) {
> +        return 0;
> +    }
> +
>      for (i = 0; i < 32; i++) {
>          if (m->vrregs.vrregs[i][0] != a->vrregs.vrregs[i][0] ||
>                  m->vrregs.vrregs[i][1] != a->vrregs.vrregs[i][1] ||
> --

Do we also need to add something to reginfo_dump_mismatch()
to print the mismatching FPSCR values ?

thanks
-- PMM