From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59441) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLO7e-0005Wv-2C for qemu-devel@nongnu.org; Fri, 08 Jul 2016 01:14:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bLO7Y-0004eN-SS for qemu-devel@nongnu.org; Fri, 08 Jul 2016 01:14:40 -0400 Date: Fri, 8 Jul 2016 15:16:09 +1000 From: David Gibson Message-ID: <20160708051608.GJ14675@voom.fritz.box> References: <1467914058-30551-1-git-send-email-nikunj@linux.vnet.ibm.com> <1467914058-30551-2-git-send-email-nikunj@linux.vnet.ibm.com> <20160708041523.GF14675@voom.fritz.box> <8760sgivhj.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="D85EWoRcDXv1uhH1" Content-Disposition: inline In-Reply-To: <8760sgivhj.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> Subject: Re: [Qemu-devel] [PATCH v3 1/4] ppc/xics: Make the ICSState a list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikunj A Dadhania Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, clg@kaod.org, benh@kernel.crashing.org --D85EWoRcDXv1uhH1 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jul 08, 2016 at 10:20:32AM +0530, Nikunj A Dadhania wrote: > David Gibson writes: >=20 > > [ Unknown signature status ] > > On Thu, Jul 07, 2016 at 11:24:15PM +0530, Nikunj A Dadhania wrote: > >> From: Benjamin Herrenschmidt > >>=20 > >> Instead of an array of fixed sized blocks, use a list, as we will need > >> to have sources with variable number of interrupts. SPAPR only uses > >> a single entry. Native will create more. If performance becomes an > >> issue we can add some hashed lookup but for now this will do fine. > >>=20 > >> Signed-off-by: Benjamin Herrenschmidt > >> [ move the initialization of list to xics_common_initfn, > >> restore xirr_owner after migration and move restoring to > >> icp_post_load] > >> Signed-off-by: Nikunj A Dadhania > >> --- > >> hw/intc/trace-events | 5 +- > >> hw/intc/xics.c | 134 ++++++++++++++++++++++++++++++++---------= --------- > >> hw/intc/xics_kvm.c | 27 +++++++--- > >> hw/intc/xics_spapr.c | 88 +++++++++++++++++++++------------ > >> hw/ppc/spapr_events.c | 2 +- > >> hw/ppc/spapr_pci.c | 5 +- > >> hw/ppc/spapr_vio.c | 2 +- > >> include/hw/ppc/xics.h | 13 ++--- > >> 8 files changed, 175 insertions(+), 101 deletions(-) > >>=20 > >> diff --git a/hw/intc/trace-events b/hw/intc/trace-events > >> index 376dd18..41ced33 100644 > >> --- a/hw/intc/trace-events > >> +++ b/hw/intc/trace-events > >> @@ -56,10 +56,11 @@ 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_alloc(int src, int irq) "source#%d, irq %d" > >> -xics_alloc_block(int src, int first, int num, bool lsi, int align) "s= ource#%d, first irq %d, %d irqs, lsi=3D%d, alignnum %d" > >> +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" > >> xics_ics_free_warn(int src, int irq) "Source#%d, irq %d is already fr= ee" > >> +xics_icp_post_load(uint32_t server_no, uint32_t xirr, uint64_t addr, = uint8_t pend) "server_no %d, xirr %#x, xirr_owner 0x%" PRIx64 ", pending %d" > >> =20 > >> # hw/intc/s390_flic_kvm.c > >> flic_create_device(int err) "flic: create device failed %d" > >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c > >> index cd48f42..0af05c9 100644 > >> --- a/hw/intc/xics.c > >> +++ b/hw/intc/xics.c > >> @@ -32,6 +32,7 @@ > >> #include "hw/hw.h" > >> #include "trace.h" > >> #include "qemu/timer.h" > >> +#include "hw/ppc/spapr.h" > >> #include "hw/ppc/xics.h" > >> #include "qemu/error-report.h" > >> #include "qapi/visitor.h" > >> @@ -96,13 +97,16 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *c= pu) > >> static void xics_common_reset(DeviceState *d) > >> { > >> XICSState *xics =3D XICS_COMMON(d); > >> + ICSState *ics; > >> int i; > >> =20 > >> for (i =3D 0; i < xics->nr_servers; i++) { > >> device_reset(DEVICE(&xics->ss[i])); > >> } > >> =20 > >> - device_reset(DEVICE(xics->ics)); > >> + QLIST_FOREACH(ics, &xics->ics, list) { > >> + device_reset(DEVICE(ics)); > >> + } > >> } > >> =20 > >> static void xics_prop_get_nr_irqs(Object *obj, Visitor *v, const char= *name, > >> @@ -134,7 +138,6 @@ static void xics_prop_set_nr_irqs(Object *obj, Vis= itor *v, const char *name, > >> } > >> =20 > >> assert(info->set_nr_irqs); > >> - assert(xics->ics); > >> info->set_nr_irqs(xics, value, errp); > >> } > >> =20 > >> @@ -174,6 +177,9 @@ static void xics_prop_set_nr_servers(Object *obj, = Visitor *v, > >> =20 > >> static void xics_common_initfn(Object *obj) > >> { > >> + XICSState *xics =3D XICS_COMMON(obj); > >> + > >> + QLIST_INIT(&xics->ics); > >> object_property_add(obj, "nr_irqs", "int", > >> xics_prop_get_nr_irqs, xics_prop_set_nr_irqs, > >> NULL, NULL, NULL); > >> @@ -212,33 +218,35 @@ static void ics_reject(ICSState *ics, int nr); > >> static void ics_resend(ICSState *ics); > >> static void ics_eoi(ICSState *ics, int nr); > >> =20 > >> -static void icp_check_ipi(XICSState *xics, int server) > >> +static void icp_check_ipi(ICPState *ss) > >> { > >> - ICPState *ss =3D xics->ss + server; > >> - > >> if (XISR(ss) && (ss->pending_priority <=3D ss->mfrr)) { > >> return; > >> } > >> =20 > >> - trace_xics_icp_check_ipi(server, ss->mfrr); > >> + trace_xics_icp_check_ipi(ss->cs->cpu_index, ss->mfrr); > > > > The dt_id might be more useful here, given the changes that are going > > on in that area. But it's not worth holding up this series just for > > that. >=20 > Yes, I am seeing those changes, will adjust accordingly if that goes > first. >=20 > > > >> =20 > >> - if (XISR(ss)) { > >> - ics_reject(xics->ics, XISR(ss)); > >> + if (XISR(ss) && ss->xirr_owner) { > >> + ics_reject(ss->xirr_owner, XISR(ss)); > >> } > >> =20 > >> ss->xirr =3D (ss->xirr & ~XISR_MASK) | XICS_IPI; > >> ss->pending_priority =3D ss->mfrr; > >> + ss->xirr_owner =3D NULL; > >> qemu_irq_raise(ss->output); > >> } > >> =20 > >> static void icp_resend(XICSState *xics, int server) > >> { > >> ICPState *ss =3D xics->ss + server; > >> + ICSState *ics; > > > > Possibly this should take icp instead of xics for consistency. Or > > else be renamed xics_resend(). But that can be a cleanup for another > > time. > > > >> if (ss->mfrr < CPPR(ss)) { > >> - icp_check_ipi(xics, server); > >> + icp_check_ipi(ss); > >> + } > >> + QLIST_FOREACH(ics, &xics->ics, list) { > >> + ics_resend(ics); > >> } > >> - ics_resend(xics->ics); > >> } > >> =20 > >> void icp_set_cppr(XICSState *xics, int server, uint8_t cppr) > >> @@ -256,7 +264,10 @@ void icp_set_cppr(XICSState *xics, int server, ui= nt8_t cppr) > >> ss->xirr &=3D ~XISR_MASK; /* Clear XISR */ > >> ss->pending_priority =3D 0xff; > >> qemu_irq_lower(ss->output); > >> - ics_reject(xics->ics, old_xisr); > >> + if (ss->xirr_owner) { > >> + ics_reject(ss->xirr_owner, old_xisr); > >> + ss->xirr_owner =3D NULL; > >> + } > >> } > >> } else { > >> if (!XISR(ss)) { > >> @@ -271,7 +282,7 @@ void icp_set_mfrr(XICSState *xics, int server, uin= t8_t mfrr) > >> =20 > >> ss->mfrr =3D mfrr; > >> if (mfrr < CPPR(ss)) { > >> - icp_check_ipi(xics, server); > >> + icp_check_ipi(ss); > >> } > >> } > >> =20 > >> @@ -282,6 +293,7 @@ uint32_t icp_accept(ICPState *ss) > >> qemu_irq_lower(ss->output); > >> ss->xirr =3D ss->pending_priority << 24; > >> ss->pending_priority =3D 0xff; > >> + ss->xirr_owner =3D NULL; > >> =20 > >> trace_xics_icp_accept(xirr, ss->xirr); > >> =20 > >> @@ -299,30 +311,40 @@ uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr) > >> void icp_eoi(XICSState *xics, int server, uint32_t xirr) > >> { > >> ICPState *ss =3D xics->ss + server; > >> + ICSState *ics; > >> + uint32_t irq; > >> =20 > >> /* Send EOI -> ICS */ > >> ss->xirr =3D (ss->xirr & ~CPPR_MASK) | (xirr & CPPR_MASK); > >> trace_xics_icp_eoi(server, xirr, ss->xirr); > >> - ics_eoi(xics->ics, xirr & XISR_MASK); > >> + irq =3D xirr & XISR_MASK; > >> + QLIST_FOREACH(ics, &xics->ics, list) { > >> + if (ics_valid_irq(ics, irq)) { > >> + ics_eoi(ics, irq); > > > > You could slightly optimize this by adding a break; here, but again > > that can be a tweak for another time. >=20 > Sure. >=20 > > > >> + } > >> + } > >> if (!XISR(ss)) { > >> icp_resend(xics, server); > >> } > >> } > >> =20 > >> -static void icp_irq(XICSState *xics, int server, int nr, uint8_t prio= rity) > >> +static void icp_irq(ICSState *ics, int server, int nr, uint8_t priori= ty) > >> { > >> + XICSState *xics =3D ics->xics; > >> ICPState *ss =3D xics->ss + server; > > > > It's kind of weird for an icp_() function to take an ics but not an > > icp. It might make more sense for it to take both, even though > > obviously it can be derived from just the ics. Again, won't hold up > > the series for this. > > > >> trace_xics_icp_irq(server, nr, priority); > >> =20 > >> if ((priority >=3D CPPR(ss)) > >> || (XISR(ss) && (ss->pending_priority <=3D priority))) { > >> - ics_reject(xics->ics, nr); > >> + ics_reject(ics, nr); > >> } else { > >> - if (XISR(ss)) { > >> - ics_reject(xics->ics, XISR(ss)); > >> + if (XISR(ss) && ss->xirr_owner) { > >> + ics_reject(ss->xirr_owner, XISR(ss)); > >> + ss->xirr_owner =3D NULL; > >> } > >> ss->xirr =3D (ss->xirr & ~XISR_MASK) | (nr & XISR_MASK); > >> + ss->xirr_owner =3D ics; > >> ss->pending_priority =3D priority; > >> trace_xics_icp_raise(ss->xirr, ss->pending_priority); > >> qemu_irq_raise(ss->output); > >> @@ -378,12 +400,42 @@ static void icp_reset(DeviceState *dev) > >> qemu_set_irq(icp->output, 0); > >> } > >> =20 > >> +static int icp_post_load(ICPState *ss, int version_id) > >> +{ > >> + sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); > >> + XICSState *xics =3D spapr->xics; > >> + uint32_t irq, i; > >> + static uint32_t server_no =3D 0; > > > > Eugh.. static locals are usually horrid. > > > >> + server_no++; > >> + irq =3D XISR(ss); > >> + if (!ss->cs || !irq) > >> + goto icpend; > > > > That's an awful use of a goto - inverting the if sense is clearer and > > no longer. >=20 > Sure. >=20 > > > >> + > >> + /* Restore the xirr_owner */ > >> + ss->xirr_owner =3D xics_find_source(xics, irq); > > > > Do we actually need this? Have you confirmed that replaying the > > icp_resend()s won't already accomplish this? >=20 > Yes, i have had migration hang on the target. Other thing its only > affecting "kernel-irqchip=3Doff" Hrm.. It would be good to understand exactly what is going on here so we can be confident we've solved it correctly. > >> + icpend: > >> + trace_xics_icp_post_load(server_no, ss->xirr, (uint64_t)ss->xirr_= owner, ss->pending_priority); > >> + if (xics->nr_servers !=3D server_no) > >> + return 0; > >> + > >> + /* All the ICPs are processed, not restore all the state */ > >> + for (i =3D 0; i < xics->nr_servers; i++) { > >> + icp_resend(xics, i); > >> + } > >> + server_no =3D 0; > > > > Eugh. This is really ugly. It really looks like we need some > > migration state on the overall xics, not just the component icps and > > icss. Dealing with backwards compatibility will be hairy, but I > > really hope we can do better than this hack. >=20 > Earlier icp_resend() was called from ics_post_load() path, and > icp_post_load gets called after ics_post_load, the xirr_owner is not > restored during ics_post_load. >=20 > If we can get icp_post_load called before ics_post_load, we would not > need the above code. Or as you suggested somehere above this. Hm, ok. I'm wondering if we can move most of the post_load logic to a top-level xicsstate structure. I think the migration layer guarantees that parent objects have their post_load called after all the subordinate objects have their state restore. Migration compat could be tricky. Since I don't think we need any actual associated with the top-level object it should be possible in theory, but I'm not sure if there's inbuilt support for an object with no on-wire presence, but just a post_load handler. >=20 > > > >> + return 0; > >> +} > >> + > >> static void icp_class_init(ObjectClass *klass, void *data) > >> { > >> DeviceClass *dc =3D DEVICE_CLASS(klass); > >> + ICPStateClass *icpc =3D ICP_CLASS(klass); > >> =20 > >> dc->reset =3D icp_reset; > >> dc->vmsd =3D &vmstate_icp_server; > >> + icpc->post_load =3D icp_post_load; > >> } > >> =20 > >> static const TypeInfo icp_info =3D { > >> @@ -405,8 +457,7 @@ static void resend_msi(ICSState *ics, int srcno) > >> if (irq->status & XICS_STATUS_REJECTED) { > >> irq->status &=3D ~XICS_STATUS_REJECTED; > >> if (irq->priority !=3D 0xff) { > >> - icp_irq(ics->xics, irq->server, srcno + ics->offset, > >> - irq->priority); > >> + icp_irq(ics, irq->server, srcno + ics->offset, irq->prior= ity); > >> } > >> } > >> } > >> @@ -419,7 +470,7 @@ static void resend_lsi(ICSState *ics, int srcno) > >> && (irq->status & XICS_STATUS_ASSERTED) > >> && !(irq->status & XICS_STATUS_SENT)) { > >> irq->status |=3D XICS_STATUS_SENT; > >> - icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->pri= ority); > >> + icp_irq(ics, irq->server, srcno + ics->offset, irq->priority); > >> } > >> } > >> =20 > >> @@ -434,7 +485,7 @@ static void set_irq_msi(ICSState *ics, int srcno, = int val) > >> irq->status |=3D XICS_STATUS_MASKED_PENDING; > >> trace_xics_masked_pending(); > >> } else { > >> - icp_irq(ics->xics, irq->server, srcno + ics->offset, irq-= >priority); > >> + icp_irq(ics, irq->server, srcno + ics->offset, irq->prior= ity); > >> } > >> } > >> } > >> @@ -473,7 +524,7 @@ static void write_xive_msi(ICSState *ics, int srcn= o) > >> } > >> =20 > >> irq->status &=3D ~XICS_STATUS_MASKED_PENDING; > >> - icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priorit= y); > >> + icp_irq(ics, irq->server, srcno + ics->offset, irq->priority); > >> } > >> =20 > >> static void write_xive_lsi(ICSState *ics, int srcno) > >> @@ -505,8 +556,11 @@ static void ics_reject(ICSState *ics, int nr) > >> ICSIRQState *irq =3D ics->irqs + nr - ics->offset; > >> =20 > >> trace_xics_ics_reject(nr, nr - ics->offset); > >> - irq->status |=3D XICS_STATUS_REJECTED; /* Irrelevant but harmless= for LSI */ > >> - irq->status &=3D ~XICS_STATUS_SENT; /* Irrelevant but harmless fo= r MSI */ > >> + if (irq->flags & XICS_FLAGS_IRQ_MSI) { > >> + irq->status |=3D XICS_STATUS_REJECTED; > >> + } else { > >> + irq->status &=3D ~XICS_STATUS_SENT; > >> + } > > > > This change appears unrelated to the rest. If not that needs an explan= ation. >=20 > I was seeing inconsistent status because of this in the trace logs, like > for LSI status as 0x7, i.e. XICS_STATUS_ASSERTED, XICS_STATUS_SENT and > XICS_STATUS_REJECTED all set, which did not make sense. So the REJECTED > would have been set in earlier interrupt cycle, and then asserted and > sent in this current one. Ah, ok. Can you split this out into a separate patch then, with an explanation that it's to remove confusing debug output? --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --D85EWoRcDXv1uhH1 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXfzcYAAoJEGw4ysog2bOSmccQALKA0z4GYAZwO4oyo7fuUubP 2TnJK3mPKC3qiQpZGaCUZyvWX6hTgyLjnh41fogjne3oLzTbIlhu/j2mWFnGuTj3 liE9CgiAa9ZfUu5tJc9bZQnavGbK7Q0bQtXBs6bAGsxFN1VgmHTxUyoaJ3lVGyQj Ly6dr7fTwNnX+gTXk5ejVNpbIGjKezqv7ANQ98avDBz5HHs65PartRC2knC5GAaY GihzPPbzfPIUmqo8vZx0IuWQOcqQYAYDt1QpzmeLQZS/BI0U6KjlKbqpBMyd/LsY sy+EVQy/e+LAWiPJJiIDK+EHLNF8Y3zRR7Ufj6yLNwjYy7S5FU7cKemdjFnZfOF3 UmooKtIN587y4fyOzqT685Qn8jHUVaaF6jZVXE9kBFoUVceaDRy/KgUeimidSGDN WCcrn9emyGUlfFsr24WUnOh0JCDl5NF+lun/QQN4z08XB+v35zRmg7vt7HZJ1lEh zhD+e2OriwwjXIVYVxd5TTksoFjwv926DbTlJ4XjGZrXi+102Huz/m/OEOahIMpI 9WJC52rFYaV9qAz3Z7zfAzpJfgnt20u+8jB1/LLTOSyGHzIRKqHZ+8KvhHFIWEXR iac4GPmk5W9osidYGktNdfJuyIG+fP6A2QL7xWYBa9R+GSvBECibfIuCq07FNN7g tVSEcqBeit/uxV2vkhgU =XByn -----END PGP SIGNATURE----- --D85EWoRcDXv1uhH1--