| Message ID | 1466704050-15108-10-git-send-email-nikunj@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jun 23, 2016 at 11:17:28PM +0530, Nikunj A Dadhania wrote: > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > The existing implementation remains same and ics-base is introduced. > > 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> > --- > hw/intc/xics.c | 101 +++++++++++++++++++++++++++++++++----------------- > hw/intc/xics_spapr.c | 36 ++++++++++-------- > include/hw/ppc/xics.h | 11 +++++- > 3 files changed, 97 insertions(+), 51 deletions(-) > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 326d21f..e2aa48d 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -220,9 +220,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); > -static void ics_eoi(ICSState *ics, int nr); > +static void ics_base_reject(ICSState *ics, uint32_t nr) AFICT these will actually work for any of the derived classes, since they call the function pointer. So I thin the original name was better than ics_base_*(). > +{ > + ICSStateClass *k = ICS_GET_CLASS(ics); > + > + if (k->reject) { > + k->reject(ics, nr); > + } > +} > + > +static void ics_base_resend(ICSState *ics) > +{ > + ICSStateClass *k = ICS_GET_CLASS(ics); > + > + if (k->resend) { > + k->resend(ics); > + } > +} > + > +static void ics_base_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, int server) > { > @@ -233,7 +256,7 @@ static void icp_check_ipi(ICPState *ss, int server) > trace_xics_icp_check_ipi(server, ss->mfrr); > > if (XISR(ss) && ss->xirr_owner) { > - ics_reject(ss->xirr_owner, XISR(ss)); > + ics_base_reject(ss->xirr_owner, XISR(ss)); > } > > ss->xirr = (ss->xirr & ~XISR_MASK) | XICS_IPI; > @@ -251,7 +274,7 @@ static void icp_resend(XICSState *xics, int server) > icp_check_ipi(ss, server); > } > QLIST_FOREACH(ics, &xics->ics, list) { > - ics_resend(ics); > + ics_base_resend(ics); > } > } > > @@ -271,7 +294,7 @@ void icp_set_cppr(XICSState *xics, int server, uint8_t cppr) > ss->pending_priority = 0xff; > qemu_irq_lower(ss->output); > if (ss->xirr_owner) { > - ics_reject(ss->xirr_owner, old_xisr); > + ics_base_reject(ss->xirr_owner, old_xisr); > ss->xirr_owner = NULL; > } > } > @@ -325,8 +348,8 @@ void icp_eoi(XICSState *xics, int server, uint32_t xirr) > trace_xics_icp_eoi(server, xirr, ss->xirr); > irq = xirr & XISR_MASK; > QLIST_FOREACH(ics, &xics->ics, list) { > - if (ics_valid_irq(ics, irq)) { > - ics_eoi(ics, irq); > + if (ics_base_valid_irq(ics, irq)) { > + ics_base_eoi(ics, irq); > } > } > if (!XISR(ss)) { > @@ -343,10 +366,10 @@ static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority) > > if ((priority >= CPPR(ss)) > || (XISR(ss) && (ss->pending_priority <= priority))) { > - ics_reject(ics, nr); > + ics_base_reject(ics, nr); > } else { > if (XISR(ss) && ss->xirr_owner) { > - ics_reject(ss->xirr_owner, XISR(ss)); > + ics_base_reject(ss->xirr_owner, XISR(ss)); > ss->xirr_owner = NULL; > } > ss->xirr = (ss->xirr & ~XISR_MASK) | (nr & XISR_MASK); > @@ -425,7 +448,7 @@ static const TypeInfo icp_info = { > /* > * ICS: Source layer > */ > -static void resend_msi(ICSState *ics, int srcno) > +static void ics_resend_msi(ICSState *ics, int srcno) > { > ICSIRQState *irq = ics->irqs + srcno; > > @@ -438,7 +461,7 @@ static void resend_msi(ICSState *ics, int srcno) > } > } > > -static void resend_lsi(ICSState *ics, int srcno) > +static void ics_resend_lsi(ICSState *ics, int srcno) > { > ICSIRQState *irq = ics->irqs + srcno; > > @@ -450,7 +473,7 @@ static void resend_lsi(ICSState *ics, int srcno) > } > } > > -static void set_irq_msi(ICSState *ics, int srcno, int val) > +static void ics_set_irq_msi(ICSState *ics, int srcno, int val) > { > ICSIRQState *irq = ics->irqs + srcno; > > @@ -466,7 +489,7 @@ 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_set_irq_lsi(ICSState *ics, int srcno, int val) > { > ICSIRQState *irq = ics->irqs + srcno; > > @@ -476,7 +499,7 @@ static void set_irq_lsi(ICSState *ics, int srcno, int val) > } else { > irq->status &= ~XICS_STATUS_ASSERTED; > } > - resend_lsi(ics, srcno); > + ics_resend_lsi(ics, srcno); > } > > static void ics_set_irq(void *opaque, int srcno, int val) > @@ -484,13 +507,13 @@ static void ics_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_set_irq_lsi(ics, srcno, val); > } else { > - set_irq_msi(ics, srcno, val); > + ics_set_irq_msi(ics, srcno, val); > } > } > > -static void write_xive_msi(ICSState *ics, int srcno) > +static void ics_write_xive_msi(ICSState *ics, int srcno) > { > ICSIRQState *irq = ics->irqs + srcno; > > @@ -503,31 +526,30 @@ 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_write_xive_lsi(ICSState *ics, int srcno) > { > - resend_lsi(ics, srcno); > + ics_resend_lsi(ics, srcno); > } > > -void ics_write_xive(ICSState *ics, int nr, int server, > - uint8_t priority, uint8_t saved_priority) > +void ics_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_write_xive(ics->offset + srcno, srcno, server, priority); > > if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { > - write_xive_lsi(ics, srcno); > + ics_write_xive_lsi(ics, srcno); > } else { > - write_xive_msi(ics, srcno); > + ics_write_xive_msi(ics, srcno); > } > } > > -static void ics_reject(ICSState *ics, int nr) > +static void ics_reject(ICSState *ics, uint32_t nr) > { > ICSIRQState *irq = ics->irqs + nr - ics->offset; > > @@ -543,14 +565,14 @@ static void ics_resend(ICSState *ics) > for (i = 0; i < ics->nr_irqs; i++) { > /* FIXME: filter by server#? */ > if (ics->irqs[i].flags & XICS_FLAGS_IRQ_LSI) { > - resend_lsi(ics, i); > + ics_resend_lsi(ics, i); > } else { > - resend_msi(ics, i); > + ics_resend_msi(ics, i); > } > } > } > > -static void ics_eoi(ICSState *ics, int nr) > +static void ics_eoi(ICSState *ics, uint32_t nr) > { > int srcno = nr - ics->offset; > ICSIRQState *irq = ics->irqs + srcno; > @@ -639,7 +661,8 @@ static const VMStateDescription vmstate_ics = { > VMSTATE_UINT32_EQUAL(nr_irqs, ICSState), > > VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs, > - vmstate_ics_irq, ICSIRQState), > + vmstate_ics_irq, > + ICSIRQState), > VMSTATE_END_OF_LIST() > }, > }; > @@ -672,17 +695,28 @@ static void ics_class_init(ObjectClass *klass, void *data) > dc->vmsd = &vmstate_ics; > dc->reset = ics_reset; > isc->post_load = ics_post_load; > + isc->reject = ics_reject; > + isc->resend = ics_resend; > + isc->eoi = ics_eoi; > } > > static const TypeInfo ics_info = { > .name = TYPE_ICS, > - .parent = TYPE_DEVICE, > + .parent = TYPE_ICS_BASE, > .instance_size = sizeof(ICSState), > .class_init = ics_class_init, > .class_size = sizeof(ICSStateClass), > .instance_init = ics_initfn, > }; > > +static const TypeInfo ics_base_info = { > + .name = TYPE_ICS_BASE, > + .parent = TYPE_DEVICE, > + .abstract = true, > + .instance_size = sizeof(ICSState), > + .class_size = sizeof(ICSStateClass), > +}; > + > /* > * Exported functions > */ > @@ -691,7 +725,7 @@ ICSState *xics_find_source(XICSState *xics, int irq) > ICSState *ics; > > QLIST_FOREACH(ics, &xics->ics, list) { > - if (ics_valid_irq(ics, irq)) { > + if (ics_base_valid_irq(ics, irq)) { > return ics; > } > } > @@ -721,6 +755,7 @@ static void xics_register_types(void) > { > type_register_static(&xics_common_info); > type_register_static(&ics_info); > + type_register_static(&ics_base_info); > type_register_static(&icp_info); > } > > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > index 8acafd9..113b067 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) || !ics) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -125,13 +125,14 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, > server = get_cpu_index_by_dt_id(rtas_ld(args, 1)); > priority = rtas_ld(args, 2); > > - if (!ics_valid_irq(ics, nr) || (server >= ics->xics->nr_servers) > + if (!ics_base_valid_irq(ics, nr) || (server >= ics->xics->nr_servers) > || (priority > 0xff)) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > return; > } > > - ics_write_xive(ics, nr, server, priority, priority); > + srcno = nr - ics->offset; > + ics_write_xive(ics, srcno, server, priority, priority); > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > @@ -142,7 +143,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) || !ics) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -151,14 +152,15 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, > > nr = rtas_ld(args, 0); > > - if (!ics_valid_irq(ics, nr)) { > + if (!ics_base_valid_irq(ics, nr)) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > return; > } > > 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, > @@ -167,7 +169,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) || !ics) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -176,13 +178,14 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr, > > nr = rtas_ld(args, 0); > > - if (!ics_valid_irq(ics, nr)) { > + if (!ics_base_valid_irq(ics, nr)) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > return; > } > > - ics_write_xive(ics, nr, ics->irqs[nr - ics->offset].server, 0xff, > - ics->irqs[nr - ics->offset].priority); > + srcno = nr - ics->offset; > + ics_write_xive(ics, srcno, ics->irqs[srcno].server, 0xff, > + ics->irqs[srcno].priority); > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > @@ -193,7 +196,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) || !ics) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -202,14 +205,15 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr, > > nr = rtas_ld(args, 0); > > - if (!ics_valid_irq(ics, nr)) { > + if (!ics_base_valid_irq(ics, nr)) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > 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_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 ee0fce2..6fb1cb4 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -117,6 +117,10 @@ struct ICPState { > bool cap_irq_xics_enabled; > }; > > +#define TYPE_ICS_BASE "ics-base" > +#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) > + > +/* Retain ics for sPAPR for migration from existing sPAPR guests */ > #define TYPE_ICS "ics" > #define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) > > @@ -133,6 +137,9 @@ struct ICSStateClass { > > void (*pre_save)(ICSState *s); > int (*post_load)(ICSState *s, int version_id); > + void (*reject)(ICSState *s, uint32_t irq); > + void (*resend)(ICSState *s); > + void (*eoi)(ICSState *s, uint32_t irq); > }; > > struct ICSState { > @@ -147,7 +154,7 @@ struct ICSState { > QLIST_ENTRY(ICSState) list; > }; > > -static inline bool ics_valid_irq(ICSState *ics, uint32_t nr) > +static inline bool ics_base_valid_irq(ICSState *ics, uint32_t nr) > { > return (ics->offset != 0) && (nr >= ics->offset) > && (nr < (ics->offset + ics->nr_irqs)); > @@ -190,7 +197,7 @@ 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); > + uint8_t priority, uint8_t saved_priority); > > void ics_set_irq_type(ICSState *ics, int srcno, bool lsi); >
David Gibson <david@gibson.dropbear.id.au> writes: > [ Unknown signature status ] > On Thu, Jun 23, 2016 at 11:17:28PM +0530, Nikunj A Dadhania wrote: >> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> >> The existing implementation remains same and ics-base is introduced. >> >> 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> >> --- >> hw/intc/xics.c | 101 +++++++++++++++++++++++++++++++++----------------- >> hw/intc/xics_spapr.c | 36 ++++++++++-------- >> include/hw/ppc/xics.h | 11 +++++- >> 3 files changed, 97 insertions(+), 51 deletions(-) >> >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c >> index 326d21f..e2aa48d 100644 >> --- a/hw/intc/xics.c >> +++ b/hw/intc/xics.c >> @@ -220,9 +220,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); >> -static void ics_eoi(ICSState *ics, int nr); >> +static void ics_base_reject(ICSState *ics, uint32_t nr) > > AFICT these will actually work for any of the derived classes, since > they call the function pointer. So I thin the original name was > better than ics_base_*(). Sure, will change. Regards Nikunj
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: > David Gibson <david@gibson.dropbear.id.au> writes: > >> [ Unknown signature status ] >> On Thu, Jun 23, 2016 at 11:17:28PM +0530, Nikunj A Dadhania wrote: >>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>> >>> The existing implementation remains same and ics-base is introduced. >>> >>> 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> >>> --- >>> hw/intc/xics.c | 101 +++++++++++++++++++++++++++++++++----------------- >>> hw/intc/xics_spapr.c | 36 ++++++++++-------- >>> include/hw/ppc/xics.h | 11 +++++- >>> 3 files changed, 97 insertions(+), 51 deletions(-) >>> >>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c >>> index 326d21f..e2aa48d 100644 >>> --- a/hw/intc/xics.c >>> +++ b/hw/intc/xics.c >>> @@ -220,9 +220,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); >>> -static void ics_eoi(ICSState *ics, int nr); >>> +static void ics_base_reject(ICSState *ics, uint32_t nr) >> >> AFICT these will actually work for any of the derived classes, since >> they call the function pointer. So I thin the original name was >> better than ics_base_*(). > > Sure, will change. I had a look at this again, we will need to use ics_base_*(), same file has the implementation of ics_reject() for TYPE_ICS. BenH's patches had renamed the class implementation as ics_simple_*(). Since we moved to using ICS_BASE, ICS and KVM_ICS, IMHO this seems to the appropriate names. Regards Nikunj
On Mon, Jun 27, 2016 at 03:41:06PM +0530, Nikunj A Dadhania wrote: > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: > > > David Gibson <david@gibson.dropbear.id.au> writes: > > > >> [ Unknown signature status ] > >> On Thu, Jun 23, 2016 at 11:17:28PM +0530, Nikunj A Dadhania wrote: > >>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > >>> > >>> The existing implementation remains same and ics-base is introduced. > >>> > >>> 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> > >>> --- > >>> hw/intc/xics.c | 101 +++++++++++++++++++++++++++++++++----------------- > >>> hw/intc/xics_spapr.c | 36 ++++++++++-------- > >>> include/hw/ppc/xics.h | 11 +++++- > >>> 3 files changed, 97 insertions(+), 51 deletions(-) > >>> > >>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c > >>> index 326d21f..e2aa48d 100644 > >>> --- a/hw/intc/xics.c > >>> +++ b/hw/intc/xics.c > >>> @@ -220,9 +220,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); > >>> -static void ics_eoi(ICSState *ics, int nr); > >>> +static void ics_base_reject(ICSState *ics, uint32_t nr) > >> > >> AFICT these will actually work for any of the derived classes, since > >> they call the function pointer. So I thin the original name was > >> better than ics_base_*(). > > > > Sure, will change. > > I had a look at this again, we will need to use ics_base_*(), same file > has the implementation of ics_reject() for TYPE_ICS. No, the ics_reject() plain names still work best for the generic versions which call via the function pointers. Instead we should find a new name for the TYPE_ICE implementations. > BenH's patches had renamed the class implementation as ics_simple_*(). > Since we moved to using ICS_BASE, ICS and KVM_ICS, IMHO this seems to > the appropriate names. No. Using ics_base_*() for the generic versions is actively misleading. Using good names for those is more important than what would usually be consistent naming practice for the TYPE_ICS implementation.
David Gibson <david@gibson.dropbear.id.au> writes: > [ Unknown signature status ] > On Mon, Jun 27, 2016 at 03:41:06PM +0530, Nikunj A Dadhania wrote: >> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: >> >> > David Gibson <david@gibson.dropbear.id.au> writes: >> > >> >> [ Unknown signature status ] >> >> On Thu, Jun 23, 2016 at 11:17:28PM +0530, Nikunj A Dadhania wrote: >> >>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> >>> >> >>> The existing implementation remains same and ics-base is introduced. >> >>> >> >>> 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> >> >>> --- >> >>> hw/intc/xics.c | 101 +++++++++++++++++++++++++++++++++----------------- >> >>> hw/intc/xics_spapr.c | 36 ++++++++++-------- >> >>> include/hw/ppc/xics.h | 11 +++++- >> >>> 3 files changed, 97 insertions(+), 51 deletions(-) >> >>> >> >>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c >> >>> index 326d21f..e2aa48d 100644 >> >>> --- a/hw/intc/xics.c >> >>> +++ b/hw/intc/xics.c >> >>> @@ -220,9 +220,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); >> >>> -static void ics_eoi(ICSState *ics, int nr); >> >>> +static void ics_base_reject(ICSState *ics, uint32_t nr) >> >> >> >> AFICT these will actually work for any of the derived classes, since >> >> they call the function pointer. So I thin the original name was >> >> better than ics_base_*(). >> > >> > Sure, will change. >> >> I had a look at this again, we will need to use ics_base_*(), same file >> has the implementation of ics_reject() for TYPE_ICS. > > No, the ics_reject() plain names still work best for the generic > versions which call via the function pointers. Instead we should find > a new name for the TYPE_ICE implementations. Should we go back to call it as TYPE_ICS_SIMPLE retaining the "ics" for migration compatibility. Rename all the related functions as ics_simple_* Similar to what we did for XICS #define TYPE_XICS_COMMON "xics-common" #define TYPE_XICS_SPAPR "xics" #define TYPE_XICS_SPAPR_KVM "xics-spapr-kvm" #define TYPE_XICS_NATIVE "xics-native" Like this #define TYPE_ICS_BASE "ics-base" #define TYPE_ICS_SIMPLE "ics" #define TYPE_KVM_ICS "icskvm" > >> BenH's patches had renamed the class implementation as ics_simple_*(). >> Since we moved to using ICS_BASE, ICS and KVM_ICS, IMHO this seems to >> the appropriate names. > > No. Using ics_base_*() for the generic versions is actively > misleading. Using good names for those is more important than what > would usually be consistent naming practice for the TYPE_ICS > implementation. Regards Nikunj
On Tue, Jun 28, 2016 at 10:36:23AM +0530, Nikunj A Dadhania wrote: > David Gibson <david@gibson.dropbear.id.au> writes: > > > [ Unknown signature status ] > > On Mon, Jun 27, 2016 at 03:41:06PM +0530, Nikunj A Dadhania wrote: > >> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: > >> > >> > David Gibson <david@gibson.dropbear.id.au> writes: > >> > > >> >> [ Unknown signature status ] > >> >> On Thu, Jun 23, 2016 at 11:17:28PM +0530, Nikunj A Dadhania wrote: > >> >>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > >> >>> > >> >>> The existing implementation remains same and ics-base is introduced. > >> >>> > >> >>> 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> > >> >>> --- > >> >>> hw/intc/xics.c | 101 +++++++++++++++++++++++++++++++++----------------- > >> >>> hw/intc/xics_spapr.c | 36 ++++++++++-------- > >> >>> include/hw/ppc/xics.h | 11 +++++- > >> >>> 3 files changed, 97 insertions(+), 51 deletions(-) > >> >>> > >> >>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c > >> >>> index 326d21f..e2aa48d 100644 > >> >>> --- a/hw/intc/xics.c > >> >>> +++ b/hw/intc/xics.c > >> >>> @@ -220,9 +220,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); > >> >>> -static void ics_eoi(ICSState *ics, int nr); > >> >>> +static void ics_base_reject(ICSState *ics, uint32_t nr) > >> >> > >> >> AFICT these will actually work for any of the derived classes, since > >> >> they call the function pointer. So I thin the original name was > >> >> better than ics_base_*(). > >> > > >> > Sure, will change. > >> > >> I had a look at this again, we will need to use ics_base_*(), same file > >> has the implementation of ics_reject() for TYPE_ICS. > > > > No, the ics_reject() plain names still work best for the generic > > versions which call via the function pointers. Instead we should find > > a new name for the TYPE_ICE implementations. > > Should we go back to call it as TYPE_ICS_SIMPLE retaining the "ics" for > migration compatibility. Rename all the related functions as > ics_simple_* > > Similar to what we did for XICS > > #define TYPE_XICS_COMMON "xics-common" > #define TYPE_XICS_SPAPR "xics" > #define TYPE_XICS_SPAPR_KVM "xics-spapr-kvm" > #define TYPE_XICS_NATIVE "xics-native" > > > Like this > > > #define TYPE_ICS_BASE "ics-base" > #define TYPE_ICS_SIMPLE "ics" > #define TYPE_KVM_ICS "icskvm" Yes, that would be ok with me. > >> BenH's patches had renamed the class implementation as ics_simple_*(). > >> Since we moved to using ICS_BASE, ICS and KVM_ICS, IMHO this seems to > >> the appropriate names. > > > > No. Using ics_base_*() for the generic versions is actively > > misleading. Using good names for those is more important than what > > would usually be consistent naming practice for the TYPE_ICS > > implementation. > > Regards > Nikunj >
diff --git a/hw/intc/xics.c b/hw/intc/xics.c index 326d21f..e2aa48d 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -220,9 +220,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); -static void ics_eoi(ICSState *ics, int nr); +static void ics_base_reject(ICSState *ics, uint32_t nr) +{ + ICSStateClass *k = ICS_GET_CLASS(ics); + + if (k->reject) { + k->reject(ics, nr); + } +} + +static void ics_base_resend(ICSState *ics) +{ + ICSStateClass *k = ICS_GET_CLASS(ics); + + if (k->resend) { + k->resend(ics); + } +} + +static void ics_base_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, int server) { @@ -233,7 +256,7 @@ static void icp_check_ipi(ICPState *ss, int server) trace_xics_icp_check_ipi(server, ss->mfrr); if (XISR(ss) && ss->xirr_owner) { - ics_reject(ss->xirr_owner, XISR(ss)); + ics_base_reject(ss->xirr_owner, XISR(ss)); } ss->xirr = (ss->xirr & ~XISR_MASK) | XICS_IPI; @@ -251,7 +274,7 @@ static void icp_resend(XICSState *xics, int server) icp_check_ipi(ss, server); } QLIST_FOREACH(ics, &xics->ics, list) { - ics_resend(ics); + ics_base_resend(ics); } } @@ -271,7 +294,7 @@ void icp_set_cppr(XICSState *xics, int server, uint8_t cppr) ss->pending_priority = 0xff; qemu_irq_lower(ss->output); if (ss->xirr_owner) { - ics_reject(ss->xirr_owner, old_xisr); + ics_base_reject(ss->xirr_owner, old_xisr); ss->xirr_owner = NULL; } } @@ -325,8 +348,8 @@ void icp_eoi(XICSState *xics, int server, uint32_t xirr) trace_xics_icp_eoi(server, xirr, ss->xirr); irq = xirr & XISR_MASK; QLIST_FOREACH(ics, &xics->ics, list) { - if (ics_valid_irq(ics, irq)) { - ics_eoi(ics, irq); + if (ics_base_valid_irq(ics, irq)) { + ics_base_eoi(ics, irq); } } if (!XISR(ss)) { @@ -343,10 +366,10 @@ static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority) if ((priority >= CPPR(ss)) || (XISR(ss) && (ss->pending_priority <= priority))) { - ics_reject(ics, nr); + ics_base_reject(ics, nr); } else { if (XISR(ss) && ss->xirr_owner) { - ics_reject(ss->xirr_owner, XISR(ss)); + ics_base_reject(ss->xirr_owner, XISR(ss)); ss->xirr_owner = NULL; } ss->xirr = (ss->xirr & ~XISR_MASK) | (nr & XISR_MASK); @@ -425,7 +448,7 @@ static const TypeInfo icp_info = { /* * ICS: Source layer */ -static void resend_msi(ICSState *ics, int srcno) +static void ics_resend_msi(ICSState *ics, int srcno) { ICSIRQState *irq = ics->irqs + srcno; @@ -438,7 +461,7 @@ static void resend_msi(ICSState *ics, int srcno) } } -static void resend_lsi(ICSState *ics, int srcno) +static void ics_resend_lsi(ICSState *ics, int srcno) { ICSIRQState *irq = ics->irqs + srcno; @@ -450,7 +473,7 @@ static void resend_lsi(ICSState *ics, int srcno) } } -static void set_irq_msi(ICSState *ics, int srcno, int val) +static void ics_set_irq_msi(ICSState *ics, int srcno, int val) { ICSIRQState *irq = ics->irqs + srcno; @@ -466,7 +489,7 @@ 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_set_irq_lsi(ICSState *ics, int srcno, int val) { ICSIRQState *irq = ics->irqs + srcno; @@ -476,7 +499,7 @@ static void set_irq_lsi(ICSState *ics, int srcno, int val) } else { irq->status &= ~XICS_STATUS_ASSERTED; } - resend_lsi(ics, srcno); + ics_resend_lsi(ics, srcno); } static void ics_set_irq(void *opaque, int srcno, int val) @@ -484,13 +507,13 @@ static void ics_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_set_irq_lsi(ics, srcno, val); } else { - set_irq_msi(ics, srcno, val); + ics_set_irq_msi(ics, srcno, val); } } -static void write_xive_msi(ICSState *ics, int srcno) +static void ics_write_xive_msi(ICSState *ics, int srcno) { ICSIRQState *irq = ics->irqs + srcno; @@ -503,31 +526,30 @@ 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_write_xive_lsi(ICSState *ics, int srcno) { - resend_lsi(ics, srcno); + ics_resend_lsi(ics, srcno); } -void ics_write_xive(ICSState *ics, int nr, int server, - uint8_t priority, uint8_t saved_priority) +void ics_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_write_xive(ics->offset + srcno, srcno, server, priority); if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) { - write_xive_lsi(ics, srcno); + ics_write_xive_lsi(ics, srcno); } else { - write_xive_msi(ics, srcno); + ics_write_xive_msi(ics, srcno); } } -static void ics_reject(ICSState *ics, int nr) +static void ics_reject(ICSState *ics, uint32_t nr) { ICSIRQState *irq = ics->irqs + nr - ics->offset; @@ -543,14 +565,14 @@ static void ics_resend(ICSState *ics) for (i = 0; i < ics->nr_irqs; i++) { /* FIXME: filter by server#? */ if (ics->irqs[i].flags & XICS_FLAGS_IRQ_LSI) { - resend_lsi(ics, i); + ics_resend_lsi(ics, i); } else { - resend_msi(ics, i); + ics_resend_msi(ics, i); } } } -static void ics_eoi(ICSState *ics, int nr) +static void ics_eoi(ICSState *ics, uint32_t nr) { int srcno = nr - ics->offset; ICSIRQState *irq = ics->irqs + srcno; @@ -639,7 +661,8 @@ static const VMStateDescription vmstate_ics = { VMSTATE_UINT32_EQUAL(nr_irqs, ICSState), VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs, - vmstate_ics_irq, ICSIRQState), + vmstate_ics_irq, + ICSIRQState), VMSTATE_END_OF_LIST() }, }; @@ -672,17 +695,28 @@ static void ics_class_init(ObjectClass *klass, void *data) dc->vmsd = &vmstate_ics; dc->reset = ics_reset; isc->post_load = ics_post_load; + isc->reject = ics_reject; + isc->resend = ics_resend; + isc->eoi = ics_eoi; } static const TypeInfo ics_info = { .name = TYPE_ICS, - .parent = TYPE_DEVICE, + .parent = TYPE_ICS_BASE, .instance_size = sizeof(ICSState), .class_init = ics_class_init, .class_size = sizeof(ICSStateClass), .instance_init = ics_initfn, }; +static const TypeInfo ics_base_info = { + .name = TYPE_ICS_BASE, + .parent = TYPE_DEVICE, + .abstract = true, + .instance_size = sizeof(ICSState), + .class_size = sizeof(ICSStateClass), +}; + /* * Exported functions */ @@ -691,7 +725,7 @@ ICSState *xics_find_source(XICSState *xics, int irq) ICSState *ics; QLIST_FOREACH(ics, &xics->ics, list) { - if (ics_valid_irq(ics, irq)) { + if (ics_base_valid_irq(ics, irq)) { return ics; } } @@ -721,6 +755,7 @@ static void xics_register_types(void) { type_register_static(&xics_common_info); type_register_static(&ics_info); + type_register_static(&ics_base_info); type_register_static(&icp_info); } diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c index 8acafd9..113b067 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) || !ics) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); @@ -125,13 +125,14 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, server = get_cpu_index_by_dt_id(rtas_ld(args, 1)); priority = rtas_ld(args, 2); - if (!ics_valid_irq(ics, nr) || (server >= ics->xics->nr_servers) + if (!ics_base_valid_irq(ics, nr) || (server >= ics->xics->nr_servers) || (priority > 0xff)) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); return; } - ics_write_xive(ics, nr, server, priority, priority); + srcno = nr - ics->offset; + ics_write_xive(ics, srcno, server, priority, priority); rtas_st(rets, 0, RTAS_OUT_SUCCESS); } @@ -142,7 +143,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) || !ics) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); @@ -151,14 +152,15 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr, nr = rtas_ld(args, 0); - if (!ics_valid_irq(ics, nr)) { + if (!ics_base_valid_irq(ics, nr)) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); return; } 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, @@ -167,7 +169,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) || !ics) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); @@ -176,13 +178,14 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr, nr = rtas_ld(args, 0); - if (!ics_valid_irq(ics, nr)) { + if (!ics_base_valid_irq(ics, nr)) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); return; } - ics_write_xive(ics, nr, ics->irqs[nr - ics->offset].server, 0xff, - ics->irqs[nr - ics->offset].priority); + srcno = nr - ics->offset; + ics_write_xive(ics, srcno, ics->irqs[srcno].server, 0xff, + ics->irqs[srcno].priority); rtas_st(rets, 0, RTAS_OUT_SUCCESS); } @@ -193,7 +196,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) || !ics) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); @@ -202,14 +205,15 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr, nr = rtas_ld(args, 0); - if (!ics_valid_irq(ics, nr)) { + if (!ics_base_valid_irq(ics, nr)) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); 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_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 ee0fce2..6fb1cb4 100644 --- a/include/hw/ppc/xics.h +++ b/include/hw/ppc/xics.h @@ -117,6 +117,10 @@ struct ICPState { bool cap_irq_xics_enabled; }; +#define TYPE_ICS_BASE "ics-base" +#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE) + +/* Retain ics for sPAPR for migration from existing sPAPR guests */ #define TYPE_ICS "ics" #define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) @@ -133,6 +137,9 @@ struct ICSStateClass { void (*pre_save)(ICSState *s); int (*post_load)(ICSState *s, int version_id); + void (*reject)(ICSState *s, uint32_t irq); + void (*resend)(ICSState *s); + void (*eoi)(ICSState *s, uint32_t irq); }; struct ICSState { @@ -147,7 +154,7 @@ struct ICSState { QLIST_ENTRY(ICSState) list; }; -static inline bool ics_valid_irq(ICSState *ics, uint32_t nr) +static inline bool ics_base_valid_irq(ICSState *ics, uint32_t nr) { return (ics->offset != 0) && (nr >= ics->offset) && (nr < (ics->offset + ics->nr_irqs)); @@ -190,7 +197,7 @@ 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); + uint8_t priority, uint8_t saved_priority); void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);