Re: [Qemu-devel] [PATCH 0/4] new Blackfin QEMU port

[Edgar E. Iglesias]

On Mon, Jan 24, 2011 at 05:23:01AM -0500, Mike Frysinger wrote:
> I finally got around to getting this port past the "only crashes" phase.
> Using the GNU sim Blackfin port as a nice working standard, it was just
> a matter of replacing the sim bits with TCG ops.  Now we have a linux-user
> port for people to play with.  No immediate plans to move on to the system
> step, but it is something I want to figure out at some point.
> 
> The port is at the point I think where people can bang on it.  The core
> (bfin-sim.c) needs a bit more cleanup though before it'll be accepted,
> but that should be largely related to the insns that I haven't gotten
> around to implementing.  The rest of the code should be fair game for
> review though if someone feels like it.
> 
> This does require some patches in order for userspace programs to be
> generally useful, but they've all been submitted at this point.
> 
> I don't know how people feel about test sizes.  We developed quite a
> comprehension testsuite for the GNU sim, and I've imported about half
> of it.  Personally, I find it invaluable to catch regressions while
> developing, but its raw size can be a bit intimidating.
> 
> As a simple comparison, we typically get about ~8mips with the GNU sim
> while executing Blackfin code, but I easily see about ~400mips with qemu.
> Obviously your numbers will vary based on workload, but hopefully not
> too much ...
> 
> My git tree with everything necessary can be found here:
>       git://sources.blackfin.uclinux.org/git/users/vapier/qemu.git


Hi,

First of all, nice work! Good to see that you've managed to get your
port running.

I've had a quick look at your git and have a few (mostly general)
comments:

* There's quite a bit of dead code left around. Please remove as
  much as possible to make things easier to read.

* There's a lot of code that doesn't use braces around if code
  bodies, for example in op_helper.c.

* Some operations seem to translate to large amounts of tcg code,
  maybe you should consider using helpers for those. An example
  is gen_rot_tl.

* Many of the helpers seem to be PURE and CONST but not declared
  so. See tcg/README for more info.

* Maybe you should rename target-bfin/opcode/bfin.h into
  target-bfin/opcodes.h? or similiar..

* gen_hwloop_check() seems to be looking at env->lbreg from the
  translator, is that OK? What happens if env->lbreg changes at
  runtime?

* cpu_get_tb_cpu_state() doesn't define any tb flags?


target-bfin/README:

"There are some things we don't bother handling in the port for speed reasons.
If you want an accurate (but not as fast) simulator, then use the GNU sim as
found in the GNU toolchain (part of gdb)."

I think QEMU should aim to be fast and correct. There is no need to
be cycle accurate but emulation should IMO always aim to produce the
correct results.
If people provide patches to improve correctness, IMO those patches
should be encouraged & accepted as long as they dont introduce unnecessary
slowdowns (e.g, unless there are faster ways to achieve the same level of
correctness). My suggestion is that you just remove those first sentences.


"- transactional parallel instructions
         - on the hardware, if a load/store causes an exception, the other
           insns do not change register states either.  in qemu, they do,
           but since those exceptions will kill the program anyways, who
           cares.  no intermediate store buffers!"

This might be true for user emulation but I assume you'd want to fix this
for system emulation (at some point in time). Maybe you should just note
that it's not supported and leave it at that.

"        - unaligned memory access exceptions
                - qemu itself doesn't support this for targets"

There are targets that emulate trapping on unaligned memory accesses, for
example MicroBlaze and IIRC SPARC.

Cheers