Skip to contentSkip to conversation

memory: tegra: Block DMA for clients HW on a faulty memory access

RFC PATCH In review Dmitry Osipenko 2 participants 4 messages May 8, 2018 – Aug 5, 2018

Subsystems (2)
Tegra architecture support Memory controller drivers
lore
Share & tools
lore

Dmitry Osipenko

author
#0 lore raw

RFC PATCH 1/1 memory: tegra: Block DMA for clients HW on a faulty memory access

Currently Memory Controller informs about erroneous memory accesses done by memory clients and that's it. Let's make it to block whole HW unit that corresponds to the misbehaving memory client in order to try to avoid memory corruptions and to stop deliberate attempts of manipulation by a misbehaving client.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/memory/tegra/mc.c       | 40 ++++++++++++++++++
 drivers/memory/tegra/tegra114.c | 64 +++++++++++++++++++++++++++++
 drivers/memory/tegra/tegra124.c | 66 +++++++++++++++++++++++++++++
 drivers/memory/tegra/tegra20.c  | 52 +++++++++++++++++++++++
 drivers/memory/tegra/tegra210.c | 73 +++++++++++++++++++++++++++++++++
 drivers/memory/tegra/tegra30.c  | 66 +++++++++++++++++++++++++++++
 include/soc/tegra/mc.h          |  3 ++
 7 files changed, 364 insertions(+)
drivers/memory/tegra/mc.c +40 -0
index 22ec959db7d3..2b5e3ceb1349 100644
@@ -448,6 +448,42 @@ static const char *const error_names[8] = {
 	[6] = "SMMU translation error",
 };
 
+static int tegra_mc_error_block_unit_dma(struct tegra_mc *mc,
+					 unsigned int client)
+{
+	const struct tegra_mc_reset_ops *rst_ops;
+	const struct tegra_mc_reset *rst;
+	unsigned int id;
+	int err;
+
+	id = mc->soc->clients[client].reset_id;
+	if (id == TEGRA_MC_CLIENT_NO_RESET)
+		return -ENXIO;
+
+	rst_ops = mc->soc->reset_ops;
+	if (!rst_ops)
+		return -EINVAL;
+
+	if (!rst_ops->block_dma)
+		return -EINVAL;
+
+	rst = tegra_mc_reset_find(mc, id);
+	if (!rst)
+		return -ENODEV;
+
+	err = rst_ops->block_dma(mc, rst);
+	if (err) {
+		dev_err_ratelimited(mc->dev, "Failed to block %s DMA: %d\n",
+				    rst->name, err);
+		return err;
+	}
+
+	dev_warn_ratelimited(mc->dev, "Faulty DMA blocked for %s unit\n",
+			     rst->name);
+
+	return 0;
+}
+
 static irqreturn_t tegra_mc_irq(int irq, void *data)
 {
 	struct tegra_mc *mc = data;

@@ -534,6 +570,8 @@ static irqreturn_t tegra_mc_irq(int irq, void *data)
 		value = mc_readl(mc, MC_ERR_ADR);
 		addr |= value;
 
+		tegra_mc_error_block_unit_dma(mc, i);
+
 		dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s (%s%s)\n",
 				    client, secure, direction, &addr, error,
 				    desc, perm);

@@ -616,6 +654,8 @@ static __maybe_unused irqreturn_t tegra20_mc_irq(int irq, void *data)
 			continue;
 		}
 
+		tegra_mc_error_block_unit_dma(mc, id);
+
 		client = mc->soc->clients[id].name;
 
 		dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s (%s)\n",
drivers/memory/tegra/tegra114.c +64 -0

448 lines not on this page · raw message

drivers/memory/tegra/tegra124.c +66 -0

462 lines not on this page · raw message

drivers/memory/tegra/tegra20.c +52 -0

211 lines not on this page · raw message

drivers/memory/tegra/tegra210.c +73 -0

511 lines not on this page · raw message

drivers/memory/tegra/tegra30.c +66 -0

462 lines not on this page · raw message

include/soc/tegra/mc.h +3 -0
index 5bf72eb4dd51..ba64cb7a1d0c 100644
@@ -35,10 +35,13 @@ struct tegra_mc_la {
 	unsigned int def;
 };
 
+#define TEGRA_MC_CLIENT_NO_RESET	UINT_MAX
+
 struct tegra_mc_client {
 	unsigned int id;
 	const char *name;
 	unsigned int swgroup;
+	unsigned int reset_id;
 
 	unsigned int fifo_size;
Signature · 1 line
2.17.0
author
#1 lore raw
Quoted 5 lines · On Tuesday, 8 May 2018 19:58:41 MSK Dmitry Osipenko wrote:

Currently Memory Controller informs about erroneous memory accesses done by memory clients and that's it. Let's make it to block whole HW unit that corresponds to the misbehaving memory client in order to try to avoid memory corruptions and to stop deliberate attempts of manipulation by a misbehaving client.

Guys, any comments? That is a kinda useful feature, in worst case only some of memory could get corrupted instead of trashing the whole memory. In my experience with T20/30, the interrupt handling latency is low and blocking happens immediately after the first page fault.

#2 lore raw

One potential issue is with host1x clients where userspace processes can submit jobs with invalid memory accesses (addresses not mapped to IOMMU). If when such a failure happens, we disable the DMA for the whole host1x client, unrelated userspace processes may see failures even though there is no problem with their jobs.

Mikko

Quoted 14 lines · Dmitry Osipenko
Quoted 5 lines · On Tuesday, 8 May 2018 19:58:41 MSK Dmitry Osipenko wrote:

Currently Memory Controller informs about erroneous memory accesses done by memory clients and that's it. Let's make it to block whole HW unit that corresponds to the misbehaving memory client in order to try to avoid memory corruptions and to stop deliberate attempts of manipulation by a misbehaving client.

Guys, any comments? That is a kinda useful feature, in worst case only some of memory could get corrupted instead of trashing the whole memory. In my experience with T20/30, the interrupt handling latency is low and blocking happens immediately after the first page fault.

author
#3 lore raw
Quoted 5 lines · On Sunday, 5 August 2018 03:11:57 MSK Mikko Perttunen wrote:

One potential issue is with host1x clients where userspace processes can submit jobs with invalid memory accesses (addresses not mapped to IOMMU). If when such a failure happens, we disable the DMA for the whole host1x client, unrelated userspace processes may see failures even though there is no problem with their jobs.

Good point, I'll take a look at partial resurrection of patch [0]. Anyway I think it's still better to fail even the unrelated jobs, rather than to have corrupted memory.

[0] https://github.com/grate-driver/linux/commit/
c0351a9d01491af2b2fefc162de1c2e4fcfaa94c#diff-20226be476307cb7ec9f60e26a9c30deR378