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

[Andreas Färber]

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

Andreas

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