* Re: [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM
2021-06-17 9:48 [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM Mikko Perttunen
@ 2021-06-17 11:00 ` Greg KH
2021-06-17 11:40 ` Mikko Perttunen
2021-06-18 4:10 ` kernel test robot
2021-06-18 8:18 ` Mian Yousaf Kaukab
2 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2021-06-17 11:00 UTC (permalink / raw)
To: Mikko Perttunen; +Cc: arnd, ykaukab, linux-tegra, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM
2021-06-17 11:00 ` Greg KH
@ 2021-06-17 11:40 ` Mikko Perttunen
0 siblings, 0 replies; 7+ messages in thread
From: Mikko Perttunen @ 2021-06-17 11:40 UTC (permalink / raw)
To: Greg KH, Mikko Perttunen; +Cc: arnd, ykaukab, linux-tegra, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM
2021-06-17 9:48 [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM Mikko Perttunen
2021-06-17 11:00 ` Greg KH
@ 2021-06-18 4:10 ` kernel test robot
2021-06-18 8:18 ` Mian Yousaf Kaukab
2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2021-06-18 4:10 UTC (permalink / raw)
To: Mikko Perttunen, arnd, gregkh
Cc: kbuild-all, ykaukab, linux-tegra, linux-kernel, Mikko Perttunen
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM
2021-06-17 9:48 [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM Mikko Perttunen
2021-06-17 11:00 ` Greg KH
2021-06-18 4:10 ` kernel test robot
@ 2021-06-18 8:18 ` Mian Yousaf Kaukab
2021-06-18 9:26 ` Mikko Perttunen
2 siblings, 1 reply; 7+ messages in thread
From: Mian Yousaf Kaukab @ 2021-06-18 8:18 UTC (permalink / raw)
To: Mikko Perttunen; +Cc: arnd, gregkh, linux-tegra, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM
2021-06-18 8:18 ` Mian Yousaf Kaukab
@ 2021-06-18 9:26 ` Mikko Perttunen
2021-06-18 13:51 ` Mian Yousaf Kaukab
0 siblings, 1 reply; 7+ messages in thread
From: Mikko Perttunen @ 2021-06-18 9:26 UTC (permalink / raw)
To: Mian Yousaf Kaukab, Mikko Perttunen
Cc: arnd, gregkh, linux-tegra, linux-kernel
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] misc: sram: Only map reserved areas in Tegra SYSRAM
2021-06-18 9:26 ` Mikko Perttunen
@ 2021-06-18 13:51 ` Mian Yousaf Kaukab
0 siblings, 0 replies; 7+ messages in thread
From: Mian Yousaf Kaukab @ 2021-06-18 13:51 UTC (permalink / raw)
To: Mikko Perttunen; +Cc: Mikko Perttunen, arnd, gregkh, linux-tegra, linux-kernel
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
> >
^ permalink raw reply [flat|nested] 7+ messages in thread