From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1L2Zka-0003qv-Co for qemu-devel@nongnu.org; Tue, 18 Nov 2008 18:12:52 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1L2ZkZ-0003qQ-Po for qemu-devel@nongnu.org; Tue, 18 Nov 2008 18:12:52 -0500 Received: from [199.232.76.173] (port=43545 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1L2ZkZ-0003qM-Jp for qemu-devel@nongnu.org; Tue, 18 Nov 2008 18:12:51 -0500 Received: from fmmailgate01.web.de ([217.72.192.221]:56416) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1L2ZkZ-0007Qn-Hi for qemu-devel@nongnu.org; Tue, 18 Nov 2008 18:12:52 -0500 Received: from smtp05.web.de (fmsmtp05.dlan.cinetic.de [172.20.4.166]) by fmmailgate01.web.de (Postfix) with ESMTP id 66365F9B0A5F for ; Wed, 19 Nov 2008 00:12:50 +0100 (CET) Received: from [88.64.11.56] (helo=[192.168.1.198]) by smtp05.web.de with asmtp (TLSv1:AES256-SHA:256) (WEB.DE 4.109 #226) id 1L2ZkY-0005j4-00 for qemu-devel@nongnu.org; Wed, 19 Nov 2008 00:12:50 +0100 Message-ID: <49234BC4.4040408@web.de> Date: Wed, 19 Nov 2008 00:12:04 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <20081117161857.26880.45423.stgit@mchn012c.ww002.siemens.net> <20081117161859.26880.9437.stgit@mchn012c.ww002.siemens.net> <4923308A.8070107@codemonkey.ws> In-Reply-To: <4923308A.8070107@codemonkey.ws> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigEE133FA8292F21012F1E5AA7" Sender: jan.kiszka@web.de Subject: [Qemu-devel] Re: [PATCH v5 16/18] gdbstub: x86: Refactor register access 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 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigEE133FA8292F21012F1E5AA7 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Anthony Liguori wrote: > Jan Kiszka wrote: >> return 10; >> - } else if (n >=3D CPU_NB_REGS + 24) { >> - n -=3D 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 =3D=3D CPU_NB_REGS) { >> - GET_REG32(env->mxcsr); >> - } + } else if (n >=3D IDX_XMM_REGS && n < IDX_XMM_REGS + >> CPU_NB_REGS) { >> + n -=3D 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; >> =20 >=20 > I think you have too much going on in this patch to review it properly.= =20 > It's not immediately obvious to me that this change results in identica= l > code. I would say this is not only because of the changes... >=20 >> } else { >> - n -=3D 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; >> } >> =20 >> -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) >> =20 >=20 > Is this rename really necessary? It improves consistency of this code, readability ("Why was it called 'i' here, but 'n' above?"). >=20 >> #define LOAD_SEG(index, sreg)\ >> tmp =3D ldl_p(mem_buf);\ >> if (tmp !=3D 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 excepti= on >> when the selector is invalid. */ >> -#define LOAD_SEG(index, sreg) do {} while(0) >> +#define LOAD_SEG(index, sreg) return 4 >> #endif >> =20 >=20 > I don't like the idea of hiding a return in a LOAD_SEG thing. You woul= d > expect for these cases to fall through unless you look at the macro > definition. The macro fortunately dies with the next patch. So you may argue I should leave that part alone and simply remove it later... BTW, there are more of such macros. Changing them would have caused more churn than I felt like I should cause. >=20 > 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 wan= t > to reswizzle code, please separate that into a separate patch. That > keeps the later smaller which requires a more fine review. Well, this is always a trade-off: Leave the code as-is, just add the functionality that I need right now, or also attempt to clean up what caused confusing or nagged me otherwise. But the latter can easily cost much more than the former. Up to a certain point I agree with your aim, but up from a certain point of split up I would have to fall back to the first approach due to time constraints. So please let me know what you think this one should be split up into. Jan --------------enigEE133FA8292F21012F1E5AA7 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iEYEARECAAYFAkkjS8QACgkQniDOoMHTA+mB0ACePH+tnS3OR9s0NIJM/Q6tf7p+ SWUAnRYFpLkX/c0tr3dXpygXfANTAeEt =XSu8 -----END PGP SIGNATURE----- --------------enigEE133FA8292F21012F1E5AA7--