Re: [Qemu-devel] [PATCH 3/4] xics: rework initialization

[Andreas Färber]

Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
> Currently RTAS and hypercalls are registered in the XICS class init
> function. The upcoming XICS-KVM will inherit from XICS but will use
> another API to register RTAS tokens with KVM so registration has
> to move from the class init function (common for both XICS and
> XICS-KVM) to the _realize function (specific to the controller).

This sounds sensible, but the reason is definitely not that there is
only class_init when you have two classes, but that registration of
global state belongs into realize.

> This moves ICS creation to _realize as there is no point to create
> some child devices in initfn() (ICS) and some in realize() (ICPs).
> 
> As initfn is no more needed, this removes it.
> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---
>  hw/intc/xics.c | 35 +++++++++++++++--------------------
>  1 file changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 283c2dd..a359f52 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -689,9 +689,23 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU 
> *cpu)
>  static void xics_realize(DeviceState *dev, Error **errp)
>  {
>      XICSState *icp = XICS(dev);
> -    ICSState *ics = icp->ics;
> +    ICSState *ics;
>      int i;
>  
> +    spapr_rtas_register("ibm,set-xive", rtas_set_xive);
> +    spapr_rtas_register("ibm,get-xive", rtas_get_xive);
> +    spapr_rtas_register("ibm,int-off", rtas_int_off);
> +    spapr_rtas_register("ibm,int-on", rtas_int_on);
> +
> +    spapr_register_hypercall(H_CPPR, h_cppr);
> +    spapr_register_hypercall(H_IPI, h_ipi);
> +    spapr_register_hypercall(H_XIRR, h_xirr);
> +    spapr_register_hypercall(H_EOI, h_eoi);
> +
> +    icp->ics = ICS(object_new(TYPE_ICS));
> +    ics = icp->ics;
> +    object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
> +
>      ics->nr_irqs = icp->nr_irqs;
>      ics->offset = XICS_IRQ_BASE;
>      ics->icp = icp;
> @@ -707,14 +721,6 @@ static void xics_realize(DeviceState *dev, Error **errp)
>      }
>  }
>  
> -static void xics_initfn(Object *obj)
> -{
> -    XICSState *xics = XICS(obj);
> -
> -    xics->ics = ICS(object_new(TYPE_ICS));
> -    object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
> -}
> -
>  static Property xics_properties[] = {
>      DEFINE_PROP_UINT32("nr_servers", XICSState, nr_servers, -1),
>      DEFINE_PROP_UINT32("nr_irqs", XICSState, nr_irqs, -1),

This looks wrong, both before and after. It should be both created using
object_initialize() and added as child property in instance_init. If
TYPE_ICS is a device, then in realize it should get realized.

Regards,
Andreas

> @@ -730,16 +736,6 @@ static void xics_class_init(ObjectClass *oc, void *data)
>      dc->props = xics_properties;
>      dc->reset = xics_reset;
>      k->cpu_setup = xics_cpu_setup;
> -
> -    spapr_rtas_register("ibm,set-xive", rtas_set_xive);
> -    spapr_rtas_register("ibm,get-xive", rtas_get_xive);
> -    spapr_rtas_register("ibm,int-off", rtas_int_off);
> -    spapr_rtas_register("ibm,int-on", rtas_int_on);
> -
> -    spapr_register_hypercall(H_CPPR, h_cppr);
> -    spapr_register_hypercall(H_IPI, h_ipi);
> -    spapr_register_hypercall(H_XIRR, h_xirr);
> -    spapr_register_hypercall(H_EOI, h_eoi);
>  }
>  
>  static const TypeInfo xics_info = {
> @@ -748,7 +744,6 @@ static const TypeInfo xics_info = {
>      .instance_size = sizeof(XICSState),
>      .class_size = sizeof(XICSStateClass),
>      .class_init    = xics_class_init,
> -    .instance_init = xics_initfn,
>  };
>  
>  static void xics_register_types(void)

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg