Re: [Qemu-devel] [RISU PATCH v5 08/13] risu: add header to trace stream

[Peter Maydell]

On 19 June 2017 at 11:46, Alex Bennée <address@hidden> wrote:
> I've also added a header packet with pc/risu op in it so we can keep
> better track of how things are going.
>
> Signed-off-by: Alex Bennée <address@hidden>
>
> ---
> v5
>   - re-base without formatting fixes
>   - dropped fprintfs in signal context
> v4
>   - split from previous patch
> ---
>  reginfo.c      | 94 
> +++++++++++++++++++++++++++++++++++++---------------------
>  risu.h         |  9 ++++++
>  risu_aarch64.c |  5 ++++
>  risu_arm.c     |  5 ++++
>  risu_m68k.c    |  5 ++++
>  risu_ppc64.c   |  5 ++++
>  6 files changed, 89 insertions(+), 34 deletions(-)
>
> diff --git a/reginfo.c b/reginfo.c
> index 90cea8f..4ff937f 100644
> --- a/reginfo.c
> +++ b/reginfo.c
> @@ -24,10 +24,19 @@ static int packet_mismatch;
>  int send_register_info(write_fn write_fn, void *uc)
>  {
>      struct reginfo ri;
> +    trace_header_t header;
>      int op;
> +
>      reginfo_init(&ri, uc);
>      op = get_risuop(&ri);
>
> +    /* Write a header with PC/op to keep in sync */
> +    header.pc = get_pc(&ri);
> +    header.risu_op = op;
> +    if (write_fn(&header, sizeof(header)) != 0) {
> +        return -1;
> +    }
> +
>      switch (op) {
>      case OP_TESTEND:
>          write_fn(&ri, sizeof(ri));
> @@ -63,46 +72,63 @@ int send_register_info(write_fn write_fn, void *uc)
>  int recv_and_compare_register_info(read_fn read_fn, respond_fn resp_fn, void 
> *uc)
>  {
>      int resp = 0, op;
> +    trace_header_t header;
>
>      reginfo_init(&master_ri, uc);
>      op = get_risuop(&master_ri);
>
> -    switch (op) {
> -    case OP_COMPARE:
> -    case OP_TESTEND:
> -    default:
> -        /* Do a simple register compare on (a) explicit request
> -         * (b) end of test (c) a non-risuop UNDEF
> -         */
> -        if (read_fn(&apprentice_ri, sizeof(apprentice_ri))) {
> -            packet_mismatch = 1;
> -            resp = 2;
> -        } else if (!reginfo_is_eq(&master_ri, &apprentice_ri)) {
> -            /* register mismatch */
> -            resp = 2;
> -        } else if (op == OP_TESTEND) {
> -            resp = 1;
> -        }
> -        resp_fn(resp);
> -        break;
> -    case OP_SETMEMBLOCK:
> -        memblock = (void *)(uintptr_t)get_reginfo_paramreg(&master_ri);
> -        break;
> -    case OP_GETMEMBLOCK:
> -        set_ucontext_paramreg(uc, get_reginfo_paramreg(&master_ri) +
> -                              (uintptr_t)memblock);
> -        break;
> -    case OP_COMPAREMEM:
> -        mem_used = 1;
> -        if (read_fn(apprentice_memblock, MEMBLOCKLEN)) {
> -            packet_mismatch = 1;
> -            resp = 2;
> -        } else if (memcmp(memblock, apprentice_memblock, MEMBLOCKLEN) != 0) {
> -            /* memory mismatch */
> -            resp = 2;
> +    if (read_fn(&header, sizeof(header)) != 0) {
> +        return -1;
> +    }
> +
> +    if (header.risu_op == op ) {

Stray extra space.

> +
> +        /* send OK for the header */
> +        resp_fn(0);
> +
> +        switch (op) {
> +        case OP_COMPARE:
> +        case OP_TESTEND:
> +        default:
> +            /* Do a simple register compare on (a) explicit request
> +             * (b) end of test (c) a non-risuop UNDEF
> +             */
> +            if (read_fn(&apprentice_ri, sizeof(apprentice_ri))) {
> +                packet_mismatch = 1;
> +                resp = 2;
> +
> +            } else if (!reginfo_is_eq(&master_ri, &apprentice_ri)) {
> +                /* register mismatch */
> +                resp = 2;
> +
> +            } else if (op == OP_TESTEND) {
> +                resp = 1;
> +            }
> +            resp_fn(resp);
> +            break;
> +        case OP_SETMEMBLOCK:
> +            memblock = (void *)(uintptr_t)get_reginfo_paramreg(&master_ri);
> +            break;
> +        case OP_GETMEMBLOCK:
> +            set_ucontext_paramreg(uc, get_reginfo_paramreg(&master_ri) +
> +                                  (uintptr_t)memblock);
> +            break;
> +        case OP_COMPAREMEM:
> +            mem_used = 1;
> +            if (read_fn(apprentice_memblock, MEMBLOCKLEN)) {
> +                packet_mismatch = 1;
> +                resp = 2;
> +            } else if (memcmp(memblock, apprentice_memblock, MEMBLOCKLEN) != 
> 0) {
> +                /* memory mismatch */
> +                resp = 2;
> +            }
> +            resp_fn(resp);
> +            break;
>          }
> +    } else {
> +        /* We are out of sync */
> +        resp = 2;
>          resp_fn(resp);
> -        break;

This is probably easier to read if structured the other way
up, so:

     if (header.risu_op != op) {
        /* out of sync */
        ...
        return resp;
     }

and then you can leave the rest of the function alone.


>      }
>
>      return resp;
> diff --git a/risu.h b/risu.h
> index 32241bc..e9e70ab 100644
> --- a/risu.h
> +++ b/risu.h
> @@ -51,6 +51,12 @@ extern int ismaster;
>
>  struct reginfo;
>
> +typedef struct
> +{
> +   uintptr_t pc;
> +   uint32_t risu_op;
> +} trace_header_t;
> +
>  /* Functions operating on reginfo */
>
>  /* To keep the read/write logic from multiplying across all arches
> @@ -102,6 +108,9 @@ uint64_t get_reginfo_paramreg(struct reginfo *ri);
>   */
>  int get_risuop(struct reginfo *ri);
>
> +/* Return the PC from a reginfo */
> +uintptr_t get_pc(struct reginfo *ri);
> +
>  /* initialize structure from a ucontext */
>  void reginfo_init(struct reginfo *ri, ucontext_t *uc);
>
> diff --git a/risu_aarch64.c b/risu_aarch64.c
> index 9c6809d..492d141 100644
> --- a/risu_aarch64.c
> +++ b/risu_aarch64.c
> @@ -40,3 +40,8 @@ int get_risuop(struct reginfo *ri)
>      uint32_t risukey = 0x00005af0;
>      return (key != risukey) ? -1 : op;
>  }
> +
> +uintptr_t get_pc(struct reginfo *ri)
> +{
> +   return ri->pc;

Indent here (and in some of the other versions of this
function) isn't matching the standard set up in earlier patches.

> +}
> diff --git a/risu_arm.c b/risu_arm.c
> index a55c6c2..5fcb2a5 100644
> --- a/risu_arm.c
> +++ b/risu_arm.c
> @@ -68,3 +68,8 @@ int get_risuop(struct reginfo *ri)
>      uint32_t risukey = (isz == 2) ? 0xdee0 : 0xe7fe5af0;
>      return (key != risukey) ? -1 : op;
>  }
> +
> +uintptr_t get_pc(struct reginfo *ri)
> +{
> +   return ri->gpreg[15];
> +}
> diff --git a/risu_m68k.c b/risu_m68k.c
> index 0bf5c14..1056eef 100644
> --- a/risu_m68k.c
> +++ b/risu_m68k.c
> @@ -33,3 +33,8 @@ int get_risuop(struct reginfo *ri)
>      uint32_t risukey = 0x4afc7000;
>      return (key != risukey) ? -1 : op;
>  }
> +
> +uintptr_t get_pc(struct reginfo *ri)
> +{
> +    return ri->gregs[R_PC];
> +}
> diff --git a/risu_ppc64.c b/risu_ppc64.c
> index eb60573..83f8d1f 100644
> --- a/risu_ppc64.c
> +++ b/risu_ppc64.c
> @@ -38,3 +38,8 @@ int get_risuop(struct reginfo *ri)
>      uint32_t risukey = 0x00005af0;
>      return (key != risukey) ? -1 : op;
>  }
> +
> +uintptr_t get_pc(struct reginfo *ri)
> +{
> +   return ri->nip;
> +}
> --

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM