From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33715) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UpQoz-0003u2-Lx for qemu-devel@nongnu.org; Wed, 19 Jun 2013 18:25:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UpQow-00079Z-5d for qemu-devel@nongnu.org; Wed, 19 Jun 2013 18:25:45 -0400 Date: Wed, 19 Jun 2013 17:25:34 -0500 From: Scott Wood In-Reply-To: <51BE0DFE.8060407@suse.de> (from afaerber@suse.de on Sun Jun 16 14:11:58 2013) Message-ID: <1371680734.11064.2@snotra> MIME-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; delsp=Yes; format=Flowed Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?q?F=E4rber?= Cc: qemu-ppc@nongnu.org, Alexander Graf , qemu-devel@nongnu.org On 06/16/2013 02:11:58 PM, Andreas F=E4rber wrote: > Am 15.06.2013 00:57, schrieb Scott Wood: > > ...which of those would make me think "hmm, there's something in =20 > here > > that I need to read before submitting patches for in-kernel mpic"? > > > > I'm not trying to be difficult -- I'm just trying to point out that > > there's room for improvement in how the QEMU community communicates =20 > to > > developers what is expected. Maybe an announcement list that just > > contains important updates and summaries? >=20 > +1 - but surely not all changes would get communicated on such a list. Sure. It doesn't need to be perfect to be a big help. > > And "making other people even more work" goes both ways... >=20 > Right. But if you're complaining about QOM and QOM realize, address =20 > your > complaints to Anthony instead. :) > We're a community, and sending patches in write-only mode conflicts =20 > with > my understanding of being e500 co-maintainer. It wasn't "write-only mode" -- I've accepted and acted on plenty of =20 other feedback in this and other patchsets (in fact, some of that =20 feedback told me specifically to use things like qdev_init_nofail, =20 which apparently is deprecated). And I'm not opposed to cleaning =20 up/modernizing existing e500 code (or delegating it to a coworker, =20 which includes Alex, who works for us part-time) if it's clear what is =20 expected. I was just looking for help with a part of QEMU that I find =20 pretty opaque, and was put off by the tone of the initial complaint. > I have no personal > advantage of this e500 KVM PIC, it just makes more work for me. So =20 > it's > your job to keep the code from bitrotting, especially when not =20 > everyone > can actually compile-test it. >=20 > Anyway, I have just sent Alex a fixup patch to squash as code says =20 > more > than a thousand words - now you have no more excuses for the future. =20 > :P Thanks. > > When would kvm_openpic_realize/unrealize/finalize get called? >=20 > Today realize is called as part of your qdev_init_nofail() or via > object_property_set_bool(). > In the future it will be called last thing before the machine starts > executing - therefore moving basic initializations into instance_init. > Documented in include/hw/qdev-core.h. >=20 > > Note that we must create the kernel side of the device in > > kvm_openpic_init(), because we need to return failure if it's not > > supported, so that the platform can fall back onto creating a normal > > QEMU openpic instead. >=20 > No, you don't have to and you shouldn't. SysBusDeviceClass::init is > legacy cruft. As you can see from my patch, kvm_openpic_realize allows > for even better error reporting. My concern isn't how good the error reporting is, but how early we =20 detect the error. If qdev_init_nofail() goes away, then when will =20 platform code have a chance to create a non-KVM openpic device =20 instead? From "devices must not create children during @realize" it =20 sounds like it might be too late at that point. Originally, I had the creation of the kernel MPIC object done by a =20 factory function. If the kernel object was able to be created, then =20 the function created a qdev object and passed the kernel ID as a =20 property. Otherwise, it returned NULL so that platform code knew to =20 create a "normal" openpic device (or error out if the user explicitly =20 asked for an in-kernel pic). Alex asked me to change this to the =20 qdev_init_nofail() approach. It sounds like we may need to change back. -Scott=