From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52404) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wuo40-0007QS-4q for qemu-devel@nongnu.org; Wed, 11 Jun 2014 15:20:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wuo3t-0008Hy-UC for qemu-devel@nongnu.org; Wed, 11 Jun 2014 15:20:00 -0400 Received: from mail-qa0-f50.google.com ([209.85.216.50]:37480) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wuo3t-0008Hl-Nr for qemu-devel@nongnu.org; Wed, 11 Jun 2014 15:19:53 -0400 Received: by mail-qa0-f50.google.com with SMTP id m5so280050qaj.23 for ; Wed, 11 Jun 2014 12:19:52 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1402326269-8573-14-git-send-email-edgar.iglesias@gmail.com> References: <1402326269-8573-1-git-send-email-edgar.iglesias@gmail.com> <1402326269-8573-14-git-send-email-edgar.iglesias@gmail.com> Date: Wed, 11 Jun 2014 14:19:52 -0500 Message-ID: From: Greg Bellows Content-Type: multipart/alternative; boundary=001a11c2df9e2cee9504fb9454cd Subject: Re: [Qemu-devel] [PATCH v2 13/17] target-arm: Use uint16_t in syndrome generators with 16bit imms List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" Cc: Peter Maydell , Peter Crosthwaite , Rob Herring , Fabian Aggeler , QEMU Developers , Alexander Graf , Blue Swirl , John Williams , pbonzini@redhat.com, =?UTF-8?B?QWxleCBCZW5uw6ll?= , Christoffer Dall , rth@twiddle.net --001a11c2df9e2cee9504fb9454cd Content-Type: text/plain; charset=UTF-8 Called out possibly missing fix below. Beside it, I'm convinced this change is beneficial, other than maybe for readability. The change does not account for uses of the affected calls in target-arm/translate.c. As well, the change has somewhat a ripple effect as certain code expects to receive or return a 32-bit value. Unless there is a functional benefit, my suggestion would be to omit this change or submit it separately as it is not really related or required for your EL2/EL3 changes. Greg On 9 June 2014 10:04, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" > > Avoids the explicit 16bit mask. No functional change. > > Signed-off-by: Edgar E. Iglesias > --- > target-arm/internals.h | 14 +++++++------- > target-arm/translate-a64.c | 2 +- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/target-arm/internals.h b/target-arm/internals.h > index 08fa697..707643e 100644 > --- a/target-arm/internals.h > +++ b/target-arm/internals.h > @@ -199,23 +199,23 @@ static inline uint32_t syn_uncategorized(void) > return (EC_UNCATEGORIZED << ARM_EL_EC_SHIFT) | ARM_EL_IL; > } > > -static inline uint32_t syn_aa64_svc(uint32_t imm16) > +static inline uint32_t syn_aa64_svc(uint16_t imm16) > { > - return (EC_AA64_SVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & > 0xffff); > + return (EC_AA64_SVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | imm16; > } > > -static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb) > +static inline uint32_t syn_aa32_svc(uint16_t imm16, bool is_thumb) > { > - return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | (imm16 & 0xffff) > + return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | imm16 > | (is_thumb ? 0 : ARM_EL_IL); > } > > -static inline uint32_t syn_aa64_bkpt(uint32_t imm16) > +static inline uint32_t syn_aa64_bkpt(uint16_t imm16) > { > - return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & > 0xffff); > + return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | imm16; > } > > -static inline uint32_t syn_aa32_bkpt(uint32_t imm16, bool is_thumb) > +static inline uint32_t syn_aa32_bkpt(uint16_t imm16, bool is_thumb) > { > return (EC_AA32_BKPT << ARM_EL_EC_SHIFT) | (imm16 & 0xffff) > | (is_thumb ? 0 : ARM_EL_IL); > Looks like you forgot to remove the mask on this one. > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c > index a9c4633..3589898 100644 > --- a/target-arm/translate-a64.c > +++ b/target-arm/translate-a64.c > @@ -1433,7 +1433,7 @@ static void disas_exc(DisasContext *s, uint32_t insn) > { > int opc = extract32(insn, 21, 3); > int op2_ll = extract32(insn, 0, 5); > - int imm16 = extract32(insn, 5, 16); > + uint16_t imm16 = extract32(insn, 5, 16); > > switch (opc) { > case 0: > -- > 1.8.3.2 > > --001a11c2df9e2cee9504fb9454cd Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Called out possibly missing fix below. =C2=A0Beside it, I&= #39;m convinced this change is beneficial, other than maybe for readability= . =C2=A0=C2=A0

The change does not account for uses of t= he affected calls in target-arm/translate.c. =C2=A0As well, the change has = somewhat a ripple effect as certain code expects to receive or return a 32-= bit value.

Unless there is a functional benefit, my suggestion wou= ld be to omit this change or submit it separately as it is not really relat= ed or required for your =C2=A0EL2/EL3 changes. =C2=A0

<= div>Greg


On 9 June 201= 4 10:04, Edgar E. Iglesias <edgar.iglesias@gmail.com>= wrote:
From: "Edgar E. Iglesias" <edgar.iglesias@gm= ail.com>

Avoids the explicit 16bit mask. No functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
---
=C2=A0target-arm/internals.h =C2=A0 =C2=A0 | 14 +++++++-------
=C2=A0target-arm/translate-a64.c | =C2=A02 +-
=C2=A02 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/target-arm/internals.h b/target-arm/internals.h
index 08fa697..707643e 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -199,23 +199,23 @@ static inline uint32_t syn_uncategorized(void)
=C2=A0 =C2=A0 =C2=A0return (EC_UNCATEGORIZED << ARM_EL_EC_SHIFT) | AR= M_EL_IL;
=C2=A0}

-static inline uint32_t syn_aa64_svc(uint32_t imm16)
+static inline uint32_t syn_aa64_svc(uint16_t imm16)
=C2=A0{
- =C2=A0 =C2=A0return (EC_AA64_SVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | = (imm16 & 0xffff);
+ =C2=A0 =C2=A0return (EC_AA64_SVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | = imm16;
=C2=A0}

-static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
+static inline uint32_t syn_aa32_svc(uint16_t imm16, bool is_thumb)
=C2=A0{
- =C2=A0 =C2=A0return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | (imm16 &= 0xffff)
+ =C2=A0 =C2=A0return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | imm16
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| (is_thumb ? 0 : ARM_EL_IL);
=C2=A0}

-static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
+static inline uint32_t syn_aa64_bkpt(uint16_t imm16)
=C2=A0{
- =C2=A0 =C2=A0return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL |= (imm16 & 0xffff);
+ =C2=A0 =C2=A0return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL |= imm16;
=C2=A0}

-static inline uint32_t syn_aa32_bkpt(uint32_t imm16, bool is_thumb)
+static inline uint32_t syn_aa32_bkpt(uint16_t imm16, bool is_thumb)
=C2=A0{
=C2=A0 =C2=A0 =C2=A0return (EC_AA32_BKPT << ARM_EL_EC_SHIFT) | (imm16= & 0xffff)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| (is_thumb ? 0 : ARM_EL_IL);

Looks like you forgot to remove the mask on this o= ne.
=C2=A0
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index a9c4633..3589898 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1433,7 +1433,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)=
=C2=A0{
=C2=A0 =C2=A0 =C2=A0int opc =3D extract32(insn, 21, 3);
=C2=A0 =C2=A0 =C2=A0int op2_ll =3D extract32(insn, 0, 5);
- =C2=A0 =C2=A0int imm16 =3D extract32(insn, 5, 16);
+ =C2=A0 =C2=A0uint16_t imm16 =3D extract32(insn, 5, 16);

=C2=A0 =C2=A0 =C2=A0switch (opc) {
=C2=A0 =C2=A0 =C2=A0case 0:
--
1.8.3.2


--001a11c2df9e2cee9504fb9454cd--