Re: [Qemu-devel] [PATCH v3 4/8] xics: minor changes and cleanups

[Alexander Graf]

On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote:

> Before XICS-KVM comes, it makes sense to clean up the emulated XICS a bit.
> 
> This does:
> 1. add assert in ics_realize()
> 2. change variable names from "k" to more informative ones
> 3. add "const" to every TypeInfo
> 4. replace fprintf(stderr, ..."\n") with error_report
> 5. replace old style qdev_init_nofail() with new style
> object_property_set_bool().
> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---
> Changes:
> v3:
> * ics_realize() fixed to be actual realize callback rather than initfn
> * asserts replaced with Error**
> ---
> hw/intc/xics.c | 42 ++++++++++++++++++++++++++++++------------
> 1 file changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index c80fa80..4d08c58 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -29,6 +29,7 @@
> #include "trace.h"
> #include "hw/ppc/spapr.h"
> #include "hw/ppc/xics.h"
> +#include "qemu/error-report.h"
> 
> /*
>  * ICP: Presentation layer
> @@ -211,7 +212,7 @@ static void icp_class_init(ObjectClass *klass, void *data)
>     dc->vmsd = &vmstate_icp_server;
> }
> 
> -static TypeInfo icp_info = {
> +static const TypeInfo icp_info = {
>     .name = TYPE_ICP,
>     .parent = TYPE_DEVICE,
>     .instance_size = sizeof(ICPState),
> @@ -442,15 +443,17 @@ static const VMStateDescription vmstate_ics = {
>     },
> };
> 
> -static int ics_realize(DeviceState *dev)
> +static void ics_realize(DeviceState *dev, Error **errp)
> {
>     ICSState *ics = ICS(dev);
> 
> +    if (!ics->nr_irqs) {
> +        error_setg(errp, "Number of interrupts needs to be greater 0");
> +        return;
> +    }
>     ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>     ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
>     ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
> -
> -    return 0;
> }
> 
> static void ics_class_init(ObjectClass *klass, void *data)
> @@ -458,13 +461,14 @@ static void ics_class_init(ObjectClass *klass, void 
> *data)
>     DeviceClass *dc = DEVICE_CLASS(klass);
>     ICSStateClass *isc = ICS_CLASS(klass);
> 
> -    dc->init = ics_realize;
> +    dc->realize = ics_realize;
>     dc->vmsd = &vmstate_ics;
>     dc->reset = ics_reset;
>     isc->post_load = ics_post_load;
> +    isc->post_load = ics_post_load;

Same line twice?

> }
> 
> -static TypeInfo ics_info = {
> +static const TypeInfo ics_info = {
>     .name = TYPE_ICS,
>     .parent = TYPE_DEVICE,
>     .instance_size = sizeof(ICSState),
> @@ -680,8 +684,8 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU 
> *cpu)
>         break;
> 
>     default:
> -        fprintf(stderr, "XICS interrupt controller does not support this CPU 
> "
> -                "bus model\n");
> +        error_report("XICS interrupt controller does not support this CPU "
> +                "bus model");
>         abort();
>     }
> }
> @@ -690,8 +694,14 @@ static void xics_realize(DeviceState *dev, Error **errp)
> {
>     XICSState *icp = XICS(dev);
>     ICSState *ics = icp->ics;
> +    Error *error = NULL;
>     int i;
> 
> +    if (!icp->nr_servers) {
> +        error_setg(errp, "Number of servers needs to be greater 0");
> +        return;
> +    }
> +
>     /* Registration of global state belongs into realize */
>     spapr_rtas_register("ibm,set-xive", rtas_set_xive);
>     spapr_rtas_register("ibm,get-xive", rtas_get_xive);
> @@ -706,7 +716,11 @@ static void xics_realize(DeviceState *dev, Error **errp)
>     ics->nr_irqs = icp->nr_irqs;
>     ics->offset = XICS_IRQ_BASE;
>     ics->icp = icp;
> -    qdev_init_nofail(DEVICE(ics));
> +    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);

Are you sure this is correct? So you are trying to force set the ics to 
realize? Is this the right way to do this? Andreas?


Alex