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 <a...@ozlabs.ru>
>>> ---
>>> 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
