* [Qemu-devel] [PATCH v4 0/4] spapr_pci: DT field fixes and PCI DT node creation in QEMU @ 2015-05-07 7:21 Nikunj A Dadhania 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 1/4] spapr_pci: encode missing 64-bit memory address space Nikunj A Dadhania ` (3 more replies) 0 siblings, 4 replies; 16+ messages in thread From: Nikunj A Dadhania @ 2015-05-07 7:21 UTC (permalink / raw) To: qemu-devel, david Cc: Nikunj A Dadhania, aik, agraf, mdroth, nikunj.dadhania, qemu-ppc The patch series creates PCI device tree(DT) nodes in QEMU. The new hotplug code needs the device node creation in QEMU. While during boot, nodes were created in SLOF. It makes more sense to consolidate the code to one place for better maintainability. Based on David's spapr-next https://github.com/dgibson/qemu/tree/spapr-next Needs new slof.bin: http://patchwork.ozlabs.org/patch/469303/ Also, patches for populating ibm,loc-code was getting very complicated with use of RTAS/HCALL Changelog V3: * Dropped duplicate macro patches * Squashed Michael's drc_index changes to enumeration patch * Use common create routine for boottime and hotplug case * Proper error handling not depending on g_assert * Encode vfio loc-code if getting loc-code from host fails Changelog V2: * Fix device tree for 64-bit encoding * Fix the class code, was failing xhci * Remove macro duplication * Fix DT fields generation for boot time device (Michael Roth) Changelog v1: * Correct indent problems reported by checkpatch(David Gibson) * Declare sPAPRFDT structure as local (David Gibson) * Re-arrange code to avoid multiple indentation (Alexey Kardashevskiy) Nikunj A Dadhania (4): spapr_pci: encode missing 64-bit memory address space spapr_pci: encode class code including Prog IF register spapr_pci: enumerate and add PCI device tree spapr_pci: populate ibm,loc-code hw/ppc/spapr_pci.c | 283 ++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 238 insertions(+), 45 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v4 1/4] spapr_pci: encode missing 64-bit memory address space 2015-05-07 7:21 [Qemu-devel] [PATCH v4 0/4] spapr_pci: DT field fixes and PCI DT node creation in QEMU Nikunj A Dadhania @ 2015-05-07 7:21 ` Nikunj A Dadhania 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 2/4] spapr_pci: encode class code including Prog IF register Nikunj A Dadhania ` (2 subsequent siblings) 3 siblings, 0 replies; 16+ messages in thread From: Nikunj A Dadhania @ 2015-05-07 7:21 UTC (permalink / raw) To: qemu-devel, david Cc: Nikunj A Dadhania, aik, agraf, mdroth, nikunj.dadhania, qemu-ppc The properties reg/assigned-resources need to encode 64-bit memory address space as part of phys.hi dword. 00 if configuration space 01 if IO region, 10 if 32-bit MEM region 11 if 64-bit MEM region Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> Reviewed-by: Thomas Huth <thuth@redhat.com> --- hw/ppc/spapr_pci.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 4df3a33..ea1a092 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -786,7 +786,13 @@ typedef struct ResourceProps { * phys.hi = 0xYYXXXXZZ, where: * 0xYY = npt000ss * ||| | - * ||| +-- space code: 1 if IO region, 2 if MEM region + * ||| +-- space code + * ||| | + * ||| + 00 if configuration space + * ||| + 01 if IO region, + * ||| + 10 if 32-bit MEM region + * ||| + 11 if 64-bit MEM region + * ||| * ||+------ for non-relocatable IO: 1 if aliased * || for relocatable IO: 1 if below 64KB * || for MEM: 1 if below 1MB @@ -846,6 +852,8 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp) reg->phys_hi = cpu_to_be32(dev_id | b_rrrrrrrr(pci_bar(d, i))); if (d->io_regions[i].type & PCI_BASE_ADDRESS_SPACE_IO) { reg->phys_hi |= cpu_to_be32(b_ss(1)); + } else if (d->io_regions[i].type & PCI_BASE_ADDRESS_MEM_TYPE_64) { + reg->phys_hi |= cpu_to_be32(b_ss(3)); } else { reg->phys_hi |= cpu_to_be32(b_ss(2)); } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v4 2/4] spapr_pci: encode class code including Prog IF register 2015-05-07 7:21 [Qemu-devel] [PATCH v4 0/4] spapr_pci: DT field fixes and PCI DT node creation in QEMU Nikunj A Dadhania 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 1/4] spapr_pci: encode missing 64-bit memory address space Nikunj A Dadhania @ 2015-05-07 7:21 ` Nikunj A Dadhania 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 3/4] spapr_pci: enumerate and add PCI device tree Nikunj A Dadhania 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code Nikunj A Dadhania 3 siblings, 0 replies; 16+ messages in thread From: Nikunj A Dadhania @ 2015-05-07 7:21 UTC (permalink / raw) To: qemu-devel, david Cc: Nikunj A Dadhania, aik, agraf, mdroth, nikunj.dadhania, qemu-ppc Current code missed the Prog IF register. All Class Code, Subclass, and Prog IF registers are needed to identify the accurate device type. For example: USB controllers use the PROG IF for denoting: USB FullSpeed, HighSpeed or SuperSpeed. Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> Reviewed-by: Thomas Huth <thuth@redhat.com> --- hw/ppc/spapr_pci.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index ea1a092..8b02a3e 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -899,8 +899,7 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset, _FDT(fdt_setprop_cell(fdt, offset, "revision-id", pci_default_read_config(dev, PCI_REVISION_ID, 1))); _FDT(fdt_setprop_cell(fdt, offset, "class-code", - pci_default_read_config(dev, PCI_CLASS_DEVICE, 2) - << 8)); + pci_default_read_config(dev, PCI_CLASS_PROG, 3))); if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) { _FDT(fdt_setprop_cell(fdt, offset, "interrupts", pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1))); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v4 3/4] spapr_pci: enumerate and add PCI device tree 2015-05-07 7:21 [Qemu-devel] [PATCH v4 0/4] spapr_pci: DT field fixes and PCI DT node creation in QEMU Nikunj A Dadhania 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 1/4] spapr_pci: encode missing 64-bit memory address space Nikunj A Dadhania 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 2/4] spapr_pci: encode class code including Prog IF register Nikunj A Dadhania @ 2015-05-07 7:21 ` Nikunj A Dadhania 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code Nikunj A Dadhania 3 siblings, 0 replies; 16+ messages in thread From: Nikunj A Dadhania @ 2015-05-07 7:21 UTC (permalink / raw) To: qemu-devel, david Cc: Nikunj A Dadhania, aik, agraf, mdroth, nikunj.dadhania, qemu-ppc All the PCI enumeration and device node creation was off-loaded to SLOF. With PCI hotplug support, code needed to be added to add device node. This creates multiple copy of the code one in SLOF and other in hotplug code. To unify this, the patch adds the pci device node creation in Qemu. For backward compatibility, a flag "qemu,phb-enumerated" is added to the phb, suggesting to SLOF to not do device node creation. Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> [ Squashed Michael's drc_index changes ] Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> --- hw/ppc/spapr_pci.c | 188 ++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 150 insertions(+), 38 deletions(-) diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 8b02a3e..12f1b9c 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -23,6 +23,7 @@ * THE SOFTWARE. */ #include "hw/hw.h" +#include "hw/sysbus.h" #include "hw/pci/pci.h" #include "hw/pci/msi.h" #include "hw/pci/msix.h" @@ -35,6 +36,7 @@ #include "qemu/error-report.h" #include "qapi/qmp/qerror.h" +#include "hw/pci/pci_bridge.h" #include "hw/pci/pci_bus.h" #include "hw/ppc/spapr_drc.h" #include "sysemu/device_tree.h" @@ -742,6 +744,31 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn) return &phb->iommu_as; } + +static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb, + PCIDevice *pdev) +{ + uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)))); + return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI, + (phb->index << 16) | + (busnr << 8) | + pdev->devfn); +} + +static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, + PCIDevice *pdev) +{ + sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev); + sPAPRDRConnectorClass *drck; + + if (!drc) { + return 0; + } + + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); + return drck->get_index(drc); +} + /* 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 */ @@ -879,12 +906,13 @@ 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, + sPAPRPHBState *sphb, const char *drc_name) { ResourceProps rp; bool is_bridge = false; int pci_status; + uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev); if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) == PCI_HEADER_TYPE_BRIDGE) { @@ -945,8 +973,13 @@ 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")); - _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name, strlen(drc_name))); - _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)); + if (drc_name) { + _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name, + strlen(drc_name))); + } + if (drc_index) { + _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)); + } _FDT(fdt_setprop_cell(fdt, offset, "#address-cells", RESOURCE_CELLS_ADDRESS)); @@ -963,30 +996,34 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset, return 0; } +typedef struct sPAPRFDT { + void *fdt; + int node_off; + sPAPRPHBState *sphb; +} sPAPRFDT; + /* create OF node for pci device and required OF DT properties */ -static void *spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev, - int drc_index, const char *drc_name, - int *dt_offset) +static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p, + const char *drc_name) { - void *fdt; - int offset, ret, fdt_size; - int slot = PCI_SLOT(dev->devfn); - int func = PCI_FUNC(dev->devfn); - char nodename[512]; + int offset, ret; + char nodename[64]; + int slot = PCI_SLOT(pdev->devfn); + int func = PCI_FUNC(pdev->devfn); - fdt = create_device_tree(&fdt_size); if (func != 0) { sprintf(nodename, "pci@%d,%d", slot, func); } else { sprintf(nodename, "pci@%d", slot); } - offset = fdt_add_subnode(fdt, 0, nodename); - ret = spapr_populate_pci_child_dt(dev, fdt, offset, phb->index, drc_index, + offset = fdt_add_subnode(p->fdt, p->node_off, nodename); + ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb, drc_name); g_assert(!ret); - - *dt_offset = offset; - return fdt; + if (ret) { + return 0; + } + return offset; } static void spapr_phb_add_pci_device(sPAPRDRConnector *drc, @@ -996,24 +1033,26 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc, { sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); DeviceState *dev = DEVICE(pdev); - int drc_index = drck->get_index(drc); const char *drc_name = drck->get_name(drc); - void *fdt = NULL; - int fdt_start_offset = 0; + int fdt_start_offset = 0, fdt_size; + sPAPRFDT s_fdt = {NULL, 0, NULL}; - /* boot-time devices get their device tree node created by SLOF, but for - * hotplugged devices we need QEMU to generate it so the guest can fetch - * it via RTAS - */ if (dev->hotplugged) { - fdt = spapr_create_pci_child_dt(phb, pdev, drc_index, drc_name, - &fdt_start_offset); + s_fdt.fdt = create_device_tree(&fdt_size); + s_fdt.sphb = phb; + s_fdt.node_off = 0; + fdt_start_offset = spapr_create_pci_child_dt(pdev, &s_fdt, drc_name); + if (!fdt_start_offset) { + error_setg(errp, "Failed to create pci child device tree node"); + goto out; + } } drck->attach(drc, DEVICE(pdev), - fdt, fdt_start_offset, !dev->hotplugged, errp); + s_fdt.fdt, fdt_start_offset, !dev->hotplugged, errp); +out: if (*errp) { - g_free(fdt); + g_free(s_fdt.fdt); } } @@ -1043,16 +1082,6 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc, drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, errp); } -static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb, - PCIDevice *pdev) -{ - uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)))); - return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI, - (phb->index << 16) | - (busnr << 8) | - pdev->devfn); -} - static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler, DeviceState *plugged_dev, Error **errp) { @@ -1482,6 +1511,75 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index) return PCI_HOST_BRIDGE(dev); } +static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev, + void *opaque) +{ + PCIBus *sec_bus; + sPAPRFDT *p = opaque; + int offset; + sPAPRFDT s_fdt; + + offset = spapr_create_pci_child_dt(pdev, p, NULL); + if (!offset) { + error_report("Failed to create pci child device tree node"); + return; + } + + if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) != + PCI_HEADER_TYPE_BRIDGE)) { + return; + } + + sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev)); + if (!sec_bus) { + return; + } + + s_fdt.fdt = p->fdt; + s_fdt.node_off = offset; + s_fdt.sphb = p->sphb; + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), + spapr_populate_pci_devices_dt, + &s_fdt); +} + +static void spapr_phb_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev, + void *opaque) +{ + unsigned int *bus_no = opaque; + unsigned int primary = *bus_no; + unsigned int secondary; + unsigned int subordinate = 0xff; + + if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) == + PCI_HEADER_TYPE_BRIDGE)) { + PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev)); + secondary = *bus_no + 1; + pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1); + pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1); + pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, secondary, 1); + *bus_no = *bus_no + 1; + if (sec_bus) { + pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, subordinate, 1); + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), + spapr_phb_pci_enumerate_bridge, + bus_no); + pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1); + } + } +} + +static void spapr_phb_pci_enumerate(sPAPRPHBState *phb) +{ + PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus; + unsigned int bus_no = 0; + + pci_for_each_device(bus, pci_bus_num(bus), + spapr_phb_pci_enumerate_bridge, + &bus_no); + +} + int spapr_populate_pci_dt(sPAPRPHBState *phb, uint32_t xics_phandle, void *fdt) @@ -1521,6 +1619,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb, cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)}; uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7]; sPAPRTCETable *tcet; + PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus; + sPAPRFDT s_fdt; /* Start populating the FDT */ sprintf(nodename, "pci@%" PRIx64, phb->buid); @@ -1570,6 +1670,18 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb, tcet->liobn, tcet->bus_offset, tcet->nb_table << tcet->page_shift); + /* Walk the bridges and program the bus numbers*/ + spapr_phb_pci_enumerate(phb); + _FDT(fdt_setprop_cell(fdt, bus_off, "qemu,phb-enumerated", 0x1)); + + /* Populate tree nodes with PCI devices attached */ + s_fdt.fdt = fdt; + s_fdt.node_off = bus_off; + s_fdt.sphb = phb; + pci_for_each_device(bus, pci_bus_num(bus), + spapr_populate_pci_devices_dt, + &s_fdt); + ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb), SPAPR_DR_CONNECTOR_TYPE_PCI); if (ret) { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code 2015-05-07 7:21 [Qemu-devel] [PATCH v4 0/4] spapr_pci: DT field fixes and PCI DT node creation in QEMU Nikunj A Dadhania ` (2 preceding siblings ...) 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 3/4] spapr_pci: enumerate and add PCI device tree Nikunj A Dadhania @ 2015-05-07 7:21 ` Nikunj A Dadhania 2015-05-08 7:42 ` Alexey Kardashevskiy 3 siblings, 1 reply; 16+ messages in thread From: Nikunj A Dadhania @ 2015-05-07 7:21 UTC (permalink / raw) To: qemu-devel, david Cc: Nikunj A Dadhania, aik, agraf, mdroth, nikunj.dadhania, qemu-ppc 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. In failure cases use: vfio_<name>:<phb-index>:<slot>.<fn> 2) Emulated devices encode as following: qemu_<name>:<phb-index>:<slot>.<fn> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> --- hw/ppc/spapr_pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 86 insertions(+), 12 deletions(-) diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 12f1b9c..d901007 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -769,6 +769,81 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, return drck->get_index(drc); } +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); + + g_file_get_contents(path, &buf, NULL, NULL); + return buf; +} + +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 make up the location code out + * 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) { + char *buf = spapr_phb_vfio_get_loc_code(sphb, pdev); + + /* + * In case of failures reading the loc-code, make it up + * indicating a vfio device + */ + if (!buf) { + buf = g_malloc(PATH_MAX); + if (!buf) { + return NULL; + } + snprintf(buf, PATH_MAX, "vfio_%s:%02d:%02d.%1d", pdev->name, + sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); + } + return buf; + } else { + return spapr_phb_get_loc_code(sphb, pdev); + } +} + /* 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 */ @@ -906,12 +981,12 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp) } static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset, - sPAPRPHBState *sphb, - const char *drc_name) + sPAPRPHBState *sphb) { ResourceProps rp; bool is_bridge = false; int pci_status; + char *buf = NULL; uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev); if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) == @@ -973,9 +1048,10 @@ 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 = spapr_ibm_get_loc_code(sphb, dev); + if (buf) { + _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf)); + g_free(buf); } if (drc_index) { _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)); @@ -1003,8 +1079,7 @@ typedef struct sPAPRFDT { } sPAPRFDT; /* create OF node for pci device and required OF DT properties */ -static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p, - const char *drc_name) +static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p) { int offset, ret; char nodename[64]; @@ -1017,8 +1092,8 @@ static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p, sprintf(nodename, "pci@%d", slot); } offset = fdt_add_subnode(p->fdt, p->node_off, nodename); - ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb, - drc_name); + + ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb); g_assert(!ret); if (ret) { return 0; @@ -1033,7 +1108,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc, { sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); DeviceState *dev = DEVICE(pdev); - const char *drc_name = drck->get_name(drc); int fdt_start_offset = 0, fdt_size; sPAPRFDT s_fdt = {NULL, 0, NULL}; @@ -1041,7 +1115,7 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc, s_fdt.fdt = create_device_tree(&fdt_size); s_fdt.sphb = phb; s_fdt.node_off = 0; - fdt_start_offset = spapr_create_pci_child_dt(pdev, &s_fdt, drc_name); + fdt_start_offset = spapr_create_pci_child_dt(pdev, &s_fdt); if (!fdt_start_offset) { error_setg(errp, "Failed to create pci child device tree node"); goto out; @@ -1519,7 +1593,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev, int offset; sPAPRFDT s_fdt; - offset = spapr_create_pci_child_dt(pdev, p, NULL); + offset = spapr_create_pci_child_dt(pdev, p); if (!offset) { error_report("Failed to create pci child device tree node"); return; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code Nikunj A Dadhania @ 2015-05-08 7:42 ` Alexey Kardashevskiy 2015-05-19 4:51 ` Nikunj A Dadhania 0 siblings, 1 reply; 16+ messages in thread From: Alexey Kardashevskiy @ 2015-05-08 7:42 UTC (permalink / raw) To: Nikunj A Dadhania, qemu-devel, david Cc: nikunj.dadhania, qemu-ppc, agraf, mdroth On 05/07/2015 05:21 PM, 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 “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. In failure cases use: > vfio_<name>:<phb-index>:<slot>.<fn> > > 2) Emulated devices encode as following: > qemu_<name>:<phb-index>:<slot>.<fn> > > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > --- > hw/ppc/spapr_pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 86 insertions(+), 12 deletions(-) > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 12f1b9c..d901007 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -769,6 +769,81 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, > return drck->get_index(drc); > } > > +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value) Does it have to be a separate function? > +{ > + 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); > + > + g_file_get_contents(path, &buf, NULL, NULL); > + return buf; > +} > + > +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 make up the location code out > + * 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)); g_strdup_printf? > + return path; > +} > + > + > +static char *spapr_ibm_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) s/spapr_ibm_get_loc_code/spapr_phb_get_loc_code/ Strange to see "ibm" in a function name, so far we have only used "ibm" in RTAS handler names :) > +{ > + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci") != NULL) { QEMU does not compare object_dynamic_cast with NULL anywhere, so: if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { > + char *buf = spapr_phb_vfio_get_loc_code(sphb, pdev); > + > + /* > + * In case of failures reading the loc-code, make it up > + * indicating a vfio device > + */ > + if (!buf) { > + buf = g_malloc(PATH_MAX); > + if (!buf) { > + return NULL; > + } > + snprintf(buf, PATH_MAX, "vfio_%s:%02d:%02d.%1d", pdev->name, > + sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); g_strdup_printf? Also, "vfio_%s:%02d:%02d.%1d" looks very similar to "qemu_%s:%02d:%02d.%1d", you could extend spapr_phb_get_loc_code() to take "vfio"/"qemu" as a parameter. > + } > + return buf; > + } else { > + return spapr_phb_get_loc_code(sphb, pdev); > + } > +} > + > /* 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 */ > @@ -906,12 +981,12 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp) > } > > static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset, > - sPAPRPHBState *sphb, > - const char *drc_name) > + sPAPRPHBState *sphb) > { > ResourceProps rp; > bool is_bridge = false; > int pci_status; > + char *buf = NULL; > uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev); > > if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) == > @@ -973,9 +1048,10 @@ 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 = spapr_ibm_get_loc_code(sphb, dev); > + if (buf) { It is rather: if (!buf) { error_report("Bad thing just happened"); return -1; } OR g_assert(!buf); Your code can only return NULL if g_malloc() failed (otherwise it will be at least "(qemu"vfio)_%s:%02d:%02d.%1d") and if this happened, something went horribly bad. > + _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf)); > + g_free(buf); > } > if (drc_index) { > _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)); > @@ -1003,8 +1079,7 @@ typedef struct sPAPRFDT { > } sPAPRFDT; > > /* create OF node for pci device and required OF DT properties */ > -static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p, > - const char *drc_name) > +static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p) > { > int offset, ret; > char nodename[64]; > @@ -1017,8 +1092,8 @@ static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p, > sprintf(nodename, "pci@%d", slot); > } > offset = fdt_add_subnode(p->fdt, p->node_off, nodename); > - ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb, > - drc_name); > + > + ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb); > g_assert(!ret); > if (ret) { > return 0; > @@ -1033,7 +1108,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc, > { > sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); > DeviceState *dev = DEVICE(pdev); > - const char *drc_name = drck->get_name(drc); > int fdt_start_offset = 0, fdt_size; > sPAPRFDT s_fdt = {NULL, 0, NULL}; > > @@ -1041,7 +1115,7 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc, > s_fdt.fdt = create_device_tree(&fdt_size); > s_fdt.sphb = phb; > s_fdt.node_off = 0; > - fdt_start_offset = spapr_create_pci_child_dt(pdev, &s_fdt, drc_name); > + fdt_start_offset = spapr_create_pci_child_dt(pdev, &s_fdt); > if (!fdt_start_offset) { > error_setg(errp, "Failed to create pci child device tree node"); > goto out; > @@ -1519,7 +1593,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev, > int offset; > sPAPRFDT s_fdt; > > - offset = spapr_create_pci_child_dt(pdev, p, NULL); > + offset = spapr_create_pci_child_dt(pdev, p); > if (!offset) { > error_report("Failed to create pci child device tree node"); > return; > -- Alexey ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code 2015-05-08 7:42 ` Alexey Kardashevskiy @ 2015-05-19 4:51 ` Nikunj A Dadhania 2015-05-19 5:04 ` Alexey Kardashevskiy 0 siblings, 1 reply; 16+ messages in thread From: Nikunj A Dadhania @ 2015-05-19 4:51 UTC (permalink / raw) To: Alexey Kardashevskiy, qemu-devel, david Cc: nikunj.dadhania, qemu-ppc, agraf, mdroth Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 05/07/2015 05:21 PM, 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 “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. In failure cases use: >> vfio_<name>:<phb-index>:<slot>.<fn> >> >> 2) Emulated devices encode as following: >> qemu_<name>:<phb-index>:<slot>.<fn> >> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >> --- >> hw/ppc/spapr_pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 86 insertions(+), 12 deletions(-) >> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index 12f1b9c..d901007 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -769,6 +769,81 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, >> return drck->get_index(drc); >> } >> >> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value) > > Does it have to be a separate function? For better readability, i would prefer it this way. > > >> +{ >> + 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); >> + >> + g_file_get_contents(path, &buf, NULL, NULL); >> + return buf; >> +} >> + >> +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 make up the location code out >> + * 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)); > > > g_strdup_printf? Sure, with this I can get rid of this function. > > > >> + return path; >> +} >> + >> + >> +static char *spapr_ibm_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) > > s/spapr_ibm_get_loc_code/spapr_phb_get_loc_code/ I can change this. > Strange to see "ibm" in a function name, so far we have only used "ibm" in > RTAS handler names :) > > >> +{ >> + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci") != NULL) { > > QEMU does not compare object_dynamic_cast with NULL anywhere, so: > > if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { Sure > > > > > >> + char *buf = spapr_phb_vfio_get_loc_code(sphb, pdev); >> + >> + /* >> + * In case of failures reading the loc-code, make it up >> + * indicating a vfio device >> + */ >> + if (!buf) { >> + buf = g_malloc(PATH_MAX); >> + if (!buf) { >> + return NULL; >> + } >> + snprintf(buf, PATH_MAX, "vfio_%s:%02d:%02d.%1d", pdev->name, >> + sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); > > > g_strdup_printf? > > Also, "vfio_%s:%02d:%02d.%1d" looks very similar to > "qemu_%s:%02d:%02d.%1d", you could extend spapr_phb_get_loc_code() to take > "vfio"/"qemu" as a parameter. Sure, let me refactor this with g_strdup_printf > > >> + } >> + return buf; >> + } else { >> + return spapr_phb_get_loc_code(sphb, pdev); >> + } >> +} >> + >> /* 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 */ >> @@ -906,12 +981,12 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp) >> } >> >> static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset, >> - sPAPRPHBState *sphb, >> - const char *drc_name) >> + sPAPRPHBState *sphb) >> { >> ResourceProps rp; >> bool is_bridge = false; >> int pci_status; >> + char *buf = NULL; >> uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev); >> >> if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) == >> @@ -973,9 +1048,10 @@ 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 = spapr_ibm_get_loc_code(sphb, dev); >> + if (buf) { > > It is rather: > > if (!buf) { > error_report("Bad thing just happened"); > return -1; > } > > OR > > g_assert(!buf); As discussed in the previous post, g_assert can be compiled out. So the above one looks good. > > Your code can only return NULL if g_malloc() failed (otherwise it will be > at least "(qemu"vfio)_%s:%02d:%02d.%1d") and if this happened, something > went horribly bad. Right. So in this case QEMU will exit. > > >> + _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf)); >> + g_free(buf); >> } >> if (drc_index) { >> _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)); >> @@ -1003,8 +1079,7 @@ typedef struct sPAPRFDT { >> } sPAPRFDT; >> >> /* create OF node for pci device and required OF DT properties */ >> -static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p, >> - const char *drc_name) >> +static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p) >> { >> int offset, ret; >> char nodename[64]; >> @@ -1017,8 +1092,8 @@ static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p, >> sprintf(nodename, "pci@%d", slot); >> } >> offset = fdt_add_subnode(p->fdt, p->node_off, nodename); >> - ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb, >> - drc_name); >> + >> + ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb); >> g_assert(!ret); >> if (ret) { >> return 0; >> @@ -1033,7 +1108,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc, >> { >> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); >> DeviceState *dev = DEVICE(pdev); >> - const char *drc_name = drck->get_name(drc); >> int fdt_start_offset = 0, fdt_size; >> sPAPRFDT s_fdt = {NULL, 0, NULL}; >> >> @@ -1041,7 +1115,7 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc, >> s_fdt.fdt = create_device_tree(&fdt_size); >> s_fdt.sphb = phb; >> s_fdt.node_off = 0; >> - fdt_start_offset = spapr_create_pci_child_dt(pdev, &s_fdt, drc_name); >> + fdt_start_offset = spapr_create_pci_child_dt(pdev, &s_fdt); >> if (!fdt_start_offset) { >> error_setg(errp, "Failed to create pci child device tree node"); >> goto out; >> @@ -1519,7 +1593,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev, >> int offset; >> sPAPRFDT s_fdt; >> >> - offset = spapr_create_pci_child_dt(pdev, p, NULL); >> + offset = spapr_create_pci_child_dt(pdev, p); >> if (!offset) { >> error_report("Failed to create pci child device tree node"); >> return; >> > Thanks for the review. Regards Nikunj ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code 2015-05-19 4:51 ` Nikunj A Dadhania @ 2015-05-19 5:04 ` Alexey Kardashevskiy 2015-05-19 5:14 ` Nikunj A Dadhania 2015-05-19 6:56 ` Nikunj A Dadhania 0 siblings, 2 replies; 16+ messages in thread From: Alexey Kardashevskiy @ 2015-05-19 5:04 UTC (permalink / raw) To: Nikunj A Dadhania, qemu-devel, david Cc: nikunj.dadhania, qemu-ppc, agraf, mdroth On 05/19/2015 02:51 PM, Nikunj A Dadhania wrote: > Alexey Kardashevskiy <aik@ozlabs.ru> writes: > >> On 05/07/2015 05:21 PM, 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 “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. In failure cases use: >>> vfio_<name>:<phb-index>:<slot>.<fn> >>> >>> 2) Emulated devices encode as following: >>> qemu_<name>:<phb-index>:<slot>.<fn> >>> >>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>> --- >>> hw/ppc/spapr_pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++------- >>> 1 file changed, 86 insertions(+), 12 deletions(-) >>> >>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>> index 12f1b9c..d901007 100644 >>> --- a/hw/ppc/spapr_pci.c >>> +++ b/hw/ppc/spapr_pci.c >>> @@ -769,6 +769,81 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, >>> return drck->get_index(drc); >>> } >>> >>> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value) >> >> Does it have to be a separate function? > > For better readability, i would prefer it this way. This is why I asked - I was having problems understanding the difference between these two having 6 words names ;) Do not insist though. > >> >> >>> +{ >>> + 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); >>> + >>> + g_file_get_contents(path, &buf, NULL, NULL); >>> + return buf; >>> +} >>> + -- Alexey ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code 2015-05-19 5:04 ` Alexey Kardashevskiy @ 2015-05-19 5:14 ` Nikunj A Dadhania 2015-05-19 6:56 ` Nikunj A Dadhania 1 sibling, 0 replies; 16+ messages in thread From: Nikunj A Dadhania @ 2015-05-19 5:14 UTC (permalink / raw) To: Alexey Kardashevskiy, qemu-devel, david Cc: nikunj.dadhania, qemu-ppc, agraf, mdroth Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 05/19/2015 02:51 PM, Nikunj A Dadhania wrote: >> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >> >>> On 05/07/2015 05:21 PM, 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 “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. In failure cases use: >>>> vfio_<name>:<phb-index>:<slot>.<fn> >>>> >>>> 2) Emulated devices encode as following: >>>> qemu_<name>:<phb-index>:<slot>.<fn> >>>> >>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>> --- >>>> hw/ppc/spapr_pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++------- >>>> 1 file changed, 86 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>>> index 12f1b9c..d901007 100644 >>>> --- a/hw/ppc/spapr_pci.c >>>> +++ b/hw/ppc/spapr_pci.c >>>> @@ -769,6 +769,81 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, >>>> return drck->get_index(drc); >>>> } >>>> >>>> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value) >>> >>> Does it have to be a separate function? >> >> For better readability, i would prefer it this way. > > This is why I asked - I was having problems understanding the difference > between these two having 6 words names ;) Do not insist though. Let me see if I can simplify this. Regards Nikunj ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code 2015-05-19 5:04 ` Alexey Kardashevskiy 2015-05-19 5:14 ` Nikunj A Dadhania @ 2015-05-19 6:56 ` Nikunj A Dadhania 2015-05-19 7:41 ` Alexey Kardashevskiy 1 sibling, 1 reply; 16+ messages in thread From: Nikunj A Dadhania @ 2015-05-19 6:56 UTC (permalink / raw) To: Alexey Kardashevskiy, qemu-devel, david Cc: nikunj.dadhania, qemu-ppc, agraf, mdroth Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 05/19/2015 02:51 PM, Nikunj A Dadhania wrote: >> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >> >>> On 05/07/2015 05:21 PM, 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 “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. In failure cases use: >>>> vfio_<name>:<phb-index>:<slot>.<fn> >>>> >>>> 2) Emulated devices encode as following: >>>> qemu_<name>:<phb-index>:<slot>.<fn> >>>> >>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>> --- >>>> hw/ppc/spapr_pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++------- >>>> 1 file changed, 86 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>>> index 12f1b9c..d901007 100644 >>>> --- a/hw/ppc/spapr_pci.c >>>> +++ b/hw/ppc/spapr_pci.c >>>> @@ -769,6 +769,81 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, >>>> return drck->get_index(drc); >>>> } >>>> >>>> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value) >>> >>> Does it have to be a separate function? >> >> For better readability, i would prefer it this way. > > This is why I asked - I was having problems understanding the difference > between these two having 6 words names ;) Do not insist though. > This is what I have now, simplified: +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) +{ + char *path = NULL, *buf = NULL; + char *host = NULL; + + /* Get the PCI VFIO host id */ + host = 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 = 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 = 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; + +err_out: + g_free(path); + return NULL; +} + +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) +{ + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { + char *buf = spapr_phb_vfio_get_loc_code(sphb, pdev); + + /* + * In case of failures reading the loc-code, make it up + * indicating a vfio device + */ + if (!buf) { + buf = g_strdup_printf("vfio_%s:%02d:%02d.%1d", pdev->name, + sphb->index, PCI_SLOT(pdev->devfn), + PCI_FUNC(pdev->devfn)); + } + return buf; + } else { + return g_strdup_printf("qemu_%s:%02d:%02d.%1d", pdev->name, + sphb->index, PCI_SLOT(pdev->devfn), + PCI_FUNC(pdev->devfn)); + } +} + Regards Nikunj ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code 2015-05-19 6:56 ` Nikunj A Dadhania @ 2015-05-19 7:41 ` Alexey Kardashevskiy 2015-05-19 8:14 ` Nikunj A Dadhania 0 siblings, 1 reply; 16+ messages in thread From: Alexey Kardashevskiy @ 2015-05-19 7:41 UTC (permalink / raw) To: Nikunj A Dadhania, qemu-devel, david Cc: nikunj.dadhania, qemu-ppc, agraf, mdroth On 05/19/2015 04:56 PM, Nikunj A Dadhania wrote: > Alexey Kardashevskiy <aik@ozlabs.ru> writes: > >> On 05/19/2015 02:51 PM, Nikunj A Dadhania wrote: >>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>> >>>> On 05/07/2015 05:21 PM, 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 “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. In failure cases use: >>>>> vfio_<name>:<phb-index>:<slot>.<fn> >>>>> >>>>> 2) Emulated devices encode as following: >>>>> qemu_<name>:<phb-index>:<slot>.<fn> >>>>> >>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>> --- >>>>> hw/ppc/spapr_pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++------- >>>>> 1 file changed, 86 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>>>> index 12f1b9c..d901007 100644 >>>>> --- a/hw/ppc/spapr_pci.c >>>>> +++ b/hw/ppc/spapr_pci.c >>>>> @@ -769,6 +769,81 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, >>>>> return drck->get_index(drc); >>>>> } >>>>> >>>>> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value) >>>> >>>> Does it have to be a separate function? >>> >>> For better readability, i would prefer it this way. >> >> This is why I asked - I was having problems understanding the difference >> between these two having 6 words names ;) Do not insist though. >> > > This is what I have now, simplified: > > +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) > +{ > + char *path = NULL, *buf = NULL; > + char *host = NULL; > + > + /* Get the PCI VFIO host id */ > + host = 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 = 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 = 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; > + > +err_out: > + g_free(path); > + return NULL; > +} > + > +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) > +{ > + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { > + char *buf = spapr_phb_vfio_get_loc_code(sphb, pdev); > + > + /* > + * In case of failures reading the loc-code, make it up > + * indicating a vfio device > + */ > + if (!buf) { > + buf = g_strdup_printf("vfio_%s:%02d:%02d.%1d", pdev->name, > + sphb->index, PCI_SLOT(pdev->devfn), > + PCI_FUNC(pdev->devfn)); > + } > + return buf; > + } else { > + return g_strdup_printf("qemu_%s:%02d:%02d.%1d", pdev->name, > + sphb->index, PCI_SLOT(pdev->devfn), > + PCI_FUNC(pdev->devfn)); > + } > +} I'd do this but I do not insist :) static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) { char *buf; char *devtype = "qemu"; if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { buf = spapr_phb_vfio_get_loc_code(sphb, pdev); if (buf) { return buf; } devtype = "vfio"; } /* * In case of failures reading the loc-code, make it up * indicating a vfio device */ buf = g_strdup_printf("%s_%s:%02d:%02d.%1d", devtype, pdev->name, sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); return buf; } -- Alexey ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code 2015-05-19 7:41 ` Alexey Kardashevskiy @ 2015-05-19 8:14 ` Nikunj A Dadhania 2015-05-19 9:40 ` Alexey Kardashevskiy 0 siblings, 1 reply; 16+ messages in thread From: Nikunj A Dadhania @ 2015-05-19 8:14 UTC (permalink / raw) To: Alexey Kardashevskiy, qemu-devel, david Cc: nikunj.dadhania, qemu-ppc, agraf, mdroth Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 05/19/2015 04:56 PM, Nikunj A Dadhania wrote: >> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >> >>> On 05/19/2015 02:51 PM, Nikunj A Dadhania wrote: >>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>>> >>>>> On 05/07/2015 05:21 PM, 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 “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. In failure cases use: >>>>>> vfio_<name>:<phb-index>:<slot>.<fn> >>>>>> >>>>>> 2) Emulated devices encode as following: >>>>>> qemu_<name>:<phb-index>:<slot>.<fn> >>>>>> >>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>>> --- >>>>>> hw/ppc/spapr_pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++------- >>>>>> 1 file changed, 86 insertions(+), 12 deletions(-) >>>>>> >>>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>>>>> index 12f1b9c..d901007 100644 >>>>>> --- a/hw/ppc/spapr_pci.c >>>>>> +++ b/hw/ppc/spapr_pci.c >>>>>> @@ -769,6 +769,81 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, >>>>>> return drck->get_index(drc); >>>>>> } >>>>>> >>>>>> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value) >>>>> >>>>> Does it have to be a separate function? >>>> >>>> For better readability, i would prefer it this way. >>> >>> This is why I asked - I was having problems understanding the difference >>> between these two having 6 words names ;) Do not insist though. >>> >> >> This is what I have now, simplified: >> >> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >> +{ >> + char *path = NULL, *buf = NULL; >> + char *host = NULL; >> + >> + /* Get the PCI VFIO host id */ >> + host = 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 = 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 = 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; >> + >> +err_out: >> + g_free(path); >> + return NULL; >> +} >> + >> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >> +{ >> + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { >> + char *buf = spapr_phb_vfio_get_loc_code(sphb, pdev); >> + >> + /* >> + * In case of failures reading the loc-code, make it up >> + * indicating a vfio device >> + */ >> + if (!buf) { >> + buf = g_strdup_printf("vfio_%s:%02d:%02d.%1d", pdev->name, >> + sphb->index, PCI_SLOT(pdev->devfn), >> + PCI_FUNC(pdev->devfn)); >> + } >> + return buf; >> + } else { >> + return g_strdup_printf("qemu_%s:%02d:%02d.%1d", pdev->name, >> + sphb->index, PCI_SLOT(pdev->devfn), >> + PCI_FUNC(pdev->devfn)); >> + } >> +} > > > I'd do this but I do not insist :) That is fine as well. > > static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) > { > char *buf; > char *devtype = "qemu"; > > if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { > buf = spapr_phb_vfio_get_loc_code(sphb, pdev); > if (buf) { > return buf; > } > devtype = "vfio"; That has to be a snprintf. > } > > /* > * In case of failures reading the loc-code, make it up > * indicating a vfio device > */ > buf = g_strdup_printf("%s_%s:%02d:%02d.%1d", > devtype, pdev->name, > sphb->index, PCI_SLOT(pdev->devfn), > PCI_FUNC(pdev->devfn)); > return buf; > } Regards Nikunj ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code 2015-05-19 8:14 ` Nikunj A Dadhania @ 2015-05-19 9:40 ` Alexey Kardashevskiy 2015-05-19 9:58 ` Nikunj A Dadhania 0 siblings, 1 reply; 16+ messages in thread From: Alexey Kardashevskiy @ 2015-05-19 9:40 UTC (permalink / raw) To: Nikunj A Dadhania, qemu-devel, david Cc: nikunj.dadhania, qemu-ppc, agraf, mdroth On 05/19/2015 06:14 PM, Nikunj A Dadhania wrote: > Alexey Kardashevskiy <aik@ozlabs.ru> writes: > >> On 05/19/2015 04:56 PM, Nikunj A Dadhania wrote: >>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>> >>>> On 05/19/2015 02:51 PM, Nikunj A Dadhania wrote: >>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>>>> >>>>>> On 05/07/2015 05:21 PM, 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 “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. In failure cases use: >>>>>>> vfio_<name>:<phb-index>:<slot>.<fn> >>>>>>> >>>>>>> 2) Emulated devices encode as following: >>>>>>> qemu_<name>:<phb-index>:<slot>.<fn> >>>>>>> >>>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>>>> --- >>>>>>> hw/ppc/spapr_pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++------- >>>>>>> 1 file changed, 86 insertions(+), 12 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>>>>>> index 12f1b9c..d901007 100644 >>>>>>> --- a/hw/ppc/spapr_pci.c >>>>>>> +++ b/hw/ppc/spapr_pci.c >>>>>>> @@ -769,6 +769,81 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, >>>>>>> return drck->get_index(drc); >>>>>>> } >>>>>>> >>>>>>> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value) >>>>>> >>>>>> Does it have to be a separate function? >>>>> >>>>> For better readability, i would prefer it this way. >>>> >>>> This is why I asked - I was having problems understanding the difference >>>> between these two having 6 words names ;) Do not insist though. >>>> >>> >>> This is what I have now, simplified: >>> >>> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >>> +{ >>> + char *path = NULL, *buf = NULL; >>> + char *host = NULL; >>> + >>> + /* Get the PCI VFIO host id */ >>> + host = 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 = 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 = 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; >>> + >>> +err_out: >>> + g_free(path); >>> + return NULL; >>> +} >>> + >>> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >>> +{ >>> + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { >>> + char *buf = spapr_phb_vfio_get_loc_code(sphb, pdev); >>> + >>> + /* >>> + * In case of failures reading the loc-code, make it up >>> + * indicating a vfio device >>> + */ >>> + if (!buf) { >>> + buf = g_strdup_printf("vfio_%s:%02d:%02d.%1d", pdev->name, >>> + sphb->index, PCI_SLOT(pdev->devfn), >>> + PCI_FUNC(pdev->devfn)); >>> + } >>> + return buf; >>> + } else { >>> + return g_strdup_printf("qemu_%s:%02d:%02d.%1d", pdev->name, >>> + sphb->index, PCI_SLOT(pdev->devfn), >>> + PCI_FUNC(pdev->devfn)); >>> + } >>> +} >> >> >> I'd do this but I do not insist :) > > That is fine as well. > >> >> static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >> { >> char *buf; >> char *devtype = "qemu"; >> >> if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { >> buf = spapr_phb_vfio_get_loc_code(sphb, pdev); >> if (buf) { >> return buf; >> } >> devtype = "vfio"; > > That has to be a snprintf. No it does not. g_strdup_printf() below is enough > >> } >> >> /* >> * In case of failures reading the loc-code, make it up >> * indicating a vfio device >> */ >> buf = g_strdup_printf("%s_%s:%02d:%02d.%1d", >> devtype, pdev->name, >> sphb->index, PCI_SLOT(pdev->devfn), >> PCI_FUNC(pdev->devfn)); >> return buf; >> } > > Regards > Nikunj > -- Alexey ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code 2015-05-19 9:40 ` Alexey Kardashevskiy @ 2015-05-19 9:58 ` Nikunj A Dadhania 2015-05-19 11:08 ` Alexander Graf 0 siblings, 1 reply; 16+ messages in thread From: Nikunj A Dadhania @ 2015-05-19 9:58 UTC (permalink / raw) To: Alexey Kardashevskiy, qemu-devel, david Cc: nikunj.dadhania, qemu-ppc, agraf, mdroth Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 05/19/2015 06:14 PM, Nikunj A Dadhania wrote: >> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >> >>> On 05/19/2015 04:56 PM, Nikunj A Dadhania wrote: >>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>>> >>>>> On 05/19/2015 02:51 PM, Nikunj A Dadhania wrote: >>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>>>>> >>>>>>> On 05/07/2015 05:21 PM, 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 “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. In failure cases use: >>>>>>>> vfio_<name>:<phb-index>:<slot>.<fn> >>>>>>>> >>>>>>>> 2) Emulated devices encode as following: >>>>>>>> qemu_<name>:<phb-index>:<slot>.<fn> >>>>>>>> >>>>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>>>>> --- >>>>>>>> hw/ppc/spapr_pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++------- >>>>>>>> 1 file changed, 86 insertions(+), 12 deletions(-) >>>>>>>> >>>>>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>>>>>>> index 12f1b9c..d901007 100644 >>>>>>>> --- a/hw/ppc/spapr_pci.c >>>>>>>> +++ b/hw/ppc/spapr_pci.c >>>>>>>> @@ -769,6 +769,81 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, >>>>>>>> return drck->get_index(drc); >>>>>>>> } >>>>>>>> >>>>>>>> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value) >>>>>>> >>>>>>> Does it have to be a separate function? >>>>>> >>>>>> For better readability, i would prefer it this way. >>>>> >>>>> This is why I asked - I was having problems understanding the difference >>>>> between these two having 6 words names ;) Do not insist though. >>>>> >>>> >>>> This is what I have now, simplified: >>>> >>>> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >>>> +{ >>>> + char *path = NULL, *buf = NULL; >>>> + char *host = NULL; >>>> + >>>> + /* Get the PCI VFIO host id */ >>>> + host = 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 = 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 = 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; >>>> + >>>> +err_out: >>>> + g_free(path); >>>> + return NULL; >>>> +} >>>> + >>>> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >>>> +{ >>>> + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { >>>> + char *buf = spapr_phb_vfio_get_loc_code(sphb, pdev); >>>> + >>>> + /* >>>> + * In case of failures reading the loc-code, make it up >>>> + * indicating a vfio device >>>> + */ >>>> + if (!buf) { >>>> + buf = g_strdup_printf("vfio_%s:%02d:%02d.%1d", pdev->name, >>>> + sphb->index, PCI_SLOT(pdev->devfn), >>>> + PCI_FUNC(pdev->devfn)); >>>> + } >>>> + return buf; >>>> + } else { >>>> + return g_strdup_printf("qemu_%s:%02d:%02d.%1d", pdev->name, >>>> + sphb->index, PCI_SLOT(pdev->devfn), >>>> + PCI_FUNC(pdev->devfn)); >>>> + } >>>> +} >>> >>> >>> I'd do this but I do not insist :) >> >> That is fine as well. >> >>> >>> static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >>> { >>> char *buf; >>> char *devtype = "qemu"; >>> >>> if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { >>> buf = spapr_phb_vfio_get_loc_code(sphb, pdev); >>> if (buf) { >>> return buf; >>> } >>> devtype = "vfio"; >> >> That has to be a snprintf. > > No it does not. g_strdup_printf() below is enough CC ppc64-softmmu/hw/ppc/spapr_pci.o hw/ppc/spapr_pci.c: In function ‘spapr_phb_get_loc_code’: hw/ppc/spapr_pci.c:807:21: error: initialization discards ‘const’ qualifier from pointer target type [-Werror] char *devtype = "qemu"; ^ hw/ppc/spapr_pci.c:814:17: error: assignment discards ‘const’ qualifier from pointer target type [-Werror] devtype = "vfio"; ^ cc1: all warnings being treated as errors make[1]: *** [hw/ppc/spapr_pci.o] Error 1 make: *** [subdir-ppc64-softmmu] Error 2 > >> >>> } >>> >>> /* >>> * In case of failures reading the loc-code, make it up >>> * indicating a vfio device >>> */ >>> buf = g_strdup_printf("%s_%s:%02d:%02d.%1d", >>> devtype, pdev->name, >>> sphb->index, PCI_SLOT(pdev->devfn), >>> PCI_FUNC(pdev->devfn)); >>> return buf; >>> } >> >> Regards >> Nikunj >> > > > -- > Alexey ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code 2015-05-19 9:58 ` Nikunj A Dadhania @ 2015-05-19 11:08 ` Alexander Graf 2015-05-20 3:13 ` Nikunj A Dadhania 0 siblings, 1 reply; 16+ messages in thread From: Alexander Graf @ 2015-05-19 11:08 UTC (permalink / raw) To: Nikunj A Dadhania, Alexey Kardashevskiy, qemu-devel, david Cc: nikunj.dadhania, qemu-ppc, mdroth On 05/19/2015 11:58 AM, Nikunj A Dadhania wrote: > Alexey Kardashevskiy <aik@ozlabs.ru> writes: > >> On 05/19/2015 06:14 PM, Nikunj A Dadhania wrote: >>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>> >>>> On 05/19/2015 04:56 PM, Nikunj A Dadhania wrote: >>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>>>> >>>>>> On 05/19/2015 02:51 PM, Nikunj A Dadhania wrote: >>>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>>>>>> >>>>>>>> On 05/07/2015 05:21 PM, 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 “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. In failure cases use: >>>>>>>>> vfio_<name>:<phb-index>:<slot>.<fn> >>>>>>>>> >>>>>>>>> 2) Emulated devices encode as following: >>>>>>>>> qemu_<name>:<phb-index>:<slot>.<fn> >>>>>>>>> >>>>>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>>>>>> --- >>>>>>>>> hw/ppc/spapr_pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++------- >>>>>>>>> 1 file changed, 86 insertions(+), 12 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>>>>>>>> index 12f1b9c..d901007 100644 >>>>>>>>> --- a/hw/ppc/spapr_pci.c >>>>>>>>> +++ b/hw/ppc/spapr_pci.c >>>>>>>>> @@ -769,6 +769,81 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, >>>>>>>>> return drck->get_index(drc); >>>>>>>>> } >>>>>>>>> >>>>>>>>> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value) >>>>>>>> Does it have to be a separate function? >>>>>>> For better readability, i would prefer it this way. >>>>>> This is why I asked - I was having problems understanding the difference >>>>>> between these two having 6 words names ;) Do not insist though. >>>>>> >>>>> This is what I have now, simplified: >>>>> >>>>> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >>>>> +{ >>>>> + char *path = NULL, *buf = NULL; >>>>> + char *host = NULL; >>>>> + >>>>> + /* Get the PCI VFIO host id */ >>>>> + host = 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 = 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 = 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; >>>>> + >>>>> +err_out: >>>>> + g_free(path); >>>>> + return NULL; >>>>> +} >>>>> + >>>>> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >>>>> +{ >>>>> + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { >>>>> + char *buf = spapr_phb_vfio_get_loc_code(sphb, pdev); >>>>> + >>>>> + /* >>>>> + * In case of failures reading the loc-code, make it up >>>>> + * indicating a vfio device >>>>> + */ >>>>> + if (!buf) { >>>>> + buf = g_strdup_printf("vfio_%s:%02d:%02d.%1d", pdev->name, >>>>> + sphb->index, PCI_SLOT(pdev->devfn), >>>>> + PCI_FUNC(pdev->devfn)); >>>>> + } >>>>> + return buf; >>>>> + } else { >>>>> + return g_strdup_printf("qemu_%s:%02d:%02d.%1d", pdev->name, >>>>> + sphb->index, PCI_SLOT(pdev->devfn), >>>>> + PCI_FUNC(pdev->devfn)); >>>>> + } >>>>> +} >>>> >>>> I'd do this but I do not insist :) >>> That is fine as well. >>> >>>> static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >>>> { >>>> char *buf; >>>> char *devtype = "qemu"; >>>> >>>> if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { >>>> buf = spapr_phb_vfio_get_loc_code(sphb, pdev); >>>> if (buf) { >>>> return buf; >>>> } >>>> devtype = "vfio"; >>> That has to be a snprintf. >> No it does not. g_strdup_printf() below is enough > CC ppc64-softmmu/hw/ppc/spapr_pci.o > hw/ppc/spapr_pci.c: In function ‘spapr_phb_get_loc_code’: > hw/ppc/spapr_pci.c:807:21: error: initialization discards ‘const’ qualifier from pointer target type [-Werror] > char *devtype = "qemu"; > ^ > hw/ppc/spapr_pci.c:814:17: error: assignment discards ‘const’ qualifier from pointer target type [-Werror] > devtype = "vfio"; > ^ > cc1: all warnings being treated as errors > make[1]: *** [hw/ppc/spapr_pci.o] Error 1 > make: *** [subdir-ppc64-softmmu] Error 2 Make it const char *devtype? Alex ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code 2015-05-19 11:08 ` Alexander Graf @ 2015-05-20 3:13 ` Nikunj A Dadhania 0 siblings, 0 replies; 16+ messages in thread From: Nikunj A Dadhania @ 2015-05-20 3:13 UTC (permalink / raw) To: Alexander Graf, Alexey Kardashevskiy, qemu-devel, david Cc: nikunj.dadhania, qemu-ppc, mdroth Alexander Graf <agraf@suse.de> writes: > On 05/19/2015 11:58 AM, Nikunj A Dadhania wrote: >> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >> >>> On 05/19/2015 06:14 PM, Nikunj A Dadhania wrote: >>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>>> >>>>> On 05/19/2015 04:56 PM, Nikunj A Dadhania wrote: >>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>>>>> >>>>>>> On 05/19/2015 02:51 PM, Nikunj A Dadhania wrote: >>>>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >>>>>>>> >>>>>>>>> On 05/07/2015 05:21 PM, 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 “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. In failure cases use: >>>>>>>>>> vfio_<name>:<phb-index>:<slot>.<fn> >>>>>>>>>> >>>>>>>>>> 2) Emulated devices encode as following: >>>>>>>>>> qemu_<name>:<phb-index>:<slot>.<fn> >>>>>>>>>> >>>>>>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>>>>>>> --- >>>>>>>>>> hw/ppc/spapr_pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++------- >>>>>>>>>> 1 file changed, 86 insertions(+), 12 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>>>>>>>>> index 12f1b9c..d901007 100644 >>>>>>>>>> --- a/hw/ppc/spapr_pci.c >>>>>>>>>> +++ b/hw/ppc/spapr_pci.c >>>>>>>>>> @@ -769,6 +769,81 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, >>>>>>>>>> return drck->get_index(drc); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value) >>>>>>>>> Does it have to be a separate function? >>>>>>>> For better readability, i would prefer it this way. >>>>>>> This is why I asked - I was having problems understanding the difference >>>>>>> between these two having 6 words names ;) Do not insist though. >>>>>>> >>>>>> This is what I have now, simplified: >>>>>> >>>>>> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >>>>>> +{ >>>>>> + char *path = NULL, *buf = NULL; >>>>>> + char *host = NULL; >>>>>> + >>>>>> + /* Get the PCI VFIO host id */ >>>>>> + host = 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 = 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 = 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; >>>>>> + >>>>>> +err_out: >>>>>> + g_free(path); >>>>>> + return NULL; >>>>>> +} >>>>>> + >>>>>> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >>>>>> +{ >>>>>> + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { >>>>>> + char *buf = spapr_phb_vfio_get_loc_code(sphb, pdev); >>>>>> + >>>>>> + /* >>>>>> + * In case of failures reading the loc-code, make it up >>>>>> + * indicating a vfio device >>>>>> + */ >>>>>> + if (!buf) { >>>>>> + buf = g_strdup_printf("vfio_%s:%02d:%02d.%1d", pdev->name, >>>>>> + sphb->index, PCI_SLOT(pdev->devfn), >>>>>> + PCI_FUNC(pdev->devfn)); >>>>>> + } >>>>>> + return buf; >>>>>> + } else { >>>>>> + return g_strdup_printf("qemu_%s:%02d:%02d.%1d", pdev->name, >>>>>> + sphb->index, PCI_SLOT(pdev->devfn), >>>>>> + PCI_FUNC(pdev->devfn)); >>>>>> + } >>>>>> +} >>>>> >>>>> I'd do this but I do not insist :) >>>> That is fine as well. >>>> >>>>> static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >>>>> { >>>>> char *buf; >>>>> char *devtype = "qemu"; >>>>> >>>>> if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { >>>>> buf = spapr_phb_vfio_get_loc_code(sphb, pdev); >>>>> if (buf) { >>>>> return buf; >>>>> } >>>>> devtype = "vfio"; >>>> That has to be a snprintf. >>> No it does not. g_strdup_printf() below is enough >> CC ppc64-softmmu/hw/ppc/spapr_pci.o >> hw/ppc/spapr_pci.c: In function ‘spapr_phb_get_loc_code’: >> hw/ppc/spapr_pci.c:807:21: error: initialization discards ‘const’ qualifier from pointer target type [-Werror] >> char *devtype = "qemu"; >> ^ >> hw/ppc/spapr_pci.c:814:17: error: assignment discards ‘const’ qualifier from pointer target type [-Werror] >> devtype = "vfio"; >> ^ >> cc1: all warnings being treated as errors >> make[1]: *** [hw/ppc/spapr_pci.o] Error 1 >> make: *** [subdir-ppc64-softmmu] Error 2 > > Make it const char *devtype? Cool, that works. Regards, Nikunj ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2015-05-20 3:13 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-05-07 7:21 [Qemu-devel] [PATCH v4 0/4] spapr_pci: DT field fixes and PCI DT node creation in QEMU Nikunj A Dadhania 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 1/4] spapr_pci: encode missing 64-bit memory address space Nikunj A Dadhania 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 2/4] spapr_pci: encode class code including Prog IF register Nikunj A Dadhania 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 3/4] spapr_pci: enumerate and add PCI device tree Nikunj A Dadhania 2015-05-07 7:21 ` [Qemu-devel] [PATCH v4 4/4] spapr_pci: populate ibm,loc-code Nikunj A Dadhania 2015-05-08 7:42 ` Alexey Kardashevskiy 2015-05-19 4:51 ` Nikunj A Dadhania 2015-05-19 5:04 ` Alexey Kardashevskiy 2015-05-19 5:14 ` Nikunj A Dadhania 2015-05-19 6:56 ` Nikunj A Dadhania 2015-05-19 7:41 ` Alexey Kardashevskiy 2015-05-19 8:14 ` Nikunj A Dadhania 2015-05-19 9:40 ` Alexey Kardashevskiy 2015-05-19 9:58 ` Nikunj A Dadhania 2015-05-19 11:08 ` Alexander Graf 2015-05-20 3:13 ` Nikunj A Dadhania
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.