On Mon, Feb 05, 2024 at 07:28:00PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Andrew Halaney <[email protected]>
> Tested-by: Andrew Halaney <[email protected]> # sc8280xp-lenovo-thinkpad-x13s
> Tested-by: Deepti Jaggi <[email protected]> #sa8775p-ride
> Reviewed-by: Elliot Berman <[email protected]>
> ---
> 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 <[email protected]> wrote:
>
> On Mon, Feb 05, 2024 at 07:28:00PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > 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 <[email protected]>
> > Reviewed-by: Andrew Halaney <[email protected]>
> > Tested-by: Andrew Halaney <[email protected]> # sc8280xp-lenovo-thinkpad-x13s
> > Tested-by: Deepti Jaggi <[email protected]> #sa8775p-ride
> > Reviewed-by: Elliot Berman <[email protected]>
> > ---
> > 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
> >