From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35819) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SdgHC-0008AO-Rj for qemu-devel@nongnu.org; Sun, 10 Jun 2012 07:25:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SdgHA-0004Ld-2K for qemu-devel@nongnu.org; Sun, 10 Jun 2012 07:25:46 -0400 Received: from mout.web.de ([212.227.17.11]:51820) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SdgH9-0004LP-If for qemu-devel@nongnu.org; Sun, 10 Jun 2012 07:25:43 -0400 Message-ID: <4FD48435.1020003@web.de> Date: Sun, 10 Jun 2012 13:25:41 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <20120610093521.GA6250@redhat.com> <4FD4738C.6020507@web.de> <20120610104929.GG6250@redhat.com> <4FD47C7D.8040706@web.de> <20120610105803.GI6250@redhat.com> <4FD47E53.70003@web.de> <20120610111742.GK6250@redhat.com> In-Reply-To: <20120610111742.GK6250@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig1BF40BCB368224072B234629" Subject: Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Alex Williamson , qemu-devel This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig1BF40BCB368224072B234629 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2012-06-10 13:17, Michael S. Tsirkin wrote: > On Sun, Jun 10, 2012 at 01:00:35PM +0200, Jan Kiszka wrote: >> On 2012-06-10 12:58, Michael S. Tsirkin wrote: >>> On Sun, Jun 10, 2012 at 12:52:45PM +0200, Jan Kiszka wrote: >>>> On 2012-06-10 12:49, Michael S. Tsirkin wrote: >>>>> On Sun, Jun 10, 2012 at 12:14:36PM +0200, Jan Kiszka wrote: >>>>>> On 2012-06-10 11:35, Michael S. Tsirkin wrote: >>>>>>> On Mon, Jun 04, 2012 at 10:52:21AM +0200, Jan Kiszka wrote: >>>>>>>> Add a property to receive a fully qualified PCI device address. >>>>>>>> >>>>>>>> Will be used by KVM device assignment. >>>>>>>> >>>>>>>> Signed-off-by: Jan Kiszka >>>>>>> >>>>>>> I'd like to ponder this a bit more. What bothers me is that this= mixes >>>>>>> two things: >>>>>>> - addressing of qemu devices >>>>>>> Using full device addresses there is a legacy feature, >>>>>>> users really should supply the parent bus and >>>>>>> the bus local address. >>>>>>> - addressing devices on the linux host for assignment >>>>>>> It so happens that the syntax matches >>>>>>> the legacy naming very closely, >>>>>>> but conceptually is completely unrelated >>>>>> >>>>>> We can keep code duplications, of course. >>>>>> >>>>>>> >>>>>>>> --- >>>>>>>> hw/qdev-properties.c | 48 +++++++++++++++++++++++++++++++++++= +++++++++++++ >>>>>>>> hw/qdev.h | 3 +++ >>>>>>>> 2 files changed, 51 insertions(+), 0 deletions(-) >>>>>>>> >>>>>>>> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c >>>>>>>> index 32e41f1..6634f22 100644 >>>>>>>> --- a/hw/qdev-properties.c >>>>>>>> +++ b/hw/qdev-properties.c >>>>>>>> @@ -946,6 +946,54 @@ PropertyInfo qdev_prop_pci_devfn =3D { >>>>>>>> .max =3D 0xFFFFFFFFULL, >>>>>>>> }; >>>>>>>> =20 >>>>>>>> +static void get_pci_devaddr(Object *obj, Visitor *v, void *opaq= ue, >>>>>>>> + const char *name, Error **errp) >>>>>>>> +{ >>>>>>>> + DeviceState *dev =3D DEVICE(obj); >>>>>>>> + Property *prop =3D opaque; >>>>>>>> + PCIDeviceAddress *addr =3D qdev_get_prop_ptr(dev, prop); >>>>>>>> + char buffer[10 + 3 + 1]; >>>>>>>> + char *p =3D buffer; >>>>>>>> + >>>>>>>> + snprintf(buffer, sizeof(buffer), "%04x:%02x:%02x.%02x", >>>>>>>> + addr->domain, addr->bus, addr->slot, addr->functio= n); >>>>>>>> + >>>>>>>> + visit_type_str(v, &p, name, errp); >>>>>>>> +} >>>>>>>> + >>>>>>>> +static void set_pci_devaddr(Object *obj, Visitor *v, void *opaq= ue, >>>>>>>> + const char *name, Error **errp) >>>>>>>> +{ >>>>>>>> + DeviceState *dev =3D DEVICE(obj); >>>>>>>> + Property *prop =3D opaque; >>>>>>>> + PCIDeviceAddress *addr =3D qdev_get_prop_ptr(dev, prop); >>>>>>>> + Error *local_err =3D NULL; >>>>>>>> + char *str; >>>>>>>> + >>>>>>>> + if (dev->state !=3D DEV_STATE_CREATED) { >>>>>>>> + error_set(errp, QERR_PERMISSION_DENIED); >>>>>>>> + return; >>>>>>>> + } >>>>>>>> + >>>>>>>> + visit_type_str(v, &str, name, &local_err); >>>>>>>> + if (local_err) { >>>>>>>> + error_propagate(errp, local_err); >>>>>>>> + return; >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (qemu_parse_pci_devaddr(str, addr, >>>>>>>> + PCI_DEVADDR_WITH_DOM_BUS_OPT | >>>>>>>> + PCI_DEVADDR_WITH_FUNC) < 0) { >>>>>>>> + error_set_from_qdev_prop_error(errp, EINVAL, dev, prop,= str); >>>>>>>> + } >>>>>>>> +} >>>>>>>> + >>>>>>>> +PropertyInfo qdev_prop_pci_devaddr =3D { >>>>>>>> + .name =3D "pci-devaddr", >>>>>>> >>>>>>> This is a very confusing name. Something like host-pci-address? >>>>>> >>>>>> That might be an option. >>>>>> >>>>>>> This also should be built on linux only. >>>>>> >>>>>> Why, what do we gain with #ifdefs? And isn't the addressing concep= t generic? >>>>> >>>>> Not the XXX:XX.X format. And not the concept of a domain. >>>>> >>>>>>> Can this be part of device assignment code instead of qdev? >>>>>> >>>>>> How does VFIO address their host devices? >>>>> >>>>> You get an fd I think. I think you don't need to know the host addr= ess. >>>> >>>> vfio_pci.c contains a nice function called "parse_hostaddr". You may= >>>> guess what it does. ;) >>> >>> Interesting. Why? This looks strange to me: >>> I would expect the admin to bind a device to vfio >>> the way it's now bound to a stub. >>> The pass /dev/vfioXXX to qemu. >> >> That's the "libvirt way". We surely also want the "qemu command line >> way" for which this kind of service is needed. >> >> Jan >> >=20 > Yes, I imagine the qemu command line passing in /dev/vfioXXX, > the libvirt way will pass in an fd for above. No? As far as I understand the API, there is no device file per assigned device. Also, this [domain:]bus:dev.fn format is more handy for the command line. Jan --------------enig1BF40BCB368224072B234629 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk/UhDUACgkQitSsb3rl5xQjzgCfYeMSu578Umifu8yXjDI2Dy9I uuYAoL4q9wx4vH9Zj0Hbn8GT15Djxo7a =1Ub+ -----END PGP SIGNATURE----- --------------enig1BF40BCB368224072B234629--