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

[Alexander Graf]

On 26.08.2013, at 16:20, Andreas Färber wrote:

> Am 26.08.2013 15:36, schrieb 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?
> 
> Yes, it's correct, I requested it. This assures that errors are
> forwarded up the chain to whomever sets realized = true. That's a
> general pattern that some devices have been ignoring. Obviously for KVM
> 
> Another request apart from checking the duplicate line I have, is to
> please split up this big patch. Everything except const should be in its
> own patch with short description for better review and independent
> ack'ing, so that we can move forward here. An explicit list of changes
> within one patch should be a yellow warning sign. ;)
> (I would prefer the patches to go through ppc-next with my Rb, but if
> you prefer nonfunctional QOM changes to go through qom-next I'm sure we
> could make that work as well.)

I'm more than happy to take them through ppc-next :).


Alex