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

[Alexander Graf]

On 27.01.2014, at 20:53, Tom Musta <address@hidden> wrote:

> On 1/27/2014 12:55 PM, Alexander Graf wrote:
>> 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.
>> 
> 
> Hmmm ... I'm not sure that I see the problem.  Perhaps the comment should be 
> clearer.
> And I guess there is really no need to compute the legal_in_user_mode flag 
> since it
> is only used once.
> 
> Prior to ISA 2.07, lq was not legal in user mode; attempting to execute lq 
> when MSR[PR]=1
> resulted in a privileged instruction exception.  Also, when MSR[PR]=0 and 
> MSR[LE]=1, an
> alignment exception was generated irrespective of the computed address.
> 
> Starting with ISA 2.07, both of these restrictions are lifted.  So the 
> proposed code is
> as follows:
> 
> 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);
>        return;
> #else
>        if (unlikely(ctx->mem_idx == 0)) {
>            gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>            return;
>        }
> 
>        if (unlikely(ctx->le_mode)) {

Right, but the fact that "we're legal in user mode" has nothing to do with "we 
can handle LE mode". I was thinking of something along the lines of

{
    bool legal_in_user_mode = (ctx->insns_flags2 & PPC2_LSQ_ISA207);
    bool can_handle_le = (ctx->insns_flags2 & PPC2_LSQ_ISA207);

    if (!legal_in_user_mode && is_in_user_mode(ctx)) {
        gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
        return;
    }

    if (!can_handle_le && ctx->le_mode) {
        gen_exception_err(ctx, POWERPC_EXCP_ALIGN, POWERPC_EXCP_ALIGN_LE);
        return;
    }

    [...]
}

>            /* Little-endian mode is not handled */
>            gen_exception_err(ctx, POWERPC_EXCP_ALIGN, POWERPC_EXCP_ALIGN_LE);
>            return;
>        }
> #endif
>    }
> 
>    int ra, rd;
>    TCGv EA;
>    ... // rest of implementation
> 
> 
> P.S.  I think there should be an alignment check after the EA is computed.

I'm fairly sure this isn't the only place missing alignment checks :). But then 
again alignment checks are tricky because your host kernel may fix them up for 
you in linux only mode and in general they're not particularly useful.


Alex