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~