provide opaque CPUState to files that are compiled once

This patch allows to unpoison CPUState and env in once-compiled files.
To achieve this, it defines an opaque struct CPUState in cpu-common.h.
This also requires tweaking the relationship between CPUState and
CPUXYZState in target files.

Unpoisoning env is needed because it is widely used as the name for
CPUState arguments.  This is anyway safe, because references to the
global register variable will not creep into target-independent files.
To this end, the patch rationalizes inclusions at the head of
target-*/exec.h.  All exec.h files now include cpu.h before defining the
global register variable env, so that inclusions from machine-independent
context will error out in cpu.h even before env is defined.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        This patch is helpful for qemu-kvm, because some of the changes
        there use CPUState in hw/* files that are now compiled only
        once.

        It can also be used to compile more files once-only, but I'm
        not doing that here.

 cpu-common.h             |    3 +++
 cpu-defs.h               |    1 +
 hw/poison.h              |    3 ---
 target-alpha/cpu.h       |    4 +---
 target-alpha/exec.h      |    6 ++----
 target-arm/cpu.h         |    6 +++---
 target-arm/exec.h        |    5 ++---
 target-cris/cpu.h        |    6 +++---
 target-cris/exec.h       |    6 +++---
 target-i386/cpu.h        |    6 +++---
 target-i386/exec.h       |    7 ++-----
 target-m68k/cpu.h        |    6 +++---
 target-m68k/exec.h       |    6 +++---
 target-microblaze/cpu.h  |    7 +++----
 target-microblaze/exec.h |    6 +++---
 target-mips/cpu.h        |    5 +----
 target-mips/exec.h       |    6 ++----
 target-ppc/cpu.h         |    3 +--
 target-ppc/exec.h        |    2 --
 target-s390x/cpu.h       |    6 +++---
 target-s390x/exec.h      |    7 +++----
 target-sh4/cpu.h         |    6 +++---
 target-sh4/exec.h        |    5 ++---
 target-sparc/cpu.h       |    6 +++---
 target-sparc/exec.h      |    6 +++---
 25 files changed, 56 insertions(+), 74 deletions(-)

On 04/02/2010 09:45 AM, Paolo Bonzini wrote:
> This patch allows to unpoison CPUState and env in once-compiled files.
> To achieve this, it defines an opaque struct CPUState in cpu-common.h.
> This also requires tweaking the relationship between CPUState and
> CPUXYZState in target files.
>
> Unpoisoning env is needed because it is widely used as the name for
> CPUState arguments.  This is anyway safe, because references to the
> global register variable will not creep into target-independent files.
> To this end, the patch rationalizes inclusions at the head of
> target-*/exec.h.  All exec.h files now include cpu.h before defining the
> global register variable env, so that inclusions from machine-independent
> context will error out in cpu.h even before env is defined.
>    


hw/* should never access CPUState.

Can you give examples of when qemu-kvm needs this?

Regards,

Anthony Liguori

> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>          This patch is helpful for qemu-kvm, because some of the changes
>          there use CPUState in hw/* files that are now compiled only
>          once.
>
>          It can also be used to compile more files once-only, but I'm
>          not doing that here.
>
>   cpu-common.h             |    3 +++
>   cpu-defs.h               |    1 +
>   hw/poison.h              |    3 ---
>   target-alpha/cpu.h       |    4 +---
>   target-alpha/exec.h      |    6 ++----
>   target-arm/cpu.h         |    6 +++---
>   target-arm/exec.h        |    5 ++---
>   target-cris/cpu.h        |    6 +++---
>   target-cris/exec.h       |    6 +++---
>   target-i386/cpu.h        |    6 +++---
>   target-i386/exec.h       |    7 ++-----
>   target-m68k/cpu.h        |    6 +++---
>   target-m68k/exec.h       |    6 +++---
>   target-microblaze/cpu.h  |    7 +++----
>   target-microblaze/exec.h |    6 +++---
>   target-mips/cpu.h        |    5 +----
>   target-mips/exec.h       |    6 ++----
>   target-ppc/cpu.h         |    3 +--
>   target-ppc/exec.h        |    2 --
>   target-s390x/cpu.h       |    6 +++---
>   target-s390x/exec.h      |    7 +++----
>   target-sh4/cpu.h         |    6 +++---
>   target-sh4/exec.h        |    5 ++---
>   target-sparc/cpu.h       |    6 +++---
>   target-sparc/exec.h      |    6 +++---
>   25 files changed, 56 insertions(+), 74 deletions(-)
>
> diff --git a/cpu-common.h b/cpu-common.h
> index b730ca0..415feb5 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -18,6 +18,9 @@
>   #include "bswap.h"
>   #include "qemu-queue.h"
>
> +struct CPUState;
> +typedef struct CPUState CPUState;
> +
>   #if !defined(CONFIG_USER_ONLY)
>
>   /* address in the RAM (different from a physical address) */
> diff --git a/cpu-defs.h b/cpu-defs.h
> index 2e94585..e8da6af 100644
> --- a/cpu-defs.h
> +++ b/cpu-defs.h
> @@ -30,6 +30,7 @@
>   #include "osdep.h"
>   #include "qemu-queue.h"
>   #include "targphys.h"
> +#include "cpu-common.h"
>
>   #ifndef TARGET_LONG_BITS
>   #error TARGET_LONG_BITS must be defined before including this header
> diff --git a/hw/poison.h b/hw/poison.h
> index d7db7f4..e7814cb 100644
> --- a/hw/poison.h
> +++ b/hw/poison.h
> @@ -33,9 +33,6 @@
>   #pragma GCC poison TARGET_PAGE_BITS
>   #pragma GCC poison TARGET_PAGE_ALIGN
>
> -#pragma GCC poison CPUState
> -#pragma GCC poison env
> -
>   #pragma GCC poison CPU_INTERRUPT_HARD
>   #pragma GCC poison CPU_INTERRUPT_EXITTB
>   #pragma GCC poison CPU_INTERRUPT_TIMER
> diff --git a/target-alpha/cpu.h b/target-alpha/cpu.h
> index 8afe16d..92283e2 100644
> --- a/target-alpha/cpu.h
> +++ b/target-alpha/cpu.h
> @@ -24,7 +24,7 @@
>
>   #define TARGET_LONG_BITS 64
>
> -#define CPUState struct CPUAlphaState
> +#define CPUAlphaState CPUState
>
>   #include "cpu-defs.h"
>
> @@ -317,8 +317,6 @@ enum {
>       IPR_LAST,
>   };
>
> -typedef struct CPUAlphaState CPUAlphaState;
> -
>   typedef struct pal_handler_t pal_handler_t;
>   struct pal_handler_t {
>       /* Reset */
> diff --git a/target-alpha/exec.h b/target-alpha/exec.h
> index 66526e2..789305f 100644
> --- a/target-alpha/exec.h
> +++ b/target-alpha/exec.h
> @@ -21,8 +21,9 @@
>   #define __ALPHA_EXEC_H__
>
>   #include "config.h"
> -
>   #include "dyngen-exec.h"
> +#include "cpu.h"
> +#include "exec-all.h"
>
>   #define TARGET_LONG_BITS 64
>
> @@ -32,9 +33,6 @@ register struct CPUAlphaState *env asm(AREG0);
>   #define SPARAM(n) ((int32_t)PARAM##n)
>   #define FP_STATUS (env->fp_status)
>
> -#include "cpu.h"
> -#include "exec-all.h"
> -
>   #if !defined(CONFIG_USER_ONLY)
>   #include "softmmu_exec.h"
>   #endif /* !defined(CONFIG_USER_ONLY) */
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 3892db4..d068b6e 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -23,7 +23,7 @@
>
>   #define ELF_MACHINE	EM_ARM
>
> -#define CPUState struct CPUARMState
> +#define CPUARMState CPUState
>
>   #include "cpu-defs.h"
>
> @@ -70,7 +70,7 @@ struct arm_boot_info;
>      s<2n+1>  maps to the most significant half of d<n>
>    */
>
> -typedef struct CPUARMState {
> +struct CPUARMState {
>       /* Regs for current mode.  */
>       uint32_t regs[16];
>       /* Frequently accessed CPSR bits are stored separately for efficiently.
> @@ -206,7 +206,7 @@ typedef struct CPUARMState {
>
>       /* These fields after the common ones so they are preserved on reset.  */
>       struct arm_boot_info *boot_info;
> -} CPUARMState;
> +};
>
>   CPUARMState *cpu_arm_init(const char *cpu_model);
>   void arm_translate_init(void);
> diff --git a/target-arm/exec.h b/target-arm/exec.h
> index 0225c3f..4042eca 100644
> --- a/target-arm/exec.h
> +++ b/target-arm/exec.h
> @@ -18,14 +18,13 @@
>    */
>   #include "config.h"
>   #include "dyngen-exec.h"
> +#include "cpu.h"
> +#include "exec-all.h"
>
>   register struct CPUARMState *env asm(AREG0);
>
>   #define M0   env->iwmmxt.val
>
> -#include "cpu.h"
> -#include "exec-all.h"
> -
>   static inline int cpu_has_work(CPUState *env)
>   {
>       return (env->interrupt_request&
> diff --git a/target-cris/cpu.h b/target-cris/cpu.h
> index 063a240..fa9b5d6 100644
> --- a/target-cris/cpu.h
> +++ b/target-cris/cpu.h
> @@ -22,7 +22,7 @@
>
>   #define TARGET_LONG_BITS 32
>
> -#define CPUState struct CPUCRISState
> +#define CPUCRISState CPUState
>
>   #include "cpu-defs.h"
>
> @@ -96,7 +96,7 @@
>
>   #define NB_MMU_MODES 2
>
> -typedef struct CPUCRISState {
> +struct CPUCRISState {
>   	uint32_t regs[16];
>   	/* P0 - P15 are referred to as special registers in the docs.  */
>   	uint32_t pregs[16];
> @@ -156,7 +156,7 @@ typedef struct CPUCRISState {
>   	} tlbsets[2][4][16];
>
>   	CPU_COMMON
> -} CPUCRISState;
> +};
>
>   CPUCRISState *cpu_cris_init(const char *cpu_model);
>   int cpu_cris_exec(CPUCRISState *s);
> diff --git a/target-cris/exec.h b/target-cris/exec.h
> index 728aa80..af0103d 100644
> --- a/target-cris/exec.h
> +++ b/target-cris/exec.h
> @@ -17,13 +17,13 @@
>    * You should have received a copy of the GNU Lesser General Public
>    * License along with this library; if not, see<http://www.gnu.org/licenses/>.
>    */
> +#include "config.h"
>   #include "dyngen-exec.h"
> -
> -register struct CPUCRISState *env asm(AREG0);
> -
>   #include "cpu.h"
>   #include "exec-all.h"
>
> +register struct CPUCRISState *env asm(AREG0);
> +
>   #if !defined(CONFIG_USER_ONLY)
>   #include "softmmu_exec.h"
>   #endif
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 548ab80..6bfb970 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -41,7 +41,7 @@
>   #define ELF_MACHINE	EM_386
>   #endif
>
> -#define CPUState struct CPUX86State
> +#define CPUX86State CPUState
>
>   #include "cpu-defs.h"
>
> @@ -581,7 +581,7 @@ typedef struct {
>
>   #define NB_MMU_MODES 2
>
> -typedef struct CPUX86State {
> +struct CPUX86State {
>       /* standard registers */
>       target_ulong regs[CPU_NB_REGS];
>       target_ulong eip;
> @@ -718,7 +718,7 @@ typedef struct CPUX86State {
>       uint16_t fpus_vmstate;
>       uint16_t fptag_vmstate;
>       uint16_t fpregs_format_vmstate;
> -} CPUX86State;
> +};
>
>   CPUX86State *cpu_x86_init(const char *cpu_model);
>   int cpu_x86_exec(CPUX86State *s);
> diff --git a/target-i386/exec.h b/target-i386/exec.h
> index 4ff3c57..29b741a 100644
> --- a/target-i386/exec.h
> +++ b/target-i386/exec.h
> @@ -18,6 +18,8 @@
>    */
>   #include "config.h"
>   #include "dyngen-exec.h"
> +#include "cpu.h"
> +#include "exec-all.h"
>
>   /* XXX: factorize this mess */
>   #ifdef TARGET_X86_64
> @@ -26,8 +28,6 @@
>   #define TARGET_LONG_BITS 32
>   #endif
>
> -#include "cpu-defs.h"
> -
>   register struct CPUX86State *env asm(AREG0);
>
>   #include "qemu-common.h"
> @@ -63,9 +63,6 @@ register struct CPUX86State *env asm(AREG0);
>   #define ST(n)  (env->fpregs[(env->fpstt + (n))&  7].d)
>   #define ST1    ST(1)
>
> -#include "cpu.h"
> -#include "exec-all.h"
> -
>   /* op_helper.c */
>   void do_interrupt(int intno, int is_int, int error_code,
>                     target_ulong next_eip, int is_hw);
> diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
> index b2f37ec..ac2fa84 100644
> --- a/target-m68k/cpu.h
> +++ b/target-m68k/cpu.h
> @@ -22,7 +22,7 @@
>
>   #define TARGET_LONG_BITS 32
>
> -#define CPUState struct CPUM68KState
> +#define CPUM68KState CPUState
>
>   #include "cpu-defs.h"
>
> @@ -56,7 +56,7 @@
>
>   #define NB_MMU_MODES 2
>
> -typedef struct CPUM68KState {
> +struct CPUM68KState {
>       uint32_t dregs[8];
>       uint32_t aregs[8];
>       uint32_t pc;
> @@ -112,7 +112,7 @@ typedef struct CPUM68KState {
>       CPU_COMMON
>
>       uint32_t features;
> -} CPUM68KState;
> +};
>
>   void m68k_tcg_init(void);
>   CPUM68KState *cpu_m68k_init(const char *cpu_model);
> diff --git a/target-m68k/exec.h b/target-m68k/exec.h
> index ece9aa0..b611282 100644
> --- a/target-m68k/exec.h
> +++ b/target-m68k/exec.h
> @@ -17,13 +17,13 @@
>    * You should have received a copy of the GNU Lesser General Public
>    * License along with this library; if not, see<http://www.gnu.org/licenses/>.
>    */
> +#include "config.h"
>   #include "dyngen-exec.h"
> -
> -register struct CPUM68KState *env asm(AREG0);
> -
>   #include "cpu.h"
>   #include "exec-all.h"
>
> +register struct CPUM68KState *env asm(AREG0);
> +
>   #if !defined(CONFIG_USER_ONLY)
>   #include "softmmu_exec.h"
>   #endif
> diff --git a/target-microblaze/cpu.h b/target-microblaze/cpu.h
> index ec2ca18..732bbca 100644
> --- a/target-microblaze/cpu.h
> +++ b/target-microblaze/cpu.h
> @@ -21,10 +21,9 @@
>
>   #define TARGET_LONG_BITS 32
>
> -#define CPUState struct CPUMBState
> +#define CPUMBState CPUState
>
>   #include "cpu-defs.h"
> -struct CPUMBState;
>   #if !defined(CONFIG_USER_ONLY)
>   #include "mmu.h"
>   #endif
> @@ -199,7 +198,7 @@ struct CPUMBState;
>   #define CC_EQ  0
>
>   #define NB_MMU_MODES    3
> -typedef struct CPUMBState {
> +struct CPUMBState {
>       uint32_t debug;
>       uint32_t btaken;
>       uint32_t btarget;
> @@ -231,7 +230,7 @@ typedef struct CPUMBState {
>   #endif
>
>       CPU_COMMON
> -} CPUMBState;
> +};
>
>   CPUState *cpu_mb_init(const char *cpu_model);
>   int cpu_mb_exec(CPUState *s);
> diff --git a/target-microblaze/exec.h b/target-microblaze/exec.h
> index 646701c..813d3d6 100644
> --- a/target-microblaze/exec.h
> +++ b/target-microblaze/exec.h
> @@ -16,13 +16,13 @@
>    * You should have received a copy of the GNU Lesser General Public
>    * License along with this library; if not, see<http://www.gnu.org/licenses/>.
>    */
> +#include "config.h"
>   #include "dyngen-exec.h"
> -
> -register struct CPUMBState *env asm(AREG0);
> -
>   #include "cpu.h"
>   #include "exec-all.h"
>
> +register struct CPUMBState *env asm(AREG0);
> +
>   #if !defined(CONFIG_USER_ONLY)
>   #include "softmmu_exec.h"
>   #endif
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index 7285636..3b2b331 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -5,7 +5,7 @@
>
>   #define ELF_MACHINE	EM_MIPS
>
> -#define CPUState struct CPUMIPSState
> +#define CPUMIPSState CPUState
>
>   #include "config.h"
>   #include "mips-defs.h"
> @@ -19,8 +19,6 @@ typedef unsigned char           uint_fast8_t;
>   typedef unsigned int            uint_fast16_t;
>   #endif
>
> -struct CPUMIPSState;
> -
>   typedef struct r4k_tlb_t r4k_tlb_t;
>   struct r4k_tlb_t {
>       target_ulong VPN;
> @@ -172,7 +170,6 @@ struct TCState {
>       int32_t CP0_Debug_tcstatus;
>   };
>
> -typedef struct CPUMIPSState CPUMIPSState;
>   struct CPUMIPSState {
>       TCState active_tc;
>       CPUMIPSFPUContext active_fpu;
> diff --git a/target-mips/exec.h b/target-mips/exec.h
> index 01e9c4d..070e425 100644
> --- a/target-mips/exec.h
> +++ b/target-mips/exec.h
> @@ -6,13 +6,11 @@
>   #include "config.h"
>   #include "mips-defs.h"
>   #include "dyngen-exec.h"
> -#include "cpu-defs.h"
> -
> -register struct CPUMIPSState *env asm(AREG0);
> -
>   #include "cpu.h"
>   #include "exec-all.h"
>
> +register struct CPUMIPSState *env asm(AREG0);
> +
>   #if !defined(CONFIG_USER_ONLY)
>   #include "softmmu_exec.h"
>   #endif /* !defined(CONFIG_USER_ONLY) */
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 2ad4486..a47f12b 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -69,7 +69,7 @@
>
>   #endif /* defined (TARGET_PPC64) */
>
> -#define CPUState struct CPUPPCState
> +#define CPUPPCState CPUState
>
>   #include "cpu-defs.h"
>
> @@ -300,7 +300,6 @@ typedef struct opc_handler_t opc_handler_t;
>
>   /*****************************************************************************/
>   /* Types used to describe some PowerPC registers */
> -typedef struct CPUPPCState CPUPPCState;
>   typedef struct ppc_tb_t ppc_tb_t;
>   typedef struct ppc_spr_t ppc_spr_t;
>   typedef struct ppc_dcr_t ppc_dcr_t;
> diff --git a/target-ppc/exec.h b/target-ppc/exec.h
> index 09f592c..651f91a 100644
> --- a/target-ppc/exec.h
> +++ b/target-ppc/exec.h
> @@ -20,9 +20,7 @@
>   #define __PPC_H__
>
>   #include "config.h"
> -
>   #include "dyngen-exec.h"
> -
>   #include "cpu.h"
>   #include "exec-all.h"
>
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index dd407b2..d30e9da 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -23,7 +23,7 @@
>
>   #define ELF_MACHINE	EM_S390
>
> -#define CPUState struct CPUS390XState
> +#define CPUS390XState CPUState
>
>   #include "cpu-defs.h"
>
> @@ -45,7 +45,7 @@ typedef union FPReg {
>       uint64_t i;
>   } FPReg;
>
> -typedef struct CPUS390XState {
> +struct CPUS390XState {
>       uint64_t regs[16];	/* GP registers */
>
>       uint32_t aregs[16];	/* access registers */
> @@ -64,7 +64,7 @@ typedef struct CPUS390XState {
>       uint64_t __excp_addr;
>
>       CPU_COMMON
> -} CPUS390XState;
> +};
>
>   #if defined(CONFIG_USER_ONLY)
>   static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
> diff --git a/target-s390x/exec.h b/target-s390x/exec.h
> index 837f853..a848f73 100644
> --- a/target-s390x/exec.h
> +++ b/target-s390x/exec.h
> @@ -17,14 +17,13 @@
>    * License along with this library; if not, see<http://www.gnu.org/licenses/>.
>    */
>
> -#include "dyngen-exec.h"
> -
> -register struct CPUS390XState *env asm(AREG0);
> -
>   #include "config.h"
> +#include "dyngen-exec.h"
>   #include "cpu.h"
>   #include "exec-all.h"
>
> +register struct CPUS390XState *env asm(AREG0);
> +
>   #if !defined(CONFIG_USER_ONLY)
>   #include "softmmu_exec.h"
>   #endif /* !defined(CONFIG_USER_ONLY) */
> diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
> index f8b1680..5535ed1 100644
> --- a/target-sh4/cpu.h
> +++ b/target-sh4/cpu.h
> @@ -36,7 +36,7 @@
>   #define SH_CPU_SH7750_ALL (SH_CPU_SH7750 | SH_CPU_SH7750S | SH_CPU_SH7750R)
>   #define SH_CPU_SH7751_ALL (SH_CPU_SH7751 | SH_CPU_SH7751R)
>
> -#define CPUState struct CPUSH4State
> +#define CPUSH4State CPUState
>
>   #include "cpu-defs.h"
>
> @@ -107,7 +107,7 @@ typedef struct memory_content {
>       struct memory_content *next;
>   } memory_content;
>
> -typedef struct CPUSH4State {
> +struct CPUSH4State {
>       int id;			/* CPU model */
>
>       uint32_t flags;		/* general execution flags */
> @@ -157,7 +157,7 @@ typedef struct CPUSH4State {
>       int intr_at_halt;		/* SR_BL ignored during sleep */
>       memory_content *movcal_backup;
>       memory_content **movcal_backup_tail;
> -} CPUSH4State;
> +};
>
>   CPUSH4State *cpu_sh4_init(const char *cpu_model);
>   int cpu_sh4_exec(CPUSH4State * s);
> diff --git a/target-sh4/exec.h b/target-sh4/exec.h
> index edd667d..b2eb306 100644
> --- a/target-sh4/exec.h
> +++ b/target-sh4/exec.h
> @@ -21,12 +21,11 @@
>
>   #include "config.h"
>   #include "dyngen-exec.h"
> -
> -register struct CPUSH4State *env asm(AREG0);
> -
>   #include "cpu.h"
>   #include "exec-all.h"
>
> +register struct CPUSH4State *env asm(AREG0);
> +
>   static inline int cpu_has_work(CPUState *env)
>   {
>       return (env->interrupt_request&  CPU_INTERRUPT_HARD);
> diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
> index 580f4d4..de329a2 100644
> --- a/target-sparc/cpu.h
> +++ b/target-sparc/cpu.h
> @@ -21,7 +21,7 @@
>   #define TARGET_VIRT_ADDR_SPACE_BITS 32
>   #endif
>
> -#define CPUState struct CPUSPARCState
> +#define CPUSPARCState CPUState
>
>   #include "cpu-defs.h"
>
> @@ -316,7 +316,7 @@ struct QEMUFile;
>   void cpu_put_timer(struct QEMUFile *f, CPUTimer *s);
>   void cpu_get_timer(struct QEMUFile *f, CPUTimer *s);
>
> -typedef struct CPUSPARCState {
> +struct CPUSPARCState {
>       target_ulong gregs[8]; /* general registers */
>       target_ulong *regwptr; /* pointer to current register window */
>       target_ulong pc;       /* program counter */
> @@ -432,7 +432,7 @@ typedef struct CPUSPARCState {
>   #define SOFTINT_REG_MASK (SOFTINT_STIMER|SOFTINT_INTRMASK|SOFTINT_TIMER)
>   #endif
>       sparc_def_t *def;
> -} CPUSPARCState;
> +};
>
>   /* helper.c */
>   CPUSPARCState *cpu_sparc_init(const char *cpu_model);
> diff --git a/target-sparc/exec.h b/target-sparc/exec.h
> index 70df828..850f7f6 100644
> --- a/target-sparc/exec.h
> +++ b/target-sparc/exec.h
> @@ -1,7 +1,10 @@
>   #ifndef EXEC_SPARC_H
>   #define EXEC_SPARC_H 1
> +
>   #include "config.h"
>   #include "dyngen-exec.h"
> +#include "cpu.h"
> +#include "exec-all.h"
>
>   register struct CPUSPARCState *env asm(AREG0);
>
> @@ -10,9 +13,6 @@ register struct CPUSPARCState *env asm(AREG0);
>   #define QT0 (env->qt0)
>   #define QT1 (env->qt1)
>
> -#include "cpu.h"
> -#include "exec-all.h"
> -
>   /* op_helper.c */
>   void do_interrupt(CPUState *env);
>
>

On 04/02/2010 05:22 PM, Anthony Liguori wrote:

> hw/* should never access CPUState.
>
> Can you give examples of when qemu-kvm needs this?

Indirectly via header files.  The problem is that GCC poisoning 
complains on prototypes too.

qemu-kvm.h references CPUState and includes cpu.h, so with the latest 
changes all files that include qemu-kvm.h break, even if they don't 
require qemu-kvm.h.  With this patch they instead get the opaque 
definition via hw/hw.h (which includes cpu-common.h), and qemu-kvm.h can 
avoid including cpu.h.

Another example is the new apic.h file created by Blue Swirl.  It 
references CPUState.  It includes apic_get_irq_delivered, so I placed 
apic_set_irq_delivered there too.  But apic_set_irq_delivered is used by 
i8259.c which is compiled once.

There are other similar cases.  Without something like this patch as a 
stopgap measure, you have to make everything compile again per-target 
which is a huge mess of conflicts.

Paolo

On 4/2/10, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 04/02/2010 05:22 PM, Anthony Liguori wrote:
>
>
> > hw/* should never access CPUState.
> >
> > Can you give examples of when qemu-kvm needs this?
> >
>
>  Indirectly via header files.  The problem is that GCC poisoning complains
> on prototypes too.
>
>  qemu-kvm.h references CPUState and includes cpu.h, so with the latest
> changes all files that include qemu-kvm.h break, even if they don't require
> qemu-kvm.h.  With this patch they instead get the opaque definition via
> hw/hw.h (which includes cpu-common.h), and qemu-kvm.h can avoid including
> cpu.h.

This is why I added #ifndef NEED_CPU_H to kvm.h.

>  Another example is the new apic.h file created by Blue Swirl.  It
> references CPUState.  It includes apic_get_irq_delivered, so I placed
> apic_set_irq_delivered there too.  But apic_set_irq_delivered is used by
> i8259.c which is compiled once.

But i8259.c is compiled per target, grep Makefile.target?

>  There are other similar cases.  Without something like this patch as a
> stopgap measure, you have to make everything compile again per-target which
> is a huge mess of conflicts.

This is wrong. There are lot of other ways to handle the need without
resorting to per-target build.

>>   qemu-kvm.h references CPUState and includes cpu.h, so with the latest
>> changes all files that include qemu-kvm.h break, even if they don't require
>> qemu-kvm.h.  With this patch they instead get the opaque definition via
>> hw/hw.h (which includes cpu-common.h), and qemu-kvm.h can avoid including
>> cpu.h.
>
> This is why I added #ifndef NEED_CPU_H to kvm.h.

Yes, but adding NEED_CPU_H everywhere is just as bad, or worse.

>>   Another example is the new apic.h file created by Blue Swirl.  It
>> references CPUState.  It includes apic_get_irq_delivered, so I placed
>> apic_set_irq_delivered there too.  But apic_set_irq_delivered is used by
>> i8259.c which is compiled once.
>
> But i8259.c is compiled per target, grep Makefile.target?

Hmm, that was something else, but apic.h was it. :-)

Paolo

On 4/2/10, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> >
> > >  qemu-kvm.h references CPUState and includes cpu.h, so with the latest
> > > changes all files that include qemu-kvm.h break, even if they don't
> require
> > > qemu-kvm.h.  With this patch they instead get the opaque definition via
> > > hw/hw.h (which includes cpu-common.h), and qemu-kvm.h can avoid
> including
> > > cpu.h.
> > >
> >
> > This is why I added #ifndef NEED_CPU_H to kvm.h.
> >
>
>  Yes, but adding NEED_CPU_H everywhere is just as bad, or worse.

I fully agree that devices should not use NEED_CPU_H.

> >
> > >  Another example is the new apic.h file created by Blue Swirl.  It
> > > references CPUState.  It includes apic_get_irq_delivered, so I placed
> > > apic_set_irq_delivered there too.  But apic_set_irq_delivered is used by
> > > i8259.c which is compiled once.
> > >
> >
> > But i8259.c is compiled per target, grep Makefile.target?
> >
>
>  Hmm, that was something else, but apic.h was it. :-)

As a concrete example, qemu-kvm's apic.c and ours differ w.r.t.
CPUState use only in kvm_save/load_lapic. But these functions could
just as easily take APICState* as their parameter and the need to pass
CPUState is gone.

On 04/02/2010 10:43 AM, Paolo Bonzini wrote:
> On 04/02/2010 05:22 PM, Anthony Liguori wrote:
>
>> hw/* should never access CPUState.
>>
>> Can you give examples of when qemu-kvm needs this?
>
> Indirectly via header files.  The problem is that GCC poisoning 
> complains on prototypes too.
>
> qemu-kvm.h references CPUState and includes cpu.h, so with the latest 
> changes all files that include qemu-kvm.h break, even if they don't 
> require qemu-kvm.h.  With this patch they instead get the opaque 
> definition via hw/hw.h (which includes cpu-common.h), and qemu-kvm.h 
> can avoid including cpu.h.
>
> Another example is the new apic.h file created by Blue Swirl.  It 
> references CPUState.  It includes apic_get_irq_delivered, so I placed 
> apic_set_irq_delivered there too.  But apic_set_irq_delivered is used 
> by i8259.c which is compiled once.
>
> There are other similar cases.  Without something like this patch as a 
> stopgap measure, you have to make everything compile again per-target 
> which is a huge mess of conflicts.

I'd rather things be compiled per-target than adding a bunch of crud 
everywhere.

Wouldn't it be easier to split up qemu-kvm.h into qemu-kvm-cpu.h and add 
the later include where it's needed (which should be very few places)?

Regards,

Anthony Liguori

> Paolo

On 04/02/2010 06:24 PM, Anthony Liguori wrote:
>
> I'd rather things be compiled per-target than adding a bunch of crud
> everywhere.

Well, this patch in particular removes more lines than it adds. :-P

Anyway---me too, given how hairy it's coming out.  Maybe (or without 
maybe) this work should have been done on a branch.

> Wouldn't it be easier to split up qemu-kvm.h into qemu-kvm-cpu.h and add
> the later include where it's needed (which should be very few places)?

I won't have time to do this if you prefer that, I guess I'll have to 
leave it to Marcelo and Avi.

Paolo

On 04/02/2010 11:43 AM, Paolo Bonzini wrote:
> On 04/02/2010 06:24 PM, Anthony Liguori wrote:
>>
>> I'd rather things be compiled per-target than adding a bunch of crud
>> everywhere.
>
> Well, this patch in particular removes more lines than it adds. :-P
>
> Anyway---me too, given how hairy it's coming out.  Maybe (or without 
> maybe) this work should have been done on a branch.

What files need to be compiled per-target to fix qemu-kvm?

Regards,

Anthony Liguori

>> Wouldn't it be easier to split up qemu-kvm.h into qemu-kvm-cpu.h and add
>> the later include where it's needed (which should be very few places)?
>
> I won't have time to do this if you prefer that, I guess I'll have to 
> leave it to Marcelo and Avi.
>
> Paolo

On 04/02/2010 07:36 PM, Anthony Liguori wrote:
> On 04/02/2010 11:43 AM, Paolo Bonzini wrote:
>> On 04/02/2010 06:24 PM, Anthony Liguori wrote:
>>>
>>> I'd rather things be compiled per-target than adding a bunch of crud
>>> everywhere.
>>
>> Well, this patch in particular removes more lines than it adds. :-P
>>
>> Anyway---me too, given how hairy it's coming out. Maybe (or without
>> maybe) this work should have been done on a branch.
>
> What files need to be compiled per-target to fix qemu-kvm?

With this patch, pci.o/i8254.o/acpi.o.  Without, also pcspk.o and acpi.o 
then I stopped.

Paolo

On 04/02/2010 12:46 PM, Paolo Bonzini wrote:
> On 04/02/2010 07:36 PM, Anthony Liguori wrote:
>> On 04/02/2010 11:43 AM, Paolo Bonzini wrote:
>>> On 04/02/2010 06:24 PM, Anthony Liguori wrote:
>>>>
>>>> I'd rather things be compiled per-target than adding a bunch of crud
>>>> everywhere.
>>>
>>> Well, this patch in particular removes more lines than it adds. :-P
>>>
>>> Anyway---me too, given how hairy it's coming out. Maybe (or without
>>> maybe) this work should have been done on a branch.
>>
>> What files need to be compiled per-target to fix qemu-kvm?
>
> With this patch, pci.o/i8254.o/acpi.o.  Without, also pcspk.o and 
> acpi.o then I stopped.

i8254.o has to be a bug.  There should be no reason to include 
qemu-kvm.h there.  Likewise with acpi.o and pcspk.o.

Regards,

Anthony Liguori

> Paolo