From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1L2Xvb-0000J6-Ss for qemu-devel@nongnu.org; Tue, 18 Nov 2008 16:16:07 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1L2XvZ-0000If-UM for qemu-devel@nongnu.org; Tue, 18 Nov 2008 16:16:07 -0500 Received: from [199.232.76.173] (port=51967 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1L2XvZ-0000Ic-Nu for qemu-devel@nongnu.org; Tue, 18 Nov 2008 16:16:05 -0500 Received: from ug-out-1314.google.com ([66.249.92.173]:50270) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1L2XvZ-0004Jp-H5 for qemu-devel@nongnu.org; Tue, 18 Nov 2008 16:16:05 -0500 Received: by ug-out-1314.google.com with SMTP id 29so360494ugc.36 for ; Tue, 18 Nov 2008 13:16:03 -0800 (PST) Message-ID: <4923308A.8070107@codemonkey.ws> Date: Tue, 18 Nov 2008 15:15:54 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v5 16/18] gdbstub: x86: Refactor register access References: <20081117161857.26880.45423.stgit@mchn012c.ww002.siemens.net> <20081117161859.26880.9437.stgit@mchn012c.ww002.siemens.net> In-Reply-To: <20081117161859.26880.9437.stgit@mchn012c.ww002.siemens.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org 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