From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52940) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKmT8-0002Sf-EQ for qemu-devel@nongnu.org; Tue, 04 Mar 2014 05:21:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKmT2-0005mp-Cg for qemu-devel@nongnu.org; Tue, 04 Mar 2014 05:21:02 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44019) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKmT2-0005mi-3l for qemu-devel@nongnu.org; Tue, 04 Mar 2014 05:20:56 -0500 Message-ID: <5315A901.2010007@redhat.com> Date: Tue, 04 Mar 2014 11:20:49 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1393901250-3922-1-git-send-email-xbing6@gmail.com> <1393901250-3922-4-git-send-email-xbing6@gmail.com> In-Reply-To: <1393901250-3922-4-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 03/10] NEED_CPU_H: remove unnecessary use of NEED_CPU_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 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 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 > #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, > }; > >