Re: [PATCH v38 11/22] target/avr: Add instruction disassembly function

[Aleksandar Markovic]

On Tuesday, December 17, 2019, Michael Rolnik <address@hidden> wrote:

Aleksandar.

1. inst.decode file

2. avr features are not accessible from avr_print_insn as it does not receive a pointer to CPU context. So, there is not way to inform the user that some instructions are not supported unless I define several different avr_print_insn functions.

OK, this is not a crucial feature. If I were you, I would leave it for future, as one of "nice to have" things. It is possible to implement it, of course, with some additions to the decoder, but don't spend your energy on that now, that is my advice.

But patch 1 restructuring is a must. You have to form several logical units out of it.

inst.decode is written to be convenient to the author (you), but it should be convenient to the reader, please rearrange items to be as in the ISA document (even though we both know it is not convenient to you).

The review takes forever, but you are up to one of the most serious tasks in QEMU, so it is expected, no reason to worry.

Best regards,

Aleksandar

 

Regards,

Michael Rolnik

On Thu, Dec 12, 2019 at 11:12 AM Aleksandar Markovic <address@hidden> wrote:

On Tue, Dec 10, 2019 at 8:18 AM Michael Rolnik <address@hidden> wrote:
>
> You are right. See at the bottom of the file. There is a comment about it
>

Sorry, what file?

I also see that you disassemble instructions regardless of what AVR
CPU the current executable is built for, don't you? OK, not a very big
deal, but can be confusing for end user if disassembly text of an
instruction that is not supported by a particular CPU is displayed as
if it is supported.

> Sent from my cell phone, please ignore typos
>
> On Tue, Dec 10, 2019, 6:21 AM Aleksandar Markovic <address@hidden> wrote:
>>
>>
>>
>> On Monday, December 9, 2019, Michael Rolnik <address@hidden> wrote:
>>>
>>> Hi Aleksandar.
>>>
>>> 1. all instructions are 16 bit long except CALL & JMP they are 32 bit long
>>
>>
>> Accordingto the doc, LDS and STS also have 32-bit coding.
>>
>>
>>>
>>> 2. next_word_used is set to true by next_word when called by append_16 when CALL & JMP are parsed
>>>
>>> Regards,
>>> Michael Rolnik
>>>
>>> On Mon, Dec 9, 2019 at 8:10 PM Aleksandar Markovic <address@hidden> wrote:
>>>>
>>>>
>>>>
>>>> On Sunday, December 8, 2019, Michael Rolnik <address@hidden> wrote:
>>>>>
>>>>> Provide function disassembles executed instruction when `-d in_asm` is
>>>>> provided
>>>>>
>>>>> Example:
>>>>> `./avr-softmmu/qemu-system-avr -bios free-rtos/Demo/AVR_ATMega2560_GCC/demo.elf -d in_asm` will produce something like the following
>>>>>
>>>>> ```
>>>>>     ...
>>>>>     IN:
>>>>>     0x0000014a:  CALL      0x3808
>>>>>
>>>>>     IN: main
>>>>>     0x00003808:  CALL      0x4b4
>>>>>
>>>>>     IN: vParTestInitialise
>>>>>     0x000004b4:  LDI       r24, 255
>>>>>     0x000004b6:  STS       r24, 0
>>>>>     0x000004b8:  MULS      r16, r20
>>>>>     0x000004ba:  OUT       $1, r24
>>>>>     0x000004bc:  LDS       r24, 0
>>>>>     0x000004be:  MULS      r16, r20
>>>>>     0x000004c0:  OUT       $2, r24
>>>>>     0x000004c2:  RET
>>>>>     ...
>>>>> ```
>>>>>
>>>>> Signed-off-by: Michael Rolnik <address@hidden>
>>>>> Suggested-by: Richard Henderson <address@hidden>
>>>>> Suggested-by: Philippe Mathieu-Daudé <address@hidden>
>>>>> Suggested-by: Aleksandar Markovic <address@hidden>
>>>>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>>>>> Tested-by: Philippe Mathieu-Daudé <address@hidden>
>>>>> ---
>>>>>  target/avr/cpu.h       |   1 +
>>>>>  target/avr/cpu.c       |   2 +-
>>>>>  target/avr/disas.c     | 226 +++++++++++++++++++++++++++++++++++++++++
>>>>>  target/avr/translate.c |  11 ++
>>>>>  4 files changed, 239 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 target/avr/disas.c
>>>>>
>>>>> diff --git a/target/avr/cpu.h b/target/avr/cpu.h
>>>>> index c217eefeb4..a8a3e7ade6 100644
>>>>> --- a/target/avr/cpu.h
>>>>> +++ b/target/avr/cpu.h
>>>>> @@ -178,6 +178,7 @@ bool avr_cpu_exec_interrupt(CPUState *cpu, int int_req);
>>>>>  hwaddr avr_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>>>>>  int avr_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
>>>>>  int avr_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>>>>> +int avr_print_insn(bfd_vma addr, disassemble_info *info);
>>>>>
>>>>>  static inline int avr_feature(CPUAVRState *env, int feature)
>>>>>  {
>>>>> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
>>>>> index c5cafcae3c..be4b921e4d 100644
>>>>> --- a/target/avr/cpu.c
>>>>> +++ b/target/avr/cpu.c
>>>>> @@ -83,7 +83,7 @@ static void avr_cpu_reset(CPUState *cs)
>>>>>  static void avr_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
>>>>>  {
>>>>>      info->mach = bfd_arch_avr;
>>>>> -    info->print_insn = NULL;
>>>>> +    info->print_insn = avr_print_insn;
>>>>>  }
>>>>>
>>>>>  static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
>>>>> diff --git a/target/avr/disas.c b/target/avr/disas.c
>>>>> new file mode 100644
>>>>> index 0000000000..22863d2eb1
>>>>> --- /dev/null
>>>>> +++ b/target/avr/disas.c
>>>>> @@ -0,0 +1,226 @@
>>>>> +/*
>>>>> + * AVR disassembler
>>>>> + *
>>>>> + * Copyright (c) 2019 Richard Henderson <address@hidden>
>>>>> + * Copyright (c) 2019 Michael Rolnik <address@hidden>
>>>>> + *
>>>>> + * This program is free software: you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License as published by
>>>>> + * the Free Software Foundation, either version 2 of the License, or
>>>>> + * (at your option) any later version.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> + * GNU General Public License for more details.
>>>>> + *
>>>>> + * You should have received a copy of the GNU General Public License
>>>>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>> + */
>>>>> +
>>>>> +#include "qemu/osdep.h"
>>>>> +#include "cpu.h"
>>>>> +
>>>>> +typedef struct {
>>>>> +    disassemble_info *info;
>>>>> +    uint16_t next_word;
>>>>> +    bool next_word_used;
>>>>> +} DisasContext;
>>>>> +
>>>>> +static int to_regs_16_31_by_one(DisasContext *ctx, int indx)
>>>>> +{
>>>>> +    return 16 + (indx % 16);
>>>>> +}
>>>>> +
>>>>> +static int to_regs_16_23_by_one(DisasContext *ctx, int indx)
>>>>> +{
>>>>> +    return 16 + (indx % 8);
>>>>> +}
>>>>> +static int to_regs_24_30_by_two(DisasContext *ctx, int indx)
>>>>> +{
>>>>> +    return 24 + (indx % 4) * 2;
>>>>> +}
>>>>> +static int to_regs_00_30_by_two(DisasContext *ctx, int indx)
>>>>> +{
>>>>> +    return (indx % 16) * 2;
>>>>> +}
>>>>> +
>>>>> +static uint16_t next_word(DisasContext *ctx)
>>>>> +{
>>>>> +    ctx->next_word_used = true;
>>>>> +    return ctx->next_word;
>>>>> +}
>>>>> +
>>>>> +static int append_16(DisasContext *ctx, int x)
>>>>> +{
>>>>> +    return x << 16 | next_word(ctx);
>>>>> +}
>>>>> +
>>>>> +
>>>>> +/* Include the auto-generated decoder.  */
>>>>> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
>>>>> +#include "decode_insn.inc.c"
>>>>> +
>>>>> +#define output(mnemonic, format, ...) \
>>>>> +    (pctx->info->fprintf_func(pctx->info->stream, "%-9s " format, \
>>>>> +                        mnemonic, ##__VA_ARGS__))
>>>>> +
>>>>> +int avr_print_insn(bfd_vma addr, disassemble_info *info)
>>>>> +{
>>>>> +    DisasContext ctx;
>>>>> +    DisasContext *pctx = &ctx;
>>>>> +    bfd_byte buffer[4];
>>>>> +    uint16_t insn;
>>>>> +    int status;
>>>>> +
>>>>> +    ctx.info = info;
>>>>> +
>>>>> +    status = info->read_memory_func(addr, buffer, 4, info);
>>>>> +    if (status != 0) {
>>>>> +        info->memory_error_func(status, addr, info);
>>>>> +        return -1;
>>>>> +    }
>>>>> +    insn = bfd_getl16(buffer);
>>>>> +    ctx.next_word = bfd_getl16(buffer + 2);
>>>>> +    ctx.next_word_used = false;
>>>>> +
>>>>> +    if (!decode_insn(&ctx, insn)) {
>>>>> +        output(".db", "0x%02x, 0x%02x", buffer[0], buffer[1]);
>>>>> +    }
>>>>> +
>>>>> +    return ctx.next_word_used ? 4 : 2;
>>>>> +}
>>>>
>>>>
>>>> Hi, Michael,
>>>>
>>>> What is the role of ctx.next_word_used, if it looks it is always "false"?
>>>>
>>>> Related to this, how do you disassemble 32-bit-coded instructions? From this patch, it looks you treat all AVR instructions as 16-bit-coded?
>>>>
>>>> Thanks,
>>>> Aleksandar
>>>>
>>>>>
>>>>> +
>>>>> +
>>>>> +#define INSN(opcode, format, ...)                                       \
>>>>> +static bool trans_##opcode(DisasContext *pctx, arg_##opcode * a)        \
>>>>> +{                                                                       \
>>>>> +    output(#opcode, format, ##__VA_ARGS__);                             \
>>>>> +    return true;                                                        \
>>>>> +}
>>>>> +
>>>>> +#define INSN_MNEMONIC(opcode, mnemonic, format, ...)                    \
>>>>> +static bool trans_##opcode(DisasContext *pctx, arg_##opcode * a)        \
>>>>> +{                                                                       \
>>>>> +    output(mnemonic, format, ##__VA_ARGS__);                            \
>>>>> +    return true;                                                        \
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + *   C       Z       N       V       S       H       T       I
>>>>> + *   0       1       2       3       4       5       6       7
>>>>> + */
>>>>> +static const char *brbc[] = {
>>>>> +    "BRCC", "BRNE", "BRPL", "BRVC", "BRGE", "BRHC", "BRTC", "BRID"
>>>>> +};
>>>>> +
>>>>> +static const char *brbs[] = {
>>>>> +    "BRCS", "BREQ", "BRMI", "BRVS", "BRLT", "BRHS", "BRTS", "BRIE"
>>>>> +};
>>>>> +
>>>>> +static const char *bset[] = {
>>>>> +    "SEC",  "SEZ",  "SEN",  "SEZ",  "SES",  "SEH",  "SET",  "SEI"
>>>>> +};
>>>>> +
>>>>> +static const char *bclr[] = {
>>>>> +    "CLC",  "CLZ",  "CLN",  "CLZ",  "CLS",  "CLH",  "CLT",  "CLI"
>>>>> +};
>>>>> +
>>>>> +INSN(ADC,    "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(ADD,    "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(ADIW,   "r%d:r%d, %d", a->rd + 1, a->rd, a->imm)
>>>>> +INSN(AND,    "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(ANDI,   "r%d, %d", a->rd, a->imm)
>>>>> +INSN(ASR,    "r%d", a->rd)
>>>>> +INSN_MNEMONIC(BCLR,  bclr[a->bit], "")
>>>>> +INSN(BLD,    "r%d, %d", a->rd, a->bit)
>>>>> +INSN_MNEMONIC(BRBC,  brbc[a->bit], ".%+d", a->imm * 2)
>>>>> +INSN_MNEMONIC(BRBS,  brbs[a->bit], ".%+d", a->imm * 2)
>>>>> +INSN(BREAK,  "")
>>>>> +INSN_MNEMONIC(BSET,  bset[a->bit], "")
>>>>> +INSN(BST,    "r%d, %d", a->rd, a->bit)
>>>>> +INSN(CALL,   "0x%x", a->imm * 2)
>>>>> +INSN(CBI,    "%d, %d", a->reg, a->bit)
>>>>> +INSN(COM,    "r%d", a->rd)
>>>>> +INSN(CP,     "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(CPC,    "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(CPI,    "r%d, %d", a->rd, a->imm)
>>>>> +INSN(CPSE,   "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(DEC,    "r%d", a->rd)
>>>>> +INSN(DES,    "%d", a->imm)
>>>>> +INSN(EICALL, "")
>>>>> +INSN(EIJMP,  "")
>>>>> +INSN(ELPM1,  "")
>>>>> +INSN(ELPM2,  "r%d, Z", a->rd)
>>>>> +INSN(ELPMX,  "r%d, Z+", a->rd)
>>>>> +INSN(EOR,    "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(FMUL,   "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(FMULS,  "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(FMULSU, "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(ICALL,  "")
>>>>> +INSN(IJMP,   "")
>>>>> +INSN(IN,     "r%d, $%d", a->rd, a->imm)
>>>>> +INSN(INC,    "r%d", a->rd)
>>>>> +INSN(JMP,    "0x%x", a->imm * 2)
>>>>> +INSN(LAC,    "Z, r%d", a->rd)
>>>>> +INSN(LAS,    "Z, r%d", a->rd)
>>>>> +INSN(LAT,    "Z, r%d", a->rd)
>>>>> +INSN(LDDY,   "r%d, Y+%d", a->rd, a->imm)
>>>>> +INSN(LDDZ,   "r%d, Z+%d", a->rd, a->imm)
>>>>> +INSN(LDI,    "r%d, %d", a->rd, a->imm)
>>>>> +INSN(LDS,    "r%d, %d", a->rd, a->imm)
>>>>> +INSN(LDX1,   "r%d, X", a->rd)
>>>>> +INSN(LDX2,   "r%d, X+", a->rd)
>>>>> +INSN(LDX3,   "r%d, -X", a->rd)
>>>>> +INSN(LDY2,   "r%d, Y+", a->rd)
>>>>> +INSN(LDY3,   "r%d, -Y", a->rd)
>>>>> +INSN(LDZ2,   "r%d, Z+", a->rd)
>>>>> +INSN(LDZ3,   "r%d, -Z", a->rd)
>>>>> +INSN(LPM1,   "")
>>>>> +INSN(LPM2,   "r%d, Z", a->rd)
>>>>> +INSN(LPMX,   "r%d, Z+", a->rd)
>>>>> +INSN(LSR,    "r%d", a->rd)
>>>>> +INSN(MOV,    "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(MOVW,   "r%d:r%d, r%d,r:r%d", a->rd + 1, a->rd, a->rr + 1, a->rr)
>>>>> +INSN(MUL,    "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(MULS,   "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(MULSU,  "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(NEG,    "r%d", a->rd)
>>>>> +INSN(NOP,    "")
>>>>> +INSN(OR,     "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(ORI,    "r%d, %d", a->rd, a->imm)
>>>>> +INSN(OUT,    "$%d, r%d", a->imm, a->rd)
>>>>> +INSN(POP,    "r%d", a->rd)
>>>>> +INSN(PUSH,   "r%d", a->rd)
>>>>> +INSN(RCALL,  ".%+d", a->imm * 2)
>>>>> +INSN(RET,    "")
>>>>> +INSN(RETI,   "")
>>>>> +INSN(RJMP,   ".%+d", a->imm * 2)
>>>>> +INSN(ROR,    "r%d", a->rd)
>>>>> +INSN(SBC,    "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(SBCI,   "r%d, %d", a->rd, a->imm)
>>>>> +INSN(SBI,    "$%d, %d", a->reg, a->bit)
>>>>> +INSN(SBIC,   "$%d, %d", a->reg, a->bit)
>>>>> +INSN(SBIS,   "$%d, %d", a->reg, a->bit)
>>>>> +INSN(SBIW,   "r%d:r%d, %d", a->rd + 1, a->rd, a->imm)
>>>>> +INSN(SBRC,   "r%d, %d", a->rr, a->bit)
>>>>> +INSN(SBRS,   "r%d, %d", a->rr, a->bit)
>>>>> +INSN(SLEEP,  "")
>>>>> +INSN(SPM,    "")
>>>>> +INSN(SPMX,   "Z+")
>>>>> +INSN(STDY,   "r%d, Y+%d", a->rd, a->imm)
>>>>> +INSN(STDZ,   "r%d, Z+%d", a->rd, a->imm)
>>>>> +INSN(STS,    "r%d, %d", a->rd, a->imm)
>>>>> +INSN(STX1,   "r%d, X", a->rr)
>>>>> +INSN(STX2,   "r%d, X+", a->rr)
>>>>> +INSN(STX3,   "r%d, -X", a->rr)
>>>>> +INSN(STY2,   "r%d, Y+", a->rd)
>>>>> +INSN(STY3,   "r%d, -Y", a->rd)
>>>>> +INSN(STZ2,   "r%d, Z+", a->rd)
>>>>> +INSN(STZ3,   "r%d, -Z", a->rd)
>>>>> +INSN(SUB,    "r%d, r%d", a->rd, a->rr)
>>>>> +INSN(SUBI,   "r%d, %d", a->rd, a->imm)
>>>>> +INSN(SWAP,   "r%d", a->rd)
>>>>> +INSN(WDR,    "")
>>>>> +INSN(XCH,    "Z, r%d", a->rd)
>>>>> +
>>>>> diff --git a/target/avr/translate.c b/target/avr/translate.c
>>>>> index c8c6f798bf..a621195817 100644
>>>>> --- a/target/avr/translate.c
>>>>> +++ b/target/avr/translate.c
>>>>> @@ -2917,6 +2917,17 @@ done_generating:
>>>>>
>>>>>      tb->size = (ctx.npc - pc_start) * 2;
>>>>>      tb->icount = num_insns;
>>>>> +
>>>>> +#ifdef DEBUG_DISAS
>>>>> +    if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
>>>>> +        && qemu_log_in_addr_range(tb->pc)) {
>>>>> +        qemu_log_lock();
>>>>> +        qemu_log("IN: %s\n", lookup_symbol(tb->pc));
>>>>> +        log_target_disas(cs, tb->pc, tb->size);
>>>>> +        qemu_log("\n");
>>>>> +        qemu_log_unlock();
>>>>> +    }
>>>>> +#endif
>>>>>  }
>>>>>
>>>>>  void restore_state_to_opc(CPUAVRState *env, TranslationBlock *tb,
>>>>> --
>>>>> 2.17.2 (Apple Git-113)
>>>>>
>>>
>>>
>>> --
>>> Best Regards,
>>> Michael Rolnik

--

Best Regards,
Michael Rolnik