From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46487) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ypsc2-0003jg-MZ for qemu-devel@nongnu.org; Wed, 06 May 2015 02:15:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ypsbz-0000os-HA for qemu-devel@nongnu.org; Wed, 06 May 2015 02:15:18 -0400 Received: from e28smtp02.in.ibm.com ([122.248.162.2]:35177) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ypsby-0000mx-T1 for qemu-devel@nongnu.org; Wed, 06 May 2015 02:15:15 -0400 Received: from /spool/local by e28smtp02.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 6 May 2015 11:45:11 +0530 From: Nikunj A Dadhania In-Reply-To: <20150505180356.36f77817@thh440s> 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> Date: Wed, 06 May 2015 11:44:59 +0530 Message-ID: <87h9rqjkpo.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: 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 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 represents= 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 *bu= s, void *opaque, int devfn) >> return &phb->iommu_as; >> } >>=20=20 >> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **va= lue) >> +{ >> + 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", hos= t); >> + g_free(host); >> + >> + return g_file_get_contents(path, value, NULL, NULL); >> +} >> + >> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDevic= e *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", bu= f); >> + 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. Sure > >> +} >> + >> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pd= ev) >> +{ >> + 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") ... Oh right, one option is to drop that... > >> + * 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 *pde= v) >> +{ >> + 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)? ... 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: "vfio_::." So more checks would be needed here in that case: if (vfio) { buf =3D vfio_get_loc_code() if (!buf)=20 buf =3D cook_vfio_get_loc_code() return buf; } else { ... } > >> +} >> + >> /* 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 */ >> @@ -881,12 +945,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 *phb, int drc_inde= x) > > As David already noted, the sPAPRPHBState related hunks likely rather > belong to an earlier patch already? Yes, I will take care, this resuled while juggling with the patches :-) Regards Nikunj