Re: [Qemu-devel] [PATCH 005/126] target-s390: Fix gdbstub

[Alexander Graf <agraf@suse.de>]

On 09/09/2012 11:04 PM, Richard Henderson wrote:
> The real gdb protocol doesn't split out pc or cc as real registers.
> Those are pseudos that are extracted as needed from the PSW.  Don't
> modify env->cc_op during read -- that way lies heisenbugs.
>
> Fill in the XXX for the fp registers.
>
> Remove duplicated defines in cpu.h.
>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>   gdbstub.c          | 78 +++++++++++++++++++++++++++++++++---------------------
>   target-s390x/cpu.h | 73 --------------------------------------------------
>   2 files changed, 48 insertions(+), 103 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 5d37dd9..6aed0b4 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1499,27 +1499,35 @@ static int cpu_gdb_write_register(CPUAlphaState *env, uint8_t *mem_buf, int n)
>   }
>   #elif defined (TARGET_S390X)
>   
> -#define NUM_CORE_REGS S390_NUM_TOTAL_REGS
> +#define NUM_CORE_REGS  S390_NUM_REGS
>   
>   static int cpu_gdb_read_register(CPUS390XState *env, uint8_t *mem_buf, int n)
>   {
> +    uint64_t val;
> +    int cc_op;
> +
>       switch (n) {
> -        case S390_PSWM_REGNUM: GET_REGL(env->psw.mask); break;
> -        case S390_PSWA_REGNUM: GET_REGL(env->psw.addr); break;
> -        case S390_R0_REGNUM ... S390_R15_REGNUM:
> -            GET_REGL(env->regs[n-S390_R0_REGNUM]); break;
> -        case S390_A0_REGNUM ... S390_A15_REGNUM:
> -            GET_REG32(env->aregs[n-S390_A0_REGNUM]); break;
> -        case S390_FPC_REGNUM: GET_REG32(env->fpc); break;
> -        case S390_F0_REGNUM ... S390_F15_REGNUM:
> -            /* XXX */
> -            break;
> -        case S390_PC_REGNUM: GET_REGL(env->psw.addr); break;
> -        case S390_CC_REGNUM:
> -            env->cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst,
> -                                 env->cc_vr);
> -            GET_REG32(env->cc_op);
> -            break;
> +    case S390_PSWM_REGNUM:
> +        val = env->psw.mask & ~(3ull << 13);
> +        cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst, env->cc_vr);
> +        val |= cc_op << 13;
> +        GET_REGL(val);
> +        break;
> +    case S390_PSWA_REGNUM:
> +        GET_REGL(env->psw.addr);
> +        break;
> +    case S390_R0_REGNUM ... S390_R15_REGNUM:
> +        GET_REGL(env->regs[n-S390_R0_REGNUM]);
> +        break;
> +    case S390_A0_REGNUM ... S390_A15_REGNUM:
> +        GET_REG32(env->aregs[n-S390_A0_REGNUM]);
> +        break;
> +    case S390_FPC_REGNUM:
> +        GET_REG32(env->fpc);
> +        break;
> +    case S390_F0_REGNUM ... S390_F15_REGNUM:
> +        GET_REG64(env->fregs[n-S390_F0_REGNUM].ll);
> +        break;
>       }
>   
>       return 0;
> @@ -1534,20 +1542,30 @@ static int cpu_gdb_write_register(CPUS390XState *env, uint8_t *mem_buf, int n)
>       tmp32 = ldl_p(mem_buf);
>   
>       switch (n) {
> -        case S390_PSWM_REGNUM: env->psw.mask = tmpl; break;
> -        case S390_PSWA_REGNUM: env->psw.addr = tmpl; break;
> -        case S390_R0_REGNUM ... S390_R15_REGNUM:
> -            env->regs[n-S390_R0_REGNUM] = tmpl; break;
> -        case S390_A0_REGNUM ... S390_A15_REGNUM:
> -            env->aregs[n-S390_A0_REGNUM] = tmp32; r=4; break;
> -        case S390_FPC_REGNUM: env->fpc = tmp32; r=4; break;
> -        case S390_F0_REGNUM ... S390_F15_REGNUM:
> -            /* XXX */
> -            break;
> -        case S390_PC_REGNUM: env->psw.addr = tmpl; break;
> -        case S390_CC_REGNUM: env->cc_op = tmp32; r=4; break;
> +    case S390_PSWM_REGNUM:
> +        env->psw.mask = tmpl;
> +        env->cc_op = (tmpl >> 13) & 3;

Are you sure this is correct? I thought gdbstub would just ignore the cc 
bits.


Alex

> +        break;
> +    case S390_PSWA_REGNUM:
> +        env->psw.addr = tmpl;
> +        break;
> +    case S390_R0_REGNUM ... S390_R15_REGNUM:
> +        env->regs[n-S390_R0_REGNUM] = tmpl;
> +        break;
> +    case S390_A0_REGNUM ... S390_A15_REGNUM:
> +        env->aregs[n-S390_A0_REGNUM] = tmp32;
> +        r = 4;
> +        break;
> +    case S390_FPC_REGNUM:
> +        env->fpc = tmp32;
> +        r = 4;
> +        break;
> +    case S390_F0_REGNUM ... S390_F15_REGNUM:
> +        env->fregs[n-S390_F0_REGNUM].ll = tmpl;
> +        break;
> +    default:
> +        return 0;
>       }
> -
>       return r;
>   }
>   #elif defined (TARGET_LM32)
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index ed81af3..471fb91 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -430,79 +430,6 @@ static inline void cpu_set_tls(CPUS390XState *env, target_ulong newtls)
>   /* Total.  */
>   #define S390_NUM_REGS 51
>   
> -/* Pseudo registers -- PC and condition code.  */
> -#define S390_PC_REGNUM S390_NUM_REGS
> -#define S390_CC_REGNUM (S390_NUM_REGS+1)
> -#define S390_NUM_PSEUDO_REGS 2
> -#define S390_NUM_TOTAL_REGS (S390_NUM_REGS+2)
> -
> -
> -
> -/* Program Status Word.  */
> -#define S390_PSWM_REGNUM 0
> -#define S390_PSWA_REGNUM 1
> -/* General Purpose Registers.  */
> -#define S390_R0_REGNUM 2
> -#define S390_R1_REGNUM 3
> -#define S390_R2_REGNUM 4
> -#define S390_R3_REGNUM 5
> -#define S390_R4_REGNUM 6
> -#define S390_R5_REGNUM 7
> -#define S390_R6_REGNUM 8
> -#define S390_R7_REGNUM 9
> -#define S390_R8_REGNUM 10
> -#define S390_R9_REGNUM 11
> -#define S390_R10_REGNUM 12
> -#define S390_R11_REGNUM 13
> -#define S390_R12_REGNUM 14
> -#define S390_R13_REGNUM 15
> -#define S390_R14_REGNUM 16
> -#define S390_R15_REGNUM 17
> -/* Access Registers.  */
> -#define S390_A0_REGNUM 18
> -#define S390_A1_REGNUM 19
> -#define S390_A2_REGNUM 20
> -#define S390_A3_REGNUM 21
> -#define S390_A4_REGNUM 22
> -#define S390_A5_REGNUM 23
> -#define S390_A6_REGNUM 24
> -#define S390_A7_REGNUM 25
> -#define S390_A8_REGNUM 26
> -#define S390_A9_REGNUM 27
> -#define S390_A10_REGNUM 28
> -#define S390_A11_REGNUM 29
> -#define S390_A12_REGNUM 30
> -#define S390_A13_REGNUM 31
> -#define S390_A14_REGNUM 32
> -#define S390_A15_REGNUM 33
> -/* Floating Point Control Word.  */
> -#define S390_FPC_REGNUM 34
> -/* Floating Point Registers.  */
> -#define S390_F0_REGNUM 35
> -#define S390_F1_REGNUM 36
> -#define S390_F2_REGNUM 37
> -#define S390_F3_REGNUM 38
> -#define S390_F4_REGNUM 39
> -#define S390_F5_REGNUM 40
> -#define S390_F6_REGNUM 41
> -#define S390_F7_REGNUM 42
> -#define S390_F8_REGNUM 43
> -#define S390_F9_REGNUM 44
> -#define S390_F10_REGNUM 45
> -#define S390_F11_REGNUM 46
> -#define S390_F12_REGNUM 47
> -#define S390_F13_REGNUM 48
> -#define S390_F14_REGNUM 49
> -#define S390_F15_REGNUM 50
> -/* Total.  */
> -#define S390_NUM_REGS 51
> -
> -/* Pseudo registers -- PC and condition code.  */
> -#define S390_PC_REGNUM S390_NUM_REGS
> -#define S390_CC_REGNUM (S390_NUM_REGS+1)
> -#define S390_NUM_PSEUDO_REGS 2
> -#define S390_NUM_TOTAL_REGS (S390_NUM_REGS+2)
> -

Hrm. Mind to quickly explain why it's ok to remove these? Where to they 
come from now? Or were they defined twice in cpu.h?

Alex

>   /* CC optimization */
>   
>   enum cc_op {