Re: [Qemu-devel] [PATCH v5 2/3] ppc: Fix duplicated typedefs to be able to compile with Clang in gnu99 mode

[Greg Kurz <groug@kaod.org>]

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