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

[Fabiano Rosas]

David Gibson <address@hidden> writes:

> 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.

GDB detects the endianness by looking at the ELF headers:

(gdb) p/x $pvr
$1 = 0x1024b0000000000
(gdb) file ~/qemu/roms/SLOF/board-qemu/llfw/stage1.elf
Reading symbols from ~/qemu/roms/SLOF/board-qemu/llfw/stage1.elf...done.
(gdb) show endian
The target endianness is set automatically (currently big endian)
(gdb) p/x $pvr
$2 = 0x4b0201
(gdb) c
Continuing.

(gdb) ^C
Program received signal SIGINT, Interrupt.
0x74a70c00000000c0 in ?? ()
(gdb) file vmlinux
Reading symbols from vmlinux...done.
(gdb) show endian
The target endianness is set automatically (currently little endian)
(gdb) p/x $pvr
$3 = 0x4b0201

The maybe_bswap_register is done due to QEMU having TARGET_WORDS_BIGENDIAN set
even after we have changed into LE mode.

>> 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);
>> > 
>>