Re: [Qemu-devel] [PATCH moxie 3/5] moxie target code

[Andreas Färber]

Am 14.02.2013 02:23, schrieb Anthony Green:
> This patch adds all of the target-moxie code to the new moxie port.
> 
> 
> Signed-off-by: Anthony Green <address@hidden>
> ---
>  target-moxie/Makefile.objs |    2 +
>  target-moxie/cpu-qom.h     |   70 +++
>  target-moxie/cpu.c         |   84 +++
>  target-moxie/cpu.h         |  181 +++++++
>  target-moxie/exec.h        |   28 +
>  target-moxie/helper.c      |  136 +++++
>  target-moxie/helper.h      |    6 +
>  target-moxie/machine.c     |   15 +
>  target-moxie/mmu.c         |   41 ++
>  target-moxie/mmu.h         |   20 +
>  target-moxie/op_helper.c   |   81 +++
>  target-moxie/translate.c   | 1222 
> ++++++++++++++++++++++++++++++++++++++++++++
>  12 files changed, 1886 insertions(+)
>  create mode 100644 target-moxie/Makefile.objs
>  create mode 100644 target-moxie/cpu-qom.h
>  create mode 100644 target-moxie/cpu.c
>  create mode 100644 target-moxie/cpu.h
>  create mode 100644 target-moxie/exec.h
>  create mode 100644 target-moxie/helper.c
>  create mode 100644 target-moxie/helper.h
>  create mode 100644 target-moxie/machine.c
>  create mode 100644 target-moxie/mmu.c
>  create mode 100644 target-moxie/mmu.h
>  create mode 100644 target-moxie/op_helper.c
>  create mode 100644 target-moxie/translate.c

Some general comments: For a new target, please do things the new way,
then you won't need a cpu-qom.h file, cf. target-openrisc/cpu.h.

Please take a look at the patches queued on qom-cpu-next for the next
merge window:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
You should rebase on that (then using [PATCH qom-cpu-next n/m] or [PATCH
qom-cpu n/m]) and change cpu_moxie_init() accordingly so that it sets
realized = true and doesn't do CPU reset, vCPU init or TCG
initialization from there and move the remaining cpu_moxie_init() to
your cpu.c.

Please drop the #if 0'ed bogus cpu_reset() implementation in translate.c.

Please see my previous messages on the topic of using CPUMoxieState vs.
MoxieCPU, with a preference for MoxieCPU except for TCG helpers:
http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg05885.html
Your gen_intermediate_code_internal() for instance would be a candidate
for MoxieCPU, and it should use bool type (and true/false) for search_pc
argument, same for any other yes/no logic.

You don't implement migration support (no CPU_SAVE_VERSION in cpu.h at
least), so please set dc->vmsd to a VMStateDescription marked as
unmigratable, as done for virtually all such targets for 1.4. Please
drop machine.c if you don't #define CPU_SAVE_VERSION, this bug was
recently fixed:
http://git.qemu.org/?p=qemu.git;a=commit;h=3ce8b2bcbff6445f84db53ef38dbc4e5dd102676

Are you sure register struct asm(AREG0) in exec.h is needed???
Blue had reworked targets to not rely on a fixed asm register
incompatible with clang/LLVM, I thought.

Please run checkpatch.pl and drop any spaces before parenthesis for
instance and use a consistent 4-char indentation.

Please use inline functions and macros to avoid hardcoded printf()s,
e.g., in cpu_moxie_handle_mmu_fault(). Old code uses DPRINTF() macros,
as a heads-up a newer proposal  was made recently for tmp105.c file:
http://lists.nongnu.org/archive/html/qemu-devel/2013-02/msg01573.html
The general idea is to keep format strings from bitrotting when
#ifdef'ed out, details may change in response to review feedback.

I also recommend against #ifdef'ing blocks of code that touch on TCG- or
tree-wide fields that may get refactored, breaking such debug code
(e.g., DEBUG_DISAS using env). If the corresponding log level is not
set, it won't execute anyway. You can use
unlikely(qemu_loglevel_mask(...)) to optimize the common case.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg