Re: [Qemu-devel] [Discussion 02/10] NEED_CPU_H: remove '#include "cpu.h"' from include/qemu-common.h

[Paolo Bonzini <pbonzini@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 <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
>