2023-09-28 17:47:26

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

From: Bartosz Golaszewski <[email protected]>

Extens the SCM memory allocator with using the SHM Bridge feature if
available on the platform. This makes the trustzone only use dedicated
buffers for SCM calls. We map the entire SCM genpool as a bridge.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
index eafecbe23770..12b12b15f46f 100644
--- a/drivers/firmware/qcom/qcom_scm-mem.c
+++ b/drivers/firmware/qcom/qcom_scm-mem.c
@@ -16,6 +16,8 @@

#include "qcom_scm.h"

+#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
+
static size_t qcom_scm_mem_pool_size = SZ_2M;
module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
ulong, 0400);
@@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
return chunk->paddr;
}

+static int qcom_scm_mem_shm_bridge_create(void)
+{
+ uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
+
+ ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
+ pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
+ ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
+ size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
+
+ return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
+ ipfn_and_s_perm, size_and_flags,
+ QCOM_SCM_VMID_HLOS);
+}
+
int qcom_scm_mem_enable(struct device *dev)
{
+ int ret;
+
INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
spin_lock_init(&qcom_scm_mem.lock);
qcom_scm_mem.dev = dev;
@@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)

gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);

- return gen_pool_add_virt(qcom_scm_mem.pool,
- (unsigned long)qcom_scm_mem.vbase,
- qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
+ ret = gen_pool_add_virt(qcom_scm_mem.pool,
+ (unsigned long)qcom_scm_mem.vbase,
+ qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
+ if (ret)
+ return ret;
+
+ ret = qcom_scm_enable_shm_bridge();
+ if (ret) {
+ if (ret == EOPNOTSUPP)
+ dev_info(dev, "SHM Bridge not supported\n");
+ else
+ return ret;
+ } else {
+ ret = qcom_scm_mem_shm_bridge_create();
+ if (ret)
+ return ret;
+
+ dev_info(dev, "SHM Bridge enabled\n");
+ }
+
+ return 0;
}
--
2.39.2


2023-09-28 21:00:26

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

On Thu, Sep 28, 2023 at 7:10 PM Elliot Berman <[email protected]> wrote:
>
>
>
> On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > Extens the SCM memory allocator with using the SHM Bridge feature if
> > available on the platform. This makes the trustzone only use dedicated
> > buffers for SCM calls. We map the entire SCM genpool as a bridge.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > ---
> > drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
> > 1 file changed, 39 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
> > index eafecbe23770..12b12b15f46f 100644
> > --- a/drivers/firmware/qcom/qcom_scm-mem.c
> > +++ b/drivers/firmware/qcom/qcom_scm-mem.c
> > @@ -16,6 +16,8 @@
> >
> > #include "qcom_scm.h"
> >
> > +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> > +
> > static size_t qcom_scm_mem_pool_size = SZ_2M;
> > module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
> > ulong, 0400);
> > @@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
> > return chunk->paddr;
> > }
> >
> > +static int qcom_scm_mem_shm_bridge_create(void)
> > +{
> > + uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
> > +
> > + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> > + pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> > + ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> > + size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> > +
> > + return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
> > + ipfn_and_s_perm, size_and_flags,
> > + QCOM_SCM_VMID_HLOS);
> > +}
> > +
> > int qcom_scm_mem_enable(struct device *dev)
> > {
> > + int ret;
> > +
> > INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
> > spin_lock_init(&qcom_scm_mem.lock);
> > qcom_scm_mem.dev = dev;
> > @@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
> >
> > gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
> >
> > - return gen_pool_add_virt(qcom_scm_mem.pool,
> > - (unsigned long)qcom_scm_mem.vbase,
> > - qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> > + ret = gen_pool_add_virt(qcom_scm_mem.pool,
> > + (unsigned long)qcom_scm_mem.vbase,
> > + qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> > + if (ret)
> > + return ret;
> > +
> > + ret = qcom_scm_enable_shm_bridge();
> > + if (ret) {
> > + if (ret == EOPNOTSUPP)
> > + dev_info(dev, "SHM Bridge not supported\n");
> > + else
> > + return ret;
> > + } else {
> > + ret = qcom_scm_mem_shm_bridge_create();
> > + if (ret)
> > + return ret;
> > +
> > + dev_info(dev, "SHM Bridge enabled\n");
>
> Do you need to add clean up (deletion) of the SHM bridge on driver remove?
>
> One easy approach I could think: implemnet devm_qcom_scm_mem_shm_bridge_create
> which calls qcom_scm_delete_shm_bridge on the clean up
> (qcom_scm_delete_shm_bridge implemented in downstream, not in this series).
>

There wouldn't be any user of these symbols yet so let's think about
it when there's a valid use-case upstream.

Bart

> > + }
> > +
> > + return 0;
> > }

2023-09-29 01:00:37

by Jeff Johnson

[permalink] [raw]
Subject: Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Extens the SCM memory allocator with using the SHM Bridge feature if

nit: s/Extens/Extend/

2023-09-29 01:41:13

by Elliot Berman

[permalink] [raw]
Subject: Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge



On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Extens the SCM memory allocator with using the SHM Bridge feature if
> available on the platform. This makes the trustzone only use dedicated
> buffers for SCM calls. We map the entire SCM genpool as a bridge.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
> 1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
> index eafecbe23770..12b12b15f46f 100644
> --- a/drivers/firmware/qcom/qcom_scm-mem.c
> +++ b/drivers/firmware/qcom/qcom_scm-mem.c
> @@ -16,6 +16,8 @@
>
> #include "qcom_scm.h"
>
> +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> +
> static size_t qcom_scm_mem_pool_size = SZ_2M;
> module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
> ulong, 0400);
> @@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
> return chunk->paddr;
> }
>
> +static int qcom_scm_mem_shm_bridge_create(void)
> +{
> + uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
> +
> + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> + pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> + ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> + size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> +
> + return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
> + ipfn_and_s_perm, size_and_flags,
> + QCOM_SCM_VMID_HLOS);
> +}
> +
> int qcom_scm_mem_enable(struct device *dev)
> {
> + int ret;
> +
> INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
> spin_lock_init(&qcom_scm_mem.lock);
> qcom_scm_mem.dev = dev;
> @@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
>
> gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
>
> - return gen_pool_add_virt(qcom_scm_mem.pool,
> - (unsigned long)qcom_scm_mem.vbase,
> - qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> + ret = gen_pool_add_virt(qcom_scm_mem.pool,
> + (unsigned long)qcom_scm_mem.vbase,
> + qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> + if (ret)
> + return ret;
> +
> + ret = qcom_scm_enable_shm_bridge();
> + if (ret) {
> + if (ret == EOPNOTSUPP)
> + dev_info(dev, "SHM Bridge not supported\n");
> + else
> + return ret;
> + } else {
> + ret = qcom_scm_mem_shm_bridge_create();
> + if (ret)
> + return ret;
> +
> + dev_info(dev, "SHM Bridge enabled\n");

Do you need to add clean up (deletion) of the SHM bridge on driver remove?

One easy approach I could think: implemnet devm_qcom_scm_mem_shm_bridge_create
which calls qcom_scm_delete_shm_bridge on the clean up
(qcom_scm_delete_shm_bridge implemented in downstream, not in this series).

> + }
> +
> + return 0;
> }

2023-09-30 01:30:41

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

On Thu, Sep 28, 2023 at 11:21 AM Bartosz Golaszewski <[email protected]> wrote:
>
> From: Bartosz Golaszewski <[email protected]>
>
> Extens the SCM memory allocator with using the SHM Bridge feature if
> available on the platform. This makes the trustzone only use dedicated
> buffers for SCM calls. We map the entire SCM genpool as a bridge.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
> 1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
> index eafecbe23770..12b12b15f46f 100644
> --- a/drivers/firmware/qcom/qcom_scm-mem.c
> +++ b/drivers/firmware/qcom/qcom_scm-mem.c
> @@ -16,6 +16,8 @@
>
> #include "qcom_scm.h"
>
> +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> +
> static size_t qcom_scm_mem_pool_size = SZ_2M;
> module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
> ulong, 0400);
> @@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
> return chunk->paddr;
> }
>
> +static int qcom_scm_mem_shm_bridge_create(void)
> +{
> + uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
> +
> + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> + pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> + ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> + size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> +
> + return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
> + ipfn_and_s_perm, size_and_flags,
> + QCOM_SCM_VMID_HLOS);
> +}
> +
> int qcom_scm_mem_enable(struct device *dev)
> {
> + int ret;
> +
> INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
> spin_lock_init(&qcom_scm_mem.lock);
> qcom_scm_mem.dev = dev;
> @@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
>
> gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
>
> - return gen_pool_add_virt(qcom_scm_mem.pool,
> - (unsigned long)qcom_scm_mem.vbase,
> - qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> + ret = gen_pool_add_virt(qcom_scm_mem.pool,
> + (unsigned long)qcom_scm_mem.vbase,
> + qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> + if (ret)
> + return ret;
> +
> + ret = qcom_scm_enable_shm_bridge();
> + if (ret) {
> + if (ret == EOPNOTSUPP)

FYI I noticed this is wrong, I will fix it in v3.

Bart

> + dev_info(dev, "SHM Bridge not supported\n");
> + else
> + return ret;
> + } else {
> + ret = qcom_scm_mem_shm_bridge_create();
> + if (ret)
> + return ret;
> +
> + dev_info(dev, "SHM Bridge enabled\n");
> + }
> +
> + return 0;
> }
> --
> 2.39.2
>

2023-10-04 22:26:02

by Maximilian Luz

[permalink] [raw]
Subject: Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

On 9/28/23 11:20, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Extens the SCM memory allocator with using the SHM Bridge feature if
> available on the platform. This makes the trustzone only use dedicated
> buffers for SCM calls. We map the entire SCM genpool as a bridge.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>

This patch breaks something in early boot on my Surface Pro X (sc8180x).
Unfortunately I can't provide many details at the moment because the
only thing I can see are RCU stalls, and the traces from them are quite
useless.

Without this patch, the rest of the series (with the fix you posted on
patch 6 applied) seems to work fine. Including both RFT qseecom patches.

I plan to have a closer look at this once I have some more time though.

Regards,
Max

> ---
> drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
> 1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
> index eafecbe23770..12b12b15f46f 100644
> --- a/drivers/firmware/qcom/qcom_scm-mem.c
> +++ b/drivers/firmware/qcom/qcom_scm-mem.c
> @@ -16,6 +16,8 @@
>
> #include "qcom_scm.h"
>
> +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> +
> static size_t qcom_scm_mem_pool_size = SZ_2M;
> module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
> ulong, 0400);
> @@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
> return chunk->paddr;
> }
>
> +static int qcom_scm_mem_shm_bridge_create(void)
> +{
> + uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
> +
> + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> + pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> + ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> + size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> +
> + return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
> + ipfn_and_s_perm, size_and_flags,
> + QCOM_SCM_VMID_HLOS);
> +}
> +
> int qcom_scm_mem_enable(struct device *dev)
> {
> + int ret;
> +
> INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
> spin_lock_init(&qcom_scm_mem.lock);
> qcom_scm_mem.dev = dev;
> @@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
>
> gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
>
> - return gen_pool_add_virt(qcom_scm_mem.pool,
> - (unsigned long)qcom_scm_mem.vbase,
> - qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> + ret = gen_pool_add_virt(qcom_scm_mem.pool,
> + (unsigned long)qcom_scm_mem.vbase,
> + qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> + if (ret)
> + return ret;
> +
> + ret = qcom_scm_enable_shm_bridge();
> + if (ret) {
> + if (ret == EOPNOTSUPP)
> + dev_info(dev, "SHM Bridge not supported\n");
> + else
> + return ret;
> + } else {
> + ret = qcom_scm_mem_shm_bridge_create();
> + if (ret)
> + return ret;
> +
> + dev_info(dev, "SHM Bridge enabled\n");
> + }
> +
> + return 0;
> }

2023-10-05 14:31:27

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

On Thu, Oct 5, 2023 at 12:24 AM Maximilian Luz <[email protected]> wrote:
>
> On 9/28/23 11:20, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > Extens the SCM memory allocator with using the SHM Bridge feature if
> > available on the platform. This makes the trustzone only use dedicated
> > buffers for SCM calls. We map the entire SCM genpool as a bridge.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
>
> This patch breaks something in early boot on my Surface Pro X (sc8180x).
> Unfortunately I can't provide many details at the moment because the
> only thing I can see are RCU stalls, and the traces from them are quite
> useless.
>
> Without this patch, the rest of the series (with the fix you posted on
> patch 6 applied) seems to work fine. Including both RFT qseecom patches.
>
> I plan to have a closer look at this once I have some more time though.
>

Can it be the PAS image loading? This is something Andrew reported and
I have it fixed for v3.

Bart

> Regards,
> Max
>
> > ---
> > drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
> > 1 file changed, 39 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
> > index eafecbe23770..12b12b15f46f 100644
> > --- a/drivers/firmware/qcom/qcom_scm-mem.c
> > +++ b/drivers/firmware/qcom/qcom_scm-mem.c
> > @@ -16,6 +16,8 @@
> >
> > #include "qcom_scm.h"
> >
> > +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> > +
> > static size_t qcom_scm_mem_pool_size = SZ_2M;
> > module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
> > ulong, 0400);
> > @@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
> > return chunk->paddr;
> > }
> >
> > +static int qcom_scm_mem_shm_bridge_create(void)
> > +{
> > + uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
> > +
> > + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> > + pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> > + ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> > + size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> > +
> > + return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
> > + ipfn_and_s_perm, size_and_flags,
> > + QCOM_SCM_VMID_HLOS);
> > +}
> > +
> > int qcom_scm_mem_enable(struct device *dev)
> > {
> > + int ret;
> > +
> > INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
> > spin_lock_init(&qcom_scm_mem.lock);
> > qcom_scm_mem.dev = dev;
> > @@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
> >
> > gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
> >
> > - return gen_pool_add_virt(qcom_scm_mem.pool,
> > - (unsigned long)qcom_scm_mem.vbase,
> > - qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> > + ret = gen_pool_add_virt(qcom_scm_mem.pool,
> > + (unsigned long)qcom_scm_mem.vbase,
> > + qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> > + if (ret)
> > + return ret;
> > +
> > + ret = qcom_scm_enable_shm_bridge();
> > + if (ret) {
> > + if (ret == EOPNOTSUPP)
> > + dev_info(dev, "SHM Bridge not supported\n");
> > + else
> > + return ret;
> > + } else {
> > + ret = qcom_scm_mem_shm_bridge_create();
> > + if (ret)
> > + return ret;
> > +
> > + dev_info(dev, "SHM Bridge enabled\n");
> > + }
> > +
> > + return 0;
> > }

2023-10-05 14:32:07

by Maximilian Luz

[permalink] [raw]
Subject: Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

Am 05/10/2023 um 09:12 schrieb Bartosz Golaszewski:
> On Thu, Oct 5, 2023 at 12:24 AM Maximilian Luz <[email protected]> wrote:
>>
>> On 9/28/23 11:20, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <[email protected]>
>>>
>>> Extens the SCM memory allocator with using the SHM Bridge feature if
>>> available on the platform. This makes the trustzone only use dedicated
>>> buffers for SCM calls. We map the entire SCM genpool as a bridge.
>>>
>>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>>
>> This patch breaks something in early boot on my Surface Pro X (sc8180x).
>> Unfortunately I can't provide many details at the moment because the
>> only thing I can see are RCU stalls, and the traces from them are quite
>> useless.
>>
>> Without this patch, the rest of the series (with the fix you posted on
>> patch 6 applied) seems to work fine. Including both RFT qseecom patches.
>>
>> I plan to have a closer look at this once I have some more time though.
>>
>
> Can it be the PAS image loading? This is something Andrew reported and
> I have it fixed for v3.

That is my current suspicion, but I haven't had the time to properly
check it yet.

Regards,
Max

> Bart
>
>> Regards,
>> Max
>>
>>> ---
>>> drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
>>> 1 file changed, 39 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
>>> index eafecbe23770..12b12b15f46f 100644
>>> --- a/drivers/firmware/qcom/qcom_scm-mem.c
>>> +++ b/drivers/firmware/qcom/qcom_scm-mem.c
>>> @@ -16,6 +16,8 @@
>>>
>>> #include "qcom_scm.h"
>>>
>>> +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
>>> +
>>> static size_t qcom_scm_mem_pool_size = SZ_2M;
>>> module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
>>> ulong, 0400);
>>> @@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
>>> return chunk->paddr;
>>> }
>>>
>>> +static int qcom_scm_mem_shm_bridge_create(void)
>>> +{
>>> + uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
>>> +
>>> + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
>>> + pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
>>> + ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
>>> + size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
>>> +
>>> + return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
>>> + ipfn_and_s_perm, size_and_flags,
>>> + QCOM_SCM_VMID_HLOS);
>>> +}
>>> +
>>> int qcom_scm_mem_enable(struct device *dev)
>>> {
>>> + int ret;
>>> +
>>> INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
>>> spin_lock_init(&qcom_scm_mem.lock);
>>> qcom_scm_mem.dev = dev;
>>> @@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
>>>
>>> gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
>>>
>>> - return gen_pool_add_virt(qcom_scm_mem.pool,
>>> - (unsigned long)qcom_scm_mem.vbase,
>>> - qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
>>> + ret = gen_pool_add_virt(qcom_scm_mem.pool,
>>> + (unsigned long)qcom_scm_mem.vbase,
>>> + qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = qcom_scm_enable_shm_bridge();
>>> + if (ret) {
>>> + if (ret == EOPNOTSUPP)
>>> + dev_info(dev, "SHM Bridge not supported\n");
>>> + else
>>> + return ret;
>>> + } else {
>>> + ret = qcom_scm_mem_shm_bridge_create();
>>> + if (ret)
>>> + return ret;
>>> +
>>> + dev_info(dev, "SHM Bridge enabled\n");
>>> + }
>>> +
>>> + return 0;
>>> }