Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 2/3] target/ppc: Add GDB callbacks for SPRs

[David Gibson]

On Thu, Jan 24, 2019 at 06:20:02PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 23/01/2019 04:01, Fabiano Rosas wrote:
> > These will be used to let GDB know about PPC's Special Purpose
> > Registers (SPR).
> > 
> > They take an index based on the order the registers appear in the XML
> > file sent by QEMU to GDB. This index does not match the actual
> > location of the registers in the env->spr array so the
> > gdb_find_spr_idx function does that conversion.
> > 
> > Signed-off-by: Fabiano Rosas <address@hidden>
> > ---
> >  target/ppc/translate_init.inc.c | 54 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/ppc/translate_init.inc.c 
> > b/target/ppc/translate_init.inc.c
> > index 710064a25d..f29ac3558a 100644
> > --- a/target/ppc/translate_init.inc.c
> > +++ b/target/ppc/translate_init.inc.c
> > @@ -9487,6 +9487,55 @@ static bool avr_need_swap(CPUPPCState *env)
> >  #endif
> >  }
> >  
> > +#if !defined(CONFIG_USER_ONLY)
> > +static int gdb_find_spr_idx(CPUPPCState *env, int n)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
> > +        ppc_spr_t *spr = &env->spr_cb[i];
> > +
> > +        if (spr->name && spr->gdb_id == n) {
> > +            return i;
> > +        }
> > +    }
> > +    return -1;
> > +}
> > +
> > +static int gdb_get_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> > +{
> > +    int reg;
> > +    int len;
> > +
> > +    reg = gdb_find_spr_idx(env, n);
> > +    if (reg < 0) {
> > +        return 0;
> > +    }
> > +
> > +    len = TARGET_LONG_SIZE;
> > +    stn_p(mem_buf, len, env->spr[reg]);
> > +    ppc_maybe_bswap_register(env, mem_buf, len);
> 
> 
> I am confused by this as it produces different results depending on the
> guest mode:


Hm, yeah, I thought the bswap here looked odd, but it wasn't obvious
to me if it was bogus here, or just a bogus gdb interface we have to
work around.

> (gdb) p $pvr
> $1 = 0x14c0000000000
> (gdb) c
> Continuing.
> Program received signal SIGINT, Interrupt.
> (gdb) p $pvr
> $2 = 0x4c0100

But that behaviour definitely looks wrong.

> First print is when I stopped the guest in the SLOF firmware (so it is
> big-endian) and then I continued and stopped gdb when the guest booted a
> little-endian system; the KVM host is little endian, the machine running
> gdb is LE too.
> 
> QEMU monitor prints the same 0x4c0100 in both cases.
> 
> I am adding the inventor of maybe_bswap_register() in cc: for
> assistance. Swapping happens:
> - once for BE: after stn_p()
>       *(unsigned long *)mem_buf is 0x14c0000000000
> - twice for LE.
> 
> 
> 
> 
> 
> > +    return len;
> > +}
> > +
> > +static int gdb_set_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> > +{
> > +    int reg;
> > +    int len;
> > +
> > +    reg = gdb_find_spr_idx(env, n);
> > +    if (reg < 0) {
> > +        return 0;
> > +    }
> > +
> > +    len = TARGET_LONG_SIZE;
> > +    ppc_maybe_bswap_register(env, mem_buf, len);
> > +    env->spr[reg] = ldn_p(mem_buf, len);
> > +
> > +    return len;
> > +}
> > +#endif
> > +
> >  static int gdb_get_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> >  {
> >      if (n < 32) {
> > @@ -9716,7 +9765,10 @@ static void ppc_cpu_realize(DeviceState *dev, Error 
> > **errp)
> >          gdb_register_coprocessor(cs, gdb_get_vsx_reg, gdb_set_vsx_reg,
> >                                   32, "power-vsx.xml", 0);
> >      }
> > -
> > +#ifndef CONFIG_USER_ONLY
> > +    gdb_register_coprocessor(cs, gdb_get_spr_reg, gdb_set_spr_reg,
> > +                             pcc->gdb_num_sprs, "power-spr.xml", 0);
> > +#endif
> >      qemu_init_vcpu(cs);
> >  
> >      pcc->parent_realize(dev, errp);
> > 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature