[v2,13/17] target-arm: Use uint16_t in syndrome generators with 16bit imms

From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>

Avoids the explicit 16bit mask. No functional change.

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

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 <edgar.iglesias@gmail.com> wrote:

> From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
>
> Avoids the explicit 16bit mask. No functional change.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> ---
>  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
>
>

On 11 June 2014 20:19, Greg Bellows <greg.bellows@linaro.org> wrote:
> Called out possibly missing fix below.  Beside it, I'm convinced this change
> is beneficial, other than maybe for readability.

"not convinced" ?

Personally, I don't much care either way, but I would suggest that
if you're trying to get a big fat patchset through it's usually better
not to throw in too many not-actually-needed patches :-)

-- PMM

"not convinced" ... I was just saying that I could not find any benefit for
it.


On 11 June 2014 16:05, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 11 June 2014 20:19, Greg Bellows <greg.bellows@linaro.org> wrote:
> > Called out possibly missing fix below.  Beside it, I'm convinced this
> change
> > is beneficial, other than maybe for readability.
>
> "not convinced" ?
>
> Personally, I don't much care either way, but I would suggest that
> if you're trying to get a big fat patchset through it's usually better
> not to throw in too many not-actually-needed patches :-)
>
> -- PMM
>

On Wed, Jun 11, 2014 at 10:05:35PM +0100, Peter Maydell wrote:
> On 11 June 2014 20:19, Greg Bellows <greg.bellows@linaro.org> wrote:
> > Called out possibly missing fix below.  Beside it, I'm convinced this change
> > is beneficial, other than maybe for readability.
> 
> "not convinced" ?
> 
> Personally, I don't much care either way, but I would suggest that
> if you're trying to get a big fat patchset through it's usually better
> not to throw in too many not-actually-needed patches :-)

Allright, I didn't have this in my v1 but added it in response to review
comments. Will drop it for v3. Agree that we better do this change separately.

Cheers,
Edgar