[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK8P3a1iN76A5ahTTQ6rCS4LjKHz8grkNGHGehLJnd0xQSnHXA@mail.gmail.com>
Date: Wed, 22 Sep 2021 21:57:23 +0200
From: Arnd Bergmann <arnd@...db.de>
To: Sergio Paracuellos <sergio.paracuellos@...il.com>
Cc: Arnd Bergmann <arnd@...db.de>,
linux-pci <linux-pci@...r.kernel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-staging@...ts.linux.dev, gregkh <gregkh@...uxfoundation.org>,
Liviu Dudau <Liviu.Dudau@....com>,
Rob Herring <robh@...nel.org>,
Catalin Marinas <catalin.marinas@....com>,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Subject: Re: [PATCH v3] PCI: of: Avoid pci_remap_iospace() when PCI_IOBASE not defined
On Wed, Sep 22, 2021 at 8:40 PM Sergio Paracuellos
<sergio.paracuellos@...il.com> wrote:
> On Wed, Sep 22, 2021 at 8:07 PM Arnd Bergmann <arnd@...db.de> wrote:
> > On Wed, Sep 22, 2021 at 7:42 PM Sergio Paracuellos
> > Ok, thank you for the detailed explanation.
> >
> > I suppose you can use the generic infrastructure in asm-generic/io.h
> > if you "#define PCI_IOBASE mips_io_port_base". In this case, you
> > should have an architecture specific implementation of
> > pci_remap_iospace() that assigns mips_io_port_base.
>
> No, that is what I tried originally defining PCI_IOBASE as
> _AC(0xa0000000, UL) [0] which is the same as KSEG1 [1] that ends in
> 'mips_io_port_base'.
Defining it as KSEG1 would be problematic because that means that
the Linux-visible port numbers are offset from the bus-visible ones.
You really want PCI_IOBASE to point to the address of port 0.
> > pci_remap_iospace() was originally meant as an architecture
> > specific helper, but it moved into generic code after all architectures
> > had the same requirements. If MIPS has different requirements,
> > then it should not be shared.
>
> I see. So, if it can not be shared, would defining 'pci_remap_iospace'
> as 'weak' acceptable? Doing in this way I guess I can redefine the
> symbol for mips to have the same I currently have but without the
> ifdef in the core APIs...
I would hope to kill off the __weak functions, and prefer using an #ifdef
around the generic implementation. One way to do it is to define
a macro with the same name, such as
#define pci_remap_iospace pci_remap_iospace
and then use #ifdef around the C function to see if that's arleady defined.
> >
> > I don't yet understand how you deal with having multiple PCIe
> > host bridge devices if they have distinct I/O port ranges.
> > Without remapping to dynamic virtual addresses, does
> > that mean that every MMIO register between the first and
> > last PCIe bridge also shows up in /dev/ioport? Or do you
> > only support port I/O on the first PCIe host bridge?
>
> For example, this board is using all available three pci ports [2] and I get:
>
> root@...bee:~# cat /proc/ioports
> 1e160000-1e16ffff : pcie@...40000
> 1e160000-1e160fff : PCI Bus 0000:01
> 1e160000-1e16000f : 0000:01:00.0
> 1e160000-1e16000f : ahci
> 1e160010-1e160017 : 0000:01:00.0
> 1e160010-1e160017 : ahci
> 1e160018-1e16001f : 0000:01:00.0
> 1e160018-1e16001f : ahci
> 1e160020-1e160023 : 0000:01:00.0
> 1e160020-1e160023 : ahci
> 1e160024-1e160027 : 0000:01:00.0
> 1e160024-1e160027 : ahci
> 1e161000-1e161fff : PCI Bus 0000:02
> 1e161000-1e16100f : 0000:02:00.0
> 1e161000-1e16100f : ahci
> 1e161010-1e161017 : 0000:02:00.0
> 1e161010-1e161017 : ahci
> 1e161018-1e16101f : 0000:02:00.0
> 1e161018-1e16101f : ahci
> 1e161020-1e161023 : 0000:02:00.0
> 1e161020-1e161023 : ahci
> 1e161024-1e161027 : 0000:02:00.0
> 1e161024-1e161027 : ahci
> 1e162000-1e162fff : PCI Bus 0000:03
> 1e162000-1e16200f : 0000:03:00.0
> 1e162000-1e16200f : ahci
> 1e162010-1e162017 : 0000:03:00.0
> 1e162010-1e162017 : ahci
> 1e162018-1e16201f : 0000:03:00.0
> 1e162018-1e16201f : ahci
> 1e162020-1e162023 : 0000:03:00.0
> 1e162020-1e162023 : ahci
> 1e162024-1e162027 : 0000:03:00.0
> 1e162024-1e162027 : ahci
Ah ok, so there are I/O ports that are at least
visible (may or may not be accessed by the driver).
I only see one host bridge here though, and it has a single
I/O port range, so maybe all three ports are inside of
a single PCI domain?
Having high numbers for the I/O ports is definitely a
problem as I mentioned. Anything that tries to access
PC-style legacy devices on the low port numbers
will now directly go on the bus accessing MMIO
registers that it shouldn't, either causing a CPU exception
or (worse) undefined behavior from random register
accesses.
Arnd
Powered by blists - more mailing lists