[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH v2 01/22] ppc/xics: remove set_nr_irqs() handler f
|
From: |
David Gibson |
|
Subject: |
Re: [Qemu-ppc] [PATCH v2 01/22] ppc/xics: remove set_nr_irqs() handler from XICSStateClass |
|
Date: |
Wed, 22 Feb 2017 14:21:23 +1100 |
|
User-agent: |
Mutt/1.7.1 (2016-10-04) |
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 <address@hidden>
> ---
>
> 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?
> 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;
> };
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature