[V2,6/9] target-ppc: Load Quadword

This patch adds the Book I (user space) Load Quadword (lq) instruction.
This instruction was introduced into Book I in Power ISA V2.07.  Previous
versions of the architecture supported this as a privileged instruction.
Previous versions of the architecture also did not support Little Endian
mode.

Note that this patch also adds the PPC_64BX flag to the Power8 model,
which enables the lq instruction.

Signed-off-by: Tom Musta <tommusta@gmail.com>
---
V2: Refactored the user-mode and LE checks per Alex Graf's review.

 target-ppc/translate.c      |   40 ++++++++++++++++++++++++----------------
 target-ppc/translate_init.c |    2 +-
 2 files changed, 25 insertions(+), 17 deletions(-)

On 31.01.2014, at 20:34, Tom Musta <tommusta@gmail.com> wrote:

> This patch adds the Book I (user space) Load Quadword (lq) instruction.
> This instruction was introduced into Book I in Power ISA V2.07.  Previous
> versions of the architecture supported this as a privileged instruction.
> Previous versions of the architecture also did not support Little Endian
> mode.
> 
> Note that this patch also adds the PPC_64BX flag to the Power8 model,
> which enables the lq instruction.
> 
> Signed-off-by: Tom Musta <tommusta@gmail.com>
> ---
> V2: Refactored the user-mode and LE checks per Alex Graf's review.
> 
> target-ppc/translate.c      |   40 ++++++++++++++++++++++++----------------
> target-ppc/translate_init.c |    2 +-
> 2 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 6ec4127..e36686e 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -2872,36 +2872,44 @@ static void gen_ld(DisasContext *ctx)
> /* lq */
> static void gen_lq(DisasContext *ctx)
> {
> -#if defined(CONFIG_USER_ONLY)
> -    gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> -#else
> -    int ra, rd;
> -    TCGv EA;
> +    /* lq is a legal user mode instruction starting in ISA 2.07 */
> +    bool legal_in_user_mode = (ctx->insns_flags2 & PPC2_LSQ_ISA207) != 0;
> +    bool le_is_supported = (ctx->insns_flags2 & PPC2_LSQ_ISA207) != 0;
> 
> -    /* Restore CPU state */
> -    if (unlikely(ctx->mem_idx == 0)) {
> +    if (!legal_in_user_mode && is_user_mode(ctx)) {
>         gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>         return;
>     }
> +
> +    if (!le_is_supported && ctx->le_mode) {
> +        gen_exception_err(ctx, POWERPC_EXCP_ALIGN, POWERPC_EXCP_ALIGN_LE);
> +        return;
> +    }
> +
> +    int ra, rd;
> +    TCGv EA;

I would've expected the compiler to emit a warning at this point because you're declaring variables outside of the beginning of a scope?


Alex

> +
>     ra = rA(ctx->opcode);
>     rd = rD(ctx->opcode);
>     if (unlikely((rd & 1) || rd == ra)) {
>         gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
>         return;
>     }
> -    if (unlikely(ctx->le_mode)) {
> -        /* Little-endian mode is not handled */
> -        gen_exception_err(ctx, POWERPC_EXCP_ALIGN, POWERPC_EXCP_ALIGN_LE);
> -        return;
> -    }
> +
>     gen_set_access_type(ctx, ACCESS_INT);
>     EA = tcg_temp_new();
>     gen_addr_imm_index(ctx, EA, 0x0F);
> -    gen_qemu_ld64(ctx, cpu_gpr[rd], EA);
> -    gen_addr_add(ctx, EA, EA, 8);
> -    gen_qemu_ld64(ctx, cpu_gpr[rd+1], EA);
> +
> +    if (unlikely(ctx->le_mode)) {
> +        gen_qemu_ld64(ctx, cpu_gpr[rd+1], EA);
> +        gen_addr_add(ctx, EA, EA, 8);
> +        gen_qemu_ld64(ctx, cpu_gpr[rd], EA);
> +    } else {
> +        gen_qemu_ld64(ctx, cpu_gpr[rd], EA);
> +        gen_addr_add(ctx, EA, EA, 8);
> +        gen_qemu_ld64(ctx, cpu_gpr[rd+1], EA);
> +    }
>     tcg_temp_free(EA);
> -#endif
> }
> #endif
> 
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 886238a..d7bcbba 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -7333,7 +7333,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>                        PPC_CACHE | PPC_CACHE_ICBI | PPC_CACHE_DCBZ |
>                        PPC_MEM_SYNC | PPC_MEM_EIEIO |
>                        PPC_MEM_TLBIE | PPC_MEM_TLBSYNC |
> -                       PPC_64B | PPC_ALTIVEC |
> +                       PPC_64B | PPC_64BX | PPC_ALTIVEC |
>                        PPC_SEGMENT_64B | PPC_SLBI |
>                        PPC_POPCNTB | PPC_POPCNTWD;
>     pcc->insns_flags2 = PPC2_VSX | PPC2_VSX207 | PPC2_DFP | PPC2_DBRX |
> -- 
> 1.7.1
> 
>

On 2/5/2014 3:43 AM, Alexander Graf wrote:
> I would've expected the compiler to emit a warning at this point because you're declaring variables outside of the beginning of a scope?

I did not see a warning on either of my test platforms (PowerPC/RHEL/GCC 4.4.7, Intel/Ubuntu/GCC 4.6.3).  But I will certainly fix.

On 02/05/2014 09:12 AM, Tom Musta wrote:
> On 2/5/2014 3:43 AM, Alexander Graf wrote:
>> I would've expected the compiler to emit a warning at this point because you're declaring variables outside of the beginning of a scope?
>

Only if you enforce C89 compilation.  But qemu requires a C99 compiler,
where it is not a warning in gcc unless you enable -Wjump-misses-init
and have a goto statement that bypasses the declaration.

> I did not see a warning on either of my test platforms (PowerPC/RHEL/GCC 4.4.7, Intel/Ubuntu/GCC 4.6.3).  But I will certainly fix.

Whether or not the compiler enforces it, some projects still like to
stick to C89 declarations-before-statements, even when otherwise
requiring C99 compiler features.