From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56007) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKml5-00048U-EA for qemu-devel@nongnu.org; Tue, 04 Mar 2014 05:39:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKml0-0002qx-KJ for qemu-devel@nongnu.org; Tue, 04 Mar 2014 05:39:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:28428) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKml0-0002qr-7z for qemu-devel@nongnu.org; Tue, 04 Mar 2014 05:39:30 -0500 Message-ID: <5315A89F.40700@redhat.com> Date: Tue, 04 Mar 2014 11:19:11 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1393901250-3922-1-git-send-email-xbing6@gmail.com> <1393901250-3922-3-git-send-email-xbing6@gmail.com> In-Reply-To: <1393901250-3922-3-git-send-email-xbing6@gmail.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Discussion 02/10] NEED_CPU_H: remove '#include "cpu.h"' from include/qemu-common.h List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xuebing Wang , qemu-devel@nongnu.org Cc: afaerber@suse.de, stefanha@redhat.com 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 . > */ > #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 > > #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 > #include > @@ -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 >