On 02/22/2017 04:21 AM, David Gibson wrote:
> On Thu, Feb 16, 2017 at 02:47:24PM +0100, Cédric Le Goater wrote:
>> Today, the ICS (Interrupt Controller Source) object is created and
>> realized by the init and realize routines of the XICS object, but some
>> of the parameters are only known at the machine level.
>>
>> These parameters are passed from the sPAPR machine to the ICS object
>> in a rather convoluted way using property handlers and a class handler
>> of the XICS object. The number of irqs required to allocate the IRQ
>> state objects in the ICS realize routine is one of them.
>>
>> Let's simplify the process by creating the ICS object along with the
>> XICS object at the machine level and link the ICS into the XICS list
>> of ICSs at this level also. In the sPAPR machine, there is only a
>> single ICS but that will change with the PowerNV machine.
>>
>> Also, QOMify the creation of the objects and get rid of the
>> superfluous code.
>>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> ---
>>
>> Changes since v1:
>>
>> - added a XICS link to the ICS object
>>
>> hw/intc/xics.c | 51 ++++++++++++++++---------------------------------
>> hw/intc/xics_kvm.c | 44 ++++++++++--------------------------------
>> hw/intc/xics_spapr.c | 34 ---------------------------------
>> hw/ppc/spapr.c | 53
>> +++++++++++++++++++++++++++++++++++----------------
>> include/hw/ppc/xics.h | 2 --
>> 5 files changed, 63 insertions(+), 121 deletions(-)
>>
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index 372b8311fb8b..e70d3b8b1095 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -151,38 +151,6 @@ static void xics_common_reset(DeviceState *d)
>> }
>> }
>>
>> -static void xics_prop_get_nr_irqs(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - XICSState *xics = XICS_COMMON(obj);
>> - int64_t value = xics->nr_irqs;
>> -
>> - visit_type_int(v, name, &value, errp);
>> -}
>> -
>> -static void xics_prop_set_nr_irqs(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - XICSState *xics = XICS_COMMON(obj);
>> - XICSStateClass *info = XICS_COMMON_GET_CLASS(xics);
>> - Error *error = NULL;
>> - int64_t value;
>> -
>> - visit_type_int(v, name, &value, &error);
>> - if (error) {
>> - error_propagate(errp, error);
>> - return;
>> - }
>> - if (xics->nr_irqs) {
>> - error_setg(errp, "Number of interrupts is already set to %u",
>> - xics->nr_irqs);
>> - return;
>> - }
>> -
>> - assert(info->set_nr_irqs);
>> - info->set_nr_irqs(xics, value, errp);
>> -}
>> -
>> void xics_set_nr_servers(XICSState *xics, uint32_t nr_servers,
>> const char *typename, Error **errp)
>> {
>> @@ -241,9 +209,6 @@ static void xics_common_initfn(Object *obj)
>> XICSState *xics = XICS_COMMON(obj);
>>
>> QLIST_INIT(&xics->ics);
>> - object_property_add(obj, "nr_irqs", "int",
>> - xics_prop_get_nr_irqs, xics_prop_set_nr_irqs,
>> - NULL, NULL, NULL);
>> object_property_add(obj, "nr_servers", "int",
>> xics_prop_get_nr_servers, xics_prop_set_nr_servers,
>> NULL, NULL, NULL);
>> @@ -737,6 +702,16 @@ static void ics_simple_initfn(Object *obj)
>> static void ics_simple_realize(DeviceState *dev, Error **errp)
>> {
>> ICSState *ics = ICS_SIMPLE(dev);
>> + Object *obj;
>> + Error *err = NULL;
>> +
>> + obj = object_property_get_link(OBJECT(dev), "xics", &err);
>> + if (!obj) {
>> + error_setg(errp, "%s: required link 'xics' not found: %s",
>> + __func__, error_get_pretty(err));
>> + return;
>> + }
>> + ics->xics = XICS_COMMON(obj);
>>
>> if (!ics->nr_irqs) {
>> error_setg(errp, "Number of interrupts needs to be greater 0");
>> @@ -746,12 +721,18 @@ static void ics_simple_realize(DeviceState *dev, Error
>> **errp)
>> ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
>> }
>>
>> +static Property ics_simple_properties[] = {
>> + DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0),
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> static void ics_simple_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> ICSStateClass *isc = ICS_BASE_CLASS(klass);
>>
>> dc->realize = ics_simple_realize;
>> + dc->props = ics_simple_properties;
>> dc->vmsd = &vmstate_ics_simple;
>> dc->reset = ics_simple_reset;
>> isc->post_load = ics_simple_post_load;
>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>> index 17694eaa8709..4a6c0522feb6 100644
>> --- a/hw/intc/xics_kvm.c
>> +++ b/hw/intc/xics_kvm.c
>> @@ -294,6 +294,16 @@ static void ics_kvm_reset(DeviceState *dev)
>> static void ics_kvm_realize(DeviceState *dev, Error **errp)
>> {
>> ICSState *ics = ICS_SIMPLE(dev);
>> + Object *obj;
>> + Error *err = NULL;
>> +
>> + obj = object_property_get_link(OBJECT(dev), "xics", &err);
>> + if (!obj) {
>> + error_setg(errp, "%s: required link 'xics' not found: %s",
>> + __func__, error_get_pretty(err));
>> + return;
>> + }
>> + ics->xics = XICS_COMMON(obj);
>
> Couldn't you have this logic in the base class rather than repeating
> it in ics simple and ics kvm?
Yes we could. But, as I am getting rid of the backlinks at the end of
the patchset, the redundancy is not much of a problem.
Thanks,
C.
>> if (!ics->nr_irqs) {
>> error_setg(errp, "Number of interrupts needs to be greater 0");
>> @@ -358,18 +368,6 @@ static void xics_kvm_cpu_setup(XICSState *xics,
>> PowerPCCPU *cpu)
>> ss->cap_irq_xics_enabled = true;
>> }
>>
>> -static void xics_kvm_set_nr_irqs(XICSState *xics, uint32_t nr_irqs,
>> - Error **errp)
>> -{
>> - ICSState *ics = QLIST_FIRST(&xics->ics);
>> -
>> - /* This needs to be deprecated ... */
>> - xics->nr_irqs = nr_irqs;
>> - if (ics) {
>> - ics->nr_irqs = nr_irqs;
>> - }
>> -}
>> -
>> static void xics_kvm_set_nr_servers(XICSState *xics, uint32_t nr_servers,
>> Error **errp)
>> {
>> @@ -389,7 +387,6 @@ static void xics_kvm_realize(DeviceState *dev, Error
>> **errp)
>> {
>> KVMXICSState *xicskvm = XICS_SPAPR_KVM(dev);
>> XICSState *xics = XICS_COMMON(dev);
>> - ICSState *ics;
>> int i, rc;
>> Error *error = NULL;
>> struct kvm_create_device xics_create_device = {
>> @@ -441,14 +438,6 @@ static void xics_kvm_realize(DeviceState *dev, Error
>> **errp)
>>
>> xicskvm->kernel_xics_fd = xics_create_device.fd;
>>
>> - QLIST_FOREACH(ics, &xics->ics, list) {
>> - object_property_set_bool(OBJECT(ics), true, "realized", &error);
>> - if (error) {
>> - error_propagate(errp, error);
>> - goto fail;
>> - }
>> - }
>> -
>> assert(xics->nr_servers);
>> for (i = 0; i < xics->nr_servers; i++) {
>> object_property_set_bool(OBJECT(&xics->ss[i]), true, "realized",
>> @@ -472,17 +461,6 @@ fail:
>> kvmppc_define_rtas_kernel_token(0, "ibm,int-off");
>> }
>>
>> -static void xics_kvm_initfn(Object *obj)
>> -{
>> - XICSState *xics = XICS_COMMON(obj);
>> - ICSState *ics;
>> -
>> - ics = ICS_SIMPLE(object_new(TYPE_ICS_KVM));
>> - object_property_add_child(obj, "ics", OBJECT(ics), NULL);
>> - ics->xics = xics;
>> - QLIST_INSERT_HEAD(&xics->ics, ics, list);
>> -}
>> -
>> static void xics_kvm_class_init(ObjectClass *oc, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(oc);
>> @@ -490,7 +468,6 @@ static void xics_kvm_class_init(ObjectClass *oc, void
>> *data)
>>
>> dc->realize = xics_kvm_realize;
>> xsc->cpu_setup = xics_kvm_cpu_setup;
>> - xsc->set_nr_irqs = xics_kvm_set_nr_irqs;
>> xsc->set_nr_servers = xics_kvm_set_nr_servers;
>> }
>>
>> @@ -499,7 +476,6 @@ static const TypeInfo xics_spapr_kvm_info = {
>> .parent = TYPE_XICS_COMMON,
>> .instance_size = sizeof(KVMXICSState),
>> .class_init = xics_kvm_class_init,
>> - .instance_init = xics_kvm_initfn,
>> };
>>
>> static void xics_kvm_register_types(void)
>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>> index 2e3f1c5e95b2..03e42a866603 100644
>> --- a/hw/intc/xics_spapr.c
>> +++ b/hw/intc/xics_spapr.c
>> @@ -239,18 +239,6 @@ static void rtas_int_on(PowerPCCPU *cpu,
>> sPAPRMachineState *spapr,
>> rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> }
>>
>> -static void xics_spapr_set_nr_irqs(XICSState *xics, uint32_t nr_irqs,
>> - Error **errp)
>> -{
>> - ICSState *ics = QLIST_FIRST(&xics->ics);
>> -
>> - /* This needs to be deprecated ... */
>> - xics->nr_irqs = nr_irqs;
>> - if (ics) {
>> - ics->nr_irqs = nr_irqs;
>> - }
>> -}
>> -
>> static void xics_spapr_set_nr_servers(XICSState *xics, uint32_t nr_servers,
>> Error **errp)
>> {
>> @@ -260,7 +248,6 @@ static void xics_spapr_set_nr_servers(XICSState *xics,
>> uint32_t nr_servers,
>> static void xics_spapr_realize(DeviceState *dev, Error **errp)
>> {
>> XICSState *xics = XICS_SPAPR(dev);
>> - ICSState *ics;
>> Error *error = NULL;
>> int i;
>>
>> @@ -282,14 +269,6 @@ static void xics_spapr_realize(DeviceState *dev, Error
>> **errp)
>> spapr_register_hypercall(H_EOI, h_eoi);
>> spapr_register_hypercall(H_IPOLL, h_ipoll);
>>
>> - QLIST_FOREACH(ics, &xics->ics, list) {
>> - object_property_set_bool(OBJECT(ics), true, "realized", &error);
>> - if (error) {
>> - error_propagate(errp, error);
>> - return;
>> - }
>> - }
>> -
>> for (i = 0; i < xics->nr_servers; i++) {
>> object_property_set_bool(OBJECT(&xics->ss[i]), true, "realized",
>> &error);
>> @@ -300,24 +279,12 @@ static void xics_spapr_realize(DeviceState *dev, Error
>> **errp)
>> }
>> }
>>
>> -static void xics_spapr_initfn(Object *obj)
>> -{
>> - XICSState *xics = XICS_SPAPR(obj);
>> - ICSState *ics;
>> -
>> - ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE));
>> - object_property_add_child(obj, "ics", OBJECT(ics), NULL);
>> - ics->xics = xics;
>> - QLIST_INSERT_HEAD(&xics->ics, ics, list);
>> -}
>> -
>> static void xics_spapr_class_init(ObjectClass *oc, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(oc);
>> XICSStateClass *xsc = XICS_SPAPR_CLASS(oc);
>>
>> dc->realize = xics_spapr_realize;
>> - xsc->set_nr_irqs = xics_spapr_set_nr_irqs;
>> xsc->set_nr_servers = xics_spapr_set_nr_servers;
>> }
>>
>> @@ -327,7 +294,6 @@ static const TypeInfo xics_spapr_info = {
>> .instance_size = sizeof(XICSState),
>> .class_size = sizeof(XICSStateClass),
>> .class_init = xics_spapr_class_init,
>> - .instance_init = xics_spapr_initfn,
>> };
>>
>> #define ICS_IRQ_FREE(ics, srcno) \
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 9b6ad0f2fd77..94b1e8e3227a 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -95,22 +95,42 @@
>>
>> #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift))
>>
>> -static XICSState *try_create_xics(const char *type, int nr_servers,
>> - int nr_irqs, Error **errp)
>> -{
>> - Error *err = NULL;
>> - DeviceState *dev;
>> +static XICSState *try_create_xics(const char *type, const char *type_ics,
>> + int nr_servers, int nr_irqs, Error **errp)
>> +{
>> + Error *err = NULL, *local_err = NULL;
>> + XICSState *xics;
>> + ICSState *ics = NULL;
>> +
>> + xics = XICS_COMMON(object_new(type));
>> + qdev_set_parent_bus(DEVICE(xics), sysbus_get_default());
>> + object_property_set_int(OBJECT(xics), nr_servers, "nr_servers", &err);
>> + object_property_set_bool(OBJECT(xics), true, "realized", &local_err);
>> + error_propagate(&err, local_err);
>> + if (err) {
>> + goto error;
>> + }
>>
>> - dev = DEVICE(object_new(type));
>> - qdev_prop_set_uint32(dev, "nr_servers", nr_servers);
>> - qdev_prop_set_uint32(dev, "nr_irqs", nr_irqs);
>> - object_property_set_bool(OBJECT(dev), true, "realized", &err);
>> + ics = ICS_SIMPLE(object_new(type_ics));
>> + object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
>> + object_property_set_int(OBJECT(ics), nr_irqs, "nr-irqs", &err);
>> + object_property_add_const_link(OBJECT(ics), "xics", OBJECT(xics), NULL);
>> + object_property_set_bool(OBJECT(ics), true, "realized", &local_err);
>> + error_propagate(&err, local_err);
>> if (err) {
>> - error_propagate(errp, err);
>> - object_unparent(OBJECT(dev));
>> - return NULL;
>> + goto error;
>> }
>> - return XICS_COMMON(dev);
>> + QLIST_INSERT_HEAD(&xics->ics, ics, list);
>> +
>> + return xics;
>> +
>> +error:
>> + error_propagate(errp, err);
>> + if (ics) {
>> + object_unparent(OBJECT(ics));
>> + }
>> + object_unparent(OBJECT(xics));
>> + return NULL;
>> }
>>
>> static XICSState *xics_system_init(MachineState *machine,
>> @@ -122,8 +142,8 @@ static XICSState *xics_system_init(MachineState *machine,
>> Error *err = NULL;
>>
>> if (machine_kernel_irqchip_allowed(machine)) {
>> - xics = try_create_xics(TYPE_XICS_SPAPR_KVM, nr_servers, nr_irqs,
>> - &err);
>> + xics = try_create_xics(TYPE_XICS_SPAPR_KVM, TYPE_ICS_KVM,
>> + nr_servers, nr_irqs, &err);
>> }
>> if (machine_kernel_irqchip_required(machine) && !xics) {
>> error_reportf_err(err,
>> @@ -134,7 +154,8 @@ static XICSState *xics_system_init(MachineState *machine,
>> }
>>
>> if (!xics) {
>> - xics = try_create_xics(TYPE_XICS_SPAPR, nr_servers, nr_irqs, errp);
>> + xics = try_create_xics(TYPE_XICS_SPAPR, TYPE_ICS_SIMPLE, nr_servers,
>> + nr_irqs, errp);
>> }
>>
>> return xics;
>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>> index 3f0c31610aa4..8fe3c4d2ceab 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -74,7 +74,6 @@ struct XICSStateClass {
>> DeviceClass parent_class;
>>
>> void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
>> - void (*set_nr_irqs)(XICSState *icp, uint32_t nr_irqs, Error **errp);
>> void (*set_nr_servers)(XICSState *icp, uint32_t nr_servers, Error
>> **errp);
>> };
>>
>> @@ -83,7 +82,6 @@ struct XICSState {
>> SysBusDevice parent_obj;
>> /*< public >*/
>> uint32_t nr_servers;
>> - uint32_t nr_irqs;
>> ICPState *ss;
>> QLIST_HEAD(, ICSState) ics;
>> };
>
