Re: [Qemu-devel] [RFC PATCH 1/2] spapr: enumerate and add PCI device tree

[David Gibson]

On Wed, Apr 22, 2015 at 04:05:33PM +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 <address@hidden>
> ---
>  hw/ppc/spapr_pci.c | 93 
> ++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 84 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 3796d54..abf71f7 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"
> @@ -938,7 +940,9 @@ 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)));
> +    if (drc_name) {
> +        _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));
>  
>      _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
> @@ -994,10 +998,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector 
> *drc,
>      void *fdt = NULL;
>      int fdt_start_offset = 0;
>  
> -    /* 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);
> @@ -1473,14 +1473,15 @@ PCIHostState *spapr_create_phb(sPAPREnvironment 
> *spapr, int index)
>      return PCI_HOST_BRIDGE(dev);
>  }
>  
> -typedef struct sPAPRTCEDT {
> +typedef struct sPAPRFDT {
>      void *fdt;
>      int node_off;
> -} sPAPRTCEDT;
> +    uint32_t index;
> +} sPAPRFDT;
>  
>  static int spapr_phb_children_dt(Object *child, void *opaque)
>  {
> -    sPAPRTCEDT *p = opaque;
> +    sPAPRFDT *p = opaque;
>      sPAPRTCETable *tcet;
>  
>      tcet = (sPAPRTCETable *) object_dynamic_cast(child, 
> TYPE_SPAPR_TCE_TABLE);
> @@ -1496,6 +1497,73 @@ static int spapr_phb_children_dt(Object *child, void 
> *opaque)
>      return 1;
>  }
>  
> +static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev, void 
> *opaque)
> +{
> +    sPAPRFDT *p = opaque;
> +    int ret, offset;
> +    int slot = PCI_SLOT(pdev->devfn);
> +    int func = PCI_FUNC(pdev->devfn);
> +    char nodename[512];
> +
> +    if (func) {
> +        sprintf(nodename, "address@hidden,%d", slot, func);
> +    } else {
> +        sprintf(nodename, "address@hidden", slot);
> +    }
> +    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, 
> NULL);
> +    g_assert(!ret);
> +
> +    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));
> +        if(sec_bus) {

You have several minor indent style problems - please run through
scripts/checkpatch.pl and correct accordingly.

> +            pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> +                                spapr_populate_pci_devices_dt,
> +                                &((sPAPRFDT){ .fdt = p->fdt, .node_off = 
> offset , .index = p->index }));

The inline structure definition is pretty hard to read.  I'd prefer to
see that declared as a local.

Otherwise, this looks ok.

> +        }
> +    }
> +    return;
> +}
> +
> +static void spapr_phb_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev, 
> void *opaque)
> +{
> +    unsigned short *bus_no = (unsigned short *) opaque;
> +    unsigned short primary = *bus_no;
> +    unsigned short secondary;
> +    unsigned short 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_PRIMARY_BUS, primary, 1);
> +            pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
> +            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 short 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)
> @@ -1534,6 +1602,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>      uint32_t interrupt_map_mask[] = {
>          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];
> +    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
>  
>      /* Start populating the FDT */
>      sprintf(nodename, "address@hidden" PRIx64, phb->buid);
> @@ -1579,7 +1648,13 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>                       sizeof(interrupt_map)));
>  
>      object_child_foreach(OBJECT(phb), spapr_phb_children_dt,
> -                         &((sPAPRTCEDT){ .fdt = fdt, .node_off = bus_off }));
> +                         &((sPAPRFDT){ .fdt = fdt, .node_off = bus_off }));
> +
> +    spapr_phb_pci_enumerate(phb);
> +    _FDT(fdt_setprop_cell(fdt, bus_off, "qemu,phb-enumerated", 0x1));
> +    pci_for_each_device(bus, pci_bus_num(bus),
> +                        spapr_populate_pci_devices_dt,
> +                        &((sPAPRFDT){ .fdt = fdt, .node_off = bus_off, 
> .index = phb->index }));
>  
>      ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
>                                  SPAPR_DR_CONNECTOR_TYPE_PCI);

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpNv_2KLHBKz.pgp
Description: PGP signature