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

[Greg Kurz]

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.

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

$ 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.
>

pgpz9BNedNIgz.pgp
Description: OpenPGP digital signature