lkml.org 
[lkml]   [2012]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC PATCH 01/10] s390/pci: base support
    On Wed, Nov 14, 2012 at 2:41 AM, Jan Glauber <jang@linux.vnet.ibm.com> wrote:
    > Add PCI support for s390, (only 64 bit mode is supported by hardware):
    > - PCI facility tests
    > - PCI instructions: pcilg, pcistg, pcistb, stpcifc, mpcifc, rpcit
    > - map readb/w/l/q and writeb/w/l/q to pcilg and pcistg instructions
    > - pci_iomap implementation
    > - memcpy_fromio/toio
    > - pci_root_ops using special pcilg/pcistg
    > - device, bus and domain allocation
    >
    > Signed-off-by: Jan Glauber <jang@linux.vnet.ibm.com>

    I think these patches are in -next already, so I just have some
    general comments & questions.

    My overall impression is that these are exceptionally well done.
    They're easy to read, well organized, and well documented. It's a
    refreshing change from a lot of the stuff that's posted.

    As with other arches that run on top of hypervisors, you have
    arch-specific code that enumerates PCI devices using hypervisor calls,
    and you hook into the PCI core at a lower level than
    pci_scan_root_bus(). That is, you call pci_create_root_bus(),
    pci_scan_single_device(), pci_bus_add_devices(), etc., directly from
    the arch code. This is the typical approach, but it does make more
    dependencies between the arch code and the PCI core than I'd like.

    Did you consider hiding any of the hypervisor details behind the PCI
    config accessor interface? If that could be done, the overall
    structure might be more similar to other architectures.

    The current config accessors only work for dev/fn 00.0 (they fail when
    "devfn != ZPCI_DEVFN"). Why is that? It looks like it precludes
    multi-function devices and basically prevents you from building an
    arbitrary PCI hierarchy.

    zpci_map_resources() is very unusual. The struct pci_dev resource[]
    table normally contains CPU physical addresses, but
    zpci_map_resources() fills it with virtual addresses. I suspect this
    has something to do with the "BAR spaces are not disjunctive on s390"
    comment. It almost sounds like that's describing host bridges where
    the PCI bus address is not equal to the CPU physical address -- in
    that case, device A and device B may have the same values in their
    BARs, but they can still be distinct if they're under host bridges
    that apply different CPU-to-bus address translations.

    Bjorn


    \
     
     \ /
      Last update: 2012-12-10 23:01    [W:3.299 / U:0.204 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site