Re: [Qemu-ppc] [PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

[Cédric Le Goater]

On 06/08/2017 07:00 PM, Greg Kurz wrote:
> On Thu, 8 Jun 2017 18:08:44 +0200
> Cédric Le Goater <address@hidden> wrote:
> 
>>>>> FWIW, other people do that as well (see hw/i386/pc_q35.c for example).    
>>>>
>>>> well, I don't see the benefits of changing a string constant by a 
>>>> define. 
>>>>  
>>>
>>> Improved semantics,  especially since the "xics" string appears in 
>>> many places with different meanings.   
>>
>> ah ? If so, we should do a cleanup up. The code seems consistent from 
>> what I can see. xics is a general name for :
>>
>>      'PowerPC interrupt controller (type 2)' 
>>
>> and it is mostly used as a prefix. There are no "xics" object, only a 
> 
> I'm only talking about "xics" as a property name actually:
> 
> $ git grep '"xics"'
> hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> hw/ppc/pnv.c:    object_property_add_const_link(OBJECT(&chip->psi), "xics",
> hw/ppc/pnv.c:        object_property_add_const_link(OBJECT(pnv_core), "xics",
> hw/ppc/pnv_core.c:    object_property_add_const_link(obj, "xics", OBJECT(xi), 
> &error_abort);
> hw/ppc/pnv_core.c:    xi = object_property_get_link(OBJECT(dev), "xics", 
> &local_err);
> hw/ppc/pnv_psi.c:    obj = object_property_get_link(OBJECT(dev), "xics", 
> &err);
> hw/ppc/pnv_psi.c:    object_property_add_const_link(OBJECT(ics), "xics", obj, 
>  &error_abort);
> hw/ppc/spapr.c:    object_property_add_const_link(obj, "xics", OBJECT(spapr), 
> &error_abort);
> hw/ppc/spapr_cpu_core.c:    object_property_add_const_link(obj, "xics", 
> OBJECT(spapr), &error_abort);
> 
> You have to read the code to know which ones are related.

The "xics" property link always point to the same object : 
the XICSFabric object which is the machine, spapr or pnv. 

> With this patch applied, it is mostly obvious, even for the newbie:

ah. the goal is to know where in the code the link was set. 
It can be even more complex with aliases.

Cheers,

C.

> 
> $ git grep -E 'IC._PROP_XICS|"xics"'
> hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), ICP_PROP_XICS, 
> &err);
> hw/intc/xics.c:        error_setg(errp, "%s: required link '" ICP_PROP_XICS 
> "' not found: %s",
> hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, 
> &err);
> hw/intc/xics.c:        error_setg(errp, "%s: required link '" ICS_PROP_XICS 
> "' not found: %s",
> 
> hw/ppc/pnv.c:    object_property_add_const_link(OBJECT(&chip->psi), "xics",
> hw/ppc/pnv.c:        object_property_add_const_link(OBJECT(pnv_core), "xics",
> 
> these two ^^ are related to...
> 
> hw/ppc/pnv_core.c:    object_property_add_const_link(obj, ICP_PROP_XICS, 
> OBJECT(xi),
> 
> hw/ppc/pnv_core.c:    xi = object_property_get_link(OBJECT(dev), "xics", 
> &local_err);
> hw/ppc/pnv_psi.c:    obj = object_property_get_link(OBJECT(dev), "xics", 
> &err);
> 
> these two ^^
> 
> hw/ppc/pnv_psi.c:    object_property_add_const_link(OBJECT(ics), 
> ICS_PROP_XICS, obj,
> hw/ppc/spapr.c:    object_property_add_const_link(obj, ICS_PROP_XICS, 
> OBJECT(spapr),
> hw/ppc/spapr_cpu_core.c:    object_property_add_const_link(obj, 
> ICP_PROP_XICS, OBJECT(spapr),
> include/hw/ppc/xics.h:#define ICP_PROP_XICS "xics"
> include/hw/ppc/xics.h:#define ICS_PROP_XICS "xics"
> 
>> XICSFabric which is a QOM interface of the machine with a bunch of 
>> handlers. It worked out pretty well for sPAPR and PowerNV.
>>
>> It won't be the case for XIVE, the interrupt controller (type 3) for 
>> the P9. It is more complex and there are quite a lot of internal 
>> states which will need to be gathered under an object. We can keep 
>> the ICPState fortunately but the sources are quite different.
>>
>> C.
>>
>