From: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> The CBO instructions were, AFAIU, disassembled erroneously. In addition, lq (load-quad, a RV128 instruction) shares the same opcode space as the CBOs, which also led to incorrect dasm for that instruction (understandable since RV128 is an experimental feature). To avoid overlaps, we now require that lq is decoded only if rd is not 0, as CBO requires the field corresponding to rd to be zero, both in dasm and decodetree. The last patch corrects the original indentation that was making checkpatch unhappy. Frédéric Pétrot (3): disas/riscv.c: Correct wrong shifts for cbo disas/riscv.c: Correct disasm of lq disas/riscv.c: Correct indent for checkpatch disas/riscv.c | 38 +++++++++++++++++++------------------- target/riscv/insn32.decode | 2 ++ 2 files changed, 21 insertions(+), 19 deletions(-) -- 2.43.0 From: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> The cache block operations are specified on bits 31:20, but for some reason a shift of 17 and a 5 bit masking were performed. Make this a raw shift of 20 prior to test for the cbo operation. Reported-by: Julien Thillard <julien.thillard@univ-grenoble-alpes.fr> Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> --- disas/riscv.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/disas/riscv.c b/disas/riscv.c index XXXXXXX..XXXXXXX 100644 --- a/disas/riscv.c +++ b/disas/riscv.c @@ -XXX,XX +XXX,XX @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) case 2: /* * 'lq' shares the "(...) 010 ..... 0001111" opcode space - * with 'cbo' insns. Check the next 5 bits to select - * what we want: + * with 'cbo' insns. * * cbo_inval 0000000 00000 ..... 010 00000 0001111 * cbo_clean 0000000 00001 ..... 010 00000 0001111 @@ -XXX,XX +XXX,XX @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) * * Anything that doesn't match these will default to 'lq'. */ - switch ((inst >> 17) & 0b11111) { + switch (inst >> 20) { case 0: op = rv_op_cbo_inval; break; case 1: op = rv_op_cbo_clean; break; case 2: op = rv_op_cbo_flush; break; -- 2.43.0 From: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> Lq shares the cmo opcode space, but the cbos must have rd = 0. Now ensure that they are matched only in that case. This implies that lq can be recognized as such only when rd != 0. Update the decoder file accordingly. Reported-by: Julien Thillard <julien.thillard@univ-grenoble-alpes.fr> Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> --- disas/riscv.c | 19 ++++++++++--------- target/riscv/insn32.decode | 2 ++ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/disas/riscv.c b/disas/riscv.c index XXXXXXX..XXXXXXX 100644 --- a/disas/riscv.c +++ b/disas/riscv.c @@ -XXX,XX +XXX,XX @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) case 2: /* * 'lq' shares the "(...) 010 ..... 0001111" opcode space - * with 'cbo' insns. + * with 'cbo' insns, but assumes rd != 0. * * cbo_inval 0000000 00000 ..... 010 00000 0001111 * cbo_clean 0000000 00001 ..... 010 00000 0001111 * cbo_flush 0000000 00010 ..... 010 00000 0001111 * cbo_zero 0000000 00100 ..... 010 00000 0001111 - * - * Anything that doesn't match these will default to 'lq'. */ - switch (inst >> 20) { - case 0: op = rv_op_cbo_inval; break; - case 1: op = rv_op_cbo_clean; break; - case 2: op = rv_op_cbo_flush; break; - case 4: op = rv_op_cbo_zero; break; - default: op = rv_op_lq; break; + if ((inst >> 7) & 0b11111) { + op = rv_op_lq; + } else { + switch (inst >> 20) { + case 0: op = rv_op_cbo_inval; break; + case 1: op = rv_op_cbo_clean; break; + case 2: op = rv_op_cbo_flush; break; + case 4: op = rv_op_cbo_zero; break; + } } } break; diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode index XXXXXXX..XXXXXXX 100644 --- a/target/riscv/insn32.decode +++ b/target/riscv/insn32.decode @@ -XXX,XX +XXX,XX @@ ldu ............ ..... 111 ..... 0000011 @i ] # *** RVI128 lq *** + # *** Catches an lq with rd = 0, which we disallow + illegal ------------ ----- 010 00000 0001111 lq ............ ..... 010 ..... 0001111 @i } sq ............ ..... 100 ..... 0100011 @s -- 2.43.0 From: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> Trival indentation patch on the cbo/lq relevant code. Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> --- disas/riscv.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/disas/riscv.c b/disas/riscv.c index XXXXXXX..XXXXXXX 100644 --- a/disas/riscv.c +++ b/disas/riscv.c @@ -XXX,XX +XXX,XX @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) case 0: op = rv_op_fence; break; case 1: op = rv_op_fence_i; break; case 2: - /* - * 'lq' shares the "(...) 010 ..... 0001111" opcode space - * with 'cbo' insns, but assumes rd != 0. - * - * cbo_inval 0000000 00000 ..... 010 00000 0001111 - * cbo_clean 0000000 00001 ..... 010 00000 0001111 - * cbo_flush 0000000 00010 ..... 010 00000 0001111 - * cbo_zero 0000000 00100 ..... 010 00000 0001111 - */ - if ((inst >> 7) & 0b11111) { - op = rv_op_lq; - } else { - switch (inst >> 20) { - case 0: op = rv_op_cbo_inval; break; - case 1: op = rv_op_cbo_clean; break; - case 2: op = rv_op_cbo_flush; break; - case 4: op = rv_op_cbo_zero; break; - } - } + /* + * 'lq' shares the "(...) 010 ..... 0001111" opcode space + * with 'cbo' insns, but assumes rd != 0. + * + * cbo_inval 0000000 00000 ..... 010 00000 0001111 + * cbo_clean 0000000 00001 ..... 010 00000 0001111 + * cbo_flush 0000000 00010 ..... 010 00000 0001111 + * cbo_zero 0000000 00100 ..... 010 00000 0001111 + */ + if ((inst >> 7) & 0b11111) { + op = rv_op_lq; + } else { + switch (inst >> 20) { + case 0: op = rv_op_cbo_inval; break; + case 1: op = rv_op_cbo_clean; break; + case 2: op = rv_op_cbo_flush; break; + case 4: op = rv_op_cbo_zero; break; + } + } } break; case 4: -- 2.43.0 From: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> The CBO instructions were, AFAIU, disassembled erroneously. In addition, lq (load-quad, a RV128 instruction) shares the same opcode space as the CBOs, which also led to incorrect dasm for that instruction (understandable since RV128 is an experimental feature). To avoid overlaps, we now require that lq is decoded only if rd is not 0, as CBO requires the field corresponding to rd to be zero, both in dasm and decodetree. Change in v2: - Move the last v1 patch to the first place to correct the original indentation that was making checkpatch unhappy before doing the real work. (Note that disas/riscv.c has overall many checkpatch issues, this patch only solves the indentation one, but otherwise keeps the same "style" as the rest of the file) Daniel, I took the liberty to keep your "Reviewed-by" tag, as only patch order and indentation changed. Frédéric Pétrot (3): disas/riscv.c: Correct indent for checkpatch disas/riscv.c: Correct wrong shifts for cbo disas/riscv.c: Correct disasm of lq disas/riscv.c | 38 +++++++++++++++++++------------------- target/riscv/insn32.decode | 2 ++ 2 files changed, 21 insertions(+), 19 deletions(-) -- 2.43.0 From: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> Trival indentation patch on the cbo/lq relevant code, in preparation for modifications in that part. Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> --- disas/riscv.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/disas/riscv.c b/disas/riscv.c index XXXXXXX..XXXXXXX 100644 --- a/disas/riscv.c +++ b/disas/riscv.c @@ -XXX,XX +XXX,XX @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) case 0: op = rv_op_fence; break; case 1: op = rv_op_fence_i; break; case 2: - /* - * 'lq' shares the "(...) 010 ..... 0001111" opcode space - * with 'cbo' insns. Check the next 5 bits to select - * what we want: - * - * cbo_inval 0000000 00000 ..... 010 00000 0001111 - * cbo_clean 0000000 00001 ..... 010 00000 0001111 - * cbo_flush 0000000 00010 ..... 010 00000 0001111 - * cbo_zero 0000000 00100 ..... 010 00000 0001111 - * - * Anything that doesn't match these will default to 'lq'. - */ - switch ((inst >> 17) & 0b11111) { - case 0: op = rv_op_cbo_inval; break; - case 1: op = rv_op_cbo_clean; break; - case 2: op = rv_op_cbo_flush; break; - case 4: op = rv_op_cbo_zero; break; - default: op = rv_op_lq; break; - } + /* + * 'lq' shares the "(...) 010 ..... 0001111" opcode space + * with 'cbo' insns. Check the next 5 bits to select + * what we want: + * + * cbo_inval 0000000 00000 ..... 010 00000 0001111 + * cbo_clean 0000000 00001 ..... 010 00000 0001111 + * cbo_flush 0000000 00010 ..... 010 00000 0001111 + * cbo_zero 0000000 00100 ..... 010 00000 0001111 + * + * Anything that doesn't match these will default to 'lq'. + */ + switch ((inst >> 17) & 0b11111) { + case 0: op = rv_op_cbo_inval; break; + case 1: op = rv_op_cbo_clean; break; + case 2: op = rv_op_cbo_flush; break; + case 4: op = rv_op_cbo_zero; break; + default: op = rv_op_lq; break; + } } break; case 4: -- 2.43.0 From: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> The cache block operations are specified on bits 31:20, but for some reason a shift of 17 and a 5 bit masking were performed. Make this a raw shift of 20 prior to test for the cbo operation. Fixes: 9273cda722 ("disas/riscv.c: add 'cbo' insns to disassembler") Reported-by: Julien Thillard <julien.thillard@univ-grenoble-alpes.fr> Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com> --- disas/riscv.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/disas/riscv.c b/disas/riscv.c index XXXXXXX..XXXXXXX 100644 --- a/disas/riscv.c +++ b/disas/riscv.c @@ -XXX,XX +XXX,XX @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) case 2: /* * 'lq' shares the "(...) 010 ..... 0001111" opcode space - * with 'cbo' insns. Check the next 5 bits to select - * what we want: + * with 'cbo' insns. * * cbo_inval 0000000 00000 ..... 010 00000 0001111 * cbo_clean 0000000 00001 ..... 010 00000 0001111 @@ -XXX,XX +XXX,XX @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) * * Anything that doesn't match these will default to 'lq'. */ - switch ((inst >> 17) & 0b11111) { + switch (inst >> 20) { case 0: op = rv_op_cbo_inval; break; case 1: op = rv_op_cbo_clean; break; case 2: op = rv_op_cbo_flush; break; -- 2.43.0 From: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> Lq shares the cmo opcode space, but the cbos must have rd = 0. Now ensure that they are matched only in that case. This implies that lq can be recognized as such only when rd != 0. Update the decoder file accordingly. Fixes: 9273cda722 ("disas/riscv.c: add 'cbo' insns to disassembler") Reported-by: Julien Thillard <julien.thillard@univ-grenoble-alpes.fr> Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com> --- disas/riscv.c | 19 ++++++++++--------- target/riscv/insn32.decode | 2 ++ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/disas/riscv.c b/disas/riscv.c index XXXXXXX..XXXXXXX 100644 --- a/disas/riscv.c +++ b/disas/riscv.c @@ -XXX,XX +XXX,XX @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) case 2: /* * 'lq' shares the "(...) 010 ..... 0001111" opcode space - * with 'cbo' insns. + * with 'cbo' insns, but assumes rd != 0. * * cbo_inval 0000000 00000 ..... 010 00000 0001111 * cbo_clean 0000000 00001 ..... 010 00000 0001111 * cbo_flush 0000000 00010 ..... 010 00000 0001111 * cbo_zero 0000000 00100 ..... 010 00000 0001111 - * - * Anything that doesn't match these will default to 'lq'. */ - switch (inst >> 20) { - case 0: op = rv_op_cbo_inval; break; - case 1: op = rv_op_cbo_clean; break; - case 2: op = rv_op_cbo_flush; break; - case 4: op = rv_op_cbo_zero; break; - default: op = rv_op_lq; break; + if ((inst >> 7) & 0b11111) { + op = rv_op_lq; + } else { + switch (inst >> 20) { + case 0: op = rv_op_cbo_inval; break; + case 1: op = rv_op_cbo_clean; break; + case 2: op = rv_op_cbo_flush; break; + case 4: op = rv_op_cbo_zero; break; + } } } break; diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode index XXXXXXX..XXXXXXX 100644 --- a/target/riscv/insn32.decode +++ b/target/riscv/insn32.decode @@ -XXX,XX +XXX,XX @@ ldu ............ ..... 111 ..... 0000011 @i ] # *** RVI128 lq *** + # *** Catches an lq with rd = 0, which we disallow + illegal ------------ ----- 010 00000 0001111 lq ............ ..... 010 ..... 0001111 @i } sq ............ ..... 100 ..... 0100011 @s -- 2.43.0