Thomas Huth <th...@redhat.com> writes:
> On Tue, 5 May 2015 14:23:54 +0530
> Nikunj A Dadhania <nik...@linux.vnet.ibm.com> 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>
>> ---
>> hw/ppc/spapr_pci.c | 108
>> ++++++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 103 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 8b02a3e..103284a 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"
>> @@ -945,7 +947,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"));
>> - _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",
>> @@ -1001,10 +1006,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);
>> @@ -1482,6 +1483,89 @@ PCIHostState *spapr_create_phb(sPAPREnvironment
>> *spapr, int index)
>> return PCI_HOST_BRIDGE(dev);
>> }
>>
>> +typedef struct sPAPRFDT {
>> + void *fdt;
>> + int node_off;
>> + uint32_t index;
>> +} sPAPRFDT;
>> +
>> +static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
>> + void *opaque)
>> +{
>> + PCIBus *sec_bus;
>> + sPAPRFDT *p = opaque;
>> + int ret, offset;
>> + int slot = PCI_SLOT(pdev->devfn);
>> + int func = PCI_FUNC(pdev->devfn);
>> + char nodename[512];
>
> That's quite a big array ....
>
>> + sPAPRFDT s_fdt;
>> +
>> + if (func) {
>> + sprintf(nodename, "pci@%d,%d", slot, func);
>> + } else {
>> + sprintf(nodename, "pci@%d", slot);
>> + }
>
> ... just for holding such a small string. Could you maybe use
> a smaller array size for nodename (especially since you call this
> function recursively)?
Sure, will change this to 32 bytes, that should be sufficient.
>
>> + offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
>> + ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0,
>> NULL);
>
> The above code sequence looks pretty much similar to
> spapr_create_pci_child_dt() ... maybe it's worth the effort to merge
> the common code of both functions into a separate helper function
> to avoid the duplication? ... not sure if this is worth the effort,
> it's just a suggestion.
I had thought about it earlier but something was not matching. Let me
have a relook, things have changed.
>
>> + g_assert(!ret);
>
> You know that assert statements can simply be disabled by a
> preprocessor switch - and in that case there is no more error handling
> here at all and the code continues silently with a partial initialized
> node!
Thanks for bringing this to notice, assert() ?
> So it's maybe better to do a proper error handling here instead?
Will change this error handling here.
>
>> + 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.index = p->index;
>> + 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 short *bus_no = (unsigned short *) opaque;
>
> opaque is a void pointer, so I think you don't need the typecast here.
Ok, QEMU has strong type checking, so by default I typecast them every time.
>
>> + unsigned short primary = *bus_no;
>> + unsigned short secondary;
>> + unsigned short subordinate = 0xff;
>
> Is there a special reason for using unsigned short variables here?
No special reason, actually unsigned char should do, as the max is bus
number can only be 255
> "unsigned int" would sound much more natural to me.
>
>> + 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);
>
> You write all value here again ... I think you c\ould either drop the
> write to the PRIMARY and SECONDARY BUS registers, or you could use a
> proper if-else instead.
I will drop the PRIMARY and SECONDARY here, that would do.
>
>> + 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;
>
> Again, why "short" ?
>
>> + 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 +1605,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 +1656,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.index = phb->index;
>> + 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) {
>
> Thomas
Regards
Nikunj
