[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/7] target/ppc: Add infrastructure for prefixed instructions
|
From: |
David Gibson |
|
Subject: |
Re: [PATCH 1/7] target/ppc: Add infrastructure for prefixed instructions |
|
Date: |
Fri, 9 Oct 2020 11:43:03 +1100 |
On Mon, Oct 05, 2020 at 01:03:13AM -0300, Gustavo Romero wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
Probably a good idea to CC future spins to Richard Henderson
<rth@twiddle.net> - by knowledge of how TCG works is only middling.
> 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.
Is there an advantage to having one table with magic offsets, rather
than 3 separately declared tables?
> 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, section 1.6.3 and chapter 7,
> particularly the new changes regarding the prefixed instructions.
>
> 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>
> ---
> target/ppc/cpu.h | 4 +-
> target/ppc/internal.h | 14 +++
> target/ppc/translate.c | 167 +++++++++++++++++++++++++++++++-
> target/ppc/translate_init.c.inc | 11 ++-
> 4 files changed, 190 insertions(+), 6 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 766e9c5c26..c5de33572c 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -979,8 +979,10 @@ struct ppc_radix_page_info {
> #define PPC_TLB_EPID_LOAD 8
> #define PPC_TLB_EPID_STORE 9
>
> -#define PPC_CPU_OPCODES_LEN 0x40
> +#define PPC_CPU_OPCODES_LEN 0xc0
> #define PPC_CPU_INDIRECT_OPCODES_LEN 0x20
> +#define PPC_CPU_PREFIXED_OPCODE_OFFSET 0x40
> +#define PPC_CPU_PREFIXED_MODIFIED_OPCODE_OFFSET 0x80
Might be worth including some of that description from the commit
message as a comment here (or on the table declaration itself).
> struct CPUPPCState {
> /* Most commonly used resources during translated code execution first */
> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> index 15d655b356..d03d691a44 100644
> --- a/target/ppc/internal.h
> +++ b/target/ppc/internal.h
> @@ -129,6 +129,7 @@ EXTRACT_SHELPER(SIMM5, 16, 5);
> EXTRACT_HELPER(UIMM5, 16, 5);
> /* 4 bits unsigned immediate value */
> EXTRACT_HELPER(UIMM4, 16, 4);
> +
> /* Bit count */
> EXTRACT_HELPER(NB, 11, 5);
> /* Shift count */
> @@ -146,6 +147,19 @@ EXTRACT_HELPER(TO, 21, 5);
>
> EXTRACT_HELPER(CRM, 12, 8);
>
> +/*
> + * Instruction prefix fields
> + *
> + * as per PowerISA 3.1 1.6.3
> + */
> +
> +/* prefixed instruction type */
> +EXTRACT_HELPER(PREFIX_TYPE, 24, 2);
> +/* 1-bit sub-type */
> +EXTRACT_HELPER(PREFIX_ST1, 23, 1);
> +/* 4-bit sub-type */
> +EXTRACT_HELPER(PREFIX_ST4, 20, 4);
> +
> #ifndef CONFIG_USER_ONLY
> EXTRACT_HELPER(SR, 16, 4);
> #endif
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index fedb9b2271..96c2997d3f 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -180,6 +180,8 @@ struct DisasContext {
> uint32_t flags;
> uint64_t insns_flags;
> uint64_t insns_flags2;
> + uint32_t prefix;
> + uint32_t prefix_subtype;
> };
>
> /* Return true iff byteswap is needed in a scalar memop */
> @@ -376,6 +378,12 @@ GEN_OPCODE(name, opc1, opc2, opc3, inval, type, PPC_NONE)
> #define GEN_HANDLER_E(name, opc1, opc2, opc3, inval, type, type2)
> \
> GEN_OPCODE(name, opc1, opc2, opc3, inval, type, type2)
>
> +#define GEN_HANDLER_E_PREFIXED(name, opc1, opc2, opc3, inval, type, type2)
> \
> +GEN_OPCODE_PREFIXED(name, opc1, opc2, opc3, inval, type, type2, false)
> +
> +#define GEN_HANDLER_E_PREFIXED_M(name, opc1, opc2, opc3, inval, type, type2)
> \
> +GEN_OPCODE_PREFIXED(name, opc1, opc2, opc3, inval, type, type2, true)
> +
> #define GEN_HANDLER2(name, onam, opc1, opc2, opc3, inval, type)
> \
> GEN_OPCODE2(name, onam, opc1, opc2, opc3, inval, type, PPC_NONE)
>
> @@ -395,6 +403,8 @@ typedef struct opcode_t {
> #endif
> opc_handler_t handler;
> const char *oname;
> + bool prefixed;
> + bool modified;
> } opcode_t;
>
> /* Helpers for priv. check */
> @@ -449,6 +459,23 @@ typedef struct opcode_t {
> },
> \
> .oname = stringify(name),
> \
> }
> +#define GEN_OPCODE_PREFIXED(name, op1, op2, op3, invl, _typ, _typ2,
> _modified)\
> +{
> \
> + .opc1 = op1,
> \
> + .opc2 = op2,
> \
> + .opc3 = op3,
> \
> + .opc4 = 0xff,
> \
> + .handler = {
> \
> + .inval1 = invl,
> \
> + .type = _typ,
> \
> + .type2 = _typ2,
> \
> + .handler = &gen_##name,
> \
> + .oname = stringify(name),
> \
> + },
> \
> + .oname = stringify(name),
> \
> + .prefixed = true,
> \
> + .modified = _modified,
> \
> +}
> #define GEN_OPCODE_DUAL(name, op1, op2, op3, invl1, invl2, _typ, _typ2)
> \
> {
> \
> .opc1 = op1,
> \
> @@ -525,6 +552,22 @@ typedef struct opcode_t {
> },
> \
> .oname = stringify(name),
> \
> }
> +#define GEN_OPCODE_PREFIXED(name, op1, op2, op3, invl, _typ, _typ2,
> _modified)\
> +{
> \
> + .opc1 = op1,
> \
> + .opc2 = op2,
> \
> + .opc3 = op3,
> \
> + .opc4 = 0xff,
> \
> + .handler = {
> \
> + .inval1 = invl,
> \
> + .type = _typ,
> \
> + .type2 = _typ2,
> \
> + .handler = &gen_##name,
> \
> + },
> \
> + .oname = stringify(name),
> \
> + .prefixed = true,
> \
> + .modified = _modified,
> \
> +}
> #define GEN_OPCODE_DUAL(name, op1, op2, op3, invl1, invl2, _typ, _typ2)
> \
> {
> \
> .opc1 = op1,
> \
> @@ -7991,6 +8034,98 @@ static bool ppc_tr_breakpoint_check(DisasContextBase
> *dcbase, CPUState *cs,
> return true;
> }
>
> +
> +
> +/*
> + * Prefixed instruction sub-types
> + *
> + * as per PowerISA 3.1 1.6.3
> + */
> +enum {
> + /* non-prefixed instruction */
Two things are labelled as "non-prefixed instruction" here..
> + PREFIX_ST_INVALID = -1,
> + /* non-prefixed instruction */
> + PREFIX_ST_NONE = 0,
> + /* Type 00 prefixed insn sub-types */
I'm guessing the types here are in binary, maybe list as 0b00 0b01 etc
for clarity.
> + PREFIX_ST_8LS, /* 8-byte load/store */
> + PREFIX_ST_8MLS, /* 8-byte masked load/store */
> + /* Type 01 prefixed insn sub-types */
> + PREFIX_ST_8RR, /* 8-byte reg-to-reg */
> + PREFIX_ST_8MRR, /* 8-byte masked reg-to-reg */
> + /* Type 10 prefixed insn sub-types */
> + PREFIX_ST_MLS, /* modified load/store */
> + PREFIX_ST_MMLS, /* modified masked load/store */
> + /* Type 11 prefixed insn sub-types */
> + PREFIX_ST_MRR, /* modified reg-to-reg */
> + PREFIX_ST_MMRR, /* modified mask reg-to-reg */
> + PREFIX_ST_MMIRR, /* modified masked immediate reg-to-reg */
> +};
> +
> +static int32_t parse_prefix_subtype(uint32_t prefix)
> +{
> + int32_t prefix_subtype = PREFIX_ST_INVALID;
> +
> + /* primary opcode 1 is reserved for instruction prefixes */
> + if (opc1(prefix) != 1) {
> + return PREFIX_ST_NONE;
> + }
> +
> + switch (PREFIX_TYPE(prefix)) {
> + case 0:
> + if (PREFIX_ST1(prefix) == 0) {
> + prefix_subtype = PREFIX_ST_8LS;
> + } else if (PREFIX_ST1(prefix) == 1) {
> + prefix_subtype = PREFIX_ST_8MLS;
> + }
> + break;
> + case 1:
> + if (PREFIX_ST4(prefix) == 0) {
> + prefix_subtype = PREFIX_ST_8RR;
> + } else if (PREFIX_ST4(prefix) == 8) {
> + prefix_subtype = PREFIX_ST_8MRR;
> + }
Is this fallthrough intentional? If so it should be commented.
> + case 2:
> + if (PREFIX_ST1(prefix) == 0) {
> + prefix_subtype = PREFIX_ST_MLS;
> + } else if (PREFIX_ST1(prefix) == 1) {
> + prefix_subtype = PREFIX_ST_MMLS;
> + }
> + break;
> + case 3:
> + if (PREFIX_ST4(prefix) == 0) {
> + prefix_subtype = PREFIX_ST_MRR;
> + } else if (PREFIX_ST4(prefix) == 8) {
> + prefix_subtype = PREFIX_ST_MMRR;
> + } else if (PREFIX_ST4(prefix) == 9) {
> + prefix_subtype = PREFIX_ST_MMIRR;
> + }
Fallthrough here doesn't matter, but probably better to have a break;
for consistency.
> + }
> +
> + return prefix_subtype;
> +}
> +
> +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;
> + }
> +
> + return table_offset + opc1(ctx->opcode);
> +}
> +
> static void ppc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
> {
> DisasContext *ctx = container_of(dcbase, DisasContext, base);
> @@ -8004,14 +8139,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);
We should generate a 0x700 here, shouldn't we?
> + } 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)];
> 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
- Re: [PATCH 2/7] target/ppc: Add support for prefixed load/store instructions, (continued)
[PATCH 3/7] tests/tcg: Add tests for prefixed load/store instructions, Gustavo Romero, 2020/10/05
[PATCH 4/7] target/ppc: Add support for paired vector load/store instructions, Gustavo Romero, 2020/10/05
[PATCH 5/7] tests/tcg: Add tests for paired vector load/store instructions, Gustavo Romero, 2020/10/05
[PATCH 6/7] target/ppc: Add support for prefixed load/store FP instructions, Gustavo Romero, 2020/10/05
[PATCH 7/7] tests/tcg: Add tests for prefixed load/store FP instructions, Gustavo Romero, 2020/10/05
Re: [PATCH 1/7] target/ppc: Add infrastructure for prefixed instructions,
David Gibson <=