Alexey Kardashevskiy <a...@ozlabs.ru> writes:
> On 05/19/2015 06:26 PM, 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 <nik...@linux.vnet.ibm.com>
>> [ Squashed Michael's drc_index changes ]
>> Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
>> Signed-off-by: Nikunj A Dadhania <nik...@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);
>
> Is this drc_index any different from the one which used to be passed to
> this function? If no, then I do not see the point in changing the prototype
> (or make another "this just makes code easier/nicer" patch).
Its the same, I can have a separate patch. As I was changing this code
the drc_index would need to be read in boot and hotplug code. So brought
over the code here.
> If yes, then it would be nice to see what the patch changed in this
> regard in the commit log.
>
>
>
>> 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)
>
> Why s/dev/pdev/?
PCIDev thats the only reason.
>
>
>> {
>> - 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];
>
> Why s/512/64/?
Earlier this was called in recursion, so there was a comment in previous
series to reduce this to lesser number.
>
> This change and the one above hide what the patch really does to
> spapr_create_pci_child_dt.
>
>
>> + 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) {
>
>
> I understand the patch is not changing this but still while we are here -
> spapr_phb_add_pci_device() is only called from spapr_phb_hot_plug_child(),
> how can dev->hotplugged be not true in this function (if it cannot, you
> could get rid of "out:"?
It gets called even when the devices are added during boot.
>
>> - 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)
>
> Just adding forward declaration would make the patch shorter.
Yes, I can do that.
>
>> -{
>> - 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)) {
>
>
> s/==/!=/ and "return" and no need in extra indent below.
Right.
>
>> + PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>> + secondary = *bus_no + 1;
>
>
> (*bus_no)++;
> secondary = *bus_no;
>
> and remove "bus_no = *bus_no + 1" below?
> In fact, I do not need much sense in having "secondary" variable in this
> function.
>
>> + 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) {
>
> same here? Just like you did in spapr_populate_pci_devices_dt(). I do not
> insist though. But having less scopes just makes it easier/nicer to wrap
> long lines in QEMU coding style (new line starts under "(").
>
>
>> + 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));
>
>
> Can we also add a hack here to scan for the "qemu,phb-enumerated" string in
> the SLOF bin image?
Really ? That would be ugly.
>
>> +
>> + /* 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) {
>>
>
>
> --
> Alexey
