Re: [Qemu-devel] [PATCH Risu 3/3] Initial implemention for ppc64le

[Peter Maydell]

On 28 October 2016 at 18:46, Jose Ricardo Ziviani
<address@hidden> wrote:
> From: Jose Ricardo Ziviani <address@hidden>
>
>  - This commit adds the initial implementation of ppc64le support
>    (client and server) for Risu.
>
> Signed-off-by: Jose Ricardo Ziviani <address@hidden>
> ---
>  configure              |   6 ++
>  risu_ppc64le.c         |  92 ++++++++++++++++++++++++++
>  risu_reginfo_ppc64le.c | 175 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  risu_reginfo_ppc64le.h |  36 ++++++++++
>  test_ppc64le.s         |  52 +++++++++++++++
>  5 files changed, 361 insertions(+)
>  create mode 100644 risu_ppc64le.c
>  create mode 100644 risu_reginfo_ppc64le.c
>  create mode 100644 risu_reginfo_ppc64le.h
>  create mode 100644 test_ppc64le.s
>
> diff --git a/configure b/configure
> index 748b48a..3e239f3 100755
> --- a/configure
> +++ b/configure
> @@ -22,6 +22,12 @@ guess_arch() {
>          ARCH="arm"
>      elif check_define __aarch64__ ; then
>          ARCH="aarch64"
> +    elif check_define __powerpc64__ ; then
> +        if check_define __BIG_ENDIAN__; then
> +            ARCH="ppc64"
> +        else
> +            ARCH="ppc64le"
> +        fi
>      else
>          echo "This cpu is not supported by risu. Try -h. " >&2
>          exit 1
> diff --git a/risu_ppc64le.c b/risu_ppc64le.c
> new file mode 100644
> index 0000000..811dd77
> --- /dev/null
> +++ b/risu_ppc64le.c
> @@ -0,0 +1,92 @@
> +/******************************************************************************
> + * Copyright (c) 2013 Linaro Limited

Wrong copyright info again.

> + * All rights reserved. This program and the accompanying materials
> + * are made available under the terms of the Eclipse Public License v1.0
> + * which accompanies this distribution, and is available at
> + * http://www.eclipse.org/legal/epl-v10.html
> + *
> + * Contributors:
> + *     Jose Ricardo Ziviani - initial implementation
> + *     based on Claudio Fontana's risu_aarch64.c
> + *     based on Peter Maydell's risu_arm.c
> + 
> *****************************************************************************/
> +
> +#include <stdio.h>
> +#include <ucontext.h>
> +#include <string.h>
> +
> +#include "risu.h"
> +#include "risu_reginfo_ppc64le.h"
> +
> +struct reginfo master_ri, apprentice_ri;
> +
> +void advance_pc(void *vuc)
> +{
> +    ucontext_t *uc = (ucontext_t*)vuc;
> +    uc->uc_mcontext.regs->nip += 4;
> +}
> +
> +int send_register_info(int sock, void *uc)
> +{
> +    struct reginfo ri;
> +    reginfo_init(&ri, uc);
> +    return send_data_pkt(sock, &ri, sizeof(ri));
> +}
> +
> +/* Read register info from the socket and compare it with that from the
> + * ucontext. Return 0 for match, 1 for end-of-test, 2 for mismatch.
> + * NB: called from a signal handler.
> + */
> +int recv_and_compare_register_info(int sock, void *uc)
> +{
> +    int resp = 0;
> +    reginfo_init(&master_ri, uc);
> +    if (recv_data_pkt(sock, &apprentice_ri, sizeof(apprentice_ri))) {
> +        printf("Packed mismatch\n");
> +        resp = 2;
> +
> +    } else if (!reginfo_is_eq(&master_ri, &apprentice_ri, uc))
> +    {
> +        /* mismatch */
> +        resp = 2;
> +    }
> +    else if (master_ri.faulting_insn == 0x5af1)
> +    {
> +        /* end of test */
> +        resp = 1;
> +    }
> +    else
> +    {
> +        /* either successful match or expected undef */
> +        resp = 0;
> +    }
> +    send_response_byte(sock, resp);
> +
> +    return resp;
> +}

This doesn't look right. You should be doing something to
get the "which risu op is this?" info out of the faulting
instruction and then handling it appropriately -- look at
the code in risu_aarch64.c. This is what lets us do memory
comparisons as well as register comparisons.


> --- /dev/null
> +++ b/risu_reginfo_ppc64le.c
> @@ -0,0 +1,175 @@
> +/******************************************************************************
> + * Copyright (c) 2013 Linaro Limited

Wrong copyright...

> + * All rights reserved. This program and the accompanying materials
> + * are made available under the terms of the Eclipse Public License v1.0
> + * which accompanies this distribution, and is available at
> + * http://www.eclipse.org/legal/epl-v10.html
> + *
> + * Contributors:
> + *     Jose Ricardo Ziviani - initial implementation
> + *     based on Claudio Fontana's risu_aarch64.c
> + *     based on Peter Maydell's risu_arm.c
> + 
> *****************************************************************************/
> +
> +#include <stdio.h>
> +#include <ucontext.h>
> +#include <string.h>
> +#include <math.h>
> +
> +#include "risu.h"
> +#include "risu_reginfo_ppc64le.h"
> +
> +#define XER 37
> +#define CCR 38
> +
> +/* reginfo_init: initialize with a ucontext */
> +void reginfo_init(struct reginfo *ri, ucontext_t *uc)
> +{
> +    ri->faulting_insn = *((uint32_t *)uc->uc_mcontext.regs->nip);
> +    ri->prev_insn = *((uint32_t *)(uc->uc_mcontext.regs->nip - 4));
> +    ri->prev_addr = uc->uc_mcontext.regs->nip - 4;

PC-type values should subtract image_start_address so that
they are kept as offsets from the start of the test code block.
Otherwise you get spurious mismatches when the master and
apprentice ends happen to allocate the code block at different
addresses.

Stack pointers also usually need some special handling.

> +
> +    int i;
> +    for (i = 0; i < NGREG; i++)
> +    {
> +        ri->gregs[i] = uc->uc_mcontext.gp_regs[i];
> +    }

I know the risu C code is a mess of different coding styles (my
fault primarily), but I think we should probably settle on using
the QEMU coding style here (four-space indent, always braces,
open-brace on same line as if/for, etc).

> +
> +    for (i = 0; i < NFPREG; i++)
> +    {
> +        ri->fpregs[i] = uc->uc_mcontext.fp_regs[i];
> +    }
> +
> +    for (i = 0; i < 32; i++)
> +    {
> +        ri->vrregs.vrregs[i][0] = uc->uc_mcontext.v_regs->vrregs[i][0];
> +        ri->vrregs.vrregs[i][1] = uc->uc_mcontext.v_regs->vrregs[i][1];
> +        ri->vrregs.vrregs[i][2] = uc->uc_mcontext.v_regs->vrregs[i][2];
> +        ri->vrregs.vrregs[i][3] = uc->uc_mcontext.v_regs->vrregs[i][3];
> +    }
> +    ri->vrregs.vscr = uc->uc_mcontext.v_regs->vscr;
> +    ri->vrregs.vrsave = uc->uc_mcontext.v_regs->vrsave;
> +}
> +
> +/* reginfo_is_eq: compare the reginfo structs, returns nonzero if equal */
> +int reginfo_is_eq(struct reginfo *master, struct reginfo *apprentice, 
> ucontext_t *uc)
> +{
> +    int i;
> +    for (i = 0; i < 32; i++)
> +    {
> +        if (i == 1 || i == 13)
> +            continue;
> +
> +        if (master->gregs[i] != apprentice->gregs[i])
> +        {
> +            fprintf(stderr, "\e[1;32mMismatch: \e[0mRegister r%d\n", i);

Please don't get fancy with escape sequences in the output.
If we want that as a feature we should add it separately from
PPC support and work out how to do it cleanly for all supported
target architectures.

thanks
-- PMM