Re: [PATCH v2 1/7] target/ppc: Add infrastructure for prefixed instructions

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7635 bytes --]

On Wed, Dec 16, 2020 at 06:07:58AM -0300, Gustavo Romero wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Some prefixed instructions (Type 0 and 1, e.g. 8-byte Load/Store or 8LS),
> have a completely seperate namespace for their primary opcodes.
> 
> Other prefixed instructions (Type 2 and 3, e.g. Modified Load/Store or MLS)
> actually re-use existing opcodes to provide a modified variant. We could
> handle these by extending the existing opcode handlers to check for the
> prefix and handle accordingly, but in some cases it is cleaner to define
> seperate handlers for these in their own table entry, and avoids the need
> to add error-handling to existing handlers for cases where they are called
> for a prefixed Type 2/3 instruction but don't implement the handling for
> them. In the future we can re-work things to support both approaches if
> cases arise where that seems warranted.
> 
> Then we have the normal non-prefixed instructions.
> 
> To handle all 3 of these cases we extend the table size 3x, with normal
> instructions indexed directly by their primary opcodes, Type 0/1 indexed by
> primary opcode + PPC_CPU_PREFIXED_OPCODE_OFFSET, and Type 2/3 indexed by
> primary opcode + PPC_CPU_PREFIXED_MODIFIED_OPCODE_OFFSET.
> 
> Various exception/SRR handling changes related to prefixed instructions are
> yet to be implemented. For instance, no alignment interrupt is generated if
> a prefixed instruction crosses the 64-byte boundary; no SRR bit related to
> prefixed instructions is set on any interrupt, like for FP unavailable
> interrupt, data storage interrupt, etc.
> 
> For details, please see PowerISA v3.1, particularly the following sections:
> 
> 1.6 Instruction Formats, p. 11
> 1.6.3  Instruction Prefix Formats, p. 22
> 3.3.2  Fixed-Point Load Instructions, p. 51
> 4.6.2  Floating-Point Load Instructions, p. 149
> Chapter 7 Interrupts, p. 1247
> 
> Signed-off-by: Michael Roth <mroth@lamentation.net>
> [ gromero: - comments clean up and updates
>            - additional comments in the commit log ]
> Signed-off-by: Gustavo Romero <gromero@linux.ibm.com>

I'm still not seeing any advantage to putting both the plain and
prefixed opcodes into a single table.  Essentially you're taking the
prefix and base opcode and encoding them into a single index.  Which
isn't inherently wrong, except that the only thing you do with that
single index is effectively decode it back into the original two
parts.

More specifically

[snip]
> +static uint32_t opc1_idx(DisasContext *ctx)
> +{
> +    uint32_t table_offset = 0;
> +
> +    switch (ctx->prefix_subtype) {
> +    case PREFIX_ST_8LS:
> +    case PREFIX_ST_8MLS:
> +    case PREFIX_ST_8RR:
> +    case PREFIX_ST_8MRR:
> +        table_offset = PPC_CPU_PREFIXED_OPCODE_OFFSET;
> +        break;
> +    case PREFIX_ST_MLS:
> +    case PREFIX_ST_MMLS:
> +    case PREFIX_ST_MRR:
> +    case PREFIX_ST_MMRR:
> +    case PREFIX_ST_MMIRR:
> +        table_offset = PPC_CPU_PREFIXED_MODIFIED_OPCODE_OFFSET;
> +        break;
> +    }
> +
> +    return table_offset + opc1(ctx->opcode);
> +}

Here, you translate the prefix type into a table offset, but...

> +
>  static void ppc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
>  {
>      DisasContext *ctx = container_of(dcbase, DisasContext, base);
> @@ -8004,14 +8142,40 @@ static void ppc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
>  
>      ctx->opcode = translator_ldl_swap(env, ctx->base.pc_next,
>                                        need_byteswap(ctx));
> +    /* check for prefix */
> +    ctx->prefix_subtype = parse_prefix_subtype(ctx->opcode);
> +    if (ctx->prefix_subtype == PREFIX_ST_INVALID) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "invalid/unsupported prefix: "
> +                      "%08x " TARGET_FMT_lx "\n",
> +                      ctx->prefix, ctx->base.pc_next);
> +    } else if (ctx->prefix_subtype != PREFIX_ST_NONE) {
> +        /*
> +         * this is the 4-byte prefix of an instruction, read the
> +         * next 4 and advance the PC
> +         *
> +         * TODO: we can optimize this to do a single load since we
> +         * read in target_long anyways already
> +         *
> +         * double-check endianess cases.
> +         *
> +         * engineering note about endianess changing based on rfid
> +         * or interrupt. does this need to be accounted for here?
> +         */
> +        ctx->prefix = ctx->opcode;
> +        ctx->base.pc_next += 4;
> +        ctx->opcode = translator_ldl_swap(env, ctx->base.pc_next,
> +                                          need_byteswap(ctx));
> +    } else {
> +        ctx->prefix = 0;
> +    }
>  
> -    LOG_DISAS("translate opcode %08x (%02x %02x %02x %02x) (%s)\n",
> +    LOG_DISAS("translate opcode %08x (%02x %02x %02x %02x) prefix %08x (%s)\n",
>                ctx->opcode, opc1(ctx->opcode), opc2(ctx->opcode),
> -              opc3(ctx->opcode), opc4(ctx->opcode),
> +              opc3(ctx->opcode), opc4(ctx->opcode), ctx->prefix,
>                ctx->le_mode ? "little" : "big");
>      ctx->base.pc_next += 4;
>      table = cpu->opcodes;
> -    handler = table[opc1(ctx->opcode)];
> +    handler = table[opc1_idx(ctx)];

Here, at the only caller, you already have some conditionals on prefix
type, so you could equally well just use a different table.

>      if (is_indirect_opcode(handler)) {
>          table = ind_table(handler);
>          handler = table[opc2(ctx->opcode)];
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index bb66526280..0ea8c2c5c1 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -9563,8 +9563,13 @@ static int insert_in_table(opc_handler_t **table, unsigned char idx,
>  }
>  
>  static int register_direct_insn(opc_handler_t **ppc_opcodes,
> -                                unsigned char idx, opc_handler_t *handler)
> +                                unsigned char idx, opc_handler_t *handler,
> +                                bool prefixed, bool modified)
>  {
> +    if (prefixed) {
> +        idx = modified ? idx + PPC_CPU_PREFIXED_MODIFIED_OPCODE_OFFSET
> +                       : idx + PPC_CPU_PREFIXED_OPCODE_OFFSET;
> +    }
>      if (insert_in_table(ppc_opcodes, idx, handler) < 0) {
>          printf("*** ERROR: opcode %02x already assigned in main "
>                 "opcode table\n", idx);
> @@ -9688,7 +9693,8 @@ static int register_insn(opc_handler_t **ppc_opcodes, opcode_t *insn)
>              }
>          }
>      } else {
> -        if (register_direct_insn(ppc_opcodes, insn->opc1, &insn->handler) < 0) {
> +        if (register_direct_insn(ppc_opcodes, insn->opc1, &insn->handler,
> +                                 insn->prefixed, insn->modified) < 0) {
>              return -1;
>          }
>      }
> @@ -9766,6 +9772,7 @@ static void dump_ppc_insns(CPUPPCState *env)
>      for (opc1 = 0x00; opc1 < PPC_CPU_OPCODES_LEN; opc1++) {
>          table = env->opcodes;
>          handler = table[opc1];
> +        /* TODO: need to update opcode dump to account for prefixed space */
>          if (is_indirect_opcode(handler)) {
>              /* opc2 is 5 bits long */
>              for (opc2 = 0; opc2 < PPC_CPU_INDIRECT_OPCODES_LEN; opc2++) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]