From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39822) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z58wY-0008Gq-8J for qemu-devel@nongnu.org; Wed, 17 Jun 2015 04:43:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z58wS-0002qi-NF for qemu-devel@nongnu.org; Wed, 17 Jun 2015 04:43:34 -0400 Received: from e23smtp06.au.ibm.com ([202.81.31.148]:50059) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z58wR-0002q7-Pt for qemu-devel@nongnu.org; Wed, 17 Jun 2015 04:43:28 -0400 Received: from /spool/local by e23smtp06.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 17 Jun 2015 18:43:22 +1000 From: Nikunj A Dadhania In-Reply-To: <20150617064936.GS13352@voom.redhat.com> References: <1434020549-26362-1-git-send-email-nikunj@linux.vnet.ibm.com> <1434020549-26362-4-git-send-email-nikunj@linux.vnet.ibm.com> <20150617064936.GS13352@voom.redhat.com> Date: Wed, 17 Jun 2015 14:12:14 +0530 Message-ID: <87k2v2enfd.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v7 3/6] spapr_pci: enumerate and add PCI device tree List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: agraf@suse.de, thuth@redhat.com, aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org David Gibson writes: > On Thu, Jun 11, 2015 at 04:32:26PM +0530, Nikunj A Dadhania wrote: >> 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 >> [ Squashed Michael's drc_index changes ] >> Signed-off-by: Michael Roth >> Signed-off-by: Nikunj A Dadhania >> --- >> hw/ppc/spapr_pci.c | 167 +++++++++++++++++++++++++++++++++++++++++++++-------- >> 1 file changed, 142 insertions(+), 25 deletions(-) >> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index 33254b3..6ef7f44 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" >> @@ -946,8 +948,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)); >> @@ -964,30 +971,38 @@ 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; > > I don't really like this structure - it seems a very ad-hoc collection > of things. Even though it means there will be a lot of parameters to > the function, I'd prefer passing them separately to > spapr_create_pci_child_dt() rather than using this structure. I added this structure with pci_for_each_device() in mind, which has following prototype. void pci_for_each_device(PCIBus *bus, int bus_num, void (*fn)(PCIBus *bus, PCIDevice *d, void *opaque), void *opaque); So per device we get this structure and populate PCI device tree entry and scan and populate bridge recursively if needed. So I had continued using this structure in spapr_create_pci_child_dt(). We cannot remove sPAPRFDT completely as we need it for PCI device tree creation. So if needed, I can change spapr_create_pci_child_dt() with more args. And structure sPAPRFDT to be used by spapr_populate_pci_devices_dt() called by pci_for_each_device(). >> @@ -997,24 +1012,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); >> } >> } [SNIP] >> +static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev, >> + void *opaque) >> +{ >> + PCIBus *sec_bus; >> + sPAPRFDT *p = opaque; [ SNIP ] >> + 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); >> +} [ SNIP ] >> @@ -1523,6 +1626,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); >> @@ -1572,6 +1677,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) { > Regards, Nikunj