Thomas Huth <th...@redhat.com> writes: > On Tue, 5 May 2015 14:23:56 +0530 > Nikunj A Dadhania <nik...@linux.vnet.ibm.com> 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 “ibm,loc-code” property with a value that represents the location >> code for that hardware entity. >> >> 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_<name>:<phb-index>:<slot>.<fn> >> >> Signed-off-by: Nikunj A Dadhania <nik...@linux.vnet.ibm.com> >> --- >> hw/ppc/spapr_pci.c | 86 >> +++++++++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 75 insertions(+), 11 deletions(-) >> >> 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; >> } >> >> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value) >> +{ >> + char *host; >> + char path[PATH_MAX]; >> + >> + host = 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, PCIDevice >> *pdev) >> +{ >> + char path[PATH_MAX], *buf = 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.
Sure > >> +} >> + >> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >> +{ >> + char *path = 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_<name>:<phb-index>:<slot>.<fn> >> + */ >> + 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 *pdev) >> +{ >> + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci") != 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 > 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_<name>:<phb-index>:<slot>.<fn>" So more checks would be needed here in that case: if (vfio) { buf = vfio_get_loc_code() if (!buf) buf = 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) >> } >> >> static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int >> offset, >> - int phb_index, int drc_index, >> - const char *drc_name) >> + sPAPRPHBState *phb, int drc_index) > > 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