Re: [PATCH v35 01/13] target/avr: Add outward facing interfaces and core CPU logic

[Michael Rolnik]

It seems to be a huge investment. this function should parse the
binary data as `decode_insn` does, so I suggest to modify decodetree
tool to make decoding information available to the instruction print
function.
what do you think?

On Thu, Nov 21, 2019 at 9:44 PM Michael Rolnik <address@hidden> wrote:
>
> On Thu, Nov 21, 2019 at 8:55 PM Philippe Mathieu-Daudé
> <address@hidden> wrote:
> >
> > Hi Michael,
> >
> > On 10/29/19 10:24 PM, Michael Rolnik wrote:
> > > This includes:
> > > - CPU data structures
> > > - object model classes and functions
> > > - migration functions
> > > - GDB hooks
> > >
> > > Co-developed-by: Michael Rolnik <address@hidden>
> > > Co-developed-by: Sarah Harris <address@hidden>
> > > Signed-off-by: Michael Rolnik <address@hidden>
> > > Signed-off-by: Sarah Harris <address@hidden>
> > > Signed-off-by: Michael Rolnik <address@hidden>
> > > Acked-by: Igor Mammedov <address@hidden>
> > > ---
> > >   gdb-xml/avr-cpu.xml    |  49 ++++
> > >   target/avr/cpu-param.h |  37 +++
> > >   target/avr/cpu-qom.h   |  54 ++++
> > >   target/avr/cpu.c       | 576 +++++++++++++++++++++++++++++++++++++++++
> > >   target/avr/cpu.h       | 253 ++++++++++++++++++
> > >   target/avr/gdbstub.c   |  85 ++++++
> > >   target/avr/machine.c   | 121 +++++++++
> > >   7 files changed, 1175 insertions(+)
> > >   create mode 100644 gdb-xml/avr-cpu.xml
> > >   create mode 100644 target/avr/cpu-param.h
> > >   create mode 100644 target/avr/cpu-qom.h
> > >   create mode 100644 target/avr/cpu.c
> > >   create mode 100644 target/avr/cpu.h
> > >   create mode 100644 target/avr/gdbstub.c
> > >   create mode 100644 target/avr/machine.c
> > >
> > > diff --git a/gdb-xml/avr-cpu.xml b/gdb-xml/avr-cpu.xml
> > > new file mode 100644
> > > index 0000000000..c4747f5b40
> > > --- /dev/null
> > > +++ b/gdb-xml/avr-cpu.xml
> > > @@ -0,0 +1,49 @@
> > > +<?xml version="1.0"?>
> > > +<!-- Copyright (C) 2018-2019 Free Software Foundation, Inc.
> > > +
> > > +     Copying and distribution of this file, with or without modification,
> > > +     are permitted in any medium without royalty provided the copyright
> > > +     notice and this notice are preserved.  -->
> > > +
> > > +<!-- Register numbers are hard-coded in order to maintain backward
> > > +     compatibility with older versions of tools that didn't use xml
> > > +     register descriptions.  -->
> > > +
> > > +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > +<feature name="org.gnu.gdb.riscv.cpu">
> > > +  <reg name="r0" bitsize="8" type="int" regnum="0"/>
> > > +  <reg name="r1" bitsize="8" type="int"/>
> > > +  <reg name="r2" bitsize="8" type="int"/>
> > > +  <reg name="r3" bitsize="8" type="int"/>
> > > +  <reg name="r4" bitsize="8" type="int"/>
> > > +  <reg name="r5" bitsize="8" type="int"/>
> > > +  <reg name="r6" bitsize="8" type="int"/>
> > > +  <reg name="r7" bitsize="8" type="int"/>
> > > +  <reg name="r8" bitsize="8" type="int"/>
> > > +  <reg name="r9" bitsize="8" type="int"/>
> > > +  <reg name="r10" bitsize="8" type="int"/>
> > > +  <reg name="r11" bitsize="8" type="int"/>
> > > +  <reg name="r12" bitsize="8" type="int"/>
> > > +  <reg name="r13" bitsize="8" type="int"/>
> > > +  <reg name="r14" bitsize="8" type="int"/>
> > > +  <reg name="r15" bitsize="8" type="int"/>
> > > +  <reg name="r16" bitsize="8" type="int"/>
> > > +  <reg name="r17" bitsize="8" type="int"/>
> > > +  <reg name="r18" bitsize="8" type="int"/>
> > > +  <reg name="r19" bitsize="8" type="int"/>
> > > +  <reg name="r20" bitsize="8" type="int"/>
> > > +  <reg name="r21" bitsize="8" type="int"/>
> > > +  <reg name="r22" bitsize="8" type="int"/>
> > > +  <reg name="r23" bitsize="8" type="int"/>
> > > +  <reg name="r24" bitsize="8" type="int"/>
> > > +  <reg name="r25" bitsize="8" type="int"/>
> > > +  <reg name="r26" bitsize="8" type="int"/>
> > > +  <reg name="r27" bitsize="8" type="int"/>
> > > +  <reg name="r28" bitsize="8" type="int"/>
> > > +  <reg name="r29" bitsize="8" type="int"/>
> > > +  <reg name="r30" bitsize="8" type="int"/>
> > > +  <reg name="r31" bitsize="8" type="int"/>
> > > +  <reg name="sreg" bitsize="8" type="int"/>
> > > +  <reg name="sp" bitsize="8" type="int"/>
> > > +  <reg name="pc" bitsize="8" type="int"/>
> > > +</feature>
> > > diff --git a/target/avr/cpu-param.h b/target/avr/cpu-param.h
> > > new file mode 100644
> > > index 0000000000..ccd1ea3429
> > > --- /dev/null
> > > +++ b/target/avr/cpu-param.h
> > > @@ -0,0 +1,37 @@
> > > +/*
> > > + * QEMU AVR CPU
> > > + *
> > > + * Copyright (c) 2019 Michael Rolnik
> > > + *
> > > + * This library is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU Lesser General Public
> > > + * License as published by the Free Software Foundation; either
> > > + * version 2.1 of the License, or (at your option) any later version.
> > > + *
> > > + * This library 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
> > > + * Lesser General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU Lesser General Public
> > > + * License along with this library; if not, see
> > > + * <http://www.gnu.org/licenses/lgpl-2.1.html>
> > > + */
> > > +
> > > +#ifndef AVR_CPU_PARAM_H
> > > +#define AVR_CPU_PARAM_H 1
> > > +
> > > +#define TARGET_LONG_BITS 32
> > > +/*
> > > + * TARGET_PAGE_BITS cannot be more than 8 bits because
> > > + * 1.  all IO registers occupy [0x0000 .. 0x00ff] address range, and they
> > > + *     should be implemented as a device and not memory
> > > + * 2.  SRAM starts at the address 0x0100
> > > + */
> > > +#define TARGET_PAGE_BITS 8
> > > +#define TARGET_PHYS_ADDR_SPACE_BITS 24
> > > +#define TARGET_VIRT_ADDR_SPACE_BITS 24
> > > +#define NB_MMU_MODES 2
> > > +
> > > +
> > > +#endif
> > > diff --git a/target/avr/cpu-qom.h b/target/avr/cpu-qom.h
> > > new file mode 100644
> > > index 0000000000..e28b58c897
> > > --- /dev/null
> > > +++ b/target/avr/cpu-qom.h
> > > @@ -0,0 +1,54 @@
> > > +/*
> > > + * QEMU AVR CPU
> > > + *
> > > + * Copyright (c) 2019 Michael Rolnik
> > > + *
> > > + * This library is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU Lesser General Public
> > > + * License as published by the Free Software Foundation; either
> > > + * version 2.1 of the License, or (at your option) any later version.
> > > + *
> > > + * This library 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
> > > + * Lesser General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU Lesser General Public
> > > + * License along with this library; if not, see
> > > + * <http://www.gnu.org/licenses/lgpl-2.1.html>
> > > + */
> > > +
> > > +#ifndef QEMU_AVR_QOM_H
> > > +#define QEMU_AVR_QOM_H
> > > +
> > > +#include "hw/core/cpu.h"
> > > +
> > > +#define TYPE_AVR_CPU "avr-cpu"
> > > +
> > > +#define AVR_CPU_CLASS(klass) \
> > > +    OBJECT_CLASS_CHECK(AVRCPUClass, (klass), TYPE_AVR_CPU)
> > > +#define AVR_CPU(obj) \
> > > +    OBJECT_CHECK(AVRCPU, (obj), TYPE_AVR_CPU)
> > > +#define AVR_CPU_GET_CLASS(obj) \
> > > +    OBJECT_GET_CLASS(AVRCPUClass, (obj), TYPE_AVR_CPU)
> > > +
> > > +/**
> > > + *  AVRCPUClass:
> > > + *  @parent_realize: The parent class' realize handler.
> > > + *  @parent_reset: The parent class' reset handler.
> > > + *  @vr: Version Register value.
> > > + *
> > > + *  A AVR CPU model.
> > > + */
> > > +typedef struct AVRCPUClass {
> > > +    /*< private >*/
> > > +    CPUClass parent_class;
> > > +    /*< public >*/
> > > +    DeviceRealize parent_realize;
> > > +    void (*parent_reset)(CPUState *cpu);
> > > +} AVRCPUClass;
> > > +
> > > +typedef struct AVRCPU AVRCPU;
> > > +
> > > +
> > > +#endif /* !defined (QEMU_AVR_CPU_QOM_H) */
> > > diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> > > new file mode 100644
> > > index 0000000000..dae56d7845
> > > --- /dev/null
> > > +++ b/target/avr/cpu.c
> > > @@ -0,0 +1,576 @@
> > > +/*
> > > + * QEMU AVR CPU
> > > + *
> > > + * Copyright (c) 2019 Michael Rolnik
> > > + *
> > > + * This library is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU Lesser General Public
> > > + * License as published by the Free Software Foundation; either
> > > + * version 2.1 of the License, or (at your option) any later version.
> > > + *
> > > + * This library 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
> > > + * Lesser General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU Lesser General Public
> > > + * License along with this library; if not, see
> > > + * <http://www.gnu.org/licenses/lgpl-2.1.html>
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qapi/error.h"
> > > +#include "qemu/qemu-print.h"
> > > +#include "exec/exec-all.h"
> > > +#include "cpu.h"
> > > +
> > > +static void avr_cpu_set_pc(CPUState *cs, vaddr value)
> > > +{
> > > +    AVRCPU *cpu = AVR_CPU(cs);
> > > +
> > > +    cpu->env.pc_w = value / 2; /* internally PC points to words */
> > > +}
> > > +
> > > +static bool avr_cpu_has_work(CPUState *cs)
> > > +{
> > > +    AVRCPU *cpu = AVR_CPU(cs);
> > > +    CPUAVRState *env = &cpu->env;
> > > +
> > > +    return (cs->interrupt_request & (CPU_INTERRUPT_HARD | 
> > > CPU_INTERRUPT_RESET))
> > > +            && cpu_interrupts_enabled(env);
> > > +}
> > > +
> > > +static void avr_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock 
> > > *tb)
> > > +{
> > > +    AVRCPU *cpu = AVR_CPU(cs);
> > > +    CPUAVRState *env = &cpu->env;
> > > +
> > > +    env->pc_w = tb->pc / 2; /* internally PC points to words */
> > > +}
> > > +
> > > +static void avr_cpu_reset(CPUState *cs)
> > > +{
> > > +    AVRCPU *cpu = AVR_CPU(cs);
> > > +    AVRCPUClass *mcc = AVR_CPU_GET_CLASS(cpu);
> > > +    CPUAVRState *env = &cpu->env;
> > > +
> > > +    mcc->parent_reset(cs);
> > > +
> > > +    env->pc_w = 0;
> > > +    env->sregI = 1;
> > > +    env->sregC = 0;
> > > +    env->sregZ = 0;
> > > +    env->sregN = 0;
> > > +    env->sregV = 0;
> > > +    env->sregS = 0;
> > > +    env->sregH = 0;
> > > +    env->sregT = 0;
> > > +
> > > +    env->rampD = 0;
> > > +    env->rampX = 0;
> > > +    env->rampY = 0;
> > > +    env->rampZ = 0;
> > > +    env->eind = 0;
> > > +    env->sp = 0;
> > > +
> > > +    env->skip = 0;
> > > +
> > > +    memset(env->r, 0, sizeof(env->r));
> > > +
> > > +    tlb_flush(cs);
> > > +}
> > > +
> > > +static void avr_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
> > > +{
> > > +    info->mach = bfd_arch_avr;
> > > +    info->print_insn = NULL;
> >
> > Why not implement the dump_ASM code? This is very useful...
> >
> > Richard what is your position on this? I'd rather enforce this as a
> > requirement for each ports.
>
> Hi Philippe.
>
> I will, however it could be a functionality of the instruction parser.
>
> Regards,
> Michael
> >
> > > +}
> > > +
> > [...]
> >
>
>
> --
> Best Regards,
> Michael Rolnik



-- 
Best Regards,
Michael Rolnik