Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 18 Apr 2022 22:37:52 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     "Maciej W. Rozycki" <macro@orcam.me.uk>
Cc:     Palmer Dabbelt <palmer@dabbelt.com>,
        Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] PCI: Avoid handing out address 0 to devices
Message-ID: <20220419033752.GA1101844@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.21.2204160049500.9383@angie.orcam.me.uk>
Precedence: bulk

On Sat, Apr 16, 2022 at 03:02:35PM +0100, Maciej W. Rozycki wrote:
> On Fri, 15 Apr 2022, Bjorn Helgaas wrote:
> ...

> > Another possibility is PCIBIOS_MIN_IO.  It's also kind of an ugly
> > special case, but at least it already exists.  Most arches define it
> > to be non-zero, which should avoid this issue.
> > 
> > Defining PCIBIOS_MIN_IO would be simple; what would we lose compared
> > to adding code in pci_bus_alloc_from_region()?
> 
>  As I explained in the change description:
> 
>   Especially I/O space ranges are particularly valuable, because
>   bridges only decode bits from 12 up and consequently where 16-bit
>   addressing is in effect, as few as 16 separate ranges can be
>   assigned to individual buses only.
> 
>   Therefore avoid handing out address 0, however rather than bumping
>   the lowest address available to PCI via PCIBIOS_MIN_IO and
>   PCIBIOS_MIN_MEM, or doing an equivalent arrangement in
>   `__pci_assign_resource', let the whole range assigned to a bus
>   start from that address and instead only avoid it for actual
>   devices.  [...]
> 
> Yes, just bumping up PCIBIOS_MIN_IO was my first thought and the
> path of least resistance.  However with the strictly hierarchical
> topology of PCIe systems the limit of 16 ranges feels so
> frighteningly low to me already as to make me rather unwilling to
> reduce it even further for a system that is free from PC legacy junk
> (no southbridge let alone ISA) and therefore does not require it.
> So I've reconsidered my initial approach and came up with this
> proposal instead.  I think it is a good compromise.

Sorry for being dense here, I think it's finally sinking in.

The problem is that making PCIBIOS_MIN_IO greater than zero would keep
us from assigning a [io 0x0000- ] window, so instead of 16 I/O bridge
windows, we could only have 15 (unless bridges support 32-bit I/O
windows).  Right?

Are you running into that limit?  Most devices don't need I/O port
space, and almost all other arches define PCIBIOS_MIN_IO, so they live
without that window today.

Sparc uses the MMIO I/O port address directly in the struct resource,
so it will never try to allocate [io 0x0000], and there's no problem
with using PCI I/O port 0:

  pci_bus 0000:00: root bus resource [io  0x804000000000-0x80400fffffff] (bus address [0x0000-0xfffffff])
  mpt2sas0: ioport(0x0000804000001000), size(256)

The sparc ioport interfaces are basically:

  ioport_map(port)  { return port; }
  ioread8(addr)     { return readb(addr); }
  inb(addr)         { return readb(addr); }

RISC-V uses the generic ones, i.e.,

  ioport_map(port)  { return PIO_OFFSET + port; }
  ioread8(addr)     { if (addr) >= PIO_RESERVED)
                        return readb(addr);
                      else
                        return inb(addr & PIO_MASK); }
  inb(addr)         { return __raw_readb(PCI_IOBASE + addr); }

Obviously RISC-V gives you prettier I/O port addresses, but at the
cost of having PCI I/O port 0 be 0 in the struct resource as well,
which makes it basically unusable.  Is it worth it?

Bjorn