qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Nikunj A Dadhania
Subject: Re: [Qemu-devel] [RFC PATCH 1/2] spapr: enumerate and add PCI device tree
Date: Wed, 29 Apr 2015 10:45:36 +0530
User-agent: Notmuch/0.17+27~gae47d61 (http://notmuchmail.org) Emacs/24.3.1 (x86_64-redhat-linux-gnu)

Alexey Kardashevskiy <address@hidden> writes:

> On 04/22/2015 08:35 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 <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) {
>
>
> Where did drc_name come from? By now, there is no "loc-code" patch in 
> spapr-next, or there is?

loc-code is not there yet, but the hotplug code generates its own name
and sends as drc_name. I am getting rid of this in the next patch.

>
>
>> +        _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;
>
> You definitely will have to rebase it - the sPAPRTCEDT is gone in what I 
> posted last time (and what I hope will get to  spapr-next first :) ).

Sure, that should be fine.

>
>
>
>>   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)) {
>
>
> Since you'll be doing respin anyway, I suggest changing "==" to "!=", add 
> return here and reduce indents. And below, and further down.

Yes, will be better.

>
>
>
>> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>> +        if(sec_bus) {
>> +            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 }));
>> +        }
>> +    }
>> +    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));
>
>
> Cannot SLOF see that there are nodes already and just not touch them?

This is more explicit. So that in absence of this, I can kick in old
SLOF enumeration code and in presence, start reading the nodes.

>
>
>> +    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);
>>
>
>
> -- 
> Alexey

Regards
Nikunj




reply via email to

[Prev in Thread] Current Thread [Next in Thread]