Received-SPF: pass (google.com: domain of linux-kernel+bounces-199675-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 3 Jun 2024 16:07:55 -0400
From: Frank Li <Frank.li@nxp.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Robin Murphy <robin.murphy@arm.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: <Zl4im579O3qjIzZ3@lizhi-Precision-Tower-5810>
References: <Zl4OpTfcfqMHELiX@lizhi-Precision-Tower-5810>
 <20240603185627.GA687746@bhelgaas>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240603185627.GA687746@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?BrSGZjDBLK9EYWntQnzr8aJSUB4HYjP20G1GfkoI7w+OxVW65mj03X8v2ASh?=
 =?us-ascii?Q?uhHkfkPBcHWUJC7wWwRGnfLCspLdHOnxhPvn+RKj335XOknQNCDa3iAsuUBp?=
 =?us-ascii?Q?3fJjID7NPyg17HRHiRqC5YG4+O6d/Ls+iUph2HxXI5FLKAaH/FjNzEfM9NCQ?=
 =?us-ascii?Q?aHY3LZgS7pg26TcJYc9WUdJhPFeu/al5J/8+cXdMD6bVaiYVx0Dt0pNEGslp?=
 =?us-ascii?Q?4WI3QvMIVV8fbEVIscKNJhvvozMb7CsqWs+Cc+jb/IRGLiUkdq+lGe9oeonX?=
 =?us-ascii?Q?KYVPPc88crRp0kKpGnVV1Rn0LQtqxfautDn/py2AWgYOK9dJBpHV6qsZ/lcZ?=
 =?us-ascii?Q?I4cQR+AtvRsUpkV7wMQUZdDVcvsrtR7jzr80R1v9duQzQ4ymcsJOB/122lD+?=
 =?us-ascii?Q?kmx/7cawKm+IbdjqsQf2dnTE7Prvl/NhMPg8nb47jbGJ6agjuGj0EuVTlZgY?=
 =?us-ascii?Q?AQBsep5EFgJGnd8QQYESjkYvIEDQaSSLCAGToLGiJPnw2D0i0131n3Le4Aqh?=
 =?us-ascii?Q?ZudExOXRe5SaRTFG0mYJQoOu66hYP8rDuOs+ufsz5ybGufEejhbE6LiwiR3R?=
 =?us-ascii?Q?86NUQpnGblteYCznRev861tVnGSd/aZPGb+pAaIV+urz5F5lKm2lOVjvFTpW?=
 =?us-ascii?Q?voLOgPM0DXHjE0dmmQoDGFxlQ6TOaL90It1tMRsi18/gXCey0YA7uz+7JgCR?=
 =?us-ascii?Q?lN0ABWKTn2UVie17iRIAGzKHguwy4REnCPwWvig05gG/M/4v0XJVe9ggAlyB?=
 =?us-ascii?Q?illJV2fpzKzAYtF1AX8Po+BpsJUVftrXnjU58bcr39MYUetccMifpnXVkGG/?=
 =?us-ascii?Q?pMaXBP7tJIDAjOuklG6QNO8q671u+RuLOfOLgGPtLvgaPlXNAwEqc9hhQJNc?=
 =?us-ascii?Q?xbHoU0UId10sglY/vqA74fYYgAhhZD8Zd9fuZbStHLcMyw8aKhxSHyA1CNTG?=
 =?us-ascii?Q?axkV/XyRWckJmy0t8ziEVFeoqztXlUaNVbviC/vo0eNCIFsCyiqaWem7wm4x?=
 =?us-ascii?Q?nqXx8JwASewDe5ZXdXhlM/9TwpQ14y5FNbN+luDkMfxJzm6ZTFXrN6u4amwz?=
 =?us-ascii?Q?Ji/wrJKyqL9w8kde0i0MR4xDfsw6R6A3Dosm4YEXq2pjTl3FdXxfuPxzerjY?=
 =?us-ascii?Q?Z7toaSw/M9gKvt7fSBeSfqQ6bE47sAa+Dptq9WtEsA3NITuUdUR0uTKyvvIo?=
 =?us-ascii?Q?NSEaFnUbsO7Oy9Sc8uU75tTHedw5/Sg/MSLy11UBgBWs2nsqNHXyM42qA9Ui?=
 =?us-ascii?Q?19N78oT1cm7kJGDbWo9QCbVj41SMaKZUTOYvJj/CWsbb9zopDE2KABnpb7wq?=
 =?us-ascii?Q?Gl0bPAJjTYzfmwpr0o1L17LrClZ2pJxOa4XDk6LPzhoJHWOGwSXzg//88IG4?=
 =?us-ascii?Q?S2RTqbOgvToZHO1+lJsNzZSdvP/Bl3BHT10RWa1VXMYzlIxncn3Kom9/7vIp?=
 =?us-ascii?Q?JBVIVzNaC3KbzeKBGZ9FWCYyrxt8IoBloUq4NgzoiHFkiszq+feVCY37pYgS?=
 =?us-ascii?Q?9AGO6ITQ+tS0d0OVGHG41qha9151lA/bTtGNIbtebC/kQEgaOrdtrtHn4bnG?=
 =?us-ascii?Q?voCCRM4MeV3ImB1DYqvlMN8nDdcw0e+E0hYozoMu?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f42ba446-b2be-4360-302a-08dc8408e1b7
X-MS-Exchange-CrossTenant-AuthSource: PAXPR04MB9642.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jun 2024 20:08:06.2367
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: p5sLWg4ekcbJrcGHIlx49P2M1oYZ3jwmBJ0zITSH248TqNloP2fHpAe8uWxUI3EP/fZK6RMLdPQZEB4kM2UWSQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBBPR04MB7835

On Mon, Jun 03, 2024 at 01:56:27PM -0500, Bjorn Helgaas wrote:
> On Mon, Jun 03, 2024 at 02:42:45PM -0400, Frank Li wrote:
> > On Mon, Jun 03, 2024 at 12:19:21PM -0500, Bjorn Helgaas wrote:
> > > 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.
> > 
> > Yes, it convert 16bit RID to 6bit stream id.
> 
> IIUC, you're saying this is not a Synopsys feature, it's an i.MX
> feature.

Yes, it is i.MX feature. But I think other vendor should have similar
situation if use old arm smmu.

> 
> > > > 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 is not good idea to depend on boot loader so much.
> 
> I meant "could this be programmed once when the Linux imx host
> controller driver is probed?"  But from the below, it sounds like
> that's not possible in general because you don't have enough stream
> IDs to do that.

Oh! sorry miss understand what your means. It is possible like what I did
at v3 version. But I think it is not good enough. 

> 
> > Some hot plug devics
> > (SD7.0) may plug after system boot. Two PCIe instances shared one set
> > of 6bits stream id (total 64). Assume total 16 assign to two PCIe
> > controllers. each have 8 stream id. If use uboot assign it static, each
> > PCIe controller have below 8 devices.  It will be failrue one controller
> > connect 7, another connect 9. but if dynamtic alloc when devices add, both
> > controller can work.
> > 
> > Although we have not so much devices now,  this way give us possility to
> > improve it in future.
> > 
> > > 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.
> > 
> > Yes, but so far, we have not met such problem now. We can improve it when
> > we really face such problem.
> 
> If this controller can only support DMA from a limited number of
> endpoints below it, I think we should figure out how to enforce that
> directly.  Maybe we can prevent drivers from enabling bus mastering or
> something.  I'm not happy with the idea of waiting for and debugging a
> report of data corruption.

It may add a pre-add hook function to pci bridge. let me do more research.

Frank

> 
> Bjorn