From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53221) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmEQN-0001ht-F7 for qemu-devel@nongnu.org; Tue, 20 Sep 2016 02:21:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmEQJ-0001pu-Bl for qemu-devel@nongnu.org; Tue, 20 Sep 2016 02:20:58 -0400 Received: from 5.mo5.mail-out.ovh.net ([87.98.173.103]:52838) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmEQJ-0001pX-3O for qemu-devel@nongnu.org; Tue, 20 Sep 2016 02:20:55 -0400 Received: from player763.ha.ovh.net (b9.ovh.net [213.186.33.59]) by mo5.mail-out.ovh.net (Postfix) with ESMTP id 73199FFC9E1 for ; Tue, 20 Sep 2016 08:20:53 +0200 (CEST) References: <1474266577-11704-1-git-send-email-nikunj@linux.vnet.ibm.com> <1474266577-11704-7-git-send-email-nikunj@linux.vnet.ibm.com> <87r38frtw1.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: <19c378eb-33d7-3e61-ec53-3de5ea40f407@kaod.org> Date: Tue, 20 Sep 2016 08:20:49 +0200 MIME-Version: 1.0 In-Reply-To: <87r38frtw1.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikunj A Dadhania , qemu-ppc@nongnu.org, david@gibson.dropbear.id.au Cc: qemu-devel@nongnu.org On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote: > C=C3=A9dric Le Goater writes: >=20 >> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote: >>> From: Benjamin Herrenschmidt >>> >>> 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 >>> Signed-off-by: Nikunj A Dadhania >>> --- >>> 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_x= irr) "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=3D%#x new pending priority=3D%#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 pr= iority) "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=3D%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; >>> =20 >>> - ics =3D ICS(object_new(TYPE_ICS)); >>> + ics =3D ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE)); >>> object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL= ); >>> ics->xics =3D xics; >>> QLIST_INSERT_HEAD(&xics->ics, ics, list); >>> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info =3D { >>> #define XISR(ss) (((ss)->xirr) & XISR_MASK) >>> #define CPPR(ss) (((ss)->xirr) >> 24) >>> =20 >>> -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 =3D ICS_GET_CLASS(ics); >> >> Shouldn't that be ICS_BASE_GET_CLASS() >=20 > The class hierarchy is something like this: >=20 > ICS_BASE -> ICS_SIMPLE -> ICS_KVM yes. but if we called ics_* with an instance of an ics class which is=20 not a ICS_SIMPLE class that will break. ICSStateClass is the baseclass, whenever we call methods on a ICSState*=20 object, we should use the method defines in ICSStateClass. > We have an instance init for ICS_SIMPLE where we set these pointers. >=20 >>> 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; >>> }; >>> =20 >>> -#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) >=20 > Yes, that is a bug. Will correct. >=20 >> >>> -#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_SIMPL= E) >>> + >>> +#define TYPE_ICS_KVM "icskvm" >>> +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM) >>> =20 >>> #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) >=20 > As explained above, not needed. We will with powernv which defines a new ics type for the phb3 msis. Thanks, C.