On Wed, Dec 16, 2020 at 06:07:58AM -0300, Gustavo Romero wrote: > From: Michael Roth <[email protected]> > > 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 <[email protected]> > [ gromero: - comments clean up and updates > - additional comments in the commit log ] > Signed-off-by: Gustavo Romero <[email protected]>
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
