| Message ID | 20190924045952.11412-5-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xics: Eliminate unnecessary class | expand |
On 24/09/2019 06:59, David Gibson wrote: > TYPE_ICS_SIMPLE is the only subtype of TYPE_ICS_BASE that's ever > instantiated, and the only one we're ever likely to want. The > existence of different classes is just a hang over from when we > (misguidedly) had separate subtypes for the KVM and non-KVM version of > the device. > > So, collapse the two classes together into just TYPE_ICS. Well, I have been maintaining another subclass for the PHB3 MSI but it has never been merged and it will require some rework. Anyhow the base ICS code is cleaner with that patch and it does not seem to break migration. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Reviewed-by: Cédric Le Goater <clg@kaod.org> C. > --- > hw/intc/xics.c | 57 ++++++++++++++++--------------------------- > hw/ppc/pnv_psi.c | 2 +- > hw/ppc/spapr_irq.c | 4 +-- > include/hw/ppc/xics.h | 17 ++----------- > 4 files changed, 26 insertions(+), 54 deletions(-) > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 9ae51bbc76..388dbba870 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -555,7 +555,7 @@ static void ics_reset_irq(ICSIRQState *irq) > > static void ics_reset(DeviceState *dev) > { > - ICSState *ics = ICS_BASE(dev); > + ICSState *ics = ICS(dev); > int i; > uint8_t flags[ics->nr_irqs]; > > @@ -573,7 +573,7 @@ static void ics_reset(DeviceState *dev) > if (kvm_irqchip_in_kernel()) { > Error *local_err = NULL; > > - ics_set_kvm_state(ICS_BASE(dev), &local_err); > + ics_set_kvm_state(ICS(dev), &local_err); > if (local_err) { > error_report_err(local_err); > } > @@ -587,7 +587,7 @@ static void ics_reset_handler(void *dev) > > static void ics_realize(DeviceState *dev, Error **errp) > { > - ICSState *ics = ICS_BASE(dev); > + ICSState *ics = ICS(dev); > Error *local_err = NULL; > Object *obj; > > @@ -609,26 +609,14 @@ static void ics_realize(DeviceState *dev, Error **errp) > qemu_register_reset(ics_reset_handler, ics); > } > > -static void ics_simple_class_init(ObjectClass *klass, void *data) > +static void ics_instance_init(Object *obj) > { > -} > - > -static const TypeInfo ics_simple_info = { > - .name = TYPE_ICS_SIMPLE, > - .parent = TYPE_ICS_BASE, > - .instance_size = sizeof(ICSState), > - .class_init = ics_simple_class_init, > - .class_size = sizeof(ICSStateClass), > -}; > - > -static void ics_base_instance_init(Object *obj) > -{ > - ICSState *ics = ICS_BASE(obj); > + ICSState *ics = ICS(obj); > > ics->offset = XICS_IRQ_BASE; > } > > -static int ics_base_pre_save(void *opaque) > +static int ics_pre_save(void *opaque) > { > ICSState *ics = opaque; > > @@ -639,7 +627,7 @@ static int ics_base_pre_save(void *opaque) > return 0; > } > > -static int ics_base_post_load(void *opaque, int version_id) > +static int ics_post_load(void *opaque, int version_id) > { > ICSState *ics = opaque; > > @@ -657,7 +645,7 @@ static int ics_base_post_load(void *opaque, int version_id) > return 0; > } > > -static const VMStateDescription vmstate_ics_base_irq = { > +static const VMStateDescription vmstate_ics_irq = { > .name = "ics/irq", > .version_id = 2, > .minimum_version_id = 1, > @@ -671,46 +659,44 @@ static const VMStateDescription vmstate_ics_base_irq = { > }, > }; > > -static const VMStateDescription vmstate_ics_base = { > +static const VMStateDescription vmstate_ics = { > .name = "ics", > .version_id = 1, > .minimum_version_id = 1, > - .pre_save = ics_base_pre_save, > - .post_load = ics_base_post_load, > + .pre_save = ics_pre_save, > + .post_load = ics_post_load, > .fields = (VMStateField[]) { > /* Sanity check */ > VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL), > > VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs, > - vmstate_ics_base_irq, > + vmstate_ics_irq, > ICSIRQState), > VMSTATE_END_OF_LIST() > }, > }; > > -static Property ics_base_properties[] = { > +static Property ics_properties[] = { > DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), > DEFINE_PROP_END_OF_LIST(), > }; > > -static void ics_base_class_init(ObjectClass *klass, void *data) > +static void ics_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > > dc->realize = ics_realize; > - dc->props = ics_base_properties; > + dc->props = ics_properties; > dc->reset = ics_reset; > - dc->vmsd = &vmstate_ics_base; > + dc->vmsd = &vmstate_ics; > } > > -static const TypeInfo ics_base_info = { > - .name = TYPE_ICS_BASE, > +static const TypeInfo ics_info = { > + .name = TYPE_ICS, > .parent = TYPE_DEVICE, > - .abstract = true, > .instance_size = sizeof(ICSState), > - .instance_init = ics_base_instance_init, > - .class_init = ics_base_class_init, > - .class_size = sizeof(ICSStateClass), > + .instance_init = ics_instance_init, > + .class_init = ics_class_init, > }; > > static const TypeInfo xics_fabric_info = { > @@ -749,8 +735,7 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > > static void xics_register_types(void) > { > - type_register_static(&ics_simple_info); > - type_register_static(&ics_base_info); > + type_register_static(&ics_info); > type_register_static(&icp_info); > type_register_static(&xics_fabric_info); > } > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c > index 8ea81e9d8e..a997f16bb4 100644 > --- a/hw/ppc/pnv_psi.c > +++ b/hw/ppc/pnv_psi.c > @@ -469,7 +469,7 @@ static void pnv_psi_power8_instance_init(Object *obj) > Pnv8Psi *psi8 = PNV8_PSI(obj); > > object_initialize_child(obj, "ics-psi", &psi8->ics, sizeof(psi8->ics), > - TYPE_ICS_SIMPLE, &error_abort, NULL); > + TYPE_ICS, &error_abort, NULL); > } > > static const uint8_t irq_to_xivr[] = { > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index ac189c5796..6c45d2a3c0 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -98,7 +98,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs, > Object *obj; > Error *local_err = NULL; > > - obj = object_new(TYPE_ICS_SIMPLE); > + obj = object_new(TYPE_ICS); > object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); > object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > &error_fatal); > @@ -109,7 +109,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs, > return; > } > > - spapr->ics = ICS_BASE(obj); > + spapr->ics = ICS(obj); > > xics_spapr_init(spapr); > } > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index 92628e7cab..d8cf206a69 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -89,21 +89,8 @@ struct PnvICPState { > uint32_t links[3]; > }; > > -#define TYPE_ICS_BASE "ics-base" > -#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE) > - > -/* Retain ics for sPAPR for migration from existing sPAPR guests */ > -#define TYPE_ICS_SIMPLE "ics" > -#define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) > - > -#define ICS_BASE_CLASS(klass) \ > - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_BASE) > -#define ICS_BASE_GET_CLASS(obj) \ > - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_BASE) > - > -struct ICSStateClass { > - DeviceClass parent_class; > -}; > +#define TYPE_ICS "ics" > +#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) > > struct ICSState { > /*< private >*/ >
On Tue, 24 Sep 2019 14:59:52 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > TYPE_ICS_SIMPLE is the only subtype of TYPE_ICS_BASE that's ever > instantiated, and the only one we're ever likely to want. The > existence of different classes is just a hang over from when we > (misguidedly) had separate subtypes for the KVM and non-KVM version of > the device. > > So, collapse the two classes together into just TYPE_ICS. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > hw/intc/xics.c | 57 ++++++++++++++++--------------------------- > hw/ppc/pnv_psi.c | 2 +- > hw/ppc/spapr_irq.c | 4 +-- > include/hw/ppc/xics.h | 17 ++----------- > 4 files changed, 26 insertions(+), 54 deletions(-) > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 9ae51bbc76..388dbba870 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -555,7 +555,7 @@ static void ics_reset_irq(ICSIRQState *irq) > > static void ics_reset(DeviceState *dev) > { > - ICSState *ics = ICS_BASE(dev); > + ICSState *ics = ICS(dev); > int i; > uint8_t flags[ics->nr_irqs]; > > @@ -573,7 +573,7 @@ static void ics_reset(DeviceState *dev) > if (kvm_irqchip_in_kernel()) { > Error *local_err = NULL; > > - ics_set_kvm_state(ICS_BASE(dev), &local_err); > + ics_set_kvm_state(ICS(dev), &local_err); > if (local_err) { > error_report_err(local_err); > } > @@ -587,7 +587,7 @@ static void ics_reset_handler(void *dev) > > static void ics_realize(DeviceState *dev, Error **errp) > { > - ICSState *ics = ICS_BASE(dev); > + ICSState *ics = ICS(dev); > Error *local_err = NULL; > Object *obj; > > @@ -609,26 +609,14 @@ static void ics_realize(DeviceState *dev, Error **errp) > qemu_register_reset(ics_reset_handler, ics); > } > > -static void ics_simple_class_init(ObjectClass *klass, void *data) > +static void ics_instance_init(Object *obj) > { > -} > - > -static const TypeInfo ics_simple_info = { > - .name = TYPE_ICS_SIMPLE, > - .parent = TYPE_ICS_BASE, > - .instance_size = sizeof(ICSState), > - .class_init = ics_simple_class_init, > - .class_size = sizeof(ICSStateClass), > -}; > - > -static void ics_base_instance_init(Object *obj) > -{ > - ICSState *ics = ICS_BASE(obj); > + ICSState *ics = ICS(obj); > > ics->offset = XICS_IRQ_BASE; > } > > -static int ics_base_pre_save(void *opaque) > +static int ics_pre_save(void *opaque) > { > ICSState *ics = opaque; > > @@ -639,7 +627,7 @@ static int ics_base_pre_save(void *opaque) > return 0; > } > > -static int ics_base_post_load(void *opaque, int version_id) > +static int ics_post_load(void *opaque, int version_id) > { > ICSState *ics = opaque; > > @@ -657,7 +645,7 @@ static int ics_base_post_load(void *opaque, int version_id) > return 0; > } > > -static const VMStateDescription vmstate_ics_base_irq = { > +static const VMStateDescription vmstate_ics_irq = { > .name = "ics/irq", > .version_id = 2, > .minimum_version_id = 1, > @@ -671,46 +659,44 @@ static const VMStateDescription vmstate_ics_base_irq = { > }, > }; > > -static const VMStateDescription vmstate_ics_base = { > +static const VMStateDescription vmstate_ics = { > .name = "ics", > .version_id = 1, > .minimum_version_id = 1, > - .pre_save = ics_base_pre_save, > - .post_load = ics_base_post_load, > + .pre_save = ics_pre_save, > + .post_load = ics_post_load, > .fields = (VMStateField[]) { > /* Sanity check */ > VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL), > > VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs, > - vmstate_ics_base_irq, > + vmstate_ics_irq, > ICSIRQState), > VMSTATE_END_OF_LIST() > }, > }; > > -static Property ics_base_properties[] = { > +static Property ics_properties[] = { > DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), > DEFINE_PROP_END_OF_LIST(), > }; > > -static void ics_base_class_init(ObjectClass *klass, void *data) > +static void ics_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > > dc->realize = ics_realize; > - dc->props = ics_base_properties; > + dc->props = ics_properties; > dc->reset = ics_reset; > - dc->vmsd = &vmstate_ics_base; > + dc->vmsd = &vmstate_ics; > } > > -static const TypeInfo ics_base_info = { > - .name = TYPE_ICS_BASE, > +static const TypeInfo ics_info = { > + .name = TYPE_ICS, > .parent = TYPE_DEVICE, > - .abstract = true, > .instance_size = sizeof(ICSState), > - .instance_init = ics_base_instance_init, > - .class_init = ics_base_class_init, > - .class_size = sizeof(ICSStateClass), > + .instance_init = ics_instance_init, > + .class_init = ics_class_init, > }; > > static const TypeInfo xics_fabric_info = { > @@ -749,8 +735,7 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > > static void xics_register_types(void) > { > - type_register_static(&ics_simple_info); > - type_register_static(&ics_base_info); > + type_register_static(&ics_info); > type_register_static(&icp_info); > type_register_static(&xics_fabric_info); > } > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c > index 8ea81e9d8e..a997f16bb4 100644 > --- a/hw/ppc/pnv_psi.c > +++ b/hw/ppc/pnv_psi.c > @@ -469,7 +469,7 @@ static void pnv_psi_power8_instance_init(Object *obj) > Pnv8Psi *psi8 = PNV8_PSI(obj); > > object_initialize_child(obj, "ics-psi", &psi8->ics, sizeof(psi8->ics), > - TYPE_ICS_SIMPLE, &error_abort, NULL); > + TYPE_ICS, &error_abort, NULL); > } > > static const uint8_t irq_to_xivr[] = { > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index ac189c5796..6c45d2a3c0 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -98,7 +98,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs, > Object *obj; > Error *local_err = NULL; > > - obj = object_new(TYPE_ICS_SIMPLE); > + obj = object_new(TYPE_ICS); > object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); > object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > &error_fatal); > @@ -109,7 +109,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs, > return; > } > > - spapr->ics = ICS_BASE(obj); > + spapr->ics = ICS(obj); > > xics_spapr_init(spapr); > } > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index 92628e7cab..d8cf206a69 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -89,21 +89,8 @@ struct PnvICPState { > uint32_t links[3]; > }; > > -#define TYPE_ICS_BASE "ics-base" > -#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE) > - > -/* Retain ics for sPAPR for migration from existing sPAPR guests */ > -#define TYPE_ICS_SIMPLE "ics" > -#define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) > - > -#define ICS_BASE_CLASS(klass) \ > - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_BASE) > -#define ICS_BASE_GET_CLASS(obj) \ > - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_BASE) > - > -struct ICSStateClass { > - DeviceClass parent_class; > -}; > +#define TYPE_ICS "ics" > +#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) > > struct ICSState { > /*< private >*/
On 9/24/19 6:59 AM, David Gibson wrote: > TYPE_ICS_SIMPLE is the only subtype of TYPE_ICS_BASE that's ever > instantiated, and the only one we're ever likely to want. The > existence of different classes is just a hang over from when we > (misguidedly) had separate subtypes for the KVM and non-KVM version of > the device. > > So, collapse the two classes together into just TYPE_ICS. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/intc/xics.c | 57 ++++++++++++++++--------------------------- > hw/ppc/pnv_psi.c | 2 +- > hw/ppc/spapr_irq.c | 4 +-- > include/hw/ppc/xics.h | 17 ++----------- > 4 files changed, 26 insertions(+), 54 deletions(-) > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 9ae51bbc76..388dbba870 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -555,7 +555,7 @@ static void ics_reset_irq(ICSIRQState *irq) > > static void ics_reset(DeviceState *dev) > { > - ICSState *ics = ICS_BASE(dev); > + ICSState *ics = ICS(dev); > int i; > uint8_t flags[ics->nr_irqs]; > > @@ -573,7 +573,7 @@ static void ics_reset(DeviceState *dev) > if (kvm_irqchip_in_kernel()) { > Error *local_err = NULL; > > - ics_set_kvm_state(ICS_BASE(dev), &local_err); > + ics_set_kvm_state(ICS(dev), &local_err); > if (local_err) { > error_report_err(local_err); > } > @@ -587,7 +587,7 @@ static void ics_reset_handler(void *dev) > > static void ics_realize(DeviceState *dev, Error **errp) > { > - ICSState *ics = ICS_BASE(dev); > + ICSState *ics = ICS(dev); > Error *local_err = NULL; > Object *obj; > > @@ -609,26 +609,14 @@ static void ics_realize(DeviceState *dev, Error **errp) > qemu_register_reset(ics_reset_handler, ics); > } > > -static void ics_simple_class_init(ObjectClass *klass, void *data) > +static void ics_instance_init(Object *obj) > { > -} > - > -static const TypeInfo ics_simple_info = { > - .name = TYPE_ICS_SIMPLE, > - .parent = TYPE_ICS_BASE, > - .instance_size = sizeof(ICSState), > - .class_init = ics_simple_class_init, > - .class_size = sizeof(ICSStateClass), > -}; > - > -static void ics_base_instance_init(Object *obj) > -{ > - ICSState *ics = ICS_BASE(obj); > + ICSState *ics = ICS(obj); > > ics->offset = XICS_IRQ_BASE; > } > > -static int ics_base_pre_save(void *opaque) > +static int ics_pre_save(void *opaque) > { > ICSState *ics = opaque; > > @@ -639,7 +627,7 @@ static int ics_base_pre_save(void *opaque) > return 0; > } > > -static int ics_base_post_load(void *opaque, int version_id) > +static int ics_post_load(void *opaque, int version_id) > { > ICSState *ics = opaque; > > @@ -657,7 +645,7 @@ static int ics_base_post_load(void *opaque, int version_id) > return 0; > } > > -static const VMStateDescription vmstate_ics_base_irq = { > +static const VMStateDescription vmstate_ics_irq = { > .name = "ics/irq", > .version_id = 2, > .minimum_version_id = 1, > @@ -671,46 +659,44 @@ static const VMStateDescription vmstate_ics_base_irq = { > }, > }; > > -static const VMStateDescription vmstate_ics_base = { > +static const VMStateDescription vmstate_ics = { > .name = "ics", > .version_id = 1, > .minimum_version_id = 1, > - .pre_save = ics_base_pre_save, > - .post_load = ics_base_post_load, > + .pre_save = ics_pre_save, > + .post_load = ics_post_load, > .fields = (VMStateField[]) { > /* Sanity check */ > VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL), > > VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs, > - vmstate_ics_base_irq, > + vmstate_ics_irq, > ICSIRQState), > VMSTATE_END_OF_LIST() > }, > }; > > -static Property ics_base_properties[] = { > +static Property ics_properties[] = { > DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), > DEFINE_PROP_END_OF_LIST(), > }; > > -static void ics_base_class_init(ObjectClass *klass, void *data) > +static void ics_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > > dc->realize = ics_realize; > - dc->props = ics_base_properties; > + dc->props = ics_properties; > dc->reset = ics_reset; > - dc->vmsd = &vmstate_ics_base; > + dc->vmsd = &vmstate_ics; > } > > -static const TypeInfo ics_base_info = { > - .name = TYPE_ICS_BASE, > +static const TypeInfo ics_info = { > + .name = TYPE_ICS, > .parent = TYPE_DEVICE, Ah now I see this, OK. > - .abstract = true, > .instance_size = sizeof(ICSState), > - .instance_init = ics_base_instance_init, > - .class_init = ics_base_class_init, > - .class_size = sizeof(ICSStateClass), > + .instance_init = ics_instance_init, > + .class_init = ics_class_init, > }; > > static const TypeInfo xics_fabric_info = { > @@ -749,8 +735,7 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > > static void xics_register_types(void) > { > - type_register_static(&ics_simple_info); > - type_register_static(&ics_base_info); > + type_register_static(&ics_info); > type_register_static(&icp_info); > type_register_static(&xics_fabric_info); > } > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c > index 8ea81e9d8e..a997f16bb4 100644 > --- a/hw/ppc/pnv_psi.c > +++ b/hw/ppc/pnv_psi.c > @@ -469,7 +469,7 @@ static void pnv_psi_power8_instance_init(Object *obj) > Pnv8Psi *psi8 = PNV8_PSI(obj); > > object_initialize_child(obj, "ics-psi", &psi8->ics, sizeof(psi8->ics), > - TYPE_ICS_SIMPLE, &error_abort, NULL); > + TYPE_ICS, &error_abort, NULL); > } > > static const uint8_t irq_to_xivr[] = { > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index ac189c5796..6c45d2a3c0 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -98,7 +98,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs, > Object *obj; > Error *local_err = NULL; > > - obj = object_new(TYPE_ICS_SIMPLE); > + obj = object_new(TYPE_ICS); > object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); > object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > &error_fatal); > @@ -109,7 +109,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs, > return; > } > > - spapr->ics = ICS_BASE(obj); > + spapr->ics = ICS(obj); > > xics_spapr_init(spapr); > } > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index 92628e7cab..d8cf206a69 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -89,21 +89,8 @@ struct PnvICPState { > uint32_t links[3]; > }; > > -#define TYPE_ICS_BASE "ics-base" > -#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE) > - > -/* Retain ics for sPAPR for migration from existing sPAPR guests */ > -#define TYPE_ICS_SIMPLE "ics" > -#define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) > - > -#define ICS_BASE_CLASS(klass) \ > - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_BASE) > -#define ICS_BASE_GET_CLASS(obj) \ > - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_BASE) > - > -struct ICSStateClass { > - DeviceClass parent_class; > -}; > +#define TYPE_ICS "ics" > +#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) > > struct ICSState { > /*< private >*/ >
On Tue, Sep 24, 2019 at 07:31:44AM +0200, Cédric Le Goater wrote: > On 24/09/2019 06:59, David Gibson wrote: > > TYPE_ICS_SIMPLE is the only subtype of TYPE_ICS_BASE that's ever > > instantiated, and the only one we're ever likely to want. The > > existence of different classes is just a hang over from when we > > (misguidedly) had separate subtypes for the KVM and non-KVM version of > > the device. > > > > So, collapse the two classes together into just TYPE_ICS. > > > Well, I have been maintaining another subclass for the PHB3 MSI > but it has never been merged and it will require some rework. Well, if you did do this again, is there an actual need for it to be a subclass of ICS_BASE, and not ICS_SIMPLE? AFAICT the merged ICS class should be fine for pnv as well. > Anyhow the base ICS code is cleaner with that patch and it > does not seem to break migration. > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > C. > > > > --- > > hw/intc/xics.c | 57 ++++++++++++++++--------------------------- > > hw/ppc/pnv_psi.c | 2 +- > > hw/ppc/spapr_irq.c | 4 +-- > > include/hw/ppc/xics.h | 17 ++----------- > > 4 files changed, 26 insertions(+), 54 deletions(-) > > > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > > index 9ae51bbc76..388dbba870 100644 > > --- a/hw/intc/xics.c > > +++ b/hw/intc/xics.c > > @@ -555,7 +555,7 @@ static void ics_reset_irq(ICSIRQState *irq) > > > > static void ics_reset(DeviceState *dev) > > { > > - ICSState *ics = ICS_BASE(dev); > > + ICSState *ics = ICS(dev); > > int i; > > uint8_t flags[ics->nr_irqs]; > > > > @@ -573,7 +573,7 @@ static void ics_reset(DeviceState *dev) > > if (kvm_irqchip_in_kernel()) { > > Error *local_err = NULL; > > > > - ics_set_kvm_state(ICS_BASE(dev), &local_err); > > + ics_set_kvm_state(ICS(dev), &local_err); > > if (local_err) { > > error_report_err(local_err); > > } > > @@ -587,7 +587,7 @@ static void ics_reset_handler(void *dev) > > > > static void ics_realize(DeviceState *dev, Error **errp) > > { > > - ICSState *ics = ICS_BASE(dev); > > + ICSState *ics = ICS(dev); > > Error *local_err = NULL; > > Object *obj; > > > > @@ -609,26 +609,14 @@ static void ics_realize(DeviceState *dev, Error **errp) > > qemu_register_reset(ics_reset_handler, ics); > > } > > > > -static void ics_simple_class_init(ObjectClass *klass, void *data) > > +static void ics_instance_init(Object *obj) > > { > > -} > > - > > -static const TypeInfo ics_simple_info = { > > - .name = TYPE_ICS_SIMPLE, > > - .parent = TYPE_ICS_BASE, > > - .instance_size = sizeof(ICSState), > > - .class_init = ics_simple_class_init, > > - .class_size = sizeof(ICSStateClass), > > -}; > > - > > -static void ics_base_instance_init(Object *obj) > > -{ > > - ICSState *ics = ICS_BASE(obj); > > + ICSState *ics = ICS(obj); > > > > ics->offset = XICS_IRQ_BASE; > > } > > > > -static int ics_base_pre_save(void *opaque) > > +static int ics_pre_save(void *opaque) > > { > > ICSState *ics = opaque; > > > > @@ -639,7 +627,7 @@ static int ics_base_pre_save(void *opaque) > > return 0; > > } > > > > -static int ics_base_post_load(void *opaque, int version_id) > > +static int ics_post_load(void *opaque, int version_id) > > { > > ICSState *ics = opaque; > > > > @@ -657,7 +645,7 @@ static int ics_base_post_load(void *opaque, int version_id) > > return 0; > > } > > > > -static const VMStateDescription vmstate_ics_base_irq = { > > +static const VMStateDescription vmstate_ics_irq = { > > .name = "ics/irq", > > .version_id = 2, > > .minimum_version_id = 1, > > @@ -671,46 +659,44 @@ static const VMStateDescription vmstate_ics_base_irq = { > > }, > > }; > > > > -static const VMStateDescription vmstate_ics_base = { > > +static const VMStateDescription vmstate_ics = { > > .name = "ics", > > .version_id = 1, > > .minimum_version_id = 1, > > - .pre_save = ics_base_pre_save, > > - .post_load = ics_base_post_load, > > + .pre_save = ics_pre_save, > > + .post_load = ics_post_load, > > .fields = (VMStateField[]) { > > /* Sanity check */ > > VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL), > > > > VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs, > > - vmstate_ics_base_irq, > > + vmstate_ics_irq, > > ICSIRQState), > > VMSTATE_END_OF_LIST() > > }, > > }; > > > > -static Property ics_base_properties[] = { > > +static Property ics_properties[] = { > > DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > -static void ics_base_class_init(ObjectClass *klass, void *data) > > +static void ics_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > > > dc->realize = ics_realize; > > - dc->props = ics_base_properties; > > + dc->props = ics_properties; > > dc->reset = ics_reset; > > - dc->vmsd = &vmstate_ics_base; > > + dc->vmsd = &vmstate_ics; > > } > > > > -static const TypeInfo ics_base_info = { > > - .name = TYPE_ICS_BASE, > > +static const TypeInfo ics_info = { > > + .name = TYPE_ICS, > > .parent = TYPE_DEVICE, > > - .abstract = true, > > .instance_size = sizeof(ICSState), > > - .instance_init = ics_base_instance_init, > > - .class_init = ics_base_class_init, > > - .class_size = sizeof(ICSStateClass), > > + .instance_init = ics_instance_init, > > + .class_init = ics_class_init, > > }; > > > > static const TypeInfo xics_fabric_info = { > > @@ -749,8 +735,7 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > > > > static void xics_register_types(void) > > { > > - type_register_static(&ics_simple_info); > > - type_register_static(&ics_base_info); > > + type_register_static(&ics_info); > > type_register_static(&icp_info); > > type_register_static(&xics_fabric_info); > > } > > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c > > index 8ea81e9d8e..a997f16bb4 100644 > > --- a/hw/ppc/pnv_psi.c > > +++ b/hw/ppc/pnv_psi.c > > @@ -469,7 +469,7 @@ static void pnv_psi_power8_instance_init(Object *obj) > > Pnv8Psi *psi8 = PNV8_PSI(obj); > > > > object_initialize_child(obj, "ics-psi", &psi8->ics, sizeof(psi8->ics), > > - TYPE_ICS_SIMPLE, &error_abort, NULL); > > + TYPE_ICS, &error_abort, NULL); > > } > > > > static const uint8_t irq_to_xivr[] = { > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > > index ac189c5796..6c45d2a3c0 100644 > > --- a/hw/ppc/spapr_irq.c > > +++ b/hw/ppc/spapr_irq.c > > @@ -98,7 +98,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs, > > Object *obj; > > Error *local_err = NULL; > > > > - obj = object_new(TYPE_ICS_SIMPLE); > > + obj = object_new(TYPE_ICS); > > object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); > > object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > > &error_fatal); > > @@ -109,7 +109,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs, > > return; > > } > > > > - spapr->ics = ICS_BASE(obj); > > + spapr->ics = ICS(obj); > > > > xics_spapr_init(spapr); > > } > > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > > index 92628e7cab..d8cf206a69 100644 > > --- a/include/hw/ppc/xics.h > > +++ b/include/hw/ppc/xics.h > > @@ -89,21 +89,8 @@ struct PnvICPState { > > uint32_t links[3]; > > }; > > > > -#define TYPE_ICS_BASE "ics-base" > > -#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE) > > - > > -/* Retain ics for sPAPR for migration from existing sPAPR guests */ > > -#define TYPE_ICS_SIMPLE "ics" > > -#define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) > > - > > -#define ICS_BASE_CLASS(klass) \ > > - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_BASE) > > -#define ICS_BASE_GET_CLASS(obj) \ > > - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_BASE) > > - > > -struct ICSStateClass { > > - DeviceClass parent_class; > > -}; > > +#define TYPE_ICS "ics" > > +#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) > > > > struct ICSState { > > /*< private >*/ > > >
On 24/09/2019 13:41, David Gibson wrote: > On Tue, Sep 24, 2019 at 07:31:44AM +0200, Cédric Le Goater wrote: >> On 24/09/2019 06:59, David Gibson wrote: >>> TYPE_ICS_SIMPLE is the only subtype of TYPE_ICS_BASE that's ever >>> instantiated, and the only one we're ever likely to want. The >>> existence of different classes is just a hang over from when we >>> (misguidedly) had separate subtypes for the KVM and non-KVM version of >>> the device. >>> >>> So, collapse the two classes together into just TYPE_ICS. >> >> >> Well, I have been maintaining another subclass for the PHB3 MSI >> but it has never been merged and it will require some rework. > > Well, if you did do this again, is there an actual need for it to be a > subclass of ICS_BASE, and not ICS_SIMPLE? AFAICT the merged ICS class > should be fine for pnv as well. the reject resend handlers might be an issue. Anyhow, let's merge this cleanup. PHB3 has been out of tree for too long. C. >> Anyhow the base ICS code is cleaner with that patch and it >> does not seem to break migration. >> >>> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >> >> >> Reviewed-by: Cédric Le Goater <clg@kaod.org> >> >> C. >> >> >>> --- >>> hw/intc/xics.c | 57 ++++++++++++++++--------------------------- >>> hw/ppc/pnv_psi.c | 2 +- >>> hw/ppc/spapr_irq.c | 4 +-- >>> include/hw/ppc/xics.h | 17 ++----------- >>> 4 files changed, 26 insertions(+), 54 deletions(-) >>> >>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c >>> index 9ae51bbc76..388dbba870 100644 >>> --- a/hw/intc/xics.c >>> +++ b/hw/intc/xics.c >>> @@ -555,7 +555,7 @@ static void ics_reset_irq(ICSIRQState *irq) >>> >>> static void ics_reset(DeviceState *dev) >>> { >>> - ICSState *ics = ICS_BASE(dev); >>> + ICSState *ics = ICS(dev); >>> int i; >>> uint8_t flags[ics->nr_irqs]; >>> >>> @@ -573,7 +573,7 @@ static void ics_reset(DeviceState *dev) >>> if (kvm_irqchip_in_kernel()) { >>> Error *local_err = NULL; >>> >>> - ics_set_kvm_state(ICS_BASE(dev), &local_err); >>> + ics_set_kvm_state(ICS(dev), &local_err); >>> if (local_err) { >>> error_report_err(local_err); >>> } >>> @@ -587,7 +587,7 @@ static void ics_reset_handler(void *dev) >>> >>> static void ics_realize(DeviceState *dev, Error **errp) >>> { >>> - ICSState *ics = ICS_BASE(dev); >>> + ICSState *ics = ICS(dev); >>> Error *local_err = NULL; >>> Object *obj; >>> >>> @@ -609,26 +609,14 @@ static void ics_realize(DeviceState *dev, Error **errp) >>> qemu_register_reset(ics_reset_handler, ics); >>> } >>> >>> -static void ics_simple_class_init(ObjectClass *klass, void *data) >>> +static void ics_instance_init(Object *obj) >>> { >>> -} >>> - >>> -static const TypeInfo ics_simple_info = { >>> - .name = TYPE_ICS_SIMPLE, >>> - .parent = TYPE_ICS_BASE, >>> - .instance_size = sizeof(ICSState), >>> - .class_init = ics_simple_class_init, >>> - .class_size = sizeof(ICSStateClass), >>> -}; >>> - >>> -static void ics_base_instance_init(Object *obj) >>> -{ >>> - ICSState *ics = ICS_BASE(obj); >>> + ICSState *ics = ICS(obj); >>> >>> ics->offset = XICS_IRQ_BASE; >>> } >>> >>> -static int ics_base_pre_save(void *opaque) >>> +static int ics_pre_save(void *opaque) >>> { >>> ICSState *ics = opaque; >>> >>> @@ -639,7 +627,7 @@ static int ics_base_pre_save(void *opaque) >>> return 0; >>> } >>> >>> -static int ics_base_post_load(void *opaque, int version_id) >>> +static int ics_post_load(void *opaque, int version_id) >>> { >>> ICSState *ics = opaque; >>> >>> @@ -657,7 +645,7 @@ static int ics_base_post_load(void *opaque, int version_id) >>> return 0; >>> } >>> >>> -static const VMStateDescription vmstate_ics_base_irq = { >>> +static const VMStateDescription vmstate_ics_irq = { >>> .name = "ics/irq", >>> .version_id = 2, >>> .minimum_version_id = 1, >>> @@ -671,46 +659,44 @@ static const VMStateDescription vmstate_ics_base_irq = { >>> }, >>> }; >>> >>> -static const VMStateDescription vmstate_ics_base = { >>> +static const VMStateDescription vmstate_ics = { >>> .name = "ics", >>> .version_id = 1, >>> .minimum_version_id = 1, >>> - .pre_save = ics_base_pre_save, >>> - .post_load = ics_base_post_load, >>> + .pre_save = ics_pre_save, >>> + .post_load = ics_post_load, >>> .fields = (VMStateField[]) { >>> /* Sanity check */ >>> VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL), >>> >>> VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs, >>> - vmstate_ics_base_irq, >>> + vmstate_ics_irq, >>> ICSIRQState), >>> VMSTATE_END_OF_LIST() >>> }, >>> }; >>> >>> -static Property ics_base_properties[] = { >>> +static Property ics_properties[] = { >>> DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> >>> -static void ics_base_class_init(ObjectClass *klass, void *data) >>> +static void ics_class_init(ObjectClass *klass, void *data) >>> { >>> DeviceClass *dc = DEVICE_CLASS(klass); >>> >>> dc->realize = ics_realize; >>> - dc->props = ics_base_properties; >>> + dc->props = ics_properties; >>> dc->reset = ics_reset; >>> - dc->vmsd = &vmstate_ics_base; >>> + dc->vmsd = &vmstate_ics; >>> } >>> >>> -static const TypeInfo ics_base_info = { >>> - .name = TYPE_ICS_BASE, >>> +static const TypeInfo ics_info = { >>> + .name = TYPE_ICS, >>> .parent = TYPE_DEVICE, >>> - .abstract = true, >>> .instance_size = sizeof(ICSState), >>> - .instance_init = ics_base_instance_init, >>> - .class_init = ics_base_class_init, >>> - .class_size = sizeof(ICSStateClass), >>> + .instance_init = ics_instance_init, >>> + .class_init = ics_class_init, >>> }; >>> >>> static const TypeInfo xics_fabric_info = { >>> @@ -749,8 +735,7 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) >>> >>> static void xics_register_types(void) >>> { >>> - type_register_static(&ics_simple_info); >>> - type_register_static(&ics_base_info); >>> + type_register_static(&ics_info); >>> type_register_static(&icp_info); >>> type_register_static(&xics_fabric_info); >>> } >>> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c >>> index 8ea81e9d8e..a997f16bb4 100644 >>> --- a/hw/ppc/pnv_psi.c >>> +++ b/hw/ppc/pnv_psi.c >>> @@ -469,7 +469,7 @@ static void pnv_psi_power8_instance_init(Object *obj) >>> Pnv8Psi *psi8 = PNV8_PSI(obj); >>> >>> object_initialize_child(obj, "ics-psi", &psi8->ics, sizeof(psi8->ics), >>> - TYPE_ICS_SIMPLE, &error_abort, NULL); >>> + TYPE_ICS, &error_abort, NULL); >>> } >>> >>> static const uint8_t irq_to_xivr[] = { >>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c >>> index ac189c5796..6c45d2a3c0 100644 >>> --- a/hw/ppc/spapr_irq.c >>> +++ b/hw/ppc/spapr_irq.c >>> @@ -98,7 +98,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs, >>> Object *obj; >>> Error *local_err = NULL; >>> >>> - obj = object_new(TYPE_ICS_SIMPLE); >>> + obj = object_new(TYPE_ICS); >>> object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); >>> object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), >>> &error_fatal); >>> @@ -109,7 +109,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs, >>> return; >>> } >>> >>> - spapr->ics = ICS_BASE(obj); >>> + spapr->ics = ICS(obj); >>> >>> xics_spapr_init(spapr); >>> } >>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h >>> index 92628e7cab..d8cf206a69 100644 >>> --- a/include/hw/ppc/xics.h >>> +++ b/include/hw/ppc/xics.h >>> @@ -89,21 +89,8 @@ struct PnvICPState { >>> uint32_t links[3]; >>> }; >>> >>> -#define TYPE_ICS_BASE "ics-base" >>> -#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE) >>> - >>> -/* Retain ics for sPAPR for migration from existing sPAPR guests */ >>> -#define TYPE_ICS_SIMPLE "ics" >>> -#define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) >>> - >>> -#define ICS_BASE_CLASS(klass) \ >>> - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_BASE) >>> -#define ICS_BASE_GET_CLASS(obj) \ >>> - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_BASE) >>> - >>> -struct ICSStateClass { >>> - DeviceClass parent_class; >>> -}; >>> +#define TYPE_ICS "ics" >>> +#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) >>> >>> struct ICSState { >>> /*< private >*/ >>> >> >
On Tue, Sep 24, 2019 at 04:06:02PM +0200, Cédric Le Goater wrote: > On 24/09/2019 13:41, David Gibson wrote: > > On Tue, Sep 24, 2019 at 07:31:44AM +0200, Cédric Le Goater wrote: > >> On 24/09/2019 06:59, David Gibson wrote: > >>> TYPE_ICS_SIMPLE is the only subtype of TYPE_ICS_BASE that's ever > >>> instantiated, and the only one we're ever likely to want. The > >>> existence of different classes is just a hang over from when we > >>> (misguidedly) had separate subtypes for the KVM and non-KVM version of > >>> the device. > >>> > >>> So, collapse the two classes together into just TYPE_ICS. > >> > >> > >> Well, I have been maintaining another subclass for the PHB3 MSI > >> but it has never been merged and it will require some rework. > > > > Well, if you did do this again, is there an actual need for it to be a > > subclass of ICS_BASE, and not ICS_SIMPLE? AFAICT the merged ICS class > > should be fine for pnv as well. > > the reject resend handlers might be an issue. Anyhow, let's merge this > cleanup. PHB3 has been out of tree for too long. Hrm, are you sure. I remember thinking the other day "whatever happened to that PHB3 patchset?". Is it actually broken, or has it just been a long time since it was posted, and therefore been forgotten by me.
On 25/09/2019 03:46, David Gibson wrote: > On Tue, Sep 24, 2019 at 04:06:02PM +0200, Cédric Le Goater wrote: >> On 24/09/2019 13:41, David Gibson wrote: >>> On Tue, Sep 24, 2019 at 07:31:44AM +0200, Cédric Le Goater wrote: >>>> On 24/09/2019 06:59, David Gibson wrote: >>>>> TYPE_ICS_SIMPLE is the only subtype of TYPE_ICS_BASE that's ever >>>>> instantiated, and the only one we're ever likely to want. The >>>>> existence of different classes is just a hang over from when we >>>>> (misguidedly) had separate subtypes for the KVM and non-KVM version of >>>>> the device. >>>>> >>>>> So, collapse the two classes together into just TYPE_ICS. >>>> >>>> >>>> Well, I have been maintaining another subclass for the PHB3 MSI >>>> but it has never been merged and it will require some rework. >>> >>> Well, if you did do this again, is there an actual need for it to be a >>> subclass of ICS_BASE, and not ICS_SIMPLE? AFAICT the merged ICS class >>> should be fine for pnv as well. >> >> the reject resend handlers might be an issue. Anyhow, let's merge this >> cleanup. PHB3 has been out of tree for too long. > > Hrm, are you sure. I remember thinking the other day "whatever > happened to that PHB3 patchset?". Is it actually broken, It is not broken. PowerNV machines can boot rather complex PCI layouts on P8 (XICS) and P9 (XIVE). See the complex configuration examples here : https://github.com/legoater/qemu/wiki/PowerNV > or has it just been a long time since it was posted, and therefore > been forgotten by me. It hasn't been posted in a long time (+1 year). Here are the latest exchanges we had in November: http://patchwork.ozlabs.org/patch/951227/ Nothing really worrying : - some XICS infrastructure are needed (currently being removed) - check if we need a separate source model for the MSI/LSI - rework the PBCQ modeling to some extent. - misc cleanups. I have been quite busy this last year. I would appreciate if someone could take ownership of the PHB part. C.
On 25/09/2019 08:04, Cédric Le Goater wrote: > On 25/09/2019 03:46, David Gibson wrote: >> On Tue, Sep 24, 2019 at 04:06:02PM +0200, Cédric Le Goater wrote: >>> On 24/09/2019 13:41, David Gibson wrote: >>>> On Tue, Sep 24, 2019 at 07:31:44AM +0200, Cédric Le Goater wrote: >>>>> On 24/09/2019 06:59, David Gibson wrote: >>>>>> TYPE_ICS_SIMPLE is the only subtype of TYPE_ICS_BASE that's ever >>>>>> instantiated, and the only one we're ever likely to want. The >>>>>> existence of different classes is just a hang over from when we >>>>>> (misguidedly) had separate subtypes for the KVM and non-KVM version of >>>>>> the device. >>>>>> >>>>>> So, collapse the two classes together into just TYPE_ICS. >>>>> >>>>> >>>>> Well, I have been maintaining another subclass for the PHB3 MSI >>>>> but it has never been merged and it will require some rework. >>>> >>>> Well, if you did do this again, is there an actual need for it to be a >>>> subclass of ICS_BASE, and not ICS_SIMPLE? AFAICT the merged ICS class >>>> should be fine for pnv as well. >>> >>> the reject resend handlers might be an issue. Anyhow, let's merge this >>> cleanup. PHB3 has been out of tree for too long. >> >> Hrm, are you sure. I remember thinking the other day "whatever >> happened to that PHB3 patchset?". Is it actually broken, > > It is not broken. > > PowerNV machines can boot rather complex PCI layouts on P8 (XICS) > and P9 (XIVE). See the complex configuration examples here : > > https://github.com/legoater/qemu/wiki/PowerNV > >> or has it just been a long time since it was posted, and therefore >> been forgotten by me. > > It hasn't been posted in a long time (+1 year). Here are the latest > exchanges we had in November: > > http://patchwork.ozlabs.org/patch/951227/ > > Nothing really worrying : > > - some XICS infrastructure are needed (currently being removed) > - check if we need a separate source model for the MSI/LSI > - rework the PBCQ modeling to some extent. > - misc cleanups. > > I have been quite busy this last year. I would appreciate if someone > could take ownership of the PHB part. Recent XICS changes have made support of PHB3 complex and I don't have time to keep it alive anymore. I am dropping the patch from my tree. In case someone wants to take over, it's here : https://github.com/legoater/qemu/commits/powernv-4.2-p8 Focus is on P9 and P10 now. Cheers, C.
diff --git a/hw/intc/xics.c b/hw/intc/xics.c index 9ae51bbc76..388dbba870 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -555,7 +555,7 @@ static void ics_reset_irq(ICSIRQState *irq) static void ics_reset(DeviceState *dev) { - ICSState *ics = ICS_BASE(dev); + ICSState *ics = ICS(dev); int i; uint8_t flags[ics->nr_irqs]; @@ -573,7 +573,7 @@ static void ics_reset(DeviceState *dev) if (kvm_irqchip_in_kernel()) { Error *local_err = NULL; - ics_set_kvm_state(ICS_BASE(dev), &local_err); + ics_set_kvm_state(ICS(dev), &local_err); if (local_err) { error_report_err(local_err); } @@ -587,7 +587,7 @@ static void ics_reset_handler(void *dev) static void ics_realize(DeviceState *dev, Error **errp) { - ICSState *ics = ICS_BASE(dev); + ICSState *ics = ICS(dev); Error *local_err = NULL; Object *obj; @@ -609,26 +609,14 @@ static void ics_realize(DeviceState *dev, Error **errp) qemu_register_reset(ics_reset_handler, ics); } -static void ics_simple_class_init(ObjectClass *klass, void *data) +static void ics_instance_init(Object *obj) { -} - -static const TypeInfo ics_simple_info = { - .name = TYPE_ICS_SIMPLE, - .parent = TYPE_ICS_BASE, - .instance_size = sizeof(ICSState), - .class_init = ics_simple_class_init, - .class_size = sizeof(ICSStateClass), -}; - -static void ics_base_instance_init(Object *obj) -{ - ICSState *ics = ICS_BASE(obj); + ICSState *ics = ICS(obj); ics->offset = XICS_IRQ_BASE; } -static int ics_base_pre_save(void *opaque) +static int ics_pre_save(void *opaque) { ICSState *ics = opaque; @@ -639,7 +627,7 @@ static int ics_base_pre_save(void *opaque) return 0; } -static int ics_base_post_load(void *opaque, int version_id) +static int ics_post_load(void *opaque, int version_id) { ICSState *ics = opaque; @@ -657,7 +645,7 @@ static int ics_base_post_load(void *opaque, int version_id) return 0; } -static const VMStateDescription vmstate_ics_base_irq = { +static const VMStateDescription vmstate_ics_irq = { .name = "ics/irq", .version_id = 2, .minimum_version_id = 1, @@ -671,46 +659,44 @@ static const VMStateDescription vmstate_ics_base_irq = { }, }; -static const VMStateDescription vmstate_ics_base = { +static const VMStateDescription vmstate_ics = { .name = "ics", .version_id = 1, .minimum_version_id = 1, - .pre_save = ics_base_pre_save, - .post_load = ics_base_post_load, + .pre_save = ics_pre_save, + .post_load = ics_post_load, .fields = (VMStateField[]) { /* Sanity check */ VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL), VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs, - vmstate_ics_base_irq, + vmstate_ics_irq, ICSIRQState), VMSTATE_END_OF_LIST() }, }; -static Property ics_base_properties[] = { +static Property ics_properties[] = { DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), DEFINE_PROP_END_OF_LIST(), }; -static void ics_base_class_init(ObjectClass *klass, void *data) +static void ics_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); dc->realize = ics_realize; - dc->props = ics_base_properties; + dc->props = ics_properties; dc->reset = ics_reset; - dc->vmsd = &vmstate_ics_base; + dc->vmsd = &vmstate_ics; } -static const TypeInfo ics_base_info = { - .name = TYPE_ICS_BASE, +static const TypeInfo ics_info = { + .name = TYPE_ICS, .parent = TYPE_DEVICE, - .abstract = true, .instance_size = sizeof(ICSState), - .instance_init = ics_base_instance_init, - .class_init = ics_base_class_init, - .class_size = sizeof(ICSStateClass), + .instance_init = ics_instance_init, + .class_init = ics_class_init, }; static const TypeInfo xics_fabric_info = { @@ -749,8 +735,7 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) static void xics_register_types(void) { - type_register_static(&ics_simple_info); - type_register_static(&ics_base_info); + type_register_static(&ics_info); type_register_static(&icp_info); type_register_static(&xics_fabric_info); } diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c index 8ea81e9d8e..a997f16bb4 100644 --- a/hw/ppc/pnv_psi.c +++ b/hw/ppc/pnv_psi.c @@ -469,7 +469,7 @@ static void pnv_psi_power8_instance_init(Object *obj) Pnv8Psi *psi8 = PNV8_PSI(obj); object_initialize_child(obj, "ics-psi", &psi8->ics, sizeof(psi8->ics), - TYPE_ICS_SIMPLE, &error_abort, NULL); + TYPE_ICS, &error_abort, NULL); } static const uint8_t irq_to_xivr[] = { diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c index ac189c5796..6c45d2a3c0 100644 --- a/hw/ppc/spapr_irq.c +++ b/hw/ppc/spapr_irq.c @@ -98,7 +98,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs, Object *obj; Error *local_err = NULL; - obj = object_new(TYPE_ICS_SIMPLE); + obj = object_new(TYPE_ICS); object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), &error_fatal); @@ -109,7 +109,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs, return; } - spapr->ics = ICS_BASE(obj); + spapr->ics = ICS(obj); xics_spapr_init(spapr); } diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h index 92628e7cab..d8cf206a69 100644 --- a/include/hw/ppc/xics.h +++ b/include/hw/ppc/xics.h @@ -89,21 +89,8 @@ struct PnvICPState { uint32_t links[3]; }; -#define TYPE_ICS_BASE "ics-base" -#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE) - -/* Retain ics for sPAPR for migration from existing sPAPR guests */ -#define TYPE_ICS_SIMPLE "ics" -#define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) - -#define ICS_BASE_CLASS(klass) \ - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_BASE) -#define ICS_BASE_GET_CLASS(obj) \ - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_BASE) - -struct ICSStateClass { - DeviceClass parent_class; -}; +#define TYPE_ICS "ics" +#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) struct ICSState { /*< private >*/
TYPE_ICS_SIMPLE is the only subtype of TYPE_ICS_BASE that's ever instantiated, and the only one we're ever likely to want. The existence of different classes is just a hang over from when we (misguidedly) had separate subtypes for the KVM and non-KVM version of the device. So, collapse the two classes together into just TYPE_ICS. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/intc/xics.c | 57 ++++++++++++++++--------------------------- hw/ppc/pnv_psi.c | 2 +- hw/ppc/spapr_irq.c | 4 +-- include/hw/ppc/xics.h | 17 ++----------- 4 files changed, 26 insertions(+), 54 deletions(-)