| Message ID | 1393901250-3922-3-git-send-email-xbing6@gmail.com |
|---|---|
| State | New |
| Headers | show |
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 >
Hi Paolo, thanks for reviewing. On 03/04/2014 06:19 PM, Paolo Bonzini wrote: > 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. I spent some time trying to reduce it. I may be wrong, but this is the smallest patch I can get to get all targets (linuxuser vs softmmu, kvm enable/disable) build. > >> 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. I guess this is my personal preference. My original idea is to emphasize that this *whole* file is "target-specific", as "target-independent" vs "target-specific" is important for qemu framework (everything except target-xxx, tcg backend and hw/arch*/). Ideally all qemu framework is designed to be target-independent, right? In my docs/api-hierarchy.txt, I am trying my best to illustrate what are "target-independent", and what are "target-specific". cpu-defs.h is NOT included in this file. The idea is the same for the rest "#ifndef NEED_CPU_H" Why use "#ifndef NEED_CPU_H, then #error ...", is because "target-xxx/cpu.h" is better included for the whole file. If "target-xxx/cpu.h" is only needed for part of the file, use "#ifdef NEED_CPU_H, then #include "cpu.h"". Thanks.
>> 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. > My idea is to keep disas/disas.h correct just by itself, regardless how it's used by *.c files. I have almost zero knowledge about static code analyzer, I am not sure whether disas.h can pass it without including "cpu.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 > I personally prefer keeping gdbstub.h correct by itself. I am not sure if this is only my personal preference or not. target-*/gdbstub.c implementers only need to know gdbstub hooks (thus gdbstub API), they don't care "cpu.h", although knowledge of "cpu.h" helps. Agree?
Il 04/03/2014 13:02, Xuebing wang ha scritto: >> >> 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. >> > > My idea is to keep disas/disas.h correct just by itself, regardless how > it's used by *.c files. In general, it's a good idea to make headers self-contained by recursively including whatever's necessary. However, there are a few very central headers that are just too complicated to make self-consistent without introducing circular references. In particular, for QEMU these are qemu-common.h and cpu.h (and also others, such as exec-all.h but perhaps the solution there is to split them). It's much simpler to ask people to include these two files in the C files. Paolo
>> 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. > In my humble opinion, qom/cpu is part of virtual machine along with other hw peripherals, thus I'd prefer vmstate to depend on qom/cpu. From object oriented design perspective, qom/cpu is one-level lower than the modeling of virtual machine, do you agree?
Il 04/03/2014 13:19, Xuebing wang ha scritto: >> >> Like Andreas I don't like this particularly. You can add >> migration/vmstate.h to qom/cpu.h instead. >> > > In my humble opinion, qom/cpu is part of virtual machine along with > other hw peripherals, thus I'd prefer vmstate to depend on qom/cpu. > > From object oriented design perspective, qom/cpu is one-level lower than > the modeling of virtual machine, do you agree? vmstate is just the name of the infrastructure that takes a description of a struct's fields and serializes it onto a QEMUFile. If you look at include/migration, there's nothing that is specific to virtual machines except perhaps the declaration of two functions vmstate_register_ram and vmstate_unregister_ram. Paolo
>> #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. > Thanks for suggestion. This patch is strictly confined to "NEED_CPU_H" and making qemu build. :-)
Il 04/03/2014 13:26, Xuebing wang ha scritto: >>> >>> #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. >> > > Thanks for suggestion. This patch is strictly confined to "NEED_CPU_H" > and making qemu build. :-) Making QEMU build is not a good idea if it complicates things. Adding dependencies to a file complicates things. What you're doing here is taking dependencies out of one central file (qemu-common.h) and adding it to a dozen peripheral files. This complicates things. Removing "#ifdef NEED_CPU_H" can be a good idea, but it has to make things simpler, otherwise it is not a good idea anymore. Paolo
On 4 March 2014 12:09, Xuebing wang <xbing6@gmail.com> wrote: > target-*/gdbstub.c implementers only need to know gdbstub hooks (thus > gdbstub API), they don't care "cpu.h", although knowledge of "cpu.h" helps. The gdbstub.c code is just a tiny part of the target-* frontend; there is no way that anybody working on the gdbstub isn't going to know already about the cpu.h interfaces. In particular, the code in those files is generally implementing the gdbstub interfaces by looking at the internal details of the target CPU. The only reason they don't just include cpu.h is simply that at the moment qemu-common.h does it for them. thanks -- PMM
Hi Peter, Thanks. You are correct, I don't know what I was thinking. :-) On 03/04/2014 08:34 PM, Peter Maydell wrote: > On 4 March 2014 12:09, Xuebing wang <xbing6@gmail.com> wrote: >> target-*/gdbstub.c implementers only need to know gdbstub hooks (thus >> gdbstub API), they don't care "cpu.h", although knowledge of "cpu.h" helps. > The gdbstub.c code is just a tiny part of the target-* frontend; there > is no way that anybody working on the gdbstub isn't going to know > already about the cpu.h interfaces. In particular, the code in those > files is generally implementing the gdbstub interfaces by looking > at the internal details of the target CPU. The only reason they don't > just include cpu.h is simply that at the moment qemu-common.h > does it for them. > > thanks > -- PMM >
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 +#include "cpu.h" /* target-xxx/cpu.h, required for TARGET_FMT_lx etc */ + 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 */ 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 + #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); 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 */ + #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 +#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 +#include "cpu.h" /* target-xxx/cpu.h, required for X86CPU, target_ulong, + TPRAccess */ + #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 +#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 */ /* 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) */ - /* 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 - /* 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, \ -} - #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 + #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); 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 */ + /* Default target word size to pointer size. */ #ifndef TCG_TARGET_REG_BITS # if UINTPTR_MAX == UINT32_MAX
Note: there is a FIXME to be addressed in this patch. This patch handles NEED_CPU_H (thus "target-xxx/cpu.h") only. The changes are: - Remove inclusion of "cpu.h" from qemu-common.h - CPU_SAVE_VERSION is defined in target-xxx/cpu.h as well, thus move it out of qemu-common.h, and move it into include/exec/cpu-all.h - To make --enable-kvm build, add '#include "config-target.h"' into sysemu/kvm.h - To make target-i386 build, change apic_internal.h and apic.h in folder include/hw/i386/ - To make target-arm build, change include/hw/arm/omap.h - To make target-alpha build (which uses VMSTATE_CPU), move VMSTATE_CPU() from include/qom/cpu.h => include/migration/vmstate.h - The rest changes are generic to all targets, to make all targets build Why use "#ifndef NEED_CPU_H, then #error ...", is because "target-xxx/cpu.h" is better included for the whole file. If "target-xxx/cpu.h" is only needed for part of the file, use "#ifdef NEED_CPU_H, then #include "cpu.h"". Notes on NEED_CPU_H ------------------- NEED_CPU_H means that code needs architecture-specific information. Here are a few examples: - CPUArchState - TARGET_LONG_BITS - TARGET_PAGE_BITS - TARGET_PHYS_ADDR_SPACE_BITS, TARGET_VIRT_ADDR_SPACE_BITS And constructs that are derived from above, examples are: - target_long, target_ulong Examples of non-architecture-dependent are: - hwaddr - vaddr - ram_addr_t Signed-off-by: Xuebing Wang <xbing6@gmail.com> --- hw/dma/soc_dma.c | 5 +++++ include/disas/disas.h | 6 +++--- include/exec/cpu-all.h | 6 ++++++ include/exec/gdbstub.h | 4 +++- include/exec/ram_addr.h | 5 +++++ include/hw/arm/omap.h | 6 ++++++ include/hw/i386/apic.h | 6 ++++++ include/hw/i386/apic_internal.h | 6 ++++++ include/hw/xen/xen.h | 2 +- include/migration/vmstate.h | 11 +++++++++++ include/qemu-common.h | 11 ----------- include/qom/cpu.h | 14 -------------- include/sysemu/kvm.h | 10 ++++++++++ tcg/tcg.h | 6 ++++++ 14 files changed, 68 insertions(+), 30 deletions(-)