Re: [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups

[Blue Swirl]

On Fri, Jun 25, 2010 at 12:52 PM, Paolo Bonzini <address@hidden> wrote:
> This is a different way to achieve the same objective as Isamu's patch.
> Basically, his patch becomes the (much simpler) patch 7 of this series,
> and everything else is something I had had lying around for a while. :)
>
> Patch 1 is simply Amit's patch, included here for convenience as it's
> not been applied yet.
>
> Patches 2 and 3 remove some dyngen-exec.h hacks at the price of requiring
> qemu-common.h included in more places.  I don't see this as a big price;
> all of these files were already including qemu-common.h indirectly,
> e.g. via cpu-all.h, just not early enough.
>
> Patches 4 provides a CPUState type, albeit an opaque one, to files that
> are not compiled per-target.  The advantage of this are apparent in
> patches 5 and 6: opaque pointers that are actually CPUState pointers
> are now type-safe, and it is even possible to define a cpu property type
> for the occasional device that has to be connected to a particular CPU
> (the PC APICs in particular).
>
> Finally, patch 7 "redoes" Isamu's patch just by moving five lines of
> code into qemu-common.h.

From just clean up or type safety point of view, this is good stuff.
But from architectural point of view, we should make it more difficult
to use CPUState in device code, not easier. This may still be fine as
a temporary measure, before all CPUState references have been removed.

I had some comments regarding individual patches.

> Amit Shah (1):
>  rtc: Remove TARGET_I386 from qemu-config.c, enables driftfix
>
> Paolo Bonzini (6):
>  include qemu-common.h when needed by the next patches
>  include stdio.h freely, remove dyngen-exec.h hacks
>  provide opaque CPUState to files that are compiled once
>  add qdev property type "cpu"
>  replace void* uses with opaque CPUState*
>  poison TARGET_xxx for compile once object
>
>  arm-semi.c                    |    2 +-
>  bsd-user/qemu.h               |    1 +
>  cpu-common.h                  |    6 +---
>  cpu-defs.h                    |    1 +
>  cpu-exec.c                    |    1 +
>  cpus.c                        |   39 ++++++++++++++++++++++--------------
>  cpus.h                        |    2 +
>  darwin-user/qemu.h            |    1 +
>  disas.c                       |    1 +
>  disas.h                       |    5 +---
>  dyngen-exec.h                 |   16 --------------
>  exec.c                        |    2 +-
>  hw/apic.c                     |    4 +-
>  hw/pc.c                       |    4 +-
>  hw/qdev-properties.c          |   44 
> +++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h                     |    5 ++++
>  linux-user/arm/nwfpe/fpa11.h  |    3 +-
>  linux-user/main.c             |    1 -
>  linux-user/qemu.h             |    1 +
>  m68k-semi.c                   |    2 +-
>  poison.h                      |    3 --
>  qemu-common.h                 |   19 ++++-------------
>  qemu-config.c                 |    2 -
>  target-alpha/cpu.h            |    4 +--
>  target-alpha/exec.h           |    6 +---
>  target-alpha/helper.c         |    1 +
>  target-alpha/op_helper.c      |    1 +
>  target-alpha/translate.c      |    2 +-
>  target-arm/cpu.h              |    6 ++--
>  target-arm/exec.h             |    5 +--
>  target-arm/helper.c           |    2 +-
>  target-arm/iwmmxt_helper.c    |    1 +
>  target-arm/neon_helper.c      |    1 +
>  target-arm/op_helper.c        |    1 +
>  target-arm/translate.c        |    1 +
>  target-cris/cpu.h             |    6 ++--
>  target-cris/exec.h            |    6 ++--
>  target-cris/helper.c          |    1 +
>  target-cris/mmu.c             |    1 +
>  target-cris/op_helper.c       |    1 +
>  target-cris/translate.c       |    2 +-
>  target-i386/cpu.h             |    6 ++--
>  target-i386/cpuid.c           |    1 +
>  target-i386/exec.h            |    7 +----
>  target-i386/helper.c          |    2 +-
>  target-i386/op_helper.c       |    1 +
>  target-i386/translate.c       |    1 +
>  target-m68k/cpu.h             |    6 ++--
>  target-m68k/exec.h            |    6 ++--
>  target-m68k/helper.c          |    2 +-
>  target-m68k/op_helper.c       |    1 +
>  target-m68k/translate.c       |    1 +
>  target-microblaze/cpu.h       |    7 ++---
>  target-microblaze/exec.h      |    6 ++--
>  target-microblaze/helper.c    |    1 +
>  target-microblaze/mmu.c       |    1 +
>  target-microblaze/op_helper.c |    1 +
>  target-microblaze/translate.c |    2 +-
>  target-mips/cpu.h             |    5 +---
>  target-mips/exec.h            |    6 +---
>  target-mips/helper.c          |    1 +
>  target-mips/op_helper.c       |    1 +
>  target-mips/translate.c       |    2 +-
>  target-ppc/cpu.h              |    3 +-
>  target-ppc/exec.h             |    2 -
>  target-ppc/helper.c           |    2 +-
>  target-ppc/op_helper.c        |    1 +
>  target-ppc/translate.c        |    2 +-
>  target-s390x/cpu.h            |    6 ++--
>  target-s390x/exec.h           |    7 ++---
>  target-s390x/helper.c         |    2 +-
>  target-s390x/op_helper.c      |    1 +
>  target-sh4/cpu.h              |    6 ++--
>  target-sh4/exec.h             |    5 +--
>  target-sh4/helper.c           |    1 +
>  target-sh4/op_helper.c        |    2 +
>  target-sh4/translate.c        |    2 +-
>  target-sparc/cpu.h            |    6 ++--
>  target-sparc/exec.h           |    3 ++
>  target-sparc/helper.c         |    2 +-
>  target-sparc/op_helper.c      |    1 +
>  target-sparc/translate.c      |    1 +
>  translate-all.c               |    1 +
>  83 files changed, 189 insertions(+), 147 deletions(-)
>
>
>