From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58008) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0NXG-0007Xt-E3 for qemu-devel@nongnu.org; Thu, 04 Jun 2015 01:17:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z0NX9-00084n-TA for qemu-devel@nongnu.org; Thu, 04 Jun 2015 01:17:46 -0400 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:56692) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0NX9-00083s-8S for qemu-devel@nongnu.org; Thu, 04 Jun 2015 01:17:39 -0400 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 4 Jun 2015 15:17:36 +1000 From: Nikunj A Dadhania In-Reply-To: <20150603194028.1ea14040@thh440s> References: <1433330757-6043-1-git-send-email-nikunj@linux.vnet.ibm.com> <1433330757-6043-6-git-send-email-nikunj@linux.vnet.ibm.com> <20150603194028.1ea14040@thh440s> Date: Thu, 04 Jun 2015 10:46:31 +0530 Message-ID: <87eglsxdcg.fsf@abhimanyu.in.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 5/6] spapr_pci: populate ibm,loc-code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: qemu-devel@nongnu.org, aik@ozlabs.ru, agraf@suse.de, mdroth@linux.vnet.ibm.com, qemu-ppc@nongnu.org, david@gibson.dropbear.id.au Thomas Huth writes: > On Wed, 3 Jun 2015 16:55: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 represents= the location >> code for that hardware entity. >>=20 >> Populate ibm,loc-code. >>=20 >> 1) PCI passthru devices need to identify with its own ibm,loc-code >> available on the host. In failure cases use: >> vfio_:::. >>=20 >> 2) Emulated devices encode as following: >> qemu_:::. >>=20 >> Signed-off-by: Nikunj A Dadhania >> --- >> hw/ppc/spapr_pci.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++= ------- >> 1 file changed, 68 insertions(+), 10 deletions(-) >>=20 >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index 4226468..986bb21 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -746,6 +746,60 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bu= s, void *opaque, int devfn) >> return &phb->iommu_as; >> } >>=20=20 >> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDevic= e *pdev) >> +{ >> + char *path =3D NULL, *buf =3D NULL, *host =3D NULL; >> + >> + /* Get the PCI VFIO host id */ >> + host =3D object_property_get_str(OBJECT(pdev), "host", NULL); >> + if (!host) { >> + goto err_out; >> + } >> + >> + /* Construct the path of the file that will give us the DT location= */ >> + path =3D g_strdup_printf("/sys/bus/pci/devices/%s/devspec", host); >> + g_free(host); >> + if (!path || !g_file_get_contents(path, &buf, NULL, NULL)) { >> + goto err_out; >> + } >> + g_free(path); >> + >> + /* Construct and read from host device tree the loc-code */ >> + path =3D g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf); >> + g_free(buf); >> + if (!path || !g_file_get_contents(path, &buf, NULL, NULL)) { >> + goto err_out; >> + } >> + return buf; > > I'd maybe change the above 4 lines into: > > if (path && g_file_get_contents(path, &buf, NULL, NULL)) { > return buf; > } > > so that you can get rid of one goto here. Wouldnt make much of a difference though !=20 >> +err_out: >> + g_free(path); >> + return NULL; >> +} >> + >> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pde= v) >> +{ >> + char *buf; >> + const char *devtype =3D "qemu"; >> + uint32_t busnr =3D pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(p= dev)))); >> + >> + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { >> + buf =3D spapr_phb_vfio_get_loc_code(sphb, pdev); >> + if (buf) { >> + return buf; >> + } >> + devtype =3D "vfio"; >> + } >> + /* >> + * For emulated devices and VFIO-failure case, make up >> + * the loc-code. >> + */ >> + buf =3D g_strdup_printf("%s_%s:%04x:%02x:%02x.%x", >> + devtype, pdev->name, sphb->index, busnr, >> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); >> + return buf; >> +} >> + >> /* Macros to operate with address in OF binding to PCI */ >> #define b_x(x, p, l) (((x) & ((1<<(l))-1)) << (p)) >> #define b_n(x) b_x((x), 31, 1) /* 0 if relocatable */ >> @@ -884,11 +938,12 @@ static void populate_resource_props(PCIDevice *d, = ResourceProps *rp) >>=20=20 >> static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int o= ffset, >> int phb_index, int drc_index, >> - const char *drc_name) >> + sPAPRPHBState *sphb) >> { >> ResourceProps rp; >> bool is_bridge =3D false; >> int pci_status; >> + char *buf =3D NULL; > > Is the "=3D NULL" required here? If not, please remove, newer version > of gcc tend to complain otherwise. > >> if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) =3D=3D >> PCI_HEADER_TYPE_BRIDGE) { >> @@ -949,10 +1004,15 @@ static int spapr_populate_pci_child_dt(PCIDevice = *dev, void *fdt, int offset, >> * processed by OF beforehand >> */ >> _FDT(fdt_setprop_string(fdt, offset, "name", "pci")); >> - if (drc_name) { >> - _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name, >> - strlen(drc_name))); >> + buf =3D spapr_phb_get_loc_code(sphb, dev); >> + if (!buf) { >> + error_report("Failed setting the ibm,loc-code"); >> + return -1; >> } >> + >> + _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf)); > > I wonder whether this will cause some Coverity warnings later ... Not sure about that. > the _FDT macro can return immediately (ugh, return in a macro ... IMHO > a bad idea...). buf is not freed in that case, and that might trigger a > warning... You are right buf may not get freed in case of FDT error. In this case let me open code this macro here with proper error handling: err =3D fdt_setprop_string(fdt, offset, "ibm,loc-code", buf); g_free(buf); if (err < 0) { return err; } Regards Nikunj