On Wed, Dec 16, 2020 at 06:07:58AM -0300, Gustavo Romero wrote: > From: Michael Roth <mdr...@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 <mr...@lamentation.net> > [ gromero: - comments clean up and updates > - additional comments in the commit log ] > Signed-off-by: Gustavo Romero <grom...@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
signature.asc
Description: PGP signature