Cédric Le Goater <c...@kaod.org> writes: > On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote: >> Cédric Le Goater <c...@kaod.org> writes: >> >>> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote: >>>> From: Benjamin Herrenschmidt <b...@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 <b...@kernel.crashing.org> >>>> Signed-off-by: Nikunj A Dadhania <nik...@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