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

[Thierry Reding <thierry.reding@gmail.com>]

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

On Thu, Mar 25, 2021 at 02:27:10PM +0000, Robin Murphy wrote:
> 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.

My understanding is that this override is basically one level higher
than the SMMU. There's a special override SID (0x7f) that can be used to
avoid memory accesses to go through the SMMU at all. That is, as long as
that passthrough SID is configured for a memory client, accesses by that
client will be routed around the SMMU. Only if a valid SID is programmed
in this override will accesses for a memory client be routed to the
SMMU.

> > 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.

The idea behind this patch series is basically to use this mechanism in
order to avoid the murky waters between ARM SMMU driver probe and SMMU
device probe, so that we can avoid the early identity mappings that make
things so complicated.

So in other words until the device has been attached to the SMMU (at
which point it's expected that any identity mappings will have been
created), the device will remain in passthrough mode through the SID
override mechanism. After the device has been attached, we'd lock the
SID to the proper value and hence enable SMMU translation.

In a typical setup it would actually be fairly common to encounter the
above. The firmware will pre-program the SID overrides and lock down the
configuration for most devices. The only one that will stay unconfigured
at the moment is display, specifically because it is the only device
that may not be in a quiescent state during boot. For all other devices
write access to the SID override register is disabled and the above just
abandons early because the subsequent operations would just be
discarded.

> > +		/*
> > +		 * 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 :/

This is mostly for myself, or anyone else looking at the integration of
all this. I don't expect this to ever happen. If it does it basically
means that the firmware isn't programming things the way they are
expected to be programmed. It's a sanity check, basically.

Thierry

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

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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu