| Message ID | 20230928092040.9420-12-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: qcom: add and enable SHM Bridge support | expand |
On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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 <bartosz.golaszewski@linaro.org> > --- > 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; > }
On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Extens the SCM memory allocator with using the SHM Bridge feature if nit: s/Extens/Extend/
On Thu, Sep 28, 2023 at 11:21 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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 <bartosz.golaszewski@linaro.org> > --- > 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 >
On 9/28/23 11:20, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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 <bartosz.golaszewski@linaro.org> 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; > }
On Thu, Oct 5, 2023 at 12:24 AM Maximilian Luz <luzmaximilian@gmail.com> wrote: > > On 9/28/23 11:20, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > 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 <bartosz.golaszewski@linaro.org> > > 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; > > }
Am 05/10/2023 um 09:12 schrieb Bartosz Golaszewski: > On Thu, Oct 5, 2023 at 12:24 AM Maximilian Luz <luzmaximilian@gmail.com> wrote: >> >> On 9/28/23 11:20, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>> >>> 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 <bartosz.golaszewski@linaro.org> >> >> 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; >>> }
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; }