Re: [Qemu-devel] [PATCH 02/14] target-ppc: Add xxinsertw instruction

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4370 bytes --]

On Thu, Jan 05, 2017 at 04:56:07PM +0530, Nikunj A Dadhania wrote:
> xxinsertw: VSX Vector Insert Word

I think this still has problems with out of bounds values.

> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  target-ppc/helper.h                 |  1 +
>  target-ppc/int_helper.c             | 25 +++++++++++++++++++++++++
>  target-ppc/translate/vsx-impl.inc.c |  5 +++--
>  target-ppc/translate/vsx-ops.inc.c  |  1 +
>  4 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index 8b30420..6c5b194 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -541,6 +541,7 @@ DEF_HELPER_2(xvrspiz, void, env, i32)
>  DEF_HELPER_2(xxperm, void, env, i32)
>  DEF_HELPER_2(xxpermr, void, env, i32)
>  DEF_HELPER_4(xxextractuw, void, env, tl, tl, i32)
> +DEF_HELPER_4(xxinsertw, void, env, tl, tl, i32)
>  
>  DEF_HELPER_2(efscfsi, i32, env, i32)
>  DEF_HELPER_2(efscfui, i32, env, i32)
> diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c
> index 63ba0e3..24e5964 100644
> --- a/target-ppc/int_helper.c
> +++ b/target-ppc/int_helper.c
> @@ -2059,6 +2059,31 @@ void helper_xxextractuw(CPUPPCState *env, target_ulong xtn,
>      putVSR(xtn, &xt, env);
>  }
>  
> +void helper_xxinsertw(CPUPPCState *env, target_ulong xtn,
> +                      target_ulong xbn, uint32_t index)
> +{
> +    ppc_vsr_t xt, xb;
> +    size_t es = sizeof(uint32_t);
> +    int ins_index, i = 0;
> +
> +    getVSR(xbn, &xb, env);
> +    getVSR(xtn, &xt, env);
> +
> +#if defined(HOST_WORDS_BIGENDIAN)
> +    ins_index = index;
> +    for (i = 0; i < es && ins_index < 16; i++, ins_index++) {
> +        xt.u8[ins_index] = xb.u8[8 - es + i];

Unlike extract, you don't mod the ins_index here.  Which IIUC, means
for UIMM > 12 you will overwrite data beyond xt, which sounds like a
very bad thing.

> +    }
> +#else
> +    ins_index = 15 - index;
> +    for (i = es - 1; i >= 0 && ins_index >= 0; i--, ins_index--) {
> +        xt.u8[ins_index] = xb.u8[8 + i];
> +    }
> +#endif
> +
> +    putVSR(xtn, &xt, env);
> +}
> +
>  #define VEXT_SIGNED(name, element, mask, cast, recast)              \
>  void helper_##name(ppc_avr_t *r, ppc_avr_t *b)                      \
>  {                                                                   \
> diff --git a/target-ppc/translate/vsx-impl.inc.c b/target-ppc/translate/vsx-impl.inc.c
> index 7977f24..c9ba0f5 100644
> --- a/target-ppc/translate/vsx-impl.inc.c
> +++ b/target-ppc/translate/vsx-impl.inc.c
> @@ -1180,7 +1180,7 @@ static void gen_xxsldwi(DisasContext *ctx)
>      tcg_temp_free_i64(xtl);
>  }
>  
> -#define VSX_EXTRACT(name)                                       \
> +#define VSX_EXTRACT_INSERT(name)                                \

Plus for UIMM > 15, this macro will set the target to 0.  That seems
right for extract, but doesn't really seem right for insert (although
*maybe* it's what the hardware does).

>  static void gen_##name(DisasContext *ctx)                       \
>  {                                                               \
>      TCGv xt, xb;                                                \
> @@ -1208,7 +1208,8 @@ static void gen_##name(DisasContext *ctx)                       \
>      tcg_temp_free_i32(t0);                                      \
>  }
>  
> -VSX_EXTRACT(xxextractuw)
> +VSX_EXTRACT_INSERT(xxextractuw)
> +VSX_EXTRACT_INSERT(xxinsertw)
>  
>  #undef GEN_XX2FORM
>  #undef GEN_XX3FORM
> diff --git a/target-ppc/translate/vsx-ops.inc.c b/target-ppc/translate/vsx-ops.inc.c
> index 473d925..096d358 100644
> --- a/target-ppc/translate/vsx-ops.inc.c
> +++ b/target-ppc/translate/vsx-ops.inc.c
> @@ -285,6 +285,7 @@ GEN_XX2FORM(xxspltw, 0x08, 0x0A, PPC2_VSX),
>  GEN_XX1FORM(xxspltib, 0x08, 0x0B, PPC2_ISA300),
>  GEN_XX3FORM_DM(xxsldwi, 0x08, 0x00),
>  GEN_XX2FORM_EXT(xxextractuw, 0x0A, 0x0A, PPC2_ISA300),
> +GEN_XX2FORM_EXT(xxinsertw, 0x0A, 0x0B, PPC2_ISA300),
>  
>  #define GEN_XXSEL_ROW(opc3) \
>  GEN_HANDLER2_E(xxsel, "xxsel", 0x3C, 0x18, opc3, 0, PPC_NONE, PPC2_VSX), \

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]