[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Discussion 02/10] NEED_CPU_H: remove '#include "cpu.h"
|
From: |
Paolo Bonzini |
|
Subject: |
Re: [Qemu-devel] [Discussion 02/10] NEED_CPU_H: remove '#include "cpu.h"' from include/qemu-common.h |
|
Date: |
Tue, 04 Mar 2014 11:19:11 +0100 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 |
Hi,
in general I agree with this patch. I have a few comments, and I
suggest that you split it in multiple patches so that it's easier to get
it in when each part is ready.
diff --git a/hw/dma/soc_dma.c b/hw/dma/soc_dma.c
index c06aabb..950f3ec 100644
--- a/hw/dma/soc_dma.c
+++ b/hw/dma/soc_dma.c
@@ -21,6 +21,11 @@
#include "qemu/timer.h"
#include "hw/arm/soc_dma.h"
+#ifndef NEED_CPU_H
+#error target-xxx/cpu.h must be included because target-specific are required
+#endif
Not needed; include/exec/cpu-defs.h already has a similar #error.
+#include "cpu.h" /* target-xxx/cpu.h, required for TARGET_FMT_lx etc */
Space line below the #include, not above.
static void transfer_mem2mem(struct soc_dma_ch_s *ch)
{
memcpy(ch->paddr[0], ch->paddr[1], ch->bytes);
diff --git a/include/disas/disas.h b/include/disas/disas.h
index c13ca9a..e5cdfd7 100644
--- a/include/disas/disas.h
+++ b/include/disas/disas.h
@@ -1,9 +1,9 @@
#ifndef _QEMU_DISAS_H
#define _QEMU_DISAS_H
-#include "qemu-common.h"
-
#ifdef NEED_CPU_H
+#include "cpu.h" /* target-xxx/cpu.h, required for target_ulong,
+ CPUArchState */
/* Disassemble this for me please... (debugging). */
void disas(FILE *out, void *code, unsigned long size);
void target_disas(FILE *out, CPUArchState *env, target_ulong code,
@@ -14,7 +14,7 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
/* Look up symbol for debugging purpose. Returns "" if unknown. */
const char *lookup_symbol(target_ulong orig_addr);
-#endif
+#endif /* NEED_CPU_H */
Perhaps the file that includes disas/disas.h can instead include cpu.h
too? Most of them already do:
$ git grep -L include.*cpu.h $(git grep -l disas/disas.h)
bsd-user/elfload.c
hw/core/loader.c
linux-user/elfload.c
vl.c
Of these, vl.c and linux-user/elfload.c should not include disas/disas.h
at all, and hw/core/loader.c is !NEED_CPU_H. So there are just two
files where you can add a #include "cpu.h" manually.
struct syminfo;
struct elf32_sym;
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 4cb4b4a..7045732 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -490,4 +490,10 @@ void qemu_mutex_unlock_ramlist(void);
int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
uint8_t *buf, int len, int is_write);
+/* CPU save/load. */
+#ifdef CPU_SAVE_VERSION
+void cpu_save(QEMUFile *f, void *opaque);
+int cpu_load(QEMUFile *f, void *opaque, int version_id);
+#endif
Good.
#endif /* CPU_ALL_H */
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index a608a26..14addcb 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -11,6 +11,8 @@
#define GDB_WATCHPOINT_ACCESS 4
#ifdef NEED_CPU_H
+#include "cpu.h" /* target-xxx/cpu.h, required for target_ulong,
+ CPUArchState */
typedef void (*gdb_syscall_complete_cb)(CPUState *cpu,
target_ulong ret, target_ulong err);
@@ -76,7 +78,7 @@ static inline int gdb_get_reg64(uint8_t *mem_buf, uint64_t
val)
#define ldtul_p(addr) ldl_p(addr)
#endif
-#endif
+#endif /* NEED_CPU_H */
#ifdef CONFIG_USER_ONLY
int gdbserver_start(int);
Same here: I'd rather add a cpu.h inclusion to the following files:
cpus.c
target-alpha/gdbstub.c
target-arm/gdbstub.c
target-arm/gdbstub64.c
target-cris/gdbstub.c
target-i386/gdbstub.c
target-lm32/gdbstub.c
target-m68k/gdbstub.c
target-microblaze/gdbstub.c
target-mips/gdbstub.c
target-openrisc/gdbstub.c
target-ppc/gdbstub.c
target-ppc/translate_init.c
target-s390x/gdbstub.c
target-sh4/gdbstub.c
target-sparc/gdbstub.c
target-xtensa/gdbstub.c
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 2edfa96..09e2aa6 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -19,6 +19,11 @@
#ifndef RAM_ADDR_H
#define RAM_ADDR_H
+#ifndef NEED_CPU_H
+#error target-xxx/cpu.h must be included because target-specific are required
+#endif
+#include "cpu.h" /* target-xxx/cpu.h, required for TARGET_PAGE_BITS etc */
As above, this #ifndef is not needed.
#ifndef CONFIG_USER_ONLY
#include "hw/xen/xen.h"
diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index b9655ee..580510f 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -17,10 +17,16 @@
* with this program; if not, see <http://www.gnu.org/licenses/>.
*/
#ifndef hw_omap_h
+
#include "exec/memory.h"
# define hw_omap_h "omap.h"
#include "hw/irq.h"
+#ifndef NEED_CPU_H
+#error target-xxx/cpu.h must be included because target-specific are required
+#endif
#ifndef not needed.
+#include "cpu.h" /* target-xxx/cpu.h, required for ARMCPU etc */
+
# define OMAP_EMIFS_BASE 0x00000000
# define OMAP2_Q0_BASE 0x00000000
# define OMAP_CS0_BASE 0x00000000
diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
index 1d48e02..a0e6922 100644
--- a/include/hw/i386/apic.h
+++ b/include/hw/i386/apic.h
@@ -1,6 +1,12 @@
#ifndef APIC_H
#define APIC_H
+#ifndef NEED_CPU_H
+#error target-xxx/cpu.h must be included because target-specific are required
+#endif
#ifndef not needed.
+#include "cpu.h" /* target-xxx/cpu.h, required for X86CPU, target_ulong,
+ TPRAccess */
Should be included after qemu-common.h.
#include "qemu-common.h"
/* apic.c */
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 70542a6..a1569a1 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -20,6 +20,12 @@
#ifndef QEMU_APIC_INTERNAL_H
#define QEMU_APIC_INTERNAL_H
+#ifndef NEED_CPU_H
+#error target-xxx/cpu.h must be included because target-specific are required
+#endif
#ifndef not needed.
+#include "cpu.h" /* target-xxx/cpu.h, required for X86CPU, target_ulong,
+ TPRAccess */
+
#include "exec/memory.h"
#include "hw/cpu/icc_bus.h"
#include "qemu/timer.h"
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index e1f88bf..34773b9 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -9,7 +9,7 @@
#include <inttypes.h>
#include "hw/irq.h"
-#include "qemu-common.h"
+#include "exec/cpu-common.h" /* for ram_addr_t */
Ok.
/* xen-machine.c */
enum xen_mode {
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index ded8e23..040cc75 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -138,8 +138,19 @@ struct VMStateDescription {
#ifdef CONFIG_USER_ONLY
extern const VMStateDescription vmstate_dummy;
+#define vmstate_cpu_common vmstate_dummy
+#else
+extern const struct VMStateDescription vmstate_cpu_common;
#endif
+#define VMSTATE_CPU() { \
+ .name = "parent_obj", \
+ .size = sizeof(CPUState), \
+ .vmsd = &vmstate_cpu_common, \
+ .flags = VMS_STRUCT, \
+ .offset = 0, \
+}
+
extern const VMStateInfo vmstate_info_bool;
extern const VMStateInfo vmstate_info_int8;
diff --git a/include/qemu-common.h b/include/qemu-common.h
index c8a58a8..cd33b2c 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -111,11 +111,6 @@ extern int use_icount;
#include "qemu/osdep.h"
#include "qemu/bswap.h"
-/* FIXME: Remove NEED_CPU_H. */
-#ifdef NEED_CPU_H
-#include "cpu.h"
-#endif /* !defined(NEED_CPU_H) */
Ok.
/* main function, renamed */
#if defined(CONFIG_COCOA)
int qemu_main(int argc, char **argv, char **envp);
@@ -273,12 +268,6 @@ bool tcg_enabled(void);
void cpu_exec_init_all(void);
-/* CPU save/load. */
-#ifdef CPU_SAVE_VERSION
-void cpu_save(QEMUFile *f, void *opaque);
-int cpu_load(QEMUFile *f, void *opaque, int version_id);
-#endif
-
Ok.
/* Unblock cpu */
void qemu_cpu_kick_self(void);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 367eda1..f0157e3 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -510,18 +510,4 @@ void qemu_init_vcpu(CPUState *cpu);
*/
void cpu_single_step(CPUState *cpu, int enabled);
-#ifdef CONFIG_SOFTMMU
-extern const struct VMStateDescription vmstate_cpu_common;
-#else
-#define vmstate_cpu_common vmstate_dummy
-#endif
-
-#define VMSTATE_CPU() { \
- .name = "parent_obj", \
- .size = sizeof(CPUState), \
- .vmsd = &vmstate_cpu_common, \
- .flags = VMS_STRUCT, \
- .offset = 0, \
-}
-
Like Andreas I don't like this particularly. You can add
migration/vmstate.h to qom/cpu.h instead.
#endif
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index a02d67c..112721d 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -19,6 +19,15 @@
#include "qemu/queue.h"
#include "qom/cpu.h"
+/* Ideally, for every appearance of NEED_CPU_H, there must be followed by
+ * the inclusion of "target-xxx/cpu.h".
+ *
+ * FIXME: find another logic other than using NEED_CPU_H.
+ */
+#ifdef NEED_CPU_H
+#include "config-target.h" /* for CONFIG_KVM */
+#endif
I think this is okay.
+
#ifdef CONFIG_KVM
#include <linux/kvm.h>
#include <linux/kvm_para.h>
@@ -169,6 +178,7 @@ int kvm_init_vcpu(CPUState *cpu);
int kvm_cpu_exec(CPUState *cpu);
#ifdef NEED_CPU_H
+#include "cpu.h" /* target-xxx/cpu.h, required for target_ulong */
void kvm_setup_guest_memory(void *start, size_t size);
void kvm_flush_coalesced_mmio_buffer(void);
Perhaps move debugging-related definitions to a new file
sysemu/kvm-debug.h and include it only from gdbstub.c, kvm-all.c,
kvm-stub.c, target-*/kvm.c.
diff --git a/tcg/tcg.h b/tcg/tcg.h
index f7efcb4..4dea41a 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -29,6 +29,12 @@
#include "qemu/bitops.h"
#include "tcg-target.h"
+#ifndef NEED_CPU_H
+#error target-xxx/cpu.h must be included because target-specific are required
+#endif
+#include "cpu.h" /* target-xxx/cpu.h, required for CPUArchState,
+ TARGET_LONG_BITS in tcg-opc.h */
#ifndef not needed.
Paolo
/* Default target word size to pointer size. */
#ifndef TCG_TARGET_REG_BITS
# if UINTPTR_MAX == UINT32_MAX
- [Qemu-devel] [Discussion 00/10] about API hierarchy, Xuebing Wang, 2014/03/03
- Re: [Qemu-devel] [Discussion 02/10] NEED_CPU_H: remove '#include "cpu.h"' from include/qemu-common.h, Xuebing wang, 2014/03/04
- Re: [Qemu-devel] [Discussion 02/10] NEED_CPU_H: remove '#include "cpu.h"' from include/qemu-common.h, Paolo Bonzini, 2014/03/04
- Re: [Qemu-devel] [Discussion 02/10] NEED_CPU_H: remove '#include "cpu.h"' from include/qemu-common.h, Xuebing wang, 2014/03/04
- Re: [Qemu-devel] [Discussion 02/10] NEED_CPU_H: remove '#include "cpu.h"' from include/qemu-common.h, Paolo Bonzini, 2014/03/04