Re: [PATCH pci-fixes 2/2] Revert "PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge"

From: Bjorn Helgaas <helgaas@kernel.org>
To: "Marek Behún" <kabel@kernel.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, pali@kernel.org
Subject: Re: [PATCH pci-fixes 2/2] Revert "PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge"
Date: Wed, 1 Dec 2021 12:01:58 -0600	[thread overview]
Message-ID: <20211201180158.GA2830321@bhelgaas> (raw)
In-Reply-To: <20211201182324.28df466c@thinkpad>

On Wed, Dec 01, 2021 at 06:23:24PM +0100, Marek Behún wrote:
> On Wed, 1 Dec 2021 06:35:18 -0600
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > On Wed, Dec 01, 2021 at 10:50:45AM +0100, Marek Behún wrote:
> > > On Tue, 30 Nov 2021 19:53:08 -0600
> > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >   
> > > > On Tue, Nov 30, 2021 at 11:29:37AM +0000, Lorenzo Pieralisi wrote:  
> > > > > On Thu, Nov 25, 2021 at 05:01:48PM +0100, Marek Behún wrote:    
> > > > > > This reverts commit 239edf686c14a9ff926dec2f350289ed7adfefe2.
> > > > > > 
> > > > > > PCI Bridge which represents aardvark's PCIe Root Port has Expansion ROM
> > > > > > Base Address register at offset 0x30, but its meaning is different than
> > > > > > PCI's Expansion ROM BAR register, although the layout is the same.
> > > > > > (This is why we thought it does the same thing.)
> > > > > > 
> > > > > > First: there is no ROM (or part of BootROM) in the A3720 SOC dedicated
> > > > > > for PCIe Root Port (or controller in RC mode) containing executable code
> > > > > > that would initialize the Root Port, suitable for execution in
> > > > > > bootloader (this is how Expansion ROM BAR is used on x86).
> > > > > > 
> > > > > > Second: in A3720 spec the register (address D0070030) is not documented
> > > > > > at all for Root Complex mode, but similar to other BAR registers, it has
> > > > > > an "entangled partner" in register D0075920, which does address
> > > > > > translation for the BAR in D0070030:
> > > > > > - the BAR register sets the address from the view of PCIe bus
> > > > > > - the translation register sets the address from the view of the CPU
> > > > > > 
> > > > > > The other BAR registers also have this entangled partner, and they
> > > > > > can be used to:
> > > > > > - in RC mode: address-checking on the receive side of the RC (they
> > > > > >   can define address ranges for memory accesses from remote Endpoints
> > > > > >   to the RC)
> > > > > > - in Endpoint mode: allow the remote CPU to access memory on A3720
> > > > > > 
> > > > > > The Expansion ROM BAR has only the Endpoint part documented, but from
> > > > > > the similarities we think that it can also be used in RC mode in that
> > > > > > way.
> > > > > > 
> > > > > > So either Expansion ROM BAR has different meaning (if the hypothesis
> > > > > > above is true), or we don't know it's meaning (since it is not
> > > > > > documented for RC mode).
> > > > > > 
> > > > > > Remove the register from the emulated bridge accessing functions.
> > > > > > 
> > > > > > Fixes: 239edf686c14 ("PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge")
> > > > > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > > > > ---
> > > > > >  drivers/pci/controller/pci-aardvark.c | 9 ---------
> > > > > >  1 file changed, 9 deletions(-)    
> > > > > 
> > > > > Bjorn,
> > > > > 
> > > > > this reverts a commit we merged the last merge window so it is
> > > > > a candidate for one of the upcoming -rcX.    
> > > > 
> > > > Sure, happy to apply the revert.
> > > > 
> > > > What problem does the revert fix?  I assume 239edf686c14 ("PCI:
> > > > aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge") broke
> > > > something, but the commit log for the revert doesn't say *what*.  How
> > > > would one notice that something broke?  
> > > 
> > > Hello Bjorn,
> > > 
> > > It doesn't break any real functionality that I know of, although it
> > > might, since the register is read pci/probe.c pci_setup_device()
> > > (pci_read_bases()).
> > > 
> > > But allowing the access to the register when it has different meaning
> > > is wrong in a similar sense that a memory leak is wrong (a memory leak
> > > also does not necessarily cause real problems, if it is small, but
> > > still we should fix it).  
> > 
> > What is the new information that led you to conclude that 239edf686c14
> > is wrong?  Apparently you originally thought the bridge had a ROM BAR,
> > but later decided it didn't, based on *something*?  New observation?
> > New understanding of the spec?
> 
> Hi Bjorn,
> 
> The new observation is that although the register is defined in
> register list (together with it's layour), it is not documented in RC
> mode, although other BAR registers are (and their meaning is something
> different from standard BAR registers in RC mode). Combined with the
> fact that there is no ROM containing executable code in the SOC (which
> we knew even before, but thought that maybe it could be somehow also
> implemented), we concluded that this register has different meaning from
> standard Expansion ROM BAR.

Thanks, applied to for-linus for v5.16!

Bjorn