Patchwork [v5,2/3] ppc: Fix duplicated typedefs to be able to compile with Clang in gnu99 mode

login
register
mail settings
Submitter Thomas Huth
Date Jan. 11, 2019, 8:17 a.m.
Message ID <1547194672-24169-3-git-send-email-thuth@redhat.com>
Download mbox | patch
Permalink /patch/697469/
State New
Headers show

Comments

Thomas Huth - Jan. 11, 2019, 8:17 a.m.
When compiling the ppc code with clang and -std=gnu99, there are a
couple of warnings/errors like this one:

  CC      ppc64-softmmu/hw/intc/xics.o
In file included from hw/intc/xics.c:35:
include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
      [-Werror,-Wtypedef-redefinition]
typedef struct ICPState ICPState;
                        ^
target/ppc/cpu.h:1181:25: note: previous definition is here
typedef struct ICPState ICPState;
                        ^
Work around the problems by including the proper headers instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/ppc/spapr.h      |  5 +++--
 include/hw/ppc/spapr_xive.h |  2 --
 target/ppc/cpu.h            | 13 +++++++++----
 3 files changed, 12 insertions(+), 8 deletions(-)
Cédric Le Goater - Jan. 11, 2019, 8:30 a.m.
On 1/11/19 9:17 AM, Thomas Huth wrote:
> When compiling the ppc code with clang and -std=gnu99, there are a
> couple of warnings/errors like this one:
> 
>   CC      ppc64-softmmu/hw/intc/xics.o
> In file included from hw/intc/xics.c:35:
> include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct ICPState ICPState;
>                         ^
> target/ppc/cpu.h:1181:25: note: previous definition is here
> typedef struct ICPState ICPState;
>                         ^
> Work around the problems by including the proper headers instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks for sorting this out,

C.

> ---
>  include/hw/ppc/spapr.h      |  5 +++--
>  include/hw/ppc/spapr_xive.h |  2 --
>  target/ppc/cpu.h            | 13 +++++++++----
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 9e01a5a..a947a0a 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -8,15 +8,16 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/ppc/spapr_ovec.h"
>  #include "hw/ppc/spapr_irq.h"
> +#include "hw/ppc/spapr_xive.h"  /* For sPAPRXive */
> +#include "hw/ppc/xics.h"        /* For ICSState */
>  
>  struct VIOsPAPRBus;
>  struct sPAPRPHBState;
>  struct sPAPRNVRAM;
> +
>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>  typedef struct sPAPREventSource sPAPREventSource;
>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> -typedef struct ICSState ICSState;
> -typedef struct sPAPRXive sPAPRXive;
>  
>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>  #define SPAPR_ENTRY_POINT       0x100
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 7fdc250..9bec919 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -41,8 +41,6 @@ bool spapr_xive_irq_claim(sPAPRXive *xive, uint32_t lisn, bool lsi);
>  bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
>  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
>  
> -typedef struct sPAPRMachineState sPAPRMachineState;
> -
>  void spapr_xive_hcall_init(sPAPRMachineState *spapr);
>  void spapr_dt_xive(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
>                     uint32_t phandle);
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 486abaf..bde4dff 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -23,6 +23,11 @@
>  #include "qemu-common.h"
>  #include "qemu/int128.h"
>  
> +#ifndef CONFIG_USER_ONLY
> +#include "hw/ppc/xive.h" /* for XiveTCTX */
> +#include "hw/ppc/xics.h" /* for ICPState */
> +#endif
> +
>  //#define PPC_EMULATE_32BITS_HYPV
>  
>  #if defined (TARGET_PPC64)
> @@ -1177,8 +1182,6 @@ do {                                            \
>  
>  typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
>  typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
> -typedef struct XiveTCTX XiveTCTX;
> -typedef struct ICPState ICPState;
>  
>  /**
>   * PowerPCCPU:
> @@ -1197,11 +1200,13 @@ struct PowerPCCPU {
>      int vcpu_id;
>      uint32_t compat_pvr;
>      PPCVirtualHypervisor *vhyp;
> -    ICPState *icp;
> -    XiveTCTX *tctx;
>      void *machine_data;
>      int32_t node_id; /* NUMA node this CPU belongs to */
>      PPCHash64Options *hash64_opts;
> +#ifndef CONFIG_USER_ONLY
> +    ICPState *icp;
> +    XiveTCTX *tctx;
> +#endif
>  
>      /* Fields related to migration compatibility hacks */
>      bool pre_2_8_migration;
>
Greg Kurz - Jan. 11, 2019, 8:40 a.m.
On Fri, 11 Jan 2019 09:17:51 +0100
Thomas Huth <thuth@redhat.com> wrote:

> When compiling the ppc code with clang and -std=gnu99, there are a
> couple of warnings/errors like this one:
> 
>   CC      ppc64-softmmu/hw/intc/xics.o
> In file included from hw/intc/xics.c:35:
> include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct ICPState ICPState;
>                         ^
> target/ppc/cpu.h:1181:25: note: previous definition is here
> typedef struct ICPState ICPState;
>                         ^
> Work around the problems by including the proper headers instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  include/hw/ppc/spapr.h      |  5 +++--
>  include/hw/ppc/spapr_xive.h |  2 --
>  target/ppc/cpu.h            | 13 +++++++++----
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 9e01a5a..a947a0a 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -8,15 +8,16 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/ppc/spapr_ovec.h"
>  #include "hw/ppc/spapr_irq.h"
> +#include "hw/ppc/spapr_xive.h"  /* For sPAPRXive */
> +#include "hw/ppc/xics.h"        /* For ICSState */
>  
>  struct VIOsPAPRBus;
>  struct sPAPRPHBState;
>  struct sPAPRNVRAM;
> +
>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>  typedef struct sPAPREventSource sPAPREventSource;
>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> -typedef struct ICSState ICSState;
> -typedef struct sPAPRXive sPAPRXive;
>  
>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>  #define SPAPR_ENTRY_POINT       0x100
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 7fdc250..9bec919 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -41,8 +41,6 @@ bool spapr_xive_irq_claim(sPAPRXive *xive, uint32_t lisn, bool lsi);
>  bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
>  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
>  
> -typedef struct sPAPRMachineState sPAPRMachineState;
> -
>  void spapr_xive_hcall_init(sPAPRMachineState *spapr);
>  void spapr_dt_xive(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
>                     uint32_t phandle);
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 486abaf..bde4dff 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -23,6 +23,11 @@
>  #include "qemu-common.h"
>  #include "qemu/int128.h"
>  
> +#ifndef CONFIG_USER_ONLY
> +#include "hw/ppc/xive.h" /* for XiveTCTX */
> +#include "hw/ppc/xics.h" /* for ICPState */
> +#endif
> +
>  //#define PPC_EMULATE_32BITS_HYPV
>  
>  #if defined (TARGET_PPC64)
> @@ -1177,8 +1182,6 @@ do {                                            \
>  
>  typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
>  typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
> -typedef struct XiveTCTX XiveTCTX;
> -typedef struct ICPState ICPState;
>  
>  /**
>   * PowerPCCPU:
> @@ -1197,11 +1200,13 @@ struct PowerPCCPU {
>      int vcpu_id;
>      uint32_t compat_pvr;
>      PPCVirtualHypervisor *vhyp;
> -    ICPState *icp;
> -    XiveTCTX *tctx;
>      void *machine_data;
>      int32_t node_id; /* NUMA node this CPU belongs to */
>      PPCHash64Options *hash64_opts;
> +#ifndef CONFIG_USER_ONLY
> +    ICPState *icp;
> +    XiveTCTX *tctx;
> +#endif
>  
>      /* Fields related to migration compatibility hacks */
>      bool pre_2_8_migration;
Daniel P. Berrange - Jan. 11, 2019, 9:22 a.m.
On Fri, Jan 11, 2019 at 09:17:51AM +0100, Thomas Huth wrote:
> When compiling the ppc code with clang and -std=gnu99, there are a
> couple of warnings/errors like this one:
> 
>   CC      ppc64-softmmu/hw/intc/xics.o
> In file included from hw/intc/xics.c:35:
> include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct ICPState ICPState;
>                         ^
> target/ppc/cpu.h:1181:25: note: previous definition is here
> typedef struct ICPState ICPState;
>                         ^
> Work around the problems by including the proper headers instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/hw/ppc/spapr.h      |  5 +++--
>  include/hw/ppc/spapr_xive.h |  2 --
>  target/ppc/cpu.h            | 13 +++++++++----
>  3 files changed, 12 insertions(+), 8 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
Philippe Mathieu-Daudé - Jan. 11, 2019, 12:59 p.m.
On 1/11/19 9:17 AM, Thomas Huth wrote:
> When compiling the ppc code with clang and -std=gnu99, there are a
> couple of warnings/errors like this one:
> 
>   CC      ppc64-softmmu/hw/intc/xics.o
> In file included from hw/intc/xics.c:35:
> include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct ICPState ICPState;
>                         ^
> target/ppc/cpu.h:1181:25: note: previous definition is here
> typedef struct ICPState ICPState;
>                         ^
> Work around the problems by including the proper headers instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/hw/ppc/spapr.h      |  5 +++--
>  include/hw/ppc/spapr_xive.h |  2 --
>  target/ppc/cpu.h            | 13 +++++++++----
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 9e01a5a..a947a0a 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -8,15 +8,16 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/ppc/spapr_ovec.h"
>  #include "hw/ppc/spapr_irq.h"
> +#include "hw/ppc/spapr_xive.h"  /* For sPAPRXive */
> +#include "hw/ppc/xics.h"        /* For ICSState */
>  
>  struct VIOsPAPRBus;
>  struct sPAPRPHBState;
>  struct sPAPRNVRAM;
> +
>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>  typedef struct sPAPREventSource sPAPREventSource;
>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> -typedef struct ICSState ICSState;
> -typedef struct sPAPRXive sPAPRXive;
>  
>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>  #define SPAPR_ENTRY_POINT       0x100
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 7fdc250..9bec919 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -41,8 +41,6 @@ bool spapr_xive_irq_claim(sPAPRXive *xive, uint32_t lisn, bool lsi);
>  bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
>  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
>  
> -typedef struct sPAPRMachineState sPAPRMachineState;
> -
>  void spapr_xive_hcall_init(sPAPRMachineState *spapr);
>  void spapr_dt_xive(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
>                     uint32_t phandle);
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 486abaf..bde4dff 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -23,6 +23,11 @@
>  #include "qemu-common.h"
>  #include "qemu/int128.h"
>  
> +#ifndef CONFIG_USER_ONLY
> +#include "hw/ppc/xive.h" /* for XiveTCTX */
> +#include "hw/ppc/xics.h" /* for ICPState */
> +#endif

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +
>  //#define PPC_EMULATE_32BITS_HYPV
>  
>  #if defined (TARGET_PPC64)
> @@ -1177,8 +1182,6 @@ do {                                            \
>  
>  typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
>  typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
> -typedef struct XiveTCTX XiveTCTX;
> -typedef struct ICPState ICPState;
>  
>  /**
>   * PowerPCCPU:
> @@ -1197,11 +1200,13 @@ struct PowerPCCPU {
>      int vcpu_id;
>      uint32_t compat_pvr;
>      PPCVirtualHypervisor *vhyp;
> -    ICPState *icp;
> -    XiveTCTX *tctx;
>      void *machine_data;
>      int32_t node_id; /* NUMA node this CPU belongs to */
>      PPCHash64Options *hash64_opts;
> +#ifndef CONFIG_USER_ONLY
> +    ICPState *icp;
> +    XiveTCTX *tctx;
> +#endif
>  
>      /* Fields related to migration compatibility hacks */
>      bool pre_2_8_migration;
>
Cédric Le Goater - Jan. 16, 2019, 11:43 a.m.
On 1/11/19 9:17 AM, Thomas Huth wrote:
> When compiling the ppc code with clang and -std=gnu99, there are a
> couple of warnings/errors like this one:
> 
>   CC      ppc64-softmmu/hw/intc/xics.o
> In file included from hw/intc/xics.c:35:
> include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct ICPState ICPState;
>                         ^
> target/ppc/cpu.h:1181:25: note: previous definition is here
> typedef struct ICPState ICPState;
>                         ^
> Work around the problems by including the proper headers instead.

Thomas,


After a closer look, I think we should use 'void *' under PowerPCCPU 
as it was the case before I introduced the second interrupt presenter.

That's a bigger change reverting bits of already merged patches. I can
take care of it if you prefer. 

I use a f29 for dev. Which compiler should I install ? 

C.

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/hw/ppc/spapr.h      |  5 +++--
>  include/hw/ppc/spapr_xive.h |  2 --
>  target/ppc/cpu.h            | 13 +++++++++----
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 9e01a5a..a947a0a 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -8,15 +8,16 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/ppc/spapr_ovec.h"
>  #include "hw/ppc/spapr_irq.h"
> +#include "hw/ppc/spapr_xive.h"  /* For sPAPRXive */
> +#include "hw/ppc/xics.h"        /* For ICSState */
>  
>  struct VIOsPAPRBus;
>  struct sPAPRPHBState;
>  struct sPAPRNVRAM;
> +
>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>  typedef struct sPAPREventSource sPAPREventSource;
>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> -typedef struct ICSState ICSState;
> -typedef struct sPAPRXive sPAPRXive;
>  
>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>  #define SPAPR_ENTRY_POINT       0x100
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 7fdc250..9bec919 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -41,8 +41,6 @@ bool spapr_xive_irq_claim(sPAPRXive *xive, uint32_t lisn, bool lsi);
>  bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
>  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
>  
> -typedef struct sPAPRMachineState sPAPRMachineState;
> -
>  void spapr_xive_hcall_init(sPAPRMachineState *spapr);
>  void spapr_dt_xive(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
>                     uint32_t phandle);
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 486abaf..bde4dff 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -23,6 +23,11 @@
>  #include "qemu-common.h"
>  #include "qemu/int128.h"
>  
> +#ifndef CONFIG_USER_ONLY
> +#include "hw/ppc/xive.h" /* for XiveTCTX */
> +#include "hw/ppc/xics.h" /* for ICPState */
> +#endif
> +
>  //#define PPC_EMULATE_32BITS_HYPV
>  
>  #if defined (TARGET_PPC64)
> @@ -1177,8 +1182,6 @@ do {                                            \
>  
>  typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
>  typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
> -typedef struct XiveTCTX XiveTCTX;
> -typedef struct ICPState ICPState;
>  
>  /**
>   * PowerPCCPU:
> @@ -1197,11 +1200,13 @@ struct PowerPCCPU {
>      int vcpu_id;
>      uint32_t compat_pvr;
>      PPCVirtualHypervisor *vhyp;
> -    ICPState *icp;
> -    XiveTCTX *tctx;
>      void *machine_data;
>      int32_t node_id; /* NUMA node this CPU belongs to */
>      PPCHash64Options *hash64_opts;
> +#ifndef CONFIG_USER_ONLY
> +    ICPState *icp;
> +    XiveTCTX *tctx;
> +#endif
>  
>      /* Fields related to migration compatibility hacks */
>      bool pre_2_8_migration;
>
Thomas Huth - Jan. 16, 2019, 11:47 a.m.
On 2019-01-16 12:43, Cédric Le Goater wrote:
> On 1/11/19 9:17 AM, Thomas Huth wrote:
>> When compiling the ppc code with clang and -std=gnu99, there are a
>> couple of warnings/errors like this one:
>>
>>   CC      ppc64-softmmu/hw/intc/xics.o
>> In file included from hw/intc/xics.c:35:
>> include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
>>       [-Werror,-Wtypedef-redefinition]
>> typedef struct ICPState ICPState;
>>                         ^
>> target/ppc/cpu.h:1181:25: note: previous definition is here
>> typedef struct ICPState ICPState;
>>                         ^
>> Work around the problems by including the proper headers instead.
> 
> Thomas,
> 
> 
> After a closer look, I think we should use 'void *' under PowerPCCPU 
> as it was the case before I introduced the second interrupt presenter.

If you don't like the #includes, why not simply do anonymous struct
forward declarations here? I think that would be better than "void *".

> That's a bigger change reverting bits of already merged patches. I can
> take care of it if you prefer. 

Could I keep the current patch in my series so that I can get the
patches finally merged? You could then do any clean up that you like on
top of it, ok?

> I use a f29 for dev. Which compiler should I install ? 

Any version of Clang with -std=gnu99 should do the job here, I think.

 Thomas
Greg Kurz - Jan. 16, 2019, 1:23 p.m.
On Wed, 16 Jan 2019 12:47:36 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 2019-01-16 12:43, Cédric Le Goater wrote:
> > On 1/11/19 9:17 AM, Thomas Huth wrote:  
> >> When compiling the ppc code with clang and -std=gnu99, there are a
> >> couple of warnings/errors like this one:
> >>
> >>   CC      ppc64-softmmu/hw/intc/xics.o
> >> In file included from hw/intc/xics.c:35:
> >> include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
> >>       [-Werror,-Wtypedef-redefinition]
> >> typedef struct ICPState ICPState;
> >>                         ^
> >> target/ppc/cpu.h:1181:25: note: previous definition is here
> >> typedef struct ICPState ICPState;
> >>                         ^
> >> Work around the problems by including the proper headers instead.  
> > 
> > Thomas,
> > 
> > 
> > After a closer look, I think we should use 'void *' under PowerPCCPU 
> > as it was the case before I introduced the second interrupt presenter.  
> 
> If you don't like the #includes, why not simply do anonymous struct
> forward declarations here? I think that would be better than "void *".
> 

That's questionable. These two fields are only used by the machine code and
the interrupt controller code.

$ git grep -E '(icp|tctx)' target/ppc/
target/ppc/cpu.h:    ICPState *icp;
target/ppc/cpu.h:    XiveTCTX *tctx;

$ git grep -E 'cpu\->(icp|tctx)' 
hw/intc/spapr_xive_kvm.c:        kvmppc_xive_cpu_set_state(cpu->tctx, &local_err);
hw/intc/spapr_xive_kvm.c:        kvmppc_xive_cpu_connect(cpu->tctx, &local_err);
hw/intc/xics_kvm.c:        icp_kvm_connect(cpu->icp, &local_err);
hw/intc/xics_kvm.c:        icp_set_kvm_state(cpu->icp, 1);
hw/intc/xics_spapr.c:    icp_set_cppr(cpu->icp, cppr);
hw/intc/xics_spapr.c:    uint32_t xirr = icp_accept(cpu->icp);
hw/intc/xics_spapr.c:    uint32_t xirr = icp_accept(cpu->icp);
hw/intc/xics_spapr.c:    icp_eoi(cpu->icp, xirr);
hw/intc/xics_spapr.c:    uint32_t xirr = icp_ipoll(cpu->icp, &mfrr);
hw/intc/xive.c:    XiveTCTX *tctx = cpu->tctx;
hw/intc/xive.c:    XiveTCTX *tctx = cpu->tctx;
hw/intc/xive.c:        XiveTCTX *tctx = cpu->tctx;
hw/ppc/pnv.c:    cpu->icp = ICP(obj);
hw/ppc/pnv.c:    return cpu ? cpu->icp : NULL;
hw/ppc/pnv.c:        icp_pic_print_info(cpu->icp, mon);
hw/ppc/pnv_core.c:    object_unparent(OBJECT(cpu->icp));
hw/ppc/spapr.c:    return cpu ? cpu->icp : NULL;
hw/ppc/spapr_cpu_core.c:    if (cpu->icp) {
hw/ppc/spapr_cpu_core.c:        object_unparent(OBJECT(cpu->icp));
hw/ppc/spapr_cpu_core.c:    if (cpu->tctx) {
hw/ppc/spapr_cpu_core.c:        object_unparent(OBJECT(cpu->tctx));
hw/ppc/spapr_irq.c:        icp_pic_print_info(cpu->icp, mon);
hw/ppc/spapr_irq.c:    cpu->icp = ICP(obj);
hw/ppc/spapr_irq.c:            icp_resend(cpu->icp);
hw/ppc/spapr_irq.c:        xive_tctx_pic_print_info(cpu->tctx, mon);
hw/ppc/spapr_irq.c:    cpu->tctx = XIVE_TCTX(obj);
hw/ppc/spapr_irq.c:    spapr_xive_set_tctx_os_cam(cpu->tctx);
hw/ppc/spapr_irq.c:        spapr_xive_set_tctx_os_cam(cpu->tctx);
hw/ppc/spapr_irq.c:        spapr_xive_set_tctx_os_cam(cpu->tctx);

It thus looks wrong to expose their type in target/ppc/cpu.h. I guess
they should be hidden behind an opaque data pointer (maybe the existing
void *machine_data ?)

> > That's a bigger change reverting bits of already merged patches. I can
> > take care of it if you prefer.   
> 
> Could I keep the current patch in my series so that I can get the
> patches finally merged? You could then do any clean up that you like on
> top of it, ok?
> 
> > I use a f29 for dev. Which compiler should I install ?   
> 
> Any version of Clang with -std=gnu99 should do the job here, I think.
> 
>  Thomas
Thomas Huth - Jan. 16, 2019, 1:44 p.m.
On 2019-01-16 14:23, Greg Kurz wrote:
> On Wed, 16 Jan 2019 12:47:36 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 2019-01-16 12:43, Cédric Le Goater wrote:
>>> On 1/11/19 9:17 AM, Thomas Huth wrote:  
>>>> When compiling the ppc code with clang and -std=gnu99, there are a
>>>> couple of warnings/errors like this one:
>>>>
>>>>   CC      ppc64-softmmu/hw/intc/xics.o
>>>> In file included from hw/intc/xics.c:35:
>>>> include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
>>>>       [-Werror,-Wtypedef-redefinition]
>>>> typedef struct ICPState ICPState;
>>>>                         ^
>>>> target/ppc/cpu.h:1181:25: note: previous definition is here
>>>> typedef struct ICPState ICPState;
>>>>                         ^
>>>> Work around the problems by including the proper headers instead.  
>>>
>>> Thomas,
>>>
>>>
>>> After a closer look, I think we should use 'void *' under PowerPCCPU 
>>> as it was the case before I introduced the second interrupt presenter.  
>>
>> If you don't like the #includes, why not simply do anonymous struct
>> forward declarations here? I think that would be better than "void *".
>>
> 
> That's questionable. These two fields are only used by the machine code and
> the interrupt controller code.
> 
> $ git grep -E '(icp|tctx)' target/ppc/
> target/ppc/cpu.h:    ICPState *icp;
> target/ppc/cpu.h:    XiveTCTX *tctx;
> 
> $ git grep -E 'cpu\->(icp|tctx)' 
> hw/intc/spapr_xive_kvm.c:        kvmppc_xive_cpu_set_state(cpu->tctx, &local_err);
> hw/intc/spapr_xive_kvm.c:        kvmppc_xive_cpu_connect(cpu->tctx, &local_err);
> hw/intc/xics_kvm.c:        icp_kvm_connect(cpu->icp, &local_err);
> hw/intc/xics_kvm.c:        icp_set_kvm_state(cpu->icp, 1);
> hw/intc/xics_spapr.c:    icp_set_cppr(cpu->icp, cppr);
> hw/intc/xics_spapr.c:    uint32_t xirr = icp_accept(cpu->icp);
> hw/intc/xics_spapr.c:    uint32_t xirr = icp_accept(cpu->icp);
> hw/intc/xics_spapr.c:    icp_eoi(cpu->icp, xirr);
> hw/intc/xics_spapr.c:    uint32_t xirr = icp_ipoll(cpu->icp, &mfrr);
> hw/intc/xive.c:    XiveTCTX *tctx = cpu->tctx;
> hw/intc/xive.c:    XiveTCTX *tctx = cpu->tctx;
> hw/intc/xive.c:        XiveTCTX *tctx = cpu->tctx;
> hw/ppc/pnv.c:    cpu->icp = ICP(obj);
> hw/ppc/pnv.c:    return cpu ? cpu->icp : NULL;
> hw/ppc/pnv.c:        icp_pic_print_info(cpu->icp, mon);
> hw/ppc/pnv_core.c:    object_unparent(OBJECT(cpu->icp));
> hw/ppc/spapr.c:    return cpu ? cpu->icp : NULL;
> hw/ppc/spapr_cpu_core.c:    if (cpu->icp) {
> hw/ppc/spapr_cpu_core.c:        object_unparent(OBJECT(cpu->icp));
> hw/ppc/spapr_cpu_core.c:    if (cpu->tctx) {
> hw/ppc/spapr_cpu_core.c:        object_unparent(OBJECT(cpu->tctx));
> hw/ppc/spapr_irq.c:        icp_pic_print_info(cpu->icp, mon);
> hw/ppc/spapr_irq.c:    cpu->icp = ICP(obj);
> hw/ppc/spapr_irq.c:            icp_resend(cpu->icp);
> hw/ppc/spapr_irq.c:        xive_tctx_pic_print_info(cpu->tctx, mon);
> hw/ppc/spapr_irq.c:    cpu->tctx = XIVE_TCTX(obj);
> hw/ppc/spapr_irq.c:    spapr_xive_set_tctx_os_cam(cpu->tctx);
> hw/ppc/spapr_irq.c:        spapr_xive_set_tctx_os_cam(cpu->tctx);
> hw/ppc/spapr_irq.c:        spapr_xive_set_tctx_os_cam(cpu->tctx);
> 
> It thus looks wrong to expose their type in target/ppc/cpu.h. I guess
> they should be hidden behind an opaque data pointer (maybe the existing
> void *machine_data ?)

While that's maybe cleaner at a quick glance first, you still have to
deal with the fact that it's used by two different machines (pseries and
pnv) ... not sure whether that easily distinguishable in xive.c for example?

 Thomas
Greg Kurz - Jan. 16, 2019, 3:11 p.m.
On Wed, 16 Jan 2019 14:44:55 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 2019-01-16 14:23, Greg Kurz wrote:
> > On Wed, 16 Jan 2019 12:47:36 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 2019-01-16 12:43, Cédric Le Goater wrote:  
> >>> On 1/11/19 9:17 AM, Thomas Huth wrote:    
> >>>> When compiling the ppc code with clang and -std=gnu99, there are a
> >>>> couple of warnings/errors like this one:
> >>>>
> >>>>   CC      ppc64-softmmu/hw/intc/xics.o
> >>>> In file included from hw/intc/xics.c:35:
> >>>> include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
> >>>>       [-Werror,-Wtypedef-redefinition]
> >>>> typedef struct ICPState ICPState;
> >>>>                         ^
> >>>> target/ppc/cpu.h:1181:25: note: previous definition is here
> >>>> typedef struct ICPState ICPState;
> >>>>                         ^
> >>>> Work around the problems by including the proper headers instead.    
> >>>
> >>> Thomas,
> >>>
> >>>
> >>> After a closer look, I think we should use 'void *' under PowerPCCPU 
> >>> as it was the case before I introduced the second interrupt presenter.    
> >>
> >> If you don't like the #includes, why not simply do anonymous struct
> >> forward declarations here? I think that would be better than "void *".
> >>  
> > 
> > That's questionable. These two fields are only used by the machine code and
> > the interrupt controller code.
> > 
> > $ git grep -E '(icp|tctx)' target/ppc/
> > target/ppc/cpu.h:    ICPState *icp;
> > target/ppc/cpu.h:    XiveTCTX *tctx;
> > 
> > $ git grep -E 'cpu\->(icp|tctx)' 
> > hw/intc/spapr_xive_kvm.c:        kvmppc_xive_cpu_set_state(cpu->tctx, &local_err);
> > hw/intc/spapr_xive_kvm.c:        kvmppc_xive_cpu_connect(cpu->tctx, &local_err);
> > hw/intc/xics_kvm.c:        icp_kvm_connect(cpu->icp, &local_err);
> > hw/intc/xics_kvm.c:        icp_set_kvm_state(cpu->icp, 1);
> > hw/intc/xics_spapr.c:    icp_set_cppr(cpu->icp, cppr);
> > hw/intc/xics_spapr.c:    uint32_t xirr = icp_accept(cpu->icp);
> > hw/intc/xics_spapr.c:    uint32_t xirr = icp_accept(cpu->icp);
> > hw/intc/xics_spapr.c:    icp_eoi(cpu->icp, xirr);
> > hw/intc/xics_spapr.c:    uint32_t xirr = icp_ipoll(cpu->icp, &mfrr);
> > hw/intc/xive.c:    XiveTCTX *tctx = cpu->tctx;
> > hw/intc/xive.c:    XiveTCTX *tctx = cpu->tctx;
> > hw/intc/xive.c:        XiveTCTX *tctx = cpu->tctx;
> > hw/ppc/pnv.c:    cpu->icp = ICP(obj);
> > hw/ppc/pnv.c:    return cpu ? cpu->icp : NULL;
> > hw/ppc/pnv.c:        icp_pic_print_info(cpu->icp, mon);
> > hw/ppc/pnv_core.c:    object_unparent(OBJECT(cpu->icp));
> > hw/ppc/spapr.c:    return cpu ? cpu->icp : NULL;
> > hw/ppc/spapr_cpu_core.c:    if (cpu->icp) {
> > hw/ppc/spapr_cpu_core.c:        object_unparent(OBJECT(cpu->icp));
> > hw/ppc/spapr_cpu_core.c:    if (cpu->tctx) {
> > hw/ppc/spapr_cpu_core.c:        object_unparent(OBJECT(cpu->tctx));
> > hw/ppc/spapr_irq.c:        icp_pic_print_info(cpu->icp, mon);
> > hw/ppc/spapr_irq.c:    cpu->icp = ICP(obj);
> > hw/ppc/spapr_irq.c:            icp_resend(cpu->icp);
> > hw/ppc/spapr_irq.c:        xive_tctx_pic_print_info(cpu->tctx, mon);
> > hw/ppc/spapr_irq.c:    cpu->tctx = XIVE_TCTX(obj);
> > hw/ppc/spapr_irq.c:    spapr_xive_set_tctx_os_cam(cpu->tctx);
> > hw/ppc/spapr_irq.c:        spapr_xive_set_tctx_os_cam(cpu->tctx);
> > hw/ppc/spapr_irq.c:        spapr_xive_set_tctx_os_cam(cpu->tctx);
> > 
> > It thus looks wrong to expose their type in target/ppc/cpu.h. I guess
> > they should be hidden behind an opaque data pointer (maybe the existing
> > void *machine_data ?)  
> 
> While that's maybe cleaner at a quick glance first, you still have to
> deal with the fact that it's used by two different machines (pseries and
> pnv) ... not sure whether that easily distinguishable in xive.c for example?
> 

FWIW xive.c is only used by pseries for the moment but this is likely to
change in the future. Cedric already has patches for this.

Anyway, your concern is legitimate, so let's have a closer look at those
cpu->icp and cpu->tctx users. Most of them are something like:

    CPU_FOREACH(cs) {
        PowerPCCPU *cpu = POWERPC_CPU(cs);

        // Do something with cpu->icp or cpu->tctx exclusively,
        // not any other field of PowerPCCPU.
    }

<thinking aloud>
I guess these could be addressed by having a list of XiveTCTX and a
list of ICPState instead of _hijacking_ the CPU list.
</thinking aloud>

Then we have the hcalls in xics_spapr.c. Since we know this is
pseries, we could rely on the existing void *machine_data, ie,
move the cpu->icp to sPAPRCPUState and access it thanks
to spapr_cpu_state().

And then, we have the xive_tm_ops in xive.c, which currently
does:

static void xive_tm_write(void *opaque, hwaddr offset,
                          uint64_t value, unsigned size)
{
    PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
    XiveTCTX *tctx = cpu->tctx;

It could do something like:

    XiveTCTX *tctx = xive_router_get_tm_tctx(opaque, offset);

with a new XiveRouterClass ->get_tm_tctx() method:

XiveTCTX *xive_router_get_tm_tctx(XiveRouter *xrtr, hwaddr offset)
{
    XiveRouterClass *xrc = XIVE_ROUTER_GET_CLASS(xrtr);

    return xrc->get_tm_tctx(xrtr, offset);
}

On pseries, if we move cpu->tctx to sPAPRCPUState, then we just
need to implement the ->get_tm_tctx() in spapr_xive.c as:

static XiveTCTX *spapr_xive_get_tm_tctx(XiveRouter *xrtr, hwaddr offset)
{
    return spapr_cpu_state(POWERPC_CPU(current_cpu))->tctx;
}

Something similar should be done for pnv.

Cheers,

--
Greg

>  Thomas
Cédric Le Goater - Jan. 16, 2019, 5:26 p.m.
On 1/16/19 4:11 PM, Greg Kurz wrote:
> On Wed, 16 Jan 2019 14:44:55 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 2019-01-16 14:23, Greg Kurz wrote:
>>> On Wed, 16 Jan 2019 12:47:36 +0100
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>   
>>>> On 2019-01-16 12:43, Cédric Le Goater wrote:  
>>>>> On 1/11/19 9:17 AM, Thomas Huth wrote:    
>>>>>> When compiling the ppc code with clang and -std=gnu99, there are a
>>>>>> couple of warnings/errors like this one:
>>>>>>
>>>>>>   CC      ppc64-softmmu/hw/intc/xics.o
>>>>>> In file included from hw/intc/xics.c:35:
>>>>>> include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
>>>>>>       [-Werror,-Wtypedef-redefinition]
>>>>>> typedef struct ICPState ICPState;
>>>>>>                         ^
>>>>>> target/ppc/cpu.h:1181:25: note: previous definition is here
>>>>>> typedef struct ICPState ICPState;
>>>>>>                         ^
>>>>>> Work around the problems by including the proper headers instead.    
>>>>>
>>>>> Thomas,
>>>>>
>>>>>
>>>>> After a closer look, I think we should use 'void *' under PowerPCCPU 
>>>>> as it was the case before I introduced the second interrupt presenter.    
>>>>
>>>> If you don't like the #includes, why not simply do anonymous struct
>>>> forward declarations here? I think that would be better than "void *".
>>>>  
>>>
>>> That's questionable. These two fields are only used by the machine code and
>>> the interrupt controller code.
>>>
>>> $ git grep -E '(icp|tctx)' target/ppc/
>>> target/ppc/cpu.h:    ICPState *icp;
>>> target/ppc/cpu.h:    XiveTCTX *tctx;
>>>
>>> $ git grep -E 'cpu\->(icp|tctx)' 
>>> hw/intc/spapr_xive_kvm.c:        kvmppc_xive_cpu_set_state(cpu->tctx, &local_err);
>>> hw/intc/spapr_xive_kvm.c:        kvmppc_xive_cpu_connect(cpu->tctx, &local_err);
>>> hw/intc/xics_kvm.c:        icp_kvm_connect(cpu->icp, &local_err);
>>> hw/intc/xics_kvm.c:        icp_set_kvm_state(cpu->icp, 1);
>>> hw/intc/xics_spapr.c:    icp_set_cppr(cpu->icp, cppr);
>>> hw/intc/xics_spapr.c:    uint32_t xirr = icp_accept(cpu->icp);
>>> hw/intc/xics_spapr.c:    uint32_t xirr = icp_accept(cpu->icp);
>>> hw/intc/xics_spapr.c:    icp_eoi(cpu->icp, xirr);
>>> hw/intc/xics_spapr.c:    uint32_t xirr = icp_ipoll(cpu->icp, &mfrr);
>>> hw/intc/xive.c:    XiveTCTX *tctx = cpu->tctx;
>>> hw/intc/xive.c:    XiveTCTX *tctx = cpu->tctx;
>>> hw/intc/xive.c:        XiveTCTX *tctx = cpu->tctx;
>>> hw/ppc/pnv.c:    cpu->icp = ICP(obj);
>>> hw/ppc/pnv.c:    return cpu ? cpu->icp : NULL;
>>> hw/ppc/pnv.c:        icp_pic_print_info(cpu->icp, mon);
>>> hw/ppc/pnv_core.c:    object_unparent(OBJECT(cpu->icp));
>>> hw/ppc/spapr.c:    return cpu ? cpu->icp : NULL;
>>> hw/ppc/spapr_cpu_core.c:    if (cpu->icp) {
>>> hw/ppc/spapr_cpu_core.c:        object_unparent(OBJECT(cpu->icp));
>>> hw/ppc/spapr_cpu_core.c:    if (cpu->tctx) {
>>> hw/ppc/spapr_cpu_core.c:        object_unparent(OBJECT(cpu->tctx));
>>> hw/ppc/spapr_irq.c:        icp_pic_print_info(cpu->icp, mon);
>>> hw/ppc/spapr_irq.c:    cpu->icp = ICP(obj);
>>> hw/ppc/spapr_irq.c:            icp_resend(cpu->icp);
>>> hw/ppc/spapr_irq.c:        xive_tctx_pic_print_info(cpu->tctx, mon);
>>> hw/ppc/spapr_irq.c:    cpu->tctx = XIVE_TCTX(obj);
>>> hw/ppc/spapr_irq.c:    spapr_xive_set_tctx_os_cam(cpu->tctx);
>>> hw/ppc/spapr_irq.c:        spapr_xive_set_tctx_os_cam(cpu->tctx);
>>> hw/ppc/spapr_irq.c:        spapr_xive_set_tctx_os_cam(cpu->tctx);
>>>
>>> It thus looks wrong to expose their type in target/ppc/cpu.h. I guess
>>> they should be hidden behind an opaque data pointer (maybe the existing
>>> void *machine_data ?)  

yes. That was sort of a mid term plan but I guess it has become short 
term now.

Anyhow, I don't think we should block Thomas gnu99 patchset because
it's not ready yet.  

>> While that's maybe cleaner at a quick glance first, you still have to
>> deal with the fact that it's used by two different machines (pseries and
>> pnv) ... not sure whether that easily distinguishable in xive.c for example?
>>
> 
> FWIW xive.c is only used by pseries for the moment but this is likely to
> change in the future. Cedric already has patches for this.

Yes. Because XIVE on baremetal has a way to indirectly perform load/store 
on the TIMA. So we can not just use the current_cpu anymore and we need 
to peek at the XIVE controller registers to know on behalf of which 
CPU we are accessing memory. This requires an extra method in the 
XiveRouter that we could use for the cleanups you are proposing.

> Anyway, your concern is legitimate, so let's have a closer look at those
> cpu->icp and cpu->tctx users. Most of them are something like:
> 
>     CPU_FOREACH(cs) {
>         PowerPCCPU *cpu = POWERPC_CPU(cs);
> 
>         // Do something with cpu->icp or cpu->tctx exclusively,
>         // not any other field of PowerPCCPU.
>     }
> 
> <thinking aloud>
> I guess these could be addressed by having a list of XiveTCTX and a
> list of ICPState instead of _hijacking_ the CPU list.
> </thinking aloud>

We removed the list of ICPState in 2.9 I think because it was 
a problem at the time. I don't remember exactly which but adding it 
back does not seem a good idea to me. 

 
> Then we have the hcalls in xics_spapr.c. Since we know this is
> pseries, we could rely on the existing void *machine_data, ie,
> move the cpu->icp to sPAPRCPUState and access it thanks
> to spapr_cpu_state().

yes. 

> And then, we have the xive_tm_ops in xive.c, which currently
> does:
> 
> static void xive_tm_write(void *opaque, hwaddr offset,
>                           uint64_t value, unsigned size)
> {
>     PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
>     XiveTCTX *tctx = cpu->tctx;
> 
> It could do something like:
> 
>     XiveTCTX *tctx = xive_router_get_tm_tctx(opaque, offset);
> 
> with a new XiveRouterClass ->get_tm_tctx() method:
> 
> XiveTCTX *xive_router_get_tm_tctx(XiveRouter *xrtr, hwaddr offset)
> {
>     XiveRouterClass *xrc = XIVE_ROUTER_GET_CLASS(xrtr);
> 
>     return xrc->get_tm_tctx(xrtr, offset);
> }

Yes. The Pnv method is nearly doing that today.
 
> On pseries, if we move cpu->tctx to sPAPRCPUState, then we just
> need to implement the ->get_tm_tctx() in spapr_xive.c as:
> 
> static XiveTCTX *spapr_xive_get_tm_tctx(XiveRouter *xrtr, hwaddr offset)
> {
>     return spapr_cpu_state(POWERPC_CPU(current_cpu))->tctx;
> }
> 
> Something similar should be done for pnv.

yes. 

It seems like it would be a good cleanup before introducing more
changes for KVM and Pnv. The CPU_FOREACH() is the only problem 
I see. I will take a look at it.

Thanks,

C.

Patch

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 9e01a5a..a947a0a 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -8,15 +8,16 @@ 
 #include "hw/mem/pc-dimm.h"
 #include "hw/ppc/spapr_ovec.h"
 #include "hw/ppc/spapr_irq.h"
+#include "hw/ppc/spapr_xive.h"  /* For sPAPRXive */
+#include "hw/ppc/xics.h"        /* For ICSState */
 
 struct VIOsPAPRBus;
 struct sPAPRPHBState;
 struct sPAPRNVRAM;
+
 typedef struct sPAPREventLogEntry sPAPREventLogEntry;
 typedef struct sPAPREventSource sPAPREventSource;
 typedef struct sPAPRPendingHPT sPAPRPendingHPT;
-typedef struct ICSState ICSState;
-typedef struct sPAPRXive sPAPRXive;
 
 #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
 #define SPAPR_ENTRY_POINT       0x100
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index 7fdc250..9bec919 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -41,8 +41,6 @@  bool spapr_xive_irq_claim(sPAPRXive *xive, uint32_t lisn, bool lsi);
 bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
 void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
 
-typedef struct sPAPRMachineState sPAPRMachineState;
-
 void spapr_xive_hcall_init(sPAPRMachineState *spapr);
 void spapr_dt_xive(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
                    uint32_t phandle);
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 486abaf..bde4dff 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -23,6 +23,11 @@ 
 #include "qemu-common.h"
 #include "qemu/int128.h"
 
+#ifndef CONFIG_USER_ONLY
+#include "hw/ppc/xive.h" /* for XiveTCTX */
+#include "hw/ppc/xics.h" /* for ICPState */
+#endif
+
 //#define PPC_EMULATE_32BITS_HYPV
 
 #if defined (TARGET_PPC64)
@@ -1177,8 +1182,6 @@  do {                                            \
 
 typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
 typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
-typedef struct XiveTCTX XiveTCTX;
-typedef struct ICPState ICPState;
 
 /**
  * PowerPCCPU:
@@ -1197,11 +1200,13 @@  struct PowerPCCPU {
     int vcpu_id;
     uint32_t compat_pvr;
     PPCVirtualHypervisor *vhyp;
-    ICPState *icp;
-    XiveTCTX *tctx;
     void *machine_data;
     int32_t node_id; /* NUMA node this CPU belongs to */
     PPCHash64Options *hash64_opts;
+#ifndef CONFIG_USER_ONLY
+    ICPState *icp;
+    XiveTCTX *tctx;
+#endif
 
     /* Fields related to migration compatibility hacks */
     bool pre_2_8_migration;