[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/7] target/ppc: Add infrastructure for prefixed instructi
From: |
David Gibson |
Subject: |
Re: [PATCH v2 1/7] target/ppc: Add infrastructure for prefixed instructions |
Date: |
Tue, 5 Jan 2021 15:39:04 +1100 |
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
signature.asc
Description: PGP signature
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH v2 1/7] target/ppc: Add infrastructure for prefixed instructions,
David Gibson <=