Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Thierry Reding <thierry.reding@gmail.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc: Will Deacon <will@kernel.org>, Joerg Roedel <joro@8bytes.org>,
	iommu@lists.linux-foundation.org,
	Jon Hunter <jonathanh@nvidia.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	linux-tegra@vger.kernel.org, Robin Murphy <robin.murphy@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 01/10] memory: tegra: Implement SID override programming
Date: Mon, 26 Apr 2021 14:13:11 +0200
Message-ID: <YIauV/BgPCZSZ8u2@orome.fritz.box> (raw)
In-Reply-To: <03e2a655-7dbf-a729-75f6-98db353e2b91@canonical.com>

[-- Attachment #1.1: Type: text/plain, Size: 6949 bytes --]

On Mon, Apr 26, 2021 at 10:28:43AM +0200, Krzysztof Kozlowski wrote:
> On 20/04/2021 19:26, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Instead of programming all SID overrides during early boot, perform the
> > operation on-demand after the SMMU translations have been set up for a
> > device. This reuses data from device tree to match memory clients for a
> > device and programs the SID specified in device tree, which corresponds
> > to the SID used for the SMMU context banks for the device.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/memory/tegra/mc.c       |  9 +++++
> >  drivers/memory/tegra/tegra186.c | 72 +++++++++++++++++++++++++++++++++
> >  include/soc/tegra/mc.h          |  3 ++
> >  3 files changed, 84 insertions(+)
> > 
> > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> > index c854639cf30c..bace5ecfe770 100644
> > --- a/drivers/memory/tegra/mc.c
> > +++ b/drivers/memory/tegra/mc.c
> > @@ -97,6 +97,15 @@ struct tegra_mc *devm_tegra_memory_controller_get(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(devm_tegra_memory_controller_get);
> >  
> > +int tegra_mc_probe_device(struct tegra_mc *mc, struct device *dev)
> > +{
> > +	if (mc->soc->ops && mc->soc->ops->probe_device)
> > +		return mc->soc->ops->probe_device(mc, dev);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(tegra_mc_probe_device);
> > +
> >  static int tegra_mc_block_dma_common(struct tegra_mc *mc,
> >  				     const struct tegra_mc_reset *rst)
> >  {
> > diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
> > index 1f87915ccd62..e65eac5764d4 100644
> > --- a/drivers/memory/tegra/tegra186.c
> > +++ b/drivers/memory/tegra/tegra186.c
> > @@ -4,6 +4,7 @@
> >   */
> >  
> >  #include <linux/io.h>
> > +#include <linux/iommu.h>
> >  #include <linux/module.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/of_device.h>
> > @@ -15,6 +16,10 @@
> >  #include <dt-bindings/memory/tegra186-mc.h>
> >  #endif
> >  
> > +#define MC_SID_STREAMID_OVERRIDE_MASK GENMASK(7, 0)
> > +#define MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED BIT(16)
> > +#define MC_SID_STREAMID_SECURITY_OVERRIDE BIT(8)
> > +
> >  static void tegra186_mc_program_sid(struct tegra_mc *mc)
> >  {
> >  	unsigned int i;
> > @@ -66,10 +71,77 @@ static int tegra186_mc_resume(struct tegra_mc *mc)
> >  	return 0;
> >  }
> >  
> > +static void tegra186_mc_client_sid_override(struct tegra_mc *mc,
> > +					    const struct tegra_mc_client *client,
> > +					    unsigned int sid)
> > +{
> > +	u32 value, old;
> > +
> > +	value = readl(mc->regs + client->regs.sid.security);
> > +	if ((value & MC_SID_STREAMID_SECURITY_OVERRIDE) == 0) {
> > +		/*
> > +		 * If the secure firmware has locked this down the override
> > +		 * for this memory client, there's nothing we can do here.
> > +		 */
> > +		if (value & MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED)
> > +			return;
> > +
> > +		/*
> > +		 * Otherwise, try to set the override itself. Typically the
> > +		 * secure firmware will never have set this configuration.
> > +		 * Instead, it will either have disabled write access to
> > +		 * this field, or it will already have set an explicit
> > +		 * override itself.
> > +		 */
> > +		WARN_ON((value & MC_SID_STREAMID_SECURITY_OVERRIDE) == 0);
> > +
> > +		value |= MC_SID_STREAMID_SECURITY_OVERRIDE;
> > +		writel(value, mc->regs + client->regs.sid.security);
> > +	}
> > +
> > +	value = readl(mc->regs + client->regs.sid.override);
> > +	old = value & MC_SID_STREAMID_OVERRIDE_MASK;
> > +
> > +	if (old != sid) {
> > +		dev_dbg(mc->dev, "overriding SID %x for %s with %x\n", old,
> > +			client->name, sid);
> > +		writel(sid, mc->regs + client->regs.sid.override);
> > +	}
> > +}
> > +
> > +static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
> > +{
> > +#if IS_ENABLED(CONFIG_IOMMU_API)
> 
> Is this part really build-time dependent? I don't see here any uses of
> IOMMU specific fields, so maybe this should be runtime choice based on
> enabled interconnect devices?

Unfortunately it is. struct iommu_fwspec is an empty structure for
!CONFIG_IOMMU_API, so the code below that tries to access fwspec->ids
fails for !CONFIG_IOMMU_API configurations if we don't protect this with
the preprocessor guard.

> 
> > +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > +	struct of_phandle_args args;
> > +	unsigned int i, index = 0;
> > +
> > +	while (!of_parse_phandle_with_args(dev->of_node, "interconnects", "#interconnect-cells",
> > +					   index, &args)) {
> > +		if (args.np == mc->dev->of_node && args.args_count != 0) {
> > +			for (i = 0; i < mc->soc->num_clients; i++) {
> > +				const struct tegra_mc_client *client = &mc->soc->clients[i];
> > +
> > +				if (client->id == args.args[0]) {
> > +					u32 sid = fwspec->ids[0] & MC_SID_STREAMID_OVERRIDE_MASK;
> > +
> > +					tegra186_mc_client_sid_override(mc, client, sid);
> > +				}
> > +			}
> > +		}
> > +
> > +		index++;
> > +	}
> > +#endif
> > +
> > +	return 0;
> > +}
> > +
> >  const struct tegra_mc_ops tegra186_mc_ops = {
> >  	.probe = tegra186_mc_probe,
> >  	.remove = tegra186_mc_remove,
> >  	.resume = tegra186_mc_resume,
> > +	.probe_device = tegra186_mc_probe_device,
> >  };
> >  
> >  #if defined(CONFIG_ARCH_TEGRA_186_SOC)
> > diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
> > index 1387747d574b..bbad6330008b 100644
> > --- a/include/soc/tegra/mc.h
> > +++ b/include/soc/tegra/mc.h
> > @@ -176,6 +176,7 @@ struct tegra_mc_ops {
> >  	int (*suspend)(struct tegra_mc *mc);
> >  	int (*resume)(struct tegra_mc *mc);
> >  	irqreturn_t (*handle_irq)(int irq, void *data);
> > +	int (*probe_device)(struct tegra_mc *mc, struct device *dev);
> >  };
> >  
> >  struct tegra_mc_soc {
> > @@ -240,4 +241,6 @@ devm_tegra_memory_controller_get(struct device *dev)
> >  }
> >  #endif
> >  
> > +int tegra_mc_probe_device(struct tegra_mc *mc, struct device *dev);
> > +
> 
> What about !CONFIG_TEGRA_MC? I think arm-smmmu will fail.

I think it doesn't fail because for !CONFIG_TEGRA_MC it basically throws
away most of nvidia_smmu_impl_init() already because ERR_PTR(-ENODEV) is
returned by devm_tegra_memory_controller_get() and so it unconditionally
fails early on already.

I say I /think/ that happens because I can't reproduce a build failure
even if I manually tweak the .config such that ARM_SMMU is enabled and
TEGRA_MC is disabled. But I can't say I fully understand why it's
working, because, yes, the symbol definitely doesn't exist. But again,
if the compiler is clever enough to figure out that that function can't
be called anyway and doesn't even want it, why bother making it more
complicated than it has to be?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-20 17:26 [PATCH v2 00/10] arm64: tegra: Prevent early SMMU faults Thierry Reding
2021-04-20 17:26 ` [PATCH v2 01/10] memory: tegra: Implement SID override programming Thierry Reding
2021-04-26  8:28   ` Krzysztof Kozlowski
2021-04-26 12:13     ` Thierry Reding [this message]
2021-04-26 14:10       ` Krzysztof Kozlowski
2021-04-20 17:26 ` [PATCH v2 02/10] dt-bindings: arm-smmu: Add Tegra186 compatible string Thierry Reding
2021-04-20 17:26 ` [PATCH v2 03/10] iommu/arm-smmu: Implement ->probe_finalize() Thierry Reding
2021-04-20 17:26 ` [PATCH v2 04/10] iommu/arm-smmu: tegra: Detect number of instances at runtime Thierry Reding
2021-04-20 22:06   ` Krishna Reddy
2021-04-20 17:26 ` [PATCH v2 05/10] iommu/arm-smmu: tegra: Implement SID override programming Thierry Reding
2021-04-20 17:26 ` [PATCH v2 06/10] iommu/arm-smmu: Use Tegra implementation on Tegra186 Thierry Reding
2021-04-20 17:26 ` [PATCH v2 07/10] arm64: tegra: Use correct compatible string for Tegra186 SMMU Thierry Reding
2021-04-20 17:26 ` [PATCH v2 08/10] arm64: tegra: Hook up memory controller to SMMU on Tegra186 Thierry Reding
2021-04-20 17:26 ` [PATCH v2 09/10] arm64: tegra: Enable SMMU support on Tegra194 Thierry Reding
2021-04-20 17:26 ` [PATCH v2 10/10] arm64: tegra: Enable SMMU support for display " Thierry Reding
2021-05-28 17:05 ` [PATCH v2 00/10] arm64: tegra: Prevent early SMMU faults Thierry Reding
2021-06-01 12:26   ` Will Deacon
2021-06-01 18:08     ` Thierry Reding
2021-06-02  7:33       ` Krzysztof Kozlowski
2021-06-02  7:35         ` Krzysztof Kozlowski
2021-06-02  8:52           ` Thierry Reding
2021-06-02 10:44             ` Krzysztof Kozlowski
2021-06-02 11:40               ` Will Deacon
2021-06-02 14:58                 ` Thierry Reding
2021-06-02 14:58                   ` Krzysztof Kozlowski
2021-06-02 14:53               ` Thierry Reding
2021-06-02 14:57                 ` Krzysztof Kozlowski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YIauV/BgPCZSZ8u2@orome.fritz.box \
    --to=thierry.reding@gmail.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jonathanh@nvidia.com \
    --cc=joro@8bytes.org \
    --cc=krzysztof.kozlowski@canonical.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=nicolinc@nvidia.com \
    --cc=robin.murphy@arm.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git