| Message ID | 20240205182810.58382-1-brgl@bgdev.pl (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: qcom: add and enable SHM Bridge support | expand |
On Mon, Feb 5, 2024 at 7:28 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > [snip] > > Tested on sm8550 and sa8775p with the Inline Crypto Engine and > remoteproc. > I forgot to mention that I also tested it on db410c where it correctly reports that: [ 0.107312] qcom_scm firmware:scm: SHM Bridge not supported and falls back to using non-SHM bridge allocator. It was also tested on other platforms by other people which is reflected in Tested-by tags in this series. Bart
On Mon, Feb 05, 2024 at 07:27:58PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > We've established the need for using separate secured memory pools for > SCM and QSEECOM Where has this need been established, what is the actual problem you're solving with this series? Does SCM and QSEECOM, as it's implemented in the kernel today, not work satisfactory? > as well as the upcoming scminvoke driver. > Is smcinvoke driver upstreaming blocked by not transitioning the scm driver to a "secure memory pool"? Is this happening now, or do we need to merge this series when that day comes? > It's also become clear that in order to be future-proof, the new > allocator must be an abstraction layer of a higher level as the SHM > Bridge will not be the only memory protection mechanism that we'll see > upstream. Hence the rename to TrustZone Memory rather than SCM Memory > allocator. > > Also to that end: the new allocator is its own module now and provides a > Kconfig choice menu for selecting the mode of operation (currently > default and SHM Bridge). > > Tested on sm8550 and sa8775p with the Inline Crypto Engine and > remoteproc. > It's reasonable to assume from mentioning of this (and Andrew's t-b), that the patchset has only been tested on recent platforms that indeed does implement shmbridge. Can you please share the list of other platforms that you've tested this on, and can you please get someone from IPQ to also give their r-b or t-b? Regards, Bjorn > v6 -> v7: > - fix a Kconfig issue: TZMEM must select GENERIC_ALLOCATOR > > v5 -> v6: > Fixed two issues reported by autobuilders: > - add a fix for memory leaks in the qseecom driver as the first patch for > easier backporting to the v6.6.y branch > - explicitly cast the bus address stored in a variable of type dma_addr_t > to phys_addr_t expected by the genpool API > > v4 -> v5: > - fix the return value from qcom_tzmem_init() if SHM Bridge is not supported > - remove a comment that's no longer useful > - collect tags > > v3 -> v4: > - include linux/sizes.h for SZ_X macros > - use dedicated RCU APIs to dereference radix tree slots > - fix kerneldocs > - fix the comment in patch 14/15: it's the hypervisor, not the TrustZone > that creates the SHM bridge > > v2 -> v3: > - restore pool management and use separate pools for different users > - don't use the new allocator in qcom_scm_pas_init_image() as the > TrustZone will create an SHM bridge for us here > - rewrite the entire series again for most part > > v1 -> v2: > - too many changes to list, it's a complete rewrite as explained above > > Bartosz Golaszewski (12): > firmware: qcom: add a dedicated TrustZone buffer allocator > firmware: qcom: scm: enable the TZ mem allocator > firmware: qcom: scm: smc: switch to using the SCM allocator > firmware: qcom: scm: make qcom_scm_assign_mem() use the TZ allocator > firmware: qcom: scm: make qcom_scm_ice_set_key() use the TZ allocator > firmware: qcom: scm: make qcom_scm_lmh_dcvsh() use the TZ allocator > firmware: qcom: scm: make qcom_scm_qseecom_app_get_id() use the TZ > allocator > firmware: qcom: qseecom: convert to using the TZ allocator > firmware: qcom: scm: add support for SHM bridge operations > firmware: qcom: tzmem: enable SHM Bridge support > firmware: qcom: scm: clarify the comment in qcom_scm_pas_init_image() > arm64: defconfig: enable SHM Bridge support for the TZ memory > allocator > > MAINTAINERS | 8 + > arch/arm64/configs/defconfig | 1 + > drivers/firmware/qcom/Kconfig | 31 ++ > drivers/firmware/qcom/Makefile | 1 + > .../firmware/qcom/qcom_qseecom_uefisecapp.c | 281 +++++--------- > drivers/firmware/qcom/qcom_scm-smc.c | 30 +- > drivers/firmware/qcom/qcom_scm.c | 179 +++++---- > drivers/firmware/qcom/qcom_scm.h | 6 + > drivers/firmware/qcom/qcom_tzmem.c | 365 ++++++++++++++++++ > drivers/firmware/qcom/qcom_tzmem.h | 13 + > include/linux/firmware/qcom/qcom_qseecom.h | 4 +- > include/linux/firmware/qcom/qcom_scm.h | 6 + > include/linux/firmware/qcom/qcom_tzmem.h | 28 ++ > 13 files changed, 685 insertions(+), 268 deletions(-) > create mode 100644 drivers/firmware/qcom/qcom_tzmem.c > create mode 100644 drivers/firmware/qcom/qcom_tzmem.h > create mode 100644 include/linux/firmware/qcom/qcom_tzmem.h > > -- > 2.40.1 >
On 17/02/2024 19:58, Bjorn Andersson wrote: > On Mon, Feb 05, 2024 at 07:27:58PM +0100, Bartosz Golaszewski wrote: >> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >> >> We've established the need for using separate secured memory pools for >> SCM and QSEECOM > > Where has this need been established, what is the actual problem you're > solving with this series? SHMbridge will restrict the amount of memory that TZ can see, making system more secure. Need for having different pools makes this more scalable overall, so that different usecases can run seamlessly. ex: loading a TA and SCM calls. > > Does SCM and QSEECOM, as it's implemented in the kernel today, not work > satisfactory? > >> as well as the upcoming scminvoke driver. >> > > Is smcinvoke driver upstreaming blocked by not transitioning the scm > driver to a "secure memory pool"? > > Is this happening now, or do we need to merge this series when that day > comes? SMCInvoke development is happening now, I see no reason for this patchset to wait for it. This series can go as it is for two reasons. 1> improves system security in general 2> Hardware Wrapped key support patches also use this which are also in good shape and tested, ready to be merged. > >> It's also become clear that in order to be future-proof, the new >> allocator must be an abstraction layer of a higher level as the SHM >> Bridge will not be the only memory protection mechanism that we'll see >> upstream. Hence the rename to TrustZone Memory rather than SCM Memory >> allocator. >> >> Also to that end: the new allocator is its own module now and provides a >> Kconfig choice menu for selecting the mode of operation (currently >> default and SHM Bridge). >> >> Tested on sm8550 and sa8775p with the Inline Crypto Engine and >> remoteproc. >> > > It's reasonable to assume from mentioning of this (and Andrew's t-b), > that the patchset has only been tested on recent platforms that indeed > does implement shmbridge. Can you please share the list of other > platforms that you've tested this on, and can you please get someone > from IPQ to also give their r-b or t-b? We will try to reachout to someone who has access to these. thanks, Srini > > Regards, > Bjorn > >> v6 -> v7: >> - fix a Kconfig issue: TZMEM must select GENERIC_ALLOCATOR >> >> v5 -> v6: >> Fixed two issues reported by autobuilders: >> - add a fix for memory leaks in the qseecom driver as the first patch for >> easier backporting to the v6.6.y branch >> - explicitly cast the bus address stored in a variable of type dma_addr_t >> to phys_addr_t expected by the genpool API >> >> v4 -> v5: >> - fix the return value from qcom_tzmem_init() if SHM Bridge is not supported >> - remove a comment that's no longer useful >> - collect tags >> >> v3 -> v4: >> - include linux/sizes.h for SZ_X macros >> - use dedicated RCU APIs to dereference radix tree slots >> - fix kerneldocs >> - fix the comment in patch 14/15: it's the hypervisor, not the TrustZone >> that creates the SHM bridge >> >> v2 -> v3: >> - restore pool management and use separate pools for different users >> - don't use the new allocator in qcom_scm_pas_init_image() as the >> TrustZone will create an SHM bridge for us here >> - rewrite the entire series again for most part >> >> v1 -> v2: >> - too many changes to list, it's a complete rewrite as explained above >> >> Bartosz Golaszewski (12): >> firmware: qcom: add a dedicated TrustZone buffer allocator >> firmware: qcom: scm: enable the TZ mem allocator >> firmware: qcom: scm: smc: switch to using the SCM allocator >> firmware: qcom: scm: make qcom_scm_assign_mem() use the TZ allocator >> firmware: qcom: scm: make qcom_scm_ice_set_key() use the TZ allocator >> firmware: qcom: scm: make qcom_scm_lmh_dcvsh() use the TZ allocator >> firmware: qcom: scm: make qcom_scm_qseecom_app_get_id() use the TZ >> allocator >> firmware: qcom: qseecom: convert to using the TZ allocator >> firmware: qcom: scm: add support for SHM bridge operations >> firmware: qcom: tzmem: enable SHM Bridge support >> firmware: qcom: scm: clarify the comment in qcom_scm_pas_init_image() >> arm64: defconfig: enable SHM Bridge support for the TZ memory >> allocator >> >> MAINTAINERS | 8 + >> arch/arm64/configs/defconfig | 1 + >> drivers/firmware/qcom/Kconfig | 31 ++ >> drivers/firmware/qcom/Makefile | 1 + >> .../firmware/qcom/qcom_qseecom_uefisecapp.c | 281 +++++--------- >> drivers/firmware/qcom/qcom_scm-smc.c | 30 +- >> drivers/firmware/qcom/qcom_scm.c | 179 +++++---- >> drivers/firmware/qcom/qcom_scm.h | 6 + >> drivers/firmware/qcom/qcom_tzmem.c | 365 ++++++++++++++++++ >> drivers/firmware/qcom/qcom_tzmem.h | 13 + >> include/linux/firmware/qcom/qcom_qseecom.h | 4 +- >> include/linux/firmware/qcom/qcom_scm.h | 6 + >> include/linux/firmware/qcom/qcom_tzmem.h | 28 ++ >> 13 files changed, 685 insertions(+), 268 deletions(-) >> create mode 100644 drivers/firmware/qcom/qcom_tzmem.c >> create mode 100644 drivers/firmware/qcom/qcom_tzmem.h >> create mode 100644 include/linux/firmware/qcom/qcom_tzmem.h >> >> -- >> 2.40.1 >>
On Mon, Feb 19, 2024 at 03:17:24PM +0000, Srinivas Kandagatla wrote: > On 17/02/2024 19:58, Bjorn Andersson wrote: > > On Mon, Feb 05, 2024 at 07:27:58PM +0100, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > We've established the need for using separate secured memory pools for > > > SCM and QSEECOM > > > > Where has this need been established, what is the actual problem you're > > solving with this series? > > SHMbridge will restrict the amount of memory that TZ can see, making system > more secure. > Then tell me this in the cover letter and commit messages. > Need for having different pools makes this more scalable overall, so that > different usecases can run seamlessly. ex: loading a TA and SCM calls. > How is it more scalable to give each "client" a chunk of 256KB instead of them sharing a pool of ~4GB memory? > > > > Does SCM and QSEECOM, as it's implemented in the kernel today, not work > > satisfactory? > > > > > as well as the upcoming scminvoke driver. > > > > > > > Is smcinvoke driver upstreaming blocked by not transitioning the scm > > driver to a "secure memory pool"? > > > > Is this happening now, or do we need to merge this series when that day > > comes? > > SMCInvoke development is happening now, I see no reason for this patchset to > wait for it. > As presented, I see no reason to merge this series. > This series can go as it is for two reasons. > 1> improves system security in general > 2> Hardware Wrapped key support patches also use this which are also in good > shape and tested, ready to be merged. > Then tell me this in the cover letter and commit messages! It's not sufficient that I happen to know the answer to these questions, neither community nor maintainer should not have to guess these things. Regards, Bjorn
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> We've established the need for using separate secured memory pools for SCM and QSEECOM as well as the upcoming scminvoke driver. It's also become clear that in order to be future-proof, the new allocator must be an abstraction layer of a higher level as the SHM Bridge will not be the only memory protection mechanism that we'll see upstream. Hence the rename to TrustZone Memory rather than SCM Memory allocator. Also to that end: the new allocator is its own module now and provides a Kconfig choice menu for selecting the mode of operation (currently default and SHM Bridge). Tested on sm8550 and sa8775p with the Inline Crypto Engine and remoteproc. v6 -> v7: - fix a Kconfig issue: TZMEM must select GENERIC_ALLOCATOR v5 -> v6: Fixed two issues reported by autobuilders: - add a fix for memory leaks in the qseecom driver as the first patch for easier backporting to the v6.6.y branch - explicitly cast the bus address stored in a variable of type dma_addr_t to phys_addr_t expected by the genpool API v4 -> v5: - fix the return value from qcom_tzmem_init() if SHM Bridge is not supported - remove a comment that's no longer useful - collect tags v3 -> v4: - include linux/sizes.h for SZ_X macros - use dedicated RCU APIs to dereference radix tree slots - fix kerneldocs - fix the comment in patch 14/15: it's the hypervisor, not the TrustZone that creates the SHM bridge v2 -> v3: - restore pool management and use separate pools for different users - don't use the new allocator in qcom_scm_pas_init_image() as the TrustZone will create an SHM bridge for us here - rewrite the entire series again for most part v1 -> v2: - too many changes to list, it's a complete rewrite as explained above Bartosz Golaszewski (12): firmware: qcom: add a dedicated TrustZone buffer allocator firmware: qcom: scm: enable the TZ mem allocator firmware: qcom: scm: smc: switch to using the SCM allocator firmware: qcom: scm: make qcom_scm_assign_mem() use the TZ allocator firmware: qcom: scm: make qcom_scm_ice_set_key() use the TZ allocator firmware: qcom: scm: make qcom_scm_lmh_dcvsh() use the TZ allocator firmware: qcom: scm: make qcom_scm_qseecom_app_get_id() use the TZ allocator firmware: qcom: qseecom: convert to using the TZ allocator firmware: qcom: scm: add support for SHM bridge operations firmware: qcom: tzmem: enable SHM Bridge support firmware: qcom: scm: clarify the comment in qcom_scm_pas_init_image() arm64: defconfig: enable SHM Bridge support for the TZ memory allocator MAINTAINERS | 8 + arch/arm64/configs/defconfig | 1 + drivers/firmware/qcom/Kconfig | 31 ++ drivers/firmware/qcom/Makefile | 1 + .../firmware/qcom/qcom_qseecom_uefisecapp.c | 281 +++++--------- drivers/firmware/qcom/qcom_scm-smc.c | 30 +- drivers/firmware/qcom/qcom_scm.c | 179 +++++---- drivers/firmware/qcom/qcom_scm.h | 6 + drivers/firmware/qcom/qcom_tzmem.c | 365 ++++++++++++++++++ drivers/firmware/qcom/qcom_tzmem.h | 13 + include/linux/firmware/qcom/qcom_qseecom.h | 4 +- include/linux/firmware/qcom/qcom_scm.h | 6 + include/linux/firmware/qcom/qcom_tzmem.h | 28 ++ 13 files changed, 685 insertions(+), 268 deletions(-) create mode 100644 drivers/firmware/qcom/qcom_tzmem.c create mode 100644 drivers/firmware/qcom/qcom_tzmem.h create mode 100644 include/linux/firmware/qcom/qcom_tzmem.h