qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH v5 16/18] gdbstub: x86: Refactor register access
Date: Tue, 18 Nov 2008 15:15:54 -0600
User-agent: Thunderbird 2.0.0.17 (X11/20080925)

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]