On 2019-01-09 18:47, Greg Kurz wrote: > On Wed, 9 Jan 2019 18:40:48 +0100 > Cédric Le Goater <c...@kaod.org> wrote: > >> On 1/9/19 6:28 PM, Daniel P. Berrangé wrote: >>> On Wed, Jan 09, 2019 at 06:13:01PM +0100, Cédric Le Goater wrote: >>>> On 1/9/19 5:39 PM, Thomas Huth wrote: >>>>> When compiling with Clang and -std=gnu99, I get the following errors: >>>>> >>>>> CC ppc64-softmmu/hw/intc/xics_spapr.o >>>>> In file included from hw/intc/xics_spapr.c:34: >>>>> include/hw/ppc/xics.h:46:25: error: redefinition of typedef 'ICSState' is >>>>> a C11 feature >>>>> [-Werror,-Wtypedef-redefinition] >>>>> typedef struct ICSState ICSState; >>>>> ^ >>>>> include/hw/ppc/spapr.h:18:25: note: previous definition is here >>>>> typedef struct ICSState ICSState; >>>>> ^ >>>>> In file included from hw/intc/xics_spapr.c:34: >>>>> include/hw/ppc/xics.h:203:34: error: redefinition of typedef >>>>> 'sPAPRMachineState' is a C11 feature >>>>> [-Werror,-Wtypedef-redefinition] >>>>> typedef struct sPAPRMachineState sPAPRMachineState; >>>>> ^ >>>>> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here >>>>> typedef struct sPAPRMachineState sPAPRMachineState; >>>>> ^ >>>>> CC ppc64-softmmu/hw/intc/spapr_xive.o >>>>> In file included from hw/intc/spapr_xive.c:19: >>>>> include/hw/ppc/spapr_xive.h:38:3: error: redefinition of typedef >>>>> 'sPAPRXive' is a C11 feature >>>>> [-Werror,-Wtypedef-redefinition] >>>>> } sPAPRXive; >>>>> ^ >>>>> include/hw/ppc/spapr.h:20:26: note: previous definition is here >>>>> typedef struct sPAPRXive sPAPRXive; >>>>> ^ >>>>> In file included from hw/intc/spapr_xive.c:19: >>>>> include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef >>>>> 'sPAPRMachineState' is a C11 feature >>>>> [-Werror,-Wtypedef-redefinition] >>>>> typedef struct sPAPRMachineState sPAPRMachineState; >>>>> ^ >>>>> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here >>>>> typedef struct sPAPRMachineState sPAPRMachineState; >>>>> ^ >>>>> CC ppc64-softmmu/hw/char/spapr_vty.o >>>>> In file included from hw/char/spapr_vty.c:8: >>>>> In file included from include/hw/ppc/spapr.h:12: >>>>> include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef >>>>> 'sPAPRMachineState' is a C11 feature >>>>> [-Werror,-Wtypedef-redefinition] >>>>> typedef struct sPAPRMachineState sPAPRMachineState; >>>>> ^ >>>>> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here >>>>> typedef struct sPAPRMachineState sPAPRMachineState; >>>>> ^ >>>>> >>>>> Since we're going to make -std=gnu99 mandatory, fix these issues >>>>> by including the right header files indead of typedeffing stuff twice. >>>>> >>>>> Signed-off-by: Thomas Huth <th...@redhat.com> >>>>> --- >>>>> include/hw/ppc/spapr.h | 4 ++-- >>>>> include/hw/ppc/spapr_xive.h | 2 +- >>>>> include/hw/ppc/xics.h | 4 ++-- >>>>> 3 files changed, 5 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >>>>> index 2c77a8b..6a5ae4f 100644 >>>>> --- a/include/hw/ppc/spapr.h >>>>> +++ b/include/hw/ppc/spapr.h >>>>> @@ -8,6 +8,8 @@ >>>>> #include "hw/mem/pc-dimm.h" >>>>> #include "hw/ppc/spapr_ovec.h" >>>>> #include "hw/ppc/spapr_irq.h" >>>>> +#include "hw/ppc/xics.h" >>>>> +#include "hw/ppc/spapr_xive.h" >>>>> >>>>> struct VIOsPAPRBus; >>>>> struct sPAPRPHBState; >>>>> @@ -15,8 +17,6 @@ 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 728735d..aff4366 100644 >>>>> --- a/include/hw/ppc/spapr_xive.h >>>>> +++ b/include/hw/ppc/spapr_xive.h >>>>> @@ -42,7 +42,7 @@ bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t >>>>> lisn); >>>>> void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon); >>>>> qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn); >>>>> >>>>> -typedef struct sPAPRMachineState sPAPRMachineState; >>>>> +#include "hw/ppc/spapr.h" /* for sPAPRMachineState */ >>>> >>>> so both files include each other, how nice ... >>> >>> If the header files are mutually dependent it makes me wonder what the >>> point of having them split up is ? >> >> The XICS interrupt controller models are used in two different machines, >> sPAPR and PowerNV. >> >>> Feels like either they need to be merged, or they need to be split up >>> and refactored even more to remove the mutual dependancy. >> >> yes or that some spapr_* definitions do not belong to the common xics.h >> file. > > Yeah, > > These should move to a new xics_spapr.h or maybe even spapr.h for simplicity: > > void spapr_dt_xics(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt, > uint32_t phandle); > int xics_kvm_init(sPAPRMachineState *spapr, Error **errp); > void xics_spapr_init(sPAPRMachineState *spapr);
Ok, I'll move them to xics_spapr.h. Thomas