| Message ID | 20240205182810.58382-3-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: qcom: add and enable SHM Bridge support | expand |
On Mon, Feb 05, 2024 at 07:28:00PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Select the TrustZone memory allocator in Kconfig and create a pool of > memory shareable with the TrustZone when probing the SCM driver. > > This will allow a gradual conversion of all relevant SCM calls to using > the dedicated allocator. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > Tested-by: Andrew Halaney <ahalaney@redhat.com> # sc8280xp-lenovo-thinkpad-x13s > Tested-by: Deepti Jaggi <quic_djaggi@quicinc.com> #sa8775p-ride > Reviewed-by: Elliot Berman <quic_eberman@quicinc.com> > --- > drivers/firmware/qcom/Kconfig | 1 + > drivers/firmware/qcom/qcom_scm.c | 16 ++++++++++++++++ > 2 files changed, 17 insertions(+) > > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig > index f18686edf415..d24d83223867 100644 > --- a/drivers/firmware/qcom/Kconfig > +++ b/drivers/firmware/qcom/Kconfig > @@ -7,6 +7,7 @@ > menu "Qualcomm firmware drivers" > > config QCOM_SCM > + select QCOM_TZMEM > tristate > > config QCOM_TZMEM > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > index 520de9b5633a..0d4c028be0c1 100644 > --- a/drivers/firmware/qcom/qcom_scm.c > +++ b/drivers/firmware/qcom/qcom_scm.c > @@ -8,8 +8,10 @@ > #include <linux/completion.h> > #include <linux/cpumask.h> > #include <linux/dma-mapping.h> > +#include <linux/err.h> > #include <linux/export.h> > #include <linux/firmware/qcom/qcom_scm.h> > +#include <linux/firmware/qcom/qcom_tzmem.h> > #include <linux/init.h> > #include <linux/interconnect.h> > #include <linux/interrupt.h> > @@ -20,9 +22,11 @@ > #include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/reset-controller.h> > +#include <linux/sizes.h> > #include <linux/types.h> > > #include "qcom_scm.h" > +#include "qcom_tzmem.h" > > static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT); > module_param(download_mode, bool, 0); > @@ -41,6 +45,8 @@ struct qcom_scm { > int scm_vote_count; > > u64 dload_mode_addr; > + > + struct qcom_tzmem_pool *mempool; > }; > > struct qcom_scm_current_perm_info { > @@ -1887,6 +1893,16 @@ static int qcom_scm_probe(struct platform_device *pdev) > if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled")) > qcom_scm_disable_sdi(); > > + ret = qcom_tzmem_enable(__scm->dev); > + if (ret) > + return dev_err_probe(__scm->dev, ret, > + "Failed to enable the TrustZone memory allocator\n"); > + > + __scm->mempool = devm_qcom_tzmem_pool_new(__scm->dev, SZ_256K); As we're not moving from the callers freely allocating what they need, to a fixed sized pool of 256kb. Please document why 256kb was choosen, so that we have something to fall back on when someone runs out of this space, or wonders "why not 128kb?". Regards, Bjorn > + if (IS_ERR(__scm->mempool)) > + return dev_err_probe(__scm->dev, PTR_ERR(__scm->mempool), > + "Failed to create the SCM memory pool\n"); > + > /* > * Initialize the QSEECOM interface. > * > -- > 2.40.1 >
On Sun, Feb 18, 2024 at 4:25 AM Bjorn Andersson <andersson@kernel.org> wrote: > > On Mon, Feb 05, 2024 at 07:28:00PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Select the TrustZone memory allocator in Kconfig and create a pool of > > memory shareable with the TrustZone when probing the SCM driver. > > > > This will allow a gradual conversion of all relevant SCM calls to using > > the dedicated allocator. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > > Tested-by: Andrew Halaney <ahalaney@redhat.com> # sc8280xp-lenovo-thinkpad-x13s > > Tested-by: Deepti Jaggi <quic_djaggi@quicinc.com> #sa8775p-ride > > Reviewed-by: Elliot Berman <quic_eberman@quicinc.com> > > --- > > drivers/firmware/qcom/Kconfig | 1 + > > drivers/firmware/qcom/qcom_scm.c | 16 ++++++++++++++++ > > 2 files changed, 17 insertions(+) > > > > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig > > index f18686edf415..d24d83223867 100644 > > --- a/drivers/firmware/qcom/Kconfig > > +++ b/drivers/firmware/qcom/Kconfig > > @@ -7,6 +7,7 @@ > > menu "Qualcomm firmware drivers" > > > > config QCOM_SCM > > + select QCOM_TZMEM > > tristate > > > > config QCOM_TZMEM > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > > index 520de9b5633a..0d4c028be0c1 100644 > > --- a/drivers/firmware/qcom/qcom_scm.c > > +++ b/drivers/firmware/qcom/qcom_scm.c > > @@ -8,8 +8,10 @@ > > #include <linux/completion.h> > > #include <linux/cpumask.h> > > #include <linux/dma-mapping.h> > > +#include <linux/err.h> > > #include <linux/export.h> > > #include <linux/firmware/qcom/qcom_scm.h> > > +#include <linux/firmware/qcom/qcom_tzmem.h> > > #include <linux/init.h> > > #include <linux/interconnect.h> > > #include <linux/interrupt.h> > > @@ -20,9 +22,11 @@ > > #include <linux/of_platform.h> > > #include <linux/platform_device.h> > > #include <linux/reset-controller.h> > > +#include <linux/sizes.h> > > #include <linux/types.h> > > > > #include "qcom_scm.h" > > +#include "qcom_tzmem.h" > > > > static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT); > > module_param(download_mode, bool, 0); > > @@ -41,6 +45,8 @@ struct qcom_scm { > > int scm_vote_count; > > > > u64 dload_mode_addr; > > + > > + struct qcom_tzmem_pool *mempool; > > }; > > > > struct qcom_scm_current_perm_info { > > @@ -1887,6 +1893,16 @@ static int qcom_scm_probe(struct platform_device *pdev) > > if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled")) > > qcom_scm_disable_sdi(); > > > > + ret = qcom_tzmem_enable(__scm->dev); > > + if (ret) > > + return dev_err_probe(__scm->dev, ret, > > + "Failed to enable the TrustZone memory allocator\n"); > > + > > + __scm->mempool = devm_qcom_tzmem_pool_new(__scm->dev, SZ_256K); > > As we're not moving from the callers freely allocating what they need, > to a fixed sized pool of 256kb. Please document why 256kb was choosen, > so that we have something to fall back on when someone runs out of this > space, or wonders "why not 128kb?". > If you worry about these pools being taken out of the total memory and prefer to have a way to avoid it, I was thinking about another build-time mode for the allocator - one where there's no pool but it just allocates chunks using dma_alloc_coherent() like before and pool size is ignored. Does it sound good? Bart > Regards, > Bjorn > > > + if (IS_ERR(__scm->mempool)) > > + return dev_err_probe(__scm->dev, PTR_ERR(__scm->mempool), > > + "Failed to create the SCM memory pool\n"); > > + > > /* > > * Initialize the QSEECOM interface. > > * > > -- > > 2.40.1 > >
On Wed, Feb 21, 2024 at 7:39 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Sun, Feb 18, 2024 at 4:25 AM Bjorn Andersson <andersson@kernel.org> wrote: > > > > On Mon, Feb 05, 2024 at 07:28:00PM +0100, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Select the TrustZone memory allocator in Kconfig and create a pool of > > > memory shareable with the TrustZone when probing the SCM driver. > > > > > > This will allow a gradual conversion of all relevant SCM calls to using > > > the dedicated allocator. > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > > > Tested-by: Andrew Halaney <ahalaney@redhat.com> # sc8280xp-lenovo-thinkpad-x13s > > > Tested-by: Deepti Jaggi <quic_djaggi@quicinc.com> #sa8775p-ride > > > Reviewed-by: Elliot Berman <quic_eberman@quicinc.com> > > > --- > > > drivers/firmware/qcom/Kconfig | 1 + > > > drivers/firmware/qcom/qcom_scm.c | 16 ++++++++++++++++ > > > 2 files changed, 17 insertions(+) > > > > > > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig > > > index f18686edf415..d24d83223867 100644 > > > --- a/drivers/firmware/qcom/Kconfig > > > +++ b/drivers/firmware/qcom/Kconfig > > > @@ -7,6 +7,7 @@ > > > menu "Qualcomm firmware drivers" > > > > > > config QCOM_SCM > > > + select QCOM_TZMEM > > > tristate > > > > > > config QCOM_TZMEM > > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > > > index 520de9b5633a..0d4c028be0c1 100644 > > > --- a/drivers/firmware/qcom/qcom_scm.c > > > +++ b/drivers/firmware/qcom/qcom_scm.c > > > @@ -8,8 +8,10 @@ > > > #include <linux/completion.h> > > > #include <linux/cpumask.h> > > > #include <linux/dma-mapping.h> > > > +#include <linux/err.h> > > > #include <linux/export.h> > > > #include <linux/firmware/qcom/qcom_scm.h> > > > +#include <linux/firmware/qcom/qcom_tzmem.h> > > > #include <linux/init.h> > > > #include <linux/interconnect.h> > > > #include <linux/interrupt.h> > > > @@ -20,9 +22,11 @@ > > > #include <linux/of_platform.h> > > > #include <linux/platform_device.h> > > > #include <linux/reset-controller.h> > > > +#include <linux/sizes.h> > > > #include <linux/types.h> > > > > > > #include "qcom_scm.h" > > > +#include "qcom_tzmem.h" > > > > > > static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT); > > > module_param(download_mode, bool, 0); > > > @@ -41,6 +45,8 @@ struct qcom_scm { > > > int scm_vote_count; > > > > > > u64 dload_mode_addr; > > > + > > > + struct qcom_tzmem_pool *mempool; > > > }; > > > > > > struct qcom_scm_current_perm_info { > > > @@ -1887,6 +1893,16 @@ static int qcom_scm_probe(struct platform_device *pdev) > > > if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled")) > > > qcom_scm_disable_sdi(); > > > > > > + ret = qcom_tzmem_enable(__scm->dev); > > > + if (ret) > > > + return dev_err_probe(__scm->dev, ret, > > > + "Failed to enable the TrustZone memory allocator\n"); > > > + > > > + __scm->mempool = devm_qcom_tzmem_pool_new(__scm->dev, SZ_256K); > > > > As we're not moving from the callers freely allocating what they need, > > to a fixed sized pool of 256kb. Please document why 256kb was choosen, > > so that we have something to fall back on when someone runs out of this > > space, or wonders "why not 128kb?". > > > > If you worry about these pools being taken out of the total memory and > prefer to have a way to avoid it, I was thinking about another > build-time mode for the allocator - one where there's no pool but it > just allocates chunks using dma_alloc_coherent() like before and pool > size is ignored. Does it sound good? > Or we could even have an argument for the initial size of the pool and then once it's exhausted, we'd add a new chunk (maybe twice the size?) and so on. Bart > Bart > > > Regards, > > Bjorn > > > > > + if (IS_ERR(__scm->mempool)) > > > + return dev_err_probe(__scm->dev, PTR_ERR(__scm->mempool), > > > + "Failed to create the SCM memory pool\n"); > > > + > > > /* > > > * Initialize the QSEECOM interface. > > > * > > > -- > > > 2.40.1 > > >
> > > As we're not moving from the callers freely allocating what they need, > > > to a fixed sized pool of 256kb. Please document why 256kb was choosen, > > > so that we have something to fall back on when someone runs out of this > > > space, or wonders "why not 128kb?". > > > > > > > If you worry about these pools being taken out of the total memory and > > prefer to have a way to avoid it, I was thinking about another > > build-time mode for the allocator - one where there's no pool but it > > just allocates chunks using dma_alloc_coherent() like before and pool > > size is ignored. Does it sound good? > > > > Or we could even have an argument for the initial size of the pool and > then once it's exhausted, we'd add a new chunk (maybe twice the size?) > and so on. Hi Bartosz, Thanks for shmbridge patch series. Few questions. 1. With current design of every client maintaining it's own pool, For any target, we might end up occupying lot more space by different clients than we actually need. 2. Also, there's no option to configure pool size for each client at runtime level and a fixed 256K value is chosen for qcom_scm/qseecom. Pool size will be same for each target and thus making it less scalabale if there's adjustment needed at target specific level. Ex: For a low DDR memory target, pool size should scale down accordingly as 256K will become a big ask but there's no way to choose specific pool size for just one target. 2.1 One way to do configure custom pool size value is to add new property in qcom_scm/qseecom or client DT node and then create pool of size with this provided value. Though there are ways to tackle this, but still clients specifying it's own pool size will always fetch more CMA region than what is actually needed. Can you please share your ideas as well for upcoming version. Regards Kuldeep
On Mon, Feb 26, 2024 at 11:30 AM Kuldeep Singh <quic_kuldsing@quicinc.com> wrote: > > > > > As we're not moving from the callers freely allocating what they need, > > > > to a fixed sized pool of 256kb. Please document why 256kb was choosen, > > > > so that we have something to fall back on when someone runs out of this > > > > space, or wonders "why not 128kb?". > > > > > > > > > > If you worry about these pools being taken out of the total memory and > > > prefer to have a way to avoid it, I was thinking about another > > > build-time mode for the allocator - one where there's no pool but it > > > just allocates chunks using dma_alloc_coherent() like before and pool > > > size is ignored. Does it sound good? > > > > > > > Or we could even have an argument for the initial size of the pool and > > then once it's exhausted, we'd add a new chunk (maybe twice the size?) > > and so on. > > Hi Bartosz, > > Thanks for shmbridge patch series. Few questions. > > 1. With current design of every client maintaining it's own pool, > For any target, we might end up occupying lot more space by > different clients than we actually need. > Technically there are only up to two, three in the future with scminvoke. > 2. Also, there's no option to configure pool size for each client at > runtime level and a fixed 256K value is chosen for qcom_scm/qseecom. You mean via a module parameter? > Pool size will be same for each target and thus making it less > scalabale if there's adjustment needed at target specific level. > Ex: For a low DDR memory target, pool size should scale down accordingly > as 256K will become a big ask but there's no way to choose specific pool > size for just one target. Do we really have any low-DDR platforms that would be affected by this change? Even for db401c the 256K is a tiny fraction of the total memory. > 2.1 One way to do configure custom pool size value is to add new > property in qcom_scm/qseecom or client DT node and then create > pool of size with this provided value. Though there are ways to > tackle this, but still clients specifying it's own pool size > will always fetch more CMA region than what is actually needed. > > Can you please share your ideas as well for upcoming version. > I will propose a new solution with several configuration options for pools. Including scaling the pool size as needed. I hope to send it this week. Bart > Regards > Kuldeep
diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig index f18686edf415..d24d83223867 100644 --- a/drivers/firmware/qcom/Kconfig +++ b/drivers/firmware/qcom/Kconfig @@ -7,6 +7,7 @@ menu "Qualcomm firmware drivers" config QCOM_SCM + select QCOM_TZMEM tristate config QCOM_TZMEM diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c index 520de9b5633a..0d4c028be0c1 100644 --- a/drivers/firmware/qcom/qcom_scm.c +++ b/drivers/firmware/qcom/qcom_scm.c @@ -8,8 +8,10 @@ #include <linux/completion.h> #include <linux/cpumask.h> #include <linux/dma-mapping.h> +#include <linux/err.h> #include <linux/export.h> #include <linux/firmware/qcom/qcom_scm.h> +#include <linux/firmware/qcom/qcom_tzmem.h> #include <linux/init.h> #include <linux/interconnect.h> #include <linux/interrupt.h> @@ -20,9 +22,11 @@ #include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/reset-controller.h> +#include <linux/sizes.h> #include <linux/types.h> #include "qcom_scm.h" +#include "qcom_tzmem.h" static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT); module_param(download_mode, bool, 0); @@ -41,6 +45,8 @@ struct qcom_scm { int scm_vote_count; u64 dload_mode_addr; + + struct qcom_tzmem_pool *mempool; }; struct qcom_scm_current_perm_info { @@ -1887,6 +1893,16 @@ static int qcom_scm_probe(struct platform_device *pdev) if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled")) qcom_scm_disable_sdi(); + ret = qcom_tzmem_enable(__scm->dev); + if (ret) + return dev_err_probe(__scm->dev, ret, + "Failed to enable the TrustZone memory allocator\n"); + + __scm->mempool = devm_qcom_tzmem_pool_new(__scm->dev, SZ_256K); + if (IS_ERR(__scm->mempool)) + return dev_err_probe(__scm->dev, PTR_ERR(__scm->mempool), + "Failed to create the SCM memory pool\n"); + /* * Initialize the QSEECOM interface. *