* Re: [PATCH v2 0/6] pseries: Move memory hotplug to the kernel
From: Cyril Bur @ 2014-11-18 22:59 UTC (permalink / raw)
To: Nathan Fontenot; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <546B9144.60003@linux.vnet.ibm.com>
On Tue, 2014-11-18 at 12:34 -0600, Nathan Fontenot wrote:
> On 11/17/2014 08:00 PM, Cyril Bur wrote:
> > Hi Nathan,
> >
> > I tried to apply these to Linus' tree and Mpes tree and to stable and
> > got several problems, I got stuck at the third hunk in patch 5.
>
> I based these patches off of mpe's -next tree. I did a fresh pull of
> mpe's tree and found that they do apply with some fuzz to the master branch.
>
Got them onto mpe's -next thanks.
> Which tree were you having issue with patch 5?
Looks like 16d0f5c4af76b0c3424290937bf1ac22adf439b1 was the cause of my
problems.
>
> -Nathan
>
> >
> > Could you point out where I'm going wrong?
> >
> > Thanks,
> >
> > Cyril
> >
> > On Mon, 2014-11-17 at 15:44 -0600, Nathan Fontenot wrote:
> >> In order to better support device hotplug (cpu, memory, and pci) in the
> >> PowerVM and PowerKVM environments, the handling of device hotplug
> >> could be updated so that the act of hotplugging a device occurs entirely
> >> in the kernel. This patch set begins to address this by moving
> >> memory hotplug to the kernel. Patches to follow will do the same
> >> for cpu and pci devices.
> >>
> >> To provide background, the current handling of memory hotplug is
> >> handled by the drmgr command. This command is invoked when memory
> >> add/remove requests are made at the HMC and conveyed to a partition
> >> through the RSCT framework. The drmgr command then performs parts
> >> of the hotplug in user-space and makes requests to the kernel to perform
> >> other pieces. This is not really ideal, we can do everything in the
> >> kernel and do it faster.
> >>
> >> In this patchset, hotplug events will now be communicated to the kernel
> >> in the form of rtas hotplug events. For PowerKVM systems this is done
> >> by qemu using the ras epow interrupt. For PowerVM systems the drmgr
> >> command will be updated to create a rtas hotplug event and send it to
> >> the kernel via a new /sys/kernel/dlpar interface. Both of these
> >> entry points for hotplug rtas events then call a common routine
> >> for handling rtas hotplug events.
> >>
> >> -Nathan
> >>
> >> Patch 1/6
> >> - Add definition of hotplug rtas event sections.
> >>
> >> Patch 2/6
> >> - Update struct of_drconf_cell to use __be64/__be32
> >>
> >> Patch 3/6
> >> - Export the dlpar_[acquire|release]drc() routines.
> >>
> >> Patch 4/6
> >> - Create the new /sys/kernel/dlpar interface
> >>
> >> Patch 5/6
> >> - Implement memory hotplug add in the kernel.
> >>
> >> Patch 6/6
> >> - Implement memory hotplug remove in the kernel.
> >>
> >> include/asm/prom.h | 10
> >> include/asm/rtas.h | 26 ++
> >> platforms/pseries/dlpar.c | 72 +++++
> >> platforms/pseries/hotplug-memory.c | 469 ++++++++++++++++++++++++++++++++++++-
> >> platforms/pseries/pseries.h | 12
> >> 5 files changed, 576 insertions(+), 13 deletions(-)
> >>
> >> _______________________________________________
> >> Linuxppc-dev mailing list
> >> Linuxppc-dev@lists.ozlabs.org
> >> https://lists.ozlabs.org/listinfo/linuxppc-dev
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
> >
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [PATCH V9 00/18] Enable SRIOV on PowerNV
From: Gavin Shan @ 2014-11-18 23:11 UTC (permalink / raw)
To: Wei Yang; +Cc: bhelgaas, linux-pci, benh, linuxppc-dev, gwshan
In-Reply-To: <1414942894-17034-1-git-send-email-weiyang@linux.vnet.ibm.com>
On Sun, Nov 02, 2014 at 11:41:16PM +0800, Wei Yang wrote:
Hello Bjorn,
Did you have available bandwidth to review it? :-)
Thanks,
Gavin
>This patchset enables the SRIOV on POWER8.
>
>The gerneral idea is put each VF into one individual PE and allocate required
>resources like MMIO/DMA/MSI. The major difficulty comes from the MMIO
>allocation and adjustment for PF's IOV BAR.
>
>On P8, we use M64BT to cover a PF's IOV BAR, which could make an individual VF
>sit in its own PE. This gives more flexiblity, while at the mean time it
>brings on some restrictions on the PF's IOV BAR size and alignment.
>
>To achieve this effect, we need to do some hack on pci devices's resources.
>1. Expand the IOV BAR properly.
> Done by pnv_pci_ioda_fixup_iov_resources().
>2. Shift the IOV BAR properly.
> Done by pnv_pci_vf_resource_shift().
>3. IOV BAR alignment is calculated by arch dependent function instead of an
> individual VF BAR size.
> Done by pnv_pcibios_sriov_resource_alignment().
>4. Take the IOV BAR alignment into consideration in the sizing and assigning.
> This is achieved by commit: "PCI: Take additional IOV BAR alignment in
> sizing and assigning"
>
>Test Environment:
> The SRIOV device tested is Emulex Lancer(10df:e220) and
> Mellanox ConnectX-3(15b3:1003) on POWER8.
>
>Examples on pass through a VF to guest through vfio:
> 1. unbind the original driver and bind to vfio-pci driver
> echo 0000:06:0d.0 > /sys/bus/pci/devices/0000:06:0d.0/driver/unbind
> echo 1102 0002 > /sys/bus/pci/drivers/vfio-pci/new_id
> Note: this should be done for each device in the same iommu_group
> 2. Start qemu and pass device through vfio
> /home/ywywyang/git/qemu-impreza/ppc64-softmmu/qemu-system-ppc64 \
> -M pseries -m 2048 -enable-kvm -nographic \
> -drive file=/home/ywywyang/kvm/fc19.img \
> -monitor telnet:localhost:5435,server,nowait -boot cd \
> -device "spapr-pci-vfio-host-bridge,id=CXGB3,iommu=26,index=6"
>
>Verify this is the exact VF response:
> 1. ping from a machine in the same subnet(the broadcast domain)
> 2. run arp -n on this machine
> 9.115.251.20 ether 00:00:c9:df:ed:bf C eth0
> 3. ifconfig in the guest
> # ifconfig eth1
> eth1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500
> inet 9.115.251.20 netmask 255.255.255.0 broadcast 9.115.251.255
> inet6 fe80::200:c9ff:fedf:edbf prefixlen 64 scopeid 0x20<link>
> ether 00:00:c9:df:ed:bf txqueuelen 1000 (Ethernet)
> RX packets 175 bytes 13278 (12.9 KiB)
> RX errors 0 dropped 0 overruns 0 frame 0
> TX packets 58 bytes 9276 (9.0 KiB)
> TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
> 4. They have the same MAC address
>
> Note: make sure you shutdown other network interfaces in guest.
>
>---
>v9:
> * make the change log consistent in the terminology
> PF's IOV BAR -> the SRIOV BAR in PF
> VF's BAR -> the normal BAR in VF's view
> * rename all newly introduced function from _sriov_ to _iov_
> * rename the document to Documentation/powerpc/pci_iov_resource_on_powernv.txt
> * add the vendor id and device id of the tested devices
> * change return value from EINVAL to ENOSYS for pci_iov_virtfn_bus() and
> pci_iov_virtfn_devfn() when it is called on PF or SRIOV is not configured
> * rebase on 3.18-rc2 and tested
>v8:
> * use weak funcion pcibios_sriov_resource_size() instead of some flag to
> retrieve the IOV BAR size.
> * add a document Documentation/powerpc/pci_resource.txt to explain the
> design.
> * make pci_iov_virtfn_bus()/pci_iov_virtfn_devfn() not inline.
> * extract a function res_to_dev_res(), so that it is more general to get
> additional size and alignment
> * fix one contention which is introduced in "powrepc/pci: Refactor pci_dn".
> the root cause is pci_get_slot() takes pci_bus_sem and leads to dead
> lock.
>v7:
> * add IORESOURCE_ARCH flag for IOV BAR on powernv platform.
> * when IOV BAR has IORESOURCE_ARCH flag, the size is retrieved from
> hardware directly. If not, calculate as usual.
> * reorder the patch set, group them by subsystem:
> PCI, powerpc, powernv
> * rebase it on 3.16-rc6
>v6:
> * remove pcibios_enable_sriov()/pcibios_disable_sriov() weak function
> similar function is moved to
> pnv_pci_enable_device_hook()/pnv_pci_disable_device_hook(). When PF is
> enabled, platform will try best to allocate resources for VFs.
> * remove pcibios_sriov_resource_size weak function
> * VF BAR size is retrieved from hardware directly in virtfn_add()
>v5:
> * merge those SRIOV related platform functions in machdep_calls
> wrap them in one CONFIG_PCI_IOV marco
> * define IODA_INVALID_M64 to replace (-1)
> use this value to represent the m64_wins is not used
> * rename pnv_pci_release_dev_dma() to pnv_pci_ioda2_release_dma_pe()
> this function is a conterpart to pnv_pci_ioda2_setup_dma_pe()
> * change dev_info() to dev_dgb() in pnv_pci_ioda_fixup_iov_resources()
> reduce some log in kernel
> * release M64 window in pnv_pci_ioda2_release_dma_pe()
>v4:
> * code format fix, eg. not exceed 80 chars
> * in commit "ppc/pnv: Add function to deconfig a PE"
> check the bus has a bridge before print the name
> remove a PE from its own PELTV
> * change the function name for sriov resource size/alignment
> * rebase on 3.16-rc3
> * VFs will not rely on device node
> As Grant Likely's comments, kernel should have the ability to handle the
> lack of device_node gracefully. Gavin restructure the pci_dn, which
> makes the VF will have pci_dn even when VF's device_node is not provided
> by firmware.
> * clean all the patch title to make them comply with one style
> * fix return value for pci_iov_virtfn_bus/pci_iov_virtfn_devfn
>v3:
> * change the return type of virtfn_bus/virtfn_devfn to int
> change the name of these two functions to pci_iov_virtfn_bus/pci_iov_virtfn_devfn
> * reduce the second parameter or pcibios_sriov_disable()
> * use data instead of pe in "ppc/pnv: allocate pe->iommu_table dynamically"
> * rename __pci_sriov_resource_size to pcibios_sriov_resource_size
> * rename __pci_sriov_resource_alignment to pcibios_sriov_resource_alignment
>v2:
> * change the return value of virtfn_bus/virtfn_devfn to 0
> * move some TCE related marco definition to
> arch/powerpc/platforms/powernv/pci.h
> * fix the __pci_sriov_resource_alignment on powernv platform
> During the sizing stage, the IOV BAR is truncated to 0, which will
> effect the order of allocation. Fix this, so that make sure BAR will be
> allocated ordered by their alignment.
>v1:
> * improve the change log for
> "PCI: Add weak __pci_sriov_resource_size() interface"
> "PCI: Add weak __pci_sriov_resource_alignment() interface"
> "PCI: take additional IOV BAR alignment in sizing and assigning"
> * wrap VF PE code in CONFIG_PCI_IOV
> * did regression test on P7.
>
>Gavin Shan (1):
> powrepc/pci: Refactor pci_dn
>
>Wei Yang (17):
> PCI/IOV: Export interface for retrieve VF's BDF
> PCI: Add weak pcibios_iov_resource_alignment() interface
> PCI: Add weak pcibios_iov_resource_size() interface
> PCI: Take additional PF's IOV BAR alignment in sizing and assigning
> powerpc/pci: Add PCI resource alignment documentation
> powerpc/pci: Don't unset pci resources for VFs
> powerpc/pci: Define pcibios_disable_device() on powerpc
> powerpc/pci: remove pci_dn->pcidev field
> powerpc/powernv: Use pci_dn in PCI config accessor
> powerpc/powernv: Allocate pe->iommu_table dynamically
> powerpc/powernv: Expand VF resources according to the number of
> total_pe
> powerpc/powernv: Implement pcibios_iov_resource_alignment() on
> powernv
> powerpc/powernv: Implement pcibios_iov_resource_size() on powernv
> powerpc/powernv: Shift VF resource with an offset
> powerpc/powernv: Allocate VF PE
> powerpc/powernv: Expanding IOV BAR, with m64_per_iov supported
> powerpc/powernv: Group VF PE when IOV BAR is big on PHB3
>
> .../powerpc/pci_iov_resource_on_powernv.txt | 75 ++
> arch/powerpc/include/asm/device.h | 3 +
> arch/powerpc/include/asm/iommu.h | 3 +
> arch/powerpc/include/asm/machdep.h | 13 +-
> arch/powerpc/include/asm/pci-bridge.h | 24 +-
> arch/powerpc/kernel/pci-common.c | 39 +
> arch/powerpc/kernel/pci-hotplug.c | 3 +
> arch/powerpc/kernel/pci_dn.c | 257 ++++++-
> arch/powerpc/platforms/powernv/eeh-powernv.c | 14 +-
> arch/powerpc/platforms/powernv/pci-ioda.c | 744 +++++++++++++++++++-
> arch/powerpc/platforms/powernv/pci.c | 87 +--
> arch/powerpc/platforms/powernv/pci.h | 13 +-
> drivers/pci/iov.c | 60 +-
> drivers/pci/setup-bus.c | 85 ++-
> include/linux/pci.h | 19 +
> 15 files changed, 1332 insertions(+), 107 deletions(-)
> create mode 100644 Documentation/powerpc/pci_iov_resource_on_powernv.txt
>
>--
>1.7.9.5
>
^ permalink raw reply
* Re: [PATCH V9 00/18] Enable SRIOV on PowerNV
From: Bjorn Helgaas @ 2014-11-18 23:40 UTC (permalink / raw)
To: Gavin Shan
Cc: linux-pci@vger.kernel.org, Wei Yang, Benjamin Herrenschmidt,
linuxppc-dev
In-Reply-To: <20141118231124.GA6212@shangw>
On Tue, Nov 18, 2014 at 4:11 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
> On Sun, Nov 02, 2014 at 11:41:16PM +0800, Wei Yang wrote:
>
> Hello Bjorn,
>
> Did you have available bandwidth to review it? :-)
I'm working on it right now :)
>>This patchset enables the SRIOV on POWER8.
>>
>>The gerneral idea is put each VF into one individual PE and allocate required
>>resources like MMIO/DMA/MSI. The major difficulty comes from the MMIO
>>allocation and adjustment for PF's IOV BAR.
>>
>>On P8, we use M64BT to cover a PF's IOV BAR, which could make an individual VF
>>sit in its own PE. This gives more flexiblity, while at the mean time it
>>brings on some restrictions on the PF's IOV BAR size and alignment.
>>
>>To achieve this effect, we need to do some hack on pci devices's resources.
>>1. Expand the IOV BAR properly.
>> Done by pnv_pci_ioda_fixup_iov_resources().
>>2. Shift the IOV BAR properly.
>> Done by pnv_pci_vf_resource_shift().
>>3. IOV BAR alignment is calculated by arch dependent function instead of an
>> individual VF BAR size.
>> Done by pnv_pcibios_sriov_resource_alignment().
>>4. Take the IOV BAR alignment into consideration in the sizing and assigning.
>> This is achieved by commit: "PCI: Take additional IOV BAR alignment in
>> sizing and assigning"
>>
>>Test Environment:
>> The SRIOV device tested is Emulex Lancer(10df:e220) and
>> Mellanox ConnectX-3(15b3:1003) on POWER8.
>>
>>Examples on pass through a VF to guest through vfio:
>> 1. unbind the original driver and bind to vfio-pci driver
>> echo 0000:06:0d.0 > /sys/bus/pci/devices/0000:06:0d.0/driver/unbind
>> echo 1102 0002 > /sys/bus/pci/drivers/vfio-pci/new_id
>> Note: this should be done for each device in the same iommu_group
>> 2. Start qemu and pass device through vfio
>> /home/ywywyang/git/qemu-impreza/ppc64-softmmu/qemu-system-ppc64 \
>> -M pseries -m 2048 -enable-kvm -nographic \
>> -drive file=/home/ywywyang/kvm/fc19.img \
>> -monitor telnet:localhost:5435,server,nowait -boot cd \
>> -device "spapr-pci-vfio-host-bridge,id=CXGB3,iommu=26,index=6"
>>
>>Verify this is the exact VF response:
>> 1. ping from a machine in the same subnet(the broadcast domain)
>> 2. run arp -n on this machine
>> 9.115.251.20 ether 00:00:c9:df:ed:bf C eth0
>> 3. ifconfig in the guest
>> # ifconfig eth1
>> eth1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500
>> inet 9.115.251.20 netmask 255.255.255.0 broadcast 9.115.251.255
>> inet6 fe80::200:c9ff:fedf:edbf prefixlen 64 scopeid 0x20<link>
>> ether 00:00:c9:df:ed:bf txqueuelen 1000 (Ethernet)
>> RX packets 175 bytes 13278 (12.9 KiB)
>> RX errors 0 dropped 0 overruns 0 frame 0
>> TX packets 58 bytes 9276 (9.0 KiB)
>> TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
>> 4. They have the same MAC address
>>
>> Note: make sure you shutdown other network interfaces in guest.
>>
>>---
>>v9:
>> * make the change log consistent in the terminology
>> PF's IOV BAR -> the SRIOV BAR in PF
>> VF's BAR -> the normal BAR in VF's view
>> * rename all newly introduced function from _sriov_ to _iov_
>> * rename the document to Documentation/powerpc/pci_iov_resource_on_powernv.txt
>> * add the vendor id and device id of the tested devices
>> * change return value from EINVAL to ENOSYS for pci_iov_virtfn_bus() and
>> pci_iov_virtfn_devfn() when it is called on PF or SRIOV is not configured
>> * rebase on 3.18-rc2 and tested
>>v8:
>> * use weak funcion pcibios_sriov_resource_size() instead of some flag to
>> retrieve the IOV BAR size.
>> * add a document Documentation/powerpc/pci_resource.txt to explain the
>> design.
>> * make pci_iov_virtfn_bus()/pci_iov_virtfn_devfn() not inline.
>> * extract a function res_to_dev_res(), so that it is more general to get
>> additional size and alignment
>> * fix one contention which is introduced in "powrepc/pci: Refactor pci_dn".
>> the root cause is pci_get_slot() takes pci_bus_sem and leads to dead
>> lock.
>>v7:
>> * add IORESOURCE_ARCH flag for IOV BAR on powernv platform.
>> * when IOV BAR has IORESOURCE_ARCH flag, the size is retrieved from
>> hardware directly. If not, calculate as usual.
>> * reorder the patch set, group them by subsystem:
>> PCI, powerpc, powernv
>> * rebase it on 3.16-rc6
>>v6:
>> * remove pcibios_enable_sriov()/pcibios_disable_sriov() weak function
>> similar function is moved to
>> pnv_pci_enable_device_hook()/pnv_pci_disable_device_hook(). When PF is
>> enabled, platform will try best to allocate resources for VFs.
>> * remove pcibios_sriov_resource_size weak function
>> * VF BAR size is retrieved from hardware directly in virtfn_add()
>>v5:
>> * merge those SRIOV related platform functions in machdep_calls
>> wrap them in one CONFIG_PCI_IOV marco
>> * define IODA_INVALID_M64 to replace (-1)
>> use this value to represent the m64_wins is not used
>> * rename pnv_pci_release_dev_dma() to pnv_pci_ioda2_release_dma_pe()
>> this function is a conterpart to pnv_pci_ioda2_setup_dma_pe()
>> * change dev_info() to dev_dgb() in pnv_pci_ioda_fixup_iov_resources()
>> reduce some log in kernel
>> * release M64 window in pnv_pci_ioda2_release_dma_pe()
>>v4:
>> * code format fix, eg. not exceed 80 chars
>> * in commit "ppc/pnv: Add function to deconfig a PE"
>> check the bus has a bridge before print the name
>> remove a PE from its own PELTV
>> * change the function name for sriov resource size/alignment
>> * rebase on 3.16-rc3
>> * VFs will not rely on device node
>> As Grant Likely's comments, kernel should have the ability to handle the
>> lack of device_node gracefully. Gavin restructure the pci_dn, which
>> makes the VF will have pci_dn even when VF's device_node is not provided
>> by firmware.
>> * clean all the patch title to make them comply with one style
>> * fix return value for pci_iov_virtfn_bus/pci_iov_virtfn_devfn
>>v3:
>> * change the return type of virtfn_bus/virtfn_devfn to int
>> change the name of these two functions to pci_iov_virtfn_bus/pci_iov_virtfn_devfn
>> * reduce the second parameter or pcibios_sriov_disable()
>> * use data instead of pe in "ppc/pnv: allocate pe->iommu_table dynamically"
>> * rename __pci_sriov_resource_size to pcibios_sriov_resource_size
>> * rename __pci_sriov_resource_alignment to pcibios_sriov_resource_alignment
>>v2:
>> * change the return value of virtfn_bus/virtfn_devfn to 0
>> * move some TCE related marco definition to
>> arch/powerpc/platforms/powernv/pci.h
>> * fix the __pci_sriov_resource_alignment on powernv platform
>> During the sizing stage, the IOV BAR is truncated to 0, which will
>> effect the order of allocation. Fix this, so that make sure BAR will be
>> allocated ordered by their alignment.
>>v1:
>> * improve the change log for
>> "PCI: Add weak __pci_sriov_resource_size() interface"
>> "PCI: Add weak __pci_sriov_resource_alignment() interface"
>> "PCI: take additional IOV BAR alignment in sizing and assigning"
>> * wrap VF PE code in CONFIG_PCI_IOV
>> * did regression test on P7.
>>
>>Gavin Shan (1):
>> powrepc/pci: Refactor pci_dn
>>
>>Wei Yang (17):
>> PCI/IOV: Export interface for retrieve VF's BDF
>> PCI: Add weak pcibios_iov_resource_alignment() interface
>> PCI: Add weak pcibios_iov_resource_size() interface
>> PCI: Take additional PF's IOV BAR alignment in sizing and assigning
>> powerpc/pci: Add PCI resource alignment documentation
>> powerpc/pci: Don't unset pci resources for VFs
>> powerpc/pci: Define pcibios_disable_device() on powerpc
>> powerpc/pci: remove pci_dn->pcidev field
>> powerpc/powernv: Use pci_dn in PCI config accessor
>> powerpc/powernv: Allocate pe->iommu_table dynamically
>> powerpc/powernv: Expand VF resources according to the number of
>> total_pe
>> powerpc/powernv: Implement pcibios_iov_resource_alignment() on
>> powernv
>> powerpc/powernv: Implement pcibios_iov_resource_size() on powernv
>> powerpc/powernv: Shift VF resource with an offset
>> powerpc/powernv: Allocate VF PE
>> powerpc/powernv: Expanding IOV BAR, with m64_per_iov supported
>> powerpc/powernv: Group VF PE when IOV BAR is big on PHB3
>>
>> .../powerpc/pci_iov_resource_on_powernv.txt | 75 ++
>> arch/powerpc/include/asm/device.h | 3 +
>> arch/powerpc/include/asm/iommu.h | 3 +
>> arch/powerpc/include/asm/machdep.h | 13 +-
>> arch/powerpc/include/asm/pci-bridge.h | 24 +-
>> arch/powerpc/kernel/pci-common.c | 39 +
>> arch/powerpc/kernel/pci-hotplug.c | 3 +
>> arch/powerpc/kernel/pci_dn.c | 257 ++++++-
>> arch/powerpc/platforms/powernv/eeh-powernv.c | 14 +-
>> arch/powerpc/platforms/powernv/pci-ioda.c | 744 +++++++++++++++++++-
>> arch/powerpc/platforms/powernv/pci.c | 87 +--
>> arch/powerpc/platforms/powernv/pci.h | 13 +-
>> drivers/pci/iov.c | 60 +-
>> drivers/pci/setup-bus.c | 85 ++-
>> include/linux/pci.h | 19 +
>> 15 files changed, 1332 insertions(+), 107 deletions(-)
>> create mode 100644 Documentation/powerpc/pci_iov_resource_on_powernv.txt
>>
>>--
>>1.7.9.5
>>
>
^ permalink raw reply
* Re: [PATCH V9 03/18] PCI: Add weak pcibios_iov_resource_size() interface
From: Bjorn Helgaas @ 2014-11-19 1:12 UTC (permalink / raw)
To: Wei Yang
Cc: benh, linux-pci, linuxppc-dev, gwshan, Donald Dutile, Myron Stowe
In-Reply-To: <1414942894-17034-4-git-send-email-weiyang@linux.vnet.ibm.com>
[+cc Don, Myron]
Hi Wei,
On Sun, Nov 02, 2014 at 11:41:19PM +0800, Wei Yang wrote:
> When retrieving VF IOV BAR in virtfn_add(), it will divide the total PF's IOV
> BAR size with the totalVF number. This is true for most cases, while may not
> be correct on some specific platform.
>
> For example on PowerNV platform, in order to fix PF's IOV BAR into a hardware
> alignment, the PF's IOV BAR size would be expended. This means the original
> method couldn't work.
>
> This patch introduces a weak pcibios_iov_resource_size() interface, which
> gives platform a chance to implement specific method to calculate the VF BAR
> resource size.
>
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> ---
> drivers/pci/iov.c | 27 +++++++++++++++++++++++++--
> include/linux/pci.h | 5 +++++
> 2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 4d1685d..6866830 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -61,6 +61,30 @@ static void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus)
> pci_remove_bus(virtbus);
> }
>
> +resource_size_t __weak pcibios_iov_resource_size(struct pci_dev *dev, int resno)
> +{
> + return 0;
> +}
> +
> +resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
> +{
> + resource_size_t size;
> + struct pci_sriov *iov;
> +
> + if (!dev->is_physfn)
> + return 0;
> +
> + size = pcibios_iov_resource_size(dev, resno);
> + if (size != 0)
> + return size;
> +
> + iov = dev->sriov;
> + size = resource_size(dev->resource + resno);
> + do_div(size, iov->total_VFs);
> +
> + return size;
> +}
> +
> static int virtfn_add(struct pci_dev *dev, int id, int reset)
> {
> int i;
> @@ -96,8 +120,7 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset)
> continue;
> virtfn->resource[i].name = pci_name(virtfn);
> virtfn->resource[i].flags = res->flags;
> - size = resource_size(res);
> - do_div(size, iov->total_VFs);
> + size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
> virtfn->resource[i].start = res->start + size * id;
Can you help me understand this?
We have previously called sriov_init() on the PF. There, we sized the VF
BARs, which are in the PF's SR-IOV Capability (SR-IOV spec sec 3.3.14).
The size we discover is the amount of space required by a single VF, so
sriov_init() adjusts PF->resource[PCI_IOV_RESOURCES + i] by multiplying
that size by PCI_SRIOV_TOTAL_VF, so this PF resource is now big enough to
hold the VF BAR[i] areas for all the possible VFs.
Now we're in virtfn_add(), setting up a new VF. The usual BARs at config
space 0x10, etc., are read-only zeroes for a VF, so we don't size them the
usual way. Instead, we carve out a slice of the
PF->resource[PCI_IOV_RESOURCES + i] area.
I thought the starting address of the VF BARn memory aperture was
prescribed by the spec in sec 2.1.1.1 and shown in figure 2-1:
BARx VFy starting address = VF BARx + (y - 1) * (VF BARx aperture size)
That's basically what the existing code does. We don't save the VF BARx
aperture size, so we recompute it by undoing the multiplication we did in
sriov_init().
But you're computing the starting address using a different VF BARx
aperture size. How does that work? I assumed this calculation was built
into the PCI device, but obviously I'm missing something.
To make it concrete, here's a made-up example:
PF SR-IOV Capability
TotalVFs = 4
NumVFs = 4
System Page Size = 4KB
VF BAR0 = [mem 0x00000000-0x00000fff] (4KB at address 0)
PF pci_dev->resource[7] = [mem 0x00000000-0x00003fff] (16KB)
VF1 pci_dev->resource[0] = [mem 0x00000000-0x00000fff]
VF2 pci_dev->resource[0] = [mem 0x00001000-0x00001fff]
VF3 pci_dev->resource[0] = [mem 0x00002000-0x00002fff]
VF4 pci_dev->resource[0] = [mem 0x00003000-0x00003fff]
You're changing this so we might use 16KB as the VF BAR0 aperture size
instead of 4KB, which would result in the following:
VF1 pci_dev->resource[0] = [mem 0x00000000-0x00003fff]
VF2 pci_dev->resource[0] = [mem 0x00004000-0x00007fff]
VF3 pci_dev->resource[0] = [mem 0x00008000-0x0000bfff]
VF4 pci_dev->resource[0] = [mem 0x0000c000-0x0000ffff]
But you didn't change sriov_init(), so the PF resource[7] is only 16KB, not
the 64KB the VFs need. And I assume the VF address decoder is wired to
claim the 4KB regions at 0x0000, 0x1000, 0x2000, 0x3000, not the ones at
0x0000, 0x4000, 0x8000, 0xc000.
Bjorn
> virtfn->resource[i].end = virtfn->resource[i].start + size - 1;
> rc = request_resource(res, &virtfn->resource[i]);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index bbf8058..2f5b454 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1162,6 +1162,8 @@ resource_size_t pcibios_window_alignment(struct pci_bus *bus,
> resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev,
> int resno,
> resource_size_t align);
> +resource_size_t pcibios_iov_resource_size(struct pci_dev *dev,
> + int resno);
>
> #define PCI_VGA_STATE_CHANGE_BRIDGE (1 << 0)
> #define PCI_VGA_STATE_CHANGE_DECODES (1 << 1)
> @@ -1666,6 +1668,7 @@ int pci_num_vf(struct pci_dev *dev);
> int pci_vfs_assigned(struct pci_dev *dev);
> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> int pci_sriov_get_totalvfs(struct pci_dev *dev);
> +resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
> #else
> static inline int pci_iov_virtfn_bus(struct pci_dev *dev, int id)
> {
> @@ -1685,6 +1688,8 @@ static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> { return 0; }
> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> { return 0; }
> +static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
> +{ return 0; }
> #endif
>
> #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
> --
> 1.7.9.5
>
^ permalink raw reply
* Re: [RFC PATCH 01/16] PCI: Enhance pci_scan_root_bus() to support default IO/MEM resources
From: Yijing Wang @ 2014-11-19 1:15 UTC (permalink / raw)
To: Liviu Dudau
Cc: Liviu Dudau, Tony Luck, Russell King, Arnd Bergmann,
linux-pci@vger.kernel.org, x86@kernel.org,
linux-kernel@vger.kernel.org, huxinwei@huawei.com, Thierry Reding,
Yijing Wang, suravee.suthikulpanit@amd.com, Bjorn Helgaas,
linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20141118142354.GH12037@e106497-lin.cambridge.arm.com>
On 2014/11/18 22:23, Liviu Dudau wrote:
> On Tue, Nov 18, 2014 at 11:46:06AM +0000, Yijing Wang wrote:
>> On 2014/11/18 17:36, Arnd Bergmann wrote:
>>> On Tuesday 18 November 2014 15:44:23 Yijing Wang wrote:
>>>> On 2014/11/17 18:08, Arnd Bergmann wrote:
>>>>> On Monday 17 November 2014 18:21:35 Yijing Wang wrote:
>>>>>> - list_for_each_entry(window, resources, list)
>>>>>> - if (window->res->flags & IORESOURCE_BUS) {
>>>>>> - found = true;
>>>>>> - break;
>>>>>> - }
>>>>>> + if (!resources) {
>>>>>> + pci_add_resource(&default_res, &ioport_resource);
>>>>>> + pci_add_resource(&default_res, &iomem_resource);
>>>>>> + pci_add_resource(&default_res, &busn_resource);
>>>>>> + } else {
>>>>>>
>>>>>
>>>>> Isn't it almost always wrong to do this? You are adding all of the
>>>>> I/O ports and memory to the host bridge, which will prevent you from
>>>>> adding another host bridge, and the iomem_resource normally
>>>>> includes a lot of addresses that are not accessible by the PCI host.
>>>>
>>>> Hi Arnd, pci host bridge windows are the ranges allow child devices to setup
>>>> from. Add all of IO/MEM here just a limit to child devices, no request for these
>>>> resources, so it won't hurt another host bridge. Some platforms have no dts or ACPI
>>>> report host bridge resources, in this case, we directly assign ioport/iomem_resources
>>>> as the root resources of PCI devices.
>>>
>>> But it would be wrong to allow hosts to allocate a device BAR that is not
>>> visible through the host bridge. I think we need to keep these separate
>>> from the general case: if you call any of the modern interfaces you have
>>> to provide the resources and a device. I notice that there is only one
>>> caller of pci_scan_bus_parented(), we should probably change that over to
>>> pci_scan_root_bus() or your new interface and remove the old one, but
>>> keep pci_scan_bus() as the only entry point for all of the legacy users
>>> that do not know about the resources.
>>
>> Ok, I will move this out of the generic interface.
>
> My suggestion would actually be to trigger a warning/error if you detect that the resources
> are missing. That way we can force the drivers to clean up.
It make sense to me, thanks.
>
> Best regards,
> Liviu
>
>>
>> Thanks!
>> Yijing.
>>
>>>
>>> Arnd
>>>
>>> .
>>>
>>
>>
>> --
>> Thanks!
>> Yijing
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [RFC PATCH 02/16] PCI: Use pci_scan_root_bus() instead of pci_scan_bus()
From: Yijing Wang @ 2014-11-19 1:19 UTC (permalink / raw)
To: Liviu Dudau
Cc: Liviu Dudau, Tony Luck, Russell King, linux-pci@vger.kernel.org,
x86@kernel.org, linux-kernel@vger.kernel.org, huxinwei@huawei.com,
Thierry Reding, Yijing Wang, suravee.suthikulpanit@amd.com,
Bjorn Helgaas, linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20141118142817.GI12037@e106497-lin.cambridge.arm.com>
On 2014/11/18 22:28, Liviu Dudau wrote:
> On Mon, Nov 17, 2014 at 10:21:36AM +0000, Yijing Wang wrote:
>> From: Yijing Wang <wangyijing0307@gmail.com>
>>
>> Now we could use pci_scan_root_bus() instead of
>> pci_scan_bus(), pass NULL resources means use the default
>> io/mem.
>
> Actually, I think this patch goes in the wrong direction. You are adding
> two additional parameters that are NULL for every call you replace.
> pci_scan_bus() was actually intended as a "shortcut with defaults" kind
> of call so that you don't have to pass a value for parameters you don't
> care about.
>
> My suggestion would be to keep the function as is and replace the call
> to pci_create_root_bus() inside with the appropriate sequence.
Hmmm, maybe you are right, provide a simple pci scan interfaces
(which don't care parent and resources) is better than use a complex
one with several NULL arguments. :)
>
> Best regards,
> Liviu
>
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
>> arch/alpha/include/asm/pci.h | 2 +-
>> arch/alpha/kernel/sys_nautilus.c | 2 +-
>> arch/cris/include/asm/pci.h | 2 +-
>> arch/ia64/include/asm/pci.h | 2 +-
>> arch/m68k/coldfire/pci.c | 2 +-
>> arch/mips/include/asm/pci.h | 2 +-
>> arch/mn10300/include/asm/pci.h | 2 +-
>> arch/sh/include/asm/pci.h | 2 +-
>> arch/sparc/include/asm/pci_32.h | 2 +-
>> arch/sparc/include/asm/pci_64.h | 2 +-
>> arch/sparc/kernel/pcic.c | 3 ++-
>> arch/unicore32/kernel/pci.c | 2 +-
>> arch/x86/include/asm/pci.h | 2 +-
>> arch/x86/pci/amd_bus.c | 2 +-
>> arch/xtensa/include/asm/pci.h | 2 +-
>> drivers/parisc/dino.c | 4 ++--
>> drivers/pci/hotplug/ibmphp_core.c | 2 +-
>> 17 files changed, 19 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/alpha/include/asm/pci.h b/arch/alpha/include/asm/pci.h
>> index f7f680f..157925a 100644
>> --- a/arch/alpha/include/asm/pci.h
>> +++ b/arch/alpha/include/asm/pci.h
>> @@ -49,7 +49,7 @@ struct pci_controller {
>> void *sysdata;
>> };
>>
>> -/* Override the logic in pci_scan_bus for skipping already-configured
>> +/* Override the logic in pci_scan_root_bus for skipping already-configured
>> bus numbers. */
>>
>> #define pcibios_assign_all_busses() 1
>> diff --git a/arch/alpha/kernel/sys_nautilus.c b/arch/alpha/kernel/sys_nautilus.c
>> index 837c0fa..1047ab3 100644
>> --- a/arch/alpha/kernel/sys_nautilus.c
>> +++ b/arch/alpha/kernel/sys_nautilus.c
>> @@ -206,7 +206,7 @@ nautilus_init_pci(void)
>> unsigned long memtop = max_low_pfn << PAGE_SHIFT;
>>
>> /* Scan our single hose. */
>> - bus = pci_scan_bus(0, alpha_mv.pci_ops, hose);
>> + bus = pci_scan_root_bus(NULL, 0, alpha_mv.pci_ops, hose, NULL);
>> hose->bus = bus;
>> pcibios_claim_one_bus(bus);
>>
>> diff --git a/arch/cris/include/asm/pci.h b/arch/cris/include/asm/pci.h
>> index cc2399c..96b33a6 100644
>> --- a/arch/cris/include/asm/pci.h
>> +++ b/arch/cris/include/asm/pci.h
>> @@ -5,7 +5,7 @@
>> #ifdef __KERNEL__
>> #include <linux/mm.h> /* for struct page */
>>
>> -/* Can be used to override the logic in pci_scan_bus for skipping
>> +/* Can be used to override the logic in pci_scan_root_bus for skipping
>> already-configured bus numbers - to be used for buggy BIOSes
>> or architectures with incomplete PCI setup by the loader */
>>
>> diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h
>> index 52af5ed..64b34d6 100644
>> --- a/arch/ia64/include/asm/pci.h
>> +++ b/arch/ia64/include/asm/pci.h
>> @@ -20,7 +20,7 @@ struct pci_vector_struct {
>> };
>>
>> /*
>> - * Can be used to override the logic in pci_scan_bus for skipping already-configured bus
>> + * Can be used to override the logic in pci_scan_root_bus for skipping already-configured bus
>> * numbers - to be used for buggy BIOSes or architectures with incomplete PCI setup by the
>> * loader.
>> */
>> diff --git a/arch/m68k/coldfire/pci.c b/arch/m68k/coldfire/pci.c
>> index df96792..4d242fb 100644
>> --- a/arch/m68k/coldfire/pci.c
>> +++ b/arch/m68k/coldfire/pci.c
>> @@ -312,7 +312,7 @@ static int __init mcf_pci_init(void)
>> set_current_state(TASK_UNINTERRUPTIBLE);
>> schedule_timeout(msecs_to_jiffies(200));
>>
>> - rootbus = pci_scan_bus(0, &mcf_pci_ops, NULL);
>> + rootbus = pci_scan_root_bus(NULL, 0, &mcf_pci_ops, NULL, NULL);
>> rootbus->resource[0] = &mcf_pci_io;
>> rootbus->resource[1] = &mcf_pci_mem;
>>
>> diff --git a/arch/mips/include/asm/pci.h b/arch/mips/include/asm/pci.h
>> index 974b0e3..212030a 100644
>> --- a/arch/mips/include/asm/pci.h
>> +++ b/arch/mips/include/asm/pci.h
>> @@ -60,7 +60,7 @@ extern void register_pci_controller(struct pci_controller *hose);
>> extern int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin);
>>
>>
>> -/* Can be used to override the logic in pci_scan_bus for skipping
>> +/* Can be used to override the logic in pci_scan_root_bus for skipping
>> already-configured bus numbers - to be used for buggy BIOSes
>> or architectures with incomplete PCI setup by the loader */
>>
>> diff --git a/arch/mn10300/include/asm/pci.h b/arch/mn10300/include/asm/pci.h
>> index 5f70af2..6305f14 100644
>> --- a/arch/mn10300/include/asm/pci.h
>> +++ b/arch/mn10300/include/asm/pci.h
>> @@ -33,7 +33,7 @@ do { \
>> #define __pcidebug(FMT, BUS, DEVFN, WHERE, ...) do {} while (0)
>> #endif
>>
>> -/* Can be used to override the logic in pci_scan_bus for skipping
>> +/* Can be used to override the logic in pci_scan_root_bus for skipping
>> * already-configured bus numbers - to be used for buggy BIOSes or
>> * architectures with incomplete PCI setup by the loader */
>>
>> diff --git a/arch/sh/include/asm/pci.h b/arch/sh/include/asm/pci.h
>> index 5b45115..69bcb78 100644
>> --- a/arch/sh/include/asm/pci.h
>> +++ b/arch/sh/include/asm/pci.h
>> @@ -3,7 +3,7 @@
>>
>> #ifdef __KERNEL__
>>
>> -/* Can be used to override the logic in pci_scan_bus for skipping
>> +/* Can be used to override the logic in pci_scan_root_bus for skipping
>> already-configured bus numbers - to be used for buggy BIOSes
>> or architectures with incomplete PCI setup by the loader */
>>
>> diff --git a/arch/sparc/include/asm/pci_32.h b/arch/sparc/include/asm/pci_32.h
>> index 53e9b49..4ef6064 100644
>> --- a/arch/sparc/include/asm/pci_32.h
>> +++ b/arch/sparc/include/asm/pci_32.h
>> @@ -5,7 +5,7 @@
>>
>> #include <linux/dma-mapping.h>
>>
>> -/* Can be used to override the logic in pci_scan_bus for skipping
>> +/* Can be used to override the logic in pci_scan_root_bus for skipping
>> * already-configured bus numbers - to be used for buggy BIOSes
>> * or architectures with incomplete PCI setup by the loader.
>> */
>> diff --git a/arch/sparc/include/asm/pci_64.h b/arch/sparc/include/asm/pci_64.h
>> index bd00a62..6e017f9 100644
>> --- a/arch/sparc/include/asm/pci_64.h
>> +++ b/arch/sparc/include/asm/pci_64.h
>> @@ -5,7 +5,7 @@
>>
>> #include <linux/dma-mapping.h>
>>
>> -/* Can be used to override the logic in pci_scan_bus for skipping
>> +/* Can be used to override the logic in pci_scan_root_bus for skipping
>> * already-configured bus numbers - to be used for buggy BIOSes
>> * or architectures with incomplete PCI setup by the loader.
>> */
>> diff --git a/arch/sparc/kernel/pcic.c b/arch/sparc/kernel/pcic.c
>> index 6cc78c2..64aafac 100644
>> --- a/arch/sparc/kernel/pcic.c
>> +++ b/arch/sparc/kernel/pcic.c
>> @@ -390,7 +390,8 @@ static void __init pcic_pbm_scan_bus(struct linux_pcic *pcic)
>> {
>> struct linux_pbm_info *pbm = &pcic->pbm;
>>
>> - pbm->pci_bus = pci_scan_bus(pbm->pci_first_busno, &pcic_ops, pbm);
>> + pbm->pci_bus = pci_scan_root_bus(NULL, pbm->pci_first_busno,
>> + &pcic_ops, pbm, NULL);
>> #if 0 /* deadwood transplanted from sparc64 */
>> pci_fill_in_pbm_cookies(pbm->pci_bus, pbm, pbm->prom_node);
>> pci_record_assignments(pbm, pbm->pci_bus);
>> diff --git a/arch/unicore32/kernel/pci.c b/arch/unicore32/kernel/pci.c
>> index 374a055..be0f261 100644
>> --- a/arch/unicore32/kernel/pci.c
>> +++ b/arch/unicore32/kernel/pci.c
>> @@ -258,7 +258,7 @@ static int __init pci_common_init(void)
>>
>> pci_puv3_preinit();
>>
>> - puv3_bus = pci_scan_bus(0, &pci_puv3_ops, NULL);
>> + puv3_bus = pci_scan_root_bus(NULL, 0, &pci_puv3_ops, NULL, NULL);
>>
>> if (!puv3_bus)
>> panic("PCI: unable to scan bus!");
>> diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
>> index 0892ea0..0540ff7 100644
>> --- a/arch/x86/include/asm/pci.h
>> +++ b/arch/x86/include/asm/pci.h
>> @@ -41,7 +41,7 @@ static inline int pci_proc_domain(struct pci_bus *bus)
>> }
>> #endif
>>
>> -/* Can be used to override the logic in pci_scan_bus for skipping
>> +/* Can be used to override the logic in pci_scan_root_bus for skipping
>> already-configured bus numbers - to be used for buggy BIOSes
>> or architectures with incomplete PCI setup by the loader */
>>
>> diff --git a/arch/x86/pci/amd_bus.c b/arch/x86/pci/amd_bus.c
>> index c20d2cc..baf6209 100644
>> --- a/arch/x86/pci/amd_bus.c
>> +++ b/arch/x86/pci/amd_bus.c
>> @@ -52,7 +52,7 @@ static struct pci_root_info __init *find_pci_root_info(int node, int link)
>>
>> /**
>> * early_root_info_init()
>> - * called before pcibios_scan_root and pci_scan_bus
>> + * called before pcibios_scan_root and pci_scan_root_bus
>> * fills the mp_bus_to_cpumask array based according
>> * to the LDT Bus Number Registers found in the northbridge.
>> */
>> diff --git a/arch/xtensa/include/asm/pci.h b/arch/xtensa/include/asm/pci.h
>> index 5d52dc4..377fae9 100644
>> --- a/arch/xtensa/include/asm/pci.h
>> +++ b/arch/xtensa/include/asm/pci.h
>> @@ -13,7 +13,7 @@
>>
>> #ifdef __KERNEL__
>>
>> -/* Can be used to override the logic in pci_scan_bus for skipping
>> +/* Can be used to override the logic in pci_scan_root_bus for skipping
>> * already-configured bus numbers - to be used for buggy BIOSes
>> * or architectures with incomplete PCI setup by the loader
>> */
>> diff --git a/drivers/parisc/dino.c b/drivers/parisc/dino.c
>> index a0580af..67c31bd 100644
>> --- a/drivers/parisc/dino.c
>> +++ b/drivers/parisc/dino.c
>> @@ -74,7 +74,7 @@
>> ** assigned a PCI bus number based on "when" it's discovered.
>> **
>> ** The "secondary" bus number is set to this before calling
>> -** pci_scan_bus(). If any PPB's are present, the scan will
>> +** pci_scan_root_bus(). If any PPB's are present, the scan will
>> ** discover them and update the "secondary" and "subordinate"
>> ** fields in Dino's pci_bus structure.
>> **
>> @@ -787,7 +787,7 @@ static int __init dino_common_init(struct parisc_device *dev,
>>
>> pcibios_register_hba(&dino_dev->hba);
>>
>> - pci_bios = &dino_bios_ops; /* used by pci_scan_bus() */
>> + pci_bios = &dino_bios_ops;
>> pci_port = &dino_port_ops;
>>
>> /*
>> diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c
>> index 3efaf4c..b486432 100644
>> --- a/drivers/pci/hotplug/ibmphp_core.c
>> +++ b/drivers/pci/hotplug/ibmphp_core.c
>> @@ -767,7 +767,7 @@ static u8 bus_structure_fixup(u8 busno)
>> (l != 0x0000) && (l != 0xffff)) {
>> debug("%s - Inside bus_structure_fixup()\n",
>> __func__);
>> - pci_scan_bus(busno, ibmphp_pci_bus->ops, NULL);
>> + pci_scan_root_bus(NULL, busno, ibmphp_pci_bus->ops, NULL, NULL);
>> break;
>> }
>> }
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [RFC PATCH 04/16] PCI: Rip out pci_bus_add_devices() from pci_scan_root_bus()
From: Yijing Wang @ 2014-11-19 1:21 UTC (permalink / raw)
To: Liviu Dudau
Cc: Liviu Dudau, Tony Luck, Russell King, linux-pci@vger.kernel.org,
x86@kernel.org, linux-kernel@vger.kernel.org, huxinwei@huawei.com,
Thierry Reding, suravee.suthikulpanit@amd.com, Bjorn Helgaas,
linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20141118143402.GJ12037@e106497-lin.cambridge.arm.com>
On 2014/11/18 22:34, Liviu Dudau wrote:
> On Mon, Nov 17, 2014 at 10:21:38AM +0000, Yijing Wang wrote:
>> Rip out pci_bus_add_devices() from pci_scan_root_bus()
>> for following reasons.
>> 1. pci_scan_root_bus() means we only do the scan, we should
>> not add pci busses.
>> 2. A lots of drviers which use pci_scan_root_bus() call
>> pci_bus_size_bridges(), pci_bus_assign_resources() after
>> pci_scan_root_bus(). But strictly speaking pci_bus_add_devices()
>> should be called after the resources assignment.
>
> This looks like a nice cleanup! Can we split this patch out
> of the series and start the ball running on getting ACKs
> for it? You are touching a lot of drivers here, you probably
> don't want the whole series to wait on approvals for this.
OK, I will separately this one out of this series.
>
> Best regards,
> Liviu
>
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
>> arch/alpha/kernel/pci.c | 1 +
>> arch/alpha/kernel/sys_nautilus.c | 1 +
>> arch/frv/mb93090-mb00/pci-vdk.c | 8 +++++---
>> arch/ia64/sn/kernel/io_init.c | 2 ++
>> arch/m68k/coldfire/pci.c | 1 +
>> arch/microblaze/pci/pci-common.c | 2 +-
>> arch/mips/pci/pci.c | 1 +
>> arch/mn10300/unit-asb2305/pci.c | 7 +++++--
>> arch/s390/pci/pci.c | 2 +-
>> arch/sh/drivers/pci/pci.c | 1 +
>> arch/sparc/kernel/leon_pci.c | 1 +
>> arch/sparc/kernel/pcic.c | 2 ++
>> arch/tile/kernel/pci.c | 3 ++-
>> arch/tile/kernel/pci_gx.c | 3 ++-
>> arch/unicore32/kernel/pci.c | 2 +-
>> arch/x86/pci/common.c | 10 ++++++----
>> arch/xtensa/kernel/pci.c | 1 +
>> drivers/pci/host/pci-xgene.c | 2 +-
>> drivers/pci/hotplug/ibmphp_core.c | 6 ++++--
>> drivers/pci/probe.c | 1 -
>> 20 files changed, 39 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
>> index 076c35c..81251a5 100644
>> --- a/arch/alpha/kernel/pci.c
>> +++ b/arch/alpha/kernel/pci.c
>> @@ -349,6 +349,7 @@ common_init_pci(void)
>>
>> pci_assign_unassigned_resources();
>> pci_fixup_irqs(alpha_mv.pci_swizzle, alpha_mv.pci_map_irq);
>> + pci_bus_add_devices(bus);
>> }
>>
>>
>> diff --git a/arch/alpha/kernel/sys_nautilus.c b/arch/alpha/kernel/sys_nautilus.c
>> index 1047ab3..60fbd77 100644
>> --- a/arch/alpha/kernel/sys_nautilus.c
>> +++ b/arch/alpha/kernel/sys_nautilus.c
>> @@ -253,6 +253,7 @@ nautilus_init_pci(void)
>> for the root bus, so just clear it. */
>> bus->self = NULL;
>> pci_fixup_irqs(alpha_mv.pci_swizzle, alpha_mv.pci_map_irq);
>> + pci_bus_add_devices(bus);
>> }
>>
>> /*
>> diff --git a/arch/frv/mb93090-mb00/pci-vdk.c b/arch/frv/mb93090-mb00/pci-vdk.c
>> index efa5d65..85f87dc 100644
>> --- a/arch/frv/mb93090-mb00/pci-vdk.c
>> +++ b/arch/frv/mb93090-mb00/pci-vdk.c
>> @@ -316,6 +316,7 @@ void pcibios_fixup_bus(struct pci_bus *bus)
>>
>> int __init pcibios_init(void)
>> {
>> + struct pci_bus *bus = NULL;
>> struct pci_ops *dir = NULL;
>> LIST_HEAD(resources);
>>
>> @@ -383,12 +384,13 @@ int __init pcibios_init(void)
>> printk("PCI: Probing PCI hardware\n");
>> pci_add_resource(&resources, &pci_ioport_resource);
>> pci_add_resource(&resources, &pci_iomem_resource);
>> - pci_scan_root_bus(NULL, 0, pci_root_ops, NULL, &resources);
>> -
>> + bus = pci_scan_root_bus(NULL, 0, pci_root_ops, NULL, &resources);
>> + if (!bus)
>> + return -ENXIO;
>> pcibios_irq_init();
>> pcibios_fixup_irqs();
>> pcibios_resource_survey();
>> -
>> + pci_bus_add_devices(bus);
>> return 0;
>> }
>>
>> diff --git a/arch/ia64/sn/kernel/io_init.c b/arch/ia64/sn/kernel/io_init.c
>> index 0b5ce82..1be65eb 100644
>> --- a/arch/ia64/sn/kernel/io_init.c
>> +++ b/arch/ia64/sn/kernel/io_init.c
>> @@ -271,7 +271,9 @@ sn_pci_controller_fixup(int segment, int busnum, struct pci_bus *bus)
>> if (bus == NULL) {
>> kfree(res);
>> kfree(controller);
>> + return;
>> }
>> + pci_bus_add_devices(bus);
>> }
>>
>> /*
>> diff --git a/arch/m68k/coldfire/pci.c b/arch/m68k/coldfire/pci.c
>> index 4d242fb..840a431 100644
>> --- a/arch/m68k/coldfire/pci.c
>> +++ b/arch/m68k/coldfire/pci.c
>> @@ -319,6 +319,7 @@ static int __init mcf_pci_init(void)
>> pci_fixup_irqs(pci_common_swizzle, mcf_pci_map_irq);
>> pci_bus_size_bridges(rootbus);
>> pci_bus_assign_resources(rootbus);
>> + pci_bus_add_devices(rootbus);
>> return 0;
>> }
>>
>> diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
>> index 9037914..9ca7e3f 100644
>> --- a/arch/microblaze/pci/pci-common.c
>> +++ b/arch/microblaze/pci/pci-common.c
>> @@ -1344,8 +1344,8 @@ static void pcibios_scan_phb(struct pci_controller *hose)
>> }
>> bus->busn_res.start = hose->first_busno;
>> hose->bus = bus;
>> -
>> hose->last_busno = bus->busn_res.end;
>> + pci_bus_add_devices(bus);
>> }
>>
>> static int __init pcibios_init(void)
>> diff --git a/arch/mips/pci/pci.c b/arch/mips/pci/pci.c
>> index 1bf60b1..f083688 100644
>> --- a/arch/mips/pci/pci.c
>> +++ b/arch/mips/pci/pci.c
>> @@ -113,6 +113,7 @@ static void pcibios_scanbus(struct pci_controller *hose)
>> if (!pci_has_flag(PCI_PROBE_ONLY)) {
>> pci_bus_size_bridges(bus);
>> pci_bus_assign_resources(bus);
>> + pci_bus_add_devices(bus);
>> }
>> }
>> }
>> diff --git a/arch/mn10300/unit-asb2305/pci.c b/arch/mn10300/unit-asb2305/pci.c
>> index 6b4339f..011af54 100644
>> --- a/arch/mn10300/unit-asb2305/pci.c
>> +++ b/arch/mn10300/unit-asb2305/pci.c
>> @@ -345,6 +345,7 @@ void pcibios_fixup_bus(struct pci_bus *bus)
>> */
>> static int __init pcibios_init(void)
>> {
>> + struct pci_bus *bus;
>> resource_size_t io_offset, mem_offset;
>> LIST_HEAD(resources);
>>
>> @@ -376,11 +377,13 @@ static int __init pcibios_init(void)
>>
>> pci_add_resource_offset(&resources, &pci_ioport_resource, io_offset);
>> pci_add_resource_offset(&resources, &pci_iomem_resource, mem_offset);
>> - pci_scan_root_bus(NULL, 0, &pci_direct_ampci, NULL, &resources);
>> -
>> + bus = pci_scan_root_bus(NULL, 0, &pci_direct_ampci, NULL, &resources);
>> + if (!bus)
>> + return 0;
>> pcibios_irq_init();
>> pcibios_fixup_irqs();
>> pcibios_resource_survey();
>> + pci_bus_add_devices(bus);
>> return 0;
>> }
>>
>> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
>> index 2fa7b14..016ea48 100644
>> --- a/arch/s390/pci/pci.c
>> +++ b/arch/s390/pci/pci.c
>> @@ -755,7 +755,7 @@ static int zpci_scan_bus(struct zpci_dev *zdev)
>> zpci_cleanup_bus_resources(zdev);
>> return -EIO;
>> }
>> -
>> + pci_bus_add_devices(zdev->bus);
>> zdev->bus->max_bus_speed = zdev->max_bus_speed;
>> return 0;
>> }
>> diff --git a/arch/sh/drivers/pci/pci.c b/arch/sh/drivers/pci/pci.c
>> index 1bc09ee..efc1051 100644
>> --- a/arch/sh/drivers/pci/pci.c
>> +++ b/arch/sh/drivers/pci/pci.c
>> @@ -69,6 +69,7 @@ static void pcibios_scanbus(struct pci_channel *hose)
>>
>> pci_bus_size_bridges(bus);
>> pci_bus_assign_resources(bus);
>> + pci_bus_add_devices(bus);
>> } else {
>> pci_free_resource_list(&resources);
>> }
>> diff --git a/arch/sparc/kernel/leon_pci.c b/arch/sparc/kernel/leon_pci.c
>> index 899b720..2971076 100644
>> --- a/arch/sparc/kernel/leon_pci.c
>> +++ b/arch/sparc/kernel/leon_pci.c
>> @@ -40,6 +40,7 @@ void leon_pci_init(struct platform_device *ofdev, struct leon_pci_info *info)
>>
>> /* Assign devices with resources */
>> pci_assign_unassigned_resources();
>> + pci_bus_add_devices(root_bus);
>> } else {
>> pci_free_resource_list(&resources);
>> }
>> diff --git a/arch/sparc/kernel/pcic.c b/arch/sparc/kernel/pcic.c
>> index 64aafac..5457814 100644
>> --- a/arch/sparc/kernel/pcic.c
>> +++ b/arch/sparc/kernel/pcic.c
>> @@ -392,6 +392,8 @@ static void __init pcic_pbm_scan_bus(struct linux_pcic *pcic)
>>
>> pbm->pci_bus = pci_scan_root_bus(NULL, pbm->pci_first_busno,
>> &pcic_ops, pbm, NULL);
>> + if (pbm->pci_bus)
>> + pci_bus_add_devices(pbm->pci_bus);
>> #if 0 /* deadwood transplanted from sparc64 */
>> pci_fill_in_pbm_cookies(pbm->pci_bus, pbm, pbm->prom_node);
>> pci_record_assignments(pbm, pbm->pci_bus);
>> diff --git a/arch/tile/kernel/pci.c b/arch/tile/kernel/pci.c
>> index 1f80a88..268423e 100644
>> --- a/arch/tile/kernel/pci.c
>> +++ b/arch/tile/kernel/pci.c
>> @@ -326,7 +326,8 @@ int __init pcibios_init(void)
>>
>> /* Configure the max_read_size and max_payload_size values. */
>> fixup_read_and_payload_sizes();
>> -
>> + if (bus)
>> + pci_bus_add_devices(bus);
>> /* Record the I/O resources in the PCI controller structure. */
>> for (i = 0; i < TILE_NUM_PCIE; i++) {
>> /*
>> diff --git a/arch/tile/kernel/pci_gx.c b/arch/tile/kernel/pci_gx.c
>> index e39f9c5..5f05a9a 100644
>> --- a/arch/tile/kernel/pci_gx.c
>> +++ b/arch/tile/kernel/pci_gx.c
>> @@ -903,7 +903,8 @@ int __init pcibios_init(void)
>> * associated with the devices read in above.
>> */
>> pci_assign_unassigned_resources();
>> -
>> + if (bus)
>> + pci_bus_add_devices(bus);
>> /* Record the I/O resources in the PCI controller structure. */
>> for (i = 0; i < num_rc_controllers; i++) {
>> struct pci_controller *controller = &pci_controllers[i];
>> diff --git a/arch/unicore32/kernel/pci.c b/arch/unicore32/kernel/pci.c
>> index be0f261..0d4c563 100644
>> --- a/arch/unicore32/kernel/pci.c
>> +++ b/arch/unicore32/kernel/pci.c
>> @@ -270,11 +270,11 @@ static int __init pci_common_init(void)
>> * Size the bridge windows.
>> */
>> pci_bus_size_bridges(puv3_bus);
>> -
>> /*
>> * Assign resources.
>> */
>> pci_bus_assign_resources(puv3_bus);
>> + pci_bus_add_devices(puv3_bus);
>> }
>>
>> return 0;
>> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
>> index 7b20bcc..b16632b 100644
>> --- a/arch/x86/pci/common.c
>> +++ b/arch/x86/pci/common.c
>> @@ -471,10 +471,12 @@ void pcibios_scan_root(int busnum)
>> x86_pci_root_bus_resources(busnum, &resources);
>> printk(KERN_DEBUG "PCI: Probing PCI hardware (bus %02x)\n", busnum);
>> bus = pci_scan_root_bus(NULL, busnum, &pci_root_ops, sd, &resources);
>> - if (!bus) {
>> - pci_free_resource_list(&resources);
>> - kfree(sd);
>> - }
>> + if (bus) {
>> + pci_bus_add_devices(bus);
>> + return;
>> + }
>> + pci_free_resource_list(&resources);
>> + kfree(sd);
>> }
>>
>> void __init pcibios_set_cache_line_size(void)
>> diff --git a/arch/xtensa/kernel/pci.c b/arch/xtensa/kernel/pci.c
>> index 5b34033..c4b5b5d 100644
>> --- a/arch/xtensa/kernel/pci.c
>> +++ b/arch/xtensa/kernel/pci.c
>> @@ -185,6 +185,7 @@ static int __init pcibios_init(void)
>> pci_controller_apertures(pci_ctrl, &resources);
>> bus = pci_scan_root_bus(NULL, pci_ctrl->first_busno,
>> pci_ctrl->ops, pci_ctrl, &resources);
>> + pci_bus_add_devices(bus);
>> pci_ctrl->bus = bus;
>> pci_ctrl->last_busno = bus->busn_res.end;
>> if (next_busno <= pci_ctrl->last_busno)
>> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
>> index 9ecabfa..e7a4f66 100644
>> --- a/drivers/pci/host/pci-xgene.c
>> +++ b/drivers/pci/host/pci-xgene.c
>> @@ -634,7 +634,7 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
>> bus = pci_scan_root_bus(&pdev->dev, 0, &xgene_pcie_ops, port, &res);
>> if (!bus)
>> return -ENOMEM;
>> -
>> + pci_bus_add_devices(bus);
>> platform_set_drvdata(pdev, port);
>> return 0;
>> }
>> diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c
>> index b486432..76ce157 100644
>> --- a/drivers/pci/hotplug/ibmphp_core.c
>> +++ b/drivers/pci/hotplug/ibmphp_core.c
>> @@ -740,7 +740,7 @@ static void ibm_unconfigure_device(struct pci_func *func)
>> */
>> static u8 bus_structure_fixup(u8 busno)
>> {
>> - struct pci_bus *bus;
>> + struct pci_bus *bus, *b;
>> struct pci_dev *dev;
>> u16 l;
>>
>> @@ -767,7 +767,9 @@ static u8 bus_structure_fixup(u8 busno)
>> (l != 0x0000) && (l != 0xffff)) {
>> debug("%s - Inside bus_structure_fixup()\n",
>> __func__);
>> - pci_scan_root_bus(NULL, busno, ibmphp_pci_bus->ops, NULL, NULL);
>> + b = pci_scan_root_bus(NULL, busno, ibmphp_pci_bus->ops, NULL, NULL);
>> + if (b)
>> + pci_bus_add_devices(b);
>> break;
>> }
>> }
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 0a02fc2..97711f3 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2101,7 +2101,6 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
>> if (!found)
>> pci_bus_update_busn_res_end(b, max);
>>
>> - pci_bus_add_devices(b);
>> return b;
>> }
>> EXPORT_SYMBOL(pci_scan_root_bus);
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [RFC PATCH 07/16] PCI: Separate pci_host_bridge creation out of pci_create_root_bus()
From: Yijing Wang @ 2014-11-19 1:42 UTC (permalink / raw)
To: Liviu Dudau
Cc: Liviu Dudau, Tony Luck, Russell King, linux-pci@vger.kernel.org,
x86@kernel.org, linux-kernel@vger.kernel.org, huxinwei@huawei.com,
Thierry Reding, suravee.suthikulpanit@amd.com, Bjorn Helgaas,
linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20141118153020.GL12037@e106497-lin.cambridge.arm.com>
On 2014/11/18 23:30, Liviu Dudau wrote:
> On Mon, Nov 17, 2014 at 10:21:41AM +0000, Yijing Wang wrote:
>> There are some common PCI infos like domain, msi_controller, these
>> infos are saved in arch PCI sysdata, and lots arch specific functions
>> like pci_domain_nr() and pcibios_msi_controller() required.
>> We could separate pci_host_bridge creation out of pci_create_root_bus(),
>> then we could put the common infos in, then we could eliminate
>> the arch specifc functions.
>>
>
> Please Cc: Yinghai Lu and Jiang Liu on future versions.
>
> More comments on the conversion of pci_create_root_bus():
That's my mistake, will add CC Yinghai and Jiang in next version.
>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
...
>> +
>> +static void pci_release_host_bridge_dev(struct device *dev)
>> +{
>> + struct pci_host_bridge *bridge = to_pci_host_bridge(dev);
>> +
>> + if (bridge->release_fn)
>> + bridge->release_fn(bridge);
>> + pci_free_resource_list(&bridge->windows);
>> + kfree(bridge);
>> +}
>> +
>> +struct pci_host_bridge *pci_create_host_bridge(
>> + struct device *parent, u32 db,
>> + struct pci_ops *ops, void *sysdata,
>
> I don't thinks it is worth moving the buses' pci_ops into pci_host_bridge. It
> might be more useful to have pci_host_bridge specific ops here.
Because we want to create pci_host_bridge before pci root bus creation,
so when we scan the root bus and child buses, we use pci_host_bridge as
the only argument, and another pci_host_info will be addes in later patch,
which one support carry the pci_host_bridge ops.
>
>> + struct list_head *resources)
>> +{
>> + int error;
>> + struct pci_bus *b;
>> + struct pci_host_bridge *host, *h;
...
>> -struct pci_bus *pci_create_root_bus(struct device *parent, u32 db,
>> - struct pci_ops *ops, void *sysdata, struct list_head *resources)
>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *bridge)
>> {
>> int error;
>> - struct pci_host_bridge *bridge;
>> struct pci_bus *b, *b2;
>> - struct pci_host_bridge_window *window, *n;
>> + struct pci_host_bridge_window *window;
>> struct resource *res;
>> resource_size_t offset;
>> char bus_addr[64];
>> char *fmt;
>> - u8 bus = PCI_BUSNUM(db);
>> + struct device *parent = bridge->dev.parent;
>>
>> b = pci_alloc_bus(NULL);
>> if (!b)
>> return NULL;
>>
>> - b->sysdata = sysdata;
>> - b->ops = ops;
>> - b->number = b->busn_res.start = bus;
>> + b->sysdata = bridge->sysdata;
>
> I think bridge should be the b->sysdata here.
? what's the meaning?
>
>> + b->ops = bridge->ops;
>
> See comment above why I don't think this is necessary.
>
>> + b->number = b->busn_res.start = bridge->busnum;
>> pci_bus_assign_domain_nr(b, parent);
>> - b2 = pci_find_bus(pci_domain_nr(b), bus);
>> + bridge->domain = pci_domain_nr(b);
>
> Do you really want to overwrite the bridge's domain with the one from a bus that
> could possibly be rejected a couple of lines further down?
>
> As an asside: if we are doing the split of pci_host_bridge from root bus creation
> it is worth in my opinion to move the domain setup in pci_create_host_bridge()
> and stop fiddling with it here.
Hi Liviu, these lines just temporary, I will remove it after all host drivers
save its domain in pci_host_bridge.
>
> Otherwise it looks to me like you are heading in the right direction.
Thanks!
Yijing.
>
> Best regards,
> Liviu
>
>> + b2 = pci_find_bus(pci_domain_nr(b), bridge->busnum);
>> if (b2) {
>> /* If we already got to this bus through a different bridge, ignore it */
>> dev_dbg(&b2->dev, "bus already known\n");
>> goto err_out;
>> }
>>
>> - bridge = pci_alloc_host_bridge(b);
>> - if (!bridge)
>> - goto err_out;
>> -
>> - bridge->dev.parent = parent;
>> - bridge->dev.release = pci_release_host_bridge_dev;
>> - dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
>> - error = pcibios_root_bridge_prepare(bridge);
>> - if (error) {
>> - kfree(bridge);
>> - goto err_out;
>> - }
>> -
>> - error = device_register(&bridge->dev);
>> - if (error) {
>> - put_device(&bridge->dev);
>> - goto err_out;
>> - }
>> b->bridge = get_device(&bridge->dev);
>> device_enable_async_suspend(b->bridge);
>> pci_set_bus_of_node(b);
>> @@ -1950,7 +1899,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db,
>>
>> b->dev.class = &pcibus_class;
>> b->dev.parent = b->bridge;
>> - dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
>> + dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), b->number);
>> error = device_register(&b->dev);
>> if (error)
>> goto class_dev_reg_err;
>> @@ -1966,12 +1915,11 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db,
>> printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev));
>>
>> /* Add initial resources to the bus */
>> - list_for_each_entry_safe(window, n, resources, list) {
>> - list_move_tail(&window->list, &bridge->windows);
>> + list_for_each_entry(window, &bridge->windows, list) {
>> res = window->res;
>> offset = window->offset;
>> if (res->flags & IORESOURCE_BUS)
>> - pci_bus_insert_busn_res(b, bus, res->end);
>> + pci_bus_insert_busn_res(b, b->number, res->end);
>> else
>> pci_bus_add_resource(b, res, 0);
>> if (offset) {
>> @@ -2001,6 +1949,25 @@ err_out:
>> return NULL;
>> }
>>
>> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus,
>> + struct pci_ops *ops, void *sysdata, struct list_head *resources)
>> +{
>> + struct pci_host_bridge *host;
>> +
>> + host = pci_create_host_bridge(parent, bus, ops,
>> + sysdata ,resources);
>> + if (!host)
>> + return NULL;
>> +
>> + host->bus = __pci_create_root_bus(host);
>> + if (!host->bus) {
>> + pci_free_host_bridge(host);
>> + return NULL;
>> + }
>> +
>> + return host->bus;
>> +}
>> +
>> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
>> {
>> struct resource *res = &b->busn_res;
>> @@ -2069,40 +2036,37 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db,
>> {
>> struct pci_host_bridge_window *window;
>> bool found = false;
>> - struct pci_bus *b;
>> - LIST_HEAD(default_res);
>> + struct pci_host_bridge *host;
>> int max;
>>
>> - if (!resources) {
>> - pci_add_resource(&default_res, &ioport_resource);
>> - pci_add_resource(&default_res, &iomem_resource);
>> - pci_add_resource(&default_res, &busn_resource);
>> - } else {
>> - list_for_each_entry(window, resources, list)
>> - if (window->res->flags & IORESOURCE_BUS) {
>> - found = true;
>> - break;
>> - }
>> - }
>> + host = pci_create_host_bridge(parent, db, ops, sysdata, resources);
>> + if (!host)
>> + return NULL;
>>
>> - b = pci_create_root_bus(parent, db, ops, sysdata,
>> - resources ? resources : &default_res);
>> - if (!b)
>> + list_for_each_entry(window, &host->windows, list)
>> + if (window->res->flags & IORESOURCE_BUS) {
>> + found = true;
>> + break;
>> + }
>> +
>> + host->bus = __pci_create_root_bus(host);
>> + if (!host->bus) {
>> + pci_free_host_bridge(host);
>> return NULL;
>> + }
>>
>> if (!found) {
>> - dev_info(&b->dev,
>> + dev_info(&host->bus->dev,
>> "No busn resource found for root bus, will use [bus %02x-ff]\n",
>> PCI_BUSNUM(db));
>> - pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255);
>> + pci_bus_insert_busn_res(host->bus, PCI_BUSNUM(db), 255);
>> }
>>
>> - max = pci_scan_child_bus(b);
>> -
>> + max = pci_scan_child_bus(host->bus);
>> if (!found)
>> - pci_bus_update_busn_res_end(b, max);
>> + pci_bus_update_busn_res_end(host->bus, max);
>>
>> - return b;
>> + return host->bus;
>> }
>> EXPORT_SYMBOL(pci_scan_root_bus);
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 8b11b38..daa7f40 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window {
>> struct pci_host_bridge {
>> struct device dev;
>> struct pci_bus *bus; /* root bus */
>> + struct list_head list;
>> struct list_head windows; /* pci_host_bridge_windows */
>> + int busnum;
>> + int domain;
>> + void *sysdata;
>> + struct pci_ops *ops;
>> void (*release_fn)(struct pci_host_bridge *);
>> void *release_data;
>> };
>> @@ -413,7 +418,9 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>> void *release_data);
>>
>> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
>> -
>> +struct pci_host_bridge *pci_create_host_bridge(
>> + struct device *parent, u32 db, struct pci_ops *ops,
>> + void *sys, struct list_head *resources);
>> /*
>> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond
>> * to P2P or CardBus bridge windows) go in a table. Additional ones (for
>> @@ -770,6 +777,8 @@ void pci_bus_add_devices(const struct pci_bus *bus);
>> struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus,
>> struct pci_ops *ops, void *sysdata,
>> struct list_head *resources);
>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *host);
>> +void pci_free_host_bridge(struct pci_host_bridge *host);
>> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
>> int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
>> void pci_bus_release_busn_res(struct pci_bus *b);
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [RFC PATCH 08/16] PCI: Introduce pci_scan_host_bridge() and pci_host_info
From: Yijing Wang @ 2014-11-19 2:09 UTC (permalink / raw)
To: Liviu Dudau
Cc: Liviu Dudau, Tony Luck, Russell King, linux-pci@vger.kernel.org,
x86@kernel.org, linux-kernel@vger.kernel.org, huxinwei@huawei.com,
Thierry Reding, Yijing Wang, suravee.suthikulpanit@amd.com,
Bjorn Helgaas, linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20141118154243.GM12037@e106497-lin.cambridge.arm.com>
>> struct pci_host_bridge *pci_create_host_bridge(
>> - struct device *parent, u32 db,
>> - struct pci_ops *ops, void *sysdata,
>> - struct list_head *resources)
>> + struct device *parent, u32 db, struct pci_ops *ops,
>> + struct pci_host_info *info)
>> {
>> int error;
>> struct pci_bus *b;
>> struct pci_host_bridge *host, *h;
>> - struct pci_host_bridge_window *window, *n;
>>
>> down_read(&pci_host_bridge_sem);
>> list_for_each_entry(h, &pci_host_bridge_list, list) {
>> @@ -53,7 +51,7 @@ struct pci_host_bridge *pci_create_host_bridge(
>> if (!host)
>> return NULL;
>>
>> - host->sysdata = sysdata;
>> + host->sysdata = info->arg;
>> host->busnum = PCI_BUSNUM(db);
>> host->domain = PCI_DOMAIN(db);
>> host->ops = ops;
>> @@ -63,18 +61,23 @@ struct pci_host_bridge *pci_create_host_bridge(
>>
>> /* this is hack, just for build, will be removed later*/
>
> Why do you need this hack? Just for calling pci_domain_nr() ?
Yes, it's temporary code, we need domain number here for pci host bridge register.
>
>> b = kzalloc(sizeof(*b), GFP_KERNEL);
>> - b->sysdata = sysdata;
>> + b->sysdata = host->sysdata;
>> pci_bus_assign_domain_nr(b, parent);
>> host->domain = pci_domain_nr(b);
>> + kfree(b);
>>
...
>> +static int pci_default_init_res(struct pci_host_bridge *host,
>> + struct pci_host_info *info)
>> +{
>> + struct pci_host_bridge_window *window, *n;
>> +
>> + if (info->res_type != PCI_HOST_RES_DEFAULT)
>> + list_for_each_entry_safe(window, n, info->resources,
>> + list)
>> + list_move_tail(&window->list, &host->windows);
>> + else
>> + info->res_type = PCI_HOST_RES_DEFAULT;
>
> I'm confused about this assignment. Isn't this a nop as the else part
> means info->res_type *is* PCI_HOST_RES_DEFAULT?
No, in this patch, host drivers pass a pci host bridge resources init hook
in pci_host_info *info, and we call this info->init_res() in pci_create_host_bridge().
+struct pci_host_info {
+ u8 res_type;
+ void *arg;
+ struct list_head *resources; /*just for build, will clean up later */
+ int (*init_res)(struct pci_host_bridge *host,
+ struct pci_host_info *info);
+};
+
>
>> +
>> + return 0;
>> +}
...
>> @@ -2038,8 +2057,13 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db,
>> bool found = false;
>> struct pci_host_bridge *host;
>> int max;
>> + struct pci_host_info info;
>> +
>> + info.arg = sysdata;
>> + info.resources = resources;
>> + info.init_res = pci_default_init_res;
>
> I have mixed feelings about this patch. While it is heading in the right direction
> of moving pci_host_bridge relevant information towards the right user, I don't think
> you picked up the right set to move. The resource list is going to be copied into
> internal pci_host_bridge list anyway, keeping another copy is not helpful *and*
> you have increased the code size.
>
> I think for now we should aim to get the *missing* data into pci_host_bridge: MSI
> controllers and PCI domain/segment. Then we can do more cleanup.
Hi Liviu, I agree with you here, the changes to resources stuff seems not a perfect
solution. In my patch 6, we could pass pci domain nr by u32 PCI_DOMBUS(domain, bus) argument,
and store it in pci_host_bridge. For msi controller, we couldn't save the msi_controller
in pci_host_bridge. Before we assume one pci host bridge only had one msi_controller,
but now something changes, Jiang introduce hierarchy irq domain in x86, and now
one pci host bridge may has more than one msi_controller. So I prefer to add a
function to pci_host_bridge something like
struct msi_controller *pci_get_msi_controller(struct pci_dev *dev)
>
>>
>> - host = pci_create_host_bridge(parent, db, ops, sysdata, resources);
>> + host = pci_create_host_bridge(parent, db, ops, &info);
>> if (!host)
>> return NULL;
>>
>> @@ -2070,6 +2094,47 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db,
>> }
>> EXPORT_SYMBOL(pci_scan_root_bus);
>>
>> +struct pci_host_bridge *pci_scan_host_bridge(
>> + struct device *parent, u32 db, struct pci_ops *ops,
>> + struct pci_host_info *info)
>> +{
>> + struct pci_host_bridge_window *window;
>> + bool found = false;
>> + struct pci_host_bridge *host;
>> + int max;
>> +
>> + host = pci_create_host_bridge(parent, db, ops, info);
>> + if (!host)
>> + return NULL;
>> +
>> + list_for_each_entry(window, &host->windows, list)
>> + if (window->res->flags & IORESOURCE_BUS) {
>> + found = true;
>> + break;
>> + }
>> +
>> + host->bus = __pci_create_root_bus(host);
>> + if (!host->bus) {
>> + pci_free_host_bridge(host);
>> + return NULL;
>> + }
>> +
>> + if (!found) {
>> + dev_info(&host->bus->dev,
>> + "No busn resource found for root bus, will use [bus %02x-ff]\n",
>> + host->busnum);
>> + pci_bus_insert_busn_res(host->bus, host->busnum, 255);
>> + }
>> +
>> + max = pci_scan_child_bus(host->bus);
>> + if (!found)
>> + pci_bus_update_busn_res_end(host->bus, max);
>> +
>> + return host;
>> +
>> +}
>> +EXPORT_SYMBOL(pci_scan_host_bridge);
>> +
>> /**
>> * pci_rescan_bus_bridge_resize - scan a PCI bus for devices.
>> * @bridge: PCI bridge for the bus to scan
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index daa7f40..a51f5f5 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -412,6 +412,21 @@ struct pci_host_bridge {
>> void *release_data;
>> };
>>
>> +struct pci_host_info {
>> + u8 res_type;
>> + void *arg;
>> + struct list_head *resources; /*just for build, will clean up later */
>> + int (*init_res)(struct pci_host_bridge *host,
>> + struct pci_host_info *info);
>> +};
>> +
>> +static inline void init_pci_host_info(struct pci_host_info *info)
>> +{
>> + memset(info, 0 , sizeof(*info));
>> +}
>
> Where is this used?
Host driver uses it to init pci_host_info.
>
>> +
>> +#define PCI_HOST_RES_DEFAULT 0x2
>> +
>
> Magic number?
Hmmm, I will rework pci host bridge resources stuff.
>
> Best regards,
> Liviu
>
>> #define to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
>> void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>> void (*release_fn)(struct pci_host_bridge *),
>> @@ -420,7 +435,7 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
>> struct pci_host_bridge *pci_create_host_bridge(
>> struct device *parent, u32 db, struct pci_ops *ops,
>> - void *sys, struct list_head *resources);
>> + struct pci_host_info *info);
>> /*
>> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond
>> * to P2P or CardBus bridge windows) go in a table. Additional ones (for
>> @@ -785,6 +800,9 @@ void pci_bus_release_busn_res(struct pci_bus *b);
>> struct pci_bus *pci_scan_root_bus(struct device *parent, u32 bus,
>> struct pci_ops *ops, void *sysdata,
>> struct list_head *resources);
>> +struct pci_host_bridge *pci_scan_host_bridge(struct device *parent,
>> + u32 db, struct pci_ops *ops,
>> + struct pci_host_info *info);
>> struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev,
>> int busnr);
>> void pcie_update_link_speed(struct pci_bus *bus, u16 link_status);
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH V9 03/18] PCI: Add weak pcibios_iov_resource_size() interface
From: Benjamin Herrenschmidt @ 2014-11-19 2:15 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Wei Yang, linux-pci, linuxppc-dev, gwshan, Donald Dutile,
Myron Stowe
In-Reply-To: <20141119011243.GA23467@google.com>
On Tue, 2014-11-18 at 18:12 -0700, Bjorn Helgaas wrote:
>
> Can you help me understand this?
>
> We have previously called sriov_init() on the PF. There, we sized the VF
> BARs, which are in the PF's SR-IOV Capability (SR-IOV spec sec 3.3.14).
> The size we discover is the amount of space required by a single VF, so
> sriov_init() adjusts PF->resource[PCI_IOV_RESOURCES + i] by multiplying
> that size by PCI_SRIOV_TOTAL_VF, so this PF resource is now big enough to
> hold the VF BAR[i] areas for all the possible VFs.
So I'll let Richard (Wei) answer on the details but I'll just chime in
about the "big picture". This isn't about changing the spacing between VFs
which is handled by the system page size.
This is about the way we create MMIO windows from the CPU to the VF BARs.
Basically, we have a (limited) set of 64-bit windows we can create that
are divided in equal sized segments (256 of them), each segment assigned
in HW to one of our Partitionable Endpoints (aka domain).
So even if we only ever create 16 VFs for a device, we need to use an
entire of these windows, which will use 256*VF_size and thus allocate
that much space. Also the window has to be naturally aligned.
We can then assign the VF BAR to a spot inside that window that corresponds
to the range of PEs that we have assigned to that device (which typically
isn't going to be the beginning of the window).
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC PATCH 07/16] PCI: Separate pci_host_bridge creation out of pci_create_root_bus()
From: Yijing Wang @ 2014-11-19 2:24 UTC (permalink / raw)
To: Liviu Dudau
Cc: Liviu Dudau, Tony Luck, Russell King, Arnd Bergmann,
linux-pci@vger.kernel.org, x86@kernel.org,
linux-kernel@vger.kernel.org, huxinwei@huawei.com, Thierry Reding,
suravee.suthikulpanit@amd.com, Bjorn Helgaas,
linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20141118144819.GK12037@e106497-lin.cambridge.arm.com>
>> We need, some platforms pass NULL pointer as host bridge parent.
>
> Yijing,
>
> May I suggest a different approach here? Rather than having to pass an opaque
> pointer that gets converted by the host bridge driver back to the private
> structure, what about promoting a new style of usage, that is similar to the
> way device drivers work? Lets try to promote the embedding of the generic
> pci_host_bridge structure in the host bridge specific structure! Then we can
> access the private data doing container_of().
>
> Something like this:
>
> struct pci_controller {
> struct pci_host_bridge bridge;
> /* private host bridge data here */
> .....
> };
>
> #define PCI_CONTROLLER(bus) ({
> struct pci_host_bridge *hb = to_pci_host_bridge(bus->bridge); \
> container_of(hb, struct pci_controller, bridge); })
>
>
> Then we can retrieve the host bridge structure from everywhere we have a device.
Hi Liviu, it looks good to me, because this change will involve lots platforms,
I would think more about it. Thanks for your suggestion very much! :)
Thanks!
Yijing.
>
> Best regards,
> Liviu
>
>>
>>>
>>>> + host = kzalloc(sizeof(*host), GFP_KERNEL);
>>>> + if (!host)
>>>> + return NULL;
>>>
>>> devm_kzalloc maybe?
>>
>> I don't know much detail about devm_kzalloc(), but we have no pci host driver
>> here, and I found no devm_kzalloc() uses in core PCI code before.
>>
>>>
>>>> + if (!resources) {
>>>> + /* Use default IO/MEM/BUS resources*/
>>>> + pci_add_resource(&host->windows, &ioport_resource);
>>>> + pci_add_resource(&host->windows, &iomem_resource);
>>>> + pci_add_resource(&host->windows, &busn_resource);
>>>> + } else {
>>>> + list_for_each_entry_safe(window, n, resources, list)
>>>> + list_move_tail(&window->list, &host->windows);
>>>> + }
>>>
>>> I think we should assume that the correct resources are passed. You
>>> could add a wrapper around this function to convert old platforms
>>> though.
>>
>> OK, I will move these code out of pci_create_host_bridge, and add a wrapper
>> to setup the default resources.
>>
>>>
>>>> +EXPORT_SYMBOL(pci_create_host_bridge);
>>>
>>> EXPORT_SYMBOL_GPL() maybe?
>>
>> OK, will update it.
>>
>>>
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index 8b11b38..daa7f40 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window {
>>>> struct pci_host_bridge {
>>>> struct device dev;
>>>> struct pci_bus *bus; /* root bus */
>>>> + struct list_head list;
>>>> struct list_head windows; /* pci_host_bridge_windows */
>>>> + int busnum;
>>>
>>> The busnum should already be implied through the bus resource.
>>
>> Yes, I will consider remove it and introduce a helper function to get the root bus number, thanks!
>>
>> Thanks!
>> Yijing.
>>
>>>
>>> Arnd
>>>
>>> .
>>>
>>
>>
>> --
>> Thanks!
>> Yijing
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH] cpuidle/powernv: Re-enable fastsleep at boot time
From: Joel Stanley @ 2014-11-19 2:34 UTC (permalink / raw)
To: Preeti U Murthy; +Cc: Michael Neuling, linuxppc-dev, linux-kernel, rjw
In-Reply-To: <20141118065603.22544.88816.stgit@preeti.in.ibm.com>
Hey Preeti,
On Tue, Nov 18, 2014 at 5:26 PM, Preeti U Murthy
<preeti@linux.vnet.ibm.com> wrote:
> Commit dcb18694 "Fix ipi on palmeto" disabled fastsleep at boot time.
I couldn't find this commit in any tree; upstream, mpe's next, nor powerkvm.
I remember testing this as a workaround for the palmetto, but I don't
recall sending it upstream.
Cheers,
Joel
^ permalink raw reply
* Re: [PATCH V9 03/18] PCI: Add weak pcibios_iov_resource_size() interface
From: Wei Yang @ 2014-11-19 3:21 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Wei Yang, Myron Stowe, linux-pci, gwshan, Donald Dutile,
Bjorn Helgaas, linuxppc-dev
In-Reply-To: <1416363332.5704.18.camel@au1.ibm.com>
On Wed, Nov 19, 2014 at 01:15:32PM +1100, Benjamin Herrenschmidt wrote:
>On Tue, 2014-11-18 at 18:12 -0700, Bjorn Helgaas wrote:
>>
>> Can you help me understand this?
>>
>> We have previously called sriov_init() on the PF. There, we sized the VF
>> BARs, which are in the PF's SR-IOV Capability (SR-IOV spec sec 3.3.14).
>> The size we discover is the amount of space required by a single VF, so
>> sriov_init() adjusts PF->resource[PCI_IOV_RESOURCES + i] by multiplying
>> that size by PCI_SRIOV_TOTAL_VF, so this PF resource is now big enough to
>> hold the VF BAR[i] areas for all the possible VFs.
>
>So I'll let Richard (Wei) answer on the details but I'll just chime in
>about the "big picture". This isn't about changing the spacing between VFs
>which is handled by the system page size.
>
>This is about the way we create MMIO windows from the CPU to the VF BARs.
>
>Basically, we have a (limited) set of 64-bit windows we can create that
>are divided in equal sized segments (256 of them), each segment assigned
>in HW to one of our Partitionable Endpoints (aka domain).
>
>So even if we only ever create 16 VFs for a device, we need to use an
>entire of these windows, which will use 256*VF_size and thus allocate
>that much space. Also the window has to be naturally aligned.
>
>We can then assign the VF BAR to a spot inside that window that corresponds
>to the range of PEs that we have assigned to that device (which typically
>isn't going to be the beginning of the window).
>
Bjorn & Ben,
Let me try to explain it. Thanks for Ben's explanation, it would be helpful. We
are not trying to change the space between VFs.
As mentioned by Ben, we use some HW to map the MMIO space to PE. But the HW
must map 256 segments with the same size. This will lead a situation like
this.
+------+------+ +------+------+------+------+
|VF#0 |VF#1 | ... | |VF#N-1|PF#A |PF#B |
+------+------+ +------+------+------+------+
Suppose N = 254 and the HW map these 256 segments to their corresponding PE#.
Then it introduces one problem, the PF#A and PF#B have been already assigned
to some PE#. We can't map one MMIO range to two different PE#.
What we have done is to "Expand the IOV BAR" to fit the whole HW 256 segments.
By doing so, the MMIO range will look like this.
+------+------+ +------+------+------+------+------+------+
|VF#0 |VF#1 | ... | |VF#N-1|blank |blank |PF#A |PF#B |
+------+------+ +------+------+------+------+------+------+
We do some tricky to "Expand" the IOV BAR, so that make sure there would not
be some overlap between VF's PE and PF's PE.
Then this will leads to the IOV BAR size change from:
IOV BAR size = (VF BAR aperture size) * VF_number
to:
IOV BAR size = (VF BAR aperture size) * 256
This is the reason we need a platform dependent method to get the VF BAR size.
Otherwise the VF BAR size would be not correct.
Now let's take a look at your example again.
PF SR-IOV Capability
TotalVFs = 4
NumVFs = 4
System Page Size = 4KB
VF BAR0 = [mem 0x00000000-0x00000fff] (4KB at address 0)
PF pci_dev->resource[7] = [mem 0x00000000-0x00003fff] (16KB)
VF1 pci_dev->resource[0] = [mem 0x00000000-0x00000fff]
VF2 pci_dev->resource[0] = [mem 0x00001000-0x00001fff]
VF3 pci_dev->resource[0] = [mem 0x00002000-0x00002fff]
VF4 pci_dev->resource[0] = [mem 0x00003000-0x00003fff]
The difference after our expanding is the IOV BAR size is 256*4KB instead of
16KB. So it will look like this:
PF pci_dev->resource[7] = [mem 0x00000000-0x000fffff] (1024KB)
VF1 pci_dev->resource[0] = [mem 0x00000000-0x00000fff]
VF2 pci_dev->resource[0] = [mem 0x00001000-0x00001fff]
VF3 pci_dev->resource[0] = [mem 0x00002000-0x00002fff]
VF4 pci_dev->resource[0] = [mem 0x00003000-0x00003fff]
...
and 252 4KB space leave not used.
So the start address and the size of VF will not change, but the PF's IOV BAR
will be expanded.
>Cheers,
>Ben.
>
--
Richard Yang
Help you, Help me
^ permalink raw reply
* Re: [PATCH V9 03/18] PCI: Add weak pcibios_iov_resource_size() interface
From: Bjorn Helgaas @ 2014-11-19 4:26 UTC (permalink / raw)
To: Wei Yang
Cc: Benjamin Herrenschmidt, linux-pci, linuxppc-dev, gwshan,
Donald Dutile, Myron Stowe
In-Reply-To: <20141119032100.GA7105@richard>
On Wed, Nov 19, 2014 at 11:21:00AM +0800, Wei Yang wrote:
> On Wed, Nov 19, 2014 at 01:15:32PM +1100, Benjamin Herrenschmidt wrote:
> >On Tue, 2014-11-18 at 18:12 -0700, Bjorn Helgaas wrote:
> >>
> >> Can you help me understand this?
> >>
> >> We have previously called sriov_init() on the PF. There, we sized the VF
> >> BARs, which are in the PF's SR-IOV Capability (SR-IOV spec sec 3.3.14).
> >> The size we discover is the amount of space required by a single VF, so
> >> sriov_init() adjusts PF->resource[PCI_IOV_RESOURCES + i] by multiplying
> >> that size by PCI_SRIOV_TOTAL_VF, so this PF resource is now big enough to
> >> hold the VF BAR[i] areas for all the possible VFs.
> >
> >So I'll let Richard (Wei) answer on the details but I'll just chime in
> >about the "big picture". This isn't about changing the spacing between VFs
> >which is handled by the system page size.
> >
> >This is about the way we create MMIO windows from the CPU to the VF BARs.
> >
> >Basically, we have a (limited) set of 64-bit windows we can create that
> >are divided in equal sized segments (256 of them), each segment assigned
> >in HW to one of our Partitionable Endpoints (aka domain).
> >
> >So even if we only ever create 16 VFs for a device, we need to use an
> >entire of these windows, which will use 256*VF_size and thus allocate
> >that much space. Also the window has to be naturally aligned.
> >
> >We can then assign the VF BAR to a spot inside that window that corresponds
> >to the range of PEs that we have assigned to that device (which typically
> >isn't going to be the beginning of the window).
> >
>
> Bjorn & Ben,
>
> Let me try to explain it. Thanks for Ben's explanation, it would be helpful. We
> are not trying to change the space between VFs.
>
> As mentioned by Ben, we use some HW to map the MMIO space to PE.
We need some documentation with pictures about what a PE is. I did find
this:
https://events.linuxfoundation.org/images/stories/slides/lfcs2013_yang.pdf
which looks like a good start, although there's not quite enough text for
me to understand, and it doesn't have much about MMIO space.
> But the HW
> must map 256 segments with the same size. This will lead a situation like
> this.
>
> +------+------+ +------+------+------+------+
> |VF#0 |VF#1 | ... | |VF#N-1|PF#A |PF#B |
> +------+------+ +------+------+------+------+
>
> Suppose N = 254 and the HW map these 256 segments to their corresponding PE#.
I guess these 256 segments are regions of CPU physical address space, and
they are being mapped to bus address space? Is there some relationship
between a PE and part of the bus address space?
> Then it introduces one problem, the PF#A and PF#B have been already assigned
> to some PE#. We can't map one MMIO range to two different PE#.
>
> What we have done is to "Expand the IOV BAR" to fit the whole HW 256 segments.
> By doing so, the MMIO range will look like this.
>
> +------+------+ +------+------+------+------+------+------+
> |VF#0 |VF#1 | ... | |VF#N-1|blank |blank |PF#A |PF#B |
> +------+------+ +------+------+------+------+------+------+
>
> We do some tricky to "Expand" the IOV BAR, so that make sure there would not
> be some overlap between VF's PE and PF's PE.
The language here is tricky. You're not actually *expanding* the IOV BAR.
The IOV BAR is a hardware thing and its size is determined by normal BAR
sizing and the number of VFs. What you're doing is reserving additional
space for that BAR, and the additional space will be unused. That's all
fine; we just need a way to describe it accurately.
> Then this will leads to the IOV BAR size change from:
>
> IOV BAR size = (VF BAR aperture size) * VF_number
>
> to:
>
> IOV BAR size = (VF BAR aperture size) * 256
>
> This is the reason we need a platform dependent method to get the VF BAR size.
> Otherwise the VF BAR size would be not correct.
>
> Now let's take a look at your example again.
>
> PF SR-IOV Capability
> TotalVFs = 4
> NumVFs = 4
> System Page Size = 4KB
> VF BAR0 = [mem 0x00000000-0x00000fff] (4KB at address 0)
>
> PF pci_dev->resource[7] = [mem 0x00000000-0x00003fff] (16KB)
> VF1 pci_dev->resource[0] = [mem 0x00000000-0x00000fff]
> VF2 pci_dev->resource[0] = [mem 0x00001000-0x00001fff]
> VF3 pci_dev->resource[0] = [mem 0x00002000-0x00002fff]
> VF4 pci_dev->resource[0] = [mem 0x00003000-0x00003fff]
>
> The difference after our expanding is the IOV BAR size is 256*4KB instead of
> 16KB. So it will look like this:
>
> PF pci_dev->resource[7] = [mem 0x00000000-0x000fffff] (1024KB)
Is the idea that you want this resource to be big enough to cover all 256
segments? I think I'm OK with increasing the size of the PF resources to
prevent overlap. That part shouldn't be too ugly.
> VF1 pci_dev->resource[0] = [mem 0x00000000-0x00000fff]
> VF2 pci_dev->resource[0] = [mem 0x00001000-0x00001fff]
> VF3 pci_dev->resource[0] = [mem 0x00002000-0x00002fff]
> VF4 pci_dev->resource[0] = [mem 0x00003000-0x00003fff]
> ...
> and 252 4KB space leave not used.
>
> So the start address and the size of VF will not change, but the PF's IOV BAR
> will be expanded.
I'm really dubious about this change to use pci_iov_resource_size(). I
think you might be doing that because if you increase the PF resource size,
dividing that increased size by total_VFs will give you garbage. E.g., in
the example above, you would compute "size = 1024KB / 4", which would make
the VF BARs appear to be 256KB instead of 4KB as they should be.
I think it would be better to solve that problem by decoupling the PF
resource size and the VF BAR size. For example, we could keep track of the
VF BAR size explicitly in struct pci_sriov, instead of computing it from
the PF resource size and total_VFs. This would keep the VF BAR size
completely platform-independent.
Bjorn
^ permalink raw reply
* [git pull] Please pull mpe.git for-linus branch (for powerpc)
From: Michael Ellerman @ 2014-11-19 5:21 UTC (permalink / raw)
To: Linus Torvalds; +Cc: scottwood, haokexin, linuxppc-dev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 904 bytes --]
Hi Linus,
Please pull one fix from Scott, he says:
This patch fixes a crash (introduced in v3.18-rc1) in the FSL MSI driver
when threaded IRQs are enabled.
The following changes since commit 8a97577a5967c1234ccc3bc1b45e4b1a58b39ea8:
Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mpe/linux (2014-11-04 11:18:29 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mpe/linux.git for-linus
for you to fetch changes up to d7ce4377494adfaf8afb15ecf4f07d399bbf13d9:
powerpc/fsl_msi: mark the msi cascade handler IRQF_NO_THREAD (2014-11-17 22:00:30 -0600)
----------------------------------------------------------------
Kevin Hao (1):
powerpc/fsl_msi: mark the msi cascade handler IRQF_NO_THREAD
arch/powerpc/sysdev/fsl_msi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc: Remove more traces of bootmem
From: Michael Ellerman @ 2014-11-19 5:29 UTC (permalink / raw)
To: David Laight; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1C9F2964@AcuExch.aculab.com>
On Tue, 2014-11-18 at 10:26 +0000, David Laight wrote:
> From: Michael Ellerman
> > Although we are now selecting NO_BOOTMEM, we still have some traces of
> > bootmem lying around. That is because even with NO_BOOTMEM there is
> > still a shim that converts bootmem calls into memblock calls, but
> > ultimately we want to remove all traces of bootmem.
> >
> > Most of the patch is conversions from alloc_bootmem() to
> > memblock_alloc(). In general a call such as:
> >
> > p = (struct foo *)alloc_bootmem(x);
> >
> > Becomes:
> >
> > p = __va(memblock_alloc(x, 0));
> >
> > We need __va() because memblock returns a physical address. We don't
> > need the cast because __va() returns a void *. The alignment value of
> > zero tells memblock to use the default alignment, which is
> > SMP_CACHE_BYTES, the same value alloc_bootmem() uses.
>
> It doesn't seem right to me to replicate __va(memblock_alloc(x, 0))
> that many times. I can imagine that the required code will change
> again at to future time, and then all the same places would need changing.
Yeah it's a bit ugly. Actually most of that code has never changed, which is
the problem, no one has bothered to update it.
> Wouldn't it be better to use:
> #define alloc_bootmem(x) __va(memblock_alloc(x, 0))
> possibly with a rename, or as a static inline.
Well that's essentially what the existing shim does. So we can't use that name.
My plan is to add a memblock_alloc_virt() which does the __va() for you. But I
didn't want the powerpc changes to get backed up behind that.
> If __va() is non-trivial you want a real function.
It can't be a function because we use it on void * as well as unsigned long,
phys_addr_t etc.
phys_to_virt() is the nicer version, though it only takes unsigned long.
cheers
^ permalink raw reply
* Re: [PATCH] cpuidle/powernv: Re-enable fastsleep at boot time
From: Preeti U Murthy @ 2014-11-19 7:08 UTC (permalink / raw)
To: Joel Stanley; +Cc: Michael Neuling, linuxppc-dev, linux-kernel, rjw
In-Reply-To: <CACPK8Xc7G6JgqPYUmGTx8O=a6jxMB0yf1weEraMBu8dcAJibuQ@mail.gmail.com>
Hi Joel,
On 11/19/2014 08:04 AM, Joel Stanley wrote:
> Hey Preeti,
>
> On Tue, Nov 18, 2014 at 5:26 PM, Preeti U Murthy
> <preeti@linux.vnet.ibm.com> wrote:
>> Commit dcb18694 "Fix ipi on palmeto" disabled fastsleep at boot time.
>
> I couldn't find this commit in any tree; upstream, mpe's next, nor powerkvm.
Oh yes you are right. I must have been looking at the wrong git branch.
I verified upstream now. The commit dcb18694 is not present.So we are
good. Thanks for pointing this out :)
Sorry for the noise!
>
> I remember testing this as a workaround for the palmetto, but I don't
> recall sending it upstream.
>
> Cheers,
>
> Joel
>
Regards
Preeti U Murthy
^ permalink raw reply
* Re: [PATCH] powerpc: Remove more traces of bootmem
From: Michael Ellerman @ 2014-11-19 7:57 UTC (permalink / raw)
To: David Laight; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <1416374957.32483.3.camel@concordia>
On Wed, 2014-11-19 at 16:29 +1100, Michael Ellerman wrote:
> On Tue, 2014-11-18 at 10:26 +0000, David Laight wrote:
> > From: Michael Ellerman
> > > Although we are now selecting NO_BOOTMEM, we still have some traces of
> > > bootmem lying around. That is because even with NO_BOOTMEM there is
> > > still a shim that converts bootmem calls into memblock calls, but
> > > ultimately we want to remove all traces of bootmem.
> > >
> > > Most of the patch is conversions from alloc_bootmem() to
> > > memblock_alloc(). In general a call such as:
> > >
> > > p = (struct foo *)alloc_bootmem(x);
> > >
> > > Becomes:
> > >
> > > p = __va(memblock_alloc(x, 0));
> > >
> > > We need __va() because memblock returns a physical address. We don't
> > > need the cast because __va() returns a void *. The alignment value of
> > > zero tells memblock to use the default alignment, which is
> > > SMP_CACHE_BYTES, the same value alloc_bootmem() uses.
> >
> > It doesn't seem right to me to replicate __va(memblock_alloc(x, 0))
> > that many times. I can imagine that the required code will change
> > again at to future time, and then all the same places would need changing.
After more grovelling through header files it turns out there is a wrapper that
does what we want - I think.
It's memblock_virt_alloc() (name reversed), and it's in bootmem.h, which is why
I didn't find it originally.
I'll try that instead.
cheers
^ permalink raw reply
* Re: [PATCH V9 03/18] PCI: Add weak pcibios_iov_resource_size() interface
From: Wei Yang @ 2014-11-19 9:27 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Wei Yang, Benjamin Herrenschmidt, Myron Stowe, linux-pci, gwshan,
Donald Dutile, linuxppc-dev
In-Reply-To: <20141119042601.GB23467@google.com>
On Tue, Nov 18, 2014 at 09:26:01PM -0700, Bjorn Helgaas wrote:
>On Wed, Nov 19, 2014 at 11:21:00AM +0800, Wei Yang wrote:
>> On Wed, Nov 19, 2014 at 01:15:32PM +1100, Benjamin Herrenschmidt wrote:
>> >On Tue, 2014-11-18 at 18:12 -0700, Bjorn Helgaas wrote:
>> >>
>> >> Can you help me understand this?
>> >>
>> >> We have previously called sriov_init() on the PF. There, we sized the VF
>> >> BARs, which are in the PF's SR-IOV Capability (SR-IOV spec sec 3.3.14).
>> >> The size we discover is the amount of space required by a single VF, so
>> >> sriov_init() adjusts PF->resource[PCI_IOV_RESOURCES + i] by multiplying
>> >> that size by PCI_SRIOV_TOTAL_VF, so this PF resource is now big enough to
>> >> hold the VF BAR[i] areas for all the possible VFs.
>> >
>> >So I'll let Richard (Wei) answer on the details but I'll just chime in
>> >about the "big picture". This isn't about changing the spacing between VFs
>> >which is handled by the system page size.
>> >
>> >This is about the way we create MMIO windows from the CPU to the VF BARs.
>> >
>> >Basically, we have a (limited) set of 64-bit windows we can create that
>> >are divided in equal sized segments (256 of them), each segment assigned
>> >in HW to one of our Partitionable Endpoints (aka domain).
>> >
>> >So even if we only ever create 16 VFs for a device, we need to use an
>> >entire of these windows, which will use 256*VF_size and thus allocate
>> >that much space. Also the window has to be naturally aligned.
>> >
>> >We can then assign the VF BAR to a spot inside that window that corresponds
>> >to the range of PEs that we have assigned to that device (which typically
>> >isn't going to be the beginning of the window).
>> >
>>
>> Bjorn & Ben,
>>
>> Let me try to explain it. Thanks for Ben's explanation, it would be helpful. We
>> are not trying to change the space between VFs.
>>
>> As mentioned by Ben, we use some HW to map the MMIO space to PE.
>
>We need some documentation with pictures about what a PE is. I did find
>this:
>
>https://events.linuxfoundation.org/images/stories/slides/lfcs2013_yang.pdf
>
>which looks like a good start, although there's not quite enough text for
>me to understand, and it doesn't have much about MMIO space.
Yes, this slide is used 2 years ago and for P7 platform.
Current solution is for P8, which we choose some different mechanism.
Especially the MMIO manipulation used in current implementation is not used in
that moment.
>> But the HW
>> must map 256 segments with the same size. This will lead a situation like
>> this.
>>
>> +------+------+ +------+------+------+------+
>> |VF#0 |VF#1 | ... | |VF#N-1|PF#A |PF#B |
>> +------+------+ +------+------+------+------+
>>
>> Suppose N = 254 and the HW map these 256 segments to their corresponding PE#.
>
>I guess these 256 segments are regions of CPU physical address space, and
>they are being mapped to bus address space? Is there some relationship
>between a PE and part of the bus address space?
>
PE is an entity for EEH, which may include a whole bus or one pci device.
When some device got some error, we need to identify which PE it belongs to.
So we have some HW to map between PE# and MMIO/DMA/MSI address.
The HW mentioned in previous letter is the one to map MMIO address to a PE#.
While this HW must map a range with 256 equal segments. And yes, this is
mapped to bus address space.
>> Then it introduces one problem, the PF#A and PF#B have been already assigned
>> to some PE#. We can't map one MMIO range to two different PE#.
>>
>> What we have done is to "Expand the IOV BAR" to fit the whole HW 256 segments.
>> By doing so, the MMIO range will look like this.
>>
>> +------+------+ +------+------+------+------+------+------+
>> |VF#0 |VF#1 | ... | |VF#N-1|blank |blank |PF#A |PF#B |
>> +------+------+ +------+------+------+------+------+------+
>>
>> We do some tricky to "Expand" the IOV BAR, so that make sure there would not
>> be some overlap between VF's PE and PF's PE.
>
>The language here is tricky. You're not actually *expanding* the IOV BAR.
>The IOV BAR is a hardware thing and its size is determined by normal BAR
>sizing and the number of VFs. What you're doing is reserving additional
>space for that BAR, and the additional space will be unused. That's all
>fine; we just need a way to describe it accurately.
>
Yes, you are right. My word is not exact.
What I am doing is to reserve more space for IOV BAR. I will make the log more
precise in next version.
>> Then this will leads to the IOV BAR size change from:
>>
>> IOV BAR size = (VF BAR aperture size) * VF_number
>>
>> to:
>>
>> IOV BAR size = (VF BAR aperture size) * 256
>>
>> This is the reason we need a platform dependent method to get the VF BAR size.
>> Otherwise the VF BAR size would be not correct.
>>
>> Now let's take a look at your example again.
>>
>> PF SR-IOV Capability
>> TotalVFs = 4
>> NumVFs = 4
>> System Page Size = 4KB
>> VF BAR0 = [mem 0x00000000-0x00000fff] (4KB at address 0)
>>
>> PF pci_dev->resource[7] = [mem 0x00000000-0x00003fff] (16KB)
>> VF1 pci_dev->resource[0] = [mem 0x00000000-0x00000fff]
>> VF2 pci_dev->resource[0] = [mem 0x00001000-0x00001fff]
>> VF3 pci_dev->resource[0] = [mem 0x00002000-0x00002fff]
>> VF4 pci_dev->resource[0] = [mem 0x00003000-0x00003fff]
>>
>> The difference after our expanding is the IOV BAR size is 256*4KB instead of
>> 16KB. So it will look like this:
>>
>> PF pci_dev->resource[7] = [mem 0x00000000-0x000fffff] (1024KB)
>
>Is the idea that you want this resource to be big enough to cover all 256
>segments? I think I'm OK with increasing the size of the PF resources to
>prevent overlap. That part shouldn't be too ugly.
>
Yes, big enough to cover all 256 segments.
Sorry for making it ugly :-(
>> VF1 pci_dev->resource[0] = [mem 0x00000000-0x00000fff]
>> VF2 pci_dev->resource[0] = [mem 0x00001000-0x00001fff]
>> VF3 pci_dev->resource[0] = [mem 0x00002000-0x00002fff]
>> VF4 pci_dev->resource[0] = [mem 0x00003000-0x00003fff]
>> ...
>> and 252 4KB space leave not used.
>>
>> So the start address and the size of VF will not change, but the PF's IOV BAR
>> will be expanded.
>
>I'm really dubious about this change to use pci_iov_resource_size(). I
>think you might be doing that because if you increase the PF resource size,
>dividing that increased size by total_VFs will give you garbage. E.g., in
>the example above, you would compute "size = 1024KB / 4", which would make
>the VF BARs appear to be 256KB instead of 4KB as they should be.
>
Yes, your understanding is correct.
>I think it would be better to solve that problem by decoupling the PF
>resource size and the VF BAR size. For example, we could keep track of the
>VF BAR size explicitly in struct pci_sriov, instead of computing it from
>the PF resource size and total_VFs. This would keep the VF BAR size
>completely platform-independent.
>
Hmm... this is another solution.
If you prefer this one, I will make a change accordingly.
Thanks for your comments :-)
>Bjorn
--
Richard Yang
Help you, Help me
^ permalink raw reply
* Fan management PowerMac7,2
From: Martin Kukač @ 2014-11-19 12:04 UTC (permalink / raw)
To: linuxppc-dev
Hello,
I was directed to this list on #ppc64/freenode IRC channel. I have
PowerMac7,2 (AGP, 2xPowerPC 970(2.2) 2GHz) and I wanted to try kernel
supporting kvm-pr module, which is not in 3.2.63 kernel provided with
Debian Wheezy. I downloaded 3.14.24, compiled it using original config
from 3.2.63 and after booting fan management does not work. I tried
therm_pm72 (which is deprecated, but works well with Debian kernel) and
windfarm_pm72 modules, but neither worked - after about five minutes
from boot all fans go to maximum and stay this way no matter the CPU
load or frequency.
I downloaded vanilla 3.2.64, compiled it the same way and there fans
work with therm_p72 well. This kernel does not have kvm-pr, so I
reverted to original kernel provided with Wheezy.
I looked to logs, i tried to search the web whether anyone had already
the same problem, but it seems that nobody else described not to say
solved this. Can anyone please point me to anything that would help? I
can provide whatever log files required.
Thanks,
Martin Kukac
^ permalink raw reply
* Re: [RFC PATCH 07/16] PCI: Separate pci_host_bridge creation out of pci_create_root_bus()
From: Liviu Dudau @ 2014-11-19 16:29 UTC (permalink / raw)
To: Yijing Wang
Cc: Tony Luck, Russell King, Arnd Bergmann, linux-pci@vger.kernel.org,
x86@kernel.org, Liviu Dudau, linux-kernel@vger.kernel.org,
huxinwei@huawei.com, Thierry Reding,
suravee.suthikulpanit@amd.com, Bjorn Helgaas,
linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <546BFF74.4030101@huawei.com>
On Wed, Nov 19, 2014 at 10:24:52AM +0800, Yijing Wang wrote:
> >> We need, some platforms pass NULL pointer as host bridge parent.
> >
> > Yijing,
> >
> > May I suggest a different approach here? Rather than having to pass an opaque
> > pointer that gets converted by the host bridge driver back to the private
> > structure, what about promoting a new style of usage, that is similar to the
> > way device drivers work? Lets try to promote the embedding of the generic
> > pci_host_bridge structure in the host bridge specific structure! Then we can
> > access the private data doing container_of().
> >
> > Something like this:
> >
> > struct pci_controller {
> > struct pci_host_bridge bridge;
> > /* private host bridge data here */
> > .....
> > };
> >
> > #define PCI_CONTROLLER(bus) ({
> > struct pci_host_bridge *hb = to_pci_host_bridge(bus->bridge); \
> > container_of(hb, struct pci_controller, bridge); })
> >
> >
> > Then we can retrieve the host bridge structure from everywhere we have a device.
>
> Hi Liviu, it looks good to me, because this change will involve lots platforms,
> I would think more about it. Thanks for your suggestion very much! :)
Given that I also look at this area maybe we should join forces and divide the problem?
Best regards,
Liviu
>
>
> Thanks!
> Yijing.
>
> >
> > Best regards,
> > Liviu
> >
> >>
> >>>
> >>>> + host = kzalloc(sizeof(*host), GFP_KERNEL);
> >>>> + if (!host)
> >>>> + return NULL;
> >>>
> >>> devm_kzalloc maybe?
> >>
> >> I don't know much detail about devm_kzalloc(), but we have no pci host driver
> >> here, and I found no devm_kzalloc() uses in core PCI code before.
> >>
> >>>
> >>>> + if (!resources) {
> >>>> + /* Use default IO/MEM/BUS resources*/
> >>>> + pci_add_resource(&host->windows, &ioport_resource);
> >>>> + pci_add_resource(&host->windows, &iomem_resource);
> >>>> + pci_add_resource(&host->windows, &busn_resource);
> >>>> + } else {
> >>>> + list_for_each_entry_safe(window, n, resources, list)
> >>>> + list_move_tail(&window->list, &host->windows);
> >>>> + }
> >>>
> >>> I think we should assume that the correct resources are passed. You
> >>> could add a wrapper around this function to convert old platforms
> >>> though.
> >>
> >> OK, I will move these code out of pci_create_host_bridge, and add a wrapper
> >> to setup the default resources.
> >>
> >>>
> >>>> +EXPORT_SYMBOL(pci_create_host_bridge);
> >>>
> >>> EXPORT_SYMBOL_GPL() maybe?
> >>
> >> OK, will update it.
> >>
> >>>
> >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >>>> index 8b11b38..daa7f40 100644
> >>>> --- a/include/linux/pci.h
> >>>> +++ b/include/linux/pci.h
> >>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window {
> >>>> struct pci_host_bridge {
> >>>> struct device dev;
> >>>> struct pci_bus *bus; /* root bus */
> >>>> + struct list_head list;
> >>>> struct list_head windows; /* pci_host_bridge_windows */
> >>>> + int busnum;
> >>>
> >>> The busnum should already be implied through the bus resource.
> >>
> >> Yes, I will consider remove it and introduce a helper function to get the root bus number, thanks!
> >>
> >> Thanks!
> >> Yijing.
> >>
> >>>
> >>> Arnd
> >>>
> >>> .
> >>>
> >>
> >>
> >> --
> >> Thanks!
> >> Yijing
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >
>
>
> --
> Thanks!
> Yijing
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
-------------------
.oooO
( )
\ ( Oooo.
\_) ( )
) /
(_/
One small step
for me ...
^ permalink raw reply
* Re: [RFC PATCH 07/16] PCI: Separate pci_host_bridge creation out of pci_create_root_bus()
From: Liviu Dudau @ 2014-11-19 16:37 UTC (permalink / raw)
To: Yijing Wang
Cc: Liviu Dudau, Tony Luck, Russell King, linux-pci@vger.kernel.org,
x86@kernel.org, linux-kernel@vger.kernel.org, huxinwei@huawei.com,
Thierry Reding, suravee.suthikulpanit@amd.com, Bjorn Helgaas,
linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <546BF59C.8060203@huawei.com>
On Wed, Nov 19, 2014 at 01:42:52AM +0000, Yijing Wang wrote:
> On 2014/11/18 23:30, Liviu Dudau wrote:
> > On Mon, Nov 17, 2014 at 10:21:41AM +0000, Yijing Wang wrote:
> >> There are some common PCI infos like domain, msi_controller, these
> >> infos are saved in arch PCI sysdata, and lots arch specific functions
> >> like pci_domain_nr() and pcibios_msi_controller() required.
> >> We could separate pci_host_bridge creation out of pci_create_root_bus(=
),
> >> then we could put the common infos in, then we could eliminate
> >> the arch specifc functions.
> >>
> >
> > Please Cc: Yinghai Lu and Jiang Liu on future versions.
> >
> > More comments on the conversion of pci_create_root_bus():
>=20
> That's my mistake, will add CC Yinghai and Jiang in next version.
>=20
> >
> >> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> >> ---
> ...
> >> +
> >> +static void pci_release_host_bridge_dev(struct device *dev)
> >> +{
> >> + struct pci_host_bridge *bridge =3D to_pci_host_bridge(dev);
> >> +
> >> + if (bridge->release_fn)
> >> + bridge->release_fn(bridge);
> >> + pci_free_resource_list(&bridge->windows);
> >> + kfree(bridge);
> >> +}
> >> +
> >> +struct pci_host_bridge *pci_create_host_bridge(
> >> + struct device *parent, u32 db,
> >> + struct pci_ops *ops, void *sysdata,
> >
> > I don't thinks it is worth moving the buses' pci_ops into pci_host_brid=
ge. It
> > might be more useful to have pci_host_bridge specific ops here.
>=20
> Because we want to create pci_host_bridge before pci root bus creation,
> so when we scan the root bus and child buses, we use pci_host_bridge as
> the only argument, and another pci_host_info will be addes in later patch=
,
> which one support carry the pci_host_bridge ops.
But pci_create_root_bus() already has a pci_ops argument, I don't see the r=
eason
to drop that.
pci_create_host_bridge() can get pci_host_bridge ops while pci_create_root_=
bus() gets
the bus ops. For find out the MSI controller, the domain number and any oth=
er HB
specific stuff, you use the HB ops. For config R/W acceses you use bus ops.
>=20
> >
> >> + struct list_head *resources)
> >> +{
> >> + int error;
> >> + struct pci_bus *b;
> >> + struct pci_host_bridge *host, *h;
> ...
> >> -struct pci_bus *pci_create_root_bus(struct device *parent, u32 db,
> >> - struct pci_ops *ops, void *sysdata, struct list_head *=
resources)
> >> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *bridge)
> >> {
> >> int error;
> >> - struct pci_host_bridge *bridge;
> >> struct pci_bus *b, *b2;
> >> - struct pci_host_bridge_window *window, *n;
> >> + struct pci_host_bridge_window *window;
> >> struct resource *res;
> >> resource_size_t offset;
> >> char bus_addr[64];
> >> char *fmt;
> >> - u8 bus =3D PCI_BUSNUM(db);
> >> + struct device *parent =3D bridge->dev.parent;
> >>
> >> b =3D pci_alloc_bus(NULL);
> >> if (!b)
> >> return NULL;
> >>
> >> - b->sysdata =3D sysdata;
> >> - b->ops =3D ops;
> >> - b->number =3D b->busn_res.start =3D bus;
> >> + b->sysdata =3D bridge->sysdata;
> >
> > I think bridge should be the b->sysdata here.
>=20
> ? what's the meaning?
Currently, bus->sysdata holds a pointer to the arch/driver host bridge stru=
cture, as passed
in pci_create_root_bus(). If you agree with my idea of wrapping the host br=
idge driver
structure around the pci_host_bridge, then we will always have a way of ret=
rieving that
information; but for backwards compatibility we could set bus->sysdata to b=
e the bridge.
Then existing macros that convert sysdata to pci_controller can be reused a=
fter being
updated.
Best regards,
Liviu
>=20
> >
> >> + b->ops =3D bridge->ops;
> >
> > See comment above why I don't think this is necessary.
> >
> >> + b->number =3D b->busn_res.start =3D bridge->busnum;
> >> pci_bus_assign_domain_nr(b, parent);
> >> - b2 =3D pci_find_bus(pci_domain_nr(b), bus);
> >> + bridge->domain =3D pci_domain_nr(b);
> >
> > Do you really want to overwrite the bridge's domain with the one from a=
bus that
> > could possibly be rejected a couple of lines further down?
> >
> > As an asside: if we are doing the split of pci_host_bridge from root bu=
s creation
> > it is worth in my opinion to move the domain setup in pci_create_host_b=
ridge()
> > and stop fiddling with it here.
>=20
>=20
> Hi Liviu, these lines just temporary, I will remove it after all host dri=
vers
> save its domain in pci_host_bridge.
>=20
>=20
> >
> > Otherwise it looks to me like you are heading in the right direction.
>=20
> Thanks!
> Yijing.
>=20
> >
> > Best regards,
> > Liviu
> >
> >> + b2 =3D pci_find_bus(pci_domain_nr(b), bridge->busnum);
> >> if (b2) {
> >> /* If we already got to this bus through a different b=
ridge, ignore it */
> >> dev_dbg(&b2->dev, "bus already known\n");
> >> goto err_out;
> >> }
> >>
> >> - bridge =3D pci_alloc_host_bridge(b);
> >> - if (!bridge)
> >> - goto err_out;
> >> -
> >> - bridge->dev.parent =3D parent;
> >> - bridge->dev.release =3D pci_release_host_bridge_dev;
> >> - dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), b=
us);
> >> - error =3D pcibios_root_bridge_prepare(bridge);
> >> - if (error) {
> >> - kfree(bridge);
> >> - goto err_out;
> >> - }
> >> -
> >> - error =3D device_register(&bridge->dev);
> >> - if (error) {
> >> - put_device(&bridge->dev);
> >> - goto err_out;
> >> - }
> >> b->bridge =3D get_device(&bridge->dev);
> >> device_enable_async_suspend(b->bridge);
> >> pci_set_bus_of_node(b);
> >> @@ -1950,7 +1899,7 @@ struct pci_bus *pci_create_root_bus(struct devic=
e *parent, u32 db,
> >>
> >> b->dev.class =3D &pcibus_class;
> >> b->dev.parent =3D b->bridge;
> >> - dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
> >> + dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), b->number=
);
> >> error =3D device_register(&b->dev);
> >> if (error)
> >> goto class_dev_reg_err;
> >> @@ -1966,12 +1915,11 @@ struct pci_bus *pci_create_root_bus(struct dev=
ice *parent, u32 db,
> >> printk(KERN_INFO "PCI host bridge to bus %s\n", dev_na=
me(&b->dev));
> >>
> >> /* Add initial resources to the bus */
> >> - list_for_each_entry_safe(window, n, resources, list) {
> >> - list_move_tail(&window->list, &bridge->windows);
> >> + list_for_each_entry(window, &bridge->windows, list) {
> >> res =3D window->res;
> >> offset =3D window->offset;
> >> if (res->flags & IORESOURCE_BUS)
> >> - pci_bus_insert_busn_res(b, bus, res->end);
> >> + pci_bus_insert_busn_res(b, b->number, res->end=
);
> >> else
> >> pci_bus_add_resource(b, res, 0);
> >> if (offset) {
> >> @@ -2001,6 +1949,25 @@ err_out:
> >> return NULL;
> >> }
> >>
> >> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus,
> >> + struct pci_ops *ops, void *sysdata, struct list_head *=
resources)
> >> +{
> >> + struct pci_host_bridge *host;
> >> +
> >> + host =3D pci_create_host_bridge(parent, bus, ops,
> >> + sysdata ,resources);
> >> + if (!host)
> >> + return NULL;
> >> +
> >> + host->bus =3D __pci_create_root_bus(host);
> >> + if (!host->bus) {
> >> + pci_free_host_bridge(host);
> >> + return NULL;
> >> + }
> >> +
> >> + return host->bus;
> >> +}
> >> +
> >> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
> >> {
> >> struct resource *res =3D &b->busn_res;
> >> @@ -2069,40 +2036,37 @@ struct pci_bus *pci_scan_root_bus(struct devic=
e *parent, u32 db,
> >> {
> >> struct pci_host_bridge_window *window;
> >> bool found =3D false;
> >> - struct pci_bus *b;
> >> - LIST_HEAD(default_res);
> >> + struct pci_host_bridge *host;
> >> int max;
> >>
> >> - if (!resources) {
> >> - pci_add_resource(&default_res, &ioport_resource);
> >> - pci_add_resource(&default_res, &iomem_resource);
> >> - pci_add_resource(&default_res, &busn_resource);
> >> - } else {
> >> - list_for_each_entry(window, resources, list)
> >> - if (window->res->flags & IORESOURCE_BUS) {
> >> - found =3D true;
> >> - break;
> >> - }
> >> - }
> >> + host =3D pci_create_host_bridge(parent, db, ops, sysdata, reso=
urces);
> >> + if (!host)
> >> + return NULL;
> >>
> >> - b =3D pci_create_root_bus(parent, db, ops, sysdata,
> >> - resources ? resources : &default_res);
> >> - if (!b)
> >> + list_for_each_entry(window, &host->windows, list)
> >> + if (window->res->flags & IORESOURCE_BUS) {
> >> + found =3D true;
> >> + break;
> >> + }
> >> +
> >> + host->bus =3D __pci_create_root_bus(host);
> >> + if (!host->bus) {
> >> + pci_free_host_bridge(host);
> >> return NULL;
> >> + }
> >>
> >> if (!found) {
> >> - dev_info(&b->dev,
> >> + dev_info(&host->bus->dev,
> >> "No busn resource found for root bus, will use [bus %=
02x-ff]\n",
> >> PCI_BUSNUM(db));
> >> - pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255);
> >> + pci_bus_insert_busn_res(host->bus, PCI_BUSNUM(db), 255=
);
> >> }
> >>
> >> - max =3D pci_scan_child_bus(b);
> >> -
> >> + max =3D pci_scan_child_bus(host->bus);
> >> if (!found)
> >> - pci_bus_update_busn_res_end(b, max);
> >> + pci_bus_update_busn_res_end(host->bus, max);
> >>
> >> - return b;
> >> + return host->bus;
> >> }
> >> EXPORT_SYMBOL(pci_scan_root_bus);
> >>
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index 8b11b38..daa7f40 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -402,7 +402,12 @@ struct pci_host_bridge_window {
> >> struct pci_host_bridge {
> >> struct device dev;
> >> struct pci_bus *bus; /* root bus */
> >> + struct list_head list;
> >> struct list_head windows; /* pci_host_bridge_windows */
> >> + int busnum;
> >> + int domain;
> >> + void *sysdata;
> >> + struct pci_ops *ops;
> >> void (*release_fn)(struct pci_host_bridge *);
> >> void *release_data;
> >> };
> >> @@ -413,7 +418,9 @@ void pci_set_host_bridge_release(struct pci_host_b=
ridge *bridge,
> >> void *release_data);
> >>
> >> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
> >> -
> >> +struct pci_host_bridge *pci_create_host_bridge(
> >> + struct device *parent, u32 db, struct pci_ops *ops,
> >> + void *sys, struct list_head *resources);
> >> /*
> >> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that co=
rrespond
> >> * to P2P or CardBus bridge windows) go in a table. Additional ones =
(for
> >> @@ -770,6 +777,8 @@ void pci_bus_add_devices(const struct pci_bus *bus=
);
> >> struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus,
> >> struct pci_ops *ops, void *sysdata=
,
> >> struct list_head *resources);
> >> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *host);
> >> +void pci_free_host_bridge(struct pci_host_bridge *host);
> >> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
> >> int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
> >> void pci_bus_release_busn_res(struct pci_bus *b);
> >> --
> >> 1.7.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-pci" i=
n
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >
>=20
>=20
> --
> Thanks!
> Yijing
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>=20
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF
^ permalink raw reply
* Re: [RFC PATCH 08/16] PCI: Introduce pci_scan_host_bridge() and pci_host_info
From: Liviu Dudau @ 2014-11-19 16:41 UTC (permalink / raw)
To: Yijing Wang
Cc: Liviu Dudau, Tony Luck, Russell King, linux-pci@vger.kernel.org,
x86@kernel.org, linux-kernel@vger.kernel.org, huxinwei@huawei.com,
Thierry Reding, Yijing Wang, suravee.suthikulpanit@amd.com,
Bjorn Helgaas, linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <546BFBC8.4030104@huawei.com>
On Wed, Nov 19, 2014 at 02:09:12AM +0000, Yijing Wang wrote:
> >> struct pci_host_bridge *pci_create_host_bridge(
> >> -=09=09struct device *parent, u32 db,=20
> >> -=09=09struct pci_ops *ops, void *sysdata,=20
> >> -=09=09struct list_head *resources)
> >> +=09=09struct device *parent, u32 db, struct pci_ops *ops,=20
> >> +=09=09struct pci_host_info *info)
> >> {
> >> =09int error;
> >> =09struct pci_bus *b;
> >> =09struct pci_host_bridge *host, *h;
> >> -=09struct pci_host_bridge_window *window, *n;
> >> =20
> >> =09down_read(&pci_host_bridge_sem);
> >> =09list_for_each_entry(h, &pci_host_bridge_list, list) {
> >> @@ -53,7 +51,7 @@ struct pci_host_bridge *pci_create_host_bridge(
> >> =09if (!host)
> >> =09=09return NULL;
> >> =20
> >> -=09host->sysdata =3D sysdata;
> >> +=09host->sysdata =3D info->arg;
> >> =09host->busnum =3D PCI_BUSNUM(db);
> >> =09host->domain =3D PCI_DOMAIN(db);
> >> =09host->ops =3D ops;
> >> @@ -63,18 +61,23 @@ struct pci_host_bridge *pci_create_host_bridge(
> >> =20
> >> =09/* this is hack, just for build, will be removed later*/
> >=20
> > Why do you need this hack? Just for calling pci_domain_nr() ?
>=20
> Yes, it's temporary code, we need domain number here for pci host bridge =
register.
>=20
> >=20
> >> =09b =3D kzalloc(sizeof(*b), GFP_KERNEL);
> >> -=09b->sysdata =3D sysdata;
> >> +=09b->sysdata =3D host->sysdata;
> >> =09pci_bus_assign_domain_nr(b, parent);
> >> =09host->domain =3D pci_domain_nr(b);
> >> +=09kfree(b);
> >> =20
> ...
> >> +static int pci_default_init_res(struct pci_host_bridge *host,
> >> +=09=09struct pci_host_info *info)
> >> +{
> >> +=09struct pci_host_bridge_window *window, *n;
> >> +
> >> +=09if (info->res_type !=3D PCI_HOST_RES_DEFAULT)=20
> >> +=09=09list_for_each_entry_safe(window, n, info->resources,
> >> +=09=09=09=09list)
> >> +=09=09=09list_move_tail(&window->list, &host->windows);
> >> +=09else
> >> +=09=09info->res_type =3D PCI_HOST_RES_DEFAULT;
> >=20
> > I'm confused about this assignment. Isn't this a nop as the else part
> > means info->res_type *is* PCI_HOST_RES_DEFAULT?
>=20
> No, in this patch, host drivers pass a pci host bridge resources init hoo=
k
> in pci_host_info *info, and we call this info->init_res() in pci_create_h=
ost_bridge().
>=20
> +struct pci_host_info {
> +=09u8 res_type;
> +=09void *arg;
> +=09struct list_head *resources; /*just for build, will clean up later */
> +=09int (*init_res)(struct pci_host_bridge *host,
> +=09=09=09struct pci_host_info *info);
> +};
> +
That's not what I've asked! Your code does:
=09if (info->res_type !=3D PCI_HOST_RES_DEFAULT)
=09=09....
=09else /* info->res_type =3D=3D PCI_HOST_RES_DEFAULT)
=09=09info->res_type =3D PCI_HOST_RES_DEFAULT;
info->res_type is already =3D=3D PCI_HOST_RES_DEFAULT in the else side, ass=
ignment is a NOP?
>=20
> >=20
> >> +
> >> +=09return 0;
> >> +}
> ...
> >> @@ -2038,8 +2057,13 @@ struct pci_bus *pci_scan_root_bus(struct device=
*parent, u32 db,
> >> =09bool found =3D false;
> >> =09struct pci_host_bridge *host;
> >> =09int max;
> >> +=09struct pci_host_info info;
> >> +=09
> >> +=09info.arg =3D sysdata;
> >> +=09info.resources =3D resources;
> >> +=09info.init_res =3D pci_default_init_res;
> >=20
> > I have mixed feelings about this patch. While it is heading in the righ=
t direction
> > of moving pci_host_bridge relevant information towards the right user, =
I don't think
> > you picked up the right set to move. The resource list is going to be c=
opied into
> > internal pci_host_bridge list anyway, keeping another copy is not helpf=
ul *and*
> > you have increased the code size.
> >=20
> > I think for now we should aim to get the *missing* data into pci_host_b=
ridge: MSI
> > controllers and PCI domain/segment. Then we can do more cleanup.
>=20
> Hi Liviu, I agree with you here, the changes to resources stuff seems not=
a perfect
> solution. In my patch 6, we could pass pci domain nr by u32 PCI_DOMBUS(do=
main, bus) argument,
> and store it in pci_host_bridge. For msi controller, we couldn't save the=
msi_controller
> in pci_host_bridge. Before we assume one pci host bridge only had one msi=
_controller,
> but now something changes, Jiang introduce hierarchy irq domain in x86, a=
nd now
> one pci host bridge may has more than one msi_controller. So I prefer to =
add a
> function to pci_host_bridge something like
>=20
> struct msi_controller *pci_get_msi_controller(struct pci_dev *dev)
Yes, good idea.
>=20
> >=20
> >> =20
> >> -=09host =3D pci_create_host_bridge(parent, db, ops, sysdata, resource=
s);
> >> +=09host =3D pci_create_host_bridge(parent, db, ops, &info);
> >> =09if (!host)
> >> =09=09return NULL;
> >> =20
> >> @@ -2070,6 +2094,47 @@ struct pci_bus *pci_scan_root_bus(struct device=
*parent, u32 db,
> >> }
> >> EXPORT_SYMBOL(pci_scan_root_bus);
> >> =20
> >> +struct pci_host_bridge *pci_scan_host_bridge(
> >> +=09=09struct device *parent, u32 db, struct pci_ops *ops,
> >> +=09=09struct pci_host_info *info)
> >> +{
> >> +=09struct pci_host_bridge_window *window;
> >> +=09bool found =3D false;
> >> +=09struct pci_host_bridge *host;
> >> +=09int max;
> >> +
> >> +=09host =3D pci_create_host_bridge(parent, db, ops, info);
> >> +=09if (!host)
> >> +=09=09return NULL;
> >> +
> >> +=09list_for_each_entry(window, &host->windows, list)
> >> +=09=09if (window->res->flags & IORESOURCE_BUS) {
> >> +=09=09=09found =3D true;
> >> +=09=09=09break;
> >> +=09=09}
> >> +
> >> +=09host->bus =3D __pci_create_root_bus(host);
> >> +=09if (!host->bus) {
> >> +=09=09pci_free_host_bridge(host);
> >> +=09=09return NULL;
> >> +=09}
> >> +
> >> +=09if (!found) {
> >> +=09=09dev_info(&host->bus->dev,
> >> +=09=09 "No busn resource found for root bus, will use [bus %02x-ff]\n=
",
> >> +=09=09=09host->busnum);
> >> +=09=09pci_bus_insert_busn_res(host->bus, host->busnum, 255);
> >> +=09}
> >> +
> >> +=09max =3D pci_scan_child_bus(host->bus);
> >> +=09if (!found)
> >> +=09=09pci_bus_update_busn_res_end(host->bus, max);
> >> +
> >> +=09return host;
> >> +
> >> +}
> >> +EXPORT_SYMBOL(pci_scan_host_bridge);
> >> +
> >> /**
> >> * pci_rescan_bus_bridge_resize - scan a PCI bus for devices.
> >> * @bridge: PCI bridge for the bus to scan
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index daa7f40..a51f5f5 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -412,6 +412,21 @@ struct pci_host_bridge {
> >> =09void *release_data;
> >> };
> >> =20
> >> +struct pci_host_info {
> >> +=09u8 res_type;
> >> +=09void *arg;
> >> +=09struct list_head *resources; /*just for build, will clean up later=
*/
> >> +=09int (*init_res)(struct pci_host_bridge *host,=20
> >> +=09=09=09struct pci_host_info *info);
> >> +};
> >> +
> >> +static inline void init_pci_host_info(struct pci_host_info *info)
> >> +{
> >> +=09memset(info, 0 , sizeof(*info));
> >> +}
> >=20
> > Where is this used?
>=20
> Host driver uses it to init pci_host_info.
Might be worth adding it that patch rather than here.
Best regards,
Liviu
>=20
> >=20
> >> +
> >> +#define PCI_HOST_RES_DEFAULT=090x2
> >> +
> >=20
> > Magic number?
>=20
> Hmmm, I will rework pci host bridge resources stuff.
>=20
> >=20
> > Best regards,
> > Liviu
> >=20
> >> #define=09to_pci_host_bridge(n) container_of(n, struct pci_host_bridg=
e, dev)
> >> void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
> >> =09=09 void (*release_fn)(struct pci_host_bridge *),
> >> @@ -420,7 +435,7 @@ void pci_set_host_bridge_release(struct pci_host_b=
ridge *bridge,
> >> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
> >> struct pci_host_bridge *pci_create_host_bridge(
> >> =09=09struct device *parent, u32 db, struct pci_ops *ops,=20
> >> -=09=09void *sys, struct list_head *resources);
> >> +=09=09struct pci_host_info *info);
> >> /*
> >> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that co=
rrespond
> >> * to P2P or CardBus bridge windows) go in a table. Additional ones =
(for
> >> @@ -785,6 +800,9 @@ void pci_bus_release_busn_res(struct pci_bus *b);
> >> struct pci_bus *pci_scan_root_bus(struct device *parent, u32 bus,
> >> =09=09=09=09=09 struct pci_ops *ops, void *sysdata,
> >> =09=09=09=09=09 struct list_head *resources);
> >> +struct pci_host_bridge *pci_scan_host_bridge(struct device *parent,
> >> +=09=09u32 db, struct pci_ops *ops,
> >> +=09=09struct pci_host_info *info);
> >> struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_de=
v *dev,
> >> =09=09=09=09int busnr);
> >> void pcie_update_link_speed(struct pci_bus *bus, u16 link_status);
> >> --=20
> >> 1.7.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-pci" i=
n
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >=20
>=20
>=20
> --=20
> Thanks!
> Yijing
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>=20
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF
^ permalink raw reply
* Re: [PATCH V9 03/18] PCI: Add weak pcibios_iov_resource_size() interface
From: Bjorn Helgaas @ 2014-11-19 17:23 UTC (permalink / raw)
To: Wei Yang
Cc: Benjamin Herrenschmidt, linux-pci, linuxppc-dev, gwshan,
Donald Dutile, Myron Stowe
In-Reply-To: <20141119092740.GA12872@richard>
On Wed, Nov 19, 2014 at 05:27:40PM +0800, Wei Yang wrote:
> On Tue, Nov 18, 2014 at 09:26:01PM -0700, Bjorn Helgaas wrote:
> >On Wed, Nov 19, 2014 at 11:21:00AM +0800, Wei Yang wrote:
> >> On Wed, Nov 19, 2014 at 01:15:32PM +1100, Benjamin Herrenschmidt wrote:
> >> >On Tue, 2014-11-18 at 18:12 -0700, Bjorn Helgaas wrote:
> >> But the HW
> >> must map 256 segments with the same size. This will lead a situation like
> >> this.
> >>
> >> +------+------+ +------+------+------+------+
> >> |VF#0 |VF#1 | ... | |VF#N-1|PF#A |PF#B |
> >> +------+------+ +------+------+------+------+
> >>
> >> Suppose N = 254 and the HW map these 256 segments to their corresponding PE#.
> >
> >I guess these 256 segments are regions of CPU physical address space, and
> >they are being mapped to bus address space? Is there some relationship
> >between a PE and part of the bus address space?
> >
>
> PE is an entity for EEH, which may include a whole bus or one pci device.
Yes, I've read that many times. What's missing is the connection between a
PE and the things in the PCI specs (buses, devices, functions, MMIO address
space, DMA, MSI, etc.) Presumably the PE structure imposes constraints on
how the core uses the standard PCI elements, but we don't really have a
clear description of those constraints yet.
> When some device got some error, we need to identify which PE it belongs to.
> So we have some HW to map between PE# and MMIO/DMA/MSI address.
>
> The HW mentioned in previous letter is the one to map MMIO address to a PE#.
> While this HW must map a range with 256 equal segments. And yes, this is
> mapped to bus address space.
> ...
> >> The difference after our expanding is the IOV BAR size is 256*4KB instead of
> >> 16KB. So it will look like this:
> >>
> >> PF pci_dev->resource[7] = [mem 0x00000000-0x000fffff] (1024KB)
> >
> >Is the idea that you want this resource to be big enough to cover all 256
> >segments? I think I'm OK with increasing the size of the PF resources to
> >prevent overlap. That part shouldn't be too ugly.
> >
>
> Yes, big enough to cover all 256 segments.
>
> Sorry for making it ugly :-(
I didn't mean that what you did was ugly. I meant that increasing the size
of the PF resource can be done cleanly.
By the way, when you do this, it would be nice if the dmesg showed the
standard PF IOV BAR sizing, and then a separate line showing the resource
expansion to deal with the PE constraints. I don't think even the standard
output is very clear -- I think we currently get something like this:
pci 0000:00:00.0 reg 0x174: [mem 0x00000000-0x00000fff]
But that is only the size of a single VF BAR aperture. Then sriov_init()
multiplies that by the number of possible VFs, but I don't think we print
the overall size of that PF resource. I think we should, because it's
misleading to print only the smaller piece. Maybe something like this:
pci 0000:00:00.0 VF BAR0: [mem 0x00000000-0x00003fff] (for 4 VFs)
And then you could do something like:
pci 0000:00:00.0 VF BAR0: [mem 0x00000000-0x000fffff] (expanded for PE alignment)
> >> VF1 pci_dev->resource[0] = [mem 0x00000000-0x00000fff]
> >> VF2 pci_dev->resource[0] = [mem 0x00001000-0x00001fff]
> >> VF3 pci_dev->resource[0] = [mem 0x00002000-0x00002fff]
> >> VF4 pci_dev->resource[0] = [mem 0x00003000-0x00003fff]
> >> ...
> >> and 252 4KB space leave not used.
> >>
> >> So the start address and the size of VF will not change, but the PF's IOV BAR
> >> will be expanded.
> >
> >I'm really dubious about this change to use pci_iov_resource_size(). I
> >think you might be doing that because if you increase the PF resource size,
> >dividing that increased size by total_VFs will give you garbage. E.g., in
> >the example above, you would compute "size = 1024KB / 4", which would make
> >the VF BARs appear to be 256KB instead of 4KB as they should be.
>
> Yes, your understanding is correct.
>
> >I think it would be better to solve that problem by decoupling the PF
> >resource size and the VF BAR size. For example, we could keep track of the
> >VF BAR size explicitly in struct pci_sriov, instead of computing it from
> >the PF resource size and total_VFs. This would keep the VF BAR size
> >completely platform-independent.
>
> Hmm... this is another solution.
>
> If you prefer this one, I will make a change accordingly.
Yes, I definitely prefer to track the VF BAR size explicitly. I think that
will make the code much clearer.
Bjorn
^ permalink raw reply
* Re: [PATCH V9 03/18] PCI: Add weak pcibios_iov_resource_size() interface
From: Benjamin Herrenschmidt @ 2014-11-19 20:51 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Wei Yang, linux-pci, linuxppc-dev, gwshan, Donald Dutile,
Myron Stowe
In-Reply-To: <20141119172350.GC23467@google.com>
On Wed, 2014-11-19 at 10:23 -0700, Bjorn Helgaas wrote:
>
> Yes, I've read that many times. What's missing is the connection between a
> PE and the things in the PCI specs (buses, devices, functions, MMIO address
> space, DMA, MSI, etc.) Presumably the PE structure imposes constraints on
> how the core uses the standard PCI elements, but we don't really have a
> clear description of those constraints yet.
Right, a "PE" is a HW concept in fact in our bridges, that essentially is
a shared isolation state between DMA, MMIO, MSIs, PCIe error messages,...
for a given "domain" or set of PCI functions.
The techniques of how the HW resources are mapped to PE and associated
constraints are slightly different from one generation of our chips to
the next. In general, P7 follows an architecture known as "IODA" and P8
"IODA2". I'm trying to get that spec made available via OpenPower but
that hasn't happened yet.
In this case we mostly care about IODA2 (P8), so I'll give a quick
description here. Wei, feel free to copy/paste that into a bit of doco
to throw into Documentation/powerpc/ along with your next spin of the patch.
The concept of "PE" is a way to group the various resources associated
with a device or a set of device to provide isolation between partitions
(ie. filtering of DMA, MSIs etc...) and to provide a mechanism to freeze
a device that is causing errors in order to limit the possibility of
propagation of bad data.
There is thus, in HW, a table of "PE" states that contains a pair of
"frozen" state bits (one for MMIO and one for DMA, they get set together
but can be cleared independently) for each PE.
When a PE is frozen, all stores in any direction are dropped and all loads
return all 1's value. MSIs are also blocked. There's a bit more state that
captures things like the details of the error that caused the freeze etc...
but that's not critical.
The interesting part is how the various type of PCIe transactions (MMIO,
DMA,...) are matched to their corresponding PEs.
I will provide a rought description of what we have on P8 (IODA2). Keep
in mind that this is all per PHB (host bridge). Each PHB is a completely
separate HW entity which replicates the entire logic, so has its own set
of PEs etc...
First, P8 has 256 PEs per PHB.
* Inbound
For DMA, MSIs and inbound PCIe error messages, we have a table (in memory but
accessed in HW by the chip) that provides a direct correspondence between
a PCIe RID (bus/dev/fn) with a PE number. We call this the RTT.
- For DMA we then provide an entire address space for each PE that can contains
two "windows", depending on the value of PCI bit 59. Each window can then be
configured to be remapped via a "TCE table" (iommu translation table), which has
various configurable characteristics which we can describe another day.
- For MSIs, we have two windows in the address space (one at the top of the 32-bit
space and one much higher) which, via a combination of the address and MSI value,
will result in one of the 2048 interrupts per bridge being triggered. There's
a PE value in the interrupt controller descriptor table as well which is compared
with the PE obtained from the RTT to "authorize" the device to emit that specific
interrupt.
- Error messages just use the RTT.
* Outbound. That's where the tricky part is.
The PHB basically has a concept of "windows" from the CPU address space to the
PCI address space. There is one M32 window and 16 M64 windows. They have different
characteristics. First what they have in common: they are configured to forward a
configurable portion of the CPU address space to the PCIe bus and must be naturally
aligned power of two in size. The rest is different:
- The M32 window:
* It is limited to 4G in size
* It drops the top bits of the address (above the size) and replaces them with
a configurable value. This is typically used to generate 32-bit PCIe accesses. We
configure that window at boot from FW and don't touch it from Linux, it's usually
set to forward a 2G portion of address space from the CPU to PCIe
0x8000_0000..0xffff_ffff. (Note: The top 64K are actually reserved for MSIs but
this is not a problem at this point, we just need to ensure Linux doesn't assign
anything there, the M32 logic ignores that however and will forward in that space
if we try).
* It is divided into 256 segments of equal size. A table in the chip provides
for each of these 256 segments a PE#. That allows to essentially assign portions
of the MMIO space to PEs on a segment granularity. For a 2G window, this is 8M.
Now, this is the "main" window we use in Linux today (excluding SR-IOV). We
basically use the trick of forcing the bridge MMIO windows onto a segment
alignment/granularity so that the space behind a bridge can be assigned to a PE.
Ideally we would like to be able to have individual functions in PE's but that
would mean using a completely different address allocation scheme where individual
function BARs can be "grouped" to fit in one or more segments....
- The M64 windows.
* Their smallest size is 1M
* They do not translate addresses (the address on PCIe is the same as the
address on the PowerBus. There is a way to also set the top 14 bits which are
not conveyed by PowerBus but we don't use this).
* They can be configured to be segmented or not. When segmented, they have
256 segments, however they are not remapped. The segment number *is* the PE
number. When no segmented, the PE number can be specified for the entire
window.
* They support overlaps in which case there is a well defined ordering of
matching (I don't remember off hand which of the lower or higher numbered
window takes priority but basically it's well defined).
We have code (fairly new compared to the M32 stuff) that exploits that for
large BARs in 64-bit space:
We create a single big M64 that covers the entire region of address space that
has been assigned by FW for the PHB (about 64G, ignore the space for the M32,
it comes out of a different "reserve"). We configure that window as segmented.
Then we do the same thing as with M32, using the bridge aligment trick, to
match to those giant segments.
Since we cannot remap, we have two additional constraints:
- We do the PE# allocation *after* the 64-bit space has been assigned since
the segments used will derive directly the PE#, we then "update" the M32 PE#
for the devices that use both 32-bit and 64-bit spaces or assign the remaining
PE# to 32-bit only devices.
- We cannot "group" segments in HW so if a device ends up using more than
one segment, we end up with more than one PE#. There is a HW mechanism to
make the freeze state cascade to "companion" PEs but that only work for PCIe
error messages (typically used so that if you freeze a switch, it freezes all
its children). So we do it in SW. We lose a bit of effectiveness of EEH in that
case, but that's the best we found. So when any of the PEs freezes, we freeze
the other ones for that "domain". We thus introduce the concept of "master PE"
which is the one used for DMA, MSIs etc... and "secondary PEs" that are used
for the remaining M64 segments.
We would like to investigate using additional M64's in "single PE" mode to
overlay over specific BARs to work around some of that, for example for devices
with very large BARs (some GPUs), it would make sense, but we haven't done it
yet.
Finally, the plan to use M64 for SR-IOV, which we describe a bit already, consists
of using those M64's. So for a given IOV BAR, we need to effectively reserve the
entire 256 segments (256 * IOV BAR size) and then "position" the BAR to start at
the beginning of a free range of segments/PEs inside that M64.
The goal is of course to be able to give a separate PE for each VF...
I hope that helps clarifying things a bit ...
Cheers,
Ben.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox