[PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM

[PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM

[Mikko Perttunen]

On Tegra186 and later, a portion of the SYSRAM may be reserved for use
by TZ. Non-TZ memory accesses to this portion, including speculative
accesses, trigger SErrors that bring down the system. This does also
happen in practice occasionally (due to speculative accesses).

To fix the issue, add a flag to the SRAM driver to only map the
device tree-specified reserved areas depending on a flag set
based on the compatibility string. This would not affect non-Tegra
systems that rely on the entire thing being memory mapped.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/misc/sram.c | 108 +++++++++++++++++++++++++++++++-------------
 drivers/misc/sram.h |   9 ++++
 2 files changed, 86 insertions(+), 31 deletions(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 93638ae2753a..a27ffb3dbea8 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -97,7 +97,24 @@ static int sram_add_partition(struct sram_dev *sram, struct sram_reserve *block,
 	struct sram_partition *part = &sram->partition[sram->partitions];
 
 	mutex_init(&part->lock);
-	part->base = sram->virt_base + block->start;
+
+	if (sram->config->map_only_reserved) {
+		void *virt_base;
+
+		if (sram->no_memory_wc)
+			virt_base = devm_ioremap_resource(sram->dev, &block->res);
+		else
+			virt_base = devm_ioremap_resource_wc(sram->dev, &block->res);
+
+		if (IS_ERR(virt_base)) {
+			dev_err(sram->dev, "could not map SRAM at %pr\n", &block->res);
+			return PTR_ERR(virt_base);
+		}
+
+		part->base = virt_base;
+	} else {
+		part->base = sram->virt_base + block->start;
+	}
 
 	if (block->pool) {
 		ret = sram_add_pool(sram, block, start, part);
@@ -198,6 +215,7 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 
 		block->start = child_res.start - res->start;
 		block->size = resource_size(&child_res);
+		block->res = child_res;
 		list_add_tail(&block->list, &reserve_list);
 
 		if (of_find_property(child, "export", NULL))
@@ -295,15 +313,17 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 		 */
 		cur_size = block->start - cur_start;
 
-		dev_dbg(sram->dev, "adding chunk 0x%lx-0x%lx\n",
-			cur_start, cur_start + cur_size);
+		if (sram->pool) {
+			dev_dbg(sram->dev, "adding chunk 0x%lx-0x%lx\n",
+				cur_start, cur_start + cur_size);
 
-		ret = gen_pool_add_virt(sram->pool,
-				(unsigned long)sram->virt_base + cur_start,
-				res->start + cur_start, cur_size, -1);
-		if (ret < 0) {
-			sram_free_partitions(sram);
-			goto err_chunks;
+			ret = gen_pool_add_virt(sram->pool,
+					(unsigned long)sram->virt_base + cur_start,
+					res->start + cur_start, cur_size, -1);
+			if (ret < 0) {
+				sram_free_partitions(sram);
+				goto err_chunks;
+			}
 		}
 
 		/* next allocation after this reserved block */
@@ -331,40 +351,66 @@ static int atmel_securam_wait(void)
 					10000, 500000);
 }
 
+static const struct sram_config default_config = {
+};
+
+static const struct sram_config atmel_securam_config = {
+	.init = atmel_securam_wait
+};
+
+/*
+ * SYSRAM contains areas that are not accessible by the
+ * kernel, such as the first 256K that is reserved for TZ.
+ * Accesses to those areas (including speculative accesses)
+ * trigger SErrors. As such we must map only the areas of
+ * SYSRAM specified in the device tree.
+ */
+static const struct sram_config tegra_sysram_config = {
+	.map_only_reserved = true,
+};
+
 static const struct of_device_id sram_dt_ids[] = {
-	{ .compatible = "mmio-sram" },
-	{ .compatible = "atmel,sama5d2-securam", .data = atmel_securam_wait },
+	{ .compatible = "mmio-sram", .data = &default_config },
+	{ .compatible = "atmel,sama5d2-securam", .data = &atmel_securam_config },
+	{ .compatible = "nvidia,tegra186-sysram", .data = &tegra_sysram_config },
+	{ .compatible = "nvidia,tegra194-sysram", .data = &tegra_sysram_config },
 	{}
 };
 
 static int sram_probe(struct platform_device *pdev)
 {
+	const struct sram_config *config;
 	struct sram_dev *sram;
 	int ret;
 	struct resource *res;
-	int (*init_func)(void);
+
+	config = of_device_get_match_data(&pdev->dev);
 
 	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
 	if (!sram)
 		return -ENOMEM;
 
 	sram->dev = &pdev->dev;
+	sram->no_memory_wc = of_property_read_bool(pdev->dev.of_node, "no-memory-wc");
+	sram->config = config;
+
+	if (!config->map_only_reserved) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (sram->no_memory_wc)
+			sram->virt_base = devm_ioremap_resource(&pdev->dev, res);
+		else
+			sram->virt_base = devm_ioremap_resource_wc(&pdev->dev, res);
+		if (IS_ERR(sram->virt_base)) {
+			dev_err(&pdev->dev, "could not map SRAM registers\n");
+			return PTR_ERR(sram->virt_base);
+		}
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (of_property_read_bool(pdev->dev.of_node, "no-memory-wc"))
-		sram->virt_base = devm_ioremap_resource(&pdev->dev, res);
-	else
-		sram->virt_base = devm_ioremap_resource_wc(&pdev->dev, res);
-	if (IS_ERR(sram->virt_base)) {
-		dev_err(&pdev->dev, "could not map SRAM registers\n");
-		return PTR_ERR(sram->virt_base);
+		sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
+						  NUMA_NO_NODE, NULL);
+		if (IS_ERR(sram->pool))
+			return PTR_ERR(sram->pool);
 	}
 
-	sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
-					  NUMA_NO_NODE, NULL);
-	if (IS_ERR(sram->pool))
-		return PTR_ERR(sram->pool);
-
 	sram->clk = devm_clk_get(sram->dev, NULL);
 	if (IS_ERR(sram->clk))
 		sram->clk = NULL;
@@ -378,15 +424,15 @@ static int sram_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sram);
 
-	init_func = of_device_get_match_data(&pdev->dev);
-	if (init_func) {
-		ret = init_func();
+	if (config->init) {
+		ret = config->init();
 		if (ret)
 			goto err_free_partitions;
 	}
 
-	dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
-		gen_pool_size(sram->pool) / 1024, sram->virt_base);
+	if (sram->pool)
+		dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
+			gen_pool_size(sram->pool) / 1024, sram->virt_base);
 
 	return 0;
 
@@ -405,7 +451,7 @@ static int sram_remove(struct platform_device *pdev)
 
 	sram_free_partitions(sram);
 
-	if (gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
+	if (sram->pool && gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
 		dev_err(sram->dev, "removed while SRAM allocated\n");
 
 	if (sram->clk)
diff --git a/drivers/misc/sram.h b/drivers/misc/sram.h
index 9c1d21ff7347..d2058d8c8f1d 100644
--- a/drivers/misc/sram.h
+++ b/drivers/misc/sram.h
@@ -5,6 +5,11 @@
 #ifndef __SRAM_H
 #define __SRAM_H
 
+struct sram_config {
+	int (*init)(void);
+	bool map_only_reserved;
+};
+
 struct sram_partition {
 	void __iomem *base;
 
@@ -15,8 +20,11 @@ struct sram_partition {
 };
 
 struct sram_dev {
+	const struct sram_config *config;
+
 	struct device *dev;
 	void __iomem *virt_base;
+	bool no_memory_wc;
 
 	struct gen_pool *pool;
 	struct clk *clk;
@@ -29,6 +37,7 @@ struct sram_reserve {
 	struct list_head list;
 	u32 start;
 	u32 size;
+	struct resource res;
 	bool export;
 	bool pool;
 	bool protect_exec;
-- 
2.30.1

Re: [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM

[Greg KH]

On Thu, Jun 17, 2021 at 12:48:04PM +0300, Mikko Perttunen wrote:
> On Tegra186 and later, a portion of the SYSRAM may be reserved for use
> by TZ. Non-TZ memory accesses to this portion, including speculative
> accesses, trigger SErrors that bring down the system. This does also
> happen in practice occasionally (due to speculative accesses).
> 
> To fix the issue, add a flag to the SRAM driver to only map the
> device tree-specified reserved areas depending on a flag set
> based on the compatibility string. This would not affect non-Tegra
> systems that rely on the entire thing being memory mapped.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  drivers/misc/sram.c | 108 +++++++++++++++++++++++++++++++-------------
>  drivers/misc/sram.h |   9 ++++
>  2 files changed, 86 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> index 93638ae2753a..a27ffb3dbea8 100644
> --- a/drivers/misc/sram.c
> +++ b/drivers/misc/sram.c
> @@ -97,7 +97,24 @@ static int sram_add_partition(struct sram_dev *sram, struct sram_reserve *block,
>  	struct sram_partition *part = &sram->partition[sram->partitions];
>  
>  	mutex_init(&part->lock);
> -	part->base = sram->virt_base + block->start;
> +
> +	if (sram->config->map_only_reserved) {
> +		void *virt_base;
> +
> +		if (sram->no_memory_wc)
> +			virt_base = devm_ioremap_resource(sram->dev, &block->res);
> +		else
> +			virt_base = devm_ioremap_resource_wc(sram->dev, &block->res);
> +
> +		if (IS_ERR(virt_base)) {
> +			dev_err(sram->dev, "could not map SRAM at %pr\n", &block->res);
> +			return PTR_ERR(virt_base);
> +		}
> +
> +		part->base = virt_base;
> +	} else {
> +		part->base = sram->virt_base + block->start;
> +	}
>  
>  	if (block->pool) {
>  		ret = sram_add_pool(sram, block, start, part);
> @@ -198,6 +215,7 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
>  
>  		block->start = child_res.start - res->start;
>  		block->size = resource_size(&child_res);
> +		block->res = child_res;
>  		list_add_tail(&block->list, &reserve_list);
>  
>  		if (of_find_property(child, "export", NULL))
> @@ -295,15 +313,17 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
>  		 */
>  		cur_size = block->start - cur_start;
>  
> -		dev_dbg(sram->dev, "adding chunk 0x%lx-0x%lx\n",
> -			cur_start, cur_start + cur_size);
> +		if (sram->pool) {
> +			dev_dbg(sram->dev, "adding chunk 0x%lx-0x%lx\n",
> +				cur_start, cur_start + cur_size);
>  
> -		ret = gen_pool_add_virt(sram->pool,
> -				(unsigned long)sram->virt_base + cur_start,
> -				res->start + cur_start, cur_size, -1);
> -		if (ret < 0) {
> -			sram_free_partitions(sram);
> -			goto err_chunks;
> +			ret = gen_pool_add_virt(sram->pool,
> +					(unsigned long)sram->virt_base + cur_start,
> +					res->start + cur_start, cur_size, -1);
> +			if (ret < 0) {
> +				sram_free_partitions(sram);
> +				goto err_chunks;
> +			}
>  		}
>  
>  		/* next allocation after this reserved block */
> @@ -331,40 +351,66 @@ static int atmel_securam_wait(void)
>  					10000, 500000);
>  }
>  
> +static const struct sram_config default_config = {
> +};

No need for a "default" config, just do not provide one and then do not
call it if it is not set.  Much simpler and less code churn.


> +
> +static const struct sram_config atmel_securam_config = {
> +	.init = atmel_securam_wait

Add a trailing , please.

> +};
> +
> +/*
> + * SYSRAM contains areas that are not accessible by the
> + * kernel, such as the first 256K that is reserved for TZ.
> + * Accesses to those areas (including speculative accesses)
> + * trigger SErrors. As such we must map only the areas of
> + * SYSRAM specified in the device tree.
> + */
> +static const struct sram_config tegra_sysram_config = {
> +	.map_only_reserved = true,
> +};
> +
>  static const struct of_device_id sram_dt_ids[] = {
> -	{ .compatible = "mmio-sram" },
> -	{ .compatible = "atmel,sama5d2-securam", .data = atmel_securam_wait },
> +	{ .compatible = "mmio-sram", .data = &default_config },
> +	{ .compatible = "atmel,sama5d2-securam", .data = &atmel_securam_config },
> +	{ .compatible = "nvidia,tegra186-sysram", .data = &tegra_sysram_config },
> +	{ .compatible = "nvidia,tegra194-sysram", .data = &tegra_sysram_config },
>  	{}
>  };
>  
>  static int sram_probe(struct platform_device *pdev)
>  {
> +	const struct sram_config *config;
>  	struct sram_dev *sram;
>  	int ret;
>  	struct resource *res;
> -	int (*init_func)(void);
> +
> +	config = of_device_get_match_data(&pdev->dev);
>  
>  	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
>  	if (!sram)
>  		return -ENOMEM;
>  
>  	sram->dev = &pdev->dev;
> +	sram->no_memory_wc = of_property_read_bool(pdev->dev.of_node, "no-memory-wc");
> +	sram->config = config;
> +
> +	if (!config->map_only_reserved) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (sram->no_memory_wc)
> +			sram->virt_base = devm_ioremap_resource(&pdev->dev, res);
> +		else
> +			sram->virt_base = devm_ioremap_resource_wc(&pdev->dev, res);
> +		if (IS_ERR(sram->virt_base)) {
> +			dev_err(&pdev->dev, "could not map SRAM registers\n");
> +			return PTR_ERR(sram->virt_base);
> +		}
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (of_property_read_bool(pdev->dev.of_node, "no-memory-wc"))
> -		sram->virt_base = devm_ioremap_resource(&pdev->dev, res);
> -	else
> -		sram->virt_base = devm_ioremap_resource_wc(&pdev->dev, res);
> -	if (IS_ERR(sram->virt_base)) {
> -		dev_err(&pdev->dev, "could not map SRAM registers\n");
> -		return PTR_ERR(sram->virt_base);
> +		sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
> +						  NUMA_NO_NODE, NULL);
> +		if (IS_ERR(sram->pool))
> +			return PTR_ERR(sram->pool);
>  	}
>  
> -	sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
> -					  NUMA_NO_NODE, NULL);
> -	if (IS_ERR(sram->pool))
> -		return PTR_ERR(sram->pool);
> -
>  	sram->clk = devm_clk_get(sram->dev, NULL);
>  	if (IS_ERR(sram->clk))
>  		sram->clk = NULL;
> @@ -378,15 +424,15 @@ static int sram_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, sram);
>  
> -	init_func = of_device_get_match_data(&pdev->dev);
> -	if (init_func) {
> -		ret = init_func();
> +	if (config->init) {
> +		ret = config->init();
>  		if (ret)
>  			goto err_free_partitions;
>  	}
>  
> -	dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
> -		gen_pool_size(sram->pool) / 1024, sram->virt_base);
> +	if (sram->pool)
> +		dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
> +			gen_pool_size(sram->pool) / 1024, sram->virt_base);
>  
>  	return 0;
>  
> @@ -405,7 +451,7 @@ static int sram_remove(struct platform_device *pdev)
>  
>  	sram_free_partitions(sram);
>  
> -	if (gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
> +	if (sram->pool && gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
>  		dev_err(sram->dev, "removed while SRAM allocated\n");
>  
>  	if (sram->clk)
> diff --git a/drivers/misc/sram.h b/drivers/misc/sram.h
> index 9c1d21ff7347..d2058d8c8f1d 100644
> --- a/drivers/misc/sram.h
> +++ b/drivers/misc/sram.h
> @@ -5,6 +5,11 @@
>  #ifndef __SRAM_H
>  #define __SRAM_H
>  
> +struct sram_config {
> +	int (*init)(void);
> +	bool map_only_reserved;
> +};
> +
>  struct sram_partition {
>  	void __iomem *base;
>  
> @@ -15,8 +20,11 @@ struct sram_partition {
>  };
>  
>  struct sram_dev {
> +	const struct sram_config *config;
> +
>  	struct device *dev;
>  	void __iomem *virt_base;
> +	bool no_memory_wc;

What does "wc" here mean?

thanks,

greg k-h

Re: [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM

[Mikko Perttunen]

On 6/17/21 2:00 PM, Greg KH wrote:
> On Thu, Jun 17, 2021 at 12:48:04PM +0300, Mikko Perttunen wrote:
>> On Tegra186 and later, a portion of the SYSRAM may be reserved for use
>> by TZ. Non-TZ memory accesses to this portion, including speculative
>> accesses, trigger SErrors that bring down the system. This does also
>> happen in practice occasionally (due to speculative accesses).
>>
>> To fix the issue, add a flag to the SRAM driver to only map the
>> device tree-specified reserved areas depending on a flag set
>> based on the compatibility string. This would not affect non-Tegra
>> systems that rely on the entire thing being memory mapped.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>>   drivers/misc/sram.c | 108 +++++++++++++++++++++++++++++++-------------
>>   drivers/misc/sram.h |   9 ++++
>>   2 files changed, 86 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
>> index 93638ae2753a..a27ffb3dbea8 100644
>> --- a/drivers/misc/sram.c
>> +++ b/drivers/misc/sram.c
>> @@ -97,7 +97,24 @@ static int sram_add_partition(struct sram_dev *sram, struct sram_reserve *block,
>>   	struct sram_partition *part = &sram->partition[sram->partitions];
>>   
>>   	mutex_init(&part->lock);
>> -	part->base = sram->virt_base + block->start;
>> +
>> +	if (sram->config->map_only_reserved) {
>> +		void *virt_base;
>> +
>> +		if (sram->no_memory_wc)
>> +			virt_base = devm_ioremap_resource(sram->dev, &block->res);
>> +		else
>> +			virt_base = devm_ioremap_resource_wc(sram->dev, &block->res);
>> +
>> +		if (IS_ERR(virt_base)) {
>> +			dev_err(sram->dev, "could not map SRAM at %pr\n", &block->res);
>> +			return PTR_ERR(virt_base);
>> +		}
>> +
>> +		part->base = virt_base;
>> +	} else {
>> +		part->base = sram->virt_base + block->start;
>> +	}
>>   
>>   	if (block->pool) {
>>   		ret = sram_add_pool(sram, block, start, part);
>> @@ -198,6 +215,7 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
>>   
>>   		block->start = child_res.start - res->start;
>>   		block->size = resource_size(&child_res);
>> +		block->res = child_res;
>>   		list_add_tail(&block->list, &reserve_list);
>>   
>>   		if (of_find_property(child, "export", NULL))
>> @@ -295,15 +313,17 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
>>   		 */
>>   		cur_size = block->start - cur_start;
>>   
>> -		dev_dbg(sram->dev, "adding chunk 0x%lx-0x%lx\n",
>> -			cur_start, cur_start + cur_size);
>> +		if (sram->pool) {
>> +			dev_dbg(sram->dev, "adding chunk 0x%lx-0x%lx\n",
>> +				cur_start, cur_start + cur_size);
>>   
>> -		ret = gen_pool_add_virt(sram->pool,
>> -				(unsigned long)sram->virt_base + cur_start,
>> -				res->start + cur_start, cur_size, -1);
>> -		if (ret < 0) {
>> -			sram_free_partitions(sram);
>> -			goto err_chunks;
>> +			ret = gen_pool_add_virt(sram->pool,
>> +					(unsigned long)sram->virt_base + cur_start,
>> +					res->start + cur_start, cur_size, -1);
>> +			if (ret < 0) {
>> +				sram_free_partitions(sram);
>> +				goto err_chunks;
>> +			}
>>   		}
>>   
>>   		/* next allocation after this reserved block */
>> @@ -331,40 +351,66 @@ static int atmel_securam_wait(void)
>>   					10000, 500000);
>>   }
>>   
>> +static const struct sram_config default_config = {
>> +};
> 
> No need for a "default" config, just do not provide one and then do not
> call it if it is not set.  Much simpler and less code churn.

Sure, will do.

> 
> 
>> +
>> +static const struct sram_config atmel_securam_config = {
>> +	.init = atmel_securam_wait
> 
> Add a trailing , please.

Will do.

> 
>> +};
>> +
>> +/*
>> + * SYSRAM contains areas that are not accessible by the
>> + * kernel, such as the first 256K that is reserved for TZ.
>> + * Accesses to those areas (including speculative accesses)
>> + * trigger SErrors. As such we must map only the areas of
>> + * SYSRAM specified in the device tree.
>> + */
>> +static const struct sram_config tegra_sysram_config = {
>> +	.map_only_reserved = true,
>> +};
>> +
>>   static const struct of_device_id sram_dt_ids[] = {
>> -	{ .compatible = "mmio-sram" },
>> -	{ .compatible = "atmel,sama5d2-securam", .data = atmel_securam_wait },
>> +	{ .compatible = "mmio-sram", .data = &default_config },
>> +	{ .compatible = "atmel,sama5d2-securam", .data = &atmel_securam_config },
>> +	{ .compatible = "nvidia,tegra186-sysram", .data = &tegra_sysram_config },
>> +	{ .compatible = "nvidia,tegra194-sysram", .data = &tegra_sysram_config },
>>   	{}
>>   };
>>   
>>   static int sram_probe(struct platform_device *pdev)
>>   {
>> +	const struct sram_config *config;
>>   	struct sram_dev *sram;
>>   	int ret;
>>   	struct resource *res;
>> -	int (*init_func)(void);
>> +
>> +	config = of_device_get_match_data(&pdev->dev);
>>   
>>   	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
>>   	if (!sram)
>>   		return -ENOMEM;
>>   
>>   	sram->dev = &pdev->dev;
>> +	sram->no_memory_wc = of_property_read_bool(pdev->dev.of_node, "no-memory-wc");
>> +	sram->config = config;
>> +
>> +	if (!config->map_only_reserved) {
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +		if (sram->no_memory_wc)
>> +			sram->virt_base = devm_ioremap_resource(&pdev->dev, res);
>> +		else
>> +			sram->virt_base = devm_ioremap_resource_wc(&pdev->dev, res);
>> +		if (IS_ERR(sram->virt_base)) {
>> +			dev_err(&pdev->dev, "could not map SRAM registers\n");
>> +			return PTR_ERR(sram->virt_base);
>> +		}
>>   
>> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> -	if (of_property_read_bool(pdev->dev.of_node, "no-memory-wc"))
>> -		sram->virt_base = devm_ioremap_resource(&pdev->dev, res);
>> -	else
>> -		sram->virt_base = devm_ioremap_resource_wc(&pdev->dev, res);
>> -	if (IS_ERR(sram->virt_base)) {
>> -		dev_err(&pdev->dev, "could not map SRAM registers\n");
>> -		return PTR_ERR(sram->virt_base);
>> +		sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
>> +						  NUMA_NO_NODE, NULL);
>> +		if (IS_ERR(sram->pool))
>> +			return PTR_ERR(sram->pool);
>>   	}
>>   
>> -	sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
>> -					  NUMA_NO_NODE, NULL);
>> -	if (IS_ERR(sram->pool))
>> -		return PTR_ERR(sram->pool);
>> -
>>   	sram->clk = devm_clk_get(sram->dev, NULL);
>>   	if (IS_ERR(sram->clk))
>>   		sram->clk = NULL;
>> @@ -378,15 +424,15 @@ static int sram_probe(struct platform_device *pdev)
>>   
>>   	platform_set_drvdata(pdev, sram);
>>   
>> -	init_func = of_device_get_match_data(&pdev->dev);
>> -	if (init_func) {
>> -		ret = init_func();
>> +	if (config->init) {
>> +		ret = config->init();
>>   		if (ret)
>>   			goto err_free_partitions;
>>   	}
>>   
>> -	dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
>> -		gen_pool_size(sram->pool) / 1024, sram->virt_base);
>> +	if (sram->pool)
>> +		dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
>> +			gen_pool_size(sram->pool) / 1024, sram->virt_base);
>>   
>>   	return 0;
>>   
>> @@ -405,7 +451,7 @@ static int sram_remove(struct platform_device *pdev)
>>   
>>   	sram_free_partitions(sram);
>>   
>> -	if (gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
>> +	if (sram->pool && gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
>>   		dev_err(sram->dev, "removed while SRAM allocated\n");
>>   
>>   	if (sram->clk)
>> diff --git a/drivers/misc/sram.h b/drivers/misc/sram.h
>> index 9c1d21ff7347..d2058d8c8f1d 100644
>> --- a/drivers/misc/sram.h
>> +++ b/drivers/misc/sram.h
>> @@ -5,6 +5,11 @@
>>   #ifndef __SRAM_H
>>   #define __SRAM_H
>>   
>> +struct sram_config {
>> +	int (*init)(void);
>> +	bool map_only_reserved;
>> +};
>> +
>>   struct sram_partition {
>>   	void __iomem *base;
>>   
>> @@ -15,8 +20,11 @@ struct sram_partition {
>>   };
>>   
>>   struct sram_dev {
>> +	const struct sram_config *config;
>> +
>>   	struct device *dev;
>>   	void __iomem *virt_base;
>> +	bool no_memory_wc;
> 
> What does "wc" here mean?

Writecombine. The code in _probe checks for the "no-memory-wc" flag in 
device tree and if it's set it uses devm_ioremap_resource instead of 
devm_ioremap_resource_wc. Now that the ioremap call can happen outside 
of _probe we need to save this property.

> 
> thanks,
> 
> greg k-h
> 

Thank you, will send a v2 with changes.

Mikko

Re: [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4211 bytes --]

Hi Mikko,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20210616]
[cannot apply to char-misc/char-misc-testing soc/for-next tegra-drm/drm/tegra/for-next v5.13-rc6 v5.13-rc5 v5.13-rc4 v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mikko-Perttunen/misc-sram-Only-map-reserved-areas-in-Tegra-SYSRAM/20210617-174946
base:    c7d4c1fd91ab4a6d2620497921a9c6bf54650ab8
config: m68k-randconfig-s031-20210618 (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/3108530c06411e371ed28777233c48d89fa61071
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mikko-Perttunen/misc-sram-Only-map-reserved-areas-in-Tegra-SYSRAM/20210617-174946
        git checkout 3108530c06411e371ed28777233c48d89fa61071
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/misc/sram.c:105:35: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void *virt_base @@     got void [noderef] __iomem * @@
   drivers/misc/sram.c:105:35: sparse:     expected void *virt_base
   drivers/misc/sram.c:105:35: sparse:     got void [noderef] __iomem *
   drivers/misc/sram.c:107:35: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void *virt_base @@     got void [noderef] __iomem * @@
   drivers/misc/sram.c:107:35: sparse:     expected void *virt_base
   drivers/misc/sram.c:107:35: sparse:     got void [noderef] __iomem *
>> drivers/misc/sram.c:114:28: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void [noderef] __iomem *base @@     got void *virt_base @@
   drivers/misc/sram.c:114:28: sparse:     expected void [noderef] __iomem *base
   drivers/misc/sram.c:114:28: sparse:     got void *virt_base

vim +105 drivers/misc/sram.c

    92	
    93	static int sram_add_partition(struct sram_dev *sram, struct sram_reserve *block,
    94				      phys_addr_t start)
    95	{
    96		int ret;
    97		struct sram_partition *part = &sram->partition[sram->partitions];
    98	
    99		mutex_init(&part->lock);
   100	
   101		if (sram->config->map_only_reserved) {
   102			void *virt_base;
   103	
   104			if (sram->no_memory_wc)
 > 105				virt_base = devm_ioremap_resource(sram->dev, &block->res);
   106			else
   107				virt_base = devm_ioremap_resource_wc(sram->dev, &block->res);
   108	
   109			if (IS_ERR(virt_base)) {
   110				dev_err(sram->dev, "could not map SRAM at %pr\n", &block->res);
   111				return PTR_ERR(virt_base);
   112			}
   113	
 > 114			part->base = virt_base;
   115		} else {
   116			part->base = sram->virt_base + block->start;
   117		}
   118	
   119		if (block->pool) {
   120			ret = sram_add_pool(sram, block, start, part);
   121			if (ret)
   122				return ret;
   123		}
   124		if (block->export) {
   125			ret = sram_add_export(sram, block, start, part);
   126			if (ret)
   127				return ret;
   128		}
   129		if (block->protect_exec) {
   130			ret = sram_check_protect_exec(sram, block, part);
   131			if (ret)
   132				return ret;
   133	
   134			ret = sram_add_pool(sram, block, start, part);
   135			if (ret)
   136				return ret;
   137	
   138			sram_add_protect_exec(part);
   139		}
   140	
   141		sram->partitions++;
   142	
   143		return 0;
   144	}
   145	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 19581 bytes --]

Re: [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM

[Mian Yousaf Kaukab]

On Thu, Jun 17, 2021 at 12:48:04PM +0300, Mikko Perttunen wrote:
> On Tegra186 and later, a portion of the SYSRAM may be reserved for use
> by TZ. Non-TZ memory accesses to this portion, including speculative
> accesses, trigger SErrors that bring down the system. This does also
> happen in practice occasionally (due to speculative accesses).
> 
> To fix the issue, add a flag to the SRAM driver to only map the
> device tree-specified reserved areas depending on a flag set
> based on the compatibility string. This would not affect non-Tegra
> systems that rely on the entire thing being memory mapped.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  drivers/misc/sram.c | 108 +++++++++++++++++++++++++++++++-------------
>  drivers/misc/sram.h |   9 ++++
>  2 files changed, 86 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> index 93638ae2753a..a27ffb3dbea8 100644
> --- a/drivers/misc/sram.c
> +++ b/drivers/misc/sram.c
> @@ -97,7 +97,24 @@ static int sram_add_partition(struct sram_dev *sram, struct sram_reserve *block,
>  	struct sram_partition *part = &sram->partition[sram->partitions];
>  
>  	mutex_init(&part->lock);
> -	part->base = sram->virt_base + block->start;
> +
> +	if (sram->config->map_only_reserved) {
> +		void *virt_base;
> +
> +		if (sram->no_memory_wc)
> +			virt_base = devm_ioremap_resource(sram->dev, &block->res);
> +		else
> +			virt_base = devm_ioremap_resource_wc(sram->dev, &block->res);
> +
> +		if (IS_ERR(virt_base)) {
> +			dev_err(sram->dev, "could not map SRAM at %pr\n", &block->res);
> +			return PTR_ERR(virt_base);
> +		}
> +
> +		part->base = virt_base;
> +	} else {
> +		part->base = sram->virt_base + block->start;
> +	}
>  
>  	if (block->pool) {
>  		ret = sram_add_pool(sram, block, start, part);
> @@ -198,6 +215,7 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
>  
>  		block->start = child_res.start - res->start;
>  		block->size = resource_size(&child_res);
> +		block->res = child_res;
>  		list_add_tail(&block->list, &reserve_list);
>  
>  		if (of_find_property(child, "export", NULL))
> @@ -295,15 +313,17 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
>  		 */
>  		cur_size = block->start - cur_start;
>  
> -		dev_dbg(sram->dev, "adding chunk 0x%lx-0x%lx\n",
> -			cur_start, cur_start + cur_size);
> +		if (sram->pool) {
> +			dev_dbg(sram->dev, "adding chunk 0x%lx-0x%lx\n",
> +				cur_start, cur_start + cur_size);
>  
> -		ret = gen_pool_add_virt(sram->pool,
> -				(unsigned long)sram->virt_base + cur_start,
> -				res->start + cur_start, cur_size, -1);
> -		if (ret < 0) {
> -			sram_free_partitions(sram);
> -			goto err_chunks;
> +			ret = gen_pool_add_virt(sram->pool,
> +					(unsigned long)sram->virt_base + cur_start,
> +					res->start + cur_start, cur_size, -1);
> +			if (ret < 0) {
> +				sram_free_partitions(sram);
> +				goto err_chunks;
> +			}
>  		}
>  
>  		/* next allocation after this reserved block */
> @@ -331,40 +351,66 @@ static int atmel_securam_wait(void)
>  					10000, 500000);
>  }
>  
> +static const struct sram_config default_config = {
> +};
> +
> +static const struct sram_config atmel_securam_config = {
> +	.init = atmel_securam_wait
> +};
> +
> +/*
> + * SYSRAM contains areas that are not accessible by the
> + * kernel, such as the first 256K that is reserved for TZ.
> + * Accesses to those areas (including speculative accesses)
> + * trigger SErrors. As such we must map only the areas of
> + * SYSRAM specified in the device tree.
> + */
> +static const struct sram_config tegra_sysram_config = {
> +	.map_only_reserved = true,

In case of Tegra sram base is 64K aligned and the reserved partitions
are 4K aligned. How this flag will work if the kernel is using 64K
page size?

> +};
> +
>  static const struct of_device_id sram_dt_ids[] = {
> -	{ .compatible = "mmio-sram" },
> -	{ .compatible = "atmel,sama5d2-securam", .data = atmel_securam_wait },
> +	{ .compatible = "mmio-sram", .data = &default_config },
> +	{ .compatible = "atmel,sama5d2-securam", .data = &atmel_securam_config },
> +	{ .compatible = "nvidia,tegra186-sysram", .data = &tegra_sysram_config },
> +	{ .compatible = "nvidia,tegra194-sysram", .data = &tegra_sysram_config },
>  	{}
>  };
>  
>  static int sram_probe(struct platform_device *pdev)
>  {
> +	const struct sram_config *config;
>  	struct sram_dev *sram;
>  	int ret;
>  	struct resource *res;
> -	int (*init_func)(void);
> +
> +	config = of_device_get_match_data(&pdev->dev);
>  
>  	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
>  	if (!sram)
>  		return -ENOMEM;
>  
>  	sram->dev = &pdev->dev;
> +	sram->no_memory_wc = of_property_read_bool(pdev->dev.of_node, "no-memory-wc");
> +	sram->config = config;
> +
> +	if (!config->map_only_reserved) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (sram->no_memory_wc)
> +			sram->virt_base = devm_ioremap_resource(&pdev->dev, res);
> +		else
> +			sram->virt_base = devm_ioremap_resource_wc(&pdev->dev, res);
> +		if (IS_ERR(sram->virt_base)) {
> +			dev_err(&pdev->dev, "could not map SRAM registers\n");
> +			return PTR_ERR(sram->virt_base);
> +		}
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (of_property_read_bool(pdev->dev.of_node, "no-memory-wc"))
> -		sram->virt_base = devm_ioremap_resource(&pdev->dev, res);
> -	else
> -		sram->virt_base = devm_ioremap_resource_wc(&pdev->dev, res);
> -	if (IS_ERR(sram->virt_base)) {
> -		dev_err(&pdev->dev, "could not map SRAM registers\n");
> -		return PTR_ERR(sram->virt_base);
> +		sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
> +						  NUMA_NO_NODE, NULL);
> +		if (IS_ERR(sram->pool))
> +			return PTR_ERR(sram->pool);
>  	}
>  
> -	sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
> -					  NUMA_NO_NODE, NULL);
> -	if (IS_ERR(sram->pool))
> -		return PTR_ERR(sram->pool);
> -
>  	sram->clk = devm_clk_get(sram->dev, NULL);
>  	if (IS_ERR(sram->clk))
>  		sram->clk = NULL;
> @@ -378,15 +424,15 @@ static int sram_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, sram);
>  
> -	init_func = of_device_get_match_data(&pdev->dev);
> -	if (init_func) {
> -		ret = init_func();
> +	if (config->init) {
> +		ret = config->init();
>  		if (ret)
>  			goto err_free_partitions;
>  	}
>  
> -	dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
> -		gen_pool_size(sram->pool) / 1024, sram->virt_base);
> +	if (sram->pool)
> +		dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
> +			gen_pool_size(sram->pool) / 1024, sram->virt_base);
>  
>  	return 0;
>  
> @@ -405,7 +451,7 @@ static int sram_remove(struct platform_device *pdev)
>  
>  	sram_free_partitions(sram);
>  
> -	if (gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
> +	if (sram->pool && gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
>  		dev_err(sram->dev, "removed while SRAM allocated\n");
>  
>  	if (sram->clk)
> diff --git a/drivers/misc/sram.h b/drivers/misc/sram.h
> index 9c1d21ff7347..d2058d8c8f1d 100644
> --- a/drivers/misc/sram.h
> +++ b/drivers/misc/sram.h
> @@ -5,6 +5,11 @@
>  #ifndef __SRAM_H
>  #define __SRAM_H
>  
> +struct sram_config {
> +	int (*init)(void);
> +	bool map_only_reserved;
> +};
> +
>  struct sram_partition {
>  	void __iomem *base;
>  
> @@ -15,8 +20,11 @@ struct sram_partition {
>  };
>  
>  struct sram_dev {
> +	const struct sram_config *config;
> +
>  	struct device *dev;
>  	void __iomem *virt_base;
> +	bool no_memory_wc;
>  
>  	struct gen_pool *pool;
>  	struct clk *clk;
> @@ -29,6 +37,7 @@ struct sram_reserve {
>  	struct list_head list;
>  	u32 start;
>  	u32 size;
> +	struct resource res;
>  	bool export;
>  	bool pool;
>  	bool protect_exec;
> -- 
> 2.30.1

BR,
Yousaf

Re: [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM

[Mikko Perttunen]

On 6/18/21 11:18 AM, Mian Yousaf Kaukab wrote:
> On Thu, Jun 17, 2021 at 12:48:04PM +0300, Mikko Perttunen wrote:
>> On Tegra186 and later, a portion of the SYSRAM may be reserved for use
>> by TZ. Non-TZ memory accesses to this portion, including speculative
>> accesses, trigger SErrors that bring down the system. This does also
>> happen in practice occasionally (due to speculative accesses).
>>
>> To fix the issue, add a flag to the SRAM driver to only map the
>> device tree-specified reserved areas depending on a flag set
>> based on the compatibility string. This would not affect non-Tegra
>> systems that rely on the entire thing being memory mapped.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>>   drivers/misc/sram.c | 108 +++++++++++++++++++++++++++++++-------------
>>   drivers/misc/sram.h |   9 ++++
>>   2 files changed, 86 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
>> index 93638ae2753a..a27ffb3dbea8 100644
>> --- a/drivers/misc/sram.c
>> +++ b/drivers/misc/sram.c
>> @@ -97,7 +97,24 @@ static int sram_add_partition(struct sram_dev *sram, struct sram_reserve *block,
>>   	struct sram_partition *part = &sram->partition[sram->partitions];
>>   
>>   	mutex_init(&part->lock);
>> -	part->base = sram->virt_base + block->start;
>> +
>> +	if (sram->config->map_only_reserved) {
>> +		void *virt_base;
>> +
>> +		if (sram->no_memory_wc)
>> +			virt_base = devm_ioremap_resource(sram->dev, &block->res);
>> +		else
>> +			virt_base = devm_ioremap_resource_wc(sram->dev, &block->res);
>> +
>> +		if (IS_ERR(virt_base)) {
>> +			dev_err(sram->dev, "could not map SRAM at %pr\n", &block->res);
>> +			return PTR_ERR(virt_base);
>> +		}
>> +
>> +		part->base = virt_base;
>> +	} else {
>> +		part->base = sram->virt_base + block->start;
>> +	}
>>   
>>   	if (block->pool) {
>>   		ret = sram_add_pool(sram, block, start, part);
>> @@ -198,6 +215,7 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
>>   
>>   		block->start = child_res.start - res->start;
>>   		block->size = resource_size(&child_res);
>> +		block->res = child_res;
>>   		list_add_tail(&block->list, &reserve_list);
>>   
>>   		if (of_find_property(child, "export", NULL))
>> @@ -295,15 +313,17 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
>>   		 */
>>   		cur_size = block->start - cur_start;
>>   
>> -		dev_dbg(sram->dev, "adding chunk 0x%lx-0x%lx\n",
>> -			cur_start, cur_start + cur_size);
>> +		if (sram->pool) {
>> +			dev_dbg(sram->dev, "adding chunk 0x%lx-0x%lx\n",
>> +				cur_start, cur_start + cur_size);
>>   
>> -		ret = gen_pool_add_virt(sram->pool,
>> -				(unsigned long)sram->virt_base + cur_start,
>> -				res->start + cur_start, cur_size, -1);
>> -		if (ret < 0) {
>> -			sram_free_partitions(sram);
>> -			goto err_chunks;
>> +			ret = gen_pool_add_virt(sram->pool,
>> +					(unsigned long)sram->virt_base + cur_start,
>> +					res->start + cur_start, cur_size, -1);
>> +			if (ret < 0) {
>> +				sram_free_partitions(sram);
>> +				goto err_chunks;
>> +			}
>>   		}
>>   
>>   		/* next allocation after this reserved block */
>> @@ -331,40 +351,66 @@ static int atmel_securam_wait(void)
>>   					10000, 500000);
>>   }
>>   
>> +static const struct sram_config default_config = {
>> +};
>> +
>> +static const struct sram_config atmel_securam_config = {
>> +	.init = atmel_securam_wait
>> +};
>> +
>> +/*
>> + * SYSRAM contains areas that are not accessible by the
>> + * kernel, such as the first 256K that is reserved for TZ.
>> + * Accesses to those areas (including speculative accesses)
>> + * trigger SErrors. As such we must map only the areas of
>> + * SYSRAM specified in the device tree.
>> + */
>> +static const struct sram_config tegra_sysram_config = {
>> +	.map_only_reserved = true,
> 
> In case of Tegra sram base is 64K aligned and the reserved partitions
> are 4K aligned. How this flag will work if the kernel is using 64K
> page size?

Good point - perhaps we need to consider another approach that just 
excludes any inaccessible areas, though I think it'll be somewhat more 
complicated and more Tegra-specific.

I'm going on vacation starting this evening so I'll look into this 
afterwards.

Thanks,
Mikko

> 
>> +};
>> +
>>   static const struct of_device_id sram_dt_ids[] = {
>> -	{ .compatible = "mmio-sram" },
>> -	{ .compatible = "atmel,sama5d2-securam", .data = atmel_securam_wait },
>> +	{ .compatible = "mmio-sram", .data = &default_config },
>> +	{ .compatible = "atmel,sama5d2-securam", .data = &atmel_securam_config },
>> +	{ .compatible = "nvidia,tegra186-sysram", .data = &tegra_sysram_config },
>> +	{ .compatible = "nvidia,tegra194-sysram", .data = &tegra_sysram_config },
>>   	{}
>>   };
>>   
>>   static int sram_probe(struct platform_device *pdev)
>>   {
>> +	const struct sram_config *config;
>>   	struct sram_dev *sram;
>>   	int ret;
>>   	struct resource *res;
>> -	int (*init_func)(void);
>> +
>> +	config = of_device_get_match_data(&pdev->dev);
>>   
>>   	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
>>   	if (!sram)
>>   		return -ENOMEM;
>>   
>>   	sram->dev = &pdev->dev;
>> +	sram->no_memory_wc = of_property_read_bool(pdev->dev.of_node, "no-memory-wc");
>> +	sram->config = config;
>> +
>> +	if (!config->map_only_reserved) {
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +		if (sram->no_memory_wc)
>> +			sram->virt_base = devm_ioremap_resource(&pdev->dev, res);
>> +		else
>> +			sram->virt_base = devm_ioremap_resource_wc(&pdev->dev, res);
>> +		if (IS_ERR(sram->virt_base)) {
>> +			dev_err(&pdev->dev, "could not map SRAM registers\n");
>> +			return PTR_ERR(sram->virt_base);
>> +		}
>>   
>> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> -	if (of_property_read_bool(pdev->dev.of_node, "no-memory-wc"))
>> -		sram->virt_base = devm_ioremap_resource(&pdev->dev, res);
>> -	else
>> -		sram->virt_base = devm_ioremap_resource_wc(&pdev->dev, res);
>> -	if (IS_ERR(sram->virt_base)) {
>> -		dev_err(&pdev->dev, "could not map SRAM registers\n");
>> -		return PTR_ERR(sram->virt_base);
>> +		sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
>> +						  NUMA_NO_NODE, NULL);
>> +		if (IS_ERR(sram->pool))
>> +			return PTR_ERR(sram->pool);
>>   	}
>>   
>> -	sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
>> -					  NUMA_NO_NODE, NULL);
>> -	if (IS_ERR(sram->pool))
>> -		return PTR_ERR(sram->pool);
>> -
>>   	sram->clk = devm_clk_get(sram->dev, NULL);
>>   	if (IS_ERR(sram->clk))
>>   		sram->clk = NULL;
>> @@ -378,15 +424,15 @@ static int sram_probe(struct platform_device *pdev)
>>   
>>   	platform_set_drvdata(pdev, sram);
>>   
>> -	init_func = of_device_get_match_data(&pdev->dev);
>> -	if (init_func) {
>> -		ret = init_func();
>> +	if (config->init) {
>> +		ret = config->init();
>>   		if (ret)
>>   			goto err_free_partitions;
>>   	}
>>   
>> -	dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
>> -		gen_pool_size(sram->pool) / 1024, sram->virt_base);
>> +	if (sram->pool)
>> +		dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
>> +			gen_pool_size(sram->pool) / 1024, sram->virt_base);
>>   
>>   	return 0;
>>   
>> @@ -405,7 +451,7 @@ static int sram_remove(struct platform_device *pdev)
>>   
>>   	sram_free_partitions(sram);
>>   
>> -	if (gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
>> +	if (sram->pool && gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
>>   		dev_err(sram->dev, "removed while SRAM allocated\n");
>>   
>>   	if (sram->clk)
>> diff --git a/drivers/misc/sram.h b/drivers/misc/sram.h
>> index 9c1d21ff7347..d2058d8c8f1d 100644
>> --- a/drivers/misc/sram.h
>> +++ b/drivers/misc/sram.h
>> @@ -5,6 +5,11 @@
>>   #ifndef __SRAM_H
>>   #define __SRAM_H
>>   
>> +struct sram_config {
>> +	int (*init)(void);
>> +	bool map_only_reserved;
>> +};
>> +
>>   struct sram_partition {
>>   	void __iomem *base;
>>   
>> @@ -15,8 +20,11 @@ struct sram_partition {
>>   };
>>   
>>   struct sram_dev {
>> +	const struct sram_config *config;
>> +
>>   	struct device *dev;
>>   	void __iomem *virt_base;
>> +	bool no_memory_wc;
>>   
>>   	struct gen_pool *pool;
>>   	struct clk *clk;
>> @@ -29,6 +37,7 @@ struct sram_reserve {
>>   	struct list_head list;
>>   	u32 start;
>>   	u32 size;
>> +	struct resource res;
>>   	bool export;
>>   	bool pool;
>>   	bool protect_exec;
>> -- 
>> 2.30.1
> 
> BR,
> Yousaf
> 

Re: [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM

[Mian Yousaf Kaukab]

On Fri, Jun 18, 2021 at 12:26:21PM +0300, Mikko Perttunen wrote:
> On 6/18/21 11:18 AM, Mian Yousaf Kaukab wrote:
> > On Thu, Jun 17, 2021 at 12:48:04PM +0300, Mikko Perttunen wrote:
> > > On Tegra186 and later, a portion of the SYSRAM may be reserved for use
> > > by TZ. Non-TZ memory accesses to this portion, including speculative
> > > accesses, trigger SErrors that bring down the system. This does also
> > > happen in practice occasionally (due to speculative accesses).
> > > 
> > > To fix the issue, add a flag to the SRAM driver to only map the
> > > device tree-specified reserved areas depending on a flag set
> > > based on the compatibility string. This would not affect non-Tegra
> > > systems that rely on the entire thing being memory mapped.
> > > 
> > > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> > > ---
> > >   drivers/misc/sram.c | 108 +++++++++++++++++++++++++++++++-------------
> > >   drivers/misc/sram.h |   9 ++++
> > >   2 files changed, 86 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> > > index 93638ae2753a..a27ffb3dbea8 100644
> > > --- a/drivers/misc/sram.c
> > > +++ b/drivers/misc/sram.c
> > > @@ -97,7 +97,24 @@ static int sram_add_partition(struct sram_dev *sram, struct sram_reserve *block,
> > >   	struct sram_partition *part = &sram->partition[sram->partitions];
> > >   	mutex_init(&part->lock);
> > > -	part->base = sram->virt_base + block->start;
> > > +
> > > +	if (sram->config->map_only_reserved) {
> > > +		void *virt_base;
> > > +
> > > +		if (sram->no_memory_wc)
> > > +			virt_base = devm_ioremap_resource(sram->dev, &block->res);
> > > +		else
> > > +			virt_base = devm_ioremap_resource_wc(sram->dev, &block->res);
> > > +
> > > +		if (IS_ERR(virt_base)) {
> > > +			dev_err(sram->dev, "could not map SRAM at %pr\n", &block->res);
> > > +			return PTR_ERR(virt_base);
> > > +		}
> > > +
> > > +		part->base = virt_base;
> > > +	} else {
> > > +		part->base = sram->virt_base + block->start;
> > > +	}
> > >   	if (block->pool) {
> > >   		ret = sram_add_pool(sram, block, start, part);
> > > @@ -198,6 +215,7 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
> > >   		block->start = child_res.start - res->start;
> > >   		block->size = resource_size(&child_res);
> > > +		block->res = child_res;
> > >   		list_add_tail(&block->list, &reserve_list);
> > >   		if (of_find_property(child, "export", NULL))
> > > @@ -295,15 +313,17 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
> > >   		 */
> > >   		cur_size = block->start - cur_start;
> > > -		dev_dbg(sram->dev, "adding chunk 0x%lx-0x%lx\n",
> > > -			cur_start, cur_start + cur_size);
> > > +		if (sram->pool) {
> > > +			dev_dbg(sram->dev, "adding chunk 0x%lx-0x%lx\n",
> > > +				cur_start, cur_start + cur_size);
> > > -		ret = gen_pool_add_virt(sram->pool,
> > > -				(unsigned long)sram->virt_base + cur_start,
> > > -				res->start + cur_start, cur_size, -1);
> > > -		if (ret < 0) {
> > > -			sram_free_partitions(sram);
> > > -			goto err_chunks;
> > > +			ret = gen_pool_add_virt(sram->pool,
> > > +					(unsigned long)sram->virt_base + cur_start,
> > > +					res->start + cur_start, cur_size, -1);
> > > +			if (ret < 0) {
> > > +				sram_free_partitions(sram);
> > > +				goto err_chunks;
> > > +			}
> > >   		}
> > >   		/* next allocation after this reserved block */
> > > @@ -331,40 +351,66 @@ static int atmel_securam_wait(void)
> > >   					10000, 500000);
> > >   }
> > > +static const struct sram_config default_config = {
> > > +};
> > > +
> > > +static const struct sram_config atmel_securam_config = {
> > > +	.init = atmel_securam_wait
> > > +};
> > > +
> > > +/*
> > > + * SYSRAM contains areas that are not accessible by the
> > > + * kernel, such as the first 256K that is reserved for TZ.
> > > + * Accesses to those areas (including speculative accesses)
> > > + * trigger SErrors. As such we must map only the areas of
> > > + * SYSRAM specified in the device tree.
> > > + */
> > > +static const struct sram_config tegra_sysram_config = {
> > > +	.map_only_reserved = true,
> > 
> > In case of Tegra sram base is 64K aligned and the reserved partitions
> > are 4K aligned. How this flag will work if the kernel is using 64K
> > page size?
> 
> Good point - perhaps we need to consider another approach that just excludes
> any inaccessible areas, though I think it'll be somewhat more complicated
> and more Tegra-specific.
Perhaps SError due to speculative access can be avoided by just using
no-memory-wc, provided the performance impact due to this is OK for
bpmp driver.
> 
> I'm going on vacation starting this evening so I'll look into this
> afterwards.
Enjoy your vacations!

> 
> Thanks,
> Mikko

BR,
Yousaf
> 
> > 
> > > +};
> > > +
> > >   static const struct of_device_id sram_dt_ids[] = {
> > > -	{ .compatible = "mmio-sram" },
> > > -	{ .compatible = "atmel,sama5d2-securam", .data = atmel_securam_wait },
> > > +	{ .compatible = "mmio-sram", .data = &default_config },
> > > +	{ .compatible = "atmel,sama5d2-securam", .data = &atmel_securam_config },
> > > +	{ .compatible = "nvidia,tegra186-sysram", .data = &tegra_sysram_config },
> > > +	{ .compatible = "nvidia,tegra194-sysram", .data = &tegra_sysram_config },
> > >   	{}
> > >   };
> > >   static int sram_probe(struct platform_device *pdev)
> > >   {
> > > +	const struct sram_config *config;
> > >   	struct sram_dev *sram;
> > >   	int ret;
> > >   	struct resource *res;
> > > -	int (*init_func)(void);
> > > +
> > > +	config = of_device_get_match_data(&pdev->dev);
> > >   	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
> > >   	if (!sram)
> > >   		return -ENOMEM;
> > >   	sram->dev = &pdev->dev;
> > > +	sram->no_memory_wc = of_property_read_bool(pdev->dev.of_node, "no-memory-wc");
> > > +	sram->config = config;
> > > +
> > > +	if (!config->map_only_reserved) {
> > > +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +		if (sram->no_memory_wc)
> > > +			sram->virt_base = devm_ioremap_resource(&pdev->dev, res);
> > > +		else
> > > +			sram->virt_base = devm_ioremap_resource_wc(&pdev->dev, res);
> > > +		if (IS_ERR(sram->virt_base)) {
> > > +			dev_err(&pdev->dev, "could not map SRAM registers\n");
> > > +			return PTR_ERR(sram->virt_base);
> > > +		}
> > > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > -	if (of_property_read_bool(pdev->dev.of_node, "no-memory-wc"))
> > > -		sram->virt_base = devm_ioremap_resource(&pdev->dev, res);
> > > -	else
> > > -		sram->virt_base = devm_ioremap_resource_wc(&pdev->dev, res);
> > > -	if (IS_ERR(sram->virt_base)) {
> > > -		dev_err(&pdev->dev, "could not map SRAM registers\n");
> > > -		return PTR_ERR(sram->virt_base);
> > > +		sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
> > > +						  NUMA_NO_NODE, NULL);
> > > +		if (IS_ERR(sram->pool))
> > > +			return PTR_ERR(sram->pool);
> > >   	}
> > > -	sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
> > > -					  NUMA_NO_NODE, NULL);
> > > -	if (IS_ERR(sram->pool))
> > > -		return PTR_ERR(sram->pool);
> > > -
> > >   	sram->clk = devm_clk_get(sram->dev, NULL);
> > >   	if (IS_ERR(sram->clk))
> > >   		sram->clk = NULL;
> > > @@ -378,15 +424,15 @@ static int sram_probe(struct platform_device *pdev)
> > >   	platform_set_drvdata(pdev, sram);
> > > -	init_func = of_device_get_match_data(&pdev->dev);
> > > -	if (init_func) {
> > > -		ret = init_func();
> > > +	if (config->init) {
> > > +		ret = config->init();
> > >   		if (ret)
> > >   			goto err_free_partitions;
> > >   	}
> > > -	dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
> > > -		gen_pool_size(sram->pool) / 1024, sram->virt_base);
> > > +	if (sram->pool)
> > > +		dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
> > > +			gen_pool_size(sram->pool) / 1024, sram->virt_base);
> > >   	return 0;
> > > @@ -405,7 +451,7 @@ static int sram_remove(struct platform_device *pdev)
> > >   	sram_free_partitions(sram);
> > > -	if (gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
> > > +	if (sram->pool && gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
> > >   		dev_err(sram->dev, "removed while SRAM allocated\n");
> > >   	if (sram->clk)
> > > diff --git a/drivers/misc/sram.h b/drivers/misc/sram.h
> > > index 9c1d21ff7347..d2058d8c8f1d 100644
> > > --- a/drivers/misc/sram.h
> > > +++ b/drivers/misc/sram.h
> > > @@ -5,6 +5,11 @@
> > >   #ifndef __SRAM_H
> > >   #define __SRAM_H
> > > +struct sram_config {
> > > +	int (*init)(void);
> > > +	bool map_only_reserved;
> > > +};
> > > +
> > >   struct sram_partition {
> > >   	void __iomem *base;
> > > @@ -15,8 +20,11 @@ struct sram_partition {
> > >   };
> > >   struct sram_dev {
> > > +	const struct sram_config *config;
> > > +
> > >   	struct device *dev;
> > >   	void __iomem *virt_base;
> > > +	bool no_memory_wc;
> > >   	struct gen_pool *pool;
> > >   	struct clk *clk;
> > > @@ -29,6 +37,7 @@ struct sram_reserve {
> > >   	struct list_head list;
> > >   	u32 start;
> > >   	u32 size;
> > > +	struct resource res;
> > >   	bool export;
> > >   	bool pool;
> > >   	bool protect_exec;
> > > -- 
> > > 2.30.1
> > 
> > BR,
> > Yousaf
> >