Re: [PATCH 3/9] memory: tegra: Implement SID override programming

[Robin Murphy <robin.murphy@arm.com>]

On 2021-03-25 13:03, 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.

Can you clarify what exactly the SID override does? I'm guessing it's 
more than just changing the ID presented to the SMMU from one value to 
another, since that alone wouldn't help under disable_bypass.

> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   drivers/memory/tegra/tegra186.c | 70 +++++++++++++++++++++++++++++++++
>   include/soc/tegra/mc.h          | 10 +++++
>   2 files changed, 80 insertions(+)
> 
> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
> index efa922d51d83..a89e8e40d875 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>
> @@ -19,6 +20,10 @@
>   #include <dt-bindings/memory/tegra194-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)
> +
>   struct tegra186_mc_client {
>   	const char *name;
>   	unsigned int id;
> @@ -1808,6 +1813,71 @@ static struct platform_driver tegra186_mc_driver = {
>   };
>   module_platform_driver(tegra186_mc_driver);
>   
> +static void tegra186_mc_client_sid_override(struct tegra_mc *mc,
> +					    const struct tegra186_mc_client *client,
> +					    unsigned int sid)
> +{
> +	u32 value, old;
> +
> +	value = readl(mc->regs + client->regs.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;

How likely is that in practice? If it's anything more than vanishingly 
rare then that would seem to be a strong pointer back towards 
persevering with the common solution that will work for everyone.

> +
> +		/*
> +		 * 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);

Given the context that's just WARN_ON(1), but either way I'm struggling 
to understand who the report is for and what they're supposed to do 
about it :/

Robin.

> +
> +		value |= MC_SID_STREAMID_SECURITY_OVERRIDE;
> +		writel(value, mc->regs + client->regs.security);
> +	}
> +
> +	value = readl(mc->regs + client->regs.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.override);
> +	}
> +}
> +
> +int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
> +{
> +	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 tegra186_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++;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tegra186_mc_probe_device);
> +
>   MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
>   MODULE_DESCRIPTION("NVIDIA Tegra186 Memory Controller driver");
>   MODULE_LICENSE("GPL v2");
> diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
> index 7be8441c6e9e..73d5ecf0e76a 100644
> --- a/include/soc/tegra/mc.h
> +++ b/include/soc/tegra/mc.h
> @@ -168,4 +168,14 @@ devm_tegra_memory_controller_get(struct device *dev)
>   }
>   #endif
>   
> +#if IS_ENABLED(CONFIG_ARCH_TEGRA_186_SOC) || \
> +    IS_ENABLED(CONFIG_ARCH_TEGRA_194_SOC)
> +int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev);
> +#else
> +static inline int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
> +{
> +	return 0;
> +}
> +#endif
> +
>   #endif /* __SOC_TEGRA_MC_H__ */
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu