From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51281) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqA6c-0007Fr-Ps for qemu-devel@nongnu.org; Wed, 06 May 2015 20:56:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YqA6X-0001Wb-Bw for qemu-devel@nongnu.org; Wed, 06 May 2015 20:56:02 -0400 Date: Thu, 7 May 2015 10:32:56 +1000 From: David Gibson Message-ID: <20150507003256.GA20149@voom.redhat.com> References: <1430816036-26408-1-git-send-email-nikunj@linux.vnet.ibm.com> <1430816036-26408-7-git-send-email-nikunj@linux.vnet.ibm.com> <20150505180356.36f77817@thh440s> <87h9rqjkpo.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="TB36FDmn/VVEgNH/" Content-Disposition: inline In-Reply-To: <87h9rqjkpo.fsf@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH v3 6/6] spapr_pci: populate ibm,loc-code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikunj A Dadhania Cc: agraf@suse.de, Thomas Huth , aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org --TB36FDmn/VVEgNH/ Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, May 06, 2015 at 11:44:59AM +0530, Nikunj A Dadhania wrote: > Thomas Huth writes: >=20 > > On Tue, 5 May 2015 14:23:56 +0530 > > Nikunj A Dadhania wrote: > > > >> Each hardware instance has a platform unique location code. The OF > >> device tree that describes a part of a hardware entity must include > >> the =E2=80=9Cibm,loc-code=E2=80=9D property with a value that represen= ts the location > >> code for that hardware entity. > >>=20 > >> Populate ibm,loc-code. > >> 1) PCI passthru devices need to identify with its own ibm,loc-code > >> available on the host. > >> 2) Emulated devices encode as following: > >> qemu_::. > >>=20 > >> Signed-off-by: Nikunj A Dadhania > >> --- > >> hw/ppc/spapr_pci.c | 86 +++++++++++++++++++++++++++++++++++++++++++++= ++------- > >> 1 file changed, 75 insertions(+), 11 deletions(-) > >>=20 > >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > >> index cbd5661..eacf0bd 100644 > >> --- a/hw/ppc/spapr_pci.c > >> +++ b/hw/ppc/spapr_pci.c > >> @@ -744,6 +744,70 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *= bus, void *opaque, int devfn) > >> return &phb->iommu_as; > >> } > >> =20 > >> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **= value) > >> +{ > >> + char *host; > >> + char path[PATH_MAX]; > >> + > >> + host =3D object_property_get_str(OBJECT(pdev), "host", NULL); > >> + if (!host) { > >> + return false; > >> + } > >> + > >> + snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/devspec", h= ost); > >> + g_free(host); > >> + > >> + return g_file_get_contents(path, value, NULL, NULL); > >> +} > >> + > >> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDev= ice *pdev) > >> +{ > >> + char path[PATH_MAX], *buf =3D NULL; > >> + > >> + /* We have a vfio host bridge lets get the path. */ > >> + if (!spapr_phb_vfio_get_devspec_value(pdev, &buf)) { > >> + return NULL; > >> + } > >> + > >> + snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", = buf); > >> + g_free(buf); > >> + > >> + if (g_file_get_contents(path, &buf, NULL, NULL)) { > >> + return buf; > >> + } else { > >> + return NULL; > >> + } > > > > Minor idea for an optimization: g_file_get_contents() should set buf to > > NULL in case of errors anyway, so you could omit the "if" and > > "return NULLL" and simply always "return buf" here. >=20 > Sure >=20 > > > >> +} > >> + > >> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *= pdev) > >> +{ > >> + char *path =3D g_malloc(PATH_MAX); > >> + > >> + if (!path) { > >> + return NULL; > >> + } > >> + > >> + /* > >> + * For non-vfio devices and failures make up the location code out > > > > This comment ("and failures") ... >=20 > Oh right, one option is to drop that... >=20 > > > >> + * of the name, slot and function. > >> + * > >> + * qemu_::. > >> + */ > >> + snprintf(path, PATH_MAX, "qemu_%s:%02d:%02d.%1d", pdev->name, > >> + sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn= )); > >> + return path; > >> +} > >> + > >> + > >> +static char *spapr_ibm_get_loc_code(sPAPRPHBState *sphb, PCIDevice *p= dev) > >> +{ > >> + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci") !=3D NULL) { > >> + return spapr_phb_vfio_get_loc_code(sphb, pdev); > >> + } else { > >> + return spapr_phb_get_loc_code(sphb, pdev); > >> + } > > > > ... does not match quite the behavior here, as far as I can see. I > > guess you also wanted to fall back to spapr_phb_get_loc_code() in case= =20 > > spapr_phb_vfio_get_loc_code() did not work as expected (eg. when the > > access to the /sys or /proc filesystem failed)? >=20 > ... In current case, if getting vfio device loc_code fails, we would not > add any ibm,loc_code info. I thought that would be the proper > behaviour. As "qemu_" would indicate it as an emulated device, which is > not true. Or encode it as follows: >=20 > "vfio_::." >=20 > So more checks would be needed here in that case: >=20 > if (vfio) { > buf =3D vfio_get_loc_code() > if (!buf)=20 > buf =3D cook_vfio_get_loc_code() > return buf; > } else { ... } If you can't determine the loc code, I think just leaving it out is probably the best option for now. We can add fancier fallbacks in future if they seem like they make sense. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --TB36FDmn/VVEgNH/ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVSrK4AAoJEGw4ysog2bOSPlYQANY+ajBedXvhLHUkTsSAiVBY bwCX1OJo+qttocZRcjmIf6F4uv1g6k771wGLfSso2MokKINuGVir3G4JMvGWS1iR K026O09kyIKHYro1SKPMGKW5affZ4h9n7V+b7y/VWj/2EzscKxsdEMU27hH6ZIzJ k0M4XZe1i9OZdq32NqfSS++3CdQqja208AFui+JF+0FLC14AMZgVKHJHGFYSAJDE y0gaq3mm/qDvYhhA4FazIJrZ9Mj6SEenMgXgnHUKz6y0poIZbigt5D0DNGR7x72V eFaNG+rrakc1/rIP8o/VFkOxrw4iXVRvC7BDdhbu8tQlaSAAxM3iNLW04LglANor 5ej1X4TVMHhvdhnkmkDOwPZ0YICpoYF5a9ucq+B96Esww+c60qIJqZtetKgy59mI tdjflSe+6CU0xopUhRr35iVgJRr9RhDnMqpkGY2ISAm9h1KkHcGmyBFuU+q3QI1k jze960JdV6G2alOzerUeApNz79rrygggEB8LVbjO/x1/jcLskKi92SwvJ55NZ1vO gc9UMvFKtHGoE2cQV8JkvZlsDzJdfgSs9TD+IOx/42vcQnrgDSoalwduG1Rs6u35 W5s/LJyThs/Iu+C1rZmjs/lNXzZlmMnS/yIhAnW5iSrhIVf3gzYuVgAq9YRdsnqv iaLZjNftpKR3jlzK7Aov =aNXx -----END PGP SIGNATURE----- --TB36FDmn/VVEgNH/--