From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=55605 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q4DqG-0005qS-4c for qemu-devel@nongnu.org; Mon, 28 Mar 2011 10:54:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q4DqE-0002L0-Bl for qemu-devel@nongnu.org; Mon, 28 Mar 2011 10:54:51 -0400 Received: from mail-vx0-f173.google.com ([209.85.220.173]:43317) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q4DqE-0002Kf-9V for qemu-devel@nongnu.org; Mon, 28 Mar 2011 10:54:50 -0400 Received: by vxb41 with SMTP id 41so2669775vxb.4 for ; Mon, 28 Mar 2011 07:54:49 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1300982333-12802-13-git-send-email-agraf@suse.de> References: <1300982333-12802-1-git-send-email-agraf@suse.de> <1300982333-12802-13-git-send-email-agraf@suse.de> Date: Mon, 28 Mar 2011 15:54:49 +0100 Message-ID: Subject: Re: [Qemu-devel] [PATCH 12/17] s390x: Prepare cpu.h for emulation From: Peter Maydell Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: QEMU-devel Developers , Aurelien Jarno , Richard Henderson On 24 March 2011 15:58, Alexander Graf wrote: > diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h Minor nits only. > - =C2=A0 =C2=A0FPReg fregs[16]; /* FP registers */ > + =C2=A0 =C2=A0CPU_DoubleU fregs[16]; /* FP registers */ These changes mean that the FPReg typedef in this file is no longer used, so you might as well delete it. Personally I prefer the way target-arm handles float regs, ie it just has 'float64 regs[32]' and relies on them being the right representation to pass in registers. This is less likely to work with float128s though, and anyway I suspect Nathan would disagree with me, so this isn't a request to change this code. > +#define EXCP_EXT =C2=A01 > + > +#define EXCP_SVC 2 /* supervisor call (syscall) */ > +#define EXCP_PGM 3 /* program interruption */ > +/* XXX */ > +#define EXCP_EXECUTE_SVC 0xff00000 /* supervisor call via execute insn *= / This comment ought to have an explanation of what the issue is that means it's 'XXX'... > + =C2=A0 =C2=A0CC_OP_ADD_64, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 /* */ > + =C2=A0 =C2=A0CC_OP_ADDU_64, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0/* */ > + =C2=A0 =C2=A0CC_OP_SUB_64, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 /* */ > + =C2=A0 =C2=A0CC_OP_SUBU_64, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0/* */ > + =C2=A0 =C2=A0CC_OP_ABS_64, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 /* */ > + =C2=A0 =C2=A0CC_OP_NABS_64, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0/* */ Why the empty comments? > +static inline uint64_t time2tod(uint64_t time) { > + =C2=A0 =C2=A0return (time << 9) / 125; > +} Could maybe use a comment about what units we're converting to and from here. -- PMM