Re: [Qemu-devel] [Discussion 03/10] NEED_CPU_H: remove unnecessary use of NEED_CPU_H

[Paolo Bonzini <pbonzini@redhat.com>]

Il 04/03/2014 03:47, Xuebing Wang ha scritto:
> Note: there is a FIXME to be addressed in this patch.
>
> For every appearance of NEED_CPU_H, there must be '#include "cpu.h"' to
> include "target-xxx/cpu.h", because the code below NEED_CPU_H depends on
> architecture-specific information.
>
> Signed-off-by: Xuebing Wang <xbing6@gmail.com>

Here I disagree.  I think that if cpu.h is needed by a header, it should 
be included by whoever needs that header.

> ---
>  include/exec/cpu-common.h |    5 +++++
>  include/hw/hw.h           |   13 +++++++++----
>  include/hw/xen/xen.h      |    2 +-
>  include/qemu/log.h        |    7 ++++---
>  include/sysemu/kvm.h      |    2 +-
>  ui/input.c                |    2 --
>  6 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index a21b65a..a696667 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -98,6 +98,11 @@ void stl_be_phys(AddressSpace *as, hwaddr addr, uint32_t val);
>  void stq_le_phys(AddressSpace *as, hwaddr addr, uint64_t val);
>  void stq_be_phys(AddressSpace *as, hwaddr addr, uint64_t val);
>
> +/*
> + * FIXME: although it's nice to keep ld/st APIs together with above,
> + *        below violates the idea that every NEED_CPU_H should be followed
> + *        by inclusion of "target-xxx/cpu.h"
> + */
>  #ifdef NEED_CPU_H
>  uint32_t lduw_phys(AddressSpace *as, hwaddr addr);
>  uint32_t ldl_phys(AddressSpace *as, hwaddr addr);
> diff --git a/include/hw/hw.h b/include/hw/hw.h
> index 33bdb92..3accd44 100644
> --- a/include/hw/hw.h
> +++ b/include/hw/hw.h
> @@ -4,7 +4,7 @@
>
>  #include "qemu-common.h"
>
> -#if !defined(CONFIG_USER_ONLY) && !defined(NEED_CPU_H)
> +#if !defined(CONFIG_USER_ONLY)
>  #include "exec/cpu-common.h"
>  #endif
>
> @@ -15,6 +15,8 @@
>  #include "qemu/log.h"
>
>  #ifdef NEED_CPU_H
> +#include "cpu.h" /* target-xxx/cpu.h, required for TARGET_LONG_BITS */
> +
>  #if TARGET_LONG_BITS == 64
>  #define qemu_put_betl qemu_put_be64
>  #define qemu_get_betl qemu_get_be64
> @@ -34,7 +36,7 @@
>  #define qemu_put_sbetls qemu_put_sbe32s
>  #define qemu_get_sbetls qemu_get_sbe32s
>  #endif
> -#endif
> +#endif /* NEED_CPU_H */
>
>  typedef void QEMUResetHandler(void *opaque);
>
> @@ -48,6 +50,8 @@ void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
>  int qemu_boot_set(const char *boot_order);
>
>  #ifdef NEED_CPU_H
> +#include "cpu.h" /* target-xxx/cpu.h, required for TARGET_LONG_BITS */
> +
>  #if TARGET_LONG_BITS == 64
>  #define VMSTATE_UINTTL_V(_f, _s, _v)                                  \
>      VMSTATE_UINT64_V(_f, _s, _v)
> @@ -63,6 +67,7 @@ int qemu_boot_set(const char *boot_order);
>  #define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
>      VMSTATE_UINT32_ARRAY_V(_f, _s, _n, _v)
>  #endif
> +
>  #define VMSTATE_UINTTL(_f, _s)                                        \
>      VMSTATE_UINTTL_V(_f, _s, 0)
>  #define VMSTATE_UINTTL_EQUAL(_f, _s)                                  \
> @@ -70,6 +75,6 @@ int qemu_boot_set(const char *boot_order);
>  #define VMSTATE_UINTTL_ARRAY(_f, _s, _n)                              \
>      VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, 0)
>
> -#endif
> +#endif /* NEED_CPU_H */
>
> -#endif
> +#endif /* QEMU_HW_H */
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index 34773b9..638817b 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -40,7 +40,7 @@ int xen_init(void);
>  int xen_hvm_init(MemoryRegion **ram_memory);
>  void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
>
> -#if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
> +#if !defined(CONFIG_USER_ONLY)
>  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
>                     struct MemoryRegion *mr);
>  void xen_modified_memory(ram_addr_t start, ram_addr_t length);
> diff --git a/include/qemu/log.h b/include/qemu/log.h
> index d515424..715accb 100644
> --- a/include/qemu/log.h
> +++ b/include/qemu/log.h
> @@ -6,9 +6,7 @@
>  #include <stdio.h>
>  #include "qemu/compiler.h"
>  #include "qom/cpu.h"
> -#ifdef NEED_CPU_H
>  #include "disas/disas.h"
> -#endif
>
>  /* Private global variables, don't use */
>  extern FILE *qemu_logfile;
> @@ -102,6 +100,9 @@ static inline void log_cpu_state_mask(int mask, CPUState *cpu, int flags)
>  }
>
>  #ifdef NEED_CPU_H
> +#include "cpu.h" /* target-xxx/cpu.h, required for target_ulong,
> +                    CPUArchState */
> +
>  /* disas() and target_disas() to qemu_logfile: */
>  static inline void log_target_disas(CPUArchState *env, target_ulong start,
>                                      target_ulong len, int flags)
> @@ -121,7 +122,7 @@ static inline void log_page_dump(void)
>      page_dump(qemu_logfile);
>  }
>  #endif
> -#endif
> +#endif /* NEED_CPU_H */
>
>
>  /* Maintenance: */
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 112721d..d1e5389 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -58,7 +58,7 @@ extern bool kvm_gsi_routing_allowed;
>  extern bool kvm_gsi_direct_mapping;
>  extern bool kvm_readonly_mem_allowed;
>
> -#if defined CONFIG_KVM || !defined NEED_CPU_H
> +#if defined CONFIG_KVM
>  #define kvm_enabled()           (kvm_allowed)

This is wrong.  There are files that do not need cpu.h and need 
kvm_enabled().

Paolo

>  /**
>   * kvm_irqchip_in_kernel:
> diff --git a/ui/input.c b/ui/input.c
> index 1c70f60..b6d216d 100644
> --- a/ui/input.c
> +++ b/ui/input.c
> @@ -187,7 +187,6 @@ static const int key_defs[] = {
>
>      [Q_KEY_CODE_INSERT] = 0xd2,
>      [Q_KEY_CODE_DELETE] = 0xd3,
> -#ifdef NEED_CPU_H
>  #if defined(TARGET_SPARC) && !defined(TARGET_SPARC64)
>      [Q_KEY_CODE_STOP] = 0xf0,
>      [Q_KEY_CODE_AGAIN] = 0xf1,
> @@ -205,7 +204,6 @@ static const int key_defs[] = {
>      [Q_KEY_CODE_META_R] = 0xfd,
>      [Q_KEY_CODE_COMPOSE] = 0xfe,
>  #endif
> -#endif
>      [Q_KEY_CODE_MAX] = 0,
>  };
>
>