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
<address@hidden> 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 <address@hidden>
> ---
>  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