Re: [PATCH 4/5] target/ppc: Base changes to allow 32/64-bit insns

[Richard Henderson]

On 4/13/21 2:11 PM, Luis Pires wrote:
>       if (ctx->exception == POWERPC_EXCP_NONE) {
> -        gen_update_nip(ctx, ctx->base.pc_next - 4);
> +        gen_update_nip(ctx, ctx->base.pc_next - ctx->insn_size);

It appears as if the major (only?) use of insn_size is this subtraction?  It 
looks like it would be better to simply save the address of the current pc 
before we begin decoding.

This change should be done as a separate patch.

> +static uint64_t ppc_load_insn(DisasContext *ctx)
> +{
> +    uint64_t insn;
> +    uint32_t insn_part;
> +
> +    /* read 4 bytes */
> +    insn_part = translator_ldl_swap(ctx->env, ctx->base.pc_next,
> +                                    need_byteswap(ctx));
> +    insn = ((uint64_t)insn_part) << 32;
> +    ctx->base.pc_next += 4;
> +    ctx->insn_size = 4;
> +
> +    if (is_insn_prefix(insn_part)) {
> +        /* read 4 more bytes */
> +        insn_part = translator_ldl_swap(ctx->env, ctx->base.pc_next,
> +                                        need_byteswap(ctx));
> +        insn |= insn_part;
> +
> +        ctx->base.pc_next += 4;
> +        ctx->insn_size += 4;
> +    }
> +
> +    return insn;
> +}

> @@ -7979,37 +8049,31 @@ static bool ppc_tr_breakpoint_check(DisasContextBase 
> *dcbase, CPUState *cs,
>   {
>       DisasContext *ctx = container_of(dcbase, DisasContext, base);
>   
> +    target_ulong insn_size = ppc_peek_next_insn_size(ctx);
> +
>       gen_debug_exception(ctx);
>       dcbase->is_jmp = DISAS_NORETURN;
>       /*
>        * The address covered by the breakpoint must be included in
>        * [tb->pc, tb->pc + tb->size) in order to for it to be properly
> -     * cleared -- thus we increment the PC here so that the logic
> -     * setting tb->size below does the right thing.
> +     * cleared -- thus we increment the PC here.
>        */
> -    ctx->base.pc_next += 4;
> +    ctx->base.pc_next += insn_size;

Here in breakpoint_check, we merely need a non-zero number.  No point in a 
change here.

> +    /* load the next insn, keeping track of the insn size */
> +    insn = ppc_load_insn(ctx);
> +
> +    if (unlikely(ctx->insn_size == 8 &&
> +                 (ctx->base.pc_next & 0x3f) == 0x04)) {
> +        /*
> +         * Raise alignment exception when a 64-bit insn crosses a
> +         * 64-byte boundary
> +         */
> +        gen_exception_err(ctx, POWERPC_EXCP_ALIGN, POWERPC_EXCP_ALIGN_INSN);

This is incorrect.

In 1.10.2 Instruction Fetches, it states that all instructions are word 
aligned, and gives an example of a prefixed instruction not aligned on a 
double-word boundrary.


r~