Received-SPF: pass (google.com: domain of linux-kernel+bounces-199501-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 3 Jun 2024 12:19:21 -0500
From: Bjorn Helgaas <helgaas@kernel.org>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Frank Li <Frank.Li@nxp.com>, Richard Zhu <hongxing.zhu@nxp.com>,
	Lucas Stach <l.stach@pengutronix.de>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
	Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, linux-pci@vger.kernel.org,
	imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
	devicetree@vger.kernel.org, Will Deacon <will@kernel.org>,
	Joerg Roedel <joro@8bytes.org>, Jason Gunthorpe <jgg@ziepe.ca>,
	Alyssa Rosenzweig <alyssa@rosenzweig.io>,
	Marc Zyngier <maz@kernel.org>
Subject: Re: [PATCH v5 08/12] PCI: imx6: Config look up table(LUT) to support
 MSI ITS and IOMMU for i.MX95
Message-ID: <20240603171921.GA685838@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <974f1d23-aba8-432e-85b5-0e4b1c2005e7@arm.com>

On Fri, May 31, 2024 at 03:58:49PM +0100, Robin Murphy wrote:
> On 2024-05-31 12:08 am, Bjorn Helgaas wrote:
> > [+cc IOMMU and pcie-apple.c folks for comment]
> > 
> > On Tue, May 28, 2024 at 03:39:21PM -0400, Frank Li wrote:
> > > For the i.MX95, configuration of a LUT is necessary to convert Bus Device
> > > Function (BDF) to stream IDs, which are utilized by both IOMMU and ITS.
> > > This involves examining the msi-map and smmu-map to ensure consistent
> > > mapping of PCI BDF to the same stream IDs. Subsequently, LUT-related
> > > registers are configured. In the absence of an msi-map, the built-in MSI
> > > controller is utilized as a fallback.
> > > 
> > > Additionally, register a PCI bus notifier to trigger imx_pcie_add_device()
> > > upon the appearance of a new PCI device and when the bus is an iMX6 PCI
> > > controller. This function configures the correct LUT based on Device Tree
> > > Settings (DTS).
> > 
> > This scheme is pretty similar to apple_pcie_bus_notifier().  If we
> > have to do this, I wish it were *more* similar, i.e., copy the
> > function names, bitmap tracking, code structure, etc.
> > 
> > I don't really know how stream IDs work, but I assume they are used on
> > most or all arm64 platforms, so I'm a little surprised that of all the
> > PCI host drivers used on arm64, only pcie-apple.c and pci-imx6.c need
> > this notifier.
> 
> This is one of those things that's mostly at the mercy of the PCIe root
> complex implementation. Typically the SMMU StreamID and/or GIC ITS DeviceID
> is derived directly from the PCI RID, sometimes with additional high-order
> bits hard-wired to disambiguate PCI segments. I believe this RID-translation
> LUT is a particular feature of the the Synopsys IP - I know there's also one
> on the NXP Layerscape platforms, but on those it's programmed by the
> bootloader, which also generates the appropriate "msi-map" and "iommu-map"
> properties to match. Ideally that's what i.MX should do as well, but hey.

Maybe this RID-translation is a feature of i.MX, not of Synopsys?  I
see that the LUT CSR accesses use IMX95_* definitions.

> If it's really necessary to do this programming from Linux, then there's
> still no point in it being dynamic - the mappings cannot ever change, since
> the rest of the kernel believes that what the DT said at boot time was
> already a property of the hardware. It would be a lot more logical, and
> likely simpler, for the driver to just read the relevant map property and
> program the entire LUT to match, all in one go at controller probe time.
> Rather like what's already commonly done with the parsing of "dma-ranges" to
> program address-translation LUTs for inbound windows.
> 
> Plus that would also give a chance of safely dealing with bad DTs specifying
> invalid ID mappings (by refusing to probe at all). As it is, returning an
> error from a child's BUS_NOTIFY_ADD_DEVICE does nothing except prevent any
> further notifiers from running at that point - the device will still be
> added, allowed to bind a driver, and able to start sending DMA/MSI traffic
> without the controller being correctly programmed, which at best won't work
> and at worst may break the whole system.

Frank, could the imx LUT be programmed once at boot-time instead of at
device-add time?  I'm guessing maybe not because apparently there is a
risk of running out of LUT entries?

It sounds like the consequences of running out of LUT entries are
catastrophic, e.g., memory corruption from mis-directed DMA?  If
that's possible, I think we need to figure out how to prevent the
device from being used, not just dev_warn() about it.

Bjorn