From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48267) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkseF-00087q-1c for qemu-devel@nongnu.org; Tue, 18 Dec 2012 03:35:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TkseC-0005I4-81 for qemu-devel@nongnu.org; Tue, 18 Dec 2012 03:35:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36813) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkseC-0005Hx-0p for qemu-devel@nongnu.org; Tue, 18 Dec 2012 03:35:32 -0500 Date: Tue, 18 Dec 2012 10:38:36 +0200 From: "Michael S. Tsirkin" Message-ID: <20121218083836.GB19090@redhat.com> References: <50CF629A.4090005@redhat.com> <50CF6384.9040809@suse.de> <20121217204811.GB30842@redhat.com> <50CF8AE7.1090803@suse.de> <20121217211831.GA11153@redhat.com> <50CF97EB.9070200@suse.de> <20121217225852.GA12661@redhat.com> <50CFB51E.5030108@suse.de> <20121218003016.GA15241@redhat.com> <50CFBD6C.9010709@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <50CFBD6C.9010709@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] virtio: make bindings typesafe List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: Anthony Liguori , Jan Kiszka , qemu-devel@nongnu.org, Alexander Graf , Christian Borntraeger , Paolo Bonzini , fred.konrad@greensocs.com On Tue, Dec 18, 2012 at 01:48:44AM +0100, Andreas F=E4rber wrote: > Am 18.12.2012 01:30, schrieb Michael S. Tsirkin: > > On Tue, Dec 18, 2012 at 01:13:18AM +0100, Andreas F=E4rber wrote: > >> Am 17.12.2012 23:58, schrieb Michael S. Tsirkin: > >>> On Mon, Dec 17, 2012 at 11:08:43PM +0100, Andreas F=E4rber wrote: > >>>> Am 17.12.2012 22:18, schrieb Michael S. Tsirkin: > >>>>> On Mon, Dec 17, 2012 at 10:13:11PM +0100, Andreas F=E4rber wrote: > >>>>>> Am 17.12.2012 21:48, schrieb Michael S. Tsirkin: > >>>>>>> On Mon, Dec 17, 2012 at 07:25:08PM +0100, Andreas F=E4rber wrot= e: > >>>>>>>> Am 17.12.2012 19:21, schrieb Paolo Bonzini: > >>>>>>>>> Il 17/12/2012 18:55, Andreas F=E4rber ha scritto: > >>>>>>>>>> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin: > >>>>>>>>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > >>>>>>>>>>> index 3ea4140..63ae888 100644 > >>>>>>>>>>> --- a/hw/virtio-pci.c > >>>>>>>>>>> +++ b/hw/virtio-pci.c > >>>>>>>>>>> @@ -98,34 +98,34 @@ bool virtio_is_big_endian(void); > >>>>>>>>>>> =20 > >>>>>>>>>>> /* virtio device */ > >>>>>>>>>>> =20 > >>>>>>>>>>> -static void virtio_pci_notify(void *opaque, uint16_t vecto= r) > >>>>>>>>>>> +static void virtio_pci_notify(DeviceState *d, uint16_t vec= tor) > >>>>>>>>>>> { > >>>>>>>>>>> - VirtIOPCIProxy *proxy =3D opaque; > >>>>>>>>>>> + VirtIOPCIProxy *proxy =3D container_of(d, VirtIOPCIPro= xy, pci_dev.qdev); > >>>>>>>>>> > >>>>>>>>>> Nack. This is going the wrong direction QOM-wise and you amo= ng all > >>>>>>>>>> others know that from PCI host bridges! > >>>>>>>>> > >>>>>>>>> Well, that's just a difference of VIRTIO_PCI_PROXY(d) vs. con= tainer_of. > >>>>>>>> > >>>>>>>> VIRTIO_PCI_PROXY(d) would be acceptable, sure. But as-is this = patch just > >>>>>>>> pushes unnecessary work on Fred, me, you or anyone else who wo= rks with QOM. > >>>>>>> > >>>>>>> What's VIRTIO_PCI_PROXY? Note this is data path we do not want = extra > >>>>>>> code. > >>>>>> > >>>>>> My complaint is the direct access of pci_dev, qdev, etc. parent = fields > >>>>>> in many places as the main change of this patch. Those mean more= places > >>>>>> to touch in a future patch. > >>>>>> > >>>>>> Use of any new-style macro hiding these - wherever the particula= r one > >>>>>> suggested may be defined or whether it needs to be added - is be= tter. > >>>>>> > >>>>>> If performance of dynamic_cast is an issue - something I'd leave= you to > >>>>>> discuss with Anthony - you can just do a C cast directly. Just d= on't > >>>>>> spread this qdev paradigm further please. > >>>>> > >>>>> OK so just > >>>>> > >>>>> #define VIRTIO_PCI_PROXY(d) container_of(d, VirtIOPCIProxy, pci_d= ev.qdev) > >>>>> > >>>>> is OK with you? > >>>> > >>>> Well, at least it's better than inlining it... > >>>> > >>>> I would've expected to see VIRTIO_PCI_PROXY(obj) defined as > >>>> OBJECT_CHECK(VirtIOPCIProxy, (obj), TYPE_something) somewhere. > >>>> > >>>> If, as you imply with "data path", this were a problem, you could = just > >>>> do VirtIOPCIProxy *proxy =3D (VirtIOPCIProxy *)d inline to allow f= or > >>>> VIRTIO_PCI_PROXY() to be used in the QOM sense elsewhere. > >>> > >>> I don't get it - where? > >>> Since we don't do runtime checks we need container_of - > >>> safer than a plain cast. > >>> > >>> Anyway, when you start doing your QOM conversions it will be > >>> easy to do what you like. > >> > >> I don't get what you don't get > >=20 > > Wha'ts the QOM way to get virtio pci proxy from > > devicestate? > > C cast is not what I am looking for. >=20 > Looking into virtio-pci.c it looks like virtio has a similar deficiency > as EHCI USB (my recent series): We lack an abstract intermediate type > TYPE_VIRTIO_PCI_PROXY to match the struct VirtIOPCIProxy shared by its > subtypes: >=20 > Object > - DeviceState > - PCIDevice > - VirtIOPCIProxy > - virtio-scsi-pci > - virtio-rng-pci > ... >=20 > Not sure if that can be extracted from Fred's series already; otherwise > I can send you a patch. I'd like to avoid the dependency - let me do the rework using simple container_of meanwhile, then Fred's series can change the cast in a single place. > Then you can do the mentioned: >=20 > #define VIRTIO_PCI_PROXY(obj) \ > OBJECT_CHECK(VirtIOPCIProxy, (obj), TYPE_VIRTIO_PCI_PROXY) >=20 > DeviceState *dev =3D ...; > VirtIOPCIProxy *proxy =3D VIRTIO_PCI_PROXY(dev); >=20 > where you consider it acceptable performance-wise and a > FAST_VIRTIO_PCI_PROXY_FROM_DEVICE(dev) or so elsewhere. >=20 > Andreas >=20 > --=20 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn= berg