Re: [Qemu-devel] [Qemu-ppc] [PATCH 5/8] target-ppc: Load Quadword

[Alexander Graf]

On 27.01.2014, at 18:54, Tom Musta <address@hidden> 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 <address@hidden>
> ---
> target-ppc/translate.c      |   45 ++++++++++++++++++++++++++++--------------
> target-ppc/translate_init.c |    2 +-
> 2 files changed, 31 insertions(+), 16 deletions(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 90cbb72..15a4d1b 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -2861,36 +2861,51 @@ static void gen_ld(DisasContext *ctx)
> /* lq */
> static void gen_lq(DisasContext *ctx)
> {
> +    /* lq is a legal user mode instruction starting in ISA 2.07 */
> +    int legal_in_user_mode = (ctx->insns_flags2 & PPC2_LSQ_ISA207) != 0;
> +
> +    if (!legal_in_user_mode) {
> #if defined(CONFIG_USER_ONLY)
> -    gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> +        gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> +        return;
> #else
> +        if (unlikely(ctx->mem_idx == 0)) {
> +            gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> +            return;
> +        }
> +
> +        if (unlikely(ctx->le_mode)) {

You're mixing two semantically separate things here. legal_in_user_mode doesn't 
really indicate that le_mode isn't usable. I'm sure if you just make this two 
if()'s with two separate bools that get assigned the same value gcc will be 
smart enough to optimize it just as well as this combined branch.

Reading through the above code we probably eventually want something like

static bool is_user_mode(DisasContext *ctx)
{
#if defined(CONFIG_USER_ONLY)
    return true;
#else
    return ctx->mem_idx == 0;
#endif
}

which would enable us to combine code like the above.


Alex