Re: [Qemu-devel] [PATCH v5 16/18] gdbstub: x86: Refactor register access

[Anthony Liguori <anthony@codemonkey.ws>]

Jan Kiszka wrote:
>          return 10;
> -    } else if (n >= CPU_NB_REGS + 24) {
> -        n -= CPU_NB_REGS + 24;
> -        if (n < CPU_NB_REGS) {
> -            stq_p(mem_buf, env->xmm_regs[n].XMM_Q(0));
> -            stq_p(mem_buf + 8, env->xmm_regs[n].XMM_Q(1));
> -            return 16;
> -        } else if (n == CPU_NB_REGS) {
> -            GET_REG32(env->mxcsr);
> -        } 
> +    } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
> +        n -= IDX_XMM_REGS;
> +        stq_p(mem_buf, env->xmm_regs[n].XMM_Q(0));
> +        stq_p(mem_buf + 8, env->xmm_regs[n].XMM_Q(1));
> +        return 16;
>   

I think you have too much going on in this patch to review it properly.  
It's not immediately obvious to me that this change results in identical 
code.

>      } else {
> -        n -= CPU_NB_REGS;
>          switch (n) {
> -        case 0: GET_REGL(env->eip);
> -        case 1: GET_REG32(env->eflags);
> -        case 2: GET_REG32(env->segs[R_CS].selector);
> -        case 3: GET_REG32(env->segs[R_SS].selector);
> -        case 4: GET_REG32(env->segs[R_DS].selector);
> -        case 5: GET_REG32(env->segs[R_ES].selector);
> -        case 6: GET_REG32(env->segs[R_FS].selector);
> -        case 7: GET_REG32(env->segs[R_GS].selector);
> -        /* 8...15 x87 regs.  */
> -        case 16: GET_REG32(env->fpuc);
> -        case 17: GET_REG32((env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11);
> -        case 18: GET_REG32(0); /* ftag */
> -        case 19: GET_REG32(0); /* fiseg */
> -        case 20: GET_REG32(0); /* fioff */
> -        case 21: GET_REG32(0); /* foseg */
> -        case 22: GET_REG32(0); /* fooff */
> -        case 23: GET_REG32(0); /* fop */
> -        /* 24+ xmm regs.  */
> +        case IDX_IP_REG:    GET_REGL(env->eip);
> +        case IDX_FLAGS_REG: GET_REG32(env->eflags);
> +
> +        case IDX_SEG_REGS:     GET_REG32(env->segs[R_CS].selector);
> +        case IDX_SEG_REGS + 1: GET_REG32(env->segs[R_SS].selector);
> +        case IDX_SEG_REGS + 2: GET_REG32(env->segs[R_DS].selector);
> +        case IDX_SEG_REGS + 3: GET_REG32(env->segs[R_ES].selector);
> +        case IDX_SEG_REGS + 4: GET_REG32(env->segs[R_FS].selector);
> +        case IDX_SEG_REGS + 5: GET_REG32(env->segs[R_GS].selector);
> +
> +        case IDX_FP_REGS + 8:  GET_REG32(env->fpuc);
> +        case IDX_FP_REGS + 9:  GET_REG32((env->fpus & ~0x3800) |
> +                                         (env->fpstt & 0x7) << 11);
> +        case IDX_FP_REGS + 10: GET_REG32(0); /* ftag */
> +        case IDX_FP_REGS + 11: GET_REG32(0); /* fiseg */
> +        case IDX_FP_REGS + 12: GET_REG32(0); /* fioff */
> +        case IDX_FP_REGS + 13: GET_REG32(0); /* foseg */
> +        case IDX_FP_REGS + 14: GET_REG32(0); /* fooff */
> +        case IDX_FP_REGS + 15: GET_REG32(0); /* fop */
> +
> +        case IDX_MXCSR_REG: GET_REG32(env->mxcsr);
>          }
>      }
>      return 0;
>  }
>  
> -static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int i)
> +static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n)
>   

Is this rename really necessary?

>  #define LOAD_SEG(index, sreg)\
>              tmp = ldl_p(mem_buf);\
>              if (tmp != env->segs[sreg].selector)\
> -                cpu_x86_load_seg(env, sreg, tmp);
> +                cpu_x86_load_seg(env, sreg, tmp);\
> +            return 4
>  #else
>  /* FIXME: Honor segment registers.  Needs to avoid raising an exception
>     when the selector is invalid.  */
> -#define LOAD_SEG(index, sreg) do {} while(0)
> +#define LOAD_SEG(index, sreg) return 4
>  #endif
>   

I don't like the idea of hiding a return in a LOAD_SEG thing.  You would 
expect for these cases to fall through unless you look at the macro 
definition.

Code cleanup patches are good, but please try and split them in such a 
way that they are easier to review.  Things like variable renames or 
introductions of #define's should be completely mechanical.  If you want 
to reswizzle code, please separate that into a separate patch.  That 
keeps the later smaller which requires a more fine review.

Regards,

Anthony Liguori