by Andrew Halaney

[permalink] [raw]

Subject: Re: [PATCH v3 13/15] firmware: qcom: tzmem: enable SHM Bridge support

On Fri, Oct 13, 2023 at 10:32:00AM +0200, Bartosz Golaszewski wrote:
> On Thu, Oct 12, 2023 at 12:17 AM Andrew Halaney <[email protected]> wrote:
> >
> > On Wed, Oct 11, 2023 at 04:14:32PM -0500, Andrew Halaney wrote:
> > > On Mon, Oct 09, 2023 at 05:34:25PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <[email protected]>
> > > >
> > > > Add a new Kconfig option for selecting the SHM Bridge mode of operation
> > > > for the TrustZone memory allocator.
> > > >
> > > > If enabled at build-time, it will still be checked for availability at
> > > > run-time. If the architecture doesn't support SHM Bridge, the allocator
> > > > will work just like in the default mode.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > > > ---
> > > > drivers/firmware/qcom/Kconfig | 10 +++++
> > > > drivers/firmware/qcom/qcom_tzmem.c | 67 +++++++++++++++++++++++++++++-
> > > > 2 files changed, 76 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig
> > > > index 237da40de832..e01407e31ae4 100644
> > > > --- a/drivers/firmware/qcom/Kconfig
> > > > +++ b/drivers/firmware/qcom/Kconfig
> > > > @@ -27,6 +27,16 @@ config QCOM_TZMEM_MODE_DEFAULT
> > > > Use the default allocator mode. The memory is page-aligned, non-cachable
> > > > and contiguous.
> > > >
> > > > +config QCOM_TZMEM_MODE_SHMBRIDGE
> > > > + bool "SHM Bridge"
> > > > + help
> > > > + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as
> > > > + in the 'Default' allocator but is also explicitly marked as an SHM Bridge
> > > > + buffer.
> > > > +
> > > > + With this selected, all buffers passed to the TrustZone must be allocated
> > > > + using the TZMem allocator or else the TrustZone will refuse to use them.
> > > > +
> > > > endchoice
> > > >
> > > > config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
> > > > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
> > > > index eee51fed756e..b3137844fe43 100644
> > > > --- a/drivers/firmware/qcom/qcom_tzmem.c
> > > > +++ b/drivers/firmware/qcom/qcom_tzmem.c
> > > > @@ -55,7 +55,72 @@ static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool)
> > > >
> > > > }
> > > >
> > > > -#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */
> > > > +#elif IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE)
> > > > +
> > > > +#include <linux/firmware/qcom/qcom_scm.h>
> > > > +
> > > > +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> > > > +
> > > > +static bool qcom_tzmem_using_shm_bridge;
> > > > +
> > > > +static int qcom_tzmem_init(void)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + ret = qcom_scm_shm_bridge_enable();
> > > > + if (ret == -EOPNOTSUPP) {
> > > > + dev_info(qcom_tzmem_dev, "SHM Bridge not supported\n");
> > > > + ret = 0;
> > > > + }
> > > > +
> > > > + if (!ret)
> > > > + qcom_tzmem_using_shm_bridge = true;
> > >
> > > Does the qcom_scm_shm_bridge_enable() returning -EOPNOTSUPP case make
> > > sense? Setting ret to 0 and then claiming we're using shm_bridge seems
> > > wrong to me.
> > >
>
> You answered yourself in the previous email. The size cannot be less
> than 4096 bytes. There's no need to check it anymore than that IMO.
>

Ok, I still think that would be nice but your call.

But this comment was about the above ret = 0 assignment. I am thinking
that's incorrect, because you fail to enable SHM bridge with
-EOPNOTSUPP, then you go ahead and tell the code to claim it is
supported with the if (!ret) conditional. This results in SHM bridge
trying to be used when its really not supported right?

Looks to me that you're really trying to return 0, not ret = 0.

> Bart
>
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool)
> > > > +{
> > > > + u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms, *handle;
> > > > + int ret;
> > > > +
> > > > + if (!qcom_tzmem_using_shm_bridge)
> > > > + return 0;
> > > > +
> > > > + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> > > > + pfn_and_ns_perm = (u64)pool->pbase | ns_perms;
> > > > + ipfn_and_s_perm = (u64)pool->pbase | ns_perms;
> > > > + size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> > >
> > > Is there any sanity checking that can be done here? I assume bits 0-11 are all
> > > flag fields (or at least unrelated to size which I assume at a minimum
> > > must be 4k aka bit 12).
> >
> > I guess qcom_tzmem_pool_new's PAGE_ALIGN would make sure this is
> > probably ok for all future users, but I do think some sanity would be
> > nice to indicate the size's allowed for SHM bridge.
> >
> > >
> > > > +
> > > > + handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> > >
> > > Consider __free(kfree) + return_ptr() usage?
> > >
> > > > + if (!handle)
> > > > + return -ENOMEM;
> > > > +
> > > > + ret = qcom_scm_shm_bridge_create(qcom_tzmem_dev, pfn_and_ns_perm,
> > > > + ipfn_and_s_perm, size_and_flags,
> > > > + QCOM_SCM_VMID_HLOS, handle);
> > > > + if (ret) {
> > > > + kfree(handle);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + pool->priv = handle;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool)
> > > > +{
> > > > + u64 *handle = pool->priv;
> > > > +
> > > > + if (!qcom_tzmem_using_shm_bridge)
> > > > + return;
> > > > +
> > > > + qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle);
> > > > + kfree(handle);
> > > > +}
> > > > +
> > > > +#endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */
> > > >
> > > > /**
> > > > * qcom_tzmem_pool_new() - Create a new TZ memory pool.
> > > > --
> > > > 2.39.2
> > > >
> >
>

2023-10-13 14:07:19

by Bartosz Golaszewski

[permalink] [raw]

Subject: Re: [PATCH v3 13/15] firmware: qcom: tzmem: enable SHM Bridge support

On Fri, Oct 13, 2023 at 3:31 PM Andrew Halaney <[email protected]> wrote:
>
> On Fri, Oct 13, 2023 at 10:32:00AM +0200, Bartosz Golaszewski wrote:
> > On Thu, Oct 12, 2023 at 12:17 AM Andrew Halaney <[email protected]> wrote:
> > >
> > > On Wed, Oct 11, 2023 at 04:14:32PM -0500, Andrew Halaney wrote:
> > > > On Mon, Oct 09, 2023 at 05:34:25PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <[email protected]>
> > > > >
> > > > > Add a new Kconfig option for selecting the SHM Bridge mode of operation
> > > > > for the TrustZone memory allocator.
> > > > >
> > > > > If enabled at build-time, it will still be checked for availability at
> > > > > run-time. If the architecture doesn't support SHM Bridge, the allocator
> > > > > will work just like in the default mode.
> > > > >
> > > > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > > > > ---
> > > > > drivers/firmware/qcom/Kconfig | 10 +++++
> > > > > drivers/firmware/qcom/qcom_tzmem.c | 67 +++++++++++++++++++++++++++++-
> > > > > 2 files changed, 76 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig
> > > > > index 237da40de832..e01407e31ae4 100644
> > > > > --- a/drivers/firmware/qcom/Kconfig
> > > > > +++ b/drivers/firmware/qcom/Kconfig
> > > > > @@ -27,6 +27,16 @@ config QCOM_TZMEM_MODE_DEFAULT
> > > > > Use the default allocator mode. The memory is page-aligned, non-cachable
> > > > > and contiguous.
> > > > >
> > > > > +config QCOM_TZMEM_MODE_SHMBRIDGE
> > > > > + bool "SHM Bridge"
> > > > > + help
> > > > > + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as
> > > > > + in the 'Default' allocator but is also explicitly marked as an SHM Bridge
> > > > > + buffer.
> > > > > +
> > > > > + With this selected, all buffers passed to the TrustZone must be allocated
> > > > > + using the TZMem allocator or else the TrustZone will refuse to use them.
> > > > > +
> > > > > endchoice
> > > > >
> > > > > config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
> > > > > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
> > > > > index eee51fed756e..b3137844fe43 100644
> > > > > --- a/drivers/firmware/qcom/qcom_tzmem.c
> > > > > +++ b/drivers/firmware/qcom/qcom_tzmem.c
> > > > > @@ -55,7 +55,72 @@ static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool)
> > > > >
> > > > > }
> > > > >
> > > > > -#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */
> > > > > +#elif IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE)
> > > > > +
> > > > > +#include <linux/firmware/qcom/qcom_scm.h>
> > > > > +
> > > > > +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> > > > > +
> > > > > +static bool qcom_tzmem_using_shm_bridge;
> > > > > +
> > > > > +static int qcom_tzmem_init(void)
> > > > > +{
> > > > > + int ret;
> > > > > +
> > > > > + ret = qcom_scm_shm_bridge_enable();
> > > > > + if (ret == -EOPNOTSUPP) {
> > > > > + dev_info(qcom_tzmem_dev, "SHM Bridge not supported\n");
> > > > > + ret = 0;
> > > > > + }
> > > > > +
> > > > > + if (!ret)
> > > > > + qcom_tzmem_using_shm_bridge = true;
> > > >
> > > > Does the qcom_scm_shm_bridge_enable() returning -EOPNOTSUPP case make
> > > > sense? Setting ret to 0 and then claiming we're using shm_bridge seems
> > > > wrong to me.
> > > >
> >
> > You answered yourself in the previous email. The size cannot be less
> > than 4096 bytes. There's no need to check it anymore than that IMO.
> >
>
> Ok, I still think that would be nice but your call.
>
> But this comment was about the above ret = 0 assignment. I am thinking
> that's incorrect, because you fail to enable SHM bridge with
> -EOPNOTSUPP, then you go ahead and tell the code to claim it is
> supported with the if (!ret) conditional. This results in SHM bridge
> trying to be used when its really not supported right?
>
> Looks to me that you're really trying to return 0, not ret = 0.
>

Gah! You're right, I should have waited with v4. Oh well, I'll respin
it next week.

Bart

> > Bart
> >
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool)
> > > > > +{
> > > > > + u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms, *handle;
> > > > > + int ret;
> > > > > +
> > > > > + if (!qcom_tzmem_using_shm_bridge)
> > > > > + return 0;
> > > > > +
> > > > > + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> > > > > + pfn_and_ns_perm = (u64)pool->pbase | ns_perms;
> > > > > + ipfn_and_s_perm = (u64)pool->pbase | ns_perms;
> > > > > + size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> > > >
> > > > Is there any sanity checking that can be done here? I assume bits 0-11 are all
> > > > flag fields (or at least unrelated to size which I assume at a minimum
> > > > must be 4k aka bit 12).
> > >
> > > I guess qcom_tzmem_pool_new's PAGE_ALIGN would make sure this is
> > > probably ok for all future users, but I do think some sanity would be
> > > nice to indicate the size's allowed for SHM bridge.
> > >
> > > >
> > > > > +
> > > > > + handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> > > >
> > > > Consider __free(kfree) + return_ptr() usage?
> > > >
> > > > > + if (!handle)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + ret = qcom_scm_shm_bridge_create(qcom_tzmem_dev, pfn_and_ns_perm,
> > > > > + ipfn_and_s_perm, size_and_flags,
> > > > > + QCOM_SCM_VMID_HLOS, handle);
> > > > > + if (ret) {
> > > > > + kfree(handle);
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + pool->priv = handle;
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool)
> > > > > +{
> > > > > + u64 *handle = pool->priv;
> > > > > +
> > > > > + if (!qcom_tzmem_using_shm_bridge)
> > > > > + return;
> > > > > +
> > > > > + qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle);
> > > > > + kfree(handle);
> > > > > +}
> > > > > +
> > > > > +#endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */
> > > > >
> > > > > /**
> > > > > * qcom_tzmem_pool_new() - Create a new TZ memory pool.
> > > > > --
> > > > > 2.39.2
> > > > >
> > >
> >
>