From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43924) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UncwQ-0006Wx-Bm for qemu-devel@nongnu.org; Fri, 14 Jun 2013 18:58:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UncwN-000340-Bk for qemu-devel@nongnu.org; Fri, 14 Jun 2013 18:57:58 -0400 Received: from co9ehsobe003.messaging.microsoft.com ([207.46.163.26]:24522 helo=co9outboundpool.messaging.microsoft.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UncwN-00033r-0m for qemu-devel@nongnu.org; Fri, 14 Jun 2013 18:57:55 -0400 Date: Fri, 14 Jun 2013 17:57:44 -0500 From: Scott Wood In-Reply-To: <51BB2FC6.4060401@suse.de> (from afaerber@suse.de on Fri Jun 14 09:59:18 2013) Message-ID: <1371250664.2996.21@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/14/2013 09:59:18 AM, Andreas F=E4rber wrote: > Am 13.06.2013 19:32, schrieb Scott Wood: > > On 06/13/2013 06:01:49 AM, Andreas F=E4rber wrote: > >> Am 12.06.2013 22:32, schrieb Scott Wood: > >> > +typedef struct KVMOpenPICState { > >> > + SysBusDevice busdev; > >> > >> SysBusDevice parent_obj; please! > >> > >> http://wiki.qemu.org/QOMConventions > > > > The word "QOMConventions" doesn't exist once in the QEMU source =20 > tree. > > How is one supposed to know that this documentation exists? :-P >=20 > I do expect people to read at least the subject of patch series on > qemu-devel, I have over 12000 currently unread e-mails from that list. They are =20 not separated into "patch" and "other". Even among the patches, they =20 get posted at least twice due to the (unique to QEMU and KVM as far as =20 I can tell) practice of reposting everything before a pull request. There's just no way I can keep up with all of this, *plus* all the =20 non-QEMU stuff I need to keep up with. Sorry. I generally rely on =20 Alex to guide me on things like qdev/QOM. Quite frankly, I didn't even =20 realize I was using QOM. I thought this was still qdev. I even create =20 it using qdev_create()... > short of individual review comments. CPU, PReP PCI, > Versatile PCI, ISA and more recently virtio series had been posted. ...which of those would make me think "hmm, there's something in here =20 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 =20 there's room for improvement in how the QEMU community communicates to =20 developers what is expected. Maybe an announcement list that just =20 contains important updates and summaries? Also, even starting on =20 http://wiki.qemu.org/ I don't see any obvious path to get to =20 QOMConventions. It's not even linked to from the main QOM page. I do understand the frustration of having to correct people on the same =20 things over and over -- I experience it myself in other contexts. But =20 there are more constructive ways to deal with it than exclamation =20 points. > > Plus, this is just copied from the non-KVM MPIC file, and I see many > > other instances throughout the source tree. >=20 > Which exactly is the reason for my grief: Your ignorance is making =20 > other > people even more work, and at least Alex should've spotted it - I =20 > can't > review all patches just because they might or might not be touching =20 > on QOM. > Just as we are supposed to not copy old Coding Style in new patches, =20 > we > should be applying new patterns and conventions in new patches, too. I'm usually the first person to complain about bad copy and paste, but =20 this is a situation where the KVM version of openpic is supposed to =20 interface with the rest of the system in exactly the same way as the =20 regular openpic. It really does not make sense to write the glue code =20 from scratch. And it's not as if the rest of QEMU had just been fixed, =20 and this patch reintroduces the old stuff. I count 166 instances of =20 "SysBusDevice busdev" and only 9 instances of "SysBusDevice =20 parent_obj". Perhaps these could all be fixed up in an automated way? And "making other people even more work" goes both ways... > >> > +static int kvm_openpic_init(SysBusDevice *dev) > >> > >> Please make this instance_init + realize functions - "dev" should =20 > rather > >> be reserved for DeviceState. > > > > Could you elaborate? I'm really not familiar with this stuff, and =20 > have > > not found much documentation. Again, this is patterned after the =20 > existing > > non-KVM openpic file. >=20 > static void kvm_openpic_init(Object *obj) should initialize simple > variables, MemoryRegions that don't depend on parameters and any QOM > properties. >=20 > static void kvm_openpic_realize(DeviceState *dev, Error **errp) should > initialize the rest. >=20 > kvm_openpic_unrealize(Device *dev, Error **errp) and > kvm_openpic_finalize(Object *obj) would be their counterparts for =20 > cleanup. When would kvm_openpic_realize/unrealize/finalize get called? Note that we must create the kernel side of the device in =20 kvm_openpic_init(), because we need to return failure if it's not =20 supported, so that the platform can fall back onto creating a normal =20 QEMU openpic instead. Also note that an in-kernel MPIC cannot be destroyed, without =20 destroying the entire VM. So I'm not sure what unrealize/finalize =20 would do. All of this is basically done the way Alex told/showed me to do it. > >> > +{ > >> > + KVMState *s =3D kvm_state; > >> > + KVMOpenPICState *opp =3D FROM_SYSBUS(typeof(*opp), dev); > >> > >> NACK, please introduce your own KVM_OPENPIC(obj) cast macro =20 > instead for > >> new devices - has been a topic for several weeks and months now. > > > > There's way too much traffic on the list for me to know about =20 > something > > just because it's "been a topic". Lately it's been too much for me =20 > to > > even scan the subject lines looking for things that look important, > > given that QEMU is only a fraction of what I spend my time on. > > > > If you'd like to update hw/intc/openpic.c to comply with the style =20 > of > > the day, then this could be updated to match... > > > > Also note that this is hardly the first time this patch has been =20 > posted > > (v1 was a few weeks ago, and there were RFC patches well before =20 > that). > > The first version may have even preceded this "topic". This seems =20 > a bit > > late in the process for a bunch of style churn, when existing code > > hasn't been updated. >=20 > I'm not talking about style churn, I'm talking about using the wrong > infrastructure and making it even more difficult to drop FROM_SYSBUS() > macro. Sigh. From what you said above, it seemed like you were asking me to =20 do this: #define KVM_OPENPIC(obj) FROM_SYSBUS(KVMOpenPICState, (obj)) Do you mean you want me to use DO_UPCAST directly? And is it forbidden =20 for DO_UPCAST to be opencoded? > Again, whether or not some particular other file uses some style =20 > doesn't > mean that it's okay to apply that to new files. Instead of complaining > it would've been a task of five minutes to supply Alex with a fixup > patch to squash/follow-up or to post a v3. QOM realize is merged since > January 2013 and was presented by Anthony in January 2012. It would have taken me much more than 5 minutes because I had no clue =20 what "QOM realize" was, not to mention other unresolved questions about =20 what you would want in a v3. Alex, are you expecting a v3 or are you happy with the version you =20 already put in ppc-next? -Scott=