Re: [Qemu-devel] [PATCH v5 3/4] spapr_pci: enumerate and add PCI device tree

[Nikunj A Dadhania]

Alexey Kardashevskiy <address@hidden> writes:

> On 05/25/2015 02:45 PM, Nikunj A Dadhania wrote:
>> Alexey Kardashevskiy <address@hidden> writes:
>>
>>>>    /* 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.
>
>
> In this file, PCIDevice is called "dev", "pdev", "d", "pci_dev" so if you 
> really want to change the name - do it once and for all occurrences OR do 
> not do this at all :)

In that case, lets not do this in this patch.

>
>
>>
>>>
>>>
>>>>    {
>>>> -    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.
>
> I'd make a separate patch with s/512/64/ and also do
> s/sprintf/snprintf/ below.

Sure.

>
>
>
>>> 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, "address@hidden,%d", slot, func);
>>>>        } else {
>>>>            sprintf(nodename, "address@hidden", 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.
> Where else? I did grep and see just one call:
>
> hw/ppc/spapr_pci.c:1087:static void 
> spapr_phb_add_pci_device(sPAPRDRConnector *drc,
> hw/ppc/spapr_pci.c:1166:    spapr_phb_add_pci_device(drc, phb, pdev, 
> &local_err);


spapr_phb_add_pci_device gets called for hotplug as well as boot device
through hp->plug = spapr_phb_hot_plug_child

I just tried to verify  passing a pci device with below code:

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index d11e2ab..5737839 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1103,6 +1103,10 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector 
*drc,
             error_setg(errp, "Failed to create pci child device tree node");
             goto out;
         }
+    } else {
+      int slot = PCI_SLOT(pdev->devfn);
+      int func = PCI_FUNC(pdev->devfn);
+      fprintf(stderr, "non-hotplug - address@hidden,%d\n", slot, func);
     }

$ ./ppc64-softmmu/qemu-system-ppc64 -machine pseries -m 2G -serial stdio
non-hotplug - address@hidden,0
non-hotplug - address@hidden,0

SLOF **********************************************************************



>
>
>
>>
>>>
>>>> -        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, "address@hidden" 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.
>
>
> Well, chances that the binary image will have this line by accident are zero.
>
> And I spent quite some time debugging SRIOV + VFIO when I realized that 
> SLOF is old on the test machine where others used to debug too. It would be 
> really nice to have a warning that something is wrong. May be extend 
> "client-architecture-support" somehow or have some release/date signature 
> in known place in SLOF... Thomas (?) also asked for this :)

Sure, I can work on this. I would not recommend grepping though.

Regards
Nikunj