From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45330) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UnBPw-0008N3-RT for qemu-devel@nongnu.org; Thu, 13 Jun 2013 13:34:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UnBPt-0001RQ-Ey for qemu-devel@nongnu.org; Thu, 13 Jun 2013 13:34:36 -0400 Date: Thu, 13 Jun 2013 12:32:30 -0500 From: Scott Wood In-Reply-To: <51B9A69D.4070808@suse.de> (from afaerber@suse.de on Thu Jun 13 06:01:49 2013) Message-ID: <1371144750.2028.14@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/13/2013 06:01:49 AM, Andreas F=E4rber wrote: > Am 12.06.2013 22:32, schrieb Scott Wood: > > +typedef struct KVMOpenPICState { > > + SysBusDevice busdev; >=20 > SysBusDevice parent_obj; please! >=20 > http://wiki.qemu.org/QOMConventions The word "QOMConventions" doesn't exist once in the QEMU source tree. =20 How is one supposed to know that this documentation exists? :-P Plus, this is just copied from the non-KVM MPIC file, and I see many =20 other instances throughout the source tree. > > +static int kvm_openpic_init(SysBusDevice *dev) >=20 > 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 have =20 not found much documentation. Again, this is patterned after the =20 existing non-KVM openpic file. > > +{ > > + KVMState *s =3D kvm_state; > > + KVMOpenPICState *opp =3D FROM_SYSBUS(typeof(*opp), dev); >=20 > NACK, please introduce your own KVM_OPENPIC(obj) cast macro instead =20 > 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 something =20 just because it's "been a topic". Lately it's been too much for me to =20 even scan the subject lines looking for things that look important, =20 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 of =20 the day, then this could be updated to match... Also note that this is hardly the first time this patch has been posted =20 (v1 was a few weeks ago, and there were RFC patches well before that). =20 The first version may have even preceded this "topic". This seems a =20 bit late in the process for a bunch of style churn, when existing code =20 hasn't been updated. > > + int kvm_openpic_model; > > + struct kvm_create_device cd =3D {0}; > > + int ret, i; > > + > > + if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) { > > + return -EINVAL; > > + } > > + > > + switch (opp->model) { > > + case OPENPIC_MODEL_FSL_MPIC_20: > > + kvm_openpic_model =3D KVM_DEV_TYPE_FSL_MPIC_20; > > + break; > > + > > + case OPENPIC_MODEL_FSL_MPIC_42: > > + kvm_openpic_model =3D KVM_DEV_TYPE_FSL_MPIC_42; > > + break; > > + > > + default: > > + return -EINVAL; > > + } >=20 > If there's only two supported enum-style options, why not make it two > devices with the value set as a class field? I'm not 100% sure what you mean here, but it is not intended that there =20 will always be only two supported options. At the least we will =20 probably support v4.3 at some point. -Scott=