From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34064) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqDs3-0005x8-0s for qemu-devel@nongnu.org; Thu, 07 May 2015 00:57:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YqDrx-00035B-Tn for qemu-devel@nongnu.org; Thu, 07 May 2015 00:57:14 -0400 Received: from e28smtp01.in.ibm.com ([122.248.162.1]:45126) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqDrx-00034g-9z for qemu-devel@nongnu.org; Thu, 07 May 2015 00:57:09 -0400 Received: from /spool/local by e28smtp01.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 7 May 2015 10:27:06 +0530 From: Nikunj A Dadhania In-Reply-To: <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> <20150507003256.GA20149@voom.redhat.com> Date: Thu, 07 May 2015 10:26:57 +0530 Message-ID: <87twvphtnq.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: David Gibson Cc: agraf@suse.de, Thomas Huth , aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org David Gibson writes: > 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 represe= nts 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=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", = host); >> >> + g_free(host); >> >> + >> >> + return g_file_get_contents(path, value, NULL, NULL); >> >> +} >> >> + >> >> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDe= vice *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 o= ut >> > >> > 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->devf= n)); >> >> + return path; >> >> +} >> >> + >> >> + >> >> +static char *spapr_ibm_get_loc_code(sPAPRPHBState *sphb, PCIDevice *= pdev) >> >> +{ >> >> + 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. I already have the fancy version now, will send it for review. Regards Nikunj