All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups
@ 2010-06-25 12:52 Paolo Bonzini
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 1/7] rtc: Remove TARGET_I386 from qemu-config.c, enables driftfix Paolo Bonzini
                   ` (8 more replies)
  0 siblings, 9 replies; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-25 12:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Isaku Yamahata

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.


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(-)

^ permalink raw reply	[flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 1/7] rtc: Remove TARGET_I386 from qemu-config.c, enables driftfix
  2010-06-25 12:52 [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Paolo Bonzini
@ 2010-06-25 12:52 ` Paolo Bonzini
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 2/7] include qemu-common.h when needed by the next patches Paolo Bonzini
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-25 12:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Isaku Yamahata

From: Amit Shah <amit.shah@redhat.com>

qemu-config.c doesn't contain any target-specific code, and the
TARGET_I386 conditional code didn't get compiled as a result. Removing
this enables the driftfix parameter for rtc.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-config.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index 95abe61..730ffd9 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -247,11 +247,9 @@ QemuOptsList qemu_rtc_opts = {
         },{
             .name = "clock",
             .type = QEMU_OPT_STRING,
-#ifdef TARGET_I386
         },{
             .name = "driftfix",
             .type = QEMU_OPT_STRING,
-#endif
         },
         { /* end if list */ }
     },
-- 
1.7.0.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 2/7] include qemu-common.h when needed by the next patches
  2010-06-25 12:52 [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Paolo Bonzini
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 1/7] rtc: Remove TARGET_I386 from qemu-config.c, enables driftfix Paolo Bonzini
@ 2010-06-25 12:52 ` Paolo Bonzini
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 3/7] include stdio.h freely, remove dyngen-exec.h hacks Paolo Bonzini
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-25 12:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Isaku Yamahata

All of these files were already including qemu-common.h indirectly,
e.g. via cpu-all.h, just not early enough.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arm-semi.c                    |    2 +-
 bsd-user/qemu.h               |    1 +
 cpu-exec.c                    |    1 +
 darwin-user/qemu.h            |    1 +
 disas.c                       |    1 +
 exec.c                        |    2 +-
 linux-user/arm/nwfpe/fpa11.h  |    3 ++-
 linux-user/main.c             |    1 -
 linux-user/qemu.h             |    1 +
 m68k-semi.c                   |    2 +-
 target-alpha/helper.c         |    1 +
 target-alpha/op_helper.c      |    1 +
 target-alpha/translate.c      |    2 +-
 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/helper.c          |    1 +
 target-cris/mmu.c             |    1 +
 target-cris/op_helper.c       |    1 +
 target-cris/translate.c       |    2 +-
 target-i386/cpuid.c           |    1 +
 target-i386/helper.c          |    2 +-
 target-i386/op_helper.c       |    1 +
 target-i386/translate.c       |    1 +
 target-m68k/helper.c          |    2 +-
 target-m68k/op_helper.c       |    1 +
 target-m68k/translate.c       |    1 +
 target-microblaze/helper.c    |    1 +
 target-microblaze/mmu.c       |    1 +
 target-microblaze/op_helper.c |    1 +
 target-microblaze/translate.c |    2 +-
 target-mips/helper.c          |    1 +
 target-mips/op_helper.c       |    1 +
 target-mips/translate.c       |    2 +-
 target-ppc/helper.c           |    2 +-
 target-ppc/op_helper.c        |    1 +
 target-ppc/translate.c        |    2 +-
 target-s390x/helper.c         |    2 +-
 target-s390x/op_helper.c      |    1 +
 target-sh4/helper.c           |    1 +
 target-sh4/op_helper.c        |    2 ++
 target-sh4/translate.c        |    2 +-
 target-sparc/helper.c         |    2 +-
 target-sparc/op_helper.c      |    1 +
 target-sparc/translate.c      |    1 +
 translate-all.c               |    1 +
 48 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/arm-semi.c b/arm-semi.c
index 0687b03..4c5ab65 100644
--- a/arm-semi.c
+++ b/arm-semi.c
@@ -26,7 +26,7 @@
 #include <stdio.h>
 #include <time.h>
 
-#include "cpu.h"
+#include "config.h"
 #ifdef CONFIG_USER_ONLY
 #include "qemu.h"
 
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 554ff8b..6450571 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -4,6 +4,7 @@
 #include <signal.h>
 #include <string.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 
 #undef DEBUG_REMAP
diff --git a/cpu-exec.c b/cpu-exec.c
index 026980a..e4e0def 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "config.h"
+#include "qemu-common.h"
 #include "exec.h"
 #include "disas.h"
 #include "tcg.h"
diff --git a/darwin-user/qemu.h b/darwin-user/qemu.h
index 462bbda..a5d53ea 100644
--- a/darwin-user/qemu.h
+++ b/darwin-user/qemu.h
@@ -4,6 +4,7 @@
 #include <signal.h>
 #include <string.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 
 #include "thunk.h"
diff --git a/disas.c b/disas.c
index 79a98de..2905459 100644
--- a/disas.c
+++ b/disas.c
@@ -1,5 +1,6 @@
 /* General "disassemble this chunk" code.  Used for debugging. */
 #include "config.h"
+#include "qemu-common.h"
 #include "dis-asm.h"
 #include "elf.h"
 #include <errno.h>
diff --git a/exec.c b/exec.c
index 7f64384..8b61259 100644
--- a/exec.c
+++ b/exec.c
@@ -31,9 +31,9 @@
 #include <unistd.h>
 #include <inttypes.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
-#include "qemu-common.h"
 #include "tcg.h"
 #include "hw/hw.h"
 #include "osdep.h"
diff --git a/linux-user/arm/nwfpe/fpa11.h b/linux-user/arm/nwfpe/fpa11.h
index 07419e2..0e64897 100644
--- a/linux-user/arm/nwfpe/fpa11.h
+++ b/linux-user/arm/nwfpe/fpa11.h
@@ -25,7 +25,8 @@
 #include <stdio.h>
 #include <errno.h>
 
-#include <cpu.h>
+#include "qemu-common.h"
+#include "cpu.h"
 
 #define GET_FPA11() (qemufpa)
 
diff --git a/linux-user/main.c b/linux-user/main.c
index 403c8d3..e0511ee 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -27,7 +27,6 @@
 #include <sys/resource.h>
 
 #include "qemu.h"
-#include "qemu-common.h"
 #include "cache-utils.h"
 /* For tb_lock */
 #include "exec-all.h"
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 1878d5a..e2bd7f8 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -4,6 +4,7 @@
 #include <signal.h>
 #include <string.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 
 #undef DEBUG_REMAP
diff --git a/m68k-semi.c b/m68k-semi.c
index d16bc67..7baa97b 100644
--- a/m68k-semi.c
+++ b/m68k-semi.c
@@ -27,7 +27,7 @@
 #include <sys/time.h>
 #include <time.h>
 
-#include "cpu.h"
+#include "config.h"
 #if defined(CONFIG_USER_ONLY)
 #include "qemu.h"
 #define SEMIHOSTING_HEAP_SIZE (128 * 1024 * 1024)
diff --git a/target-alpha/helper.c b/target-alpha/helper.c
index b6d2160..043dbc1 100644
--- a/target-alpha/helper.c
+++ b/target-alpha/helper.c
@@ -21,6 +21,7 @@
 #include <stdlib.h>
 #include <stdio.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "softfloat.h"
diff --git a/target-alpha/op_helper.c b/target-alpha/op_helper.c
index ff5ae26..39a6a85 100644
--- a/target-alpha/op_helper.c
+++ b/target-alpha/op_helper.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "qemu-common.h"
 #include "exec.h"
 #include "host-utils.h"
 #include "softfloat.h"
diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index 3a1c625..72eeb09 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -21,12 +21,12 @@
 #include <stdlib.h>
 #include <stdio.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "disas.h"
 #include "host-utils.h"
 #include "tcg-op.h"
-#include "qemu-common.h"
 
 #include "helper.h"
 #define GEN_HELPER 1
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 63e5dc7..79f5be8 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2,11 +2,11 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "gdbstub.h"
 #include "helpers.h"
-#include "qemu-common.h"
 #include "host-utils.h"
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/loader.h"
diff --git a/target-arm/iwmmxt_helper.c b/target-arm/iwmmxt_helper.c
index 3332f70..ec8c456 100644
--- a/target-arm/iwmmxt_helper.c
+++ b/target-arm/iwmmxt_helper.c
@@ -22,6 +22,7 @@
 #include <stdlib.h>
 #include <stdio.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "helpers.h"
diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index 5e6452b..ae9d955 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -9,6 +9,7 @@
 #include <stdlib.h>
 #include <stdio.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "helpers.h"
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 9b1a014..6f0f4ce 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -16,6 +16,7 @@
  * 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/>.
  */
+#include "qemu-common.h"
 #include "exec.h"
 #include "helpers.h"
 
diff --git a/target-arm/translate.c b/target-arm/translate.c
index a28e2ff..3c37626 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -24,6 +24,7 @@
 #include <string.h>
 #include <inttypes.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "disas.h"
diff --git a/target-cris/helper.c b/target-cris/helper.c
index 240bda0..e781aa9 100644
--- a/target-cris/helper.c
+++ b/target-cris/helper.c
@@ -22,6 +22,7 @@
 #include <string.h>
 
 #include "config.h"
+#include "qemu-common.h"
 #include "cpu.h"
 #include "mmu.h"
 #include "exec-all.h"
diff --git a/target-cris/mmu.c b/target-cris/mmu.c
index d09e921..bc6c8da 100644
--- a/target-cris/mmu.c
+++ b/target-cris/mmu.c
@@ -25,6 +25,7 @@
 #include <stdlib.h>
 
 #include "config.h"
+#include "qemu-common.h"
 #include "cpu.h"
 #include "mmu.h"
 #include "exec-all.h"
diff --git a/target-cris/op_helper.c b/target-cris/op_helper.c
index a60da94..9e21799 100644
--- a/target-cris/op_helper.c
+++ b/target-cris/op_helper.c
@@ -18,6 +18,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "qemu-common.h"
 #include "exec.h"
 #include "mmu.h"
 #include "helper.h"
diff --git a/target-cris/translate.c b/target-cris/translate.c
index 45c7682..b4bc99b 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -29,6 +29,7 @@
 #include <string.h>
 #include <inttypes.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "disas.h"
@@ -36,7 +37,6 @@
 #include "helper.h"
 #include "mmu.h"
 #include "crisv32-decode.h"
-#include "qemu-common.h"
 
 #define GEN_HELPER 1
 #include "helper.h"
diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 6a0f7ca..c47518e 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -21,6 +21,7 @@
 #include <string.h>
 #include <inttypes.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "kvm.h"
 
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 718394c..3eabb44 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -23,9 +23,9 @@
 #include <inttypes.h>
 #include <signal.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
-#include "qemu-common.h"
 #include "kvm.h"
 
 //#define DEBUG_MMU
diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
index c1256f4..d68be64 100644
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #define CPU_NO_GLOBAL_REGS
+#include "qemu-common.h"
 #include "exec.h"
 #include "exec-all.h"
 #include "host-utils.h"
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 708b0a1..cbc2bcd 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -23,6 +23,7 @@
 #include <inttypes.h>
 #include <signal.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "disas.h"
diff --git a/target-m68k/helper.c b/target-m68k/helper.c
index b4ebb14..a0d1e35 100644
--- a/target-m68k/helper.c
+++ b/target-m68k/helper.c
@@ -22,9 +22,9 @@
 #include <string.h>
 
 #include "config.h"
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
-#include "qemu-common.h"
 #include "gdbstub.h"
 
 #include "helpers.h"
diff --git a/target-m68k/op_helper.c b/target-m68k/op_helper.c
index 0711107..b98d257 100644
--- a/target-m68k/op_helper.c
+++ b/target-m68k/op_helper.c
@@ -16,6 +16,7 @@
  * 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/>.
  */
+#include "qemu-common.h"
 #include "exec.h"
 #include "helpers.h"
 
diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 5351880..234e263 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -24,6 +24,7 @@
 #include <inttypes.h>
 
 #include "config.h"
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "disas.h"
diff --git a/target-microblaze/helper.c b/target-microblaze/helper.c
index 5230b52..e575fe9 100644
--- a/target-microblaze/helper.c
+++ b/target-microblaze/helper.c
@@ -22,6 +22,7 @@
 #include <assert.h>
 
 #include "config.h"
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "host-utils.h"
diff --git a/target-microblaze/mmu.c b/target-microblaze/mmu.c
index b38f7d9..224c2fd 100644
--- a/target-microblaze/mmu.c
+++ b/target-microblaze/mmu.c
@@ -20,6 +20,7 @@
 #include <stdlib.h>
 #include <assert.h>
 
+#include "qemu-common.h"
 #include "config.h"
 #include "cpu.h"
 #include "exec-all.h"
diff --git a/target-microblaze/op_helper.c b/target-microblaze/op_helper.c
index be0c829..568fe88 100644
--- a/target-microblaze/op_helper.c
+++ b/target-microblaze/op_helper.c
@@ -18,6 +18,7 @@
  */
 
 #include <assert.h>
+#include "qemu-common.h"
 #include "exec.h"
 #include "helper.h"
 #include "host-utils.h"
diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
index ca54e2c..f7a01fe 100644
--- a/target-microblaze/translate.c
+++ b/target-microblaze/translate.c
@@ -24,13 +24,13 @@
 #include <inttypes.h>
 #include <assert.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "disas.h"
 #include "tcg-op.h"
 #include "helper.h"
 #include "microblaze-decode.h"
-#include "qemu-common.h"
 
 #define GEN_HELPER 1
 #include "helper.h"
diff --git a/target-mips/helper.c b/target-mips/helper.c
index ea221ab..84bf8a2 100644
--- a/target-mips/helper.c
+++ b/target-mips/helper.c
@@ -23,6 +23,7 @@
 #include <inttypes.h>
 #include <signal.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index d09d6ed..c0e56f4 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include <stdlib.h>
+#include "qemu-common.h"
 #include "exec.h"
 
 #include "host-utils.h"
diff --git a/target-mips/translate.c b/target-mips/translate.c
index d43d72d..e4d94f6 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -26,11 +26,11 @@
 #include <string.h>
 #include <inttypes.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "disas.h"
 #include "tcg-op.h"
-#include "qemu-common.h"
 
 #include "helper.h"
 #define GEN_HELPER 1
diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index d342b09..a114eb9 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -23,10 +23,10 @@
 #include <inttypes.h>
 #include <signal.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "helper_regs.h"
-#include "qemu-common.h"
 #include "kvm.h"
 
 //#define DEBUG_MMU
diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 3c3aa60..89faf93 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include <string.h>
+#include "qemu-common.h"
 #include "exec.h"
 #include "host-utils.h"
 #include "helper.h"
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 66e1c0d..ff6f7c1 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -22,11 +22,11 @@
 #include <string.h>
 #include <inttypes.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "disas.h"
 #include "tcg-op.h"
-#include "qemu-common.h"
 #include "host-utils.h"
 
 #include "helper.h"
diff --git a/target-s390x/helper.c b/target-s390x/helper.c
index 4a5297b..d474bca 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -21,10 +21,10 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "gdbstub.h"
-#include "qemu-common.h"
 
 #include <linux/kvm.h>
 #include "kvm.h"
diff --git a/target-s390x/op_helper.c b/target-s390x/op_helper.c
index 402df2d..54982f3 100644
--- a/target-s390x/op_helper.c
+++ b/target-s390x/op_helper.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "qemu-common.h"
 #include "exec.h"
 
 /*****************************************************************************/
diff --git a/target-sh4/helper.c b/target-sh4/helper.c
index 9e70352..63d48c1 100644
--- a/target-sh4/helper.c
+++ b/target-sh4/helper.c
@@ -23,6 +23,7 @@
 #include <inttypes.h>
 #include <signal.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "hw/sh_intc.h"
diff --git a/target-sh4/op_helper.c b/target-sh4/op_helper.c
index 2e5f555..93d2684 100644
--- a/target-sh4/op_helper.c
+++ b/target-sh4/op_helper.c
@@ -18,6 +18,8 @@
  */
 #include <assert.h>
 #include <stdlib.h>
+
+#include "qemu-common.h"
 #include "exec.h"
 #include "helper.h"
 
diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index d0d6c00..3f0393a 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -26,11 +26,11 @@
 #define SH4_DEBUG_DISAS
 //#define SH4_SINGLE_STEP
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "disas.h"
 #include "tcg-op.h"
-#include "qemu-common.h"
 
 #include "helper.h"
 #define GEN_HELPER 1
diff --git a/target-sparc/helper.c b/target-sparc/helper.c
index aa1fd63..1c6319f 100644
--- a/target-sparc/helper.c
+++ b/target-sparc/helper.c
@@ -23,9 +23,9 @@
 #include <inttypes.h>
 #include <signal.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
-#include "qemu-common.h"
 
 //#define DEBUG_MMU
 //#define DEBUG_FEATURES
diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c
index be3c1e0..547793f 100644
--- a/target-sparc/op_helper.c
+++ b/target-sparc/op_helper.c
@@ -1,3 +1,4 @@
+#include "qemu-common.h"
 #include "exec.h"
 #include "host-utils.h"
 #include "helper.h"
diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index 23f9519..b91355b 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -24,6 +24,7 @@
 #include <string.h>
 #include <inttypes.h>
 
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "disas.h"
diff --git a/translate-all.c b/translate-all.c
index efcfb9a..d9fe23f 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -25,6 +25,7 @@
 #include "config.h"
 
 #define NO_CPU_IO_DEFS
+#include "qemu-common.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "disas.h"
-- 
1.7.0.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 3/7] include stdio.h freely, remove dyngen-exec.h hacks
  2010-06-25 12:52 [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Paolo Bonzini
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 1/7] rtc: Remove TARGET_I386 from qemu-config.c, enables driftfix Paolo Bonzini
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 2/7] include qemu-common.h when needed by the next patches Paolo Bonzini
@ 2010-06-25 12:52 ` Paolo Bonzini
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 4/7] provide opaque CPUState to files that are compiled once Paolo Bonzini
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-25 12:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Isaku Yamahata

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 disas.h       |    5 +----
 dyngen-exec.h |   16 ----------------
 qemu-common.h |    7 -------
 3 files changed, 1 insertions(+), 27 deletions(-)

diff --git a/disas.h b/disas.h
index 6a9332d..1af0511 100644
--- a/disas.h
+++ b/disas.h
@@ -2,17 +2,14 @@
 #define _QEMU_DISAS_H
 
 #include "qemu-common.h"
+#include <stdio.h>
 
 #ifdef NEED_CPU_H
 /* Disassemble this for me please... (debugging). */
 void disas(FILE *out, void *code, unsigned long size);
 void target_disas(FILE *out, target_ulong code, target_ulong size, int flags);
-
-/* The usual mess... FIXME: Remove this condition once dyngen-exec.h is gone */
-#ifndef __DYNGEN_EXEC_H__
 void monitor_disas(Monitor *mon, CPUState *env,
                    target_ulong pc, int nb_insn, int is_physical, int flags);
-#endif
 
 /* Look up symbol for debugging purpose.  Returns "" if unknown. */
 const char *lookup_symbol(target_ulong orig_addr);
diff --git a/dyngen-exec.h b/dyngen-exec.h
index 5bfef3f..d65b618 100644
--- a/dyngen-exec.h
+++ b/dyngen-exec.h
@@ -19,13 +19,6 @@
 #if !defined(__DYNGEN_EXEC_H__)
 #define __DYNGEN_EXEC_H__
 
-/* prevent Solaris from trying to typedef FILE in gcc's
-   include/floatingpoint.h which will conflict with the
-   definition down below */
-#ifdef __sun__
-#define _FILEDEFED
-#endif
-
 /* NOTE: standard headers should be used with special care at this
    point because host CPU registers are used as global variables. Some
    host headers do not allow that. */
@@ -40,15 +33,6 @@
 /* XXX: This may be wrong for 64-bit ILP32 hosts.  */
 typedef void * host_reg_t;
 
-#ifdef CONFIG_BSD
-typedef struct __sFILE FILE;
-#else
-typedef struct FILE FILE;
-#endif
-extern int fprintf(FILE *, const char *, ...);
-extern int fputs(const char *, FILE *);
-extern int printf(const char *, ...);
-
 #if defined(__i386__)
 #define AREG0 "ebp"
 #elif defined(__x86_64__)
diff --git a/qemu-common.h b/qemu-common.h
index 3fb2f0b..ac839aa 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -18,11 +18,6 @@ typedef struct QEMUFile QEMUFile;
 typedef struct QEMUBH QEMUBH;
 typedef struct DeviceState DeviceState;
 
-/* Hack around the mess dyngen-exec.h causes: We need QEMU_NORETURN in files that
-   cannot include the following headers without conflicts. This condition has
-   to be removed once dyngen is gone. */
-#ifndef __DYNGEN_EXEC_H__
-
 /* we put basic includes here to avoid repeating them in device drivers */
 #include <stdlib.h>
 #include <stdio.h>
@@ -293,6 +288,4 @@ static inline uint8_t from_bcd(uint8_t val)
 
 #include "module.h"
 
-#endif /* dyngen-exec.h hack */
-
 #endif
-- 
1.7.0.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 4/7] provide opaque CPUState to files that are compiled once
  2010-06-25 12:52 [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Paolo Bonzini
                   ` (2 preceding siblings ...)
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 3/7] include stdio.h freely, remove dyngen-exec.h hacks Paolo Bonzini
@ 2010-06-25 12:52 ` Paolo Bonzini
  2010-06-27 19:17   ` Blue Swirl
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 5/7] add qdev property type "cpu" Paolo Bonzini
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-25 12:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Isaku Yamahata

This patch unpoisons CPUState and env in once-compiled files.
To achieve this, it defines an opaque struct CPUState in cpu-common.h.
This also requires tweaking the relationship between CPUState and
CPUXYZState in target files.

Unpoisoning env is needed because it is widely used as the name for
CPUState arguments.  To avoid having references to the global register
variable creeping into target-independent files, the patch rationalizes
inclusions at the head of target-*/exec.h.  All exec.h files now include
cpu.h explicitly and very early.  Inclusions from machine-independent
context will then error out in cpu-defs.h, even if env is not poisoned.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-common.h             |    3 +++
 cpu-defs.h               |    1 +
 poison.h                 |    3 ---
 target-alpha/cpu.h       |    4 +---
 target-alpha/exec.h      |    6 ++----
 target-arm/cpu.h         |    6 +++---
 target-arm/exec.h        |    5 ++---
 target-cris/cpu.h        |    6 +++---
 target-cris/exec.h       |    6 +++---
 target-i386/cpu.h        |    6 +++---
 target-i386/exec.h       |    7 ++-----
 target-m68k/cpu.h        |    6 +++---
 target-m68k/exec.h       |    6 +++---
 target-microblaze/cpu.h  |    7 +++----
 target-microblaze/exec.h |    6 +++---
 target-mips/cpu.h        |    5 +----
 target-mips/exec.h       |    6 ++----
 target-ppc/cpu.h         |    3 +--
 target-ppc/exec.h        |    2 --
 target-s390x/cpu.h       |    6 +++---
 target-s390x/exec.h      |    7 +++----
 target-sh4/cpu.h         |    6 +++---
 target-sh4/exec.h        |    5 ++---
 target-sparc/cpu.h       |    6 +++---
 target-sparc/exec.h      |    3 +++
 25 files changed, 56 insertions(+), 71 deletions(-)

diff --git a/cpu-common.h b/cpu-common.h
index b24cecc..f325e60 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -18,6 +18,9 @@
 #include "bswap.h"
 #include "qemu-queue.h"
 
+struct CPUState;
+typedef struct CPUState CPUState;
+
 #if !defined(CONFIG_USER_ONLY)
 
 /* address in the RAM (different from a physical address) */
diff --git a/cpu-defs.h b/cpu-defs.h
index 8d4bf86..f56e85b 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -30,6 +30,7 @@
 #include "osdep.h"
 #include "qemu-queue.h"
 #include "targphys.h"
+#include "cpu-common.h"
 
 #ifndef TARGET_LONG_BITS
 #error TARGET_LONG_BITS must be defined before including this header
diff --git a/poison.h b/poison.h
index d7db7f4..e7814cb 100644
--- a/poison.h
+++ b/poison.h
@@ -33,9 +33,6 @@
 #pragma GCC poison TARGET_PAGE_BITS
 #pragma GCC poison TARGET_PAGE_ALIGN
 
-#pragma GCC poison CPUState
-#pragma GCC poison env
-
 #pragma GCC poison CPU_INTERRUPT_HARD
 #pragma GCC poison CPU_INTERRUPT_EXITTB
 #pragma GCC poison CPU_INTERRUPT_TIMER
diff --git a/target-alpha/cpu.h b/target-alpha/cpu.h
index 314d6ac..795b2bd 100644
--- a/target-alpha/cpu.h
+++ b/target-alpha/cpu.h
@@ -24,7 +24,7 @@
 
 #define TARGET_LONG_BITS 64
 
-#define CPUState struct CPUAlphaState
+#define CPUAlphaState CPUState
 
 #include "cpu-defs.h"
 
@@ -317,8 +317,6 @@ enum {
     IPR_LAST,
 };
 
-typedef struct CPUAlphaState CPUAlphaState;
-
 typedef struct pal_handler_t pal_handler_t;
 struct pal_handler_t {
     /* Reset */
diff --git a/target-alpha/exec.h b/target-alpha/exec.h
index 66526e2..789305f 100644
--- a/target-alpha/exec.h
+++ b/target-alpha/exec.h
@@ -21,8 +21,9 @@
 #define __ALPHA_EXEC_H__
 
 #include "config.h"
-
 #include "dyngen-exec.h"
+#include "cpu.h"
+#include "exec-all.h"
 
 #define TARGET_LONG_BITS 64
 
@@ -32,9 +33,6 @@ register struct CPUAlphaState *env asm(AREG0);
 #define SPARAM(n) ((int32_t)PARAM##n)
 #define FP_STATUS (env->fp_status)
 
-#include "cpu.h"
-#include "exec-all.h"
-
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif /* !defined(CONFIG_USER_ONLY) */
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index f3d138d..b6cf887 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -23,7 +23,7 @@
 
 #define ELF_MACHINE	EM_ARM
 
-#define CPUState struct CPUARMState
+#define CPUARMState CPUState
 
 #include "cpu-defs.h"
 
@@ -70,7 +70,7 @@ struct arm_boot_info;
    s<2n+1> maps to the most significant half of d<n>
  */
 
-typedef struct CPUARMState {
+struct CPUARMState {
     /* Regs for current mode.  */
     uint32_t regs[16];
     /* Frequently accessed CPSR bits are stored separately for efficiently.
@@ -206,7 +206,7 @@ typedef struct CPUARMState {
     } cp[15];
     void *nvic;
     struct arm_boot_info *boot_info;
-} CPUARMState;
+};
 
 CPUARMState *cpu_arm_init(const char *cpu_model);
 void arm_translate_init(void);
diff --git a/target-arm/exec.h b/target-arm/exec.h
index 0225c3f..4042eca 100644
--- a/target-arm/exec.h
+++ b/target-arm/exec.h
@@ -18,14 +18,13 @@
  */
 #include "config.h"
 #include "dyngen-exec.h"
+#include "cpu.h"
+#include "exec-all.h"
 
 register struct CPUARMState *env asm(AREG0);
 
 #define M0   env->iwmmxt.val
 
-#include "cpu.h"
-#include "exec-all.h"
-
 static inline int cpu_has_work(CPUState *env)
 {
     return (env->interrupt_request &
diff --git a/target-cris/cpu.h b/target-cris/cpu.h
index a62d57c..6cb080a 100644
--- a/target-cris/cpu.h
+++ b/target-cris/cpu.h
@@ -22,7 +22,7 @@
 
 #define TARGET_LONG_BITS 32
 
-#define CPUState struct CPUCRISState
+#define CPUCRISState CPUState
 
 #include "cpu-defs.h"
 
@@ -96,7 +96,7 @@
 
 #define NB_MMU_MODES 2
 
-typedef struct CPUCRISState {
+struct CPUCRISState {
 	uint32_t regs[16];
 	/* P0 - P15 are referred to as special registers in the docs.  */
 	uint32_t pregs[16];
@@ -158,7 +158,7 @@ typedef struct CPUCRISState {
 	void *load_info;
 
 	CPU_COMMON
-} CPUCRISState;
+};
 
 CPUCRISState *cpu_cris_init(const char *cpu_model);
 int cpu_cris_exec(CPUCRISState *s);
diff --git a/target-cris/exec.h b/target-cris/exec.h
index 728aa80..af0103d 100644
--- a/target-cris/exec.h
+++ b/target-cris/exec.h
@@ -17,13 +17,13 @@
  * 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/>.
  */
+#include "config.h"
 #include "dyngen-exec.h"
-
-register struct CPUCRISState *env asm(AREG0);
-
 #include "cpu.h"
 #include "exec-all.h"
 
+register struct CPUCRISState *env asm(AREG0);
+
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 8dafa0d..929f252 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -41,7 +41,7 @@
 #define ELF_MACHINE	EM_386
 #endif
 
-#define CPUState struct CPUX86State
+#define CPUX86State CPUState
 
 #include "cpu-defs.h"
 
@@ -581,7 +581,7 @@ typedef struct {
 
 #define NB_MMU_MODES 2
 
-typedef struct CPUX86State {
+struct CPUX86State {
     /* standard registers */
     target_ulong regs[CPU_NB_REGS];
     target_ulong eip;
@@ -718,7 +718,7 @@ typedef struct CPUX86State {
     uint16_t fpus_vmstate;
     uint16_t fptag_vmstate;
     uint16_t fpregs_format_vmstate;
-} CPUX86State;
+};
 
 CPUX86State *cpu_x86_init(const char *cpu_model);
 int cpu_x86_exec(CPUX86State *s);
diff --git a/target-i386/exec.h b/target-i386/exec.h
index 4ff3c57..29b741a 100644
--- a/target-i386/exec.h
+++ b/target-i386/exec.h
@@ -18,6 +18,8 @@
  */
 #include "config.h"
 #include "dyngen-exec.h"
+#include "cpu.h"
+#include "exec-all.h"
 
 /* XXX: factorize this mess */
 #ifdef TARGET_X86_64
@@ -26,8 +28,6 @@
 #define TARGET_LONG_BITS 32
 #endif
 
-#include "cpu-defs.h"
-
 register struct CPUX86State *env asm(AREG0);
 
 #include "qemu-common.h"
@@ -63,9 +63,6 @@ register struct CPUX86State *env asm(AREG0);
 #define ST(n)  (env->fpregs[(env->fpstt + (n)) & 7].d)
 #define ST1    ST(1)
 
-#include "cpu.h"
-#include "exec-all.h"
-
 /* op_helper.c */
 void do_interrupt(int intno, int is_int, int error_code,
                   target_ulong next_eip, int is_hw);
diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
index b2f37ec..ac2fa84 100644
--- a/target-m68k/cpu.h
+++ b/target-m68k/cpu.h
@@ -22,7 +22,7 @@
 
 #define TARGET_LONG_BITS 32
 
-#define CPUState struct CPUM68KState
+#define CPUM68KState CPUState
 
 #include "cpu-defs.h"
 
@@ -56,7 +56,7 @@
 
 #define NB_MMU_MODES 2
 
-typedef struct CPUM68KState {
+struct CPUM68KState {
     uint32_t dregs[8];
     uint32_t aregs[8];
     uint32_t pc;
@@ -112,7 +112,7 @@ typedef struct CPUM68KState {
     CPU_COMMON
 
     uint32_t features;
-} CPUM68KState;
+};
 
 void m68k_tcg_init(void);
 CPUM68KState *cpu_m68k_init(const char *cpu_model);
diff --git a/target-m68k/exec.h b/target-m68k/exec.h
index ece9aa0..b611282 100644
--- a/target-m68k/exec.h
+++ b/target-m68k/exec.h
@@ -17,13 +17,13 @@
  * 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/>.
  */
+#include "config.h"
 #include "dyngen-exec.h"
-
-register struct CPUM68KState *env asm(AREG0);
-
 #include "cpu.h"
 #include "exec-all.h"
 
+register struct CPUM68KState *env asm(AREG0);
+
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif
diff --git a/target-microblaze/cpu.h b/target-microblaze/cpu.h
index ff8c8c8..f40f57f 100644
--- a/target-microblaze/cpu.h
+++ b/target-microblaze/cpu.h
@@ -21,10 +21,9 @@
 
 #define TARGET_LONG_BITS 32
 
-#define CPUState struct CPUMBState
+#define CPUMBState CPUState
 
 #include "cpu-defs.h"
-struct CPUMBState;
 #if !defined(CONFIG_USER_ONLY)
 #include "mmu.h"
 #endif
@@ -199,7 +198,7 @@ struct CPUMBState;
 #define CC_EQ  0
 
 #define NB_MMU_MODES    3
-typedef struct CPUMBState {
+struct CPUMBState {
     uint32_t debug;
     uint32_t btaken;
     uint32_t btarget;
@@ -230,7 +229,7 @@ typedef struct CPUMBState {
 #endif
 
     CPU_COMMON
-} CPUMBState;
+};
 
 CPUState *cpu_mb_init(const char *cpu_model);
 int cpu_mb_exec(CPUState *s);
diff --git a/target-microblaze/exec.h b/target-microblaze/exec.h
index 646701c..813d3d6 100644
--- a/target-microblaze/exec.h
+++ b/target-microblaze/exec.h
@@ -16,13 +16,13 @@
  * 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/>.
  */
+#include "config.h"
 #include "dyngen-exec.h"
-
-register struct CPUMBState *env asm(AREG0);
-
 #include "cpu.h"
 #include "exec-all.h"
 
+register struct CPUMBState *env asm(AREG0);
+
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif
diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index c21b8e4..b0e86c2 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -5,7 +5,7 @@
 
 #define ELF_MACHINE	EM_MIPS
 
-#define CPUState struct CPUMIPSState
+#define CPUMIPSState CPUState
 
 #include "config.h"
 #include "mips-defs.h"
@@ -19,8 +19,6 @@ typedef unsigned char           uint_fast8_t;
 typedef unsigned int            uint_fast16_t;
 #endif
 
-struct CPUMIPSState;
-
 typedef struct r4k_tlb_t r4k_tlb_t;
 struct r4k_tlb_t {
     target_ulong VPN;
@@ -172,7 +170,6 @@ struct TCState {
     int32_t CP0_Debug_tcstatus;
 };
 
-typedef struct CPUMIPSState CPUMIPSState;
 struct CPUMIPSState {
     TCState active_tc;
     CPUMIPSFPUContext active_fpu;
diff --git a/target-mips/exec.h b/target-mips/exec.h
index 01e9c4d..070e425 100644
--- a/target-mips/exec.h
+++ b/target-mips/exec.h
@@ -6,13 +6,11 @@
 #include "config.h"
 #include "mips-defs.h"
 #include "dyngen-exec.h"
-#include "cpu-defs.h"
-
-register struct CPUMIPSState *env asm(AREG0);
-
 #include "cpu.h"
 #include "exec-all.h"
 
+register struct CPUMIPSState *env asm(AREG0);
+
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif /* !defined(CONFIG_USER_ONLY) */
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 2ad4486..a47f12b 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -69,7 +69,7 @@
 
 #endif /* defined (TARGET_PPC64) */
 
-#define CPUState struct CPUPPCState
+#define CPUPPCState CPUState
 
 #include "cpu-defs.h"
 
@@ -300,7 +300,6 @@ typedef struct opc_handler_t opc_handler_t;
 
 /*****************************************************************************/
 /* Types used to describe some PowerPC registers */
-typedef struct CPUPPCState CPUPPCState;
 typedef struct ppc_tb_t ppc_tb_t;
 typedef struct ppc_spr_t ppc_spr_t;
 typedef struct ppc_dcr_t ppc_dcr_t;
diff --git a/target-ppc/exec.h b/target-ppc/exec.h
index 09f592c..651f91a 100644
--- a/target-ppc/exec.h
+++ b/target-ppc/exec.h
@@ -20,9 +20,7 @@
 #define __PPC_H__
 
 #include "config.h"
-
 #include "dyngen-exec.h"
-
 #include "cpu.h"
 #include "exec-all.h"
 
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index dd407b2..d30e9da 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -23,7 +23,7 @@
 
 #define ELF_MACHINE	EM_S390
 
-#define CPUState struct CPUS390XState
+#define CPUS390XState CPUState
 
 #include "cpu-defs.h"
 
@@ -45,7 +45,7 @@ typedef union FPReg {
     uint64_t i;
 } FPReg;
 
-typedef struct CPUS390XState {
+struct CPUS390XState {
     uint64_t regs[16];	/* GP registers */
 
     uint32_t aregs[16];	/* access registers */
@@ -64,7 +64,7 @@ typedef struct CPUS390XState {
     uint64_t __excp_addr;
 
     CPU_COMMON
-} CPUS390XState;
+};
 
 #if defined(CONFIG_USER_ONLY)
 static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
diff --git a/target-s390x/exec.h b/target-s390x/exec.h
index 837f853..a848f73 100644
--- a/target-s390x/exec.h
+++ b/target-s390x/exec.h
@@ -17,14 +17,13 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "dyngen-exec.h"
-
-register struct CPUS390XState *env asm(AREG0);
-
 #include "config.h"
+#include "dyngen-exec.h"
 #include "cpu.h"
 #include "exec-all.h"
 
+register struct CPUS390XState *env asm(AREG0);
+
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif /* !defined(CONFIG_USER_ONLY) */
diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
index f8b1680..5535ed1 100644
--- a/target-sh4/cpu.h
+++ b/target-sh4/cpu.h
@@ -36,7 +36,7 @@
 #define SH_CPU_SH7750_ALL (SH_CPU_SH7750 | SH_CPU_SH7750S | SH_CPU_SH7750R)
 #define SH_CPU_SH7751_ALL (SH_CPU_SH7751 | SH_CPU_SH7751R)
 
-#define CPUState struct CPUSH4State
+#define CPUSH4State CPUState
 
 #include "cpu-defs.h"
 
@@ -107,7 +107,7 @@ typedef struct memory_content {
     struct memory_content *next;
 } memory_content;
 
-typedef struct CPUSH4State {
+struct CPUSH4State {
     int id;			/* CPU model */
 
     uint32_t flags;		/* general execution flags */
@@ -157,7 +157,7 @@ typedef struct CPUSH4State {
     int intr_at_halt;		/* SR_BL ignored during sleep */
     memory_content *movcal_backup;
     memory_content **movcal_backup_tail;
-} CPUSH4State;
+};
 
 CPUSH4State *cpu_sh4_init(const char *cpu_model);
 int cpu_sh4_exec(CPUSH4State * s);
diff --git a/target-sh4/exec.h b/target-sh4/exec.h
index edd667d..b2eb306 100644
--- a/target-sh4/exec.h
+++ b/target-sh4/exec.h
@@ -21,12 +21,11 @@
 
 #include "config.h"
 #include "dyngen-exec.h"
-
-register struct CPUSH4State *env asm(AREG0);
-
 #include "cpu.h"
 #include "exec-all.h"
 
+register struct CPUSH4State *env asm(AREG0);
+
 static inline int cpu_has_work(CPUState *env)
 {
     return (env->interrupt_request & CPU_INTERRUPT_HARD);
diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
index 8f0484b..a852ee4 100644
--- a/target-sparc/cpu.h
+++ b/target-sparc/cpu.h
@@ -21,7 +21,7 @@
 # endif
 #endif
 
-#define CPUState struct CPUSPARCState
+#define CPUSPARCState CPUState
 
 #include "cpu-defs.h"
 
@@ -318,7 +318,7 @@ struct QEMUFile;
 void cpu_put_timer(struct QEMUFile *f, CPUTimer *s);
 void cpu_get_timer(struct QEMUFile *f, CPUTimer *s);
 
-typedef struct CPUSPARCState {
+struct CPUSPARCState {
     target_ulong gregs[8]; /* general registers */
     target_ulong *regwptr; /* pointer to current register window */
     target_ulong pc;       /* program counter */
@@ -436,7 +436,7 @@ typedef struct CPUSPARCState {
 #define SOFTINT_REG_MASK (SOFTINT_STIMER|SOFTINT_INTRMASK|SOFTINT_TIMER)
 #endif
     sparc_def_t *def;
-} CPUSPARCState;
+};
 
 #ifndef NO_CPU_IO_DEFS
 /* helper.c */
diff --git a/target-sparc/exec.h b/target-sparc/exec.h
index c84e055..260b91e 100644
--- a/target-sparc/exec.h
+++ b/target-sparc/exec.h
@@ -1,7 +1,10 @@
 #ifndef EXEC_SPARC_H
 #define EXEC_SPARC_H 1
+
 #include "config.h"
 #include "dyngen-exec.h"
+#include "cpu.h"
+#include "exec-all.h"
 
 register struct CPUSPARCState *env asm(AREG0);
 
-- 
1.7.0.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 5/7] add qdev property type "cpu"
  2010-06-25 12:52 [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Paolo Bonzini
                   ` (3 preceding siblings ...)
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 4/7] provide opaque CPUState to files that are compiled once Paolo Bonzini
@ 2010-06-25 12:52 ` Paolo Bonzini
  2010-06-26  7:46   ` Markus Armbruster
  2010-06-27 18:45   ` [Qemu-devel] " Blue Swirl
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 6/7] replace void* uses with opaque CPUState* Paolo Bonzini
                   ` (3 subsequent siblings)
  8 siblings, 2 replies; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-25 12:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Isaku Yamahata

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c               |   16 ++++++++++++++++
 cpus.h               |    2 ++
 hw/qdev-properties.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h            |    5 +++++
 4 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/cpus.c b/cpus.c
index fcd0f09..da6ec44 100644
--- a/cpus.c
+++ b/cpus.c
@@ -91,6 +91,22 @@ void cpu_synchronize_all_post_init(void)
     }
 }
 
+CPUState *cpu_get_by_id(int id)
+{
+    CPUState *cpu;
+
+    for (cpu = first_cpu; cpu; cpu = cpu->next_cpu)
+        if (cpu->cpu_index == id)
+            return cpu;
+
+    return NULL;
+}
+
+int cpu_get_id(CPUState *env)
+{
+    return env->cpu_index;
+}
+
 int cpu_is_stopped(CPUState *env)
 {
     return !vm_running || env->stopped;
diff --git a/cpus.h b/cpus.h
index 774150a..df3c193 100644
--- a/cpus.h
+++ b/cpus.h
@@ -6,6 +6,8 @@ int qemu_init_main_loop(void);
 void qemu_main_loop_start(void);
 void resume_all_vcpus(void);
 void pause_all_vcpus(void);
+CPUState *cpu_get_by_id(int id);
+int cpu_get_id(CPUState *env);
 
 /* vl.c */
 extern int smp_cores;
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 5a8739d..2759c83 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -1,6 +1,7 @@
 #include "net.h"
 #include "qdev.h"
 #include "qerror.h"
+#include "cpus.h"
 
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
 {
@@ -281,6 +282,44 @@ PropertyInfo qdev_prop_string = {
     .free  = free_string,
 };
 
+/* --- cpu --- */
+
+static int parse_cpu(DeviceState *dev, Property *prop, const char *str)
+{
+    CPUState **ptr = qdev_get_prop_ptr(dev, prop);
+    char *end;
+    int id;
+
+    if (!*str)
+        return -ENOENT;
+
+    id = strtol (str, &end, 0);
+    if (*end)
+        return -ENOENT;
+
+    *ptr = cpu_get_by_id(id);
+    if (*ptr == NULL)
+        return -ENOENT;
+    return 0;
+}
+
+static int print_cpu(DeviceState *dev, Property *prop, char *dest, size_t len)
+{
+    CPUState **ptr = qdev_get_prop_ptr(dev, prop);
+    if (*ptr)
+        return snprintf(dest, len, "CPU #%d", cpu_get_id(*ptr));
+    else
+	return snprintf(dest, len, "CPU #<null>");
+}
+
+PropertyInfo qdev_prop_cpu = {
+    .name  = "cpu",
+    .type  = PROP_TYPE_CPU,
+    .size  = sizeof(DriveInfo*),
+    .parse = parse_cpu,
+    .print = print_cpu,
+};
+
 /* --- drive --- */
 
 static int parse_drive(DeviceState *dev, Property *prop, const char *str)
@@ -657,6 +696,11 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
     qdev_prop_set(dev, name, &value, PROP_TYPE_PTR);
 }
 
+void qdev_prop_set_cpu(DeviceState *dev, const char *name, CPUState *value)
+{
+    qdev_prop_set(dev, name, &value, PROP_TYPE_CPU);
+}
+
 void qdev_prop_set_defaults(DeviceState *dev, Property *props)
 {
     if (!props)
diff --git a/hw/qdev.h b/hw/qdev.h
index be5ad67..eec2f52 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -90,6 +90,7 @@ enum PropertyType {
     PROP_TYPE_VLAN,
     PROP_TYPE_PTR,
     PROP_TYPE_BIT,
+    PROP_TYPE_CPU,
 };
 
 struct PropertyInfo {
@@ -203,6 +204,7 @@ extern PropertyInfo qdev_prop_drive;
 extern PropertyInfo qdev_prop_netdev;
 extern PropertyInfo qdev_prop_vlan;
 extern PropertyInfo qdev_prop_pci_devfn;
+extern PropertyInfo qdev_prop_cpu;
 
 #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
         .name      = (_name),                                    \
@@ -257,6 +259,8 @@ extern PropertyInfo qdev_prop_pci_devfn;
     DEFINE_PROP(_n, _s, _f, qdev_prop_drive, DriveInfo*)
 #define DEFINE_PROP_MACADDR(_n, _s, _f)         \
     DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr)
+#define DEFINE_PROP_CPU(_n, _s, _f)             \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_cpu, CPUState*)
 
 #define DEFINE_PROP_END_OF_LIST()               \
     {}
@@ -276,6 +280,7 @@ void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *valu
 void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *value);
 void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState *value);
 void qdev_prop_set_drive(DeviceState *dev, const char *name, DriveInfo *value);
+void qdev_prop_set_cpu(DeviceState *dev, const char *name, CPUState *value);
 void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value);
 /* FIXME: Remove opaque pointer properties.  */
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
-- 
1.7.0.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 6/7] replace void* uses with opaque CPUState*
  2010-06-25 12:52 [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Paolo Bonzini
                   ` (4 preceding siblings ...)
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 5/7] add qdev property type "cpu" Paolo Bonzini
@ 2010-06-25 12:52 ` Paolo Bonzini
  2010-06-26  7:49   ` Markus Armbruster
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 7/7] poison TARGET_xxx for compile once object Paolo Bonzini
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-25 12:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Isaku Yamahata

Because we all love type safety, don't we?

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-common.h  |    1 -
 cpus.c        |   23 ++++++++---------------
 hw/apic.c     |    4 ++--
 hw/pc.c       |    4 ++--
 qemu-common.h |    7 ++++---
 5 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/cpu-common.h b/cpu-common.h
index f325e60..d905258 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -19,7 +19,6 @@
 #include "qemu-queue.h"
 
 struct CPUState;
-typedef struct CPUState CPUState;
 
 #if !defined(CONFIG_USER_ONLY)
 
diff --git a/cpus.c b/cpus.c
index da6ec44..5b62e27 100644
--- a/cpus.c
+++ b/cpus.c
@@ -259,10 +259,8 @@ void qemu_main_loop_start(void)
 {
 }
 
-void qemu_init_vcpu(void *_env)
+void qemu_init_vcpu(CPUState *env)
 {
-    CPUState *env = _env;
-
     env->nr_cores = smp_cores;
     env->nr_threads = smp_threads;
     if (kvm_enabled())
@@ -270,7 +268,7 @@ void qemu_init_vcpu(void *_env)
     return;
 }
 
-int qemu_cpu_self(void *env)
+int qemu_cpu_self(CPUState *env)
 {
     return 1;
 }
@@ -288,7 +286,7 @@ void pause_all_vcpus(void)
 {
 }
 
-void qemu_cpu_kick(void *env)
+void qemu_cpu_kick(CPUState *env)
 {
     return;
 }
@@ -524,16 +522,14 @@ static void *tcg_cpu_thread_fn(void *arg)
     return NULL;
 }
 
-void qemu_cpu_kick(void *_env)
+void qemu_cpu_kick(CPUState *env)
 {
-    CPUState *env = _env;
     qemu_cond_broadcast(env->halt_cond);
     qemu_thread_signal(env->thread, SIG_IPI);
 }
 
-int qemu_cpu_self(void *_env)
+int qemu_cpu_self(CPUState *env)
 {
-    CPUState *env = _env;
     QemuThread this;
 
     qemu_thread_self(&this);
@@ -666,9 +662,8 @@ void resume_all_vcpus(void)
     }
 }
 
-static void tcg_init_vcpu(void *_env)
+static void tcg_init_vcpu(CPUState *env)
 {
-    CPUState *env = _env;
     /* share a single thread for all cpus with TCG */
     if (!tcg_cpu_thread) {
         env->thread = qemu_mallocz(sizeof(QemuThread));
@@ -695,10 +690,8 @@ static void kvm_start_vcpu(CPUState *env)
         qemu_cond_timedwait(&qemu_cpu_cond, &qemu_global_mutex, 100);
 }
 
-void qemu_init_vcpu(void *_env)
+void qemu_init_vcpu(CPUState *env)
 {
-    CPUState *env = _env;
-
     env->nr_cores = smp_cores;
     env->nr_threads = smp_threads;
     if (kvm_enabled())
@@ -840,7 +833,7 @@ void set_cpu_log(const char *optarg)
 int64_t cpu_get_icount(void)
 {
     int64_t icount;
-    CPUState *env = cpu_single_env;;
+    CPUState *env = cpu_single_env;
 
     icount = qemu_icount;
     if (env) {
diff --git a/hw/apic.c b/hw/apic.c
index d686b51..85737c4 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -94,7 +94,7 @@ typedef struct APICState APICState;
 
 struct APICState {
     SysBusDevice busdev;
-    void *cpu_env;
+    CPUState *cpu_env;
     uint32_t apicbase;
     uint8_t id;
     uint8_t arb_id;
@@ -1006,7 +1006,7 @@ static SysBusDeviceInfo apic_info = {
     .qdev.no_user = 1,
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT8("id", APICState, id, -1),
-        DEFINE_PROP_PTR("cpu_env", APICState, cpu_env),
+        DEFINE_PROP_CPU("cpu_env", APICState, cpu_env),
         DEFINE_PROP_END_OF_LIST(),
     }
 };
diff --git a/hw/pc.c b/hw/pc.c
index 1848151..0497260 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -766,7 +766,7 @@ DeviceState *cpu_get_current_apic(void)
     }
 }
 
-static DeviceState *apic_init(void *env, uint8_t apic_id)
+static DeviceState *apic_init(CPUState *env, uint8_t apic_id)
 {
     DeviceState *dev;
     SysBusDevice *d;
@@ -774,7 +774,7 @@ static DeviceState *apic_init(void *env, uint8_t apic_id)
 
     dev = qdev_create(NULL, "apic");
     qdev_prop_set_uint8(dev, "id", apic_id);
-    qdev_prop_set_ptr(dev, "cpu_env", env);
+    qdev_prop_set_cpu(dev, "cpu_env", env);
     qdev_init_nofail(dev);
     d = sysbus_from_qdev(dev);
 
diff --git a/qemu-common.h b/qemu-common.h
index ac839aa..8339cb1 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -17,6 +17,7 @@ typedef struct QEMUTimer QEMUTimer;
 typedef struct QEMUFile QEMUFile;
 typedef struct QEMUBH QEMUBH;
 typedef struct DeviceState DeviceState;
+typedef struct CPUState CPUState;
 
 /* we put basic includes here to avoid repeating them in device drivers */
 #include <stdlib.h>
@@ -239,8 +240,8 @@ void qemu_service_io(void);
 void qemu_notify_event(void);
 
 /* Unblock cpu */
-void qemu_cpu_kick(void *env);
-int qemu_cpu_self(void *env);
+void qemu_cpu_kick(CPUState *env);
+int qemu_cpu_self(CPUState *env);
 
 /* work queue */
 struct qemu_work_item {
@@ -253,7 +254,7 @@ struct qemu_work_item {
 #ifdef CONFIG_USER_ONLY
 #define qemu_init_vcpu(env) do { } while (0)
 #else
-void qemu_init_vcpu(void *env);
+void qemu_init_vcpu(CPUState *env);
 #endif
 
 typedef struct QEMUIOVector {
-- 
1.7.0.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 7/7] poison TARGET_xxx for compile once object
  2010-06-25 12:52 [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Paolo Bonzini
                   ` (5 preceding siblings ...)
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 6/7] replace void* uses with opaque CPUState* Paolo Bonzini
@ 2010-06-25 12:52 ` Paolo Bonzini
  2010-06-25 22:47 ` [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Richard Henderson
  2010-06-27 19:32 ` [Qemu-devel] " Blue Swirl
  8 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-25 12:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Isaku Yamahata

prevents those ifdefs from creeping in again.

Cc: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-common.h  |    4 ----
 qemu-common.h |    5 +----
 2 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/cpu-common.h b/cpu-common.h
index d905258..639c58d 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -11,10 +11,6 @@
 #include "targphys.h"
 #endif
 
-#ifndef NEED_CPU_H
-#include "poison.h"
-#endif
-
 #include "bswap.h"
 #include "qemu-queue.h"
 
diff --git a/qemu-common.h b/qemu-common.h
index 8339cb1..423639b 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -86,15 +86,12 @@ static inline char *realpath(const char *path, char *resolved_path)
 
 /* FIXME: Remove NEED_CPU_H.  */
 #ifndef NEED_CPU_H
-
 #include <setjmp.h>
 #include "osdep.h"
 #include "bswap.h"
-
+#include "poison.h"
 #else
-
 #include "cpu.h"
-
 #endif /* !defined(NEED_CPU_H) */
 
 /* bottom halves */
-- 
1.7.0.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups
  2010-06-25 12:52 [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Paolo Bonzini
                   ` (6 preceding siblings ...)
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 7/7] poison TARGET_xxx for compile once object Paolo Bonzini
@ 2010-06-25 22:47 ` Richard Henderson
  2010-06-27 12:39   ` [Qemu-devel] " Paolo Bonzini
  2010-06-27 19:32 ` [Qemu-devel] " Blue Swirl
  8 siblings, 1 reply; 23+ messages in thread
From: Richard Henderson @ 2010-06-25 22:47 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

On 06/25/2010 05:52 AM, Paolo Bonzini 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.
> 
> 
> 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

Reviewed-by: Richard Henderson <rth@twiddle.net>

I like this cleanup.  Although I would personally prefer an additional
patch that removes the define silliness that patch 4 works around.  In
other words I think there's no point in having CPUARMState et al; we
should use CPUState universally.


r~

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] add qdev property type "cpu"
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 5/7] add qdev property type "cpu" Paolo Bonzini
@ 2010-06-26  7:46   ` Markus Armbruster
  2010-06-27 12:48     ` [Qemu-devel] " Paolo Bonzini
  2010-06-27 18:45   ` [Qemu-devel] " Blue Swirl
  1 sibling, 1 reply; 23+ messages in thread
From: Markus Armbruster @ 2010-06-26  7:46 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

Paolo Bonzini <pbonzini@redhat.com> writes:

[...]
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 5a8739d..2759c83 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -1,6 +1,7 @@
>  #include "net.h"
>  #include "qdev.h"
>  #include "qerror.h"
> +#include "cpus.h"
>  
>  void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
>  {
> @@ -281,6 +282,44 @@ PropertyInfo qdev_prop_string = {
>      .free  = free_string,
>  };
>  
> +/* --- cpu --- */
> +
> +static int parse_cpu(DeviceState *dev, Property *prop, const char *str)
> +{
> +    CPUState **ptr = qdev_get_prop_ptr(dev, prop);
> +    char *end;
> +    int id;
> +
> +    if (!*str)
> +        return -ENOENT;
> +
> +    id = strtol (str, &end, 0);
> +    if (*end)
> +        return -ENOENT;
> +
> +    *ptr = cpu_get_by_id(id);
> +    if (*ptr == NULL)
> +        return -ENOENT;
> +    return 0;
> +}
> +
> +static int print_cpu(DeviceState *dev, Property *prop, char *dest, size_t len)
> +{
> +    CPUState **ptr = qdev_get_prop_ptr(dev, prop);
> +    if (*ptr)
> +        return snprintf(dest, len, "CPU #%d", cpu_get_id(*ptr));
> +    else
> +	return snprintf(dest, len, "CPU #<null>");
> +}

Hmm.  Parse method doesn't accept output of the print method.  Not so
nice.  Is the "CPU #" decoration essential?

Oh, and beware of the Secret Tab Police.

[...]

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] replace void* uses with opaque CPUState*
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 6/7] replace void* uses with opaque CPUState* Paolo Bonzini
@ 2010-06-26  7:49   ` Markus Armbruster
  0 siblings, 0 replies; 23+ messages in thread
From: Markus Armbruster @ 2010-06-26  7:49 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

Paolo Bonzini <pbonzini@redhat.com> writes:

> Because we all love type safety, don't we?

And incomplete types!  Much better choice for an abstract data type than
abusing poor old "void *".

You also get rid of a use of the DEFINE_PROP_PTR() abomination.  Thanks!

^ permalink raw reply	[flat|nested] 23+ messages in thread
* [Qemu-devel] Re: [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups
  2010-06-25 22:47 ` [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Richard Henderson
@ 2010-06-27 12:39   ` Paolo Bonzini
  0 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-27 12:39 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

On 06/26/2010 12:47 AM, Richard Henderson wrote:
> Although I would personally prefer an additional
> patch that removes the define silliness that patch 4 works around.

Sure---I just wanted to avoid getting lost in conflicts until the first 
part is approved.

Paolo

^ permalink raw reply	[flat|nested] 23+ messages in thread
* [Qemu-devel] Re: [PATCH 5/7] add qdev property type "cpu"
  2010-06-26  7:46   ` Markus Armbruster
@ 2010-06-27 12:48     ` Paolo Bonzini
  2010-06-29 11:42       ` Markus Armbruster
  0 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-27 12:48 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

> Hmm.  Parse method doesn't accept output of the print method.  Not so
> nice.  Is the "CPU #" decoration essential?

I noticed the same in parse/print string:

static int parse_string(DeviceState *dev, Property *prop, const char *str)
{
     char **ptr = qdev_get_prop_ptr(dev, prop);

     if (*ptr)
         qemu_free(*ptr);
     *ptr = qemu_strdup(str);
     return 0;
}

static int print_string(DeviceState *dev, Property *prop, char *dest, 
size_t len)
{
     char **ptr = qdev_get_prop_ptr(dev, prop);
     if (!*ptr)
         return snprintf(dest, len, "<null>");
     return snprintf(dest, len, "\"%s\"", *ptr);
}

It looks like printing representation is chosen "for the user", not for 
parsing.

Paolo

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] add qdev property type "cpu"
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 5/7] add qdev property type "cpu" Paolo Bonzini
  2010-06-26  7:46   ` Markus Armbruster
@ 2010-06-27 18:45   ` Blue Swirl
  1 sibling, 0 replies; 23+ messages in thread
From: Blue Swirl @ 2010-06-27 18:45 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

On Fri, Jun 25, 2010 at 12:52 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cpus.c               |   16 ++++++++++++++++
>  cpus.h               |    2 ++
>  hw/qdev-properties.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h            |    5 +++++
>  4 files changed, 67 insertions(+), 0 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index fcd0f09..da6ec44 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -91,6 +91,22 @@ void cpu_synchronize_all_post_init(void)
>     }
>  }
>
> +CPUState *cpu_get_by_id(int id)
> +{
> +    CPUState *cpu;
> +
> +    for (cpu = first_cpu; cpu; cpu = cpu->next_cpu)
> +        if (cpu->cpu_index == id)
> +            return cpu;

CODING_STYLE, also below.

> +
> +    return NULL;
> +}
> +
> +int cpu_get_id(CPUState *env)
> +{
> +    return env->cpu_index;
> +}
> +
>  int cpu_is_stopped(CPUState *env)
>  {
>     return !vm_running || env->stopped;
> diff --git a/cpus.h b/cpus.h
> index 774150a..df3c193 100644
> --- a/cpus.h
> +++ b/cpus.h
> @@ -6,6 +6,8 @@ int qemu_init_main_loop(void);
>  void qemu_main_loop_start(void);
>  void resume_all_vcpus(void);
>  void pause_all_vcpus(void);
> +CPUState *cpu_get_by_id(int id);
> +int cpu_get_id(CPUState *env);
>
>  /* vl.c */
>  extern int smp_cores;
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 5a8739d..2759c83 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -1,6 +1,7 @@
>  #include "net.h"
>  #include "qdev.h"
>  #include "qerror.h"
> +#include "cpus.h"
>
>  void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
>  {
> @@ -281,6 +282,44 @@ PropertyInfo qdev_prop_string = {
>     .free  = free_string,
>  };
>
> +/* --- cpu --- */
> +
> +static int parse_cpu(DeviceState *dev, Property *prop, const char *str)
> +{
> +    CPUState **ptr = qdev_get_prop_ptr(dev, prop);
> +    char *end;
> +    int id;
> +
> +    if (!*str)
> +        return -ENOENT;
> +
> +    id = strtol (str, &end, 0);
> +    if (*end)
> +        return -ENOENT;
> +
> +    *ptr = cpu_get_by_id(id);
> +    if (*ptr == NULL)
> +        return -ENOENT;
> +    return 0;
> +}
> +
> +static int print_cpu(DeviceState *dev, Property *prop, char *dest, size_t len)
> +{
> +    CPUState **ptr = qdev_get_prop_ptr(dev, prop);
> +    if (*ptr)
> +        return snprintf(dest, len, "CPU #%d", cpu_get_id(*ptr));
> +    else
> +       return snprintf(dest, len, "CPU #<null>");
> +}
> +
> +PropertyInfo qdev_prop_cpu = {
> +    .name  = "cpu",
> +    .type  = PROP_TYPE_CPU,
> +    .size  = sizeof(DriveInfo*),
> +    .parse = parse_cpu,
> +    .print = print_cpu,
> +};
> +
>  /* --- drive --- */
>
>  static int parse_drive(DeviceState *dev, Property *prop, const char *str)
> @@ -657,6 +696,11 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
>     qdev_prop_set(dev, name, &value, PROP_TYPE_PTR);
>  }
>
> +void qdev_prop_set_cpu(DeviceState *dev, const char *name, CPUState *value)
> +{
> +    qdev_prop_set(dev, name, &value, PROP_TYPE_CPU);
> +}
> +
>  void qdev_prop_set_defaults(DeviceState *dev, Property *props)
>  {
>     if (!props)
> diff --git a/hw/qdev.h b/hw/qdev.h
> index be5ad67..eec2f52 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -90,6 +90,7 @@ enum PropertyType {
>     PROP_TYPE_VLAN,
>     PROP_TYPE_PTR,
>     PROP_TYPE_BIT,
> +    PROP_TYPE_CPU,
>  };
>
>  struct PropertyInfo {
> @@ -203,6 +204,7 @@ extern PropertyInfo qdev_prop_drive;
>  extern PropertyInfo qdev_prop_netdev;
>  extern PropertyInfo qdev_prop_vlan;
>  extern PropertyInfo qdev_prop_pci_devfn;
> +extern PropertyInfo qdev_prop_cpu;
>
>  #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
>         .name      = (_name),                                    \
> @@ -257,6 +259,8 @@ extern PropertyInfo qdev_prop_pci_devfn;
>     DEFINE_PROP(_n, _s, _f, qdev_prop_drive, DriveInfo*)
>  #define DEFINE_PROP_MACADDR(_n, _s, _f)         \
>     DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr)
> +#define DEFINE_PROP_CPU(_n, _s, _f)             \
> +    DEFINE_PROP(_n, _s, _f, qdev_prop_cpu, CPUState*)
>
>  #define DEFINE_PROP_END_OF_LIST()               \
>     {}
> @@ -276,6 +280,7 @@ void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *valu
>  void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *value);
>  void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState *value);
>  void qdev_prop_set_drive(DeviceState *dev, const char *name, DriveInfo *value);
> +void qdev_prop_set_cpu(DeviceState *dev, const char *name, CPUState *value);
>  void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value);
>  /* FIXME: Remove opaque pointer properties.  */
>  void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
> --
> 1.7.0.1
>
>
>
>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] provide opaque CPUState to files that are compiled once
  2010-06-25 12:52 ` [Qemu-devel] [PATCH 4/7] provide opaque CPUState to files that are compiled once Paolo Bonzini
@ 2010-06-27 19:17   ` Blue Swirl
  2010-06-28  8:04     ` Paolo Bonzini
  0 siblings, 1 reply; 23+ messages in thread
From: Blue Swirl @ 2010-06-27 19:17 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

On Fri, Jun 25, 2010 at 12:52 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This patch unpoisons CPUState and env in once-compiled files.
> To achieve this, it defines an opaque struct CPUState in cpu-common.h.
> This also requires tweaking the relationship between CPUState and
> CPUXYZState in target files.
>
> Unpoisoning env is needed because it is widely used as the name for
> CPUState arguments.  To avoid having references to the global register
> variable creeping into target-independent files, the patch rationalizes
> inclusions at the head of target-*/exec.h.  All exec.h files now include
> cpu.h explicitly and very early.  Inclusions from machine-independent
> context will then error out in cpu-defs.h, even if env is not poisoned.

I'm not comfortable with this part. Accidental use of the global
register variable can cause subtle bugs. I'd rather rename 'env' to
something more obvious and less likely to collide, like
'global_reg_env' and always poison that. Then we could replace 'env1'
etc with just 'env'.

>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cpu-common.h             |    3 +++
>  cpu-defs.h               |    1 +
>  poison.h                 |    3 ---
>  target-alpha/cpu.h       |    4 +---
>  target-alpha/exec.h      |    6 ++----
>  target-arm/cpu.h         |    6 +++---
>  target-arm/exec.h        |    5 ++---
>  target-cris/cpu.h        |    6 +++---
>  target-cris/exec.h       |    6 +++---
>  target-i386/cpu.h        |    6 +++---
>  target-i386/exec.h       |    7 ++-----
>  target-m68k/cpu.h        |    6 +++---
>  target-m68k/exec.h       |    6 +++---
>  target-microblaze/cpu.h  |    7 +++----
>  target-microblaze/exec.h |    6 +++---
>  target-mips/cpu.h        |    5 +----
>  target-mips/exec.h       |    6 ++----
>  target-ppc/cpu.h         |    3 +--
>  target-ppc/exec.h        |    2 --
>  target-s390x/cpu.h       |    6 +++---
>  target-s390x/exec.h      |    7 +++----
>  target-sh4/cpu.h         |    6 +++---
>  target-sh4/exec.h        |    5 ++---
>  target-sparc/cpu.h       |    6 +++---
>  target-sparc/exec.h      |    3 +++
>  25 files changed, 56 insertions(+), 71 deletions(-)
>
> diff --git a/cpu-common.h b/cpu-common.h
> index b24cecc..f325e60 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -18,6 +18,9 @@
>  #include "bswap.h"
>  #include "qemu-queue.h"
>
> +struct CPUState;
> +typedef struct CPUState CPUState;
> +
>  #if !defined(CONFIG_USER_ONLY)
>
>  /* address in the RAM (different from a physical address) */
> diff --git a/cpu-defs.h b/cpu-defs.h
> index 8d4bf86..f56e85b 100644
> --- a/cpu-defs.h
> +++ b/cpu-defs.h
> @@ -30,6 +30,7 @@
>  #include "osdep.h"
>  #include "qemu-queue.h"
>  #include "targphys.h"
> +#include "cpu-common.h"
>
>  #ifndef TARGET_LONG_BITS
>  #error TARGET_LONG_BITS must be defined before including this header
> diff --git a/poison.h b/poison.h
> index d7db7f4..e7814cb 100644
> --- a/poison.h
> +++ b/poison.h
> @@ -33,9 +33,6 @@
>  #pragma GCC poison TARGET_PAGE_BITS
>  #pragma GCC poison TARGET_PAGE_ALIGN
>
> -#pragma GCC poison CPUState
> -#pragma GCC poison env
> -
>  #pragma GCC poison CPU_INTERRUPT_HARD
>  #pragma GCC poison CPU_INTERRUPT_EXITTB
>  #pragma GCC poison CPU_INTERRUPT_TIMER
> diff --git a/target-alpha/cpu.h b/target-alpha/cpu.h
> index 314d6ac..795b2bd 100644
> --- a/target-alpha/cpu.h
> +++ b/target-alpha/cpu.h
> @@ -24,7 +24,7 @@
>
>  #define TARGET_LONG_BITS 64
>
> -#define CPUState struct CPUAlphaState
> +#define CPUAlphaState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -317,8 +317,6 @@ enum {
>     IPR_LAST,
>  };
>
> -typedef struct CPUAlphaState CPUAlphaState;
> -
>  typedef struct pal_handler_t pal_handler_t;
>  struct pal_handler_t {
>     /* Reset */
> diff --git a/target-alpha/exec.h b/target-alpha/exec.h
> index 66526e2..789305f 100644
> --- a/target-alpha/exec.h
> +++ b/target-alpha/exec.h
> @@ -21,8 +21,9 @@
>  #define __ALPHA_EXEC_H__
>
>  #include "config.h"
> -
>  #include "dyngen-exec.h"
> +#include "cpu.h"
> +#include "exec-all.h"
>
>  #define TARGET_LONG_BITS 64
>
> @@ -32,9 +33,6 @@ register struct CPUAlphaState *env asm(AREG0);
>  #define SPARAM(n) ((int32_t)PARAM##n)
>  #define FP_STATUS (env->fp_status)
>
> -#include "cpu.h"
> -#include "exec-all.h"
> -
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif /* !defined(CONFIG_USER_ONLY) */
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index f3d138d..b6cf887 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -23,7 +23,7 @@
>
>  #define ELF_MACHINE    EM_ARM
>
> -#define CPUState struct CPUARMState
> +#define CPUARMState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -70,7 +70,7 @@ struct arm_boot_info;
>    s<2n+1> maps to the most significant half of d<n>
>  */
>
> -typedef struct CPUARMState {
> +struct CPUARMState {
>     /* Regs for current mode.  */
>     uint32_t regs[16];
>     /* Frequently accessed CPSR bits are stored separately for efficiently.
> @@ -206,7 +206,7 @@ typedef struct CPUARMState {
>     } cp[15];
>     void *nvic;
>     struct arm_boot_info *boot_info;
> -} CPUARMState;
> +};
>
>  CPUARMState *cpu_arm_init(const char *cpu_model);
>  void arm_translate_init(void);
> diff --git a/target-arm/exec.h b/target-arm/exec.h
> index 0225c3f..4042eca 100644
> --- a/target-arm/exec.h
> +++ b/target-arm/exec.h
> @@ -18,14 +18,13 @@
>  */
>  #include "config.h"
>  #include "dyngen-exec.h"
> +#include "cpu.h"
> +#include "exec-all.h"
>
>  register struct CPUARMState *env asm(AREG0);
>
>  #define M0   env->iwmmxt.val
>
> -#include "cpu.h"
> -#include "exec-all.h"
> -
>  static inline int cpu_has_work(CPUState *env)
>  {
>     return (env->interrupt_request &
> diff --git a/target-cris/cpu.h b/target-cris/cpu.h
> index a62d57c..6cb080a 100644
> --- a/target-cris/cpu.h
> +++ b/target-cris/cpu.h
> @@ -22,7 +22,7 @@
>
>  #define TARGET_LONG_BITS 32
>
> -#define CPUState struct CPUCRISState
> +#define CPUCRISState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -96,7 +96,7 @@
>
>  #define NB_MMU_MODES 2
>
> -typedef struct CPUCRISState {
> +struct CPUCRISState {
>        uint32_t regs[16];
>        /* P0 - P15 are referred to as special registers in the docs.  */
>        uint32_t pregs[16];
> @@ -158,7 +158,7 @@ typedef struct CPUCRISState {
>        void *load_info;
>
>        CPU_COMMON
> -} CPUCRISState;
> +};
>
>  CPUCRISState *cpu_cris_init(const char *cpu_model);
>  int cpu_cris_exec(CPUCRISState *s);
> diff --git a/target-cris/exec.h b/target-cris/exec.h
> index 728aa80..af0103d 100644
> --- a/target-cris/exec.h
> +++ b/target-cris/exec.h
> @@ -17,13 +17,13 @@
>  * 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/>.
>  */
> +#include "config.h"
>  #include "dyngen-exec.h"
> -
> -register struct CPUCRISState *env asm(AREG0);
> -
>  #include "cpu.h"
>  #include "exec-all.h"
>
> +register struct CPUCRISState *env asm(AREG0);
> +
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 8dafa0d..929f252 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -41,7 +41,7 @@
>  #define ELF_MACHINE    EM_386
>  #endif
>
> -#define CPUState struct CPUX86State
> +#define CPUX86State CPUState
>
>  #include "cpu-defs.h"
>
> @@ -581,7 +581,7 @@ typedef struct {
>
>  #define NB_MMU_MODES 2
>
> -typedef struct CPUX86State {
> +struct CPUX86State {
>     /* standard registers */
>     target_ulong regs[CPU_NB_REGS];
>     target_ulong eip;
> @@ -718,7 +718,7 @@ typedef struct CPUX86State {
>     uint16_t fpus_vmstate;
>     uint16_t fptag_vmstate;
>     uint16_t fpregs_format_vmstate;
> -} CPUX86State;
> +};
>
>  CPUX86State *cpu_x86_init(const char *cpu_model);
>  int cpu_x86_exec(CPUX86State *s);
> diff --git a/target-i386/exec.h b/target-i386/exec.h
> index 4ff3c57..29b741a 100644
> --- a/target-i386/exec.h
> +++ b/target-i386/exec.h
> @@ -18,6 +18,8 @@
>  */
>  #include "config.h"
>  #include "dyngen-exec.h"
> +#include "cpu.h"
> +#include "exec-all.h"
>
>  /* XXX: factorize this mess */
>  #ifdef TARGET_X86_64
> @@ -26,8 +28,6 @@
>  #define TARGET_LONG_BITS 32
>  #endif
>
> -#include "cpu-defs.h"
> -
>  register struct CPUX86State *env asm(AREG0);
>
>  #include "qemu-common.h"
> @@ -63,9 +63,6 @@ register struct CPUX86State *env asm(AREG0);
>  #define ST(n)  (env->fpregs[(env->fpstt + (n)) & 7].d)
>  #define ST1    ST(1)
>
> -#include "cpu.h"
> -#include "exec-all.h"
> -
>  /* op_helper.c */
>  void do_interrupt(int intno, int is_int, int error_code,
>                   target_ulong next_eip, int is_hw);
> diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
> index b2f37ec..ac2fa84 100644
> --- a/target-m68k/cpu.h
> +++ b/target-m68k/cpu.h
> @@ -22,7 +22,7 @@
>
>  #define TARGET_LONG_BITS 32
>
> -#define CPUState struct CPUM68KState
> +#define CPUM68KState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -56,7 +56,7 @@
>
>  #define NB_MMU_MODES 2
>
> -typedef struct CPUM68KState {
> +struct CPUM68KState {
>     uint32_t dregs[8];
>     uint32_t aregs[8];
>     uint32_t pc;
> @@ -112,7 +112,7 @@ typedef struct CPUM68KState {
>     CPU_COMMON
>
>     uint32_t features;
> -} CPUM68KState;
> +};
>
>  void m68k_tcg_init(void);
>  CPUM68KState *cpu_m68k_init(const char *cpu_model);
> diff --git a/target-m68k/exec.h b/target-m68k/exec.h
> index ece9aa0..b611282 100644
> --- a/target-m68k/exec.h
> +++ b/target-m68k/exec.h
> @@ -17,13 +17,13 @@
>  * 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/>.
>  */
> +#include "config.h"
>  #include "dyngen-exec.h"
> -
> -register struct CPUM68KState *env asm(AREG0);
> -
>  #include "cpu.h"
>  #include "exec-all.h"
>
> +register struct CPUM68KState *env asm(AREG0);
> +
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif
> diff --git a/target-microblaze/cpu.h b/target-microblaze/cpu.h
> index ff8c8c8..f40f57f 100644
> --- a/target-microblaze/cpu.h
> +++ b/target-microblaze/cpu.h
> @@ -21,10 +21,9 @@
>
>  #define TARGET_LONG_BITS 32
>
> -#define CPUState struct CPUMBState
> +#define CPUMBState CPUState
>
>  #include "cpu-defs.h"
> -struct CPUMBState;
>  #if !defined(CONFIG_USER_ONLY)
>  #include "mmu.h"
>  #endif
> @@ -199,7 +198,7 @@ struct CPUMBState;
>  #define CC_EQ  0
>
>  #define NB_MMU_MODES    3
> -typedef struct CPUMBState {
> +struct CPUMBState {
>     uint32_t debug;
>     uint32_t btaken;
>     uint32_t btarget;
> @@ -230,7 +229,7 @@ typedef struct CPUMBState {
>  #endif
>
>     CPU_COMMON
> -} CPUMBState;
> +};
>
>  CPUState *cpu_mb_init(const char *cpu_model);
>  int cpu_mb_exec(CPUState *s);
> diff --git a/target-microblaze/exec.h b/target-microblaze/exec.h
> index 646701c..813d3d6 100644
> --- a/target-microblaze/exec.h
> +++ b/target-microblaze/exec.h
> @@ -16,13 +16,13 @@
>  * 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/>.
>  */
> +#include "config.h"
>  #include "dyngen-exec.h"
> -
> -register struct CPUMBState *env asm(AREG0);
> -
>  #include "cpu.h"
>  #include "exec-all.h"
>
> +register struct CPUMBState *env asm(AREG0);
> +
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index c21b8e4..b0e86c2 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -5,7 +5,7 @@
>
>  #define ELF_MACHINE    EM_MIPS
>
> -#define CPUState struct CPUMIPSState
> +#define CPUMIPSState CPUState
>
>  #include "config.h"
>  #include "mips-defs.h"
> @@ -19,8 +19,6 @@ typedef unsigned char           uint_fast8_t;
>  typedef unsigned int            uint_fast16_t;
>  #endif
>
> -struct CPUMIPSState;
> -
>  typedef struct r4k_tlb_t r4k_tlb_t;
>  struct r4k_tlb_t {
>     target_ulong VPN;
> @@ -172,7 +170,6 @@ struct TCState {
>     int32_t CP0_Debug_tcstatus;
>  };
>
> -typedef struct CPUMIPSState CPUMIPSState;
>  struct CPUMIPSState {
>     TCState active_tc;
>     CPUMIPSFPUContext active_fpu;
> diff --git a/target-mips/exec.h b/target-mips/exec.h
> index 01e9c4d..070e425 100644
> --- a/target-mips/exec.h
> +++ b/target-mips/exec.h
> @@ -6,13 +6,11 @@
>  #include "config.h"
>  #include "mips-defs.h"
>  #include "dyngen-exec.h"
> -#include "cpu-defs.h"
> -
> -register struct CPUMIPSState *env asm(AREG0);
> -
>  #include "cpu.h"
>  #include "exec-all.h"
>
> +register struct CPUMIPSState *env asm(AREG0);
> +
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif /* !defined(CONFIG_USER_ONLY) */
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 2ad4486..a47f12b 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -69,7 +69,7 @@
>
>  #endif /* defined (TARGET_PPC64) */
>
> -#define CPUState struct CPUPPCState
> +#define CPUPPCState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -300,7 +300,6 @@ typedef struct opc_handler_t opc_handler_t;
>
>  /*****************************************************************************/
>  /* Types used to describe some PowerPC registers */
> -typedef struct CPUPPCState CPUPPCState;
>  typedef struct ppc_tb_t ppc_tb_t;
>  typedef struct ppc_spr_t ppc_spr_t;
>  typedef struct ppc_dcr_t ppc_dcr_t;
> diff --git a/target-ppc/exec.h b/target-ppc/exec.h
> index 09f592c..651f91a 100644
> --- a/target-ppc/exec.h
> +++ b/target-ppc/exec.h
> @@ -20,9 +20,7 @@
>  #define __PPC_H__
>
>  #include "config.h"
> -
>  #include "dyngen-exec.h"
> -
>  #include "cpu.h"
>  #include "exec-all.h"
>
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index dd407b2..d30e9da 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -23,7 +23,7 @@
>
>  #define ELF_MACHINE    EM_S390
>
> -#define CPUState struct CPUS390XState
> +#define CPUS390XState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -45,7 +45,7 @@ typedef union FPReg {
>     uint64_t i;
>  } FPReg;
>
> -typedef struct CPUS390XState {
> +struct CPUS390XState {
>     uint64_t regs[16]; /* GP registers */
>
>     uint32_t aregs[16];        /* access registers */
> @@ -64,7 +64,7 @@ typedef struct CPUS390XState {
>     uint64_t __excp_addr;
>
>     CPU_COMMON
> -} CPUS390XState;
> +};
>
>  #if defined(CONFIG_USER_ONLY)
>  static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
> diff --git a/target-s390x/exec.h b/target-s390x/exec.h
> index 837f853..a848f73 100644
> --- a/target-s390x/exec.h
> +++ b/target-s390x/exec.h
> @@ -17,14 +17,13 @@
>  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>  */
>
> -#include "dyngen-exec.h"
> -
> -register struct CPUS390XState *env asm(AREG0);
> -
>  #include "config.h"
> +#include "dyngen-exec.h"
>  #include "cpu.h"
>  #include "exec-all.h"
>
> +register struct CPUS390XState *env asm(AREG0);
> +
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif /* !defined(CONFIG_USER_ONLY) */
> diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
> index f8b1680..5535ed1 100644
> --- a/target-sh4/cpu.h
> +++ b/target-sh4/cpu.h
> @@ -36,7 +36,7 @@
>  #define SH_CPU_SH7750_ALL (SH_CPU_SH7750 | SH_CPU_SH7750S | SH_CPU_SH7750R)
>  #define SH_CPU_SH7751_ALL (SH_CPU_SH7751 | SH_CPU_SH7751R)
>
> -#define CPUState struct CPUSH4State
> +#define CPUSH4State CPUState
>
>  #include "cpu-defs.h"
>
> @@ -107,7 +107,7 @@ typedef struct memory_content {
>     struct memory_content *next;
>  } memory_content;
>
> -typedef struct CPUSH4State {
> +struct CPUSH4State {
>     int id;                    /* CPU model */
>
>     uint32_t flags;            /* general execution flags */
> @@ -157,7 +157,7 @@ typedef struct CPUSH4State {
>     int intr_at_halt;          /* SR_BL ignored during sleep */
>     memory_content *movcal_backup;
>     memory_content **movcal_backup_tail;
> -} CPUSH4State;
> +};
>
>  CPUSH4State *cpu_sh4_init(const char *cpu_model);
>  int cpu_sh4_exec(CPUSH4State * s);
> diff --git a/target-sh4/exec.h b/target-sh4/exec.h
> index edd667d..b2eb306 100644
> --- a/target-sh4/exec.h
> +++ b/target-sh4/exec.h
> @@ -21,12 +21,11 @@
>
>  #include "config.h"
>  #include "dyngen-exec.h"
> -
> -register struct CPUSH4State *env asm(AREG0);
> -
>  #include "cpu.h"
>  #include "exec-all.h"
>
> +register struct CPUSH4State *env asm(AREG0);
> +
>  static inline int cpu_has_work(CPUState *env)
>  {
>     return (env->interrupt_request & CPU_INTERRUPT_HARD);
> diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
> index 8f0484b..a852ee4 100644
> --- a/target-sparc/cpu.h
> +++ b/target-sparc/cpu.h
> @@ -21,7 +21,7 @@
>  # endif
>  #endif
>
> -#define CPUState struct CPUSPARCState
> +#define CPUSPARCState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -318,7 +318,7 @@ struct QEMUFile;
>  void cpu_put_timer(struct QEMUFile *f, CPUTimer *s);
>  void cpu_get_timer(struct QEMUFile *f, CPUTimer *s);
>
> -typedef struct CPUSPARCState {
> +struct CPUSPARCState {
>     target_ulong gregs[8]; /* general registers */
>     target_ulong *regwptr; /* pointer to current register window */
>     target_ulong pc;       /* program counter */
> @@ -436,7 +436,7 @@ typedef struct CPUSPARCState {
>  #define SOFTINT_REG_MASK (SOFTINT_STIMER|SOFTINT_INTRMASK|SOFTINT_TIMER)
>  #endif
>     sparc_def_t *def;
> -} CPUSPARCState;
> +};
>
>  #ifndef NO_CPU_IO_DEFS
>  /* helper.c */
> diff --git a/target-sparc/exec.h b/target-sparc/exec.h
> index c84e055..260b91e 100644
> --- a/target-sparc/exec.h
> +++ b/target-sparc/exec.h
> @@ -1,7 +1,10 @@
>  #ifndef EXEC_SPARC_H
>  #define EXEC_SPARC_H 1
> +
>  #include "config.h"
>  #include "dyngen-exec.h"
> +#include "cpu.h"
> +#include "exec-all.h"
>
>  register struct CPUSPARCState *env asm(AREG0);
>
> --
> 1.7.0.1
>
>
>
>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups
  2010-06-25 12:52 [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Paolo Bonzini
                   ` (7 preceding siblings ...)
  2010-06-25 22:47 ` [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Richard Henderson
@ 2010-06-27 19:32 ` Blue Swirl
  2010-06-28  8:20   ` Paolo Bonzini
  8 siblings, 1 reply; 23+ messages in thread
From: Blue Swirl @ 2010-06-27 19:32 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

On Fri, Jun 25, 2010 at 12:52 PM, Paolo Bonzini <pbonzini@redhat.com> 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(-)
>
>
>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] provide opaque CPUState to files that are compiled once
  2010-06-27 19:17   ` Blue Swirl
@ 2010-06-28  8:04     ` Paolo Bonzini
  2010-06-28 14:21       ` Blue Swirl
  0 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-28  8:04 UTC (permalink / raw)
  To: Blue Swirl; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

On 06/27/2010 09:17 PM, Blue Swirl wrote:
> I'm not comfortable with this part. Accidental use of the global
> register variable can cause subtle bugs. I'd rather rename 'env' to
> something more obvious and less likely to collide, like
> 'global_reg_env' and always poison that. Then we could replace 'env1'
> etc with just 'env'.

This is not very different from before thanks to the reordering of 
includes done in this patch.

All target-*/exec.h files now start with

#include "config.h"
#include "dyngen-exec.h"
#include "cpu.h"
#include "exec-all.h"

// sometimes a few #defines

register struct CPUAlphaState *env asm(AREG0);

And so they cannot use the global env unless NEED_CPU_H is defined.  If 
anything, it's clearer than before because the structure of the initial 
#includes is the same for all targets.

It's true that a "NEED_GLOBAL_ENV" would provide even better safety, but 
that's something for a separate patch series.  It's particularly easy to 
do after replacing CPU<Target>State with CPUState, so that it can be 
moved into exec-all.h, but this series is already big enough IMO.  Let's 
do cleanups one thing at a time please.

Paolo

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups
  2010-06-27 19:32 ` [Qemu-devel] " Blue Swirl
@ 2010-06-28  8:20   ` Paolo Bonzini
  2010-06-28 15:11     ` Blue Swirl
  0 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-28  8:20 UTC (permalink / raw)
  To: Blue Swirl; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

On 06/27/2010 09:32 PM, Blue Swirl wrote:
>  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 don't see it at all as a temporary measure.  In theory, only devices 
compiled per-target would need access to CPUState, that's true. 
However, there are conflicting goals which make an opaque CPUState 
preferrable.

First, converting devices such as APIC to qdev requires knowledge of 
CPUState in qdev, unless you want to keep DEFINE_PROP_PTR (whose removal 
is much more interesting) or sweep it under the void* blanket.  PICs are 
likely to have CPUState members, e.g. hw/pxa2xx_pic.c (and BTW indirect 
access via functions to these fields is making emulation a little bit 
slower).

Also, for things compiled in libhw that are not part of device code, 
requiring knowledge of CPUState is absolutely not problematic and the 
only alternative loses type-safety and so it is inferior.

Paolo

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] provide opaque CPUState to files that are compiled once
  2010-06-28  8:04     ` Paolo Bonzini
@ 2010-06-28 14:21       ` Blue Swirl
  0 siblings, 0 replies; 23+ messages in thread
From: Blue Swirl @ 2010-06-28 14:21 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

On Mon, Jun 28, 2010 at 8:04 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 06/27/2010 09:17 PM, Blue Swirl wrote:
>>
>> I'm not comfortable with this part. Accidental use of the global
>> register variable can cause subtle bugs. I'd rather rename 'env' to
>> something more obvious and less likely to collide, like
>> 'global_reg_env' and always poison that. Then we could replace 'env1'
>> etc with just 'env'.
>
> This is not very different from before thanks to the reordering of includes
> done in this patch.
>
> All target-*/exec.h files now start with
>
> #include "config.h"
> #include "dyngen-exec.h"
> #include "cpu.h"
> #include "exec-all.h"
>
> // sometimes a few #defines
>
> register struct CPUAlphaState *env asm(AREG0);
>
> And so they cannot use the global env unless NEED_CPU_H is defined.  If
> anything, it's clearer than before because the structure of the initial
> #includes is the same for all targets.
>
> It's true that a "NEED_GLOBAL_ENV" would provide even better safety, but
> that's something for a separate patch series.  It's particularly easy to do
> after replacing CPU<Target>State with CPUState, so that it can be moved into
> exec-all.h, but this series is already big enough IMO.  Let's do cleanups
> one thing at a time please.

Fine, but then let's not unpoison env with this patch set, please.

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups
  2010-06-28  8:20   ` Paolo Bonzini
@ 2010-06-28 15:11     ` Blue Swirl
  2010-06-28 16:21       ` Paolo Bonzini
  0 siblings, 1 reply; 23+ messages in thread
From: Blue Swirl @ 2010-06-28 15:11 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

On Mon, Jun 28, 2010 at 8:20 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 06/27/2010 09:32 PM, Blue Swirl wrote:
>>
>>  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 don't see it at all as a temporary measure.  In theory, only devices
> compiled per-target would need access to CPUState, that's true.

There is no need for any device to have access to CPUState fields. All
such accesses are either poor design or a shortcut taken for
performance. The only device where the design calls for CPUState
access is vmmouse and even there we could avoid the direct access. In
all other cases, only board physical address size and CPU endianness
matters but these should be fixed at some point by a better bus
design.

> However,
> there are conflicting goals which make an opaque CPUState preferrable.
>
> First, converting devices such as APIC to qdev requires knowledge of
> CPUState in qdev, unless you want to keep DEFINE_PROP_PTR (whose removal is
> much more interesting) or sweep it under the void* blanket.  PICs are likely
> to have CPUState members, e.g. hw/pxa2xx_pic.c (and BTW indirect access via
> functions to these fields is making emulation a little bit slower).

One way to clean up PICs would be to use qemu_irq to signal CPU
interrupts, but there are probably others.

>
> Also, for things compiled in libhw that are not part of device code,
> requiring knowledge of CPUState is absolutely not problematic and the only
> alternative loses type-safety and so it is inferior.

Completely untrue. Devices (whether part of libhw etc. or not) have no
need (from architectural point of view) to access CPUState contents.
It's clearly problematic. It's also possible to get type safety
without CPUState references in the devices.

Anyway, as I mentioned earlier, I think it's OK to apply the series
(after the problems are fixed), because of the short term gains in
cleanups and type safety. But the goal must be to make all devices
independent of the CPU model. If we never reach that goal (for
example, because of the performance issues), I guess you would not be
sad.

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups
  2010-06-28 15:11     ` Blue Swirl
@ 2010-06-28 16:21       ` Paolo Bonzini
  0 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-28 16:21 UTC (permalink / raw)
  To: Blue Swirl; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

>> I don't see it at all as a temporary measure.  In theory, only devices
>> compiled per-target would need access to CPUState, that's true.
>
> There is no need for any device to have access to CPUState fields.

That's why this patch defines CPUState as opaque (i.e. incomplete). 
Granted, an opaque CPUState plus a bunch of accessors is not any better 
than a fully-visible CPUState, if the accessors are very much tied to 
that particular CPUState type.  (But then, we also agree it is also a 
tiny little bit better than void*).

>> First, converting devices such as APIC to qdev requires knowledge of
>> CPUState in qdev, unless you want to keep DEFINE_PROP_PTR (whose removal is
>> much more interesting) or sweep it under the void* blanket.  PICs are likely
>> to have CPUState members, e.g. hw/pxa2xx_pic.c (and BTW indirect access via
>> functions to these fields is making emulation a little bit slower).
>
> One way to clean up PICs would be to use qemu_irq to signal CPU
> interrupts, but there are probably others.

In the end the devices will be wired to a particular CPU and these 
"wires" will likely be more complex than a 1-bit IRQ, so qemu_irq only 
makes limited sense as it is now.

What you have to pass may be simply a 32-bit value as in hw/etraxfs_pic, 
or a more complicated payload as in apic_bus_deliver, but anyway at some 
point you'll be coupling either:

- the device to the CPU, as it happens now.  As above, the CPU may be 
accessed via void* and functions, or via fields directly, but in any 
case the coupling is there.

- the CPU to the device.  If you used qemu_irq that would be the most 
likely outcome: the CPU knows about some DeviceState, and it downcasts 
it to an expected device type in the IRQ handler.  Again, you only have 
an illusion of decoupling and, given the lengthy discussion about IRQ 
payloads on qemu-devel only a few weeks ago, I'm not optimist it's going 
to go away.

>> Also, for things compiled in libhw that are not part of device code,
>> requiring knowledge of CPUState is absolutely not problematic and the only
>> alternative loses type-safety and so it is inferior.
>
> Completely untrue. Devices (whether part of libhw etc. or not) have no
> need (from architectural point of view) to access CPUState contents.
> It's clearly problematic. It's also possible to get type safety
> without CPUState references in the devices.

I explicitly said "things compiled in libhw that are not part of device 
code", most of which you added.

Anyway, thanks for the review and the (partial) ack.  I'll rework the 
patch series and resend.

Paolo

^ permalink raw reply	[flat|nested] 23+ messages in thread
* [Qemu-devel] Re: [PATCH 5/7] add qdev property type "cpu"
  2010-06-27 12:48     ` [Qemu-devel] " Paolo Bonzini
@ 2010-06-29 11:42       ` Markus Armbruster
  2010-06-29 13:52         ` Paolo Bonzini
  0 siblings, 1 reply; 23+ messages in thread
From: Markus Armbruster @ 2010-06-29 11:42 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

Paolo Bonzini <pbonzini@redhat.com> writes:

>> Hmm.  Parse method doesn't accept output of the print method.  Not so
>> nice.  Is the "CPU #" decoration essential?
>
> I noticed the same in parse/print string:
>
> static int parse_string(DeviceState *dev, Property *prop, const char *str)
> {
>     char **ptr = qdev_get_prop_ptr(dev, prop);
>
>     if (*ptr)
>         qemu_free(*ptr);
>     *ptr = qemu_strdup(str);
>     return 0;
> }
>
> static int print_string(DeviceState *dev, Property *prop, char *dest,
> size_t len)
> {
>     char **ptr = qdev_get_prop_ptr(dev, prop);
>     if (!*ptr)
>         return snprintf(dest, len, "<null>");
>     return snprintf(dest, len, "\"%s\"", *ptr);
> }
>
> It looks like printing representation is chosen "for the user", not
> for parsing.

Yes, but does that mean we *should* add such decorations?

For me, the print method should be reasonably close to what the parse
method accepts.  A bit like Lisp's princ, whose ouput is close, but not
identical to what the reader accepts (for output acceptable to the
reader, use prin1).

As to "for the user":

    dev-prop: cpu_env = CPU #0
    dev-prop: cpu_env = 0

Both are equally intelligible, in my opinion: obvious if you know what
the property is about, gibberish if you don't.  The latter is slightly
easier to parse with simple tools like AWK.

By the way, print_string() should escape double-quote and funny
characters.

^ permalink raw reply	[flat|nested] 23+ messages in thread
* [Qemu-devel] Re: [PATCH 5/7] add qdev property type "cpu"
  2010-06-29 11:42       ` Markus Armbruster
@ 2010-06-29 13:52         ` Paolo Bonzini
  0 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2010-06-29 13:52 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Amit Shah, Isaku Yamahata, qemu-devel

On 06/29/2010 01:42 PM, Markus Armbruster wrote:
> As to "for the user":
>
>      dev-prop: cpu_env = CPU #0
>      dev-prop: cpu_env = 0
>
> Both are equally intelligible, in my opinion: obvious if you know what
> the property is about, gibberish if you don't.  The latter is slightly
> easier to parse with simple tools like AWK.

I'll remove the decoration (though the overall problem remains).

Paolo

^ permalink raw reply	[flat|nested] 23+ messages in thread
end of thread, other threads:[~2010-06-29 13:52 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-25 12:52 [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Paolo Bonzini
2010-06-25 12:52 ` [Qemu-devel] [PATCH 1/7] rtc: Remove TARGET_I386 from qemu-config.c, enables driftfix Paolo Bonzini
2010-06-25 12:52 ` [Qemu-devel] [PATCH 2/7] include qemu-common.h when needed by the next patches Paolo Bonzini
2010-06-25 12:52 ` [Qemu-devel] [PATCH 3/7] include stdio.h freely, remove dyngen-exec.h hacks Paolo Bonzini
2010-06-25 12:52 ` [Qemu-devel] [PATCH 4/7] provide opaque CPUState to files that are compiled once Paolo Bonzini
2010-06-27 19:17   ` Blue Swirl
2010-06-28  8:04     ` Paolo Bonzini
2010-06-28 14:21       ` Blue Swirl
2010-06-25 12:52 ` [Qemu-devel] [PATCH 5/7] add qdev property type "cpu" Paolo Bonzini
2010-06-26  7:46   ` Markus Armbruster
2010-06-27 12:48     ` [Qemu-devel] " Paolo Bonzini
2010-06-29 11:42       ` Markus Armbruster
2010-06-29 13:52         ` Paolo Bonzini
2010-06-27 18:45   ` [Qemu-devel] " Blue Swirl
2010-06-25 12:52 ` [Qemu-devel] [PATCH 6/7] replace void* uses with opaque CPUState* Paolo Bonzini
2010-06-26  7:49   ` Markus Armbruster
2010-06-25 12:52 ` [Qemu-devel] [PATCH 7/7] poison TARGET_xxx for compile once object Paolo Bonzini
2010-06-25 22:47 ` [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Richard Henderson
2010-06-27 12:39   ` [Qemu-devel] " Paolo Bonzini
2010-06-27 19:32 ` [Qemu-devel] " Blue Swirl
2010-06-28  8:20   ` Paolo Bonzini
2010-06-28 15:11     ` Blue Swirl
2010-06-28 16:21       ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.