| Message ID | 1474266577-11704-7-git-send-email-nikunj@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote: > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > The existing implementation remains same and ics-base is introduced. The > type name "ics" is retained, and all the related functions renamed as > ics_simple_* > > This will allow different implementations for the source controllers > such as the MSI support of PHB3 on Power8 which uses in-memory state > tables for example. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> This patch had a Reviewed-by from David. Did you change anything in v4 ? Thanks, C. > --- > hw/intc/trace-events | 10 ++-- > hw/intc/xics.c | 143 +++++++++++++++++++++++++++++++------------------- > hw/intc/xics_kvm.c | 10 ++-- > hw/intc/xics_spapr.c | 28 +++++----- > include/hw/ppc/xics.h | 23 +++++--- > 5 files changed, 131 insertions(+), 83 deletions(-) > > diff --git a/hw/intc/trace-events b/hw/intc/trace-events > index aa342a8..a367b46 100644 > --- a/hw/intc/trace-events > +++ b/hw/intc/trace-events > @@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr) "icp_accept: XIRR %#"PRIx3 > xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi: server %d given XIRR %#"PRIx32" new XIRR %#"PRIx32 > xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to deliver irq %#"PRIx32" priority %#x" > xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new XIRR=%#x new pending priority=%#x" > -xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]" > +xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]" > xics_masked_pending(void) "set_irq_msi: masked pending" > -xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]" > -xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x" > -xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]" > -xics_ics_eoi(int nr) "ics_eoi: irq %#x" > +xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]" > +xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x" > +xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]" > +xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x" > xics_alloc(int irq) "irq %d" > xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d irqs, lsi=%d, alignnum %d" > xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs" > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index c7901c4..b15751e 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics) > { > ICSState *ics; > > - ics = ICS(object_new(TYPE_ICS)); > + ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE)); > object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL); > ics->xics = xics; > QLIST_INSERT_HEAD(&xics->ics, ics, list); > @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = { > #define XISR(ss) (((ss)->xirr) & XISR_MASK) > #define CPPR(ss) (((ss)->xirr) >> 24) > > -static void ics_reject(ICSState *ics, int nr); > -static void ics_resend(ICSState *ics, int server); > -static void ics_eoi(ICSState *ics, int nr); > +static void ics_reject(ICSState *ics, uint32_t nr) > +{ > + ICSStateClass *k = ICS_GET_CLASS(ics); > + > + if (k->reject) { > + k->reject(ics, nr); > + } > +} > + > +static void ics_resend(ICSState *ics, int server) > +{ > + ICSStateClass *k = ICS_GET_CLASS(ics); > + > + if (k->resend) { > + k->resend(ics, server); > + } > +} > + > +static void ics_eoi(ICSState *ics, int nr) > +{ > + ICSStateClass *k = ICS_GET_CLASS(ics); > + > + if (k->eoi) { > + k->eoi(ics, nr); > + } > +} > > static void icp_check_ipi(ICPState *ss) > { > @@ -462,7 +485,7 @@ static const TypeInfo icp_info = { > /* > * ICS: Source layer > */ > -static void resend_msi(ICSState *ics, int srcno) > +static void ics_simple_resend_msi(ICSState *ics, int srcno) > { > ICSIRQState *irq = ics->irqs + srcno; > > @@ -475,7 +498,7 @@ static void resend_msi(ICSState *ics, int srcno) > } > } > > -static void resend_lsi(ICSState *ics, int srcno) > +static void ics_simple_resend_lsi(ICSState *ics, int srcno) > { > ICSIRQState *irq = ics->irqs + srcno; > > @@ -487,11 +510,11 @@ static void resend_lsi(ICSState *ics, int srcno) > } > } > > -static void set_irq_msi(ICSState *ics, int srcno, int val) > +static void ics_simple_set_irq_msi(ICSState *ics, int srcno, int val) > { > ICSIRQState *irq = ics->irqs + srcno; > > - trace_xics_set_irq_msi(srcno, srcno + ics->offset); > + trace_xics_ics_simple_set_irq_msi(srcno, srcno + ics->offset); > > if (val) { > if (irq->priority == 0xff) { > @@ -503,31 +526,31 @@ static void set_irq_msi(ICSState *ics, int srcno, int val) > } > } > > -static void set_irq_lsi(ICSState *ics, int srcno, int val) > +static void ics_simple_set_irq_lsi(ICSState *ics, int srcno, int val) > { > ICSIRQState *irq = ics->irqs + srcno; > > - trace_xics_set_irq_lsi(srcno, srcno + ics->offset); > + trace_xics_ics_simple_set_irq_lsi(srcno, srcno + ics->offset); > if (val) { > irq->status |= XICS_STATUS_ASSERTED; > } else { > irq->status &= ~XICS_STATUS_ASSERTED; > } > - resend_lsi(ics, srcno); > + ics_simple_resend_lsi(ics, srcno); > } > > -static void ics_set_irq(void *opaque, int srcno, int val) > +static void ics_simple_set_irq(void *opaque, int srcno, int val) > { > ICSState *ics = (ICSState *)opaque; > > if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { > - set_irq_lsi(ics, srcno, val); > + ics_simple_set_irq_lsi(ics, srcno, val); > } else { > - set_irq_msi(ics, srcno, val); > + ics_simple_set_irq_msi(ics, srcno, val); > } > } > > -static void write_xive_msi(ICSState *ics, int srcno) > +static void ics_simple_write_xive_msi(ICSState *ics, int srcno) > { > ICSIRQState *irq = ics->irqs + srcno; > > @@ -540,35 +563,35 @@ static void write_xive_msi(ICSState *ics, int srcno) > icp_irq(ics, irq->server, srcno + ics->offset, irq->priority); > } > > -static void write_xive_lsi(ICSState *ics, int srcno) > +static void ics_simple_write_xive_lsi(ICSState *ics, int srcno) > { > - resend_lsi(ics, srcno); > + ics_simple_resend_lsi(ics, srcno); > } > > -void ics_write_xive(ICSState *ics, int nr, int server, > - uint8_t priority, uint8_t saved_priority) > +void ics_simple_write_xive(ICSState *ics, int srcno, int server, > + uint8_t priority, uint8_t saved_priority) > { > - int srcno = nr - ics->offset; > ICSIRQState *irq = ics->irqs + srcno; > > irq->server = server; > irq->priority = priority; > irq->saved_priority = saved_priority; > > - trace_xics_ics_write_xive(nr, srcno, server, priority); > + trace_xics_ics_simple_write_xive(ics->offset + srcno, srcno, server, > + priority); > > if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { > - write_xive_lsi(ics, srcno); > + ics_simple_write_xive_lsi(ics, srcno); > } else { > - write_xive_msi(ics, srcno); > + ics_simple_write_xive_msi(ics, srcno); > } > } > > -static void ics_reject(ICSState *ics, int nr) > +static void ics_simple_reject(ICSState *ics, uint32_t nr) > { > ICSIRQState *irq = ics->irqs + nr - ics->offset; > > - trace_xics_ics_reject(nr, nr - ics->offset); > + trace_xics_ics_simple_reject(nr, nr - ics->offset); > if (irq->flags & XICS_FLAGS_IRQ_MSI) { > irq->status |= XICS_STATUS_REJECTED; > } else if (irq->flags & XICS_FLAGS_IRQ_LSI) { > @@ -576,7 +599,7 @@ static void ics_reject(ICSState *ics, int nr) > } > } > > -static void ics_resend(ICSState *ics, int server) > +static void ics_simple_resend(ICSState *ics, int server) > { > int i; > ICPState *ss = ics->xics->ss + server; > @@ -594,28 +617,28 @@ static void ics_resend(ICSState *ics, int server) > qemu_irq_raise(ss->output); > continue; > } > - resend_lsi(ics, i); > + ics_simple_resend_lsi(ics, i); > } else { > - resend_msi(ics, i); > + ics_simple_resend_msi(ics, i); > } > } > } > > -static void ics_eoi(ICSState *ics, int nr) > +static void ics_simple_eoi(ICSState *ics, uint32_t nr) > { > int srcno = nr - ics->offset; > ICSIRQState *irq = ics->irqs + srcno; > > - trace_xics_ics_eoi(nr); > + trace_xics_ics_simple_eoi(nr); > > if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { > irq->status &= ~XICS_STATUS_SENT; > } > } > > -static void ics_reset(DeviceState *dev) > +static void ics_simple_reset(DeviceState *dev) > { > - ICSState *ics = ICS(dev); > + ICSState *ics = ICS_SIMPLE(dev); > int i; > uint8_t flags[ics->nr_irqs]; > > @@ -632,7 +655,7 @@ static void ics_reset(DeviceState *dev) > } > } > > -static void ics_dispatch_pre_save(void *opaque) > +static void ics_simple_dispatch_pre_save(void *opaque) > { > ICSState *ics = opaque; > ICSStateClass *info = ICS_GET_CLASS(ics); > @@ -642,7 +665,7 @@ static void ics_dispatch_pre_save(void *opaque) > } > } > > -static int ics_dispatch_post_load(void *opaque, int version_id) > +static int ics_simple_dispatch_post_load(void *opaque, int version_id) > { > ICSState *ics = opaque; > ICSStateClass *info = ICS_GET_CLASS(ics); > @@ -654,7 +677,7 @@ static int ics_dispatch_post_load(void *opaque, int version_id) > return 0; > } > > -static const VMStateDescription vmstate_ics_irq = { > +static const VMStateDescription vmstate_ics_simple_irq = { > .name = "ics/irq", > .version_id = 2, > .minimum_version_id = 1, > @@ -668,57 +691,70 @@ static const VMStateDescription vmstate_ics_irq = { > }, > }; > > -static const VMStateDescription vmstate_ics = { > +static const VMStateDescription vmstate_ics_simple = { > .name = "ics", > .version_id = 1, > .minimum_version_id = 1, > - .pre_save = ics_dispatch_pre_save, > - .post_load = ics_dispatch_post_load, > + .pre_save = ics_simple_dispatch_pre_save, > + .post_load = ics_simple_dispatch_post_load, > .fields = (VMStateField[]) { > /* Sanity check */ > VMSTATE_UINT32_EQUAL(nr_irqs, ICSState), > > VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs, > - vmstate_ics_irq, ICSIRQState), > + vmstate_ics_simple_irq, > + ICSIRQState), > VMSTATE_END_OF_LIST() > }, > }; > > -static void ics_initfn(Object *obj) > +static void ics_simple_initfn(Object *obj) > { > - ICSState *ics = ICS(obj); > + ICSState *ics = ICS_SIMPLE(obj); > > ics->offset = XICS_IRQ_BASE; > } > > -static void ics_realize(DeviceState *dev, Error **errp) > +static void ics_simple_realize(DeviceState *dev, Error **errp) > { > - ICSState *ics = ICS(dev); > + ICSState *ics = ICS_SIMPLE(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->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs); > + ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs); > } > > -static void ics_class_init(ObjectClass *klass, void *data) > +static void ics_simple_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > + ICSStateClass *isc = ICS_CLASS(klass); > > - dc->realize = ics_realize; > - dc->vmsd = &vmstate_ics; > - dc->reset = ics_reset; > + dc->realize = ics_simple_realize; > + dc->vmsd = &vmstate_ics_simple; > + dc->reset = ics_simple_reset; > + isc->reject = ics_simple_reject; > + isc->resend = ics_simple_resend; > + isc->eoi = ics_simple_eoi; > } > > -static const TypeInfo ics_info = { > - .name = TYPE_ICS, > +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), > + .instance_init = ics_simple_initfn, > +}; > + > +static const TypeInfo ics_base_info = { > + .name = TYPE_ICS_BASE, > .parent = TYPE_DEVICE, > + .abstract = true, > .instance_size = sizeof(ICSState), > - .class_init = ics_class_init, > .class_size = sizeof(ICSStateClass), > - .instance_init = ics_initfn, > }; > > /* > @@ -758,7 +794,8 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > static void xics_register_types(void) > { > type_register_static(&xics_common_info); > - type_register_static(&ics_info); > + type_register_static(&ics_simple_info); > + type_register_static(&ics_base_info); > type_register_static(&icp_info); > } > > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c > index 04fa7cb..89862df 100644 > --- a/hw/intc/xics_kvm.c > +++ b/hw/intc/xics_kvm.c > @@ -272,7 +272,7 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val) > > static void ics_kvm_reset(DeviceState *dev) > { > - ICSState *ics = ICS(dev); > + ICSState *ics = ICS_SIMPLE(dev); > int i; > uint8_t flags[ics->nr_irqs]; > > @@ -293,7 +293,7 @@ static void ics_kvm_reset(DeviceState *dev) > > static void ics_kvm_realize(DeviceState *dev, Error **errp) > { > - ICSState *ics = ICS(dev); > + ICSState *ics = ICS_SIMPLE(dev); > > if (!ics->nr_irqs) { > error_setg(errp, "Number of interrupts needs to be greater 0"); > @@ -315,8 +315,8 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data) > } > > static const TypeInfo ics_kvm_info = { > - .name = TYPE_KVM_ICS, > - .parent = TYPE_ICS, > + .name = TYPE_ICS_KVM, > + .parent = TYPE_ICS_SIMPLE, > .instance_size = sizeof(ICSState), > .class_init = ics_kvm_class_init, > }; > @@ -492,7 +492,7 @@ static void xics_kvm_initfn(Object *obj) > XICSState *xics = XICS_COMMON(obj); > ICSState *ics; > > - ics = ICS(object_new(TYPE_KVM_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); > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > index 270f20e..4dd1399 100644 > --- a/hw/intc/xics_spapr.c > +++ b/hw/intc/xics_spapr.c > @@ -114,7 +114,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, > uint32_t nret, target_ulong rets) > { > ICSState *ics = QLIST_FIRST(&spapr->xics->ics); > - uint32_t nr, server, priority; > + uint32_t nr, srcno, server, priority; > > if ((nargs != 3) || (nret != 1)) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -135,7 +135,8 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, > return; > } > > - ics_write_xive(ics, nr, server, priority, priority); > + srcno = nr - ics->offset; > + ics_simple_write_xive(ics, srcno, server, priority, priority); > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > @@ -146,7 +147,7 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, > uint32_t nret, target_ulong rets) > { > ICSState *ics = QLIST_FIRST(&spapr->xics->ics); > - uint32_t nr; > + uint32_t nr, srcno; > > if ((nargs != 1) || (nret != 3)) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -165,8 +166,9 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, > } > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > - rtas_st(rets, 1, ics->irqs[nr - ics->offset].server); > - rtas_st(rets, 2, ics->irqs[nr - ics->offset].priority); > + srcno = nr - ics->offset; > + rtas_st(rets, 1, ics->irqs[srcno].server); > + rtas_st(rets, 2, ics->irqs[srcno].priority); > } > > static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr, > @@ -175,7 +177,7 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr, > uint32_t nret, target_ulong rets) > { > ICSState *ics = QLIST_FIRST(&spapr->xics->ics); > - uint32_t nr; > + uint32_t nr, srcno; > > if ((nargs != 1) || (nret != 1)) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -193,8 +195,9 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr, > return; > } > > - ics_write_xive(ics, nr, ics->irqs[nr - ics->offset].server, 0xff, > - ics->irqs[nr - ics->offset].priority); > + srcno = nr - ics->offset; > + ics_simple_write_xive(ics, srcno, ics->irqs[srcno].server, 0xff, > + ics->irqs[srcno].priority); > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > @@ -205,7 +208,7 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr, > uint32_t nret, target_ulong rets) > { > ICSState *ics = QLIST_FIRST(&spapr->xics->ics); > - uint32_t nr; > + uint32_t nr, srcno; > > if ((nargs != 1) || (nret != 1)) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -223,9 +226,10 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr, > return; > } > > - ics_write_xive(ics, nr, ics->irqs[nr - ics->offset].server, > - ics->irqs[nr - ics->offset].saved_priority, > - ics->irqs[nr - ics->offset].saved_priority); > + srcno = nr - ics->offset; > + ics_simple_write_xive(ics, srcno, ics->irqs[srcno].server, > + ics->irqs[srcno].saved_priority, > + ics->irqs[srcno].saved_priority); > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index a7a1e54..2231f2a 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -119,22 +119,29 @@ struct ICPState { > bool cap_irq_xics_enabled; > }; > > -#define TYPE_ICS "ics" > -#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) > +#define TYPE_ICS_BASE "ics-base" > +#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) > > -#define TYPE_KVM_ICS "icskvm" > -#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS) > +/* 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 TYPE_ICS_KVM "icskvm" > +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM) > > #define ICS_CLASS(klass) \ > - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS) > + OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_SIMPLE) > #define ICS_GET_CLASS(obj) \ > - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS) > + OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_SIMPLE) > > struct ICSStateClass { > DeviceClass parent_class; > > void (*pre_save)(ICSState *s); > int (*post_load)(ICSState *s, int version_id); > + void (*reject)(ICSState *s, uint32_t irq); > + void (*resend)(ICSState *s, int server); > + void (*eoi)(ICSState *s, uint32_t irq); > }; > > struct ICSState { > @@ -191,8 +198,8 @@ uint32_t icp_accept(ICPState *ss); > uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr); > void icp_eoi(XICSState *icp, int server, uint32_t xirr); > > -void ics_write_xive(ICSState *ics, int nr, int server, > - uint8_t priority, uint8_t saved_priority); > +void ics_simple_write_xive(ICSState *ics, int nr, int server, > + uint8_t priority, uint8_t saved_priority); > > void ics_set_irq_type(ICSState *ics, int srcno, bool lsi); > >
On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote: > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > The existing implementation remains same and ics-base is introduced. The > type name "ics" is retained, and all the related functions renamed as > ics_simple_* > > This will allow different implementations for the source controllers > such as the MSI support of PHB3 on Power8 which uses in-memory state > tables for example. A couple of issues below regarding the class helpers, > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > --- > hw/intc/trace-events | 10 ++-- > hw/intc/xics.c | 143 +++++++++++++++++++++++++++++++------------------- > hw/intc/xics_kvm.c | 10 ++-- > hw/intc/xics_spapr.c | 28 +++++----- > include/hw/ppc/xics.h | 23 +++++--- > 5 files changed, 131 insertions(+), 83 deletions(-) > > diff --git a/hw/intc/trace-events b/hw/intc/trace-events > index aa342a8..a367b46 100644 > --- a/hw/intc/trace-events > +++ b/hw/intc/trace-events > @@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr) "icp_accept: XIRR %#"PRIx3 > xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi: server %d given XIRR %#"PRIx32" new XIRR %#"PRIx32 > xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to deliver irq %#"PRIx32" priority %#x" > xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new XIRR=%#x new pending priority=%#x" > -xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]" > +xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]" > xics_masked_pending(void) "set_irq_msi: masked pending" > -xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]" > -xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x" > -xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]" > -xics_ics_eoi(int nr) "ics_eoi: irq %#x" > +xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]" > +xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x" > +xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]" > +xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x" > xics_alloc(int irq) "irq %d" > xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d irqs, lsi=%d, alignnum %d" > xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs" > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index c7901c4..b15751e 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics) > { > ICSState *ics; > > - ics = ICS(object_new(TYPE_ICS)); > + ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE)); > object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL); > ics->xics = xics; > QLIST_INSERT_HEAD(&xics->ics, ics, list); > @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = { > #define XISR(ss) (((ss)->xirr) & XISR_MASK) > #define CPPR(ss) (((ss)->xirr) >> 24) > > -static void ics_reject(ICSState *ics, int nr); > -static void ics_resend(ICSState *ics, int server); > -static void ics_eoi(ICSState *ics, int nr); > +static void ics_reject(ICSState *ics, uint32_t nr) > +{ > + ICSStateClass *k = ICS_GET_CLASS(ics); Shouldn't that be ICS_BASE_GET_CLASS() > + if (k->reject) { > + k->reject(ics, nr); > + } > +} > + > +static void ics_resend(ICSState *ics, int server) > +{ > + ICSStateClass *k = ICS_GET_CLASS(ics); here also > + > + if (k->resend) { > + k->resend(ics, server); > + } > +} > + > +static void ics_eoi(ICSState *ics, int nr) > +{ > + ICSStateClass *k = ICS_GET_CLASS(ics); and here. > + > + if (k->eoi) { > + k->eoi(ics, nr); > + } > +} > > static void icp_check_ipi(ICPState *ss) > { > @@ -462,7 +485,7 @@ static const TypeInfo icp_info = { > /* > * ICS: Source layer > */ > -static void resend_msi(ICSState *ics, int srcno) > +static void ics_simple_resend_msi(ICSState *ics, int srcno) > { > ICSIRQState *irq = ics->irqs + srcno; > > @@ -475,7 +498,7 @@ static void resend_msi(ICSState *ics, int srcno) > } > } > > -static void resend_lsi(ICSState *ics, int srcno) > +static void ics_simple_resend_lsi(ICSState *ics, int srcno) > { > ICSIRQState *irq = ics->irqs + srcno; > > @@ -487,11 +510,11 @@ static void resend_lsi(ICSState *ics, int srcno) > } > } > > -static void set_irq_msi(ICSState *ics, int srcno, int val) > +static void ics_simple_set_irq_msi(ICSState *ics, int srcno, int val) > { > ICSIRQState *irq = ics->irqs + srcno; > > - trace_xics_set_irq_msi(srcno, srcno + ics->offset); > + trace_xics_ics_simple_set_irq_msi(srcno, srcno + ics->offset); > > if (val) { > if (irq->priority == 0xff) { > @@ -503,31 +526,31 @@ static void set_irq_msi(ICSState *ics, int srcno, int val) > } > } > > -static void set_irq_lsi(ICSState *ics, int srcno, int val) > +static void ics_simple_set_irq_lsi(ICSState *ics, int srcno, int val) > { > ICSIRQState *irq = ics->irqs + srcno; > > - trace_xics_set_irq_lsi(srcno, srcno + ics->offset); > + trace_xics_ics_simple_set_irq_lsi(srcno, srcno + ics->offset); > if (val) { > irq->status |= XICS_STATUS_ASSERTED; > } else { > irq->status &= ~XICS_STATUS_ASSERTED; > } > - resend_lsi(ics, srcno); > + ics_simple_resend_lsi(ics, srcno); > } > > -static void ics_set_irq(void *opaque, int srcno, int val) > +static void ics_simple_set_irq(void *opaque, int srcno, int val) > { > ICSState *ics = (ICSState *)opaque; > > if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { > - set_irq_lsi(ics, srcno, val); > + ics_simple_set_irq_lsi(ics, srcno, val); > } else { > - set_irq_msi(ics, srcno, val); > + ics_simple_set_irq_msi(ics, srcno, val); > } > } > > -static void write_xive_msi(ICSState *ics, int srcno) > +static void ics_simple_write_xive_msi(ICSState *ics, int srcno) > { > ICSIRQState *irq = ics->irqs + srcno; > > @@ -540,35 +563,35 @@ static void write_xive_msi(ICSState *ics, int srcno) > icp_irq(ics, irq->server, srcno + ics->offset, irq->priority); > } > > -static void write_xive_lsi(ICSState *ics, int srcno) > +static void ics_simple_write_xive_lsi(ICSState *ics, int srcno) > { > - resend_lsi(ics, srcno); > + ics_simple_resend_lsi(ics, srcno); > } > > -void ics_write_xive(ICSState *ics, int nr, int server, > - uint8_t priority, uint8_t saved_priority) > +void ics_simple_write_xive(ICSState *ics, int srcno, int server, > + uint8_t priority, uint8_t saved_priority) > { > - int srcno = nr - ics->offset; > ICSIRQState *irq = ics->irqs + srcno; > > irq->server = server; > irq->priority = priority; > irq->saved_priority = saved_priority; > > - trace_xics_ics_write_xive(nr, srcno, server, priority); > + trace_xics_ics_simple_write_xive(ics->offset + srcno, srcno, server, > + priority); > > if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { > - write_xive_lsi(ics, srcno); > + ics_simple_write_xive_lsi(ics, srcno); > } else { > - write_xive_msi(ics, srcno); > + ics_simple_write_xive_msi(ics, srcno); > } > } > > -static void ics_reject(ICSState *ics, int nr) > +static void ics_simple_reject(ICSState *ics, uint32_t nr) > { > ICSIRQState *irq = ics->irqs + nr - ics->offset; > > - trace_xics_ics_reject(nr, nr - ics->offset); > + trace_xics_ics_simple_reject(nr, nr - ics->offset); > if (irq->flags & XICS_FLAGS_IRQ_MSI) { > irq->status |= XICS_STATUS_REJECTED; > } else if (irq->flags & XICS_FLAGS_IRQ_LSI) { > @@ -576,7 +599,7 @@ static void ics_reject(ICSState *ics, int nr) > } > } > > -static void ics_resend(ICSState *ics, int server) > +static void ics_simple_resend(ICSState *ics, int server) > { > int i; > ICPState *ss = ics->xics->ss + server; > @@ -594,28 +617,28 @@ static void ics_resend(ICSState *ics, int server) > qemu_irq_raise(ss->output); > continue; > } > - resend_lsi(ics, i); > + ics_simple_resend_lsi(ics, i); > } else { > - resend_msi(ics, i); > + ics_simple_resend_msi(ics, i); > } > } > } > > -static void ics_eoi(ICSState *ics, int nr) > +static void ics_simple_eoi(ICSState *ics, uint32_t nr) > { > int srcno = nr - ics->offset; > ICSIRQState *irq = ics->irqs + srcno; > > - trace_xics_ics_eoi(nr); > + trace_xics_ics_simple_eoi(nr); > > if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { > irq->status &= ~XICS_STATUS_SENT; > } > } > > -static void ics_reset(DeviceState *dev) > +static void ics_simple_reset(DeviceState *dev) > { > - ICSState *ics = ICS(dev); > + ICSState *ics = ICS_SIMPLE(dev); > int i; > uint8_t flags[ics->nr_irqs]; > > @@ -632,7 +655,7 @@ static void ics_reset(DeviceState *dev) > } > } > > -static void ics_dispatch_pre_save(void *opaque) > +static void ics_simple_dispatch_pre_save(void *opaque) > { > ICSState *ics = opaque; > ICSStateClass *info = ICS_GET_CLASS(ics); and here. > @@ -642,7 +665,7 @@ static void ics_dispatch_pre_save(void *opaque) > } > } > > -static int ics_dispatch_post_load(void *opaque, int version_id) > +static int ics_simple_dispatch_post_load(void *opaque, int version_id) > { > ICSState *ics = opaque; > ICSStateClass *info = ICS_GET_CLASS(ics); and here. > @@ -654,7 +677,7 @@ static int ics_dispatch_post_load(void *opaque, int version_id) > return 0; > } > > -static const VMStateDescription vmstate_ics_irq = { > +static const VMStateDescription vmstate_ics_simple_irq = { > .name = "ics/irq", > .version_id = 2, > .minimum_version_id = 1, > @@ -668,57 +691,70 @@ static const VMStateDescription vmstate_ics_irq = { > }, > }; > > -static const VMStateDescription vmstate_ics = { > +static const VMStateDescription vmstate_ics_simple = { > .name = "ics", > .version_id = 1, > .minimum_version_id = 1, > - .pre_save = ics_dispatch_pre_save, > - .post_load = ics_dispatch_post_load, > + .pre_save = ics_simple_dispatch_pre_save, > + .post_load = ics_simple_dispatch_post_load, > .fields = (VMStateField[]) { > /* Sanity check */ > VMSTATE_UINT32_EQUAL(nr_irqs, ICSState), > > VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs, > - vmstate_ics_irq, ICSIRQState), > + vmstate_ics_simple_irq, > + ICSIRQState), > VMSTATE_END_OF_LIST() > }, > }; > > -static void ics_initfn(Object *obj) > +static void ics_simple_initfn(Object *obj) > { > - ICSState *ics = ICS(obj); > + ICSState *ics = ICS_SIMPLE(obj); > > ics->offset = XICS_IRQ_BASE; > } > > -static void ics_realize(DeviceState *dev, Error **errp) > +static void ics_simple_realize(DeviceState *dev, Error **errp) > { > - ICSState *ics = ICS(dev); > + ICSState *ics = ICS_SIMPLE(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->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs); > + ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs); > } > > -static void ics_class_init(ObjectClass *klass, void *data) > +static void ics_simple_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > + ICSStateClass *isc = ICS_CLASS(klass); > > - dc->realize = ics_realize; > - dc->vmsd = &vmstate_ics; > - dc->reset = ics_reset; > + dc->realize = ics_simple_realize; > + dc->vmsd = &vmstate_ics_simple; > + dc->reset = ics_simple_reset; > + isc->reject = ics_simple_reject; > + isc->resend = ics_simple_resend; > + isc->eoi = ics_simple_eoi; > } > > -static const TypeInfo ics_info = { > - .name = TYPE_ICS, > +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), > + .instance_init = ics_simple_initfn, > +}; > + > +static const TypeInfo ics_base_info = { > + .name = TYPE_ICS_BASE, > .parent = TYPE_DEVICE, > + .abstract = true, > .instance_size = sizeof(ICSState), > - .class_init = ics_class_init, > .class_size = sizeof(ICSStateClass), > - .instance_init = ics_initfn, > }; > > /* > @@ -758,7 +794,8 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > static void xics_register_types(void) > { > type_register_static(&xics_common_info); > - type_register_static(&ics_info); > + type_register_static(&ics_simple_info); > + type_register_static(&ics_base_info); > type_register_static(&icp_info); > } > > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c > index 04fa7cb..89862df 100644 > --- a/hw/intc/xics_kvm.c > +++ b/hw/intc/xics_kvm.c > @@ -272,7 +272,7 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val) > > static void ics_kvm_reset(DeviceState *dev) > { > - ICSState *ics = ICS(dev); > + ICSState *ics = ICS_SIMPLE(dev); > int i; > uint8_t flags[ics->nr_irqs]; > > @@ -293,7 +293,7 @@ static void ics_kvm_reset(DeviceState *dev) > > static void ics_kvm_realize(DeviceState *dev, Error **errp) > { > - ICSState *ics = ICS(dev); > + ICSState *ics = ICS_SIMPLE(dev); > > if (!ics->nr_irqs) { > error_setg(errp, "Number of interrupts needs to be greater 0"); > @@ -315,8 +315,8 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data) and in ics_kvm_class_init also. > } > > static const TypeInfo ics_kvm_info = { > - .name = TYPE_KVM_ICS, > - .parent = TYPE_ICS, > + .name = TYPE_ICS_KVM, > + .parent = TYPE_ICS_SIMPLE, > .instance_size = sizeof(ICSState), > .class_init = ics_kvm_class_init, > }; > @@ -492,7 +492,7 @@ static void xics_kvm_initfn(Object *obj) > XICSState *xics = XICS_COMMON(obj); > ICSState *ics; > > - ics = ICS(object_new(TYPE_KVM_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); > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > index 270f20e..4dd1399 100644 > --- a/hw/intc/xics_spapr.c > +++ b/hw/intc/xics_spapr.c > @@ -114,7 +114,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, > uint32_t nret, target_ulong rets) > { > ICSState *ics = QLIST_FIRST(&spapr->xics->ics); > - uint32_t nr, server, priority; > + uint32_t nr, srcno, server, priority; > > if ((nargs != 3) || (nret != 1)) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -135,7 +135,8 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, > return; > } > > - ics_write_xive(ics, nr, server, priority, priority); > + srcno = nr - ics->offset; > + ics_simple_write_xive(ics, srcno, server, priority, priority); > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > @@ -146,7 +147,7 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, > uint32_t nret, target_ulong rets) > { > ICSState *ics = QLIST_FIRST(&spapr->xics->ics); > - uint32_t nr; > + uint32_t nr, srcno; > > if ((nargs != 1) || (nret != 3)) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -165,8 +166,9 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, > } > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > - rtas_st(rets, 1, ics->irqs[nr - ics->offset].server); > - rtas_st(rets, 2, ics->irqs[nr - ics->offset].priority); > + srcno = nr - ics->offset; > + rtas_st(rets, 1, ics->irqs[srcno].server); > + rtas_st(rets, 2, ics->irqs[srcno].priority); > } > > static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr, > @@ -175,7 +177,7 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr, > uint32_t nret, target_ulong rets) > { > ICSState *ics = QLIST_FIRST(&spapr->xics->ics); > - uint32_t nr; > + uint32_t nr, srcno; > > if ((nargs != 1) || (nret != 1)) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -193,8 +195,9 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr, > return; > } > > - ics_write_xive(ics, nr, ics->irqs[nr - ics->offset].server, 0xff, > - ics->irqs[nr - ics->offset].priority); > + srcno = nr - ics->offset; > + ics_simple_write_xive(ics, srcno, ics->irqs[srcno].server, 0xff, > + ics->irqs[srcno].priority); > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > @@ -205,7 +208,7 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr, > uint32_t nret, target_ulong rets) > { > ICSState *ics = QLIST_FIRST(&spapr->xics->ics); > - uint32_t nr; > + uint32_t nr, srcno; > > if ((nargs != 1) || (nret != 1)) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -223,9 +226,10 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr, > return; > } > > - ics_write_xive(ics, nr, ics->irqs[nr - ics->offset].server, > - ics->irqs[nr - ics->offset].saved_priority, > - ics->irqs[nr - ics->offset].saved_priority); > + srcno = nr - ics->offset; > + ics_simple_write_xive(ics, srcno, ics->irqs[srcno].server, > + ics->irqs[srcno].saved_priority, > + ics->irqs[srcno].saved_priority); > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index a7a1e54..2231f2a 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -119,22 +119,29 @@ struct ICPState { > bool cap_irq_xics_enabled; > }; > > -#define TYPE_ICS "ics" > -#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) > +#define TYPE_ICS_BASE "ics-base" > +#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) #define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE) > -#define TYPE_KVM_ICS "icskvm" > -#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS) > +/* 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 TYPE_ICS_KVM "icskvm" > +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM) > > #define ICS_CLASS(klass) \ > - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS) > + OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_SIMPLE) > #define ICS_GET_CLASS(obj) \ > - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS) > + OBJECT_GET_CLASS(ICSStateClass, (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) Cheers, C. > struct ICSStateClass { > DeviceClass parent_class; > > void (*pre_save)(ICSState *s); > int (*post_load)(ICSState *s, int version_id); > + void (*reject)(ICSState *s, uint32_t irq); > + void (*resend)(ICSState *s, int server); > + void (*eoi)(ICSState *s, uint32_t irq); > }; > > struct ICSState { > @@ -191,8 +198,8 @@ uint32_t icp_accept(ICPState *ss); > uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr); > void icp_eoi(XICSState *icp, int server, uint32_t xirr); > > -void ics_write_xive(ICSState *ics, int nr, int server, > - uint8_t priority, uint8_t saved_priority); > +void ics_simple_write_xive(ICSState *ics, int nr, int server, > + uint8_t priority, uint8_t saved_priority); > > void ics_set_irq_type(ICSState *ics, int srcno, bool lsi); > >
Cédric Le Goater <clg@kaod.org> writes: > On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote: >> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> >> The existing implementation remains same and ics-base is introduced. The >> type name "ics" is retained, and all the related functions renamed as >> ics_simple_* >> >> This will allow different implementations for the source controllers >> such as the MSI support of PHB3 on Power8 which uses in-memory state >> tables for example. >> >> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > > This patch had a Reviewed-by from David. Did you change anything > in v4 ? No changes, I missed adding reviewed-by in v4. Regards Nikunj
Cédric Le Goater <clg@kaod.org> writes: > On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote: >> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> >> The existing implementation remains same and ics-base is introduced. The >> type name "ics" is retained, and all the related functions renamed as >> ics_simple_* >> >> This will allow different implementations for the source controllers >> such as the MSI support of PHB3 on Power8 which uses in-memory state >> tables for example. > > A couple of issues below regarding the class helpers, > >> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >> --- >> hw/intc/trace-events | 10 ++-- >> hw/intc/xics.c | 143 +++++++++++++++++++++++++++++++------------------- >> hw/intc/xics_kvm.c | 10 ++-- >> hw/intc/xics_spapr.c | 28 +++++----- >> include/hw/ppc/xics.h | 23 +++++--- >> 5 files changed, 131 insertions(+), 83 deletions(-) >> >> diff --git a/hw/intc/trace-events b/hw/intc/trace-events >> index aa342a8..a367b46 100644 >> --- a/hw/intc/trace-events >> +++ b/hw/intc/trace-events >> @@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr) "icp_accept: XIRR %#"PRIx3 >> xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi: server %d given XIRR %#"PRIx32" new XIRR %#"PRIx32 >> xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to deliver irq %#"PRIx32" priority %#x" >> xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new XIRR=%#x new pending priority=%#x" >> -xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]" >> +xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]" >> xics_masked_pending(void) "set_irq_msi: masked pending" >> -xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]" >> -xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x" >> -xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]" >> -xics_ics_eoi(int nr) "ics_eoi: irq %#x" >> +xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]" >> +xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x" >> +xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]" >> +xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x" >> xics_alloc(int irq) "irq %d" >> xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d irqs, lsi=%d, alignnum %d" >> xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs" >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c >> index c7901c4..b15751e 100644 >> --- a/hw/intc/xics.c >> +++ b/hw/intc/xics.c >> @@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics) >> { >> ICSState *ics; >> >> - ics = ICS(object_new(TYPE_ICS)); >> + ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE)); >> object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL); >> ics->xics = xics; >> QLIST_INSERT_HEAD(&xics->ics, ics, list); >> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = { >> #define XISR(ss) (((ss)->xirr) & XISR_MASK) >> #define CPPR(ss) (((ss)->xirr) >> 24) >> >> -static void ics_reject(ICSState *ics, int nr); >> -static void ics_resend(ICSState *ics, int server); >> -static void ics_eoi(ICSState *ics, int nr); >> +static void ics_reject(ICSState *ics, uint32_t nr) >> +{ >> + ICSStateClass *k = ICS_GET_CLASS(ics); > > Shouldn't that be ICS_BASE_GET_CLASS() The class hierarchy is something like this: ICS_BASE -> ICS_SIMPLE -> ICS_KVM We have an instance init for ICS_SIMPLE where we set these pointers. >> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h >> index a7a1e54..2231f2a 100644 >> --- a/include/hw/ppc/xics.h >> +++ b/include/hw/ppc/xics.h >> @@ -119,22 +119,29 @@ struct ICPState { >> bool cap_irq_xics_enabled; >> }; >> >> -#define TYPE_ICS "ics" >> -#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) >> +#define TYPE_ICS_BASE "ics-base" >> +#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) > > #define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE) Yes, that is a bug. Will correct. > >> -#define TYPE_KVM_ICS "icskvm" >> -#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS) >> +/* 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 TYPE_ICS_KVM "icskvm" >> +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM) >> >> #define ICS_CLASS(klass) \ >> - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS) >> + OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_SIMPLE) >> #define ICS_GET_CLASS(obj) \ >> - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS) >> + OBJECT_GET_CLASS(ICSStateClass, (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) As explained above, not needed. Regards Nikunj
On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote: > Cédric Le Goater <clg@kaod.org> writes: > >> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote: >>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>> >>> The existing implementation remains same and ics-base is introduced. The >>> type name "ics" is retained, and all the related functions renamed as >>> ics_simple_* >>> >>> This will allow different implementations for the source controllers >>> such as the MSI support of PHB3 on Power8 which uses in-memory state >>> tables for example. >> >> A couple of issues below regarding the class helpers, >> >>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>> --- >>> hw/intc/trace-events | 10 ++-- >>> hw/intc/xics.c | 143 +++++++++++++++++++++++++++++++------------------- >>> hw/intc/xics_kvm.c | 10 ++-- >>> hw/intc/xics_spapr.c | 28 +++++----- >>> include/hw/ppc/xics.h | 23 +++++--- >>> 5 files changed, 131 insertions(+), 83 deletions(-) >>> >>> diff --git a/hw/intc/trace-events b/hw/intc/trace-events >>> index aa342a8..a367b46 100644 >>> --- a/hw/intc/trace-events >>> +++ b/hw/intc/trace-events >>> @@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr) "icp_accept: XIRR %#"PRIx3 >>> xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi: server %d given XIRR %#"PRIx32" new XIRR %#"PRIx32 >>> xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to deliver irq %#"PRIx32" priority %#x" >>> xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new XIRR=%#x new pending priority=%#x" >>> -xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]" >>> +xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]" >>> xics_masked_pending(void) "set_irq_msi: masked pending" >>> -xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]" >>> -xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x" >>> -xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]" >>> -xics_ics_eoi(int nr) "ics_eoi: irq %#x" >>> +xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]" >>> +xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x" >>> +xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]" >>> +xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x" >>> xics_alloc(int irq) "irq %d" >>> xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d irqs, lsi=%d, alignnum %d" >>> xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs" >>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c >>> index c7901c4..b15751e 100644 >>> --- a/hw/intc/xics.c >>> +++ b/hw/intc/xics.c >>> @@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics) >>> { >>> ICSState *ics; >>> >>> - ics = ICS(object_new(TYPE_ICS)); >>> + ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE)); >>> object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL); >>> ics->xics = xics; >>> QLIST_INSERT_HEAD(&xics->ics, ics, list); >>> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = { >>> #define XISR(ss) (((ss)->xirr) & XISR_MASK) >>> #define CPPR(ss) (((ss)->xirr) >> 24) >>> >>> -static void ics_reject(ICSState *ics, int nr); >>> -static void ics_resend(ICSState *ics, int server); >>> -static void ics_eoi(ICSState *ics, int nr); >>> +static void ics_reject(ICSState *ics, uint32_t nr) >>> +{ >>> + ICSStateClass *k = ICS_GET_CLASS(ics); >> >> Shouldn't that be ICS_BASE_GET_CLASS() > > The class hierarchy is something like this: > > ICS_BASE -> ICS_SIMPLE -> ICS_KVM yes. but if we called ics_* with an instance of an ics class which is not a ICS_SIMPLE class that will break. ICSStateClass is the baseclass, whenever we call methods on a ICSState* object, we should use the method defines in ICSStateClass. > We have an instance init for ICS_SIMPLE where we set these pointers. > >>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h >>> index a7a1e54..2231f2a 100644 >>> --- a/include/hw/ppc/xics.h >>> +++ b/include/hw/ppc/xics.h >>> @@ -119,22 +119,29 @@ struct ICPState { >>> bool cap_irq_xics_enabled; >>> }; >>> >>> -#define TYPE_ICS "ics" >>> -#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) >>> +#define TYPE_ICS_BASE "ics-base" >>> +#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) >> >> #define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE) > > Yes, that is a bug. Will correct. > >> >>> -#define TYPE_KVM_ICS "icskvm" >>> -#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS) >>> +/* 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 TYPE_ICS_KVM "icskvm" >>> +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM) >>> >>> #define ICS_CLASS(klass) \ >>> - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS) >>> + OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_SIMPLE) >>> #define ICS_GET_CLASS(obj) \ >>> - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS) >>> + OBJECT_GET_CLASS(ICSStateClass, (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) > > As explained above, not needed. We will with powernv which defines a new ics type for the phb3 msis. Thanks, C.
Cédric Le Goater <clg@kaod.org> writes: > On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote: >> Cédric Le Goater <clg@kaod.org> writes: >> >>> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote: >>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>> >>>> The existing implementation remains same and ics-base is introduced. The >>>> type name "ics" is retained, and all the related functions renamed as >>>> ics_simple_* >>>> >>>> This will allow different implementations for the source controllers >>>> such as the MSI support of PHB3 on Power8 which uses in-memory state >>>> tables for example. >>> >>> A couple of issues below regarding the class helpers, >>> >>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>> --- >>>> hw/intc/trace-events | 10 ++-- >>>> hw/intc/xics.c | 143 +++++++++++++++++++++++++++++++------------------- >>>> hw/intc/xics_kvm.c | 10 ++-- >>>> hw/intc/xics_spapr.c | 28 +++++----- >>>> include/hw/ppc/xics.h | 23 +++++--- >>>> 5 files changed, 131 insertions(+), 83 deletions(-) >>>> >>>> diff --git a/hw/intc/trace-events b/hw/intc/trace-events >>>> index aa342a8..a367b46 100644 >>>> --- a/hw/intc/trace-events >>>> +++ b/hw/intc/trace-events >>>> @@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr) "icp_accept: XIRR %#"PRIx3 >>>> xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi: server %d given XIRR %#"PRIx32" new XIRR %#"PRIx32 >>>> xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to deliver irq %#"PRIx32" priority %#x" >>>> xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new XIRR=%#x new pending priority=%#x" >>>> -xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]" >>>> +xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]" >>>> xics_masked_pending(void) "set_irq_msi: masked pending" >>>> -xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]" >>>> -xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x" >>>> -xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]" >>>> -xics_ics_eoi(int nr) "ics_eoi: irq %#x" >>>> +xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]" >>>> +xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x" >>>> +xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]" >>>> +xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x" >>>> xics_alloc(int irq) "irq %d" >>>> xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d irqs, lsi=%d, alignnum %d" >>>> xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs" >>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c >>>> index c7901c4..b15751e 100644 >>>> --- a/hw/intc/xics.c >>>> +++ b/hw/intc/xics.c >>>> @@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics) >>>> { >>>> ICSState *ics; >>>> >>>> - ics = ICS(object_new(TYPE_ICS)); >>>> + ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE)); >>>> object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL); >>>> ics->xics = xics; >>>> QLIST_INSERT_HEAD(&xics->ics, ics, list); >>>> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = { >>>> #define XISR(ss) (((ss)->xirr) & XISR_MASK) >>>> #define CPPR(ss) (((ss)->xirr) >> 24) >>>> >>>> -static void ics_reject(ICSState *ics, int nr); >>>> -static void ics_resend(ICSState *ics, int server); >>>> -static void ics_eoi(ICSState *ics, int nr); >>>> +static void ics_reject(ICSState *ics, uint32_t nr) >>>> +{ >>>> + ICSStateClass *k = ICS_GET_CLASS(ics); >>> >>> Shouldn't that be ICS_BASE_GET_CLASS() >> >> The class hierarchy is something like this: >> >> ICS_BASE -> ICS_SIMPLE -> ICS_KVM > > yes. but if we called ics_* with an instance of an ics class which is > not a ICS_SIMPLE class that will break. Correct > ICSStateClass is the baseclass, whenever we call methods on a ICSState* > object, we should use the method defines in ICSStateClass. Hmm, in that case I need to initialize base class methods in instance_init of ics_simple >> We have an instance init for ICS_SIMPLE where we set these pointers. >> >>>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h >>>> index a7a1e54..2231f2a 100644 >>>> --- a/include/hw/ppc/xics.h >>>> +++ b/include/hw/ppc/xics.h >>>> @@ -119,22 +119,29 @@ struct ICPState { >>>> bool cap_irq_xics_enabled; >>>> }; >>>> >>>> -#define TYPE_ICS "ics" >>>> -#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) >>>> +#define TYPE_ICS_BASE "ics-base" >>>> +#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) >>> >>> #define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE) >> >> Yes, that is a bug. Will correct. >> >>> >>>> -#define TYPE_KVM_ICS "icskvm" >>>> -#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS) >>>> +/* 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 TYPE_ICS_KVM "icskvm" >>>> +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM) >>>> >>>> #define ICS_CLASS(klass) \ >>>> - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS) >>>> + OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_SIMPLE) >>>> #define ICS_GET_CLASS(obj) \ >>>> - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS) >>>> + OBJECT_GET_CLASS(ICSStateClass, (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) >> >> As explained above, not needed. > > We will with powernv which defines a new ics type for the phb3 msis. Right, will change it. Regards Nikunj
On 09/20/2016 08:29 AM, Nikunj A Dadhania wrote: > Cédric Le Goater <clg@kaod.org> writes: > >> On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote: >>> Cédric Le Goater <clg@kaod.org> writes: >>> >>>> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote: >>>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>>> >>>>> The existing implementation remains same and ics-base is introduced. The >>>>> type name "ics" is retained, and all the related functions renamed as >>>>> ics_simple_* >>>>> >>>>> This will allow different implementations for the source controllers >>>>> such as the MSI support of PHB3 on Power8 which uses in-memory state >>>>> tables for example. >>>> >>>> A couple of issues below regarding the class helpers, >>>> >>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>> --- >>>>> hw/intc/trace-events | 10 ++-- >>>>> hw/intc/xics.c | 143 +++++++++++++++++++++++++++++++------------------- >>>>> hw/intc/xics_kvm.c | 10 ++-- >>>>> hw/intc/xics_spapr.c | 28 +++++----- >>>>> include/hw/ppc/xics.h | 23 +++++--- >>>>> 5 files changed, 131 insertions(+), 83 deletions(-) >>>>> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = { >>>>> #define XISR(ss) (((ss)->xirr) & XISR_MASK) >>>>> #define CPPR(ss) (((ss)->xirr) >> 24) >>>>> >>>>> -static void ics_reject(ICSState *ics, int nr); >>>>> -static void ics_resend(ICSState *ics, int server); >>>>> -static void ics_eoi(ICSState *ics, int nr); >>>>> +static void ics_reject(ICSState *ics, uint32_t nr) >>>>> +{ >>>>> + ICSStateClass *k = ICS_GET_CLASS(ics); >>>> >>>> Shouldn't that be ICS_BASE_GET_CLASS() >>> >>> The class hierarchy is something like this: >>> >>> ICS_BASE -> ICS_SIMPLE -> ICS_KVM >> >> yes. but if we called ics_* with an instance of an ics class which is >> not a ICS_SIMPLE class that will break. > > Correct > >> ICSStateClass is the baseclass, whenever we call methods on a ICSState* >> object, we should use the method defines in ICSStateClass. > > Hmm, in that case I need to initialize base class methods in > instance_init of ics_simple yes but this is done, no ? I see : 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->vmsd = &vmstate_ics_simple; dc->reset = ics_simple_reset; isc->post_load = ics_simple_post_load; isc->reject = ics_simple_reject; isc->resend = ics_simple_resend; isc->eoi = ics_simple_eoi; } Thanks, C.
Cédric Le Goater <clg@kaod.org> writes: > On 09/20/2016 08:29 AM, Nikunj A Dadhania wrote: >> Cédric Le Goater <clg@kaod.org> writes: >> >>> On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote: >>>> Cédric Le Goater <clg@kaod.org> writes: >>>> >>>>> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote: >>>>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>>>> >>>>>> The existing implementation remains same and ics-base is introduced. The >>>>>> type name "ics" is retained, and all the related functions renamed as >>>>>> ics_simple_* >>>>>> >>>>>> This will allow different implementations for the source controllers >>>>>> such as the MSI support of PHB3 on Power8 which uses in-memory state >>>>>> tables for example. >>>>> >>>>> A couple of issues below regarding the class helpers, >>>>> >>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>>> --- >>>>>> hw/intc/trace-events | 10 ++-- >>>>>> hw/intc/xics.c | 143 +++++++++++++++++++++++++++++++------------------- >>>>>> hw/intc/xics_kvm.c | 10 ++-- >>>>>> hw/intc/xics_spapr.c | 28 +++++----- >>>>>> include/hw/ppc/xics.h | 23 +++++--- >>>>>> 5 files changed, 131 insertions(+), 83 deletions(-) > >>>>>> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = { >>>>>> #define XISR(ss) (((ss)->xirr) & XISR_MASK) >>>>>> #define CPPR(ss) (((ss)->xirr) >> 24) >>>>>> >>>>>> -static void ics_reject(ICSState *ics, int nr); >>>>>> -static void ics_resend(ICSState *ics, int server); >>>>>> -static void ics_eoi(ICSState *ics, int nr); >>>>>> +static void ics_reject(ICSState *ics, uint32_t nr) >>>>>> +{ >>>>>> + ICSStateClass *k = ICS_GET_CLASS(ics); >>>>> >>>>> Shouldn't that be ICS_BASE_GET_CLASS() >>>> >>>> The class hierarchy is something like this: >>>> >>>> ICS_BASE -> ICS_SIMPLE -> ICS_KVM >>> >>> yes. but if we called ics_* with an instance of an ics class which is >>> not a ICS_SIMPLE class that will break. >> >> Correct >> >>> ICSStateClass is the baseclass, whenever we call methods on a ICSState* >>> object, we should use the method defines in ICSStateClass. >> >> Hmm, in that case I need to initialize base class methods in >> instance_init of ics_simple > > yes but this is done, no ? I see : > > static void ics_simple_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > ICSStateClass *isc = ICS_BASE_CLASS(klass); Right. Currently, we have this: + ICSStateClass *isc = ICS_CLASS(klass); > > dc->realize = ics_simple_realize; > dc->vmsd = &vmstate_ics_simple; > dc->reset = ics_simple_reset; > isc->post_load = ics_simple_post_load; > isc->reject = ics_simple_reject; > isc->resend = ics_simple_resend; > isc->eoi = ics_simple_eoi; Regards Nikunj
On 09/20/2016 10:10 AM, Nikunj A Dadhania wrote: > Cédric Le Goater <clg@kaod.org> writes: > >> On 09/20/2016 08:29 AM, Nikunj A Dadhania wrote: >>> Cédric Le Goater <clg@kaod.org> writes: >>> >>>> On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote: >>>>> Cédric Le Goater <clg@kaod.org> writes: >>>>> >>>>>> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote: >>>>>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>>>>> >>>>>>> The existing implementation remains same and ics-base is introduced. The >>>>>>> type name "ics" is retained, and all the related functions renamed as >>>>>>> ics_simple_* >>>>>>> >>>>>>> This will allow different implementations for the source controllers >>>>>>> such as the MSI support of PHB3 on Power8 which uses in-memory state >>>>>>> tables for example. >>>>>> >>>>>> A couple of issues below regarding the class helpers, >>>>>> >>>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>>>> --- >>>>>>> hw/intc/trace-events | 10 ++-- >>>>>>> hw/intc/xics.c | 143 +++++++++++++++++++++++++++++++------------------- >>>>>>> hw/intc/xics_kvm.c | 10 ++-- >>>>>>> hw/intc/xics_spapr.c | 28 +++++----- >>>>>>> include/hw/ppc/xics.h | 23 +++++--- >>>>>>> 5 files changed, 131 insertions(+), 83 deletions(-) >> >>>>>>> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = { >>>>>>> #define XISR(ss) (((ss)->xirr) & XISR_MASK) >>>>>>> #define CPPR(ss) (((ss)->xirr) >> 24) >>>>>>> >>>>>>> -static void ics_reject(ICSState *ics, int nr); >>>>>>> -static void ics_resend(ICSState *ics, int server); >>>>>>> -static void ics_eoi(ICSState *ics, int nr); >>>>>>> +static void ics_reject(ICSState *ics, uint32_t nr) >>>>>>> +{ >>>>>>> + ICSStateClass *k = ICS_GET_CLASS(ics); >>>>>> >>>>>> Shouldn't that be ICS_BASE_GET_CLASS() >>>>> >>>>> The class hierarchy is something like this: >>>>> >>>>> ICS_BASE -> ICS_SIMPLE -> ICS_KVM >>>> >>>> yes. but if we called ics_* with an instance of an ics class which is >>>> not a ICS_SIMPLE class that will break. >>> >>> Correct >>> >>>> ICSStateClass is the baseclass, whenever we call methods on a ICSState* >>>> object, we should use the method defines in ICSStateClass. >>> >>> Hmm, in that case I need to initialize base class methods in >>> instance_init of ics_simple >> >> yes but this is done, no ? I see : >> >> static void ics_simple_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> ICSStateClass *isc = ICS_BASE_CLASS(klass); > > Right. > > Currently, we have this: > > + ICSStateClass *isc = ICS_CLASS(klass); oh yes and same in the kvm ICS. The name are bit confusing as we are introducing SIMPLE but not the associated macros. You can check on my 2.8 github branch, I got something working there : https://github.com/legoater/qemu/commit/d0b492707debc7192f0860e2c5fc2daa9fd453ac Thanks, C.
Cédric Le Goater <clg@kaod.org> writes: > On 09/20/2016 10:10 AM, Nikunj A Dadhania wrote: >>>>>>>> -static void ics_reject(ICSState *ics, int nr); >>>>>>>> -static void ics_resend(ICSState *ics, int server); >>>>>>>> -static void ics_eoi(ICSState *ics, int nr); >>>>>>>> +static void ics_reject(ICSState *ics, uint32_t nr) >>>>>>>> +{ >>>>>>>> + ICSStateClass *k = ICS_GET_CLASS(ics); >>>>>>> >>>>>>> Shouldn't that be ICS_BASE_GET_CLASS() >>>>>> >>>>>> The class hierarchy is something like this: >>>>>> >>>>>> ICS_BASE -> ICS_SIMPLE -> ICS_KVM >>>>> >>>>> yes. but if we called ics_* with an instance of an ics class which is >>>>> not a ICS_SIMPLE class that will break. >>>> >>>> Correct >>>> >>>>> ICSStateClass is the baseclass, whenever we call methods on a ICSState* >>>>> object, we should use the method defines in ICSStateClass. >>>> >>>> Hmm, in that case I need to initialize base class methods in >>>> instance_init of ics_simple >>> >>> yes but this is done, no ? I see : >>> >>> static void ics_simple_class_init(ObjectClass *klass, void *data) >>> { >>> DeviceClass *dc = DEVICE_CLASS(klass); >>> ICSStateClass *isc = ICS_BASE_CLASS(klass); >> >> Right. >> >> Currently, we have this: >> >> + ICSStateClass *isc = ICS_CLASS(klass); > > oh yes and same in the kvm ICS. The name are bit confusing as we are > introducing SIMPLE but not the associated macros. > > You can check on my 2.8 github branch, I got something working there : > > https://github.com/legoater/qemu/commit/d0b492707debc7192f0860e2c5fc2daa9fd453ac Cool. :-) Thanks for testing. Regards, Nikunj
On 09/20/2016 11:41 AM, Nikunj A Dadhania wrote: > Cédric Le Goater <clg@kaod.org> writes: >> On 09/20/2016 10:10 AM, Nikunj A Dadhania wrote: >>>>>>>>> -static void ics_reject(ICSState *ics, int nr); >>>>>>>>> -static void ics_resend(ICSState *ics, int server); >>>>>>>>> -static void ics_eoi(ICSState *ics, int nr); >>>>>>>>> +static void ics_reject(ICSState *ics, uint32_t nr) >>>>>>>>> +{ >>>>>>>>> + ICSStateClass *k = ICS_GET_CLASS(ics); >>>>>>>> >>>>>>>> Shouldn't that be ICS_BASE_GET_CLASS() >>>>>>> >>>>>>> The class hierarchy is something like this: >>>>>>> >>>>>>> ICS_BASE -> ICS_SIMPLE -> ICS_KVM >>>>>> >>>>>> yes. but if we called ics_* with an instance of an ics class which is >>>>>> not a ICS_SIMPLE class that will break. >>>>> >>>>> Correct >>>>> >>>>>> ICSStateClass is the baseclass, whenever we call methods on a ICSState* >>>>>> object, we should use the method defines in ICSStateClass. >>>>> >>>>> Hmm, in that case I need to initialize base class methods in >>>>> instance_init of ics_simple >>>> >>>> yes but this is done, no ? I see : >>>> >>>> static void ics_simple_class_init(ObjectClass *klass, void *data) >>>> { >>>> DeviceClass *dc = DEVICE_CLASS(klass); >>>> ICSStateClass *isc = ICS_BASE_CLASS(klass); >>> >>> Right. >>> >>> Currently, we have this: >>> >>> + ICSStateClass *isc = ICS_CLASS(klass); >> >> oh yes and same in the kvm ICS. The name are bit confusing as we are >> introducing SIMPLE but not the associated macros. >> >> You can check on my 2.8 github branch, I got something working there : >> >> https://github.com/legoater/qemu/commit/d0b492707debc7192f0860e2c5fc2daa9fd453ac > > Cool. :-) > > Thanks for testing. I did some tests for pnv, pseries tcg and kvm. It didn't see any migration issues, yet. Do you have some scenario to reproduce the fix you are trying to add ? That's under tcg with a e1000 ? Thanks, C.
Cédric Le Goater <clg@kaod.org> writes: > On 09/20/2016 11:41 AM, Nikunj A Dadhania wrote: >> Cédric Le Goater <clg@kaod.org> writes: >>> On 09/20/2016 10:10 AM, Nikunj A Dadhania wrote: >>>>>>>>>> -static void ics_reject(ICSState *ics, int nr); >>>>>>>>>> -static void ics_resend(ICSState *ics, int server); >>>>>>>>>> -static void ics_eoi(ICSState *ics, int nr); >>>>>>>>>> +static void ics_reject(ICSState *ics, uint32_t nr) >>>>>>>>>> +{ >>>>>>>>>> + ICSStateClass *k = ICS_GET_CLASS(ics); >>>>>>>>> >>>>>>>>> Shouldn't that be ICS_BASE_GET_CLASS() >>>>>>>> >>>>>>>> The class hierarchy is something like this: >>>>>>>> >>>>>>>> ICS_BASE -> ICS_SIMPLE -> ICS_KVM >>>>>>> >>>>>>> yes. but if we called ics_* with an instance of an ics class which is >>>>>>> not a ICS_SIMPLE class that will break. >>>>>> >>>>>> Correct >>>>>> >>>>>>> ICSStateClass is the baseclass, whenever we call methods on a ICSState* >>>>>>> object, we should use the method defines in ICSStateClass. >>>>>> >>>>>> Hmm, in that case I need to initialize base class methods in >>>>>> instance_init of ics_simple >>>>> >>>>> yes but this is done, no ? I see : >>>>> >>>>> static void ics_simple_class_init(ObjectClass *klass, void *data) >>>>> { >>>>> DeviceClass *dc = DEVICE_CLASS(klass); >>>>> ICSStateClass *isc = ICS_BASE_CLASS(klass); >>>> >>>> Right. >>>> >>>> Currently, we have this: >>>> >>>> + ICSStateClass *isc = ICS_CLASS(klass); >>> >>> oh yes and same in the kvm ICS. The name are bit confusing as we are >>> introducing SIMPLE but not the associated macros. >>> >>> You can check on my 2.8 github branch, I got something working there : >>> >>> https://github.com/legoater/qemu/commit/d0b492707debc7192f0860e2c5fc2daa9fd453ac >> >> Cool. :-) >> >> Thanks for testing. > > I did some tests for pnv, pseries tcg and kvm. > > It didn't see any migration issues, yet. Do you have some scenario > to reproduce the fix you are trying to add ? That's under tcg with > a e1000 ? Oh that is pretty complex: Here is how I was reproducing the issue, it does not reproduce always, but at least 1 in 3 times, and sometimes when lucky 3 in 3 times as well. * Start a guest with single cpu, kernel-irqchip=off, e1000 card * From the guest start a flood ping (ssh to guest) * From the guest console trigger xmon * Migrate to the target (localhost) * Once the vm migrates to the target, exit xmon * In success case, i get the console, and the flood ping continues * In case of failure, console does not respond and within few seconds I get a crash Regards, Nikunj
diff --git a/hw/intc/trace-events b/hw/intc/trace-events index aa342a8..a367b46 100644 --- a/hw/intc/trace-events +++ b/hw/intc/trace-events @@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr) "icp_accept: XIRR %#"PRIx3 xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi: server %d given XIRR %#"PRIx32" new XIRR %#"PRIx32 xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to deliver irq %#"PRIx32" priority %#x" xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new XIRR=%#x new pending priority=%#x" -xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]" +xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]" xics_masked_pending(void) "set_irq_msi: masked pending" -xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]" -xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x" -xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]" -xics_ics_eoi(int nr) "ics_eoi: irq %#x" +xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]" +xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x" +xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]" +xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x" xics_alloc(int irq) "irq %d" xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d irqs, lsi=%d, alignnum %d" xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs" diff --git a/hw/intc/xics.c b/hw/intc/xics.c index c7901c4..b15751e 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics) { ICSState *ics; - ics = ICS(object_new(TYPE_ICS)); + ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE)); object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL); ics->xics = xics; QLIST_INSERT_HEAD(&xics->ics, ics, list); @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = { #define XISR(ss) (((ss)->xirr) & XISR_MASK) #define CPPR(ss) (((ss)->xirr) >> 24) -static void ics_reject(ICSState *ics, int nr); -static void ics_resend(ICSState *ics, int server); -static void ics_eoi(ICSState *ics, int nr); +static void ics_reject(ICSState *ics, uint32_t nr) +{ + ICSStateClass *k = ICS_GET_CLASS(ics); + + if (k->reject) { + k->reject(ics, nr); + } +} + +static void ics_resend(ICSState *ics, int server) +{ + ICSStateClass *k = ICS_GET_CLASS(ics); + + if (k->resend) { + k->resend(ics, server); + } +} + +static void ics_eoi(ICSState *ics, int nr) +{ + ICSStateClass *k = ICS_GET_CLASS(ics); + + if (k->eoi) { + k->eoi(ics, nr); + } +} static void icp_check_ipi(ICPState *ss) { @@ -462,7 +485,7 @@ static const TypeInfo icp_info = { /* * ICS: Source layer */ -static void resend_msi(ICSState *ics, int srcno) +static void ics_simple_resend_msi(ICSState *ics, int srcno) { ICSIRQState *irq = ics->irqs + srcno; @@ -475,7 +498,7 @@ static void resend_msi(ICSState *ics, int srcno) } } -static void resend_lsi(ICSState *ics, int srcno) +static void ics_simple_resend_lsi(ICSState *ics, int srcno) { ICSIRQState *irq = ics->irqs + srcno; @@ -487,11 +510,11 @@ static void resend_lsi(ICSState *ics, int srcno) } } -static void set_irq_msi(ICSState *ics, int srcno, int val) +static void ics_simple_set_irq_msi(ICSState *ics, int srcno, int val) { ICSIRQState *irq = ics->irqs + srcno; - trace_xics_set_irq_msi(srcno, srcno + ics->offset); + trace_xics_ics_simple_set_irq_msi(srcno, srcno + ics->offset); if (val) { if (irq->priority == 0xff) { @@ -503,31 +526,31 @@ static void set_irq_msi(ICSState *ics, int srcno, int val) } } -static void set_irq_lsi(ICSState *ics, int srcno, int val) +static void ics_simple_set_irq_lsi(ICSState *ics, int srcno, int val) { ICSIRQState *irq = ics->irqs + srcno; - trace_xics_set_irq_lsi(srcno, srcno + ics->offset); + trace_xics_ics_simple_set_irq_lsi(srcno, srcno + ics->offset); if (val) { irq->status |= XICS_STATUS_ASSERTED; } else { irq->status &= ~XICS_STATUS_ASSERTED; } - resend_lsi(ics, srcno); + ics_simple_resend_lsi(ics, srcno); } -static void ics_set_irq(void *opaque, int srcno, int val) +static void ics_simple_set_irq(void *opaque, int srcno, int val) { ICSState *ics = (ICSState *)opaque; if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { - set_irq_lsi(ics, srcno, val); + ics_simple_set_irq_lsi(ics, srcno, val); } else { - set_irq_msi(ics, srcno, val); + ics_simple_set_irq_msi(ics, srcno, val); } } -static void write_xive_msi(ICSState *ics, int srcno) +static void ics_simple_write_xive_msi(ICSState *ics, int srcno) { ICSIRQState *irq = ics->irqs + srcno; @@ -540,35 +563,35 @@ static void write_xive_msi(ICSState *ics, int srcno) icp_irq(ics, irq->server, srcno + ics->offset, irq->priority); } -static void write_xive_lsi(ICSState *ics, int srcno) +static void ics_simple_write_xive_lsi(ICSState *ics, int srcno) { - resend_lsi(ics, srcno); + ics_simple_resend_lsi(ics, srcno); } -void ics_write_xive(ICSState *ics, int nr, int server, - uint8_t priority, uint8_t saved_priority) +void ics_simple_write_xive(ICSState *ics, int srcno, int server, + uint8_t priority, uint8_t saved_priority) { - int srcno = nr - ics->offset; ICSIRQState *irq = ics->irqs + srcno; irq->server = server; irq->priority = priority; irq->saved_priority = saved_priority; - trace_xics_ics_write_xive(nr, srcno, server, priority); + trace_xics_ics_simple_write_xive(ics->offset + srcno, srcno, server, + priority); if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { - write_xive_lsi(ics, srcno); + ics_simple_write_xive_lsi(ics, srcno); } else { - write_xive_msi(ics, srcno); + ics_simple_write_xive_msi(ics, srcno); } } -static void ics_reject(ICSState *ics, int nr) +static void ics_simple_reject(ICSState *ics, uint32_t nr) { ICSIRQState *irq = ics->irqs + nr - ics->offset; - trace_xics_ics_reject(nr, nr - ics->offset); + trace_xics_ics_simple_reject(nr, nr - ics->offset); if (irq->flags & XICS_FLAGS_IRQ_MSI) { irq->status |= XICS_STATUS_REJECTED; } else if (irq->flags & XICS_FLAGS_IRQ_LSI) { @@ -576,7 +599,7 @@ static void ics_reject(ICSState *ics, int nr) } } -static void ics_resend(ICSState *ics, int server) +static void ics_simple_resend(ICSState *ics, int server) { int i; ICPState *ss = ics->xics->ss + server; @@ -594,28 +617,28 @@ static void ics_resend(ICSState *ics, int server) qemu_irq_raise(ss->output); continue; } - resend_lsi(ics, i); + ics_simple_resend_lsi(ics, i); } else { - resend_msi(ics, i); + ics_simple_resend_msi(ics, i); } } } -static void ics_eoi(ICSState *ics, int nr) +static void ics_simple_eoi(ICSState *ics, uint32_t nr) { int srcno = nr - ics->offset; ICSIRQState *irq = ics->irqs + srcno; - trace_xics_ics_eoi(nr); + trace_xics_ics_simple_eoi(nr); if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { irq->status &= ~XICS_STATUS_SENT; } } -static void ics_reset(DeviceState *dev) +static void ics_simple_reset(DeviceState *dev) { - ICSState *ics = ICS(dev); + ICSState *ics = ICS_SIMPLE(dev); int i; uint8_t flags[ics->nr_irqs]; @@ -632,7 +655,7 @@ static void ics_reset(DeviceState *dev) } } -static void ics_dispatch_pre_save(void *opaque) +static void ics_simple_dispatch_pre_save(void *opaque) { ICSState *ics = opaque; ICSStateClass *info = ICS_GET_CLASS(ics); @@ -642,7 +665,7 @@ static void ics_dispatch_pre_save(void *opaque) } } -static int ics_dispatch_post_load(void *opaque, int version_id) +static int ics_simple_dispatch_post_load(void *opaque, int version_id) { ICSState *ics = opaque; ICSStateClass *info = ICS_GET_CLASS(ics); @@ -654,7 +677,7 @@ static int ics_dispatch_post_load(void *opaque, int version_id) return 0; } -static const VMStateDescription vmstate_ics_irq = { +static const VMStateDescription vmstate_ics_simple_irq = { .name = "ics/irq", .version_id = 2, .minimum_version_id = 1, @@ -668,57 +691,70 @@ static const VMStateDescription vmstate_ics_irq = { }, }; -static const VMStateDescription vmstate_ics = { +static const VMStateDescription vmstate_ics_simple = { .name = "ics", .version_id = 1, .minimum_version_id = 1, - .pre_save = ics_dispatch_pre_save, - .post_load = ics_dispatch_post_load, + .pre_save = ics_simple_dispatch_pre_save, + .post_load = ics_simple_dispatch_post_load, .fields = (VMStateField[]) { /* Sanity check */ VMSTATE_UINT32_EQUAL(nr_irqs, ICSState), VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs, - vmstate_ics_irq, ICSIRQState), + vmstate_ics_simple_irq, + ICSIRQState), VMSTATE_END_OF_LIST() }, }; -static void ics_initfn(Object *obj) +static void ics_simple_initfn(Object *obj) { - ICSState *ics = ICS(obj); + ICSState *ics = ICS_SIMPLE(obj); ics->offset = XICS_IRQ_BASE; } -static void ics_realize(DeviceState *dev, Error **errp) +static void ics_simple_realize(DeviceState *dev, Error **errp) { - ICSState *ics = ICS(dev); + ICSState *ics = ICS_SIMPLE(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->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs); + ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs); } -static void ics_class_init(ObjectClass *klass, void *data) +static void ics_simple_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); + ICSStateClass *isc = ICS_CLASS(klass); - dc->realize = ics_realize; - dc->vmsd = &vmstate_ics; - dc->reset = ics_reset; + dc->realize = ics_simple_realize; + dc->vmsd = &vmstate_ics_simple; + dc->reset = ics_simple_reset; + isc->reject = ics_simple_reject; + isc->resend = ics_simple_resend; + isc->eoi = ics_simple_eoi; } -static const TypeInfo ics_info = { - .name = TYPE_ICS, +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), + .instance_init = ics_simple_initfn, +}; + +static const TypeInfo ics_base_info = { + .name = TYPE_ICS_BASE, .parent = TYPE_DEVICE, + .abstract = true, .instance_size = sizeof(ICSState), - .class_init = ics_class_init, .class_size = sizeof(ICSStateClass), - .instance_init = ics_initfn, }; /* @@ -758,7 +794,8 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) static void xics_register_types(void) { type_register_static(&xics_common_info); - type_register_static(&ics_info); + type_register_static(&ics_simple_info); + type_register_static(&ics_base_info); type_register_static(&icp_info); } diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c index 04fa7cb..89862df 100644 --- a/hw/intc/xics_kvm.c +++ b/hw/intc/xics_kvm.c @@ -272,7 +272,7 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val) static void ics_kvm_reset(DeviceState *dev) { - ICSState *ics = ICS(dev); + ICSState *ics = ICS_SIMPLE(dev); int i; uint8_t flags[ics->nr_irqs]; @@ -293,7 +293,7 @@ static void ics_kvm_reset(DeviceState *dev) static void ics_kvm_realize(DeviceState *dev, Error **errp) { - ICSState *ics = ICS(dev); + ICSState *ics = ICS_SIMPLE(dev); if (!ics->nr_irqs) { error_setg(errp, "Number of interrupts needs to be greater 0"); @@ -315,8 +315,8 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data) } static const TypeInfo ics_kvm_info = { - .name = TYPE_KVM_ICS, - .parent = TYPE_ICS, + .name = TYPE_ICS_KVM, + .parent = TYPE_ICS_SIMPLE, .instance_size = sizeof(ICSState), .class_init = ics_kvm_class_init, }; @@ -492,7 +492,7 @@ static void xics_kvm_initfn(Object *obj) XICSState *xics = XICS_COMMON(obj); ICSState *ics; - ics = ICS(object_new(TYPE_KVM_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); diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c index 270f20e..4dd1399 100644 --- a/hw/intc/xics_spapr.c +++ b/hw/intc/xics_spapr.c @@ -114,7 +114,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, uint32_t nret, target_ulong rets) { ICSState *ics = QLIST_FIRST(&spapr->xics->ics); - uint32_t nr, server, priority; + uint32_t nr, srcno, server, priority; if ((nargs != 3) || (nret != 1)) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); @@ -135,7 +135,8 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, return; } - ics_write_xive(ics, nr, server, priority, priority); + srcno = nr - ics->offset; + ics_simple_write_xive(ics, srcno, server, priority, priority); rtas_st(rets, 0, RTAS_OUT_SUCCESS); } @@ -146,7 +147,7 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, uint32_t nret, target_ulong rets) { ICSState *ics = QLIST_FIRST(&spapr->xics->ics); - uint32_t nr; + uint32_t nr, srcno; if ((nargs != 1) || (nret != 3)) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); @@ -165,8 +166,9 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, } rtas_st(rets, 0, RTAS_OUT_SUCCESS); - rtas_st(rets, 1, ics->irqs[nr - ics->offset].server); - rtas_st(rets, 2, ics->irqs[nr - ics->offset].priority); + srcno = nr - ics->offset; + rtas_st(rets, 1, ics->irqs[srcno].server); + rtas_st(rets, 2, ics->irqs[srcno].priority); } static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr, @@ -175,7 +177,7 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr, uint32_t nret, target_ulong rets) { ICSState *ics = QLIST_FIRST(&spapr->xics->ics); - uint32_t nr; + uint32_t nr, srcno; if ((nargs != 1) || (nret != 1)) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); @@ -193,8 +195,9 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr, return; } - ics_write_xive(ics, nr, ics->irqs[nr - ics->offset].server, 0xff, - ics->irqs[nr - ics->offset].priority); + srcno = nr - ics->offset; + ics_simple_write_xive(ics, srcno, ics->irqs[srcno].server, 0xff, + ics->irqs[srcno].priority); rtas_st(rets, 0, RTAS_OUT_SUCCESS); } @@ -205,7 +208,7 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr, uint32_t nret, target_ulong rets) { ICSState *ics = QLIST_FIRST(&spapr->xics->ics); - uint32_t nr; + uint32_t nr, srcno; if ((nargs != 1) || (nret != 1)) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); @@ -223,9 +226,10 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr, return; } - ics_write_xive(ics, nr, ics->irqs[nr - ics->offset].server, - ics->irqs[nr - ics->offset].saved_priority, - ics->irqs[nr - ics->offset].saved_priority); + srcno = nr - ics->offset; + ics_simple_write_xive(ics, srcno, ics->irqs[srcno].server, + ics->irqs[srcno].saved_priority, + ics->irqs[srcno].saved_priority); rtas_st(rets, 0, RTAS_OUT_SUCCESS); } diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h index a7a1e54..2231f2a 100644 --- a/include/hw/ppc/xics.h +++ b/include/hw/ppc/xics.h @@ -119,22 +119,29 @@ struct ICPState { bool cap_irq_xics_enabled; }; -#define TYPE_ICS "ics" -#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) +#define TYPE_ICS_BASE "ics-base" +#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) -#define TYPE_KVM_ICS "icskvm" -#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS) +/* 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 TYPE_ICS_KVM "icskvm" +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM) #define ICS_CLASS(klass) \ - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS) + OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_SIMPLE) #define ICS_GET_CLASS(obj) \ - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS) + OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_SIMPLE) struct ICSStateClass { DeviceClass parent_class; void (*pre_save)(ICSState *s); int (*post_load)(ICSState *s, int version_id); + void (*reject)(ICSState *s, uint32_t irq); + void (*resend)(ICSState *s, int server); + void (*eoi)(ICSState *s, uint32_t irq); }; struct ICSState { @@ -191,8 +198,8 @@ uint32_t icp_accept(ICPState *ss); uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr); void icp_eoi(XICSState *icp, int server, uint32_t xirr); -void ics_write_xive(ICSState *ics, int nr, int server, - uint8_t priority, uint8_t saved_priority); +void ics_simple_write_xive(ICSState *ics, int nr, int server, + uint8_t priority, uint8_t saved_priority); void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);