[PATCH 00/11] arm64: qcom: add and enable SHM Bridge support

[PATCH 00/11] arm64: qcom: add and enable SHM Bridge support

[Bartosz Golaszewski]

SHM Bridge is a mechanism allowing to map limited areas of kernel's
virtual memory to physical addresses and share those with the
trustzone in order to not expose the entire RAM for SMC calls.

This series adds support for Qualcomm SHM Bridge in form of a platform
driver and library functions available to users. It enables SHM Bridge
support for three platforms and contains a bunch of cleanups for
qcom-scm.

Bartosz Golaszewski (11):
  firmware: qcom-scm: drop unneeded 'extern' specifiers
  firmware: qcom-scm: order includes alphabetically
  firmware: qcom-scm: atomically assign and read the global __scm
    pointer
  firmware: qcom-scm: add support for SHM bridge operations
  dt-bindings: document the Qualcomm TEE Shared Memory Bridge
  firmware: qcom-shm-bridge: new driver
  firmware: qcom-scm: use SHM bridge if available
  arm64: defconfig: enable Qualcomm SHM bridge module
  arm64: dts: qcom: sm8450: enable SHM bridge
  arm64: dts: qcom: sa8775p: enable SHM bridge
  arm64: dts: qcom: sm8150: enable SHM bridge

 .../bindings/firmware/qcom,shm-bridge.yaml    |  36 ++
 arch/arm64/boot/dts/qcom/sa8775p.dtsi         |   4 +
 arch/arm64/boot/dts/qcom/sm8150.dtsi          |   4 +
 arch/arm64/boot/dts/qcom/sm8450.dtsi          |   4 +
 arch/arm64/configs/defconfig                  |   1 +
 drivers/firmware/Kconfig                      |   8 +
 drivers/firmware/Makefile                     |   1 +
 drivers/firmware/qcom-shm-bridge.c            | 452 ++++++++++++++++++
 drivers/firmware/qcom_scm-smc.c               |  20 +-
 drivers/firmware/qcom_scm.c                   | 106 +++-
 drivers/firmware/qcom_scm.h                   |   3 +
 include/linux/firmware/qcom/qcom_scm.h        | 109 +++--
 include/linux/firmware/qcom/shm-bridge.h      |  32 ++
 13 files changed, 712 insertions(+), 68 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
 create mode 100644 drivers/firmware/qcom-shm-bridge.c
 create mode 100644 include/linux/firmware/qcom/shm-bridge.h

-- 
2.39.2

[PATCH 01/11] firmware: qcom-scm: drop unneeded 'extern' specifiers

[Bartosz Golaszewski]

The 'extern' specifier in front of a function declaration has no effect.
Remove all of them from the qcom-scm header.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/linux/firmware/qcom/qcom_scm.h | 101 ++++++++++++-------------
 1 file changed, 48 insertions(+), 53 deletions(-)

diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index 250ea4efb7cb..0187fc54249e 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -59,12 +59,12 @@ enum qcom_scm_ice_cipher {
 #define QCOM_SCM_PERM_RW (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)
 #define QCOM_SCM_PERM_RWX (QCOM_SCM_PERM_RW | QCOM_SCM_PERM_EXEC)
 
-extern bool qcom_scm_is_available(void);
+bool qcom_scm_is_available(void);
 
-extern int qcom_scm_set_cold_boot_addr(void *entry);
-extern int qcom_scm_set_warm_boot_addr(void *entry);
-extern void qcom_scm_cpu_power_down(u32 flags);
-extern int qcom_scm_set_remote_state(u32 state, u32 id);
+int qcom_scm_set_cold_boot_addr(void *entry);
+int qcom_scm_set_warm_boot_addr(void *entry);
+void qcom_scm_cpu_power_down(u32 flags);
+int qcom_scm_set_remote_state(u32 state, u32 id);
 
 struct qcom_scm_pas_metadata {
 	void *ptr;
@@ -72,54 +72,49 @@ struct qcom_scm_pas_metadata {
 	ssize_t size;
 };
 
-extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
-				   size_t size,
-				   struct qcom_scm_pas_metadata *ctx);
+int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
+			    struct qcom_scm_pas_metadata *ctx);
 void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
-extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
-				  phys_addr_t size);
-extern int qcom_scm_pas_auth_and_reset(u32 peripheral);
-extern int qcom_scm_pas_shutdown(u32 peripheral);
-extern bool qcom_scm_pas_supported(u32 peripheral);
-
-extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
-extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
-
-extern bool qcom_scm_restore_sec_cfg_available(void);
-extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
-extern int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size);
-extern int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare);
-extern int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size);
-extern int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
-					  u32 cp_nonpixel_start,
-					  u32 cp_nonpixel_size);
-extern int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
-			       u64 *src,
-			       const struct qcom_scm_vmperm *newvm,
-			       unsigned int dest_cnt);
-
-extern bool qcom_scm_ocmem_lock_available(void);
-extern int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset,
-			       u32 size, u32 mode);
-extern int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id, u32 offset,
-				 u32 size);
-
-extern bool qcom_scm_ice_available(void);
-extern int qcom_scm_ice_invalidate_key(u32 index);
-extern int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
-				enum qcom_scm_ice_cipher cipher,
-				u32 data_unit_size);
-
-extern bool qcom_scm_hdcp_available(void);
-extern int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
-			     u32 *resp);
-
-extern int qcom_scm_iommu_set_pt_format(u32 sec_id, u32 ctx_num, u32 pt_fmt);
-extern int qcom_scm_qsmmu500_wait_safe_toggle(bool en);
-
-extern int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
-			      u64 limit_node, u32 node_id, u64 version);
-extern int qcom_scm_lmh_profile_change(u32 profile_id);
-extern bool qcom_scm_lmh_dcvsh_available(void);
+int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);
+int qcom_scm_pas_auth_and_reset(u32 peripheral);
+int qcom_scm_pas_shutdown(u32 peripheral);
+bool qcom_scm_pas_supported(u32 peripheral);
+
+int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
+int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
+
+bool qcom_scm_restore_sec_cfg_available(void);
+int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
+int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size);
+int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare);
+int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size);
+int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
+				   u32 cp_nonpixel_start,
+				   u32 cp_nonpixel_size);
+int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, u64 *src,
+			const struct qcom_scm_vmperm *newvm,
+			unsigned int dest_cnt);
+
+bool qcom_scm_ocmem_lock_available(void);
+int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset,
+			u32 size, u32 mode);
+int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id, u32 offset, u32 size);
+
+bool qcom_scm_ice_available(void);
+int qcom_scm_ice_invalidate_key(u32 index);
+int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
+			 enum qcom_scm_ice_cipher cipher,
+			 u32 data_unit_size);
+
+bool qcom_scm_hdcp_available(void);
+int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp);
+
+int qcom_scm_iommu_set_pt_format(u32 sec_id, u32 ctx_num, u32 pt_fmt);
+int qcom_scm_qsmmu500_wait_safe_toggle(bool en);
+
+int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
+		       u64 limit_node, u32 node_id, u64 version);
+int qcom_scm_lmh_profile_change(u32 profile_id);
+bool qcom_scm_lmh_dcvsh_available(void);
 
 #endif
-- 
2.39.2

Re: [PATCH 01/11] firmware: qcom-scm: drop unneeded 'extern' specifiers

[Krzysztof Kozlowski]

On 28/08/2023 21:24, Bartosz Golaszewski wrote:
> The 'extern' specifier in front of a function declaration has no effect.
> Remove all of them from the qcom-scm header.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  include/linux/firmware/qcom/qcom_scm.h | 101 ++++++++++++------

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 01/11] firmware: qcom-scm: drop unneeded 'extern' specifiers

[Bjorn Andersson]

On Mon, Aug 28, 2023 at 09:24:57PM +0200, Bartosz Golaszewski wrote:
> The 'extern' specifier in front of a function declaration has no effect.
> Remove all of them from the qcom-scm header.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Bjorn Andersson <andersson@kernel.org>

I wanted to pick the first two patches of the series, but they
unfortunately doesn't apply. Feel free to resubmit them on their own.

Regards,
Bjorn

[PATCH 02/11] firmware: qcom-scm: order includes alphabetically

[Bartosz Golaszewski]

For easier maintenance order the included headers in qcom_scm.c
alphabetically.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/firmware/qcom_scm.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index fde33acd46b7..980fcfa20b9f 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -2,24 +2,25 @@
 /* Copyright (c) 2010,2015,2019 The Linux Foundation. All rights reserved.
  * Copyright (C) 2015 Linaro Ltd.
  */
-#include <linux/platform_device.h>
-#include <linux/init.h>
-#include <linux/interrupt.h>
+
+#include <linux/arm-smccc.h>
+#include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/cpumask.h>
-#include <linux/export.h>
 #include <linux/dma-mapping.h>
+#include <linux/export.h>
+#include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/init.h>
 #include <linux/interconnect.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
-#include <linux/types.h>
-#include <linux/firmware/qcom/qcom_scm.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
-#include <linux/clk.h>
+#include <linux/platform_device.h>
 #include <linux/reset-controller.h>
-#include <linux/arm-smccc.h>
+#include <linux/types.h>
 
 #include "qcom_scm.h"
 
-- 
2.39.2

Re: [PATCH 02/11] firmware: qcom-scm: order includes alphabetically

[Krzysztof Kozlowski]

On 28/08/2023 21:24, Bartosz Golaszewski wrote:
> For easier maintenance order the included headers in qcom_scm.c
> alphabetically.

I assume they are all needed.


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH 03/11] firmware: qcom-scm: atomically assign and read the global __scm pointer

[Bartosz Golaszewski]

Checking for the availability of SCM bridge can happen from any context.
It's only by chance that we haven't run into concurrency issues but with
the upcoming SHM Bridge driver that will be initiated at the same
initcall level, we need to assure the assignment and readback of the
__scm pointer is atomic.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/firmware/qcom_scm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 980fcfa20b9f..422de70faff8 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1331,7 +1331,7 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr)
  */
 bool qcom_scm_is_available(void)
 {
-	return !!__scm;
+	return !!READ_ONCE(__scm);
 }
 EXPORT_SYMBOL(qcom_scm_is_available);
 
@@ -1477,8 +1477,8 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	__scm = scm;
-	__scm->dev = &pdev->dev;
+	scm->dev = &pdev->dev;
+	WRITE_ONCE(__scm, scm);
 
 	init_completion(&__scm->waitq_comp);
 
-- 
2.39.2

Re: [PATCH 03/11] firmware: qcom-scm: atomically assign and read the global __scm pointer

[Krzysztof Kozlowski]

On 28/08/2023 21:24, Bartosz Golaszewski wrote:
> Checking for the availability of SCM bridge can happen from any context.
> It's only by chance that we haven't run into concurrency issues but with
> the upcoming SHM Bridge driver that will be initiated at the same
> initcall level, we need to assure the assignment and readback of the
> __scm pointer is atomic.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/firmware/qcom_scm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 980fcfa20b9f..422de70faff8 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -1331,7 +1331,7 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr)
>   */
>  bool qcom_scm_is_available(void)
>  {
> -	return !!__scm;
> +	return !!READ_ONCE(__scm);
>  }
>  EXPORT_SYMBOL(qcom_scm_is_available);
>  
> @@ -1477,8 +1477,8 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	__scm = scm;
> -	__scm->dev = &pdev->dev;
> +	scm->dev = &pdev->dev;
> +	WRITE_ONCE(__scm, scm);

Your re-ordering is not effective here, I think. I don't understand it's
purpose exactly, but scm->dev assignment is not WRITE_ONCE(), thus it
can be reordered:

"compiler is also forbidden from reordering successive instances of
READ_ONCE and WRITE_ONCE" <- so compiler is not forbidden to reorder
other stuff.

"Ensuring that the compiler does not fold, spindle, or otherwise
mutilate accesses that either do not require ordering or that interact"
<- which means you do not require ordering here.

Best regards,
Krzysztof

Re: [PATCH 03/11] firmware: qcom-scm: atomically assign and read the global __scm pointer

[Bartosz Golaszewski]

On Tue, 29 Aug 2023 at 09:59, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 28/08/2023 21:24, Bartosz Golaszewski wrote:
> > Checking for the availability of SCM bridge can happen from any context.
> > It's only by chance that we haven't run into concurrency issues but with
> > the upcoming SHM Bridge driver that will be initiated at the same
> > initcall level, we need to assure the assignment and readback of the
> > __scm pointer is atomic.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/firmware/qcom_scm.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index 980fcfa20b9f..422de70faff8 100644
> > --- a/drivers/firmware/qcom_scm.c
> > +++ b/drivers/firmware/qcom_scm.c
> > @@ -1331,7 +1331,7 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr)
> >   */
> >  bool qcom_scm_is_available(void)
> >  {
> > -     return !!__scm;
> > +     return !!READ_ONCE(__scm);
> >  }
> >  EXPORT_SYMBOL(qcom_scm_is_available);
> >
> > @@ -1477,8 +1477,8 @@ static int qcom_scm_probe(struct platform_device *pdev)
> >       if (ret)
> >               return ret;
> >
> > -     __scm = scm;
> > -     __scm->dev = &pdev->dev;
> > +     scm->dev = &pdev->dev;
> > +     WRITE_ONCE(__scm, scm);
>
> Your re-ordering is not effective here, I think. I don't understand it's
> purpose exactly, but scm->dev assignment is not WRITE_ONCE(), thus it
> can be reordered:
>
> "compiler is also forbidden from reordering successive instances of
> READ_ONCE and WRITE_ONCE" <- so compiler is not forbidden to reorder
> other stuff.
>
> "Ensuring that the compiler does not fold, spindle, or otherwise
> mutilate accesses that either do not require ordering or that interact"
> <- which means you do not require ordering here.
>

Hmm, I had the list_add() implementation in mind as well as examples
from https://www.kernel.org/doc/Documentation/memory-barriers.txt and
was under the impression that WRITE_ONCE() here is enough. I need to
double check it.

Bart

Re: [PATCH 03/11] firmware: qcom-scm: atomically assign and read the global __scm pointer

[Krzysztof Kozlowski]

On 29/08/2023 14:31, Bartosz Golaszewski wrote:
> On Tue, 29 Aug 2023 at 09:59, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 28/08/2023 21:24, Bartosz Golaszewski wrote:
>>> Checking for the availability of SCM bridge can happen from any context.
>>> It's only by chance that we haven't run into concurrency issues but with
>>> the upcoming SHM Bridge driver that will be initiated at the same
>>> initcall level, we need to assure the assignment and readback of the
>>> __scm pointer is atomic.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>> ---
>>>  drivers/firmware/qcom_scm.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>>> index 980fcfa20b9f..422de70faff8 100644
>>> --- a/drivers/firmware/qcom_scm.c
>>> +++ b/drivers/firmware/qcom_scm.c
>>> @@ -1331,7 +1331,7 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr)
>>>   */
>>>  bool qcom_scm_is_available(void)
>>>  {
>>> -     return !!__scm;
>>> +     return !!READ_ONCE(__scm);
>>>  }
>>>  EXPORT_SYMBOL(qcom_scm_is_available);
>>>
>>> @@ -1477,8 +1477,8 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>>       if (ret)
>>>               return ret;
>>>
>>> -     __scm = scm;
>>> -     __scm->dev = &pdev->dev;
>>> +     scm->dev = &pdev->dev;
>>> +     WRITE_ONCE(__scm, scm);
>>
>> Your re-ordering is not effective here, I think. I don't understand it's
>> purpose exactly, but scm->dev assignment is not WRITE_ONCE(), thus it
>> can be reordered:
>>
>> "compiler is also forbidden from reordering successive instances of
>> READ_ONCE and WRITE_ONCE" <- so compiler is not forbidden to reorder
>> other stuff.
>>
>> "Ensuring that the compiler does not fold, spindle, or otherwise
>> mutilate accesses that either do not require ordering or that interact"
>> <- which means you do not require ordering here.
>>
> 
> Hmm, I had the list_add() implementation in mind as well as examples
> from https://www.kernel.org/doc/Documentation/memory-barriers.txt and
> was under the impression that WRITE_ONCE() here is enough. I need to
> double check it.

It all depends what do you want to achieve. If strict ordering of both
writes, then all the examples and descriptions from memory-barriers.txt
say that you need WRITE_ONCE in both places.

If you want to achieve something else, like scm->dev visible for other
CPUs before __scm=scm then you would need full barriers, IMHO.


Best regards,
Krzysztof

Re: [PATCH 03/11] firmware: qcom-scm: atomically assign and read the global __scm pointer

[Om Prakash Singh]

On 8/29/2023 12:54 AM, Bartosz Golaszewski wrote:
> Checking for the availability of SCM bridge can happen from any context.
> It's only by chance that we haven't run into concurrency issues but with
> the upcoming SHM Bridge driver that will be initiated at the same
> initcall level, we need to assure the assignment and readback of the
> __scm pointer is atomic.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/firmware/qcom_scm.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 980fcfa20b9f..422de70faff8 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -1331,7 +1331,7 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr)
>    */
>   bool qcom_scm_is_available(void)
>   {
> -	return !!__scm;
> +	return !!READ_ONCE(__scm);
>   }
>   EXPORT_SYMBOL(qcom_scm_is_available);
>   
> @@ -1477,8 +1477,8 @@ static int qcom_scm_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> -	__scm = scm;
> -	__scm->dev = &pdev->dev;
> +	scm->dev = &pdev->dev;
> +	WRITE_ONCE(__scm, scm);

In my opinion "__scm = scm;" assignment should be done at the end of 
probe function success so that qcom_scm_is_available() return true only 
when probe is completed.

Other changes may not be needed.

>   	init_completion(&__scm->waitq_comp);
>   

Re: [PATCH 03/11] firmware: qcom-scm: atomically assign and read the global __scm pointer

[Bartosz Golaszewski]

On Tue, 17 Oct 2023 at 10:25, Om Prakash Singh
<quic_omprsing@quicinc.com> wrote:
>
>
>
> On 8/29/2023 12:54 AM, Bartosz Golaszewski wrote:
> > Checking for the availability of SCM bridge can happen from any context.
> > It's only by chance that we haven't run into concurrency issues but with
> > the upcoming SHM Bridge driver that will be initiated at the same
> > initcall level, we need to assure the assignment and readback of the
> > __scm pointer is atomic.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >   drivers/firmware/qcom_scm.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index 980fcfa20b9f..422de70faff8 100644
> > --- a/drivers/firmware/qcom_scm.c
> > +++ b/drivers/firmware/qcom_scm.c
> > @@ -1331,7 +1331,7 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr)
> >    */
> >   bool qcom_scm_is_available(void)
> >   {
> > -     return !!__scm;
> > +     return !!READ_ONCE(__scm);
> >   }
> >   EXPORT_SYMBOL(qcom_scm_is_available);
> >
> > @@ -1477,8 +1477,8 @@ static int qcom_scm_probe(struct platform_device *pdev)
> >       if (ret)
> >               return ret;
> >
> > -     __scm = scm;
> > -     __scm->dev = &pdev->dev;
> > +     scm->dev = &pdev->dev;
> > +     WRITE_ONCE(__scm, scm);
>
> In my opinion "__scm = scm;" assignment should be done at the end of
> probe function success so that qcom_scm_is_available() return true only
> when probe is completed.
>
> Other changes may not be needed.
>

Om, this is v1. We're at v4 (soon v5).

I agree with the comment but it's outside the scope of this series. I
may address it later.

It has been like this for a long time. Typically this module is
built-in and initialized before all other drivers so it has never
caused problems.

Bart

> >       init_completion(&__scm->waitq_comp);
> >

[PATCH 04/11] firmware: qcom-scm: add support for SHM bridge operations

[Bartosz Golaszewski]

Add low-level primitives for enabling SHM bridge support, creating SHM
bridge pools and testing the availability of SHM bridges to qcom-scm. We
don't yet provide a way to destroy the bridges as the first user will
not require it.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/firmware/qcom_scm.c            | 83 ++++++++++++++++++++++++++
 drivers/firmware/qcom_scm.h            |  3 +
 include/linux/firmware/qcom/qcom_scm.h |  8 +++
 3 files changed, 94 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 422de70faff8..f45d5a424424 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -31,6 +31,8 @@ module_param(download_mode, bool, 0);
 #define SCM_HAS_IFACE_CLK	BIT(1)
 #define SCM_HAS_BUS_CLK		BIT(2)
 
+#define SCM_SHM_BRIDGE_NOTSUPP	4
+
 struct qcom_scm {
 	struct device *dev;
 	struct clk *core_clk;
@@ -45,6 +47,8 @@ struct qcom_scm {
 	int scm_vote_count;
 
 	u64 dload_mode_addr;
+
+	bool shm_bridge_enabled;
 };
 
 struct qcom_scm_current_perm_info {
@@ -1248,6 +1252,85 @@ bool qcom_scm_lmh_dcvsh_available(void)
 }
 EXPORT_SYMBOL(qcom_scm_lmh_dcvsh_available);
 
+bool qcom_scm_shm_bridge_available(void)
+{
+	if (!qcom_scm_is_available())
+		return false;
+
+	return READ_ONCE(__scm->shm_bridge_enabled);
+}
+EXPORT_SYMBOL_GPL(qcom_scm_shm_bridge_available);
+
+/*
+ * Must not be called unless qcom_scm_shm_bridge_available() returned true
+ * first.
+ */
+int qcom_scm_enable_shm_bridge(void)
+{
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_MP,
+		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
+		.owner = ARM_SMCCC_OWNER_SIP
+	};
+
+	struct qcom_scm_res res;
+	int ret;
+
+	ret = qcom_scm_call(__scm->dev, &desc, &res);
+	if (!ret && !res.result[0])
+		WRITE_ONCE(__scm->shm_bridge_enabled, true);
+
+	if (res.result[0] == SCM_SHM_BRIDGE_NOTSUPP)
+		ret = -EOPNOTSUPP;
+
+	return ret ?: res.result[0];
+}
+EXPORT_SYMBOL_GPL(qcom_scm_enable_shm_bridge);
+
+int qcom_scm_create_shm_bridge(struct device *dev, u64 pfn_and_ns_perm_flags,
+			       u64 ipfn_and_s_perm_flags, u64 size_and_flags,
+			       u64 ns_vmids, u64 *handle)
+{
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_MP,
+		.cmd = QCOM_SCM_MP_SHM_BRDIGE_CREATE,
+		.owner = ARM_SMCCC_OWNER_SIP
+	};
+
+	struct qcom_scm_res res;
+	int ret;
+
+	desc.args[0] = pfn_and_ns_perm_flags;
+	desc.args[1] = ipfn_and_s_perm_flags;
+	desc.args[2] = size_and_flags;
+	desc.args[3] = ns_vmids;
+
+	desc.arginfo = QCOM_SCM_ARGS(4, QCOM_SCM_VAL, QCOM_SCM_VAL,
+				     QCOM_SCM_VAL, QCOM_SCM_VAL);
+
+	ret = qcom_scm_call(dev ?: __scm->dev, &desc, &res);
+
+	if (handle && !ret)
+		*handle = res.result[1];
+
+	return ret ?: res.result[0];
+}
+EXPORT_SYMBOL_GPL(qcom_scm_create_shm_bridge);
+
+int qcom_scm_delete_shm_bridge(struct device *dev, u64 handle)
+{
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_MP,
+		.cmd = QCOM_SCM_MP_SHM_BRIDGE_DELETE,
+		.owner = ARM_SMCCC_OWNER_SIP,
+		.args[0] = handle,
+		.arginfo = QCOM_SCM_ARGS(1, QCOM_SCM_VAL),
+	};
+
+	return qcom_scm_call(dev ?: __scm->dev, &desc, NULL);
+}
+EXPORT_SYMBOL_GPL(qcom_scm_delete_shm_bridge);
+
 int qcom_scm_lmh_profile_change(u32 profile_id)
 {
 	struct qcom_scm_desc desc = {
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index e6e512bd57d1..44d60d06344b 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -111,6 +111,9 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
 #define QCOM_SCM_MP_IOMMU_SET_CP_POOL_SIZE	0x05
 #define QCOM_SCM_MP_VIDEO_VAR			0x08
 #define QCOM_SCM_MP_ASSIGN			0x16
+#define QCOM_SCM_MP_SHM_BRIDGE_ENABLE		0x1c
+#define QCOM_SCM_MP_SHM_BRIDGE_DELETE		0x1d
+#define QCOM_SCM_MP_SHM_BRDIGE_CREATE		0x1e
 
 #define QCOM_SCM_SVC_OCMEM		0x0f
 #define QCOM_SCM_OCMEM_LOCK_CMD		0x01
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index 0187fc54249e..100770380d97 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -5,6 +5,7 @@
 #ifndef __QCOM_SCM_H
 #define __QCOM_SCM_H
 
+#include <linux/device.h>
 #include <linux/err.h>
 #include <linux/types.h>
 #include <linux/cpumask.h>
@@ -117,4 +118,11 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
 int qcom_scm_lmh_profile_change(u32 profile_id);
 bool qcom_scm_lmh_dcvsh_available(void);
 
+bool qcom_scm_shm_bridge_available(void);
+int qcom_scm_enable_shm_bridge(void);
+int qcom_scm_create_shm_bridge(struct device *dev, u64 pfn_and_ns_perm_flags,
+			       u64 ipfn_and_s_perm_flags, u64 size_and_flags,
+			       u64 ns_vmids, u64 *handle);
+int qcom_scm_delete_shm_bridge(struct device *dev, u64 handle);
+
 #endif
-- 
2.39.2

[PATCH 05/11] dt-bindings: document the Qualcomm TEE Shared Memory Bridge

[Bartosz Golaszewski]

Add Device Tree bindings for Qualcomm TEE Shared Memory Brige - a
mechanism that allows sharing memory buffers between trustzone and the
kernel.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 .../bindings/firmware/qcom,shm-bridge.yaml    | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml

diff --git a/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
new file mode 100644
index 000000000000..f660962b7b86
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
@@ -0,0 +1,36 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/firmware/qcom,shm-bridge.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: QCOM Shared Memory Bridge
+
+description: |
+  Qualcomm TEE Shared Memory Bridge allows sharing limited areas of kernel's
+  virtual memory with the trustzone in order to avoid mapping the entire RAM.
+
+maintainers:
+  - Bjorn Andersson <andersson@kernel.org>
+  - Konrad Dybcio <konrad.dybcio@linaro.org>
+  - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qcom,shm-bridge-sa8775p
+          - qcom,shm-bridge-sm8150
+          - qcom,shm-bridge-sm8450
+      - const: qcom,shm-bridge
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    firmware {
+         compatible = "qcom,shm-bridge-sa8775p", "qcom,shm-bridge";
+    };
-- 
2.39.2

Re: [PATCH 05/11] dt-bindings: document the Qualcomm TEE Shared Memory Bridge

[Krzysztof Kozlowski]

On 28/08/2023 21:25, Bartosz Golaszewski wrote:
> Add Device Tree bindings for Qualcomm TEE Shared Memory Brige - a
> mechanism that allows sharing memory buffers between trustzone and the
> kernel.

Subject prefix:
dt-bindings: firmware:



> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  .../bindings/firmware/qcom,shm-bridge.yaml    | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> 
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> new file mode 100644
> index 000000000000..f660962b7b86
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> @@ -0,0 +1,36 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/firmware/qcom,shm-bridge.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: QCOM Shared Memory Bridge
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  Qualcomm TEE Shared Memory Bridge allows sharing limited areas of kernel's
> +  virtual memory with the trustzone in order to avoid mapping the entire RAM.
> +
> +maintainers:
> +  - Bjorn Andersson <andersson@kernel.org>
> +  - Konrad Dybcio <konrad.dybcio@linaro.org>
> +  - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - qcom,shm-bridge-sa8775p
> +          - qcom,shm-bridge-sm8150
> +          - qcom,shm-bridge-sm8450
> +      - const: qcom,shm-bridge
> +

Looks quite empty... Why this cannot be part of qcom,scm? IOW, why do
you need new binding if you do not have any resources here and the block
is essentially feature of qcom,scm firmware?

> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    firmware {
> +         compatible = "qcom,shm-bridge-sa8775p", "qcom,shm-bridge";

Use 4 spaces for example indentation.

> +    };

Best regards,
Krzysztof

Re: [PATCH 05/11] dt-bindings: document the Qualcomm TEE Shared Memory Bridge

[Konrad Dybcio]

On 29.08.2023 10:02, Krzysztof Kozlowski wrote:
> On 28/08/2023 21:25, Bartosz Golaszewski wrote:
>> Add Device Tree bindings for Qualcomm TEE Shared Memory Brige - a
>> mechanism that allows sharing memory buffers between trustzone and the
>> kernel.
> 
> Subject prefix:
> dt-bindings: firmware:
> 
> 
> 
>>
>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> ---
>>  .../bindings/firmware/qcom,shm-bridge.yaml    | 36 +++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
>> new file mode 100644
>> index 000000000000..f660962b7b86
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
>> @@ -0,0 +1,36 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/firmware/qcom,shm-bridge.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: QCOM Shared Memory Bridge
>> +
>> +description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
>> +  Qualcomm TEE Shared Memory Bridge allows sharing limited areas of kernel's
>> +  virtual memory with the trustzone in order to avoid mapping the entire RAM.
>> +
>> +maintainers:
>> +  - Bjorn Andersson <andersson@kernel.org>
>> +  - Konrad Dybcio <konrad.dybcio@linaro.org>
>> +  - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - qcom,shm-bridge-sa8775p
>> +          - qcom,shm-bridge-sm8150
>> +          - qcom,shm-bridge-sm8450
>> +      - const: qcom,shm-bridge
>> +
> 
> Looks quite empty... Why this cannot be part of qcom,scm? IOW, why do
> you need new binding if you do not have any resources here and the block
> is essentially feature of qcom,scm firmware?
Since it's "discoverable" (via retval of an scm call), I'd second the
idea of probing this from within the SCM driver.

Konrad

Re: [PATCH 05/11] dt-bindings: document the Qualcomm TEE Shared Memory Bridge

[Bartosz Golaszewski]

On Tue, 29 Aug 2023 at 11:30, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 29.08.2023 10:02, Krzysztof Kozlowski wrote:
> > On 28/08/2023 21:25, Bartosz Golaszewski wrote:
> >> Add Device Tree bindings for Qualcomm TEE Shared Memory Brige - a
> >> mechanism that allows sharing memory buffers between trustzone and the
> >> kernel.
> >
> > Subject prefix:
> > dt-bindings: firmware:
> >
> >
> >
> >>
> >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >> ---
> >>  .../bindings/firmware/qcom,shm-bridge.yaml    | 36 +++++++++++++++++++
> >>  1 file changed, 36 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> >> new file mode 100644
> >> index 000000000000..f660962b7b86
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> >> @@ -0,0 +1,36 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/firmware/qcom,shm-bridge.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: QCOM Shared Memory Bridge
> >> +
> >> +description: |
> >
> > Do not need '|' unless you need to preserve formatting.
> >
> >> +  Qualcomm TEE Shared Memory Bridge allows sharing limited areas of kernel's
> >> +  virtual memory with the trustzone in order to avoid mapping the entire RAM.
> >> +
> >> +maintainers:
> >> +  - Bjorn Andersson <andersson@kernel.org>
> >> +  - Konrad Dybcio <konrad.dybcio@linaro.org>
> >> +  - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >> +
> >> +properties:
> >> +  compatible:
> >> +    items:
> >> +      - enum:
> >> +          - qcom,shm-bridge-sa8775p
> >> +          - qcom,shm-bridge-sm8150
> >> +          - qcom,shm-bridge-sm8450
> >> +      - const: qcom,shm-bridge
> >> +
> >
> > Looks quite empty... Why this cannot be part of qcom,scm? IOW, why do
> > you need new binding if you do not have any resources here and the block
> > is essentially feature of qcom,scm firmware?
> Since it's "discoverable" (via retval of an scm call), I'd second the
> idea of probing this from within the SCM driver.
>
> Konrad

Downstream has a bunch of DT switches that we don't support for now
upstream. I disagree about shoehorning this into the SCM driver. It
really is a layer on top of SCM but also SCM is a user of this
interface.

I will send a v2 with QCom ICE as a second user so that exporting
symbols pointed out by Krzysztof as having no users make more sense.
Hopefully keeping it separate will make more sense too.

If anything, the SHM Bridge code should stay in a separate compilation
unit even as part of SCM.

Bart

Re: [PATCH 05/11] dt-bindings: document the Qualcomm TEE Shared Memory Bridge

[Krzysztof Kozlowski]

On 30/08/2023 15:48, Bartosz Golaszewski wrote:
> On Tue, 29 Aug 2023 at 11:30, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 29.08.2023 10:02, Krzysztof Kozlowski wrote:
>>> On 28/08/2023 21:25, Bartosz Golaszewski wrote:
>>>> Add Device Tree bindings for Qualcomm TEE Shared Memory Brige - a
>>>> mechanism that allows sharing memory buffers between trustzone and the
>>>> kernel.
>>>
>>> Subject prefix:
>>> dt-bindings: firmware:
>>>
>>>
>>>
>>>>
>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>> ---
>>>>  .../bindings/firmware/qcom,shm-bridge.yaml    | 36 +++++++++++++++++++
>>>>  1 file changed, 36 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
>>>> new file mode 100644
>>>> index 000000000000..f660962b7b86
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
>>>> @@ -0,0 +1,36 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/firmware/qcom,shm-bridge.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: QCOM Shared Memory Bridge
>>>> +
>>>> +description: |
>>>
>>> Do not need '|' unless you need to preserve formatting.
>>>
>>>> +  Qualcomm TEE Shared Memory Bridge allows sharing limited areas of kernel's
>>>> +  virtual memory with the trustzone in order to avoid mapping the entire RAM.
>>>> +
>>>> +maintainers:
>>>> +  - Bjorn Andersson <andersson@kernel.org>
>>>> +  - Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> +  - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    items:
>>>> +      - enum:
>>>> +          - qcom,shm-bridge-sa8775p
>>>> +          - qcom,shm-bridge-sm8150
>>>> +          - qcom,shm-bridge-sm8450
>>>> +      - const: qcom,shm-bridge
>>>> +
>>>
>>> Looks quite empty... Why this cannot be part of qcom,scm? IOW, why do
>>> you need new binding if you do not have any resources here and the block
>>> is essentially feature of qcom,scm firmware?
>> Since it's "discoverable" (via retval of an scm call), I'd second the
>> idea of probing this from within the SCM driver.
>>
>> Konrad
> 
> Downstream has a bunch of DT switches that we don't support for now
> upstream. I disagree about shoehorning this into the SCM driver. It
> really is a layer on top of SCM but also SCM is a user of this
> interface.

Sure, for the driver makes sense, but it does not really explain why DT
node is needed. It is not separate hardware. I doubt it is even separate
firmware, but part of SCM.

Best regards,
Krzysztof

Re: [PATCH 05/11] dt-bindings: document the Qualcomm TEE Shared Memory Bridge

[Bartosz Golaszewski]

On Wed, 30 Aug 2023 at 16:31, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 30/08/2023 15:48, Bartosz Golaszewski wrote:
> > On Tue, 29 Aug 2023 at 11:30, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >> On 29.08.2023 10:02, Krzysztof Kozlowski wrote:
> >>> On 28/08/2023 21:25, Bartosz Golaszewski wrote:
> >>>> Add Device Tree bindings for Qualcomm TEE Shared Memory Brige - a
> >>>> mechanism that allows sharing memory buffers between trustzone and the
> >>>> kernel.
> >>>
> >>> Subject prefix:
> >>> dt-bindings: firmware:
> >>>
> >>>
> >>>
> >>>>
> >>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>> ---
> >>>>  .../bindings/firmware/qcom,shm-bridge.yaml    | 36 +++++++++++++++++++
> >>>>  1 file changed, 36 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..f660962b7b86
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> >>>> @@ -0,0 +1,36 @@
> >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/firmware/qcom,shm-bridge.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: QCOM Shared Memory Bridge
> >>>> +
> >>>> +description: |
> >>>
> >>> Do not need '|' unless you need to preserve formatting.
> >>>
> >>>> +  Qualcomm TEE Shared Memory Bridge allows sharing limited areas of kernel's
> >>>> +  virtual memory with the trustzone in order to avoid mapping the entire RAM.
> >>>> +
> >>>> +maintainers:
> >>>> +  - Bjorn Andersson <andersson@kernel.org>
> >>>> +  - Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>> +  - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>> +
> >>>> +properties:
> >>>> +  compatible:
> >>>> +    items:
> >>>> +      - enum:
> >>>> +          - qcom,shm-bridge-sa8775p
> >>>> +          - qcom,shm-bridge-sm8150
> >>>> +          - qcom,shm-bridge-sm8450
> >>>> +      - const: qcom,shm-bridge
> >>>> +
> >>>
> >>> Looks quite empty... Why this cannot be part of qcom,scm? IOW, why do
> >>> you need new binding if you do not have any resources here and the block
> >>> is essentially feature of qcom,scm firmware?
> >> Since it's "discoverable" (via retval of an scm call), I'd second the
> >> idea of probing this from within the SCM driver.
> >>
> >> Konrad
> >
> > Downstream has a bunch of DT switches that we don't support for now
> > upstream. I disagree about shoehorning this into the SCM driver. It
> > really is a layer on top of SCM but also SCM is a user of this
> > interface.
>
> Sure, for the driver makes sense, but it does not really explain why DT
> node is needed. It is not separate hardware. I doubt it is even separate
> firmware, but part of SCM.
>
> Best regards,
> Krzysztof
>

Because not all platforms support it and it's the simplest way of
marking the ones that do. Both SHM and SCM nodes sit on the firmware
node anyway. What do you recommend? A property of the SCM node? Like
'qcom,shm-bridge` or something?

Bart

Re: [PATCH 05/11] dt-bindings: document the Qualcomm TEE Shared Memory Bridge

[Krzysztof Kozlowski]

On 30/08/2023 16:39, Bartosz Golaszewski wrote:
> On Wed, 30 Aug 2023 at 16:31, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 30/08/2023 15:48, Bartosz Golaszewski wrote:
>>> On Tue, 29 Aug 2023 at 11:30, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>>>
>>>> On 29.08.2023 10:02, Krzysztof Kozlowski wrote:
>>>>> On 28/08/2023 21:25, Bartosz Golaszewski wrote:
>>>>>> Add Device Tree bindings for Qualcomm TEE Shared Memory Brige - a
>>>>>> mechanism that allows sharing memory buffers between trustzone and the
>>>>>> kernel.
>>>>>
>>>>> Subject prefix:
>>>>> dt-bindings: firmware:
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>> ---
>>>>>>  .../bindings/firmware/qcom,shm-bridge.yaml    | 36 +++++++++++++++++++
>>>>>>  1 file changed, 36 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..f660962b7b86
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
>>>>>> @@ -0,0 +1,36 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/firmware/qcom,shm-bridge.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: QCOM Shared Memory Bridge
>>>>>> +
>>>>>> +description: |
>>>>>
>>>>> Do not need '|' unless you need to preserve formatting.
>>>>>
>>>>>> +  Qualcomm TEE Shared Memory Bridge allows sharing limited areas of kernel's
>>>>>> +  virtual memory with the trustzone in order to avoid mapping the entire RAM.
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Bjorn Andersson <andersson@kernel.org>
>>>>>> +  - Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>> +  - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    items:
>>>>>> +      - enum:
>>>>>> +          - qcom,shm-bridge-sa8775p
>>>>>> +          - qcom,shm-bridge-sm8150
>>>>>> +          - qcom,shm-bridge-sm8450
>>>>>> +      - const: qcom,shm-bridge
>>>>>> +
>>>>>
>>>>> Looks quite empty... Why this cannot be part of qcom,scm? IOW, why do
>>>>> you need new binding if you do not have any resources here and the block
>>>>> is essentially feature of qcom,scm firmware?
>>>> Since it's "discoverable" (via retval of an scm call), I'd second the
>>>> idea of probing this from within the SCM driver.
>>>>
>>>> Konrad
>>>
>>> Downstream has a bunch of DT switches that we don't support for now
>>> upstream. I disagree about shoehorning this into the SCM driver. It
>>> really is a layer on top of SCM but also SCM is a user of this
>>> interface.
>>
>> Sure, for the driver makes sense, but it does not really explain why DT
>> node is needed. It is not separate hardware. I doubt it is even separate
>> firmware, but part of SCM.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Because not all platforms support it and it's the simplest way of

Platforms like SoCs or boards?

> marking the ones that do. Both SHM and SCM nodes sit on the firmware
> node anyway. What do you recommend? A property of the SCM node? Like
> 'qcom,shm-bridge` or something?

If the first - you talk about SoCs - then you have everything needed
already: SCM compatibles. This defines it fully.

If it varies by boards with one SoC, would be different case, but I
really doubt it.

Best regards,
Krzysztof

Re: [PATCH 05/11] dt-bindings: document the Qualcomm TEE Shared Memory Bridge

[Bartosz Golaszewski]

On Wed, 30 Aug 2023 at 16:58, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 30/08/2023 16:39, Bartosz Golaszewski wrote:
> > On Wed, 30 Aug 2023 at 16:31, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 30/08/2023 15:48, Bartosz Golaszewski wrote:
> >>> On Tue, 29 Aug 2023 at 11:30, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>>>
> >>>> On 29.08.2023 10:02, Krzysztof Kozlowski wrote:
> >>>>> On 28/08/2023 21:25, Bartosz Golaszewski wrote:
> >>>>>> Add Device Tree bindings for Qualcomm TEE Shared Memory Brige - a
> >>>>>> mechanism that allows sharing memory buffers between trustzone and the
> >>>>>> kernel.
> >>>>>
> >>>>> Subject prefix:
> >>>>> dt-bindings: firmware:
> >>>>>
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>> ---
> >>>>>>  .../bindings/firmware/qcom,shm-bridge.yaml    | 36 +++++++++++++++++++
> >>>>>>  1 file changed, 36 insertions(+)
> >>>>>>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..f660962b7b86
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> >>>>>> @@ -0,0 +1,36 @@
> >>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>>> +%YAML 1.2
> >>>>>> +---
> >>>>>> +$id: http://devicetree.org/schemas/firmware/qcom,shm-bridge.yaml#
> >>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>> +
> >>>>>> +title: QCOM Shared Memory Bridge
> >>>>>> +
> >>>>>> +description: |
> >>>>>
> >>>>> Do not need '|' unless you need to preserve formatting.
> >>>>>
> >>>>>> +  Qualcomm TEE Shared Memory Bridge allows sharing limited areas of kernel's
> >>>>>> +  virtual memory with the trustzone in order to avoid mapping the entire RAM.
> >>>>>> +
> >>>>>> +maintainers:
> >>>>>> +  - Bjorn Andersson <andersson@kernel.org>
> >>>>>> +  - Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>>>> +  - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>> +
> >>>>>> +properties:
> >>>>>> +  compatible:
> >>>>>> +    items:
> >>>>>> +      - enum:
> >>>>>> +          - qcom,shm-bridge-sa8775p
> >>>>>> +          - qcom,shm-bridge-sm8150
> >>>>>> +          - qcom,shm-bridge-sm8450
> >>>>>> +      - const: qcom,shm-bridge
> >>>>>> +
> >>>>>
> >>>>> Looks quite empty... Why this cannot be part of qcom,scm? IOW, why do
> >>>>> you need new binding if you do not have any resources here and the block
> >>>>> is essentially feature of qcom,scm firmware?
> >>>> Since it's "discoverable" (via retval of an scm call), I'd second the
> >>>> idea of probing this from within the SCM driver.
> >>>>
> >>>> Konrad
> >>>
> >>> Downstream has a bunch of DT switches that we don't support for now
> >>> upstream. I disagree about shoehorning this into the SCM driver. It
> >>> really is a layer on top of SCM but also SCM is a user of this
> >>> interface.
> >>
> >> Sure, for the driver makes sense, but it does not really explain why DT
> >> node is needed. It is not separate hardware. I doubt it is even separate
> >> firmware, but part of SCM.
> >>
> >> Best regards,
> >> Krzysztof
> >>
> >
> > Because not all platforms support it and it's the simplest way of
>
> Platforms like SoCs or boards?
>
> > marking the ones that do. Both SHM and SCM nodes sit on the firmware
> > node anyway. What do you recommend? A property of the SCM node? Like
> > 'qcom,shm-bridge` or something?
>
> If the first - you talk about SoCs - then you have everything needed
> already: SCM compatibles. This defines it fully.
>
> If it varies by boards with one SoC, would be different case, but I
> really doubt it.
>

Ok, makes sense. Thanks.

Bartosz

[PATCH 06/11] firmware: qcom-shm-bridge: new driver

[Bartosz Golaszewski]

This module is a platform driver that also exposes an interface for
kernel users to allocate blocks of memory shared with the trustzone.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/firmware/Kconfig                 |   8 +
 drivers/firmware/Makefile                |   1 +
 drivers/firmware/qcom-shm-bridge.c       | 452 +++++++++++++++++++++++
 include/linux/firmware/qcom/shm-bridge.h |  32 ++
 4 files changed, 493 insertions(+)
 create mode 100644 drivers/firmware/qcom-shm-bridge.c
 create mode 100644 include/linux/firmware/qcom/shm-bridge.h

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b59e3041fd62..39f35ba18779 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -226,6 +226,14 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
 
 	  Say Y here to enable "download mode" by default.
 
+config QCOM_SHM_BRIDGE
+	bool "Qualcomm SHM bridge driver"
+	depends on QCOM_SCM
+	help
+	  Say yes here to enable support for Qualcomm TEE Shared Memory Bridge.
+	  This module exposes interfaces that allow kernel users to allocate
+	  blocks of memory shared with the trustzone.
+
 config SYSFB
 	bool
 	select BOOT_VESA_SUPPORT
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 28fcddcd688f..ba1590cf959c 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
 obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
 obj-$(CONFIG_QCOM_SCM)		+= qcom-scm.o
 qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
+obj-$(CONFIG_QCOM_SHM_BRIDGE)	+= qcom-shm-bridge.o
 obj-$(CONFIG_SYSFB)		+= sysfb.o
 obj-$(CONFIG_SYSFB_SIMPLEFB)	+= sysfb_simplefb.o
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
diff --git a/drivers/firmware/qcom-shm-bridge.c b/drivers/firmware/qcom-shm-bridge.c
new file mode 100644
index 000000000000..db76c5c5061d
--- /dev/null
+++ b/drivers/firmware/qcom-shm-bridge.c
@@ -0,0 +1,452 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 Linaro Limited
+ */
+
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/device/bus.h>
+#include <linux/dma-mapping.h>
+#include <linux/export.h>
+#include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/firmware/qcom/shm-bridge.h>
+#include <linux/genalloc.h>
+#include <linux/init.h>
+#include <linux/kref.h>
+#include <linux/list.h>
+#include <linux/mod_devicetable.h>
+#include <linux/moduleparam.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/radix-tree.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
+
+DEFINE_FREE(qcom_shm_bridge_pool, struct qcom_shm_bridge_pool *,
+	    if (_T) qcom_shm_bridge_pool_unref(_T))
+
+/*
+ * Size of the global fall-back pool can be adjusted using this param. Use 4M
+ * as a sane default.
+ */
+static unsigned int qcom_shm_bridge_default_pool_size = SZ_4M;
+module_param_named(default_pool_size, qcom_shm_bridge_default_pool_size,
+		   uint, 0644);
+
+struct qcom_shm_bridge_pool {
+	struct device *dev;
+	void *vaddr;
+	phys_addr_t paddr;
+	size_t size;
+	uint64_t handle;
+	struct gen_pool *genpool;
+	struct list_head chunks;
+	spinlock_t lock;
+	struct kref refcount;
+};
+
+struct qcom_shm_bridge_chunk {
+	void *vaddr;
+	size_t size;
+	struct qcom_shm_bridge_pool *parent;
+	struct list_head siblings;
+};
+
+/* This is the global fall-back pool, used if user doesn't supply their own. */
+static struct qcom_shm_bridge_pool *qcom_shm_bridge_default_pool;
+
+static RADIX_TREE(qcom_shm_bridge_chunks, GFP_ATOMIC);
+static DEFINE_SPINLOCK(qcom_shm_bridge_chunks_lock);
+
+static void qcom_shm_bridge_pool_release(struct kref *kref)
+{
+	struct qcom_shm_bridge_pool *pool =
+		container_of(kref, struct qcom_shm_bridge_pool, refcount);
+
+	if (pool->handle)
+		qcom_scm_delete_shm_bridge(pool->dev, pool->handle);
+
+	if (pool->genpool)
+		gen_pool_destroy(pool->genpool);
+
+	if (pool->paddr)
+		dma_unmap_single(pool->dev, pool->paddr, pool->size,
+				 DMA_TO_DEVICE);
+
+	if (pool->vaddr)
+		free_pages((unsigned long)pool->vaddr, get_order(pool->size));
+
+	put_device(pool->dev);
+	kfree(pool);
+}
+
+static int qcom_shm_bridge_create(struct qcom_shm_bridge_pool *pool)
+{
+	uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_vmids,
+		 ns_perms, handle;
+	int ret;
+
+	ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
+	ns_vmids = QCOM_SCM_VMID_HLOS;
+	pfn_and_ns_perm = (u64)pool->paddr | ns_perms;
+	ipfn_and_s_perm = (u64)pool->paddr | ns_perms;
+	size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
+
+	ret = qcom_scm_create_shm_bridge(pool->dev, pfn_and_ns_perm,
+					 ipfn_and_s_perm, size_and_flags,
+					 ns_vmids, &handle);
+	if (!ret)
+		pool->handle = handle;
+
+	return ret;
+}
+
+static struct qcom_shm_bridge_pool *
+qcom_shm_bridge_pool_new_for_dev(struct device *dev, size_t size)
+{
+	struct qcom_shm_bridge_pool *pool __free(qcom_shm_bridge_pool) = NULL;
+	int ret;
+
+	if (!qcom_scm_shm_bridge_available())
+		return ERR_PTR(-ENODEV);
+
+	if (!size)
+		return ERR_PTR(-EINVAL);
+
+	pool = kzalloc(sizeof(*pool), GFP_KERNEL);
+	if (!pool)
+		return ERR_PTR(-ENOMEM);
+
+	pool->dev = get_device(dev);
+	pool->size = size;
+	INIT_LIST_HEAD(&pool->chunks);
+	spin_lock_init(&pool->lock);
+	kref_init(&pool->refcount);
+
+	pool->vaddr = (void *)__get_free_pages(GFP_KERNEL | GFP_DMA,
+					       get_order(pool->size));
+	if (!pool->vaddr)
+		return ERR_PTR(-ENOMEM);
+
+	pool->paddr = dma_map_single(pool->dev, pool->vaddr, pool->size,
+				     DMA_TO_DEVICE);
+	if (dma_mapping_error(dev, pool->paddr))
+		return ERR_PTR(-ENOMEM);
+
+	pool->genpool = gen_pool_create(PAGE_SHIFT, -1);
+	if (!pool->genpool)
+		return ERR_PTR(-ENOMEM);
+
+	gen_pool_set_algo(pool->genpool, gen_pool_best_fit, NULL);
+
+	ret = gen_pool_add_virt(pool->genpool, (unsigned long)pool->vaddr,
+				pool->paddr, pool->size, -1);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = qcom_shm_bridge_create(pool);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return no_free_ptr(pool);
+}
+
+/**
+ * qcom_shm_bridge_pool_new - Create a new SHM Bridge memory pool.
+ *
+ * @size: Size of the pool.
+ *
+ * Creates a new Shared Memory Bridge pool from which users can allocate memory
+ * chunks. Must be called from process context.
+ *
+ * Return:
+ * Pointer to the newly created SHM Bridge pool with reference count set to 1
+ * or ERR_PTR().
+ */
+struct qcom_shm_bridge_pool *qcom_shm_bridge_pool_new(size_t size)
+{
+	struct device *dev;
+
+	dev = bus_find_device_by_name(&platform_bus_type, NULL,
+				      "qcom-shm-bridge");
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
+	return qcom_shm_bridge_pool_new_for_dev(dev, size);
+}
+EXPORT_SYMBOL_GPL(qcom_shm_bridge_pool_new);
+
+/**
+ * qcom_shm_bridge_pool_ref - Increate the refcount of an SHM Bridge pool.
+ *
+ * @pool: SHM Bridge pool of which the reference count to increase.
+ *
+ * Return:
+ * Pointer to the same pool object.
+ */
+struct qcom_shm_bridge_pool *
+qcom_shm_bridge_pool_ref(struct qcom_shm_bridge_pool *pool)
+{
+	kref_get(&pool->refcount);
+
+	return pool;
+}
+EXPORT_SYMBOL_GPL(qcom_shm_bridge_pool_ref);
+
+/**
+ * qcom_shm_bridge_pool_unref - Decrease the refcount of an SHM Bridge pool.
+ *
+ * @pool: SHM Bridge pool of which the reference count to decrease.
+ *
+ * Once the reference count reaches 0, the pool is released.
+ */
+void qcom_shm_bridge_pool_unref(struct qcom_shm_bridge_pool *pool)
+{
+	kref_put(&pool->refcount, qcom_shm_bridge_pool_release);
+}
+EXPORT_SYMBOL_GPL(qcom_shm_bridge_pool_unref);
+
+static void devm_qcom_shm_bridge_pool_unref(void *data)
+{
+	struct qcom_shm_bridge_pool *pool = data;
+
+	qcom_shm_bridge_pool_unref(pool);
+}
+
+/**
+ * devm_qcom_shm_bridge_pool_new - Managed variant of qcom_shm_bridge_pool_new.
+ *
+ * @dev: Device for which to map memory and which will manage this pool.
+ * @size: Size of the pool.
+ *
+ * Return:
+ * Pointer to the newly created SHM Bridge pool with reference count set to 1
+ * or ERR_PTR().
+ */
+struct qcom_shm_bridge_pool *
+devm_qcom_shm_bridge_pool_new(struct device *dev, size_t size)
+{
+	struct qcom_shm_bridge_pool *pool;
+	int ret;
+
+	pool = qcom_shm_bridge_pool_new(size);
+	if (IS_ERR(pool))
+		return pool;
+
+	ret = devm_add_action_or_reset(dev, devm_qcom_shm_bridge_pool_unref,
+				       pool);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return pool;
+}
+EXPORT_SYMBOL_GPL(devm_qcom_shm_bridge_pool_new);
+
+/**
+ * qcom_shm_bridge_alloc - Allocate a chunk of memory from an SHM Bridge pool.
+ *
+ * @pool: Pool to allocate memory from. May be NULL.
+ * @size: Number of bytes to allocate.
+ * @gfp: Allocation flags.
+ *
+ * If pool is NULL then the global fall-back pool is used.
+ *
+ * Return:
+ * Virtual address of the allocated memory or ERR_PTR(). Must be freed using
+ * qcom_shm_bridge_free().
+ */
+void *qcom_shm_bridge_alloc(struct qcom_shm_bridge_pool *pool,
+			    size_t size, gfp_t gfp)
+{
+	struct qcom_shm_bridge_chunk *chunk __free(kfree) = NULL;
+	unsigned long vaddr;
+	int ret;
+
+	if (!pool) {
+		pool = READ_ONCE(qcom_shm_bridge_default_pool);
+		if (!pool)
+			return ERR_PTR(-ENODEV);
+	}
+
+	if (!size || size > pool->size)
+		return ERR_PTR(-EINVAL);
+
+	size = roundup(size, 1 << PAGE_SHIFT);
+
+	chunk = kzalloc(sizeof(*chunk), gfp);
+	if (!chunk)
+		return ERR_PTR(-ENOMEM);
+
+	guard(spinlock_irqsave)(&pool->lock);
+
+	vaddr = gen_pool_alloc(pool->genpool, size);
+	if (!vaddr)
+		return ERR_PTR(-ENOMEM);
+
+	chunk->vaddr = (void *)vaddr;
+	chunk->size = size;
+	chunk->parent = pool;
+	list_add_tail(&chunk->siblings, &pool->chunks);
+	qcom_shm_bridge_pool_ref(pool);
+
+	guard(spinlock_irqsave)(&qcom_shm_bridge_chunks_lock);
+
+	ret = radix_tree_insert(&qcom_shm_bridge_chunks, vaddr, chunk);
+	if (ret) {
+		gen_pool_free(pool->genpool, vaddr, chunk->size);
+		return ERR_PTR(ret);
+	}
+
+	return no_free_ptr(chunk)->vaddr;
+}
+EXPORT_SYMBOL_GPL(qcom_shm_bridge_alloc);
+
+/**
+ * qcom_shm_bridge_free - Free SHM Bridge memory allocated from the pool.
+ *
+ * @vaddr: Virtual address of the allocated memory to free.
+ */
+void qcom_shm_bridge_free(void *vaddr)
+{
+	struct qcom_shm_bridge_chunk *chunk;
+	struct qcom_shm_bridge_pool *pool;
+
+	scoped_guard(spinlock_irqsave, &qcom_shm_bridge_chunks_lock)
+		chunk = radix_tree_delete_item(&qcom_shm_bridge_chunks,
+					       (unsigned long)vaddr, NULL);
+	if (!chunk)
+		goto out_warn;
+
+	pool = chunk->parent;
+
+	guard(spinlock_irqsave)(&pool->lock);
+
+	list_for_each_entry(chunk, &pool->chunks, siblings) {
+		if (vaddr != chunk->vaddr)
+			continue;
+
+		gen_pool_free(pool->genpool, (unsigned long)chunk->vaddr,
+			      chunk->size);
+		list_del(&chunk->siblings);
+		qcom_shm_bridge_pool_unref(pool);
+		kfree(chunk);
+		return;
+	}
+
+out_warn:
+	WARN(1, "Virtual address %p not allocated for SHM bridge", vaddr);
+}
+EXPORT_SYMBOL_GPL(qcom_shm_bridge_free);
+
+/**
+ * devm_qcom_shm_bridge_alloc - Managed variant of qcom_shm_bridge_alloc.
+ *
+ * @dev: Managing device.
+ * @pool: Pool to allocate memory from.
+ * @size: Number of bytes to allocate.
+ * @gfp: Allocation flags.
+ *
+ * Return:
+ * Virtual address of the allocated memory or ERR_PTR().
+ */
+void *devm_qcom_shm_bridge_alloc(struct device *dev,
+				 struct qcom_shm_bridge_pool *pool,
+				 size_t size, gfp_t gfp)
+{
+	void *vaddr;
+	int ret;
+
+	vaddr = qcom_shm_bridge_alloc(pool, size, gfp);
+	if (IS_ERR(vaddr))
+		return vaddr;
+
+	ret = devm_add_action_or_reset(dev, qcom_shm_bridge_free, vaddr);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return vaddr;
+}
+EXPORT_SYMBOL_GPL(devm_qcom_shm_bridge_alloc);
+
+/**
+ * qcom_shm_bridge_to_phys_addr - Translate address from virtual to physical.
+ *
+ * @vaddr: Virtual address to translate.
+ *
+ * Return:
+ * Physical address corresponding to 'vaddr'.
+ */
+phys_addr_t qcom_shm_bridge_to_phys_addr(void *vaddr)
+{
+	struct qcom_shm_bridge_chunk *chunk;
+	struct qcom_shm_bridge_pool *pool;
+
+	guard(spinlock_irqsave)(&qcom_shm_bridge_chunks_lock);
+
+	chunk = radix_tree_lookup(&qcom_shm_bridge_chunks,
+				  (unsigned long)vaddr);
+	if (!chunk)
+		return 0;
+
+	pool = chunk->parent;
+
+	guard(spinlock_irqsave)(&pool->lock);
+
+	return gen_pool_virt_to_phys(pool->genpool, (unsigned long)vaddr);
+}
+EXPORT_SYMBOL_GPL(qcom_shm_bridge_to_phys_addr);
+
+static int qcom_shm_bridge_probe(struct platform_device *pdev)
+{
+	struct qcom_shm_bridge_pool *default_pool;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	/*
+	 * We need to wait for the SCM device to be created and bound to the
+	 * SCM driver.
+	 */
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
+	ret = qcom_scm_enable_shm_bridge();
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to enable the SHM bridge\n");
+
+	default_pool = qcom_shm_bridge_pool_new_for_dev(
+				dev, qcom_shm_bridge_default_pool_size);
+	if (IS_ERR(default_pool))
+		return dev_err_probe(dev, PTR_ERR(default_pool),
+				     "Failed to create the default SHM Bridge pool\n");
+
+	WRITE_ONCE(qcom_shm_bridge_default_pool, default_pool);
+
+	return 0;
+}
+
+static const struct of_device_id qcom_shm_bridge_of_match[] = {
+	{ .compatible = "qcom,shm-bridge", },
+	{ }
+};
+
+static struct platform_driver qcom_shm_bridge_driver = {
+	.driver = {
+		.name = "qcom-shm-bridge",
+		.of_match_table = qcom_shm_bridge_of_match,
+		/*
+		 * Once enabled, the SHM Bridge feature cannot be disabled so
+		 * there's no reason to ever unbind the driver.
+		 */
+		.suppress_bind_attrs = true,
+	},
+	.probe = qcom_shm_bridge_probe,
+};
+
+static int __init qcom_shm_bridge_init(void)
+{
+	return platform_driver_register(&qcom_shm_bridge_driver);
+}
+subsys_initcall(qcom_shm_bridge_init);
diff --git a/include/linux/firmware/qcom/shm-bridge.h b/include/linux/firmware/qcom/shm-bridge.h
new file mode 100644
index 000000000000..df066a2f6d91
--- /dev/null
+++ b/include/linux/firmware/qcom/shm-bridge.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Linaro Limited
+ */
+
+#ifndef _LINUX_QCOM_SHM_BRIDGE
+#define _LINUX_QCOM_SHM_BRIDGE
+
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/gfp.h>
+#include <linux/types.h>
+
+struct qcom_shm_bridge_pool;
+
+struct qcom_shm_bridge_pool *qcom_shm_bridge_pool_new(size_t size);
+struct qcom_shm_bridge_pool *
+qcom_shm_bridge_pool_ref(struct qcom_shm_bridge_pool *pool);
+void qcom_shm_bridge_pool_unref(struct qcom_shm_bridge_pool *pool);
+struct qcom_shm_bridge_pool *
+devm_qcom_shm_bridge_pool_new(struct device *dev, size_t size);
+
+void *qcom_shm_bridge_alloc(struct qcom_shm_bridge_pool *pool,
+			    size_t size, gfp_t gfp);
+void qcom_shm_bridge_free(void *vaddr);
+void *devm_qcom_shm_bridge_alloc(struct device *dev,
+				 struct qcom_shm_bridge_pool *pool,
+				 size_t size, gfp_t gfp);
+
+phys_addr_t qcom_shm_bridge_to_phys_addr(void *vaddr);
+
+#endif /* _LINUX_QCOM_SHM_BRIDGE */
-- 
2.39.2

Re: [PATCH 06/11] firmware: qcom-shm-bridge: new driver

[Krzysztof Kozlowski]

On 28/08/2023 21:25, Bartosz Golaszewski wrote:
> This module is a platform driver that also exposes an interface for
> kernel users to allocate blocks of memory shared with the trustzone.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/firmware/Kconfig                 |   8 +
>  drivers/firmware/Makefile                |   1 +
>  drivers/firmware/qcom-shm-bridge.c       | 452 +++++++++++++++++++++++
>  include/linux/firmware/qcom/shm-bridge.h |  32 ++
>  4 files changed, 493 insertions(+)
>  create mode 100644 drivers/firmware/qcom-shm-bridge.c
>  create mode 100644 include/linux/firmware/qcom/shm-bridge.h
> 

...

> +/**
> + * qcom_shm_bridge_to_phys_addr - Translate address from virtual to physical.
> + *
> + * @vaddr: Virtual address to translate.
> + *
> + * Return:
> + * Physical address corresponding to 'vaddr'.
> + */
> +phys_addr_t qcom_shm_bridge_to_phys_addr(void *vaddr)
> +{
> +	struct qcom_shm_bridge_chunk *chunk;
> +	struct qcom_shm_bridge_pool *pool;
> +
> +	guard(spinlock_irqsave)(&qcom_shm_bridge_chunks_lock);
> +
> +	chunk = radix_tree_lookup(&qcom_shm_bridge_chunks,
> +				  (unsigned long)vaddr);
> +	if (!chunk)
> +		return 0;
> +
> +	pool = chunk->parent;
> +
> +	guard(spinlock_irqsave)(&pool->lock);

Why both locks are spinlocks? The locks are used quite a lot.

> +
> +	return gen_pool_virt_to_phys(pool->genpool, (unsigned long)vaddr);
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_to_phys_addr);
> +
> +static int qcom_shm_bridge_probe(struct platform_device *pdev)
> +{
> +	struct qcom_shm_bridge_pool *default_pool;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	/*
> +	 * We need to wait for the SCM device to be created and bound to the
> +	 * SCM driver.
> +	 */
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;

I think we miss here (and in all other drivers) device links to qcm.

> +
> +	ret = qcom_scm_enable_shm_bridge();
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable the SHM bridge\n");
> +
> +	default_pool = qcom_shm_bridge_pool_new_for_dev(
> +				dev, qcom_shm_bridge_default_pool_size);
> +	if (IS_ERR(default_pool))
> +		return dev_err_probe(dev, PTR_ERR(default_pool),
> +				     "Failed to create the default SHM Bridge pool\n");
> +
> +	WRITE_ONCE(qcom_shm_bridge_default_pool, default_pool);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id qcom_shm_bridge_of_match[] = {
> +	{ .compatible = "qcom,shm-bridge", },
> +	{ }
> +};
> +
> +static struct platform_driver qcom_shm_bridge_driver = {
> +	.driver = {
> +		.name = "qcom-shm-bridge",
> +		.of_match_table = qcom_shm_bridge_of_match,
> +		/*
> +		 * Once enabled, the SHM Bridge feature cannot be disabled so
> +		 * there's no reason to ever unbind the driver.
> +		 */
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe = qcom_shm_bridge_probe,
> +};
> +
> +static int __init qcom_shm_bridge_init(void)
> +{
> +	return platform_driver_register(&qcom_shm_bridge_driver);
> +}
> +subsys_initcall(qcom_shm_bridge_init);

Why this is part of subsystem? Should be rather device_initcall... or
simply module (and a tristate).

Best regards,
Krzysztof

Re: [PATCH 06/11] firmware: qcom-shm-bridge: new driver

[Bartosz Golaszewski]

On Tue, 29 Aug 2023 at 10:18, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 28/08/2023 21:25, Bartosz Golaszewski wrote:
> > This module is a platform driver that also exposes an interface for
> > kernel users to allocate blocks of memory shared with the trustzone.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/firmware/Kconfig                 |   8 +
> >  drivers/firmware/Makefile                |   1 +
> >  drivers/firmware/qcom-shm-bridge.c       | 452 +++++++++++++++++++++++
> >  include/linux/firmware/qcom/shm-bridge.h |  32 ++
> >  4 files changed, 493 insertions(+)
> >  create mode 100644 drivers/firmware/qcom-shm-bridge.c
> >  create mode 100644 include/linux/firmware/qcom/shm-bridge.h
> >
>
> ...
>
> > +/**
> > + * qcom_shm_bridge_to_phys_addr - Translate address from virtual to physical.
> > + *
> > + * @vaddr: Virtual address to translate.
> > + *
> > + * Return:
> > + * Physical address corresponding to 'vaddr'.
> > + */
> > +phys_addr_t qcom_shm_bridge_to_phys_addr(void *vaddr)
> > +{
> > +     struct qcom_shm_bridge_chunk *chunk;
> > +     struct qcom_shm_bridge_pool *pool;
> > +
> > +     guard(spinlock_irqsave)(&qcom_shm_bridge_chunks_lock);
> > +
> > +     chunk = radix_tree_lookup(&qcom_shm_bridge_chunks,
> > +                               (unsigned long)vaddr);
> > +     if (!chunk)
> > +             return 0;
> > +
> > +     pool = chunk->parent;
> > +
> > +     guard(spinlock_irqsave)(&pool->lock);
>
> Why both locks are spinlocks? The locks are used quite a lot.

I'm not sure what to answer. The first one protects the global chunk
mapping stored in the radix tree. The second one protects a single
memory pool from concurrent access. Both can be modified from any
context, hence spinlocks.

>
> > +
> > +     return gen_pool_virt_to_phys(pool->genpool, (unsigned long)vaddr);
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_shm_bridge_to_phys_addr);
> > +
> > +static int qcom_shm_bridge_probe(struct platform_device *pdev)
> > +{
> > +     struct qcom_shm_bridge_pool *default_pool;
> > +     struct device *dev = &pdev->dev;
> > +     int ret;
> > +
> > +     /*
> > +      * We need to wait for the SCM device to be created and bound to the
> > +      * SCM driver.
> > +      */
> > +     if (!qcom_scm_is_available())
> > +             return -EPROBE_DEFER;
>
> I think we miss here (and in all other drivers) device links to qcm.
>

Well, SCM, once probed, cannot be unbound. What would device links
guarantee above that?

> > +
> > +     ret = qcom_scm_enable_shm_bridge();
> > +     if (ret)
> > +             return dev_err_probe(dev, ret,
> > +                                  "Failed to enable the SHM bridge\n");
> > +
> > +     default_pool = qcom_shm_bridge_pool_new_for_dev(
> > +                             dev, qcom_shm_bridge_default_pool_size);
> > +     if (IS_ERR(default_pool))
> > +             return dev_err_probe(dev, PTR_ERR(default_pool),
> > +                                  "Failed to create the default SHM Bridge pool\n");
> > +
> > +     WRITE_ONCE(qcom_shm_bridge_default_pool, default_pool);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id qcom_shm_bridge_of_match[] = {
> > +     { .compatible = "qcom,shm-bridge", },
> > +     { }
> > +};
> > +
> > +static struct platform_driver qcom_shm_bridge_driver = {
> > +     .driver = {
> > +             .name = "qcom-shm-bridge",
> > +             .of_match_table = qcom_shm_bridge_of_match,
> > +             /*
> > +              * Once enabled, the SHM Bridge feature cannot be disabled so
> > +              * there's no reason to ever unbind the driver.
> > +              */
> > +             .suppress_bind_attrs = true,
> > +     },
> > +     .probe = qcom_shm_bridge_probe,
> > +};
> > +
> > +static int __init qcom_shm_bridge_init(void)
> > +{
> > +     return platform_driver_register(&qcom_shm_bridge_driver);
> > +}
> > +subsys_initcall(qcom_shm_bridge_init);
>
> Why this is part of subsystem? Should be rather device_initcall... or
> simply module (and a tristate).
>

We want it to get up as soon as possible (right after SCM, because SCM
is the first user).

Bartosz

> Best regards,
> Krzysztof
>

Re: [PATCH 06/11] firmware: qcom-shm-bridge: new driver

[Krzysztof Kozlowski]

On 29/08/2023 15:24, Bartosz Golaszewski wrote:
>>> +phys_addr_t qcom_shm_bridge_to_phys_addr(void *vaddr)
>>> +{
>>> +     struct qcom_shm_bridge_chunk *chunk;
>>> +     struct qcom_shm_bridge_pool *pool;
>>> +
>>> +     guard(spinlock_irqsave)(&qcom_shm_bridge_chunks_lock);
>>> +
>>> +     chunk = radix_tree_lookup(&qcom_shm_bridge_chunks,
>>> +                               (unsigned long)vaddr);
>>> +     if (!chunk)
>>> +             return 0;
>>> +
>>> +     pool = chunk->parent;
>>> +
>>> +     guard(spinlock_irqsave)(&pool->lock);
>>
>> Why both locks are spinlocks? The locks are used quite a lot.
> 
> I'm not sure what to answer. The first one protects the global chunk
> mapping stored in the radix tree. The second one protects a single
> memory pool from concurrent access. Both can be modified from any
> context, hence spinlocks.

Not much PREEMPT friendly, although indeed protected code is small. At
least here, I did not check other places.

> 
>>
>>> +
>>> +     return gen_pool_virt_to_phys(pool->genpool, (unsigned long)vaddr);
>>> +}
>>> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_to_phys_addr);
>>> +
>>> +static int qcom_shm_bridge_probe(struct platform_device *pdev)
>>> +{
>>> +     struct qcom_shm_bridge_pool *default_pool;
>>> +     struct device *dev = &pdev->dev;
>>> +     int ret;
>>> +
>>> +     /*
>>> +      * We need to wait for the SCM device to be created and bound to the
>>> +      * SCM driver.
>>> +      */
>>> +     if (!qcom_scm_is_available())
>>> +             return -EPROBE_DEFER;
>>
>> I think we miss here (and in all other drivers) device links to qcm.
>>
> 
> Well, SCM, once probed, cannot be unbound. What would device links
> guarantee above that?

Runtime PM, probe ordering (dependencies) detection.

> 
>>> +
>>> +     ret = qcom_scm_enable_shm_bridge();
>>> +     if (ret)
>>> +             return dev_err_probe(dev, ret,
>>> +                                  "Failed to enable the SHM bridge\n");
>>> +
>>> +     default_pool = qcom_shm_bridge_pool_new_for_dev(
>>> +                             dev, qcom_shm_bridge_default_pool_size);
>>> +     if (IS_ERR(default_pool))
>>> +             return dev_err_probe(dev, PTR_ERR(default_pool),
>>> +                                  "Failed to create the default SHM Bridge pool\n");
>>> +
>>> +     WRITE_ONCE(qcom_shm_bridge_default_pool, default_pool);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct of_device_id qcom_shm_bridge_of_match[] = {
>>> +     { .compatible = "qcom,shm-bridge", },
>>> +     { }
>>> +};
>>> +
>>> +static struct platform_driver qcom_shm_bridge_driver = {
>>> +     .driver = {
>>> +             .name = "qcom-shm-bridge",
>>> +             .of_match_table = qcom_shm_bridge_of_match,
>>> +             /*
>>> +              * Once enabled, the SHM Bridge feature cannot be disabled so
>>> +              * there's no reason to ever unbind the driver.
>>> +              */
>>> +             .suppress_bind_attrs = true,
>>> +     },
>>> +     .probe = qcom_shm_bridge_probe,
>>> +};
>>> +
>>> +static int __init qcom_shm_bridge_init(void)
>>> +{
>>> +     return platform_driver_register(&qcom_shm_bridge_driver);
>>> +}
>>> +subsys_initcall(qcom_shm_bridge_init);
>>
>> Why this is part of subsystem? Should be rather device_initcall... or
>> simply module (and a tristate).
>>
> 
> We want it to get up as soon as possible (right after SCM, because SCM
> is the first user).

Then probably should be populated/spawned by SCM.

Best regards,
Krzysztof

Re: [PATCH 06/11] firmware: qcom-shm-bridge: new driver

[Bartosz Golaszewski]

On Tue, 29 Aug 2023 at 18:47, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 29/08/2023 15:24, Bartosz Golaszewski wrote:
> >>> +phys_addr_t qcom_shm_bridge_to_phys_addr(void *vaddr)
> >>> +{
> >>> +     struct qcom_shm_bridge_chunk *chunk;
> >>> +     struct qcom_shm_bridge_pool *pool;
> >>> +
> >>> +     guard(spinlock_irqsave)(&qcom_shm_bridge_chunks_lock);
> >>> +
> >>> +     chunk = radix_tree_lookup(&qcom_shm_bridge_chunks,
> >>> +                               (unsigned long)vaddr);
> >>> +     if (!chunk)
> >>> +             return 0;
> >>> +
> >>> +     pool = chunk->parent;
> >>> +
> >>> +     guard(spinlock_irqsave)(&pool->lock);
> >>
> >> Why both locks are spinlocks? The locks are used quite a lot.
> >
> > I'm not sure what to answer. The first one protects the global chunk
> > mapping stored in the radix tree. The second one protects a single
> > memory pool from concurrent access. Both can be modified from any
> > context, hence spinlocks.
>
> Not much PREEMPT friendly, although indeed protected code is small. At
> least here, I did not check other places.
>
> >
> >>
> >>> +
> >>> +     return gen_pool_virt_to_phys(pool->genpool, (unsigned long)vaddr);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_to_phys_addr);
> >>> +
> >>> +static int qcom_shm_bridge_probe(struct platform_device *pdev)
> >>> +{
> >>> +     struct qcom_shm_bridge_pool *default_pool;
> >>> +     struct device *dev = &pdev->dev;
> >>> +     int ret;
> >>> +
> >>> +     /*
> >>> +      * We need to wait for the SCM device to be created and bound to the
> >>> +      * SCM driver.
> >>> +      */
> >>> +     if (!qcom_scm_is_available())
> >>> +             return -EPROBE_DEFER;
> >>
> >> I think we miss here (and in all other drivers) device links to qcm.
> >>
> >
> > Well, SCM, once probed, cannot be unbound. What would device links
> > guarantee above that?
>
> Runtime PM, probe ordering (dependencies) detection.
>

Shouldn't we cross that bridge when we get there? SCM has no support
for runtime PM. Probe ordering is quite well handled with a simple
probe deferral. This is also not a parent-child relationship. SHM
Bridge calls into the trustzone using SCM, but SCM is also a user of
SHM Bridge.

> >
> >>> +
> >>> +     ret = qcom_scm_enable_shm_bridge();
> >>> +     if (ret)
> >>> +             return dev_err_probe(dev, ret,
> >>> +                                  "Failed to enable the SHM bridge\n");
> >>> +
> >>> +     default_pool = qcom_shm_bridge_pool_new_for_dev(
> >>> +                             dev, qcom_shm_bridge_default_pool_size);
> >>> +     if (IS_ERR(default_pool))
> >>> +             return dev_err_probe(dev, PTR_ERR(default_pool),
> >>> +                                  "Failed to create the default SHM Bridge pool\n");
> >>> +
> >>> +     WRITE_ONCE(qcom_shm_bridge_default_pool, default_pool);
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static const struct of_device_id qcom_shm_bridge_of_match[] = {
> >>> +     { .compatible = "qcom,shm-bridge", },
> >>> +     { }
> >>> +};
> >>> +
> >>> +static struct platform_driver qcom_shm_bridge_driver = {
> >>> +     .driver = {
> >>> +             .name = "qcom-shm-bridge",
> >>> +             .of_match_table = qcom_shm_bridge_of_match,
> >>> +             /*
> >>> +              * Once enabled, the SHM Bridge feature cannot be disabled so
> >>> +              * there's no reason to ever unbind the driver.
> >>> +              */
> >>> +             .suppress_bind_attrs = true,
> >>> +     },
> >>> +     .probe = qcom_shm_bridge_probe,
> >>> +};
> >>> +
> >>> +static int __init qcom_shm_bridge_init(void)
> >>> +{
> >>> +     return platform_driver_register(&qcom_shm_bridge_driver);
> >>> +}
> >>> +subsys_initcall(qcom_shm_bridge_init);
> >>
> >> Why this is part of subsystem? Should be rather device_initcall... or
> >> simply module (and a tristate).
> >>
> >
> > We want it to get up as soon as possible (right after SCM, because SCM
> > is the first user).
>
> Then probably should be populated/spawned by SCM.
>

I really prefer probe deferral over one platform driver creating
platform devices for another. The device is on the DT, let's let OF
populate it as it should.

Bart

Re: [PATCH 06/11] firmware: qcom-shm-bridge: new driver

[Krzysztof Kozlowski]

On 30/08/2023 15:09, Bartosz Golaszewski wrote:

>>>>> +
>>>>> +     return gen_pool_virt_to_phys(pool->genpool, (unsigned long)vaddr);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_to_phys_addr);
>>>>> +
>>>>> +static int qcom_shm_bridge_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +     struct qcom_shm_bridge_pool *default_pool;
>>>>> +     struct device *dev = &pdev->dev;
>>>>> +     int ret;
>>>>> +
>>>>> +     /*
>>>>> +      * We need to wait for the SCM device to be created and bound to the
>>>>> +      * SCM driver.
>>>>> +      */
>>>>> +     if (!qcom_scm_is_available())
>>>>> +             return -EPROBE_DEFER;
>>>>
>>>> I think we miss here (and in all other drivers) device links to qcm.
>>>>
>>>
>>> Well, SCM, once probed, cannot be unbound. What would device links
>>> guarantee above that?
>>
>> Runtime PM, probe ordering (dependencies) detection.
>>
> 
> Shouldn't we cross that bridge when we get there? SCM has no support
> for runtime PM. Probe ordering is quite well handled with a simple
> probe deferral. This is also not a parent-child relationship. SHM
> Bridge calls into the trustzone using SCM, but SCM is also a user of
> SHM Bridge.

OK

Best regards,
Krzysztof

Re: [PATCH 06/11] firmware: qcom-shm-bridge: new driver

[Krzysztof Kozlowski]

On 28/08/2023 21:25, Bartosz Golaszewski wrote:
> +/**
> + * qcom_shm_bridge_pool_new - Create a new SHM Bridge memory pool.
> + *
> + * @size: Size of the pool.
> + *
> + * Creates a new Shared Memory Bridge pool from which users can allocate memory
> + * chunks. Must be called from process context.
> + *
> + * Return:
> + * Pointer to the newly created SHM Bridge pool with reference count set to 1
> + * or ERR_PTR().
> + */
> +struct qcom_shm_bridge_pool *qcom_shm_bridge_pool_new(size_t size)
> +{
> +	struct device *dev;
> +
> +	dev = bus_find_device_by_name(&platform_bus_type, NULL,
> +				      "qcom-shm-bridge");
> +	if (!dev)
> +		return ERR_PTR(-ENODEV);
> +
> +	return qcom_shm_bridge_pool_new_for_dev(dev, size);
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_pool_new);

I do not see an user of this. Do not add exported symbols without users.
Drop export. I would even suggest drop the function entirely, why do we
need dead code?

> +
> +/**
> + * qcom_shm_bridge_pool_ref - Increate the refcount of an SHM Bridge pool.
> + *
> + * @pool: SHM Bridge pool of which the reference count to increase.
> + *
> + * Return:
> + * Pointer to the same pool object.
> + */
> +struct qcom_shm_bridge_pool *
> +qcom_shm_bridge_pool_ref(struct qcom_shm_bridge_pool *pool)
> +{
> +	kref_get(&pool->refcount);
> +
> +	return pool;
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_pool_ref);

Ditto

> +
> +/**
> + * qcom_shm_bridge_pool_unref - Decrease the refcount of an SHM Bridge pool.
> + *
> + * @pool: SHM Bridge pool of which the reference count to decrease.
> + *
> + * Once the reference count reaches 0, the pool is released.
> + */
> +void qcom_shm_bridge_pool_unref(struct qcom_shm_bridge_pool *pool)
> +{
> +	kref_put(&pool->refcount, qcom_shm_bridge_pool_release);
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_pool_unref);

Ditto

> +
> +static void devm_qcom_shm_bridge_pool_unref(void *data)
> +{
> +	struct qcom_shm_bridge_pool *pool = data;
> +
> +	qcom_shm_bridge_pool_unref(pool);
> +}
> +
> +/**
> + * devm_qcom_shm_bridge_pool_new - Managed variant of qcom_shm_bridge_pool_new.
> + *
> + * @dev: Device for which to map memory and which will manage this pool.
> + * @size: Size of the pool.
> + *
> + * Return:
> + * Pointer to the newly created SHM Bridge pool with reference count set to 1
> + * or ERR_PTR().
> + */
> +struct qcom_shm_bridge_pool *
> +devm_qcom_shm_bridge_pool_new(struct device *dev, size_t size)
> +{
> +	struct qcom_shm_bridge_pool *pool;
> +	int ret;
> +
> +	pool = qcom_shm_bridge_pool_new(size);
> +	if (IS_ERR(pool))
> +		return pool;
> +
> +	ret = devm_add_action_or_reset(dev, devm_qcom_shm_bridge_pool_unref,
> +				       pool);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return pool;
> +}
> +EXPORT_SYMBOL_GPL(devm_qcom_shm_bridge_pool_new);

Ditto

> +
> +/**
> + * qcom_shm_bridge_alloc - Allocate a chunk of memory from an SHM Bridge pool.
> + *
> + * @pool: Pool to allocate memory from. May be NULL.
> + * @size: Number of bytes to allocate.
> + * @gfp: Allocation flags.
> + *
> + * If pool is NULL then the global fall-back pool is used.
> + *
> + * Return:
> + * Virtual address of the allocated memory or ERR_PTR(). Must be freed using
> + * qcom_shm_bridge_free().
> + */
> +void *qcom_shm_bridge_alloc(struct qcom_shm_bridge_pool *pool,
> +			    size_t size, gfp_t gfp)
> +{
> +	struct qcom_shm_bridge_chunk *chunk __free(kfree) = NULL;
> +	unsigned long vaddr;
> +	int ret;

...

> +
> +	return no_free_ptr(chunk)->vaddr;
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_alloc);
> +
> +/**
> + * qcom_shm_bridge_free - Free SHM Bridge memory allocated from the pool.
> + *
> + * @vaddr: Virtual address of the allocated memory to free.
> + */
> +void qcom_shm_bridge_free(void *vaddr)
> +{
> +	struct qcom_shm_bridge_chunk *chunk;
> +	struct qcom_shm_bridge_pool *pool;
> +
> +	scoped_guard(spinlock_irqsave, &qcom_shm_bridge_chunks_lock)
> +		chunk = radix_tree_delete_item(&qcom_shm_bridge_chunks,
> +					       (unsigned long)vaddr, NULL);
> +	if (!chunk)
> +		goto out_warn;
> +
> +	pool = chunk->parent;
> +
> +	guard(spinlock_irqsave)(&pool->lock);
> +
> +	list_for_each_entry(chunk, &pool->chunks, siblings) {
> +		if (vaddr != chunk->vaddr)
> +			continue;
> +
> +		gen_pool_free(pool->genpool, (unsigned long)chunk->vaddr,
> +			      chunk->size);
> +		list_del(&chunk->siblings);
> +		qcom_shm_bridge_pool_unref(pool);
> +		kfree(chunk);
> +		return;
> +	}
> +
> +out_warn:
> +	WARN(1, "Virtual address %p not allocated for SHM bridge", vaddr);
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_free);
> +

...

> +phys_addr_t qcom_shm_bridge_to_phys_addr(void *vaddr)
> +{
> +	struct qcom_shm_bridge_chunk *chunk;
> +	struct qcom_shm_bridge_pool *pool;
> +
> +	guard(spinlock_irqsave)(&qcom_shm_bridge_chunks_lock);
> +
> +	chunk = radix_tree_lookup(&qcom_shm_bridge_chunks,
> +				  (unsigned long)vaddr);
> +	if (!chunk)
> +		return 0;
> +
> +	pool = chunk->parent;
> +
> +	guard(spinlock_irqsave)(&pool->lock);
> +
> +	return gen_pool_virt_to_phys(pool->genpool, (unsigned long)vaddr);
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_to_phys_addr);

Ditto

Best regards,
Krzysztof

[PATCH 07/11] firmware: qcom-scm: use SHM bridge if available

[Bartosz Golaszewski]

Allocate the memory for SCM call arguments from the Shared Memory Bridge
if it's available.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/firmware/qcom_scm-smc.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
index 16cf88acfa8e..6045be600c2a 100644
--- a/drivers/firmware/qcom_scm-smc.c
+++ b/drivers/firmware/qcom_scm-smc.c
@@ -11,6 +11,7 @@
 #include <linux/firmware/qcom/qcom_scm.h>
 #include <linux/arm-smccc.h>
 #include <linux/dma-mapping.h>
+#include <linux/firmware/qcom/shm-bridge.h>
 
 #include "qcom_scm.h"
 
@@ -161,6 +162,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 				    ARM_SMCCC_SMC_32 : ARM_SMCCC_SMC_64;
 	struct arm_smccc_res smc_res;
 	struct arm_smccc_args smc = {0};
+	bool using_shm_bridge = qcom_scm_shm_bridge_available();
 
 	smc.args[0] = ARM_SMCCC_CALL_VAL(
 		smccc_call_type,
@@ -173,8 +175,12 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 
 	if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
 		alloc_len = SCM_SMC_N_EXT_ARGS * sizeof(u64);
-		args_virt = kzalloc(PAGE_ALIGN(alloc_len), flag);
-
+		if (using_shm_bridge)
+			args_virt = qcom_shm_bridge_alloc(NULL,
+							  PAGE_ALIGN(alloc_len),
+							  flag);
+		else
+			args_virt = kzalloc(PAGE_ALIGN(alloc_len), flag);
 		if (!args_virt)
 			return -ENOMEM;
 
@@ -196,7 +202,10 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 					   DMA_TO_DEVICE);
 
 		if (dma_mapping_error(dev, args_phys)) {
-			kfree(args_virt);
+			if (using_shm_bridge)
+				qcom_shm_bridge_free(args_virt);
+			else
+				kfree(args_virt);
 			return -ENOMEM;
 		}
 
@@ -208,7 +217,10 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 
 	if (args_virt) {
 		dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
-		kfree(args_virt);
+		if (using_shm_bridge)
+			qcom_shm_bridge_free(args_virt);
+		else
+			kfree(args_virt);
 	}
 
 	if (ret)
-- 
2.39.2

Re: [PATCH 07/11] firmware: qcom-scm: use SHM bridge if available

[kernel test robot]

Hi Bartosz,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on arm64/for-next/core linus/master v6.5]
[cannot apply to next-20230828]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/firmware-qcom-scm-drop-unneeded-extern-specifiers/20230829-032930
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230828192507.117334-8-bartosz.golaszewski%40linaro.org
patch subject: [PATCH 07/11] firmware: qcom-scm: use SHM bridge if available
config: m68k-randconfig-r006-20230829 (https://download.01.org/0day-ci/archive/20230829/202308291359.QP042fIw-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230829/202308291359.QP042fIw-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308291359.QP042fIw-lkp@intel.com/

All errors (new ones prefixed by >>):

   m68k-linux-ld: drivers/firmware/qcom_scm-smc.o: in function `__scm_smc_call':
>> drivers/firmware/qcom_scm-smc.c:179:(.text+0xa8): undefined reference to `qcom_shm_bridge_alloc'
>> m68k-linux-ld: drivers/firmware/qcom_scm-smc.c:221:(.text+0x1ac): undefined reference to `qcom_shm_bridge_free'
   m68k-linux-ld: drivers/firmware/qcom_scm-smc.c:206:(.text+0x23a): undefined reference to `qcom_shm_bridge_free'


vim +179 drivers/firmware/qcom_scm-smc.c

   148	
   149	
   150	int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
   151			   enum qcom_scm_convention qcom_convention,
   152			   struct qcom_scm_res *res, bool atomic)
   153	{
   154		int arglen = desc->arginfo & 0xf;
   155		int i, ret;
   156		dma_addr_t args_phys = 0;
   157		void *args_virt = NULL;
   158		size_t alloc_len;
   159		gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
   160		u32 smccc_call_type = atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL;
   161		u32 qcom_smccc_convention = (qcom_convention == SMC_CONVENTION_ARM_32) ?
   162					    ARM_SMCCC_SMC_32 : ARM_SMCCC_SMC_64;
   163		struct arm_smccc_res smc_res;
   164		struct arm_smccc_args smc = {0};
   165		bool using_shm_bridge = qcom_scm_shm_bridge_available();
   166	
   167		smc.args[0] = ARM_SMCCC_CALL_VAL(
   168			smccc_call_type,
   169			qcom_smccc_convention,
   170			desc->owner,
   171			SCM_SMC_FNID(desc->svc, desc->cmd));
   172		smc.args[1] = desc->arginfo;
   173		for (i = 0; i < SCM_SMC_N_REG_ARGS; i++)
   174			smc.args[i + SCM_SMC_FIRST_REG_IDX] = desc->args[i];
   175	
   176		if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
   177			alloc_len = SCM_SMC_N_EXT_ARGS * sizeof(u64);
   178			if (using_shm_bridge)
 > 179				args_virt = qcom_shm_bridge_alloc(NULL,
   180								  PAGE_ALIGN(alloc_len),
   181								  flag);
   182			else
   183				args_virt = kzalloc(PAGE_ALIGN(alloc_len), flag);
   184			if (!args_virt)
   185				return -ENOMEM;
   186	
   187			if (qcom_smccc_convention == ARM_SMCCC_SMC_32) {
   188				__le32 *args = args_virt;
   189	
   190				for (i = 0; i < SCM_SMC_N_EXT_ARGS; i++)
   191					args[i] = cpu_to_le32(desc->args[i +
   192							      SCM_SMC_FIRST_EXT_IDX]);
   193			} else {
   194				__le64 *args = args_virt;
   195	
   196				for (i = 0; i < SCM_SMC_N_EXT_ARGS; i++)
   197					args[i] = cpu_to_le64(desc->args[i +
   198							      SCM_SMC_FIRST_EXT_IDX]);
   199			}
   200	
   201			args_phys = dma_map_single(dev, args_virt, alloc_len,
   202						   DMA_TO_DEVICE);
   203	
   204			if (dma_mapping_error(dev, args_phys)) {
   205				if (using_shm_bridge)
   206					qcom_shm_bridge_free(args_virt);
   207				else
   208					kfree(args_virt);
   209				return -ENOMEM;
   210			}
   211	
   212			smc.args[SCM_SMC_LAST_REG_IDX] = args_phys;
   213		}
   214	
   215		/* ret error check follows after args_virt cleanup*/
   216		ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
   217	
   218		if (args_virt) {
   219			dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
   220			if (using_shm_bridge)
 > 221				qcom_shm_bridge_free(args_virt);
   222			else
   223				kfree(args_virt);
   224		}
   225	
   226		if (ret)
   227			return ret;
   228	
   229		if (res) {
   230			res->result[0] = smc_res.a1;
   231			res->result[1] = smc_res.a2;
   232			res->result[2] = smc_res.a3;
   233		}
   234	
   235		return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
   236	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 07/11] firmware: qcom-scm: use SHM bridge if available

[kernel test robot]

Hi Bartosz,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on arm64/for-next/core linus/master v6.5]
[cannot apply to next-20230828]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/firmware-qcom-scm-drop-unneeded-extern-specifiers/20230829-032930
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230828192507.117334-8-bartosz.golaszewski%40linaro.org
patch subject: [PATCH 07/11] firmware: qcom-scm: use SHM bridge if available
config: arm-defconfig (https://download.01.org/0day-ci/archive/20230829/202308291337.PSyslzRx-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230829/202308291337.PSyslzRx-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308291337.PSyslzRx-lkp@intel.com/

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: drivers/firmware/qcom_scm-smc.o: in function `__scm_smc_call':
>> qcom_scm-smc.c:(.text+0x39c): undefined reference to `qcom_shm_bridge_free'
>> arm-linux-gnueabi-ld: qcom_scm-smc.c:(.text+0x3f0): undefined reference to `qcom_shm_bridge_alloc'
>> arm-linux-gnueabi-ld: qcom_scm-smc.c:(.text+0x53c): undefined reference to `qcom_shm_bridge_free'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 08/11] arm64: defconfig: enable Qualcomm SHM bridge module

[Bartosz Golaszewski]

Most Qualcomm architectures support SHM bridge. Enable it as a built-in
module in arm64 defconfig.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index a25d783dfb95..7f982d9966e3 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -248,6 +248,7 @@ CONFIG_ARM_SCPI_PROTOCOL=y
 CONFIG_RASPBERRYPI_FIRMWARE=y
 CONFIG_INTEL_STRATIX10_SERVICE=y
 CONFIG_INTEL_STRATIX10_RSU=m
+CONFIG_QCOM_SHM_BRIDGE=y
 CONFIG_EFI_CAPSULE_LOADER=y
 CONFIG_IMX_SCU=y
 CONFIG_IMX_SCU_PD=y
-- 
2.39.2

[PATCH 09/11] arm64: dts: qcom: sm8450: enable SHM bridge

[Bartosz Golaszewski]

Enable SHM bridge on sm8450 platforms.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 5cd7296c7660..eacb2658e3ec 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -289,6 +289,10 @@ scm: scm {
 			interconnects = <&aggre2_noc MASTER_CRYPTO 0 &mc_virt SLAVE_EBI1 0>;
 			#reset-cells = <1>;
 		};
+
+		shm-bridge {
+			compatible = "qcom,shm-bridge-sm8450", "qcom,shm-bridge";
+		};
 	};
 
 	clk_virt: interconnect-0 {
-- 
2.39.2

[PATCH 10/11] arm64: dts: qcom: sa8775p: enable SHM bridge

[Bartosz Golaszewski]

Enable SHM bridge on sa8775p platforms.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index b130136acffe..d8614d15750e 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -209,6 +209,10 @@ firmware {
 		scm {
 			compatible = "qcom,scm-sa8775p", "qcom,scm";
 		};
+
+		shm-bridge {
+			compatible = "qcom,shm-bridge-sa8775p", "qcom,shm-bridge";
+		};
 	};
 
 	aggre1_noc: interconnect-aggre1-noc {
-- 
2.39.2

[PATCH 11/11] arm64: dts: qcom: sm8150: enable SHM bridge

[Bartosz Golaszewski]

Enable SHM bridge on sm8150 platforms.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8150.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
index b46e55bb8bde..ffb0b9d82bea 100644
--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
@@ -607,6 +607,10 @@ scm: scm {
 			compatible = "qcom,scm-sm8150", "qcom,scm";
 			#reset-cells = <1>;
 		};
+
+		shm-bridge {
+			compatible = "qcom,shm-bridge-sm8150", "qcom,shm-bridge";
+		};
 	};
 
 	memory@80000000 {
-- 
2.39.2

Re: [PATCH 00/11] arm64: qcom: add and enable SHM Bridge support

[Dmitry Baryshkov]

On Mon, 28 Aug 2023 at 22:29, Bartosz Golaszewski
<bartosz.golaszewski@linaro.org> wrote:
>
> SHM Bridge is a mechanism allowing to map limited areas of kernel's
> virtual memory to physical addresses and share those with the
> trustzone in order to not expose the entire RAM for SMC calls.
>
> This series adds support for Qualcomm SHM Bridge in form of a platform
> driver and library functions available to users. It enables SHM Bridge
> support for three platforms and contains a bunch of cleanups for
> qcom-scm.

Which users do you expect for this API?

Also, could you please describe your design a bit more? Why have you
implemented the shm-bridge as a separate driver rather than a part of
the SCM driver?

>
> Bartosz Golaszewski (11):
>   firmware: qcom-scm: drop unneeded 'extern' specifiers
>   firmware: qcom-scm: order includes alphabetically
>   firmware: qcom-scm: atomically assign and read the global __scm
>     pointer
>   firmware: qcom-scm: add support for SHM bridge operations
>   dt-bindings: document the Qualcomm TEE Shared Memory Bridge
>   firmware: qcom-shm-bridge: new driver
>   firmware: qcom-scm: use SHM bridge if available
>   arm64: defconfig: enable Qualcomm SHM bridge module
>   arm64: dts: qcom: sm8450: enable SHM bridge
>   arm64: dts: qcom: sa8775p: enable SHM bridge
>   arm64: dts: qcom: sm8150: enable SHM bridge
>
>  .../bindings/firmware/qcom,shm-bridge.yaml    |  36 ++
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi         |   4 +
>  arch/arm64/boot/dts/qcom/sm8150.dtsi          |   4 +
>  arch/arm64/boot/dts/qcom/sm8450.dtsi          |   4 +
>  arch/arm64/configs/defconfig                  |   1 +
>  drivers/firmware/Kconfig                      |   8 +
>  drivers/firmware/Makefile                     |   1 +
>  drivers/firmware/qcom-shm-bridge.c            | 452 ++++++++++++++++++
>  drivers/firmware/qcom_scm-smc.c               |  20 +-
>  drivers/firmware/qcom_scm.c                   | 106 +++-
>  drivers/firmware/qcom_scm.h                   |   3 +
>  include/linux/firmware/qcom/qcom_scm.h        | 109 +++--
>  include/linux/firmware/qcom/shm-bridge.h      |  32 ++
>  13 files changed, 712 insertions(+), 68 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
>  create mode 100644 drivers/firmware/qcom-shm-bridge.c
>  create mode 100644 include/linux/firmware/qcom/shm-bridge.h
>
> --
> 2.39.2
>


-- 
With best wishes
Dmitry

Re: [PATCH 00/11] arm64: qcom: add and enable SHM Bridge support

[Bartosz Golaszewski]

On Mon, 28 Aug 2023 at 23:24, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Mon, 28 Aug 2023 at 22:29, Bartosz Golaszewski
> <bartosz.golaszewski@linaro.org> wrote:
> >
> > SHM Bridge is a mechanism allowing to map limited areas of kernel's
> > virtual memory to physical addresses and share those with the
> > trustzone in order to not expose the entire RAM for SMC calls.
> >
> > This series adds support for Qualcomm SHM Bridge in form of a platform
> > driver and library functions available to users. It enables SHM Bridge
> > support for three platforms and contains a bunch of cleanups for
> > qcom-scm.
>
> Which users do you expect for this API?
>

This series adds a single user: the SCM driver. We have another user
almost ready for upstream in the form of the scminvoke driver and I
learned today, I can already convert another user upstream right now
that I will try to get ready for v2.

> Also, could you please describe your design a bit more? Why have you
> implemented the shm-bridge as a separate driver rather than a part of
> the SCM driver?
>

It's self-contained enough to be put into a separate module and not
all platforms support it so in order to avoid unnecessary ifdeffery in
the scm driver, I made it separate.

Bart

> >
> > Bartosz Golaszewski (11):
> >   firmware: qcom-scm: drop unneeded 'extern' specifiers
> >   firmware: qcom-scm: order includes alphabetically
> >   firmware: qcom-scm: atomically assign and read the global __scm
> >     pointer
> >   firmware: qcom-scm: add support for SHM bridge operations
> >   dt-bindings: document the Qualcomm TEE Shared Memory Bridge
> >   firmware: qcom-shm-bridge: new driver
> >   firmware: qcom-scm: use SHM bridge if available
> >   arm64: defconfig: enable Qualcomm SHM bridge module
> >   arm64: dts: qcom: sm8450: enable SHM bridge
> >   arm64: dts: qcom: sa8775p: enable SHM bridge
> >   arm64: dts: qcom: sm8150: enable SHM bridge
> >
> >  .../bindings/firmware/qcom,shm-bridge.yaml    |  36 ++
> >  arch/arm64/boot/dts/qcom/sa8775p.dtsi         |   4 +
> >  arch/arm64/boot/dts/qcom/sm8150.dtsi          |   4 +
> >  arch/arm64/boot/dts/qcom/sm8450.dtsi          |   4 +
> >  arch/arm64/configs/defconfig                  |   1 +
> >  drivers/firmware/Kconfig                      |   8 +
> >  drivers/firmware/Makefile                     |   1 +
> >  drivers/firmware/qcom-shm-bridge.c            | 452 ++++++++++++++++++
> >  drivers/firmware/qcom_scm-smc.c               |  20 +-
> >  drivers/firmware/qcom_scm.c                   | 106 +++-
> >  drivers/firmware/qcom_scm.h                   |   3 +
> >  include/linux/firmware/qcom/qcom_scm.h        | 109 +++--
> >  include/linux/firmware/qcom/shm-bridge.h      |  32 ++
> >  13 files changed, 712 insertions(+), 68 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml
> >  create mode 100644 drivers/firmware/qcom-shm-bridge.c
> >  create mode 100644 include/linux/firmware/qcom/shm-bridge.h
> >
> > --
> > 2.39.2
> >
>
>
> --
> With best wishes
> Dmitry

Re: [PATCH 00/11] arm64: qcom: add and enable SHM Bridge support

[Dmitry Baryshkov]

On Tue, 29 Aug 2023 at 22:03, Bartosz Golaszewski
<bartosz.golaszewski@linaro.org> wrote:
>
> On Mon, 28 Aug 2023 at 23:24, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Mon, 28 Aug 2023 at 22:29, Bartosz Golaszewski
> > <bartosz.golaszewski@linaro.org> wrote:
> > >
> > > SHM Bridge is a mechanism allowing to map limited areas of kernel's
> > > virtual memory to physical addresses and share those with the
> > > trustzone in order to not expose the entire RAM for SMC calls.
> > >
> > > This series adds support for Qualcomm SHM Bridge in form of a platform
> > > driver and library functions available to users. It enables SHM Bridge
> > > support for three platforms and contains a bunch of cleanups for
> > > qcom-scm.
> >
> > Which users do you expect for this API?
> >
>
> This series adds a single user: the SCM driver. We have another user
> almost ready for upstream in the form of the scminvoke driver and I
> learned today, I can already convert another user upstream right now
> that I will try to get ready for v2.
>
> > Also, could you please describe your design a bit more? Why have you
> > implemented the shm-bridge as a separate driver rather than a part of
> > the SCM driver?
> >
>
> It's self-contained enough to be put into a separate module and not
> all platforms support it so in order to avoid unnecessary ifdeffery in
> the scm driver, I made it separate.

Judging from other reviews, I'm not the only one who questioned this
design. I still suppose that it might be better to move it into the
SCM driver. You can put ifdef's to the header file defining the
interface between SCM and SHM bridge part.

-- 
With best wishes
Dmitry

Re: (subset) [PATCH 00/11] arm64: qcom: add and enable SHM Bridge support

[Bjorn Andersson]

On Mon, 28 Aug 2023 21:24:56 +0200, Bartosz Golaszewski wrote:
> SHM Bridge is a mechanism allowing to map limited areas of kernel's
> virtual memory to physical addresses and share those with the
> trustzone in order to not expose the entire RAM for SMC calls.
> 
> This series adds support for Qualcomm SHM Bridge in form of a platform
> driver and library functions available to users. It enables SHM Bridge
> support for three platforms and contains a bunch of cleanups for
> qcom-scm.
> 
> [...]

Applied, thanks!

[01/11] firmware: qcom-scm: drop unneeded 'extern' specifiers
        commit: 2758ac3a11d78af56e6969af04dec611806a62de
[02/11] firmware: qcom-scm: order includes alphabetically
        commit: bc7fbb5ea701b22c09c0fa5acbc122207283366a

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>