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

[Cédric Le Goater]

On 1/16/19 4:11 PM, Greg Kurz wrote:
> On Wed, 16 Jan 2019 14:44:55 +0100
> Thomas Huth <address@hidden> wrote:
> 
>> On 2019-01-16 14:23, Greg Kurz wrote:
>>> On Wed, 16 Jan 2019 12:47:36 +0100
>>> Thomas Huth <address@hidden> 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.