| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z2FDp1zQ7JzxQKJT@lpieralisi>
Date: Tue, 17 Dec 2024 10:25:59 +0100
From: Lorenzo Pieralisi <lpieralisi@...nel.org>
To: Frank Li <Frank.li@....com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Richard Zhu <hongxing.zhu@....com>,
Lucas Stach <l.stach@...gutronix.de>,
Krzysztof Wilczyński <kw@...ux.com>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Rob Herring <robh@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
imx@...ts.linux.dev, alyssa@...enzweig.io, bpf@...r.kernel.org,
broonie@...nel.org, jgg@...pe.ca, joro@...tes.org,
lgirdwood@...il.com, maz@...nel.org, p.zabel@...gutronix.de,
robin.murphy@....com, will@...nel.org
Subject: Re: [PATCH v8 2/2] PCI: imx6: Add IOMMU and ITS MSI support for
i.MX95
On Fri, Dec 13, 2024 at 12:20:40PM -0500, Frank Li wrote:
> On Fri, Dec 13, 2024 at 10:32:28AM +0100, Lorenzo Pieralisi wrote:
> > On Thu, Dec 12, 2024 at 12:29:16PM -0500, Frank Li wrote:
> > > On Thu, Dec 12, 2024 at 05:46:09PM +0100, Lorenzo Pieralisi wrote:
> > > > On Tue, Dec 10, 2024 at 05:48:59PM -0500, 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 checking msi-map and iommu-map device tree properties 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.
> > > >
> > > > This is wrong information. What you want to say is that if an msi-map
> > > > isn't detected this means that the platform relies on DWC built-in
> > > > controller for MSIs (that does not need streamIDs handling).
> > > >
> > > > That's quite different from what you are writing here.
> > >
> > > How about ?
> > >
> > > "If an msi-map isn't detected, platform relies on DWC built-in controller
> > > for MSIs that does not need streamdIDs"
> >
> > Right. Question: what happens if DT shows that there are SMMU and/or
> > ITS bindings/mappings but the SMMU driver and ITS driver are either not
> > enabled or have not probed ?
>
> It is little bit complex.
> iommu:
> Case 1:
> iommu{
> status = "disabled"
> };
>
> PCI driver normal probed. if RID is in range of iommu-map, not
> any functional impact and harmless.
> If RID is out of range of iommu-map, "false alarm" will return.
> enable PCI EP device failure, but actually it can work without IOMMU.
What does "false alarm" mean in practice ? PCI device enable fails
but actually it should not ? That does not look like a false alarm to me.
>
> Case 2:
> iommu {
> status = "Okay"
> }
> but iommu driver have not probed yet. PCI Host bridge driver
> should defer till iommu probed.
>
> Worst case is "false alarm". But this happen is very rare if DTS is
> correct.
Explain what this means.
> MSI:
> case 1:
> msi-controller {
> status = "disabled";
> }
> Whole all dwc drivers will be broken.
What MSI controller. Please make an effort to be precise and explain.
Thanks,
Lorenzo
> case 2:
> msi-controller {
> status = "Okay"
> }
> if msi driver have not probed yet, PCI Host bridge driver will
> defer.
>
> Frank
>
> >
> > I assume the LUT programming makes no difference (it is useless yes but
> > should be harmless too) in this case but wanted to check with you.
> >
> > Thanks,
> > Lorenzo
> >
> > >
> > > >
> > > > >
> > > > > Register a PCI bus callback function to handle enable_device() and
> > > > > disable_device() operations, setting up the LUT whenever a new PCI device
> > > > > is enabled.
> > > > >
> > > > > Acked-by: Richard Zhu <hongxing.zhu@....com>
> > > > > Signed-off-by: Frank Li <Frank.Li@....com>
> > >
> > > [...]
> > >
> > > > > + int err_i, err_m;
> > > > > + u32 sid;
> > > > > +
> > > > > + dev = imx_pcie->pci->dev;
> > > > > +
> > > > > + target = NULL;
> > > > > + err_i = of_map_id(dev->of_node, rid, "iommu-map", "iommu-map-mask", &target, &sid_i);
> > > > > + if (target) {
> > > > > + of_node_put(target);
> > > > > + } else {
> > > > > + /*
> > > > > + * "target == NULL && err_i == 0" means use 1:1 map RID to
> > > >
> > > > Is it what it means ? Or does it mean that the iommu-map property was found
> > > > and RID is out of range ?
> > >
> > > yes, if this happen, sid_i will be equal to RID.
> > >
> > > >
> > > > Could you point me at a sample dts for this host bridge please ?
> > >
> > > https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi
> > >
> > > /* 0x10~0x17 stream id for pci0 */
> > > iommu-map = <0x000 &smmu 0x10 0x1>,
> > > <0x100 &smmu 0x11 0x7>;
> > >
> > > /* msi part */
> > > msi-map = <0x000 &its 0x10 0x1>,
> > > <0x100 &its 0x11 0x7>;
> > >
> > > >
> > > > > + * stream ID. Hardware can't support this because stream ID
> > > > > + * only 5bits
> > > >
> > > > It is 5 or 6 bits ? From GENMASK(5, 0) above it should be 6.
> > >
> > > Sorry for typo. it is 6bits.
> > >
> > > >
> > > > > + */
> > > > > + err_i = -EINVAL;
> > > > > + }
> > > > > +
> > > > > + target = NULL;
> > > > > + err_m = of_map_id(dev->of_node, rid, "msi-map", "msi-map-mask", &target, &sid_m);
> > > > > +
> > > > > + /*
> > > > > + * err_m target
> > > > > + * 0 NULL Use 1:1 map RID to stream ID,
> > > >
> > > > Again, is that what it really means ?
> > > >
> > > > > + * Current hardware can't support it,
> > > > > + * So return -EINVAL.
> > > > > + * != 0 NULL msi-map not exist, use built-in MSI.
> > > >
> > > > does not exist.
> > > >
> > > > > + * 0 != NULL Get correct streamID from RID.
> > > > > + * != 0 != NULL Unexisted case, never happen.
> > > >
> > > > "Invalid combination"
> > > >
> > > > > + */
> > > > > + if (!err_m && !target)
> > > > > + return -EINVAL;
> > > > > + else if (target)
> > > > > + of_node_put(target); /* Find stream ID map entry for RID in msi-map */
> > > > > +
> > > > > + /*
> > > > > + * msi-map iommu-map
> > > > > + * N N DWC MSI Ctrl
> > > > > + * Y Y ITS + SMMU, require the same sid
> > > > > + * Y N ITS
> > > > > + * N Y DWC MSI Ctrl + SMMU
> > > > > + */
> > > > > + if (err_i && err_m)
> > > > > + return 0;
> > > > > +
> > > > > + if (!err_i && !err_m) {
> > > > > + /*
> > > > > + * MSI glue layer auto add 2 bits controller ID ahead of stream
> > > >
> > > > What's "MSI glue layer" ?
> > >
> > > It is common term for IC desgin, which connect IP's signal to platform with
> > > some simple logic. Inside chip, when connect LUT output 6bit streamIDs
> > > to MSI controller, there are 2bits hardcode controller ID information
> > > append to 6 bits streamID.
> > >
> > > Glue Layer
> > > <==========>
> > > ┌─────┐ ┌──────────┐
> > > │ LUT │ 6bit stream ID │ │
> > > │ ┼─────────────────►│ MSI │
> > > └─────┘ 2bit ctrl ID │ │
> > > ┌───────────►│ │
> > > │ │ │
> > > 00 PCIe0 │ │ │
> > > 01 ENETC │ │ │
> > > 10 PCIe1 │ │ │
> > > │ └──────────┘
> > >
> > > >
> > > > > + * ID, so mask this 2bits to get stream ID.
> > > > > + * But IOMMU glue layer doesn't do that.
> > > >
> > > > and "IOMMU glue layer" ?
> > >
> > > See above.
> > >
> > > Frank
> > >
> > > >
> > > > > + */
> > > > > + if (sid_i != (sid_m & IMX95_SID_MASK)) {
> > > > > + dev_err(dev, "iommu-map and msi-map entries mismatch!\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + sid = sid_i;
> > > >
> > > > err_i could be != 0 here, I understand that the end result is
> > > > fine given how the code is written but it is misleading.
> > > >
> > > > if (!err_i)
> > > > else if (!err_m)
> > >
> > > Okay
> > >
> > > >
> > > > > + if (!err_m)
> > > > > + sid = sid_m & IMX95_SID_MASK;
> > > > > +
> > > > > + return imx_pcie_add_lut(imx_pcie, rid, sid);
> > > > > +}
> > > > > +
> > > > > +static void imx_pcie_disable_device(struct pci_host_bridge *bridge, struct pci_dev *pdev)
> > > > > +{
> > > > > + struct imx_pcie *imx_pcie;
> > > > > +
> > > > > + imx_pcie = to_imx_pcie(to_dw_pcie_from_pp(bridge->sysdata));
> > > > > + imx_pcie_remove_lut(imx_pcie, pci_dev_id(pdev));
> > > > > +}
> > > > > +
> > > > > static int imx_pcie_host_init(struct dw_pcie_rp *pp)
> > > > > {
> > > > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > @@ -946,6 +1122,11 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
> > > > > }
> > > > > }
> > > > >
> > > > > + if (pp->bridge && imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_LUT)) {
> > > > > + pp->bridge->enable_device = imx_pcie_enable_device;
> > > > > + pp->bridge->disable_device = imx_pcie_disable_device;
> > > > > + }
> > > > > +
> > > > > imx_pcie_assert_core_reset(imx_pcie);
> > > > >
> > > > > if (imx_pcie->drvdata->init_phy)
> > > > > @@ -1330,6 +1511,8 @@ static int imx_pcie_probe(struct platform_device *pdev)
> > > > > imx_pcie->pci = pci;
> > > > > imx_pcie->drvdata = of_device_get_match_data(dev);
> > > > >
> > > > > + mutex_init(&imx_pcie->lock);
> > > > > +
> > > > > /* Find the PHY if one is defined, only imx7d uses it */
> > > > > np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0);
> > > > > if (np) {
> > > > > @@ -1627,7 +1810,8 @@ static const struct imx_pcie_drvdata drvdata[] = {
> > > > > },
> > > > > [IMX95] = {
> > > > > .variant = IMX95,
> > > > > - .flags = IMX_PCIE_FLAG_HAS_SERDES,
> > > > > + .flags = IMX_PCIE_FLAG_HAS_SERDES |
> > > > > + IMX_PCIE_FLAG_HAS_LUT,
> > > > > .clk_names = imx8mq_clks,
> > > > > .clks_cnt = ARRAY_SIZE(imx8mq_clks),
> > > > > .ltssm_off = IMX95_PE0_GEN_CTRL_3,
> > > > >
> > > > > --
> > > > > 2.34.1
> > > > >
Powered by blists - more mailing lists