On Tuesday, December 17, 2019, Michael Rolnik <mrol...@gmail.com> 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 <
> aleksandar.m.m...@gmail.com> wrote:
>
>> On Tue, Dec 10, 2019 at 8:18 AM Michael Rolnik <mrol...@gmail.com> 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 <
>> aleksandar.m.m...@gmail.com> wrote:
>> >>
>> >>
>> >>
>> >> On Monday, December 9, 2019, Michael Rolnik <mrol...@gmail.com> 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 <
>> aleksandar.m.m...@gmail.com> wrote:
>> >>>>
>> >>>>
>> >>>>
>> >>>> On Sunday, December 8, 2019, Michael Rolnik <mrol...@gmail.com>
>> 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 <mrol...@gmail.com>
>> >>>>> Suggested-by: Richard Henderson <richard.hender...@linaro.org>
>> >>>>> Suggested-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>> >>>>> Suggested-by: Aleksandar Markovic <aleksandar.m.m...@gmail.com>
>> >>>>> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>> >>>>> Tested-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>> >>>>> ---
>> >>>>> 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 <r...@twiddle.net>
>> >>>>> + * Copyright (c) 2019 Michael Rolnik <mrol...@gmail.com>
>> >>>>> + *
>> >>>>> + * 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
>
