Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 16 Apr 2022 15:02:35 +0100 (BST)
From:   "Maciej W. Rozycki" <macro@orcam.me.uk>
To:     Bjorn Helgaas <helgaas@kernel.org>
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
In-Reply-To: <20220415183912.GA824311@bhelgaas>
Message-ID: <alpine.DEB.2.21.2204160049500.9383@angie.orcam.me.uk>
References: <20220415183912.GA824311@bhelgaas>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Fri, 15 Apr 2022, Bjorn Helgaas wrote:

> > > I guess the question is whether we want to reserve port 0 and MMIO
> > > address 0 as being "invalid".  That makes the first space smaller than
> > > the others, but it's not *much* smaller and it's an unlikely
> > > configuration to begin with.
> > 
> >  Unfortunately just as IRQ 0 is considered special and barring the 8254 
> > special exception for PC-style legacy junk it means "no IRQ", similarly 
> > I/O port or MMIO address 0 is considered "no device" in several places.  
> > One I have identified as noted above is `uart_configure_port':
> > 
> > 	/*
> > 	 * If there isn't a port here, don't do anything further.
> > 	 */
> > 	if (!port->iobase && !port->mapbase && !port->membase)
> > 		return;
> > 
> > So even if we let address 0 through it will be rejected downstream here 
> > and there and the device won't work.
> 
> This is a driver question, which I think is secondary.  If necessary,
> we can fix drivers after figuring out what the PCI core should do.

 This goes back to ISA and user-supplied driver options given as kernel 
parameters, so I am fairly sure it's been a part of our user interface 
since forever.  Changing these assumptions would surely break something 
for someone out there, so I would be very wary making such changes.

> > > We do have the IORESOURCE_UNSET flag bit that could possibly be used
> > > in pci_iomap_range() instead of testing for "!start".  Or maybe
> > > there's a way to allocate address 0 instead of special-casing the
> > > allocator?  Just thinking out loud here.
> 
> 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.

  Maciej