On Wed, 16 Jan 2019 12:47:36 +0100
Thomas Huth <th...@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
